On Wed, 15 Sep 2010 11:43:56 +0200, Felipe Balbi <balbi@xxxxxx> wrote:
+int usb_add_udc(struct device *parent, struct usb_udc *udc)
+{
+ struct device *dev;
+ unsigned long flags;
+ int ret;
+
+ dev_vdbg(parent, "registering UDC\n");
+
+ dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+ if (!dev) {
+ ret = -ENOMEM;
+ goto err1;
+ }
+
+ 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, udc->name);
In theory udc->name can have "%". If we assume it has no "%" than
dev_set_name() above can be changed as well.
+ if (ret)
+ goto err2;
+
+ ret = device_add(dev);
+ if (ret)
+ goto err2;
+
+ ret = device_register(&udc->gadget->dev);
+ if (ret)
+ goto err3;
+
+
+ spin_lock_irqsave(&udc_lock, flags);
+ list_add_tail(&udc->list, &udc_list);
+ spin_unlock_irqrestore(&udc_lock, flags);
+
+ return 0;
+
+err3:
+ device_unregister(dev);
+
+err2:
+ kfree(dev);
+
+err1:
+ return ret;
+}
+void usb_del_udc(struct usb_udc *udc)
+{
+ unsigned long flags;
+
+ dev_vdbg(udc->dev->parent, "unregistering UDC\n");
+
+ if (udc->busy)
I'd say "if (WARN_ON(udc->busy))".
+ usb_gadget_unregister_driver(udc->driver);
Maybe add a __usb_gadget_unregister_driver() function which would
take udc as argument and unregister the driver?
Also, note that a concurrent call to usb_gadget_register_driver()
could grab this UDC and register a driver to it. The list_del() should
be moved above the condition.
+ device_unregister(udc->dev);
+ device_unregister(&udc->gadget->dev);
+
+ spin_lock_irqsave(&udc_lock, flags);
+ list_del(&udc->list);
+ spin_unlock_irqrestore(&udc_lock, flags);
+}
So, after proposed changes, the function would look as follows:
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);
spin_unlock_irqrestore(&udc_lock, flags);
if (WARN_ON(udc->busy))
__usb_gadget_unregister_driver(udc);
device_unregister(udc->dev);
device_unregister(&udc->gadget->dev);
}
The __usb_gadget_unregister_driver() would look as follows:
static void __usb_gadget_unregister_driver(struct usb_udc *udc)
{
(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);
udc->driver = NULL;
udc->dev->driver = NULL;
smb_wb();
udc->busy = false;
}
Also note, that you should first unbind, stop and disconnect and only then mark UDC
as not busy. Otherwise, concurrent usb_gadget_register_driver() could take the UDC
while driver is being unregistered.
Alternatively, you could create two lists: of free and busy UDCs and move UDCs from
one list to the other.
+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->busy) {
+ pr_debug("using UDC '%s'\n", udc->name);
This pr_debug() seems redundant. Information is printed after the loop
anyway.
+ break;
+ }
+
+ pr_debug("couldn't find an available UDC\n");
+ ret = -ENODEV;
+ spin_unlock_irqrestore(&udc_lock, flags);
+ goto err0;
Matter of taste but I would prefer a simple "return -ENODEV;" rather
than a goto.
+ }
+
+ 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",
+ 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:
+ udc->busy = false;
+ udc->driver = NULL;
+ udc->dev->driver = NULL;
The above needs some kind of synchronisation, for instance:
udc->driver = NULL;
udc->dev->driver = NULL;
smp_wb();
udc->busy = false;
+
+err0:
+ return ret;
+}
+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)
+ continue;
+
+ if (udc->busy)
+ break;
+
+ ret = -ENODEV;
+ spin_unlock_irqrestore(&udc_lock, flags);
+ goto out;
+ }
You can get an invalid UDC here if there was no UDC that matched driver.
+
+ udc->busy = false;
+ udc->driver = NULL;
+ udc->dev->driver = NULL;
The same comment as in my __usb_gadget_unregister_driver() above applies.
Concurrent call to usb_gadget_register_driver() could now grab the UDC that
is not yet fully free to use.
+ spin_unlock_irqrestore(&udc_lock, flags);
+
+ (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);
+
+out:
+ return ret;
+}
With __usb_gadget_unregister_driver(), this would look something like:
int usb_gadget_unregister_driver(struct usb_gadget_driver *driver)
{
struct usb_udc *udc = NULL;
unsigned long flags;
bool ret = -ENODEV;
if (!driver || !driver->unbind)
return -EINVAL;
spin_lock_irqsave(&udc_lock, flags);
list_for_each_entry(udc, &udc_list, list)
if (udc->busy && udc->driver == driver) {
ret = 0;
break;
}
spin_unlock_irqrestore(&udc_lock, flags);
if (!ret)
__usb_gadget_unregister_driver(udc);
return ret;
}
At this point, I only wonder if calling usb_gadget_unregister_driver() and usb_del_udc()
won't break something. But that's a pathological situation anyway so maybe we don't have
to care?
diff --git a/include/linux/usb/udc.h b/include/linux/usb/udc.h
@@ -0,0 +1,54 @@
+/**
+ * struct usb_udc - describes one usb device controller
+ * @gadget - the gadget. Always provide this.
+ * @driver - the gadget driver pointer. Always provide this.
Actually, always set to NULL. This field is modified only by the
class code, right? UDC driver should not touch it. Or am I missing
something.
+ * @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
+ * driver by controller driver (musb, omap_udc, atmel, etc).
+ * @gadget is expected to be already initialized by that time.
+ */
+struct usb_udc {
+ struct usb_gadget *gadget;
+ struct usb_gadget_driver *driver;
+ struct device *dev;
+
+ struct list_head list;
+
+ unsigned busy:1;
I started wondering if this field is really needed. After all,
this field is set only if driver is not-NULL.
+
+ const char *name;
Is that required? Wouldn't dev's or gadget's dev's name be enough?
+};
+
+extern int usb_add_udc(struct device *, struct usb_udc *);
+extern void usb_del_udc(struct usb_udc *);
+
+
Empty lines at the end of file.
--
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
