linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v6 0/4] usb/gadget: independent registration of gadgets and gadget drivers
@ 2015-11-20  8:54 Marek Szyprowski
  2015-11-20  8:54 ` [PATCH v6 1/4] usb: gadget: bind UDC by name passed via usb_gadget_driver structure Marek Szyprowski
                   ` (3 more replies)
  0 siblings, 4 replies; 13+ messages in thread
From: Marek Szyprowski @ 2015-11-20  8:54 UTC (permalink / raw)
  To: linux-usb, linux-kernel
  Cc: Marek Szyprowski, Ruslan Bilovol, Bartlomiej Zolnierkiewicz

Hello,

This is a resurrection of the patches initially submitted by Ruslan
Bilovol in the following thread: https://lkml.org/lkml/2015/6/22/554

The changes since the original submission (v5) includes rebase onto
latest linux-next branch, simplification of the code requested by Alan
Stern and Felipe Balbi and removal of a patch, which deleted
__init/__exit attributes (this change has been already merged).

This feature is urgently needed, because it is not longer possible to
use workaround to avoid deferred probe in UDC drivers due to
not-yet-probed i2c regulator drivers (for more information see
https://lkml.org/lkml/2015/10/30/374 ).

This patchset has been successfully tested on Odroid XU3 boards with
DWC3 UDC driver being deferred by missing regulator drivers.

Best regards
Marek Szyprowski
Samsung R&D Institute Poland


Patch summary:

Ruslan Bilovol (4):
  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: udc-core: independent registration of gadgets and gadget
    drivers

 drivers/usb/gadget/configfs.c     | 27 ++++++-------
 drivers/usb/gadget/udc/udc-core.c | 81 +++++++++++++++++++++++----------------
 include/linux/usb/gadget.h        |  8 +++-
 3 files changed, 68 insertions(+), 48 deletions(-)

-- 
1.9.2


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v6 1/4] usb: gadget: bind UDC by name passed via usb_gadget_driver structure
  2015-11-20  8:54 [PATCH v6 0/4] usb/gadget: independent registration of gadgets and gadget drivers Marek Szyprowski
@ 2015-11-20  8:54 ` Marek Szyprowski
  2015-11-20  9:21   ` Peter Chen
  2015-11-20  8:54 ` [PATCH v6 2/4] usb: gadget: configfs: pass UDC name via usb_gadget_driver struct Marek Szyprowski
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 13+ messages in thread
From: Marek Szyprowski @ 2015-11-20  8:54 UTC (permalink / raw)
  To: linux-usb, linux-kernel
  Cc: Marek Szyprowski, Ruslan Bilovol, Bartlomiej Zolnierkiewicz

From: Ruslan Bilovol <ruslan.bilovol@gmail.com>

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>
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.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 f660afb..429d64e 100644
--- a/drivers/usb/gadget/udc/udc-core.c
+++ b/drivers/usb/gadget/udc/udc-core.c
@@ -549,21 +549,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 3d583a1..b32e44f 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -1012,6 +1012,8 @@ static inline int usb_gadget_activate(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
@@ -1072,6 +1074,8 @@ struct usb_gadget_driver {
 
 	/* FIXME support safe rmmod */
 	struct device_driver	driver;
+
+	char			*udc_name;
 };
 
 
-- 
1.9.2


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v6 2/4] usb: gadget: configfs: pass UDC name via usb_gadget_driver struct
  2015-11-20  8:54 [PATCH v6 0/4] usb/gadget: independent registration of gadgets and gadget drivers Marek Szyprowski
  2015-11-20  8:54 ` [PATCH v6 1/4] usb: gadget: bind UDC by name passed via usb_gadget_driver structure Marek Szyprowski
@ 2015-11-20  8:54 ` Marek Szyprowski
  2015-11-20  9:22   ` Peter Chen
  2015-11-20  8:54 ` [PATCH v6 3/4] usb: gadget: udc-core: remove unused usb_udc_attach_driver() Marek Szyprowski
  2015-11-20  8:54 ` [PATCH v6 4/4] usb: gadget: udc-core: independent registration of gadgets and gadget drivers Marek Szyprowski
  3 siblings, 1 reply; 13+ messages in thread
From: Marek Szyprowski @ 2015-11-20  8:54 UTC (permalink / raw)
  To: linux-usb, linux-kernel
  Cc: Marek Szyprowski, Ruslan Bilovol, Bartlomiej Zolnierkiewicz

From: Ruslan Bilovol <ruslan.bilovol@gmail.com>

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>
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.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 163d305..0bc6865 100644
--- a/drivers/usb/gadget/configfs.c
+++ b/drivers/usb/gadget/configfs.c
@@ -56,7 +56,6 @@ struct gadget_info {
 	struct list_head string_list;
 	struct list_head available_func;
 
-	const char *udc_name;
 	struct usb_composite_driver composite;
 	struct usb_composite_dev cdev;
 	bool use_os_desc;
@@ -233,21 +232,21 @@ static ssize_t gadget_dev_desc_bcdUSB_store(struct config_item *item,
 
 static ssize_t gadget_dev_desc_UDC_show(struct config_item *item, char *page)
 {
-	return sprintf(page, "%s\n", to_gadget_info(item)->udc_name ?: "");
+	return sprintf(page, "%s\n", to_gadget_info(item)->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;
 }
 
@@ -271,14 +270,16 @@ static ssize_t gadget_dev_desc_UDC_store(struct config_item *item,
 		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;
@@ -427,9 +428,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) {
@@ -873,10 +874,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.2


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v6 3/4] usb: gadget: udc-core: remove unused usb_udc_attach_driver()
  2015-11-20  8:54 [PATCH v6 0/4] usb/gadget: independent registration of gadgets and gadget drivers Marek Szyprowski
  2015-11-20  8:54 ` [PATCH v6 1/4] usb: gadget: bind UDC by name passed via usb_gadget_driver structure Marek Szyprowski
  2015-11-20  8:54 ` [PATCH v6 2/4] usb: gadget: configfs: pass UDC name via usb_gadget_driver struct Marek Szyprowski
@ 2015-11-20  8:54 ` Marek Szyprowski
  2015-11-20  8:54 ` [PATCH v6 4/4] usb: gadget: udc-core: independent registration of gadgets and gadget drivers Marek Szyprowski
  3 siblings, 0 replies; 13+ messages in thread
From: Marek Szyprowski @ 2015-11-20  8:54 UTC (permalink / raw)
  To: linux-usb, linux-kernel
  Cc: Marek Szyprowski, Ruslan Bilovol, Bartlomiej Zolnierkiewicz

From: Ruslan Bilovol <ruslan.bilovol@gmail.com>

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>
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.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 429d64e..f76ebc8 100644
--- a/drivers/usb/gadget/udc/udc-core.c
+++ b/drivers/usb/gadget/udc/udc-core.c
@@ -520,32 +520,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 b32e44f..e11f5a2 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -1121,8 +1121,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.2


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v6 4/4] usb: gadget: udc-core: independent registration of gadgets and gadget drivers
  2015-11-20  8:54 [PATCH v6 0/4] usb/gadget: independent registration of gadgets and gadget drivers Marek Szyprowski
                   ` (2 preceding siblings ...)
  2015-11-20  8:54 ` [PATCH v6 3/4] usb: gadget: udc-core: remove unused usb_udc_attach_driver() Marek Szyprowski
@ 2015-11-20  8:54 ` Marek Szyprowski
  2015-11-20  9:26   ` Peter Chen
                     ` (2 more replies)
  3 siblings, 3 replies; 13+ messages in thread
From: Marek Szyprowski @ 2015-11-20  8:54 UTC (permalink / raw)
  To: linux-usb, linux-kernel
  Cc: Marek Szyprowski, Ruslan Bilovol, Bartlomiej Zolnierkiewicz

From: Ruslan Bilovol <ruslan.bilovol@gmail.com>

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>
[simplified code as requested by Alan Stern and Felipe Balbi]
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/usb/gadget/udc/udc-core.c | 43 +++++++++++++++++++++++++++++++--------
 include/linux/usb/gadget.h        |  2 ++
 2 files changed, 37 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/gadget/udc/udc-core.c b/drivers/usb/gadget/udc/udc-core.c
index f76ebc8..461b311 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
@@ -356,6 +360,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);
@@ -403,6 +408,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;
@@ -475,9 +492,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);
@@ -535,11 +559,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) {
@@ -549,9 +569,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);
@@ -577,6 +600,10 @@ int usb_gadget_unregister_driver(struct usb_gadget_driver *driver)
 			break;
 		}
 
+	if (ret) {
+		list_del(&driver->pending);
+		ret = 0;
+	}
 	mutex_unlock(&udc_lock);
 	return ret;
 }
diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index e11f5a2..a3436bf 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -1014,6 +1014,7 @@ static inline int usb_gadget_activate(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
@@ -1076,6 +1077,7 @@ struct usb_gadget_driver {
 	struct device_driver	driver;
 
 	char			*udc_name;
+	struct list_head	pending;
 };
 
 
-- 
1.9.2


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH v6 1/4] usb: gadget: bind UDC by name passed via usb_gadget_driver structure
  2015-11-20  8:54 ` [PATCH v6 1/4] usb: gadget: bind UDC by name passed via usb_gadget_driver structure Marek Szyprowski
@ 2015-11-20  9:21   ` Peter Chen
  0 siblings, 0 replies; 13+ messages in thread
From: Peter Chen @ 2015-11-20  9:21 UTC (permalink / raw)
  To: Marek Szyprowski
  Cc: linux-usb, linux-kernel, Ruslan Bilovol, Bartlomiej Zolnierkiewicz

On Fri, Nov 20, 2015 at 09:54:09AM +0100, Marek Szyprowski wrote:
> From: Ruslan Bilovol <ruslan.bilovol@gmail.com>
> 
> 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>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.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 f660afb..429d64e 100644
> --- a/drivers/usb/gadget/udc/udc-core.c
> +++ b/drivers/usb/gadget/udc/udc-core.c
> @@ -549,21 +549,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 3d583a1..b32e44f 100644
> --- a/include/linux/usb/gadget.h
> +++ b/include/linux/usb/gadget.h
> @@ -1012,6 +1012,8 @@ static inline int usb_gadget_activate(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
> @@ -1072,6 +1074,8 @@ struct usb_gadget_driver {
>  
>  	/* FIXME support safe rmmod */
>  	struct device_driver	driver;
> +
> +	char			*udc_name;
>  };
>  

When trying to apply for testing, I meet below warning:

Applying: usb: gadget: bind UDC by name passed via usb_gadget_driver
structure
WARNING: please, no space before tabs
#55: FILE: include/linux/usb/gadget.h:1016:
+ * ^Ithis driver will be bound to any available UDC.$

>  
> -- 
> 1.9.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 

Best Regards,
Peter Chen

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v6 2/4] usb: gadget: configfs: pass UDC name via usb_gadget_driver struct
  2015-11-20  8:54 ` [PATCH v6 2/4] usb: gadget: configfs: pass UDC name via usb_gadget_driver struct Marek Szyprowski
@ 2015-11-20  9:22   ` Peter Chen
  0 siblings, 0 replies; 13+ messages in thread
From: Peter Chen @ 2015-11-20  9:22 UTC (permalink / raw)
  To: Marek Szyprowski
  Cc: linux-usb, linux-kernel, Ruslan Bilovol, Bartlomiej Zolnierkiewicz

On Fri, Nov 20, 2015 at 09:54:10AM +0100, Marek Szyprowski wrote:
> From: Ruslan Bilovol <ruslan.bilovol@gmail.com>
> 
> 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>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.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 163d305..0bc6865 100644
> --- a/drivers/usb/gadget/configfs.c
> +++ b/drivers/usb/gadget/configfs.c
> @@ -56,7 +56,6 @@ struct gadget_info {
>  	struct list_head string_list;
>  	struct list_head available_func;
>  
> -	const char *udc_name;
>  	struct usb_composite_driver composite;
>  	struct usb_composite_dev cdev;
>  	bool use_os_desc;
> @@ -233,21 +232,21 @@ static ssize_t gadget_dev_desc_bcdUSB_store(struct config_item *item,
>  
>  static ssize_t gadget_dev_desc_UDC_show(struct config_item *item, char *page)
>  {
> -	return sprintf(page, "%s\n", to_gadget_info(item)->udc_name ?: "");
> +	return sprintf(page, "%s\n", to_gadget_info(item)->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;
>  }
>  
> @@ -271,14 +270,16 @@ static ssize_t gadget_dev_desc_UDC_store(struct config_item *item,
>  		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;
> @@ -427,9 +428,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) {
> @@ -873,10 +874,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;
>  }
> -- 

Applying: usb: gadget: configfs: pass UDC name via usb_gadget_driver
struct
WARNING: line over 80 characters
#18: FILE: drivers/usb/gadget/configfs.c:235:
+	return sprintf(page, "%s\n",
	to_gadget_info(item)->composite.gadget_driver.udc_name ?: "");


-- 

Best Regards,
Peter Chen

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v6 4/4] usb: gadget: udc-core: independent registration of gadgets and gadget drivers
  2015-11-20  8:54 ` [PATCH v6 4/4] usb: gadget: udc-core: independent registration of gadgets and gadget drivers Marek Szyprowski
@ 2015-11-20  9:26   ` Peter Chen
  2015-11-20  9:45     ` Marek Szyprowski
  2015-11-20 16:27   ` Alan Stern
  2015-11-23  7:44   ` Peter Chen
  2 siblings, 1 reply; 13+ messages in thread
From: Peter Chen @ 2015-11-20  9:26 UTC (permalink / raw)
  To: Marek Szyprowski
  Cc: linux-usb, linux-kernel, Ruslan Bilovol, Bartlomiej Zolnierkiewicz

On Fri, Nov 20, 2015 at 09:54:12AM +0100, Marek Szyprowski wrote:
> From: Ruslan Bilovol <ruslan.bilovol@gmail.com>
> 
> 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>
> [simplified code as requested by Alan Stern and Felipe Balbi]
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/usb/gadget/udc/udc-core.c | 43 +++++++++++++++++++++++++++++++--------
>  include/linux/usb/gadget.h        |  2 ++
>  2 files changed, 37 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/usb/gadget/udc/udc-core.c b/drivers/usb/gadget/udc/udc-core.c
> index f76ebc8..461b311 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
> @@ -356,6 +360,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);
> @@ -403,6 +408,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;
> @@ -475,9 +492,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);
> @@ -535,11 +559,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) {
> @@ -549,9 +569,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);
> @@ -577,6 +600,10 @@ int usb_gadget_unregister_driver(struct usb_gadget_driver *driver)
>  			break;
>  		}
>  
> +	if (ret) {
> +		list_del(&driver->pending);
> +		ret = 0;
> +	}
>  	mutex_unlock(&udc_lock);
>  	return ret;
>  }
> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
> index e11f5a2..a3436bf 100644
> --- a/include/linux/usb/gadget.h
> +++ b/include/linux/usb/gadget.h
> @@ -1014,6 +1014,7 @@ static inline int usb_gadget_activate(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
> @@ -1076,6 +1077,7 @@ struct usb_gadget_driver {
>  	struct device_driver	driver;
>  
>  	char			*udc_name;
> +	struct list_head	pending;
>  };
>  
>  

And it seems can't apply for felipe's testing/next which I just rebased
on it.

-- 

Best Regards,
Peter Chen

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v6 4/4] usb: gadget: udc-core: independent registration of gadgets and gadget drivers
  2015-11-20  9:26   ` Peter Chen
@ 2015-11-20  9:45     ` Marek Szyprowski
  2015-11-20  9:51       ` Peter Chen
  0 siblings, 1 reply; 13+ messages in thread
From: Marek Szyprowski @ 2015-11-20  9:45 UTC (permalink / raw)
  To: Peter Chen
  Cc: linux-usb, linux-kernel, Ruslan Bilovol, Bartlomiej Zolnierkiewicz

Hello,

On 2015-11-20 10:26, Peter Chen wrote:
> On Fri, Nov 20, 2015 at 09:54:12AM +0100, Marek Szyprowski wrote:
>> From: Ruslan Bilovol <ruslan.bilovol@gmail.com>
>>
>> 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>
>> [simplified code as requested by Alan Stern and Felipe Balbi]
>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> ---
>>   drivers/usb/gadget/udc/udc-core.c | 43 +++++++++++++++++++++++++++++++--------
>>   include/linux/usb/gadget.h        |  2 ++
>>   2 files changed, 37 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/udc/udc-core.c b/drivers/usb/gadget/udc/udc-core.c
>> index f76ebc8..461b311 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
>> @@ -356,6 +360,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);
>> @@ -403,6 +408,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;
>> @@ -475,9 +492,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);
>> @@ -535,11 +559,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) {
>> @@ -549,9 +569,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);
>> @@ -577,6 +600,10 @@ int usb_gadget_unregister_driver(struct usb_gadget_driver *driver)
>>   			break;
>>   		}
>>   
>> +	if (ret) {
>> +		list_del(&driver->pending);
>> +		ret = 0;
>> +	}
>>   	mutex_unlock(&udc_lock);
>>   	return ret;
>>   }
>> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
>> index e11f5a2..a3436bf 100644
>> --- a/include/linux/usb/gadget.h
>> +++ b/include/linux/usb/gadget.h
>> @@ -1014,6 +1014,7 @@ static inline int usb_gadget_activate(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
>> @@ -1076,6 +1077,7 @@ struct usb_gadget_driver {
>>   	struct device_driver	driver;
>>   
>>   	char			*udc_name;
>> +	struct list_head	pending;
>>   };
>>   
>>   
> And it seems can't apply for felipe's testing/next which I just rebased
> on it.

I really have no idea why it fails for You. The patchset applies 
correctly on both
Felipe's 'next' and 'testing/next' branches from today.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland


^ permalink raw reply	[flat|nested] 13+ messages in thread

* RE: [PATCH v6 4/4] usb: gadget: udc-core: independent registration of gadgets and gadget drivers
  2015-11-20  9:45     ` Marek Szyprowski
@ 2015-11-20  9:51       ` Peter Chen
  0 siblings, 0 replies; 13+ messages in thread
From: Peter Chen @ 2015-11-20  9:51 UTC (permalink / raw)
  To: Marek Szyprowski
  Cc: linux-usb, linux-kernel, Ruslan Bilovol, Bartlomiej Zolnierkiewicz

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1050 bytes --]


  
> >>
> > And it seems can't apply for felipe's testing/next which I just
> > rebased on it.
> 
> I really have no idea why it fails for You. The patchset applies correctly on both
> Felipe's 'next' and 'testing/next' branches from today.
> 

I may know the reason, it may be because I changed include/linux/usb/gadget.h due to
your 1st patch's warning, since I enable checkpatch.pl hook for applying, I need to
fix it after applying more.

Applying: usb: gadget: udc-core: independent registration of gadgets and gadget drivers
fatal: sha1 information is lacking or useless (include/linux/usb/gadget.h).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 usb: gadget: udc-core: independent registration of gadgets and gadget drivers

Peter

> Best regards
> --
> Marek Szyprowski, PhD
> Samsung R&D Institute Poland

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v6 4/4] usb: gadget: udc-core: independent registration of gadgets and gadget drivers
  2015-11-20  8:54 ` [PATCH v6 4/4] usb: gadget: udc-core: independent registration of gadgets and gadget drivers Marek Szyprowski
  2015-11-20  9:26   ` Peter Chen
@ 2015-11-20 16:27   ` Alan Stern
  2015-11-23  7:40     ` Peter Chen
  2015-11-23  7:44   ` Peter Chen
  2 siblings, 1 reply; 13+ messages in thread
From: Alan Stern @ 2015-11-20 16:27 UTC (permalink / raw)
  To: Marek Szyprowski
  Cc: linux-usb, linux-kernel, Ruslan Bilovol, Bartlomiej Zolnierkiewicz

On Fri, 20 Nov 2015, Marek Szyprowski wrote:

> From: Ruslan Bilovol <ruslan.bilovol@gmail.com>
> 
> 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>
> [simplified code as requested by Alan Stern and Felipe Balbi]
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

...

> @@ -475,9 +492,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);
> +	}

It looks like there is a race here with usb_gadget_unregister_driver().  
Would it be okay to hold the udc_lock mutex throughout the whole "if"  
statement?

Alan Stern


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v6 4/4] usb: gadget: udc-core: independent registration of gadgets and gadget drivers
  2015-11-20 16:27   ` Alan Stern
@ 2015-11-23  7:40     ` Peter Chen
  0 siblings, 0 replies; 13+ messages in thread
From: Peter Chen @ 2015-11-23  7:40 UTC (permalink / raw)
  To: Alan Stern
  Cc: Marek Szyprowski, linux-usb, linux-kernel, Ruslan Bilovol,
	Bartlomiej Zolnierkiewicz

On Fri, Nov 20, 2015 at 11:27:36AM -0500, Alan Stern wrote:
> On Fri, 20 Nov 2015, Marek Szyprowski wrote:
> 
> > From: Ruslan Bilovol <ruslan.bilovol@gmail.com>
> > 
> > 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>
> > [simplified code as requested by Alan Stern and Felipe Balbi]
> > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> 
> ...
> 
> > @@ -475,9 +492,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);
> > +	}
> 
> It looks like there is a race here with usb_gadget_unregister_driver().  
> Would it be okay to hold the udc_lock mutex throughout the whole "if"  
> statement?
> 

+1

In fact, only one mutex_lock/mutex_unlock is needed at this function.
-- 

Best Regards,
Peter Chen

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v6 4/4] usb: gadget: udc-core: independent registration of gadgets and gadget drivers
  2015-11-20  8:54 ` [PATCH v6 4/4] usb: gadget: udc-core: independent registration of gadgets and gadget drivers Marek Szyprowski
  2015-11-20  9:26   ` Peter Chen
  2015-11-20 16:27   ` Alan Stern
@ 2015-11-23  7:44   ` Peter Chen
  2 siblings, 0 replies; 13+ messages in thread
From: Peter Chen @ 2015-11-23  7:44 UTC (permalink / raw)
  To: Marek Szyprowski
  Cc: linux-usb, linux-kernel, Ruslan Bilovol, Bartlomiej Zolnierkiewicz

On Fri, Nov 20, 2015 at 09:54:12AM +0100, Marek Szyprowski wrote:
> From: Ruslan Bilovol <ruslan.bilovol@gmail.com>
> 
> 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>
> [simplified code as requested by Alan Stern and Felipe Balbi]
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/usb/gadget/udc/udc-core.c | 43 +++++++++++++++++++++++++++++++--------
>  include/linux/usb/gadget.h        |  2 ++
>  2 files changed, 37 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/usb/gadget/udc/udc-core.c b/drivers/usb/gadget/udc/udc-core.c
> index f76ebc8..461b311 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
> @@ -356,6 +360,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);
> @@ -403,6 +408,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;
> @@ -475,9 +492,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);
> @@ -535,11 +559,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) {
> @@ -549,9 +569,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);

There is an warning when trying to apply it:

WARNING: quoted string split across lines.

I tested this series, it works ok for both module and build-in function,
it fixed gadget function broken if it is build-in at imx6qdl sabreauto board.

Tested-by: Peter Chen <peter.chen@freescale.com>

-- 

Best Regards,
Peter Chen

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2015-11-23  7:48 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-11-20  8:54 [PATCH v6 0/4] usb/gadget: independent registration of gadgets and gadget drivers Marek Szyprowski
2015-11-20  8:54 ` [PATCH v6 1/4] usb: gadget: bind UDC by name passed via usb_gadget_driver structure Marek Szyprowski
2015-11-20  9:21   ` Peter Chen
2015-11-20  8:54 ` [PATCH v6 2/4] usb: gadget: configfs: pass UDC name via usb_gadget_driver struct Marek Szyprowski
2015-11-20  9:22   ` Peter Chen
2015-11-20  8:54 ` [PATCH v6 3/4] usb: gadget: udc-core: remove unused usb_udc_attach_driver() Marek Szyprowski
2015-11-20  8:54 ` [PATCH v6 4/4] usb: gadget: udc-core: independent registration of gadgets and gadget drivers Marek Szyprowski
2015-11-20  9:26   ` Peter Chen
2015-11-20  9:45     ` Marek Szyprowski
2015-11-20  9:51       ` Peter Chen
2015-11-20 16:27   ` Alan Stern
2015-11-23  7:40     ` Peter Chen
2015-11-23  7:44   ` Peter Chen

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).