usbip: handle length at sysfs show() functions
PATCH 1/1 usbip: handle length at sysfs show() functions
From: Márton Németh <[email protected]>
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/ .
---
drivers/staging/usbip/stub_dev.c +1 -1
@@ -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);
drivers/staging/usbip/stub_main.c +8 -4
@@ -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)
drivers/staging/usbip/usbip_common.c +1 -1
@@ -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,
drivers/staging/usbip/vhci_sysfs.c +24 -12
@@ -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);
Quoted 10 lines · Németh Márton
From: Márton Németh <[email protected]>
The sysfs
show()functions shall return the actual content length of the result buffer. According toDocumentation/filesystems/sysfs.txt:215thescnprintf()function is preferred.See also the article titled "
snprintf()confusion" at
http://lwn.net/Articles/69419/ .Signed-off-by Márton Németh <[email protected]>
This does not apply anymore, care to redo it?
thanks,
greg k-h
Quoted 16 lines · Németh Márton
From: Márton Németh <[email protected]>
The sysfs
show()functions shall return the actual content length of the result buffer. According toDocumentation/filesystems/sysfs.txt:215thescnprintf()function is preferred.See also the article titled "
snprintf()confusion" at
http://lwn.net/Articles/69419/ .Signed-off-by Márton Németh <[email protected]>---
drivers/staging/usbip/stub_dev.c +0
-0
drivers/staging/usbip/stub_dev.c static ssize_t show_status(struct device *dev, struct device_attribute *attr, · line 83
Quoted 5 lines
drivers/staging/usbip/stub_dev.c +1
-1@@ -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:
Quoted 8 lines
drivers/staging/usbip/stub_main.c +1
-0@@ -79,18 +79,22 @@ static ssize_t show_match_busid(struct device_driver *drv, char *buf) {int i;char *out = buf; +int count =0 ;
drivers/staging/usbip/stub_main.c static ssize_t show_match_busid(struct device_driver *drv, char *buf) · line 91
Quoted 10 lines
drivers/staging/usbip/stub_main.c +5
-2spin_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
Quoted 8 lines · Greg KH
Quoted 6 lines · Németh Márton
The sysfs
show()functions shall return the actual content length of the result buffer. According toDocumentation/filesystems/sysfs.txt:215thescnprintf()function is preferred.See also the article titled "
snprintf()confusion" at
http://lwn.net/Articles/69419/ .
Quoted 8 lines
[...]
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
Quoted 32 lines · Németh Márton
Quoted 8 lines · Greg KH
Quoted 6 lines · Németh Márton
The sysfs
show()functions shall return the actual content length of the result buffer. According toDocumentation/filesystems/sysfs.txt:215thescnprintf()function is preferred.See also the article titled "
snprintf()confusion" at
http://lwn.net/Articles/69419/ .Quoted 8 lines
[...]
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
Quoted 3 lines
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
Quoted 31 lines · Greg KH
Quoted 30 lines · Németh Márton
Quoted 7 lines · Greg KH
Quoted 6 lines · Németh Márton
The sysfs
show()functions shall return the actual content length of the result buffer. According toDocumentation/filesystems/sysfs.txt:215thescnprintf()function is preferred.See also the article titled "
snprintf()confusion" at
http://lwn.net/Articles/69419/ .Quoted 8 lines
[...]
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
Quoted 5 lines
Quoted 3 lines
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.
Quoted 5 lines
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 .
Quoted 2 lines
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
Quoted 8 lines · Németh Márton
Quoted 1 line
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.
Quoted 5 lines
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.txtfor/sys/devices/platform/vhci_hcd/statusfile 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.
Quoted 11 lines
Quoted 5 lines
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.
Quoted 8 lines
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"
Quoted 8 lines
- 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 onlocal_busidthe usb device file can be found in /sys usingsysfs_open_device()
All of those can be placed in individual files under the different port directories, so we should be fine.
Quoted 7 lines
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*/maxchildwhich is "number of ports if hub" according tolinux/usb.h:410;
Yes.
Quoted 1 line
- we have
/sys/bus/usb/devices/*-*/speedto identify the device speed;
Yes.
Quoted 2 lines
- We have already bus number at
/sys/bus/usb/devices/usb*/busnumor at/sys/bus/usb/devices/*-*/busnum;
Yes.
Quoted 6 lines
- 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
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 73 lines · David Chang
Hi,
2011/6/9 Greg KH <[email protected]>
Quoted 61 lines · N?meth M?rton
Quoted 1 line
Quoted 1 line
Ick, I doubt it as there are lots of tools that parse that file
already.
Quoted 2 lines
usbip is still part of the staging directory. In dmesg the following
appear:
Quoted 2 lines
|
usbip_common_mod: module is from the staging directory, the quality isunknown, you have been warned.
Quoted 2 lines
|
usbip_common_mod: usbip common driver1.0
|vhci_hcd: module is from the staging directory, the quality is unknown,you have been warned.
Quoted 2 lines
so this means that usbip is a work-in-progress, it might be changed
anytime. On
Quoted 1 line
the other hand we can do this nice way: a new entry in
Documentation/feature-removal-schedule.txtQuoted 1 line
for
/sys/devices/platform/vhci_hcd/statusfile removal, let's say it willbe
Quoted 2 lines
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.
Quoted 11 lines
But yes, you are correct, this should not be in sysfs at all.
Quoted 4 lines
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 hasIs that really necessary as they are just "virtual" ports :) We can put that in a single sysfs file if needed.
Quoted 4 lines
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):
Quoted 4 lines
"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"Quoted 8 lines
- 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 onlocal_busidthe usb device file can be found in /sys usingsysfs_open_device()Quoted 3 lines
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.
Quoted 9 lines
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
Quoted 47 lines
Regards,
David ChangQuoted 43 lines
Quoted 2 lines
Note that the socket parameter is only printed out as a debug
information: it
Quoted 5 lines
is not used anywhere.
Maybe most of the file content is redundant, because:
- we have
/sys/bus/usb/devices/usb*/maxchildwhich is "number of portsif hub"
Quoted 1 line
according to linux/usb.h:410 ;Yes.
Quoted 1 line
- we have
/sys/bus/usb/devices/*-*/speedto identify the device speed;Yes.
Quoted 2 lines
- We have already bus number at
/sys/bus/usb/devices/usb*/busnumor at/sys/bus/usb/devices/*-*/busnum;Yes.
Quoted 2 lines
- we also have
/sys/bus/usb/devices/*-*/devnum;
- it is possbile to collect all the devices from
/sys/bus/usb/devices/*-*Quoted 4 lines
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
matt mooney írta:
Quoted 75 lines · David Chang
Quoted 64 lines
Hi,
2011/6/9 Greg KH <[email protected]>
Quoted 53 lines · N?meth M?rton
Quoted 1 line
Quoted 1 line
Ick, I doubt it as there are lots of tools that parse that file
already.
Quoted 1 line
usbip is still part of the staging directory. In dmesg the following
appear:
Quoted 1 line
|
usbip_common_mod: module is from the staging directory, the quality isunknown, you have been warned.
Quoted 2 lines
|
usbip_common_mod: usbip common driver1.0
|vhci_hcd: module is from the staging directory, the quality is unknown,you have been warned.
Quoted 1 line
so this means that usbip is a work-in-progress, it might be changed
anytime. On
Quoted 1 line
the other hand we can do this nice way: a new entry in
Documentation/feature-removal-schedule.txtQuoted 1 line
for
/sys/devices/platform/vhci_hcd/statusfile removal, let's say it willbe
Quoted 2 lines
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.
Quoted 9 lines
But yes, you are correct, this should not be in sysfs at all.
Quoted 3 lines
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 hasIs that really necessary as they are just "virtual" ports :) We can put that in a single sysfs file if needed.
Quoted 4 lines
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):
Quoted 4 lines
"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"Quoted 8 lines
- 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 onlocal_busidthe usb device file can be found in /sys usingsysfs_open_device()Quoted 3 lines
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.
Quoted 9 lines
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!
Quoted 6 lines
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.
Quoted 49 lines
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
Quoted 42 lines
Regards,
David ChangQuoted 38 lines
Quoted 1 line
Note that the socket parameter is only printed out as a debug
information: it
Quoted 5 lines
is not used anywhere.
Maybe most of the file content is redundant, because:
- we have
/sys/bus/usb/devices/usb*/maxchildwhich is "number of portsif hub"
Quoted 1 line
according to linux/usb.h:410 ;Yes.
Quoted 1 line
- we have
/sys/bus/usb/devices/*-*/speedto identify the device speed;Yes.
Quoted 2 lines
- We have already bus number at
/sys/bus/usb/devices/usb*/busnumor at/sys/bus/usb/devices/*-*/busnum;Yes.
Quoted 2 lines
- we also have
/sys/bus/usb/devices/*-*/devnum;
- it is possbile to collect all the devices from
/sys/bus/usb/devices/*-*Quoted 4 lines
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
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 <[email protected]>:
Quoted 148 lines · David Chang
Quoted 73 lines
Hi,
2011/6/9 Greg KH <[email protected]>
Quoted 61 lines · N?meth M?rton
Quoted 1 line
Quoted 1 line
Ick, I doubt it as there are lots of tools that parse that file
already.
Quoted 2 lines
usbip is still part of the staging directory. In dmesg the following
appear:
Quoted 2 lines
|
usbip_common_mod: module is from the staging directory, the quality isunknown, you have been warned.
Quoted 2 lines
|
usbip_common_mod: usbip common driver1.0
|vhci_hcd: module is from the staging directory, the quality is unknown,you have been warned.
Quoted 2 lines
so this means that usbip is a work-in-progress, it might be changed
anytime. On
Quoted 1 line
the other hand we can do this nice way: a new entry in
Documentation/feature-removal-schedule.txt
Quoted 1 line
for
/sys/devices/platform/vhci_hcd/statusfile removal, let's say it willbe
Quoted 2 lines
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.
Quoted 11 lines
But yes, you are correct, this should not be in sysfs at all.
Quoted 4 lines
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 hasIs that really necessary as they are just "virtual" ports :) We can put that in a single sysfs file if needed.
Quoted 4 lines
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):
Quoted 4 lines
"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"Quoted 8 lines
- 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 onlocal_busidthe usb device file can be found in /sys usingsysfs_open_device()Quoted 3 lines
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.
Quoted 9 lines
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
Quoted 47 lines
Regards,
David ChangQuoted 43 lines
Quoted 2 lines
Note that the socket parameter is only printed out as a debug
information: it
Quoted 5 lines
is not used anywhere.
Maybe most of the file content is redundant, because:
- we have
/sys/bus/usb/devices/usb*/maxchildwhich is "number of portsif hub"
Quoted 1 line
according to
linux/usb.h:410;Yes.
Quoted 1 line
- we have
/sys/bus/usb/devices/*-*/speedto identify the device speed;Yes.
Quoted 2 lines
- We have already bus number at
/sys/bus/usb/devices/usb*/busnumor at/sys/bus/usb/devices/*-*/busnum;Yes.
Quoted 2 lines
- we also have
/sys/bus/usb/devices/*-*/devnum;
- it is possbile to collect all the devices from
/sys/bus/usb/devices/*-*Quoted 4 lines
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/_______________________________________________
devel mailing list
[email protected]
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
Signature · 2 lines
Thanks ~ Matt Chen
Quoted 96 lines · N?meth M?rton
Quoted 31 lines · Greg KH
Quoted 30 lines · N?meth M?rton
Quoted 7 lines · Greg KH
Quoted 6 lines · N?meth M?rton
The sysfs
show()functions shall return the actual content length of the result buffer. According toDocumentation/filesystems/sysfs.txt:215thescnprintf()function is preferred.See also the article titled "
snprintf()confusion" at
http://lwn.net/Articles/69419/ .Quoted 8 lines
[...]
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-0Quoted 5 lines
Quoted 3 lines
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.txtfor/sys/devices/platform/vhci_hcd/statusfile removal, let's say it will be removed before the usbip goes to mainline. In parallel the new interface
can be developed.Quoted 5 lines
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 haslibsrc/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 onlocal_busidthe usb device file can be found in /sys usingsysfs_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*/maxchildwhich is "number of ports if hub" according tolinux/usb.h:410;
- we have/sys/bus/usb/devices/*-*/speedto identify the device speed;
- We have already bus number at/sys/bus/usb/devices/usb*/busnumor 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 .
Quoted 6 lines
Quoted 2 lines
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