[PATCH] mtdblock: warn if opened on NAND

[Ezequiel Garcia]

H Bjørn,

Thanks a lot for taking care of this. I know it's been a pain point for users.

On Mon, Mar 28, 2022 at 1:11 PM Bjørn Mork <bjorn at mork.no> wrote:
>
> Warning on every translated mtd partition results in excessive log noise
> if this driver is loaded:
>
>   nand: device found, Manufacturer ID: 0xc2, Chip ID: 0xf1
>   nand: Macronix MX30LF1G18AC
>   nand: 128 MiB, SLC, erase size: 128 KiB, page size: 2048, OOB size: 64
>   mt7621-nand 1e003000.nand: ECC strength adjusted to 4 bits
>   read_bbt: found bbt at block 1023
>   10 fixed-partitions partitions found on MTD device mt7621-nand
>   Creating 10 MTD partitions on "mt7621-nand":
>   0x000000000000-0x000000080000 : "Bootloader"
>   mtdblock: MTD device 'Bootloader' is NAND, please consider using UBI block devices instead.
>   0x000000080000-0x000000100000 : "Config"
>   mtdblock: MTD device 'Config' is NAND, please consider using UBI block devices instead.
>   0x000000100000-0x000000140000 : "Factory"
>   mtdblock: MTD device 'Factory' is NAND, please consider using UBI block devices instead.
>   0x000000140000-0x000002000000 : "Kernel"
>   mtdblock: MTD device 'Kernel' is NAND, please consider using UBI block devices instead.
>   0x000000540000-0x000002000000 : "ubi"
>   mtdblock: MTD device 'ubi' is NAND, please consider using UBI block devices instead.
>   0x000002140000-0x000004000000 : "Kernel2"
>   mtdblock: MTD device 'Kernel2' is NAND, please consider using UBI block devices instead.
>   0x000004000000-0x000004100000 : "wwan"
>   mtdblock: MTD device 'wwan' is NAND, please consider using UBI block devices instead.
>   0x000004100000-0x000005100000 : "data"
>   mtdblock: MTD device 'data' is NAND, please consider using UBI block devices instead.
>   0x000005100000-0x000005200000 : "rom-d"
>   mtdblock: MTD device 'rom-d' is NAND, please consider using UBI block devices instead.
>   0x000005200000-0x000005280000 : "reserve"
>   mtdblock: MTD device 'reserve' is NAND, please consider using UBI block devices instead.
>   mtk_soc_eth 1e100000.ethernet eth0: mediatek frame engine at 0xbe100000, irq 21
>
> This is more likely to annoy than to help users of embedded distros where
> this driver is enabled by default.  Making the blockdevs available does
> not imply that they are in use, and warning about bootloader partitions
> or other devices which obviously never will be mounted is more confusing
> than helpful.
>
> Move the warning to open(), where it will be of more use - actually warning
> anyone who mounts a file system on NAND using mtdblock.
>
> Fixes: e07403a8c6be ("mtdblock: Warn if added for a NAND device")
> Signed-off-by: Bjørn Mork <bjorn at mork.no>
> ---
> I don't know what to do about the mtdblock_ro part of the original
> patch since it doesn't have an open hook. Adding one just to print
> this warning seems a bit like overkill...  Maybe just drop the
> warning there? What harm is there accessing NAND devices anyway?
>

If I remember correctly, rhe reason for adding all these warnings came
after getting questions from developers unaware of UBI and UBI block.
These users/developers were building  products with
read-only filesystem on top of MTD block, on NAND devices.

I believe mtdblock itself isn't as needed as it used to be,
given you can mount JFFS2 directly on a MTD char device.

Internet's trucks remain heavily loaded with documentation and
tutorials pointing people to mtdblock. So the idea was to balance
that with some self-documentation in the form of a warning.

I think adding an mtdblock_ro open hook is totally justified if it points
users in the right direction. Alternatively, we can move the warning
to the upper mtd_blkdevs.c layer, and put it in blktrans_open.

mtd_blkdevs.c is used by some other drivers so maybe that's not desirable.

Regards,
Ezequiel

> Leaving that decision up to you experts :-)
>
> I'd really like to see this fix get into stable before the masses
> start noticing now that OpenWrt is moving to 5.15.  As you may or
> not know, OpenWrt unconditionally includes mtdblock whether the
> target is NAND or NOR.  So this will cause lots of noise on any
> NAND OpenWrt device.
>



>
> Bjørn
>
>  drivers/mtd/mtdblock.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/mtd/mtdblock.c b/drivers/mtd/mtdblock.c
> index 03e3de3a5d79..1e94e7d10b8b 100644
> --- a/drivers/mtd/mtdblock.c
> +++ b/drivers/mtd/mtdblock.c
> @@ -257,6 +257,10 @@ static int mtdblock_open(struct mtd_blktrans_dev *mbd)
>                 return 0;
>         }
>
> +       if (mtd_type_is_nand(mbd->mtd))
> +               pr_warn("%s: MTD device '%s' is NAND, please consider using UBI block devices instead.\n",
> +                       mbd->tr->name, mbd->mtd->name);
> +
>         /* OK, it's not open. Create cache info for it */
>         mtdblk->count = 1;
>         mutex_init(&mtdblk->cache_mutex);
> @@ -322,10 +326,6 @@ static void mtdblock_add_mtd(struct mtd_blktrans_ops *tr, struct mtd_info *mtd)
>         if (!(mtd->flags & MTD_WRITEABLE))
>                 dev->mbd.readonly = 1;
>
> -       if (mtd_type_is_nand(mtd))
> -               pr_warn("%s: MTD device '%s' is NAND, please consider using UBI block devices instead.\n",
> -                       tr->name, mtd->name);
> -
>         if (add_mtd_blktrans_dev(&dev->mbd))
>                 kfree(dev);
>  }
> --
> 2.30.2
>