[PATCH 0/2] Add binding rule *any* support for USB Gadget Configfs

[PATCH 0/2] Add binding rule *any* support for USB Gadget Configfs

[changbin.du]

From: "Du, Changbin" <changbin.du@intel.com>

When I am configuring Gadget Configfs, I need found a exactly UDC name before
I can start my gadget. But, really I doesn't care about which UDC name is used,
because I have only controller. "any" is a good option.

Du, Changbin (2):
  usb: configfs: allow UDC binding rule configured as binding to *any*
    UDC
  Documentation: gadget_configfs: update UDC binding

 Documentation/usb/gadget_configfs.txt |  6 ++++--
 drivers/usb/gadget/configfs.c         | 22 ++++++++++++++--------
 2 files changed, 18 insertions(+), 10 deletions(-)

-- 
2.7.4

[PATCH 1/2] usb: configfs: allow UDC binding rule configured as binding to *any* UDC

[changbin.du]

From: "Du, Changbin" <changbin.du@gmail.com>

On most platforms, there is only one device controller available.
In this case, we desn't care the UDC's name. So let's ignore the
name by setting 'UDC' to 'any'. And also we can change UDC name
at any time if it is not binded (no need set to "" first).

Signed-off-by: Du, Changbin <changbin.du@gmail.com>
Signed-off-by: Du, Changbin <changbin.du@intel.com>
---
 drivers/usb/gadget/configfs.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
index b6f60ca..5da2991 100644
--- a/drivers/usb/gadget/configfs.c
+++ b/drivers/usb/gadget/configfs.c
@@ -230,16 +230,18 @@ 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)
 {
-	char *udc_name = to_gadget_info(item)->composite.gadget_driver.udc_name;
+	struct gadget_info *gi = to_gadget_info(item);
+	char *udc_name = gi->composite.gadget_driver.udc_name;
 
-	return sprintf(page, "%s\n", udc_name ?: "");
+	return sprintf(page, "%s\n", udc_name ?:
+			(gi->cdev.gadget ? "any" : ""));
 }
 
 static int unregister_gadget(struct gadget_info *gi)
 {
 	int ret;
 
-	if (!gi->composite.gadget_driver.udc_name)
+	if (!gi->cdev.gadget)
 		return -ENODEV;
 
 	ret = usb_gadget_unregister_driver(&gi->composite.gadget_driver);
@@ -270,10 +272,14 @@ static ssize_t gadget_dev_desc_UDC_store(struct config_item *item,
 		if (ret)
 			goto err;
 	} else {
-		if (gi->composite.gadget_driver.udc_name) {
+		if (gi->cdev.gadget) {
 			ret = -EBUSY;
 			goto err;
 		}
+		if (!strcmp(name, "any")) {
+			kfree(name);
+			name = NULL;
+		}
 		gi->composite.gadget_driver.udc_name = name;
 		ret = usb_gadget_probe_driver(&gi->composite.gadget_driver);
 		if (ret) {
@@ -428,9 +434,9 @@ static int config_usb_cfg_unlink(
 	 * remove the function.
 	 */
 	mutex_lock(&gi->lock);
-	if (gi->composite.gadget_driver.udc_name)
+	if (gi->cdev.gadget)
 		unregister_gadget(gi);
-	WARN_ON(gi->composite.gadget_driver.udc_name);
+	WARN_ON(gi->cdev.gadget);
 
 	list_for_each_entry(f, &cfg->func_list, list) {
 		if (f->fi == fi) {
@@ -873,10 +879,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->composite.gadget_driver.udc_name)
+	if (gi->cdev.gadget)
 		unregister_gadget(gi);
 	cdev->os_desc_config = NULL;
-	WARN_ON(gi->composite.gadget_driver.udc_name);
+	WARN_ON(gi->cdev.gadget);
 	mutex_unlock(&gi->lock);
 	return 0;
 }
-- 
2.7.4

Re: [PATCH 1/2] usb: configfs: allow UDC binding rule configured as binding to *any* UDC

[Krzysztof Opasiak]

On 05/03/2016 05:04 AM, changbin.du@intel.com wrote:
> From: "Du, Changbin" <changbin.du@gmail.com>
> 
> On most platforms, there is only one device controller available.
> In this case, we desn't care the UDC's name. So let's ignore the
> name by setting 'UDC' to 'any'.

Hmm libubsgx allows to do this for a very long time. You simply pass
NULL instead of pointer to usbg_udc.

It is also possible to do this from command line, just simply:

$ echo `ls -1 /sys/class/udc | head -n 1` > UDC

So if we can easily do this from user space what's the benefit of adding
this special "any" keyword to kernel?

> And also we can change UDC name
> at any time if it is not binded (no need set to "" first).
> 

Not sure if:

$ echo "" > UDC

is really a problem. Personally I'm quite used to situation in which I
have to turn the light off before turning it on once again;)

Cheers,
-- 
Krzysztof Opasiak
Samsung R&D Institute Poland
Samsung Electronics

RE: [PATCH 1/2] usb: configfs: allow UDC binding rule configured as binding to *any* UDC

[Du, Changbin]

Hi,
> > On most platforms, there is only one device controller available.
> > In this case, we desn't care the UDC's name. So let's ignore the
> > name by setting 'UDC' to 'any'.
> 
> Hmm libubsgx allows to do this for a very long time. You simply pass
> NULL instead of pointer to usbg_udc.
> 
> It is also possible to do this from command line, just simply:
> 
> $ echo `ls -1 /sys/class/udc | head -n 1` > UDC
> 
> So if we can easily do this from user space what's the benefit of adding
> this special "any" keyword to kernel?
> 
Well, it is just for *easy to use*. Looking up /sys/class/udc mostly
can be skipped. The UDC core support this convenience behavior, 
so why don't we export it with a little change?

> > And also we can change UDC name
> > at any time if it is not binded (no need set to "" first).
> >
> 
> Not sure if:
> 
> $ echo "" > UDC
> 
> is really a problem. Personally I'm quite used to situation in which I
> have to turn the light off before turning it on once again;)
> 
That is not a problem. But just avoid pseudo 'busy'. If gadget is not 
bind, it is free to reconfigure it. So seem no need block re-configuration.

In a word, this patch is just an improvement, not to fix any issues or
add new function.

> Cheers,
> --
> Krzysztof Opasiak
> Samsung R&D Institute Poland
> Samsung Electronics

Thanks,
Du, Changbin

Re: [PATCH 1/2] usb: configfs: allow UDC binding rule configured as binding to *any* UDC

[Krzysztof Opasiak]

Hi,

On 05/05/2016 07:46 AM, Du, Changbin wrote:
> Hi,
>>> On most platforms, there is only one device controller available.
>>> In this case, we desn't care the UDC's name. So let's ignore the
>>> name by setting 'UDC' to 'any'.
>>
>> Hmm libubsgx allows to do this for a very long time. You simply pass
>> NULL instead of pointer to usbg_udc.
>>
>> It is also possible to do this from command line, just simply:
>>
>> $ echo `ls -1 /sys/class/udc | head -n 1` > UDC
>>
>> So if we can easily do this from user space what's the benefit of adding
>> this special "any" keyword to kernel?
>>
> Well, it is just for *easy to use*. Looking up /sys/class/udc mostly
> can be skipped. The UDC core support this convenience behavior, 
> so why don't we export it with a little change?
> 

Well, I'm not sure if any configfs interface has been proposed as easy
to use from cmd line. I think they all has been proposed as  *usable*
from cmd line but not necessarily *easy to use*.

That's why most of configfs clients has some support in userspace. For
example for target there is a taget-cli and for usb gadget we have
libusbg/libusbgx.

So the functionality which you proposed here is not only already
implemented in libusbgx but also can be easily achieved from cmd line
like I showed above.

In addition this patch will break existing userspace tools (at least
libusbgx for sure) as it assumes that:

cat UDC

should return an empty string or an valid UDC name which can be found
inside /sys/class/udc.

After this patch the kernel can return some kind of magic string "any"
which obviously will cannot be found in udc dir.

>>> And also we can change UDC name
>>> at any time if it is not binded (no need set to "" first).
>>>
>>
>> Not sure if:
>>
>> $ echo "" > UDC
>>
>> is really a problem. Personally I'm quite used to situation in which I
>> have to turn the light off before turning it on once again;)
>>
> That is not a problem. But just avoid pseudo 'busy'. If gadget is not 
> bind, it is free to reconfigure it. So seem no need block re-configuration.
> 

What do you mean pseudo 'busy'? If we do:

echo <udc-name> > UDC

then gadget should be really bound to some udc and potentially really busy.

> In a word, this patch is just an improvement, not to fix any issues or
> add new function.

So it doesn't add any new functionality and breaks existing user space
tools.

Cheers,
-- 
Krzysztof Opasiak
Samsung R&D Institute Poland
Samsung Electronics

RE: [PATCH 1/2] usb: configfs: allow UDC binding rule configured as binding to *any* UDC

[Du, Changbin]

> >>> On most platforms, there is only one device controller available.
> >>> In this case, we desn't care the UDC's name. So let's ignore the
> >>> name by setting 'UDC' to 'any'.
> >>
> >> Hmm libubsgx allows to do this for a very long time. You simply pass
> >> NULL instead of pointer to usbg_udc.
> >>
> >> It is also possible to do this from command line, just simply:
> >>
> >> $ echo `ls -1 /sys/class/udc | head -n 1` > UDC
> >>
> >> So if we can easily do this from user space what's the benefit of adding
> >> this special "any" keyword to kernel?
> >>
> > Well, it is just for *easy to use*. Looking up /sys/class/udc mostly
> > can be skipped. The UDC core support this convenience behavior,
> > so why don't we export it with a little change?
> >
> 
> Well, I'm not sure if any configfs interface has been proposed as easy
> to use from cmd line. I think they all has been proposed as  *usable*
> from cmd line but not necessarily *easy to use*.
> 
> That's why most of configfs clients has some support in userspace. For
> example for target there is a taget-cli and for usb gadget we have
> libusbg/libusbgx.
> 
Glade to know this tool, it is much more easy to use than interact with sysfs.
I'd like use it. Just see you are the main contributor of this project. :)


> So the functionality which you proposed here is not only already
> implemented in libusbgx but also can be easily achieved from cmd line
> like I showed above.
> 
> In addition this patch will break existing userspace tools (at least
> libusbgx for sure) as it assumes that:
> 
> cat UDC
> 
> should return an empty string or an valid UDC name which can be found
> inside /sys/class/udc.

If so, this is not good.

> >> is really a problem. Personally I'm quite used to situation in which I
> >> have to turn the light off before turning it on once again;)
> >>
> > That is not a problem. But just avoid pseudo 'busy'. If gadget is not
> > bind, it is free to reconfigure it. So seem no need block re-configuration.
> >
> 
> What do you mean pseudo 'busy'? If we do:
> 
> echo <udc-name> > UDC
> 
Sorry, please ignore this. I find if no UDC available, the config will be queued
to a list, and will bind it when a UDC module install. So it is really busy.

> then gadget should be really bound to some udc and potentially really busy.
> 
> > In a word, this patch is just an improvement, not to fix any issues or
> > add new function.
> 
> So it doesn't add any new functionality and breaks existing user space
> tools.
> 

Ok, regarding there is a better tool, this change doesn't make much sense. 
So just abandon it.

> Cheers,
> --
> Krzysztof Opasiak
> Samsung R&D Institute Poland
> Samsung Electronics

Best Regards,
Du, Changbin

Re: [PATCH 1/2] usb: configfs: allow UDC binding rule configured as binding to *any* UDC

[Krzysztof Opasiak]

On 05/06/2016 04:46 AM, Du, Changbin wrote:
(...)
>> Well, I'm not sure if any configfs interface has been proposed as easy
>> to use from cmd line. I think they all has been proposed as  *usable*
>> from cmd line but not necessarily *easy to use*.
>>
>> That's why most of configfs clients has some support in userspace. For
>> example for target there is a taget-cli and for usb gadget we have
>> libusbg/libusbgx.
>>
> Glade to know this tool, it is much more easy to use than interact with sysfs.
> I'd like use it. Just see you are the main contributor of this project. :)
> 

That's true;) personally I would recommend you using libusbgx[1] instead
of libusbg[2] as it is far more recent and usable (292 commits vs 128;) )

(...)

>>
>> What do you mean pseudo 'busy'? If we do:
>>
>> echo <udc-name> > UDC
>>
> Sorry, please ignore this. I find if no UDC available, the config will be queued
> to a list, and will bind it when a UDC module install. So it is really busy.
> 
>> then gadget should be really bound to some udc and potentially really busy.
>>
>>> In a word, this patch is just an improvement, not to fix any issues or
>>> add new function.
>>
>> So it doesn't add any new functionality and breaks existing user space
>> tools.
>>

Yes, currently it's true but it's a bug which I have fixed yesterday[3]

Footnotes:
1 - https://github.com/libusbgx/libusbgx
2 - https://github.com/libusbg/libusbg
3 - http://marc.info/?l=linux-usb&m=146243801207458&w=2

Cheers,
-- 
Krzysztof Opasiak
Samsung R&D Institute Poland
Samsung Electronics

Re: [PATCH 1/2] usb: configfs: allow UDC binding rule configured as binding to *any* UDC

[Pavel Machek]

On Tue 2016-05-03 11:04:24, changbin.du@intel.com wrote:
> From: "Du, Changbin" <changbin.du@gmail.com>
> 
> On most platforms, there is only one device controller available.
> In this case, we desn't care the UDC's name. So let's ignore the
> name by setting 'UDC' to 'any'. And also we can change UDC name
> at any time if it is not binded (no need set to "" first).

making "any" special does not look like a good idea. What if it really
is "any"?

Return nothing instead, not even \n?

> Signed-off-by: Du, Changbin <changbin.du@gmail.com>
> Signed-off-by: Du, Changbin <changbin.du@intel.com>

I don't think this is how you should sign it off.

Best regards,

									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

RE: [PATCH 1/2] usb: configfs: allow UDC binding rule configured as binding to *any* UDC

[Du, Changbin]

Thanks, Machek, This patch has already been dropped.

> On Tue 2016-05-03 11:04:24, changbin.du@intel.com wrote:
> > From: "Du, Changbin" <changbin.du@gmail.com>
> >
> > On most platforms, there is only one device controller available.
> > In this case, we desn't care the UDC's name. So let's ignore the
> > name by setting 'UDC' to 'any'. And also we can change UDC name
> > at any time if it is not binded (no need set to "" first).
> 
> making "any" special does not look like a good idea. What if it really
> is "any"?
> 
> Return nothing instead, not even \n?
> 
> > Signed-off-by: Du, Changbin <changbin.du@gmail.com>
> > Signed-off-by: Du, Changbin <changbin.du@intel.com>
> 
> I don't think this is how you should sign it off.
> 
> Best regards,
> 
> 									Pavel
> 
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures)
> http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[PATCH 2/2] Documentation: gadget_configfs: update UDC binding

[changbin.du]

From: "Du, Changbin" <changbin.du@gmail.com>

Add the usage of new binding mode 'any'.
$ echo any > UDC

Signed-off-by: Du, Changbin <changbin.du@gmail.com>
Signed-off-by: Du, Changbin <changbin.du@intel.com>
---
 Documentation/usb/gadget_configfs.txt | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/Documentation/usb/gadget_configfs.txt b/Documentation/usb/gadget_configfs.txt
index 635e574..1edeb4f 100644
--- a/Documentation/usb/gadget_configfs.txt
+++ b/Documentation/usb/gadget_configfs.txt
@@ -205,11 +205,13 @@ In order to enable the gadget it must be bound to a UDC (USB Device Controller).
 
 $ echo <udc name> > UDC
 
-where <udc name> is one of those found in /sys/class/udc/*
+where <udc name> is one of those found in /sys/class/udc/* or "any" if you
+doesn't care about which controller to bind.
 e.g.:
 
 $ echo s3c-hsotg > UDC
-
+or
+$ echo any > UDC
 
 6. Disabling the gadget
 -----------------------
-- 
2.7.4