[PATCH] usbip: handle length at sysfs show() functions

[PATCH] usbip: handle length at sysfs show() functions

[Németh Márton]

From: Márton Németh <nm127@freemail.hu>

The sysfs show() functions shall return the actual content length of
the result buffer. According to Documentation/filesystems/sysfs.txt:215
the scnprintf() function is preferred.

See also the article titled "snprintf() confusion" at
http://lwn.net/Articles/69419/ .

Signed-off-by: Márton Németh <nm127@freemail.hu>
---
diff --git a/drivers/staging/usbip/stub_dev.c b/drivers/staging/usbip/stub_dev.c
index 6e99ec8..af5f107 100644
--- a/drivers/staging/usbip/stub_dev.c
+++ b/drivers/staging/usbip/stub_dev.c
@@ -80,7 +80,7 @@ static ssize_t show_status(struct device *dev, struct device_attribute *attr,
 	status = sdev->ud.status;
 	spin_unlock(&sdev->ud.lock);

-	return snprintf(buf, PAGE_SIZE, "%d\n", status);
+	return scnprintf(buf, PAGE_SIZE, "%d\n", status);
 }
 static DEVICE_ATTR(usbip_status, S_IRUGO, show_status, NULL);

diff --git a/drivers/staging/usbip/stub_main.c b/drivers/staging/usbip/stub_main.c
index e9085d6..604e6bb 100644
--- a/drivers/staging/usbip/stub_main.c
+++ b/drivers/staging/usbip/stub_main.c
@@ -79,18 +79,22 @@ static ssize_t show_match_busid(struct device_driver *drv, char *buf)
 {
 	int i;
 	char *out = buf;
+	int count = 0;

 	spin_lock(&busid_table_lock);

 	for (i = 0; i < MAX_BUSID; i++)
-		if (busid_table[i].name[0])
-			out += sprintf(out, "%s ", busid_table[i].name);
+		if (busid_table[i].name[0]) {
+			count += scnprintf(out, PAGE_SIZE - count,
+					"%s ", busid_table[i].name);
+			out = buf + count;
+		}

 	spin_unlock(&busid_table_lock);

-	out += sprintf(out, "\n");
+	count += scnprintf(out, PAGE_SIZE - count, "\n");

-	return out - buf;
+	return count;
 }

 static int add_match_busid(char *busid)
diff --git a/drivers/staging/usbip/usbip_common.c b/drivers/staging/usbip/usbip_common.c
index 433a3b6..127fdbb 100644
--- a/drivers/staging/usbip/usbip_common.c
+++ b/drivers/staging/usbip/usbip_common.c
@@ -43,7 +43,7 @@ EXPORT_SYMBOL_GPL(dev_attr_usbip_debug);
 static ssize_t show_flag(struct device *dev, struct device_attribute *attr,
 			 char *buf)
 {
-	return sprintf(buf, "%lx\n", usbip_debug_flag);
+	return scnprintf(buf, PAGE_SIZE, "%lx\n", usbip_debug_flag);
 }

 static ssize_t store_flag(struct device *dev, struct device_attribute *attr,
diff --git a/drivers/staging/usbip/vhci_sysfs.c b/drivers/staging/usbip/vhci_sysfs.c
index d9736f9..1571882 100644
--- a/drivers/staging/usbip/vhci_sysfs.c
+++ b/drivers/staging/usbip/vhci_sysfs.c
@@ -27,10 +27,11 @@

 /* Sysfs entry to show port status */
 static ssize_t show_status(struct device *dev, struct device_attribute *attr,
-			   char *out)
+			   char *buf)
 {
-	char *s = out;
+	char *out = buf;
 	int i = 0;
+	int count;

 	BUG_ON(!the_controller || !out);

@@ -46,32 +47,43 @@ static ssize_t show_status(struct device *dev, struct device_attribute *attr,
 	 * up /proc/net/{tcp,tcp6}. Also, a userland program may remember a
 	 * port number and its peer IP address.
 	 */
-	out += sprintf(out, "prt sta spd bus dev socket           "
-		       "local_busid\n");
+	count = scnprintf(out, PAGE_SIZE,
+		"prt sta spd bus dev socket           local_busid\n");
+	out = buf + count;

 	for (i = 0; i < VHCI_NPORTS; i++) {
 		struct vhci_device *vdev = port_to_vdev(i);

 		spin_lock(&vdev->ud.lock);
-		out += sprintf(out, "%03u %03u ", i, vdev->ud.status);
+		count += scnprintf(out, PAGE_SIZE - count,
+			"%03u %03u ", i, vdev->ud.status);
+		out = buf + count;

 		if (vdev->ud.status == VDEV_ST_USED) {
-			out += sprintf(out, "%03u %08x ",
-				       vdev->speed, vdev->devid);
-			out += sprintf(out, "%16p ", vdev->ud.tcp_socket);
-			out += sprintf(out, "%s", dev_name(&vdev->udev->dev));
+			count += scnprintf(out, PAGE_SIZE - count,
+				"%03u %08x ", vdev->speed, vdev->devid);
+			out = buf + count;
+			count += scnprintf(out, PAGE_SIZE - count,
+				"%16p ", vdev->ud.tcp_socket);
+			out = buf + count;
+			count += scnprintf(out, PAGE_SIZE - count,
+				"%s", dev_name(&vdev->udev->dev));
+			out = buf + count;

 		} else {
-			out += sprintf(out, "000 000 000 0000000000000000 0-0");
+			count += scnprintf(out, PAGE_SIZE - count,
+				"000 000 000 0000000000000000 0-0");
+			out = buf + count;
 		}

-		out += sprintf(out, "\n");
+		count += scnprintf(out, PAGE_SIZE - count, "\n");
+		out = buf + count;
 		spin_unlock(&vdev->ud.lock);
 	}

 	spin_unlock(&the_controller->lock);

-	return out - s;
+	return count;
 }
 static DEVICE_ATTR(status, S_IRUGO, show_status, NULL);

Re: [PATCH] usbip: handle length at sysfs show() functions

[Greg KH]

On Wed, Jun 01, 2011 at 07:14:07AM +0200, Németh Márton wrote:
> From: Márton Németh <nm127@freemail.hu>
> 
> The sysfs show() functions shall return the actual content length of
> the result buffer. According to Documentation/filesystems/sysfs.txt:215
> the scnprintf() function is preferred.
> 
> See also the article titled "snprintf() confusion" at
> http://lwn.net/Articles/69419/ .
> 
> Signed-off-by: Márton Németh <nm127@freemail.hu>

This does not apply anymore, care to redo it?

thanks,

greg k-h

Re: [PATCH] usbip: handle length at sysfs show() functions

[Greg KH]

On Wed, Jun 01, 2011 at 07:14:07AM +0200, Németh Márton wrote:
> From: Márton Németh <nm127@freemail.hu>
> 
> The sysfs show() functions shall return the actual content length of
> the result buffer. According to Documentation/filesystems/sysfs.txt:215
> the scnprintf() function is preferred.
> 
> See also the article titled "snprintf() confusion" at
> http://lwn.net/Articles/69419/ .
> 
> Signed-off-by: Márton Németh <nm127@freemail.hu>
> ---
> diff --git a/drivers/staging/usbip/stub_dev.c b/drivers/staging/usbip/stub_dev.c
> index 6e99ec8..af5f107 100644
> --- a/drivers/staging/usbip/stub_dev.c
> +++ b/drivers/staging/usbip/stub_dev.c
> @@ -80,7 +80,7 @@ static ssize_t show_status(struct device *dev, struct device_attribute *attr,
>  	status = sdev->ud.status;
>  	spin_unlock(&sdev->ud.lock);
> 
> -	return snprintf(buf, PAGE_SIZE, "%d\n", status);
> +	return scnprintf(buf, PAGE_SIZE, "%d\n", status);

Actually, this can be changed to just be:
	return sprintf(buf, "%d\n", status);
as obviously an integer will never be bigger than the sysfs buffer.

Good rule of thumb, if you care about the size of the sysfs buffer, you
are doing something wrong.

Case in point:

> --- a/drivers/staging/usbip/stub_main.c
> +++ b/drivers/staging/usbip/stub_main.c
> @@ -79,18 +79,22 @@ static ssize_t show_match_busid(struct device_driver *drv, char *buf)
>  {
>  	int i;
>  	char *out = buf;
> +	int count = 0;
> 
>  	spin_lock(&busid_table_lock);
> 
>  	for (i = 0; i < MAX_BUSID; i++)
> -		if (busid_table[i].name[0])
> -			out += sprintf(out, "%s ", busid_table[i].name);
> +		if (busid_table[i].name[0]) {
> +			count += scnprintf(out, PAGE_SIZE - count,
> +					"%s ", busid_table[i].name);
> +			out = buf + count;
> +		}

Here we are doing lots of work to try to put more than one value in the
sysfs file, and return the proper data to the kernel about how big the
buffer we used.

That's wrong, and violates the "one value per file" sysfs rule, so that
should be fixed instead of trying to change the sprintf() call.

Same goes for other places in this patch.

Care to redo that instead?

thanks,

greg k-h

Re: [PATCH] usbip: handle length at sysfs show() functions

[Németh Márton]

Greg KH wrote:
> On Wed, Jun 01, 2011 at 07:14:07AM +0200, Németh Márton wrote:
>> The sysfs show() functions shall return the actual content length of
>> the result buffer. According to Documentation/filesystems/sysfs.txt:215
>> the scnprintf() function is preferred.
>>
>> See also the article titled "snprintf() confusion" at
>> http://lwn.net/Articles/69419/ .
>
> [...]
>
> Here we are doing lots of work to try to put more than one value in the
> sysfs file, and return the proper data to the kernel about how big the
> buffer we used.
> 
> That's wrong, and violates the "one value per file" sysfs rule, so that
> should be fixed instead of trying to change the sprintf() call.

As I understand there is a need to change the design here. Currently I
get the following content when vhci-hcd is loaded but not yet used:

$ cat /sys/devices/platform/vhci_hcd/status
prt sta spd bus dev socket           local_busid
000 004 000 000 000 0000000000000000 0-0
001 004 000 000 000 0000000000000000 0-0
002 004 000 000 000 0000000000000000 0-0
003 004 000 000 000 0000000000000000 0-0
004 004 000 000 000 0000000000000000 0-0
005 004 000 000 000 0000000000000000 0-0
006 004 000 000 000 0000000000000000 0-0
007 004 000 000 000 0000000000000000 0-0

The fields are: port, status, speed, device ID, socket pointer and
local busid name. This is too complex for sysfs. Maybe we could extend
the devices file of usbfs with some new rows?

The current output of the devices file already contains some fields
with same name, but I don't have the overview to see whether this
could replace the current /sys/devices/platform/vhci_hcd/status file.

$ cat /proc/bus/usb/devices
[...]

T:  Bus=02 Lev=01 Prnt=01 Port=00 Cnt=01 Dev#=  3 Spd=1.5  MxCh= 0
D:  Ver= 1.10 Cls=00(>ifc ) Sub=00 Prot=00 MxPS= 8 #Cfgs=  1
P:  Vendor=0458 ProdID=003a Rev= 1.00
S:  Manufacturer=Genius
S:  Product=Optical Mouse
C:* #Ifs= 1 Cfg#= 1 Atr=a0 MxPwr=100mA
I:* If#= 0 Alt= 0 #EPs= 1 Cls=03(HID  ) Sub=01 Prot=02 Driver=usbhid
E:  Ad=81(I) Atr=03(Int.) MxPS=   4 Ivl=10ms

[...]

Regards,

	Márton Németh

Re: [PATCH] usbip: handle length at sysfs show() functions

[Greg KH]

On Wed, Jun 08, 2011 at 07:26:58AM +0200, Németh Márton wrote:
> Greg KH wrote:
> > On Wed, Jun 01, 2011 at 07:14:07AM +0200, Németh Márton wrote:
> >> The sysfs show() functions shall return the actual content length of
> >> the result buffer. According to Documentation/filesystems/sysfs.txt:215
> >> the scnprintf() function is preferred.
> >>
> >> See also the article titled "snprintf() confusion" at
> >> http://lwn.net/Articles/69419/ .
> >
> > [...]
> >
> > Here we are doing lots of work to try to put more than one value in the
> > sysfs file, and return the proper data to the kernel about how big the
> > buffer we used.
> > 
> > That's wrong, and violates the "one value per file" sysfs rule, so that
> > should be fixed instead of trying to change the sprintf() call.
> 
> As I understand there is a need to change the design here. Currently I
> get the following content when vhci-hcd is loaded but not yet used:
> 
> $ cat /sys/devices/platform/vhci_hcd/status
> prt sta spd bus dev socket           local_busid
> 000 004 000 000 000 0000000000000000 0-0
> 001 004 000 000 000 0000000000000000 0-0
> 002 004 000 000 000 0000000000000000 0-0
> 003 004 000 000 000 0000000000000000 0-0
> 004 004 000 000 000 0000000000000000 0-0
> 005 004 000 000 000 0000000000000000 0-0
> 006 004 000 000 000 0000000000000000 0-0
> 007 004 000 000 000 0000000000000000 0-0
> 
> The fields are: port, status, speed, device ID, socket pointer and
> local busid name. This is too complex for sysfs. Maybe we could extend
> the devices file of usbfs with some new rows?

Ick, I doubt it as there are lots of tools that parse that file already.

But yes, you are correct, this should not be in sysfs at all.

What's the use for this file?  Who uses it?  Is it just debugging
output?  Information for people to gaze at if they feel like it?
Something else?

Once we figure that out, then we can determine where it should go
(debugfs, sysfs in a different file format, usbfs, etc.)

thanks,

greg k-h

Re: [PATCH] usbip: handle length at sysfs show() functions

[Németh Márton]

Greg KH wrote:
> On Wed, Jun 08, 2011 at 07:26:58AM +0200, Németh Márton wrote:
>> Greg KH wrote:
>>> On Wed, Jun 01, 2011 at 07:14:07AM +0200, Németh Márton wrote:
>>>> The sysfs show() functions shall return the actual content length of
>>>> the result buffer. According to Documentation/filesystems/sysfs.txt:215
>>>> the scnprintf() function is preferred.
>>>>
>>>> See also the article titled "snprintf() confusion" at
>>>> http://lwn.net/Articles/69419/ .
>>> [...]
>>>
>>> Here we are doing lots of work to try to put more than one value in the
>>> sysfs file, and return the proper data to the kernel about how big the
>>> buffer we used.
>>>
>>> That's wrong, and violates the "one value per file" sysfs rule, so that
>>> should be fixed instead of trying to change the sprintf() call.
>> As I understand there is a need to change the design here. Currently I
>> get the following content when vhci-hcd is loaded but not yet used:
>>
>> $ cat /sys/devices/platform/vhci_hcd/status
>> prt sta spd bus dev socket           local_busid
>> 000 004 000 000 000 0000000000000000 0-0
>> 001 004 000 000 000 0000000000000000 0-0
>> 002 004 000 000 000 0000000000000000 0-0
>> 003 004 000 000 000 0000000000000000 0-0
>> 004 004 000 000 000 0000000000000000 0-0
>> 005 004 000 000 000 0000000000000000 0-0
>> 006 004 000 000 000 0000000000000000 0-0
>> 007 004 000 000 000 0000000000000000 0-0
>>
>> The fields are: port, status, speed, device ID, socket pointer and
>> local busid name. This is too complex for sysfs. Maybe we could extend
>> the devices file of usbfs with some new rows?
> 
> Ick, I doubt it as there are lots of tools that parse that file already.

usbip is still part of the staging directory. In dmesg the following appear:

| usbip_common_mod: module is from the staging directory, the quality is unknown, you have been warned.
| usbip_common_mod: usbip common driver1.0
| vhci_hcd: module is from the staging directory, the quality is unknown, you have been warned.

so this means that usbip is a work-in-progress, it might be changed anytime. On
the other hand we can do this nice way: a new entry in  Documentation/feature-removal-schedule.txt
for /sys/devices/platform/vhci_hcd/status file removal, let's say it will be
removed before the usbip goes to mainline. In parallel the new interface
can be developed.

> But yes, you are correct, this should not be in sysfs at all.
> 
> What's the use for this file?  Who uses it?  Is it just debugging
> output?  Information for people to gaze at if they feel like it?
> Something else?

Based on the user space source code at drivers/staging/usbip/userspace/
I can identify the following usages:

libsrc/vhci_driver.c::get_nports():
 - finding out how many ports the VHCI has

libsrc/vhci_driver.c::parse_status():
 - VHCI port number to identify virtual ports
 - fetching the status of each VHCI ports whether it is
     - vdev does not connect a remote device: (status = VDEV_ST_NULL = 4):
       "Port Available"
     - vdev is used, but the USB address is not assigned yet (status =
       VDEV_ST_NOTASSIGNED = 5): "Port Initializing"
     - used (status = VDEV_ST_USED = 6): "Port in Use"
     - error (VDEV_ST_ERROR = 7): "Port Error"
 - the speed can be unknown/low/full/high/variable
 - it looks like the bus column was merged with the device column but I
   currently cannot find when
 - the device ID is splited to the upper 16bits: bus number, and lower
   16bits: device number
 - based on local_busid the usb device file can be found in /sys using
   sysfs_open_device()

Note that the socket parameter is only printed out as a debug information: it
is not used anywhere.

Maybe most of the file content is redundant, because:

 - we have /sys/bus/usb/devices/usb*/maxchild which is "number of ports if hub"
   according to linux/usb.h:410 ;
 - we have /sys/bus/usb/devices/*-*/speed to identify the device speed;
 - We have already bus number at /sys/bus/usb/devices/usb*/busnum or at
   /sys/bus/usb/devices/*-*/busnum ;
 - we also have /sys/bus/usb/devices/*-*/devnum ;
 - it is possbile to collect all the devices from /sys/bus/usb/devices/*-*
   filtering to the first number to /sys/bus/usb/devices/usb*/busnum .

The only thing which is special for VHCI is the status for each port:
DEV_ST_NULL/VDEV_ST_NOTASSIGNED/VDEV_ST_USED/VDEV_ST_ERROR .

> Once we figure that out, then we can determine where it should go
> (debugfs, sysfs in a different file format, usbfs, etc.)

Maybe the status could fit under /sys/devices/platform/vhci_hcd/*-*/status .
This file could contain only one number showing the status of that single
device.

Regards,

	Márton Németh

Re: [PATCH] usbip: handle length at sysfs show() functions

[Greg KH]

On Wed, Jun 08, 2011 at 11:27:20PM +0200, Németh Márton wrote:
> > Ick, I doubt it as there are lots of tools that parse that file already.
> 
> usbip is still part of the staging directory. In dmesg the following appear:
> 
> | usbip_common_mod: module is from the staging directory, the quality is unknown, you have been warned.
> | usbip_common_mod: usbip common driver1.0
> | vhci_hcd: module is from the staging directory, the quality is unknown, you have been warned.
> 
> so this means that usbip is a work-in-progress, it might be changed anytime. On
> the other hand we can do this nice way: a new entry in  Documentation/feature-removal-schedule.txt
> for /sys/devices/platform/vhci_hcd/status file removal, let's say it will be
> removed before the usbip goes to mainline. In parallel the new interface
> can be developed.

Or we can just fix it properly, as we have the userspace tools in the
kernel now as well, and the interface is obviously broken.  That's what
being in the staging tree allows us to do.

> > But yes, you are correct, this should not be in sysfs at all.
> > 
> > What's the use for this file?  Who uses it?  Is it just debugging
> > output?  Information for people to gaze at if they feel like it?
> > Something else?
> 
> Based on the user space source code at drivers/staging/usbip/userspace/
> I can identify the following usages:
> 
> libsrc/vhci_driver.c::get_nports():
>  - finding out how many ports the VHCI has

Is that really necessary as they are just "virtual" ports :)
We can put that in a single sysfs file if needed.

> libsrc/vhci_driver.c::parse_status():
>  - VHCI port number to identify virtual ports
>  - fetching the status of each VHCI ports whether it is
>      - vdev does not connect a remote device: (status = VDEV_ST_NULL = 4):
>        "Port Available"
>      - vdev is used, but the USB address is not assigned yet (status =
>        VDEV_ST_NOTASSIGNED = 5): "Port Initializing"
>      - used (status = VDEV_ST_USED = 6): "Port in Use"
>      - error (VDEV_ST_ERROR = 7): "Port Error"
>  - the speed can be unknown/low/full/high/variable
>  - it looks like the bus column was merged with the device column but I
>    currently cannot find when
>  - the device ID is splited to the upper 16bits: bus number, and lower
>    16bits: device number
>  - based on local_busid the usb device file can be found in /sys using
>    sysfs_open_device()

All of those can be placed in individual files under the different port
directories, so we should be fine.

> Note that the socket parameter is only printed out as a debug information: it
> is not used anywhere.
> 
> Maybe most of the file content is redundant, because:
> 
>  - we have /sys/bus/usb/devices/usb*/maxchild which is "number of ports if hub"
>    according to linux/usb.h:410 ;

Yes.

>  - we have /sys/bus/usb/devices/*-*/speed to identify the device speed;

Yes.

>  - We have already bus number at /sys/bus/usb/devices/usb*/busnum or at
>    /sys/bus/usb/devices/*-*/busnum ;

Yes.

>  - we also have /sys/bus/usb/devices/*-*/devnum ;
>  - it is possbile to collect all the devices from /sys/bus/usb/devices/*-*
>    filtering to the first number to /sys/bus/usb/devices/usb*/busnum .
> 
> The only thing which is special for VHCI is the status for each port:
> DEV_ST_NULL/VDEV_ST_NOTASSIGNED/VDEV_ST_USED/VDEV_ST_ERROR .

So we add a status file and we are set.

Anyone care to send patches to fix this all up?

thanks,

greg k-h

Re: [PATCH] usbip: handle length at sysfs show() functions

[matt mooney]

On 23:27 Wed 08 Jun     , N?meth M?rton wrote:
> Greg KH wrote:
> > On Wed, Jun 08, 2011 at 07:26:58AM +0200, N?meth M?rton wrote:
> >> Greg KH wrote:
> >>> On Wed, Jun 01, 2011 at 07:14:07AM +0200, N?meth M?rton wrote:
> >>>> The sysfs show() functions shall return the actual content length of
> >>>> the result buffer. According to Documentation/filesystems/sysfs.txt:215
> >>>> the scnprintf() function is preferred.
> >>>>
> >>>> See also the article titled "snprintf() confusion" at
> >>>> http://lwn.net/Articles/69419/ .
> >>> [...]
> >>>
> >>> Here we are doing lots of work to try to put more than one value in the
> >>> sysfs file, and return the proper data to the kernel about how big the
> >>> buffer we used.
> >>>
> >>> That's wrong, and violates the "one value per file" sysfs rule, so that
> >>> should be fixed instead of trying to change the sprintf() call.
> >> As I understand there is a need to change the design here. Currently I
> >> get the following content when vhci-hcd is loaded but not yet used:
> >>
> >> $ cat /sys/devices/platform/vhci_hcd/status
> >> prt sta spd bus dev socket           local_busid
> >> 000 004 000 000 000 0000000000000000 0-0
> >> 001 004 000 000 000 0000000000000000 0-0
> >> 002 004 000 000 000 0000000000000000 0-0
> >> 003 004 000 000 000 0000000000000000 0-0
> >> 004 004 000 000 000 0000000000000000 0-0
> >> 005 004 000 000 000 0000000000000000 0-0
> >> 006 004 000 000 000 0000000000000000 0-0
> >> 007 004 000 000 000 0000000000000000 0-0
> >>
> >> The fields are: port, status, speed, device ID, socket pointer and
> >> local busid name. This is too complex for sysfs. Maybe we could extend
> >> the devices file of usbfs with some new rows?
> > 
> > Ick, I doubt it as there are lots of tools that parse that file already.
> 
> usbip is still part of the staging directory. In dmesg the following appear:
> 
> | usbip_common_mod: module is from the staging directory, the quality is unknown, you have been warned.
> | usbip_common_mod: usbip common driver1.0
> | vhci_hcd: module is from the staging directory, the quality is unknown, you have been warned.
> 
> so this means that usbip is a work-in-progress, it might be changed anytime. On
> the other hand we can do this nice way: a new entry in  Documentation/feature-removal-schedule.txt
> for /sys/devices/platform/vhci_hcd/status file removal, let's say it will be
> removed before the usbip goes to mainline. In parallel the new interface
> can be developed.
> 
> > But yes, you are correct, this should not be in sysfs at all.
> >
> > What's the use for this file?  Who uses it?  Is it just debugging
> > output?  Information for people to gaze at if they feel like it?
> > Something else?
> 
> Based on the user space source code at drivers/staging/usbip/userspace/
> I can identify the following usages:
> 
> libsrc/vhci_driver.c::get_nports():
>  - finding out how many ports the VHCI has
> 
> libsrc/vhci_driver.c::parse_status():
>  - VHCI port number to identify virtual ports
>  - fetching the status of each VHCI ports whether it is
>      - vdev does not connect a remote device: (status = VDEV_ST_NULL = 4):
>        "Port Available"
>      - vdev is used, but the USB address is not assigned yet (status =
>        VDEV_ST_NOTASSIGNED = 5): "Port Initializing"
>      - used (status = VDEV_ST_USED = 6): "Port in Use"
>      - error (VDEV_ST_ERROR = 7): "Port Error"
>  - the speed can be unknown/low/full/high/variable
>  - it looks like the bus column was merged with the device column but I
>    currently cannot find when
>  - the device ID is splited to the upper 16bits: bus number, and lower
>    16bits: device number
>  - based on local_busid the usb device file can be found in /sys using
>    sysfs_open_device()
> 
> Note that the socket parameter is only printed out as a debug information: it
> is not used anywhere.
> 
> Maybe most of the file content is redundant, because:
> 
>  - we have /sys/bus/usb/devices/usb*/maxchild which is "number of ports if hub"
>    according to linux/usb.h:410 ;
>  - we have /sys/bus/usb/devices/*-*/speed to identify the device speed;
>  - We have already bus number at /sys/bus/usb/devices/usb*/busnum or at
>    /sys/bus/usb/devices/*-*/busnum ;
>  - we also have /sys/bus/usb/devices/*-*/devnum ;
>  - it is possbile to collect all the devices from /sys/bus/usb/devices/*-*
>    filtering to the first number to /sys/bus/usb/devices/usb*/busnum .
> 
> The only thing which is special for VHCI is the status for each port:
> DEV_ST_NULL/VDEV_ST_NOTASSIGNED/VDEV_ST_USED/VDEV_ST_ERROR .
> 
> > Once we figure that out, then we can determine where it should go
> > (debugfs, sysfs in a different file format, usbfs, etc.)
> 
> Maybe the status could fit under /sys/devices/platform/vhci_hcd/*-*/status .
> This file could contain only one number showing the status of that single
> device.

This is something I have been meaning to get to. So yes, I agree we should
eliminate the bulk of that sysfs file and use the ../vhci_hcd/*-*/status
structure.

-mfm

Re: [PATCH] usbip: handle length at sysfs show() functions

[matt mooney]

On 18:28 Mon 27 Jun     , David Chang wrote:
> Hi,
> 
> 2011/6/9 Greg KH <greg@kroah.com>
> 
> > On Wed, Jun 08, 2011 at 11:27:20PM +0200, N?meth M?rton wrote:
> > > > Ick, I doubt it as there are lots of tools that parse that file
> > already.
> > >
> > > usbip is still part of the staging directory. In dmesg the following
> > appear:
> > >
> > > | usbip_common_mod: module is from the staging directory, the quality is
> > unknown, you have been warned.
> > > | usbip_common_mod: usbip common driver1.0
> > > | vhci_hcd: module is from the staging directory, the quality is unknown,
> > you have been warned.
> > >
> > > so this means that usbip is a work-in-progress, it might be changed
> > anytime. On
> > > the other hand we can do this nice way: a new entry in
> >  Documentation/feature-removal-schedule.txt
> > > for /sys/devices/platform/vhci_hcd/status file removal, let's say it will
> > be
> > > removed before the usbip goes to mainline. In parallel the new interface
> > > can be developed.
> >
> > Or we can just fix it properly, as we have the userspace tools in the
> > kernel now as well, and the interface is obviously broken.  That's what
> > being in the staging tree allows us to do.
> >
> >
> >
> > > But yes, you are correct, this should not be in sysfs at all.
> > > >
> > > > What's the use for this file?  Who uses it?  Is it just debugging
> > > > output?  Information for people to gaze at if they feel like it?
> > > > Something else?
> > >
> > > Based on the user space source code at drivers/staging/usbip/userspace/
> > > I can identify the following usages:
> > >
> > > libsrc/vhci_driver.c::get_nports():
> > >  - finding out how many ports the VHCI has
> >
> > Is that really necessary as they are just "virtual" ports :)
> > We can put that in a single sysfs file if needed.
> >
> > > libsrc/vhci_driver.c::parse_status():
> > >  - VHCI port number to identify virtual ports
> > >  - fetching the status of each VHCI ports whether it is
> > >      - vdev does not connect a remote device: (status = VDEV_ST_NULL =
> > 4):
> > >        "Port Available"
> > >      - vdev is used, but the USB address is not assigned yet (status =
> > >        VDEV_ST_NOTASSIGNED = 5): "Port Initializing"
> > >      - used (status = VDEV_ST_USED = 6): "Port in Use"
> > >      - error (VDEV_ST_ERROR = 7): "Port Error"
> > >  - the speed can be unknown/low/full/high/variable
> > >  - it looks like the bus column was merged with the device column but I
> > >    currently cannot find when
> > >  - the device ID is splited to the upper 16bits: bus number, and lower
> > >    16bits: device number
> > >  - based on local_busid the usb device file can be found in /sys using
> > >    sysfs_open_device()
> >
> > All of those can be placed in individual files under the different port
> > directories, so we should be fine.
> >
> 
> I would like to help on this. :)
> 
> And I just want to make sure that I understand your discussion.
> 
> 1. remove current port status table file (
> /sys/devices/platform/vhci_hcd/status )
> 2. create each port in path "/sys/devices/platform/vhci-hcd" as a directory
> ( /sys/devices/platform/vhci_hcd/ports/[0][0-7] )
> 3. put the port info/status files into each port directory (
> /sys/devices/platform/vhci_hcd/ports/*/status )
> 
> Any suggestions are welcome, thanks!
> 

I think it seems like ../ports/.. is unnecessary. We should use
../vhci_hcd/*-*/status as suggested by Nemeth. I do believe that we will
probably need to encode the vhci-hcd port number in there somewhere to allow
remote devices on different machines but same busid to be imported.

Nemeth are you already working on this?

I need to add the `usbip port' command. I will add a template with some basic
functionality but will wait on the actual port access.

-mfm

> Regards,
> David Chang
> 
> 
> >
> > > Note that the socket parameter is only printed out as a debug
> > information: it
> > > is not used anywhere.
> > >
> > > Maybe most of the file content is redundant, because:
> > >
> > >  - we have /sys/bus/usb/devices/usb*/maxchild which is "number of ports
> > if hub"
> > >    according to linux/usb.h:410 ;
> >
> > Yes.
> >
> > >  - we have /sys/bus/usb/devices/*-*/speed to identify the device speed;
> >
> > Yes.
> >
> > >  - We have already bus number at /sys/bus/usb/devices/usb*/busnum or at
> > >    /sys/bus/usb/devices/*-*/busnum ;
> >
> > Yes.
> >
> > >  - we also have /sys/bus/usb/devices/*-*/devnum ;
> > >  - it is possbile to collect all the devices from
> > /sys/bus/usb/devices/*-*
> > >    filtering to the first number to /sys/bus/usb/devices/usb*/busnum .
> > >
> > > The only thing which is special for VHCI is the status for each port:
> > > DEV_ST_NULL/VDEV_ST_NOTASSIGNED/VDEV_ST_USED/VDEV_ST_ERROR .
> >
> > So we add a status file and we are set.
> >
> > Anyone care to send patches to fix this all up?
> >
> > thanks,
> >
> > greg k-h
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/
> >

Re: [PATCH] usbip: handle length at sysfs show() functions

[Németh Márton]

matt mooney írta:
> On 18:28 Mon 27 Jun     , David Chang wrote:
>> Hi,
>>
>> 2011/6/9 Greg KH <greg@kroah.com>
>>
>>> On Wed, Jun 08, 2011 at 11:27:20PM +0200, N?meth M?rton wrote:
>>>>> Ick, I doubt it as there are lots of tools that parse that file
>>> already.
>>>> usbip is still part of the staging directory. In dmesg the following
>>> appear:
>>>> | usbip_common_mod: module is from the staging directory, the quality is
>>> unknown, you have been warned.
>>>> | usbip_common_mod: usbip common driver1.0
>>>> | vhci_hcd: module is from the staging directory, the quality is unknown,
>>> you have been warned.
>>>> so this means that usbip is a work-in-progress, it might be changed
>>> anytime. On
>>>> the other hand we can do this nice way: a new entry in
>>>  Documentation/feature-removal-schedule.txt
>>>> for /sys/devices/platform/vhci_hcd/status file removal, let's say it will
>>> be
>>>> removed before the usbip goes to mainline. In parallel the new interface
>>>> can be developed.
>>> Or we can just fix it properly, as we have the userspace tools in the
>>> kernel now as well, and the interface is obviously broken.  That's what
>>> being in the staging tree allows us to do.
>>>
>>>
>>>
>>>> But yes, you are correct, this should not be in sysfs at all.
>>>>> What's the use for this file?  Who uses it?  Is it just debugging
>>>>> output?  Information for people to gaze at if they feel like it?
>>>>> Something else?
>>>> Based on the user space source code at drivers/staging/usbip/userspace/
>>>> I can identify the following usages:
>>>>
>>>> libsrc/vhci_driver.c::get_nports():
>>>>  - finding out how many ports the VHCI has
>>> Is that really necessary as they are just "virtual" ports :)
>>> We can put that in a single sysfs file if needed.
>>>
>>>> libsrc/vhci_driver.c::parse_status():
>>>>  - VHCI port number to identify virtual ports
>>>>  - fetching the status of each VHCI ports whether it is
>>>>      - vdev does not connect a remote device: (status = VDEV_ST_NULL =
>>> 4):
>>>>        "Port Available"
>>>>      - vdev is used, but the USB address is not assigned yet (status =
>>>>        VDEV_ST_NOTASSIGNED = 5): "Port Initializing"
>>>>      - used (status = VDEV_ST_USED = 6): "Port in Use"
>>>>      - error (VDEV_ST_ERROR = 7): "Port Error"
>>>>  - the speed can be unknown/low/full/high/variable
>>>>  - it looks like the bus column was merged with the device column but I
>>>>    currently cannot find when
>>>>  - the device ID is splited to the upper 16bits: bus number, and lower
>>>>    16bits: device number
>>>>  - based on local_busid the usb device file can be found in /sys using
>>>>    sysfs_open_device()
>>> All of those can be placed in individual files under the different port
>>> directories, so we should be fine.
>>>
>> I would like to help on this. :)
>>
>> And I just want to make sure that I understand your discussion.
>>
>> 1. remove current port status table file (
>> /sys/devices/platform/vhci_hcd/status )
>> 2. create each port in path "/sys/devices/platform/vhci-hcd" as a directory
>> ( /sys/devices/platform/vhci_hcd/ports/[0][0-7] )
>> 3. put the port info/status files into each port directory (
>> /sys/devices/platform/vhci_hcd/ports/*/status )
>>
>> Any suggestions are welcome, thanks!
>>
> 
> I think it seems like ../ports/.. is unnecessary. We should use
> ../vhci_hcd/*-*/status as suggested by Nemeth. I do believe that we will
> probably need to encode the vhci-hcd port number in there somewhere to allow
> remote devices on different machines but same busid to be imported.
> 
> Nemeth are you already working on this?

No, I'm not.

> I need to add the `usbip port' command. I will add a template with some basic
> functionality but will wait on the actual port access.
> 
> -mfm
> 
>> Regards,
>> David Chang
>>
>>
>>>> Note that the socket parameter is only printed out as a debug
>>> information: it
>>>> is not used anywhere.
>>>>
>>>> Maybe most of the file content is redundant, because:
>>>>
>>>>  - we have /sys/bus/usb/devices/usb*/maxchild which is "number of ports
>>> if hub"
>>>>    according to linux/usb.h:410 ;
>>> Yes.
>>>
>>>>  - we have /sys/bus/usb/devices/*-*/speed to identify the device speed;
>>> Yes.
>>>
>>>>  - We have already bus number at /sys/bus/usb/devices/usb*/busnum or at
>>>>    /sys/bus/usb/devices/*-*/busnum ;
>>> Yes.
>>>
>>>>  - we also have /sys/bus/usb/devices/*-*/devnum ;
>>>>  - it is possbile to collect all the devices from
>>> /sys/bus/usb/devices/*-*
>>>>    filtering to the first number to /sys/bus/usb/devices/usb*/busnum .
>>>>
>>>> The only thing which is special for VHCI is the status for each port:
>>>> DEV_ST_NULL/VDEV_ST_NOTASSIGNED/VDEV_ST_USED/VDEV_ST_ERROR .
>>> So we add a status file and we are set.
>>>
>>> Anyone care to send patches to fix this all up?
>>>
>>> thanks,
>>>
>>> greg k-h
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>> Please read the FAQ at  http://www.tux.org/lkml/
>>>
> 
> 

Re: [PATCH] usbip: handle length at sysfs show() functions

[Matt Chen]

Hi Matt,
So you are working on "usbip port" command now ?
Because I found no way to look up any port which is bound to remote
device in usbip command.

2011/6/29 matt mooney <mfm@muteddisk.com>:
> On 18:28 Mon 27 Jun     , David Chang wrote:
>> Hi,
>>
>> 2011/6/9 Greg KH <greg@kroah.com>
>>
>> > On Wed, Jun 08, 2011 at 11:27:20PM +0200, N?meth M?rton wrote:
>> > > > Ick, I doubt it as there are lots of tools that parse that file
>> > already.
>> > >
>> > > usbip is still part of the staging directory. In dmesg the following
>> > appear:
>> > >
>> > > | usbip_common_mod: module is from the staging directory, the quality is
>> > unknown, you have been warned.
>> > > | usbip_common_mod: usbip common driver1.0
>> > > | vhci_hcd: module is from the staging directory, the quality is unknown,
>> > you have been warned.
>> > >
>> > > so this means that usbip is a work-in-progress, it might be changed
>> > anytime. On
>> > > the other hand we can do this nice way: a new entry in
>> >  Documentation/feature-removal-schedule.txt
>> > > for /sys/devices/platform/vhci_hcd/status file removal, let's say it will
>> > be
>> > > removed before the usbip goes to mainline. In parallel the new interface
>> > > can be developed.
>> >
>> > Or we can just fix it properly, as we have the userspace tools in the
>> > kernel now as well, and the interface is obviously broken.  That's what
>> > being in the staging tree allows us to do.
>> >
>> >
>> >
>> > > But yes, you are correct, this should not be in sysfs at all.
>> > > >
>> > > > What's the use for this file?  Who uses it?  Is it just debugging
>> > > > output?  Information for people to gaze at if they feel like it?
>> > > > Something else?
>> > >
>> > > Based on the user space source code at drivers/staging/usbip/userspace/
>> > > I can identify the following usages:
>> > >
>> > > libsrc/vhci_driver.c::get_nports():
>> > >  - finding out how many ports the VHCI has
>> >
>> > Is that really necessary as they are just "virtual" ports :)
>> > We can put that in a single sysfs file if needed.
>> >
>> > > libsrc/vhci_driver.c::parse_status():
>> > >  - VHCI port number to identify virtual ports
>> > >  - fetching the status of each VHCI ports whether it is
>> > >      - vdev does not connect a remote device: (status = VDEV_ST_NULL =
>> > 4):
>> > >        "Port Available"
>> > >      - vdev is used, but the USB address is not assigned yet (status =
>> > >        VDEV_ST_NOTASSIGNED = 5): "Port Initializing"
>> > >      - used (status = VDEV_ST_USED = 6): "Port in Use"
>> > >      - error (VDEV_ST_ERROR = 7): "Port Error"
>> > >  - the speed can be unknown/low/full/high/variable
>> > >  - it looks like the bus column was merged with the device column but I
>> > >    currently cannot find when
>> > >  - the device ID is splited to the upper 16bits: bus number, and lower
>> > >    16bits: device number
>> > >  - based on local_busid the usb device file can be found in /sys using
>> > >    sysfs_open_device()
>> >
>> > All of those can be placed in individual files under the different port
>> > directories, so we should be fine.
>> >
>>
>> I would like to help on this. :)
>>
>> And I just want to make sure that I understand your discussion.
>>
>> 1. remove current port status table file (
>> /sys/devices/platform/vhci_hcd/status )
>> 2. create each port in path "/sys/devices/platform/vhci-hcd" as a directory
>> ( /sys/devices/platform/vhci_hcd/ports/[0][0-7] )
>> 3. put the port info/status files into each port directory (
>> /sys/devices/platform/vhci_hcd/ports/*/status )
>>
>> Any suggestions are welcome, thanks!
>>
>
> I think it seems like ../ports/.. is unnecessary. We should use
> ../vhci_hcd/*-*/status as suggested by Nemeth. I do believe that we will
> probably need to encode the vhci-hcd port number in there somewhere to allow
> remote devices on different machines but same busid to be imported.
>
> Nemeth are you already working on this?
>
> I need to add the `usbip port' command. I will add a template with some basic
> functionality but will wait on the actual port access.
>
> -mfm
>
>> Regards,
>> David Chang
>>
>>
>> >
>> > > Note that the socket parameter is only printed out as a debug
>> > information: it
>> > > is not used anywhere.
>> > >
>> > > Maybe most of the file content is redundant, because:
>> > >
>> > >  - we have /sys/bus/usb/devices/usb*/maxchild which is "number of ports
>> > if hub"
>> > >    according to linux/usb.h:410 ;
>> >
>> > Yes.
>> >
>> > >  - we have /sys/bus/usb/devices/*-*/speed to identify the device speed;
>> >
>> > Yes.
>> >
>> > >  - We have already bus number at /sys/bus/usb/devices/usb*/busnum or at
>> > >    /sys/bus/usb/devices/*-*/busnum ;
>> >
>> > Yes.
>> >
>> > >  - we also have /sys/bus/usb/devices/*-*/devnum ;
>> > >  - it is possbile to collect all the devices from
>> > /sys/bus/usb/devices/*-*
>> > >    filtering to the first number to /sys/bus/usb/devices/usb*/busnum .
>> > >
>> > > The only thing which is special for VHCI is the status for each port:
>> > > DEV_ST_NULL/VDEV_ST_NOTASSIGNED/VDEV_ST_USED/VDEV_ST_ERROR .
>> >
>> > So we add a status file and we are set.
>> >
>> > Anyone care to send patches to fix this all up?
>> >
>> > thanks,
>> >
>> > greg k-h
>> > --
>> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> > the body of a message to majordomo@vger.kernel.org
>> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> > Please read the FAQ at  http://www.tux.org/lkml/
>> >
> _______________________________________________
> devel mailing list
> devel@linuxdriverproject.org
> http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
>



-- 
Thanks ~
Matt Chen