Re: [PATCH] scsi: Add VPD helper

[Boaz Harrosh <bharrosh@panasas.com>]

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