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

[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 ReineckeTeamlead Storage & Networking
h...@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,

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

[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

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/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,
>>  _attr_queue_depth.attr,
>>  _attr_queue_type.attr,
>>  _attr_wwid.attr,
>> +_attr_blacklist.attr,
>>  #ifdef CONFIG_SCSI_DH
>>  _attr_dh_state.attr,
>>  _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 ReineckeTeamlead Storage & Networking
h...@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 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,
>   _attr_queue_depth.attr,
>   _attr_queue_type.attr,
>   _attr_wwid.attr,
> + _attr_blacklist.attr,
>  #ifdef CONFIG_SCSI_DH
>   _attr_dh_state.attr,
>   _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 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 ReineckeTeamlead Storage & Networking
h...@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,

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

[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 
---
 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 
 #include 
 #include 
+#include 
 
 #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 intvalue;
+   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,
_attr_queue_depth.attr,
_attr_queue_type.attr,
_attr_wwid.attr,
+   _attr_blacklist.attr,
 #ifdef CONFIG_SCSI_DH
_attr_dh_state.attr,
_attr_access_state.attr,
-- 
1.8.5.6