Re: [PATCH 1/1] mtd: block2mtd: add support for an optional custom MTD label

[Joachim Wiberg <troglobit@gmail.com>]

Hi Miquèl!

On Fri, Oct 08, 2021 at 09:27, Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> troglobit@gmail.com wrote on Tue,  5 Oct 2021 10:03:59 +0200:
>> [snip]
>> @@ -304,7 +307,7 @@ static struct block2mtd_dev *add_device(char *devname, int erase_size,
>>  
>>  	list_add(&dev->list, &blkmtd_device_list);
>>  	pr_info("mtd%d: [%s] erase_size = %dKiB [%d]\n",
>> -		dev->mtd.index,
>> +		dev->mtd.index, label ? label :
> Can you put this on the next line?

Sure thing!

>> @@ -381,8 +384,8 @@ static int block2mtd_setup2(const char *val)
>>  	/* 80 for device, 12 for erase size, 80 for name, 8 for timeout */
>>  	char buf[80 + 12 + 80 + 8];
>>  	char *str = buf;
>> -	char *token[2];
>> -	char *name;
>> +	char *token[3];
> This number deserves a define and should be used later on as well.

OK, I'll see what I can do, it's for the positional module arguments,
so a FOO_MAX_ARGS or sth, I guess ...

>> @@ -395,7 +398,7 @@ static int block2mtd_setup2(const char *val)
>>  	strcpy(str, val);
>>  	kill_final_newline(str);
>>  
>> -	for (i = 0; i < 2; i++)
>> +	for (i = 0; i < 3; i++)

used here as well ...

>>  		token[i] = strsep(&str, ",");

But what about each separate arg, is there any convention?  I'll see
what I can find in other modules.

>>  	if (str) {
>> @@ -414,7 +417,7 @@ static int block2mtd_setup2(const char *val)
>>  		return 0;
>>  	}
>>  
>> -	if (token[1]) {
>> +	if (token[1] && strlen(token[1])) {
>
> This change is not related to your commit I believe, please split if
> my assumption is right.

Well, it's a logical consequence of adding another module argument, but
you're right, this could be hit before as well if someone added multiple
commas without argument.  (That breaks erase_size default value.)

Maybe this and the defines fit in the same separate "cleanup" patch?

>> -	add_device(name, erase_size, timeout);
>> +	if (token[2]) {
>> +		label = token[2];
>> +		pr_info("Using custom MTD label '%s' for dev %s\n", label, name);
> Not sure this qualifies as an info log message, perhaps a debug one
> would be more appropriate?

OK!  (Just tried to follow the same log level used for erase_size.)

Thank you for the review, I'll post a v2 later today, or during the
weekend :)

Cheers
 /Joachim
 

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/