[PATCH] mtd/block2mtd: don't poke into block layer internals
Richard Weinberger
richard at nod.at
Thu Oct 14 12:28:19 PDT 2021
----- Ursprüngliche Mail -----
> Von: "hch" <hch at lst.de>
> An: joern at lazybastard.org, "Miquel Raynal" <miquel.raynal at bootlin.com>, "richard" <richard at nod.at>, "Vignesh
> Raghavendra" <vigneshr at ti.com>
> CC: "linux-mtd" <linux-mtd at 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 at 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.
More information about the linux-mtd
mailing list