Round II on sysfs attributes

Round II on sysfs attributes

[James Bottomley]

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


The attached patch makes the sysfs attributes a property of the host
template (I can't really see a reason why the actual attributes need to
change per host, so I think the template is the better place for them).

I've also provided helper functions to modify the attributes in the LLD
init routines.

An illustration of how this works for the 53c700 will follow.

James


[-- Attachment #2: tmp.diff --]
[-- Type: text/plain, Size: 9483 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.1185  -> 1.1189 
#	drivers/scsi/hosts.c	1.65    -> 1.66   
#	drivers/scsi/scsi_sysfs.c	1.16    -> 1.19   
#	 drivers/scsi/scsi.h	1.79    -> 1.80   
#	drivers/scsi/hosts.h	1.64    -> 1.65   
#	drivers/scsi/scsi_priv.h	1.7     -> 1.10   
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 03/05/21	jejb@mulgrave.(none)	1.1186
# SCSI: Move shost_attrs and sdev_attrs into the host template
# 
# This would allow the LLD to clone or modify them at slave_configure time. 
# 
# Also expose routines to facilitate this
# --------------------------------------------
# 03/05/22	jejb@mulgrave.(none)	1.1187
# Update modify functions to take the template not the host
# --------------------------------------------
# 03/05/23	jejb@mulgrave.(none)	1.1188
# More sysfs attribute updates
# --------------------------------------------
# 03/05/23	jejb@mulgrave.(none)	1.1189
# do a bit more adjusting of what should be in scsi.h and what in scsi_priv.h
# --------------------------------------------
#
diff -Nru a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
--- a/drivers/scsi/hosts.c	Fri May 23 12:35:11 2003
+++ b/drivers/scsi/hosts.c	Fri May 23 12:35:11 2003
@@ -321,7 +321,12 @@
            shost_tp->eh_host_reset_handler == NULL) {
 		printk(KERN_ERR "ERROR: SCSI host `%s' has no error handling\nERROR: This is not a safe way to run your SCSI host\nERROR: The error handling must be added to this driver\n", shost_tp->proc_name);
 		dump_stack();
-        } 
+        }
+	if(shost_tp->shost_attrs == NULL)
+		/* if its not set in the template, use the default */
+		 shost_tp->shost_attrs = scsi_sysfs_shost_attrs;
+	if(shost_tp->sdev_attrs == NULL)
+		 shost_tp->sdev_attrs = scsi_sysfs_sdev_attrs;
 	gfp_mask = GFP_KERNEL;
 	if (shost_tp->unchecked_isa_dma && xtr_bytes)
 		gfp_mask |= __GFP_DMA;
diff -Nru a/drivers/scsi/hosts.h b/drivers/scsi/hosts.h
--- a/drivers/scsi/hosts.h	Fri May 23 12:35:11 2003
+++ b/drivers/scsi/hosts.h	Fri May 23 12:35:11 2003
@@ -356,6 +356,16 @@
      * FIXME: This should probably be a value in the template */
     #define SCSI_DEFAULT_HOST_BLOCKED	7
 
+    /*
+     * pointer to the sysfs class properties for this host
+     */
+    struct class_device_attribute **shost_attrs;
+
+    /*
+     * Pointer to the SCSI device properties for this host
+     */
+    struct device_attribute **sdev_attrs;
+
 } Scsi_Host_Template;
 
 /*
diff -Nru a/drivers/scsi/scsi.h b/drivers/scsi/scsi.h
--- a/drivers/scsi/scsi.h	Fri May 23 12:35:11 2003
+++ b/drivers/scsi/scsi.h	Fri May 23 12:35:11 2003
@@ -683,4 +683,9 @@
 
 int scsi_set_medium_removal(Scsi_Device *dev, char state);
 
+extern int scsi_sysfs_modify_sdev_attribute(struct device_attribute ***dev_attrs,
+					    struct device_attribute *attr);
+extern int scsi_sysfs_modify_shost_attribute(struct class_device_attribute ***class_attrs,
+					     struct class_device_attribute *attr);
+
 #endif /* _SCSI_H */
diff -Nru a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h
--- a/drivers/scsi/scsi_priv.h	Fri May 23 12:35:11 2003
+++ b/drivers/scsi/scsi_priv.h	Fri May 23 12:35:11 2003
@@ -131,4 +131,9 @@
 extern int scsi_sysfs_register(void);
 extern void scsi_sysfs_unregister(void);
 
+/* definitions for the linker default sections covering the host
+ * class and device attributes */
+extern struct class_device_attribute *scsi_sysfs_shost_attrs[];
+extern struct device_attribute *scsi_sysfs_sdev_attrs[];
+
 #endif /* _SCSI_PRIV_H */
diff -Nru a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
--- a/drivers/scsi/scsi_sysfs.c	Fri May 23 12:35:11 2003
+++ b/drivers/scsi/scsi_sysfs.c	Fri May 23 12:35:11 2003
@@ -45,12 +45,13 @@
 shost_rd_attr(sg_tablesize, "%hu\n");
 shost_rd_attr(unchecked_isa_dma, "%d\n");
 
-static struct class_device_attribute *const shost_attrs[] = {
+struct class_device_attribute *scsi_sysfs_shost_attrs[] = {
 	&class_device_attr_unique_id,
 	&class_device_attr_host_busy,
 	&class_device_attr_cmd_per_lun,
 	&class_device_attr_sg_tablesize,
 	&class_device_attr_unchecked_isa_dma,
+	NULL
 };
 
 static struct class shost_class = {
@@ -243,7 +244,8 @@
 
 static DEVICE_ATTR(rescan, S_IRUGO | S_IWUSR, show_rescan_field, store_rescan_field)
 
-static struct device_attribute * const sdev_attrs[] = {
+/* Default template for device attributes.  May NOT be modified */
+struct device_attribute *scsi_sysfs_sdev_attrs[] = {
 	&dev_attr_device_blocked,
 	&dev_attr_queue_depth,
 	&dev_attr_type,
@@ -254,6 +256,7 @@
 	&dev_attr_rev,
 	&dev_attr_online,
 	&dev_attr_rescan,
+	NULL
 };
 
 static void scsi_device_release(struct device *dev)
@@ -287,9 +290,9 @@
 	if (error)
 		return error;
 
-	for (i = 0; !error && i < ARRAY_SIZE(sdev_attrs); i++)
+	for (i = 0; !error && sdev->host->hostt->sdev_attrs[i] != NULL; i++)
 		error = device_create_file(&sdev->sdev_driverfs_dev,
-					   sdev_attrs[i]);
+					   sdev->host->hostt->sdev_attrs[i]);
 
 	if (error)
 		scsi_device_unregister(sdev);
@@ -305,8 +308,8 @@
 {
 	int i;
 
-	for (i = 0; i < ARRAY_SIZE(sdev_attrs); i++)
-		device_remove_file(&sdev->sdev_driverfs_dev, sdev_attrs[i]);
+	for (i = 0; sdev->host->hostt->sdev_attrs[i] != NULL; i++)
+		device_remove_file(&sdev->sdev_driverfs_dev, sdev->host->hostt->sdev_attrs[i]);
 	device_unregister(&sdev->sdev_driverfs_dev);
 }
 
@@ -357,9 +360,9 @@
 	if (error)
 		goto clean_device;
 
-	for (i = 0; !error && i < ARRAY_SIZE(shost_attrs); i++)
+	for (i = 0; !error && shost->hostt->shost_attrs[i] != NULL; i++)
 		error = class_device_create_file(&shost->class_dev,
-					   shost_attrs[i]);
+					   shost->hostt->shost_attrs[i]);
 	if (error)
 		goto clean_class;
 
@@ -383,3 +386,104 @@
 	device_del(&shost->host_gendev);
 }
 
+/** scsi_sysfs_modify_shost_attribute - modify or add a host class attribute
+ *
+ * @class_attrs:host class attribute list to be added to or modified
+ * @attr:	individual attribute to change or added
+ *
+ * returns zero if successful or error if not
+ **/
+int scsi_sysfs_modify_shost_attribute(struct class_device_attribute ***class_attrs,
+				      struct class_device_attribute *attr)
+{
+	int modify = 0;
+	int num_attrs;
+
+	if(*class_attrs == NULL)
+		*class_attrs = scsi_sysfs_shost_attrs;
+
+	for(num_attrs=0; (*class_attrs)[num_attrs] != NULL; num_attrs++)
+		if(strcmp((*class_attrs)[num_attrs]->attr.name, attr->attr.name) == 0)
+			modify = num_attrs;
+
+	if(*class_attrs == scsi_sysfs_shost_attrs || !modify) {
+		struct class_device_attribute **tmp_attrs = kmalloc(sizeof(struct class_device_attribute)*(num_attrs + (modify ? 0 : 1)), GFP_KERNEL);
+		if(tmp_attrs == NULL)
+			return -ENOMEM;
+		memcpy(tmp_attrs, *class_attrs, sizeof(struct class_device_attribute)*num_attrs);
+		if(*class_attrs != scsi_sysfs_shost_attrs)
+			kfree(*class_attrs);
+		*class_attrs = tmp_attrs;
+	}
+	if(modify) {
+		/* spare the caller from having to copy things it's
+		 * not interested in */
+		struct class_device_attribute *old_attr =
+			(*class_attrs)[modify];
+		/* extend permissions */
+		attr->attr.mode |= old_attr->attr.mode;
+
+		/* override null show/store with default */
+		if(attr->show == NULL)
+			attr->show = old_attr->show;
+		if(attr->store == NULL)
+			attr->store = old_attr->store;
+		(*class_attrs)[modify] = attr;
+	} else {
+		(*class_attrs)[num_attrs] = attr;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(scsi_sysfs_modify_shost_attribute);
+
+/** scsi_sysfs_modify_sdev_attribute - modify or add a host device attribute
+ *
+ * @dev_attrs:	pointer to the attribute list to be added to or modified
+ * @attr:	individual attribute to change or added
+ *
+ * returns zero if successful or error if not
+ **/
+int scsi_sysfs_modify_sdev_attribute(struct device_attribute ***dev_attrs,
+				     struct device_attribute *attr)
+{
+	int modify = 0;
+	int num_attrs;
+
+	if(*dev_attrs == NULL)
+		*dev_attrs = scsi_sysfs_sdev_attrs;
+
+	for(num_attrs=0; (*dev_attrs)[num_attrs] != NULL; num_attrs++)
+		if(strcmp((*dev_attrs)[num_attrs]->attr.name, attr->attr.name) == 0)
+			modify = num_attrs;
+
+	if(*dev_attrs == scsi_sysfs_sdev_attrs || !modify) {
+		struct device_attribute **tmp_attrs = kmalloc(sizeof(struct device_attribute)*(num_attrs + (modify ? 0 : 1)), GFP_KERNEL);
+		if(tmp_attrs == NULL)
+			return -ENOMEM;
+		memcpy(tmp_attrs, *dev_attrs, sizeof(struct device_attribute)*num_attrs);
+		if(*dev_attrs != scsi_sysfs_sdev_attrs)
+			kfree(*dev_attrs);
+		*dev_attrs = tmp_attrs;
+	}
+	if(modify) {
+		/* spare the caller from having to copy things it's
+		 * not interested in */
+		struct device_attribute *old_attr =
+			(*dev_attrs)[modify];
+		/* extend permissions */
+		attr->attr.mode |= old_attr->attr.mode;
+
+		/* override null show/store with default */
+		if(attr->show == NULL)
+			attr->show = old_attr->show;
+		if(attr->store == NULL)
+			attr->store = old_attr->store;
+		(*dev_attrs)[modify] = attr;
+	} else {
+		(*dev_attrs)[num_attrs] = attr;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(scsi_sysfs_modify_sdev_attribute);

Re: Round II on sysfs attributes

[Patrick Mansfield]

On Fri, May 23, 2003 at 12:38:24PM -0400, James Bottomley wrote:
> 
> The attached patch makes the sysfs attributes a property of the host
> template (I can't really see a reason why the actual attributes need to
> change per host, so I think the template is the better place for them).
> 
> I've also provided helper functions to modify the attributes in the LLD
> init routines.
> 
> An illustration of how this works for the 53c700 will follow.
> 
> James

Any examples of potential LLDD specific host attributes?

Looks good for modifying attributes.

For additional attributes, why not an LLDD specific function to add and
remove them?

> diff -Nru a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
> --- a/drivers/scsi/hosts.c	Fri May 23 12:35:11 2003
> +++ b/drivers/scsi/hosts.c	Fri May 23 12:35:11 2003
> @@ -321,7 +321,12 @@
>             shost_tp->eh_host_reset_handler == NULL) {
>  		printk(KERN_ERR "ERROR: SCSI host `%s' has no error handling\nERROR: This is not a safe way to run your SCSI host\nERROR: The error handling must be added to this driver\n", shost_tp->proc_name);
>  		dump_stack();
> -        } 
> +        }
> +	if(shost_tp->shost_attrs == NULL)
> +		/* if its not set in the template, use the default */
> +		 shost_tp->shost_attrs = scsi_sysfs_shost_attrs;
> +	if(shost_tp->sdev_attrs == NULL)
> +		 shost_tp->sdev_attrs = scsi_sysfs_sdev_attrs;
>  	gfp_mask = GFP_KERNEL;
>  	if (shost_tp->unchecked_isa_dma && xtr_bytes)
>  		gfp_mask |= __GFP_DMA;

Code style nit: in the above and elsewhere use "if (", "for (" instead of
"if(" or  "for(".

> +int scsi_sysfs_modify_shost_attribute(struct class_device_attribute ***class_attrs,
> +				      struct class_device_attribute *attr)
> +{
> +	int modify = 0;
> +	int num_attrs;
> +
> +	if(*class_attrs == NULL)
> +		*class_attrs = scsi_sysfs_shost_attrs;
> +
> +	for(num_attrs=0; (*class_attrs)[num_attrs] != NULL; num_attrs++)
> +		if(strcmp((*class_attrs)[num_attrs]->attr.name, attr->attr.name) == 0)
> +			modify = num_attrs;
> +
> +	if(*class_attrs == scsi_sysfs_shost_attrs || !modify) {
> +		struct class_device_attribute **tmp_attrs = kmalloc(sizeof(struct class_device_attribute)*(num_attrs + (modify ? 0 : 1)), GFP_KERNEL);
> +		if(tmp_attrs == NULL)
> +			return -ENOMEM;
> +		memcpy(tmp_attrs, *class_attrs, sizeof(struct class_device_attribute)*num_attrs);
> +		if(*class_attrs != scsi_sysfs_shost_attrs)
> +			kfree(*class_attrs);
> +		*class_attrs = tmp_attrs;

For completeness, an allocated *class_attrs should be freed on LLDD module
exit. Same for the sdev attrs.

-- Patrick Mansfield

Re: Round II on sysfs attributes

[Mike Anderson]

James Bottomley [James.Bottomley@SteelEye.com] wrote:
>  static void scsi_device_release(struct device *dev)
> @@ -287,9 +290,9 @@
>  	if (error)
>  		return error;
>  
> -	for (i = 0; !error && i < ARRAY_SIZE(sdev_attrs); i++)
> +	for (i = 0; !error && sdev->host->hostt->sdev_attrs[i] != NULL; i++)
>  		error = device_create_file(&sdev->sdev_driverfs_dev,
> -					   sdev_attrs[i]);
> +					   sdev->host->hostt->sdev_attrs[i]);
>  
>  	if (error)
>  		scsi_device_unregister(sdev);

What about this style of bounds check.
for (i = 0; !error && sdev->host->hostt->sdev_attrs[i]; i++)

> +/** scsi_sysfs_modify_shost_attribute - modify or add a host class attribute
> + *
> + * @class_attrs:host class attribute list to be added to or modified
> + * @attr:	individual attribute to change or added
> + *
> + * returns zero if successful or error if not
> + **/
> +int scsi_sysfs_modify_shost_attribute(struct class_device_attribute ***class_attrs,
> +				      struct class_device_attribute *attr)
> +{
> +	int modify = 0;
> +	int num_attrs;
> +
> +	if(*class_attrs == NULL)
> +		*class_attrs = scsi_sysfs_shost_attrs;
> +
> +	for(num_attrs=0; (*class_attrs)[num_attrs] != NULL; num_attrs++)
> +		if(strcmp((*class_attrs)[num_attrs]->attr.name, attr->attr.name) == 0)
> +			modify = num_attrs;
> +
> +	if(*class_attrs == scsi_sysfs_shost_attrs || !modify) {
> +		struct class_device_attribute **tmp_attrs = kmalloc(sizeof(struct class_device_attribute)*(num_attrs + (modify ? 0 : 1)), GFP_KERNEL);

kfree of this memory in driver?

> +		if(tmp_attrs == NULL)
> +			return -ENOMEM;
> +		memcpy(tmp_attrs, *class_attrs, sizeof(struct class_device_attribute)*num_attrs);
> +		if(*class_attrs != scsi_sysfs_shost_attrs)
> +			kfree(*class_attrs);
> +		*class_attrs = tmp_attrs;
> +	}
> +	if(modify) {
> +		/* spare the caller from having to copy things it's
> +		 * not interested in */
> +		struct class_device_attribute *old_attr =
> +			(*class_attrs)[modify];
> +		/* extend permissions */
> +		attr->attr.mode |= old_attr->attr.mode;
> +
> +		/* override null show/store with default */
> +		if(attr->show == NULL)
> +			attr->show = old_attr->show;
> +		if(attr->store == NULL)
> +			attr->store = old_attr->store;
> +		(*class_attrs)[modify] = attr;
> +	} else {
> +		(*class_attrs)[num_attrs] = attr;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(scsi_sysfs_modify_shost_attribute);
> +

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

Re: Round II on sysfs attributes

[James Bottomley]

On Fri, 2003-05-23 at 14:30, Patrick Mansfield wrote:
> On Fri, May 23, 2003 at 12:38:24PM -0400, James Bottomley wrote:
> > 
> > The attached patch makes the sysfs attributes a property of the host
> > template (I can't really see a reason why the actual attributes need to
> > change per host, so I think the template is the better place for them).
> > 
> > I've also provided helper functions to modify the attributes in the LLD
> > init routines.
> > 
> > An illustration of how this works for the 53c700 will follow.
> > 
> > James
> 
> Any examples of potential LLDD specific host attributes?

Certainly things like the firmware SEEPROM loading that aic7xxx does. 
Possibly global (or per host) resource counts for HBA's that have single
issue queue (again, aic7xxx; and even the 53c700 has this).

> Looks good for modifying attributes.
> 
> For additional attributes, why not an LLDD specific function to add and
> remove them?

That would require knowledge of what the "official" host attributes are,
so it seemed to me that leaving it up to the routine to determine
whether it's an addition or an override was more appropriate.  If we
have a separate routine for adds it would have to have an error case for
adding an existing resource...

James

Re: Round II on sysfs attributes

[James Bottomley]

On Fri, 2003-05-23 at 14:51, Mike Anderson wrote:
> James Bottomley [James.Bottomley@SteelEye.com] wrote:
> >  static void scsi_device_release(struct device *dev)
> > @@ -287,9 +290,9 @@
> >  	if (error)
> >  		return error;
> >  
> > -	for (i = 0; !error && i < ARRAY_SIZE(sdev_attrs); i++)
> > +	for (i = 0; !error && sdev->host->hostt->sdev_attrs[i] != NULL; i++)
> >  		error = device_create_file(&sdev->sdev_driverfs_dev,
> > -					   sdev_attrs[i]);
> > +					   sdev->host->hostt->sdev_attrs[i]);
> >  
> >  	if (error)
> >  		scsi_device_unregister(sdev);
> 
> What about this style of bounds check.
> for (i = 0; !error && sdev->host->hostt->sdev_attrs[i]; i++)

It's just personal, but I prefer the explicit check against an object
rather than relying on the promotional to logical value rules.  I don't
see that it matters much either way.


> kfree of this memory in driver?

Yes, tricky issue.  I'd really rather it be done at template release
time (which we don't have) since we've sort of moved away from template
register/unregister.

I don't like an explicit kfree in the driver since I was trying to hide
the memory manipluations from the LLDs.  I suppose we could make the
model per host, rather than per template and use the add/remove host
interfaces, but this property is really a per template property. I'm
open to suggestions.

James