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

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Joachim,

troglobit@gmail.com wrote on Fri, 08 Oct 2021 10:26:46 +0200:

> 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.)

In this case you can keep this change in the commit but you need to
explain it in the commit message.

> 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.)

Ok then keep the driver consistent, it's not a bit deal and we can
lower the level later if it's too noisy.

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


Thanks,
Miquèl

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