Re: [PATCH v3 1/3] mtd: block2mtd: Ensure that block2mtd is triggered after block devices are presented.

[Brian Norris <computersforpeace@gmail.com>]

You asked for review again... I guess I forgot about this patch series
for some time. I think this patch is OK, except for a trivial comment
below. But I have some comments on the next few.

On Sun, Nov 09, 2014 at 07:21:13AM -0500, Rodrigo Freire wrote:
> From: Felix Fietkau <nbd@openwrt.org>
> 
> mtd: block2mtd: Ensure that block2mtd is triggered after block devices are presented.
> 
> Ensures that block2mtd is triggered after the block devices are enumerated
> at boot time.
> This issue is seen on BCM2835 (Raspberry Pi) systems when mounting JFFS2
> block2mtd filesystems, probably because of the delay on enumerating a USB
> MMC card reader.
> 
> Signed-off-by: Felix Fietkau <nbd@openwrt.org>
> Signed-off-by: Rodrigo Freire <rfreire@redhat.com>
> Signed-off-by: Herton Krzesinski <herton@redhat.com>
> ---
> V3: Separated on a single patch, fixes a compiling warning, cosmethic changes
> V2: Uses kstrdup, removed PAGE_MASK.
> --- a/drivers/mtd/devices/block2mtd.c	2014-11-07 16:40:14.638676860 -0200
> +++ b/drivers/mtd/devices/block2mtd.c	2014-11-07 17:44:45.277769924 -0200
[...]
> @@ -225,15 +237,28 @@ static struct block2mtd_dev *add_device(
>  
>  	/* Get a handle on the device */
>  	bdev = blkdev_get_by_path(devname, mode, dev);
> -#ifndef MODULE
> -	if (IS_ERR(bdev)) {
>  
> -		/* We might not have rootfs mounted at this point. Try
> -		   to resolve the device name by other means. */
> +#ifndef MODULE
> +/*
> + * We might not have the root device mounted at this point.
> + * Try to resolve the device name by other means.
> + */

That's some interesting comment indentation.

> +	for (i = 0; IS_ERR(bdev) && i <= timeout; i++) {
> +		dev_t devt;
>  
> -		dev_t devt = name_to_dev_t(devname);
> -		if (devt)
> -			bdev = blkdev_get_by_dev(devt, mode, dev);
> +		if (i)
> +			/*
> +			 * Calling wait_for_device_probe in the first loop
> +			 * was not enough, sleep for a bit in subsequent
> +			 * go-arounds.
> +			 */
> +			msleep(1000);
> +		wait_for_device_probe();
> +
> +		devt = name_to_dev_t(devname);
> +		if (!devt)
> +			continue;
> +		bdev = blkdev_get_by_dev(devt, mode, dev);
>  	}
>  #endif
>  
> @@ -280,6 +305,7 @@ static struct block2mtd_dev *add_device(
>  		/* Device didn't get added, so free the entry */
>  		goto err_destroy_mutex;
>  	}
> +
>  	list_add(&dev->list, &blkmtd_device_list);
>  	pr_info("mtd%d: [%s] erase_size = %dKiB [%d]\n",
>  		dev->mtd.index,
> @@ -348,16 +374,19 @@ static inline void kill_final_newline(ch
>  
>  #ifndef MODULE
>  static int block2mtd_init_called = 0;
> -static char block2mtd_paramline[80 + 12]; /* 80 for device, 12 for erase size */
> +/* 80 for device, 12 for erase size */
> +static char block2mtd_paramline[80 + 12];
>  #endif
>  
>  static int block2mtd_setup2(const char *val)
>  {
> -	char buf[80 + 12]; /* 80 for device, 12 for erase size */
> +	/* 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;
>  	size_t erase_size = PAGE_SIZE;
> +	unsigned long timeout = MTD_DEFAULT_TIMEOUT;
>  	int i, ret;
>  
>  	if (strnlen(val, sizeof(buf)) >= sizeof(buf)) {
> @@ -395,7 +424,7 @@ static int block2mtd_setup2(const char *
>  		}
>  	}
>  
> -	add_device(name, erase_size);
> +	add_device(name, erase_size, timeout);
>  
>  	return 0;
>  }
> @@ -463,8 +492,7 @@ static void block2mtd_exit(void)
>  	}
>  }
>  
> -
> -module_init(block2mtd_init);
> +late_initcall(block2mtd_init);

This technically could have problems if you want to use block2mtd with
UBI, now, since it also uses late_initcall(), and it can add UBI
attachments via module parameters too (ubi.mtd=<mtd-name>).

I'm not too worried about this, though, since block2mtd is really not
meant for serious use (despite your usage here).

>  module_exit(block2mtd_exit);
>  
>  MODULE_LICENSE("GPL");

Pushed this patch to l2-mtd.git, with comment fixups.

Brian