[PATCHv3 0/5] Hi all,

[PATCHv3 0/5] Hi all,

[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

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

 drivers/scsi/scsi_debug.c   | 25 +++++++++-----
 drivers/scsi/scsi_devinfo.c | 67 +++++++++++++++++++-----------------
 drivers/scsi/scsi_scan.c    |  1 +
 drivers/scsi/scsi_sysfs.c   | 83 +++++++++++++++++++++++++++++++++++++++++++++
 include/scsi/scsi_devinfo.h | 77 +++++++++++++++++++++++++++--------------
 5 files changed, 187 insertions(+), 66 deletions(-)

-- 
1.8.5.6

[PATCHv3 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>
---
 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 dc095a2..c8a3f8d 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

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

[Bart Van Assche]

On Fri, 2017-08-11 at 16:23 +0200, Hannes Reinecke wrote:
> For testing purposes we need to be able to pass in the inquiry
> vendor and model.

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

[PATCHv3 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/scsi_scan.c  |  1 +
 drivers/scsi/scsi_sysfs.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 84 insertions(+)

diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 3832ba5..e4e4374 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 5e8ace2..9adec62 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,50 @@ static const char *scsi_access_state_name(unsigned char state)
 }
 #endif
 
+static const struct {
+	unsigned int	value;
+	char		*name;
+} sdev_bflags[] = {
+	{ BLIST_NOLUN, "NOLUN" },
+	{ BLIST_FORCELUN, "FORCELUN" },
+	{ BLIST_BORKEN, "BORKEN" },
+	{ BLIST_KEY, "KEY" },
+	{ BLIST_SINGLELUN, "SINGLELUN" },
+	{ BLIST_NOTQ, "NOTQ" },
+	{ BLIST_SPARSELUN, "SPARSELUN" },
+	{ BLIST_MAX5LUN, "MAX5LUN" },
+	{ BLIST_ISROM, "ISROM" },
+	{ BLIST_LARGELUN, "LARGELUN" },
+	{ BLIST_INQUIRY_36, "INQUIRY_36" },
+	{ BLIST_NOSTARTONADD, "NOSTARTONADD" },
+	{ BLIST_REPORTLUN2, "REPORTLUN2" },
+	{ BLIST_NOREPORTLUN, "NOREPORTLUN" },
+	{ BLIST_NOT_LOCKABLE, "NOT_LOCKABLE" },
+	{ BLIST_NO_ULD_ATTACH, "NO_ULD_ATTACH" },
+	{ BLIST_SELECT_NO_ATN, "SELECT_NO_ATN" },
+	{ BLIST_RETRY_HWERROR, "RETRY_HWERROR" },
+	{ BLIST_MAX_512, "MAX_512" },
+	{ BLIST_NO_DIF, "NO_DIF" },
+	{ BLIST_SKIP_VPD_PAGES, "SKIP_VPD_PAGES" },
+	{ BLIST_TRY_VPD_PAGES, "TRY_VPD_PAGES" },
+	{ BLIST_NO_RSOC, "NO_RSOC" },
+	{ BLIST_MAX_1024, "MAX_1024" },
+};
+
+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;
@@ -953,6 +998,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, "0x%X", bflags);
+		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,
@@ -1138,6 +1220,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: [PATCHv3 2/5] scsi: Export blacklist flags to sysfs

[Martin K. Petersen]

Hannes,

> +		name = sdev_bflags_name(bflags);
> +		if (name)
> +			blen = snprintf(ptr, strlen(name) + 1,
> +					"%s", name);
> +		else
> +			blen = snprintf(ptr, 67, "0x%X", bflags);

It seems this else statement facilitates papering over the fact that
scsi_sysfs.c and scsi_devinfo.h can get out of sync.

-- 
Martin K. Petersen	Oracle Linux Engineering

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

[Hannes Reinecke]

On 08/15/2017 05:18 AM, Martin K. Petersen wrote:
> 
> Hannes,
> 
>> +		name = sdev_bflags_name(bflags);
>> +		if (name)
>> +			blen = snprintf(ptr, strlen(name) + 1,
>> +					"%s", name);
>> +		else
>> +			blen = snprintf(ptr, 67, "0x%X", bflags);
> 
> It seems this else statement facilitates papering over the fact that
> scsi_sysfs.c and scsi_devinfo.h can get out of sync.
> 
But there is no good way of avoiding that, is there?
Out of necessity the definition of the bits and the decoding are two
distinct steps, and as such are always prone to differences.

So what is your suggestion here?

Having the 'blacklist' sysfs entry gives us a nice way of figuring out
if the selection algorithm in scsi_devinfo.c works as designed.
Without it it's quite hard to validate this...

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

[Bart Van Assche]

On Tue, 2017-08-15 at 10:03 +0200, Hannes Reinecke wrote:
> On 08/15/2017 05:18 AM, Martin K. Petersen wrote:
> > 
> > Hannes,
> > 
> > > +		name = sdev_bflags_name(bflags);
> > > +		if (name)
> > > +			blen = snprintf(ptr, strlen(name) + 1,
> > > +					"%s", name);
> > > +		else
> > > +			blen = snprintf(ptr, 67, "0x%X", bflags);
> > 
> > It seems this else statement facilitates papering over the fact that
> > scsi_sysfs.c and scsi_devinfo.h can get out of sync.
> > 
> 
> But there is no good way of avoiding that, is there?

Hello Hannes,

How about running the following shell code from a makefile, storing the
result in a file and #including that file from drivers/scsi/scsi_sysfs.c?

sed -n 's/^#define BLIST_\([^[:blank:]]*\).*/\t{ BLIST_\1, "\1" },/p' include/scsi/scsi_devinfo.h

Bart.

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

[Hannes Reinecke]

On 08/16/2017 12:47 AM, Bart Van Assche wrote:
> On Tue, 2017-08-15 at 10:03 +0200, Hannes Reinecke wrote:
>> On 08/15/2017 05:18 AM, Martin K. Petersen wrote:
>>>
>>> Hannes,
>>>
>>>> +		name = sdev_bflags_name(bflags);
>>>> +		if (name)
>>>> +			blen = snprintf(ptr, strlen(name) + 1,
>>>> +					"%s", name);
>>>> +		else
>>>> +			blen = snprintf(ptr, 67, "0x%X", bflags);
>>>
>>> It seems this else statement facilitates papering over the fact that
>>> scsi_sysfs.c and scsi_devinfo.h can get out of sync.
>>>
>>
>> But there is no good way of avoiding that, is there?
> 
> Hello Hannes,
> 
> How about running the following shell code from a makefile, storing the
> result in a file and #including that file from drivers/scsi/scsi_sysfs.c?
> 
> sed -n 's/^#define BLIST_\([^[:blank:]]*\).*/\t{ BLIST_\1, "\1" },/p' include/scsi/scsi_devinfo.h
> 
Hmm. Yeah, should be possible.
I'll see if I'm able to massage this into the Makefile.

I still would like to keep the above, as the admin can feed blacklist
flags via the kernel commandline, and we don't do any validity checks on
that. So we might end up with invalid flags after all.

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

[Martin K. Petersen]

Hannes,

> I still would like to keep the above, as the admin can feed blacklist
> flags via the kernel commandline, and we don't do any validity checks on
> that. So we might end up with invalid flags after all.

I suggest you handle this by printing something along the lines of
"INVALID_BIT_SET(42)" for each bad flag.

Also: Brownie points for adding a new sysfs interface to augment the
existing /proc goo for adding entries. Preferably one that takes BLIST
names instead of huge hex numbers.

-- 
Martin K. Petersen	Oracle Linux Engineering

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

[Martin K. Petersen]

Hannes,

>> It seems this else statement facilitates papering over the fact that
>> scsi_sysfs.c and scsi_devinfo.h can get out of sync.

> But there is no good way of avoiding that, is there?
> Out of necessity the definition of the bits and the decoding are two
> distinct steps, and as such are always prone to differences.
>
> So what is your suggestion here?

I was merely fishing for comments being added to both files stating that
it was important to keep them in sync.

However, if you can automate it using Bart's approach, stringify magic
or something else then that's cool. We have several places where
something like that would be useful.

-- 
Martin K. Petersen	Oracle Linux Engineering

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

[Bart Van Assche]

On Fri, 2017-08-11 at 16:23 +0200, Hannes Reinecke wrote:
> @@ -1138,6 +1220,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,

Hello Hannes,

Wouldn't debugfs be more appropriate for this attribute than sysfs?

Thanks,

Bart.

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

[Hannes Reinecke]

On 08/15/2017 04:24 PM, Bart Van Assche wrote:
> On Fri, 2017-08-11 at 16:23 +0200, Hannes Reinecke wrote:
>> @@ -1138,6 +1220,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,
> 
> Hello Hannes,
> 
> Wouldn't debugfs be more appropriate for this attribute than sysfs?
> 
I so don't mind.
I just need it somewhere in sysfs/debugfs so that I can validate the
selection logic.

Same goes for hexvalue vs decoded output; I just need to attribute;
contents can be discussed.

Martin?

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

[Martin K. Petersen]

Hannes,

>> Wouldn't debugfs be more appropriate for this attribute than sysfs?
>> 
> I so don't mind.
> I just need it somewhere in sysfs/debugfs so that I can validate the
> selection logic.
>
> Same goes for hexvalue vs decoded output; I just need to attribute;
> contents can be discussed.

I don't have a problem having it in sysfs. We have several users that
tweak the device blacklist in their boot scripts

Given that we already have an interface for setting the flags, we might
as well be able to print them.

-- 
Martin K. Petersen	Oracle Linux Engineering

[PATCHv3 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 | 77 ++++++++++++++++++++++++++++++---------------
 1 file changed, 51 insertions(+), 26 deletions(-)

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

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

[Bart Van Assche]

On Fri, 2017-08-11 at 16:23 +0200, Hannes Reinecke wrote:
> +typedef __u32 __bitwise sdev_bflags_t;
> +
> +/* Only scan LUN 0 */
> +#define BLIST_NOLUN		((__force sdev_bflags_t)(1 << 0))

Hello Hannes,

Is the new typedef used anywhere outside the BLIST flag definitions?
If not, how about skipping the introduction of a typedef and using
u32 __bitwise directly?

Thanks,

Bart.

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

[Hannes Reinecke]

On 08/16/2017 12:52 AM, Bart Van Assche wrote:
> On Fri, 2017-08-11 at 16:23 +0200, Hannes Reinecke wrote:
>> +typedef __u32 __bitwise sdev_bflags_t;
>> +
>> +/* Only scan LUN 0 */
>> +#define BLIST_NOLUN		((__force sdev_bflags_t)(1 << 0))
> 
> Hello Hannes,
> 
> Is the new typedef used anywhere outside the BLIST flag definitions?
> If not, how about skipping the introduction of a typedef and using
> u32 __bitwise directly?
> 
Yeah, can do.

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)

[PATCHv3 4/5] scsi: whitespace fixes in scsi_devinfo.c

[Hannes Reinecke]

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

diff --git a/drivers/scsi/scsi_devinfo.c b/drivers/scsi/scsi_devinfo.c
index 28fea83..776c701 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.
  **/
@@ -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

[PATCHv3 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>
---
 drivers/scsi/scsi_devinfo.c | 29 ++++++++++++++++-------------
 1 file changed, 16 insertions(+), 13 deletions(-)

diff --git a/drivers/scsi/scsi_devinfo.c b/drivers/scsi/scsi_devinfo.c
index 776c701..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,22 +454,25 @@ 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 {
 			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;
 		}
 	}
-- 
1.8.5.6

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

[Alan Stern]

On Fri, 11 Aug 2017, 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.

You really should fix this sentence.

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

Aside from the patch description,

Reviewed-by: Alan Stern <stern@rowland.harvard.edu>

>  drivers/scsi/scsi_devinfo.c | 29 ++++++++++++++++-------------
>  1 file changed, 16 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_devinfo.c b/drivers/scsi/scsi_devinfo.c
> index 776c701..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,22 +454,25 @@ 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 {
>  			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;
>  		}
>  	}
> 

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

[Bart Van Assche]

On Fri, 2017-08-11 at 16:23 +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;

"card"? Did you perhaps mean "care"?

> 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

The above sentence contradicts the first sentence.

> +			/*
> +			 * @model specifies the full string, and
> +			 * must be larger or equal to devinfo->model
> +			 */

What does "larger than or equal to" mean for strings? Did you perhaps mean that
devinfo->model must be a prefix of @model?

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

Is this a whitespace-only change? If so, please fold it in the previous patch.

Thanks,

Bart.

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

[Hannes Reinecke]

On 08/16/2017 01:03 AM, Bart Van Assche wrote:
> On Fri, 2017-08-11 at 16:23 +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;
> 
> "card"? Did you perhaps mean "care"?
> 
Yes, indeed.

>> 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
> 
> The above sentence contradicts the first sentence.
> 
Ah. Will be rewriting this.

>> +			/*
>> +			 * @model specifies the full string, and
>> +			 * must be larger or equal to devinfo->model
>> +			 */
> 
> What does "larger than or equal to" mean for strings? Did you perhaps mean that
> devinfo->model must be a prefix of @model?
> 
Yes.

>>  			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;
> 
> Is this a whitespace-only change? If so, please fold it in the previous patch.
> 
Ok.

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)