Re: [PATCHv3 5/5] usb: gadget: udc-core: independent registration of gadgets and gadget drivers

[Alan Stern]

On Tue, 17 Feb 2015, Ruslan Bilovol wrote:

> Change behavior during registration of gadgets and
> gadget drivers in udc-core. Instead of previous
> approach when for successful probe of usb gadget driver
> at least one usb gadget should be already registered
> use another one where gadget drivers and gadgets
> can be registered in udc-core independently.
> 
> Independent registration of gadgets and gadget drivers
> is useful for built-in into kernel gadget and gadget
> driver case - because it's possible that gadget is
> really probed only on late_init stage (due to deferred
> probe) whereas gadget driver's probe is silently failed
> on module_init stage due to no any UDC added.
> 
> Also it is useful for modules case - now there is no
> difference what module to insert first: gadget module
> or gadget driver one.
> 
> Signed-off-by: Ruslan Bilovol <ruslan.bilovol@xxxxxxxxx>
> ---
>  drivers/usb/gadget/udc/udc-core.c | 51 ++++++++++++++++++++++++++++++---------
>  include/linux/usb/gadget.h        |  2 ++
>  2 files changed, 42 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/usb/gadget/udc/udc-core.c b/drivers/usb/gadget/udc/udc-core.c
> index a960f3f..9e82497 100644
> --- a/drivers/usb/gadget/udc/udc-core.c
> +++ b/drivers/usb/gadget/udc/udc-core.c
> @@ -48,8 +48,11 @@ struct usb_udc {
>  
>  static struct class *udc_class;
>  static LIST_HEAD(udc_list);
> +static LIST_HEAD(gadget_driver_pending_list);
>  static DEFINE_MUTEX(udc_lock);
>  
> +static int udc_bind_to_driver(struct usb_udc *udc,
> +					struct usb_gadget_driver *driver);

Strange indentation, not at all like the other continuation lines in 
this source file.

Also, there should be a blank line here, as in the original file.

>  /* ------------------------------------------------------------------------- */
>  
>  #ifdef	CONFIG_HAS_DMA
> @@ -243,6 +246,7 @@ int usb_add_gadget_udc_release(struct device *parent, struct usb_gadget *gadget,
>  		void (*release)(struct device *dev))
>  {
>  	struct usb_udc		*udc;
> +	struct usb_gadget_driver *pgadget;

Don't call it "pgadget".  None of the other pointer variables in this 
file have an extra "p" added to the beginnings of their names.

>  	int			ret = -ENOMEM;
>  
>  	udc = kzalloc(sizeof(*udc), GFP_KERNEL);
> @@ -288,6 +292,18 @@ int usb_add_gadget_udc_release(struct device *parent, struct usb_gadget *gadget,
>  
>  	usb_gadget_set_state(gadget, USB_STATE_NOTATTACHED);
>  
> +	/* pick up one of pending gadget drivers */
> +	list_for_each_entry(pgadget, &gadget_driver_pending_list, pending) {
> +		if (!pgadget->udc_name || strcmp(pgadget->udc_name,
> +						dev_name(&udc->dev)) == 0) {
> +			ret = udc_bind_to_driver(udc, pgadget);

Are you sure it's safe to call this routine while holding the udc_lock 
mutex?

> +			if (ret)
> +				goto err4;
> +			list_del(&pgadget->pending);

Use list_del_init().

> +			break;
> +		}
> +	}
> +
>  	mutex_unlock(&udc_lock);
>  
>  	return 0;
> @@ -364,10 +380,11 @@ found:
>  	dev_vdbg(gadget->dev.parent, "unregistering gadget\n");
>  
>  	list_del(&udc->list);
> -	mutex_unlock(&udc_lock);
> -
> -	if (udc->driver)
> +	if (udc->driver) {
> +		list_add(&udc->driver->pending, &gadget_driver_pending_list);
>  		usb_gadget_remove_driver(udc);

Are you sure it's safe to call this routine while holding the udc_lock
mutex?

> +	}
> +	mutex_unlock(&udc_lock);
>  
>  	kobject_uevent(&udc->dev.kobj, KOBJ_REMOVE);
>  	flush_work(&gadget->work);
> @@ -426,24 +443,26 @@ int usb_gadget_probe_driver(struct usb_gadget_driver *driver)
>  			if (!ret)
>  				break;
>  		}
> -		if (ret)
> -			ret = -ENODEV;
> -		else if (udc->driver)
> -			ret = -EBUSY;
> -		else
> +		if (!ret && udc->driver) {
> +			mutex_unlock(&udc_lock);
> +			return -EBUSY;

This is a judgement call.  It might be better to return 0 and add the 
gadget driver to the pending list.

> +		} else if (!ret) {
>  			goto found;
> +		}
>  	} else {
>  		list_for_each_entry(udc, &udc_list, list) {
>  			/* For now we take the first one */
>  			if (!udc->driver)
>  				goto found;
>  		}
> -		ret = -ENODEV;
>  	}
>  
> -	pr_debug("couldn't find an available UDC\n");
> +	list_add_tail(&driver->pending, &gadget_driver_pending_list);
> +	pr_info("udc-core: couldn't find an available UDC "
> +			"- added [%s] to list of pending drivers\n",
> +			driver->function);
>  	mutex_unlock(&udc_lock);
> -	return ret;
> +	return 0;
>  found:

Call list_init(&driver->pending) here.  Or at the start of this 
routine, if you prefer.

>  	ret = udc_bind_to_driver(udc, driver);
>  	mutex_unlock(&udc_lock);
> @@ -469,6 +488,16 @@ int usb_gadget_unregister_driver(struct usb_gadget_driver *driver)
>  			break;
>  		}
>  
> +	if (ret) {
> +		struct usb_gadget_driver *tmp;
> +
> +		list_for_each_entry(tmp, &gadget_driver_pending_list, pending)
> +			if (tmp == driver) {
> +				list_del(&driver->pending);
> +				ret = 0;
> +				break;
> +			}
> +	}

If you add the list_init and list_del_init above, this loop won't be 
needed.  You can just call list_del.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/