* RFC: decouple block2mtd from block layer internals @ 2021-10-14 14:52 Christoph Hellwig 2021-10-14 14:52 ` [PATCH] mtd/block2mtd: don't poke into " Christoph Hellwig 0 siblings, 1 reply; 7+ messages in thread From: Christoph Hellwig @ 2021-10-14 14:52 UTC (permalink / raw) To: joern, miquel.raynal, richard, vigneshr; +Cc: linux-mtd 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/ ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] mtd/block2mtd: don't poke into block layer internals 2021-10-14 14:52 RFC: decouple block2mtd from block layer internals Christoph Hellwig @ 2021-10-14 14:52 ` Christoph Hellwig 2021-10-14 19:28 ` Richard Weinberger 0 siblings, 1 reply; 7+ messages in thread From: Christoph Hellwig @ 2021-10-14 14:52 UTC (permalink / raw) To: joern, miquel.raynal, richard, vigneshr; +Cc: linux-mtd 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/ ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] mtd/block2mtd: don't poke into block layer internals 2021-10-14 14:52 ` [PATCH] mtd/block2mtd: don't poke into " Christoph Hellwig @ 2021-10-14 19:28 ` Richard Weinberger 2021-10-15 6:28 ` hch 2021-10-19 5:53 ` hch 0 siblings, 2 replies; 7+ messages in thread From: Richard Weinberger @ 2021-10-14 19:28 UTC (permalink / raw) To: hch; +Cc: joern, Miquel Raynal, Vignesh Raghavendra, linux-mtd ----- 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/ ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] mtd/block2mtd: don't poke into block layer internals 2021-10-14 19:28 ` Richard Weinberger @ 2021-10-15 6:28 ` hch 2021-10-15 6:53 ` Miquel Raynal 2021-10-19 5:53 ` hch 1 sibling, 1 reply; 7+ messages in thread From: hch @ 2021-10-15 6:28 UTC (permalink / raw) To: Richard Weinberger Cc: hch, joern, Miquel Raynal, Vignesh Raghavendra, linux-mtd 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/ ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] mtd/block2mtd: don't poke into block layer internals 2021-10-15 6:28 ` hch @ 2021-10-15 6:53 ` Miquel Raynal 0 siblings, 0 replies; 7+ messages in thread From: Miquel Raynal @ 2021-10-15 6:53 UTC (permalink / raw) To: hch; +Cc: Richard Weinberger, joern, Vignesh Raghavendra, linux-mtd 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/ ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] mtd/block2mtd: don't poke into block layer internals 2021-10-14 19:28 ` Richard Weinberger 2021-10-15 6:28 ` hch @ 2021-10-19 5:53 ` hch 2021-10-22 7:50 ` Richard Weinberger 1 sibling, 1 reply; 7+ messages in thread From: hch @ 2021-10-19 5:53 UTC (permalink / raw) To: Richard Weinberger Cc: hch, joern, Miquel Raynal, Vignesh Raghavendra, linux-mtd 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/ ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] mtd/block2mtd: don't poke into block layer internals 2021-10-19 5:53 ` hch @ 2021-10-22 7:50 ` Richard Weinberger 0 siblings, 0 replies; 7+ messages in thread From: Richard Weinberger @ 2021-10-22 7:50 UTC (permalink / raw) To: hch; +Cc: joern, Miquel Raynal, Vignesh Raghavendra, linux-mtd ----- 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/ ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-10-22 7:51 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-10-14 14:52 RFC: decouple block2mtd from block layer internals Christoph Hellwig 2021-10-14 14:52 ` [PATCH] mtd/block2mtd: don't poke into " Christoph Hellwig 2021-10-14 19:28 ` Richard Weinberger 2021-10-15 6:28 ` hch 2021-10-15 6:53 ` Miquel Raynal 2021-10-19 5:53 ` hch 2021-10-22 7:50 ` Richard Weinberger
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.