RFC: decouple block2mtd from block layer internals

RFC: decouple block2mtd from block layer internals

[Christoph Hellwig]

Hi all,

while looking for abuses of ->bd_inode, block2mtd scored high on the list.
This completely UNTESTED patch tries to move it off that and use the proper
file abstraction instead.  Are there any current users of this code that
could give it a spin?



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

[PATCH] mtd/block2mtd: don't poke into block layer internals

[Christoph Hellwig]

block2mtd is a bit of a mess as it pokes directly into the block layer
internal page cache.  Switch it to use the proper file abstraction
instead.  Note that this contains a small behavior change in that erase
now unconditionally writes all Fs instead of first scanning for them.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/mtd/devices/block2mtd.c | 203 ++++++++------------------------
 1 file changed, 48 insertions(+), 155 deletions(-)

diff --git a/drivers/mtd/devices/block2mtd.c b/drivers/mtd/devices/block2mtd.c
index c08721b11642b..ea8ec9956c3e8 100644
--- a/drivers/mtd/devices/block2mtd.c
+++ b/drivers/mtd/devices/block2mtd.c
@@ -34,7 +34,7 @@
 /* Info for the block device */
 struct block2mtd_dev {
 	struct list_head list;
-	struct block_device *blkdev;
+	struct file *file;
 	struct mtd_info mtd;
 	struct mutex write_mutex;
 };
@@ -43,58 +43,35 @@ struct block2mtd_dev {
 /* Static info about the MTD, used in cleanup_module */
 static LIST_HEAD(blkmtd_device_list);
 
-
-static struct page *page_read(struct address_space *mapping, pgoff_t index)
-{
-	return read_mapping_page(mapping, index, NULL);
-}
+static u_long allfs[PAGE_SIZE / sizeof(u_long)] = {
+	[0 ... (PAGE_SIZE / sizeof(u_long)) - 1] = -1UL,
+};
 
 /* erase a specified part of the device */
-static int _block2mtd_erase(struct block2mtd_dev *dev, loff_t to, size_t len)
-{
-	struct address_space *mapping = dev->blkdev->bd_inode->i_mapping;
-	struct page *page;
-	pgoff_t index = to >> PAGE_SHIFT;	// page index
-	int pages = len >> PAGE_SHIFT;
-	u_long *p;
-	u_long *max;
-
-	while (pages) {
-		page = page_read(mapping, index);
-		if (IS_ERR(page))
-			return PTR_ERR(page);
-
-		max = page_address(page) + PAGE_SIZE;
-		for (p=page_address(page); p<max; p++)
-			if (*p != -1UL) {
-				lock_page(page);
-				memset(page_address(page), 0xff, PAGE_SIZE);
-				set_page_dirty(page);
-				unlock_page(page);
-				balance_dirty_pages_ratelimited(mapping);
-				break;
-			}
-
-		put_page(page);
-		pages--;
-		index++;
-	}
-	return 0;
-}
 static int block2mtd_erase(struct mtd_info *mtd, struct erase_info *instr)
 {
 	struct block2mtd_dev *dev = mtd->priv;
-	size_t from = instr->addr;
-	size_t len = instr->len;
-	int err;
+	loff_t end = instr->addr + instr->len;
+	size_t total_len = instr->len;
+	loff_t from = instr->addr;
+	ssize_t ret;
 
 	mutex_lock(&dev->write_mutex);
-	err = _block2mtd_erase(dev, from, len);
+	do {
+		size_t len = min_t(size_t, total_len, PAGE_SIZE);
+
+		ret = kernel_write(dev->file, allfs, len, &from);
+		if (ret < 0)
+			goto fail;
+		total_len -= ret;
+	} while (from < end);
 	mutex_unlock(&dev->write_mutex);
-	if (err)
-		pr_err("erase failed err = %d\n", err);
 
-	return err;
+	return 0;
+
+fail:
+	pr_err("erase failed err = %zd\n", ret);
+	return ret;
 }
 
 
@@ -102,72 +79,12 @@ static int block2mtd_read(struct mtd_info *mtd, loff_t from, size_t len,
 		size_t *retlen, u_char *buf)
 {
 	struct block2mtd_dev *dev = mtd->priv;
-	struct page *page;
-	pgoff_t index = from >> PAGE_SHIFT;
-	int offset = from & (PAGE_SIZE-1);
-	int cpylen;
-
-	while (len) {
-		if ((offset + len) > PAGE_SIZE)
-			cpylen = PAGE_SIZE - offset;	// multiple pages
-		else
-			cpylen = len;	// this page
-		len = len - cpylen;
-
-		page = page_read(dev->blkdev->bd_inode->i_mapping, index);
-		if (IS_ERR(page))
-			return PTR_ERR(page);
-
-		memcpy(buf, page_address(page) + offset, cpylen);
-		put_page(page);
-
-		if (retlen)
-			*retlen += cpylen;
-		buf += cpylen;
-		offset = 0;
-		index++;
-	}
-	return 0;
-}
+	ssize_t ret;
 
-
-/* write data to the underlying device */
-static int _block2mtd_write(struct block2mtd_dev *dev, const u_char *buf,
-		loff_t to, size_t len, size_t *retlen)
-{
-	struct page *page;
-	struct address_space *mapping = dev->blkdev->bd_inode->i_mapping;
-	pgoff_t index = to >> PAGE_SHIFT;	// page index
-	int offset = to & ~PAGE_MASK;	// page offset
-	int cpylen;
-
-	while (len) {
-		if ((offset+len) > PAGE_SIZE)
-			cpylen = PAGE_SIZE - offset;	// multiple pages
-		else
-			cpylen = len;			// this page
-		len = len - cpylen;
-
-		page = page_read(mapping, index);
-		if (IS_ERR(page))
-			return PTR_ERR(page);
-
-		if (memcmp(page_address(page)+offset, buf, cpylen)) {
-			lock_page(page);
-			memcpy(page_address(page) + offset, buf, cpylen);
-			set_page_dirty(page);
-			unlock_page(page);
-			balance_dirty_pages_ratelimited(mapping);
-		}
-		put_page(page);
-
-		if (retlen)
-			*retlen += cpylen;
-
-		buf += cpylen;
-		offset = 0;
-		index++;
-	}
+	ret = kernel_read(dev->file, buf, len, &from);
+	if (ret < 0)
+		return ret;
+	*retlen += ret;
 	return 0;
 }
 
@@ -176,14 +93,17 @@ static int block2mtd_write(struct mtd_info *mtd, loff_t to, size_t len,
 		size_t *retlen, const u_char *buf)
 {
 	struct block2mtd_dev *dev = mtd->priv;
-	int err;
+	ssize_t ret;
 
 	mutex_lock(&dev->write_mutex);
-	err = _block2mtd_write(dev, buf, to, len, retlen);
+	ret = kernel_write(dev->file, buf, len, &to);
+	if (ret > 0) {
+		*retlen += ret;
+		ret = 0;
+	}
 	mutex_unlock(&dev->write_mutex);
-	if (err > 0)
-		err = 0;
-	return err;
+
+	return ret;
 }
 
 
@@ -191,8 +111,8 @@ static int block2mtd_write(struct mtd_info *mtd, loff_t to, size_t len,
 static void block2mtd_sync(struct mtd_info *mtd)
 {
 	struct block2mtd_dev *dev = mtd->priv;
-	sync_blockdev(dev->blkdev);
-	return;
+
+	vfs_fsync(dev->file, 1);
 }
 
 
@@ -203,10 +123,9 @@ static void block2mtd_free_device(struct block2mtd_dev *dev)
 
 	kfree(dev->mtd.name);
 
-	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->file) {
+		vfs_fsync(dev->file, 1);
+		fput(dev->file);
 	}
 
 	kfree(dev);
@@ -216,11 +135,6 @@ static void block2mtd_free_device(struct block2mtd_dev *dev)
 static struct block2mtd_dev *add_device(char *devname, int erase_size,
 		int timeout)
 {
-#ifndef MODULE
-	int i;
-#endif
-	const fmode_t mode = FMODE_READ | FMODE_WRITE | FMODE_EXCL;
-	struct block_device *bdev;
 	struct block2mtd_dev *dev;
 	char *name;
 
@@ -231,45 +145,24 @@ static struct block2mtd_dev *add_device(char *devname, int erase_size,
 	if (!dev)
 		return NULL;
 
-	/* Get a handle on the device */
-	bdev = blkdev_get_by_path(devname, mode, dev);
-
-#ifndef MODULE
-	/*
-	 * We might not have the root device mounted at this point.
-	 * Try to resolve the device name by other means.
-	 */
-	for (i = 0; IS_ERR(bdev) && i <= timeout; i++) {
-		dev_t devt;
-
-		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);
+	dev->file = filp_open(devname, O_RDWR | O_EXCL, 0);
+	if (IS_ERR(dev->file)) {
+		dev->file = NULL;
+		pr_err("error: cannot open device %s\n", devname);
+		goto err_free_block2mtd;
 	}
-#endif
 
-	if (IS_ERR(bdev)) {
+	if (!S_ISBLK(file_inode(dev->file)->i_mode)) {
 		pr_err("error: cannot open device %s\n", devname);
 		goto err_free_block2mtd;
 	}
-	dev->blkdev = bdev;
 
-	if (MAJOR(bdev->bd_dev) == MTD_BLOCK_MAJOR) {
+	if (MAJOR(file_inode(dev->file)->i_rdev) == MTD_BLOCK_MAJOR) {
 		pr_err("attempting to use an MTD device as a block device\n");
 		goto err_free_block2mtd;
 	}
 
-	if ((long)dev->blkdev->bd_inode->i_size % erase_size) {
+	if ((long)i_size_read(dev->file->f_mapping->host) % erase_size) {
 		pr_err("erasesize must be a divisor of device size\n");
 		goto err_free_block2mtd;
 	}
@@ -284,7 +177,7 @@ static struct block2mtd_dev *add_device(char *devname, int erase_size,
 
 	dev->mtd.name = name;
 
-	dev->mtd.size = dev->blkdev->bd_inode->i_size & PAGE_MASK;
+	dev->mtd.size = i_size_read(dev->file->f_mapping->host) & PAGE_MASK;
 	dev->mtd.erasesize = erase_size;
 	dev->mtd.writesize = 1;
 	dev->mtd.writebufsize = PAGE_SIZE;
-- 
2.30.2


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

Re: [PATCH] mtd/block2mtd: don't poke into block layer internals

[Richard Weinberger]

----- Ursprüngliche Mail -----
> Von: "hch" <hch@lst.de>
> An: joern@lazybastard.org, "Miquel Raynal" <miquel.raynal@bootlin.com>, "richard" <richard@nod.at>, "Vignesh
> Raghavendra" <vigneshr@ti.com>
> CC: "linux-mtd" <linux-mtd@lists.infradead.org>
> Gesendet: Donnerstag, 14. Oktober 2021 16:52:06
> Betreff: [PATCH] mtd/block2mtd: don't poke into block layer internals

> block2mtd is a bit of a mess as it pokes directly into the block layer
> internal page cache.  Switch it to use the proper file abstraction

Nice cleanup! :-)

I guess the cleanup can also be applied to nandsim. While it does not touch
the block layer, it still messes with the page cache.

> instead.  Note that this contains a small behavior change in that erase
> now unconditionally writes all Fs instead of first scanning for them.

Unless you have a strong opinoin I'd like to keep the scanning.
The original use case of block2mtd is using Compact Flash (ATA)
as MTD. Some of this devices are super stupid and I fear the 0xFF scanning
is here to avoid programming 0xFF bytes into the NAND.
Just to be on the safe side...

> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> drivers/mtd/devices/block2mtd.c | 203 ++++++++------------------------
> 1 file changed, 48 insertions(+), 155 deletions(-)
> 
> diff --git a/drivers/mtd/devices/block2mtd.c b/drivers/mtd/devices/block2mtd.c
> index c08721b11642b..ea8ec9956c3e8 100644
> --- a/drivers/mtd/devices/block2mtd.c
> +++ b/drivers/mtd/devices/block2mtd.c
> @@ -34,7 +34,7 @@
> /* Info for the block device */
> struct block2mtd_dev {
> 	struct list_head list;
> -	struct block_device *blkdev;
> +	struct file *file;
> 	struct mtd_info mtd;
> 	struct mutex write_mutex;
> };
> @@ -43,58 +43,35 @@ struct block2mtd_dev {
> /* Static info about the MTD, used in cleanup_module */
> static LIST_HEAD(blkmtd_device_list);
> 
> -
> -static struct page *page_read(struct address_space *mapping, pgoff_t index)
> -{
> -	return read_mapping_page(mapping, index, NULL);
> -}
> +static u_long allfs[PAGE_SIZE / sizeof(u_long)] = {
> +	[0 ... (PAGE_SIZE / sizeof(u_long)) - 1] = -1UL,
> +};
> 
> /* erase a specified part of the device */
> -static int _block2mtd_erase(struct block2mtd_dev *dev, loff_t to, size_t len)
> -{
> -	struct address_space *mapping = dev->blkdev->bd_inode->i_mapping;
> -	struct page *page;
> -	pgoff_t index = to >> PAGE_SHIFT;	// page index
> -	int pages = len >> PAGE_SHIFT;
> -	u_long *p;
> -	u_long *max;
> -
> -	while (pages) {
> -		page = page_read(mapping, index);
> -		if (IS_ERR(page))
> -			return PTR_ERR(page);
> -
> -		max = page_address(page) + PAGE_SIZE;
> -		for (p=page_address(page); p<max; p++)
> -			if (*p != -1UL) {
> -				lock_page(page);
> -				memset(page_address(page), 0xff, PAGE_SIZE);
> -				set_page_dirty(page);
> -				unlock_page(page);
> -				balance_dirty_pages_ratelimited(mapping);
> -				break;
> -			}
> -
> -		put_page(page);
> -		pages--;
> -		index++;
> -	}
> -	return 0;
> -}
> static int block2mtd_erase(struct mtd_info *mtd, struct erase_info *instr)
> {
> 	struct block2mtd_dev *dev = mtd->priv;
> -	size_t from = instr->addr;
> -	size_t len = instr->len;
> -	int err;
> +	loff_t end = instr->addr + instr->len;
> +	size_t total_len = instr->len;
> +	loff_t from = instr->addr;
> +	ssize_t ret;
> 
> 	mutex_lock(&dev->write_mutex);
> -	err = _block2mtd_erase(dev, from, len);
> +	do {
> +		size_t len = min_t(size_t, total_len, PAGE_SIZE);
> +
> +		ret = kernel_write(dev->file, allfs, len, &from);
> +		if (ret < 0)
> +			goto fail;
> +		total_len -= ret;
> +	} while (from < end);
> 	mutex_unlock(&dev->write_mutex);
> -	if (err)
> -		pr_err("erase failed err = %d\n", err);
> 
> -	return err;
> +	return 0;
> +
> +fail:
> +	pr_err("erase failed err = %zd\n", ret);
> +	return ret;
> }
> 
> 
> @@ -102,72 +79,12 @@ static int block2mtd_read(struct mtd_info *mtd, loff_t
> from, size_t len,
> 		size_t *retlen, u_char *buf)
> {
> 	struct block2mtd_dev *dev = mtd->priv;
> -	struct page *page;
> -	pgoff_t index = from >> PAGE_SHIFT;
> -	int offset = from & (PAGE_SIZE-1);
> -	int cpylen;
> -
> -	while (len) {
> -		if ((offset + len) > PAGE_SIZE)
> -			cpylen = PAGE_SIZE - offset;	// multiple pages
> -		else
> -			cpylen = len;	// this page
> -		len = len - cpylen;
> -
> -		page = page_read(dev->blkdev->bd_inode->i_mapping, index);
> -		if (IS_ERR(page))
> -			return PTR_ERR(page);
> -
> -		memcpy(buf, page_address(page) + offset, cpylen);
> -		put_page(page);
> -
> -		if (retlen)
> -			*retlen += cpylen;
> -		buf += cpylen;
> -		offset = 0;
> -		index++;
> -	}
> -	return 0;
> -}
> +	ssize_t ret;
> 
> -
> -/* write data to the underlying device */
> -static int _block2mtd_write(struct block2mtd_dev *dev, const u_char *buf,
> -		loff_t to, size_t len, size_t *retlen)
> -{
> -	struct page *page;
> -	struct address_space *mapping = dev->blkdev->bd_inode->i_mapping;
> -	pgoff_t index = to >> PAGE_SHIFT;	// page index
> -	int offset = to & ~PAGE_MASK;	// page offset
> -	int cpylen;
> -
> -	while (len) {
> -		if ((offset+len) > PAGE_SIZE)
> -			cpylen = PAGE_SIZE - offset;	// multiple pages
> -		else
> -			cpylen = len;			// this page
> -		len = len - cpylen;
> -
> -		page = page_read(mapping, index);
> -		if (IS_ERR(page))
> -			return PTR_ERR(page);
> -
> -		if (memcmp(page_address(page)+offset, buf, cpylen)) {
> -			lock_page(page);
> -			memcpy(page_address(page) + offset, buf, cpylen);
> -			set_page_dirty(page);
> -			unlock_page(page);
> -			balance_dirty_pages_ratelimited(mapping);
> -		}
> -		put_page(page);
> -
> -		if (retlen)
> -			*retlen += cpylen;
> -
> -		buf += cpylen;
> -		offset = 0;
> -		index++;
> -	}
> +	ret = kernel_read(dev->file, buf, len, &from);
> +	if (ret < 0)
> +		return ret;
> +	*retlen += ret;
> 	return 0;
> }
> 
> @@ -176,14 +93,17 @@ static int block2mtd_write(struct mtd_info *mtd, loff_t to,
> size_t len,
> 		size_t *retlen, const u_char *buf)
> {
> 	struct block2mtd_dev *dev = mtd->priv;
> -	int err;
> +	ssize_t ret;
> 
> 	mutex_lock(&dev->write_mutex);
> -	err = _block2mtd_write(dev, buf, to, len, retlen);
> +	ret = kernel_write(dev->file, buf, len, &to);
> +	if (ret > 0) {
> +		*retlen += ret;
> +		ret = 0;
> +	}
> 	mutex_unlock(&dev->write_mutex);
> -	if (err > 0)
> -		err = 0;
> -	return err;
> +
> +	return ret;
> }
> 
> 
> @@ -191,8 +111,8 @@ static int block2mtd_write(struct mtd_info *mtd, loff_t to,
> size_t len,
> static void block2mtd_sync(struct mtd_info *mtd)
> {
> 	struct block2mtd_dev *dev = mtd->priv;
> -	sync_blockdev(dev->blkdev);
> -	return;
> +
> +	vfs_fsync(dev->file, 1);
> }
> 
> 
> @@ -203,10 +123,9 @@ static void block2mtd_free_device(struct block2mtd_dev
> *dev)
> 
> 	kfree(dev->mtd.name);
> 
> -	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->file) {
> +		vfs_fsync(dev->file, 1);
> +		fput(dev->file);
> 	}
> 
> 	kfree(dev);
> @@ -216,11 +135,6 @@ static void block2mtd_free_device(struct block2mtd_dev
> *dev)
> static struct block2mtd_dev *add_device(char *devname, int erase_size,
> 		int timeout)
> {
> -#ifndef MODULE
> -	int i;
> -#endif
> -	const fmode_t mode = FMODE_READ | FMODE_WRITE | FMODE_EXCL;
> -	struct block_device *bdev;
> 	struct block2mtd_dev *dev;
> 	char *name;
> 
> @@ -231,45 +145,24 @@ static struct block2mtd_dev *add_device(char *devname, int
> erase_size,
> 	if (!dev)
> 		return NULL;
> 
> -	/* Get a handle on the device */
> -	bdev = blkdev_get_by_path(devname, mode, dev);
> -
> -#ifndef MODULE
> -	/*
> -	 * We might not have the root device mounted at this point.
> -	 * Try to resolve the device name by other means.
> -	 */
> -	for (i = 0; IS_ERR(bdev) && i <= timeout; i++) {
> -		dev_t devt;
> -
> -		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);
> +	dev->file = filp_open(devname, O_RDWR | O_EXCL, 0);
> +	if (IS_ERR(dev->file)) {
> +		dev->file = NULL;
> +		pr_err("error: cannot open device %s\n", devname);
> +		goto err_free_block2mtd;
> 	}
> -#endif
> 
> -	if (IS_ERR(bdev)) {
> +	if (!S_ISBLK(file_inode(dev->file)->i_mode)) {

I think we can actually weaken that check to allow regular files too.
Then one can directly use a file as backend. These days people use block2mtd
sometimes with a file backend via a loop device.

What do you think?

Thanks,
//richard

P.s: While reading this driver I found another issue. There is no way to remove an MTD at runtime.
Miquel, what do you think? Shall we limit block2mtd to one MTD?
The current interface via module parameters is horrible.

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

Re: [PATCH] mtd/block2mtd: don't poke into block layer internals

[hch]

On Thu, Oct 14, 2021 at 09:28:19PM +0200, Richard Weinberger wrote:
> > instead.  Note that this contains a small behavior change in that erase
> > now unconditionally writes all Fs instead of first scanning for them.
> 
> Unless you have a strong opinoin I'd like to keep the scanning.
> The original use case of block2mtd is using Compact Flash (ATA)
> as MTD. Some of this devices are super stupid and I fear the 0xFF scanning
> is here to avoid programming 0xFF bytes into the NAND.
> Just to be on the safe side...

Hmm.  Doing the right first is quite a bit of overhead, especially as
unlike the direct page cache access we can't just poke into the page
without copying it.

> I think we can actually weaken that check to allow regular files too.
> Then one can directly use a file as backend. These days people use block2mtd
> sometimes with a file backend via a loop device.

Yes, a file backend will work just fine.

> P.s: While reading this driver I found another issue. There is no way to remove an MTD at runtime.
> Miquel, what do you think? Shall we limit block2mtd to one MTD?
> The current interface via module parameters is horrible.
---end quoted text---

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

Re: [PATCH] mtd/block2mtd: don't poke into block layer internals

[Miquel Raynal]

Hi Richard,

hch@lst.de wrote on Fri, 15 Oct 2021 08:28:09 +0200:

> On Thu, Oct 14, 2021 at 09:28:19PM +0200, Richard Weinberger wrote:
> > > instead.  Note that this contains a small behavior change in that erase
> > > now unconditionally writes all Fs instead of first scanning for them.  
> > 
> > Unless you have a strong opinoin I'd like to keep the scanning.
> > The original use case of block2mtd is using Compact Flash (ATA)
> > as MTD. Some of this devices are super stupid and I fear the 0xFF scanning
> > is here to avoid programming 0xFF bytes into the NAND.
> > Just to be on the safe side...  
> 
> Hmm.  Doing the right first is quite a bit of overhead, especially as
> unlike the direct page cache access we can't just poke into the page
> without copying it.
> 
> > I think we can actually weaken that check to allow regular files too.
> > Then one can directly use a file as backend. These days people use block2mtd
> > sometimes with a file backend via a loop device.  
> 
> Yes, a file backend will work just fine.
> 
> > P.s: While reading this driver I found another issue. There is no way to remove an MTD at runtime.
> > Miquel, what do you think? Shall we limit block2mtd to one MTD?
> > The current interface via module parameters is horrible.

I believe if we do this we will someday find someone who *wants*
several block2mtd devices and we'll be back to today's situation.

What about an extra parameter where you provide the mtd device to
remove it? Or perhaps a new interface if block2mtd is actually used
in another pseudo-fs? (like configfs or perhaps debugfs if this is
suitable)

Thanks,
Miquèl

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

Re: [PATCH] mtd/block2mtd: don't poke into block layer internals

[hch]

On Thu, Oct 14, 2021 at 09:28:19PM +0200, Richard Weinberger wrote:
> > instead.  Note that this contains a small behavior change in that erase
> > now unconditionally writes all Fs instead of first scanning for them.
> 
> Unless you have a strong opinoin I'd like to keep the scanning.
> The original use case of block2mtd is using Compact Flash (ATA)
> as MTD. Some of this devices are super stupid and I fear the 0xFF scanning
> is here to avoid programming 0xFF bytes into the NAND.
> Just to be on the safe side...

I played with keeping the scanning, but it really is a mess.  Shouldn't
the higher layers (ubi, jffs2, etc) know what has already been erased?

Btw, Joerns address bounces.  Does anyone knw if he still wants to
maintain block2mtd and has a new address or if he should be removed from
the maintainers list?

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

Re: [PATCH] mtd/block2mtd: don't poke into block layer internals

[Richard Weinberger]

----- Ursprüngliche Mail -----
> Von: "hch" <hch@lst.de>
> An: "richard" <richard@nod.at>
> CC: "hch" <hch@lst.de>, "joern" <joern@lazybastard.org>, "Miquel Raynal" <miquel.raynal@bootlin.com>, "Vignesh
> Raghavendra" <vigneshr@ti.com>, "linux-mtd" <linux-mtd@lists.infradead.org>
> Gesendet: Dienstag, 19. Oktober 2021 07:53:09
> Betreff: Re: [PATCH] mtd/block2mtd: don't poke into block layer internals

> On Thu, Oct 14, 2021 at 09:28:19PM +0200, Richard Weinberger wrote:
>> > instead.  Note that this contains a small behavior change in that erase
>> > now unconditionally writes all Fs instead of first scanning for them.
>> 
>> Unless you have a strong opinoin I'd like to keep the scanning.
>> The original use case of block2mtd is using Compact Flash (ATA)
>> as MTD. Some of this devices are super stupid and I fear the 0xFF scanning
>> is here to avoid programming 0xFF bytes into the NAND.
>> Just to be on the safe side...
> 
> I played with keeping the scanning, but it really is a mess.  Shouldn't
> the higher layers (ubi, jffs2, etc) know what has already been erased?

Well, the higher layers are allowed to unconditionally re-erase a block.
I had a short discussion with Miquel, since it is not entirely clear what
the purpose of the read-before-write logic in the erase handler is, let's
keep on the safe side and keep it.

Either it is a performance optimization because erase was super slow on
compact flash cards at the time when Joern created block2mtd.
Or they have been super stupid^Wsimple and writing 0xFF bytes to an erased
region (via mtd_erase) was problematic.
 
> Btw, Joerns address bounces.  Does anyone know if he still wants to
> maintain block2mtd and has a new address or if he should be removed from
> the maintainers list?

I sent a patch to remove him from the maintainers file.
If Joern returns I'll happily revert the patch.

Thanks,
//richard

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