lkml.org 
[lkml]   [2021]   [Aug]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 1/2] mtd: mtdconcat: Judge callback function existence getting from master for each partition
    Hi Zhihao,

    Zhihao Cheng <chengzhihao1@huawei.com> wrote on Sat, 31 Jul 2021
    10:32:42 +0800:

    > Since commit 46b5889cc2c5("mtd: implement proper partition handling")
    > applied, mtd partition device won't hold some callback functions, such
    > as _block_isbad, _block_markbad, etc. Besides, function mtd_block_isbad()
    > will get mtd device's master mtd device, then invokes master mtd device's
    > callback function. So, following process may result mtd_block_isbad()
    > always return 0, even though mtd device has bad blocks:
    >
    > 1. Split a mtd device into 3 partitions: PA, PB, PC
    > [ Each mtd partition device won't has callback function _block_isbad(). ]
    > 2. Concatenate PA and PB as a new mtd device PN
    > [ mtd_concat_create() finds out each subdev has no callback function
    > _block_isbad(), so PN won't be assigned callback function
    > concat_block_isbad(). ]
    > Then, mtd_block_isbad() checks "!master->_block_isbad" is true, will
    > always return 0.
    >
    > Reproducer:
    > // reproduce.c
    > static int __init init_diy_module(void)
    > {
    > struct mtd_info *mtd[2];
    > struct mtd_info *mtd_combine = NULL;
    >
    > mtd[0] = get_mtd_device_nm("NAND simulator partition 0");
    > if (!mtd[0]) {
    > pr_err("cannot find mtd1\n");
    > return -EINVAL;
    > }
    > mtd[1] = get_mtd_device_nm("NAND simulator partition 1");
    > if (!mtd[1]) {
    > pr_err("cannot find mtd2\n");
    > return -EINVAL;
    > }
    >
    > put_mtd_device(mtd[0]);
    > put_mtd_device(mtd[1]);
    >
    > mtd_combine = mtd_concat_create(mtd, 2, "Combine mtd");
    > if (mtd_combine == NULL) {
    > pr_err("comnoine fail\n");
    > return -EINVAL;
    > }
    >
    > mtd_device_register(mtd_combine, NULL, 0);
    > pr_info("Combine success\n");
    >
    > return 0;
    > }
    >
    > 1. ID="0x20,0xac,0x00,0x15"
    > 2. modprobe nandsim id_bytes=$ID parts=50,100 badblocks=100
    > 3. insmod reproduce.ko
    > 4. flash_erase /dev/mtd3 0 0
    > libmtd: error!: MEMERASE64 ioctl failed for eraseblock 100 (mtd3)
    > error 5 (Input/output error)
    > // Should be "flash_erase: Skipping bad block at 00c80000"
    >
    > Fixes: 46b5889cc2c54bac ("mtd: implement proper partition handling")
    > Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
    > ---
    > drivers/mtd/mtdconcat.c | 20 ++++++++++++--------
    > 1 file changed, 12 insertions(+), 8 deletions(-)
    >
    > diff --git a/drivers/mtd/mtdconcat.c b/drivers/mtd/mtdconcat.c
    > index 6e4d0017c0bd..ea130eeb54d5 100644
    > --- a/drivers/mtd/mtdconcat.c
    > +++ b/drivers/mtd/mtdconcat.c
    > @@ -641,6 +641,7 @@ struct mtd_info *mtd_concat_create(struct mtd_info *subdev[], /* subdevices to c
    > int i;
    > size_t size;
    > struct mtd_concat *concat;
    > + struct mtd_info *subdev_master = NULL;
    > uint32_t max_erasesize, curr_erasesize;
    > int num_erase_region;
    > int max_writebufsize = 0;
    > @@ -679,17 +680,19 @@ struct mtd_info *mtd_concat_create(struct mtd_info *subdev[], /* subdevices to c
    > concat->mtd.subpage_sft = subdev[0]->subpage_sft;
    > concat->mtd.oobsize = subdev[0]->oobsize;
    > concat->mtd.oobavail = subdev[0]->oobavail;
    > - if (subdev[0]->_writev)
    > +
    > + subdev_master = mtd_get_master(subdev[0]);
    > + if (subdev_master->_writev)
    > concat->mtd._writev = concat_writev;
    > - if (subdev[0]->_read_oob)
    > + if (subdev_master->_read_oob)
    > concat->mtd._read_oob = concat_read_oob;
    > - if (subdev[0]->_write_oob)
    > + if (subdev_master->_write_oob)
    > concat->mtd._write_oob = concat_write_oob;
    > - if (subdev[0]->_block_isbad)
    > + if (subdev_master->_block_isbad)
    > concat->mtd._block_isbad = concat_block_isbad;
    > - if (subdev[0]->_block_markbad)
    > + if (subdev_master->_block_markbad)
    > concat->mtd._block_markbad = concat_block_markbad;
    > - if (subdev[0]->_panic_write)
    > + if (subdev_master->_panic_write)
    > concat->mtd._panic_write = concat_panic_write;
    >
    > concat->mtd.ecc_stats.badblocks = subdev[0]->ecc_stats.badblocks;
    > @@ -721,14 +724,15 @@ struct mtd_info *mtd_concat_create(struct mtd_info *subdev[], /* subdevices to c
    > subdev[i]->flags & MTD_WRITEABLE;
    > }
    >
    > + subdev_master = mtd_get_master(subdev[i]);
    > concat->mtd.size += subdev[i]->size;
    > concat->mtd.ecc_stats.badblocks +=
    > subdev[i]->ecc_stats.badblocks;
    > if (concat->mtd.writesize != subdev[i]->writesize ||
    > concat->mtd.subpage_sft != subdev[i]->subpage_sft ||
    > concat->mtd.oobsize != subdev[i]->oobsize ||
    > - !concat->mtd._read_oob != !subdev[i]->_read_oob ||
    > - !concat->mtd._write_oob != !subdev[i]->_write_oob) {
    > + !concat->mtd._read_oob != !subdev_master->_read_oob ||
    > + !concat->mtd._write_oob != !subdev_master->_write_oob) {

    Do you really need this change?

    > kfree(concat);
    > printk("Incompatible OOB or ECC data on \"%s\"\n",
    > subdev[i]->name);

    Thanks,
    Miquèl

    \
     
     \ /
      Last update: 2021-08-06 21:29    [W:3.073 / U:0.432 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site