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

[Ruslan Bilovol]

Hi Alan,

On Tue, Feb 17, 2015 at 11:51 PM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> 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.

Will fix it

>
>>  /* ------------------------------------------------------------------------- */
>>
>>  #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.

It seems this name is confusing (it is intended to have "pending
gadget" meaning). Will change it in next patch version

>
>>       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?

Yes, it's safe here, in the only place where it was used before
this routine is also called while holding the udc_lock mutex
(see usb_gadget_probe_driver)

>
>> +                     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?

Should not be any issue here but it's possible to prevent this, will do it
in next patch version

>
>> +     }
>> +     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.

Yes, that makes sense, will change it

>
>> +             } 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.

I disagree with this. This function is externally visible and we can't
guarantee that some buggy code will not call it with uninitialized
'pending' list_head. For example, if it never called usb_gadget_probe_driver()
but calls usb_gadget_unregister_driver().
As per my opinion it's better to check it and return -ENODEV rather than
fail on deleting of uninitialized list_head. In this case adding the list_init
and list_del_init above is not needed.

Best regards,
Ruslan
--
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/