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

[Hannes Reinecke <hare@suse.de>]

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)