[PATCH 0/2] scsi: Fix endless loop of ATA hard resets due to VPD reads

[PATCH 0/2] scsi: Fix endless loop of ATA hard resets due to VPD reads

[Alexander Duyck]

Recent changes to the kernel pulled in during the merge window have
resulted in my system generating an endless loop of the following type of
errors:

[  318.965756] ata14: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
[  318.968457] ata14.00: configured for UDMA/66
[  318.970656] ata14: EH complete
[  318.984366] ata14.00: exception Emask 0x0 SAct 0x0 SErr 0x0 action 0x6
[  318.986854] ata14.00: irq_stat 0x40000001
[  318.989138] ata14.00: cmd a0/01:00:00:00:01/00:00:00:00:00/a0 tag 22 dma 16640 in
         Inquiry 12 01 00 00 ff 00res 00/00:00:00:00:00/00:00:00:00:00/00 Emask 0x3 (HSM violation)
[  318.995986] ata14: hard resetting link

I bisected the issue and found the patch responsible for the issue was
commit 09e2b0b14690 "scsi: rescan VPD attributes".  This commit contained
several issues.

First, the commit had changed the behavior in terms of what devices we
called scsi_attach_vpd() for.  As a result we were calling it for devices
that didn't support a scsi_level of 6, SCSI 3, so VPD accesses could
result in errors.

Second, the commit as well as a follow-on patch for it contained a number
of RCU errors.  Specifically the code was structured such that we had
accesses outside of RCU locked regions, and repeated use of the RCU
protected pointer without using the proper accessors.  As such it was
possible to get into a serious corruption situation should a pointer be
updated.

Ultimately neither of these bugs were my root cause.  It turns out the
Marvel Console SCSI device in my system needed to have a flag set to
disable VPD access in order to keep things from looping through the error
repeatedly.  In order to resolve it I had to add the kernel parameter
"scsi_mod.dev_flags=Marvell:Console:0x4000000".  This allowed my system to
boot without any errors, however the first two issues described above are
still relevent so I thought I would provide the patches since I had already
written them up.

---

Alexander Duyck (2):
      scsi: Do not attach VPD to devices that don't support it
      scsi: Fix RCU handling for VPD pages


 drivers/scsi/scsi.c        |   55 ++++++++++++++++++++++++--------------------
 drivers/scsi/scsi_lib.c    |   12 +++++-----
 drivers/scsi/scsi_scan.c   |    3 +-
 drivers/scsi/scsi_sysfs.c  |   14 ++++++-----
 include/scsi/scsi_device.h |   14 +++++++----
 5 files changed, 54 insertions(+), 44 deletions(-)

--

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

[PATCH 2/2] scsi: Fix RCU handling for VPD pages

[Alexander Duyck]

This patch is meant to fix the RCU handling for VPD pages.  The original
code had a number of issues including the fact that the local variables
were being declared as __rcu, the RCU variable being directly accessed
outside of the RCU locked region, and the fact that length was not
associated with the data so it would be possible to get a mix and match of
the length for one VPD page with the data from another.

Fixes: 09e2b0b14690 ("scsi: rescan VPD attributes")
Signed-off-by: Alexander Duyck <aduyck@mirantis.com>
---
 drivers/scsi/scsi.c        |   52 +++++++++++++++++++++++---------------------
 drivers/scsi/scsi_lib.c    |   12 +++++-----
 drivers/scsi/scsi_sysfs.c  |   14 +++++++-----
 include/scsi/scsi_device.h |   14 ++++++++----
 4 files changed, 50 insertions(+), 42 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index ed085e78c893..143b384fd145 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -782,7 +782,7 @@ void scsi_attach_vpd(struct scsi_device *sdev)
 	int vpd_len = SCSI_VPD_PG_LEN;
 	int pg80_supported = 0;
 	int pg83_supported = 0;
-	unsigned char __rcu *vpd_buf, *orig_vpd_buf = NULL;
+	unsigned char *vpd_buf;
 
 	if (sdev->scsi_level < SCSI_3)
 		return;
@@ -816,58 +816,60 @@ retry_pg0:
 	vpd_len = SCSI_VPD_PG_LEN;
 
 	if (pg80_supported) {
+		struct scsi_vpd_pg *vpd, *orig_vpd;
 retry_pg80:
-		vpd_buf = kmalloc(vpd_len, GFP_KERNEL);
-		if (!vpd_buf)
+		vpd = kmalloc(sizeof(*vpd) + vpd_len, GFP_KERNEL);
+		if (!vpd)
 			return;
 
-		result = scsi_vpd_inquiry(sdev, vpd_buf, 0x80, vpd_len);
+		result = scsi_vpd_inquiry(sdev, vpd->buf, 0x80, vpd_len);
 		if (result < 0) {
-			kfree(vpd_buf);
+			kfree(vpd);
 			return;
 		}
 		if (result > vpd_len) {
 			vpd_len = result;
-			kfree(vpd_buf);
+			kfree(vpd);
 			goto retry_pg80;
 		}
+		vpd->len = result;
+
 		mutex_lock(&sdev->inquiry_mutex);
-		orig_vpd_buf = sdev->vpd_pg80;
-		sdev->vpd_pg80_len = result;
-		rcu_assign_pointer(sdev->vpd_pg80, vpd_buf);
+		orig_vpd = rcu_dereference_protected(sdev->vpd_pg80, 1);
+		rcu_assign_pointer(sdev->vpd_pg80, vpd);
 		mutex_unlock(&sdev->inquiry_mutex);
-		synchronize_rcu();
-		if (orig_vpd_buf) {
-			kfree(orig_vpd_buf);
-			orig_vpd_buf = NULL;
-		}
+
+		if (orig_vpd)
+			kfree_rcu(orig_vpd, rcu);
 		vpd_len = SCSI_VPD_PG_LEN;
 	}
 
 	if (pg83_supported) {
+		struct scsi_vpd_pg *vpd, *orig_vpd;
 retry_pg83:
-		vpd_buf = kmalloc(vpd_len, GFP_KERNEL);
-		if (!vpd_buf)
+		vpd = kmalloc(sizeof(*vpd) + vpd_len, GFP_KERNEL);
+		if (!vpd)
 			return;
 
-		result = scsi_vpd_inquiry(sdev, vpd_buf, 0x83, vpd_len);
+		result = scsi_vpd_inquiry(sdev, vpd->buf, 0x83, vpd_len);
 		if (result < 0) {
-			kfree(vpd_buf);
+			kfree(vpd);
 			return;
 		}
 		if (result > vpd_len) {
 			vpd_len = result;
-			kfree(vpd_buf);
+			kfree(vpd);
 			goto retry_pg83;
 		}
+		vpd->len = result;
+
 		mutex_lock(&sdev->inquiry_mutex);
-		orig_vpd_buf = sdev->vpd_pg83;
-		sdev->vpd_pg83_len = result;
-		rcu_assign_pointer(sdev->vpd_pg83, vpd_buf);
+		orig_vpd = rcu_dereference_protected(sdev->vpd_pg83, 1);
+		rcu_assign_pointer(sdev->vpd_pg83, vpd);
 		mutex_unlock(&sdev->inquiry_mutex);
-		synchronize_rcu();
-		if (orig_vpd_buf)
-			kfree(orig_vpd_buf);
+
+		if (orig_vpd)
+			kfree_rcu(orig_vpd, rcu);
 	}
 }
 
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index fa6b2c4eb7a2..e44f66bc4c90 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -3175,7 +3175,7 @@ int scsi_vpd_lun_id(struct scsi_device *sdev, char *id, size_t id_len)
 	u8 cur_id_type = 0xff;
 	u8 cur_id_size = 0;
 	unsigned char *d, *cur_id_str;
-	unsigned char __rcu *vpd_pg83;
+	struct scsi_vpd_pg *vpd_pg83;
 	int id_size = -EINVAL;
 
 	rcu_read_lock();
@@ -3205,8 +3205,8 @@ int scsi_vpd_lun_id(struct scsi_device *sdev, char *id, size_t id_len)
 	}
 
 	memset(id, 0, id_len);
-	d = vpd_pg83 + 4;
-	while (d < vpd_pg83 + sdev->vpd_pg83_len) {
+	d = vpd_pg83->buf + 4;
+	while (d < vpd_pg83->buf + vpd_pg83->len) {
 		/* Skip designators not referring to the LUN */
 		if ((d[1] & 0x30) != 0x00)
 			goto next_desig;
@@ -3308,7 +3308,7 @@ EXPORT_SYMBOL(scsi_vpd_lun_id);
 int scsi_vpd_tpg_id(struct scsi_device *sdev, int *rel_id)
 {
 	unsigned char *d;
-	unsigned char __rcu *vpd_pg83;
+	struct scsi_vpd_pg *vpd_pg83;
 	int group_id = -EAGAIN, rel_port = -1;
 
 	rcu_read_lock();
@@ -3318,8 +3318,8 @@ int scsi_vpd_tpg_id(struct scsi_device *sdev, int *rel_id)
 		return -ENXIO;
 	}
 
-	d = sdev->vpd_pg83 + 4;
-	while (d < sdev->vpd_pg83 + sdev->vpd_pg83_len) {
+	d = vpd_pg83->buf + 4;
+	while (d < vpd_pg83->buf + vpd_pg83->len) {
 		switch (d[1] & 0xf) {
 		case 0x4:
 			/* Relative target port */
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 4f18a851e2c7..061c6ec539dc 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -761,13 +761,15 @@ 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);			\
-	int ret;							\
-	if (!sdev->vpd_##_page)						\
-		return -EINVAL;						\
+	struct scsi_vpd_pg *vpd_pg;					\
+	ssize_t ret = -EINVAL;						\
+									\
 	rcu_read_lock();						\
-	ret = memory_read_from_buffer(buf, count, &off,			\
-				      rcu_dereference(sdev->vpd_##_page), \
-				       sdev->vpd_##_page##_len);	\
+	vpd_pg = rcu_dereference(sdev->vpd_##_page);			\
+	if (vpd_pg)							\
+		ret = memory_read_from_buffer(buf, count, &off,		\
+					      vpd_pg->buf,		\
+					      vpd_pg->len);		\
 	rcu_read_unlock();						\
 	return ret;						\
 }									\
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index f63a16760ae9..073a411c18ae 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -74,6 +74,13 @@ struct scsi_event {
 	 */
 };
 
+#define SCSI_VPD_PG_LEN                255
+struct scsi_vpd_pg {
+	struct rcu_head rcu;
+	int len;
+	unsigned char buf[0];
+};
+
 struct scsi_device {
 	struct Scsi_Host *host;
 	struct request_queue *request_queue;
@@ -116,11 +123,8 @@ struct scsi_device {
 	const char * model;		/* ... after scan; point to static string */
 	const char * rev;		/* ... "nullnullnullnull" before scan */
 
-#define SCSI_VPD_PG_LEN                255
-	int vpd_pg83_len;
-	unsigned char __rcu *vpd_pg83;
-	int vpd_pg80_len;
-	unsigned char __rcu *vpd_pg80;
+	struct scsi_vpd_pg __rcu *vpd_pg80;
+	struct scsi_vpd_pg __rcu *vpd_pg83;
 	unsigned char current_tag;	/* current tag */
 	struct scsi_target      *sdev_target;   /* used only for single_lun */
 

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 2/2] scsi: Fix RCU handling for VPD pages

[Hannes Reinecke]

On 01/21/2016 07:35 AM, Alexander Duyck wrote:
> This patch is meant to fix the RCU handling for VPD pages.  The original
> code had a number of issues including the fact that the local variables
> were being declared as __rcu, the RCU variable being directly accessed
> outside of the RCU locked region, and the fact that length was not
> associated with the data so it would be possible to get a mix and match of
> the length for one VPD page with the data from another.
> 
> Fixes: 09e2b0b14690 ("scsi: rescan VPD attributes")
> Signed-off-by: Alexander Duyck <aduyck@mirantis.com>
> ---
>  drivers/scsi/scsi.c        |   52 +++++++++++++++++++++++---------------------
>  drivers/scsi/scsi_lib.c    |   12 +++++-----
>  drivers/scsi/scsi_sysfs.c  |   14 +++++++-----
>  include/scsi/scsi_device.h |   14 ++++++++----
>  4 files changed, 50 insertions(+), 42 deletions(-)
> 
Thanks for fixing this up. I didn't really like the two distinct
variables for vpd buffer and length, too, but hadn't thought of
using a struct for here.

Reviewed-by: Hannes Reinecke <hare@suse.com>

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)

Re: [PATCH 2/2] scsi: Fix RCU handling for VPD pages

[Hannes Reinecke]

On 01/21/2016 07:35 AM, Alexander Duyck wrote:
> This patch is meant to fix the RCU handling for VPD pages.  The original
> code had a number of issues including the fact that the local variables
> were being declared as __rcu, the RCU variable being directly accessed
> outside of the RCU locked region, and the fact that length was not
> associated with the data so it would be possible to get a mix and match of
> the length for one VPD page with the data from another.
> 
> Fixes: 09e2b0b14690 ("scsi: rescan VPD attributes")
> Signed-off-by: Alexander Duyck <aduyck@mirantis.com>
> ---
>  drivers/scsi/scsi.c        |   52 +++++++++++++++++++++++---------------------
>  drivers/scsi/scsi_lib.c    |   12 +++++-----
>  drivers/scsi/scsi_sysfs.c  |   14 +++++++-----
>  include/scsi/scsi_device.h |   14 ++++++++----
>  4 files changed, 50 insertions(+), 42 deletions(-)
> 
Thanks for fixing this up. I didn't really like the two distinct
variables for vpd buffer and length, too, but hadn't thought of
using a struct for here.

Reviewed-by: Hannes Reinecke <hare@suse.com>

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

Re: [PATCH 0/2] scsi: Fix endless loop of ATA hard resets due to VPD reads

[Kirill A. Shutemov]

On Wed, Jan 20, 2016 at 10:35:15PM -0800, Alexander Duyck wrote:
> Ultimately neither of these bugs were my root cause.  It turns out the
> Marvel Console SCSI device in my system needed to have a flag set to
> disable VPD access in order to keep things from looping through the error
> repeatedly.  In order to resolve it I had to add the kernel parameter
> "scsi_mod.dev_flags=Marvell:Console:0x4000000".  This allowed my system to
> boot without any errors, however the first two issues described above are
> still relevent so I thought I would provide the patches since I had already
> written them up.

I have the same problem.

Shouldn't we put quirk for that?

>From d5ad5e1ee4128c454f39d7f3ccaa0b202e0e8534 Mon Sep 17 00:00:00 2001
From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
Date: Tue, 2 Feb 2016 12:44:04 +0300
Subject: [PATCH] scsi: add Marvell Console to the ignore VPD pages blacklist

With current upstream, I see these messages in loop.

	ata14: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
	ata14.00: configured for UDMA/66
	ata14: EH complete
	ata14.00: exception Emask 0x0 SAct 0x0 SErr 0x0 action 0x6
	ata14.00: irq_stat 0x40000001
	ata14.00: cmd a0/01:00:00:00:01/00:00:00:00:00/a0 tag 22 dma 16640 in
	Inquiry 12 01 00 00 ff 00res 00/00:00:00:00:00/00:00:00:00:00/00 Emask 0x3 (HSM violation)
	ata14: hard resetting link

Looks like we should blacklist the device to stop it.

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
---
 drivers/scsi/scsi_devinfo.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/scsi/scsi_devinfo.c b/drivers/scsi/scsi_devinfo.c
index 47b9d13f97b8..da2e068ee47d 100644
--- a/drivers/scsi/scsi_devinfo.c
+++ b/drivers/scsi/scsi_devinfo.c
@@ -205,6 +205,7 @@ static struct {
 	{"Intel", "Multi-Flex", NULL, BLIST_NO_RSOC},
 	{"iRiver", "iFP Mass Driver", NULL, BLIST_NOT_LOCKABLE | BLIST_INQUIRY_36},
 	{"LASOUND", "CDX7405", "3.10", BLIST_MAX5LUN | BLIST_SINGLELUN},
+	{"Marvell", "Console", NULL, BLIST_SKIP_VPD_PAGES},
 	{"MATSHITA", "PD-1", NULL, BLIST_FORCELUN | BLIST_SINGLELUN},
 	{"MATSHITA", "DMC-LC5", NULL, BLIST_NOT_LOCKABLE | BLIST_INQUIRY_36},
 	{"MATSHITA", "DMC-LC40", NULL, BLIST_NOT_LOCKABLE | BLIST_INQUIRY_36},
-- 
 Kirill A. Shutemov

Re: [PATCH 0/2] scsi: Fix endless loop of ATA hard resets due to VPD reads

[Martin K. Petersen]

>>>>> "Kirill" == Kirill A Shutemov <kirill@shutemov.name> writes:

Kirill> I have the same problem.

Kirill> Shouldn't we put quirk for that?

I was hoping that Hannes' patch would do the trick so we could avoid
blacklisting:

	https://patchwork.kernel.org/patch/8079011/

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH 0/2] scsi: Fix endless loop of ATA hard resets due to VPD reads

[Kirill A. Shutemov]

On Tue, Feb 02, 2016 at 09:45:48PM -0500, Martin K. Petersen wrote:
> >>>>> "Kirill" == Kirill A Shutemov <kirill@shutemov.name> writes:
> 
> Kirill> I have the same problem.
> 
> Kirill> Shouldn't we put quirk for that?
> 
> I was hoping that Hannes' patch would do the trick so we could avoid
> blacklisting:
> 
> 	https://patchwork.kernel.org/patch/8079011/

It didn't help me.

-- 
 Kirill A. Shutemov

Re: [PATCH 0/2] scsi: Fix endless loop of ATA hard resets due to VPD reads

[Alexander Duyck]

On Tue, Feb 2, 2016 at 10:48 PM, Kirill A. Shutemov
<kirill@shutemov.name> wrote:
> On Tue, Feb 02, 2016 at 09:45:48PM -0500, Martin K. Petersen wrote:
>> >>>>> "Kirill" == Kirill A Shutemov <kirill@shutemov.name> writes:
>>
>> Kirill> I have the same problem.
>>
>> Kirill> Shouldn't we put quirk for that?
>>
>> I was hoping that Hannes' patch would do the trick so we could avoid
>> blacklisting:
>>
>>       https://patchwork.kernel.org/patch/8079011/
>
> It didn't help me.

My patch didn't resolve the Marvell issue, it just resolved other
cases that could also lead to the same problem.

I added a line to my kernel parameters to blacklist the Marvell
console.  I wasn't sure if blacklisting would have been acceptable.
That is why I called out that I still had to add that line to my
kernel in the cover letter for my patch set.

- Alex

Re: [PATCH 0/2] scsi: Fix endless loop of ATA hard resets due to VPD reads

[Martin K. Petersen]

>>>>> "Kirill" == Kirill A Shutemov <kirill@shutemov.name> writes:

Kirill> It didn't help me.

OK, that's fine. I'll queue your patch if we can get somebody to review
it...

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH 0/2] scsi: Fix endless loop of ATA hard resets due to VPD reads

[Todd Fujinaka]

I've tried to read the code but I'm not sure where the strings are getting
parsed. I'm afraid I'm only familiar with the Ethernet drivers. In any case,
the patch didn't help and I had to add this second Marvell line. I'm not sure
if you can just wildcard the second and third fields or not.

I'm also trying to reply to a mailing-list that I'm not subscribed to using git
send-email. Please forgive the errors.

Todd Fujinaka <todd.fujinaka@intel.com>

-----

diff --git a/drivers/scsi/scsi_devinfo.c b/drivers/scsi/scsi_devinfo.c
index 47b9d13..bbfbfd9 100644
--- a/drivers/scsi/scsi_devinfo.c
+++ b/drivers/scsi/scsi_devinfo.c
@@ -205,6 +205,8 @@ static struct {
 	{"Intel", "Multi-Flex", NULL, BLIST_NO_RSOC},
 	{"iRiver", "iFP Mass Driver", NULL, BLIST_NOT_LOCKABLE | BLIST_INQUIRY_36},
 	{"LASOUND", "CDX7405", "3.10", BLIST_MAX5LUN | BLIST_SINGLELUN},
+	{"Marvell", "Console", NULL, BLIST_SKIP_VPD_PAGES},
+	{"Marvell", "91xx Config", "1.01", BLIST_SKIP_VPD_PAGES},
 	{"MATSHITA", "PD-1", NULL, BLIST_FORCELUN | BLIST_SINGLELUN},
 	{"MATSHITA", "DMC-LC5", NULL, BLIST_NOT_LOCKABLE | BLIST_INQUIRY_36},
 	{"MATSHITA", "DMC-LC40", NULL, BLIST_NOT_LOCKABLE | BLIST_INQUIRY_36},

Re: [PATCH 0/2] scsi: Fix endless loop of ATA hard resets due to VPD reads

[Martin K. Petersen]

>>>>> "Todd" == Todd Fujinaka <todd.fujinaka@intel.com> writes:

Todd,

Todd> I've tried to read the code but I'm not sure where the strings are
Todd> getting parsed. I'm afraid I'm only familiar with the Ethernet
Todd> drivers. In any case, the patch didn't help and I had to add this
Todd> second Marvell line. I'm not sure if you can just wildcard the
Todd> second and third fields or not.

I tweaked your patch to match all revisions.

Applied to 4.5/scsi-fixes.

Thank you!

-- 
Martin K. Petersen	Oracle Linux Engineering