[PATCHv8 0/5] Display EVPD pages in sysfs

[PATCHv8 0/5] Display EVPD pages in sysfs

[Hannes Reinecke]

Here's now v8 of my patch series to display VPD pages
in sysfs.
Changes to v7:
- Modify ses driver to use vpd_pg83 information
- Use get_unaligned_XX where possible
- Split off modifications to scsi_vpd_inquiry()
  into a separate patch.

So with this series all outstanding issues should be resolved.
As noted by Bart van Assche the information in the VPD pages
is _not_ updated when a sense code for LUN Inquiry data changed
was received. This would involved a re-design of the current
scanning infrastructure, which is out of scope for this patch
series.

Hannes Reinecke (5):
  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

 drivers/scsi/scsi.c        | 104 ++++++++++++++++++--
 drivers/scsi/scsi_scan.c   |   3 +
 drivers/scsi/scsi_sysfs.c  | 234 +++++++++++++++++++++++++++------------------
 drivers/scsi/ses.c         |  38 ++------
 include/scsi/scsi_device.h |   5 +
 5 files changed, 257 insertions(+), 127 deletions(-)

-- 
1.7.12.4

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

[Muthukumar R]

On Mon, Mar 10, 2014 at 7:28 AM, Hannes Reinecke <hare@suse.de> 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..ecaeff1 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.
>   */
Typo:                              ^^^

 Should be: vpd page


>  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;


You mean:

            if (result < 0)
                   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
>
> --
> 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/5] scsi: Return VPD page length in scsi_vpd_inquiry()

[Douglas Gilbert]

On 14-03-12 10:14 PM, Muthukumar R wrote:
> On Mon, Mar 10, 2014 at 7:28 AM, Hannes Reinecke <hare@suse.de> 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..ecaeff1 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.
>>    */
> Typo:                              ^^^
>
>   Should be: vpd page
>
>
>>   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;
>
>
> You mean:
>
>              if (result < 0)
>                     goto fail;

I think that he means:
      if (result < 4)
           goto fail;

The first 4 bytes of a VPD page are a header with buf[2] and
buf[3] being the length of the following payload. Without
both of them "fail" is a quite accurate description.

That said, it is hard to get a number between 0 and 3 with:
    return get_unaligned_be16(&buffer[2]) + 4;
but if you wanted to take resid into account it is possible.

IMO this snippet is fine.


Doug Gilbert

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

[Muthukumar R]

On Wed, Mar 12, 2014 at 2:43 PM, Douglas Gilbert <dgilbert@interlog.com> wrote:
> On 14-03-12 10:14 PM, Muthukumar R wrote:
>>
>> On Mon, Mar 10, 2014 at 7:28 AM, Hannes Reinecke <hare@suse.de> 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..ecaeff1 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.
>>>    */
>>
>> Typo:                              ^^^
>>
>>   Should be: vpd page
>>
>>
>>>   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;
>>
>>
>>
>> You mean:
>>
>>              if (result < 0)
>>                     goto fail;
>
>
> I think that he means:
>
>      if (result < 4)
>           goto fail;
>
> The first 4 bytes of a VPD page are a header with buf[2] and
> buf[3] being the length of the following payload. Without
> both of them "fail" is a quite accurate description.
>
> That said, it is hard to get a number between 0 and 3 with:
>
>    return get_unaligned_be16(&buffer[2]) + 4;
> but if you wanted to take resid into account it is possible.
>
> IMO this snippet is fine.
>

I see.. thanks. Comment is slightly misleading but I guess its OK in this case.


>
> Doug Gilbert
>
>
>
>

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

[Hannes Reinecke]

On 03/12/2014 10:14 PM, Muthukumar R wrote:
> On Mon, Mar 10, 2014 at 7:28 AM, Hannes Reinecke <hare@suse.de> 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..ecaeff1 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.
>>   */
> Typo:                              ^^^
> 
>  Should be: vpd page
> 
> 
Ah. Correct.

>>  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;
> 
> 
> You mean:
> 
>             if (result < 0)
>                    goto fail;
> 
> 
No. See above. The actual data starts at offset 4.

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/5] 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  | 44 +++++++++++++++++++++++++++++++-
 include/scsi/scsi_device.h |  3 +++
 4 files changed, 111 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index ecaeff1..f753373 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..5d5e55a 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,43 @@ 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 buf_len)
+{
+	int len = 0, i;
+
+	if (!pg_buf)
+		return -EINVAL;
+
+	len = 0;
+	for (i = 0; i < pg_len; i += 16) {
+		if (len + 2 > buf_len) {
+			len = -EFAULT;
+			break;
+		}
+		hex_dump_to_buffer(pg_buf + i, pg_len, 16, 1,
+				   buf + len, buf_len - len, 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, PAGE_SIZE);				\
+}									\
+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 +942,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 +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_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 3/5] Add EVPD page 0x83 to sysfs

[James Bottomley]

On Mon, 2014-03-10 at 15:28 +0100, Hannes Reinecke wrote:
> 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.

Christoph's suggestion of binary sysfs attributes for this rather than
the text ones you have is better ... because your current ones are going
to truncate when they run off the one page of data sysfs text attributes
get (i.e. about 2k of vpd).

James

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

[Hannes Reinecke]

On 03/10/2014 07:24 PM, James Bottomley wrote:
> On Mon, 2014-03-10 at 15:28 +0100, Hannes Reinecke wrote:
>> 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.
>
> Christoph's suggestion of binary sysfs attributes for this rather than
> the text ones you have is better ... because your current ones are going
> to truncate when they run off the one page of data sysfs text attributes
> get (i.e. about 2k of vpd).
>
Yes, I thought of that, too.
I thought to remember that binary attributes are reserved for 
firmware/hardware-dependent interfaces.
If that's not the case I'll be moving to a binary attribute here.
Will be resending the patchset.

What should happen with the first patch in the series, then?
When moving to a binary attribute the first patch isn't required 
anymore; should I drop it or send as a separate patch?

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 3/5] Add EVPD page 0x83 to sysfs

[James Bottomley]

On Mon, 2014-03-10 at 21:52 +0100, Hannes Reinecke wrote:
> On 03/10/2014 07:24 PM, James Bottomley wrote:
> > On Mon, 2014-03-10 at 15:28 +0100, Hannes Reinecke wrote:
> >> 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.
> >
> > Christoph's suggestion of binary sysfs attributes for this rather than
> > the text ones you have is better ... because your current ones are going
> > to truncate when they run off the one page of data sysfs text attributes
> > get (i.e. about 2k of vpd).
> >
> Yes, I thought of that, too.
> I thought to remember that binary attributes are reserved for 
> firmware/hardware-dependent interfaces.
> If that's not the case I'll be moving to a binary attribute here.
> Will be resending the patchset.
> 
> What should happen with the first patch in the series, then?
> When moving to a binary attribute the first patch isn't required 
> anymore; should I drop it or send as a separate patch?

It can be applied separately.

I also still think we can get around the caching problem by requesting
the vpd page every time.  Arrays will change it under us and the cache
will go stale.  Even enclosures sometimes swallow unplug plug events and
simply change the vpd data.

James


James

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

[Hannes Reinecke]

On 03/11/2014 05:14 AM, James Bottomley wrote:
> On Mon, 2014-03-10 at 21:52 +0100, Hannes Reinecke wrote:
>> On 03/10/2014 07:24 PM, James Bottomley wrote:
>>> On Mon, 2014-03-10 at 15:28 +0100, Hannes Reinecke wrote:
>>>> 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.
>>>
>>> Christoph's suggestion of binary sysfs attributes for this rather than
>>> the text ones you have is better ... because your current ones are going
>>> to truncate when they run off the one page of data sysfs text attributes
>>> get (i.e. about 2k of vpd).
>>>
>> Yes, I thought of that, too.
>> I thought to remember that binary attributes are reserved for
>> firmware/hardware-dependent interfaces.
>> If that's not the case I'll be moving to a binary attribute here.
>> Will be resending the patchset.
>>
>> What should happen with the first patch in the series, then?
>> When moving to a binary attribute the first patch isn't required
>> anymore; should I drop it or send as a separate patch?
>
> It can be applied separately.
>
> I also still think we can get around the caching problem by requesting
> the vpd page every time.  Arrays will change it under us and the cache
> will go stale.  Even enclosures sometimes swallow unplug plug events and
> simply change the vpd data.
>
Hmm. The whole idea of having the vpd data in sysfs is precisely so that 
one does _not_ have to do I/O to access it.
I think using a flag to invalidate data would be better here.

And will keep Bart happy :-)

Will be updating the 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: [PATCH 3/5] Add EVPD page 0x83 to sysfs

[James Bottomley]

On Tue, 2014-03-11 at 12:56 +0100, Hannes Reinecke wrote:
> On 03/11/2014 05:14 AM, James Bottomley wrote:
> > On Mon, 2014-03-10 at 21:52 +0100, Hannes Reinecke wrote:
> >> On 03/10/2014 07:24 PM, James Bottomley wrote:
> >>> On Mon, 2014-03-10 at 15:28 +0100, Hannes Reinecke wrote:
> >>>> 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.
> >>>
> >>> Christoph's suggestion of binary sysfs attributes for this rather than
> >>> the text ones you have is better ... because your current ones are going
> >>> to truncate when they run off the one page of data sysfs text attributes
> >>> get (i.e. about 2k of vpd).
> >>>
> >> Yes, I thought of that, too.
> >> I thought to remember that binary attributes are reserved for
> >> firmware/hardware-dependent interfaces.
> >> If that's not the case I'll be moving to a binary attribute here.
> >> Will be resending the patchset.
> >>
> >> What should happen with the first patch in the series, then?
> >> When moving to a binary attribute the first patch isn't required
> >> anymore; should I drop it or send as a separate patch?
> >
> > It can be applied separately.
> >
> > I also still think we can get around the caching problem by requesting
> > the vpd page every time.  Arrays will change it under us and the cache
> > will go stale.  Even enclosures sometimes swallow unplug plug events and
> > simply change the vpd data.
> >
> Hmm. The whole idea of having the vpd data in sysfs is precisely so that 
> one does _not_ have to do I/O to access it.

No, the point is to provide a uniform interface to User Space to prevent
*userspace* from having to do I/O (which it frequently gets wrong).
What we do under the covers is up to us.  What's the reason for not
wanting to do I/O every time?  Getting VPD data isn't a time critical
operation is it?  so what's the benefit of caching it?

James


> I think using a flag to invalidate data would be better here.
> 
> And will keep Bart happy :-)
> 
> Will be updating the patchset.
> 
> Cheers,
> 
> Hannes

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

[Martin K. Petersen]

>>>>> "James" == James Bottomley <jbottomley@parallels.com> writes:

James> What we do under the covers is up to us.  What's the reason for
James> not wanting to do I/O every time?  Getting VPD data isn't a time
James> critical operation is it?  so what's the benefit of caching it?

XCopy. Want us to do a VPD page query for every command (~32 megs of
data)?

-- 
Martin K. Petersen	Oracle Linux Engineering

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

[James Bottomley]

On Wed, 2014-03-12 at 21:53 -0400, Martin K. Petersen wrote:
> >>>>> "James" == James Bottomley <jbottomley@parallels.com> writes:
> 
> James> What we do under the covers is up to us.  What's the reason for
> James> not wanting to do I/O every time?  Getting VPD data isn't a time
> James> critical operation is it?  so what's the benefit of caching it?
> 
> XCopy. Want us to do a VPD page query for every command (~32 megs of
> data)?

Really?  OK, caching it is.

James

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

[hch]

On Mon, Mar 10, 2014 at 09:52:03PM +0100, Hannes Reinecke wrote:
> Yes, I thought of that, too.
> I thought to remember that binary attributes are reserved for
> firmware/hardware-dependent interfaces.

That's what we're dealing with here, aren't we?

> What should happen with the first patch in the series, then?
> When moving to a binary attribute the first patch isn't required
> anymore; should I drop it or send as a separate patch?

To me it seems useful even without the rest.

[PATCH 4/5] 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 f753373..de31a82 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 5d5e55a..fd7956e 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -789,6 +789,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,
@@ -946,6 +947,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;
 }
 
@@ -971,6 +976,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

[PATCH 5/5] 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

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

[James Bottomley]

On Mon, 2014-03-10 at 15:28 +0100, Hannes Reinecke wrote:
> The scsi_device now has VPD page83 information attached, so
> there is no need to query it again.

This is a bit new for a v8 patch, have you actually tested it?  I'm
asking because it looks to me like the vpd attach is done just before
the configure whereas the bindings are usually done in add.  I suspect
this will work for ses because it's a device model binding rather than a
transport class binding and the former occurs later, but knowing it
actually all works would be useful.

James


> 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,

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

[Hannes Reinecke]

On 03/10/2014 07:20 PM, James Bottomley wrote:
> On Mon, 2014-03-10 at 15:28 +0100, Hannes Reinecke wrote:
>> The scsi_device now has VPD page83 information attached, so
>> there is no need to query it again.
>
> This is a bit new for a v8 patch, have you actually tested it?  I'm
> asking because it looks to me like the vpd attach is done just before
> the configure whereas the bindings are usually done in add.  I suspect
> this will work for ses because it's a device model binding rather than a
> transport class binding and the former occurs later, but knowing it
> actually all works would be useful.
>
Yes, correct. But the device_add() call is done in
scsi_sysfs_add_sdev(), which is done at the end of scsi_add_lun().
So that will be okay.

But nevertheless I'll be cross-checking.

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