Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752184Ab1F2CYP (ORCPT ); Tue, 28 Jun 2011 22:24:15 -0400 Received: from qmta02.emeryville.ca.mail.comcast.net ([76.96.30.24]:54001 "EHLO qmta02.emeryville.ca.mail.comcast.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751289Ab1F2CYJ (ORCPT ); Tue, 28 Jun 2011 22:24:09 -0400 Date: Tue, 28 Jun 2011 19:23:21 -0700 From: matt mooney To: N?meth M?rton Cc: Greg KH , Greg Kroah-Hartman , Kulikov Vasiliy , Endre Kollar , Arjan Mels , Ilia Mirkin , David Chang , Himanshu Chauhan , Max Vozeler , Arnd Bergmann , usbip-devel@lists.sourceforge.net, devel@driverdev.osuosl.org, LKML Subject: Re: [PATCH] usbip: handle length at sysfs show() functions Message-ID: <20110629022321.GA52889@haskell.muteddisk.com> Mail-Followup-To: N?meth M?rton , Greg KH , Greg Kroah-Hartman , Kulikov Vasiliy , Endre Kollar , Arjan Mels , Ilia Mirkin , David Chang , Himanshu Chauhan , Max Vozeler , Arnd Bergmann , usbip-devel@lists.sourceforge.net, devel@driverdev.osuosl.org, LKML References: <4DE5CA9F.6010303@freemail.hu> <20110607213418.GB11102@kroah.com> <4DEF0822.7070500@freemail.hu> <20110608160931.GB21645@kroah.com> <4DEFE938.70408@freemail.hu> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4DEFE938.70408@freemail.hu> User-Agent: Mutt/1.4.2.3i Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5287 Lines: 114 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 -- 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/