[PATCH 0/5] Extends block2mtd and ubi drivers

[PATCH 0/5] Extends block2mtd and ubi drivers

[Pali Rohár]

This patch series extends block2mtd and ubi drivers to better handle
read-only devices and allow to load UBI image from local file which was
created for nand device.

Tested for Nokia N900 with Maemo 5 rootfs ubifs image
(rootfs_RX-51_2009SE_21.2011.38-1_PR_MR0) which has erase size 128k,
write size 2k and nand subpage shift 2.

$ losetup -r /dev/loop0 rootfs_RX-51_2009SE_21.2011.38-1_PR_MR0.ubifs
$ echo -n /dev/loop0,131072,2048,2 > /sys/module/block2mtd/parameters/block2mtd
$ ubiattach -p /dev/mtd0
$ mount /dev/ubi0_0 /mnt/ubi -t ubifs
...
$ umount /dev/ubi0_0
$ ubidetach -p /dev/mtd0
$ echo -n del=/dev/loop0 > /sys/module/block2mtd/parameters/block2mtd
$ losetup -d /dev/loop0

Pali Rohár (5):
  mtd: block2mtd: Check for valid user supplied erase size
  mtd: block2mtd: Add support for specifying MTD write size and subpage
    shift
  mtd: block2mtd: Fallback to read-only mode
  mtd: block2mtd: Add support for deleting block2mtd mapping
  ubi: Allow to use read-only UBI volume with not enough PEBs

 drivers/mtd/devices/block2mtd.c |  129 ++++++++++++++++++++++++++++++---------
 drivers/mtd/ubi/vtbl.c          |   14 +++--
 2 files changed, 110 insertions(+), 33 deletions(-)

-- 
1.7.9.5

[PATCH 1/5] mtd: block2mtd: Check for valid user supplied erase size

[Pali Rohár]

Erase size is limited to 32bit unsigned integer, but value parsed from user
is limited up to size_t C type.

Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
---
 drivers/mtd/devices/block2mtd.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/devices/block2mtd.c b/drivers/mtd/devices/block2mtd.c
index 7c887f1..ee47cdd 100644
--- a/drivers/mtd/devices/block2mtd.c
+++ b/drivers/mtd/devices/block2mtd.c
@@ -419,7 +419,7 @@ static int block2mtd_setup2(const char *val)
 
 	if (token[1]) {
 		ret = parse_num(&erase_size, token[1]);
-		if (ret) {
+		if (ret || erase_size > U32_MAX) {
 			pr_err("illegal erase size\n");
 			return 0;
 		}
-- 
1.7.9.5

[PATCH 2/5] mtd: block2mtd: Add support for specifying MTD write size and subpage shift

[Pali Rohár]

It is needed for creating emulated devices suitable for using in UBI layer
and with UBIFS.

Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
---
 drivers/mtd/devices/block2mtd.c |   55 ++++++++++++++++++++++++++++++---------
 1 file changed, 42 insertions(+), 13 deletions(-)

diff --git a/drivers/mtd/devices/block2mtd.c b/drivers/mtd/devices/block2mtd.c
index ee47cdd..5ba5fad 100644
--- a/drivers/mtd/devices/block2mtd.c
+++ b/drivers/mtd/devices/block2mtd.c
@@ -3,6 +3,7 @@
  *
  * Copyright (C) 2001,2002	Simon Evans <spse@secret.org.uk>
  * Copyright (C) 2004-2006	Joern Engel <joern@wh.fh-wedel.de>
+ * Copyright (C) 2012-2017	Pali Rohár <pali.rohar@gmail.com>
  *
  * Licence: GPL
  */
@@ -218,8 +219,8 @@ static void block2mtd_free_device(struct block2mtd_dev *dev)
 }
 
 
-static struct block2mtd_dev *add_device(char *devname, int erase_size,
-		int timeout)
+static struct block2mtd_dev *add_device(char *devname, uint32_t erase_size,
+		uint32_t write_size, int subpage_sft, int timeout)
 {
 #ifndef MODULE
 	int i;
@@ -279,6 +280,11 @@ static struct block2mtd_dev *add_device(char *devname, int erase_size,
 		goto err_free_block2mtd;
 	}
 
+	if ((long)dev->blkdev->bd_inode->i_size % write_size) {
+		pr_err("writesize must be divisor of device size\n");
+		goto err_free_block2mtd;
+	}
+
 	mutex_init(&dev->write_mutex);
 
 	/* Setup the MTD structure */
@@ -291,7 +297,8 @@ static struct block2mtd_dev *add_device(char *devname, int erase_size,
 
 	dev->mtd.size = dev->blkdev->bd_inode->i_size & PAGE_MASK;
 	dev->mtd.erasesize = erase_size;
-	dev->mtd.writesize = 1;
+	dev->mtd.writesize = write_size;
+	dev->mtd.subpage_sft = subpage_sft;
 	dev->mtd.writebufsize = PAGE_SIZE;
 	dev->mtd.type = MTD_RAM;
 	dev->mtd.flags = MTD_CAP_RAM;
@@ -308,10 +315,12 @@ 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",
+	pr_info("mtd%d: [%s] erase_size = %dKiB [%d], write_size = %dKiB [%d], subpage_sft = %d\n",
 		dev->mtd.index,
 		dev->mtd.name + strlen("block2mtd: "),
-		dev->mtd.erasesize >> 10, dev->mtd.erasesize);
+		dev->mtd.erasesize >> 10, dev->mtd.erasesize,
+		dev->mtd.writesize >> 10, dev->mtd.writesize,
+		dev->mtd.subpage_sft);
 	return dev;
 
 err_destroy_mutex:
@@ -375,18 +384,20 @@ static inline void kill_final_newline(char *str)
 
 #ifndef MODULE
 static int block2mtd_init_called = 0;
-/* 80 for device, 12 for erase size */
-static char block2mtd_paramline[80 + 12];
+/* 80 for device, 12 for erase size, 12 for write size, 3 for subpage_sft */
+static char block2mtd_paramline[80 + 12 + 12 + 3];
 #endif
 
 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];
+	/* 80 for name, 80 for device, 12 for erase size, 12 for write size, 3 for subpage_sft */
+	char buf[80 + 12 + 80 + 12 + 3];
 	char *str = buf;
-	char *token[2];
+	char *token[4];
 	char *name;
 	size_t erase_size = PAGE_SIZE;
+	size_t write_size = 1;
+	size_t subpage_sft = 0;
 	unsigned long timeout = MTD_DEFAULT_TIMEOUT;
 	int i, ret;
 
@@ -398,7 +409,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 < 4; i++)
 		token[i] = strsep(&str, ",");
 
 	if (str) {
@@ -425,7 +436,23 @@ static int block2mtd_setup2(const char *val)
 		}
 	}
 
-	add_device(name, erase_size, timeout);
+	if (token[2]) {
+		ret = parse_num(&write_size, token[2]);
+		if (ret || write_size > U32_MAX) {
+			pr_err("illegal write size");
+			return 0;
+		}
+	}
+
+	if (token[3]) {
+		ret = parse_num(&subpage_sft, token[3]);
+		if (ret || subpage_sft > INT_MAX) {
+			pr_err("illegal subpage_sft");
+			return 0;
+		}
+	}
+
+	add_device(name, erase_size, write_size, subpage_sft, timeout);
 
 	return 0;
 }
@@ -459,7 +486,9 @@ static int block2mtd_setup(const char *val, 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, "Block device, erase size (default page size), "
+	"write size (default 1), nand subpage shift (default 0) "
+	"\"block2mtd=<dev>[,<erasesize>[,<writesize>[,<subpage_sft>]]]\"");
 
 static int __init block2mtd_init(void)
 {
-- 
1.7.9.5

[PATCH 3/5] mtd: block2mtd: Fallback to read-only mode

[Pali Rohár]

Sometimes block device is readonly (e.g. loopbacks). This patch allows
block2mtd to fallback and create read-only MTD_ROM device instead of
fatal failure.

Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
---
 drivers/mtd/devices/block2mtd.c |   39 ++++++++++++++++++++++++++++++++-------
 1 file changed, 32 insertions(+), 7 deletions(-)

diff --git a/drivers/mtd/devices/block2mtd.c b/drivers/mtd/devices/block2mtd.c
index 5ba5fad..ad96937 100644
--- a/drivers/mtd/devices/block2mtd.c
+++ b/drivers/mtd/devices/block2mtd.c
@@ -38,6 +38,7 @@ struct block2mtd_dev {
 	struct block_device *blkdev;
 	struct mtd_info mtd;
 	struct mutex write_mutex;
+	bool ro_mode;
 };
 
 
@@ -212,7 +213,10 @@ static void block2mtd_free_device(struct block2mtd_dev *dev)
 	if (dev->blkdev) {
 		invalidate_mapping_pages(dev->blkdev->bd_inode->i_mapping,
 					0, -1);
-		blkdev_put(dev->blkdev, FMODE_READ|FMODE_WRITE|FMODE_EXCL);
+		if (dev->ro_mode)
+			blkdev_put(dev->blkdev, FMODE_READ);
+		else
+			blkdev_put(dev->blkdev, FMODE_READ|FMODE_WRITE|FMODE_EXCL);
 	}
 
 	kfree(dev);
@@ -240,6 +244,13 @@ static struct block2mtd_dev *add_device(char *devname, uint32_t erase_size,
 	/* Get a handle on the device */
 	bdev = blkdev_get_by_path(devname, mode, dev);
 
+	/* Try fallback to read only mode */
+	if (IS_ERR(bdev)) {
+		bdev = blkdev_get_by_path(devname, FMODE_READ, dev);
+		if (!IS_ERR(bdev))
+			dev->ro_mode = true;
+	}
+
 #ifndef MODULE
 	/*
 	 * We might not have the root device mounted at this point.
@@ -261,6 +272,13 @@ static struct block2mtd_dev *add_device(char *devname, uint32_t erase_size,
 		if (!devt)
 			continue;
 		bdev = blkdev_get_by_dev(devt, mode, dev);
+
+		/* Try fallback to read only mode */
+		if (IS_ERR(bdev)) {
+			bdev = blkdev_get_by_path(devname, FMODE_READ, dev);
+			if (!IS_ERR(bdev))
+				dev->ro_mode = true;
+		}
 	}
 #endif
 
@@ -295,16 +313,22 @@ static struct block2mtd_dev *add_device(char *devname, uint32_t erase_size,
 
 	dev->mtd.name = name;
 
+	if (dev->ro_mode) {
+		dev->mtd.type = MTD_ROM;
+		dev->mtd.flags = MTD_CAP_ROM;
+	} else {
+		dev->mtd.type = MTD_RAM;
+		dev->mtd.flags = MTD_CAP_RAM;
+		dev->mtd._erase = block2mtd_erase;
+		dev->mtd._write = block2mtd_write;
+		dev->mtd._sync = block2mtd_sync;
+	}
+
 	dev->mtd.size = dev->blkdev->bd_inode->i_size & PAGE_MASK;
 	dev->mtd.erasesize = erase_size;
 	dev->mtd.writesize = write_size;
 	dev->mtd.subpage_sft = subpage_sft;
 	dev->mtd.writebufsize = PAGE_SIZE;
-	dev->mtd.type = MTD_RAM;
-	dev->mtd.flags = MTD_CAP_RAM;
-	dev->mtd._erase = block2mtd_erase;
-	dev->mtd._write = block2mtd_write;
-	dev->mtd._sync = block2mtd_sync;
 	dev->mtd._read = block2mtd_read;
 	dev->mtd.priv = dev;
 	dev->mtd.owner = THIS_MODULE;
@@ -315,9 +339,10 @@ static struct block2mtd_dev *add_device(char *devname, uint32_t erase_size,
 	}
 
 	list_add(&dev->list, &blkmtd_device_list);
-	pr_info("mtd%d: [%s] erase_size = %dKiB [%d], write_size = %dKiB [%d], subpage_sft = %d\n",
+	pr_info("mtd%d: [%s] mode = %s, erase_size = %dKiB [%d], write_size = %dKiB [%d], subpage_sft = %d\n",
 		dev->mtd.index,
 		dev->mtd.name + strlen("block2mtd: "),
+		(dev->ro_mode ? "RO" : "R/W"),
 		dev->mtd.erasesize >> 10, dev->mtd.erasesize,
 		dev->mtd.writesize >> 10, dev->mtd.writesize,
 		dev->mtd.subpage_sft);
-- 
1.7.9.5

[PATCH 4/5] mtd: block2mtd: Add support for deleting block2mtd mapping

[Pali Rohár]

This patch allows user to delete block2mtd mapping via parameters file
/sys/module/block2mtd/parameters/block2mtd

Syntax is "del=device", e.g.:

$ echo -n del=/dev/loop0 > /sys/module/block2mtd/parameters/block2mtd

Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
---
 drivers/mtd/devices/block2mtd.c |   35 +++++++++++++++++++++++++++--------
 1 file changed, 27 insertions(+), 8 deletions(-)

diff --git a/drivers/mtd/devices/block2mtd.c b/drivers/mtd/devices/block2mtd.c
index ad96937..9b6f7b5 100644
--- a/drivers/mtd/devices/block2mtd.c
+++ b/drivers/mtd/devices/block2mtd.c
@@ -355,6 +355,19 @@ static struct block2mtd_dev *add_device(char *devname, uint32_t erase_size,
 	return NULL;
 }
 
+static void del_device(struct block2mtd_dev *dev)
+{
+	if (!dev->ro_mode)
+		block2mtd_sync(&dev->mtd);
+	mtd_device_unregister(&dev->mtd);
+	mutex_destroy(&dev->write_mutex);
+	pr_info("mtd%d: [%s] removed\n",
+		dev->mtd.index,
+		dev->mtd.name + strlen("block2mtd: "));
+	list_del(&dev->list);
+	block2mtd_free_device(dev);
+}
+
 
 /* This function works similar to reguler strtoul.  In addition, it
  * allows some suffixes for a more human-readable number format:
@@ -477,6 +490,19 @@ static int block2mtd_setup2(const char *val)
 		}
 	}
 
+	if (strncmp(name, "del=", strlen("del=")) == 0) {
+		struct list_head *pos, *next;
+		list_for_each_safe(pos, next, &blkmtd_device_list) {
+			struct block2mtd_dev *dev =
+				list_entry(pos, typeof(*dev), list);
+			if (strcmp(dev->mtd.name + strlen("block2mtd: "),
+				name + strlen("del=")) != 0)
+				continue;
+			del_device(dev);
+			return 0;
+		}
+	}
+
 	add_device(name, erase_size, write_size, subpage_sft, timeout);
 
 	return 0;
@@ -536,14 +562,7 @@ static void block2mtd_exit(void)
 	/* Remove the MTD devices */
 	list_for_each_safe(pos, next, &blkmtd_device_list) {
 		struct block2mtd_dev *dev = list_entry(pos, typeof(*dev), list);
-		block2mtd_sync(&dev->mtd);
-		mtd_device_unregister(&dev->mtd);
-		mutex_destroy(&dev->write_mutex);
-		pr_info("mtd%d: [%s] removed\n",
-			dev->mtd.index,
-			dev->mtd.name + strlen("block2mtd: "));
-		list_del(&dev->list);
-		block2mtd_free_device(dev);
+		del_device(dev);
 	}
 }
 
-- 
1.7.9.5

[PATCH 5/5] ubi: Allow to use read-only UBI volume with not enough PEBs

[Pali Rohár]

In read-only mode is skipped auto-resize. For pre-build images ready for
auto-resize there can be reserved more PEBs as whole size of pre-build
image. In read-only we do not do any write operation therefore this would
allow to use read-only UBI volume which is not auto-resized yet.

Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
---
 drivers/mtd/ubi/vtbl.c |   14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/mtd/ubi/vtbl.c b/drivers/mtd/ubi/vtbl.c
index 263743e..1d708c5 100644
--- a/drivers/mtd/ubi/vtbl.c
+++ b/drivers/mtd/ubi/vtbl.c
@@ -240,8 +240,10 @@ static int vtbl_check(const struct ubi_device *ubi,
 		if (reserved_pebs > ubi->good_peb_count) {
 			ubi_err(ubi, "too large reserved_pebs %d, good PEBs %d",
 				reserved_pebs, ubi->good_peb_count);
-			err = 9;
-			goto bad;
+			if (!ubi->ro_mode) {
+				err = 9;
+				goto bad;
+			}
 		}
 
 		if (name_len > UBI_VOL_NAME_MAX) {
@@ -652,10 +654,12 @@ static int init_volumes(struct ubi_device *ubi,
 		if (ubi->corr_peb_count)
 			ubi_err(ubi, "%d PEBs are corrupted and not used",
 				ubi->corr_peb_count);
-		return -ENOSPC;
+		if (!ubi->ro_mode)
+			return -ENOSPC;
+	} else {
+		ubi->rsvd_pebs += reserved_pebs;
+		ubi->avail_pebs -= reserved_pebs;
 	}
-	ubi->rsvd_pebs += reserved_pebs;
-	ubi->avail_pebs -= reserved_pebs;
 
 	return 0;
 }
-- 
1.7.9.5

Re: [PATCH 2/5] mtd: block2mtd: Add support for specifying MTD write size and subpage shift

[Richard Weinberger]

Pali,

Am 02.06.2017 um 17:43 schrieb Pali Rohár:
> It is needed for creating emulated devices suitable for using in UBI layer
> and with UBIFS.

Why?

It is not clear to me why kind of MTD you are constructing.
subpages are NAND specific you create RAM and ROM MTDs.
Sure, the MTD allows such constructs but I'm not sure whether this is a good idea.

Thanks,
//richard

Re: [PATCH 0/5] Extends block2mtd and ubi drivers

[Richard Weinberger]

Pali,

Am 02.06.2017 um 17:43 schrieb Pali Rohár:
> This patch series extends block2mtd and ubi drivers to better handle
> read-only devices and allow to load UBI image from local file which was
> created for nand device.
> 
> Tested for Nokia N900 with Maemo 5 rootfs ubifs image
> (rootfs_RX-51_2009SE_21.2011.38-1_PR_MR0) which has erase size 128k,
> write size 2k and nand subpage shift 2.

What is the use case behind this series?

Did you see my nandsim rework some time ago?
http://lists.infradead.org/pipermail/linux-mtd/2016-September/069422.html
If you need a way to load files/nanddumps as NAND devices, this should be a good
starting point.
This reminds me that I need to revive that series. :-)

> $ losetup -r /dev/loop0 rootfs_RX-51_2009SE_21.2011.38-1_PR_MR0.ubifs
> $ echo -n /dev/loop0,131072,2048,2 > /sys/module/block2mtd/parameters/block2mtd
> $ ubiattach -p /dev/mtd0
> $ mount /dev/ubi0_0 /mnt/ubi -t ubifs
> ...
> $ umount /dev/ubi0_0
> $ ubidetach -p /dev/mtd0
> $ echo -n del=/dev/loop0 > /sys/module/block2mtd/parameters/block2mtd
> $ losetup -d /dev/loop0

The module-parameter interface is odd. IMHO we should not extend it.

Thanks,
//richard

Re: [PATCH 0/5] Extends block2mtd and ubi drivers

[Pali Rohár]

On Friday 02 June 2017 18:17:06 Richard Weinberger wrote:
> Pali,
> 
> Am 02.06.2017 um 17:43 schrieb Pali Rohár:
> > This patch series extends block2mtd and ubi drivers to better handle
> > read-only devices and allow to load UBI image from local file which was
> > created for nand device.
> > 
> > Tested for Nokia N900 with Maemo 5 rootfs ubifs image
> > (rootfs_RX-51_2009SE_21.2011.38-1_PR_MR0) which has erase size 128k,
> > write size 2k and nand subpage shift 2.
> 
> What is the use case behind this series?

Take existing ubi image (where is one ubifs volume) and unpack it. Or
rather unpack-modify-pack to do some small changes.

> Did you see my nandsim rework some time ago?
> http://lists.infradead.org/pipermail/linux-mtd/2016-September/069422.html

No yet. I have my patches since 2012, but I decided after cleaning up
them to send... As I think they could be useful for other people.

> If you need a way to load files/nanddumps as NAND devices, this should be a good
> starting point.
> This reminds me that I need to revive that series. :-)

nandsim.ko has problem that needs to be loaded with special parameters
compatible with characteristic of target nand for which is ubi image
prepared. Plus it is nand similator and not layer to translate arbitrary
file image on disk to mtd device. block2mtd is what is doing this part.

> > $ losetup -r /dev/loop0 rootfs_RX-51_2009SE_21.2011.38-1_PR_MR0.ubifs
> > $ echo -n /dev/loop0,131072,2048,2 > /sys/module/block2mtd/parameters/block2mtd
> > $ ubiattach -p /dev/mtd0
> > $ mount /dev/ubi0_0 /mnt/ubi -t ubifs
> > ...
> > $ umount /dev/ubi0_0
> > $ ubidetach -p /dev/mtd0
> > $ echo -n del=/dev/loop0 > /sys/module/block2mtd/parameters/block2mtd
> > $ losetup -d /dev/loop0
> 
> The module-parameter interface is odd. IMHO we should not extend it.

That file is used for adding new mapping from block device to mtd
device. Currently there is no other way how to specify that mapping or
removing mapping.

If you have better idea, let me know and I would try to implement it.

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH 2/5] mtd: block2mtd: Add support for specifying MTD write size and subpage shift

[Pali Rohár]

On Friday 02 June 2017 18:13:02 Richard Weinberger wrote:
> Pali,
> 
> Am 02.06.2017 um 17:43 schrieb Pali Rohár:
> > It is needed for creating emulated devices suitable for using in UBI layer
> > and with UBIFS.
> 
> Why?

ubifs depends on write size of nand. And without those parameters as
specified in cover letter I'm unable to mount N900 rootfs image exported
via block2mtd. ubifs reject such image.

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH 2/5] mtd: block2mtd: Add support for specifying MTD write size and subpage shift

[Richard Weinberger]

Pali,

Am 05.06.2017 um 13:21 schrieb Pali Rohár:
> On Friday 02 June 2017 18:13:02 Richard Weinberger wrote:
>> Pali,
>>
>> Am 02.06.2017 um 17:43 schrieb Pali Rohár:
>>> It is needed for creating emulated devices suitable for using in UBI layer
>>> and with UBIFS.
>>
>> Why?
> 
> ubifs depends on write size of nand. And without those parameters as
> specified in cover letter I'm unable to mount N900 rootfs image exported
> via block2mtd. ubifs reject such image.

Hmm, so you render block2mtd into a semi-NAND chip? :)

Thanks,
//richard

Re: [PATCH 2/5] mtd: block2mtd: Add support for specifying MTD write size and subpage shift

[Pali Rohár]

On Monday 05 June 2017 13:23:22 Richard Weinberger wrote:
> Pali,
> 
> Am 05.06.2017 um 13:21 schrieb Pali Rohár:
> > On Friday 02 June 2017 18:13:02 Richard Weinberger wrote:
> >> Pali,
> >>
> >> Am 02.06.2017 um 17:43 schrieb Pali Rohár:
> >>> It is needed for creating emulated devices suitable for using in UBI layer
> >>> and with UBIFS.
> >>
> >> Why?
> > 
> > ubifs depends on write size of nand. And without those parameters as
> > specified in cover letter I'm unable to mount N900 rootfs image exported
> > via block2mtd. ubifs reject such image.
> 
> Hmm, so you render block2mtd into a semi-NAND chip? :)

Probably you can call it like that. But it is still MTD device...

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH 2/5] mtd: block2mtd: Add support for specifying MTD write size and subpage shift

[Richard Weinberger]

Pali,

Am 05.06.2017 um 13:25 schrieb Pali Rohár:
> On Monday 05 June 2017 13:23:22 Richard Weinberger wrote:
>> Pali,
>>
>> Am 05.06.2017 um 13:21 schrieb Pali Rohár:
>>> On Friday 02 June 2017 18:13:02 Richard Weinberger wrote:
>>>> Pali,
>>>>
>>>> Am 02.06.2017 um 17:43 schrieb Pali Rohár:
>>>>> It is needed for creating emulated devices suitable for using in UBI layer
>>>>> and with UBIFS.
>>>>
>>>> Why?
>>>
>>> ubifs depends on write size of nand. And without those parameters as
>>> specified in cover letter I'm unable to mount N900 rootfs image exported
>>> via block2mtd. ubifs reject such image.
>>
>> Hmm, so you render block2mtd into a semi-NAND chip? :)
> 
> Probably you can call it like that. But it is still MTD device...

This is what I meant in my other mail.
You add NAND specific properties but still denote it as MTD_RAM/ROM.
I'm not sure whether this is a good idea.

Thanks,
//richard

Re: [PATCH 2/5] mtd: block2mtd: Add support for specifying MTD write size and subpage shift

[Pali Rohár]

On Monday 05 June 2017 13:27:18 Richard Weinberger wrote:
> Pali,
> 
> Am 05.06.2017 um 13:25 schrieb Pali Rohár:
> > On Monday 05 June 2017 13:23:22 Richard Weinberger wrote:
> >> Pali,
> >>
> >> Am 05.06.2017 um 13:21 schrieb Pali Rohár:
> >>> On Friday 02 June 2017 18:13:02 Richard Weinberger wrote:
> >>>> Pali,
> >>>>
> >>>> Am 02.06.2017 um 17:43 schrieb Pali Rohár:
> >>>>> It is needed for creating emulated devices suitable for using in UBI layer
> >>>>> and with UBIFS.
> >>>>
> >>>> Why?
> >>>
> >>> ubifs depends on write size of nand. And without those parameters as
> >>> specified in cover letter I'm unable to mount N900 rootfs image exported
> >>> via block2mtd. ubifs reject such image.
> >>
> >> Hmm, so you render block2mtd into a semi-NAND chip? :)
> > 
> > Probably you can call it like that. But it is still MTD device...
> 
> This is what I meant in my other mail.
> You add NAND specific properties but still denote it as MTD_RAM/ROM.
> I'm not sure whether this is a good idea.

Ok, lets wait what other people think.

At least patches like fallback or check should be less problematic and
could be applied separately.

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH 2/5] mtd: block2mtd: Add support for specifying MTD write size and subpage shift

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 938 bytes --]

Hi!

> >>>> Am 02.06.2017 um 17:43 schrieb Pali Rohár:
> >>>>> It is needed for creating emulated devices suitable for using in UBI layer
> >>>>> and with UBIFS.
> >>>>
> >>>> Why?
> >>>
> >>> ubifs depends on write size of nand. And without those parameters as
> >>> specified in cover letter I'm unable to mount N900 rootfs image exported
> >>> via block2mtd. ubifs reject such image.
> >>
> >> Hmm, so you render block2mtd into a semi-NAND chip? :)
> > 
> > Probably you can call it like that. But it is still MTD device...
> 
> This is what I meant in my other mail.
> You add NAND specific properties but still denote it as MTD_RAM/ROM.
> I'm not sure whether this is a good idea.

Can you suggest any other way for Pali to mount rootfs image on his
PC?

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 2/5] mtd: block2mtd: Add support for specifying MTD write size and subpage shift

[Richard Weinberger]

Pavel,

Am 18.06.2017 um 12:06 schrieb Pavel Machek:
> Hi!
> 
>>>>>> Am 02.06.2017 um 17:43 schrieb Pali Rohár:
>>>>>>> It is needed for creating emulated devices suitable for using in UBI layer
>>>>>>> and with UBIFS.
>>>>>>
>>>>>> Why?
>>>>>
>>>>> ubifs depends on write size of nand. And without those parameters as
>>>>> specified in cover letter I'm unable to mount N900 rootfs image exported
>>>>> via block2mtd. ubifs reject such image.
>>>>
>>>> Hmm, so you render block2mtd into a semi-NAND chip? :)
>>>
>>> Probably you can call it like that. But it is still MTD device...
>>
>> This is what I meant in my other mail.
>> You add NAND specific properties but still denote it as MTD_RAM/ROM.
>> I'm not sure whether this is a good idea.
> 
> Can you suggest any other way for Pali to mount rootfs image on his
> PC?

Are you my micro manager? ;-)

As stated in my other mail, we have nandsim and there is work going on
to allow specifying arbitrary NAND sizes.
An alternative approach can be found here:
https://lkml.org/lkml/2016/5/12/296

Thanks,
//richard

Re: [PATCH 2/5] mtd: block2mtd: Add support for specifying MTD write size and subpage shift

[Pali Rohár]

On Sunday 18 June 2017 12:11:40 Richard Weinberger wrote:
> Pavel,
> 
> Am 18.06.2017 um 12:06 schrieb Pavel Machek:
> > Hi!
> > 
> >>>>>> Am 02.06.2017 um 17:43 schrieb Pali Rohár:
> >>>>>>> It is needed for creating emulated devices suitable for using in UBI layer
> >>>>>>> and with UBIFS.
> >>>>>>
> >>>>>> Why?
> >>>>>
> >>>>> ubifs depends on write size of nand. And without those parameters as
> >>>>> specified in cover letter I'm unable to mount N900 rootfs image exported
> >>>>> via block2mtd. ubifs reject such image.
> >>>>
> >>>> Hmm, so you render block2mtd into a semi-NAND chip? :)
> >>>
> >>> Probably you can call it like that. But it is still MTD device...
> >>
> >> This is what I meant in my other mail.
> >> You add NAND specific properties but still denote it as MTD_RAM/ROM.
> >> I'm not sure whether this is a good idea.
> > 
> > Can you suggest any other way for Pali to mount rootfs image on his
> > PC?
> 
> Are you my micro manager? ;-)
> 
> As stated in my other mail, we have nandsim and there is work going on
> to allow specifying arbitrary NAND sizes.
> An alternative approach can be found here:
> https://lkml.org/lkml/2016/5/12/296
> 
> Thanks,
> //richard

Back to the this patch series. There are also other patches in this
series. If you at least comment/review other patches if this one is
"problematic"?

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH 1/5] mtd: block2mtd: Check for valid user supplied erase size

[Richard Weinberger]

On Fri, Jun 2, 2017 at 5:43 PM, Pali Rohár <pali.rohar@gmail.com> wrote:
> Erase size is limited to 32bit unsigned integer, but value parsed from user
> is limited up to size_t C type.
>
> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> ---
>  drivers/mtd/devices/block2mtd.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/mtd/devices/block2mtd.c b/drivers/mtd/devices/block2mtd.c
> index 7c887f1..ee47cdd 100644
> --- a/drivers/mtd/devices/block2mtd.c
> +++ b/drivers/mtd/devices/block2mtd.c
> @@ -419,7 +419,7 @@ static int block2mtd_setup2(const char *val)
>
>         if (token[1]) {
>                 ret = parse_num(&erase_size, token[1]);
> -               if (ret) {
> +               if (ret || erase_size > U32_MAX) {
>                         pr_err("illegal erase size\n");
>                         return 0;
>                 }

Reviewed-by: Richard Weinberger <richard@nod.at>

-- 
Thanks,
//richard

Re: [PATCH 3/5] mtd: block2mtd: Fallback to read-only mode

[Richard Weinberger]

Pali,

On Fri, Jun 2, 2017 at 5:43 PM, Pali Rohár <pali.rohar@gmail.com> wrote:
> Sometimes block device is readonly (e.g. loopbacks). This patch allows
> block2mtd to fallback and create read-only MTD_ROM device instead of
> fatal failure.
>
> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> ---
>  drivers/mtd/devices/block2mtd.c |   39 ++++++++++++++++++++++++++++++++-------
>  1 file changed, 32 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/mtd/devices/block2mtd.c b/drivers/mtd/devices/block2mtd.c
> index 5ba5fad..ad96937 100644
> --- a/drivers/mtd/devices/block2mtd.c
> +++ b/drivers/mtd/devices/block2mtd.c
> @@ -38,6 +38,7 @@ struct block2mtd_dev {
>         struct block_device *blkdev;
>         struct mtd_info mtd;
>         struct mutex write_mutex;
> +       bool ro_mode;
>  };
>
>
> @@ -212,7 +213,10 @@ static void block2mtd_free_device(struct block2mtd_dev *dev)
>         if (dev->blkdev) {
>                 invalidate_mapping_pages(dev->blkdev->bd_inode->i_mapping,
>                                         0, -1);
> -               blkdev_put(dev->blkdev, FMODE_READ|FMODE_WRITE|FMODE_EXCL);
> +               if (dev->ro_mode)
> +                       blkdev_put(dev->blkdev, FMODE_READ);
> +               else
> +                       blkdev_put(dev->blkdev, FMODE_READ|FMODE_WRITE|FMODE_EXCL);
>         }
>
>         kfree(dev);
> @@ -240,6 +244,13 @@ static struct block2mtd_dev *add_device(char *devname, uint32_t erase_size,
>         /* Get a handle on the device */
>         bdev = blkdev_get_by_path(devname, mode, dev);
>
> +       /* Try fallback to read only mode */
> +       if (IS_ERR(bdev)) {
> +               bdev = blkdev_get_by_path(devname, FMODE_READ, dev);
> +               if (!IS_ERR(bdev))
> +                       dev->ro_mode = true;
> +       }
> +

Please use bdev_read_only() instead of blindly trying again with another mode.

>  #ifndef MODULE
>         /*
>          * We might not have the root device mounted at this point.
> @@ -261,6 +272,13 @@ static struct block2mtd_dev *add_device(char *devname, uint32_t erase_size,
>                 if (!devt)
>                         continue;
>                 bdev = blkdev_get_by_dev(devt, mode, dev);
> +
> +               /* Try fallback to read only mode */
> +               if (IS_ERR(bdev)) {
> +                       bdev = blkdev_get_by_path(devname, FMODE_READ, dev);
> +                       if (!IS_ERR(bdev))
> +                               dev->ro_mode = true;
> +               }

Same here.

>         }
>  #endif
>
> @@ -295,16 +313,22 @@ static struct block2mtd_dev *add_device(char *devname, uint32_t erase_size,
>
>         dev->mtd.name = name;
>
> +       if (dev->ro_mode) {
> +               dev->mtd.type = MTD_ROM;
> +               dev->mtd.flags = MTD_CAP_ROM;
> +       } else {
> +               dev->mtd.type = MTD_RAM;
> +               dev->mtd.flags = MTD_CAP_RAM;
> +               dev->mtd._erase = block2mtd_erase;
> +               dev->mtd._write = block2mtd_write;
> +               dev->mtd._sync = block2mtd_sync;
> +       }
> +
>         dev->mtd.size = dev->blkdev->bd_inode->i_size & PAGE_MASK;
>         dev->mtd.erasesize = erase_size;
>         dev->mtd.writesize = write_size;
>         dev->mtd.subpage_sft = subpage_sft;
>         dev->mtd.writebufsize = PAGE_SIZE;
> -       dev->mtd.type = MTD_RAM;
> -       dev->mtd.flags = MTD_CAP_RAM;
> -       dev->mtd._erase = block2mtd_erase;
> -       dev->mtd._write = block2mtd_write;
> -       dev->mtd._sync = block2mtd_sync;
>         dev->mtd._read = block2mtd_read;
>         dev->mtd.priv = dev;
>         dev->mtd.owner = THIS_MODULE;
> @@ -315,9 +339,10 @@ static struct block2mtd_dev *add_device(char *devname, uint32_t erase_size,
>         }
>
>         list_add(&dev->list, &blkmtd_device_list);
> -       pr_info("mtd%d: [%s] erase_size = %dKiB [%d], write_size = %dKiB [%d], subpage_sft = %d\n",
> +       pr_info("mtd%d: [%s] mode = %s, erase_size = %dKiB [%d], write_size = %dKiB [%d], subpage_sft = %d\n",
>                 dev->mtd.index,
>                 dev->mtd.name + strlen("block2mtd: "),
> +               (dev->ro_mode ? "RO" : "R/W"),
>                 dev->mtd.erasesize >> 10, dev->mtd.erasesize,
>                 dev->mtd.writesize >> 10, dev->mtd.writesize,
>                 dev->mtd.subpage_sft);
> --
> 1.7.9.5
>
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/



-- 
Thanks,
//richard

Re: [PATCH 4/5] mtd: block2mtd: Add support for deleting block2mtd mapping

[Richard Weinberger]

Pali,

On Fri, Jun 2, 2017 at 5:43 PM, Pali Rohár <pali.rohar@gmail.com> wrote:
> This patch allows user to delete block2mtd mapping via parameters file
> /sys/module/block2mtd/parameters/block2mtd
>
> Syntax is "del=device", e.g.:
>
> $ echo -n del=/dev/loop0 > /sys/module/block2mtd/parameters/block2mtd

As I wrote in an earlier mail, I hate this interface.
I suggest adding a decent ioctl based interface for dynamic block2mtd's.

-- 
Thanks,
//richard

Re: [PATCH 5/5] ubi: Allow to use read-only UBI volume with not enough PEBs

[Richard Weinberger]

Pali,

On Fri, Jun 2, 2017 at 5:43 PM, Pali Rohár <pali.rohar@gmail.com> wrote:
> In read-only mode is skipped auto-resize. For pre-build images ready for
> auto-resize there can be reserved more PEBs as whole size of pre-build
> image. In read-only we do not do any write operation therefore this would
> allow to use read-only UBI volume which is not auto-resized yet.
>
> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> ---
>  drivers/mtd/ubi/vtbl.c |   14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/mtd/ubi/vtbl.c b/drivers/mtd/ubi/vtbl.c
> index 263743e..1d708c5 100644
> --- a/drivers/mtd/ubi/vtbl.c
> +++ b/drivers/mtd/ubi/vtbl.c
> @@ -240,8 +240,10 @@ static int vtbl_check(const struct ubi_device *ubi,
>                 if (reserved_pebs > ubi->good_peb_count) {
>                         ubi_err(ubi, "too large reserved_pebs %d, good PEBs %d",
>                                 reserved_pebs, ubi->good_peb_count);
> -                       err = 9;
> -                       goto bad;
> +                       if (!ubi->ro_mode) {
> +                               err = 9;
> +                               goto bad;
> +                       }

I fear this is not correct, it will disable a legit self-check of UBI volumes.
If the read-only volume is corrupted/truncated and you miss PEBs, this
check will no longer
trigger.

Especially when dealing with nanddumps, truncation is a common problem.

-- 
Thanks,
//richard

Re: [PATCH 4/5] mtd: block2mtd: Add support for deleting block2mtd mapping

[Pali Rohár]

On Friday 21 July 2017 21:56:41 Richard Weinberger wrote:
> Pali,
> 
> On Fri, Jun 2, 2017 at 5:43 PM, Pali Rohár <pali.rohar@gmail.com> wrote:
> > This patch allows user to delete block2mtd mapping via parameters file
> > /sys/module/block2mtd/parameters/block2mtd
> >
> > Syntax is "del=device", e.g.:
> >
> > $ echo -n del=/dev/loop0 > /sys/module/block2mtd/parameters/block2mtd
> 
> As I wrote in an earlier mail, I hate this interface.
> I suggest adding a decent ioctl based interface for dynamic block2mtd's.

ioctl interface has a problem that cannot be easily used from shell. It
needs to have another tool which uses it. On the other hand sysfs
interface can be used from bash script which is better.

Any idea for better interface without need for ioctl?

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH 5/5] ubi: Allow to use read-only UBI volume with not enough PEBs

[Pali Rohár]

On Friday 21 July 2017 22:12:51 Richard Weinberger wrote:
> Pali,
> 
> On Fri, Jun 2, 2017 at 5:43 PM, Pali Rohár <pali.rohar@gmail.com> wrote:
> > In read-only mode is skipped auto-resize. For pre-build images ready for
> > auto-resize there can be reserved more PEBs as whole size of pre-build
> > image. In read-only we do not do any write operation therefore this would
> > allow to use read-only UBI volume which is not auto-resized yet.
> >
> > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> > ---
> >  drivers/mtd/ubi/vtbl.c |   14 +++++++++-----
> >  1 file changed, 9 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/mtd/ubi/vtbl.c b/drivers/mtd/ubi/vtbl.c
> > index 263743e..1d708c5 100644
> > --- a/drivers/mtd/ubi/vtbl.c
> > +++ b/drivers/mtd/ubi/vtbl.c
> > @@ -240,8 +240,10 @@ static int vtbl_check(const struct ubi_device *ubi,
> >                 if (reserved_pebs > ubi->good_peb_count) {
> >                         ubi_err(ubi, "too large reserved_pebs %d, good PEBs %d",
> >                                 reserved_pebs, ubi->good_peb_count);
> > -                       err = 9;
> > -                       goto bad;
> > +                       if (!ubi->ro_mode) {
> > +                               err = 9;
> > +                               goto bad;
> > +                       }
> 
> I fear this is not correct, it will disable a legit self-check of UBI volumes.
> If the read-only volume is corrupted/truncated and you miss PEBs, this
> check will no longer
> trigger.
> 
> Especially when dealing with nanddumps, truncation is a common problem.

Any idea how to fix it? Or how to handle read-only images which are
marked for auto-resize?

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH 3/5] mtd: block2mtd: Fallback to read-only mode

[Pali Rohár]

On Friday 21 July 2017 21:53:26 Richard Weinberger wrote:
> > @@ -240,6 +244,13 @@ static struct block2mtd_dev *add_device(char *devname, uint32_t erase_size,
> >         /* Get a handle on the device */
> >         bdev = blkdev_get_by_path(devname, mode, dev);
> >
> > +       /* Try fallback to read only mode */
> > +       if (IS_ERR(bdev)) {
> > +               bdev = blkdev_get_by_path(devname, FMODE_READ, dev);
> > +               if (!IS_ERR(bdev))
> > +                       dev->ro_mode = true;
> > +       }
> > +
> 
> Please use bdev_read_only() instead of blindly trying again with another mode.
> 
> >  #ifndef MODULE
> >         /*
> >          * We might not have the root device mounted at this point.
> > @@ -261,6 +272,13 @@ static struct block2mtd_dev *add_device(char *devname, uint32_t erase_size,
> >                 if (!devt)
> >                         continue;
> >                 bdev = blkdev_get_by_dev(devt, mode, dev);
> > +
> > +               /* Try fallback to read only mode */
> > +               if (IS_ERR(bdev)) {
> > +                       bdev = blkdev_get_by_path(devname, FMODE_READ, dev);
> > +                       if (!IS_ERR(bdev))
> > +                               dev->ro_mode = true;
> > +               }
> 
> Same here.

Ok, I will change it.

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH 4/5] mtd: block2mtd: Add support for deleting block2mtd mapping

[Richard Weinberger]

Pali,

Am 25.07.2017 um 16:24 schrieb Pali Rohár:
> On Friday 21 July 2017 21:56:41 Richard Weinberger wrote:
>> Pali,
>>
>> On Fri, Jun 2, 2017 at 5:43 PM, Pali Rohár <pali.rohar@gmail.com> wrote:
>>> This patch allows user to delete block2mtd mapping via parameters file
>>> /sys/module/block2mtd/parameters/block2mtd
>>>
>>> Syntax is "del=device", e.g.:
>>>
>>> $ echo -n del=/dev/loop0 > /sys/module/block2mtd/parameters/block2mtd
>>
>> As I wrote in an earlier mail, I hate this interface.
>> I suggest adding a decent ioctl based interface for dynamic block2mtd's.
> 
> ioctl interface has a problem that cannot be easily used from shell. It
> needs to have another tool which uses it. On the other hand sysfs
> interface can be used from bash script which is better.
> 
> Any idea for better interface without need for ioctl?

No, please use ioctl and add a proper tool to mtd-utils.
in MTD we use ioctl for all non-trivial interfaces. :-)

Thanks,
//richard

Re: [PATCH 5/5] ubi: Allow to use read-only UBI volume with not enough PEBs

[Richard Weinberger]

Pali,

Am 25.07.2017 um 16:27 schrieb Pali Rohár:
>> I fear this is not correct, it will disable a legit self-check of UBI volumes.
>> If the read-only volume is corrupted/truncated and you miss PEBs, this
>> check will no longer
>> trigger.
>>
>> Especially when dealing with nanddumps, truncation is a common problem.
> 
> Any idea how to fix it? Or how to handle read-only images which are
> marked for auto-resize?

I'd vote for rejecting images that have auto-resize set when the MTD is read-only.
In fact, using UBI on top of a read-only MTD is very uncommon and not recommended (for NAND).
The auto-resize flag should be also only set when you just have created it using mkfs.ubifs.
Why would you inspect such an image with the kernel UBIFS unless you're hunting down a bug
in mkfs.ubifs?

Thanks,
//richard

Re: [PATCH 5/5] ubi: Allow to use read-only UBI volume with not enough PEBs

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 1190 bytes --]

On Sunday 06 August 2017 11:43:25 Richard Weinberger wrote:
> Pali,
> 
> Am 25.07.2017 um 16:27 schrieb Pali Rohár:
> >> I fear this is not correct, it will disable a legit self-check of
> >> UBI volumes. If the read-only volume is corrupted/truncated and
> >> you miss PEBs, this check will no longer
> >> trigger.
> >> 
> >> Especially when dealing with nanddumps, truncation is a common
> >> problem.
> > 
> > Any idea how to fix it? Or how to handle read-only images which are
> > marked for auto-resize?
> 
> I'd vote for rejecting images that have auto-resize set when the MTD
> is read-only. In fact, using UBI on top of a read-only MTD is very
> uncommon and not recommended (for NAND). The auto-resize flag should
> be also only set when you just have created it using mkfs.ubifs. Why
> would you inspect such an image with the kernel UBIFS unless you're
> hunting down a bug in mkfs.ubifs?
> 
> Thanks,
> //richard

E.g. because when I get UBIFS image and I want to unpack it.

IMO UBIFS image which have auto-resize set is also valid UBIFS image and 
kernel should be able to read it too, even in R/O mode.

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]