[PATCH] scsi: Add VPD helper

[PATCH] scsi: Add VPD helper

[Matthew Wilcox]

Based on prior work by Martin Petersen and James Bottomley, this patch
adds a generic helper for retrieving VPD pages from SCSI devices.

Signed-off-by: Matthew Wilcox <willy@linux.intel.com>
---
 drivers/scsi/scsi.c        |  104 ++++++++++++++++++++++++++++++++++++++++++++
 include/scsi/scsi_device.h |    1 +
 2 files changed, 105 insertions(+), 0 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index f8b79d4..46b7942 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -971,6 +971,110 @@ int scsi_track_queue_full(struct scsi_device *sdev, int depth)
 EXPORT_SYMBOL(scsi_track_queue_full);
 
 /**
+ * scsi_vpd_inquiry - Request a device provide us with a VPD page
+ * @sdev: The device to ask
+ * @buffer: Where to put the result
+ * @page: Which Vital Product Data to return
+ * @len: The length of the buffer
+ *
+ * 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.
+ */
+static int scsi_vpd_inquiry(struct scsi_device *sdev, unsigned char *buffer,
+							u8 page, unsigned len)
+{
+	int result;
+	unsigned char cmd[16];
+
+	cmd[0] = INQUIRY;
+	cmd[1] = 1;		/* EVPD */
+	cmd[2] = page;
+	cmd[3] = len >> 8;
+	cmd[4] = len & 0xff;
+	cmd[5] = 0;		/* Control byte */
+
+	/*
+	 * I'm not convinced we need to try quite this hard to get VPD, but
+	 * all the existing users tried this hard.
+	 */
+	result = scsi_execute_req(sdev, cmd, DMA_FROM_DEVICE, buffer,
+				  len + 4, NULL, 30 * HZ, 3, NULL);
+	if (result)
+		return result;
+
+	/* Sanity check that we got the page back that we asked for */
+	if (buffer[1] != page)
+		return -EIO;
+
+	return 0;
+}
+
+/**
+ * scsi_get_vpd_page - Get Vital Product Data from a SCSI device
+ * @sdev: The device to ask
+ * @page: Which Vital Product Data to return
+ *
+ * SCSI devices may optionally supply Vital Product Data.  Each 'page'
+ * of VPD is defined in the appropriate SCSI document (eg SPC, SBC).
+ * If the device supports this VPD page, this routine returns a pointer
+ * to a buffer containing the data from that page.  The caller is
+ * responsible for calling kfree() on this pointer when it is no longer
+ * needed.  If we cannot retrieve the VPD page this routine returns %NULL.
+ */
+unsigned char *scsi_get_vpd_page(struct scsi_device *sdev, u8 page)
+{
+	int i, result;
+	unsigned int len;
+	unsigned char *buf = kmalloc(259, GFP_KERNEL);
+
+	if (!buf)
+		return NULL;
+
+	/* Ask for all the pages supported by this device */ 
+	result = scsi_vpd_inquiry(sdev, buf, 0, 255);
+	if (result)
+		goto fail;
+
+	/* If the user actually wanted this page, we can skip the rest */
+	if (page == 0)
+		return buf;
+
+	for (i = 0; i < buf[3]; i++)
+		if (buf[i + 4] == page)
+			goto found;
+	/* The device claims it doesn't support the requested page */
+	goto fail;
+
+ found:
+	result = scsi_vpd_inquiry(sdev, buf, page, 255);
+	if (result)
+		goto fail;
+
+	/*
+	 * Some pages are longer than 255 bytes.  The actual length of
+	 * the page is returned in the header.
+	 */
+	len = (buf[2] << 8) | buf[3];
+	if (len <= 255)
+		return buf;
+
+	kfree(buf);
+	buf = kmalloc(len + 4, GFP_KERNEL);
+	result = scsi_vpd_inquiry(sdev, buf, page, len);
+	if (result)
+		goto fail;
+
+	return buf;
+
+ fail:
+	kfree(buf);
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(scsi_get_vpd_page);
+
+/**
  * scsi_device_get  -  get an additional reference to a scsi_device
  * @sdev:	device to get a reference to
  *
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 01a4c58..9576690 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -340,6 +340,7 @@ extern int scsi_mode_select(struct scsi_device *sdev, int pf, int sp,
 			    struct scsi_sense_hdr *);
 extern int scsi_test_unit_ready(struct scsi_device *sdev, int timeout,
 				int retries, struct scsi_sense_hdr *sshdr);
+extern unsigned char *scsi_get_vpd_page(struct scsi_device *, u8 page);
 extern int scsi_device_set_state(struct scsi_device *sdev,
 				 enum scsi_device_state state);
 extern struct scsi_event *sdev_evt_alloc(enum scsi_device_event evt_type,
-- 
1.5.6.5

[PATCH] ses: Use new scsi VPD helper

[Matthew Wilcox]

SES had its own code to retrieve VPD from devices; convert it to use the
new scsi_get_vpd_page helper.

Signed-off-by: Matthew Wilcox <willy@linux.intel.com>
---
 drivers/scsi/ses.c |   29 +++--------------------------
 1 files changed, 3 insertions(+), 26 deletions(-)

diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
index 7f0df29..95e48ad 100644
--- a/drivers/scsi/ses.c
+++ b/drivers/scsi/ses.c
@@ -345,44 +345,21 @@ static int ses_enclosure_find_by_addr(struct enclosure_device *edev,
 	return 0;
 }
 
-#define VPD_INQUIRY_SIZE 36
-
 static void ses_match_to_enclosure(struct enclosure_device *edev,
 				   struct scsi_device *sdev)
 {
-	unsigned char *buf = kmalloc(VPD_INQUIRY_SIZE, GFP_KERNEL);
+	unsigned char *buf;
 	unsigned char *desc;
 	u16 vpd_len;
 	struct efd efd = {
 		.addr = 0,
 	};
-	unsigned char cmd[] = {
-		INQUIRY,
-		1,
-		0x83,
-		VPD_INQUIRY_SIZE >> 8,
-		VPD_INQUIRY_SIZE & 0xff,
-		0
-	};
 
+	buf = scsi_get_vpd_page(sdev, 0x83);
 	if (!buf)
 		return;
 
-	if (scsi_execute_req(sdev, cmd, DMA_FROM_DEVICE, buf,
-			     VPD_INQUIRY_SIZE, NULL, SES_TIMEOUT, SES_RETRIES,
-			     NULL))
-		goto free;
-
-	vpd_len = (buf[2] << 8) + buf[3];
-	kfree(buf);
-	buf = kmalloc(vpd_len, GFP_KERNEL);
-	if (!buf)
-		return;
-	cmd[3] = vpd_len >> 8;
-	cmd[4] = vpd_len & 0xff;
-	if (scsi_execute_req(sdev, cmd, DMA_FROM_DEVICE, buf,
-			     vpd_len, NULL, SES_TIMEOUT, SES_RETRIES, NULL))
-		goto free;
+	vpd_len = (buf[2] << 8) | buf[3];
 
 	desc = buf + 4;
 	while (desc < buf + vpd_len) {
-- 
1.5.6.5

Re: [PATCH] ses: Use new scsi VPD helper

[James Bottomley]

On Wed, 2008-12-31 at 13:12 -0500, Matthew Wilcox wrote:
> SES had its own code to retrieve VPD from devices; convert it to use the
> new scsi_get_vpd_page helper.
> 
> Signed-off-by: Matthew Wilcox <willy@linux.intel.com>
> ---
>  drivers/scsi/ses.c |   29 +++--------------------------
>  1 files changed, 3 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
> index 7f0df29..95e48ad 100644
> --- a/drivers/scsi/ses.c
> +++ b/drivers/scsi/ses.c
> @@ -345,44 +345,21 @@ static int ses_enclosure_find_by_addr(struct enclosure_device *edev,
>  	return 0;
>  }
>  
> -#define VPD_INQUIRY_SIZE 36
> -
>  static void ses_match_to_enclosure(struct enclosure_device *edev,
>  				   struct scsi_device *sdev)
>  {
> -	unsigned char *buf = kmalloc(VPD_INQUIRY_SIZE, GFP_KERNEL);
> +	unsigned char *buf;
>  	unsigned char *desc;
>  	u16 vpd_len;
>  	struct efd efd = {
>  		.addr = 0,
>  	};
> -	unsigned char cmd[] = {
> -		INQUIRY,
> -		1,
> -		0x83,
> -		VPD_INQUIRY_SIZE >> 8,
> -		VPD_INQUIRY_SIZE & 0xff,
> -		0
> -	};
>  
> +	buf = scsi_get_vpd_page(sdev, 0x83);
>  	if (!buf)
>  		return;
>  
> -	if (scsi_execute_req(sdev, cmd, DMA_FROM_DEVICE, buf,
> -			     VPD_INQUIRY_SIZE, NULL, SES_TIMEOUT, SES_RETRIES,
> -			     NULL))
> -		goto free;
> -
> -	vpd_len = (buf[2] << 8) + buf[3];
> -	kfree(buf);
> -	buf = kmalloc(vpd_len, GFP_KERNEL);
> -	if (!buf)
> -		return;
> -	cmd[3] = vpd_len >> 8;
> -	cmd[4] = vpd_len & 0xff;
> -	if (scsi_execute_req(sdev, cmd, DMA_FROM_DEVICE, buf,
> -			     vpd_len, NULL, SES_TIMEOUT, SES_RETRIES, NULL))
> -		goto free;
> +	vpd_len = (buf[2] << 8) | buf[3];

This was actually wrong in the original code.  It should be

pd_len = (buf[2] << 8) | buf[3] + 4;

to account for the header offset in the returned data length.

James

Re: [PATCH] ses: Use new scsi VPD helper

[Matthew Wilcox]

On Wed, Dec 31, 2008 at 12:59:38PM -0600, James Bottomley wrote:
> This was actually wrong in the original code.  It should be
> 
> pd_len = (buf[2] << 8) | buf[3] + 4;
> 
> to account for the header offset in the returned data length.

Ah yes.  I got it right in the scsi_get_vpd_page() code, and then forgot
about it while changing the ses code.  Thanks for spotting that.

vpd_len also needs to be unsigned int, as a u16 could wrap.  Here's a
replacement patch:

diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
index 7f0df29..95d16b3 100644
--- a/drivers/scsi/ses.c
+++ b/drivers/scsi/ses.c
@@ -345,44 +345,21 @@ static int ses_enclosure_find_by_addr(struct enclosure_device *edev,
 	return 0;
 }
 
-#define VPD_INQUIRY_SIZE 36
-
 static void ses_match_to_enclosure(struct enclosure_device *edev,
 				   struct scsi_device *sdev)
 {
-	unsigned char *buf = kmalloc(VPD_INQUIRY_SIZE, GFP_KERNEL);
+	unsigned char *buf;
 	unsigned char *desc;
-	u16 vpd_len;
+	unsigned int vpd_len;
 	struct efd efd = {
 		.addr = 0,
 	};
-	unsigned char cmd[] = {
-		INQUIRY,
-		1,
-		0x83,
-		VPD_INQUIRY_SIZE >> 8,
-		VPD_INQUIRY_SIZE & 0xff,
-		0
-	};
 
+	buf = scsi_get_vpd_page(sdev, 0x83);
 	if (!buf)
 		return;
 
-	if (scsi_execute_req(sdev, cmd, DMA_FROM_DEVICE, buf,
-			     VPD_INQUIRY_SIZE, NULL, SES_TIMEOUT, SES_RETRIES,
-			     NULL))
-		goto free;
-
-	vpd_len = (buf[2] << 8) + buf[3];
-	kfree(buf);
-	buf = kmalloc(vpd_len, GFP_KERNEL);
-	if (!buf)
-		return;
-	cmd[3] = vpd_len >> 8;
-	cmd[4] = vpd_len & 0xff;
-	if (scsi_execute_req(sdev, cmd, DMA_FROM_DEVICE, buf,
-			     vpd_len, NULL, SES_TIMEOUT, SES_RETRIES, NULL))
-		goto free;
+	vpd_len = ((buf[2] << 8) | buf[3]) + 4;
 
 	desc = buf + 4;
 	while (desc < buf + vpd_len) {

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH] scsi: Add VPD helper

[Boaz Harrosh]

Matthew Wilcox wrote:
> Based on prior work by Martin Petersen and James Bottomley, this patch
> adds a generic helper for retrieving VPD pages from SCSI devices.
> 
> Signed-off-by: Matthew Wilcox <willy@linux.intel.com>
> ---
>  drivers/scsi/scsi.c        |  104 ++++++++++++++++++++++++++++++++++++++++++++
>  include/scsi/scsi_device.h |    1 +
>  2 files changed, 105 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> index f8b79d4..46b7942 100644
> --- a/drivers/scsi/scsi.c
> +++ b/drivers/scsi/scsi.c
> @@ -971,6 +971,110 @@ int scsi_track_queue_full(struct scsi_device *sdev, int depth)
>  EXPORT_SYMBOL(scsi_track_queue_full);
>  
>  /**
> + * scsi_vpd_inquiry - Request a device provide us with a VPD page
> + * @sdev: The device to ask
> + * @buffer: Where to put the result
> + * @page: Which Vital Product Data to return
> + * @len: The length of the buffer
> + *
> + * 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.
> + */
> +static int scsi_vpd_inquiry(struct scsi_device *sdev, unsigned char *buffer,
> +							u8 page, unsigned len)
> +{
> +	int result;
> +	unsigned char cmd[16];
> +
> +	cmd[0] = INQUIRY;
> +	cmd[1] = 1;		/* EVPD */
> +	cmd[2] = page;
> +	cmd[3] = len >> 8;
> +	cmd[4] = len & 0xff;
> +	cmd[5] = 0;		/* Control byte */
> +
+	buffer[1] = -1;
see below
> +	/*
> +	 * I'm not convinced we need to try quite this hard to get VPD, but
> +	 * all the existing users tried this hard.
> +	 */
> +	result = scsi_execute_req(sdev, cmd, DMA_FROM_DEVICE, buffer,
> +				  len + 4, NULL, 30 * HZ, 3, NULL);
> +	if (result)
> +		return result;
> +
> +	/* Sanity check that we got the page back that we asked for */
> +	if (buffer[1] != page)
> +		return -EIO;

Maybe it's just me but, if you are going to if() on a buffer byte, then you need
to make sure its initial value. Or at least document the need for a -1 at
buffer[1]

> +
> +	return 0;
> +}
> +
> +/**
> + * scsi_get_vpd_page - Get Vital Product Data from a SCSI device
> + * @sdev: The device to ask
> + * @page: Which Vital Product Data to return
> + *
> + * SCSI devices may optionally supply Vital Product Data.  Each 'page'
> + * of VPD is defined in the appropriate SCSI document (eg SPC, SBC).
> + * If the device supports this VPD page, this routine returns a pointer
> + * to a buffer containing the data from that page.  The caller is
> + * responsible for calling kfree() on this pointer when it is no longer
> + * needed.  If we cannot retrieve the VPD page this routine returns %NULL.
> + */
> +unsigned char *scsi_get_vpd_page(struct scsi_device *sdev, u8 page)
> +{
> +	int i, result;
> +	unsigned int len;
> +	unsigned char *buf = kmalloc(259, GFP_KERNEL);
> +
> +	if (!buf)
> +		return NULL;
> +
> +	/* Ask for all the pages supported by this device */ 
> +	result = scsi_vpd_inquiry(sdev, buf, 0, 255);
> +	if (result)
> +		goto fail;
> +
> +	/* If the user actually wanted this page, we can skip the rest */
> +	if (page == 0)
> +		return buf;
> +
> +	for (i = 0; i < buf[3]; i++)
> +		if (buf[i + 4] == page)
> +			goto found;
> +	/* The device claims it doesn't support the requested page */
> +	goto fail;
> +
> + found:
> +	result = scsi_vpd_inquiry(sdev, buf, page, 255);
> +	if (result)
> +		goto fail;
> +
> +	/*
> +	 * Some pages are longer than 255 bytes.  The actual length of
> +	 * the page is returned in the header.
> +	 */
> +	len = (buf[2] << 8) | buf[3];
> +	if (len <= 255)
> +		return buf;
> +
> +	kfree(buf);
> +	buf = kmalloc(len + 4, GFP_KERNEL);
> +	result = scsi_vpd_inquiry(sdev, buf, page, len);
> +	if (result)
> +		goto fail;
> +
> +	return buf;
> +
> + fail:
> +	kfree(buf);
> +	return NULL;
> +}
> +EXPORT_SYMBOL_GPL(scsi_get_vpd_page);
> +
> +/**
>   * scsi_device_get  -  get an additional reference to a scsi_device
>   * @sdev:	device to get a reference to
>   *
> diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
> index 01a4c58..9576690 100644
> --- a/include/scsi/scsi_device.h
> +++ b/include/scsi/scsi_device.h
> @@ -340,6 +340,7 @@ extern int scsi_mode_select(struct scsi_device *sdev, int pf, int sp,
>  			    struct scsi_sense_hdr *);
>  extern int scsi_test_unit_ready(struct scsi_device *sdev, int timeout,
>  				int retries, struct scsi_sense_hdr *sshdr);
> +extern unsigned char *scsi_get_vpd_page(struct scsi_device *, u8 page);
>  extern int scsi_device_set_state(struct scsi_device *sdev,
>  				 enum scsi_device_state state);
>  extern struct scsi_event *sdev_evt_alloc(enum scsi_device_event evt_type,

Boaz

Re: [PATCH] scsi: Add VPD helper

[Matthew Wilcox]

On Thu, Jan 01, 2009 at 11:19:33AM +0200, Boaz Harrosh wrote:
> +	buffer[1] = -1;
> see below
> > +	/*
> > +	 * I'm not convinced we need to try quite this hard to get VPD, but
> > +	 * all the existing users tried this hard.
> > +	 */
> > +	result = scsi_execute_req(sdev, cmd, DMA_FROM_DEVICE, buffer,
> > +				  len + 4, NULL, 30 * HZ, 3, NULL);
> > +	if (result)
> > +		return result;
> > +
> > +	/* Sanity check that we got the page back that we asked for */
> > +	if (buffer[1] != page)
> > +		return -EIO;
> 
> Maybe it's just me but, if you are going to if() on a buffer byte, then you need
> to make sure its initial value. Or at least document the need for a -1 at
> buffer[1]

buffer[] is an array of unsigned char, so -1 is 255.  We could be asking
for page 0xff, so your initialisation doesn't help reject bogus data.

Your concern here, presumably, is that the device might transfer less than
two bytes, and not return an error.  I think this is quite unlikely --
and in any case, such a misbehaving device is really no different from a
device which transfers complete garbage.

I suppose we could pass a 'resid' to scsi_execute_req and check that at
least four bytes was transferred.

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH] scsi: Add VPD helper

[James Bottomley]

On Wed, 2008-12-31 at 13:12 -0500, Matthew Wilcox wrote:
> Based on prior work by Martin Petersen and James Bottomley, this patch
> adds a generic helper for retrieving VPD pages from SCSI devices.
> 
> Signed-off-by: Matthew Wilcox <willy@linux.intel.com>

Could you run checkpatch on these before sending, please:

ERROR: trailing whitespace
#131: FILE: drivers/scsi/scsi.c:1035:
+^I/* Ask for all the pages supported by this device */ $

I've fixed it up manually this time.

James