[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 (2/3)

[James Bottomley]

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


This implements a generic host and device based show/store using
property attributes in SCSI.

I've done it for both LLDs and ULDs, but I'm not convinced that the ULD
one is needed: I think it will probably get subsumed by the driver class
changes.

James
 

[-- Attachment #2: tmp.diff --]
[-- Type: text/plain, Size: 13949 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.1197  -> 1.1198 
#	drivers/scsi/scsi_sysfs.c	1.11    -> 1.12   
#	drivers/scsi/hosts.h	1.60    -> 1.61   
#	 drivers/scsi/scsi.c	1.109   -> 1.110  
#	drivers/scsi/scsi_priv.h	1.1     -> 1.2    
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 03/05/03	jejb@raven.il.steeleye.com	1.1198
# SCSI: add ULD and LLD device/host property show/store
# 
# This implements a property show/store method for arbitrary string
# based properties which can be exposed through the sysfs interface.
# 
# The essential problem to be solved here is that properties can be
# read easily enough in the mid-layer but they require co-operation
# from the LLD to set them
# --------------------------------------------
#
diff -Nru a/drivers/scsi/hosts.h b/drivers/scsi/hosts.h
--- a/drivers/scsi/hosts.h	Sat May  3 14:24:25 2003
+++ b/drivers/scsi/hosts.h	Sat May  3 14:24:25 2003
@@ -266,6 +266,22 @@
     int (* bios_param)(struct scsi_device *, struct block_device *,
 		    sector_t, int []);
 
+    /* 
+     * Used to show/store generic ASCII properties
+     *
+     * If filled in, it will be called before the generic SCSI driver
+     * processes the properties.
+     */
+    ssize_t (* host_property_show)(struct Scsi_Host *, char *,
+				   struct attribute *);
+    ssize_t (* host_property_store)(struct Scsi_Host *, const char *,
+				    size_t, struct attribute *);
+    ssize_t (* device_property_show)(struct scsi_device *, char *,
+				     struct attribute *);
+    ssize_t (* device_property_store)(struct scsi_device *, const char *,
+				      size_t, struct attribute *);
+
+
     /*
      * This determines if we will use a non-interrupt driven
      * or an interrupt driven scheme,  It is set to the maximum number
@@ -538,6 +554,10 @@
     int (*init_command)(Scsi_Cmnd *);     /* Used by new queueing code. 
                                            Selects command for blkdevs */
     void (*rescan)(Scsi_Device *);
+    ssize_t (* device_property_show)(struct scsi_device *, char *,
+				     struct attribute *);
+    ssize_t (* device_property_store)(struct scsi_device *, const char *,
+				      size_t, struct attribute *);
     struct device_driver scsi_driverfs_driver;
 };
 
diff -Nru a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
--- a/drivers/scsi/scsi.c	Sat May  3 14:24:25 2003
+++ b/drivers/scsi/scsi.c	Sat May  3 14:24:25 2003
@@ -89,7 +89,7 @@
  * List of all highlevel drivers.
  */
 LIST_HEAD(scsi_devicelist);
-static DECLARE_RWSEM(scsi_devicelist_mutex);
+DECLARE_RWSEM(scsi_devicelist_mutex);
 
 /*
  * Note - the initial logging level can be set here to log events at boot time.
diff -Nru a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h
--- a/drivers/scsi/scsi_priv.h	Sat May  3 14:24:25 2003
+++ b/drivers/scsi/scsi_priv.h	Sat May  3 14:24:25 2003
@@ -143,4 +143,7 @@
 extern struct list_head scsi_dev_info_list;
 extern int scsi_dev_info_list_add_str(char *);
 
+extern struct list_head scsi_devicelist;
+extern struct rw_semaphore scsi_devicelist_mutex;
+
 #endif /* _SCSI_PRIV_H */
diff -Nru a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
--- a/drivers/scsi/scsi_sysfs.c	Sat May  3 14:24:25 2003
+++ b/drivers/scsi/scsi_sysfs.c	Sat May  3 14:24:25 2003
@@ -16,41 +16,20 @@
 
 #include "scsi_priv.h"
 
-/*
- * shost_show_function: macro to create an attr function that can be used to
- * show a non-bit field.
- */
-#define shost_show_function(field, format_string)			\
-static ssize_t								\
-show_##field (struct device *dev, char *buf)				\
-{									\
-	struct Scsi_Host *shost = to_scsi_host(dev);			\
-	return snprintf (buf, 20, format_string, shost->field);	\
-}
-
-/*
- * shost_rd_attr: macro to create a function and attribute variable for a
- * read only field.
- */
-#define shost_rd_attr(field, format_string)				\
-	shost_show_function(field, format_string)				\
-static DEVICE_ATTR(field, S_IRUGO, show_##field, NULL)
-
-/*
- * Create the actual show/store functions and data structures.
- */
-shost_rd_attr(unique_id, "%u\n");
-shost_rd_attr(host_busy, "%hu\n");
-shost_rd_attr(cmd_per_lun, "%hd\n");
-shost_rd_attr(sg_tablesize, "%hu\n");
-shost_rd_attr(unchecked_isa_dma, "%d\n");
-
-static struct device_attribute *const shost_attrs[] = {
-	&dev_attr_unique_id,
-	&dev_attr_host_busy,
-	&dev_attr_cmd_per_lun,
-	&dev_attr_sg_tablesize,
-	&dev_attr_unchecked_isa_dma,
+/* FIXME: need generic attributes for the host */
+
+/* generic attributes to appear in device directory */
+struct device_attribute sdev_attrs[] = {
+	{ .attr = { .name = "device_blocked", .mode = S_IRUGO }, },
+	{ .attr = { .name = "queue_depth", .mode = S_IRUGO | S_IWUSR }, },
+	{ .attr = { .name = "type", .mode = S_IRUGO }, },
+	{ .attr = { .name = "scsi_level", .mode = S_IRUGO }, },
+	{ .attr = { .name = "access_count", .mode = S_IRUGO }, },
+	{ .attr = { .name = "vendor", .mode = S_IRUGO }, },
+	{ .attr = { .name = "model", .mode = S_IRUGO }, },
+	{ .attr = { .name = "rev", .mode = S_IRUGO }, },
+	{ .attr = { .name = "online", .mode = S_IRUGO }, },
+	{ .attr = { .name = "rescan", .mode = S_IRUGO | S_IWUSR }, },
 };
 
 /**
@@ -105,10 +84,154 @@
         return 0;
 }
 
+#define SCSI_SYSFS_MAX_BUF	20
+
+static ssize_t scsi_sysfs_device_show(struct device * dev, char * buf,
+				      struct attribute *attr)
+{
+	char *name = attr->name;
+	struct scsi_device *sdev = to_scsi_device(dev);
+	struct Scsi_Device_Template *tpnt;
+	int ret = 0;
+
+	/* process any host based overrides */
+
+	if(sdev->host->hostt->device_property_show) {
+		ret = sdev->host->hostt->device_property_show(sdev, buf, attr);
+		if(ret != 0)
+			return ret;
+	}
+
+	/* pass to every upper level driver.  ULDs must check to see
+	 * if they're interested in the device type *before* doing
+	 * anything with the property */
+
+	down_read(&scsi_devicelist_mutex);
+	list_for_each_entry(tpnt, &scsi_devicelist, list) {
+		if(tpnt->device_property_show) {
+			ret = tpnt->device_property_show(sdev, buf, attr);
+
+			if(ret != 0) {
+				up_read(&scsi_devicelist_mutex);
+				return ret;
+			}
+		}
+
+	}
+	up_read(&scsi_devicelist_mutex);
+
+	/* generic processing */
+
+	if (strcmp(name, "device_blocked") == 0) {
+		ret = snprintf(buf, SCSI_SYSFS_MAX_BUF, "%d\n",
+			       sdev->device_blocked);
+	} else if (strcmp(name, "queue_depth") == 0) {
+		ret = snprintf(buf, SCSI_SYSFS_MAX_BUF, "%d\n",
+			       sdev->queue_depth);
+	} else if (strcmp(name, "type") == 0) {
+		ret  = snprintf(buf, SCSI_SYSFS_MAX_BUF, "%d\n",
+				sdev->type);
+	} else if (strcmp(name, "scsi_level") == 0) {
+		ret = snprintf(buf, SCSI_SYSFS_MAX_BUF, "%d\n",
+			       sdev->scsi_level);
+	} else if (strcmp(name, "access_count") == 0) {
+		ret = snprintf(buf, SCSI_SYSFS_MAX_BUF, "%d\n",
+			       sdev->access_count);
+	} else if (strcmp(name, "vendor") == 0) {
+		ret = snprintf(buf, SCSI_SYSFS_MAX_BUF, "%.8s\n",
+			       sdev->vendor);
+	} else if (strcmp(name, "model") == 0) {
+		ret = snprintf(buf, SCSI_SYSFS_MAX_BUF, "%.16s\n",
+			       sdev->model);
+	} else if (strcmp(name, "rev") == 0) {
+		ret = snprintf(buf, SCSI_SYSFS_MAX_BUF, "%.4s\n",
+			       sdev->rev);
+	} else if (strcmp(name, "online") == 0) {
+		ret = snprintf(buf, SCSI_SYSFS_MAX_BUF, "%d\n",
+			       sdev->online);
+	}
+
+	return ret;
+}
+
+static size_t scsi_sysfs_device_store(struct device * dev, const char * buf,
+				      size_t count, struct attribute *attr)
+{
+	char *name = attr->name;
+	struct scsi_device *sdev = to_scsi_device(dev);
+	struct Scsi_Device_Template *tpnt;
+	int ret = 0;
+
+	/* process any host based overrides */
+
+	if(sdev->host->hostt->device_property_store) {
+		ret = sdev->host->hostt->device_property_store(sdev, buf,
+							       count, attr);
+		if(ret < 0)
+			return ret;
+
+	}
+	/* pass to every upper level driver.  ULDs must check to see
+	 * if they're interested in the device type *before* doing
+	 * anything with the property */
+
+	down_read(&scsi_devicelist_mutex);
+	list_for_each_entry(tpnt, &scsi_devicelist, list) {
+		if(tpnt->device_property_store) {
+			ret = tpnt->device_property_store(sdev, buf,
+							  count, attr);
+
+			if(ret < 0) {
+				up_read(&scsi_devicelist_mutex);
+				return ret;
+			}
+		}
+				
+	}
+	up_read(&scsi_devicelist_mutex);
+
+	/* generic processing */
+
+	if (strcmp(name, "rescan") == 0) {
+		ret = count;
+		scsi_rescan_device(sdev);
+	} else if (strcmp(name, "online") == 0) {
+		/* FIXME: will do online/offline here when available */
+	}
+		
+	return ret;
+}
+	
+static ssize_t scsi_sysfs_show(struct device * dev, char * buf,
+			       struct attribute *attr)
+{
+	/* FIXME: If we actually had a device type corresponding to a
+	 * host, we'd check for it here and then do host stuff 
+	if(0)
+		return scsi_sysfs_host_show(dev, buf, attr);
+	else
+	*/
+		return scsi_sysfs_device_show(dev, buf, attr);
+}
+
+static ssize_t scsi_sysfs_store(struct device * dev, const char * buf,
+			       size_t count, struct attribute *attr)
+{
+	/* FIXME: If we actually had a device type corresponding to a
+	 * host, we'd check for it here and then do host stuff
+	if(0)
+		return scsi_sysfs_host_store(dev, buf, count, attr);
+	else
+	*/
+		return scsi_sysfs_device_store(dev, buf, count, attr);
+}
+
 
 static struct bus_type scsi_bus_type = {
         .name		= "scsi",
         .match		= scsi_bus_match,
+	.show		= scsi_sysfs_show,
+	.store		= scsi_sysfs_store,
 };
 
 
@@ -154,125 +277,6 @@
 }
 
 
-/*
- * sdev_show_function: macro to create an attr function that can be used to
- * show a non-bit field.
- */
-#define sdev_show_function(field, format_string)				\
-static ssize_t								\
-show_##field (struct device *dev, char *buf)				\
-{									\
-	struct scsi_device *sdev;					\
-	sdev = to_scsi_device(dev);					\
-	return snprintf (buf, 20, format_string, sdev->field);		\
-}									\
-
-/*
- * sdev_rd_attr: macro to create a function and attribute variable for a
- * read only field.
- */
-#define sdev_rd_attr(field, format_string)				\
-	sdev_show_function(field, format_string)				\
-static DEVICE_ATTR(field, S_IRUGO, show_##field, NULL)
-
-
-/*
- * sdev_rd_attr: create a function and attribute variable for a
- * read/write field.
- */
-#define sdev_rw_attr(field, format_string)				\
-	sdev_show_function(field, format_string)				\
-									\
-static ssize_t								\
-sdev_store_##field (struct device *dev, const char *buf, size_t count)	\
-{									\
-	struct scsi_device *sdev;					\
-	sdev = to_scsi_device(dev);					\
-	snscanf (buf, 20, format_string, &sdev->field);			\
-	return count;							\
-}									\
-static DEVICE_ATTR(field, S_IRUGO | S_IWUSR, show_##field, sdev_store_##field)
-
-/*
- * sdev_rd_attr: create a function and attribute variable for a
- * read/write bit field.
- */
-#define sdev_rw_attr_bit(field)						\
-	sdev_show_function(field, "%d\n")					\
-									\
-static ssize_t								\
-sdev_store_##field (struct device *dev, const char *buf, size_t count)	\
-{									\
-	int ret;							\
-	struct scsi_device *sdev;					\
-	ret = scsi_sdev_check_buf_bit(buf);				\
-	if (ret >= 0)	{						\
-		sdev = to_scsi_device(dev);				\
-		sdev->field = ret;					\
-		ret = count;						\
-	}								\
-	return ret;							\
-}									\
-static DEVICE_ATTR(field, S_IRUGO | S_IWUSR, show_##field, sdev_store_##field)
-
-/*
- * scsi_sdev_check_buf_bit: return 0 if buf is "0", return 1 if buf is "1",
- * else return -EINVAL.
- */
-static int scsi_sdev_check_buf_bit(const char *buf)
-{
-	if ((buf[1] == '\0') || ((buf[1] == '\n') && (buf[2] == '\0'))) {
-		if (buf[0] == '1')
-			return 1;
-		else if (buf[0] == '0')
-			return 0;
-		else 
-			return -EINVAL;
-	} else
-		return -EINVAL;
-}
-
-/*
- * Create the actual show/store functions and data structures.
- */
-sdev_rd_attr (device_blocked, "%d\n");
-sdev_rd_attr (queue_depth, "%d\n");
-sdev_rd_attr (type, "%d\n");
-sdev_rd_attr (scsi_level, "%d\n");
-sdev_rd_attr (access_count, "%d\n");
-sdev_rd_attr (vendor, "%.8s\n");
-sdev_rd_attr (model, "%.16s\n");
-sdev_rd_attr (rev, "%.4s\n");
-sdev_rw_attr_bit (online);
-
-static ssize_t
-show_rescan_field (struct device *dev, char *buf)
-{
-	return 0; 
-}
-
-static ssize_t
-store_rescan_field (struct device *dev, const char *buf, size_t count) 
-{
-	scsi_rescan_device(to_scsi_device(dev));
-	return 0;
-}
-
-static DEVICE_ATTR(rescan, S_IRUGO | S_IWUSR, show_rescan_field, store_rescan_field)
-
-static struct device_attribute * const sdev_attrs[] = {
-	&dev_attr_device_blocked,
-	&dev_attr_queue_depth,
-	&dev_attr_type,
-	&dev_attr_scsi_level,
-	&dev_attr_access_count,
-	&dev_attr_vendor,
-	&dev_attr_model,
-	&dev_attr_rev,
-	&dev_attr_online,
-	&dev_attr_rescan,
-};
-
 /**
  * scsi_device_register - register a scsi device with the scsi bus
  * @sdev:	scsi_device to register
@@ -295,7 +299,7 @@
 
 	for (i = 0; !error && i < ARRAY_SIZE(sdev_attrs); i++)
 		error = device_create_file(&sdev->sdev_driverfs_dev,
-					   sdev_attrs[i]);
+					   &sdev_attrs[i]);
 
 	if (error)
 		scsi_device_unregister(sdev);
@@ -312,6 +316,6 @@
 	int i;
 
 	for (i = 0; i < ARRAY_SIZE(sdev_attrs); i++)
-		device_remove_file(&sdev->sdev_driverfs_dev, sdev_attrs[i]);
+		device_remove_file(&sdev->sdev_driverfs_dev, &sdev_attrs[i]);
 	device_unregister(&sdev->sdev_driverfs_dev);
 }

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

[James Bottomley]

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

This illustrates how the new show/store interface may be used for adding
and setting properties.

The attached patch implements a store method in 53c700 which allows the
setting of the queue_depth parameter.

It also adds an "outstanding_tags" attribute which provides all the
information that the 53c700 procfs_info routine now provides.

Hopefully, this interface will be simple and flexible enough to allow us
to migrate all of the SCSI drivers away from proc and on to sysfs.

James


[-- Attachment #2: tmp.diff --]
[-- Type: text/plain, Size: 3831 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.1198  -> 1.1199 
#	drivers/scsi/53c700.c	1.29    -> 1.30   
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 03/05/03	jejb@raven.il.steeleye.com	1.1199
# SCSI: implement properties in the 53c700 driver
# 
# Add a store for the queue_depth setting, and a show for a new
# property called outstanding tags (this is the information currently
# shown by the drivers procfs routine).
# --------------------------------------------
#
diff -Nru a/drivers/scsi/53c700.c b/drivers/scsi/53c700.c
--- a/drivers/scsi/53c700.c	Sat May  3 14:27:29 2003
+++ b/drivers/scsi/53c700.c	Sat May  3 14:27:29 2003
@@ -172,6 +172,17 @@
 STATIC void NCR_700_chip_reset(struct Scsi_Host *host);
 STATIC int NCR_700_slave_configure(Scsi_Device *SDpnt);
 STATIC void NCR_700_slave_destroy(Scsi_Device *SDpnt);
+STATIC ssize_t NCR_700_device_property_show(struct scsi_device * dev,
+					    char * buf,
+					    struct attribute *attr);
+STATIC ssize_t NCR_700_device_property_store(struct scsi_device * dev,
+					     const char * buf,
+					     size_t count,
+					     struct attribute *attr);
+
+struct device_attribute NCR_700_sdev_attrs[] = {
+	{ .attr = { .name = "outstanding_tags", .mode = S_IRUGO }, },
+};
 
 static char *NCR_700_phase[] = {
 	"",
@@ -278,6 +289,8 @@
 	tpnt->proc_info = NCR_700_proc_directory_info;
 	tpnt->slave_configure = NCR_700_slave_configure;
 	tpnt->slave_destroy = NCR_700_slave_destroy;
+	tpnt->device_property_show = NCR_700_device_property_show;
+	tpnt->device_property_store = NCR_700_device_property_store;
 	tpnt->use_blk_tcq = 1;
 	tpnt->highmem_io = 1;
 	
@@ -2007,6 +2020,8 @@
 STATIC int
 NCR_700_slave_configure(Scsi_Device *SDp)
 {
+	int i;
+
 	/* to do here: allocate memory; build a queue_full list */
 	if(SDp->tagged_supported) {
 		/* do TCQ stuff here */
@@ -2014,14 +2029,63 @@
 		/* initialise to default depth */
 		scsi_adjust_queue_depth(SDp, 0, SDp->host->cmd_per_lun);
 	}
+
+	for (i = 0; i < ARRAY_SIZE(NCR_700_sdev_attrs); i++)
+		device_create_file(&SDp->sdev_driverfs_dev,
+				   &NCR_700_sdev_attrs[i]);
 	return 0;
 }
 
 STATIC void
 NCR_700_slave_destroy(Scsi_Device *SDp)
 {
-	/* to do here: deallocate memory */
+	int i;
+
+	/* FIXME: slave_destroy can be called if no driverfs entry
+	 * has been created.  We really need a slave_unconfigure for
+	 * this type of thing */
+	if(SDp->sdev_driverfs_dev.node.next != NULL)
+		for (i = 0; i < ARRAY_SIZE(NCR_700_sdev_attrs); i++)
+			device_remove_file(&SDp->sdev_driverfs_dev,
+					   &NCR_700_sdev_attrs[i]);
+
+}
+
+STATIC ssize_t
+NCR_700_device_property_show(struct scsi_device * sdev, char * buf,
+			     struct attribute *attr)
+{
+	char *name = attr->name;
+	int ret = 0;
+
+	if (strcmp(name, "outstanding_tags") == 0) {
+		ret = snprintf(buf, 20, "%d\n", NCR_700_get_depth(sdev));
+	}
+	return ret;
 }
+
+STATIC ssize_t
+NCR_700_device_property_store(struct scsi_device * sdev, const char * buf,
+			      size_t count, struct attribute *attr)
+{
+	char *name = attr->name;
+
+	if (strcmp(name, "queue_depth") == 0) {
+		long new_depth = simple_strtol(buf, NULL, 0);
+		if (new_depth > NCR_700_MAX_TAGS) {
+			return -EINVAL;
+		} else if (new_depth < 0) {
+			/* FIXME: turn off tags need to quiesce the
+			 * device to do this */
+			return -EINVAL;
+		} else {
+			scsi_adjust_queue_depth(sdev, 0, new_depth);
+		}
+	}
+	return 0;
+}
+
+
 
 EXPORT_SYMBOL(NCR_700_detect);
 EXPORT_SYMBOL(NCR_700_release);

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