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

matt mooney
2011-06-29T02:24:15Z

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