| 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
| ||
|
Message-ID: <Pine.LNX.4.44L0.1502171640360.1362-100000@iolanthe.rowland.org>
Date: Tue, 17 Feb 2015 16:51:26 -0500 (EST)
From: Alan Stern <stern@...land.harvard.edu>
To: Ruslan Bilovol <ruslan.bilovol@...il.com>
cc: balbi@...com, <linux-usb@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, <k.opasiak@...sung.com>,
<peter.chen@...escale.com>, <gregkh@...uxfoundation.org>,
<andrzej.p@...sung.com>
Subject: Re: [PATCHv3 5/5] usb: gadget: udc-core: independent registration
of gadgets and gadget drivers
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@...il.com>
> ---
> 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@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists