This commit fixes a possible race condition in IDC class when
usb_del_udc() is called concurrently to
usb_gadget_register_driver() or usb_gadget_unregister_driver().
* NOT FOR MERGING, NOT TESTED, NOT EVEN COMPILED *
Signed-off-by: Michal Nazarewicz <m.nazarewicz@xxxxxxxxxxx>
---
drivers/usb/gadget/udc-core.c | 96 +++++++++++++++++++++++++---------------
include/linux/usb/udc.h | 3 -
2 files changed, 60 insertions(+), 39 deletions(-)
On Wed, 22 Sep 2010 11:24:59 +0200, Michał Nazarewicz wrote:
> Also, the more I think about it, the more I'm convinced that this needs
> some synchronisation. If usb_del_udc() is called at the same time as
> usb_gadget_unregister_driver() funky stuff can happen. Funky stuff will
> also happen if usb_del_udc() is called at the same time as
> usb_gadget_register_driver().
>
> I think, spin lock should be replaced with a mutex and the all of the
> operations need to be performed when holding the mutex. Alternatively,
> mutex can be used in addition to spin lock and then usb_add_udc() could
> work without locking the mutex. Or, per-UDC mutex needs to be created
> and registering, unregistering and deleting would be performed while
> holding this mutex.
After more thought I came up with a scheme where usage of atomic
operations (xchg and cmpxchg) is enough to guarantee correctness.
The idea is that the driver field can have additional value:
&udc_claimed. This value means that usb_del_udc(),
usb_gadget_register_driver() or usb_gadget_unregister_driver() does
something with the UDC. So basically, UDC can be in either of three
states: free, busy or claimed where claimed means the above.
usb_gadget_register_driver() treats claimed UDCs as busy,
usb_gadget_unregister_driver() as ones of no interest, and
usb_del_udc() waits till the UDC is unclaimed.
To make it possible for usb_del_udc() to wait till
usb_*register_gadget_driver() finishes work on UDC a wait queue is
added.
The busy flag has been removed.
diff --git a/drivers/usb/gadget/udc-core.c b/drivers/usb/gadget/udc-core.c
index 9334a91..6e40005 100644
--- a/drivers/usb/gadget/udc-core.c
+++ b/drivers/usb/gadget/udc-core.c
@@ -31,6 +31,8 @@ static struct class *udc_class;
static struct device_type udc_device_type;
static LIST_HEAD(udc_list);
static spinlock_t udc_lock;
+static DECLARE_WAIT_QUEUE_HEAD(udc_wq);
+static struct usb_gadget_driver udc_claimed;
/* ------------------------------------------------------------------------- */
@@ -98,8 +100,7 @@ int usb_add_udc(struct device *parent, struct usb_udc *udc)
unsigned long flags;
int ret;
- if (!udc->gadget || !udc->gadget->ops ||
- !udc->gadget->ops->start) {
+ if (!udc->gadget || !udc->gadget->ops || !udc->gadget->ops->start) {
dev_dbg(parent, "unitializaed gadget\n");
return -EINVAL;
}
@@ -158,6 +159,34 @@ err1:
}
EXPORT_SYMBOL_GPL(usb_add_udc);
+
+static struct usb_gadget_driver *udc_claim_driver_wait(struct usb_udc *udc)
+{
+ struct usb_gadget_driver *driver;
+ wait_event(udc_wq, (driver = xchg(&udc->driver, &udc_claimed))
+ != &udc_claimed);
+ return driver;
+}
+
+static int udc_claim_driver_if(struct usb_udc *udc,
+ struct usb_gadget_driver *old)
+{
+ return cmpxchg(&udc->driver, old, &udc_claimed) == old
+ ? 0 : -EBUSY;
+}
+
+static void udc_unclaim_driver(struct usb_udc *udc,
+ struct usb_gadget_driver *driver)
+{
+ smp_wmb();
+ udc->driver = driver;
+ wake_up_all(&udc_wq);
+}
+
+static void __usb_gadget_unregister_driver(struct usb_udc *udc,
+ struct usb_gadget_driver *driver);
+
+
/**
* usb_del_udc - deletes @udc from udc_list
* @udc: the udc to be removed.
@@ -168,6 +197,7 @@ EXPORT_SYMBOL_GPL(usb_add_udc);
void usb_del_udc(struct usb_udc *udc)
{
unsigned long flags;
+ struct usb_gadget_driver *driver;
dev_vdbg(udc->dev->parent, "unregistering UDC\n");
@@ -175,8 +205,9 @@ void usb_del_udc(struct usb_udc *udc)
list_del(&udc->list);
spin_unlock_irqrestore(&udc_lock, flags);
- if (udc->busy)
- usb_gadget_unregister_driver(udc->driver);
+ driver = udc_claim_driver_wait(udc);
+ if (driver)
+ __usb_gatget_unregister_driver(udc, driver);
device_unregister(udc->dev);
device_unregister(&udc->gadget->dev);
@@ -190,33 +221,30 @@ int usb_gadget_register_driver(struct usb_gadget_driver *driver)
{
struct usb_udc *udc = NULL;
unsigned long flags;
- int ret;
+ int ret = -ENODEV;
if (!driver || !driver->bind || !driver->setup)
return -EINVAL;
spin_lock_irqsave(&udc_lock, flags);
- list_for_each_entry(udc, &udc_list, list) {
- if (!udc->busy) {
- pr_debug("using UDC '%s'\n", udc->name);
+ list_for_each_entry(udc, &udc_list, list)
+ if (!udc_claim_driver_if(udc, NULL)) {
+ ret = 0;
break;
}
+ spin_unlock_irqrestore(&udc_lock, flags);
+
+ if (ret) {
pr_debug("couldn't find an available UDC\n");
- ret = -ENODEV;
- spin_unlock_irqrestore(&udc_lock, flags);
- goto err0;
+ return ret;
}
dev_dbg(udc->dev, "registering UDC driver [%s]\n",
driver->function);
- udc->busy = true;
- udc->driver = driver;
udc->dev->driver = &driver->driver;
- spin_unlock_irqrestore(&udc_lock, flags);
-
ret = usb_gadget_start(udc->gadget);
if (ret) {
dev_err(udc->dev, "failed to start %s --> %d\n",
@@ -239,6 +267,7 @@ int usb_gadget_register_driver(struct usb_gadget_driver *driver)
}
kobject_uevent(&udc->dev->kobj, KOBJ_ADD);
+ udc_unclaim_driver(udc, driver);
return 0;
@@ -249,12 +278,9 @@ err2:
usb_gadget_stop(udc->gadget);
err1:
- udc->driver = NULL;
udc->dev->driver = NULL;
- wmb();
- udc->busy = false;
+ udc_unclaim_driver(udc, NULL);
-err0:
return ret;
}
EXPORT_SYMBOL_GPL(usb_gadget_register_driver);
@@ -263,26 +289,31 @@ int usb_gadget_unregister_driver(struct usb_gadget_driver *driver)
{
struct usb_udc *udc = NULL;
unsigned long flags;
- int ret = 0;
+ int ret = -ENODEV;
if (!driver || !driver->unbind)
return -EINVAL;
spin_lock_irqsave(&udc_lock, flags);
- list_for_each_entry(udc, &udc_list, list) {
- if (udc->driver == driver)
+ list_for_each_entry(udc, &udc_list, list)
+ if (!udc_claim_driver_if(udc, driver)) {
+ ret = 0;
break;
+ }
+ spin_unlock_irqrestore(&udc_lock, flags);
- if (udc->driver != driver || !udc->busy)
- continue;
-
- ret = -ENODEV;
- spin_unlock_irqrestore(&udc_lock, flags);
- goto out;
+ if (!ret) {
+ __usb_gatget_unregister_driver(udc, driver);
+ udc_unclaim_driver(udc, NULL);
}
- spin_unlock_irqrestore(&udc_lock, flags);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(usb_gadget_unregister_driver);
+static void __usb_gadget_unregister_driver(struct usb_udc *udc,
+ struct usb_gadget_driver *driver)
+{
(void) usb_gadget_vbus_draw(udc->gadget, 0);
dev_dbg(udc->dev, "unregistering UDC driver [%s]\n", driver->function);
@@ -292,15 +323,8 @@ int usb_gadget_unregister_driver(struct usb_gadget_driver *driver)
usb_gadget_stop(udc->gadget);
usb_gadget_disconnect(udc->gadget);
- udc->driver = NULL;
udc->dev->driver = NULL;
- wmb();
- udc->busy = false;
-
-out:
- return ret;
}
-EXPORT_SYMBOL_GPL(usb_gadget_unregister_driver);
/* ------------------------------------------------------------------------- */
diff --git a/include/linux/usb/udc.h b/include/linux/usb/udc.h
index 6332df0..ce44468 100644
--- a/include/linux/usb/udc.h
+++ b/include/linux/usb/udc.h
@@ -32,7 +32,6 @@
* @driver - the gadget driver pointer. For use by the class code
* @dev - the child device to the actual controller
* @list - for use by the udc class driver
- * @busy - true when this udc already has @driver and @gadget
* @name - a human friendly name representation
*
* a pointer to this structure should be passed to udc class
@@ -46,8 +45,6 @@ struct usb_udc {
struct list_head list;
- unsigned busy:1;
-
const char *name;
};
--
1.7.1
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
