[PATCH v5 0/5] usb/gadget: independent registration of gadgets and gadget drivers

[PATCH v5 0/5] usb/gadget: independent registration of gadgets and gadget drivers

[Ruslan Bilovol]

This patchset adds independent registration of gadgets
and gadget drivers to udc-core. This is very useful for
built-in modules into kernel case since it's possible
situation that gadget driver is probing at a time
when no gadgets are registered in udc-core.
In this case instead of silently failing without
of any attempt to recover, with independent registration
of gadgets and gadget drivers there is no matter
in which order gadgets and gadget drivers are
probed/registered.

This patch has side-effect on gadget drivers that had
__init/__exit attributes on some paths like bind/unbind
and (since bind/unbind may happen at any time) should
not use them now. This is covered by forth patch

=====================
v5:
 - this set of patches (that I already forgot about) has been
   successfully tested by Maxime Ripard, who sent me confirmation via
   personal email, so his Tested-by tag is added to the patches
 - rebased onto latest 'next' branch of Felipe Balbi's tree

v4:
 - misc fixes - addressed Alan's and Sergei's comments
 - rebased onto latest 'next' branch of Felipe Balbi's tree
 
v3:
 - addressed Alan's comments - now UDC name and pending
   gadget drivers list is a part of struct usb_gadget_driver.
 - removed usb_udc_attach_driver() function that became unused
   and not needed now

v2:
 - changed first patch to have only deferred probe part
   (because Gadget Bus seems to be better variant when
   some more complicated behavior will be required)
 - rebased to latest 'next' branch of Felipe Balbi's tree

Ruslan Bilovol (5):
  usb: gadget: bind UDC by name passed via usb_gadget_driver structure
  usb: gadget: configfs: pass UDC name via usb_gadget_driver struct
  usb: gadget: udc-core: remove unused usb_udc_attach_driver()
  usb: gadget: legacy: don't use __init/__exit attributes for
    bind/unbind path
  usb: gadget: udc-core: independent registration of gadgets and gadget
    drivers

 drivers/usb/gadget/configfs.c            | 27 +++++-----
 drivers/usb/gadget/legacy/acm_ms.c       |  6 +--
 drivers/usb/gadget/legacy/audio.c        |  6 +--
 drivers/usb/gadget/legacy/cdc2.c         |  6 +--
 drivers/usb/gadget/legacy/dbgp.c         |  2 +-
 drivers/usb/gadget/legacy/ether.c        |  8 +--
 drivers/usb/gadget/legacy/gmidi.c        |  6 +--
 drivers/usb/gadget/legacy/hid.c          |  6 +--
 drivers/usb/gadget/legacy/mass_storage.c |  4 +-
 drivers/usb/gadget/legacy/multi.c        | 16 +++---
 drivers/usb/gadget/legacy/ncm.c          |  6 +--
 drivers/usb/gadget/legacy/nokia.c        |  6 +--
 drivers/usb/gadget/legacy/printer.c      |  6 +--
 drivers/usb/gadget/legacy/serial.c       |  2 +-
 drivers/usb/gadget/legacy/webcam.c       |  4 +-
 drivers/usb/gadget/legacy/zero.c         |  2 +-
 drivers/usb/gadget/udc/udc-core.c        | 87 ++++++++++++++++++++------------
 include/linux/usb/gadget.h               |  8 ++-
 18 files changed, 117 insertions(+), 91 deletions(-)

-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v5 1/5] usb: gadget: bind UDC by name passed via usb_gadget_driver structure

[Ruslan Bilovol]

Introduce new 'udc_name' member to usb_gadget_driver structure.
The 'udc_name' is a name of UDC that usb_gadget_driver should
be bound to. If udc_name is NULL, it will be bound to any
available UDC.

Tested-by: Maxime Ripard <maxime.ripard@free-electrons.com>
Signed-off-by: Ruslan Bilovol <ruslan.bilovol@gmail.com>
---
 drivers/usb/gadget/udc/udc-core.c | 24 +++++++++++++++++++-----
 include/linux/usb/gadget.h        |  4 ++++
 2 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/gadget/udc/udc-core.c b/drivers/usb/gadget/udc/udc-core.c
index d69c355..b431129 100644
--- a/drivers/usb/gadget/udc/udc-core.c
+++ b/drivers/usb/gadget/udc/udc-core.c
@@ -456,21 +456,35 @@ EXPORT_SYMBOL_GPL(usb_udc_attach_driver);
 int usb_gadget_probe_driver(struct usb_gadget_driver *driver)
 {
 	struct usb_udc		*udc = NULL;
-	int			ret;
+	int			ret = -ENODEV;
 
 	if (!driver || !driver->bind || !driver->setup)
 		return -EINVAL;
 
 	mutex_lock(&udc_lock);
-	list_for_each_entry(udc, &udc_list, list) {
-		/* For now we take the first one */
-		if (!udc->driver)
+	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)
+			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;
+		}
 	}
 
 	pr_debug("couldn't find an available UDC\n");
 	mutex_unlock(&udc_lock);
-	return -ENODEV;
+	return ret;
 found:
 	ret = udc_bind_to_driver(udc, driver);
 	mutex_unlock(&udc_lock);
diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index 4f3dfb7..8bba379 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -824,6 +824,8 @@ static inline int usb_gadget_disconnect(struct usb_gadget *gadget)
  * @reset: Invoked on USB bus reset. It is mandatory for all gadget drivers
  *	and should be called in_interrupt.
  * @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.
  *
  * Devices are disabled till a gadget driver successfully bind()s, which
  * means the driver will handle setup() requests needed to enumerate (and
@@ -884,6 +886,8 @@ struct usb_gadget_driver {
 
 	/* FIXME support safe rmmod */
 	struct device_driver	driver;
+
+	char			*udc_name;
 };
 
 
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v5 2/5] usb: gadget: configfs: pass UDC name via usb_gadget_driver struct

[Ruslan Bilovol]

Now when udc-core supports binding to specific UDC by passing
its name via 'udc_name' member of usb_gadget_driver struct,
switch to this generic approach.

Tested-by: Maxime Ripard <maxime.ripard@free-electrons.com>
Signed-off-by: Ruslan Bilovol <ruslan.bilovol@gmail.com>
---
 drivers/usb/gadget/configfs.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
index c42765b..efad021 100644
--- a/drivers/usb/gadget/configfs.c
+++ b/drivers/usb/gadget/configfs.c
@@ -54,7 +54,6 @@ struct gadget_info {
 	struct list_head string_list;
 	struct list_head available_func;
 
-	const char *udc_name;
 #ifdef CONFIG_USB_OTG
 	struct usb_otg_descriptor otg;
 #endif
@@ -230,21 +229,21 @@ static ssize_t gadget_dev_desc_bcdUSB_store(struct gadget_info *gi,
 
 static ssize_t gadget_dev_desc_UDC_show(struct gadget_info *gi, char *page)
 {
-	return sprintf(page, "%s\n", gi->udc_name ?: "");
+	return sprintf(page, "%s\n", gi->composite.gadget_driver.udc_name ?: "");
 }
 
 static int unregister_gadget(struct gadget_info *gi)
 {
 	int ret;
 
-	if (!gi->udc_name)
+	if (!gi->composite.gadget_driver.udc_name)
 		return -ENODEV;
 
 	ret = usb_gadget_unregister_driver(&gi->composite.gadget_driver);
 	if (ret)
 		return ret;
-	kfree(gi->udc_name);
-	gi->udc_name = NULL;
+	kfree(gi->composite.gadget_driver.udc_name);
+	gi->composite.gadget_driver.udc_name = NULL;
 	return 0;
 }
 
@@ -267,14 +266,16 @@ static ssize_t gadget_dev_desc_UDC_store(struct gadget_info *gi,
 		if (ret)
 			goto err;
 	} else {
-		if (gi->udc_name) {
+		if (gi->composite.gadget_driver.udc_name) {
 			ret = -EBUSY;
 			goto err;
 		}
-		ret = usb_udc_attach_driver(name, &gi->composite.gadget_driver);
-		if (ret)
+		gi->composite.gadget_driver.udc_name = name;
+		ret = usb_gadget_probe_driver(&gi->composite.gadget_driver);
+		if (ret) {
+			gi->composite.gadget_driver.udc_name = NULL;
 			goto err;
-		gi->udc_name = name;
+		}
 	}
 	mutex_unlock(&gi->lock);
 	return len;
@@ -438,9 +439,9 @@ static int config_usb_cfg_unlink(
 	 * remove the function.
 	 */
 	mutex_lock(&gi->lock);
-	if (gi->udc_name)
+	if (gi->composite.gadget_driver.udc_name)
 		unregister_gadget(gi);
-	WARN_ON(gi->udc_name);
+	WARN_ON(gi->composite.gadget_driver.udc_name);
 
 	list_for_each_entry(f, &cfg->func_list, list) {
 		if (f->fi == fi) {
@@ -917,10 +918,10 @@ static int os_desc_unlink(struct config_item *os_desc_ci,
 	struct usb_composite_dev *cdev = &gi->cdev;
 
 	mutex_lock(&gi->lock);
-	if (gi->udc_name)
+	if (gi->composite.gadget_driver.udc_name)
 		unregister_gadget(gi);
 	cdev->os_desc_config = NULL;
-	WARN_ON(gi->udc_name);
+	WARN_ON(gi->composite.gadget_driver.udc_name);
 	mutex_unlock(&gi->lock);
 	return 0;
 }
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v5 3/5] usb: gadget: udc-core: remove unused usb_udc_attach_driver()

[Ruslan Bilovol]

Now when last user of usb_udc_attach_driver() is switched
to passing UDC name via usb_gadget_driver struct, it's safe
to remove this function

Tested-by: Maxime Ripard <maxime.ripard@free-electrons.com>
Signed-off-by: Ruslan Bilovol <ruslan.bilovol@gmail.com>
---
 drivers/usb/gadget/udc/udc-core.c | 26 --------------------------
 include/linux/usb/gadget.h        |  2 --
 2 files changed, 28 deletions(-)

diff --git a/drivers/usb/gadget/udc/udc-core.c b/drivers/usb/gadget/udc/udc-core.c
index b431129..441877d 100644
--- a/drivers/usb/gadget/udc/udc-core.c
+++ b/drivers/usb/gadget/udc/udc-core.c
@@ -427,32 +427,6 @@ err1:
 	return ret;
 }
 
-int usb_udc_attach_driver(const char *name, struct usb_gadget_driver *driver)
-{
-	struct usb_udc *udc = NULL;
-	int ret = -ENODEV;
-
-	mutex_lock(&udc_lock);
-	list_for_each_entry(udc, &udc_list, list) {
-		ret = strcmp(name, dev_name(&udc->dev));
-		if (!ret)
-			break;
-	}
-	if (ret) {
-		ret = -ENODEV;
-		goto out;
-	}
-	if (udc->driver) {
-		ret = -EBUSY;
-		goto out;
-	}
-	ret = udc_bind_to_driver(udc, driver);
-out:
-	mutex_unlock(&udc_lock);
-	return ret;
-}
-EXPORT_SYMBOL_GPL(usb_udc_attach_driver);
-
 int usb_gadget_probe_driver(struct usb_gadget_driver *driver)
 {
 	struct usb_udc		*udc = NULL;
diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index 8bba379..5482d13 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -933,8 +933,6 @@ extern int usb_add_gadget_udc_release(struct device *parent,
 		struct usb_gadget *gadget, void (*release)(struct device *dev));
 extern int usb_add_gadget_udc(struct device *parent, struct usb_gadget *gadget);
 extern void usb_del_gadget_udc(struct usb_gadget *gadget);
-extern int usb_udc_attach_driver(const char *name,
-		struct usb_gadget_driver *driver);
 
 /*-------------------------------------------------------------------------*/
 
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v5 4/5] usb: gadget: legacy: don't use __init/__exit attributes for bind/unbind path

[Ruslan Bilovol]

In order to prepare to independent gadgets and
gadget drivers registration in udc-core, some of the
functions can't have __init/__exit attributes (almost
only bind/unbind callbacks are affected)

Tested-by: Maxime Ripard <maxime.ripard@free-electrons.com>
Signed-off-by: Ruslan Bilovol <ruslan.bilovol@gmail.com>
---
 drivers/usb/gadget/legacy/acm_ms.c       |  6 +++---
 drivers/usb/gadget/legacy/audio.c        |  6 +++---
 drivers/usb/gadget/legacy/cdc2.c         |  6 +++---
 drivers/usb/gadget/legacy/dbgp.c         |  2 +-
 drivers/usb/gadget/legacy/ether.c        |  8 ++++----
 drivers/usb/gadget/legacy/gmidi.c        |  6 +++---
 drivers/usb/gadget/legacy/hid.c          |  6 +++---
 drivers/usb/gadget/legacy/mass_storage.c |  4 ++--
 drivers/usb/gadget/legacy/multi.c        | 16 ++++++++--------
 drivers/usb/gadget/legacy/ncm.c          |  6 +++---
 drivers/usb/gadget/legacy/nokia.c        |  6 +++---
 drivers/usb/gadget/legacy/printer.c      |  6 +++---
 drivers/usb/gadget/legacy/serial.c       |  2 +-
 drivers/usb/gadget/legacy/webcam.c       |  4 ++--
 drivers/usb/gadget/legacy/zero.c         |  2 +-
 15 files changed, 43 insertions(+), 43 deletions(-)

diff --git a/drivers/usb/gadget/legacy/acm_ms.c b/drivers/usb/gadget/legacy/acm_ms.c
index c30b7b5..3a48aab 100644
--- a/drivers/usb/gadget/legacy/acm_ms.c
+++ b/drivers/usb/gadget/legacy/acm_ms.c
@@ -121,7 +121,7 @@ static struct usb_function *f_msg;
 /*
  * We _always_ have both ACM and mass storage functions.
  */
-static int __init acm_ms_do_config(struct usb_configuration *c)
+static int acm_ms_do_config(struct usb_configuration *c)
 {
 	struct fsg_opts *opts;
 	int	status;
@@ -174,7 +174,7 @@ static struct usb_configuration acm_ms_config_driver = {
 
 /*-------------------------------------------------------------------------*/
 
-static int __init acm_ms_bind(struct usb_composite_dev *cdev)
+static int acm_ms_bind(struct usb_composite_dev *cdev)
 {
 	struct usb_gadget	*gadget = cdev->gadget;
 	struct fsg_opts		*opts;
@@ -249,7 +249,7 @@ fail_get_msg:
 	return status;
 }
 
-static int __exit acm_ms_unbind(struct usb_composite_dev *cdev)
+static int acm_ms_unbind(struct usb_composite_dev *cdev)
 {
 	usb_put_function(f_msg);
 	usb_put_function_instance(fi_msg);
diff --git a/drivers/usb/gadget/legacy/audio.c b/drivers/usb/gadget/legacy/audio.c
index f46a395..ba95518 100644
--- a/drivers/usb/gadget/legacy/audio.c
+++ b/drivers/usb/gadget/legacy/audio.c
@@ -167,7 +167,7 @@ static const struct usb_descriptor_header *otg_desc[] = {
 
 /*-------------------------------------------------------------------------*/
 
-static int __init audio_do_config(struct usb_configuration *c)
+static int audio_do_config(struct usb_configuration *c)
 {
 	int status;
 
@@ -216,7 +216,7 @@ static struct usb_configuration audio_config_driver = {
 
 /*-------------------------------------------------------------------------*/
 
-static int __init audio_bind(struct usb_composite_dev *cdev)
+static int audio_bind(struct usb_composite_dev *cdev)
 {
 #ifndef CONFIG_GADGET_UAC1
 	struct f_uac2_opts	*uac2_opts;
@@ -276,7 +276,7 @@ fail:
 	return status;
 }
 
-static int __exit audio_unbind(struct usb_composite_dev *cdev)
+static int audio_unbind(struct usb_composite_dev *cdev)
 {
 #ifdef CONFIG_GADGET_UAC1
 	if (!IS_ERR_OR_NULL(f_uac1))
diff --git a/drivers/usb/gadget/legacy/cdc2.c b/drivers/usb/gadget/legacy/cdc2.c
index 2e85d94..8d1985c 100644
--- a/drivers/usb/gadget/legacy/cdc2.c
+++ b/drivers/usb/gadget/legacy/cdc2.c
@@ -104,7 +104,7 @@ static struct usb_function_instance *fi_ecm;
 /*
  * We _always_ have both CDC ECM and CDC ACM functions.
  */
-static int __init cdc_do_config(struct usb_configuration *c)
+static int cdc_do_config(struct usb_configuration *c)
 {
 	int	status;
 
@@ -153,7 +153,7 @@ static struct usb_configuration cdc_config_driver = {
 
 /*-------------------------------------------------------------------------*/
 
-static int __init cdc_bind(struct usb_composite_dev *cdev)
+static int cdc_bind(struct usb_composite_dev *cdev)
 {
 	struct usb_gadget	*gadget = cdev->gadget;
 	struct f_ecm_opts	*ecm_opts;
@@ -211,7 +211,7 @@ fail:
 	return status;
 }
 
-static int __exit cdc_unbind(struct usb_composite_dev *cdev)
+static int cdc_unbind(struct usb_composite_dev *cdev)
 {
 	usb_put_function(f_acm);
 	usb_put_function_instance(fi_serial);
diff --git a/drivers/usb/gadget/legacy/dbgp.c b/drivers/usb/gadget/legacy/dbgp.c
index 633683a..7c42b01 100644
--- a/drivers/usb/gadget/legacy/dbgp.c
+++ b/drivers/usb/gadget/legacy/dbgp.c
@@ -284,7 +284,7 @@ fail_1:
 	return -ENODEV;
 }
 
-static int __init dbgp_bind(struct usb_gadget *gadget,
+static int dbgp_bind(struct usb_gadget *gadget,
 		struct usb_gadget_driver *driver)
 {
 	int err, stp;
diff --git a/drivers/usb/gadget/legacy/ether.c b/drivers/usb/gadget/legacy/ether.c
index c5fdc61..4283969 100644
--- a/drivers/usb/gadget/legacy/ether.c
+++ b/drivers/usb/gadget/legacy/ether.c
@@ -222,7 +222,7 @@ static struct usb_function *f_rndis;
  * the first one present.  That's to make Microsoft's drivers happy,
  * and to follow DOCSIS 1.0 (cable modem standard).
  */
-static int __init rndis_do_config(struct usb_configuration *c)
+static int rndis_do_config(struct usb_configuration *c)
 {
 	int status;
 
@@ -264,7 +264,7 @@ MODULE_PARM_DESC(use_eem, "use CDC EEM mode");
 /*
  * We _always_ have an ECM, CDC Subset, or EEM configuration.
  */
-static int __init eth_do_config(struct usb_configuration *c)
+static int eth_do_config(struct usb_configuration *c)
 {
 	int status = 0;
 
@@ -318,7 +318,7 @@ static struct usb_configuration eth_config_driver = {
 
 /*-------------------------------------------------------------------------*/
 
-static int __init eth_bind(struct usb_composite_dev *cdev)
+static int eth_bind(struct usb_composite_dev *cdev)
 {
 	struct usb_gadget	*gadget = cdev->gadget;
 	struct f_eem_opts	*eem_opts = NULL;
@@ -447,7 +447,7 @@ fail:
 	return status;
 }
 
-static int __exit eth_unbind(struct usb_composite_dev *cdev)
+static int eth_unbind(struct usb_composite_dev *cdev)
 {
 	if (has_rndis()) {
 		usb_put_function(f_rndis);
diff --git a/drivers/usb/gadget/legacy/gmidi.c b/drivers/usb/gadget/legacy/gmidi.c
index e02a095..c098281 100644
--- a/drivers/usb/gadget/legacy/gmidi.c
+++ b/drivers/usb/gadget/legacy/gmidi.c
@@ -118,7 +118,7 @@ static struct usb_gadget_strings *dev_strings[] = {
 static struct usb_function_instance *fi_midi;
 static struct usb_function *f_midi;
 
-static int __exit midi_unbind(struct usb_composite_dev *dev)
+static int midi_unbind(struct usb_composite_dev *dev)
 {
 	usb_put_function(f_midi);
 	usb_put_function_instance(fi_midi);
@@ -133,7 +133,7 @@ static struct usb_configuration midi_config = {
 	.MaxPower	= CONFIG_USB_GADGET_VBUS_DRAW,
 };
 
-static int __init midi_bind_config(struct usb_configuration *c)
+static int midi_bind_config(struct usb_configuration *c)
 {
 	int status;
 
@@ -150,7 +150,7 @@ static int __init midi_bind_config(struct usb_configuration *c)
 	return 0;
 }
 
-static int __init midi_bind(struct usb_composite_dev *cdev)
+static int midi_bind(struct usb_composite_dev *cdev)
 {
 	struct f_midi_opts *midi_opts;
 	int status;
diff --git a/drivers/usb/gadget/legacy/hid.c b/drivers/usb/gadget/legacy/hid.c
index 614b06d..11de01b 100644
--- a/drivers/usb/gadget/legacy/hid.c
+++ b/drivers/usb/gadget/legacy/hid.c
@@ -106,7 +106,7 @@ static struct usb_gadget_strings *dev_strings[] = {
 
 /****************************** Configurations ******************************/
 
-static int __init do_config(struct usb_configuration *c)
+static int do_config(struct usb_configuration *c)
 {
 	struct hidg_func_node *e, *n;
 	int status = 0;
@@ -147,7 +147,7 @@ static struct usb_configuration config_driver = {
 
 /****************************** Gadget Bind ******************************/
 
-static int __init hid_bind(struct usb_composite_dev *cdev)
+static int hid_bind(struct usb_composite_dev *cdev)
 {
 	struct usb_gadget *gadget = cdev->gadget;
 	struct list_head *tmp;
@@ -205,7 +205,7 @@ put:
 	return status;
 }
 
-static int __exit hid_unbind(struct usb_composite_dev *cdev)
+static int hid_unbind(struct usb_composite_dev *cdev)
 {
 	struct hidg_func_node *n;
 
diff --git a/drivers/usb/gadget/legacy/mass_storage.c b/drivers/usb/gadget/legacy/mass_storage.c
index 8e27a8c..d169d43 100644
--- a/drivers/usb/gadget/legacy/mass_storage.c
+++ b/drivers/usb/gadget/legacy/mass_storage.c
@@ -130,7 +130,7 @@ static int msg_thread_exits(struct fsg_common *common)
 	return 0;
 }
 
-static int __init msg_do_config(struct usb_configuration *c)
+static int msg_do_config(struct usb_configuration *c)
 {
 	struct fsg_opts *opts;
 	int ret;
@@ -170,7 +170,7 @@ static struct usb_configuration msg_config_driver = {
 
 /****************************** Gadget Bind ******************************/
 
-static int __init msg_bind(struct usb_composite_dev *cdev)
+static int msg_bind(struct usb_composite_dev *cdev)
 {
 	static const struct fsg_operations ops = {
 		.thread_exits = msg_thread_exits,
diff --git a/drivers/usb/gadget/legacy/multi.c b/drivers/usb/gadget/legacy/multi.c
index 39d27bb..4a4a1a1 100644
--- a/drivers/usb/gadget/legacy/multi.c
+++ b/drivers/usb/gadget/legacy/multi.c
@@ -149,7 +149,7 @@ static struct usb_function *f_acm_rndis;
 static struct usb_function *f_rndis;
 static struct usb_function *f_msg_rndis;
 
-static __init int rndis_do_config(struct usb_configuration *c)
+static int rndis_do_config(struct usb_configuration *c)
 {
 	struct fsg_opts *fsg_opts;
 	int ret;
@@ -206,7 +206,7 @@ err_func_rndis:
 	return ret;
 }
 
-static __ref int rndis_config_register(struct usb_composite_dev *cdev)
+static int rndis_config_register(struct usb_composite_dev *cdev)
 {
 	static struct usb_configuration config = {
 		.bConfigurationValue	= MULTI_RNDIS_CONFIG_NUM,
@@ -221,7 +221,7 @@ static __ref int rndis_config_register(struct usb_composite_dev *cdev)
 
 #else
 
-static __ref int rndis_config_register(struct usb_composite_dev *cdev)
+static int rndis_config_register(struct usb_composite_dev *cdev)
 {
 	return 0;
 }
@@ -237,7 +237,7 @@ static struct usb_function *f_acm_multi;
 static struct usb_function *f_ecm;
 static struct usb_function *f_msg_multi;
 
-static __init int cdc_do_config(struct usb_configuration *c)
+static int cdc_do_config(struct usb_configuration *c)
 {
 	struct fsg_opts *fsg_opts;
 	int ret;
@@ -295,7 +295,7 @@ err_func_ecm:
 	return ret;
 }
 
-static __ref int cdc_config_register(struct usb_composite_dev *cdev)
+static int cdc_config_register(struct usb_composite_dev *cdev)
 {
 	static struct usb_configuration config = {
 		.bConfigurationValue	= MULTI_CDC_CONFIG_NUM,
@@ -310,7 +310,7 @@ static __ref int cdc_config_register(struct usb_composite_dev *cdev)
 
 #else
 
-static __ref int cdc_config_register(struct usb_composite_dev *cdev)
+static int cdc_config_register(struct usb_composite_dev *cdev)
 {
 	return 0;
 }
@@ -321,7 +321,7 @@ static __ref int cdc_config_register(struct usb_composite_dev *cdev)
 
 /****************************** Gadget Bind ******************************/
 
-static int __ref multi_bind(struct usb_composite_dev *cdev)
+static int multi_bind(struct usb_composite_dev *cdev)
 {
 	struct usb_gadget *gadget = cdev->gadget;
 #ifdef CONFIG_USB_G_MULTI_CDC
@@ -466,7 +466,7 @@ fail:
 	return status;
 }
 
-static int __exit multi_unbind(struct usb_composite_dev *cdev)
+static int multi_unbind(struct usb_composite_dev *cdev)
 {
 #ifdef CONFIG_USB_G_MULTI_CDC
 	usb_put_function(f_msg_multi);
diff --git a/drivers/usb/gadget/legacy/ncm.c b/drivers/usb/gadget/legacy/ncm.c
index e90e23d..fb14e0c 100644
--- a/drivers/usb/gadget/legacy/ncm.c
+++ b/drivers/usb/gadget/legacy/ncm.c
@@ -107,7 +107,7 @@ static struct usb_function *f_ncm;
 
 /*-------------------------------------------------------------------------*/
 
-static int __init ncm_do_config(struct usb_configuration *c)
+static int ncm_do_config(struct usb_configuration *c)
 {
 	int status;
 
@@ -143,7 +143,7 @@ static struct usb_configuration ncm_config_driver = {
 
 /*-------------------------------------------------------------------------*/
 
-static int __init gncm_bind(struct usb_composite_dev *cdev)
+static int gncm_bind(struct usb_composite_dev *cdev)
 {
 	struct usb_gadget	*gadget = cdev->gadget;
 	struct f_ncm_opts	*ncm_opts;
@@ -186,7 +186,7 @@ fail:
 	return status;
 }
 
-static int __exit gncm_unbind(struct usb_composite_dev *cdev)
+static int gncm_unbind(struct usb_composite_dev *cdev)
 {
 	if (!IS_ERR_OR_NULL(f_ncm))
 		usb_put_function(f_ncm);
diff --git a/drivers/usb/gadget/legacy/nokia.c b/drivers/usb/gadget/legacy/nokia.c
index 9b8fd70..7ad8398 100644
--- a/drivers/usb/gadget/legacy/nokia.c
+++ b/drivers/usb/gadget/legacy/nokia.c
@@ -118,7 +118,7 @@ static struct usb_function_instance *fi_obex1;
 static struct usb_function_instance *fi_obex2;
 static struct usb_function_instance *fi_phonet;
 
-static int __init nokia_bind_config(struct usb_configuration *c)
+static int nokia_bind_config(struct usb_configuration *c)
 {
 	struct usb_function *f_acm;
 	struct usb_function *f_phonet = NULL;
@@ -224,7 +224,7 @@ err_get_acm:
 	return status;
 }
 
-static int __init nokia_bind(struct usb_composite_dev *cdev)
+static int nokia_bind(struct usb_composite_dev *cdev)
 {
 	struct usb_gadget	*gadget = cdev->gadget;
 	int			status;
@@ -307,7 +307,7 @@ err_usb:
 	return status;
 }
 
-static int __exit nokia_unbind(struct usb_composite_dev *cdev)
+static int nokia_unbind(struct usb_composite_dev *cdev)
 {
 	if (!IS_ERR_OR_NULL(f_obex1_cfg2))
 		usb_put_function(f_obex1_cfg2);
diff --git a/drivers/usb/gadget/legacy/printer.c b/drivers/usb/gadget/legacy/printer.c
index d5b6ee7..0201fc5 100644
--- a/drivers/usb/gadget/legacy/printer.c
+++ b/drivers/usb/gadget/legacy/printer.c
@@ -126,7 +126,7 @@ static struct usb_configuration printer_cfg_driver = {
 	.bmAttributes		= USB_CONFIG_ATT_ONE | USB_CONFIG_ATT_SELFPOWER,
 };
 
-static int __init printer_do_config(struct usb_configuration *c)
+static int printer_do_config(struct usb_configuration *c)
 {
 	struct usb_gadget	*gadget = c->cdev->gadget;
 	int			status = 0;
@@ -152,7 +152,7 @@ static int __init printer_do_config(struct usb_configuration *c)
 	return status;
 }
 
-static int __init printer_bind(struct usb_composite_dev *cdev)
+static int printer_bind(struct usb_composite_dev *cdev)
 {
 	struct f_printer_opts *opts;
 	int ret, len;
@@ -191,7 +191,7 @@ static int __init printer_bind(struct usb_composite_dev *cdev)
 	return ret;
 }
 
-static int __exit printer_unbind(struct usb_composite_dev *cdev)
+static int printer_unbind(struct usb_composite_dev *cdev)
 {
 	usb_put_function(f_printer);
 	usb_put_function_instance(fi_printer);
diff --git a/drivers/usb/gadget/legacy/serial.c b/drivers/usb/gadget/legacy/serial.c
index 1f5f978..812780f 100644
--- a/drivers/usb/gadget/legacy/serial.c
+++ b/drivers/usb/gadget/legacy/serial.c
@@ -174,7 +174,7 @@ out:
 	return ret;
 }
 
-static int __init gs_bind(struct usb_composite_dev *cdev)
+static int gs_bind(struct usb_composite_dev *cdev)
 {
 	int			status;
 
diff --git a/drivers/usb/gadget/legacy/webcam.c b/drivers/usb/gadget/legacy/webcam.c
index 04a3da2..d200ab4 100644
--- a/drivers/usb/gadget/legacy/webcam.c
+++ b/drivers/usb/gadget/legacy/webcam.c
@@ -334,7 +334,7 @@ static const struct uvc_descriptor_header * const uvc_ss_streaming_cls[] = {
  * USB configuration
  */
 
-static int __init
+static int
 webcam_config_bind(struct usb_configuration *c)
 {
 	int status = 0;
@@ -368,7 +368,7 @@ webcam_unbind(struct usb_composite_dev *cdev)
 	return 0;
 }
 
-static int __init
+static int
 webcam_bind(struct usb_composite_dev *cdev)
 {
 	struct f_uvc_opts *uvc_opts;
diff --git a/drivers/usb/gadget/legacy/zero.c b/drivers/usb/gadget/legacy/zero.c
index 5ee9515..39ce35a 100644
--- a/drivers/usb/gadget/legacy/zero.c
+++ b/drivers/usb/gadget/legacy/zero.c
@@ -272,7 +272,7 @@ static struct usb_function_instance *func_inst_lb;
 module_param_named(qlen, gzero_options.qlen, uint, S_IRUGO|S_IWUSR);
 MODULE_PARM_DESC(qlen, "depth of loopback queue");
 
-static int __init zero_bind(struct usb_composite_dev *cdev)
+static int zero_bind(struct usb_composite_dev *cdev)
 {
 	struct f_ss_opts	*ss_opts;
 	struct f_lb_opts	*lb_opts;
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v5 5/5] usb: gadget: udc-core: independent registration of gadgets and gadget drivers

[Ruslan Bilovol]

Change behavior during registration of gadgets and
gadget drivers in udc-core. Instead of previous
approach when for successful probe of usb gadget driver
at least one usb gadget should be already registered
use another one where gadget drivers and gadgets
can be registered in udc-core independently.

Independent registration of gadgets and gadget drivers
is useful for built-in into kernel gadget and gadget
driver case - because it's possible that gadget is
really probed only on late_init stage (due to deferred
probe) whereas gadget driver's probe is silently failed
on module_init stage due to no any UDC added.

Also it is useful for modules case - now there is no
difference what module to insert first: gadget module
or gadget driver one.

Tested-by: Maxime Ripard <maxime.ripard@free-electrons.com>
Signed-off-by: Ruslan Bilovol <ruslan.bilovol@gmail.com>
---
 drivers/usb/gadget/udc/udc-core.c | 49 ++++++++++++++++++++++++++++++++-------
 include/linux/usb/gadget.h        |  2 ++
 2 files changed, 43 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/gadget/udc/udc-core.c b/drivers/usb/gadget/udc/udc-core.c
index 441877d..5a764ea 100644
--- a/drivers/usb/gadget/udc/udc-core.c
+++ b/drivers/usb/gadget/udc/udc-core.c
@@ -51,8 +51,12 @@ 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);
+
 /* ------------------------------------------------------------------------- */
 
 #ifdef	CONFIG_HAS_DMA
@@ -264,6 +268,7 @@ int usb_add_gadget_udc_release(struct device *parent, struct usb_gadget *gadget,
 		void (*release)(struct device *dev))
 {
 	struct usb_udc		*udc;
+	struct usb_gadget_driver *driver;
 	int			ret = -ENOMEM;
 
 	udc = kzalloc(sizeof(*udc), GFP_KERNEL);
@@ -311,6 +316,18 @@ int usb_add_gadget_udc_release(struct device *parent, struct usb_gadget *gadget,
 	usb_gadget_set_state(gadget, USB_STATE_NOTATTACHED);
 	udc->vbus = true;
 
+	/* pick up one of pending gadget drivers */
+	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)
+				goto err4;
+			list_del(&driver->pending);
+			break;
+		}
+	}
+
 	mutex_unlock(&udc_lock);
 
 	return 0;
@@ -382,9 +399,16 @@ void usb_del_gadget_udc(struct usb_gadget *gadget)
 	list_del(&udc->list);
 	mutex_unlock(&udc_lock);
 
-	if (udc->driver)
+	if (udc->driver) {
+		struct usb_gadget_driver *driver = udc->driver;
+
 		usb_gadget_remove_driver(udc);
 
+		mutex_lock(&udc_lock);
+		list_add(&driver->pending, &gadget_driver_pending_list);
+		mutex_unlock(&udc_lock);
+	}
+
 	kobject_uevent(&udc->dev.kobj, KOBJ_REMOVE);
 	flush_work(&gadget->work);
 	device_unregister(&udc->dev);
@@ -442,11 +466,7 @@ int usb_gadget_probe_driver(struct usb_gadget_driver *driver)
 			if (!ret)
 				break;
 		}
-		if (ret)
-			ret = -ENODEV;
-		else if (udc->driver)
-			ret = -EBUSY;
-		else
+		if (!ret && !udc->driver)
 			goto found;
 	} else {
 		list_for_each_entry(udc, &udc_list, list) {
@@ -456,9 +476,12 @@ int usb_gadget_probe_driver(struct usb_gadget_driver *driver)
 		}
 	}
 
-	pr_debug("couldn't find an available UDC\n");
+	list_add_tail(&driver->pending, &gadget_driver_pending_list);
+	pr_info("udc-core: couldn't find an available UDC "
+			"- added [%s] to list of pending drivers\n",
+			driver->function);
 	mutex_unlock(&udc_lock);
-	return ret;
+	return 0;
 found:
 	ret = udc_bind_to_driver(udc, driver);
 	mutex_unlock(&udc_lock);
@@ -484,6 +507,16 @@ int usb_gadget_unregister_driver(struct usb_gadget_driver *driver)
 			break;
 		}
 
+	if (ret) {
+		struct usb_gadget_driver *tmp;
+
+		list_for_each_entry(tmp, &gadget_driver_pending_list, pending)
+			if (tmp == driver) {
+				list_del(&driver->pending);
+				ret = 0;
+				break;
+			}
+	}
 	mutex_unlock(&udc_lock);
 	return ret;
 }
diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index 5482d13..9c6fe22 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -826,6 +826,7 @@ static inline int usb_gadget_disconnect(struct usb_gadget *gadget)
  * @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.
  *
  * Devices are disabled till a gadget driver successfully bind()s, which
  * means the driver will handle setup() requests needed to enumerate (and
@@ -888,6 +889,7 @@ struct usb_gadget_driver {
 	struct device_driver	driver;
 
 	char			*udc_name;
+	struct list_head	pending;
 };
 
 
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v5 2/5] usb: gadget: configfs: pass UDC name via usb_gadget_driver struct

[Krzysztof Opasiak]

Hello,

On 06/23/2015 12:01 AM, Ruslan Bilovol wrote:
> Now when udc-core supports binding to specific UDC by passing
> its name via 'udc_name' member of usb_gadget_driver struct,
> switch to this generic approach.
>
> Tested-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> Signed-off-by: Ruslan Bilovol <ruslan.bilovol@gmail.com>
> ---
>   drivers/usb/gadget/configfs.c | 27 ++++++++++++++-------------
>   1 file changed, 14 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
> index c42765b..efad021 100644
> --- a/drivers/usb/gadget/configfs.c
> +++ b/drivers/usb/gadget/configfs.c
> @@ -54,7 +54,6 @@ struct gadget_info {
>   	struct list_head string_list;
>   	struct list_head available_func;
>
> -	const char *udc_name;
>   #ifdef CONFIG_USB_OTG
>   	struct usb_otg_descriptor otg;
>   #endif
> @@ -230,21 +229,21 @@ static ssize_t gadget_dev_desc_bcdUSB_store(struct gadget_info *gi,
>
>   static ssize_t gadget_dev_desc_UDC_show(struct gadget_info *gi, char *page)
>   {
> -	return sprintf(page, "%s\n", gi->udc_name ?: "");
> +	return sprintf(page, "%s\n", gi->composite.gadget_driver.udc_name ?: "");
>   }
>
>   static int unregister_gadget(struct gadget_info *gi)
>   {
>   	int ret;
>
> -	if (!gi->udc_name)
> +	if (!gi->composite.gadget_driver.udc_name)
>   		return -ENODEV;
>
>   	ret = usb_gadget_unregister_driver(&gi->composite.gadget_driver);
>   	if (ret)
>   		return ret;
> -	kfree(gi->udc_name);
> -	gi->udc_name = NULL;
> +	kfree(gi->composite.gadget_driver.udc_name);
> +	gi->composite.gadget_driver.udc_name = NULL;
>   	return 0;
>   }
>
> @@ -267,14 +266,16 @@ static ssize_t gadget_dev_desc_UDC_store(struct gadget_info *gi,
>   		if (ret)
>   			goto err;
>   	} else {
> -		if (gi->udc_name) {
> +		if (gi->composite.gadget_driver.udc_name) {

You are using this very long if condition in a few places, maybe it 
would be more suitable to define a macro or inline function for this? 
Something like gadget_dev_bound() or gadget_dev_is_active() or some 
other more suitable name.

Best regards,
-- 
Krzysztof Opasiak
Samsung R&D Institute Poland
Samsung Electronics
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v5 5/5] usb: gadget: udc-core: independent registration of gadgets and gadget drivers

[Alan Stern]

On Tue, 23 Jun 2015, Ruslan Bilovol wrote:

> Change behavior during registration of gadgets and
> gadget drivers in udc-core. Instead of previous
> approach when for successful probe of usb gadget driver
> at least one usb gadget should be already registered
> use another one where gadget drivers and gadgets
> can be registered in udc-core independently.
> 
> Independent registration of gadgets and gadget drivers
> is useful for built-in into kernel gadget and gadget
> driver case - because it's possible that gadget is
> really probed only on late_init stage (due to deferred
> probe) whereas gadget driver's probe is silently failed
> on module_init stage due to no any UDC added.
> 
> Also it is useful for modules case - now there is no
> difference what module to insert first: gadget module
> or gadget driver one.
> 
> Tested-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> Signed-off-by: Ruslan Bilovol <ruslan.bilovol@gmail.com>

> @@ -484,6 +507,16 @@ int usb_gadget_unregister_driver(struct usb_gadget_driver *driver)
>  			break;
>  		}
>  
> +	if (ret) {
> +		struct usb_gadget_driver *tmp;
> +
> +		list_for_each_entry(tmp, &gadget_driver_pending_list, pending)
> +			if (tmp == driver) {
> +				list_del(&driver->pending);
> +				ret = 0;
> +				break;
> +			}
> +	}

Weren't you going to replace this loop with a simple list_del()?  IIRC,
this is the third time I have asked you to make this change.

Alan Stern

Re: [PATCH v5 2/5] usb: gadget: configfs: pass UDC name via usb_gadget_driver struct

[Ruslan Bilovol]

Hi Krzysztof,

On Tue, Jun 23, 2015 at 9:54 AM, Krzysztof Opasiak
<k.opasiak@samsung.com> wrote:
> Hello,
>
> On 06/23/2015 12:01 AM, Ruslan Bilovol wrote:
>>
>> Now when udc-core supports binding to specific UDC by passing
>> its name via 'udc_name' member of usb_gadget_driver struct,
>> switch to this generic approach.
>>
>> Tested-by: Maxime Ripard <maxime.ripard@free-electrons.com>
>> Signed-off-by: Ruslan Bilovol <ruslan.bilovol@gmail.com>
>> ---
>>   drivers/usb/gadget/configfs.c | 27 ++++++++++++++-------------
>>   1 file changed, 14 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
>> index c42765b..efad021 100644
>> --- a/drivers/usb/gadget/configfs.c
>> +++ b/drivers/usb/gadget/configfs.c
>> @@ -54,7 +54,6 @@ struct gadget_info {
>>         struct list_head string_list;
>>         struct list_head available_func;
>>
>> -       const char *udc_name;
>>   #ifdef CONFIG_USB_OTG
>>         struct usb_otg_descriptor otg;
>>   #endif
>> @@ -230,21 +229,21 @@ static ssize_t gadget_dev_desc_bcdUSB_store(struct
>> gadget_info *gi,
>>
>>   static ssize_t gadget_dev_desc_UDC_show(struct gadget_info *gi, char
>> *page)
>>   {
>> -       return sprintf(page, "%s\n", gi->udc_name ?: "");
>> +       return sprintf(page, "%s\n", gi->composite.gadget_driver.udc_name
>> ?: "");
>>   }
>>
>>   static int unregister_gadget(struct gadget_info *gi)
>>   {
>>         int ret;
>>
>> -       if (!gi->udc_name)
>> +       if (!gi->composite.gadget_driver.udc_name)
>>                 return -ENODEV;
>>
>>         ret = usb_gadget_unregister_driver(&gi->composite.gadget_driver);
>>         if (ret)
>>                 return ret;
>> -       kfree(gi->udc_name);
>> -       gi->udc_name = NULL;
>> +       kfree(gi->composite.gadget_driver.udc_name);
>> +       gi->composite.gadget_driver.udc_name = NULL;
>>         return 0;
>>   }
>>
>> @@ -267,14 +266,16 @@ static ssize_t gadget_dev_desc_UDC_store(struct
>> gadget_info *gi,
>>                 if (ret)
>>                         goto err;
>>         } else {
>> -               if (gi->udc_name) {
>> +               if (gi->composite.gadget_driver.udc_name) {
>
>
> You are using this very long if condition in a few places, maybe it would be
> more suitable to define a macro or inline function for this? Something like
> gadget_dev_bound() or gadget_dev_is_active() or some other more suitable
> name.

Yes, it makes sense, since there are 11 places where this long access
to udc_name is used.
However, it is used not only in if conditions but also for changing
udc_name, so need to add some getters/setters, something like
#define gi_to_udc_name(gi)
((gi)->composite.gadget_driver.udc_name)
#define gi_set_udc_name(gi,name)
((gi)->composite.gadget_driver.udc_name = (name))

Best regards,
Ruslan

Re: [PATCH v5 5/5] usb: gadget: udc-core: independent registration of gadgets and gadget drivers

[Ruslan Bilovol]

Hi Alan,

On Tue, Jun 23, 2015 at 5:08 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Tue, 23 Jun 2015, Ruslan Bilovol wrote:
>
>> Change behavior during registration of gadgets and
>> gadget drivers in udc-core. Instead of previous
>> approach when for successful probe of usb gadget driver
>> at least one usb gadget should be already registered
>> use another one where gadget drivers and gadgets
>> can be registered in udc-core independently.
>>
>> Independent registration of gadgets and gadget drivers
>> is useful for built-in into kernel gadget and gadget
>> driver case - because it's possible that gadget is
>> really probed only on late_init stage (due to deferred
>> probe) whereas gadget driver's probe is silently failed
>> on module_init stage due to no any UDC added.
>>
>> Also it is useful for modules case - now there is no
>> difference what module to insert first: gadget module
>> or gadget driver one.
>>
>> Tested-by: Maxime Ripard <maxime.ripard@free-electrons.com>
>> Signed-off-by: Ruslan Bilovol <ruslan.bilovol@gmail.com>
>
>> @@ -484,6 +507,16 @@ int usb_gadget_unregister_driver(struct usb_gadget_driver *driver)
>>                       break;
>>               }
>>
>> +     if (ret) {
>> +             struct usb_gadget_driver *tmp;
>> +
>> +             list_for_each_entry(tmp, &gadget_driver_pending_list, pending)
>> +                     if (tmp == driver) {
>> +                             list_del(&driver->pending);
>> +                             ret = 0;
>> +                             break;
>> +                     }
>> +     }
>
> Weren't you going to replace this loop with a simple list_del()?  IIRC,
> this is the third time I have asked you to make this change.

I understand the improvement that replacing this loop with a list_del()
may bring for us, but I disagree with doing it in this particular case.

The reason is simple. The usb_gadget_unregister_driver() funciton is
externally visible so we can get junk as input. Current implementation
checks passed pointer and only after that does list_del(), or
returns -EINVAL. Your variant will do list_del() unconditionally, that
may cause a kernel crash or unexpected behavior in case of junk
passed with *driver. The list_del_init() usage can't help here since
there is no way to check that list_head structure is initialized with correct
data or contains junk.

There is no noticeable performance loss with current implementation,just
because current use case is pretty simple: one gadget driver per one UDC,
and usually there is only one UDC per machine (or rare cases with few
UDCs), thus number of pending gadget drivers is relatively small.
We can return back to this discussion if someone needs to register
many gadget drivers, and want to improve performance, because
there are few existing places (not created by me) in this file that uses
same approach of walking through list of registered gadget drivers.

As a bottom line, choosing between stability and little performance
improvement, I prefer stability.

Best regards,
Ruslan

Re: [PATCH v5 5/5] usb: gadget: udc-core: independent registration of gadgets and gadget drivers

[Alan Stern]

On Sun, 28 Jun 2015, Ruslan Bilovol wrote:

> > Weren't you going to replace this loop with a simple list_del()?  IIRC,
> > this is the third time I have asked you to make this change.
> 
> I understand the improvement that replacing this loop with a list_del()
> may bring for us, but I disagree with doing it in this particular case.
> 
> The reason is simple. The usb_gadget_unregister_driver() funciton is
> externally visible so we can get junk as input. Current implementation
> checks passed pointer and only after that does list_del(), or
> returns -EINVAL. Your variant will do list_del() unconditionally, that
> may cause a kernel crash or unexpected behavior in case of junk
> passed with *driver. The list_del_init() usage can't help here since
> there is no way to check that list_head structure is initialized with correct
> data or contains junk.

That's right.

> There is no noticeable performance loss with current implementation,just
> because current use case is pretty simple: one gadget driver per one UDC,
> and usually there is only one UDC per machine (or rare cases with few
> UDCs), thus number of pending gadget drivers is relatively small.
> We can return back to this discussion if someone needs to register
> many gadget drivers, and want to improve performance, because
> there are few existing places (not created by me) in this file that uses
> same approach of walking through list of registered gadget drivers.
> 
> As a bottom line, choosing between stability and little performance
> improvement, I prefer stability.

It's not really a question of code size or performance.  As you say,
the difference in each is minimal.

It _is_ a question of style.  Adding unnecessary code to check for
something that shouldn't need to be checked looks bad.  Other kernel
developers reading that code will notice it and wonder why it's there.  
That's the argument for getting rid of the loop.

Your argument for keeping the loop is to prevent crashes when the 
function is called by a buggy driver.  Lots of other people have made 
similar arguments in the past.  But that's not how the kernel is 
written -- we don't go out of our way to cover up potential bugs.

If a driver is buggy, we _want_ it to cause a crash!  How else are we 
going to know about the bug?  Sure, there might be other symptoms that 
someone might eventually notice.  But nobody can miss an oops.

In short, don't try to protect against mistakes in other people's code.  
Let them stand out so they can get fixed!

Alan Stern

Re: [PATCH v5 5/5] usb: gadget: udc-core: independent registration of gadgets and gadget drivers

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2197 bytes --]

On Sat, Jun 27, 2015 at 07:47:28PM -0400, Alan Stern wrote:
> On Sun, 28 Jun 2015, Ruslan Bilovol wrote:
> 
> > > Weren't you going to replace this loop with a simple list_del()?  IIRC,
> > > this is the third time I have asked you to make this change.
> > 
> > I understand the improvement that replacing this loop with a list_del()
> > may bring for us, but I disagree with doing it in this particular case.
> > 
> > The reason is simple. The usb_gadget_unregister_driver() funciton is
> > externally visible so we can get junk as input. Current implementation
> > checks passed pointer and only after that does list_del(), or
> > returns -EINVAL. Your variant will do list_del() unconditionally, that
> > may cause a kernel crash or unexpected behavior in case of junk
> > passed with *driver. The list_del_init() usage can't help here since
> > there is no way to check that list_head structure is initialized with correct
> > data or contains junk.
> 
> That's right.
> 
> > There is no noticeable performance loss with current implementation,just
> > because current use case is pretty simple: one gadget driver per one UDC,
> > and usually there is only one UDC per machine (or rare cases with few
> > UDCs), thus number of pending gadget drivers is relatively small.
> > We can return back to this discussion if someone needs to register
> > many gadget drivers, and want to improve performance, because
> > there are few existing places (not created by me) in this file that uses
> > same approach of walking through list of registered gadget drivers.
> > 
> > As a bottom line, choosing between stability and little performance
> > improvement, I prefer stability.
> 
> It's not really a question of code size or performance.  As you say,
> the difference in each is minimal.
> 
> It _is_ a question of style.  Adding unnecessary code to check for
> something that shouldn't need to be checked looks bad.  Other kernel
> developers reading that code will notice it and wonder why it's there.  
> That's the argument for getting rid of the loop.

yeah, if someone gives us junk as input, they deserve to oops.
list_del() is enough.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v5 0/5] usb/gadget: independent registration of gadgets and gadget drivers

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1120 bytes --]

Hi,

On Tue, Jun 23, 2015 at 01:01:09AM +0300, Ruslan Bilovol wrote:
> This patchset adds independent registration of gadgets
> and gadget drivers to udc-core. This is very useful for
> built-in modules into kernel case since it's possible
> situation that gadget driver is probing at a time
> when no gadgets are registered in udc-core.
> In this case instead of silently failing without
> of any attempt to recover, with independent registration
> of gadgets and gadget drivers there is no matter
> in which order gadgets and gadget drivers are
> probed/registered.
> 
> This patch has side-effect on gadget drivers that had
> __init/__exit attributes on some paths like bind/unbind
> and (since bind/unbind may happen at any time) should
> not use them now. This is covered by forth patch
> 

Has there been any progress on these patches? They're fixing some real
issue that we're seeing, and it seems to both work quite well and not
generate a lot of pushback.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v5 0/5] usb/gadget: independent registration of gadgets and gadget drivers

[Ruslan Bilovol]

Hi Maxime,

On Mon, Oct 19, 2015 at 11:11 AM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> Hi,
>
> On Tue, Jun 23, 2015 at 01:01:09AM +0300, Ruslan Bilovol wrote:
>> This patchset adds independent registration of gadgets
>> and gadget drivers to udc-core. This is very useful for
>> built-in modules into kernel case since it's possible
>> situation that gadget driver is probing at a time
>> when no gadgets are registered in udc-core.
>> In this case instead of silently failing without
>> of any attempt to recover, with independent registration
>> of gadgets and gadget drivers there is no matter
>> in which order gadgets and gadget drivers are
>> probed/registered.
>>
>> This patch has side-effect on gadget drivers that had
>> __init/__exit attributes on some paths like bind/unbind
>> and (since bind/unbind may happen at any time) should
>> not use them now. This is covered by forth patch
>>
>
> Has there been any progress on these patches? They're fixing some real
> issue that we're seeing, and it seems to both work quite well and not
> generate a lot of pushback.

This patch series has stack on review due to different views
on checking input parameters of externally visible function.

I see there is no any way to get these patches accepted other
than skip checking validity of some input parameters as
was pointed by Alan, although I disagree with it.

I will post updated patch series later

Best regards,
Ruslan


>
> Thanks!
> Maxime
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux, Kernel and Android engineering
> http://free-electrons.com

Re: [PATCH v5 0/5] usb/gadget: independent registration of gadgets and gadget drivers

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1562 bytes --]

Hi Ruslan,

On Mon, Nov 02, 2015 at 06:44:00PM +0200, Ruslan Bilovol wrote:
> >> This patchset adds independent registration of gadgets
> >> and gadget drivers to udc-core. This is very useful for
> >> built-in modules into kernel case since it's possible
> >> situation that gadget driver is probing at a time
> >> when no gadgets are registered in udc-core.
> >> In this case instead of silently failing without
> >> of any attempt to recover, with independent registration
> >> of gadgets and gadget drivers there is no matter
> >> in which order gadgets and gadget drivers are
> >> probed/registered.
> >>
> >> This patch has side-effect on gadget drivers that had
> >> __init/__exit attributes on some paths like bind/unbind
> >> and (since bind/unbind may happen at any time) should
> >> not use them now. This is covered by forth patch
> >>
> >
> > Has there been any progress on these patches? They're fixing some real
> > issue that we're seeing, and it seems to both work quite well and not
> > generate a lot of pushback.
> 
> This patch series has stack on review due to different views
> on checking input parameters of externally visible function.
> 
> I see there is no any way to get these patches accepted other
> than skip checking validity of some input parameters as
> was pointed by Alan, although I disagree with it.
> 
> I will post updated patch series later

Great, thanks a lot!

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]