Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1393003 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 1/2] usb: configfs: allow UDC binding rule configured as binding to *any* UDC changbin.du@intel.com - 2016-05-03 05:20 +0200
    Re: [PATCH 1/2] usb: configfs: allow UDC binding rule configured as  binding to *any* UDC Krzysztof Opasiak <k.opasiak@samsung.com> - 2016-05-04 10:20 +0200
      RE: [PATCH 1/2] usb: configfs: allow UDC binding rule configured as  binding to *any* UDC "Du, Changbin" <changbin.du@intel.com> - 2016-05-05 07:50 +0200
        Re: [PATCH 1/2] usb: configfs: allow UDC binding rule configured as  binding to *any* UDC Krzysztof Opasiak <k.opasiak@samsung.com> - 2016-05-05 09:40 +0200
          RE: [PATCH 1/2] usb: configfs: allow UDC binding rule configured as  binding to *any* UDC "Du, Changbin" <changbin.du@intel.com> - 2016-05-06 04:50 +0200
            Re: [PATCH 1/2] usb: configfs: allow UDC binding rule configured as  binding to *any* UDC Krzysztof Opasiak <k.opasiak@samsung.com> - 2016-05-06 08:00 +0200

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

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

[toc] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

> >>> 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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web