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

[Bart Van Assche <Bart.VanAssche@wdc.com>]

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.