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

["Németh Márton" <nm127@freemail.hu>]

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