| Message ID | 1502461401-94630-3-git-send-email-hare@suse.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
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.
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
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.
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
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.
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.
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.
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
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.
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,
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(+)