[PATCH] 0/2 use sysfs bus dev_attrs for scsi_device attributes

[PATCH] 0/2 use sysfs bus dev_attrs for scsi_device attributes

[Patrick Mansfield]

A couple patches so that when pending changes to sysfs/hotplug are made,
patches discussed here:

	http://marc.theaimsgroup.com/?t=110939742700002&r=1&w=2

We will get the hotplug event for a scsi_device only after all the default
scsi_device attributes are created.

The removal of the attr_changed_internally() was done so queue_type can
more easily be used as a default attribute (using the bus dev_attrs, there
is no way for some devices to create it read only, and for others to
create it as read write).

Tested with qla2300 and qla1280 drivers - neither currently support
writable queue_depth or queue_type.

The attr_overridden (and post these patches, scsi_sysfs_sdev_attrs) should
someday be removed, the only attribute being overridden (versus a host
specific scsi_device attribute) today is queue_depth, and we have
shost->change_queue_depth that can be used instead.

-- Patrick Mansfield

[PATCH] 1/2 remove attr_changed_internally

[Patrick Mansfield]

Get rid of the attr_changed_internally(), and always create queue_type and
queue_depth as read/write, and then writes fail if not supported.

Signed-off-by: Patrick Mansfield <patmans@us.ibm.com>

--- linux-2.6.11/drivers/scsi/scsi_sysfs.c	2005-03-02 02:59:50.000000000 -0800
+++ sattrs-linux-2.6.11/drivers/scsi/scsi_sysfs.c	2005-03-02 09:27:15.000000000 -0800
@@ -312,7 +312,6 @@
  * 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 (vendor, "%.8s\n");
@@ -392,41 +391,9 @@
 
 static DEVICE_ATTR(state, S_IRUGO | S_IWUSR, show_state_field, store_state_field);
 
-static ssize_t
-show_queue_type_field(struct device *dev, char *buf)
-{
-	struct scsi_device *sdev = to_scsi_device(dev);
-	const char *name = "none";
-
-	if (sdev->ordered_tags)
-		name = "ordered";
-	else if (sdev->simple_tags)
-		name = "simple";
-
-	return snprintf(buf, 20, "%s\n", name);
-}
-
-static DEVICE_ATTR(queue_type, S_IRUGO, show_queue_type_field, NULL);
-
-
-/* Default template for device attributes.  May NOT be modified */
-static struct device_attribute *scsi_sysfs_sdev_attrs[] = {
-	&dev_attr_device_blocked,
-	&dev_attr_queue_depth,
-	&dev_attr_queue_type,
-	&dev_attr_type,
-	&dev_attr_scsi_level,
-	&dev_attr_vendor,
-	&dev_attr_model,
-	&dev_attr_rev,
-	&dev_attr_rescan,
-	&dev_attr_delete,
-	&dev_attr_state,
-	&dev_attr_timeout,
-	NULL
-};
+sdev_show_function (queue_depth, "%d\n");
 
-static ssize_t sdev_store_queue_depth_rw(struct device *dev, const char *buf,
+static ssize_t sdev_store_queue_depth(struct device *dev, const char *buf,
 					 size_t count)
 {
 	int depth, retval;
@@ -448,11 +415,25 @@
 	return count;
 }
 
-static struct device_attribute sdev_attr_queue_depth_rw =
+static struct device_attribute dev_attr_queue_depth =
 	__ATTR(queue_depth, S_IRUGO | S_IWUSR, sdev_show_queue_depth,
-	       sdev_store_queue_depth_rw);
+	       sdev_store_queue_depth);
 
-static ssize_t sdev_store_queue_type_rw(struct device *dev, const char *buf,
+static ssize_t
+show_queue_type_field(struct device *dev, char *buf)
+{
+	struct scsi_device *sdev = to_scsi_device(dev);
+	const char *name = "none";
+
+	if (sdev->ordered_tags)
+		name = "ordered";
+	else if (sdev->simple_tags)
+		name = "simple";
+
+	return snprintf(buf, 20, "%s\n", name);
+}
+
+static ssize_t sdev_store_queue_type(struct device *dev, const char *buf,
 					size_t count)
 {
 	struct scsi_device *sdev = to_scsi_device(dev);
@@ -480,23 +461,26 @@
 	return count;
 }
 
-static struct device_attribute sdev_attr_queue_type_rw =
+static struct device_attribute dev_attr_queue_type =
 	__ATTR(queue_type, S_IRUGO | S_IWUSR, show_queue_type_field,
-	       sdev_store_queue_type_rw);
-
-static struct device_attribute *attr_changed_internally(
-		struct Scsi_Host *shost,
-		struct device_attribute * attr)
-{
-	if (!strcmp("queue_depth", attr->attr.name)
-	    && shost->hostt->change_queue_depth)
-		return &sdev_attr_queue_depth_rw;
-	else if (!strcmp("queue_type", attr->attr.name)
-	    && shost->hostt->change_queue_type)
-		return &sdev_attr_queue_type_rw;
-	return attr;
-}
+	       sdev_store_queue_type);
 
+/* Default template for device attributes.  May NOT be modified */
+static struct device_attribute *scsi_sysfs_sdev_attrs[] = {
+	&dev_attr_device_blocked,
+	&dev_attr_queue_depth,
+	&dev_attr_queue_type,
+	&dev_attr_type,
+	&dev_attr_scsi_level,
+	&dev_attr_vendor,
+	&dev_attr_model,
+	&dev_attr_rev,
+	&dev_attr_rescan,
+	&dev_attr_delete,
+	&dev_attr_state,
+	&dev_attr_timeout,
+	NULL
+};
 
 static struct device_attribute *attr_overridden(
 		struct device_attribute **attrs,
@@ -602,10 +586,8 @@
 	for (i = 0; scsi_sysfs_sdev_attrs[i]; i++) {
 		if (!attr_overridden(sdev->host->hostt->sdev_attrs,
 					scsi_sysfs_sdev_attrs[i])) {
-			struct device_attribute * attr = 
-				attr_changed_internally(sdev->host, 
-							scsi_sysfs_sdev_attrs[i]);
-			error = device_create_file(&sdev->sdev_gendev, attr);
+			error = device_create_file(&sdev->sdev_gendev,
+						   scsi_sysfs_sdev_attrs[i]);
 			if (error) {
 				scsi_remove_device(sdev);
 				goto out;

[PATCH] 2/2 Use bus dev_attrs to create scsi_device attributes

[Patrick Mansfield]

Use bus dev_attrs to create the default scsi_device attributes.

Note sdev_default_attrs is not a pointer to an array (like
scsi_sysfs_sdev_attrs), and so DEVICE_ATTR's can be removed, and __ATTR
used instaed.

Signed-off-by: Patrick Mansfield <patmans@us.ibm.com>

--- sattrs-linux-2.6.11/drivers/scsi/s1-scsi_sysfs.c	2005-03-02 09:58:21.000000000 -0800
+++ sattrs-linux-2.6.11/drivers/scsi/scsi_sysfs.c	2005-03-02 10:22:25.000000000 -0800
@@ -193,40 +193,6 @@
 	.release	= scsi_device_cls_release,
 };
 
-/* all probing is done in the individual ->probe routines */
-static int scsi_bus_match(struct device *dev, struct device_driver *gendrv)
-{
-	struct scsi_device *sdp = to_scsi_device(dev);
-	if (sdp->no_uld_attach)
-		return 0;
-	return (sdp->inq_periph_qual == SCSI_INQ_PQ_CON)? 1: 0;
-}
-
-struct bus_type scsi_bus_type = {
-        .name		= "scsi",
-        .match		= scsi_bus_match,
-};
-
-int scsi_sysfs_register(void)
-{
-	int error;
-
-	error = bus_register(&scsi_bus_type);
-	if (!error) {
-		error = class_register(&sdev_class);
-		if (error)
-			bus_unregister(&scsi_bus_type);
-	}
-
-	return error;
-}
-
-void scsi_sysfs_unregister(void)
-{
-	class_unregister(&sdev_class);
-	bus_unregister(&scsi_bus_type);
-}
-
 /*
  * sdev_show_function: macro to create an attr function that can be used to
  * show a non-bit field.
@@ -245,8 +211,7 @@
  * read only field.
  */
 #define sdev_rd_attr(field, format_string)				\
-	sdev_show_function(field, format_string)			\
-static DEVICE_ATTR(field, S_IRUGO, sdev_show_##field, NULL);
+	sdev_show_function(field, format_string)
 
 
 /*
@@ -263,8 +228,7 @@
 	sdev = to_scsi_device(dev);					\
 	snscanf (buf, 20, format_string, &sdev->field);			\
 	return count;							\
-}									\
-static DEVICE_ATTR(field, S_IRUGO | S_IWUSR, sdev_show_##field, sdev_store_##field);
+}
 
 /* Currently we don't export bit fields, but we might in future,
  * so leave this code in */
@@ -288,8 +252,7 @@
 		ret = count;						\
 	}								\
 	return ret;							\
-}									\
-static DEVICE_ATTR(field, S_IRUGO | S_IWUSR, sdev_show_##field, sdev_store_##field);
+}
 
 /*
  * scsi_sdev_check_buf_bit: return 0 if buf is "0", return 1 if buf is "1",
@@ -336,15 +299,13 @@
 	sdev->timeout = timeout * HZ;
 	return count;
 }
-static DEVICE_ATTR(timeout, S_IRUGO | S_IWUSR, sdev_show_timeout, sdev_store_timeout);
 
 static ssize_t
-store_rescan_field (struct device *dev, const char *buf, size_t count) 
+sdev_store_rescan (struct device *dev, const char *buf, size_t count) 
 {
 	scsi_rescan_device(dev);
 	return count;
 }
-static DEVICE_ATTR(rescan, S_IWUSR, NULL, store_rescan_field);
 
 static ssize_t sdev_store_delete(struct device *dev, const char *buf,
 				 size_t count)
@@ -352,10 +313,9 @@
 	scsi_remove_device(to_scsi_device(dev));
 	return count;
 };
-static DEVICE_ATTR(delete, S_IWUSR, NULL, sdev_store_delete);
 
 static ssize_t
-store_state_field(struct device *dev, const char *buf, size_t count)
+sdev_store_state(struct device *dev, const char *buf, size_t count)
 {
 	int i;
 	struct scsi_device *sdev = to_scsi_device(dev);
@@ -378,7 +338,7 @@
 }
 
 static ssize_t
-show_state_field(struct device *dev, char *buf)
+sdev_show_state(struct device *dev, char *buf)
 {
 	struct scsi_device *sdev = to_scsi_device(dev);
 	const char *name = scsi_device_state_name(sdev->sdev_state);
@@ -389,8 +349,6 @@
 	return snprintf(buf, 20, "%s\n", name);
 }
 
-static DEVICE_ATTR(state, S_IRUGO | S_IWUSR, show_state_field, store_state_field);
-
 sdev_show_function (queue_depth, "%d\n");
 
 static ssize_t sdev_store_queue_depth(struct device *dev, const char *buf,
@@ -420,7 +378,7 @@
 	       sdev_store_queue_depth);
 
 static ssize_t
-show_queue_type_field(struct device *dev, char *buf)
+sdev_show_queue_type(struct device *dev, char *buf)
 {
 	struct scsi_device *sdev = to_scsi_device(dev);
 	const char *name = "none";
@@ -461,25 +419,29 @@
 	return count;
 }
 
-static struct device_attribute dev_attr_queue_type =
-	__ATTR(queue_type, S_IRUGO | S_IWUSR, show_queue_type_field,
-	       sdev_store_queue_type);
+#define SDEV_ATTR_RO(_name) __ATTR(_name, S_IRUGO, sdev_show_##_name, NULL)
+#define SDEV_ATTR_WO(_name) __ATTR(_name, S_IWUSR, NULL, sdev_store_##_name)
+#define SDEV_ATTR_RW(_name) __ATTR(_name, S_IRUGO | S_IWUSR, \
+				   sdev_show_##_name, sdev_store_##_name)
+
+/* Default scsi_device attributes, cannot be overidden */
+static struct device_attribute sdev_default_attrs[] = {
+	SDEV_ATTR_RO(device_blocked),
+	SDEV_ATTR_RW(queue_type),
+	SDEV_ATTR_RO(type),
+	SDEV_ATTR_RO(scsi_level),
+	SDEV_ATTR_RO(vendor),
+	SDEV_ATTR_RO(model),
+	SDEV_ATTR_RO(rev),
+	SDEV_ATTR_WO(rescan),
+	SDEV_ATTR_WO(delete),
+	SDEV_ATTR_RW(state),
+	SDEV_ATTR_RW(timeout),
+	__ATTR_NULL
+};
 
-/* Default template for device attributes.  May NOT be modified */
 static struct device_attribute *scsi_sysfs_sdev_attrs[] = {
-	&dev_attr_device_blocked,
-	&dev_attr_queue_depth,
-	&dev_attr_queue_type,
-	&dev_attr_type,
-	&dev_attr_scsi_level,
-	&dev_attr_vendor,
-	&dev_attr_model,
-	&dev_attr_rev,
-	&dev_attr_rescan,
-	&dev_attr_delete,
-	&dev_attr_state,
-	&dev_attr_timeout,
-	NULL
+	&dev_attr_queue_depth
 };
 
 static struct device_attribute *attr_overridden(
@@ -518,6 +480,41 @@
 	return device_create_file(dev, attr);
 }
 
+/* all probing is done in the individual ->probe routines */
+static int scsi_bus_match(struct device *dev, struct device_driver *gendrv)
+{
+	struct scsi_device *sdp = to_scsi_device(dev);
+	if (sdp->no_uld_attach)
+		return 0;
+	return (sdp->inq_periph_qual == SCSI_INQ_PQ_CON)? 1: 0;
+}
+
+struct bus_type scsi_bus_type = {
+        .name		= "scsi",
+        .match		= scsi_bus_match,
+	.dev_attrs	= sdev_default_attrs,
+};
+
+int scsi_sysfs_register(void)
+{
+	int error;
+
+	error = bus_register(&scsi_bus_type);
+	if (!error) {
+		error = class_register(&sdev_class);
+		if (error)
+			bus_unregister(&scsi_bus_type);
+	}
+
+	return error;
+}
+
+void scsi_sysfs_unregister(void)
+{
+	class_unregister(&sdev_class);
+	bus_unregister(&scsi_bus_type);
+}
+
 static void scsi_target_dev_release(struct device *dev)
 {
 	struct scsi_target *starget = to_scsi_target(dev);

Re: [PATCH] 2/2 Use bus dev_attrs to create scsi_device attributes

[Patrick Mansfield]

On Wed, Mar 02, 2005 at 11:46:47AM -0800, Patrick Mansfield wrote:
> Use bus dev_attrs to create the default scsi_device attributes.
> 
> Note sdev_default_attrs is not a pointer to an array (like
> scsi_sysfs_sdev_attrs), and so DEVICE_ATTR's can be removed, and __ATTR
> used instaed.

Any comments on this? Should I resend these patches?

Do we need a new driver interface that allows the driver to explicitly
send the hotplug after all attributes have been created?

Such a new interface would allow scsi to continue to create attributes
(based on the underlying HBA or transport capability) that are sometimes
read only, and sometimes read+write. The current driver interface does not
allow default attributes to have different read/write properties.

And scsi could keep its current host override ability (we can't use the
current override code with the default attributes).

In any case, we need changes so udev can stop waiting for sysfs attributes.

> Signed-off-by: Patrick Mansfield <patmans@us.ibm.com>
> 
> --- sattrs-linux-2.6.11/drivers/scsi/s1-scsi_sysfs.c	2005-03-02 09:58:21.000000000 -0800
> +++ sattrs-linux-2.6.11/drivers/scsi/scsi_sysfs.c	2005-03-02 10:22:25.000000000 -0800
> @@ -193,40 +193,6 @@
>  	.release	= scsi_device_cls_release,
>  };
>  
> -/* all probing is done in the individual ->probe routines */
> -static int scsi_bus_match(struct device *dev, struct device_driver *gendrv)
> -{
> -	struct scsi_device *sdp = to_scsi_device(dev);
> -	if (sdp->no_uld_attach)
> -		return 0;
> -	return (sdp->inq_periph_qual == SCSI_INQ_PQ_CON)? 1: 0;
> -}
> -
> -struct bus_type scsi_bus_type = {
> -        .name		= "scsi",
> -        .match		= scsi_bus_match,
> -};
> -
> -int scsi_sysfs_register(void)
> -{
> -	int error;
> -
> -	error = bus_register(&scsi_bus_type);
> -	if (!error) {
> -		error = class_register(&sdev_class);
> -		if (error)
> -			bus_unregister(&scsi_bus_type);
> -	}
> -
> -	return error;
> -}
> -
> -void scsi_sysfs_unregister(void)
> -{
> -	class_unregister(&sdev_class);
> -	bus_unregister(&scsi_bus_type);
> -}
> -
>  /*
>   * sdev_show_function: macro to create an attr function that can be used to
>   * show a non-bit field.
> @@ -245,8 +211,7 @@
>   * read only field.
>   */
>  #define sdev_rd_attr(field, format_string)				\
> -	sdev_show_function(field, format_string)			\
> -static DEVICE_ATTR(field, S_IRUGO, sdev_show_##field, NULL);
> +	sdev_show_function(field, format_string)
>  
>  
>  /*
> @@ -263,8 +228,7 @@
>  	sdev = to_scsi_device(dev);					\
>  	snscanf (buf, 20, format_string, &sdev->field);			\
>  	return count;							\
> -}									\
> -static DEVICE_ATTR(field, S_IRUGO | S_IWUSR, sdev_show_##field, sdev_store_##field);
> +}
>  
>  /* Currently we don't export bit fields, but we might in future,
>   * so leave this code in */
> @@ -288,8 +252,7 @@
>  		ret = count;						\
>  	}								\
>  	return ret;							\
> -}									\
> -static DEVICE_ATTR(field, S_IRUGO | S_IWUSR, sdev_show_##field, sdev_store_##field);
> +}
>  
>  /*
>   * scsi_sdev_check_buf_bit: return 0 if buf is "0", return 1 if buf is "1",
> @@ -336,15 +299,13 @@
>  	sdev->timeout = timeout * HZ;
>  	return count;
>  }
> -static DEVICE_ATTR(timeout, S_IRUGO | S_IWUSR, sdev_show_timeout, sdev_store_timeout);
>  
>  static ssize_t
> -store_rescan_field (struct device *dev, const char *buf, size_t count) 
> +sdev_store_rescan (struct device *dev, const char *buf, size_t count) 
>  {
>  	scsi_rescan_device(dev);
>  	return count;
>  }
> -static DEVICE_ATTR(rescan, S_IWUSR, NULL, store_rescan_field);
>  
>  static ssize_t sdev_store_delete(struct device *dev, const char *buf,
>  				 size_t count)
> @@ -352,10 +313,9 @@
>  	scsi_remove_device(to_scsi_device(dev));
>  	return count;
>  };
> -static DEVICE_ATTR(delete, S_IWUSR, NULL, sdev_store_delete);
>  
>  static ssize_t
> -store_state_field(struct device *dev, const char *buf, size_t count)
> +sdev_store_state(struct device *dev, const char *buf, size_t count)
>  {
>  	int i;
>  	struct scsi_device *sdev = to_scsi_device(dev);
> @@ -378,7 +338,7 @@
>  }
>  
>  static ssize_t
> -show_state_field(struct device *dev, char *buf)
> +sdev_show_state(struct device *dev, char *buf)
>  {
>  	struct scsi_device *sdev = to_scsi_device(dev);
>  	const char *name = scsi_device_state_name(sdev->sdev_state);
> @@ -389,8 +349,6 @@
>  	return snprintf(buf, 20, "%s\n", name);
>  }
>  
> -static DEVICE_ATTR(state, S_IRUGO | S_IWUSR, show_state_field, store_state_field);
> -
>  sdev_show_function (queue_depth, "%d\n");
>  
>  static ssize_t sdev_store_queue_depth(struct device *dev, const char *buf,
> @@ -420,7 +378,7 @@
>  	       sdev_store_queue_depth);
>  
>  static ssize_t
> -show_queue_type_field(struct device *dev, char *buf)
> +sdev_show_queue_type(struct device *dev, char *buf)
>  {
>  	struct scsi_device *sdev = to_scsi_device(dev);
>  	const char *name = "none";
> @@ -461,25 +419,29 @@
>  	return count;
>  }
>  
> -static struct device_attribute dev_attr_queue_type =
> -	__ATTR(queue_type, S_IRUGO | S_IWUSR, show_queue_type_field,
> -	       sdev_store_queue_type);
> +#define SDEV_ATTR_RO(_name) __ATTR(_name, S_IRUGO, sdev_show_##_name, NULL)
> +#define SDEV_ATTR_WO(_name) __ATTR(_name, S_IWUSR, NULL, sdev_store_##_name)
> +#define SDEV_ATTR_RW(_name) __ATTR(_name, S_IRUGO | S_IWUSR, \
> +				   sdev_show_##_name, sdev_store_##_name)
> +
> +/* Default scsi_device attributes, cannot be overidden */
> +static struct device_attribute sdev_default_attrs[] = {
> +	SDEV_ATTR_RO(device_blocked),
> +	SDEV_ATTR_RW(queue_type),
> +	SDEV_ATTR_RO(type),
> +	SDEV_ATTR_RO(scsi_level),
> +	SDEV_ATTR_RO(vendor),
> +	SDEV_ATTR_RO(model),
> +	SDEV_ATTR_RO(rev),
> +	SDEV_ATTR_WO(rescan),
> +	SDEV_ATTR_WO(delete),
> +	SDEV_ATTR_RW(state),
> +	SDEV_ATTR_RW(timeout),
> +	__ATTR_NULL
> +};
>  
> -/* Default template for device attributes.  May NOT be modified */
>  static struct device_attribute *scsi_sysfs_sdev_attrs[] = {
> -	&dev_attr_device_blocked,
> -	&dev_attr_queue_depth,
> -	&dev_attr_queue_type,
> -	&dev_attr_type,
> -	&dev_attr_scsi_level,
> -	&dev_attr_vendor,
> -	&dev_attr_model,
> -	&dev_attr_rev,
> -	&dev_attr_rescan,
> -	&dev_attr_delete,
> -	&dev_attr_state,
> -	&dev_attr_timeout,
> -	NULL
> +	&dev_attr_queue_depth
>  };
>  
>  static struct device_attribute *attr_overridden(
> @@ -518,6 +480,41 @@
>  	return device_create_file(dev, attr);
>  }
>  
> +/* all probing is done in the individual ->probe routines */
> +static int scsi_bus_match(struct device *dev, struct device_driver *gendrv)
> +{
> +	struct scsi_device *sdp = to_scsi_device(dev);
> +	if (sdp->no_uld_attach)
> +		return 0;
> +	return (sdp->inq_periph_qual == SCSI_INQ_PQ_CON)? 1: 0;
> +}
> +
> +struct bus_type scsi_bus_type = {
> +        .name		= "scsi",
> +        .match		= scsi_bus_match,
> +	.dev_attrs	= sdev_default_attrs,
> +};
> +
> +int scsi_sysfs_register(void)
> +{
> +	int error;
> +
> +	error = bus_register(&scsi_bus_type);
> +	if (!error) {
> +		error = class_register(&sdev_class);
> +		if (error)
> +			bus_unregister(&scsi_bus_type);
> +	}
> +
> +	return error;
> +}
> +
> +void scsi_sysfs_unregister(void)
> +{
> +	class_unregister(&sdev_class);
> +	bus_unregister(&scsi_bus_type);
> +}
> +
>  static void scsi_target_dev_release(struct device *dev)
>  {
>  	struct scsi_target *starget = to_scsi_target(dev);

Re: [PATCH] 2/2 Use bus dev_attrs to create scsi_device attributes

[James Bottomley]

On Wed, 2005-03-16 at 14:45 -0800, Patrick Mansfield wrote:
> Any comments on this? Should I resend these patches?

Well, the basic comment is that there are a lot of features that SCSI
has that the driver core lacks:

1) Attribute overrides.  This is actually part of the published API for
SCSI
2) Ability to add extra attributes---several drivers use this
3) Ability to change attribute permissions based on capabilities.

I really rather like all of these features.

Initially when we did all this work for SCSI, there wasn't much
enthusiasm for incorporating these features into drivers/base.  You
could look and see what it would take to do this, and propose it to
Greg ... If he's happy, I'd have no problem moving over because now we
won't lose any functionality.

James

Re: [PATCH] 2/2 Use bus dev_attrs to create scsi_device attributes

[Greg KH]

On Thu, Mar 17, 2005 at 09:53:21AM -0500, James Bottomley wrote:
> On Wed, 2005-03-16 at 14:45 -0800, Patrick Mansfield wrote:
> > Any comments on this? Should I resend these patches?
> 
> Well, the basic comment is that there are a lot of features that SCSI
> has that the driver core lacks:
> 
> 1) Attribute overrides.  This is actually part of the published API for
> SCSI
> 2) Ability to add extra attributes---several drivers use this
> 3) Ability to change attribute permissions based on capabilities.
> 
> I really rather like all of these features.
> 
> Initially when we did all this work for SCSI, there wasn't much
> enthusiasm for incorporating these features into drivers/base.  You
> could look and see what it would take to do this, and propose it to
> Greg ... If he's happy, I'd have no problem moving over because now we
> won't lose any functionality.

I would be very interested in seeing the above stuff move into the
driver core.

thanks,

greg k-h

Re: [PATCH] 2/2 Use bus dev_attrs to create scsi_device attributes

[Kay Sievers]

On Thu, Mar 17, 2005 at 09:08:53AM -0800, Greg KH wrote:
> On Thu, Mar 17, 2005 at 09:53:21AM -0500, James Bottomley wrote:
> > On Wed, 2005-03-16 at 14:45 -0800, Patrick Mansfield wrote:
> > > Any comments on this? Should I resend these patches?
> > 
> > Well, the basic comment is that there are a lot of features that SCSI
> > has that the driver core lacks:
> > 
> > 1) Attribute overrides.  This is actually part of the published API for SCSI
> > 2) Ability to add extra attributes---several drivers use this
> > 3) Ability to change attribute permissions based on capabilities.

> I would be very interested in seeing the above stuff move into the
> driver core.

Point 3) seems to work for me. :)

Thanks,
Kay

---
sysfs: allow change of permissions for already created attributes

Signed-off-by: Kay Sievers <kay.sievers@vrfy.org>

===== fs/sysfs/file.c 1.23 vs edited =====
--- 1.23/fs/sysfs/file.c	2005-02-26 15:48:19 +01:00
+++ edited/fs/sysfs/file.c	2005-03-30 04:16:46 +02:00
@@ -428,6 +428,39 @@ int sysfs_update_file(struct kobject * k
 
 
 /**
+ * sysfs_chmod_file - update the modified mode value on an object attribute.
+ * @kobj: object we're acting for.
+ * @mode: file permissions.
+ *
+ */
+int sysfs_chmod_file(struct kobject *kobj, const struct attribute *attr)
+{
+	struct dentry *dir = kobj->dentry;
+	struct dentry *victim;
+	struct sysfs_dirent *sd;
+	umode_t mode = (attr->mode & S_IALLUGO) | S_IFREG;
+	int res = -ENOENT;
+
+	down(&dir->d_inode->i_sem);
+	victim = sysfs_get_dentry(dir, attr->name);
+	if (!IS_ERR(victim)) {
+		if (victim->d_inode &&
+		    (victim->d_parent->d_inode == dir->d_inode)) {
+			sd = victim->d_fsdata;
+			sd->s_mode = mode;
+			victim->d_inode->i_mode = mode;
+			dput(victim);
+			res = 0;
+		}
+	}
+	up(&dir->d_inode->i_sem);
+
+	return res;
+}
+EXPORT_SYMBOL_GPL(sysfs_chmod_file);
+
+
+/**
  *	sysfs_remove_file - remove an object attribute.
  *	@kobj:	object we're acting for.
  *	@attr:	attribute descriptor.
===== include/linux/sysfs.h 1.39 vs edited =====
--- 1.39/include/linux/sysfs.h	2004-12-21 18:31:01 +01:00
+++ edited/include/linux/sysfs.h	2005-03-30 04:12:38 +02:00
@@ -99,6 +99,9 @@ sysfs_create_file(struct kobject *, cons
 extern int
 sysfs_update_file(struct kobject *, const struct attribute *);
 
+extern int
+sysfs_chmod_file(struct kobject *kobj, const struct attribute *attr);
+
 extern void
 sysfs_remove_file(struct kobject *, const struct attribute *);
 
@@ -137,6 +140,10 @@ static inline int sysfs_create_file(stru
 }
 
 static inline int sysfs_update_file(struct kobject * k, const struct attribute * a)
+{
+	return 0;
+}
+static inline int sysfs_chmod_file(struct kobject *kobj, const struct attribute *attr)
 {
 	return 0;
 }

Re: [PATCH] 2/2 Use bus dev_attrs to create scsi_device attributes

[Greg KH]

On Wed, Mar 30, 2005 at 05:15:55AM +0200, Kay Sievers wrote:
> ---
> sysfs: allow change of permissions for already created attributes
> 
> Signed-off-by: Kay Sievers <kay.sievers@vrfy.org>
> 
> ===== fs/sysfs/file.c 1.23 vs edited =====
> --- 1.23/fs/sysfs/file.c	2005-02-26 15:48:19 +01:00
> +++ edited/fs/sysfs/file.c	2005-03-30 04:16:46 +02:00
> @@ -428,6 +428,39 @@ int sysfs_update_file(struct kobject * k
>  
>  
>  /**
> + * sysfs_chmod_file - update the modified mode value on an object attribute.
> + * @kobj: object we're acting for.
> + * @mode: file permissions.
> + *
> + */
> +int sysfs_chmod_file(struct kobject *kobj, const struct attribute *attr)

Your documentation doesn't match up with your function arguments :)

Shouldn't you want an extra mode variable?  Makes it easier to override
than remembering to change the mode in the attribute before calling the
function.

thanks,

greg k-h

Re: [PATCH] 2/2 Use bus dev_attrs to create scsi_device attributes

[Kay Sievers]

On Tue, Mar 29, 2005 at 08:20:43PM -0800, Greg KH wrote:
> On Wed, Mar 30, 2005 at 05:15:55AM +0200, Kay Sievers wrote:
> >  /**
> > + * sysfs_chmod_file - update the modified mode value on an object attribute.
> > + * @kobj: object we're acting for.
> > + * @mode: file permissions.
> > + *
> > + */
> > +int sysfs_chmod_file(struct kobject *kobj, const struct attribute *attr)
> 
> Your documentation doesn't match up with your function arguments :)

Bah, I tried both versions. :)

> Shouldn't you want an extra mode variable?  Makes it easier to override
> than remembering to change the mode in the attribute before calling the
> function.

If that's better, sure. Here is a new patch.

Thanks,
Kay

---
sysfs: allow changing the permissions for already created attributes

Signed-off-by: Kay Sievers <kay.sievers@vrfy.org>

===== fs/sysfs/file.c 1.23 vs edited =====
--- 1.23/fs/sysfs/file.c	2005-02-26 15:48:19 +01:00
+++ edited/fs/sysfs/file.c	2005-03-30 20:01:16 +02:00
@@ -428,6 +428,41 @@ int sysfs_update_file(struct kobject * k
 
 
 /**
+ * sysfs_chmod_file - update the modified mode value on an object attribute.
+ * @kobj: object we're acting for.
+ * @attr: attribute descriptor.
+ * @mode: file permissions.
+ *
+ */
+int sysfs_chmod_file(struct kobject *kobj, struct attribute *attr, mode_t mode)
+{
+	struct dentry *dir = kobj->dentry;
+	struct dentry *victim;
+	struct sysfs_dirent *sd;
+	umode_t umode = (mode & S_IALLUGO) | S_IFREG;
+	int res = -ENOENT;
+
+	down(&dir->d_inode->i_sem);
+	victim = sysfs_get_dentry(dir, attr->name);
+	if (!IS_ERR(victim)) {
+		if (victim->d_inode &&
+		    (victim->d_parent->d_inode == dir->d_inode)) {
+			sd = victim->d_fsdata;
+			attr->mode = mode;
+			sd->s_mode = umode;
+			victim->d_inode->i_mode = umode;
+			dput(victim);
+			res = 0;
+		}
+	}
+	up(&dir->d_inode->i_sem);
+
+	return res;
+}
+EXPORT_SYMBOL_GPL(sysfs_chmod_file);
+
+
+/**
  *	sysfs_remove_file - remove an object attribute.
  *	@kobj:	object we're acting for.
  *	@attr:	attribute descriptor.
===== include/linux/sysfs.h 1.39 vs edited =====
--- 1.39/include/linux/sysfs.h	2004-12-21 18:31:01 +01:00
+++ edited/include/linux/sysfs.h	2005-03-30 15:06:23 +02:00
@@ -99,6 +99,9 @@ sysfs_create_file(struct kobject *, cons
 extern int
 sysfs_update_file(struct kobject *, const struct attribute *);
 
+extern int
+sysfs_chmod_file(struct kobject *kobj, struct attribute *attr, mode_t mode);
+
 extern void
 sysfs_remove_file(struct kobject *, const struct attribute *);
 
@@ -137,6 +140,10 @@ static inline int sysfs_create_file(stru
 }
 
 static inline int sysfs_update_file(struct kobject * k, const struct attribute * a)
+{
+	return 0;
+}
+static inline int sysfs_chmod_file(struct kobject *kobj, struct attribute *attr, mode_t mode)
 {
 	return 0;
 }

Re: [PATCH] 2/2 Use bus dev_attrs to create scsi_device attributes

[Greg KH]

On Wed, Mar 30, 2005 at 08:07:02PM +0200, Kay Sievers wrote:
> On Tue, Mar 29, 2005 at 08:20:43PM -0800, Greg KH wrote:
> > On Wed, Mar 30, 2005 at 05:15:55AM +0200, Kay Sievers wrote:
> > >  /**
> > > + * sysfs_chmod_file - update the modified mode value on an object attribute.
> > > + * @kobj: object we're acting for.
> > > + * @mode: file permissions.
> > > + *
> > > + */
> > > +int sysfs_chmod_file(struct kobject *kobj, const struct attribute *attr)
> > 
> > Your documentation doesn't match up with your function arguments :)
> 
> Bah, I tried both versions. :)
> 
> > Shouldn't you want an extra mode variable?  Makes it easier to override
> > than remembering to change the mode in the attribute before calling the
> > function.
> 
> If that's better, sure. Here is a new patch.

Looks good, applied to my tree.

thanks,

greg k-h

Re: [PATCH] 2/2 Use bus dev_attrs to create scsi_device attributes

[Kay Sievers]

On Thu, 2005-03-17 at 09:53 -0500, James Bottomley wrote:
> On Wed, 2005-03-16 at 14:45 -0800, Patrick Mansfield wrote:
> > Any comments on this? Should I resend these patches?
> 
> Well, the basic comment is that there are a lot of features that SCSI
> has that the driver core lacks:
> 
> 1) Attribute overrides.  This is actually part of the published API for SCSI

What does this exactly mean? Handle the same attribute dynamically with
a different source of data?

> 2) Ability to add extra attributes---several drivers use this

What is missing in the driver core here? Why can't you add/remove
attribute files at any time? 

Thanks,
Kay

Re: [PATCH] 2/2 Use bus dev_attrs to create scsi_device attributes

[Patrick Mansfield]

On Wed, Mar 30, 2005 at 08:32:44PM +0200, Kay Sievers wrote:
> On Thu, 2005-03-17 at 09:53 -0500, James Bottomley wrote:
> > On Wed, 2005-03-16 at 14:45 -0800, Patrick Mansfield wrote:
> > > Any comments on this? Should I resend these patches?
> > 
> > Well, the basic comment is that there are a lot of features that SCSI
> > has that the driver core lacks:
> > 
> > 1) Attribute overrides.  This is actually part of the published API for SCSI
> 
> What does this exactly mean? Handle the same attribute dynamically with
> a different source of data?

Well a different function handled by the host driver, rather than the
function in scsi core, for example, the twa_queue_depth_attr in
drivers/scsi/3w-9xxx.c overrides the simple read attr function in
scsi_syfs.c created via this line:

	sdev_rd_attr (queue_depth, "%d\n").  

The patch left this in but only for queue_depth, not for all of the
scsi_devie attributes. We have sht->change_queue_depth that can already
handle this, some drivers use it, but some still use the sdev_attrs to
override the queue_depth. IMO, we should get rid of the override, James
doesn't agree.

James - are you OK with allowing only specific overrides of attributes?

> > 2) Ability to add extra attributes---several drivers use this
> 
> What is missing in the driver core here? Why can't you add/remove
> attribute files at any time? 

The patch left this alone, but should be changed to make sure duplicate
(that we can't override) attributes get errors. It is just a nice way for
host adapters to add attributes to a scsi_device.

But, for any of these non bus_attr and class attributes, the hotplug
notification (for scsi_device or the upper level block/char device) will
still come before the attributes are created. This is also true for the
transport attributes. This is a potential problem, I know of no udev
program or hotplug helpers that use any of these attributes.

I see no easy way around this problem without a multi-step registration,
like a device_init(), device_ready(). So a device_add would just be:

	device_init()
	device_ready()

Is your kobject_hotplug patch still pending?

We also need the following, else dev_attrs will show up after the probe
and hotplug event for a matching driver (for example, an sd block hotplug
event is generated via the device_attach() call). This could be causing
problems today, AFAIR you already hack around this problem in udev.

--- bleed-2.5/drivers/base/orig-bus.c	Tue Mar 15 17:08:18 2005
+++ bleed-2.5/drivers/base/bus.c	Wed Mar 30 10:48:34 2005
@@ -458,14 +458,14 @@ int bus_add_device(struct device * dev)
 	int error = 0;
 
 	if (bus) {
+		device_add_attrs(bus, dev);
+		sysfs_create_link(&bus->devices.kobj, &dev->kobj, dev->bus_id);
+		sysfs_create_link(&dev->kobj, &dev->bus->subsys.kset.kobj, "bus");
 		down_write(&dev->bus->subsys.rwsem);
 		pr_debug("bus %s: add device %s\n", bus->name, dev->bus_id);
 		list_add_tail(&dev->bus_list, &dev->bus->devices.list);
 		device_attach(dev);
 		up_write(&dev->bus->subsys.rwsem);
-		device_add_attrs(bus, dev);
-		sysfs_create_link(&bus->devices.kobj, &dev->kobj, dev->bus_id);
-		sysfs_create_link(&dev->kobj, &dev->bus->subsys.kset.kobj, "bus");
 	}
 	return error;
 }

-- Patrick Mansfield

Re: [PATCH] 2/2 Use bus dev_attrs to create scsi_device attributes

[Kay Sievers]

On Wed, 2005-03-30 at 13:44 -0800, Patrick Mansfield wrote:
> On Wed, Mar 30, 2005 at 08:32:44PM +0200, Kay Sievers wrote:
> > On Thu, 2005-03-17 at 09:53 -0500, James Bottomley wrote:
> > > On Wed, 2005-03-16 at 14:45 -0800, Patrick Mansfield wrote:
> > > > Any comments on this? Should I resend these patches?
> > > 
> > > Well, the basic comment is that there are a lot of features that SCSI
> > > has that the driver core lacks:
> > > 
> > > 1) Attribute overrides.  This is actually part of the published API for SCSI
> > 
> > What does this exactly mean? Handle the same attribute dynamically with
> > a different source of data?
> 
> Well a different function handled by the host driver, rather than the
> function in scsi core, for example, the twa_queue_depth_attr in
> drivers/scsi/3w-9xxx.c overrides the simple read attr function in
> scsi_syfs.c created via this line:
> 
> 	sdev_rd_attr (queue_depth, "%d\n").  
> 
> The patch left this in but only for queue_depth, not for all of the
> scsi_devie attributes. We have sht->change_queue_depth that can already
> handle this, some drivers use it, but some still use the sdev_attrs to
> override the queue_depth. IMO, we should get rid of the override, James
> doesn't agree.
> 
> James - are you OK with allowing only specific overrides of attributes?

Ok, I see. But how could that be solved in the core. By allowing to
override the sysfs-operations of already created attributes instead of
letting scsi dispatch these calls itself?

> > > 2) Ability to add extra attributes---several drivers use this
> > 
> > What is missing in the driver core here? Why can't you add/remove
> > attribute files at any time? 
> 
> The patch left this alone, but should be changed to make sure duplicate
> (that we can't override) attributes get errors. It is just a nice way for
> host adapters to add attributes to a scsi_device.
> 
> But, for any of these non bus_attr and class attributes, the hotplug
> notification (for scsi_device or the upper level block/char device) will
> still come before the attributes are created. This is also true for the
> transport attributes. This is a potential problem, I know of no udev
> program or hotplug helpers that use any of these attributes.

I was thinking about adding a special key to udev that will wait for a
attribute to show up. Just like the wait_for_sysfs logic we do today,
but only triggered by a explicit rule-key. In that case a user of that
attribute would need to add a rule to wait for a specific attribute.

> I see no easy way around this problem without a multi-step registration,
> like a device_init(), device_ready(). So a device_add would just be:
> 
> 	device_init()
> 	device_ready()
> 
> Is your kobject_hotplug patch still pending?

It's im -mm now.

> We also need the following, else dev_attrs will show up after the probe
> and hotplug event for a matching driver (for example, an sd block hotplug
> event is generated via the device_attach() call). This could be causing
> problems today, AFAIR you already hack around this problem in udev.

If that is possible, it would be nice, so we can get rid of the
hard-coded lists in udev, that are doing stat() loops at event time
until the attribute arrives.

Thanks,
Kay