[PATCHv9 0/6] Display EVPD pages in sysfs

[PATCHv9 0/6] Display EVPD pages in sysfs

[Hannes Reinecke]

Here's now v9 of my patch series to display VPD pages
in sysfs.
Changes to v8:
- Fixed 'vpg' spelling error
- Moved to binary attributes
- Implement VPD page refreshing by either calling 'rescan'
  or due to a SCSI sense event.

So with this series all outstanding issues should be resolved.
And even Bart should be happy now as the vpd information can
be refreshed at any time.

Note, that with the move to binary sysfs attributes we lost
the functionality to hide the vpd page attributes if those
pages aren't supported.
I deliberately used the grouping functionality for the binary
attribute here, otherwise it would introduce a race window
during which the uevent was sent but no vpd page attribute
was present.
If that turns out to be a problem we should be fixing the
sysfs core to allow an 'is_visible' callback for binary
attributes, too.

Hannes Reinecke (6):
  scsi_sysfs: Implement 'is_visible' callback
  scsi: Return VPD page length in scsi_vpd_inquiry()
  Add EVPD page 0x83 to sysfs
  Add EVPD page 0x80 to sysfs
  ses: Use vpd information from scsi_device
  Invalidate VPD page data

 drivers/scsi/scsi.c        | 149 ++++++++++++++++++++++++++++--
 drivers/scsi/scsi_error.c  |   1 +
 drivers/scsi/scsi_scan.c   |  10 ++-
 drivers/scsi/scsi_sysfs.c  | 220 ++++++++++++++++++++++++++-------------------
 drivers/scsi/ses.c         |  38 +++-----
 include/scsi/scsi_device.h |   7 ++
 6 files changed, 297 insertions(+), 128 deletions(-)

-- 
1.7.12.4

[PATCH 1/6] 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>
---
 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/6] scsi: Return VPD page length in scsi_vpd_inquiry()

[Hannes Reinecke]

We should be returning the number of bytes of the
requested VPD page in scsi_vpd_inquiry.
This makes it easier for the caller to verify the
required space.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/scsi.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index d8afec8..9e08d3d 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 vpd 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 get_unaligned_be16(&buffer[2]) + 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;
-- 
1.7.12.4

Re: [PATCH 2/6] scsi: Return VPD page length in scsi_vpd_inquiry()

[Tomas Henzl]

On 03/15/2014 09:51 AM, Hannes Reinecke wrote:
> We should be returning the number of bytes of the
> requested VPD page in scsi_vpd_inquiry.
> This makes it easier for the caller to verify the
> required space.
>
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>  drivers/scsi/scsi.c | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> index d8afec8..9e08d3d 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 vpd 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;

The result of scsi_execute_req should be evaluated, it may return a large positive number
like DRIVER_ERROR << 24.

(resending, my previous mail was rejected by the mailer)
Cheers, Tomas

> @@ -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 get_unaligned_be16(&buffer[2]) + 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;

[PATCH 3/6] 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        | 62 ++++++++++++++++++++++++++++++++++++++++++++++
 drivers/scsi/scsi_scan.c   |  3 +++
 drivers/scsi/scsi_sysfs.c  | 31 ++++++++++++++++++++++-
 include/scsi/scsi_device.h |  3 +++
 4 files changed, 98 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 9e08d3d..0aa1925 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -1042,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..a11bead 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);
 
@@ -754,8 +755,31 @@ store_queue_type_field(struct device *dev, struct device_attribute *attr,
 static DEVICE_ATTR(queue_type, S_IRUGO | S_IWUSR, show_queue_type_field,
 		   store_queue_type_field);
 
+#define sdev_vpd_pg_attr(_page)						\
+static ssize_t							\
+show_vpd_##_page(struct file *filp, struct kobject *kobj,	\
+		 struct bin_attribute *bin_attr,			\
+		 char *buf, loff_t off, size_t count)			\
+{									\
+	struct device *dev = container_of(kobj, struct device, kobj);	\
+	struct scsi_device *sdev = to_scsi_device(dev);			\
+	if (!sdev->vpd_##_page)						\
+		return -EINVAL;						\
+	return memory_read_from_buffer(buf, count, &off,		\
+				       sdev->vpd_##_page,		\
+				       sdev->vpd_##_page##_len);	\
+}									\
+static struct bin_attribute dev_attr_vpd_##_page = {		\
+	.attr =	{.name = __stringify(vpd_##_page), .mode = S_IRUGO },	\
+	.size = 0,							\
+	.read = show_vpd_##_page,					\
+};
+
+sdev_vpd_pg_attr(pg83);
+
 static ssize_t
-show_iostat_counterbits(struct device *dev, struct device_attribute *attr, 				char *buf)
+show_iostat_counterbits(struct device *dev, struct device_attribute *attr,
+			char *buf)
 {
 	return snprintf(buf, 20, "%d\n", (int)sizeof(atomic_t) * 8);
 }
@@ -939,8 +963,13 @@ static struct attribute *scsi_sdev_attrs[] = {
 	NULL
 };
 
+static struct bin_attribute *scsi_sdev_bin_attrs[] = {
+	&dev_attr_vpd_pg83,
+	NULL
+};
 static struct attribute_group scsi_sdev_attr_group = {
 	.attrs =	scsi_sdev_attrs,
+	.bin_attrs =	scsi_sdev_bin_attrs,
 	.is_visible =	scsi_sdev_attr_is_visible,
 };
 
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

[PATCH 4/6] 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  |  3 +++
 include/scsi/scsi_device.h |  2 ++
 3 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 0aa1925..2669cb8 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 a11bead..8c916d0 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -416,6 +416,7 @@ static void scsi_device_dev_release_usercontext(struct work_struct *work)
 	scsi_target_reap(scsi_target(sdev));
 
 	kfree(sdev->vpd_pg83);
+	kfree(sdev->vpd_pg80);
 	kfree(sdev->inquiry);
 	kfree(sdev);
 
@@ -776,6 +777,7 @@ static struct bin_attribute dev_attr_vpd_##_page = {		\
 };
 
 sdev_vpd_pg_attr(pg83);
+sdev_vpd_pg_attr(pg80);
 
 static ssize_t
 show_iostat_counterbits(struct device *dev, struct device_attribute *attr,
@@ -965,6 +967,7 @@ static struct attribute *scsi_sdev_attrs[] = {
 
 static struct bin_attribute *scsi_sdev_bin_attrs[] = {
 	&dev_attr_vpd_pg83,
+	&dev_attr_vpd_pg80,
 	NULL
 };
 static struct attribute_group scsi_sdev_attr_group = {
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

[PATCH 5/6] ses: Use vpd information from scsi_device

[Hannes Reinecke]

The scsi_device now has VPD page83 information attached, so
there is no need to query it again.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/ses.c | 38 ++++++++++----------------------------
 1 file changed, 10 insertions(+), 28 deletions(-)

diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
index eba183c..80bfece 100644
--- a/drivers/scsi/ses.c
+++ b/drivers/scsi/ses.c
@@ -25,6 +25,7 @@
 #include <linux/module.h>
 #include <linux/kernel.h>
 #include <linux/enclosure.h>
+#include <asm/unaligned.h>
 
 #include <scsi/scsi.h>
 #include <scsi/scsi_cmnd.h>
@@ -448,27 +449,18 @@ static void ses_enclosure_data_process(struct enclosure_device *edev,
 static void ses_match_to_enclosure(struct enclosure_device *edev,
 				   struct scsi_device *sdev)
 {
-	unsigned char *buf;
 	unsigned char *desc;
-	unsigned int vpd_len;
 	struct efd efd = {
 		.addr = 0,
 	};
 
-	buf = kmalloc(INIT_ALLOC_SIZE, GFP_KERNEL);
-	if (!buf || scsi_get_vpd_page(sdev, 0x83, buf, INIT_ALLOC_SIZE))
-		goto free;
-
 	ses_enclosure_data_process(edev, to_scsi_device(edev->edev.parent), 0);
 
-	vpd_len = ((buf[2] << 8) | buf[3]) + 4;
-	kfree(buf);
-	buf = kmalloc(vpd_len, GFP_KERNEL);
-	if (!buf ||scsi_get_vpd_page(sdev, 0x83, buf, vpd_len))
-		goto free;
+	if (!sdev->vpd_pg83_len)
+		return;
 
-	desc = buf + 4;
-	while (desc < buf + vpd_len) {
+	desc = sdev->vpd_pg83 + 4;
+	while (desc < sdev->vpd_pg83 + sdev->vpd_pg83_len) {
 		enum scsi_protocol proto = desc[0] >> 4;
 		u8 code_set = desc[0] & 0x0f;
 		u8 piv = desc[1] & 0x80;
@@ -478,25 +470,15 @@ static void ses_match_to_enclosure(struct enclosure_device *edev,
 
 		if (piv && code_set == 1 && assoc == 1
 		    && proto == SCSI_PROTOCOL_SAS && type == 3 && len == 8)
-			efd.addr = (u64)desc[4] << 56 |
-				(u64)desc[5] << 48 |
-				(u64)desc[6] << 40 |
-				(u64)desc[7] << 32 |
-				(u64)desc[8] << 24 |
-				(u64)desc[9] << 16 |
-				(u64)desc[10] << 8 |
-				(u64)desc[11];
+			efd.addr = get_unaligned_be64(&desc[4]);
 
 		desc += len + 4;
 	}
-	if (!efd.addr)
-		goto free;
+	if (efd.addr) {
+		efd.dev = &sdev->sdev_gendev;
 
-	efd.dev = &sdev->sdev_gendev;
-
-	enclosure_for_each_device(ses_enclosure_find_by_addr, &efd);
- free:
-	kfree(buf);
+		enclosure_for_each_device(ses_enclosure_find_by_addr, &efd);
+	}
 }
 
 static int ses_intf_add(struct device *cdev,
-- 
1.7.12.4

[PATCH 6/6] Invalidate VPD page data

[Hannes Reinecke]

Add a flag 'vpd_invalid' to the SCSI device to indicate that
the VPD data needs to be refreshed. This is required if either
a manual rescan is triggered or if the sense code INQUIRY DATA
HAS CHANGED has been received.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/scsi.c        | 91 ++++++++++++++++++++++++++++++++++------------
 drivers/scsi/scsi_error.c  |  1 +
 drivers/scsi/scsi_scan.c   |  7 +++-
 drivers/scsi/scsi_sysfs.c  |  6 ++-
 drivers/scsi/ses.c         |  2 +-
 include/scsi/scsi_device.h |  2 +
 6 files changed, 82 insertions(+), 27 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 2669cb8..971b099 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -1056,10 +1056,11 @@ void scsi_attach_vpd(struct scsi_device *sdev)
 	int vpd_len = 255;
 	int pg80_supported = 0;
 	int pg83_supported = 0;
-	unsigned char *vpd_buf;
+	unsigned char *vpd_buf, *tmp_pg;
 
 	if (sdev->skip_vpd_pages)
 		return;
+
 retry_pg0:
 	vpd_buf = kmalloc(vpd_len, GFP_KERNEL);
 	if (!vpd_buf)
@@ -1087,45 +1088,89 @@ retry_pg0:
 	}
 	kfree(vpd_buf);
 
-	if (pg80_supported) {
 retry_pg80:
+	if (pg80_supported) {
 		vpd_buf = kmalloc(vpd_len, GFP_KERNEL);
 		if (!vpd_buf)
-			return;
-
-		result = scsi_vpd_inquiry(sdev, vpd_buf, 0x80, vpd_len);
+			result = -ENOMEM;
+		else
+			result = scsi_vpd_inquiry(sdev, vpd_buf,
+						  0x80, vpd_len);
 		if (result < 0) {
 			kfree(vpd_buf);
+			spin_lock(&sdev->reconfig_lock);
+			tmp_pg = sdev->vpd_pg80;
+			sdev->vpd_pg80 = NULL;
+			sdev->vpd_pg80_len = result;
+			kfree(tmp_pg);
+			spin_unlock(&sdev->reconfig_lock);
+			/*
+			 * An unexpected error occurred,
+			 * do not clear vpd_invalid flag
+			 */
 			return;
+		} else {
+			if (result > vpd_len) {
+				vpd_len = result;
+				kfree(vpd_buf);
+				goto retry_pg80;
+			}
+			spin_lock(&sdev->reconfig_lock);
+			sdev->vpd_pg80 = vpd_buf;
+			sdev->vpd_pg80_len = result;
+			spin_unlock(&sdev->reconfig_lock);
 		}
-		if (result > vpd_len) {
-			vpd_len = result;
-			kfree(vpd_buf);
-			goto retry_pg80;
-		}
-		sdev->vpd_pg80_len = result;
-		sdev->vpd_pg80 = vpd_buf;
+	} else {
+		spin_lock(&sdev->reconfig_lock);
+		tmp_pg = sdev->vpd_pg80;
+		sdev->vpd_pg80 = NULL;
+		sdev->vpd_pg80_len = -ENOENT;
+		kfree(tmp_pg);
+		spin_unlock(&sdev->reconfig_lock);
 	}
 
-	if (pg83_supported) {
 retry_pg83:
+	if (pg83_supported) {
 		vpd_buf = kmalloc(vpd_len, GFP_KERNEL);
 		if (!vpd_buf)
-			return;
-
-		result = scsi_vpd_inquiry(sdev, vpd_buf, 0x83, vpd_len);
+			result = -ENOMEM;
+		else
+			result = scsi_vpd_inquiry(sdev, vpd_buf,
+						  0x83, vpd_len);
 		if (result < 0) {
 			kfree(vpd_buf);
+			spin_lock(&sdev->reconfig_lock);
+			tmp_pg = sdev->vpd_pg83;
+			sdev->vpd_pg83 = NULL;
+			sdev->vpd_pg83_len = result;
+			kfree(tmp_pg);
+			spin_unlock(&sdev->reconfig_lock);
+			/*
+			 * An unexpected error occurred,
+			 * do not clear vpd_invalid flag
+			 */
 			return;
+		} else {
+			if (result > vpd_len) {
+				vpd_len = result;
+				kfree(vpd_buf);
+				goto retry_pg83;
+			}
+			spin_lock(&sdev->reconfig_lock);
+			sdev->vpd_pg83 = vpd_buf;
+			sdev->vpd_pg83_len = result;
+			spin_unlock(&sdev->reconfig_lock);
 		}
-		if (result > vpd_len) {
-			vpd_len = result;
-			kfree(vpd_buf);
-			goto retry_pg83;
-		}
-		sdev->vpd_pg83_len = result;
-		sdev->vpd_pg83 = vpd_buf;
+	} else {
+		spin_lock(&sdev->reconfig_lock);
+		tmp_pg = sdev->vpd_pg83;
+		sdev->vpd_pg83 = NULL;
+		sdev->vpd_pg83_len = -ENOENT;
+		kfree(tmp_pg);
+		spin_unlock(&sdev->reconfig_lock);
 	}
+
+	sdev->vpd_invalid = 0;
 }
 
 /**
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 78b004d..b1468c7 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -393,6 +393,7 @@ static void scsi_report_sense(struct scsi_device *sdev,
 
 	if (sshdr->sense_key == UNIT_ATTENTION) {
 		if (sshdr->asc == 0x3f && sshdr->ascq == 0x03) {
+			sdev->vpd_invalid = 1;
 			evt_type = SDEV_EVT_INQUIRY_CHANGE_REPORTED;
 			sdev_printk(KERN_WARNING, sdev,
 				    "Inquiry data has changed");
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 154bb5e..d177929 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -252,6 +252,7 @@ static struct scsi_device *scsi_alloc_sdev(struct scsi_target *starget,
 	INIT_LIST_HEAD(&sdev->starved_entry);
 	INIT_LIST_HEAD(&sdev->event_list);
 	spin_lock_init(&sdev->list_lock);
+	spin_lock_init(&sdev->reconfig_lock);
 	INIT_WORK(&sdev->event_work, scsi_evt_thread);
 	INIT_WORK(&sdev->requeue_work, scsi_requeue_run_queue);
 
@@ -1558,7 +1559,11 @@ EXPORT_SYMBOL(scsi_add_device);
 void scsi_rescan_device(struct device *dev)
 {
 	struct scsi_driver *drv;
-	
+	struct scsi_device *sdev = to_scsi_device(dev);
+
+	sdev->vpd_invalid = 1;
+	scsi_attach_vpd(sdev);
+
 	if (!dev->driver)
 		return;
 
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 8c916d0..315d1d3 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -764,8 +764,10 @@ show_vpd_##_page(struct file *filp, struct kobject *kobj,	\
 {									\
 	struct device *dev = container_of(kobj, struct device, kobj);	\
 	struct scsi_device *sdev = to_scsi_device(dev);			\
-	if (!sdev->vpd_##_page)						\
-		return -EINVAL;						\
+	if (sdev->vpd_invalid)						\
+		scsi_attach_vpd(sdev);					\
+	if (sdev->vpd_##_page##_len < 0)				\
+		return sdev->vpd_##_page##_len;				\
 	return memory_read_from_buffer(buf, count, &off,		\
 				       sdev->vpd_##_page,		\
 				       sdev->vpd_##_page##_len);	\
diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
index 80bfece..773766a 100644
--- a/drivers/scsi/ses.c
+++ b/drivers/scsi/ses.c
@@ -456,7 +456,7 @@ static void ses_match_to_enclosure(struct enclosure_device *edev,
 
 	ses_enclosure_data_process(edev, to_scsi_device(edev->edev.parent), 0);
 
-	if (!sdev->vpd_pg83_len)
+	if (sdev->vpd_pg83_len < 4)
 		return;
 
 	desc = sdev->vpd_pg83 + 4;
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 2ea8212..9fa5771 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -117,6 +117,7 @@ struct scsi_device {
 	unsigned char *vpd_pg83;
 	unsigned char vpd_pg80_len;
 	unsigned char *vpd_pg80;
+	spinlock_t reconfig_lock;
 	unsigned char current_tag;	/* current tag */
 	struct scsi_target      *sdev_target;   /* used only for single_lun */
 
@@ -171,6 +172,7 @@ struct scsi_device {
 	unsigned is_visible:1;	/* is the device visible in sysfs */
 	unsigned wce_default_on:1;	/* Cache is ON by default */
 	unsigned no_dif:1;	/* T10 PI (DIF) should be disabled */
+	unsigned vpd_invalid:1; /* VPD data needs to be refreshed */
 
 	atomic_t disk_events_disable_depth; /* disable depth for disk events */
 
-- 
1.7.12.4

Re: [PATCH 6/6] Invalidate VPD page data

[Jeremy Linton]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 3/15/2014 3:51 AM, Hannes Reinecke wrote:
> Add a flag 'vpd_invalid' to the SCSI device to indicate that the VPD data
> needs to be refreshed. This is required if either a manual rescan is
> triggered or if the sense code INQUIRY DATA HAS CHANGED has been received.


> --- a/drivers/scsi/scsi_error.c +++ b/drivers/scsi/scsi_error.c @@ -393,6
> +393,7 @@ static void scsi_report_sense(struct scsi_device *sdev,
> 
> if (sshdr->sense_key == UNIT_ATTENTION) { if (sshdr->asc == 0x3f &&
> sshdr->ascq == 0x03) { +			sdev->vpd_invalid = 1;


	I didn't study the whole code path but does the VPD data get updated on a
6/2900? I suspect it should be.
	I can imagine a number of cases where the luns changed check condition gets
preempted/lost by a device reset. I guess much of that should be masked by the
port login/logout, but its probably better to be safe...





-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (MingW32)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQEcBAEBAgAGBQJTJ3MRAAoJEL5i86xrzcy7vsAIAKyiMPZ0FBlLRxlQsGQxHaet
8FTCoj0GtgE1hmw+BfLvKzdR5VqMNt/yTSsJd/8OZrykDQ298TQlfgoSle7/dpYp
FDaMq2uXINGpe+EC/OvVGH8GJbOgdjLectwu2EqKhkMblpyBPM83XXWNOD1lbLYf
/TN/WPug9s5NOwdwSxeNhZRZKVw/9T33fxVKlXQg/sExfjIeFqHSTxIRH9bvktvw
/ewe85P8WNtTXwZUGj1O3PaPzg0B98+LgHmAJNYREBf7t/mDZpkR492Ty9fRKkxi
SauSIvdaNWuc28a88xaJGD+WRDPqSbLjecpNnWiYNfbNrNKx/WoJUpfVJS+Ltmk=
=mfSZ
-----END PGP SIGNATURE-----

Re: [PATCH 6/6] Invalidate VPD page data

[Hannes Reinecke]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 03/17/2014 11:11 PM, Jeremy Linton wrote:
> On 3/15/2014 3:51 AM, Hannes Reinecke wrote:
>> Add a flag 'vpd_invalid' to the SCSI device to indicate that
>> the VPD data needs to be refreshed. This is required if
>> either a manual rescan is triggered or if the sense code
>> INQUIRY DATA HAS CHANGED has been received.
> 
> 
>> --- a/drivers/scsi/scsi_error.c +++
>> b/drivers/scsi/scsi_error.c @@ -393,6 +393,7 @@ static void
>> scsi_report_sense(struct scsi_device *sdev,
> 
>> if (sshdr->sense_key == UNIT_ATTENTION) { if (sshdr->asc ==
>> 0x3f && sshdr->ascq == 0x03) { +			sdev->vpd_invalid = 1;
> 
> 
> I didn't study the whole code path but does the VPD data get
> updated on a 6/2900? I suspect it should be. I can imagine a
> number of cases where the luns changed check condition gets 
> preempted/lost by a device reset. I guess much of that should
> be masked by the port login/logout, but its probably better to
> be safe...
> 
Argl.

I was hoping to avoid that; I've already had a rather lengthy
discussion with NetApp about handling Power-on-Reset UA.

We should be discussion that at LSF; Power-on-Reset UA handling
(and queued UA handling in general) has some implications which
could do with a proper elaboration. Rumours have it that Fred
Knight from NetApp will also at LSF, so we'll have someone to ask
for any technical issues :-).

And that's precisely why I hooked the 'sdev->vpd_invalid' flag
into the 'rescan' attribute, so that it can be refreshed on demand.

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)
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.19 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJTJ+1KAAoJEGz4yi9OyKjP8G8P/2+fO+nYNCJa9KyMfx9/IUke
VA5Ap81H2tbyUkmZp9irnegV9HNg7OF66MMNo+o5MvB96RZGCnma5pjUfmwpk4AW
as188YlrT36FSC769LH+7n7EA5rcCVuCzRkZkiqjvh7xG36Z7yg4HpKjx1aX0tHh
3iTvYy1IuPfhzti4W533lybmrFYJi7Izjr/qHV/AjeWlkmsGSVf/eb4A84dKskjn
RB0ahFrS86HVCOOvH233tsjNrN1ToP7+nNQT4cyDGT1mUqQZHQlNCz75zhJBUjAL
QuT31T2SiT0dS3EmpLIU/oI0A0rN8NbJo7zV1LpbVTHvJCVrNtkUtjMtxFV0/CMk
/ppWZGyIRjkPcCzNS01QVI1bUywpPbOhfVjJiG15gYY1Ef6z6/uktuX9SNOGWAzw
1MYylda+c10rg3I47nZTjahTKnnGkKBi7OLVXqi0hs0lHW7gA0UpZLYkovcOL4bA
7fYwABVDmfP65bzWl1tk9qmWoLeneD9TW5aaLkQ8d8qr/pd3oo1zd+RC6cgBQVXH
A2tRHTF36+R7C8kOO937tvQ2QIWa5YXNlgeaQkYAoOegJq0CWTB2kSe+f/h9EBtv
+YNLNPldhHD3bj0Poqrw2wXVfEWFlXD5Blz/2ioHPzzOx6EkQgH5uNGzyuyKR+5p
MMNGFIoQZSyqfJJNVtIK
=0T1G
-----END PGP SIGNATURE-----
--
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 6/6] Invalidate VPD page data

[James Bottomley]

On Tue, 2014-03-18 at 07:52 +0100, Hannes Reinecke wrote:
> On 03/17/2014 11:11 PM, Jeremy Linton wrote:
> > On 3/15/2014 3:51 AM, Hannes Reinecke wrote:
> >> Add a flag 'vpd_invalid' to the SCSI device to indicate that
> >> the VPD data needs to be refreshed. This is required if
> >> either a manual rescan is triggered or if the sense code
> >> INQUIRY DATA HAS CHANGED has been received.
> > 
> > 
> >> --- a/drivers/scsi/scsi_error.c +++
> >> b/drivers/scsi/scsi_error.c @@ -393,6 +393,7 @@ static void
> >> scsi_report_sense(struct scsi_device *sdev,
> > 
> >> if (sshdr->sense_key == UNIT_ATTENTION) { if (sshdr->asc ==
> >> 0x3f && sshdr->ascq == 0x03) { +			sdev->vpd_invalid = 1;
> > 
> > 
> > I didn't study the whole code path but does the VPD data get
> > updated on a 6/2900? I suspect it should be. I can imagine a
> > number of cases where the luns changed check condition gets 
> > preempted/lost by a device reset. I guess much of that should
> > be masked by the port login/logout, but its probably better to
> > be safe...
> > 
> Argl.
> 
> I was hoping to avoid that; I've already had a rather lengthy
> discussion with NetApp about handling Power-on-Reset UA.
> 
> We should be discussion that at LSF; Power-on-Reset UA handling
> (and queued UA handling in general) has some implications which
> could do with a proper elaboration. Rumours have it that Fred
> Knight from NetApp will also at LSF, so we'll have someone to ask
> for any technical issues :-).

You don't have to rely on rumour; the attendee list is online:

https://docs.google.com/spreadsheet/pub?key=0ArurRVMVCSnkdHU2Zk1KbFhmeVZFVmFMQ19nakJYaFE&gid=1

I'm not sure there's a full session on power on/reset UA handling.
Right at the moment, we eat any UA after a reset.  Perhaps we should
collect the sense and dump it if it's what we expect.

James


> And that's precisely why I hooked the 'sdev->vpd_invalid' flag
> into the 'rescan' attribute, so that it can be refreshed on demand.
> 
> Cheers,
> 
> Hannes
> --
> 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 6/6] Invalidate VPD page data

[Elliott, Robert (Server Storage)]

> On 03/17/2014 11:11 PM, Jeremy Linton wrote:
> >
> > I didn't study the whole code path but does the VPD data get
> > updated on a 6/2900? I suspect it should be. I can imagine a
> > number of cases where the luns changed check condition gets
> > preempted/lost by a device reset. I guess much of that should
> > be masked by the port login/logout, but its probably better to
> > be safe...
> >

See the unit attention condition precedence level table in the 
SCSI Architecture Model (sam5r15 table 54 page 121).

POWER ON, RESET, OR BUS DEVICE RESET OCCURRED 
(29h 00h) has the highest precedence, meaning that
any other unit attention condition might also have 
occurred but no longer be individually reported.

Similarly, all the others in table 54 have precedence over
(and thus imply the possibility of) INQUIRY DATA HAS 
CHANGED.

In addition to the codes in table 54, since INQUIRY DATA 
HAS CHANGED has a nonzero ASCQ (3Fh 03h),  it also has 
lower precedence than 3Fh 00h TARGET OPERATING 
CONDITIONS HAVE CHANGED.

For generic SCSI code, the "protocol specific" codes
may be the hardest to handle.  That covers these old 
parallel SCSI codes:
29h 05h TRANSCEIVER MODE CHANGED TO SINGLE-ENDED
29h 06h TRANSCEIVER MODE CHANGED TO LVD
but allows newer protocols to add codes if needed.

You may need the LLD to report an optional list of 
extra codes to consider at that precedence level.

Re: [PATCH 6/6] Invalidate VPD page data

[James Bottomley]

On Sat, 2014-03-15 at 09:51 +0100, Hannes Reinecke wrote:
> Add a flag 'vpd_invalid' to the SCSI device to indicate that
> the VPD data needs to be refreshed. This is required if either
> a manual rescan is triggered or if the sense code INQUIRY DATA
> HAS CHANGED has been received.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>  drivers/scsi/scsi.c        | 91 ++++++++++++++++++++++++++++++++++------------
>  drivers/scsi/scsi_error.c  |  1 +
>  drivers/scsi/scsi_scan.c   |  7 +++-
>  drivers/scsi/scsi_sysfs.c  |  6 ++-
>  drivers/scsi/ses.c         |  2 +-
>  include/scsi/scsi_device.h |  2 +
>  6 files changed, 82 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> index 2669cb8..971b099 100644
> --- a/drivers/scsi/scsi.c
> +++ b/drivers/scsi/scsi.c
> @@ -1056,10 +1056,11 @@ void scsi_attach_vpd(struct scsi_device *sdev)
>  	int vpd_len = 255;
>  	int pg80_supported = 0;
>  	int pg83_supported = 0;
> -	unsigned char *vpd_buf;
> +	unsigned char *vpd_buf, *tmp_pg;
>  
>  	if (sdev->skip_vpd_pages)
>  		return;
> +
>  retry_pg0:
>  	vpd_buf = kmalloc(vpd_len, GFP_KERNEL);
>  	if (!vpd_buf)
> @@ -1087,45 +1088,89 @@ retry_pg0:
>  	}
>  	kfree(vpd_buf);
>  
> -	if (pg80_supported) {
>  retry_pg80:
> +	if (pg80_supported) {
>  		vpd_buf = kmalloc(vpd_len, GFP_KERNEL);
>  		if (!vpd_buf)
> -			return;
> -
> -		result = scsi_vpd_inquiry(sdev, vpd_buf, 0x80, vpd_len);
> +			result = -ENOMEM;
> +		else
> +			result = scsi_vpd_inquiry(sdev, vpd_buf,
> +						  0x80, vpd_len);
>  		if (result < 0) {
>  			kfree(vpd_buf);
> +			spin_lock(&sdev->reconfig_lock);
> +			tmp_pg = sdev->vpd_pg80;
> +			sdev->vpd_pg80 = NULL;
> +			sdev->vpd_pg80_len = result;
> +			kfree(tmp_pg);
> +			spin_unlock(&sdev->reconfig_lock);
> +			/*
> +			 * An unexpected error occurred,
> +			 * do not clear vpd_invalid flag
> +			 */
>  			return;
> +		} else {
> +			if (result > vpd_len) {
> +				vpd_len = result;
> +				kfree(vpd_buf);
> +				goto retry_pg80;
> +			}
> +			spin_lock(&sdev->reconfig_lock);
> +			sdev->vpd_pg80 = vpd_buf;
> +			sdev->vpd_pg80_len = result;
> +			spin_unlock(&sdev->reconfig_lock);
>  		}
> -		if (result > vpd_len) {
> -			vpd_len = result;
> -			kfree(vpd_buf);
> -			goto retry_pg80;
> -		}
> -		sdev->vpd_pg80_len = result;
> -		sdev->vpd_pg80 = vpd_buf;
> +	} else {
> +		spin_lock(&sdev->reconfig_lock);
> +		tmp_pg = sdev->vpd_pg80;
> +		sdev->vpd_pg80 = NULL;
> +		sdev->vpd_pg80_len = -ENOENT;
> +		kfree(tmp_pg);
> +		spin_unlock(&sdev->reconfig_lock);

Actually, this reconfig lock thing doesn't work.  If you look in ses,
we're looping over the vpd_lengths and taking offsets into the data
while the data is changing, regardless of the lock.

Firstly, you can just do a kfree on sdev->vpd_pgxx; no need for the
tmp_pg.  Secondly, we would now need a helper to return the vpd_pgxx
which sees the lock and probably copies the data to make sure we can't
get a change while using the data.

I've dropped this patch for now.

James