Re: [PATCH 3/9] [SCSI] mvsas: Add driver version and interrupt coalescing to device attributes in sysfs

[Greg KH <greg@kroah.com>]

On Thu, May 26, 2011 at 08:23:18PM -0700, Xiangliang Yu wrote:
> > >>Subject: Re: [PATCH 3/9] [SCSI] mvsas: Add driver version and interrupt >>>coalescing to device attributes in sysfs
> 
> > >>On Thu, May 26, 2011 at 02:00:21AM -0700, Xiangliang Yu wrote:
> > >
> > >Subject: Re: [PATCH 3/9] [SCSI] mvsas: Add driver version and interrupt >coalescing to device attributes in sysfs
> > >> 
> > >> -- Add support for driver version and interrupt coalescing device attributes.
> > >>
> > >>When you add a sysfs file, you must also add a Documentation/ABI file as
> > >>well.  Please do that here.
> > >
> > >These device attributes are sysft class properties for the scsi host,
> >> and the sysfs files are within scsi host sysfs directory. so, do you
> >> think I need add a ABI file?  and , I can't find anything about scsi
> >> host sysfs file under directory Documentation/ABI.
> 
> >Yes, again, if you add a sysfs file, you need to add documentation
> >explaining exactly what it is there for and what it does.
> >
> >And if there isn't already a file for this device, then please add it.
> How about this:
> 
> .../ABI/testing/sysfs-bus-pci-scsi-devices-mvsas   |   13 +++++++++++++
>  1 files changed, 13 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/ABI/testing/sysfs-bus-pci-scsi-devices-mvsas
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-pci-scsi-devices-mvsas b/Documentation/ABI/testing/sysfs-bus-pci-scsi
> -devices-mvsas
> new file mode 100644
> index 0000000..45f597e
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-pci-scsi-devices-mvsas
> @@ -0,0 +1,13 @@
> +What:          /sys/devices/pci/<devices>/<dev>/host/scsi_host/host/driver_version
> +Date:          May 2011
> +Kernel Version:        2.6.39
> +Contact:       yuxiangl@marvell.com
> +Description:   Display the mvsas drivers version

A driver version should never show up down here in sysfs, use the proper
place for this like the rest of the kernel's drivers do (hint, look at
MODULE_VERSION() and where that ends up, in your module's sysfs
directory.)

> +What:          /sys/devices/pci/<devices>/<dev>/host/scsi_host/host/interrupt_coalescing
> +Date:          May 2011
> +Kernel Version:        2.6.39

2.6.39 was released already, is this file in that release?

> +Contact:       yuxiangl@marvell.com
> +Description:   Determines the maximum time the 88SE94XX waits after the occurrence of a
> +               Command Done before generating an interrupt.The maximum number of the 
> +               variable is less than 0x10000.

Why would a user, or anyone else, ever want to be able to change this?
Why wouldn't this just be something that the driver handles
automagically so the user never has to worry about it at all?

You also don't specify the units involved here, which is not good.

See, when you document things, it's easier to notice when you did
something you didn't need to do (i.e. the driver version thing).  :)

thanks,

greg k-h