[RFC] support for sysfs string based properties for SCSI (1/3)

[RFC] support for sysfs string based properties for SCSI (1/3)

[James Bottomley]

[-- Attachment #1: Type: text/plain, Size: 816 bytes --]


This first patch is of general interest (the other two are going to the
SCSI list only).

The problem this seeks to solve is that we have a bunch of properties in
SCSI that we'd like to expose through the sysfs interface.  The
mid-layer can get their values, but setting them requires co-operation
from the host drivers, thus we'd like to expose a show/store interface
to all the SCSI drivers.

The current one call back per sysfs file is a bit unwieldy for
encapsulating in an interface like this.  what this patch does is to
allow a fallback show/store method of the bus type (if the device type
doesn't exist).  However, the bus_type show/store passes in the
attribute so a comparison may be done against the name of the attribute.

For details of how all this gets used, see the following SCSI patches.

James


[-- Attachment #2: tmp.diff --]
[-- Type: text/plain, Size: 3127 bytes --]

===== drivers/scsi/NCR_D700.c 1.8 vs edited =====
--- 1.8/drivers/scsi/NCR_D700.c	Sun Apr 27 05:31:39 2003
+++ edited/drivers/scsi/NCR_D700.c	Thu May  1 11:11:48 2003
@@ -169,16 +169,18 @@
 	struct Scsi_Host	*hosts[2];
 };
 
-static int NCR_D700_probe_one(struct NCR_D700_private *p, int chan,
+static int 
+NCR_D700_probe_one(struct NCR_D700_private *p, int siop,
 		int irq, int slot, u32 region, int differential)
 {
 	struct NCR_700_Host_Parameters *hostdata;
 	struct Scsi_Host *host;
+	int ret;
 
 	hostdata = kmalloc(sizeof(*hostdata), GFP_KERNEL);
 	if (!hostdata) {
-		printk(KERN_ERR "NCR D700: Failed to allocate host data "
-				"for channel %d, detatching\n", chan);
+		printk(KERN_ERR "NCR D700: SIOP%d: Failed to allocate host"
+		       "data, detatching\n", siop);
 		return -ENOMEM;
 	}
 	memset(hostdata, 0, sizeof(*hostdata));
@@ -186,41 +188,49 @@
 	if (!request_region(region, 64, "NCR_D700")) {
 		printk(KERN_ERR "NCR D700: Failed to reserve IO region 0x%x\n",
 				region);
-		kfree(hostdata);
-		return -ENODEV;
+		ret = -ENODEV;
+		goto region_failed;
 	}
 		
 	/* Fill in the three required pieces of hostdata */
 	hostdata->base = region;
-	hostdata->differential = (((1<<chan) & differential) != 0);
+	hostdata->differential = (((1<<siop) & differential) != 0);
 	hostdata->clock = NCR_D700_CLOCK_MHZ;
 
-	/* and register the chip */
+	/* and register the siop */
 	host = NCR_700_detect(&NCR_D700_driver_template, hostdata);
 	if (!host) {
-		kfree(hostdata);
-		release_region(host->base, 64);
-		return -ENOMEM;
+		ret = -ENOMEM;
+		goto detect_failed;
 	}
 
 	host->irq = irq;
 	/* FIXME: Read this from SUS */
-	host->this_id = id_array[slot * 2 + chan];
+	host->this_id = id_array[slot * 2 + siop];
 	printk(KERN_NOTICE "NCR D700: SIOP%d, SCSI id is %d\n",
-			chan, host->this_id);
+			siop, host->this_id);
 	if (request_irq(irq, NCR_700_intr, SA_SHIRQ, "NCR_D700", host)) {
-		printk(KERN_ERR "NCR D700, channel %d: irq problem, "
-				"detatching\n", chan);
-		scsi_unregister(host);
-		NCR_700_release(host);
-		return -ENODEV;
+		printk(KERN_ERR "NCR D700: SIOP%d: irq problem, "
+				"detatching\n", siop);
+		ret = -ENODEV;
+		goto irq_failed;
 	}
 
-	scsi_add_host(host, NULL);
+	scsi_add_host(host, p->dev);
 
-	p->hosts[chan] = host;
+	p->hosts[siop] = host;
 	hostdata->dev = p->dev;
 	return 0;
+
+ irq_failed:
+	scsi_unregister(host);
+	NCR_700_release(host);
+ detect_failed:
+	release_region(host->base, 64);
+ region_failed:
+	kfree(hostdata);
+
+	return ret;
 }
 
 /* Detect a D700 card.  Note, because of the setup --- the chips are
@@ -305,8 +315,15 @@
 
 	/* plumb in both 700 chips */
 	for (i = 0; i < 2; i++) {
-		found |= NCR_D700_probe_one(p, i, irq, slot,
-				offset_addr | (0x80 * i), differential);
+		int err;
+
+		if ((err = NCR_D700_probe_one(p, i, irq, slot,
+					      offset_addr + (0x80 * i),
+					      differential)) != 0)
+			printk("D700: SIOP%d: probe failed, error = %d\n",
+			       i, err);
+		else
+			found++;
 	}
 
 	if (!found) {

Re: [RFC] support for sysfs string based properties for SCSI (1/3)

[James Bottomley]

[-- Attachment #1: Type: text/plain, Size: 957 bytes --]

On Sat, 2003-05-03 at 14:11, James Bottomley wrote:
> 
> This first patch is of general interest (the other two are going to the
> SCSI list only).
> 
> The problem this seeks to solve is that we have a bunch of properties in
> SCSI that we'd like to expose through the sysfs interface.  The
> mid-layer can get their values, but setting them requires co-operation
> from the host drivers, thus we'd like to expose a show/store interface
> to all the SCSI drivers.
> 
> The current one call back per sysfs file is a bit unwieldy for
> encapsulating in an interface like this.  what this patch does is to
> allow a fallback show/store method of the bus type (if the device type
> doesn't exist).  However, the bus_type show/store passes in the
> attribute so a comparison may be done against the name of the attribute.
> 
> For details of how all this gets used, see the following SCSI patches.
> 
> James

And this time with the correct attachment.

James


[-- Attachment #2: Type: text/plain, Size: 1821 bytes --]

# This is a BitKeeper generated patch for the following project:
# Project Name: Linux kernel tree
# This patch format is intended for GNU patch command version 2.5 or higher.
# This patch includes the following deltas:
#	           ChangeSet	1.1196  -> 1.1197 
#	 drivers/base/core.c	1.65    -> 1.66   
#	include/linux/device.h	1.87    -> 1.88   
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 03/05/03	jejb@raven.il.steeleye.com	1.1197
# sysfs: add default show/store for bus_type
# 
# These are used if the device_attribute show/store are empty.  They
# allow buses to do string based parsing of the device properties.
# --------------------------------------------
#
diff -Nru a/drivers/base/core.c b/drivers/base/core.c
--- a/drivers/base/core.c	Sat May  3 14:18:21 2003
+++ b/drivers/base/core.c	Sat May  3 14:18:21 2003
@@ -42,6 +42,8 @@
 
 	if (dev_attr->show)
 		ret = dev_attr->show(dev,buf);
+	else if (dev->bus->show)
+		ret = dev->bus->show(dev, buf, attr);
 	return ret;
 }
 
@@ -55,6 +57,8 @@
 
 	if (dev_attr->store)
 		ret = dev_attr->store(dev,buf,count);
+	else if (dev->bus->store)
+		ret = dev->bus->store(dev,buf,count,attr);
 	return ret;
 }
 
diff -Nru a/include/linux/device.h b/include/linux/device.h
--- a/include/linux/device.h	Sat May  3 14:18:21 2003
+++ b/include/linux/device.h	Sat May  3 14:18:21 2003
@@ -74,6 +74,10 @@
 	struct device * (*add)	(struct device * parent, char * bus_id);
 	int		(*hotplug) (struct device *dev, char **envp, 
 				    int num_envp, char *buffer, int buffer_size);
+	ssize_t (*show)(struct device * dev, char * buf,
+			struct attribute *attr);
+	ssize_t (*store)(struct device * dev, const char * buf, size_t count,
+			 struct attribute *attr);
 };
 
 

Re: [RFC] support for sysfs string based properties for SCSI (1/3)

[Greg KH]

On Sat, May 03, 2003 at 02:19:23PM -0500, James Bottomley wrote:
> diff -Nru a/drivers/base/core.c b/drivers/base/core.c
> --- a/drivers/base/core.c	Sat May  3 14:18:21 2003
> +++ b/drivers/base/core.c	Sat May  3 14:18:21 2003
> @@ -42,6 +42,8 @@
>  
>  	if (dev_attr->show)
>  		ret = dev_attr->show(dev,buf);
> +	else if (dev->bus->show)
> +		ret = dev->bus->show(dev, buf, attr);
>  	return ret;

Can't you do this by using the class interface instead?

This also forces you to do a lot of string compares within the bus show
function (as your example did) which is almost as unwieldy as just
having individual show functions, right?  :)

thanks,

greg k-h

Re: [RFC] support for sysfs string based properties for SCSI (1/3)

[James Bottomley]

On Mon, 2003-05-05 at 12:02, Greg KH wrote:
> On Sat, May 03, 2003 at 02:19:23PM -0500, James Bottomley wrote:
> > diff -Nru a/drivers/base/core.c b/drivers/base/core.c
> > --- a/drivers/base/core.c	Sat May  3 14:18:21 2003
> > +++ b/drivers/base/core.c	Sat May  3 14:18:21 2003
> > @@ -42,6 +42,8 @@
> >  
> >  	if (dev_attr->show)
> >  		ret = dev_attr->show(dev,buf);
> > +	else if (dev->bus->show)
> > +		ret = dev->bus->show(dev, buf, attr);
> >  	return ret;
> 
> Can't you do this by using the class interface instead?

I don't know, I haven't digested the class interface patches yet, since
they just appeared this morning.

> This also forces you to do a lot of string compares within the bus show
> function (as your example did) which is almost as unwieldy as just
> having individual show functions, right?  :)

Nothing prevents users from doing it the callback way.  However,
callbacks aren't a scaleable interface for properties that have to be
shared and overridden.

I agree string compares are unwieldy (and smack of XML), so I'm open to
suggestions of a better way of doing it that has the same flexibility of
the string method...

James

Re: [RFC] support for sysfs string based properties for SCSI (1/3)

[Greg KH]

On Mon, May 05, 2003 at 12:08:35PM -0500, James Bottomley wrote:
> On Mon, 2003-05-05 at 12:02, Greg KH wrote:
> > On Sat, May 03, 2003 at 02:19:23PM -0500, James Bottomley wrote:
> > > diff -Nru a/drivers/base/core.c b/drivers/base/core.c
> > > --- a/drivers/base/core.c	Sat May  3 14:18:21 2003
> > > +++ b/drivers/base/core.c	Sat May  3 14:18:21 2003
> > > @@ -42,6 +42,8 @@
> > >  
> > >  	if (dev_attr->show)
> > >  		ret = dev_attr->show(dev,buf);
> > > +	else if (dev->bus->show)
> > > +		ret = dev->bus->show(dev, buf, attr);
> > >  	return ret;
> > 
> > Can't you do this by using the class interface instead?
> 
> I don't know, I haven't digested the class interface patches yet, since
> they just appeared this morning.

I think Mike has a patch queued up that takes advantage of the class
code, which might address all of these issues.  Mike?

> > This also forces you to do a lot of string compares within the bus show
> > function (as your example did) which is almost as unwieldy as just
> > having individual show functions, right?  :)
> 
> Nothing prevents users from doing it the callback way.  However,
> callbacks aren't a scaleable interface for properties that have to be
> shared and overridden.

I don't understand the "shared and overridden" aspect.  Do you mean a
default attribute for all types of SCSI devices, with the ability for an
individual device to override the attribute with it's own values if it
wants to?  That still seems doable within the driver model today,
without having to rely on bus specific functions.

Hm, this is _really_ calling for what Pat calls "attributes".  Take a
look at the ones in the class model, and let me know if those would work
for you for devices.  If so, I'll be glad to add them, which should help
you out here.

Hope this helps,

greg k-h

Re: [RFC] support for sysfs string based properties for SCSI (1/3)

[Mike Anderson]

Greg KH [greg@kroah.com] wrote:
> On Mon, May 05, 2003 at 12:08:35PM -0500, James Bottomley wrote:
> > On Mon, 2003-05-05 at 12:02, Greg KH wrote:
> > > On Sat, May 03, 2003 at 02:19:23PM -0500, James Bottomley wrote:
> > > > diff -Nru a/drivers/base/core.c b/drivers/base/core.c
> > > > --- a/drivers/base/core.c	Sat May  3 14:18:21 2003
> > > > +++ b/drivers/base/core.c	Sat May  3 14:18:21 2003
> > > > @@ -42,6 +42,8 @@
> > > >  
> > > >  	if (dev_attr->show)
> > > >  		ret = dev_attr->show(dev,buf);
> > > > +	else if (dev->bus->show)
> > > > +		ret = dev->bus->show(dev, buf, attr);
> > > >  	return ret;
> > > 
> > > Can't you do this by using the class interface instead?
> > 
> > I don't know, I haven't digested the class interface patches yet, since
> > they just appeared this morning.
> 
> I think Mike has a patch queued up that takes advantage of the class
> code, which might address all of these issues.  Mike?
> 

The patches I sent add class support for scsi_host, but this only gives
granularity of attributes specific to scsi_host. There is no built in
functionality to override show or store handler functions.


-andmike
--
Michael Anderson
andmike@us.ibm.com

Re: [RFC] support for sysfs string based properties for SCSI (1/3)

[James Bottomley]

On Mon, 2003-05-05 at 12:17, Greg KH wrote:
> On Mon, May 05, 2003 at 12:08:35PM -0500, James Bottomley wrote:
> > Nothing prevents users from doing it the callback way.  However,
> > callbacks aren't a scaleable interface for properties that have to be
> > shared and overridden.
> 
> I don't understand the "shared and overridden" aspect.  Do you mean a
> default attribute for all types of SCSI devices, with the ability for an
> individual device to override the attribute with it's own values if it
> wants to?  That still seems doable within the driver model today,
> without having to rely on bus specific functions.

Yes, but the problem is how to communicate the override between the HBA
driver and SCSI. The override is required because changing some
properties may require changes at many levels.  The queue_depth in the
example I gave is the obvious one:

- the device needs to make sure it has internal resources for the
revised queue depth.  It also has to implement the change.
- the mid-layer needs to do the adjustment
- As does the block layer (I didn't implement this one in my patch).

The interface obviously belongs in the SCSI API, but one API entry per
property causes the interface to explode and also makes it quite
difficult to add new ones, so we need a generic get/set property
interface; however, then we need to know what property, hence the
strings.

> Hm, this is _really_ calling for what Pat calls "attributes".  Take a
> look at the ones in the class model, and let me know if those would work
> for you for devices.  If so, I'll be glad to add them, which should help
> you out here.

Well, I analysed the class interface.  It currently won't do what we
need, but it might be able to if it supported an inheritance, so there
would be a device class which could then be extended by the drivers to
override the properties they need.  However, isn't this type of
inheritance going to be a real pain using function pointers, and if we
only support overrides of show/store, it's probably simpler just to
support the strings based interface.

James

Re: [RFC] support for sysfs string based properties for SCSI (1/3)

[Patrick Mochel]

On 3 May 2003, James Bottomley wrote:

> On Sat, 2003-05-03 at 14:11, James Bottomley wrote:
> > 
> > This first patch is of general interest (the other two are going to the
> > SCSI list only).
> > 
> > The problem this seeks to solve is that we have a bunch of properties in
> > SCSI that we'd like to expose through the sysfs interface.  The
> > mid-layer can get their values, but setting them requires co-operation
> > from the host drivers, thus we'd like to expose a show/store interface
> > to all the SCSI drivers.
> > 
> > The current one call back per sysfs file is a bit unwieldy for
> > encapsulating in an interface like this.  what this patch does is to
> > allow a fallback show/store method of the bus type (if the device type
> > doesn't exist).  However, the bus_type show/store passes in the
> > attribute so a comparison may be done against the name of the attribute.

I'm not very fond of your implementation. I like the concept, and it 
works, but there's a much better way, despite requiring some changes to 
the kobject core. 

Ideally, what you'd like to do, instead of adding more conditionals to
normal call chain down to the lowest-level sysfs show()/store() methods,
you'd like to bypass them and define your own methods higher up; e.g. to
replace dev_attr_{show,store} in this case.

But you can't, because in order to define attributes for a device object, 
you must use the device_attribute infrastructure, which means using the 
call chain I've mandated. :)

We could modify the lowest-level methods, but that requires changing every 
attribute definition, and we'll still end up with the macro hell we 
already see to export many attributes that all just a little different. 

So, what I propose is killing struct kobj_type. We can move ->release() 
into struct kset, adding a small amount of overhead to ksets of an 
identical type (trivial). We can move ->sysfs_ops and ->default_attrs into 
say struct attribute_group and allow subsystems to register arbitrary 
groups of attributes of attributes for kobjects. 

This will allow us to keep identical behavior everywhere, with a little 
glue, but will also solve your problem nicely. You will do something like:


struct attribute my_attrs[] = {
	foo,
	bar,
	baz,
	NULL,
};

struct attribute_group my_group {
	.ops	= {
		my_attr_show,
		my_attr_store,
	},
	.attrs	= my_attrs,
};

int my_register()
{
	...
	if ((error = device_register(&mydev.dev)))
		goto Err;
	if ((error = attr_grp_register(&mydev.dev.kobj,&my_group)))
		goto Unregister;
	...
}

my_show() and my_store() will get pointers to the kobjects, which you'll
have to convert into the proper type, and pointer to the attributes, so
you'll be able to tell which attribute is requested from a much higher
level. 

We can still keep device_attribute arround, but in general I don't think
it's that useful to route every attribute request through the core. Using
them is fine, and easy. But attributes travel in herds, and appear when an
object is registered with a subsystem, or some extension of a subsystem.

It's easier to define one set of callbacks + lookup mechanism for 
attribute by name, than it is to macro-ize the hell out of your code, 
provided your dealing with a fair number of attributes per object.


	-pat