[PATCH v3 0/6] scsi: handle special return codes for ABORTED COMMAND

[PATCH v3 0/6] scsi: handle special return codes for ABORTED COMMAND

[Martin Wilck]

Here is another attempt to handle the special return codes for ABORTED COMMAND
for certain SCSI devices. Following MKP's recommendation, I've created two
new BLIST flags, simplifying the code in scsi_error.c compared to the previous
versions. Rather than using "free" bits, I increased the length of
blist_flag_t to 64 bit, and used previously unused bits. I also added checking
for obsolete and unused bits.

For the blist_flag_t size increase, I used sparse to try and avoid regressions;
that necessitated fixing sparse's errors for the current code first.

Martin Wilck (6):
  ilog2: create truly constant version for sparse
  scsi: use const_ilog2 for array indices
  scsi: devinfo: change blist_flag_t to 64bit
  scsi: devinfo: warn on undefined blist flags
  scsi: devinfo: add BLIST_RETRY_ITF for EMC Symmetrix
  scsi: devinfo: BLIST_RETRY_ASC_C1 for Fujitsu ETERNUS

 drivers/scsi/Makefile       |  2 +-
 drivers/scsi/scsi_debugfs.c |  2 +-
 drivers/scsi/scsi_devinfo.c | 28 +++++++++++++----
 drivers/scsi/scsi_error.c   |  7 +++++
 drivers/scsi/scsi_sysfs.c   |  2 +-
 include/linux/log2.h        | 35 ++++++++++++++-------
 include/scsi/scsi_device.h  |  2 +-
 include/scsi/scsi_devinfo.h | 75 ++++++++++++++++++++++++++++++---------------
 8 files changed, 107 insertions(+), 46 deletions(-)

-- 
2.16.1

[PATCH v3 1/6] ilog2: create truly constant version for sparse

[Martin Wilck]

Sparse emits errors about ilog2() in array indices because of the use of
__ilog2_32() and __ilog2_64(), rightly so
(https://www.spinics.net/lists/linux-sparse/msg03471.html).

Create a const_ilog2() variant that works with sparse for this
scenario.

(Note: checkpatch.pl complains about missing parentheses, but that appears
to be a false positive. I can get rid of the warning simply by inserting
whitespace, making checkpatch "see" the whole macro).

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 include/linux/log2.h | 35 ++++++++++++++++++++++++-----------
 1 file changed, 24 insertions(+), 11 deletions(-)

diff --git a/include/linux/log2.h b/include/linux/log2.h
index 41a1ae0..2af7f778 100644
--- a/include/linux/log2.h
+++ b/include/linux/log2.h
@@ -72,16 +72,13 @@ unsigned long __rounddown_pow_of_two(unsigned long n)
 }
 
 /**
- * ilog2 - log base 2 of 32-bit or a 64-bit unsigned value
+ * const_ilog2 - log base 2 of 32-bit or a 64-bit constant unsigned value
  * @n: parameter
  *
- * constant-capable log of base 2 calculation
- * - this can be used to initialise global variables from constant data, hence
- * the massive ternary operator construction
- *
- * selects the appropriately-sized optimised version depending on sizeof(n)
+ * Use this where sparse expects a true constant expression, e.g. for array
+ * indices.
  */
-#define ilog2(n)				\
+#define const_ilog2(n)				\
 (						\
 	__builtin_constant_p(n) ? (		\
 		(n) < 2 ? 0 :			\
@@ -147,10 +144,26 @@ unsigned long __rounddown_pow_of_two(unsigned long n)
 		(n) & (1ULL <<  4) ?  4 :	\
 		(n) & (1ULL <<  3) ?  3 :	\
 		(n) & (1ULL <<  2) ?  2 :	\
-		1 ) :				\
-	(sizeof(n) <= 4) ?			\
-	__ilog2_u32(n) :			\
-	__ilog2_u64(n)				\
+		1) :				\
+	-1)
+
+/**
+ * ilog2 - log base 2 of 32-bit or a 64-bit unsigned value
+ * @n: parameter
+ *
+ * constant-capable log of base 2 calculation
+ * - this can be used to initialise global variables from constant data, hence
+ * the massive ternary operator construction
+ *
+ * selects the appropriately-sized optimised version depending on sizeof(n)
+ */
+#define ilog2(n) \
+( \
+	__builtin_constant_p(n) ?	\
+	const_ilog2(n) :		\
+	(sizeof(n) <= 4) ?		\
+	__ilog2_u32(n) :		\
+	__ilog2_u64(n)			\
  )
 
 /**
-- 
2.16.1

Re: [PATCH v3 1/6] ilog2: create truly constant version for sparse

[Linus Torvalds]

On Tue, Apr 17, 2018 at 4:35 PM, Martin Wilck <mwilck@suse.com> wrote:
> Sparse emits errors about ilog2() in array indices because of the use of
> __ilog2_32() and __ilog2_64(),

If sparse warns about it, then presumably gcc with -Wvla warns about it too?

And if thats the case, then __builtin_constant_p() and a ternary
operation isn't good enough, because gcc -Wvla is even more anal than
sparse is.

Which is why we have that __is_constexpr() magic for min/max().

So I suspect that what you'd want is

  #define ilog2(n) \
        __builtin_choose_expr(__is_constexpr(n), \
                const_ilog2(n), \
                __builtin_choose_expr(sizeof(n) <= 4, \
                        __ilog2_u32(n), \
                        __ilog2_u64(n)))

or something. Hmm?

           Linus

Re: [PATCH v3 1/6] ilog2: create truly constant version for sparse

[Martin Wilck]

On Tue, 2018-04-17 at 17:07 -0700, Linus Torvalds wrote:
> On Tue, Apr 17, 2018 at 4:35 PM, Martin Wilck <mwilck@suse.com>
> wrote:
> > Sparse emits errors about ilog2() in array indices because of the
> > use of
> > __ilog2_32() and __ilog2_64(),
> 
> If sparse warns about it, then presumably gcc with -Wvla warns about
> it too?

No, it doesn't (gcc 7.3.0). -> https://paste.opensuse.org/27471594
It doesn't even warn on an expression like this:

  #define SIZE (1<<10)
  static int foo[ilog2(SIZE)];

sparse 0.5.2 doesn't warn about that either. It emits "error: bad
integer constant expression" only if ilog2 is used in an array
initializer, like this:

  #define SIZE (1<<10)
  #define SUBS (1<<5)
  static int foo [ilog2(SIZE)] = {
          [ilog2(SUBS)] = 0,
  };

So maybe I was wrong, and this is actually a false positive in sparse.

> So I suspect that what you'd want is
> 
>   #define ilog2(n) \
>         __builtin_choose_expr(__is_constexpr(n), \
>                 const_ilog2(n), \
>                 __builtin_choose_expr(sizeof(n) <= 4, \
>                         __ilog2_u32(n), \
>                         __ilog2_u64(n)))
> 
> or something. Hmm?

Do you want me to convert the patch to your approach anyway?
Or should I throw this away and report to sparse?

Regards and thanks,
Martin

PS: apologies to all recipients for the broken cc list in my post.
-- 
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH v3 1/6] ilog2: create truly constant version for sparse

[Luc Van Oostenryck]

On Wed, Apr 18, 2018 at 10:12:54AM +0200, Martin Wilck wrote:
> On Tue, 2018-04-17 at 17:07 -0700, Linus Torvalds wrote:
> > On Tue, Apr 17, 2018 at 4:35 PM, Martin Wilck <mwilck@suse.com>
> > wrote:
> > > Sparse emits errors about ilog2() in array indices because of the
> > > use of
> > > __ilog2_32() and __ilog2_64(),
> > 
> > If sparse warns about it, then presumably gcc with -Wvla warns about
> > it too?
> 
> No, it doesn't (gcc 7.3.0). -> https://paste.opensuse.org/27471594
> It doesn't even warn on an expression like this:
> 
>   #define SIZE (1<<10)
>   static int foo[ilog2(SIZE)];
> 
> sparse 0.5.2 doesn't warn about that either. It emits "error: bad
> integer constant expression" only if ilog2 is used in an array

sparse supports VLAs at syntaxic level but not much more. Anything
needing directly or indirectly the array size will give this error.

-- Luc Van Oostenryck

Re: [PATCH v3 1/6] ilog2: create truly constant version for sparse

[Linus Torvalds]

On Wed, Apr 18, 2018 at 1:12 AM, Martin Wilck <mwilck@suse.com> wrote:
>
> No, it doesn't (gcc 7.3.0). -> https://paste.opensuse.org/27471594
> It doesn't even warn on an expression like this:
>
>   #define SIZE (1<<10)
>   static int foo[ilog2(SIZE)];

Ok, I think this is the "random gcc versions act differently" issue.

Sometimes __builtin_constant_p() to a ternary operation acts as a
constant expression, and sometimes it doesn't.

Oh well.

> sparse 0.5.2 doesn't warn about that either.

Yeah, sparse doesn't get picky about "constant value" vs "constant
expression" normally. But some things do use the strict form, and
array index expressions is one of those cases.

> So maybe I was wrong, and this is actually a false positive in sparse.

No, it's correct, it's just that the semantics of exactly when
something is considered constant are a bit fluid.

> Do you want me to convert the patch to your approach anyway?

I suspect using the __is_constexpr() trick would make it rather more
technically correct, but would actually generate much worse code.

Because it would make us do that dynamic "__ilog2_uXX()" function call
even when you have a compile-time constant value, if it wasn't
actually a constant expression (ie a constant argument passed to an
inline function, for example).

So I guess your patch is fine, but I also wonder if we should just
default sparse to allow the good old non-strict "constant values are
ok" for indexes.

And turn on strict mode only with -pedantic or something.

                 Linus

Re: [PATCH v3 1/6] ilog2: create truly constant version for sparse

[Rasmus Villemoes]

On 2018-04-18 23:21, Linus Torvalds wrote:
> On Wed, Apr 18, 2018 at 1:12 AM, Martin Wilck <mwilck@suse.com> wrote:
>>
>> No, it doesn't (gcc 7.3.0). -> https://paste.opensuse.org/27471594
>> It doesn't even warn on an expression like this:
>>
>>   #define SIZE (1<<10)
>>   static int foo[ilog2(SIZE)];
> 
> Ok, I think this is the "random gcc versions act differently" issue.
> 
> Sometimes __builtin_constant_p() to a ternary operation acts as a
> constant expression, and sometimes it doesn't.

So __builtin_constant_p on an actual ICE always fold immediately to 1.
Looking at the gcc history, that seems to have been there since at least
2000, but likely forever. And when it's the controlling expression of a
ternary, it's apparently a stronger 1 than a literal 1:

int foo(int);
#define SIZE (1<<10)
#define half(x) (__builtin_constant_p(x) ? (x)>>1 : foo(x))
int bla[half(SIZE)];
int bla2[1 ? 123 : foo(3)+2];

on godbolt.org, gcc 4.1 and gcc4.4 are perfectly happy with this, but
newer gccs complain

  error: variably modified 'bla2' at file scope.

Argh.

> I suspect using the __is_constexpr() trick would make it rather more
> technically correct, but would actually generate much worse code.
> 
> Because it would make us do that dynamic "__ilog2_uXX()" function call
> even when you have a compile-time constant value, if it wasn't
> actually a constant expression (ie a constant argument passed to an
> inline function, for example).

It's a bit ugly, but I suppose one could keep a __builtin_constant_p()
check in the not-ICE-branch, so there's really three cases (ICE,
constant due to various optimizations, really not known at compile
time). But don't we use gcc's intrinsics for fls when available, so that
gcc should be able to know the semantics of __builtin_fls(some-constant)
and hence evaluate that itself at compile time?

Somewhat unrelated, we should probably move the __is_constexpr helper
from kernel.h to some more basic compiler header, to avoid cyclic header
dependencies.

Rasmus

[PATCH v3 2/6] scsi: use const_ilog2 for array indices

[Martin Wilck]

Use the just introduced const_ilog2() macro to avoid sparse
errors.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 drivers/scsi/scsi_debugfs.c | 2 +-
 drivers/scsi/scsi_sysfs.c   | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/scsi_debugfs.c b/drivers/scsi/scsi_debugfs.c
index b784002..c5a8756 100644
--- a/drivers/scsi/scsi_debugfs.c
+++ b/drivers/scsi/scsi_debugfs.c
@@ -4,7 +4,7 @@
 #include <scsi/scsi_dbg.h>
 #include "scsi_debugfs.h"
 
-#define SCSI_CMD_FLAG_NAME(name) [ilog2(SCMD_##name)] = #name
+#define SCSI_CMD_FLAG_NAME(name)[const_ilog2(SCMD_##name)] = #name
 static const char *const scsi_cmd_flags[] = {
 	SCSI_CMD_FLAG_NAME(TAGGED),
 	SCSI_CMD_FLAG_NAME(UNCHECKED_ISA_DMA),
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index e56a4ac..feacd7a 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -968,7 +968,7 @@ sdev_show_wwid(struct device *dev, struct device_attribute *attr,
 static DEVICE_ATTR(wwid, S_IRUGO, sdev_show_wwid, NULL);
 
 #define BLIST_FLAG_NAME(name)					\
-	[ilog2((__force unsigned int)BLIST_##name)] = #name
+	[const_ilog2((__force unsigned int)BLIST_##name)] = #name
 static const char *const sdev_bflags_name[] = {
 #include "scsi_devinfo_tbl.c"
 };
-- 
2.16.1

[PATCH v3 3/6] scsi: devinfo: change blist_flag_t to 64bit

[Martin Wilck]

Space for SCSI blist flags is gradually running out. Change the
type to __u64. And fix a checkpatch complaint about symbolic
mode flags in scsi_devinfo.c.

Make checkpatch happy by replacing simple_strtoul() with kstrtoull().

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 drivers/scsi/scsi_devinfo.c | 18 +++++++++++-----
 drivers/scsi/scsi_sysfs.c   |  2 +-
 include/scsi/scsi_device.h  |  2 +-
 include/scsi/scsi_devinfo.h | 50 ++++++++++++++++++++++-----------------------
 4 files changed, 40 insertions(+), 32 deletions(-)

diff --git a/drivers/scsi/scsi_devinfo.c b/drivers/scsi/scsi_devinfo.c
index e5370d7..fc6b755 100644
--- a/drivers/scsi/scsi_devinfo.c
+++ b/drivers/scsi/scsi_devinfo.c
@@ -361,8 +361,16 @@ int scsi_dev_info_list_add_keyed(int compatible, char *vendor, char *model,
 	scsi_strcpy_devinfo("model", devinfo->model, sizeof(devinfo->model),
 			    model, compatible);
 
-	if (strflags)
-		flags = (__force blist_flags_t)simple_strtoul(strflags, NULL, 0);
+	if (strflags) {
+		unsigned long long val;
+		int ret = kstrtoull(strflags, 0, &val);
+
+		if (ret != 0) {
+			kfree(devinfo);
+			return ret;
+		}
+		flags = (__force blist_flags_t)val;
+	}
 	devinfo->flags = flags;
 	devinfo->compatible = compatible;
 
@@ -615,7 +623,7 @@ static int devinfo_seq_show(struct seq_file *m, void *v)
 	    devinfo_table->name)
 		seq_printf(m, "[%s]:\n", devinfo_table->name);
 
-	seq_printf(m, "'%.8s' '%.16s' 0x%x\n",
+	seq_printf(m, "'%.8s' '%.16s' 0x%llx\n",
 		   devinfo->vendor, devinfo->model, devinfo->flags);
 	return 0;
 }
@@ -734,9 +742,9 @@ MODULE_PARM_DESC(dev_flags,
 	 " list entries for vendor and model with an integer value of flags"
 	 " to the scsi device info list");
 
-module_param_named(default_dev_flags, scsi_default_dev_flags, int, S_IRUGO|S_IWUSR);
+module_param_named(default_dev_flags, scsi_default_dev_flags, ullong, 0644);
 MODULE_PARM_DESC(default_dev_flags,
-		 "scsi default device flag integer value");
+		 "scsi default device flag uint64_t value");
 
 /**
  * scsi_exit_devinfo - remove /proc/scsi/device_info & the scsi_dev_info_list
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index feacd7a..43c27ce 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -968,7 +968,7 @@ sdev_show_wwid(struct device *dev, struct device_attribute *attr,
 static DEVICE_ATTR(wwid, S_IRUGO, sdev_show_wwid, NULL);
 
 #define BLIST_FLAG_NAME(name)					\
-	[const_ilog2((__force unsigned int)BLIST_##name)] = #name
+	[const_ilog2((__force __u64)BLIST_##name)] = #name
 static const char *const sdev_bflags_name[] = {
 #include "scsi_devinfo_tbl.c"
 };
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 7ae177c..4c36af6 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -15,7 +15,7 @@ struct scsi_cmnd;
 struct scsi_lun;
 struct scsi_sense_hdr;
 
-typedef unsigned int __bitwise blist_flags_t;
+typedef __u64 __bitwise blist_flags_t;
 
 struct scsi_mode_data {
 	__u32	length;
diff --git a/include/scsi/scsi_devinfo.h b/include/scsi/scsi_devinfo.h
index ea67c32..e206d29 100644
--- a/include/scsi/scsi_devinfo.h
+++ b/include/scsi/scsi_devinfo.h
@@ -6,55 +6,55 @@
  */
 
 /* Only scan LUN 0 */
-#define BLIST_NOLUN		((__force blist_flags_t)(1 << 0))
+#define BLIST_NOLUN		((__force blist_flags_t)(1ULL << 0))
 /* Known to have LUNs, force scanning.
  * DEPRECATED: Use max_luns=N */
-#define BLIST_FORCELUN		((__force blist_flags_t)(1 << 1))
+#define BLIST_FORCELUN		((__force blist_flags_t)(1ULL << 1))
 /* Flag for broken handshaking */
-#define BLIST_BORKEN		((__force blist_flags_t)(1 << 2))
+#define BLIST_BORKEN		((__force blist_flags_t)(1ULL << 2))
 /* unlock by special command */
-#define BLIST_KEY		((__force blist_flags_t)(1 << 3))
+#define BLIST_KEY		((__force blist_flags_t)(1ULL << 3))
 /* Do not use LUNs in parallel */
-#define BLIST_SINGLELUN		((__force blist_flags_t)(1 << 4))
+#define BLIST_SINGLELUN		((__force blist_flags_t)(1ULL << 4))
 /* Buggy Tagged Command Queuing */
-#define BLIST_NOTQ		((__force blist_flags_t)(1 << 5))
+#define BLIST_NOTQ		((__force blist_flags_t)(1ULL << 5))
 /* Non consecutive LUN numbering */
-#define BLIST_SPARSELUN		((__force blist_flags_t)(1 << 6))
+#define BLIST_SPARSELUN		((__force blist_flags_t)(1ULL << 6))
 /* Avoid LUNS >= 5 */
-#define BLIST_MAX5LUN		((__force blist_flags_t)(1 << 7))
+#define BLIST_MAX5LUN		((__force blist_flags_t)(1ULL << 7))
 /* Treat as (removable) CD-ROM */
-#define BLIST_ISROM		((__force blist_flags_t)(1 << 8))
+#define BLIST_ISROM		((__force blist_flags_t)(1ULL << 8))
 /* LUNs past 7 on a SCSI-2 device */
-#define BLIST_LARGELUN		((__force blist_flags_t)(1 << 9))
+#define BLIST_LARGELUN		((__force blist_flags_t)(1ULL << 9))
 /* override additional length field */
-#define BLIST_INQUIRY_36	((__force blist_flags_t)(1 << 10))
+#define BLIST_INQUIRY_36	((__force blist_flags_t)(1ULL << 10))
 /* do not do automatic start on add */
-#define BLIST_NOSTARTONADD	((__force blist_flags_t)(1 << 12))
+#define BLIST_NOSTARTONADD	((__force blist_flags_t)(1ULL << 12))
 /* try REPORT_LUNS even for SCSI-2 devs (if HBA supports more than 8 LUNs) */
-#define BLIST_REPORTLUN2	((__force blist_flags_t)(1 << 17))
+#define BLIST_REPORTLUN2	((__force blist_flags_t)(1ULL << 17))
 /* don't try REPORT_LUNS scan (SCSI-3 devs) */
-#define BLIST_NOREPORTLUN	((__force blist_flags_t)(1 << 18))
+#define BLIST_NOREPORTLUN	((__force blist_flags_t)(1ULL << 18))
 /* don't use PREVENT-ALLOW commands */
-#define BLIST_NOT_LOCKABLE	((__force blist_flags_t)(1 << 19))
+#define BLIST_NOT_LOCKABLE	((__force blist_flags_t)(1ULL << 19))
 /* device is actually for RAID config */
-#define BLIST_NO_ULD_ATTACH	((__force blist_flags_t)(1 << 20))
+#define BLIST_NO_ULD_ATTACH	((__force blist_flags_t)(1ULL << 20))
 /* select without ATN */
-#define BLIST_SELECT_NO_ATN	((__force blist_flags_t)(1 << 21))
+#define BLIST_SELECT_NO_ATN	((__force blist_flags_t)(1ULL << 21))
 /* retry HARDWARE_ERROR */
-#define BLIST_RETRY_HWERROR	((__force blist_flags_t)(1 << 22))
+#define BLIST_RETRY_HWERROR	((__force blist_flags_t)(1ULL << 22))
 /* maximum 512 sector cdb length */
-#define BLIST_MAX_512		((__force blist_flags_t)(1 << 23))
+#define BLIST_MAX_512		((__force blist_flags_t)(1ULL << 23))
 /* Disable T10 PI (DIF) */
-#define BLIST_NO_DIF		((__force blist_flags_t)(1 << 25))
+#define BLIST_NO_DIF		((__force blist_flags_t)(1ULL << 25))
 /* Ignore SBC-3 VPD pages */
-#define BLIST_SKIP_VPD_PAGES	((__force blist_flags_t)(1 << 26))
+#define BLIST_SKIP_VPD_PAGES	((__force blist_flags_t)(1ULL << 26))
 /* Attempt to read VPD pages */
-#define BLIST_TRY_VPD_PAGES	((__force blist_flags_t)(1 << 28))
+#define BLIST_TRY_VPD_PAGES	((__force blist_flags_t)(1ULL << 28))
 /* don't try to issue RSOC */
-#define BLIST_NO_RSOC		((__force blist_flags_t)(1 << 29))
+#define BLIST_NO_RSOC		((__force blist_flags_t)(1ULL << 29))
 /* maximum 1024 sector cdb length */
-#define BLIST_MAX_1024		((__force blist_flags_t)(1 << 30))
+#define BLIST_MAX_1024		((__force blist_flags_t)(1ULL << 30))
 /* Use UNMAP limit for WRITE SAME */
-#define BLIST_UNMAP_LIMIT_WS	((__force blist_flags_t)(1 << 31))
+#define BLIST_UNMAP_LIMIT_WS	((__force blist_flags_t)(1ULL << 31))
 
 #endif
-- 
2.16.1

[PATCH v3 4/6] scsi: devinfo: warn on undefined blist flags

[Martin Wilck]

Warn if a device (or the user) sets blist flags which are unknown
or have been removed. This should enable us to reuse freed blist
bits in later releases.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 drivers/scsi/Makefile       |  2 +-
 drivers/scsi/scsi_devinfo.c |  6 ++++++
 include/scsi/scsi_devinfo.h | 21 +++++++++++++++++++++
 3 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/Makefile b/drivers/scsi/Makefile
index d5135ef..fd901a5 100644
--- a/drivers/scsi/Makefile
+++ b/drivers/scsi/Makefile
@@ -190,7 +190,7 @@ $(obj)/53c700.o $(MODVERDIR)/$(obj)/53c700.ver: $(obj)/53c700_d.h
 $(obj)/scsi_sysfs.o: $(obj)/scsi_devinfo_tbl.c
 
 quiet_cmd_bflags = GEN     $@
-	cmd_bflags = sed -n 's/.*BLIST_\([A-Z0-9_]*\) *.*/BLIST_FLAG_NAME(\1),/p' $< > $@
+	cmd_bflags = sed -n 's/.*define *BLIST_\([A-Z0-9_]*\) *.*/BLIST_FLAG_NAME(\1),/p' $< > $@
 
 $(obj)/scsi_devinfo_tbl.c: include/scsi/scsi_devinfo.h
 	$(call if_changed,bflags)
diff --git a/drivers/scsi/scsi_devinfo.c b/drivers/scsi/scsi_devinfo.c
index fc6b755..c05843a 100644
--- a/drivers/scsi/scsi_devinfo.c
+++ b/drivers/scsi/scsi_devinfo.c
@@ -371,6 +371,12 @@ int scsi_dev_info_list_add_keyed(int compatible, char *vendor, char *model,
 		}
 		flags = (__force blist_flags_t)val;
 	}
+	if (flags & __BLIST_UNUSED_MASK) {
+		pr_err("scsi_devinfo (%s:%s): unsupported flags 0x%llx",
+		       vendor, model, flags & __BLIST_UNUSED_MASK);
+		kfree(devinfo);
+		return -EINVAL;
+	}
 	devinfo->flags = flags;
 	devinfo->compatible = compatible;
 
diff --git a/include/scsi/scsi_devinfo.h b/include/scsi/scsi_devinfo.h
index e206d29..3434e14 100644
--- a/include/scsi/scsi_devinfo.h
+++ b/include/scsi/scsi_devinfo.h
@@ -28,8 +28,13 @@
 #define BLIST_LARGELUN		((__force blist_flags_t)(1ULL << 9))
 /* override additional length field */
 #define BLIST_INQUIRY_36	((__force blist_flags_t)(1ULL << 10))
+#define __BLIST_UNUSED_11	((__force blist_flags_t)(1ULL << 11))
 /* do not do automatic start on add */
 #define BLIST_NOSTARTONADD	((__force blist_flags_t)(1ULL << 12))
+#define __BLIST_UNUSED_13	((__force blist_flags_t)(1ULL << 13))
+#define __BLIST_UNUSED_14	((__force blist_flags_t)(1ULL << 14))
+#define __BLIST_UNUSED_15	((__force blist_flags_t)(1ULL << 15))
+#define __BLIST_UNUSED_16	((__force blist_flags_t)(1ULL << 16))
 /* try REPORT_LUNS even for SCSI-2 devs (if HBA supports more than 8 LUNs) */
 #define BLIST_REPORTLUN2	((__force blist_flags_t)(1ULL << 17))
 /* don't try REPORT_LUNS scan (SCSI-3 devs) */
@@ -44,10 +49,12 @@
 #define BLIST_RETRY_HWERROR	((__force blist_flags_t)(1ULL << 22))
 /* maximum 512 sector cdb length */
 #define BLIST_MAX_512		((__force blist_flags_t)(1ULL << 23))
+#define __BLIST_UNUSED_24	((__force blist_flags_t)(1ULL << 24))
 /* Disable T10 PI (DIF) */
 #define BLIST_NO_DIF		((__force blist_flags_t)(1ULL << 25))
 /* Ignore SBC-3 VPD pages */
 #define BLIST_SKIP_VPD_PAGES	((__force blist_flags_t)(1ULL << 26))
+#define __BLIST_UNUSED_27	((__force blist_flags_t)(1ULL << 27))
 /* Attempt to read VPD pages */
 #define BLIST_TRY_VPD_PAGES	((__force blist_flags_t)(1ULL << 28))
 /* don't try to issue RSOC */
@@ -57,4 +64,18 @@
 /* Use UNMAP limit for WRITE SAME */
 #define BLIST_UNMAP_LIMIT_WS	((__force blist_flags_t)(1ULL << 31))
 
+#define __BLIST_LAST_USED BLIST_UNMAP_LIMIT_WS
+
+#define __BLIST_HIGH_UNUSED (~(__BLIST_LAST_USED | \
+			       (__force blist_flags_t) \
+			       ((__force __u64)__BLIST_LAST_USED - 1ULL)))
+#define __BLIST_UNUSED_MASK (__BLIST_UNUSED_11 | \
+			     __BLIST_UNUSED_13 | \
+			     __BLIST_UNUSED_14 | \
+			     __BLIST_UNUSED_15 | \
+			     __BLIST_UNUSED_16 | \
+			     __BLIST_UNUSED_24 | \
+			     __BLIST_UNUSED_27 | \
+			     __BLIST_HIGH_UNUSED)
+
 #endif
-- 
2.16.1

[PATCH v3 5/6] scsi: devinfo: add BLIST_RETRY_ITF for EMC Symmetrix

[Martin Wilck]

EMC Symmetrix returns 'internal target error' for a variety
of conditions, most of which will be transient.
So we should always retry it, even with failfast set.
Otherwise we'd get spurious path flaps with multipath.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 drivers/scsi/scsi_devinfo.c | 3 ++-
 drivers/scsi/scsi_error.c   | 4 ++++
 include/scsi/scsi_devinfo.h | 4 +++-
 3 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/scsi_devinfo.c b/drivers/scsi/scsi_devinfo.c
index c05843a..f7b94c1 100644
--- a/drivers/scsi/scsi_devinfo.c
+++ b/drivers/scsi/scsi_devinfo.c
@@ -161,7 +161,8 @@ static struct {
 	{"DGC", "RAID", NULL, BLIST_SPARSELUN},	/* EMC CLARiiON, storage on LUN 0 */
 	{"DGC", "DISK", NULL, BLIST_SPARSELUN},	/* EMC CLARiiON, no storage on LUN 0 */
 	{"EMC",  "Invista", "*", BLIST_SPARSELUN | BLIST_LARGELUN},
-	{"EMC", "SYMMETRIX", NULL, BLIST_SPARSELUN | BLIST_LARGELUN | BLIST_REPORTLUN2},
+	{"EMC", "SYMMETRIX", NULL, BLIST_SPARSELUN | BLIST_LARGELUN |
+	 BLIST_REPORTLUN2 | BLIST_RETRY_ITF},
 	{"EMULEX", "MD21/S2     ESDI", NULL, BLIST_SINGLELUN},
 	{"easyRAID", "16P", NULL, BLIST_NOREPORTLUN},
 	{"easyRAID", "X6P", NULL, BLIST_NOREPORTLUN},
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index ac3b1c3..1dee91f 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -38,6 +38,7 @@
 #include <scsi/scsi_host.h>
 #include <scsi/scsi_ioctl.h>
 #include <scsi/scsi_dh.h>
+#include <scsi/scsi_devinfo.h>
 #include <scsi/sg.h>
 
 #include "scsi_priv.h"
@@ -524,6 +525,9 @@ int scsi_check_sense(struct scsi_cmnd *scmd)
 		if (sshdr.asc == 0x10) /* DIF */
 			return SUCCESS;
 
+		if (sshdr.asc == 0x44 && sdev->sdev_bflags & BLIST_RETRY_ITF)
+			return ADD_TO_MLQUEUE;
+
 		return NEEDS_RETRY;
 	case NOT_READY:
 	case UNIT_ATTENTION:
diff --git a/include/scsi/scsi_devinfo.h b/include/scsi/scsi_devinfo.h
index 3434e14..91a327e 100644
--- a/include/scsi/scsi_devinfo.h
+++ b/include/scsi/scsi_devinfo.h
@@ -63,8 +63,10 @@
 #define BLIST_MAX_1024		((__force blist_flags_t)(1ULL << 30))
 /* Use UNMAP limit for WRITE SAME */
 #define BLIST_UNMAP_LIMIT_WS	((__force blist_flags_t)(1ULL << 31))
+/* Always retry ABORTED_COMMAND with Internal Target Failure */
+#define BLIST_RETRY_ITF		((__force blist_flags_t)(1ULL << 32))
 
-#define __BLIST_LAST_USED BLIST_UNMAP_LIMIT_WS
+#define __BLIST_LAST_USED BLIST_RETRY_ITF
 
 #define __BLIST_HIGH_UNUSED (~(__BLIST_LAST_USED | \
 			       (__force blist_flags_t) \
-- 
2.16.1

[PATCH v3 6/6] scsi: devinfo: BLIST_RETRY_ASC_C1 for Fujitsu ETERNUS

[Martin Wilck]

On Fujitsu ETERNUS systems, sense code ABORTED COMMAND with ASC/Q C1/01 is
used to indicate temporary condition where the storage-internal path to a
target is switched from one controller to another. SCSI commands that
return with this error code must be retried unconditionally (i.e. without
the "maybe_retry" logic in scsi_decide_disposition); otherwise dm-multipath
might initiate a failover from a healthy path e.g. for REQ_FAILFAST_DEV
commands.

Introduce a new blist flag for this case.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 drivers/scsi/scsi_devinfo.c | 1 +
 drivers/scsi/scsi_error.c   | 3 +++
 include/scsi/scsi_devinfo.h | 4 +++-
 3 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_devinfo.c b/drivers/scsi/scsi_devinfo.c
index f7b94c1..e75a50f 100644
--- a/drivers/scsi/scsi_devinfo.c
+++ b/drivers/scsi/scsi_devinfo.c
@@ -168,6 +168,7 @@ static struct {
 	{"easyRAID", "X6P", NULL, BLIST_NOREPORTLUN},
 	{"easyRAID", "F8", NULL, BLIST_NOREPORTLUN},
 	{"FSC", "CentricStor", "*", BLIST_SPARSELUN | BLIST_LARGELUN},
+	{"FUJITSU", "ETERNUS_DXM", "*", BLIST_RETRY_ASC_C1},
 	{"Generic", "USB SD Reader", "1.00", BLIST_FORCELUN | BLIST_INQUIRY_36},
 	{"Generic", "USB Storage-SMC", "0180", BLIST_FORCELUN | BLIST_INQUIRY_36},
 	{"Generic", "USB Storage-SMC", "0207", BLIST_FORCELUN | BLIST_INQUIRY_36},
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 1dee91f..896b991 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -527,6 +527,9 @@ int scsi_check_sense(struct scsi_cmnd *scmd)
 
 		if (sshdr.asc == 0x44 && sdev->sdev_bflags & BLIST_RETRY_ITF)
 			return ADD_TO_MLQUEUE;
+		if (sshdr.asc == 0xc1 && sshdr.ascq == 0x01 &&
+		    sdev->sdev_bflags & BLIST_RETRY_ASC_C1)
+			return ADD_TO_MLQUEUE;
 
 		return NEEDS_RETRY;
 	case NOT_READY:
diff --git a/include/scsi/scsi_devinfo.h b/include/scsi/scsi_devinfo.h
index 91a327e..3fdb322 100644
--- a/include/scsi/scsi_devinfo.h
+++ b/include/scsi/scsi_devinfo.h
@@ -65,8 +65,10 @@
 #define BLIST_UNMAP_LIMIT_WS	((__force blist_flags_t)(1ULL << 31))
 /* Always retry ABORTED_COMMAND with Internal Target Failure */
 #define BLIST_RETRY_ITF		((__force blist_flags_t)(1ULL << 32))
+/* Always retry ABORTED_COMMAND with ASC 0xc1 */
+#define BLIST_RETRY_ASC_C1	((__force blist_flags_t)(1ULL << 33))
 
-#define __BLIST_LAST_USED BLIST_RETRY_ITF
+#define __BLIST_LAST_USED BLIST_RETRY_ASC_C1
 
 #define __BLIST_HIGH_UNUSED (~(__BLIST_LAST_USED | \
 			       (__force blist_flags_t) \
-- 
2.16.1

Re: [PATCH v3 0/6] scsi: handle special return codes for ABORTED COMMAND

[Martin K. Petersen]

Martin,

> Here is another attempt to handle the special return codes for ABORTED
> COMMAND for certain SCSI devices. Following MKP's recommendation, I've
> created two new BLIST flags, simplifying the code in scsi_error.c
> compared to the previous versions. Rather than using "free" bits, I
> increased the length of blist_flag_t to 64 bit, and used previously
> unused bits. I also added checking for obsolete and unused bits.
>
> For the blist_flag_t size increase, I used sparse to try and avoid
> regressions; that necessitated fixing sparse's errors for the current
> code first.

Much better, thanks for reworking this. Applied to 4.18/scsi-queue.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH v3 0/6] scsi: handle special return codes for ABORTED COMMAND

[Martin Wilck]

On Fri, 2018-04-20 at 19:15 -0400, Martin K. Petersen wrote:
> 
> Much better, thanks for reworking this. Applied to 4.18/scsi-queue.

Thank you!

By the way, I've been wondering whether declaring blist_flags_t
__bitwise was a wise decision. blist_flags_t is kernel-internal, thus
endianness doesn't matter. OTOH, using __bitwise requires explicit
casts in many places, which may suppress warnings about integer size
mismatch and made me overlook some places where I had to change
"unsigned long" to "unsigned long long" in the first place
(in the submitted and applied version I think I caught them all).

Regards,
Martin

-- 
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)