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

This patch adds support for an optional MTD label for mtd2block emulated
MTD devices.  Useful when, e.g. testing device images using Qemu.  The
following /etc/fstab line in can then be used to mount a file system
regardless of the actual MTD partition number:

    mtd:Config  /mnt    jffs2   noatime,nodiratime      0    0

Kernel command line syntax:

    block2mtd.block2mtd=/dev/sda,,Config

The ',,' is the optional erase_size, which like before this patch,
defaults to PAGE_SIZE when omitted.

Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
---
 drivers/mtd/devices/block2mtd.c | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

Hi Joachim,

troglobit@gmail.com wrote on Tue,  5 Oct 2021 10:03:59 +0200:

> This patch adds support for an optional MTD label for mtd2block emulated
> MTD devices.  Useful when, e.g. testing device images using Qemu.  The
> following /etc/fstab line in can then be used to mount a file system
> regardless of the actual MTD partition number:
> 
>     mtd:Config  /mnt    jffs2   noatime,nodiratime      0    0
> 
> Kernel command line syntax:
> 
>     block2mtd.block2mtd=/dev/sda,,Config
> 
> The ',,' is the optional erase_size, which like before this patch,
> defaults to PAGE_SIZE when omitted.
> 
> Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
> ---
>  drivers/mtd/devices/block2mtd.c | 26 +++++++++++++++++---------
>  1 file changed, 17 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/mtd/devices/block2mtd.c b/drivers/mtd/devices/block2mtd.c
> index c08721b11642..9465b8ff48f6 100644
> --- a/drivers/mtd/devices/block2mtd.c
> +++ b/drivers/mtd/devices/block2mtd.c
> @@ -214,7 +214,7 @@ static void block2mtd_free_device(struct block2mtd_dev *dev)
>  
>  
>  static struct block2mtd_dev *add_device(char *devname, int erase_size,
> -		int timeout)
> +		char *label, int timeout)
>  {
>  #ifndef MODULE
>  	int i;
> @@ -278,7 +278,10 @@ static struct block2mtd_dev *add_device(char *devname, int erase_size,
>  
>  	/* Setup the MTD structure */
>  	/* make the name contain the block device in */
> -	name = kasprintf(GFP_KERNEL, "block2mtd: %s", devname);
> +	if (!label)
> +		name = kasprintf(GFP_KERNEL, "block2mtd: %s", devname);
> +	else
> +		name = kstrdup(label, GFP_KERNEL);
>  	if (!name)
>  		goto err_destroy_mutex;
>  
> @@ -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?

>  		dev->mtd.name + strlen("block2mtd: "),
>  		dev->mtd.erasesize >> 10, dev->mtd.erasesize);
>  	return dev;
> @@ -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.

> +	char *name, *label = NULL;
>  	size_t erase_size = PAGE_SIZE;
>  	unsigned long timeout = MTD_DEFAULT_TIMEOUT;
>  	int i, ret;
> @@ -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++)
>  		token[i] = strsep(&str, ",");
>  
>  	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.

>  		ret = parse_num(&erase_size, token[1]);
>  		if (ret) {
>  			pr_err("illegal erase size\n");
> @@ -422,7 +425,12 @@ static int block2mtd_setup2(const char *val)
>  		}
>  	}
>  
> -	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?

> +	}
> +
> +	add_device(name, erase_size, label, timeout);
>  
>  	return 0;
>  }
> @@ -456,7 +464,7 @@ static int block2mtd_setup(const char *val, const struct kernel_param *kp)
>  
>  
>  module_param_call(block2mtd, block2mtd_setup, NULL, NULL, 0200);
> -MODULE_PARM_DESC(block2mtd, "Device to use. \"block2mtd=<dev>[,<erasesize>]\"");
> +MODULE_PARM_DESC(block2mtd, "Device to use. \"block2mtd=<dev>[,[<erasesize>][,<label>]]\"");
>  
>  static int __init block2mtd_init(void)
>  {


Thanks,
Miquèl

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

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