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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 27 May 2011 15:33:02 +0800
From:	Greg KH <greg@...ah.com>
To:	Xiangliang Yu <yuxiangl@...vell.com>
Cc:	"James.Bottomley@...e.de" <James.Bottomley@...e.de>,
	"jslaby@...e.cz" <jslaby@...e.cz>,
	"linux-scsi@...r.kernel.org" <linux-scsi@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Jacky Feng <jfeng@...vell.com>
Subject: Re: [PATCH 3/9] [SCSI] mvsas: Add driver version and interrupt
 coalescing to device 	attributes in sysfs

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@...vell.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@...vell.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/