[PATCHv7 0/3][Resend] Display EVPD pages in sysfs

[PATCHv7 0/3][Resend] Display EVPD pages in sysfs

[Hannes Reinecke]

Hi all,

After discussion with jejb I've dropped the EVPD parsing.
So with this version we're just displaying the EVPD page
0x80 and 0x83 as hexdumps; no parsing is attempted.
This drastically simplifies the patch, and we don't
have to worry about any parsing errors in kernel space.
Of course we'll need a parser in userspace, but that
doesn't need to do any I/O. So it's still a very nice
gain.

(I had to redo the patchset as it no longer applied to
 -rc2. Hence the resend. Sorry for that.)

Hannes Reinecke (3):
  scsi_sysfs: Implement 'is_visible' callback
  Add EVPD page 0x83 to sysfs
  Add EVPD page 0x80 to sysfs

 drivers/scsi/scsi.c        | 104 ++++++++++++++++++--
 drivers/scsi/scsi_scan.c   |   3 +
 drivers/scsi/scsi_sysfs.c  | 229 +++++++++++++++++++++++++++------------------
 include/scsi/scsi_device.h |   5 +
 4 files changed, 242 insertions(+), 99 deletions(-)

-- 
1.7.12.4

[PATCH 1/3] scsi_sysfs: Implement 'is_visible' callback

[Hannes Reinecke]

Instead of modifying attributes after the device has been created
we should be using the 'is_visible' callback to avoid races.

Signed-off-by: Hannes Reinecke <hare@suse.de>
Reviewed-by: Christoph Hellwig <hch@infradead.org>
---
 drivers/scsi/scsi_sysfs.c | 184 +++++++++++++++++++++++-----------------------
 1 file changed, 93 insertions(+), 91 deletions(-)

diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 9117d0b..196e59a 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -579,7 +579,6 @@ static int scsi_sdev_check_buf_bit(const char *buf)
  * 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 (device_busy, "%d\n");
 sdev_rd_attr (type, "%d\n");
 sdev_rd_attr (scsi_level, "%d\n");
@@ -723,7 +722,37 @@ show_queue_type_field(struct device *dev, struct device_attribute *attr,
 	return snprintf(buf, 20, "%s\n", name);
 }
 
-static DEVICE_ATTR(queue_type, S_IRUGO, show_queue_type_field, NULL);
+static ssize_t
+store_queue_type_field(struct device *dev, struct device_attribute *attr,
+		       const char *buf, size_t count)
+{
+	struct scsi_device *sdev = to_scsi_device(dev);
+	struct scsi_host_template *sht = sdev->host->hostt;
+	int tag_type = 0, retval;
+	int prev_tag_type = scsi_get_tag_type(sdev);
+
+	if (!sdev->tagged_supported || !sht->change_queue_type)
+		return -EINVAL;
+
+	if (strncmp(buf, "ordered", 7) == 0)
+		tag_type = MSG_ORDERED_TAG;
+	else if (strncmp(buf, "simple", 6) == 0)
+		tag_type = MSG_SIMPLE_TAG;
+	else if (strncmp(buf, "none", 4) != 0)
+		return -EINVAL;
+
+	if (tag_type == prev_tag_type)
+		return count;
+
+	retval = sht->change_queue_type(sdev, tag_type);
+	if (retval < 0)
+		return retval;
+
+	return count;
+}
+
+static DEVICE_ATTR(queue_type, S_IRUGO | S_IWUSR, show_queue_type_field,
+		   store_queue_type_field);
 
 static ssize_t
 show_iostat_counterbits(struct device *dev, struct device_attribute *attr, 				char *buf)
@@ -797,46 +826,9 @@ DECLARE_EVT(soft_threshold_reached, SOFT_THRESHOLD_REACHED_REPORTED)
 DECLARE_EVT(mode_parameter_change_reported, MODE_PARAMETER_CHANGE_REPORTED)
 DECLARE_EVT(lun_change_reported, LUN_CHANGE_REPORTED)
 
-/* Default template for device attributes.  May NOT be modified */
-static struct attribute *scsi_sdev_attrs[] = {
-	&dev_attr_device_blocked.attr,
-	&dev_attr_type.attr,
-	&dev_attr_scsi_level.attr,
-	&dev_attr_device_busy.attr,
-	&dev_attr_vendor.attr,
-	&dev_attr_model.attr,
-	&dev_attr_rev.attr,
-	&dev_attr_rescan.attr,
-	&dev_attr_delete.attr,
-	&dev_attr_state.attr,
-	&dev_attr_timeout.attr,
-	&dev_attr_eh_timeout.attr,
-	&dev_attr_iocounterbits.attr,
-	&dev_attr_iorequest_cnt.attr,
-	&dev_attr_iodone_cnt.attr,
-	&dev_attr_ioerr_cnt.attr,
-	&dev_attr_modalias.attr,
-	REF_EVT(media_change),
-	REF_EVT(inquiry_change_reported),
-	REF_EVT(capacity_change_reported),
-	REF_EVT(soft_threshold_reached),
-	REF_EVT(mode_parameter_change_reported),
-	REF_EVT(lun_change_reported),
-	NULL
-};
-
-static struct attribute_group scsi_sdev_attr_group = {
-	.attrs =	scsi_sdev_attrs,
-};
-
-static const struct attribute_group *scsi_sdev_attr_groups[] = {
-	&scsi_sdev_attr_group,
-	NULL
-};
-
 static ssize_t
-sdev_store_queue_depth_rw(struct device *dev, struct device_attribute *attr,
-			  const char *buf, size_t count)
+sdev_store_queue_depth(struct device *dev, struct device_attribute *attr,
+		       const char *buf, size_t count)
 {
 	int depth, retval;
 	struct scsi_device *sdev = to_scsi_device(dev);
@@ -859,10 +851,10 @@ sdev_store_queue_depth_rw(struct device *dev, struct device_attribute *attr,
 
 	return count;
 }
+sdev_show_function(queue_depth, "%d\n");
 
-static struct device_attribute sdev_attr_queue_depth_rw =
-	__ATTR(queue_depth, S_IRUGO | S_IWUSR, sdev_show_queue_depth,
-	       sdev_store_queue_depth_rw);
+static DEVICE_ATTR(queue_depth, S_IRUGO | S_IWUSR, sdev_show_queue_depth,
+		   sdev_store_queue_depth);
 
 static ssize_t
 sdev_show_queue_ramp_up_period(struct device *dev,
@@ -890,40 +882,73 @@ sdev_store_queue_ramp_up_period(struct device *dev,
 	return period;
 }
 
-static struct device_attribute sdev_attr_queue_ramp_up_period =
-	__ATTR(queue_ramp_up_period, S_IRUGO | S_IWUSR,
-	       sdev_show_queue_ramp_up_period,
-	       sdev_store_queue_ramp_up_period);
+static DEVICE_ATTR(queue_ramp_up_period, S_IRUGO | S_IWUSR,
+		   sdev_show_queue_ramp_up_period,
+		   sdev_store_queue_ramp_up_period);
 
-static ssize_t
-sdev_store_queue_type_rw(struct device *dev, struct device_attribute *attr,
-			 const char *buf, size_t count)
+static umode_t scsi_sdev_attr_is_visible(struct kobject *kobj,
+					 struct attribute *attr, int i)
 {
+	struct device *dev = container_of(kobj, struct device, kobj);
 	struct scsi_device *sdev = to_scsi_device(dev);
-	struct scsi_host_template *sht = sdev->host->hostt;
-	int tag_type = 0, retval;
-	int prev_tag_type = scsi_get_tag_type(sdev);
 
-	if (!sdev->tagged_supported || !sht->change_queue_type)
-		return -EINVAL;
 
-	if (strncmp(buf, "ordered", 7) == 0)
-		tag_type = MSG_ORDERED_TAG;
-	else if (strncmp(buf, "simple", 6) == 0)
-		tag_type = MSG_SIMPLE_TAG;
-	else if (strncmp(buf, "none", 4) != 0)
-		return -EINVAL;
+	if (attr == &dev_attr_queue_depth.attr &&
+	    !sdev->host->hostt->change_queue_depth)
+		return S_IRUGO;
 
-	if (tag_type == prev_tag_type)
-		return count;
+	if (attr == &dev_attr_queue_ramp_up_period.attr &&
+	    !sdev->host->hostt->change_queue_depth)
+		return 0;
 
-	retval = sht->change_queue_type(sdev, tag_type);
-	if (retval < 0)
-		return retval;
+	if (attr == &dev_attr_queue_type.attr &&
+	    !sdev->host->hostt->change_queue_type)
+		return S_IRUGO;
 
-	return count;
+	return attr->mode;
 }
 
+/* Default template for device attributes.  May NOT be modified */
+static struct attribute *scsi_sdev_attrs[] = {
+	&dev_attr_device_blocked.attr,
+	&dev_attr_type.attr,
+	&dev_attr_scsi_level.attr,
+	&dev_attr_device_busy.attr,
+	&dev_attr_vendor.attr,
+	&dev_attr_model.attr,
+	&dev_attr_rev.attr,
+	&dev_attr_rescan.attr,
+	&dev_attr_delete.attr,
+	&dev_attr_state.attr,
+	&dev_attr_timeout.attr,
+	&dev_attr_eh_timeout.attr,
+	&dev_attr_iocounterbits.attr,
+	&dev_attr_iorequest_cnt.attr,
+	&dev_attr_iodone_cnt.attr,
+	&dev_attr_ioerr_cnt.attr,
+	&dev_attr_modalias.attr,
+	&dev_attr_queue_depth.attr,
+	&dev_attr_queue_type.attr,
+	&dev_attr_queue_ramp_up_period.attr,
+	REF_EVT(media_change),
+	REF_EVT(inquiry_change_reported),
+	REF_EVT(capacity_change_reported),
+	REF_EVT(soft_threshold_reached),
+	REF_EVT(mode_parameter_change_reported),
+	REF_EVT(lun_change_reported),
+	NULL
+};
+
+static struct attribute_group scsi_sdev_attr_group = {
+	.attrs =	scsi_sdev_attrs,
+	.is_visible =	scsi_sdev_attr_is_visible,
+};
+
+static const struct attribute_group *scsi_sdev_attr_groups[] = {
+	&scsi_sdev_attr_group,
+	NULL
+};
+
 static int scsi_target_add(struct scsi_target *starget)
 {
 	int error;
@@ -946,10 +971,6 @@ static int scsi_target_add(struct scsi_target *starget)
 	return 0;
 }
 
-static struct device_attribute sdev_attr_queue_type_rw =
-	__ATTR(queue_type, S_IRUGO | S_IWUSR, show_queue_type_field,
-	       sdev_store_queue_type_rw);
-
 /**
  * scsi_sysfs_add_sdev - add scsi device to sysfs
  * @sdev:	scsi_device to add
@@ -1003,25 +1024,6 @@ int scsi_sysfs_add_sdev(struct scsi_device *sdev)
 	transport_add_device(&sdev->sdev_gendev);
 	sdev->is_visible = 1;
 
-	/* create queue files, which may be writable, depending on the host */
-	if (sdev->host->hostt->change_queue_depth) {
-		error = device_create_file(&sdev->sdev_gendev,
-					   &sdev_attr_queue_depth_rw);
-		error = device_create_file(&sdev->sdev_gendev,
-					   &sdev_attr_queue_ramp_up_period);
-	}
-	else
-		error = device_create_file(&sdev->sdev_gendev, &dev_attr_queue_depth);
-	if (error)
-		return error;
-
-	if (sdev->host->hostt->change_queue_type)
-		error = device_create_file(&sdev->sdev_gendev, &sdev_attr_queue_type_rw);
-	else
-		error = device_create_file(&sdev->sdev_gendev, &dev_attr_queue_type);
-	if (error)
-		return error;
-
 	error = bsg_register_queue(rq, &sdev->sdev_gendev, NULL, NULL);
 
 	if (error)
-- 
1.7.12.4

[PATCH 2/3] Add EVPD page 0x83 to sysfs

[Hannes Reinecke]

EVPD page 0x83 is used to uniquely identify the device.
So instead of having each and every program issue a separate
SG_IO call to retrieve this information it does make far more
sense to display it in sysfs.

Cc: Jeremy Linton <jlinton@tributary.com>
Cc: Kay Sievers <kay@vrfy.org>
Cc: Doug Gilbert <dgilbert@interlog.com>
Cc: Kai Makisara <kai.makisara@kolumbus.fi>
Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/scsi.c        | 79 ++++++++++++++++++++++++++++++++++++++++++----
 drivers/scsi/scsi_scan.c   |  3 ++
 drivers/scsi/scsi_sysfs.c  | 39 ++++++++++++++++++++++-
 include/scsi/scsi_device.h |  3 ++
 4 files changed, 116 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index d8afec8..57105e0 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -954,7 +954,7 @@ EXPORT_SYMBOL(scsi_track_queue_full);
  * This is an internal helper function.  You probably want to use
  * scsi_get_vpd_page instead.
  *
- * Returns 0 on success or a negative error number.
+ * Returns size of the vpg page on success or a negative error number.
  */
 static int scsi_vpd_inquiry(struct scsi_device *sdev, unsigned char *buffer,
 							u8 page, unsigned len)
@@ -962,6 +962,9 @@ static int scsi_vpd_inquiry(struct scsi_device *sdev, unsigned char *buffer,
 	int result;
 	unsigned char cmd[16];
 
+	if (len < 4)
+		return -EINVAL;
+
 	cmd[0] = INQUIRY;
 	cmd[1] = 1;		/* EVPD */
 	cmd[2] = page;
@@ -982,7 +985,7 @@ static int scsi_vpd_inquiry(struct scsi_device *sdev, unsigned char *buffer,
 	if (buffer[1] != page)
 		return -EIO;
 
-	return 0;
+	return (buffer[2] << 8) + buffer[3] + 4;
 }
 
 /**
@@ -1009,18 +1012,18 @@ int scsi_get_vpd_page(struct scsi_device *sdev, u8 page, unsigned char *buf,
 
 	/* Ask for all the pages supported by this device */
 	result = scsi_vpd_inquiry(sdev, buf, 0, buf_len);
-	if (result)
+	if (result < 4)
 		goto fail;
 
 	/* If the user actually wanted this page, we can skip the rest */
 	if (page == 0)
 		return 0;
 
-	for (i = 0; i < min((int)buf[3], buf_len - 4); i++)
-		if (buf[i + 4] == page)
+	for (i = 4; i < min(result, buf_len); i++)
+		if (buf[i] == page)
 			goto found;
 
-	if (i < buf[3] && i >= buf_len - 4)
+	if (i < result && i >= buf_len)
 		/* ran off the end of the buffer, give us benefit of doubt */
 		goto found;
 	/* The device claims it doesn't support the requested page */
@@ -1028,7 +1031,7 @@ int scsi_get_vpd_page(struct scsi_device *sdev, u8 page, unsigned char *buf,
 
  found:
 	result = scsi_vpd_inquiry(sdev, buf, page, buf_len);
-	if (result)
+	if (result < 0)
 		goto fail;
 
 	return 0;
@@ -1039,6 +1042,68 @@ int scsi_get_vpd_page(struct scsi_device *sdev, u8 page, unsigned char *buf,
 EXPORT_SYMBOL_GPL(scsi_get_vpd_page);
 
 /**
+ * scsi_attach_vpd - Attach Vital Product Data to a SCSI device structure
+ * @sdev: The device to ask
+ *
+ * Attach the 'Device Identification' VPD page (0x83) to a SCSI device
+ * structure. This information can be used to identify the device
+ * uniquely.
+ */
+void scsi_attach_vpd(struct scsi_device *sdev)
+{
+	int result, i;
+	int vpd_len = 255;
+	int pg83_supported = 0;
+	unsigned char *vpd_buf;
+
+	if (sdev->skip_vpd_pages)
+		return;
+retry_pg0:
+	vpd_buf = kmalloc(vpd_len, GFP_KERNEL);
+	if (!vpd_buf)
+		return;
+
+	/* Ask for all the pages supported by this device */
+	result = scsi_vpd_inquiry(sdev, vpd_buf, 0, vpd_len);
+	if (result < 0) {
+		kfree(vpd_buf);
+		return;
+	}
+	if (result > vpd_len) {
+		vpd_len = result;
+		kfree(vpd_buf);
+		goto retry_pg0;
+	}
+
+	for (i = 4; i < result; i++) {
+		if (vpd_buf[i] == 0x83) {
+			pg83_supported = 1;
+		}
+	}
+	kfree(vpd_buf);
+
+	if (pg83_supported) {
+retry_pg83:
+		vpd_buf = kmalloc(vpd_len, GFP_KERNEL);
+		if (!vpd_buf)
+			return;
+
+		result = scsi_vpd_inquiry(sdev, vpd_buf, 0x83, vpd_len);
+		if (result < 0) {
+			kfree(vpd_buf);
+			return;
+		}
+		if (result > vpd_len) {
+			vpd_len = result;
+			kfree(vpd_buf);
+			goto retry_pg83;
+		}
+		sdev->vpd_pg83_len = result;
+		sdev->vpd_pg83 = vpd_buf;
+	}
+}
+
+/**
  * scsi_report_opcode - Find out if a given command opcode is supported
  * @sdev:	scsi device to query
  * @buffer:	scratch buffer (must be at least 20 bytes long)
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 307a811..154bb5e 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -929,6 +929,9 @@ static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result,
 	if (*bflags & BLIST_SKIP_VPD_PAGES)
 		sdev->skip_vpd_pages = 1;
 
+	if (sdev->scsi_level >= SCSI_3)
+		scsi_attach_vpd(sdev);
+
 	transport_configure_device(&sdev->sdev_gendev);
 
 	if (sdev->host->hostt->slave_configure) {
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 196e59a..ca19448 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -415,6 +415,7 @@ static void scsi_device_dev_release_usercontext(struct work_struct *work)
 
 	scsi_target_reap(scsi_target(sdev));
 
+	kfree(sdev->vpd_pg83);
 	kfree(sdev->inquiry);
 	kfree(sdev);
 
@@ -755,7 +756,38 @@ static DEVICE_ATTR(queue_type, S_IRUGO | S_IWUSR, show_queue_type_field,
 		   store_queue_type_field);
 
 static ssize_t
-show_iostat_counterbits(struct device *dev, struct device_attribute *attr, 				char *buf)
+show_vpd_pg(const unsigned char *pg_buf, int pg_len, char *buf)
+{
+	int len = 0, i;
+
+	if (!pg_buf)
+		return -EINVAL;
+
+	len = 0;
+	for (i = 0; i < pg_len; i += 16) {
+		hex_dump_to_buffer(pg_buf + i, pg_len, 16, 1,
+				   buf + len, PAGE_SIZE, false);
+		strcat(buf + len, "\n");
+		len += strlen(buf + len);
+	}
+	return len;
+}
+
+#define sdev_vpd_pg_attr(page) \
+static ssize_t						   \
+show_vpd_##page(struct device *dev, struct device_attribute *attr, \
+		char *buf)						\
+{									\
+	struct scsi_device *sdev = to_scsi_device(dev);			\
+	return show_vpd_pg(sdev->vpd_##page, sdev->vpd_##page##_len, buf); \
+}									\
+static DEVICE_ATTR(vpd_##page, S_IRUGO, show_vpd_##page, NULL);
+
+sdev_vpd_pg_attr(pg83);
+
+static ssize_t
+show_iostat_counterbits(struct device *dev, struct device_attribute *attr,
+			char *buf)
 {
 	return snprintf(buf, 20, "%d\n", (int)sizeof(atomic_t) * 8);
 }
@@ -905,6 +937,10 @@ static umode_t scsi_sdev_attr_is_visible(struct kobject *kobj,
 	    !sdev->host->hostt->change_queue_type)
 		return S_IRUGO;
 
+	if (attr == &dev_attr_vpd_pg83.attr &&
+	    !sdev->vpd_pg83)
+		return 0;
+
 	return attr->mode;
 }
 
@@ -930,6 +966,7 @@ static struct attribute *scsi_sdev_attrs[] = {
 	&dev_attr_queue_depth.attr,
 	&dev_attr_queue_type.attr,
 	&dev_attr_queue_ramp_up_period.attr,
+	&dev_attr_vpd_pg83.attr,
 	REF_EVT(media_change),
 	REF_EVT(inquiry_change_reported),
 	REF_EVT(capacity_change_reported),
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index d65fbec..d209f04 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -113,6 +113,8 @@ struct scsi_device {
 	const char * vendor;		/* [back_compat] point into 'inquiry' ... */
 	const char * model;		/* ... after scan; point to static string */
 	const char * rev;		/* ... "nullnullnullnull" before scan */
+	unsigned char vpd_pg83_len;
+	unsigned char *vpd_pg83;
 	unsigned char current_tag;	/* current tag */
 	struct scsi_target      *sdev_target;   /* used only for single_lun */
 
@@ -309,6 +311,7 @@ extern int scsi_add_device(struct Scsi_Host *host, uint channel,
 extern int scsi_register_device_handler(struct scsi_device_handler *scsi_dh);
 extern void scsi_remove_device(struct scsi_device *);
 extern int scsi_unregister_device_handler(struct scsi_device_handler *scsi_dh);
+void scsi_attach_vpd(struct scsi_device *sdev);
 
 extern int scsi_device_get(struct scsi_device *);
 extern void scsi_device_put(struct scsi_device *);
-- 
1.7.12.4

Re: [PATCH 2/3] Add EVPD page 0x83 to sysfs

[Bart Van Assche]

On 02/13/14 11:28, Hannes Reinecke wrote:
>  static ssize_t
> -show_iostat_counterbits(struct device *dev, struct device_attribute *attr, 				char *buf)
> +show_vpd_pg(const unsigned char *pg_buf, int pg_len, char *buf)
> +{
> +	int len = 0, i;
> +
> +	if (!pg_buf)
> +		return -EINVAL;
> +
> +	len = 0;
> +	for (i = 0; i < pg_len; i += 16) {
> +		hex_dump_to_buffer(pg_buf + i, pg_len, 16, 1,
> +				   buf + len, PAGE_SIZE, false);
> +		strcat(buf + len, "\n");
> +		len += strlen(buf + len);
> +	}
> +	return len;
> +}

It might be a good idea to add the output buffer length as an argument
in show_vpd_pg() and to check explicitly whether or not there is
sufficient space left in the output buffer.

Bart.

Re: [PATCH 2/3] Add EVPD page 0x83 to sysfs

[Hannes Reinecke]

On 03/02/2014 09:34 AM, Bart Van Assche wrote:
> On 02/13/14 11:28, Hannes Reinecke wrote:
>>  static ssize_t
>> -show_iostat_counterbits(struct device *dev, struct device_attribute *attr, 				char *buf)
>> +show_vpd_pg(const unsigned char *pg_buf, int pg_len, char *buf)
>> +{
>> +	int len = 0, i;
>> +
>> +	if (!pg_buf)
>> +		return -EINVAL;
>> +
>> +	len = 0;
>> +	for (i = 0; i < pg_len; i += 16) {
>> +		hex_dump_to_buffer(pg_buf + i, pg_len, 16, 1,
>> +				   buf + len, PAGE_SIZE, false);
>> +		strcat(buf + len, "\n");
>> +		len += strlen(buf + len);
>> +	}
>> +	return len;
>> +}
> 
> It might be a good idea to add the output buffer length as an argument
> in show_vpd_pg() and to check explicitly whether or not there is
> sufficient space left in the output buffer.
> 
Good point. Will be doing so.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] Add EVPD page 0x83 to sysfs

[Bart Van Assche]

On 02/13/14 11:28, Hannes Reinecke wrote:
> -	return 0;
> +	return (buffer[2] << 8) + buffer[3] + 4;

Has it been considered to use get_unaligned_be16() instead of open
coding this function ?

Bart.

Re: [PATCH 2/3] Add EVPD page 0x83 to sysfs

[Hannes Reinecke]

On 03/02/2014 09:36 AM, Bart Van Assche wrote:
> On 02/13/14 11:28, Hannes Reinecke wrote:
>> -	return 0;
>> +	return (buffer[2] << 8) + buffer[3] + 4;
> 
> Has it been considered to use get_unaligned_be16() instead of open
> coding this function ?
> 
Ok.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/3] Add EVPD page 0x80 to sysfs

[Hannes Reinecke]

Some older devices (most notably tapes) will only report reliable
information in page 0x80 (Unit Serial Number). So export this
in the sysfs attribute 'vpd_pg80'.

Cc: Doug Gilbert <dgilbert@interlog.com>
Cc: Jeremy Linton <jlinton@tributary.com>
Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/scsi.c        | 27 ++++++++++++++++++++++++++-
 drivers/scsi/scsi_sysfs.c  |  6 ++++++
 include/scsi/scsi_device.h |  2 ++
 3 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 57105e0..defa62d 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -1045,7 +1045,8 @@ EXPORT_SYMBOL_GPL(scsi_get_vpd_page);
  * scsi_attach_vpd - Attach Vital Product Data to a SCSI device structure
  * @sdev: The device to ask
  *
- * Attach the 'Device Identification' VPD page (0x83) to a SCSI device
+ * Attach the 'Device Identification' VPD page (0x83) and the
+ * 'Unit Serial Number' VPD page (0x80) to a SCSI device
  * structure. This information can be used to identify the device
  * uniquely.
  */
@@ -1053,6 +1054,7 @@ void scsi_attach_vpd(struct scsi_device *sdev)
 {
 	int result, i;
 	int vpd_len = 255;
+	int pg80_supported = 0;
 	int pg83_supported = 0;
 	unsigned char *vpd_buf;
 
@@ -1076,12 +1078,35 @@ retry_pg0:
 	}
 
 	for (i = 4; i < result; i++) {
+		if (vpd_buf[i] == 0x80) {
+			pg80_supported = 1;
+		}
 		if (vpd_buf[i] == 0x83) {
 			pg83_supported = 1;
 		}
 	}
 	kfree(vpd_buf);
 
+	if (pg80_supported) {
+retry_pg80:
+		vpd_buf = kmalloc(vpd_len, GFP_KERNEL);
+		if (!vpd_buf)
+			return;
+
+		result = scsi_vpd_inquiry(sdev, vpd_buf, 0x80, vpd_len);
+		if (result < 0) {
+			kfree(vpd_buf);
+			return;
+		}
+		if (result > vpd_len) {
+			vpd_len = result;
+			kfree(vpd_buf);
+			goto retry_pg80;
+		}
+		sdev->vpd_pg80_len = result;
+		sdev->vpd_pg80 = vpd_buf;
+	}
+
 	if (pg83_supported) {
 retry_pg83:
 		vpd_buf = kmalloc(vpd_len, GFP_KERNEL);
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index ca19448..03f97ab 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -784,6 +784,7 @@ show_vpd_##page(struct device *dev, struct device_attribute *attr, \
 static DEVICE_ATTR(vpd_##page, S_IRUGO, show_vpd_##page, NULL);
 
 sdev_vpd_pg_attr(pg83);
+sdev_vpd_pg_attr(pg80);
 
 static ssize_t
 show_iostat_counterbits(struct device *dev, struct device_attribute *attr,
@@ -941,6 +942,10 @@ static umode_t scsi_sdev_attr_is_visible(struct kobject *kobj,
 	    !sdev->vpd_pg83)
 		return 0;
 
+	if (attr == &dev_attr_vpd_pg80.attr &&
+	    !sdev->vpd_pg80)
+		return 0;
+
 	return attr->mode;
 }
 
@@ -966,6 +971,7 @@ static struct attribute *scsi_sdev_attrs[] = {
 	&dev_attr_queue_depth.attr,
 	&dev_attr_queue_type.attr,
 	&dev_attr_queue_ramp_up_period.attr,
+	&dev_attr_vpd_pg80.attr,
 	&dev_attr_vpd_pg83.attr,
 	REF_EVT(media_change),
 	REF_EVT(inquiry_change_reported),
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index d209f04..2ea8212 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -115,6 +115,8 @@ struct scsi_device {
 	const char * rev;		/* ... "nullnullnullnull" before scan */
 	unsigned char vpd_pg83_len;
 	unsigned char *vpd_pg83;
+	unsigned char vpd_pg80_len;
+	unsigned char *vpd_pg80;
 	unsigned char current_tag;	/* current tag */
 	struct scsi_target      *sdev_target;   /* used only for single_lun */
 
-- 
1.7.12.4

Re: [PATCHv7 0/3][Resend] Display EVPD pages in sysfs

[Hannes Reinecke]

On 02/13/2014 11:27 AM, Hannes Reinecke wrote:
> Hi all,
> 
> After discussion with jejb I've dropped the EVPD parsing.
> So with this version we're just displaying the EVPD page
> 0x80 and 0x83 as hexdumps; no parsing is attempted.
> This drastically simplifies the patch, and we don't
> have to worry about any parsing errors in kernel space.
> Of course we'll need a parser in userspace, but that
> doesn't need to do any I/O. So it's still a very nice
> gain.
> 
> (I had to redo the patchset as it no longer applied to
>  -rc2. Hence the resend. Sorry for that.)
> 
> Hannes Reinecke (3):
>   scsi_sysfs: Implement 'is_visible' callback
>   Add EVPD page 0x83 to sysfs
>   Add EVPD page 0x80 to sysfs
> 
>  drivers/scsi/scsi.c        | 104 ++++++++++++++++++--
>  drivers/scsi/scsi_scan.c   |   3 +
>  drivers/scsi/scsi_sysfs.c  | 229 +++++++++++++++++++++++++++------------------
>  include/scsi/scsi_device.h |   5 +
>  4 files changed, 242 insertions(+), 99 deletions(-)
> 
James, ping?

Anything amiss with that patchset?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv7 0/3][Resend] Display EVPD pages in sysfs

[Bart Van Assche]

On 02/13/14 11:27, Hannes Reinecke wrote:
> After discussion with jejb I've dropped the EVPD parsing.
> So with this version we're just displaying the EVPD page
> 0x80 and 0x83 as hexdumps; no parsing is attempted.
> This drastically simplifies the patch, and we don't
> have to worry about any parsing errors in kernel space.
> Of course we'll need a parser in userspace, but that
> doesn't need to do any I/O. So it's still a very nice
> gain.

A general comment about this patch series: I think the cached copies of
these pages should be refreshed at least after an INQUIRY DATA HAS
CHANGED unit attention code has been received. Some SCSI target
implementations allow to change this data after a LUN has been created.

Bart.

Re: [PATCHv7 0/3][Resend] Display EVPD pages in sysfs

[Hannes Reinecke]

On 03/02/2014 09:53 AM, Bart Van Assche wrote:
> On 02/13/14 11:27, Hannes Reinecke wrote:
>> After discussion with jejb I've dropped the EVPD parsing.
>> So with this version we're just displaying the EVPD page
>> 0x80 and 0x83 as hexdumps; no parsing is attempted.
>> This drastically simplifies the patch, and we don't
>> have to worry about any parsing errors in kernel space.
>> Of course we'll need a parser in userspace, but that
>> doesn't need to do any I/O. So it's still a very nice
>> gain.
> 
> A general comment about this patch series: I think the cached copies of
> these pages should be refreshed at least after an INQUIRY DATA HAS
> CHANGED unit attention code has been received. Some SCSI target
> implementations allow to change this data after a LUN has been created.
> 
Yes, eventually. But this needs to be handled in a general context,
as (potentially) even the inquiry string itself has been invalidated
after receiving such an event.
So we should be doing a rescan of the scsi device upon receiving
such an event. But this is a general problem, not one particular to
this patchset.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv7 0/3][Resend] Display EVPD pages in sysfs

[Bart Van Assche]

On 03/05/14 09:00, Hannes Reinecke wrote:
> On 03/02/2014 09:53 AM, Bart Van Assche wrote:
>> A general comment about this patch series: I think the cached copies of
>> these pages should be refreshed at least after an INQUIRY DATA HAS
>> CHANGED unit attention code has been received. Some SCSI target
>> implementations allow to change this data after a LUN has been created.
>
> Yes, eventually. But this needs to be handled in a general context,
> as (potentially) even the inquiry string itself has been invalidated
> after receiving such an event.
> So we should be doing a rescan of the scsi device upon receiving
> such an event. But this is a general problem, not one particular to
> this patchset.

Sorry but since the ALUA patch series is based on this patch series I'm
afraid that the ALUA patch series introduces a regression that seems
unacceptable to me. SCSI target implementations like LIO allow to remove
and re-add a LUN after initial discovery of a SCSI host. My concern here
is that the caching introduced by this patch series and which is used in
the ALUA patch series will cause INQUIRY data not to be updated after it
has been changed at the target side. Today the scsi_dh_alua handler
processes such INQUIRY data changes fine. Does this make sense to you ?

Thanks,

Bart.

Re: [PATCHv7 0/3][Resend] Display EVPD pages in sysfs

[Hannes Reinecke]

On 03/05/2014 09:23 AM, Bart Van Assche wrote:
> On 03/05/14 09:00, Hannes Reinecke wrote:
>> On 03/02/2014 09:53 AM, Bart Van Assche wrote:
>>> A general comment about this patch series: I think the cached copies of
>>> these pages should be refreshed at least after an INQUIRY DATA HAS
>>> CHANGED unit attention code has been received. Some SCSI target
>>> implementations allow to change this data after a LUN has been created.
>>
>> Yes, eventually. But this needs to be handled in a general context,
>> as (potentially) even the inquiry string itself has been invalidated
>> after receiving such an event.
>> So we should be doing a rescan of the scsi device upon receiving
>> such an event. But this is a general problem, not one particular to
>> this patchset.
> 
> Sorry but since the ALUA patch series is based on this patch series I'm
> afraid that the ALUA patch series introduces a regression that seems
> unacceptable to me. SCSI target implementations like LIO allow to remove
> and re-add a LUN after initial discovery of a SCSI host. My concern here
> is that the caching introduced by this patch series and which is used in
> the ALUA patch series will cause INQUIRY data not to be updated after it
> has been changed at the target side. Today the scsi_dh_alua handler
> processes such INQUIRY data changes fine. Does this make sense to you ?
> 
Currently, the SCSI stack itself is not well-equipped to handle
these kind of setups anyway.
(The only known working solution is to remove the device _entirely_
and have the HBA rescan the LUN).
So while you are right I'm still preferring to keep those two issues
separate.

James, any preferences here?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCHv7 0/3] Display EVPD pages in sysfs

[Hannes Reinecke]

Hi all,

After discussion with jejb I've dropped the EVPD parsing.
So with this version we're just displaying the EVPD page
0x80 and 0x83 as hexdumps; no parsing is attempted.
This drastically simplifies the patch, and we don't
have to worry about any parsing errors in kernel space.

Hannes Reinecke (3):
  scsi_sysfs: Implement 'is_visible' callback
  Add EVPD page 0x83 to sysfs
  Add EVPD page 0x80 to sysfs

 drivers/scsi/scsi.c        | 104 ++++++++++++++++++--
 drivers/scsi/scsi_scan.c   |   3 +
 drivers/scsi/scsi_sysfs.c  | 229 +++++++++++++++++++++++++++------------------
 include/scsi/scsi.h        |   4 +-
 include/scsi/scsi_device.h |   5 +
 5 files changed, 245 insertions(+), 100 deletions(-)

-- 
1.7.12.4

[PATCH 1/3] scsi_sysfs: Implement 'is_visible' callback

[Hannes Reinecke]

Instead of modifying attributes after the device has been created
we should be using the 'is_visible' callback to avoid races.

Signed-off-by: Hannes Reinecke <hare@suse.de>
Reviewed-by: Christoph Hellwig <hch@infradead.org>
---
 drivers/scsi/scsi_sysfs.c | 184 +++++++++++++++++++++++-----------------------
 1 file changed, 93 insertions(+), 91 deletions(-)

diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 9117d0b..196e59a 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -579,7 +579,6 @@ static int scsi_sdev_check_buf_bit(const char *buf)
  * 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 (device_busy, "%d\n");
 sdev_rd_attr (type, "%d\n");
 sdev_rd_attr (scsi_level, "%d\n");
@@ -723,7 +722,37 @@ show_queue_type_field(struct device *dev, struct device_attribute *attr,
 	return snprintf(buf, 20, "%s\n", name);
 }
 
-static DEVICE_ATTR(queue_type, S_IRUGO, show_queue_type_field, NULL);
+static ssize_t
+store_queue_type_field(struct device *dev, struct device_attribute *attr,
+		       const char *buf, size_t count)
+{
+	struct scsi_device *sdev = to_scsi_device(dev);
+	struct scsi_host_template *sht = sdev->host->hostt;
+	int tag_type = 0, retval;
+	int prev_tag_type = scsi_get_tag_type(sdev);
+
+	if (!sdev->tagged_supported || !sht->change_queue_type)
+		return -EINVAL;
+
+	if (strncmp(buf, "ordered", 7) == 0)
+		tag_type = MSG_ORDERED_TAG;
+	else if (strncmp(buf, "simple", 6) == 0)
+		tag_type = MSG_SIMPLE_TAG;
+	else if (strncmp(buf, "none", 4) != 0)
+		return -EINVAL;
+
+	if (tag_type == prev_tag_type)
+		return count;
+
+	retval = sht->change_queue_type(sdev, tag_type);
+	if (retval < 0)
+		return retval;
+
+	return count;
+}
+
+static DEVICE_ATTR(queue_type, S_IRUGO | S_IWUSR, show_queue_type_field,
+		   store_queue_type_field);
 
 static ssize_t
 show_iostat_counterbits(struct device *dev, struct device_attribute *attr, 				char *buf)
@@ -797,46 +826,9 @@ DECLARE_EVT(soft_threshold_reached, SOFT_THRESHOLD_REACHED_REPORTED)
 DECLARE_EVT(mode_parameter_change_reported, MODE_PARAMETER_CHANGE_REPORTED)
 DECLARE_EVT(lun_change_reported, LUN_CHANGE_REPORTED)
 
-/* Default template for device attributes.  May NOT be modified */
-static struct attribute *scsi_sdev_attrs[] = {
-	&dev_attr_device_blocked.attr,
-	&dev_attr_type.attr,
-	&dev_attr_scsi_level.attr,
-	&dev_attr_device_busy.attr,
-	&dev_attr_vendor.attr,
-	&dev_attr_model.attr,
-	&dev_attr_rev.attr,
-	&dev_attr_rescan.attr,
-	&dev_attr_delete.attr,
-	&dev_attr_state.attr,
-	&dev_attr_timeout.attr,
-	&dev_attr_eh_timeout.attr,
-	&dev_attr_iocounterbits.attr,
-	&dev_attr_iorequest_cnt.attr,
-	&dev_attr_iodone_cnt.attr,
-	&dev_attr_ioerr_cnt.attr,
-	&dev_attr_modalias.attr,
-	REF_EVT(media_change),
-	REF_EVT(inquiry_change_reported),
-	REF_EVT(capacity_change_reported),
-	REF_EVT(soft_threshold_reached),
-	REF_EVT(mode_parameter_change_reported),
-	REF_EVT(lun_change_reported),
-	NULL
-};
-
-static struct attribute_group scsi_sdev_attr_group = {
-	.attrs =	scsi_sdev_attrs,
-};
-
-static const struct attribute_group *scsi_sdev_attr_groups[] = {
-	&scsi_sdev_attr_group,
-	NULL
-};
-
 static ssize_t
-sdev_store_queue_depth_rw(struct device *dev, struct device_attribute *attr,
-			  const char *buf, size_t count)
+sdev_store_queue_depth(struct device *dev, struct device_attribute *attr,
+		       const char *buf, size_t count)
 {
 	int depth, retval;
 	struct scsi_device *sdev = to_scsi_device(dev);
@@ -859,10 +851,10 @@ sdev_store_queue_depth_rw(struct device *dev, struct device_attribute *attr,
 
 	return count;
 }
+sdev_show_function(queue_depth, "%d\n");
 
-static struct device_attribute sdev_attr_queue_depth_rw =
-	__ATTR(queue_depth, S_IRUGO | S_IWUSR, sdev_show_queue_depth,
-	       sdev_store_queue_depth_rw);
+static DEVICE_ATTR(queue_depth, S_IRUGO | S_IWUSR, sdev_show_queue_depth,
+		   sdev_store_queue_depth);
 
 static ssize_t
 sdev_show_queue_ramp_up_period(struct device *dev,
@@ -890,40 +882,73 @@ sdev_store_queue_ramp_up_period(struct device *dev,
 	return period;
 }
 
-static struct device_attribute sdev_attr_queue_ramp_up_period =
-	__ATTR(queue_ramp_up_period, S_IRUGO | S_IWUSR,
-	       sdev_show_queue_ramp_up_period,
-	       sdev_store_queue_ramp_up_period);
+static DEVICE_ATTR(queue_ramp_up_period, S_IRUGO | S_IWUSR,
+		   sdev_show_queue_ramp_up_period,
+		   sdev_store_queue_ramp_up_period);
 
-static ssize_t
-sdev_store_queue_type_rw(struct device *dev, struct device_attribute *attr,
-			 const char *buf, size_t count)
+static umode_t scsi_sdev_attr_is_visible(struct kobject *kobj,
+					 struct attribute *attr, int i)
 {
+	struct device *dev = container_of(kobj, struct device, kobj);
 	struct scsi_device *sdev = to_scsi_device(dev);
-	struct scsi_host_template *sht = sdev->host->hostt;
-	int tag_type = 0, retval;
-	int prev_tag_type = scsi_get_tag_type(sdev);
 
-	if (!sdev->tagged_supported || !sht->change_queue_type)
-		return -EINVAL;
 
-	if (strncmp(buf, "ordered", 7) == 0)
-		tag_type = MSG_ORDERED_TAG;
-	else if (strncmp(buf, "simple", 6) == 0)
-		tag_type = MSG_SIMPLE_TAG;
-	else if (strncmp(buf, "none", 4) != 0)
-		return -EINVAL;
+	if (attr == &dev_attr_queue_depth.attr &&
+	    !sdev->host->hostt->change_queue_depth)
+		return S_IRUGO;
 
-	if (tag_type == prev_tag_type)
-		return count;
+	if (attr == &dev_attr_queue_ramp_up_period.attr &&
+	    !sdev->host->hostt->change_queue_depth)
+		return 0;
 
-	retval = sht->change_queue_type(sdev, tag_type);
-	if (retval < 0)
-		return retval;
+	if (attr == &dev_attr_queue_type.attr &&
+	    !sdev->host->hostt->change_queue_type)
+		return S_IRUGO;
 
-	return count;
+	return attr->mode;
 }
 
+/* Default template for device attributes.  May NOT be modified */
+static struct attribute *scsi_sdev_attrs[] = {
+	&dev_attr_device_blocked.attr,
+	&dev_attr_type.attr,
+	&dev_attr_scsi_level.attr,
+	&dev_attr_device_busy.attr,
+	&dev_attr_vendor.attr,
+	&dev_attr_model.attr,
+	&dev_attr_rev.attr,
+	&dev_attr_rescan.attr,
+	&dev_attr_delete.attr,
+	&dev_attr_state.attr,
+	&dev_attr_timeout.attr,
+	&dev_attr_eh_timeout.attr,
+	&dev_attr_iocounterbits.attr,
+	&dev_attr_iorequest_cnt.attr,
+	&dev_attr_iodone_cnt.attr,
+	&dev_attr_ioerr_cnt.attr,
+	&dev_attr_modalias.attr,
+	&dev_attr_queue_depth.attr,
+	&dev_attr_queue_type.attr,
+	&dev_attr_queue_ramp_up_period.attr,
+	REF_EVT(media_change),
+	REF_EVT(inquiry_change_reported),
+	REF_EVT(capacity_change_reported),
+	REF_EVT(soft_threshold_reached),
+	REF_EVT(mode_parameter_change_reported),
+	REF_EVT(lun_change_reported),
+	NULL
+};
+
+static struct attribute_group scsi_sdev_attr_group = {
+	.attrs =	scsi_sdev_attrs,
+	.is_visible =	scsi_sdev_attr_is_visible,
+};
+
+static const struct attribute_group *scsi_sdev_attr_groups[] = {
+	&scsi_sdev_attr_group,
+	NULL
+};
+
 static int scsi_target_add(struct scsi_target *starget)
 {
 	int error;
@@ -946,10 +971,6 @@ static int scsi_target_add(struct scsi_target *starget)
 	return 0;
 }
 
-static struct device_attribute sdev_attr_queue_type_rw =
-	__ATTR(queue_type, S_IRUGO | S_IWUSR, show_queue_type_field,
-	       sdev_store_queue_type_rw);
-
 /**
  * scsi_sysfs_add_sdev - add scsi device to sysfs
  * @sdev:	scsi_device to add
@@ -1003,25 +1024,6 @@ int scsi_sysfs_add_sdev(struct scsi_device *sdev)
 	transport_add_device(&sdev->sdev_gendev);
 	sdev->is_visible = 1;
 
-	/* create queue files, which may be writable, depending on the host */
-	if (sdev->host->hostt->change_queue_depth) {
-		error = device_create_file(&sdev->sdev_gendev,
-					   &sdev_attr_queue_depth_rw);
-		error = device_create_file(&sdev->sdev_gendev,
-					   &sdev_attr_queue_ramp_up_period);
-	}
-	else
-		error = device_create_file(&sdev->sdev_gendev, &dev_attr_queue_depth);
-	if (error)
-		return error;
-
-	if (sdev->host->hostt->change_queue_type)
-		error = device_create_file(&sdev->sdev_gendev, &sdev_attr_queue_type_rw);
-	else
-		error = device_create_file(&sdev->sdev_gendev, &dev_attr_queue_type);
-	if (error)
-		return error;
-
 	error = bsg_register_queue(rq, &sdev->sdev_gendev, NULL, NULL);
 
 	if (error)
-- 
1.7.12.4

[PATCHv6 0/3] Display EVPD pages in sysfs

[Hannes Reinecke]

Hi all,

This version now includes the review from hch, and I've
added a new patch to display EVPD page 0x80 in sysfs, too.
And some more sanity checks to play nice with broken devices.

Hannes Reinecke (3):
  scsi_sysfs: Implement 'is_visible' callback
  Add EVPD page 0x83 entries to sysfs
  Add EVPD page 0x80 to sysfs

 drivers/scsi/scsi_scan.c   |   5 +
 drivers/scsi/scsi_sysfs.c  | 559 +++++++++++++++++++++++++++++++++++++--------
 include/scsi/scsi.h        |   4 +-
 include/scsi/scsi_device.h |   6 +
 4 files changed, 481 insertions(+), 93 deletions(-)

-- 
1.7.12.4

[PATCH 1/3] scsi_sysfs: Implement 'is_visible' callback

[Hannes Reinecke]

Instead of modifying attributes after the device has been created
we should be using the 'is_visible' callback to avoid races.

Signed-off-by: Hannes Reinecke <hare@suse.de>
Reviewed-by: Christoph Hellwig <hch@infradead.org>
---
 drivers/scsi/scsi_sysfs.c | 184 +++++++++++++++++++++++-----------------------
 1 file changed, 93 insertions(+), 91 deletions(-)

diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 9117d0b..196e59a 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -579,7 +579,6 @@ static int scsi_sdev_check_buf_bit(const char *buf)
  * 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 (device_busy, "%d\n");
 sdev_rd_attr (type, "%d\n");
 sdev_rd_attr (scsi_level, "%d\n");
@@ -723,7 +722,37 @@ show_queue_type_field(struct device *dev, struct device_attribute *attr,
 	return snprintf(buf, 20, "%s\n", name);
 }
 
-static DEVICE_ATTR(queue_type, S_IRUGO, show_queue_type_field, NULL);
+static ssize_t
+store_queue_type_field(struct device *dev, struct device_attribute *attr,
+		       const char *buf, size_t count)
+{
+	struct scsi_device *sdev = to_scsi_device(dev);
+	struct scsi_host_template *sht = sdev->host->hostt;
+	int tag_type = 0, retval;
+	int prev_tag_type = scsi_get_tag_type(sdev);
+
+	if (!sdev->tagged_supported || !sht->change_queue_type)
+		return -EINVAL;
+
+	if (strncmp(buf, "ordered", 7) == 0)
+		tag_type = MSG_ORDERED_TAG;
+	else if (strncmp(buf, "simple", 6) == 0)
+		tag_type = MSG_SIMPLE_TAG;
+	else if (strncmp(buf, "none", 4) != 0)
+		return -EINVAL;
+
+	if (tag_type == prev_tag_type)
+		return count;
+
+	retval = sht->change_queue_type(sdev, tag_type);
+	if (retval < 0)
+		return retval;
+
+	return count;
+}
+
+static DEVICE_ATTR(queue_type, S_IRUGO | S_IWUSR, show_queue_type_field,
+		   store_queue_type_field);
 
 static ssize_t
 show_iostat_counterbits(struct device *dev, struct device_attribute *attr, 				char *buf)
@@ -797,46 +826,9 @@ DECLARE_EVT(soft_threshold_reached, SOFT_THRESHOLD_REACHED_REPORTED)
 DECLARE_EVT(mode_parameter_change_reported, MODE_PARAMETER_CHANGE_REPORTED)
 DECLARE_EVT(lun_change_reported, LUN_CHANGE_REPORTED)
 
-/* Default template for device attributes.  May NOT be modified */
-static struct attribute *scsi_sdev_attrs[] = {
-	&dev_attr_device_blocked.attr,
-	&dev_attr_type.attr,
-	&dev_attr_scsi_level.attr,
-	&dev_attr_device_busy.attr,
-	&dev_attr_vendor.attr,
-	&dev_attr_model.attr,
-	&dev_attr_rev.attr,
-	&dev_attr_rescan.attr,
-	&dev_attr_delete.attr,
-	&dev_attr_state.attr,
-	&dev_attr_timeout.attr,
-	&dev_attr_eh_timeout.attr,
-	&dev_attr_iocounterbits.attr,
-	&dev_attr_iorequest_cnt.attr,
-	&dev_attr_iodone_cnt.attr,
-	&dev_attr_ioerr_cnt.attr,
-	&dev_attr_modalias.attr,
-	REF_EVT(media_change),
-	REF_EVT(inquiry_change_reported),
-	REF_EVT(capacity_change_reported),
-	REF_EVT(soft_threshold_reached),
-	REF_EVT(mode_parameter_change_reported),
-	REF_EVT(lun_change_reported),
-	NULL
-};
-
-static struct attribute_group scsi_sdev_attr_group = {
-	.attrs =	scsi_sdev_attrs,
-};
-
-static const struct attribute_group *scsi_sdev_attr_groups[] = {
-	&scsi_sdev_attr_group,
-	NULL
-};
-
 static ssize_t
-sdev_store_queue_depth_rw(struct device *dev, struct device_attribute *attr,
-			  const char *buf, size_t count)
+sdev_store_queue_depth(struct device *dev, struct device_attribute *attr,
+		       const char *buf, size_t count)
 {
 	int depth, retval;
 	struct scsi_device *sdev = to_scsi_device(dev);
@@ -859,10 +851,10 @@ sdev_store_queue_depth_rw(struct device *dev, struct device_attribute *attr,
 
 	return count;
 }
+sdev_show_function(queue_depth, "%d\n");
 
-static struct device_attribute sdev_attr_queue_depth_rw =
-	__ATTR(queue_depth, S_IRUGO | S_IWUSR, sdev_show_queue_depth,
-	       sdev_store_queue_depth_rw);
+static DEVICE_ATTR(queue_depth, S_IRUGO | S_IWUSR, sdev_show_queue_depth,
+		   sdev_store_queue_depth);
 
 static ssize_t
 sdev_show_queue_ramp_up_period(struct device *dev,
@@ -890,40 +882,73 @@ sdev_store_queue_ramp_up_period(struct device *dev,
 	return period;
 }
 
-static struct device_attribute sdev_attr_queue_ramp_up_period =
-	__ATTR(queue_ramp_up_period, S_IRUGO | S_IWUSR,
-	       sdev_show_queue_ramp_up_period,
-	       sdev_store_queue_ramp_up_period);
+static DEVICE_ATTR(queue_ramp_up_period, S_IRUGO | S_IWUSR,
+		   sdev_show_queue_ramp_up_period,
+		   sdev_store_queue_ramp_up_period);
 
-static ssize_t
-sdev_store_queue_type_rw(struct device *dev, struct device_attribute *attr,
-			 const char *buf, size_t count)
+static umode_t scsi_sdev_attr_is_visible(struct kobject *kobj,
+					 struct attribute *attr, int i)
 {
+	struct device *dev = container_of(kobj, struct device, kobj);
 	struct scsi_device *sdev = to_scsi_device(dev);
-	struct scsi_host_template *sht = sdev->host->hostt;
-	int tag_type = 0, retval;
-	int prev_tag_type = scsi_get_tag_type(sdev);
 
-	if (!sdev->tagged_supported || !sht->change_queue_type)
-		return -EINVAL;
 
-	if (strncmp(buf, "ordered", 7) == 0)
-		tag_type = MSG_ORDERED_TAG;
-	else if (strncmp(buf, "simple", 6) == 0)
-		tag_type = MSG_SIMPLE_TAG;
-	else if (strncmp(buf, "none", 4) != 0)
-		return -EINVAL;
+	if (attr == &dev_attr_queue_depth.attr &&
+	    !sdev->host->hostt->change_queue_depth)
+		return S_IRUGO;
 
-	if (tag_type == prev_tag_type)
-		return count;
+	if (attr == &dev_attr_queue_ramp_up_period.attr &&
+	    !sdev->host->hostt->change_queue_depth)
+		return 0;
 
-	retval = sht->change_queue_type(sdev, tag_type);
-	if (retval < 0)
-		return retval;
+	if (attr == &dev_attr_queue_type.attr &&
+	    !sdev->host->hostt->change_queue_type)
+		return S_IRUGO;
 
-	return count;
+	return attr->mode;
 }
 
+/* Default template for device attributes.  May NOT be modified */
+static struct attribute *scsi_sdev_attrs[] = {
+	&dev_attr_device_blocked.attr,
+	&dev_attr_type.attr,
+	&dev_attr_scsi_level.attr,
+	&dev_attr_device_busy.attr,
+	&dev_attr_vendor.attr,
+	&dev_attr_model.attr,
+	&dev_attr_rev.attr,
+	&dev_attr_rescan.attr,
+	&dev_attr_delete.attr,
+	&dev_attr_state.attr,
+	&dev_attr_timeout.attr,
+	&dev_attr_eh_timeout.attr,
+	&dev_attr_iocounterbits.attr,
+	&dev_attr_iorequest_cnt.attr,
+	&dev_attr_iodone_cnt.attr,
+	&dev_attr_ioerr_cnt.attr,
+	&dev_attr_modalias.attr,
+	&dev_attr_queue_depth.attr,
+	&dev_attr_queue_type.attr,
+	&dev_attr_queue_ramp_up_period.attr,
+	REF_EVT(media_change),
+	REF_EVT(inquiry_change_reported),
+	REF_EVT(capacity_change_reported),
+	REF_EVT(soft_threshold_reached),
+	REF_EVT(mode_parameter_change_reported),
+	REF_EVT(lun_change_reported),
+	NULL
+};
+
+static struct attribute_group scsi_sdev_attr_group = {
+	.attrs =	scsi_sdev_attrs,
+	.is_visible =	scsi_sdev_attr_is_visible,
+};
+
+static const struct attribute_group *scsi_sdev_attr_groups[] = {
+	&scsi_sdev_attr_group,
+	NULL
+};
+
 static int scsi_target_add(struct scsi_target *starget)
 {
 	int error;
@@ -946,10 +971,6 @@ static int scsi_target_add(struct scsi_target *starget)
 	return 0;
 }
 
-static struct device_attribute sdev_attr_queue_type_rw =
-	__ATTR(queue_type, S_IRUGO | S_IWUSR, show_queue_type_field,
-	       sdev_store_queue_type_rw);
-
 /**
  * scsi_sysfs_add_sdev - add scsi device to sysfs
  * @sdev:	scsi_device to add
@@ -1003,25 +1024,6 @@ int scsi_sysfs_add_sdev(struct scsi_device *sdev)
 	transport_add_device(&sdev->sdev_gendev);
 	sdev->is_visible = 1;
 
-	/* create queue files, which may be writable, depending on the host */
-	if (sdev->host->hostt->change_queue_depth) {
-		error = device_create_file(&sdev->sdev_gendev,
-					   &sdev_attr_queue_depth_rw);
-		error = device_create_file(&sdev->sdev_gendev,
-					   &sdev_attr_queue_ramp_up_period);
-	}
-	else
-		error = device_create_file(&sdev->sdev_gendev, &dev_attr_queue_depth);
-	if (error)
-		return error;
-
-	if (sdev->host->hostt->change_queue_type)
-		error = device_create_file(&sdev->sdev_gendev, &sdev_attr_queue_type_rw);
-	else
-		error = device_create_file(&sdev->sdev_gendev, &dev_attr_queue_type);
-	if (error)
-		return error;
-
 	error = bsg_register_queue(rq, &sdev->sdev_gendev, NULL, NULL);
 
 	if (error)
-- 
1.7.12.4