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

[matt mooney <mfm@muteddisk.com>]

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