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
[email protected] +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)
