linux-kernel - Re: [PATCH v2 1/2] usb: gadget: udc-core: independent registration of gadgets and gadget drivers

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAB=otbTfJKQ8rhfvGDCteLi8sQd-nmrYeYVt-6w1awWPX8Xexw@mail.gmail.com>
Date:	Tue, 17 Feb 2015 23:00:29 +0200
From:	Ruslan Bilovol <ruslan.bilovol@...il.com>
To:	Alan Stern <stern@...land.harvard.edu>
Cc:	"Balbi, Felipe" <balbi@...com>,
	Krzysztof Opasiak <k.opasiak@...sung.com>,
	Peter Chen <peter.chen@...escale.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
	Andrzej Pietrasiewicz <andrzej.p@...sung.com>
Subject: Re: [PATCH v2 1/2] usb: gadget: udc-core: independent registration of
 gadgets and gadget drivers

On Mon, Feb 16, 2015 at 1:23 AM, Alan Stern <stern@...land.harvard.edu> wrote:
> On Mon, 16 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@...il.com>
>> ---
>>  drivers/usb/gadget/udc/udc-core.c | 77 +++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 74 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/udc/udc-core.c b/drivers/usb/gadget/udc/udc-core.c
>> index 5a81cb0..71b6942 100644
>> --- a/drivers/usb/gadget/udc/udc-core.c
>> +++ b/drivers/usb/gadget/udc/udc-core.c
>> @@ -46,10 +46,18 @@ struct usb_udc {
>>       struct list_head                list;
>>  };
>>
>> +struct pending_gadget_driver {
>> +     struct usb_gadget_driver        *driver;
>> +     char                            *udc_name;
>> +     struct list_head                list;
>> +};
>
> Don't make this a separate structure.  It should be embedded in the
> usb_gadget_driver structure.  This will make a lot of the changes below
> much simpler.

Yes, but require more changes across gadget sources. Will do it in next version

>
>> @@ -288,6 +297,24 @@ int usb_add_gadget_udc_release(struct device *parent, struct usb_gadget *gadget,
>>
>>       usb_gadget_set_state(gadget, USB_STATE_NOTATTACHED);
>>
>> +     if (!list_empty(&gadget_driver_pending_list)) {
>> +             pending = list_first_entry(&gadget_driver_pending_list,
>> +                                     struct pending_gadget_driver, list);
>> +
>> +             if (pending->udc_name) {
>> +                     if (!strcmp(pending->udc_name, dev_name(&udc->dev))) {
>> +                             udc_bind_to_driver(udc, pending->driver);
>> +                             list_del(&pending->list);
>> +                             kfree(pending->udc_name);
>> +                             kfree(pending);
>> +                     }
>> +             } else {
>> +                     udc_bind_to_driver(udc, pending->driver);
>> +                     list_del(&pending->list);
>> +                     kfree(pending);
>> +             }
>
> This code can be simplified too:
>
>                 if (!pending->udc_name || strcmp(pending->udc_name,
>                                 dev_name(&udc->dev) == 0) {
>                         ...

Good catch!

>> +     }
>> +
>>       mutex_unlock(&udc_lock);
>>
>>       return 0;
>
> Don't you want to add a driver back to the pending list
> usb_del_gadget_udc() after the call to usb_gadget_remove_driver?

Will be added in next version, should be few lines change

>
>> @@ -423,7 +450,27 @@ int usb_udc_attach_driver(const char *name, struct usb_gadget_driver *driver)
>>                       break;
>>       }
>>       if (ret) {
>> -             ret = -ENODEV;
>> +             struct pending_gadget_driver *pending;
>> +
>> +             pending = kzalloc(sizeof(*pending), GFP_KERNEL);
>> +             if (!pending) {
>> +                     ret = -ENOMEM;
>> +                     goto out;
>> +             }
>> +             pending->driver = driver;
>> +             pending->udc_name = kstrdup(name, GFP_KERNEL);
>> +             if (!pending->udc_name) {
>> +                     kfree(pending);
>> +                     ret = -ENOMEM;
>> +                     goto out;
>> +             }
>
> Why do you need to copy the name?  Just use the original.

Because it's only place we keep it. Will not be needed in next version of patch

Best regards,
Ruslan

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