[PATCHv4 0/5] Fixup blacklist handling

[PATCHv4 0/5] Fixup blacklist handling

[Hannes Reinecke]

the SCSI blacklist handling seems to be rather tricky issue;
everytime a fix is included it tends to break other devices.
This patchset attempt to simplify the devlist handling yet again,
but this time implementing the framework for regression testing, too.
A patch adding a regression test to the blktest suite will follow.

As usual, comments and reviews are welcome.

Changes to v1:
- Implement exact match for vendor string as suggested by Bart
- Straigten out issues pointed out by Alan Stern
- Reshuffle patches

Changes to v2:
- Simplify code as indicated by Alan Stern
- Display blacklist flags verbatim
- Reformat blacklist flags definition for better readability

Changed to v3:
- Add reviews from Alan Stern
- Generate blacklist flag definitions
- Include reviews from Martin Petersen

Hannes Reinecke (5):
  scsi_debug: allow to specify inquiry vendor and model
  scsi: Export blacklist flags to sysfs
  scsi_devinfo: Reformat blacklist flags
  scsi_devinfo: Whitespace fixes
  scsi_devinfo: fixup string compare

 drivers/scsi/Makefile       |  8 +++++
 drivers/scsi/mktbl.pl       | 26 ++++++++++++++++
 drivers/scsi/scsi_debug.c   | 25 ++++++++++-----
 drivers/scsi/scsi_devinfo.c | 67 ++++++++++++++++++++-------------------
 drivers/scsi/scsi_scan.c    |  1 +
 drivers/scsi/scsi_sysfs.c   | 60 +++++++++++++++++++++++++++++++++++
 include/scsi/scsi_devinfo.h | 76 +++++++++++++++++++++++++++++----------------
 7 files changed, 197 insertions(+), 66 deletions(-)
 create mode 100644 drivers/scsi/mktbl.pl

-- 
1.8.5.6

[PATCHv4 1/5] scsi_debug: allow to specify inquiry vendor and model

[Hannes Reinecke]

For testing purposes we need to be able to pass in the inquiry
vendor and model.

Signed-off-by: Hannes Reinecke <hare@suse.com>
Reviewed-by: Bart Van Assche <bart.vanassche@wdc.com>
---
 drivers/scsi/scsi_debug.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 3be980d..698aa29 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -953,9 +953,9 @@ static int fetch_to_dev_buffer(struct scsi_cmnd *scp, unsigned char *arr,
 }
 
 
-static const char * inq_vendor_id = "Linux   ";
-static const char * inq_product_id = "scsi_debug      ";
-static const char *inq_product_rev = "0186";	/* version less '.' */
+static char sdebug_inq_vendor_id[9] = "Linux   ";
+static char sdebug_inq_product_id[17] = "scsi_debug      ";
+static char sdebug_inq_product_rev[5] = "0186";	/* version less '.' */
 /* Use some locally assigned NAAs for SAS addresses. */
 static const u64 naa3_comp_a = 0x3222222000000000ULL;
 static const u64 naa3_comp_b = 0x3333333000000000ULL;
@@ -975,8 +975,8 @@ static int inquiry_vpd_83(unsigned char *arr, int port_group_id,
 	arr[0] = 0x2;	/* ASCII */
 	arr[1] = 0x1;
 	arr[2] = 0x0;
-	memcpy(&arr[4], inq_vendor_id, 8);
-	memcpy(&arr[12], inq_product_id, 16);
+	memcpy(&arr[4], sdebug_inq_vendor_id, 8);
+	memcpy(&arr[12], sdebug_inq_product_id, 16);
 	memcpy(&arr[28], dev_id_str, dev_id_str_len);
 	num = 8 + 16 + dev_id_str_len;
 	arr[3] = num;
@@ -1408,9 +1408,9 @@ static int resp_inquiry(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
 	arr[6] = 0x10; /* claim: MultiP */
 	/* arr[6] |= 0x40; ... claim: EncServ (enclosure services) */
 	arr[7] = 0xa; /* claim: LINKED + CMDQUE */
-	memcpy(&arr[8], inq_vendor_id, 8);
-	memcpy(&arr[16], inq_product_id, 16);
-	memcpy(&arr[32], inq_product_rev, 4);
+	memcpy(&arr[8], sdebug_inq_vendor_id, 8);
+	memcpy(&arr[16], sdebug_inq_product_id, 16);
+	memcpy(&arr[32], sdebug_inq_product_rev, 4);
 	/* version descriptors (2 bytes each) follow */
 	put_unaligned_be16(0xc0, arr + 58);   /* SAM-6 no version claimed */
 	put_unaligned_be16(0x5c0, arr + 60);  /* SPC-5 no version claimed */
@@ -4152,6 +4152,12 @@ static int schedule_resp(struct scsi_cmnd *cmnd, struct sdebug_dev_info *devip,
 module_param_named(fake_rw, sdebug_fake_rw, int, S_IRUGO | S_IWUSR);
 module_param_named(guard, sdebug_guard, uint, S_IRUGO);
 module_param_named(host_lock, sdebug_host_lock, bool, S_IRUGO | S_IWUSR);
+module_param_string(inq_vendor, sdebug_inq_vendor_id,
+		    sizeof(sdebug_inq_vendor_id), S_IRUGO|S_IWUSR);
+module_param_string(inq_product, sdebug_inq_product_id,
+		    sizeof(sdebug_inq_product_id), S_IRUGO|S_IWUSR);
+module_param_string(inq_rev, sdebug_inq_product_rev,
+		    sizeof(sdebug_inq_product_rev), S_IRUGO|S_IWUSR);
 module_param_named(lbpu, sdebug_lbpu, int, S_IRUGO);
 module_param_named(lbpws, sdebug_lbpws, int, S_IRUGO);
 module_param_named(lbpws10, sdebug_lbpws10, int, S_IRUGO);
@@ -4203,6 +4209,9 @@ static int schedule_resp(struct scsi_cmnd *cmnd, struct sdebug_dev_info *devip,
 MODULE_PARM_DESC(fake_rw, "fake reads/writes instead of copying (def=0)");
 MODULE_PARM_DESC(guard, "protection checksum: 0=crc, 1=ip (def=0)");
 MODULE_PARM_DESC(host_lock, "host_lock is ignored (def=0)");
+MODULE_PARM_DESC(inq_vendor, "SCSI INQUIRY vendor string (def=\"Linux\")");
+MODULE_PARM_DESC(inq_product, "SCSI INQUIRY product string (def=\"scsi_debug\")");
+MODULE_PARM_DESC(inq_rev, "SCSI INQUIRY revision string (def=\"scsi_debug\")");
 MODULE_PARM_DESC(lbpu, "enable LBP, support UNMAP command (def=0)");
 MODULE_PARM_DESC(lbpws, "enable LBP, support WRITE SAME(16) with UNMAP bit (def=0)");
 MODULE_PARM_DESC(lbpws10, "enable LBP, support WRITE SAME(10) with UNMAP bit (def=0)");
-- 
1.8.5.6

[PATCHv4 2/5] scsi: Export blacklist flags to sysfs

[Hannes Reinecke]

Each scsi device is scanned according to the found blacklist flags,
but this information is never presented to sysfs.
This makes it quite hard to figure out if blacklisting worked as
expected.
With this patch we're exporting an additional attribute 'blacklist'
containing the blacklist flags for this device.

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/Makefile     |  8 +++++++
 drivers/scsi/mktbl.pl     | 26 ++++++++++++++++++++
 drivers/scsi/scsi_scan.c  |  1 +
 drivers/scsi/scsi_sysfs.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 95 insertions(+)
 create mode 100644 drivers/scsi/mktbl.pl

diff --git a/drivers/scsi/Makefile b/drivers/scsi/Makefile
index 93dbe58..b25d094 100644
--- a/drivers/scsi/Makefile
+++ b/drivers/scsi/Makefile
@@ -191,6 +191,14 @@ clean-files :=	53c700_d.h 53c700_u.h
 
 $(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 = $(PERL) -s $(src)/mktbl.pl BLIST $< > $@
+
+$(obj)/scsi_devinfo_tbl.c: include/scsi/scsi_devinfo.h
+	$(call if_changed,bflags)
+
 # If you want to play with the firmware, uncomment
 # GENERATE_FIRMWARE := 1
 
diff --git a/drivers/scsi/mktbl.pl b/drivers/scsi/mktbl.pl
new file mode 100644
index 0000000..0dc5cdb
--- /dev/null
+++ b/drivers/scsi/mktbl.pl
@@ -0,0 +1,26 @@
+#!/usr/bin/perl
+#
+# Build a mapping table for C definitions.
+#
+# Copyright (c) 2017 Hannes Reinecke, SUSE Linux GmbH.
+# All rights reserved.
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public Licence
+# as published by the Free Software Foundation; either version
+# 2 of the Licence, or (at your option) any later version.
+#
+
+my $prf;
+
+$prf = $ARGV[0];
+open IN_FILE, "<$ARGV[1]" || die;
+print "\t/*\n\t * Automatically generated by ", $0, ".\n";
+print "\t * Do not edit.\n\t */\n";
+while (<IN_FILE>) {
+    chomp;
+    if (/^#define ${prf}_([^[:blank:]]*).*/) {
+	print "\t{ ", $prf, "_", $1, ", \"", $1, "\" },\n";
+    }
+}
+close IN_FILE || die;
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index fd88dab..075918b 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -984,6 +984,7 @@ static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result,
 		scsi_attach_vpd(sdev);
 
 	sdev->max_queue_depth = sdev->queue_depth;
+	sdev->sdev_bflags = *bflags;
 
 	/*
 	 * Ok, the device is now all set up, we can
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index e9b78db..dbda738 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -20,6 +20,7 @@
 #include <scsi/scsi_dh.h>
 #include <scsi/scsi_transport.h>
 #include <scsi/scsi_driver.h>
+#include <scsi/scsi_devinfo.h>
 
 #include "scsi_priv.h"
 #include "scsi_logging.h"
@@ -110,6 +111,27 @@ static const char *scsi_access_state_name(unsigned char state)
 }
 #endif
 
+static const struct {
+	unsigned int	value;
+	char		*name;
+} sdev_bflags[] = {
+#include "scsi_devinfo_tbl.c"
+};
+
+static const char *sdev_bflags_name(unsigned int bflags)
+{
+	int i;
+	const char *name = NULL;
+
+	for (i = 0; i < ARRAY_SIZE(sdev_bflags); i++) {
+		if (sdev_bflags[i].value == bflags) {
+			name = sdev_bflags[i].name;
+			break;
+		}
+	}
+	return name;
+}
+
 static int check_set(unsigned long long *val, char *src)
 {
 	char *last;
@@ -955,6 +977,43 @@ static DEVICE_ATTR(queue_depth, S_IRUGO | S_IWUSR, sdev_show_queue_depth,
 }
 static DEVICE_ATTR(wwid, S_IRUGO, sdev_show_wwid, NULL);
 
+static ssize_t
+sdev_show_blacklist(struct device *dev, struct device_attribute *attr,
+		    char *buf)
+{
+	struct scsi_device *sdev = to_scsi_device(dev);
+	int i;
+	char *ptr = buf;
+	ssize_t len = 0;
+
+	for (i = 0; i < sizeof(unsigned int) * 8; i++) {
+		unsigned int bflags = (unsigned int)1 << i;
+		ssize_t blen;
+		const char *name = NULL;
+
+		if (!(bflags & sdev->sdev_bflags))
+			continue;
+
+		if (ptr != buf) {
+			blen = snprintf(ptr, 2, " ");
+			ptr += blen;
+			len += blen;
+		}
+		name = sdev_bflags_name(bflags);
+		if (name)
+			blen = snprintf(ptr, strlen(name) + 1,
+					"%s", name);
+		else
+			blen = snprintf(ptr, 67, "INVALID_BIT(%d)", i);
+		ptr += blen;
+		len += blen;
+	}
+	if (len)
+		len += snprintf(ptr, 2, "\n");
+	return len;
+}
+static DEVICE_ATTR(blacklist, S_IRUGO, sdev_show_blacklist, NULL);
+
 #ifdef CONFIG_SCSI_DH
 static ssize_t
 sdev_show_dh_state(struct device *dev, struct device_attribute *attr,
@@ -1140,6 +1199,7 @@ static umode_t scsi_sdev_bin_attr_is_visible(struct kobject *kobj,
 	&dev_attr_queue_depth.attr,
 	&dev_attr_queue_type.attr,
 	&dev_attr_wwid.attr,
+	&dev_attr_blacklist.attr,
 #ifdef CONFIG_SCSI_DH
 	&dev_attr_dh_state.attr,
 	&dev_attr_access_state.attr,
-- 
1.8.5.6

Re: [PATCHv4 2/5] scsi: Export blacklist flags to sysfs

[Bart Van Assche]

On Tue, 2017-08-29 at 10:49 +0200, Hannes Reinecke wrote:
> [ ... ]
> +$(obj)/scsi_sysfs.o: $(obj)/scsi_devinfo_tbl.c
> +
> +quiet_cmd_bflags = GEN     $@
> +	cmd_bflags = $(PERL) -s $(src)/mktbl.pl BLIST $< > $@

Hello Hannes,

Is it considered acceptable to require that perl is available to build the
kernel? People who build the kernel for embedded systems probably won't be
happy if this is a new requirement. See e.g. "PATCH [0/3]: Simplify the
kernel build by removing perl" (https://lkml.org/lkml/2009/1/2/20).

Have you noticed that for other generated files a _shipped version is
provided?

> +
> +$(obj)/scsi_devinfo_tbl.c: include/scsi/scsi_devinfo.h
> +	$(call if_changed,bflags)
> +
>  # If you want to play with the firmware, uncomment
>  # GENERATE_FIRMWARE := 1
>  
> diff --git a/drivers/scsi/mktbl.pl b/drivers/scsi/mktbl.pl
> [ ... ]
> +my $prf;
> +
> +$prf = $ARGV[0];
> +open IN_FILE, "<$ARGV[1]" || die;
> +print "\t/*\n\t * Automatically generated by ", $0, ".\n";
> +print "\t * Do not edit.\n\t */\n";
> +while (<IN_FILE>) {
> +    chomp;
> +    if (/^#define ${prf}_([^[:blank:]]*).*/) {
> +	print "\t{ ", $prf, "_", $1, ", \"", $1, "\" },\n";
> +    }
> +}
> +close IN_FILE || die;

Can this be done with sed? Do we really need Perl for this?

> [ ... ]
> +static const struct {
> +	unsigned int	value;
> +	char		*name;

Can 'char *' be changed into 'const char *'?

> +} sdev_bflags[] = {
> +#include "scsi_devinfo_tbl.c"
> +};
> +
> +static const char *sdev_bflags_name(unsigned int bflags)
> +{
> +	int i;
> +	const char *name = NULL;
> +
> +	for (i = 0; i < ARRAY_SIZE(sdev_bflags); i++) {
> +		if (sdev_bflags[i].value == bflags) {
> +			name = sdev_bflags[i].name;
> +			break;
> +		}
> +	}
> +	return name;
> +}

How about using ilog2() of the BLIST_* values as index of the table such that
an array lookup can be used instead of a for-loop over the entire array?

> +
>  static int check_set(unsigned long long *val, char *src)
>  {
>  	char *last;
> @@ -955,6 +977,43 @@ static DEVICE_ATTR(queue_depth, S_IRUGO | S_IWUSR, sdev_show_queue_depth,
>  }
>  static DEVICE_ATTR(wwid, S_IRUGO, sdev_show_wwid, NULL);
>  
> +static ssize_t
> +sdev_show_blacklist(struct device *dev, struct device_attribute *attr,
> +		    char *buf)
> +{
> +	struct scsi_device *sdev = to_scsi_device(dev);
> +	int i;
> +	char *ptr = buf;
> +	ssize_t len = 0;
> +
> +	for (i = 0; i < sizeof(unsigned int) * 8; i++) {
> +		unsigned int bflags = (unsigned int)1 << i;

How about using 1U instead of (unsigned int)1?

> +		ssize_t blen;
> +		const char *name = NULL;
> +
> +		if (!(bflags & sdev->sdev_bflags))
> +			continue;
> +
> +		if (ptr != buf) {
> +			blen = snprintf(ptr, 2, " ");
> +			ptr += blen;
> +			len += blen;
> +		}

Should this code check whether or not it overflows the output buffer @buf?

> +		name = sdev_bflags_name(bflags);
> +		if (name)
> +			blen = snprintf(ptr, strlen(name) + 1,
> +					"%s", name);
> +		else
> +			blen = snprintf(ptr, 67, "INVALID_BIT(%d)", i);
> +		ptr += blen;
> +		len += blen;
> +	}

Should this code check too whether or not it overflows the output buffer @buf?

Thanks,

Bart.

Re: [PATCHv4 2/5] scsi: Export blacklist flags to sysfs

[Hannes Reinecke]

On 08/29/2017 06:02 PM, Bart Van Assche wrote:
> On Tue, 2017-08-29 at 10:49 +0200, Hannes Reinecke wrote:
>> [ ... ]
>> +$(obj)/scsi_sysfs.o: $(obj)/scsi_devinfo_tbl.c
>> +
>> +quiet_cmd_bflags = GEN     $@
>> +	cmd_bflags = $(PERL) -s $(src)/mktbl.pl BLIST $< > $@
> 
> Hello Hannes,
> 
> Is it considered acceptable to require that perl is available to build the
> kernel? People who build the kernel for embedded systems probably won't be
> happy if this is a new requirement. See e.g. "PATCH [0/3]: Simplify the
> kernel build by removing perl" (https://lkml.org/lkml/2009/1/2/20).
> 
> Have you noticed that for other generated files a _shipped version is
> provided?
> 
Ah. Good point. Will be doing so.

>> +
>> +$(obj)/scsi_devinfo_tbl.c: include/scsi/scsi_devinfo.h
>> +	$(call if_changed,bflags)
>> +
>>  # If you want to play with the firmware, uncomment
>>  # GENERATE_FIRMWARE := 1
>>  
>> diff --git a/drivers/scsi/mktbl.pl b/drivers/scsi/mktbl.pl
>> [ ... ]
>> +my $prf;
>> +
>> +$prf = $ARGV[0];
>> +open IN_FILE, "<$ARGV[1]" || die;
>> +print "\t/*\n\t * Automatically generated by ", $0, ".\n";
>> +print "\t * Do not edit.\n\t */\n";
>> +while (<IN_FILE>) {
>> +    chomp;
>> +    if (/^#define ${prf}_([^[:blank:]]*).*/) {
>> +	print "\t{ ", $prf, "_", $1, ", \"", $1, "\" },\n";
>> +    }
>> +}
>> +close IN_FILE || die;
> 
> Can this be done with sed? Do we really need Perl for this?
> 
In principle, sure. I just failed to stuff everything into a makefile
line, so perl was easier to code.

>> [ ... ]
>> +static const struct {
>> +	unsigned int	value;
>> +	char		*name;
> 
> Can 'char *' be changed into 'const char *'?
> 
Sure; why not.

>> +} sdev_bflags[] = {
>> +#include "scsi_devinfo_tbl.c"
>> +};
>> +
>> +static const char *sdev_bflags_name(unsigned int bflags)
>> +{
>> +	int i;
>> +	const char *name = NULL;
>> +
>> +	for (i = 0; i < ARRAY_SIZE(sdev_bflags); i++) {
>> +		if (sdev_bflags[i].value == bflags) {
>> +			name = sdev_bflags[i].name;
>> +			break;
>> +		}
>> +	}
>> +	return name;
>> +}
> 
> How about using ilog2() of the BLIST_* values as index of the table such that
> an array lookup can be used instead of a for-loop over the entire array?
> 
Not sure if that'll work. The BLIST_* values in fact form a sparse
array, so the lookup array 'sdev_bflags' actually an associative array.
And I dare not convert it to a normal array as then I couldn't to the
automatice table creation anymore.

So I'm not sure if ilog2 will save me anything here.

>> +
>>  static int check_set(unsigned long long *val, char *src)
>>  {
>>  	char *last;
>> @@ -955,6 +977,43 @@ static DEVICE_ATTR(queue_depth, S_IRUGO | S_IWUSR, sdev_show_queue_depth,
>>  }
>>  static DEVICE_ATTR(wwid, S_IRUGO, sdev_show_wwid, NULL);
>>  
>> +static ssize_t
>> +sdev_show_blacklist(struct device *dev, struct device_attribute *attr,
>> +		    char *buf)
>> +{
>> +	struct scsi_device *sdev = to_scsi_device(dev);
>> +	int i;
>> +	char *ptr = buf;
>> +	ssize_t len = 0;
>> +
>> +	for (i = 0; i < sizeof(unsigned int) * 8; i++) {
>> +		unsigned int bflags = (unsigned int)1 << i;
> 
> How about using 1U instead of (unsigned int)1?
> 
Yeah, ok.

>> +		ssize_t blen;
>> +		const char *name = NULL;
>> +
>> +		if (!(bflags & sdev->sdev_bflags))
>> +			continue;
>> +
>> +		if (ptr != buf) {
>> +			blen = snprintf(ptr, 2, " ");
>> +			ptr += blen;
>> +			len += blen;
>> +		}
> 
> Should this code check whether or not it overflows the output buffer @buf?
> 
Hmm. In principle, yes.
However, as we're only have a limited number of possible arguments and
hence a fixed upper limit of what we can print into the buffer I'm not
sure if that's required.
But I'll check.

>> +		name = sdev_bflags_name(bflags);
>> +		if (name)
>> +			blen = snprintf(ptr, strlen(name) + 1,
>> +					"%s", name);
>> +		else
>> +			blen = snprintf(ptr, 67, "INVALID_BIT(%d)", i);
>> +		ptr += blen;
>> +		len += blen;
>> +	}
> 
> Should this code check too whether or not it overflows the output buffer @buf?
> 
See above.

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: [PATCHv4 2/5] scsi: Export blacklist flags to sysfs

[Bart Van Assche]

On Wed, 2017-08-30 at 12:50 +0200, Hannes Reinecke wrote:
> On 08/29/2017 06:02 PM, Bart Van Assche wrote:
> > On Tue, 2017-08-29 at 10:49 +0200, Hannes Reinecke wrote:
> > > +} sdev_bflags[] = {
> > > +#include "scsi_devinfo_tbl.c"
> > > +};
> > > +
> > > +static const char *sdev_bflags_name(unsigned int bflags)
> > > +{
> > > +	int i;
> > > +	const char *name = NULL;
> > > +
> > > +	for (i = 0; i < ARRAY_SIZE(sdev_bflags); i++) {
> > > +		if (sdev_bflags[i].value == bflags) {
> > > +			name = sdev_bflags[i].name;
> > > +			break;
> > > +		}
> > > +	}
> > > +	return name;
> > > +}
> > 
> > How about using ilog2() of the BLIST_* values as index of the table such that
> > an array lookup can be used instead of a for-loop over the entire array?
> 
> Not sure if that'll work. The BLIST_* values in fact form a sparse
> array, so the lookup array 'sdev_bflags' actually an associative array.
> And I dare not convert it to a normal array as then I couldn't to the
> automatice table creation anymore.
> 
> So I'm not sure if ilog2 will save me anything here.

Hello Hannes,

Are you aware that there is already code upstream for converting a bitmask
into a list of names of flags that supports sparse bit definitions? From
block/blk-mq-debugfs.c:

#define HCTX_FLAG_NAME(name) [ilog2(BLK_MQ_F_##name)] = #name
static const char *const hctx_flag_name[] = {
	HCTX_FLAG_NAME(SHOULD_MERGE),
	HCTX_FLAG_NAME(TAG_SHARED),
	HCTX_FLAG_NAME(SG_MERGE),
	HCTX_FLAG_NAME(BLOCKING),
	HCTX_FLAG_NAME(NO_SCHED),
};
#undef HCTX_FLAG_NAME

static int blk_flags_show(struct seq_file *m, const unsigned long flags,
			  const char *const *flag_name, int flag_name_count)
{
	bool sep = false;
	int i;

	for (i = 0; i < sizeof(flags) * BITS_PER_BYTE; i++) {
		if (!(flags & BIT(i)))
			continue;
		if (sep)
			seq_puts(m, "|");
		sep = true;
		if (i < flag_name_count && flag_name[i])
			seq_puts(m, flag_name[i]);
		else
			seq_printf(m, "%d", i);
	}
	return 0;
}

[ ... ]
	blk_flags_show(m, hctx->flags ^ BLK_ALLOC_POLICY_TO_MQ_FLAG(alloc_policy),
		       hctx_flag_name, ARRAY_SIZE(hctx_flag_name));
[ ... ]

Bart.

Re: [PATCHv4 2/5] scsi: Export blacklist flags to sysfs

[kbuild test robot]

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

Hi Hannes,

[auto build test ERROR on mkp-scsi/for-next]
[also build test ERROR on v4.13-rc7 next-20170829]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Hannes-Reinecke/Fixup-blacklist-handling/20170830-065524
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next
config: i386-randconfig-n0-201735 (attached as .config)
compiler: gcc-4.8 (Debian 4.8.4-1) 4.8.4
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

>> Can't open perl script "drivers/scsi/mktbl.pl": No such file or directory

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 33185 bytes --]

[PATCHv4 3/5] scsi_devinfo: Reformat blacklist flags

[Hannes Reinecke]

Reformat blacklist flags to make the values easier to read and
to enhance error checking.

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

diff --git a/include/scsi/scsi_devinfo.h b/include/scsi/scsi_devinfo.h
index 9592570..7a2329c 100644
--- a/include/scsi/scsi_devinfo.h
+++ b/include/scsi/scsi_devinfo.h
@@ -3,31 +3,55 @@
 /*
  * Flags for SCSI devices that need special treatment
  */
-#define BLIST_NOLUN     	0x001	/* Only scan LUN 0 */
-#define BLIST_FORCELUN  	0x002	/* Known to have LUNs, force scanning,
-					   deprecated: Use max_luns=N */
-#define BLIST_BORKEN    	0x004	/* Flag for broken handshaking */
-#define BLIST_KEY       	0x008	/* unlock by special command */
-#define BLIST_SINGLELUN 	0x010	/* Do not use LUNs in parallel */
-#define BLIST_NOTQ		0x020	/* Buggy Tagged Command Queuing */
-#define BLIST_SPARSELUN 	0x040	/* Non consecutive LUN numbering */
-#define BLIST_MAX5LUN		0x080	/* Avoid LUNS >= 5 */
-#define BLIST_ISROM     	0x100	/* Treat as (removable) CD-ROM */
-#define BLIST_LARGELUN		0x200	/* LUNs past 7 on a SCSI-2 device */
-#define BLIST_INQUIRY_36	0x400	/* override additional length field */
-#define BLIST_NOSTARTONADD	0x1000	/* do not do automatic start on add */
-#define BLIST_REPORTLUN2	0x20000	/* try REPORT_LUNS even for SCSI-2 devs
- 					   (if HBA supports more than 8 LUNs) */
-#define BLIST_NOREPORTLUN	0x40000	/* don't try REPORT_LUNS scan (SCSI-3 devs) */
-#define BLIST_NOT_LOCKABLE	0x80000	/* don't use PREVENT-ALLOW commands */
-#define BLIST_NO_ULD_ATTACH	0x100000 /* device is actually for RAID config */
-#define BLIST_SELECT_NO_ATN	0x200000 /* select without ATN */
-#define BLIST_RETRY_HWERROR	0x400000 /* retry HARDWARE_ERROR */
-#define BLIST_MAX_512		0x800000 /* maximum 512 sector cdb length */
-#define BLIST_NO_DIF		0x2000000 /* Disable T10 PI (DIF) */
-#define BLIST_SKIP_VPD_PAGES	0x4000000 /* Ignore SBC-3 VPD pages */
-#define BLIST_TRY_VPD_PAGES	0x10000000 /* Attempt to read VPD pages */
-#define BLIST_NO_RSOC		0x20000000 /* don't try to issue RSOC */
-#define BLIST_MAX_1024		0x40000000 /* maximum 1024 sector cdb length */
+
+/* Only scan LUN 0 */
+#define BLIST_NOLUN		((__force __u32 __bitwise)(1 << 0))
+/* Known to have LUNs, force scanning.
+ * DEPRECATED: Use max_luns=N */
+#define BLIST_FORCELUN		((__force __u32 __bitwise)(1 << 1))
+/* Flag for broken handshaking */
+#define BLIST_BORKEN		((__force __u32 __bitwise)(1 << 2))
+/* unlock by special command */
+#define BLIST_KEY		((__force __u32 __bitwise)(1 << 3))
+/* Do not use LUNs in parallel */
+#define BLIST_SINGLELUN		((__force __u32 __bitwise)(1 << 4))
+/* Buggy Tagged Command Queuing */
+#define BLIST_NOTQ		((__force __u32 __bitwise)(1 << 5))
+/* Non consecutive LUN numbering */
+#define BLIST_SPARSELUN		((__force __u32 __bitwise)(1 << 6))
+/* Avoid LUNS >= 5 */
+#define BLIST_MAX5LUN		((__force __u32 __bitwise)(1 << 7))
+/* Treat as (removable) CD-ROM */
+#define BLIST_ISROM		((__force __u32 __bitwise)(1 << 8))
+/* LUNs past 7 on a SCSI-2 device */
+#define BLIST_LARGELUN		((__force __u32 __bitwise)(1 << 9))
+/* override additional length field */
+#define BLIST_INQUIRY_36	((__force __u32 __bitwise)(1 << 10))
+/* do not do automatic start on add */
+#define BLIST_NOSTARTONADD	((__force __u32 __bitwise)(1 << 12))
+/* try REPORT_LUNS even for SCSI-2 devs (if HBA supports more than 8 LUNs) */
+#define BLIST_REPORTLUN2	((__force __u32 __bitwise)(1 << 17))
+/* don't try REPORT_LUNS scan (SCSI-3 devs) */
+#define BLIST_NOREPORTLUN	((__force __u32 __bitwise)(1 << 18))
+/* don't use PREVENT-ALLOW commands */
+#define BLIST_NOT_LOCKABLE	((__force __u32 __bitwise)(1 << 19))
+/* device is actually for RAID config */
+#define BLIST_NO_ULD_ATTACH	((__force __u32 __bitwise)(1 << 20))
+/* select without ATN */
+#define BLIST_SELECT_NO_ATN	((__force __u32 __bitwise)(1 << 21))
+/* retry HARDWARE_ERROR */
+#define BLIST_RETRY_HWERROR	((__force __u32 __bitwise)(1 << 22))
+/* maximum 512 sector cdb length */
+#define BLIST_MAX_512		((__force __u32 __bitwise)(1 << 23))
+/* Disable T10 PI (DIF) */
+#define BLIST_NO_DIF		((__force __u32 __bitwise)(1 << 25))
+/* Ignore SBC-3 VPD pages */
+#define BLIST_SKIP_VPD_PAGES	((__force __u32 __bitwise)(1 << 26))
+/* Attempt to read VPD pages */
+#define BLIST_TRY_VPD_PAGES	((__force __u32 __bitwise)(1 << 28))
+/* don't try to issue RSOC */
+#define BLIST_NO_RSOC		((__force __u32 __bitwise)(1 << 29))
+/* maximum 1024 sector cdb length */
+#define BLIST_MAX_1024		((__force __u32 __bitwise)(1 << 30))
 
 #endif
-- 
1.8.5.6

Re: [PATCHv4 3/5] scsi_devinfo: Reformat blacklist flags

[Bart Van Assche]

On Tue, 2017-08-29 at 10:49 +0200, Hannes Reinecke wrote:
> Reformat blacklist flags to make the values easier to read and
> to enhance error checking.

Reviewed-by: Bart Van Assche <bart.vanassche@wdc.com>

[PATCHv4 4/5] scsi_devinfo: Whitespace fixes

[Hannes Reinecke]

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/scsi_devinfo.c | 44 ++++++++++++++++++++++----------------------
 1 file changed, 22 insertions(+), 22 deletions(-)

diff --git a/drivers/scsi/scsi_devinfo.c b/drivers/scsi/scsi_devinfo.c
index 28fea83..6858ad8 100644
--- a/drivers/scsi/scsi_devinfo.c
+++ b/drivers/scsi/scsi_devinfo.c
@@ -304,8 +304,8 @@ static void scsi_strcpy_devinfo(char *name, char *to, size_t to_length,
 			 */
 			to[from_length] = '\0';
 		} else {
-			/* 
-			 * space pad the string if it is short. 
+			/*
+			 * space pad the string if it is short.
 			 */
 			strncpy(&to[from_length], spaces,
 				to_length - from_length);
@@ -325,10 +325,10 @@ static void scsi_strcpy_devinfo(char *name, char *to, size_t to_length,
  * @flags:	if strflags NULL, use this flag value
  *
  * Description:
- * 	Create and add one dev_info entry for @vendor, @model, @strflags or
- * 	@flag. If @compatible, add to the tail of the list, do not space
- * 	pad, and set devinfo->compatible. The scsi_static_device_list entries
- * 	are added with @compatible 1 and @clfags NULL.
+ *	Create and add one dev_info entry for @vendor, @model, @strflags or
+ *	@flag. If @compatible, add to the tail of the list, do not space
+ *	pad, and set devinfo->compatible. The scsi_static_device_list entries
+ *	are added with @compatible 1 and @clfags NULL.
  *
  * Returns: 0 OK, -error on failure.
  **/
@@ -350,11 +350,11 @@ static int scsi_dev_info_list_add(int compatible, char *vendor, char *model,
  * @key:	specify list to use
  *
  * Description:
- * 	Create and add one dev_info entry for @vendor, @model,
- * 	@strflags or @flag in list specified by @key. If @compatible,
- * 	add to the tail of the list, do not space pad, and set
- * 	devinfo->compatible. The scsi_static_device_list entries are
- * 	added with @compatible 1 and @clfags NULL.
+ *	Create and add one dev_info entry for @vendor, @model,
+ *	@strflags or @flag in list specified by @key. If @compatible,
+ *	add to the tail of the list, do not space pad, and set
+ *	devinfo->compatible. The scsi_static_device_list entries are
+ *	added with @compatible 1 and @clfags NULL.
  *
  * Returns: 0 OK, -error on failure.
  **/
@@ -405,7 +405,7 @@ int scsi_dev_info_list_add_keyed(int compatible, char *vendor, char *model,
  *
  * Description:
  *	Finds the first dev_info entry matching @vendor, @model
- * 	in list specified by @key.
+ *	in list specified by @key.
  *
  * Returns: pointer to matching entry, or ERR_PTR on failure.
  **/
@@ -467,9 +467,9 @@ static struct scsi_dev_info_list *scsi_dev_info_list_find(const char *vendor,
 			return devinfo;
 		} else {
 			if (!memcmp(devinfo->vendor, vendor,
-				     sizeof(devinfo->vendor)) &&
-			     !memcmp(devinfo->model, model,
-				      sizeof(devinfo->model)))
+				    sizeof(devinfo->vendor)) &&
+			    !memcmp(devinfo->model, model,
+				    sizeof(devinfo->model)))
 				return devinfo;
 		}
 	}
@@ -508,10 +508,10 @@ int scsi_dev_info_list_del_keyed(char *vendor, char *model, int key)
  * @dev_list:	string of device flags to add
  *
  * Description:
- * 	Parse dev_list, and add entries to the scsi_dev_info_list.
- * 	dev_list is of the form "vendor:product:flag,vendor:product:flag".
- * 	dev_list is modified via strsep. Can be called for command line
- * 	addition, for proc or mabye a sysfs interface.
+ *	Parse dev_list, and add entries to the scsi_dev_info_list.
+ *	dev_list is of the form "vendor:product:flag,vendor:product:flag".
+ *	dev_list is modified via strsep. Can be called for command line
+ *	addition, for proc or mabye a sysfs interface.
  *
  * Returns: 0 if OK, -error on failure.
  **/
@@ -701,7 +701,7 @@ static int proc_scsi_devinfo_open(struct inode *inode, struct file *file)
 	return seq_open(file, &scsi_devinfo_seq_ops);
 }
 
-/* 
+/*
  * proc_scsi_dev_info_write - allow additions to scsi_dev_info_list via /proc.
  *
  * Description: Adds a black/white list entry for vendor and model with an
@@ -840,8 +840,8 @@ int scsi_dev_info_remove_list(int key)
  * scsi_init_devinfo - set up the dynamic device list.
  *
  * Description:
- * 	Add command line entries from scsi_dev_flags, then add
- * 	scsi_static_device_list entries to the scsi device info list.
+ *	Add command line entries from scsi_dev_flags, then add
+ *	scsi_static_device_list entries to the scsi device info list.
  */
 int __init scsi_init_devinfo(void)
 {
-- 
1.8.5.6

[PATCHv4 5/5] scsi_devinfo: fixup string compare

[Hannes Reinecke]

When checking the model and vendor string we need to use the
minimum value of either string, otherwise we'll miss out on
wildcard matches.
And we should take card when matching with zero size strings;
results might be unpredictable.
With this patch the rules for matching devinfo strings are
as follows:
- Vendor strings must match exactly
- Empty Model strings will only match if the devinfo model
  is also empty
- Model strings shorter than the devinfo model string will
  not match

Fixes: 5e7ff2c ("SCSI: fix new bug in scsi_dev_info_list string matching")
Signed-off-by: Hannes Reinecke <hare@suse.com>
Reviewed-by: Alan Stern <stern@rowland.harvard.edu>
---
 drivers/scsi/scsi_devinfo.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/scsi/scsi_devinfo.c b/drivers/scsi/scsi_devinfo.c
index 6858ad8..d39b27c 100644
--- a/drivers/scsi/scsi_devinfo.c
+++ b/drivers/scsi/scsi_devinfo.c
@@ -399,8 +399,8 @@ int scsi_dev_info_list_add_keyed(int compatible, char *vendor, char *model,
 
 /**
  * scsi_dev_info_list_find - find a matching dev_info list entry.
- * @vendor:	vendor string
- * @model:	model (product) string
+ * @vendor:	full vendor string
+ * @model:	full model (product) string
  * @key:	specify list to use
  *
  * Description:
@@ -415,7 +415,7 @@ static struct scsi_dev_info_list *scsi_dev_info_list_find(const char *vendor,
 	struct scsi_dev_info_list *devinfo;
 	struct scsi_dev_info_list_table *devinfo_table =
 		scsi_devinfo_lookup_by_key(key);
-	size_t vmax, mmax;
+	size_t vmax, mmax, mlen;
 	const char *vskip, *mskip;
 
 	if (IS_ERR(devinfo_table))
@@ -454,15 +454,18 @@ static struct scsi_dev_info_list *scsi_dev_info_list_find(const char *vendor,
 			    dev_info_list) {
 		if (devinfo->compatible) {
 			/*
-			 * Behave like the older version of get_device_flags.
+			 * vendor strings must be an exact match
 			 */
-			if (memcmp(devinfo->vendor, vskip, vmax) ||
-					(vmax < sizeof(devinfo->vendor) &&
-						devinfo->vendor[vmax]))
+			if (vmax != strlen(devinfo->vendor) ||
+			    memcmp(devinfo->vendor, vskip, vmax))
 				continue;
-			if (memcmp(devinfo->model, mskip, mmax) ||
-					(mmax < sizeof(devinfo->model) &&
-						devinfo->model[mmax]))
+
+			/*
+			 * @model specifies the full string, and
+			 * must be larger or equal to devinfo->model
+			 */
+			mlen = strlen(devinfo->model);
+			if (mmax < mlen || memcmp(devinfo->model, mskip, mlen))
 				continue;
 			return devinfo;
 		} else {
-- 
1.8.5.6

Re: [PATCHv4 5/5] scsi_devinfo: fixup string compare

[Bart Van Assche]

On Tue, 2017-08-29 at 10:49 +0200, Hannes Reinecke wrote:
> When checking the model and vendor string we need to use the
> minimum value of either string, otherwise we'll miss out on
> wildcard matches.
> And we should take card when matching with zero size strings;
> results might be unpredictable.
> With this patch the rules for matching devinfo strings are
> as follows:
> - Vendor strings must match exactly
> - Empty Model strings will only match if the devinfo model
>   is also empty
> - Model strings shorter than the devinfo model string will
>   not match

Reviewed-by: Bart Van Assche <bart.vanassche@wdc.com>