[RFC PATCH 0/4] USB: gadget: Create a bus for gadgets

[RFC PATCH 0/4] USB: gadget: Create a bus for gadgets

[Alan Stern]

Everyone:

The following series of patches implements Greg's suggestion that 
gadgets should be registered on some sort of bus.  It turns out that the 
best way to do this is to create a new "gadget" bus, with specialized 
matching and probing routines, rather than using an existing bus.

Patches 1-3 are simple preparations for the big change.  They stand on 
their own, make useful little changes, and could be merged by themselves
without committing to adding the "gadget" bus.  Patch 4 is main one.

I'm posting this series for feedback from the Gadget/UDC maintainer and 
others.  If everything works out okay, the patches can be submitted for 
real once the upcoming merge window closes.

Alan Stern

[RFC PATCH 1/4] USB: gadget: Rename usb_gadget_probe_driver()

[Alan Stern]

In preparation for adding a "gadget" bus, this patch renames
usb_gadget_probe_driver() to usb_gadget_register_driver().  The new
name will be more accurate, since gadget drivers will be registered on
the gadget bus and the probing will be done by the driver core, not
the UDC core.

Signed-off-by: Alan Stern <stern@rowland.harvard.edu>


 drivers/usb/gadget/composite.c         |    2 +-
 drivers/usb/gadget/legacy/dbgp.c       |    2 +-
 drivers/usb/gadget/legacy/inode.c      |    2 +-
 drivers/usb/gadget/legacy/raw_gadget.c |    4 ++--
 drivers/usb/gadget/udc/core.c          |    4 ++--
 include/linux/usb/gadget.h             |    4 ++--
 6 files changed, 9 insertions(+), 9 deletions(-)

Index: usb-devel/drivers/usb/gadget/composite.c
===================================================================
--- usb-devel.orig/drivers/usb/gadget/composite.c
+++ usb-devel/drivers/usb/gadget/composite.c
@@ -2500,7 +2500,7 @@ int usb_composite_probe(struct usb_compo
 	gadget_driver->driver.name = driver->name;
 	gadget_driver->max_speed = driver->max_speed;
 
-	return usb_gadget_probe_driver(gadget_driver);
+	return usb_gadget_register_driver(gadget_driver);
 }
 EXPORT_SYMBOL_GPL(usb_composite_probe);
 
Index: usb-devel/drivers/usb/gadget/legacy/dbgp.c
===================================================================
--- usb-devel.orig/drivers/usb/gadget/legacy/dbgp.c
+++ usb-devel/drivers/usb/gadget/legacy/dbgp.c
@@ -422,7 +422,7 @@ static struct usb_gadget_driver dbgp_dri
 
 static int __init dbgp_init(void)
 {
-	return usb_gadget_probe_driver(&dbgp_driver);
+	return usb_gadget_register_driver(&dbgp_driver);
 }
 
 static void __exit dbgp_exit(void)
Index: usb-devel/drivers/usb/gadget/legacy/inode.c
===================================================================
--- usb-devel.orig/drivers/usb/gadget/legacy/inode.c
+++ usb-devel/drivers/usb/gadget/legacy/inode.c
@@ -1873,7 +1873,7 @@ dev_config (struct file *fd, const char
 	else
 		gadgetfs_driver.max_speed = USB_SPEED_FULL;
 
-	value = usb_gadget_probe_driver(&gadgetfs_driver);
+	value = usb_gadget_register_driver(&gadgetfs_driver);
 	if (value != 0) {
 		spin_lock_irq(&dev->lock);
 		goto fail;
Index: usb-devel/drivers/usb/gadget/legacy/raw_gadget.c
===================================================================
--- usb-devel.orig/drivers/usb/gadget/legacy/raw_gadget.c
+++ usb-devel/drivers/usb/gadget/legacy/raw_gadget.c
@@ -510,12 +510,12 @@ static int raw_ioctl_run(struct raw_dev
 	}
 	spin_unlock_irqrestore(&dev->lock, flags);
 
-	ret = usb_gadget_probe_driver(&dev->driver);
+	ret = usb_gadget_register_driver(&dev->driver);
 
 	spin_lock_irqsave(&dev->lock, flags);
 	if (ret) {
 		dev_err(dev->dev,
-			"fail, usb_gadget_probe_driver returned %d\n", ret);
+			"fail, usb_gadget_register_driver returned %d\n", ret);
 		dev->state = STATE_DEV_FAILED;
 		goto out_unlock;
 	}
Index: usb-devel/drivers/usb/gadget/udc/core.c
===================================================================
--- usb-devel.orig/drivers/usb/gadget/udc/core.c
+++ usb-devel/drivers/usb/gadget/udc/core.c
@@ -1523,7 +1523,7 @@ err1:
 	return ret;
 }
 
-int usb_gadget_probe_driver(struct usb_gadget_driver *driver)
+int usb_gadget_register_driver(struct usb_gadget_driver *driver)
 {
 	struct usb_udc		*udc = NULL;
 	int			ret = -ENODEV;
@@ -1568,7 +1568,7 @@ found:
 	mutex_unlock(&udc_lock);
 	return ret;
 }
-EXPORT_SYMBOL_GPL(usb_gadget_probe_driver);
+EXPORT_SYMBOL_GPL(usb_gadget_register_driver);
 
 int usb_gadget_unregister_driver(struct usb_gadget_driver *driver)
 {
Index: usb-devel/include/linux/usb/gadget.h
===================================================================
--- usb-devel.orig/include/linux/usb/gadget.h
+++ usb-devel/include/linux/usb/gadget.h
@@ -745,7 +745,7 @@ struct usb_gadget_driver {
  */
 
 /**
- * usb_gadget_probe_driver - probe a gadget driver
+ * usb_gadget_register_driver - register a gadget driver
  * @driver: the driver being registered
  * Context: can sleep
  *
@@ -755,7 +755,7 @@ struct usb_gadget_driver {
  * registration call returns.  It's expected that the @bind() function will
  * be in init sections.
  */
-int usb_gadget_probe_driver(struct usb_gadget_driver *driver);
+int usb_gadget_register_driver(struct usb_gadget_driver *driver);
 
 /**
  * usb_gadget_unregister_driver - unregister a gadget driver

[RFC PATCH 2/4] USB: gadget: Register udc before gadget

[Alan Stern]

In preparation for adding a "gadget" bus, this patch reverses the
order of registration of udc and gadget devices in usb_add_gadget().

The current code adds the gadget device first, probably because that
was more convenient at the time and the order didn't really matter.
But with the upcoming change, adding the gadget will cause driver
probing to occur.  Unwinding that on the error pathway will become
much more obtrusive, not to mention the fact that a gadget driver
might not work properly before the udc is registered.  It's better to
register the udc device first, particularly since that doesn't involve
a bus or driver binding and therefore is simpler to unwind.

For symmetry, the order of unregistration in usb_del_gadget() is
likewise reversed.

Signed-off-by: Alan Stern <stern@rowland.harvard.edu>

---

 drivers/usb/gadget/udc/core.c |   17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

Index: usb-devel/drivers/usb/gadget/udc/core.c
===================================================================
--- usb-devel.orig/drivers/usb/gadget/udc/core.c
+++ usb-devel/drivers/usb/gadget/udc/core.c
@@ -1308,10 +1308,6 @@ int usb_add_gadget(struct usb_gadget *ga
 	if (ret)
 		goto err_put_udc;
 
-	ret = device_add(&gadget->dev);
-	if (ret)
-		goto err_put_udc;
-
 	udc->gadget = gadget;
 	gadget->udc = udc;
 
@@ -1327,15 +1323,22 @@ int usb_add_gadget(struct usb_gadget *ga
 	usb_gadget_set_state(gadget, USB_STATE_NOTATTACHED);
 	udc->vbus = true;
 
+	ret = device_add(&gadget->dev);
+	if (ret)
+		goto err_del_udc;
+
 	/* pick up one of pending gadget drivers */
 	ret = check_pending_gadget_drivers(udc);
 	if (ret)
-		goto err_del_udc;
+		goto err_del_gadget;
 
 	mutex_unlock(&udc_lock);
 
 	return 0;
 
+ err_del_gadget:
+	device_del(&gadget->dev);
+
  err_del_udc:
 	flush_work(&gadget->work);
 	device_del(&udc->dev);
@@ -1344,8 +1347,6 @@ int usb_add_gadget(struct usb_gadget *ga
 	list_del(&udc->list);
 	mutex_unlock(&udc_lock);
 
-	device_del(&gadget->dev);
-
  err_put_udc:
 	put_device(&udc->dev);
 
@@ -1469,8 +1470,8 @@ void usb_del_gadget(struct usb_gadget *g
 
 	kobject_uevent(&udc->dev.kobj, KOBJ_REMOVE);
 	flush_work(&gadget->work);
-	device_unregister(&udc->dev);
 	device_del(&gadget->dev);
+	device_unregister(&udc->dev);
 }
 EXPORT_SYMBOL_GPL(usb_del_gadget);
 

[RFC PATCH 3/4] USB: gadget: Fix mistakes in UDC core kerneldoc

[Alan Stern]

This patch fixes some minor mistakes in the UDC core's kerneldoc.

Signed-off-by: Alan Stern <stern@rowland.harvard.edu>

---

 drivers/usb/gadget/udc/core.c |   14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

Index: usb-devel/drivers/usb/gadget/udc/core.c
===================================================================
--- usb-devel.orig/drivers/usb/gadget/udc/core.c
+++ usb-devel/drivers/usb/gadget/udc/core.c
@@ -1262,9 +1262,6 @@ static int check_pending_gadget_drivers(
  * device.
  * @gadget: the gadget to be initialized.
  * @release: a gadget release function.
- *
- * Returns zero on success, negative errno otherwise.
- * Calls the gadget release function in the latter case.
  */
 void usb_initialize_gadget(struct device *parent, struct usb_gadget *gadget,
 		void (*release)(struct device *dev))
@@ -1441,11 +1438,10 @@ static void usb_gadget_remove_driver(str
 }
 
 /**
- * usb_del_gadget - deletes @udc from udc_list
- * @gadget: the gadget to be removed.
+ * usb_del_gadget - deletes a gadget and unregisters its udc
+ * @gadget: the gadget to be deleted.
  *
- * This will call usb_gadget_unregister_driver() if
- * the @udc is still busy.
+ * This will unbind @gadget, if it is bound.
  * It will not do a final usb_put_gadget().
  */
 void usb_del_gadget(struct usb_gadget *gadget)
@@ -1476,8 +1472,8 @@ void usb_del_gadget(struct usb_gadget *g
 EXPORT_SYMBOL_GPL(usb_del_gadget);
 
 /**
- * usb_del_gadget_udc - deletes @udc from udc_list
- * @gadget: the gadget to be removed.
+ * usb_del_gadget_udc - unregisters a gadget
+ * @gadget: the gadget to be unregistered.
  *
  * Calls usb_del_gadget() and does a final usb_put_gadget().
  */

[RFC PATCH 4/4] USB: gadget: Add a new bus for gadgets

[Alan Stern]

This patch adds a "gadget" bus and uses it for registering gadgets and
their drivers.  From now on, bindings will be managed by the driver
core rather than through ad-hoc manipulations in the UDC core.

As part of this change, the driver_pending_list is removed.  The UDC
core won't need to keep track of unbound drivers for later binding,
because the driver core handles all of that for us.

However, we do need one new feature: a way to prevent gadget drivers
from being bound to more than one gadget at a time.  The existing code
does this automatically, but the driver core doesn't -- it's perfectly
happy to bind a single driver to all the matching devices on the bus.
The patch adds a new bitflag to the usb_gadget_driver structure for
this purpose.

A nice side effect of this change is a reduction in the total lines of
code, since now the driver core will do part of the work that the UDC
used to do.

A possible future patch could add udc devices to the gadget bus, say
as a separate device type.

Signed-off-by: Alan Stern <stern@rowland.harvard.edu>

---

 drivers/usb/gadget/udc/core.c |  248 +++++++++++++++++++-----------------------
 include/linux/usb/gadget.h    |   26 ++--
 2 files changed, 135 insertions(+), 139 deletions(-)

Index: usb-devel/drivers/usb/gadget/udc/core.c
===================================================================
--- usb-devel.orig/drivers/usb/gadget/udc/core.c
+++ usb-devel/drivers/usb/gadget/udc/core.c
@@ -23,6 +23,8 @@
 
 #include "trace.h"
 
+static struct bus_type gadget_bus_type;
+
 /**
  * struct usb_udc - describes one usb device controller
  * @driver: the gadget driver pointer. For use by the class code
@@ -47,11 +49,9 @@ 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);
+/* Protects udc_list, udc->driver, driver->is_bound, and related calls */
+static DEFINE_MUTEX(udc_lock);
 
 /* ------------------------------------------------------------------------- */
 
@@ -1238,24 +1238,6 @@ static void usb_udc_nop_release(struct d
 	dev_vdbg(dev, "%s\n", __func__);
 }
 
-/* should be called with udc_lock held */
-static int check_pending_gadget_drivers(struct usb_udc *udc)
-{
-	struct usb_gadget_driver *driver;
-	int ret = 0;
-
-	list_for_each_entry(driver, &gadget_driver_pending_list, pending)
-		if (!driver->udc_name || strcmp(driver->udc_name,
-						dev_name(&udc->dev)) == 0) {
-			ret = udc_bind_to_driver(udc, driver);
-			if (ret != -EPROBE_DEFER)
-				list_del_init(&driver->pending);
-			break;
-		}
-
-	return ret;
-}
-
 /**
  * usb_initialize_gadget - initialize a gadget and its embedded struct device
  * @parent: the parent device to this udc. Usually the controller driver's
@@ -1276,6 +1258,7 @@ void usb_initialize_gadget(struct device
 		gadget->dev.release = usb_udc_nop_release;
 
 	device_initialize(&gadget->dev);
+	gadget->dev.bus = &gadget_bus_type;
 }
 EXPORT_SYMBOL_GPL(usb_initialize_gadget);
 
@@ -1312,6 +1295,7 @@ int usb_add_gadget(struct usb_gadget *ga
 
 	mutex_lock(&udc_lock);
 	list_add_tail(&udc->list, &udc_list);
+	mutex_unlock(&udc_lock);
 
 	ret = device_add(&udc->dev);
 	if (ret)
@@ -1324,23 +1308,14 @@ int usb_add_gadget(struct usb_gadget *ga
 	if (ret)
 		goto err_del_udc;
 
-	/* pick up one of pending gadget drivers */
-	ret = check_pending_gadget_drivers(udc);
-	if (ret)
-		goto err_del_gadget;
-
-	mutex_unlock(&udc_lock);
-
 	return 0;
 
- err_del_gadget:
-	device_del(&gadget->dev);
-
  err_del_udc:
 	flush_work(&gadget->work);
 	device_del(&udc->dev);
 
  err_unlist_udc:
+	mutex_lock(&udc_lock);
 	list_del(&udc->list);
 	mutex_unlock(&udc_lock);
 
@@ -1419,24 +1394,6 @@ int usb_add_gadget_udc(struct device *pa
 }
 EXPORT_SYMBOL_GPL(usb_add_gadget_udc);
 
-static void usb_gadget_remove_driver(struct usb_udc *udc)
-{
-	dev_dbg(&udc->dev, "unregistering UDC driver [%s]\n",
-			udc->driver->function);
-
-	kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE);
-
-	usb_gadget_disconnect(udc->gadget);
-	usb_gadget_disable_async_callbacks(udc);
-	if (udc->gadget->irq)
-		synchronize_irq(udc->gadget->irq);
-	udc->driver->unbind(udc->gadget);
-	usb_gadget_udc_stop(udc);
-
-	udc->driver = NULL;
-	udc->gadget->dev.driver = NULL;
-}
-
 /**
  * usb_del_gadget - deletes a gadget and unregisters its udc
  * @gadget: the gadget to be deleted.
@@ -1455,13 +1412,6 @@ void usb_del_gadget(struct usb_gadget *g
 
 	mutex_lock(&udc_lock);
 	list_del(&udc->list);
-
-	if (udc->driver) {
-		struct usb_gadget_driver *driver = udc->driver;
-
-		usb_gadget_remove_driver(udc);
-		list_add(&driver->pending, &gadget_driver_pending_list);
-	}
 	mutex_unlock(&udc_lock);
 
 	kobject_uevent(&udc->dev.kobj, KOBJ_REMOVE);
@@ -1486,119 +1436,143 @@ EXPORT_SYMBOL_GPL(usb_del_gadget_udc);
 
 /* ------------------------------------------------------------------------- */
 
-static int udc_bind_to_driver(struct usb_udc *udc, struct usb_gadget_driver *driver)
+static int gadget_match_driver(struct device *dev, struct device_driver *drv)
 {
-	int ret;
+	struct usb_gadget *gadget = dev_to_usb_gadget(dev);
+	struct usb_udc *udc = gadget->udc;
+	struct usb_gadget_driver *driver = container_of(drv,
+			struct usb_gadget_driver, driver);
 
-	dev_dbg(&udc->dev, "registering UDC driver [%s]\n",
-			driver->function);
+	/* If the driver specifies a udc_name, it must match the UDC's name */
+	if (driver->udc_name &&
+			strcmp(driver->udc_name, dev_name(&udc->dev)) != 0)
+		return 0;
 
+	/* Otherwise any gadget driver matches any UDC */
+	return 1;
+}
+
+static int gadget_bind_driver(struct device *dev)
+{
+	struct usb_gadget *gadget = dev_to_usb_gadget(dev);
+	struct usb_udc *udc = gadget->udc;
+	struct usb_gadget_driver *driver = container_of(dev->driver,
+			struct usb_gadget_driver, driver);
+	int ret = 0;
+
+	mutex_lock(&udc_lock);
+	if (driver->is_bound) {
+		mutex_unlock(&udc_lock);
+		return -ENXIO;		/* Driver binds to only one gadget */
+	}
+	driver->is_bound = true;
 	udc->driver = driver;
-	udc->gadget->dev.driver = &driver->driver;
+	mutex_unlock(&udc_lock);
+
+	dev_dbg(&udc->dev, "binding gadget driver [%s]\n", driver->function);
 
 	usb_gadget_udc_set_speed(udc, driver->max_speed);
 
+	mutex_lock(&udc_lock);
 	ret = driver->bind(udc->gadget, driver);
 	if (ret)
-		goto err1;
+		goto err_bind;
+
 	ret = usb_gadget_udc_start(udc);
-	if (ret) {
-		driver->unbind(udc->gadget);
-		goto err1;
-	}
+	if (ret)
+		goto err_start;
 	usb_gadget_enable_async_callbacks(udc);
 	usb_udc_connect_control(udc);
+	mutex_unlock(&udc_lock);
 
 	kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE);
 	return 0;
-err1:
+
+ err_start:
+	driver->unbind(udc->gadget);
+
+ err_bind:
 	if (ret != -EISNAM)
 		dev_err(&udc->dev, "failed to start %s: %d\n",
-			udc->driver->function, ret);
+			driver->function, ret);
+
 	udc->driver = NULL;
-	udc->gadget->dev.driver = NULL;
+	driver->is_bound = false;
+	mutex_unlock(&udc_lock);
+
 	return ret;
 }
 
-int usb_gadget_register_driver(struct usb_gadget_driver *driver)
+static void gadget_unbind_driver(struct device *dev)
 {
-	struct usb_udc		*udc = NULL;
-	int			ret = -ENODEV;
+	struct usb_gadget *gadget = dev_to_usb_gadget(dev);
+	struct usb_udc *udc = gadget->udc;
+	struct usb_gadget_driver *driver = udc->driver;
+
+	dev_dbg(&udc->dev, "unbinding gadget driver [%s]\n", driver->function);
+
+	kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE);
+
+	mutex_lock(&udc_lock);
+	usb_gadget_disconnect(gadget);
+	usb_gadget_disable_async_callbacks(udc);
+	if (gadget->irq)
+		synchronize_irq(gadget->irq);
+	udc->driver->unbind(gadget);
+	usb_gadget_udc_stop(udc);
+
+	driver->is_bound = false;
+	udc->driver = NULL;
+	mutex_unlock(&udc_lock);
+}
+
+/* ------------------------------------------------------------------------- */
+
+int usb_gadget_register_driver_owner(struct usb_gadget_driver *driver,
+		struct module *owner, const char *mod_name)
+{
+	int ret;
 
 	if (!driver || !driver->bind || !driver->setup)
 		return -EINVAL;
 
+	driver->driver.bus = &gadget_bus_type;
+	driver->driver.owner = owner;
+	driver->driver.mod_name = mod_name;
+	ret = driver_register(&driver->driver);
+	if (ret) {
+		pr_warn("%s: driver registration failed: %d\n",
+				driver->function, ret);
+		return ret;
+	}
+
 	mutex_lock(&udc_lock);
-	if (driver->udc_name) {
-		list_for_each_entry(udc, &udc_list, list) {
-			ret = strcmp(driver->udc_name, dev_name(&udc->dev));
-			if (!ret)
-				break;
-		}
-		if (ret)
-			ret = -ENODEV;
-		else if (udc->driver)
+	if (!driver->is_bound) {
+		if (driver->match_existing_only) {
+			pr_warn("%s: couldn't find an available UDC or it's busy\n",
+					driver->function);
 			ret = -EBUSY;
-		else
-			goto found;
-	} else {
-		list_for_each_entry(udc, &udc_list, list) {
-			/* For now we take the first one */
-			if (!udc->driver)
-				goto found;
+		} else {
+			pr_info("%s: couldn't find an available UDC\n",
+					driver->function);
 		}
-	}
-
-	if (!driver->match_existing_only) {
-		list_add_tail(&driver->pending, &gadget_driver_pending_list);
-		pr_info("couldn't find an available UDC - added [%s] to list of pending drivers\n",
-			driver->function);
 		ret = 0;
 	}
-
 	mutex_unlock(&udc_lock);
+
 	if (ret)
-		pr_warn("couldn't find an available UDC or it's busy: %d\n", ret);
-	return ret;
-found:
-	ret = udc_bind_to_driver(udc, driver);
-	mutex_unlock(&udc_lock);
+		driver_unregister(&driver->driver);
 	return ret;
 }
-EXPORT_SYMBOL_GPL(usb_gadget_register_driver);
+EXPORT_SYMBOL_GPL(usb_gadget_register_driver_owner);
 
 int usb_gadget_unregister_driver(struct usb_gadget_driver *driver)
 {
-	struct usb_udc		*udc = NULL;
-	int			ret = -ENODEV;
-
 	if (!driver || !driver->unbind)
 		return -EINVAL;
 
-	mutex_lock(&udc_lock);
-	list_for_each_entry(udc, &udc_list, list) {
-		if (udc->driver == driver) {
-			usb_gadget_remove_driver(udc);
-			usb_gadget_set_state(udc->gadget,
-					     USB_STATE_NOTATTACHED);
-
-			/* Maybe there is someone waiting for this UDC? */
-			check_pending_gadget_drivers(udc);
-			/*
-			 * For now we ignore bind errors as probably it's
-			 * not a valid reason to fail other's gadget unbind
-			 */
-			ret = 0;
-			break;
-		}
-	}
-
-	if (ret) {
-		list_del(&driver->pending);
-		ret = 0;
-	}
-	mutex_unlock(&udc_lock);
-	return ret;
+	driver_unregister(&driver->driver);
+	return 0;
 }
 EXPORT_SYMBOL_GPL(usb_gadget_unregister_driver);
 
@@ -1750,8 +1724,17 @@ static int usb_udc_uevent(struct device
 	return 0;
 }
 
+static struct bus_type gadget_bus_type = {
+	.name = "gadget",
+	.probe = gadget_bind_driver,
+	.remove = gadget_unbind_driver,
+	.match = gadget_match_driver,
+};
+
 static int __init usb_udc_init(void)
 {
+	int rc;
+
 	udc_class = class_create(THIS_MODULE, "udc");
 	if (IS_ERR(udc_class)) {
 		pr_err("failed to create udc class --> %ld\n",
@@ -1760,12 +1743,17 @@ static int __init usb_udc_init(void)
 	}
 
 	udc_class->dev_uevent = usb_udc_uevent;
-	return 0;
+
+	rc = bus_register(&gadget_bus_type);
+	if (rc)
+		class_destroy(udc_class);
+	return rc;
 }
 subsys_initcall(usb_udc_init);
 
 static void __exit usb_udc_exit(void)
 {
+	bus_unregister(&gadget_bus_type);
 	class_destroy(udc_class);
 }
 module_exit(usb_udc_exit);
Index: usb-devel/include/linux/usb/gadget.h
===================================================================
--- usb-devel.orig/include/linux/usb/gadget.h
+++ usb-devel/include/linux/usb/gadget.h
@@ -664,9 +664,9 @@ static inline int usb_gadget_check_confi
  * @driver: Driver model state for this driver.
  * @udc_name: A name of UDC this driver should be bound to. If udc_name is NULL,
  *	this driver will be bound to any available UDC.
- * @pending: UDC core private data used for deferred probe of this driver.
- * @match_existing_only: If udc is not found, return an error and don't add this
- *      gadget driver to list of pending driver
+ * @match_existing_only: If udc is not found, return an error and fail
+ *	the driver registration
+ * @is_bound: Allow a driver to be bound to only one gadget
  *
  * Devices are disabled till a gadget driver successfully bind()s, which
  * means the driver will handle setup() requests needed to enumerate (and
@@ -729,8 +729,8 @@ struct usb_gadget_driver {
 	struct device_driver	driver;
 
 	char			*udc_name;
-	struct list_head	pending;
 	unsigned                match_existing_only:1;
+	bool			is_bound:1;
 };
 
 
@@ -740,22 +740,30 @@ struct usb_gadget_driver {
 /* driver modules register and unregister, as usual.
  * these calls must be made in a context that can sleep.
  *
- * these will usually be implemented directly by the hardware-dependent
- * usb bus interface driver, which will only support a single driver.
+ * A gadget driver can be bound to only one gadget at a time.
  */
 
 /**
- * usb_gadget_register_driver - register a gadget driver
+ * usb_gadget_register_driver_owner - register a gadget driver
  * @driver: the driver being registered
+ * @owner: the driver module
+ * @mod_name: the driver module's build name
  * Context: can sleep
  *
  * Call this in your gadget driver's module initialization function,
- * to tell the underlying usb controller driver about your driver.
+ * to tell the underlying UDC controller driver about your driver.
  * The @bind() function will be called to bind it to a gadget before this
  * registration call returns.  It's expected that the @bind() function will
  * be in init sections.
+ *
+ * Use the macro defined below instead of calling this directly.
  */
-int usb_gadget_register_driver(struct usb_gadget_driver *driver);
+int usb_gadget_register_driver_owner(struct usb_gadget_driver *driver,
+		struct module *owner, const char *mod_name);
+
+/* use a define to avoid include chaining to get THIS_MODULE & friends */
+#define usb_gadget_register_driver(driver) \
+	usb_gadget_register_driver_owner(driver, THIS_MODULE, KBUILD_MODNAME)
 
 /**
  * usb_gadget_unregister_driver - unregister a gadget driver

Re: [RFC PATCH 4/4] USB: gadget: Add a new bus for gadgets

[Pavan Kondeti]

Hi Alan,

On Sun, Mar 20, 2022 at 03:51:04PM -0400, Alan Stern wrote:
> This patch adds a "gadget" bus and uses it for registering gadgets and
> their drivers.  From now on, bindings will be managed by the driver
> core rather than through ad-hoc manipulations in the UDC core.
> 
> As part of this change, the driver_pending_list is removed.  The UDC
> core won't need to keep track of unbound drivers for later binding,
> because the driver core handles all of that for us.
> 
> However, we do need one new feature: a way to prevent gadget drivers
> from being bound to more than one gadget at a time.  The existing code
> does this automatically, but the driver core doesn't -- it's perfectly
> happy to bind a single driver to all the matching devices on the bus.
> The patch adds a new bitflag to the usb_gadget_driver structure for
> this purpose.
> 
> A nice side effect of this change is a reduction in the total lines of
> code, since now the driver core will do part of the work that the UDC
> used to do.
> 
> A possible future patch could add udc devices to the gadget bus, say
> as a separate device type.

Can you please elaborate on this? This UDC device will be added to gadget bus
but not bound to any driver, correct?

> 
> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> 
> ---
> 
>  drivers/usb/gadget/udc/core.c |  248 +++++++++++++++++++-----------------------
>  include/linux/usb/gadget.h    |   26 ++--
>  2 files changed, 135 insertions(+), 139 deletions(-)
> 

<snip>

>  
>  /* ------------------------------------------------------------------------- */
>  
> -static int udc_bind_to_driver(struct usb_udc *udc, struct usb_gadget_driver *driver)
> +static int gadget_match_driver(struct device *dev, struct device_driver *drv)
>  {
> -	int ret;
> +	struct usb_gadget *gadget = dev_to_usb_gadget(dev);
> +	struct usb_udc *udc = gadget->udc;
> +	struct usb_gadget_driver *driver = container_of(drv,
> +			struct usb_gadget_driver, driver);
>  
> -	dev_dbg(&udc->dev, "registering UDC driver [%s]\n",
> -			driver->function);
> +	/* If the driver specifies a udc_name, it must match the UDC's name */
> +	if (driver->udc_name &&
> +			strcmp(driver->udc_name, dev_name(&udc->dev)) != 0)
> +		return 0;
>  
> +	/* Otherwise any gadget driver matches any UDC */
> +	return 1;
> +}
> +

Would it be better if we add the driver->is_bound check here so that probe is
not invoked? your patch checks it later at the probe.

Thanks,
Pavan

Re: [RFC PATCH 4/4] USB: gadget: Add a new bus for gadgets

[Alan Stern]

On Wed, Mar 23, 2022 at 12:25:28PM +0530, Pavan Kondeti wrote:
> Hi Alan,
> 
> On Sun, Mar 20, 2022 at 03:51:04PM -0400, Alan Stern wrote:
> > This patch adds a "gadget" bus and uses it for registering gadgets and
> > their drivers.  From now on, bindings will be managed by the driver
> > core rather than through ad-hoc manipulations in the UDC core.
> > 
> > As part of this change, the driver_pending_list is removed.  The UDC
> > core won't need to keep track of unbound drivers for later binding,
> > because the driver core handles all of that for us.
> > 
> > However, we do need one new feature: a way to prevent gadget drivers
> > from being bound to more than one gadget at a time.  The existing code
> > does this automatically, but the driver core doesn't -- it's perfectly
> > happy to bind a single driver to all the matching devices on the bus.
> > The patch adds a new bitflag to the usb_gadget_driver structure for
> > this purpose.
> > 
> > A nice side effect of this change is a reduction in the total lines of
> > code, since now the driver core will do part of the work that the UDC
> > used to do.
> > 
> > A possible future patch could add udc devices to the gadget bus, say
> > as a separate device type.
> 
> Can you please elaborate on this? This UDC device will be added to gadget bus
> but not bound to any driver, correct?

The UDC/gadget subsystem is designed a little strangely.  For each UDC 
hardware device, the UDC core creates _two_ software representations: a 
struct usb_udc and a struct usb_gadget.  Both of these contain an 
embedded struct device, so the physical UDC hardware corresponds to two 
software "devices".

Currently neither of these devices gets registered on any bus.  There is 
a driver associated with the usb_gadget device, but not in the usual way 
(that is, it doesn't use the normal driver-core binding mechanism).

My patch keeps both of these device structures, but it registers the 
usb_gadget device on the new gadget bus and uses the driver core to do 
normal binding.  The usb_udc device still is not registered on any bus 
and does not have a driver.

> > Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> > 
> > ---
> > 
> >  drivers/usb/gadget/udc/core.c |  248 +++++++++++++++++++-----------------------
> >  include/linux/usb/gadget.h    |   26 ++--
> >  2 files changed, 135 insertions(+), 139 deletions(-)
> > 
> 
> <snip>
> 
> >  
> >  /* ------------------------------------------------------------------------- */
> >  
> > -static int udc_bind_to_driver(struct usb_udc *udc, struct usb_gadget_driver *driver)
> > +static int gadget_match_driver(struct device *dev, struct device_driver *drv)
> >  {
> > -	int ret;
> > +	struct usb_gadget *gadget = dev_to_usb_gadget(dev);
> > +	struct usb_udc *udc = gadget->udc;
> > +	struct usb_gadget_driver *driver = container_of(drv,
> > +			struct usb_gadget_driver, driver);
> >  
> > -	dev_dbg(&udc->dev, "registering UDC driver [%s]\n",
> > -			driver->function);
> > +	/* If the driver specifies a udc_name, it must match the UDC's name */
> > +	if (driver->udc_name &&
> > +			strcmp(driver->udc_name, dev_name(&udc->dev)) != 0)
> > +		return 0;
> >  
> > +	/* Otherwise any gadget driver matches any UDC */
> > +	return 1;
> > +}
> > +
> 
> Would it be better if we add the driver->is_bound check here so that probe is
> not invoked? your patch checks it later at the probe.

Yes, you're right; the check could be moved here.  But this is only 
because the driver core holds the device lock the whole time while it 
does matching and probing.  If the lock were held during probing but not 
matching, it would then be possible for two processes to concurrently 
bind the same driver to two gadgets.

As far as I know, the driver core does not promise to hold the device 
lock during both matching and probing, so it may not be safe to depend 
on this behavior.  Maybe I'm wrong about this...

On the other hand, it wouldn't hurt to do the is_bound check in both 
places; it's a very cheap operation.  Thanks for the suggestion.

Alan Stern

Re: [RFC PATCH 1/4] USB: gadget: Rename usb_gadget_probe_driver()

[Jun Li]

Alan Stern <stern@rowland.harvard.edu> 于2022年3月22日周二 06:57写道：
>
> In preparation for adding a "gadget" bus, this patch renames
> usb_gadget_probe_driver() to usb_gadget_register_driver().  The new
> name will be more accurate, since gadget drivers will be registered on
> the gadget bus and the probing will be done by the driver core, not
> the UDC core.
>
> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>

Missed one rename change:
diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
index 1fb837d9271e..4141206bb0ed 100644
--- a/drivers/usb/gadget/configfs.c
+++ b/drivers/usb/gadget/configfs.c
@@ -284,7 +284,7 @@ static ssize_t gadget_dev_desc_UDC_store(struct
config_item *item,
                        goto err;
                }
                gi->composite.gadget_driver.udc_name = name;
-               ret = usb_gadget_probe_driver(&gi->composite.gadget_driver);
+               ret = usb_gadget_register_driver(&gi->composite.gadget_driver);
                if (ret) {
                        gi->composite.gadget_driver.udc_name = NULL;
                        goto err;
Li Jun
>
>
>  drivers/usb/gadget/composite.c         |    2 +-
>  drivers/usb/gadget/legacy/dbgp.c       |    2 +-
>  drivers/usb/gadget/legacy/inode.c      |    2 +-
>  drivers/usb/gadget/legacy/raw_gadget.c |    4 ++--
>  drivers/usb/gadget/udc/core.c          |    4 ++--
>  include/linux/usb/gadget.h             |    4 ++--
>  6 files changed, 9 insertions(+), 9 deletions(-)
>
> Index: usb-devel/drivers/usb/gadget/composite.c
> ===================================================================
> --- usb-devel.orig/drivers/usb/gadget/composite.c
> +++ usb-devel/drivers/usb/gadget/composite.c
> @@ -2500,7 +2500,7 @@ int usb_composite_probe(struct usb_compo
>         gadget_driver->driver.name = driver->name;
>         gadget_driver->max_speed = driver->max_speed;
>
> -       return usb_gadget_probe_driver(gadget_driver);
> +       return usb_gadget_register_driver(gadget_driver);
>  }
>  EXPORT_SYMBOL_GPL(usb_composite_probe);
>
> Index: usb-devel/drivers/usb/gadget/legacy/dbgp.c
> ===================================================================
> --- usb-devel.orig/drivers/usb/gadget/legacy/dbgp.c
> +++ usb-devel/drivers/usb/gadget/legacy/dbgp.c
> @@ -422,7 +422,7 @@ static struct usb_gadget_driver dbgp_dri
>
>  static int __init dbgp_init(void)
>  {
> -       return usb_gadget_probe_driver(&dbgp_driver);
> +       return usb_gadget_register_driver(&dbgp_driver);
>  }
>
>  static void __exit dbgp_exit(void)
> Index: usb-devel/drivers/usb/gadget/legacy/inode.c
> ===================================================================
> --- usb-devel.orig/drivers/usb/gadget/legacy/inode.c
> +++ usb-devel/drivers/usb/gadget/legacy/inode.c
> @@ -1873,7 +1873,7 @@ dev_config (struct file *fd, const char
>         else
>                 gadgetfs_driver.max_speed = USB_SPEED_FULL;
>
> -       value = usb_gadget_probe_driver(&gadgetfs_driver);
> +       value = usb_gadget_register_driver(&gadgetfs_driver);
>         if (value != 0) {
>                 spin_lock_irq(&dev->lock);
>                 goto fail;
> Index: usb-devel/drivers/usb/gadget/legacy/raw_gadget.c
> ===================================================================
> --- usb-devel.orig/drivers/usb/gadget/legacy/raw_gadget.c
> +++ usb-devel/drivers/usb/gadget/legacy/raw_gadget.c
> @@ -510,12 +510,12 @@ static int raw_ioctl_run(struct raw_dev
>         }
>         spin_unlock_irqrestore(&dev->lock, flags);
>
> -       ret = usb_gadget_probe_driver(&dev->driver);
> +       ret = usb_gadget_register_driver(&dev->driver);
>
>         spin_lock_irqsave(&dev->lock, flags);
>         if (ret) {
>                 dev_err(dev->dev,
> -                       "fail, usb_gadget_probe_driver returned %d\n", ret);
> +                       "fail, usb_gadget_register_driver returned %d\n", ret);
>                 dev->state = STATE_DEV_FAILED;
>                 goto out_unlock;
>         }
> Index: usb-devel/drivers/usb/gadget/udc/core.c
> ===================================================================
> --- usb-devel.orig/drivers/usb/gadget/udc/core.c
> +++ usb-devel/drivers/usb/gadget/udc/core.c
> @@ -1523,7 +1523,7 @@ err1:
>         return ret;
>  }
>
> -int usb_gadget_probe_driver(struct usb_gadget_driver *driver)
> +int usb_gadget_register_driver(struct usb_gadget_driver *driver)
>  {
>         struct usb_udc          *udc = NULL;
>         int                     ret = -ENODEV;
> @@ -1568,7 +1568,7 @@ found:
>         mutex_unlock(&udc_lock);
>         return ret;
>  }
> -EXPORT_SYMBOL_GPL(usb_gadget_probe_driver);
> +EXPORT_SYMBOL_GPL(usb_gadget_register_driver);
>
>  int usb_gadget_unregister_driver(struct usb_gadget_driver *driver)
>  {
> Index: usb-devel/include/linux/usb/gadget.h
> ===================================================================
> --- usb-devel.orig/include/linux/usb/gadget.h
> +++ usb-devel/include/linux/usb/gadget.h
> @@ -745,7 +745,7 @@ struct usb_gadget_driver {
>   */
>
>  /**
> - * usb_gadget_probe_driver - probe a gadget driver
> + * usb_gadget_register_driver - register a gadget driver
>   * @driver: the driver being registered
>   * Context: can sleep
>   *
> @@ -755,7 +755,7 @@ struct usb_gadget_driver {
>   * registration call returns.  It's expected that the @bind() function will
>   * be in init sections.
>   */
> -int usb_gadget_probe_driver(struct usb_gadget_driver *driver);
> +int usb_gadget_register_driver(struct usb_gadget_driver *driver);
>
>  /**
>   * usb_gadget_unregister_driver - unregister a gadget driver

Re: [RFC PATCH 1/4] USB: gadget: Rename usb_gadget_probe_driver()

[Alan Stern]

On Tue, Mar 22, 2022 at 08:57:04PM +0800, Jun Li wrote:
> Alan Stern <stern@rowland.harvard.edu> 于2022年3月22日周二 06:57写道：
> >
> > In preparation for adding a "gadget" bus, this patch renames
> > usb_gadget_probe_driver() to usb_gadget_register_driver().  The new
> > name will be more accurate, since gadget drivers will be registered on
> > the gadget bus and the probing will be done by the driver core, not
> > the UDC core.
> >
> > Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> 
> Missed one rename change:
> diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
> index 1fb837d9271e..4141206bb0ed 100644
> --- a/drivers/usb/gadget/configfs.c
> +++ b/drivers/usb/gadget/configfs.c
> @@ -284,7 +284,7 @@ static ssize_t gadget_dev_desc_UDC_store(struct
> config_item *item,
>                         goto err;
>                 }
>                 gi->composite.gadget_driver.udc_name = name;
> -               ret = usb_gadget_probe_driver(&gi->composite.gadget_driver);
> +               ret = usb_gadget_register_driver(&gi->composite.gadget_driver);
>                 if (ret) {
>                         gi->composite.gadget_driver.udc_name = NULL;
>                         goto err;
> Li Jun

Ah, yes indeed; how careless of me.  Thanks for pointing this out.

Alan Stern

Re: [RFC PATCH 0/4] USB: gadget: Create a bus for gadgets

[Greg KH]

On Sun, Mar 20, 2022 at 03:45:48PM -0400, Alan Stern wrote:
> Everyone:
> 
> The following series of patches implements Greg's suggestion that 
> gadgets should be registered on some sort of bus.  It turns out that the 
> best way to do this is to create a new "gadget" bus, with specialized 
> matching and probing routines, rather than using an existing bus.
> 
> Patches 1-3 are simple preparations for the big change.  They stand on 
> their own, make useful little changes, and could be merged by themselves
> without committing to adding the "gadget" bus.  Patch 4 is main one.
> 
> I'm posting this series for feedback from the Gadget/UDC maintainer and 
> others.  If everything works out okay, the patches can be submitted for 
> real once the upcoming merge window closes.

At first glance, this looks good to me, many thanks for working on this!

greg k-h

[PATCH 1/4] USB: gadget: Rename usb_gadget_probe_driver()

[Alan Stern]

In preparation for adding a "gadget" bus, this patch renames
usb_gadget_probe_driver() to usb_gadget_register_driver().  The new
name will be more accurate, since gadget drivers will be registered on
the gadget bus and the probing will be done by the driver core, not
the UDC core.

Signed-off-by: Alan Stern <stern@rowland.harvard.edu>

---


[as1976]


 drivers/usb/gadget/composite.c         |    2 +-
 drivers/usb/gadget/configfs.c          |    2 +-
 drivers/usb/gadget/legacy/dbgp.c       |    2 +-
 drivers/usb/gadget/legacy/inode.c      |    2 +-
 drivers/usb/gadget/legacy/raw_gadget.c |    4 ++--
 drivers/usb/gadget/udc/core.c          |    4 ++--
 include/linux/usb/gadget.h             |    4 ++--
 7 files changed, 10 insertions(+), 10 deletions(-)

Index: usb-devel/drivers/usb/gadget/composite.c
===================================================================
--- usb-devel.orig/drivers/usb/gadget/composite.c
+++ usb-devel/drivers/usb/gadget/composite.c
@@ -2505,7 +2505,7 @@ int usb_composite_probe(struct usb_compo
 	gadget_driver->driver.name = driver->name;
 	gadget_driver->max_speed = driver->max_speed;
 
-	return usb_gadget_probe_driver(gadget_driver);
+	return usb_gadget_register_driver(gadget_driver);
 }
 EXPORT_SYMBOL_GPL(usb_composite_probe);
 
Index: usb-devel/drivers/usb/gadget/legacy/dbgp.c
===================================================================
--- usb-devel.orig/drivers/usb/gadget/legacy/dbgp.c
+++ usb-devel/drivers/usb/gadget/legacy/dbgp.c
@@ -422,7 +422,7 @@ static struct usb_gadget_driver dbgp_dri
 
 static int __init dbgp_init(void)
 {
-	return usb_gadget_probe_driver(&dbgp_driver);
+	return usb_gadget_register_driver(&dbgp_driver);
 }
 
 static void __exit dbgp_exit(void)
Index: usb-devel/drivers/usb/gadget/legacy/inode.c
===================================================================
--- usb-devel.orig/drivers/usb/gadget/legacy/inode.c
+++ usb-devel/drivers/usb/gadget/legacy/inode.c
@@ -1873,7 +1873,7 @@ dev_config (struct file *fd, const char
 	else
 		gadgetfs_driver.max_speed = USB_SPEED_FULL;
 
-	value = usb_gadget_probe_driver(&gadgetfs_driver);
+	value = usb_gadget_register_driver(&gadgetfs_driver);
 	if (value != 0) {
 		spin_lock_irq(&dev->lock);
 		goto fail;
Index: usb-devel/drivers/usb/gadget/legacy/raw_gadget.c
===================================================================
--- usb-devel.orig/drivers/usb/gadget/legacy/raw_gadget.c
+++ usb-devel/drivers/usb/gadget/legacy/raw_gadget.c
@@ -510,12 +510,12 @@ static int raw_ioctl_run(struct raw_dev
 	}
 	spin_unlock_irqrestore(&dev->lock, flags);
 
-	ret = usb_gadget_probe_driver(&dev->driver);
+	ret = usb_gadget_register_driver(&dev->driver);
 
 	spin_lock_irqsave(&dev->lock, flags);
 	if (ret) {
 		dev_err(dev->dev,
-			"fail, usb_gadget_probe_driver returned %d\n", ret);
+			"fail, usb_gadget_register_driver returned %d\n", ret);
 		dev->state = STATE_DEV_FAILED;
 		goto out_unlock;
 	}
Index: usb-devel/drivers/usb/gadget/udc/core.c
===================================================================
--- usb-devel.orig/drivers/usb/gadget/udc/core.c
+++ usb-devel/drivers/usb/gadget/udc/core.c
@@ -1523,7 +1523,7 @@ err1:
 	return ret;
 }
 
-int usb_gadget_probe_driver(struct usb_gadget_driver *driver)
+int usb_gadget_register_driver(struct usb_gadget_driver *driver)
 {
 	struct usb_udc		*udc = NULL, *iter;
 	int			ret = -ENODEV;
@@ -1572,7 +1572,7 @@ found:
 	mutex_unlock(&udc_lock);
 	return ret;
 }
-EXPORT_SYMBOL_GPL(usb_gadget_probe_driver);
+EXPORT_SYMBOL_GPL(usb_gadget_register_driver);
 
 int usb_gadget_unregister_driver(struct usb_gadget_driver *driver)
 {
Index: usb-devel/include/linux/usb/gadget.h
===================================================================
--- usb-devel.orig/include/linux/usb/gadget.h
+++ usb-devel/include/linux/usb/gadget.h
@@ -745,7 +745,7 @@ struct usb_gadget_driver {
  */
 
 /**
- * usb_gadget_probe_driver - probe a gadget driver
+ * usb_gadget_register_driver - register a gadget driver
  * @driver: the driver being registered
  * Context: can sleep
  *
@@ -755,7 +755,7 @@ struct usb_gadget_driver {
  * registration call returns.  It's expected that the @bind() function will
  * be in init sections.
  */
-int usb_gadget_probe_driver(struct usb_gadget_driver *driver);
+int usb_gadget_register_driver(struct usb_gadget_driver *driver);
 
 /**
  * usb_gadget_unregister_driver - unregister a gadget driver
Index: usb-devel/drivers/usb/gadget/configfs.c
===================================================================
--- usb-devel.orig/drivers/usb/gadget/configfs.c
+++ usb-devel/drivers/usb/gadget/configfs.c
@@ -284,7 +284,7 @@ static ssize_t gadget_dev_desc_UDC_store
 			goto err;
 		}
 		gi->composite.gadget_driver.udc_name = name;
-		ret = usb_gadget_probe_driver(&gi->composite.gadget_driver);
+		ret = usb_gadget_register_driver(&gi->composite.gadget_driver);
 		if (ret) {
 			gi->composite.gadget_driver.udc_name = NULL;
 			goto err;

[PATCH 2/4] USB: gadget: Register udc before gadget

[Alan Stern]

In preparation for adding a "gadget" bus, this patch reverses the
order of registration of udc and gadget devices in usb_add_gadget().

The current code adds the gadget device first, probably because that
was more convenient at the time and the order didn't really matter.
But with the upcoming change, adding the gadget will cause driver
probing to occur.  Unwinding that on the error pathway will become
much more obtrusive, not to mention the fact that a gadget driver
might not work properly before the udc is registered.  It's better to
register the udc device first, particularly since that doesn't involve
a bus or driver binding and therefore is simpler to unwind.

For symmetry, the order of unregistration in usb_del_gadget() is
likewise reversed.

Signed-off-by: Alan Stern <stern@rowland.harvard.edu>

---


[as1977]


 drivers/usb/gadget/udc/core.c |   17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

Index: usb-devel/drivers/usb/gadget/udc/core.c
===================================================================
--- usb-devel.orig/drivers/usb/gadget/udc/core.c
+++ usb-devel/drivers/usb/gadget/udc/core.c
@@ -1308,10 +1308,6 @@ int usb_add_gadget(struct usb_gadget *ga
 	if (ret)
 		goto err_put_udc;
 
-	ret = device_add(&gadget->dev);
-	if (ret)
-		goto err_put_udc;
-
 	udc->gadget = gadget;
 	gadget->udc = udc;
 
@@ -1327,15 +1323,22 @@ int usb_add_gadget(struct usb_gadget *ga
 	usb_gadget_set_state(gadget, USB_STATE_NOTATTACHED);
 	udc->vbus = true;
 
+	ret = device_add(&gadget->dev);
+	if (ret)
+		goto err_del_udc;
+
 	/* pick up one of pending gadget drivers */
 	ret = check_pending_gadget_drivers(udc);
 	if (ret)
-		goto err_del_udc;
+		goto err_del_gadget;
 
 	mutex_unlock(&udc_lock);
 
 	return 0;
 
+ err_del_gadget:
+	device_del(&gadget->dev);
+
  err_del_udc:
 	flush_work(&gadget->work);
 	device_del(&udc->dev);
@@ -1344,8 +1347,6 @@ int usb_add_gadget(struct usb_gadget *ga
 	list_del(&udc->list);
 	mutex_unlock(&udc_lock);
 
-	device_del(&gadget->dev);
-
  err_put_udc:
 	put_device(&udc->dev);
 
@@ -1469,8 +1470,8 @@ void usb_del_gadget(struct usb_gadget *g
 
 	kobject_uevent(&udc->dev.kobj, KOBJ_REMOVE);
 	flush_work(&gadget->work);
-	device_unregister(&udc->dev);
 	device_del(&gadget->dev);
+	device_unregister(&udc->dev);
 }
 EXPORT_SYMBOL_GPL(usb_del_gadget);
 

[PATCH 3/4] USB: gadget: Fix mistakes in UDC core kerneldoc

[Alan Stern]

This patch fixes some minor mistakes in the UDC core's kerneldoc.

Signed-off-by: Alan Stern <stern@rowland.harvard.edu>

---


[as1978]


 drivers/usb/gadget/udc/core.c |   14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

Index: usb-devel/drivers/usb/gadget/udc/core.c
===================================================================
--- usb-devel.orig/drivers/usb/gadget/udc/core.c
+++ usb-devel/drivers/usb/gadget/udc/core.c
@@ -1262,9 +1262,6 @@ static int check_pending_gadget_drivers(
  * device.
  * @gadget: the gadget to be initialized.
  * @release: a gadget release function.
- *
- * Returns zero on success, negative errno otherwise.
- * Calls the gadget release function in the latter case.
  */
 void usb_initialize_gadget(struct device *parent, struct usb_gadget *gadget,
 		void (*release)(struct device *dev))
@@ -1441,11 +1438,10 @@ static void usb_gadget_remove_driver(str
 }
 
 /**
- * usb_del_gadget - deletes @udc from udc_list
- * @gadget: the gadget to be removed.
+ * usb_del_gadget - deletes a gadget and unregisters its udc
+ * @gadget: the gadget to be deleted.
  *
- * This will call usb_gadget_unregister_driver() if
- * the @udc is still busy.
+ * This will unbind @gadget, if it is bound.
  * It will not do a final usb_put_gadget().
  */
 void usb_del_gadget(struct usb_gadget *gadget)
@@ -1476,8 +1472,8 @@ void usb_del_gadget(struct usb_gadget *g
 EXPORT_SYMBOL_GPL(usb_del_gadget);
 
 /**
- * usb_del_gadget_udc - deletes @udc from udc_list
- * @gadget: the gadget to be removed.
+ * usb_del_gadget_udc - unregisters a gadget
+ * @gadget: the gadget to be unregistered.
  *
  * Calls usb_del_gadget() and does a final usb_put_gadget().
  */

[PATCH 4/4] USB: gadget: Add a new bus for gadgets

[Alan Stern]

This patch adds a "gadget" bus and uses it for registering gadgets and
their drivers.  From now on, bindings will be managed by the driver
core rather than through ad-hoc manipulations in the UDC core.

As part of this change, the driver_pending_list is removed.  The UDC
core won't need to keep track of unbound drivers for later binding,
because the driver core handles all of that for us.

However, we do need one new feature: a way to prevent gadget drivers
from being bound to more than one gadget at a time.  The existing code
does this automatically, but the driver core doesn't -- it's perfectly
happy to bind a single driver to all the matching devices on the bus.
The patch adds a new bitflag to the usb_gadget_driver structure for
this purpose.

A nice side effect of this change is a reduction in the total lines of
code, since now the driver core will do part of the work that the UDC
used to do.

A possible future patch could add udc devices to the gadget bus, say
as a separate device type.

Signed-off-by: Alan Stern <stern@rowland.harvard.edu>

---


[as1979]


 drivers/usb/gadget/udc/core.c |  256 ++++++++++++++++++++----------------------
 include/linux/usb/gadget.h    |   26 ++--
 2 files changed, 139 insertions(+), 143 deletions(-)

Index: usb-devel/drivers/usb/gadget/udc/core.c
===================================================================
--- usb-devel.orig/drivers/usb/gadget/udc/core.c
+++ usb-devel/drivers/usb/gadget/udc/core.c
@@ -23,6 +23,8 @@
 
 #include "trace.h"
 
+static struct bus_type gadget_bus_type;
+
 /**
  * struct usb_udc - describes one usb device controller
  * @driver: the gadget driver pointer. For use by the class code
@@ -47,11 +49,9 @@ 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);
+/* Protects udc_list, udc->driver, driver->is_bound, and related calls */
+static DEFINE_MUTEX(udc_lock);
 
 /* ------------------------------------------------------------------------- */
 
@@ -1238,24 +1238,6 @@ static void usb_udc_nop_release(struct d
 	dev_vdbg(dev, "%s\n", __func__);
 }
 
-/* should be called with udc_lock held */
-static int check_pending_gadget_drivers(struct usb_udc *udc)
-{
-	struct usb_gadget_driver *driver;
-	int ret = 0;
-
-	list_for_each_entry(driver, &gadget_driver_pending_list, pending)
-		if (!driver->udc_name || strcmp(driver->udc_name,
-						dev_name(&udc->dev)) == 0) {
-			ret = udc_bind_to_driver(udc, driver);
-			if (ret != -EPROBE_DEFER)
-				list_del_init(&driver->pending);
-			break;
-		}
-
-	return ret;
-}
-
 /**
  * usb_initialize_gadget - initialize a gadget and its embedded struct device
  * @parent: the parent device to this udc. Usually the controller driver's
@@ -1276,6 +1258,7 @@ void usb_initialize_gadget(struct device
 		gadget->dev.release = usb_udc_nop_release;
 
 	device_initialize(&gadget->dev);
+	gadget->dev.bus = &gadget_bus_type;
 }
 EXPORT_SYMBOL_GPL(usb_initialize_gadget);
 
@@ -1312,6 +1295,7 @@ int usb_add_gadget(struct usb_gadget *ga
 
 	mutex_lock(&udc_lock);
 	list_add_tail(&udc->list, &udc_list);
+	mutex_unlock(&udc_lock);
 
 	ret = device_add(&udc->dev);
 	if (ret)
@@ -1324,23 +1308,14 @@ int usb_add_gadget(struct usb_gadget *ga
 	if (ret)
 		goto err_del_udc;
 
-	/* pick up one of pending gadget drivers */
-	ret = check_pending_gadget_drivers(udc);
-	if (ret)
-		goto err_del_gadget;
-
-	mutex_unlock(&udc_lock);
-
 	return 0;
 
- err_del_gadget:
-	device_del(&gadget->dev);
-
  err_del_udc:
 	flush_work(&gadget->work);
 	device_del(&udc->dev);
 
  err_unlist_udc:
+	mutex_lock(&udc_lock);
 	list_del(&udc->list);
 	mutex_unlock(&udc_lock);
 
@@ -1419,24 +1394,6 @@ int usb_add_gadget_udc(struct device *pa
 }
 EXPORT_SYMBOL_GPL(usb_add_gadget_udc);
 
-static void usb_gadget_remove_driver(struct usb_udc *udc)
-{
-	dev_dbg(&udc->dev, "unregistering UDC driver [%s]\n",
-			udc->driver->function);
-
-	kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE);
-
-	usb_gadget_disconnect(udc->gadget);
-	usb_gadget_disable_async_callbacks(udc);
-	if (udc->gadget->irq)
-		synchronize_irq(udc->gadget->irq);
-	udc->driver->unbind(udc->gadget);
-	usb_gadget_udc_stop(udc);
-
-	udc->driver = NULL;
-	udc->gadget->dev.driver = NULL;
-}
-
 /**
  * usb_del_gadget - deletes a gadget and unregisters its udc
  * @gadget: the gadget to be deleted.
@@ -1455,13 +1412,6 @@ void usb_del_gadget(struct usb_gadget *g
 
 	mutex_lock(&udc_lock);
 	list_del(&udc->list);
-
-	if (udc->driver) {
-		struct usb_gadget_driver *driver = udc->driver;
-
-		usb_gadget_remove_driver(udc);
-		list_add(&driver->pending, &gadget_driver_pending_list);
-	}
 	mutex_unlock(&udc_lock);
 
 	kobject_uevent(&udc->dev.kobj, KOBJ_REMOVE);
@@ -1486,123 +1436,147 @@ EXPORT_SYMBOL_GPL(usb_del_gadget_udc);
 
 /* ------------------------------------------------------------------------- */
 
-static int udc_bind_to_driver(struct usb_udc *udc, struct usb_gadget_driver *driver)
+static int gadget_match_driver(struct device *dev, struct device_driver *drv)
 {
-	int ret;
+	struct usb_gadget *gadget = dev_to_usb_gadget(dev);
+	struct usb_udc *udc = gadget->udc;
+	struct usb_gadget_driver *driver = container_of(drv,
+			struct usb_gadget_driver, driver);
+
+	/* If the driver specifies a udc_name, it must match the UDC's name */
+	if (driver->udc_name &&
+			strcmp(driver->udc_name, dev_name(&udc->dev)) != 0)
+		return 0;
+
+	/* If the driver is already bound to a gadget, it doesn't match */
+	if (driver->is_bound)
+		return 0;
+
+	/* Otherwise any gadget driver matches any UDC */
+	return 1;
+}
 
-	dev_dbg(&udc->dev, "registering UDC driver [%s]\n",
-			driver->function);
+static int gadget_bind_driver(struct device *dev)
+{
+	struct usb_gadget *gadget = dev_to_usb_gadget(dev);
+	struct usb_udc *udc = gadget->udc;
+	struct usb_gadget_driver *driver = container_of(dev->driver,
+			struct usb_gadget_driver, driver);
+	int ret = 0;
 
+	mutex_lock(&udc_lock);
+	if (driver->is_bound) {
+		mutex_unlock(&udc_lock);
+		return -ENXIO;		/* Driver binds to only one gadget */
+	}
+	driver->is_bound = true;
 	udc->driver = driver;
-	udc->gadget->dev.driver = &driver->driver;
+	mutex_unlock(&udc_lock);
+
+	dev_dbg(&udc->dev, "binding gadget driver [%s]\n", driver->function);
 
 	usb_gadget_udc_set_speed(udc, driver->max_speed);
 
+	mutex_lock(&udc_lock);
 	ret = driver->bind(udc->gadget, driver);
 	if (ret)
-		goto err1;
+		goto err_bind;
+
 	ret = usb_gadget_udc_start(udc);
-	if (ret) {
-		driver->unbind(udc->gadget);
-		goto err1;
-	}
+	if (ret)
+		goto err_start;
 	usb_gadget_enable_async_callbacks(udc);
 	usb_udc_connect_control(udc);
+	mutex_unlock(&udc_lock);
 
 	kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE);
 	return 0;
-err1:
+
+ err_start:
+	driver->unbind(udc->gadget);
+
+ err_bind:
 	if (ret != -EISNAM)
 		dev_err(&udc->dev, "failed to start %s: %d\n",
-			udc->driver->function, ret);
+			driver->function, ret);
+
 	udc->driver = NULL;
-	udc->gadget->dev.driver = NULL;
+	driver->is_bound = false;
+	mutex_unlock(&udc_lock);
+
 	return ret;
 }
 
-int usb_gadget_register_driver(struct usb_gadget_driver *driver)
+static void gadget_unbind_driver(struct device *dev)
+{
+	struct usb_gadget *gadget = dev_to_usb_gadget(dev);
+	struct usb_udc *udc = gadget->udc;
+	struct usb_gadget_driver *driver = udc->driver;
+
+	dev_dbg(&udc->dev, "unbinding gadget driver [%s]\n", driver->function);
+
+	kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE);
+
+	mutex_lock(&udc_lock);
+	usb_gadget_disconnect(gadget);
+	usb_gadget_disable_async_callbacks(udc);
+	if (gadget->irq)
+		synchronize_irq(gadget->irq);
+	udc->driver->unbind(gadget);
+	usb_gadget_udc_stop(udc);
+
+	driver->is_bound = false;
+	udc->driver = NULL;
+	mutex_unlock(&udc_lock);
+}
+
+/* ------------------------------------------------------------------------- */
+
+int usb_gadget_register_driver_owner(struct usb_gadget_driver *driver,
+		struct module *owner, const char *mod_name)
 {
-	struct usb_udc		*udc = NULL, *iter;
-	int			ret = -ENODEV;
+	int ret;
 
 	if (!driver || !driver->bind || !driver->setup)
 		return -EINVAL;
 
+	driver->driver.bus = &gadget_bus_type;
+	driver->driver.owner = owner;
+	driver->driver.mod_name = mod_name;
+	ret = driver_register(&driver->driver);
+	if (ret) {
+		pr_warn("%s: driver registration failed: %d\n",
+				driver->function, ret);
+		return ret;
+	}
+
 	mutex_lock(&udc_lock);
-	if (driver->udc_name) {
-		list_for_each_entry(iter, &udc_list, list) {
-			ret = strcmp(driver->udc_name, dev_name(&iter->dev));
-			if (ret)
-				continue;
-			udc = iter;
-			break;
-		}
-		if (ret)
-			ret = -ENODEV;
-		else if (udc->driver)
+	if (!driver->is_bound) {
+		if (driver->match_existing_only) {
+			pr_warn("%s: couldn't find an available UDC or it's busy\n",
+					driver->function);
 			ret = -EBUSY;
-		else
-			goto found;
-	} else {
-		list_for_each_entry(iter, &udc_list, list) {
-			/* For now we take the first one */
-			if (iter->driver)
-				continue;
-			udc = iter;
-			goto found;
+		} else {
+			pr_info("%s: couldn't find an available UDC\n",
+					driver->function);
 		}
-	}
-
-	if (!driver->match_existing_only) {
-		list_add_tail(&driver->pending, &gadget_driver_pending_list);
-		pr_info("couldn't find an available UDC - added [%s] to list of pending drivers\n",
-			driver->function);
 		ret = 0;
 	}
-
 	mutex_unlock(&udc_lock);
+
 	if (ret)
-		pr_warn("couldn't find an available UDC or it's busy: %d\n", ret);
-	return ret;
-found:
-	ret = udc_bind_to_driver(udc, driver);
-	mutex_unlock(&udc_lock);
+		driver_unregister(&driver->driver);
 	return ret;
 }
-EXPORT_SYMBOL_GPL(usb_gadget_register_driver);
+EXPORT_SYMBOL_GPL(usb_gadget_register_driver_owner);
 
 int usb_gadget_unregister_driver(struct usb_gadget_driver *driver)
 {
-	struct usb_udc		*udc = NULL;
-	int			ret = -ENODEV;
-
 	if (!driver || !driver->unbind)
 		return -EINVAL;
 
-	mutex_lock(&udc_lock);
-	list_for_each_entry(udc, &udc_list, list) {
-		if (udc->driver == driver) {
-			usb_gadget_remove_driver(udc);
-			usb_gadget_set_state(udc->gadget,
-					     USB_STATE_NOTATTACHED);
-
-			/* Maybe there is someone waiting for this UDC? */
-			check_pending_gadget_drivers(udc);
-			/*
-			 * For now we ignore bind errors as probably it's
-			 * not a valid reason to fail other's gadget unbind
-			 */
-			ret = 0;
-			break;
-		}
-	}
-
-	if (ret) {
-		list_del(&driver->pending);
-		ret = 0;
-	}
-	mutex_unlock(&udc_lock);
-	return ret;
+	driver_unregister(&driver->driver);
+	return 0;
 }
 EXPORT_SYMBOL_GPL(usb_gadget_unregister_driver);
 
@@ -1754,8 +1728,17 @@ static int usb_udc_uevent(struct device
 	return 0;
 }
 
+static struct bus_type gadget_bus_type = {
+	.name = "gadget",
+	.probe = gadget_bind_driver,
+	.remove = gadget_unbind_driver,
+	.match = gadget_match_driver,
+};
+
 static int __init usb_udc_init(void)
 {
+	int rc;
+
 	udc_class = class_create(THIS_MODULE, "udc");
 	if (IS_ERR(udc_class)) {
 		pr_err("failed to create udc class --> %ld\n",
@@ -1764,12 +1747,17 @@ static int __init usb_udc_init(void)
 	}
 
 	udc_class->dev_uevent = usb_udc_uevent;
-	return 0;
+
+	rc = bus_register(&gadget_bus_type);
+	if (rc)
+		class_destroy(udc_class);
+	return rc;
 }
 subsys_initcall(usb_udc_init);
 
 static void __exit usb_udc_exit(void)
 {
+	bus_unregister(&gadget_bus_type);
 	class_destroy(udc_class);
 }
 module_exit(usb_udc_exit);
Index: usb-devel/include/linux/usb/gadget.h
===================================================================
--- usb-devel.orig/include/linux/usb/gadget.h
+++ usb-devel/include/linux/usb/gadget.h
@@ -664,9 +664,9 @@ static inline int usb_gadget_check_confi
  * @driver: Driver model state for this driver.
  * @udc_name: A name of UDC this driver should be bound to. If udc_name is NULL,
  *	this driver will be bound to any available UDC.
- * @pending: UDC core private data used for deferred probe of this driver.
- * @match_existing_only: If udc is not found, return an error and don't add this
- *      gadget driver to list of pending driver
+ * @match_existing_only: If udc is not found, return an error and fail
+ *	the driver registration
+ * @is_bound: Allow a driver to be bound to only one gadget
  *
  * Devices are disabled till a gadget driver successfully bind()s, which
  * means the driver will handle setup() requests needed to enumerate (and
@@ -729,8 +729,8 @@ struct usb_gadget_driver {
 	struct device_driver	driver;
 
 	char			*udc_name;
-	struct list_head	pending;
 	unsigned                match_existing_only:1;
+	bool			is_bound:1;
 };
 
 
@@ -740,22 +740,30 @@ struct usb_gadget_driver {
 /* driver modules register and unregister, as usual.
  * these calls must be made in a context that can sleep.
  *
- * these will usually be implemented directly by the hardware-dependent
- * usb bus interface driver, which will only support a single driver.
+ * A gadget driver can be bound to only one gadget at a time.
  */
 
 /**
- * usb_gadget_register_driver - register a gadget driver
+ * usb_gadget_register_driver_owner - register a gadget driver
  * @driver: the driver being registered
+ * @owner: the driver module
+ * @mod_name: the driver module's build name
  * Context: can sleep
  *
  * Call this in your gadget driver's module initialization function,
- * to tell the underlying usb controller driver about your driver.
+ * to tell the underlying UDC controller driver about your driver.
  * The @bind() function will be called to bind it to a gadget before this
  * registration call returns.  It's expected that the @bind() function will
  * be in init sections.
+ *
+ * Use the macro defined below instead of calling this directly.
  */
-int usb_gadget_register_driver(struct usb_gadget_driver *driver);
+int usb_gadget_register_driver_owner(struct usb_gadget_driver *driver,
+		struct module *owner, const char *mod_name);
+
+/* use a define to avoid include chaining to get THIS_MODULE & friends */
+#define usb_gadget_register_driver(driver) \
+	usb_gadget_register_driver_owner(driver, THIS_MODULE, KBUILD_MODNAME)
 
 /**
  * usb_gadget_unregister_driver - unregister a gadget driver

Re: [PATCH 4/4] USB: gadget: Add a new bus for gadgets

[Geert Uytterhoeven]

 	Hi Alan,

On Sat, 23 Apr 2022, Alan Stern wrote:
> This patch adds a "gadget" bus and uses it for registering gadgets and
> their drivers.  From now on, bindings will be managed by the driver
> core rather than through ad-hoc manipulations in the UDC core.
>
> As part of this change, the driver_pending_list is removed.  The UDC
> core won't need to keep track of unbound drivers for later binding,
> because the driver core handles all of that for us.
>
> However, we do need one new feature: a way to prevent gadget drivers
> from being bound to more than one gadget at a time.  The existing code
> does this automatically, but the driver core doesn't -- it's perfectly
> happy to bind a single driver to all the matching devices on the bus.
> The patch adds a new bitflag to the usb_gadget_driver structure for
> this purpose.
>
> A nice side effect of this change is a reduction in the total lines of
> code, since now the driver core will do part of the work that the UDC
> used to do.
>
> A possible future patch could add udc devices to the gadget bus, say
> as a separate device type.
>
> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>

Thanks for your patch, which is now commit fc274c1e997314bf ("USB:
gadget: Add a new bus for gadgets") in usb-next.

This patch cause a regression on the Renesas Salvator-XS development
board, as R-Car H3 has multiple USB gadget devices:

     sysfs: cannot create duplicate filename '/bus/gadget/devices/gadget'
     CPU: 2 PID: 1 Comm: swapper/0 Not tainted 5.18.0-rc1-arm64-renesas-00074-gfc274c1e9973 #1587
     Hardware name: Renesas Salvator-X 2nd version board based on r8a77951 (DT)
     Call trace:
      dump_backtrace+0xcc/0xd8
      show_stack+0x14/0x30
      dump_stack_lvl+0x88/0xb0
      dump_stack+0x14/0x2c
      sysfs_warn_dup+0x60/0x78
      sysfs_do_create_link_sd.isra.0+0xe4/0xf0
      sysfs_create_link+0x20/0x40
      bus_add_device+0x64/0x110
      device_add+0x31c/0x850
      usb_add_gadget+0x124/0x1a0
      usb_add_gadget_udc_release+0x1c/0x50
      usb_add_gadget_udc+0x10/0x18
      renesas_usb3_probe+0x450/0x728
      platform_probe+0x64/0xd0
      really_probe+0x100/0x2a0
      __driver_probe_device+0x74/0xd8
      driver_probe_device+0x3c/0xe0
      __driver_attach+0x80/0x110
      bus_for_each_dev+0x6c/0xc0
      driver_attach+0x20/0x28
      bus_add_driver+0x138/0x1e0
      driver_register+0x60/0x110
      __platform_driver_register+0x24/0x30
      renesas_usb3_driver_init+0x18/0x20
      do_one_initcall+0x15c/0x31c
      kernel_init_freeable+0x2f0/0x354
      kernel_init+0x20/0x120
      ret_from_fork+0x10/0x20
     renesas_usb3: probe of ee020000.usb failed with error -17
     ...
     renesas_usbhs: probe of e6590000.usb failed with error -17

After boot-up, only one gadget device is visible:

     root@h3-salvator-xs:~# ls -l /sys/bus/gadget/devices/
     total 0
     lrwxrwxrwx 1 root root 0 Feb 14  2019 gadget -> ../../../devices/platform/soc/e659c000.usb/gadget
     root@h3-salvator-xs:~#

Reverting this patch fixes the issue.

Gr{oetje,eeting}s,

 						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
 							    -- Linus Torvalds

Re: [PATCH 4/4] USB: gadget: Add a new bus for gadgets

[Alan Stern]

On Tue, May 03, 2022 at 12:14:30PM +0200, Geert Uytterhoeven wrote:
> 	Hi Alan,
> 
> On Sat, 23 Apr 2022, Alan Stern wrote:
> > This patch adds a "gadget" bus and uses it for registering gadgets and
> > their drivers.  From now on, bindings will be managed by the driver
> > core rather than through ad-hoc manipulations in the UDC core.
> > 
> > As part of this change, the driver_pending_list is removed.  The UDC
> > core won't need to keep track of unbound drivers for later binding,
> > because the driver core handles all of that for us.
> > 
> > However, we do need one new feature: a way to prevent gadget drivers
> > from being bound to more than one gadget at a time.  The existing code
> > does this automatically, but the driver core doesn't -- it's perfectly
> > happy to bind a single driver to all the matching devices on the bus.
> > The patch adds a new bitflag to the usb_gadget_driver structure for
> > this purpose.
> > 
> > A nice side effect of this change is a reduction in the total lines of
> > code, since now the driver core will do part of the work that the UDC
> > used to do.
> > 
> > A possible future patch could add udc devices to the gadget bus, say
> > as a separate device type.
> > 
> > Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> 
> Thanks for your patch, which is now commit fc274c1e997314bf ("USB:
> gadget: Add a new bus for gadgets") in usb-next.
> 
> This patch cause a regression on the Renesas Salvator-XS development
> board, as R-Car H3 has multiple USB gadget devices:

Then these gadgets ought to have distinct names in order to avoid the 
conflict below:

>     sysfs: cannot create duplicate filename '/bus/gadget/devices/gadget'
>     CPU: 2 PID: 1 Comm: swapper/0 Not tainted 5.18.0-rc1-arm64-renesas-00074-gfc274c1e9973 #1587
>     Hardware name: Renesas Salvator-X 2nd version board based on r8a77951 (DT)
>     Call trace:
>      dump_backtrace+0xcc/0xd8
>      show_stack+0x14/0x30
>      dump_stack_lvl+0x88/0xb0
>      dump_stack+0x14/0x2c
>      sysfs_warn_dup+0x60/0x78
>      sysfs_do_create_link_sd.isra.0+0xe4/0xf0
>      sysfs_create_link+0x20/0x40
>      bus_add_device+0x64/0x110
>      device_add+0x31c/0x850
>      usb_add_gadget+0x124/0x1a0
>      usb_add_gadget_udc_release+0x1c/0x50
>      usb_add_gadget_udc+0x10/0x18
>      renesas_usb3_probe+0x450/0x728
...

Having three gadget devices, all named "gadget", doesn't seem like a 
good idea.

> After boot-up, only one gadget device is visible:

Naturally, since the first registration succeeds and the later ones fail 
because they can't reuse the same name.

>     root@h3-salvator-xs:~# ls -l /sys/bus/gadget/devices/
>     total 0
>     lrwxrwxrwx 1 root root 0 Feb 14  2019 gadget -> ../../../devices/platform/soc/e659c000.usb/gadget
>     root@h3-salvator-xs:~#
> 
> Reverting this patch fixes the issue.

This doesn't seem like it should be too hard to fix, although I'm not 
at all familiar with the renesas-usb3 driver.  Do you know who maintains 
that driver?  Is it you?

Alan Stern

Re: [PATCH 4/4] USB: gadget: Add a new bus for gadgets

[Geert Uytterhoeven]

Hi Alan,

On Tue, May 3, 2022 at 5:14 PM Alan Stern <stern@rowland.harvard.edu> wrote:
> On Tue, May 03, 2022 at 12:14:30PM +0200, Geert Uytterhoeven wrote:
> > On Sat, 23 Apr 2022, Alan Stern wrote:
> > > This patch adds a "gadget" bus and uses it for registering gadgets and
> > > their drivers.  From now on, bindings will be managed by the driver
> > > core rather than through ad-hoc manipulations in the UDC core.
> > >
> > > As part of this change, the driver_pending_list is removed.  The UDC
> > > core won't need to keep track of unbound drivers for later binding,
> > > because the driver core handles all of that for us.
> > >
> > > However, we do need one new feature: a way to prevent gadget drivers
> > > from being bound to more than one gadget at a time.  The existing code
> > > does this automatically, but the driver core doesn't -- it's perfectly
> > > happy to bind a single driver to all the matching devices on the bus.
> > > The patch adds a new bitflag to the usb_gadget_driver structure for
> > > this purpose.
> > >
> > > A nice side effect of this change is a reduction in the total lines of
> > > code, since now the driver core will do part of the work that the UDC
> > > used to do.
> > >
> > > A possible future patch could add udc devices to the gadget bus, say
> > > as a separate device type.
> > >
> > > Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> >
> > Thanks for your patch, which is now commit fc274c1e997314bf ("USB:
> > gadget: Add a new bus for gadgets") in usb-next.
> >
> > This patch cause a regression on the Renesas Salvator-XS development
> > board, as R-Car H3 has multiple USB gadget devices:
>
> Then these gadgets ought to have distinct names in order to avoid the
> conflict below:
>
> >     sysfs: cannot create duplicate filename '/bus/gadget/devices/gadget'
> >     CPU: 2 PID: 1 Comm: swapper/0 Not tainted 5.18.0-rc1-arm64-renesas-00074-gfc274c1e9973 #1587
> >     Hardware name: Renesas Salvator-X 2nd version board based on r8a77951 (DT)
> >     Call trace:
> >      dump_backtrace+0xcc/0xd8
> >      show_stack+0x14/0x30
> >      dump_stack_lvl+0x88/0xb0
> >      dump_stack+0x14/0x2c
> >      sysfs_warn_dup+0x60/0x78
> >      sysfs_do_create_link_sd.isra.0+0xe4/0xf0
> >      sysfs_create_link+0x20/0x40
> >      bus_add_device+0x64/0x110
> >      device_add+0x31c/0x850
> >      usb_add_gadget+0x124/0x1a0
> >      usb_add_gadget_udc_release+0x1c/0x50
> >      usb_add_gadget_udc+0x10/0x18
> >      renesas_usb3_probe+0x450/0x728
> ...
>
> Having three gadget devices, all named "gadget", doesn't seem like a
> good idea.

I'm not so sure where these names are coming from.
`git grep '"gadget"'` points to the following likely targets:

drivers/usb/gadget/udc/core.c:  dev_set_name(&gadget->dev, "gadget");
drivers/usb/renesas_usbhs/mod_gadget.c: gpriv->mod.name         = "gadget";

Changing both names reveals the problem is actually caused by
the former ;-)

> This doesn't seem like it should be too hard to fix, although I'm not
> at all familiar with the renesas-usb3 driver.  Do you know who maintains
> that driver?  Is it you?

Adding Shimoda-san to CC (but he's enjoying Golden Week).

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 4/4] USB: gadget: Add a new bus for gadgets

[Alan Stern]

On Tue, May 03, 2022 at 05:27:08PM +0200, Geert Uytterhoeven wrote:
> Hi Alan,
> 
> On Tue, May 3, 2022 at 5:14 PM Alan Stern <stern@rowland.harvard.edu> wrote:
> > On Tue, May 03, 2022 at 12:14:30PM +0200, Geert Uytterhoeven wrote:
> > > On Sat, 23 Apr 2022, Alan Stern wrote:
> > > > This patch adds a "gadget" bus and uses it for registering gadgets and
> > > > their drivers.  From now on, bindings will be managed by the driver
> > > > core rather than through ad-hoc manipulations in the UDC core.
> > > >
> > > > As part of this change, the driver_pending_list is removed.  The UDC
> > > > core won't need to keep track of unbound drivers for later binding,
> > > > because the driver core handles all of that for us.
> > > >
> > > > However, we do need one new feature: a way to prevent gadget drivers
> > > > from being bound to more than one gadget at a time.  The existing code
> > > > does this automatically, but the driver core doesn't -- it's perfectly
> > > > happy to bind a single driver to all the matching devices on the bus.
> > > > The patch adds a new bitflag to the usb_gadget_driver structure for
> > > > this purpose.
> > > >
> > > > A nice side effect of this change is a reduction in the total lines of
> > > > code, since now the driver core will do part of the work that the UDC
> > > > used to do.
> > > >
> > > > A possible future patch could add udc devices to the gadget bus, say
> > > > as a separate device type.
> > > >
> > > > Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> > >
> > > Thanks for your patch, which is now commit fc274c1e997314bf ("USB:
> > > gadget: Add a new bus for gadgets") in usb-next.
> > >
> > > This patch cause a regression on the Renesas Salvator-XS development
> > > board, as R-Car H3 has multiple USB gadget devices:
> >
> > Then these gadgets ought to have distinct names in order to avoid the
> > conflict below:
> >
> > >     sysfs: cannot create duplicate filename '/bus/gadget/devices/gadget'
> > >     CPU: 2 PID: 1 Comm: swapper/0 Not tainted 5.18.0-rc1-arm64-renesas-00074-gfc274c1e9973 #1587
> > >     Hardware name: Renesas Salvator-X 2nd version board based on r8a77951 (DT)
> > >     Call trace:
> > >      dump_backtrace+0xcc/0xd8
> > >      show_stack+0x14/0x30
> > >      dump_stack_lvl+0x88/0xb0
> > >      dump_stack+0x14/0x2c
> > >      sysfs_warn_dup+0x60/0x78
> > >      sysfs_do_create_link_sd.isra.0+0xe4/0xf0
> > >      sysfs_create_link+0x20/0x40
> > >      bus_add_device+0x64/0x110
> > >      device_add+0x31c/0x850
> > >      usb_add_gadget+0x124/0x1a0
> > >      usb_add_gadget_udc_release+0x1c/0x50
> > >      usb_add_gadget_udc+0x10/0x18
> > >      renesas_usb3_probe+0x450/0x728
> > ...
> >
> > Having three gadget devices, all named "gadget", doesn't seem like a
> > good idea.
> 
> I'm not so sure where these names are coming from.
> `git grep '"gadget"'` points to the following likely targets:
> 
> drivers/usb/gadget/udc/core.c:  dev_set_name(&gadget->dev, "gadget");
> drivers/usb/renesas_usbhs/mod_gadget.c: gpriv->mod.name         = "gadget";
> 
> Changing both names reveals the problem is actually caused by
> the former ;-)

Ah, good.

One way to attack this would be to keep a static counter and dynamically 
set the name to "gadget.%d" using the counter's value.  Or keep a bitmap 
of allocated gadget numbers and use the first available number.

Felipe, Greg, any opinions?

Ironically, the UDC driver itself provides a name in gadget->name.  But 
that string isn't unique either (in renesas-usb3, for instance, it is 
always set to "renesas_usb3"), so it won't help solve this problem.

> > This doesn't seem like it should be too hard to fix, although I'm not
> > at all familiar with the renesas-usb3 driver.  Do you know who maintains
> > that driver?  Is it you?
> 
> Adding Shimoda-san to CC (but he's enjoying Golden Week).

It looks like the problem has to be solved in the gadget core rather 
than in the UDC driver.  So we won't need to modify the driver after 
all.

Alan Stern

Re: [PATCH 4/4] USB: gadget: Add a new bus for gadgets

[Greg KH]

On Tue, May 03, 2022 at 11:48:33AM -0400, Alan Stern wrote:
> On Tue, May 03, 2022 at 05:27:08PM +0200, Geert Uytterhoeven wrote:
> > Hi Alan,
> > 
> > On Tue, May 3, 2022 at 5:14 PM Alan Stern <stern@rowland.harvard.edu> wrote:
> > > On Tue, May 03, 2022 at 12:14:30PM +0200, Geert Uytterhoeven wrote:
> > > > On Sat, 23 Apr 2022, Alan Stern wrote:
> > > > > This patch adds a "gadget" bus and uses it for registering gadgets and
> > > > > their drivers.  From now on, bindings will be managed by the driver
> > > > > core rather than through ad-hoc manipulations in the UDC core.
> > > > >
> > > > > As part of this change, the driver_pending_list is removed.  The UDC
> > > > > core won't need to keep track of unbound drivers for later binding,
> > > > > because the driver core handles all of that for us.
> > > > >
> > > > > However, we do need one new feature: a way to prevent gadget drivers
> > > > > from being bound to more than one gadget at a time.  The existing code
> > > > > does this automatically, but the driver core doesn't -- it's perfectly
> > > > > happy to bind a single driver to all the matching devices on the bus.
> > > > > The patch adds a new bitflag to the usb_gadget_driver structure for
> > > > > this purpose.
> > > > >
> > > > > A nice side effect of this change is a reduction in the total lines of
> > > > > code, since now the driver core will do part of the work that the UDC
> > > > > used to do.
> > > > >
> > > > > A possible future patch could add udc devices to the gadget bus, say
> > > > > as a separate device type.
> > > > >
> > > > > Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> > > >
> > > > Thanks for your patch, which is now commit fc274c1e997314bf ("USB:
> > > > gadget: Add a new bus for gadgets") in usb-next.
> > > >
> > > > This patch cause a regression on the Renesas Salvator-XS development
> > > > board, as R-Car H3 has multiple USB gadget devices:
> > >
> > > Then these gadgets ought to have distinct names in order to avoid the
> > > conflict below:
> > >
> > > >     sysfs: cannot create duplicate filename '/bus/gadget/devices/gadget'
> > > >     CPU: 2 PID: 1 Comm: swapper/0 Not tainted 5.18.0-rc1-arm64-renesas-00074-gfc274c1e9973 #1587
> > > >     Hardware name: Renesas Salvator-X 2nd version board based on r8a77951 (DT)
> > > >     Call trace:
> > > >      dump_backtrace+0xcc/0xd8
> > > >      show_stack+0x14/0x30
> > > >      dump_stack_lvl+0x88/0xb0
> > > >      dump_stack+0x14/0x2c
> > > >      sysfs_warn_dup+0x60/0x78
> > > >      sysfs_do_create_link_sd.isra.0+0xe4/0xf0
> > > >      sysfs_create_link+0x20/0x40
> > > >      bus_add_device+0x64/0x110
> > > >      device_add+0x31c/0x850
> > > >      usb_add_gadget+0x124/0x1a0
> > > >      usb_add_gadget_udc_release+0x1c/0x50
> > > >      usb_add_gadget_udc+0x10/0x18
> > > >      renesas_usb3_probe+0x450/0x728
> > > ...
> > >
> > > Having three gadget devices, all named "gadget", doesn't seem like a
> > > good idea.
> > 
> > I'm not so sure where these names are coming from.
> > `git grep '"gadget"'` points to the following likely targets:
> > 
> > drivers/usb/gadget/udc/core.c:  dev_set_name(&gadget->dev, "gadget");
> > drivers/usb/renesas_usbhs/mod_gadget.c: gpriv->mod.name         = "gadget";
> > 
> > Changing both names reveals the problem is actually caused by
> > the former ;-)
> 
> Ah, good.
> 
> One way to attack this would be to keep a static counter and dynamically 
> set the name to "gadget.%d" using the counter's value.  Or keep a bitmap 
> of allocated gadget numbers and use the first available number.
> 
> Felipe, Greg, any opinions?

Just use an idr structure for the number, that's the simplest way to
track that.

thanks,

greg k-h

Re: [PATCH 4/4] USB: gadget: Add a new bus for gadgets

[Alan Stern]

On Tue, May 03, 2022 at 11:48:33AM -0400, Alan Stern wrote:
> On Tue, May 03, 2022 at 05:27:08PM +0200, Geert Uytterhoeven wrote:
> > Hi Alan,
> > 
> > On Tue, May 3, 2022 at 5:14 PM Alan Stern <stern@rowland.harvard.edu> wrote:
> > > On Tue, May 03, 2022 at 12:14:30PM +0200, Geert Uytterhoeven wrote:
> > > > On Sat, 23 Apr 2022, Alan Stern wrote:
> > > > > This patch adds a "gadget" bus and uses it for registering gadgets and
> > > > > their drivers.  From now on, bindings will be managed by the driver
> > > > > core rather than through ad-hoc manipulations in the UDC core.
> > > > >
> > > > > As part of this change, the driver_pending_list is removed.  The UDC
> > > > > core won't need to keep track of unbound drivers for later binding,
> > > > > because the driver core handles all of that for us.
> > > > >
> > > > > However, we do need one new feature: a way to prevent gadget drivers
> > > > > from being bound to more than one gadget at a time.  The existing code
> > > > > does this automatically, but the driver core doesn't -- it's perfectly
> > > > > happy to bind a single driver to all the matching devices on the bus.
> > > > > The patch adds a new bitflag to the usb_gadget_driver structure for
> > > > > this purpose.
> > > > >
> > > > > A nice side effect of this change is a reduction in the total lines of
> > > > > code, since now the driver core will do part of the work that the UDC
> > > > > used to do.
> > > > >
> > > > > A possible future patch could add udc devices to the gadget bus, say
> > > > > as a separate device type.
> > > > >
> > > > > Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> > > >
> > > > Thanks for your patch, which is now commit fc274c1e997314bf ("USB:
> > > > gadget: Add a new bus for gadgets") in usb-next.
> > > >
> > > > This patch cause a regression on the Renesas Salvator-XS development
> > > > board, as R-Car H3 has multiple USB gadget devices:
> > >
> > > Then these gadgets ought to have distinct names in order to avoid the
> > > conflict below:

Geert:

Can you test the patch below?  It ought to fix the problem (although it 
might end up causing other problems down the line...)

Alan Stern


Index: usb-devel/drivers/usb/gadget/udc/core.c
===================================================================
--- usb-devel.orig/drivers/usb/gadget/udc/core.c
+++ usb-devel/drivers/usb/gadget/udc/core.c
@@ -12,6 +12,7 @@
 #include <linux/module.h>
 #include <linux/device.h>
 #include <linux/list.h>
+#include <linux/idr.h>
 #include <linux/err.h>
 #include <linux/dma-mapping.h>
 #include <linux/sched/task_stack.h>
@@ -23,6 +24,8 @@
 
 #include "trace.h"
 
+static DEFINE_IDA(gadget_id_numbers);
+
 static struct bus_type gadget_bus_type;
 
 /**
@@ -1248,7 +1251,6 @@ static void usb_udc_nop_release(struct d
 void usb_initialize_gadget(struct device *parent, struct usb_gadget *gadget,
 		void (*release)(struct device *dev))
 {
-	dev_set_name(&gadget->dev, "gadget");
 	INIT_WORK(&gadget->work, usb_gadget_state_work);
 	gadget->dev.parent = parent;
 
@@ -1304,12 +1306,21 @@ int usb_add_gadget(struct usb_gadget *ga
 	usb_gadget_set_state(gadget, USB_STATE_NOTATTACHED);
 	udc->vbus = true;
 
+	ret = ida_alloc(&gadget_id_numbers, GFP_KERNEL);
+	if (ret < 0)
+		goto err_del_udc;
+	gadget->id_number = ret;
+	dev_set_name(&gadget->dev, "gadget.%d", ret);
+
 	ret = device_add(&gadget->dev);
 	if (ret)
-		goto err_del_udc;
+		goto err_free_id;
 
 	return 0;
 
+ err_free_id:
+	ida_free(&gadget_id_numbers, gadget->id_number);
+
  err_del_udc:
 	flush_work(&gadget->work);
 	device_del(&udc->dev);
@@ -1417,6 +1428,7 @@ void usb_del_gadget(struct usb_gadget *g
 	kobject_uevent(&udc->dev.kobj, KOBJ_REMOVE);
 	flush_work(&gadget->work);
 	device_del(&gadget->dev);
+	ida_free(&gadget_id_numbers, gadget->id_number);
 	device_unregister(&udc->dev);
 }
 EXPORT_SYMBOL_GPL(usb_del_gadget);
Index: usb-devel/include/linux/usb/gadget.h
===================================================================
--- usb-devel.orig/include/linux/usb/gadget.h
+++ usb-devel/include/linux/usb/gadget.h
@@ -386,6 +386,7 @@ struct usb_gadget_ops {
  * @lpm_capable: If the gadget max_speed is FULL or HIGH, this flag
  *	indicates that it supports LPM as per the LPM ECN & errata.
  * @irq: the interrupt number for device controller.
+ * @id_number: a unique ID number for ensuring that gadget names are distinct
  *
  * Gadgets have a mostly-portable "gadget driver" implementing device
  * functions, handling all usb configurations and interfaces.  Gadget
@@ -446,6 +447,7 @@ struct usb_gadget {
 	unsigned			connected:1;
 	unsigned			lpm_capable:1;
 	int				irq;
+	int				id_number;
 };
 #define work_to_gadget(w)	(container_of((w), struct usb_gadget, work))
 

Re: [PATCH 4/4] USB: gadget: Add a new bus for gadgets

[Geert Uytterhoeven]

Hi Alan,

On Sat, May 7, 2022 at 5:36 PM Alan Stern <stern@rowland.harvard.edu> wrote:
> On Tue, May 03, 2022 at 11:48:33AM -0400, Alan Stern wrote:
> > On Tue, May 03, 2022 at 05:27:08PM +0200, Geert Uytterhoeven wrote:
> > > On Tue, May 3, 2022 at 5:14 PM Alan Stern <stern@rowland.harvard.edu> wrote:
> > > > On Tue, May 03, 2022 at 12:14:30PM +0200, Geert Uytterhoeven wrote:
> > > > > On Sat, 23 Apr 2022, Alan Stern wrote:
> > > > > > This patch adds a "gadget" bus and uses it for registering gadgets and
> > > > > > their drivers.  From now on, bindings will be managed by the driver
> > > > > > core rather than through ad-hoc manipulations in the UDC core.
> > > > > >
> > > > > > As part of this change, the driver_pending_list is removed.  The UDC
> > > > > > core won't need to keep track of unbound drivers for later binding,
> > > > > > because the driver core handles all of that for us.
> > > > > >
> > > > > > However, we do need one new feature: a way to prevent gadget drivers
> > > > > > from being bound to more than one gadget at a time.  The existing code
> > > > > > does this automatically, but the driver core doesn't -- it's perfectly
> > > > > > happy to bind a single driver to all the matching devices on the bus.
> > > > > > The patch adds a new bitflag to the usb_gadget_driver structure for
> > > > > > this purpose.
> > > > > >
> > > > > > A nice side effect of this change is a reduction in the total lines of
> > > > > > code, since now the driver core will do part of the work that the UDC
> > > > > > used to do.
> > > > > >
> > > > > > A possible future patch could add udc devices to the gadget bus, say
> > > > > > as a separate device type.
> > > > > >
> > > > > > Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> > > > >
> > > > > Thanks for your patch, which is now commit fc274c1e997314bf ("USB:
> > > > > gadget: Add a new bus for gadgets") in usb-next.
> > > > >
> > > > > This patch cause a regression on the Renesas Salvator-XS development
> > > > > board, as R-Car H3 has multiple USB gadget devices:
> > > >
> > > > Then these gadgets ought to have distinct names in order to avoid the
> > > > conflict below:
>
> Geert:
>
> Can you test the patch below?  It ought to fix the problem (although it

Thanks!

root@h3-salvator-xs:~# ls -l /sys/bus/gadget/devices/
total 0
lrwxrwxrwx 1 root root 0 Feb 14  2019 gadget.0 ->
../../../devices/platform/soc/e659c000.usb/gadget.0
lrwxrwxrwx 1 root root 0 Feb 14  2019 gadget.1 ->
../../../devices/platform/soc/ee020000.usb/gadget.1
lrwxrwxrwx 1 root root 0 Feb 14  2019 gadget.2 ->
../../../devices/platform/soc/e6590000.usb/gadget.2

Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>

LGTM, so
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

> might end up causing other problems down the line...)

Can you please elaborate? I'm not too familiar with UBS gadgets.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 4/4] USB: gadget: Add a new bus for gadgets

[Alan Stern]

On Mon, May 09, 2022 at 09:46:25AM +0200, Geert Uytterhoeven wrote:
> > Geert:
> >
> > Can you test the patch below?  It ought to fix the problem (although it
> 
> Thanks!
> 
> root@h3-salvator-xs:~# ls -l /sys/bus/gadget/devices/
> total 0
> lrwxrwxrwx 1 root root 0 Feb 14  2019 gadget.0 ->
> ../../../devices/platform/soc/e659c000.usb/gadget.0
> lrwxrwxrwx 1 root root 0 Feb 14  2019 gadget.1 ->
> ../../../devices/platform/soc/ee020000.usb/gadget.1
> lrwxrwxrwx 1 root root 0 Feb 14  2019 gadget.2 ->
> ../../../devices/platform/soc/e6590000.usb/gadget.2
> 
> Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
> 
> LGTM, so
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Thanks!

> > might end up causing other problems down the line...)
> 
> Can you please elaborate? I'm not too familiar with UBS gadgets.

I was concerned about the fact that changing the name of a file, 
directory, or symbolic link in sysfs means changing a user API, and so 
it might cause some existing programs to fail.  That would be a 
regression.

Perhaps the best way to work around the problem is to leave the name set 
to "gadget" if the ID number is 0, while adding the ID number on to the 
name if the value is > 0.  What do you think?

Alan Stern

Re: [PATCH 4/4] USB: gadget: Add a new bus for gadgets

[Geert Uytterhoeven]

Hi Alan,

On Mon, May 9, 2022 at 4:15 PM Alan Stern <stern@rowland.harvard.edu> wrote:
> On Mon, May 09, 2022 at 09:46:25AM +0200, Geert Uytterhoeven wrote:
> > > Geert:
> > >
> > > Can you test the patch below?  It ought to fix the problem (although it
> >
> > Thanks!
> >
> > root@h3-salvator-xs:~# ls -l /sys/bus/gadget/devices/
> > total 0
> > lrwxrwxrwx 1 root root 0 Feb 14  2019 gadget.0 ->
> > ../../../devices/platform/soc/e659c000.usb/gadget.0
> > lrwxrwxrwx 1 root root 0 Feb 14  2019 gadget.1 ->
> > ../../../devices/platform/soc/ee020000.usb/gadget.1
> > lrwxrwxrwx 1 root root 0 Feb 14  2019 gadget.2 ->
> > ../../../devices/platform/soc/e6590000.usb/gadget.2
> >
> > Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
> >
> > LGTM, so
> > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
>
> Thanks!
>
> > > might end up causing other problems down the line...)
> >
> > Can you please elaborate? I'm not too familiar with UBS gadgets.
>
> I was concerned about the fact that changing the name of a file,
> directory, or symbolic link in sysfs means changing a user API, and so
> it might cause some existing programs to fail.  That would be a
> regression.
>
> Perhaps the best way to work around the problem is to leave the name set
> to "gadget" if the ID number is 0, while adding the ID number on to the
> name if the value is > 0.  What do you think?

Oh, you mean the "gadget.N" subdirs, which are the targets of the
symlinks above? These were indeed named "gadget" before.

Would it be possible to append the ".N" suffixes only to the actual
symlinks, while keeping the target directory names unchanged?
E.g. /sys/bus/gadget/devices/gadget.0 ->
../../../devices/platform/soc/e659c000.usb/gadget

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 4/4] USB: gadget: Add a new bus for gadgets

[Alan Stern]

On Mon, May 09, 2022 at 04:42:01PM +0200, Geert Uytterhoeven wrote:
> Hi Alan,
> 
> On Mon, May 9, 2022 at 4:15 PM Alan Stern <stern@rowland.harvard.edu> wrote:
> > On Mon, May 09, 2022 at 09:46:25AM +0200, Geert Uytterhoeven wrote:
> > > > Geert:
> > > >
> > > > Can you test the patch below?  It ought to fix the problem (although it
> > >
> > > Thanks!
> > >
> > > root@h3-salvator-xs:~# ls -l /sys/bus/gadget/devices/
> > > total 0
> > > lrwxrwxrwx 1 root root 0 Feb 14  2019 gadget.0 ->
> > > ../../../devices/platform/soc/e659c000.usb/gadget.0
> > > lrwxrwxrwx 1 root root 0 Feb 14  2019 gadget.1 ->
> > > ../../../devices/platform/soc/ee020000.usb/gadget.1
> > > lrwxrwxrwx 1 root root 0 Feb 14  2019 gadget.2 ->
> > > ../../../devices/platform/soc/e6590000.usb/gadget.2
> > >
> > > Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > >
> > > LGTM, so
> > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> >
> > Thanks!
> >
> > > > might end up causing other problems down the line...)
> > >
> > > Can you please elaborate? I'm not too familiar with UBS gadgets.
> >
> > I was concerned about the fact that changing the name of a file,
> > directory, or symbolic link in sysfs means changing a user API, and so
> > it might cause some existing programs to fail.  That would be a
> > regression.
> >
> > Perhaps the best way to work around the problem is to leave the name set
> > to "gadget" if the ID number is 0, while adding the ID number on to the
> > name if the value is > 0.  What do you think?
> 
> Oh, you mean the "gadget.N" subdirs, which are the targets of the
> symlinks above? These were indeed named "gadget" before.
> 
> Would it be possible to append the ".N" suffixes only to the actual
> symlinks, while keeping the target directory names unchanged?
> E.g. /sys/bus/gadget/devices/gadget.0 ->
> ../../../devices/platform/soc/e659c000.usb/gadget

No, it's not possible.  Or at least, not without significant changes to 
the driver core.  Besides, people expect these names to be the same.

Alan Stern

Re: [PATCH 4/4] USB: gadget: Add a new bus for gadgets

[Greg KH]

On Mon, May 09, 2022 at 11:05:06AM -0400, Alan Stern wrote:
> On Mon, May 09, 2022 at 04:42:01PM +0200, Geert Uytterhoeven wrote:
> > Hi Alan,
> > 
> > On Mon, May 9, 2022 at 4:15 PM Alan Stern <stern@rowland.harvard.edu> wrote:
> > > On Mon, May 09, 2022 at 09:46:25AM +0200, Geert Uytterhoeven wrote:
> > > > > Geert:
> > > > >
> > > > > Can you test the patch below?  It ought to fix the problem (although it
> > > >
> > > > Thanks!
> > > >
> > > > root@h3-salvator-xs:~# ls -l /sys/bus/gadget/devices/
> > > > total 0
> > > > lrwxrwxrwx 1 root root 0 Feb 14  2019 gadget.0 ->
> > > > ../../../devices/platform/soc/e659c000.usb/gadget.0
> > > > lrwxrwxrwx 1 root root 0 Feb 14  2019 gadget.1 ->
> > > > ../../../devices/platform/soc/ee020000.usb/gadget.1
> > > > lrwxrwxrwx 1 root root 0 Feb 14  2019 gadget.2 ->
> > > > ../../../devices/platform/soc/e6590000.usb/gadget.2
> > > >
> > > > Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > >
> > > > LGTM, so
> > > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > >
> > > Thanks!
> > >
> > > > > might end up causing other problems down the line...)
> > > >
> > > > Can you please elaborate? I'm not too familiar with UBS gadgets.
> > >
> > > I was concerned about the fact that changing the name of a file,
> > > directory, or symbolic link in sysfs means changing a user API, and so
> > > it might cause some existing programs to fail.  That would be a
> > > regression.
> > >
> > > Perhaps the best way to work around the problem is to leave the name set
> > > to "gadget" if the ID number is 0, while adding the ID number on to the
> > > name if the value is > 0.  What do you think?
> > 
> > Oh, you mean the "gadget.N" subdirs, which are the targets of the
> > symlinks above? These were indeed named "gadget" before.
> > 
> > Would it be possible to append the ".N" suffixes only to the actual
> > symlinks, while keeping the target directory names unchanged?
> > E.g. /sys/bus/gadget/devices/gadget.0 ->
> > ../../../devices/platform/soc/e659c000.usb/gadget
> 
> No, it's not possible.  Or at least, not without significant changes to 
> the driver core.  Besides, people expect these names to be the same.

It should always be ok to change the names of devices as those are not
going to be persistent / determinisitic.  It's the attributes of devices
that are supposed to be used to determine those types of things.

So I think let's start out with the .N suffix for everything for now.
I'll be glad to submit the fixed patch to the Android kernel build
system to see what their testing comes back with to see if they happened
to make any name assumptions as that's the largest user of this
codebase.

thanks,

greg k-h

Re: [PATCH 4/4] USB: gadget: Add a new bus for gadgets

[Alan Stern]

On Mon, May 09, 2022 at 06:23:31PM +0200, Greg KH wrote:
> On Mon, May 09, 2022 at 11:05:06AM -0400, Alan Stern wrote:
> > On Mon, May 09, 2022 at 04:42:01PM +0200, Geert Uytterhoeven wrote:
> > > Hi Alan,
> > > 
> > > On Mon, May 9, 2022 at 4:15 PM Alan Stern <stern@rowland.harvard.edu> wrote:
> > > > On Mon, May 09, 2022 at 09:46:25AM +0200, Geert Uytterhoeven wrote:
> > > > > > Geert:
> > > > > >
> > > > > > Can you test the patch below?  It ought to fix the problem (although it
> > > > >
> > > > > Thanks!
> > > > >
> > > > > root@h3-salvator-xs:~# ls -l /sys/bus/gadget/devices/
> > > > > total 0
> > > > > lrwxrwxrwx 1 root root 0 Feb 14  2019 gadget.0 ->
> > > > > ../../../devices/platform/soc/e659c000.usb/gadget.0
> > > > > lrwxrwxrwx 1 root root 0 Feb 14  2019 gadget.1 ->
> > > > > ../../../devices/platform/soc/ee020000.usb/gadget.1
> > > > > lrwxrwxrwx 1 root root 0 Feb 14  2019 gadget.2 ->
> > > > > ../../../devices/platform/soc/e6590000.usb/gadget.2
> > > > >
> > > > > Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > > >
> > > > > LGTM, so
> > > > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > >
> > > > Thanks!
> > > >
> > > > > > might end up causing other problems down the line...)
> > > > >
> > > > > Can you please elaborate? I'm not too familiar with UBS gadgets.
> > > >
> > > > I was concerned about the fact that changing the name of a file,
> > > > directory, or symbolic link in sysfs means changing a user API, and so
> > > > it might cause some existing programs to fail.  That would be a
> > > > regression.
> > > >
> > > > Perhaps the best way to work around the problem is to leave the name set
> > > > to "gadget" if the ID number is 0, while adding the ID number on to the
> > > > name if the value is > 0.  What do you think?
> > > 
> > > Oh, you mean the "gadget.N" subdirs, which are the targets of the
> > > symlinks above? These were indeed named "gadget" before.
> > > 
> > > Would it be possible to append the ".N" suffixes only to the actual
> > > symlinks, while keeping the target directory names unchanged?
> > > E.g. /sys/bus/gadget/devices/gadget.0 ->
> > > ../../../devices/platform/soc/e659c000.usb/gadget
> > 
> > No, it's not possible.  Or at least, not without significant changes to 
> > the driver core.  Besides, people expect these names to be the same.
> 
> It should always be ok to change the names of devices as those are not
> going to be persistent / determinisitic.  It's the attributes of devices
> that are supposed to be used to determine those types of things.
> 
> So I think let's start out with the .N suffix for everything for now.
> I'll be glad to submit the fixed patch to the Android kernel build
> system to see what their testing comes back with to see if they happened
> to make any name assumptions as that's the largest user of this
> codebase.

Okay.  Do you need me to send it as a proper patch before you try it 
out?

Alan Stern

Re: [PATCH 4/4] USB: gadget: Add a new bus for gadgets

[Greg KH]

On Mon, May 09, 2022 at 12:47:19PM -0400, Alan Stern wrote:
> On Mon, May 09, 2022 at 06:23:31PM +0200, Greg KH wrote:
> > On Mon, May 09, 2022 at 11:05:06AM -0400, Alan Stern wrote:
> > > On Mon, May 09, 2022 at 04:42:01PM +0200, Geert Uytterhoeven wrote:
> > > > Hi Alan,
> > > > 
> > > > On Mon, May 9, 2022 at 4:15 PM Alan Stern <stern@rowland.harvard.edu> wrote:
> > > > > On Mon, May 09, 2022 at 09:46:25AM +0200, Geert Uytterhoeven wrote:
> > > > > > > Geert:
> > > > > > >
> > > > > > > Can you test the patch below?  It ought to fix the problem (although it
> > > > > >
> > > > > > Thanks!
> > > > > >
> > > > > > root@h3-salvator-xs:~# ls -l /sys/bus/gadget/devices/
> > > > > > total 0
> > > > > > lrwxrwxrwx 1 root root 0 Feb 14  2019 gadget.0 ->
> > > > > > ../../../devices/platform/soc/e659c000.usb/gadget.0
> > > > > > lrwxrwxrwx 1 root root 0 Feb 14  2019 gadget.1 ->
> > > > > > ../../../devices/platform/soc/ee020000.usb/gadget.1
> > > > > > lrwxrwxrwx 1 root root 0 Feb 14  2019 gadget.2 ->
> > > > > > ../../../devices/platform/soc/e6590000.usb/gadget.2
> > > > > >
> > > > > > Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > > > >
> > > > > > LGTM, so
> > > > > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > > >
> > > > > Thanks!
> > > > >
> > > > > > > might end up causing other problems down the line...)
> > > > > >
> > > > > > Can you please elaborate? I'm not too familiar with UBS gadgets.
> > > > >
> > > > > I was concerned about the fact that changing the name of a file,
> > > > > directory, or symbolic link in sysfs means changing a user API, and so
> > > > > it might cause some existing programs to fail.  That would be a
> > > > > regression.
> > > > >
> > > > > Perhaps the best way to work around the problem is to leave the name set
> > > > > to "gadget" if the ID number is 0, while adding the ID number on to the
> > > > > name if the value is > 0.  What do you think?
> > > > 
> > > > Oh, you mean the "gadget.N" subdirs, which are the targets of the
> > > > symlinks above? These were indeed named "gadget" before.
> > > > 
> > > > Would it be possible to append the ".N" suffixes only to the actual
> > > > symlinks, while keeping the target directory names unchanged?
> > > > E.g. /sys/bus/gadget/devices/gadget.0 ->
> > > > ../../../devices/platform/soc/e659c000.usb/gadget
> > > 
> > > No, it's not possible.  Or at least, not without significant changes to 
> > > the driver core.  Besides, people expect these names to be the same.
> > 
> > It should always be ok to change the names of devices as those are not
> > going to be persistent / determinisitic.  It's the attributes of devices
> > that are supposed to be used to determine those types of things.
> > 
> > So I think let's start out with the .N suffix for everything for now.
> > I'll be glad to submit the fixed patch to the Android kernel build
> > system to see what their testing comes back with to see if they happened
> > to make any name assumptions as that's the largest user of this
> > codebase.
> 
> Okay.  Do you need me to send it as a proper patch before you try it 
> out?

Yes please.

thanks,

greg k-h

[PATCH] USB: gadget: Add ID numbers to gadget names

[Alan Stern]

Putting USB gadgets on a new bus of their own encounters a problem
when multiple gadgets are present: They all have the same name!  The
driver core fails with a "sys: cannot create duplicate filename" error
when creating any of the /sys/bus/gadget/devices/<gadget-name>
symbolic links after the first.

This patch fixes the problem by adding a ".N" suffix to each gadget's
name when the gadget is registered (where N is a unique ID number),
thus making the names distinct.

Reported-and-tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Fixes: fc274c1e9973 ("USB: gadget: Add a new bus for gadgets")

---


[as1980]


 drivers/usb/gadget/udc/core.c |   16 ++++++++++++++--
 include/linux/usb/gadget.h    |    2 ++
 2 files changed, 16 insertions(+), 2 deletions(-)

Index: usb-devel/drivers/usb/gadget/udc/core.c
===================================================================
--- usb-devel.orig/drivers/usb/gadget/udc/core.c
+++ usb-devel/drivers/usb/gadget/udc/core.c
@@ -12,6 +12,7 @@
 #include <linux/module.h>
 #include <linux/device.h>
 #include <linux/list.h>
+#include <linux/idr.h>
 #include <linux/err.h>
 #include <linux/dma-mapping.h>
 #include <linux/sched/task_stack.h>
@@ -23,6 +24,8 @@
 
 #include "trace.h"
 
+static DEFINE_IDA(gadget_id_numbers);
+
 static struct bus_type gadget_bus_type;
 
 /**
@@ -1248,7 +1251,6 @@ static void usb_udc_nop_release(struct d
 void usb_initialize_gadget(struct device *parent, struct usb_gadget *gadget,
 		void (*release)(struct device *dev))
 {
-	dev_set_name(&gadget->dev, "gadget");
 	INIT_WORK(&gadget->work, usb_gadget_state_work);
 	gadget->dev.parent = parent;
 
@@ -1304,12 +1306,21 @@ int usb_add_gadget(struct usb_gadget *ga
 	usb_gadget_set_state(gadget, USB_STATE_NOTATTACHED);
 	udc->vbus = true;
 
+	ret = ida_alloc(&gadget_id_numbers, GFP_KERNEL);
+	if (ret < 0)
+		goto err_del_udc;
+	gadget->id_number = ret;
+	dev_set_name(&gadget->dev, "gadget.%d", ret);
+
 	ret = device_add(&gadget->dev);
 	if (ret)
-		goto err_del_udc;
+		goto err_free_id;
 
 	return 0;
 
+ err_free_id:
+	ida_free(&gadget_id_numbers, gadget->id_number);
+
  err_del_udc:
 	flush_work(&gadget->work);
 	device_del(&udc->dev);
@@ -1417,6 +1428,7 @@ void usb_del_gadget(struct usb_gadget *g
 	kobject_uevent(&udc->dev.kobj, KOBJ_REMOVE);
 	flush_work(&gadget->work);
 	device_del(&gadget->dev);
+	ida_free(&gadget_id_numbers, gadget->id_number);
 	device_unregister(&udc->dev);
 }
 EXPORT_SYMBOL_GPL(usb_del_gadget);
Index: usb-devel/include/linux/usb/gadget.h
===================================================================
--- usb-devel.orig/include/linux/usb/gadget.h
+++ usb-devel/include/linux/usb/gadget.h
@@ -386,6 +386,7 @@ struct usb_gadget_ops {
  * @lpm_capable: If the gadget max_speed is FULL or HIGH, this flag
  *	indicates that it supports LPM as per the LPM ECN & errata.
  * @irq: the interrupt number for device controller.
+ * @id_number: a unique ID number for ensuring that gadget names are distinct
  *
  * Gadgets have a mostly-portable "gadget driver" implementing device
  * functions, handling all usb configurations and interfaces.  Gadget
@@ -446,6 +447,7 @@ struct usb_gadget {
 	unsigned			connected:1;
 	unsigned			lpm_capable:1;
 	int				irq;
+	int				id_number;
 };
 #define work_to_gadget(w)	(container_of((w), struct usb_gadget, work))
 

Re: [PATCH] USB: gadget: Add ID numbers to gadget names

[Greg KH]

On Tue, May 10, 2022 at 11:51:29AM -0400, Alan Stern wrote:
> Putting USB gadgets on a new bus of their own encounters a problem
> when multiple gadgets are present: They all have the same name!  The
> driver core fails with a "sys: cannot create duplicate filename" error
> when creating any of the /sys/bus/gadget/devices/<gadget-name>
> symbolic links after the first.
> 
> This patch fixes the problem by adding a ".N" suffix to each gadget's
> name when the gadget is registered (where N is a unique ID number),
> thus making the names distinct.
> 
> Reported-and-tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Fixes: fc274c1e9973 ("USB: gadget: Add a new bus for gadgets")
> 
> ---
> 
> 
> [as1980]

Thanks, let me run this through the Android test suite first.  You can
see the results here:
	https://android-review.googlesource.com/c/kernel/common/+/2095109

I'll let you know how it goes as I do not know if you are not logged in
if you can see the test results or not, gerrit is odd...

thanks,

greg k-h

Re: [PATCH] USB: gadget: Add ID numbers to gadget names

[Greg KH]

On Wed, May 11, 2022 at 05:17:48PM +0200, Greg KH wrote:
> On Tue, May 10, 2022 at 11:51:29AM -0400, Alan Stern wrote:
> > Putting USB gadgets on a new bus of their own encounters a problem
> > when multiple gadgets are present: They all have the same name!  The
> > driver core fails with a "sys: cannot create duplicate filename" error
> > when creating any of the /sys/bus/gadget/devices/<gadget-name>
> > symbolic links after the first.
> > 
> > This patch fixes the problem by adding a ".N" suffix to each gadget's
> > name when the gadget is registered (where N is a unique ID number),
> > thus making the names distinct.
> > 
> > Reported-and-tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > Fixes: fc274c1e9973 ("USB: gadget: Add a new bus for gadgets")
> > 
> > ---
> > 
> > 
> > [as1980]
> 
> Thanks, let me run this through the Android test suite first.  You can
> see the results here:
> 	https://android-review.googlesource.com/c/kernel/common/+/2095109
> 
> I'll let you know how it goes as I do not know if you are not logged in
> if you can see the test results or not, gerrit is odd...

All passed, so I'll go queue this up now, thanks!

greg k-h

Re: [RFC PATCH 0/4] USB: gadget: Create a bus for gadgets

[Alan Stern]

On Fri, Apr 22, 2022 at 03:30:06PM +0200, Greg KH wrote:
> On Sun, Mar 20, 2022 at 03:45:48PM -0400, Alan Stern wrote:
> > Everyone:
> > 
> > The following series of patches implements Greg's suggestion that 
> > gadgets should be registered on some sort of bus.  It turns out that the 
> > best way to do this is to create a new "gadget" bus, with specialized 
> > matching and probing routines, rather than using an existing bus.
> > 
> > Patches 1-3 are simple preparations for the big change.  They stand on 
> > their own, make useful little changes, and could be merged by themselves
> > without committing to adding the "gadget" bus.  Patch 4 is main one.
> > 
> > I'm posting this series for feedback from the Gadget/UDC maintainer and 
> > others.  If everything works out okay, the patches can be submitted for 
> > real once the upcoming merge window closes.
> 
> At first glance, this looks good to me, many thanks for working on this!

Okay, patches 1-4 have now been submitted officially.

Alan Stern