[PATCH 1/2] scsi: Do not attach VPD to devices that don't support it

[PATCH 1/2] scsi: Do not attach VPD to devices that don't support it

[Alexander Duyck]

The patch "scsi: rescan VPD attributes" introduced a regression in which
devices that don't support VPD were being scanned for VPD attributes
anyway.  This could cause issues for this parts and should be avoided so
the check for scsi_level has been moved out of scsi_add_lun and into
scsi_attach_vpd so that all callers will not scan VPD for devices that
don't support it.

Fixes: 09e2b0b14690 ("scsi: rescan VPD attributes")
Signed-off-by: Alexander Duyck <aduyck@mirantis.com>
---
 drivers/scsi/scsi.c      |    3 +++
 drivers/scsi/scsi_scan.c |    3 +--
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index b1bf42b93fcc..ed085e78c893 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -784,6 +784,9 @@ void scsi_attach_vpd(struct scsi_device *sdev)
 	int pg83_supported = 0;
 	unsigned char __rcu *vpd_buf, *orig_vpd_buf = NULL;
 
+	if (sdev->scsi_level < SCSI_3)
+		return;
+
 	if (sdev->skip_vpd_pages)
 		return;
 retry_pg0:
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 6a820668d442..1b16c89e0cf9 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -986,8 +986,7 @@ static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result,
 		}
 	}
 
-	if (sdev->scsi_level >= SCSI_3)
-		scsi_attach_vpd(sdev);
+	scsi_attach_vpd(sdev);
 
 	sdev->max_queue_depth = sdev->queue_depth;
 

Re: [PATCH 1/2] scsi: Do not attach VPD to devices that don't support it

[Hannes Reinecke]

[-- Attachment #1: Type: text/plain, Size: 2011 bytes --]

On 01/21/2016 07:35 AM, Alexander Duyck wrote:
> The patch "scsi: rescan VPD attributes" introduced a regression in which
> devices that don't support VPD were being scanned for VPD attributes
> anyway.  This could cause issues for this parts and should be avoided so
> the check for scsi_level has been moved out of scsi_add_lun and into
> scsi_attach_vpd so that all callers will not scan VPD for devices that
> don't support it.
> 
> Fixes: 09e2b0b14690 ("scsi: rescan VPD attributes")
> Signed-off-by: Alexander Duyck <aduyck@mirantis.com>
> ---
>  drivers/scsi/scsi.c      |    3 +++
>  drivers/scsi/scsi_scan.c |    3 +--
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> index b1bf42b93fcc..ed085e78c893 100644
> --- a/drivers/scsi/scsi.c
> +++ b/drivers/scsi/scsi.c
> @@ -784,6 +784,9 @@ void scsi_attach_vpd(struct scsi_device *sdev)
>  	int pg83_supported = 0;
>  	unsigned char __rcu *vpd_buf, *orig_vpd_buf = NULL;
>  
> +	if (sdev->scsi_level < SCSI_3)
> +		return;
> +
>  	if (sdev->skip_vpd_pages)
>  		return;
>  retry_pg0:
> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> index 6a820668d442..1b16c89e0cf9 100644
> --- a/drivers/scsi/scsi_scan.c
> +++ b/drivers/scsi/scsi_scan.c
> @@ -986,8 +986,7 @@ static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result,
>  		}
>  	}
>  
> -	if (sdev->scsi_level >= SCSI_3)
> -		scsi_attach_vpd(sdev);
> +	scsi_attach_vpd(sdev);
>  
>  	sdev->max_queue_depth = sdev->queue_depth;
>  
> 
Isn't this slightly pointless, given that we're testing the inverse
condition in scsi_attach_vpd()?

And in anycase, I guess we should be using the same logic sd.c is
using. Please see the attached patch.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-scsi-Do-not-attach-VPD-to-devices-that-don-t-support.patch --]
[-- Type: text/x-patch; name="0001-scsi-Do-not-attach-VPD-to-devices-that-don-t-support.patch", Size: 3906 bytes --]

From bc662c5a0255e868746ef317e2eff04dc3fcfac5 Mon Sep 17 00:00:00 2001
From: Hannes Reinecke <hare@suse.de>
Date: Thu, 21 Jan 2016 08:18:49 +0100
Subject: [PATCH] scsi: Do not attach VPD to devices that don't support it

The patch "scsi: rescan VPD attributes" introduced a regression in which
devices that don't support VPD were being scanned for VPD attributes
anyway.  This could cause issues for this parts and should be avoided so
the check for scsi_level has been moved out of scsi_add_lun and into
scsi_attach_vpd so that all callers will not scan VPD for devices that
don't support it.

Fixes: 09e2b0b14690 ("scsi: rescan VPD attributes")

Suggested-by: Alexander Duyck <aduyck@mirantis.com>
Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/scsi.c        |  3 ++-
 drivers/scsi/sd.c          | 19 +------------------
 include/scsi/scsi_device.h | 25 +++++++++++++++++++++++++
 3 files changed, 28 insertions(+), 19 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index b1bf42b..1deb6ad 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -784,8 +784,9 @@ void scsi_attach_vpd(struct scsi_device *sdev)
 	int pg83_supported = 0;
 	unsigned char __rcu *vpd_buf, *orig_vpd_buf = NULL;
 
-	if (sdev->skip_vpd_pages)
+	if (!scsi_device_supports_vpd(sdev))
 		return;
+
 retry_pg0:
 	vpd_buf = kmalloc(vpd_len, GFP_KERNEL);
 	if (!vpd_buf)
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 5451980..868d58c 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2789,23 +2789,6 @@ static void sd_read_write_same(struct scsi_disk *sdkp, unsigned char *buffer)
 		sdkp->ws10 = 1;
 }
 
-static int sd_try_extended_inquiry(struct scsi_device *sdp)
-{
-	/* Attempt VPD inquiry if the device blacklist explicitly calls
-	 * for it.
-	 */
-	if (sdp->try_vpd_pages)
-		return 1;
-	/*
-	 * Although VPD inquiries can go to SCSI-2 type devices,
-	 * some USB ones crash on receiving them, and the pages
-	 * we currently ask for are for SPC-3 and beyond
-	 */
-	if (sdp->scsi_level > SCSI_SPC_2 && !sdp->skip_vpd_pages)
-		return 1;
-	return 0;
-}
-
 /**
  *	sd_revalidate_disk - called the first time a new disk is seen,
  *	performs disk spin up, read_capacity, etc.
@@ -2844,7 +2827,7 @@ static int sd_revalidate_disk(struct gendisk *disk)
 	if (sdkp->media_present) {
 		sd_read_capacity(sdkp, buffer);
 
-		if (sd_try_extended_inquiry(sdp)) {
+		if (scsi_device_supports_vpd(sdp)) {
 			sd_read_block_provisioning(sdkp);
 			sd_read_block_limits(sdkp);
 			sd_read_block_characteristics(sdkp);
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index a5fc682..d9aea6c 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -515,6 +515,31 @@ static inline int scsi_device_tpgs(struct scsi_device *sdev)
 	return sdev->inquiry ? (sdev->inquiry[5] >> 4) & 0x3 : 0;
 }
 
+/**
+ * scsi_device_supports_vpd - test if a device supports VPD pages
+ * @sdev: the &struct scsi_device to test
+ *
+ * If the 'try_vpd_pages' flag is set it takes precedence.
+ * Otherwise we will assume VPD pages are supported if the
+ * SCSI level is at least SPC-3 and 'skip_vpd_pages' is not set.
+ */
+static inline int scsi_device_supports_vpd(struct scsi_device *sdev)
+{
+	/* Attempt VPD inquiry if the device blacklist explicitly calls
+	 * for it.
+	 */
+	if (sdev->try_vpd_pages)
+		return 1;
+	/*
+	 * Although VPD inquiries can go to SCSI-2 type devices,
+	 * some USB ones crash on receiving them, and the pages
+	 * we currently ask for are for SPC-3 and beyond
+	 */
+	if (sdev->scsi_level > SCSI_SPC_2 && !sdev->skip_vpd_pages)
+		return 1;
+	return 0;
+}
+
 #define MODULE_ALIAS_SCSI_DEVICE(type) \
 	MODULE_ALIAS("scsi:t-" __stringify(type) "*")
 #define SCSI_DEVICE_MODALIAS_FMT "scsi:t-0x%02x"
-- 
1.8.5.6

Re: [PATCH 1/2] scsi: Do not attach VPD to devices that don't support it

[Alexander Duyck]

On Wed, Jan 20, 2016 at 11:37 PM, Hannes Reinecke <hare@suse.de> wrote:
> On 01/21/2016 07:35 AM, Alexander Duyck wrote:
>> The patch "scsi: rescan VPD attributes" introduced a regression in which
>> devices that don't support VPD were being scanned for VPD attributes
>> anyway.  This could cause issues for this parts and should be avoided so
>> the check for scsi_level has been moved out of scsi_add_lun and into
>> scsi_attach_vpd so that all callers will not scan VPD for devices that
>> don't support it.
>>
>> Fixes: 09e2b0b14690 ("scsi: rescan VPD attributes")
>> Signed-off-by: Alexander Duyck <aduyck@mirantis.com>
>> ---
>>  drivers/scsi/scsi.c      |    3 +++
>>  drivers/scsi/scsi_scan.c |    3 +--
>>  2 files changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
>> index b1bf42b93fcc..ed085e78c893 100644
>> --- a/drivers/scsi/scsi.c
>> +++ b/drivers/scsi/scsi.c
>> @@ -784,6 +784,9 @@ void scsi_attach_vpd(struct scsi_device *sdev)
>>       int pg83_supported = 0;
>>       unsigned char __rcu *vpd_buf, *orig_vpd_buf = NULL;
>>
>> +     if (sdev->scsi_level < SCSI_3)
>> +             return;
>> +
>>       if (sdev->skip_vpd_pages)
>>               return;
>>  retry_pg0:
>> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
>> index 6a820668d442..1b16c89e0cf9 100644
>> --- a/drivers/scsi/scsi_scan.c
>> +++ b/drivers/scsi/scsi_scan.c
>> @@ -986,8 +986,7 @@ static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result,
>>               }
>>       }
>>
>> -     if (sdev->scsi_level >= SCSI_3)
>> -             scsi_attach_vpd(sdev);
>> +     scsi_attach_vpd(sdev);
>>
>>       sdev->max_queue_depth = sdev->queue_depth;
>>
>>
> Isn't this slightly pointless, given that we're testing the inverse
> condition in scsi_attach_vpd()?

I'm not sure what you are getting at.  What I basically did is move
the check here into the function.  No point in checking it in 2 spots
when checking it inside the function is good enough.

> And in anycase, I guess we should be using the same logic sd.c is
> using. Please see the attached patch.

The attached patch looks good as it also takes care of the opt-in case
which I had overlooked.  The only bit missing is the fact that we are
still checking scsi_level twice when we don't need to.

- Alex

Re: [PATCH 1/2] scsi: Do not attach VPD to devices that don't support it

[Martin K. Petersen]

>>>>> "Hannes" == Hannes Reinecke <hare@suse.de> writes:

Hannes> And in anycase, I guess we should be using the same logic sd.c
Hannes> is using. Please see the attached patch.

I'm OK with this change but please send it as a proper patch submission
so somebody else can review it.

-- 
Martin K. Petersen	Oracle Linux Engineering

[PATCH 0/2] scsi: vpd sanity checks

[Hannes Reinecke]

Hi all,

here's a combined patchset to clear up the sanity checks for VPD page
support. The first patch streamlines the function to check for VPD page
support, and the second one lowers the minimal SCSI version for VPD
page support to SPC-2, to avoid having to whitelist a lot of devices.

As usual, comments and reviews are welcome.

Hannes Reinecke (2):
  scsi: Do not attach VPD to devices that don't support it
  scsi: vpd pages are mandatory for SPC-2

 drivers/scsi/scsi.c        |  3 ++-
 drivers/scsi/sd.c          | 19 +------------------
 include/scsi/scsi_device.h | 25 +++++++++++++++++++++++++
 3 files changed, 28 insertions(+), 19 deletions(-)

-- 
2.6.2

[PATCH 1/2] scsi: Do not attach VPD to devices that don't support it

[Hannes Reinecke]

The patch "scsi: rescan VPD attributes" introduced a regression in which
devices that don't support VPD were being scanned for VPD attributes
anyway.  This could cause issues for this parts and should be avoided so
the check for scsi_level has been moved out of scsi_add_lun and into
scsi_attach_vpd so that all callers will not scan VPD for devices that
don't support it.

Fixes: 09e2b0b14690 ("scsi: rescan VPD attributes")

Suggested-by: Alexander Duyck <aduyck@mirantis.com>
Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/scsi.c        |  3 ++-
 drivers/scsi/sd.c          | 19 +------------------
 include/scsi/scsi_device.h | 25 +++++++++++++++++++++++++
 3 files changed, 28 insertions(+), 19 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index b1bf42b..1deb6ad 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -784,8 +784,9 @@ void scsi_attach_vpd(struct scsi_device *sdev)
 	int pg83_supported = 0;
 	unsigned char __rcu *vpd_buf, *orig_vpd_buf = NULL;
 
-	if (sdev->skip_vpd_pages)
+	if (!scsi_device_supports_vpd(sdev))
 		return;
+
 retry_pg0:
 	vpd_buf = kmalloc(vpd_len, GFP_KERNEL);
 	if (!vpd_buf)
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 5a5457a..70042f2 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2795,23 +2795,6 @@ static void sd_read_write_same(struct scsi_disk *sdkp, unsigned char *buffer)
 		sdkp->ws10 = 1;
 }
 
-static int sd_try_extended_inquiry(struct scsi_device *sdp)
-{
-	/* Attempt VPD inquiry if the device blacklist explicitly calls
-	 * for it.
-	 */
-	if (sdp->try_vpd_pages)
-		return 1;
-	/*
-	 * Although VPD inquiries can go to SCSI-2 type devices,
-	 * some USB ones crash on receiving them, and the pages
-	 * we currently ask for are for SPC-3 and beyond
-	 */
-	if (sdp->scsi_level > SCSI_SPC_2 && !sdp->skip_vpd_pages)
-		return 1;
-	return 0;
-}
-
 static inline u32 logical_to_sectors(struct scsi_device *sdev, u32 blocks)
 {
 	return blocks << (ilog2(sdev->sector_size) - 9);
@@ -2856,7 +2839,7 @@ static int sd_revalidate_disk(struct gendisk *disk)
 	if (sdkp->media_present) {
 		sd_read_capacity(sdkp, buffer);
 
-		if (sd_try_extended_inquiry(sdp)) {
+		if (scsi_device_supports_vpd(sdp)) {
 			sd_read_block_provisioning(sdkp);
 			sd_read_block_limits(sdkp);
 			sd_read_block_characteristics(sdkp);
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 61341d3..1d4a329 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -523,6 +523,31 @@ static inline int scsi_device_tpgs(struct scsi_device *sdev)
 	return sdev->inquiry ? (sdev->inquiry[5] >> 4) & 0x3 : 0;
 }
 
+/**
+ * scsi_device_supports_vpd - test if a device supports VPD pages
+ * @sdev: the &struct scsi_device to test
+ *
+ * If the 'try_vpd_pages' flag is set it takes precedence.
+ * Otherwise we will assume VPD pages are supported if the
+ * SCSI level is at least SPC-3 and 'skip_vpd_pages' is not set.
+ */
+static inline int scsi_device_supports_vpd(struct scsi_device *sdev)
+{
+	/* Attempt VPD inquiry if the device blacklist explicitly calls
+	 * for it.
+	 */
+	if (sdev->try_vpd_pages)
+		return 1;
+	/*
+	 * Although VPD inquiries can go to SCSI-2 type devices,
+	 * some USB ones crash on receiving them, and the pages
+	 * we currently ask for are for SPC-3 and beyond
+	 */
+	if (sdev->scsi_level > SCSI_SPC_2 && !sdev->skip_vpd_pages)
+		return 1;
+	return 0;
+}
+
 #define MODULE_ALIAS_SCSI_DEVICE(type) \
 	MODULE_ALIAS("scsi:t-" __stringify(type) "*")
 #define SCSI_DEVICE_MODALIAS_FMT "scsi:t-0x%02x"
-- 
2.6.2

[PATCH 2/2] scsi: vpd pages are mandatory for SPC-2

[Hannes Reinecke]

VPD pages 0x0 and 0x83 are mandatory even for SPC-2, so we should
be lowering the restriction to avoid having to whitelist every
SPC-2 compliant device.

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 include/scsi/scsi_device.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 1d4a329..40a789f 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -541,9 +541,9 @@ static inline int scsi_device_supports_vpd(struct scsi_device *sdev)
 	/*
 	 * Although VPD inquiries can go to SCSI-2 type devices,
 	 * some USB ones crash on receiving them, and the pages
-	 * we currently ask for are for SPC-3 and beyond
+	 * we currently ask for are mandatory for SPC-2 and beyond
 	 */
-	if (sdev->scsi_level > SCSI_SPC_2 && !sdev->skip_vpd_pages)
+	if (sdev->scsi_level >= SCSI_SPC_2 && !sdev->skip_vpd_pages)
 		return 1;
 	return 0;
 }
-- 
2.6.2

Re: [PATCH 1/2] scsi: Do not attach VPD to devices that don't support it

[Johannes Thumshirn]

On 2016-04-01 08:57, Hannes Reinecke wrote:
> The patch "scsi: rescan VPD attributes" introduced a regression in 
> which
> devices that don't support VPD were being scanned for VPD attributes
> anyway.  This could cause issues for this parts and should be avoided 
> so
> the check for scsi_level has been moved out of scsi_add_lun and into
> scsi_attach_vpd so that all callers will not scan VPD for devices that
> don't support it.
> 
> Fixes: 09e2b0b14690 ("scsi: rescan VPD attributes")
> 
> Suggested-by: Alexander Duyck <aduyck@mirantis.com>
> Signed-off-by: Hannes Reinecke <hare@suse.com>

Looks good,
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>

Re: [PATCH 2/2] scsi: vpd pages are mandatory for SPC-2

[Johannes Thumshirn]

On 2016-04-01 08:57, Hannes Reinecke wrote:
> VPD pages 0x0 and 0x83 are mandatory even for SPC-2, so we should
> be lowering the restriction to avoid having to whitelist every
> SPC-2 compliant device.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.com>

Looks good,
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>

Re: [PATCH 1/2] scsi: Do not attach VPD to devices that don't support it

[Martin K. Petersen]

>>>>> "Hannes" == Hannes Reinecke <hare@suse.de> writes:

Hannes> The patch "scsi: rescan VPD attributes" introduced a regression
Hannes> in which devices that don't support VPD were being scanned for
Hannes> VPD attributes anyway.  This could cause issues for this parts
Hannes> and should be avoided so the check for scsi_level has been moved
Hannes> out of scsi_add_lun and into scsi_attach_vpd so that all callers
Hannes> will not scan VPD for devices that don't support it.

Applied to 4.6/scsi-fixes, CC:stable 4.5+.

Thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH 2/2] scsi: vpd pages are mandatory for SPC-2

[Martin K. Petersen]

>>>>> "Hannes" == Hannes Reinecke <hare@suse.de> writes:

Hannes> VPD pages 0x0 and 0x83 are mandatory even for SPC-2, so we
Hannes> should be lowering the restriction to avoid having to whitelist
Hannes> every SPC-2 compliant device.

Linus took patch 1/2 for 4.6rc3 so I have rebased 4.7/scsi-queue and
applied patch 2/2.

-- 
Martin K. Petersen	Oracle Linux Engineering