On Mon, 04 Oct 2010 13:35:09 +0200, Felipe Balbi <balbi@xxxxxx> wrote:
diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
+int usb_add_udc(struct device *parent, struct usb_udc *udc)
+{
+ struct device *dev;
+ unsigned long flags;
+ int ret;
+
+ if (!udc->gadget || !udc->gadget->ops ||
+ !udc->gadget->ops->start) {
+ dev_dbg(parent, "unitializaed gadget\n");
+ return -EINVAL;
+ }
+
+ dev_vdbg(parent, "registering UDC\n");
+
+ dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+ if (!dev) {
+ ret = -ENOMEM;
+ goto err1;
+ }
+
+ spin_lock_irqsave(&udc_lock, flags);
Spin lock is not needed for initializing. Just when adding UDC to
the list. Just move it before list_add_tail();
+
+ device_initialize(dev);
+
+ dev->class = udc_class;
+ dev->type = &udc_device_type;
+ dev->parent = parent;
+ dev->release = usb_udc_release;
+ dev_set_drvdata(dev, udc);
+ udc->dev = dev;
+ udc->gadget->dev.parent = udc->dev;
+ udc->gadget->dev.release = usb_udc_nop_release;
+ udc->gadget->dev.dma_mask = parent->dma_mask;
+
+ ret = dev_set_name(dev, "%s", udc->name);
+ if (ret)
+ goto err2;
+
+ ret = dev_set_name(&udc->gadget->dev, "%s", udc->name);
+ if (ret)
+ goto err2;
+
+ ret = device_add(dev);
+ if (ret)
+ goto err2;
This should be:
if (ret) {
put_device(dev);
goto err2;
}
+
+ ret = device_register(&udc->gadget->dev);
+ if (ret)
+ goto err3;
Shouldn't it be:
if (ret) {
put_device(&udc->gadget->dev);
goto err3;
}
+
+
+ list_add_tail(&udc->list, &udc_list);
+ spin_unlock_irqrestore(&udc_lock, flags);
+
+ return 0;
+
+err3:
+ device_unregister(dev);
+
+err2:
+ kfree(dev);
This is a double free. usb_udc_release() calls kfree() already.
Just drop this line.
+ spin_unlock_irqrestore(&udc_lock, flags);
+
+err1:
+ return ret;
+}
+EXPORT_SYMBOL_GPL(usb_add_udc);
+void usb_del_udc(struct usb_udc *udc)
+{
+ unsigned long flags;
+
+ dev_vdbg(udc->dev->parent, "unregistering UDC\n");
+
+ spin_lock_irqsave(&udc_lock, flags);
+ list_del(&udc->list);
+
+ if (udc->driver) {
+ spin_unlock_irqrestore(&udc_lock, flags);
+ usb_gadget_unregister_driver(udc->driver);
+ spin_lock_irqsave(&udc_lock, flags);
You need additional function:
static void __usb_gadget_unregister_driver(struct usb_udc *udc);
which will be called under the spinlock and will unregister driver from given
UDC. The above won't work since UDC is no longer on the list and
usb_gadget_unregister_driver() looks for UDC on a list.
+ }
+
+ device_unregister(udc->dev);
+ device_unregister(&udc->gadget->dev);
+ spin_unlock_irqrestore(&udc_lock, flags);
Unregistering can be done outside of spin lock.
+}
+EXPORT_SYMBOL_GPL(usb_del_udc);
+int usb_gadget_register_driver(struct usb_gadget_driver *driver)
+{
+ struct usb_udc *udc = NULL;
+ unsigned long flags;
+ int ret;
+
+ if (!driver || !driver->bind || !driver->setup)
+ return -EINVAL;
+
+ spin_lock_irqsave(&udc_lock, flags);
+ list_for_each_entry(udc, &udc_list, list) {
+ if (!udc->driver) {
+ pr_debug("using UDC '%s'\n", udc->name);
+ break;
+ }
+
+ if (udc->driver)
+ continue;
+
+ pr_debug("couldn't find an available UDC\n");
+ ret = -ENODEV;
+ spin_unlock_irqrestore(&udc_lock, flags);
+ goto err0;
+ }
+
+ dev_dbg(udc->dev, "registering UDC driver [%s]\n",
+ driver->function);
+
+ udc->driver = driver;
+ udc->dev->driver = &driver->driver;
+
+ spin_unlock_irqrestore(&udc_lock, flags);
This has still the same problem as I described before. What if usb_del_udc()
gets called now?
+
+ ret = usb_gadget_start(udc->gadget);
+ if (ret) {
+ dev_err(udc->dev, "failed to start %s --> %d\n",
+ udc->name, ret);
+ goto err1;
+ }
+
+ ret = driver->bind(udc->gadget);
+ if (ret) {
+ dev_err(udc->dev, "bind to driver %s failed --> %d\n",
+ driver->function, ret);
+ goto err2;
+ }
+
+ ret = usb_gadget_connect(udc->gadget);
+ if (ret) {
+ dev_err(udc->dev, "failed to start %s --> %d\n",
+ udc->name, ret);
+ goto err3;
+ }
+
+ kobject_uevent(&udc->dev->kobj, KOBJ_ADD);
+
+ return 0;
+
+err3:
+ driver->unbind(udc->gadget);
+
+err2:
+ usb_gadget_stop(udc->gadget);
+
+err1:
+ spin_lock_irqsave(&udc_lock, flags);
+ udc->driver = NULL;
+ udc->dev->driver = NULL;
+ spin_unlock_irqrestore(&udc_lock, flags);
+
+err0:
+ return ret;
+}
+EXPORT_SYMBOL_GPL(usb_gadget_register_driver);
+
+int usb_gadget_unregister_driver(struct usb_gadget_driver *driver)
+{
+ struct usb_udc *udc = NULL;
+ unsigned long flags;
+ int ret = 0;
+
+ if (!driver || !driver->unbind)
+ return -EINVAL;
+
+ spin_lock_irqsave(&udc_lock, flags);
+ list_for_each_entry(udc, &udc_list, list) {
+ if (udc->driver == driver)
+ break;
+
+ if (udc->driver != driver)
+ continue;
+
+ ret = -ENODEV;
+ spin_unlock_irqrestore(&udc_lock, flags);
+ goto out;
The above must be outside of the loop.
+ }
+
+ spin_unlock_irqrestore(&udc_lock, flags);
Again, if usb_udc_del() gets called after this spin lock but before driver
gets properly unregistered, daemons will come out of user's nose.
+
+ (void) usb_gadget_vbus_draw(udc->gadget, 0);
+
+ dev_dbg(udc->dev, "unregistering UDC driver [%s]\n", driver->function);
+ kobject_uevent(&udc->dev->kobj, KOBJ_REMOVE);
+
+ driver->unbind(udc->gadget);
+ usb_gadget_stop(udc->gadget);
+ usb_gadget_disconnect(udc->gadget);
+
+ spin_lock_irqsave(&udc_lock, flags);
+ udc->driver = NULL;
+ udc->dev->driver = NULL;
+ spin_unlock_irqrestore(&udc_lock, flags);
+
+out:
+ return ret;
+}
+EXPORT_SYMBOL_GPL(usb_gadget_unregister_driver);
--
Best regards, _ _
| Humble Liege of Serenely Enlightened Majesty of o' \,=./ `o
| Computer Science, MichaÅ "mina86" Nazarewicz (o o)
+----[mina86*mina86.com]---[mina86*jabber.org]----ooO--(_)--Ooo--
--
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
