| Message ID | 20210731023243.3977104-2-chengzhihao1@huawei.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | mtd: mtdconcat: Fix callback functions check | expand |
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
在 2021/8/7 3:28, Miquel Raynal 写道: Hi Miquel, > Hi Zhihao, > > Zhihao Cheng <chengzhihao1@huawei.com> wrote on Sat, 31 Jul 2021 > 10:32:42 +0800: > @@ -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? I think both "!concat->mtd._read_oob != !subdev[i]->_read_oob" and "!concat->mtd._write_oob != !subdev[i]->_write_oob" need to be modified otherwise concatenating goes failure. I thought there exists two problems: 1. Wrong callback fetching in mtd partition device 2. Warning for existence of _read and _read_oob at the same time so I solved them in two steps to make history commit logs a bit clear. Though these two patches can be combined to one.
Hi Zhihao, Zhihao Cheng <chengzhihao1@huawei.com> wrote on Sat, 7 Aug 2021 10:15:46 +0800: > 在 2021/8/7 3:28, Miquel Raynal 写道: > Hi Miquel, > > Hi Zhihao, > > > > Zhihao Cheng <chengzhihao1@huawei.com> wrote on Sat, 31 Jul 2021 > > 10:32:42 +0800: > > @@ -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? > > I think both "!concat->mtd._read_oob != !subdev[i]->_read_oob" and "!concat->mtd._write_oob != !subdev[i]->_write_oob" need to be modified otherwise concatenating goes failure. > > I thought there exists two problems: > > 1. Wrong callback fetching in mtd partition device > > 2. Warning for existence of _read and _read_oob at the same time > > so I solved them in two steps to make history commit logs a bit clear. > > Though these two patches can be combined to one. No please keep the split. What I mean here is that I don't think your fix is valid. Maybe we should propagate these callbacks as well instead of trying to hack into this condition. I don't see why you should check against subdev[i] for half of the callbacks and check for subdev_master for the last two. Thanks, Miquèl
在 2021/8/7 18:32, Miquel Raynal 写道: Hi Miquel, > Hi Zhihao, > > Zhihao Cheng <chengzhihao1@huawei.com> wrote on Sat, 7 Aug 2021 > 10:15:46 +0800: > >> 在 2021/8/7 3:28, Miquel Raynal 写道: >> Hi Miquel, >>> Hi Zhihao, >>> >>> Zhihao Cheng <chengzhihao1@huawei.com> wrote on Sat, 31 Jul 2021 >>> 10:32:42 +0800: >>> @@ -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? >> I think both "!concat->mtd._read_oob != !subdev[i]->_read_oob" and "!concat->mtd._write_oob != !subdev[i]->_write_oob" need to be modified otherwise concatenating goes failure. >> >> I thought there exists two problems: >> >> 1. Wrong callback fetching in mtd partition device >> >> 2. Warning for existence of _read and _read_oob at the same time >> >> so I solved them in two steps to make history commit logs a bit clear. >> >> Though these two patches can be combined to one. > No please keep the split. > > What I mean here is that I don't think your fix is valid. Maybe we > should propagate these callbacks as well instead of trying to hack into > this condition. I don't see why you should check against subdev[i] for > half of the callbacks and check for subdev_master for the last two. I have the following understanding of mtd: 1. The existence of mtd partition device's callbacks(what can mtd do, _read, _write, _read_oob, etc.) is decided by mtd master device. All mtd common functions (mtd_read, mtd_read_oob) pass master mtd device rather than partition mtd device as parameter, because nand_chip(initialized as master mtd device) process requests. So there are two steps in mtd common function: fetch master mtd device; invoke master mtd devices's callback and pass in master mtd device. Propogating callbacks to partition mtd device may bring some imperfections: A. No adaptions to mtd common functions: partition mtd device's callbacks will never be invoked, they are only used to judge whether assigin concatenated device's callback while concatenating. Looks weird. @@ -86,6 +86,61 @@ static struct mtd_info *allocate_partition(struct mtd_info *parent, child->part.offset = part->offset; INIT_LIST_HEAD(&child->partitions); + if (parent->_read) + child->_read = parent->_read; + if (parent->_write) + child->_write = parent->_write; [...] + if (parent->_read_oob) + child->_read_oob = parent->_read_oob; + if (parent->_write_oob) B. Re-import removed partition mtd device's callbacks and adapt mtd common functions: Current implemention is simplier than the version before 46b5889cc2c54("mtd: implement proper partition handling"). Adapting mtd common functions is a risky thing, which could effect other modules, should we do that? +static int part_read(struct mtd_info *mtd, loff_t from, size_t len, + size_t *retlen, u_char *buf) +{ + struct mtd_part *part = mtd_to_part(mtd); + struct mtd_ecc_stats stats; + int res; + + stats = part->parent->ecc_stats; + res = part->parent->_read(part->parent, from + part->offset, len, + retlen, buf); + if (unlikely(mtd_is_eccerr(res))) + mtd->ecc_stats.failed += + part->parent->ecc_stats.failed - stats.failed; + else + mtd->ecc_stats.corrected += + part->parent->ecc_stats.corrected - stats.corrected; + return res; +} static int mtd_read_oob_std(struct mtd_info *mtd, loff_t from, struct mtd_oob_ops *ops) { - struct mtd_info *master = mtd_get_master(mtd); int ret; - from = mtd_get_master_ofs(mtd, from); - if (master->_read_oob) - ret = master->_read_oob(master, from, ops); + if (mtd->_read_oob) + ret = mtd->_read_oob(mtd, from, ops); else - ret = master->_read(master, from, ops->len, &ops->retlen, + ret = mtd->_read(mtd, from, ops->len, &ops->retlen, ops->datbuf); 2. Checking against subdev[i] for data members and check against subdev_master for the callbacks looks weird. I modified it based on the assumption that partition mtd device' callbacks inherit from master mtd device but data members(name, size) may not. Maybe I should add some comment to explain why checking against subdev[i] for data members and checking against subdev_master for the callbacks. So, which method is better? Any other method?
Hi Zhihao, Zhihao Cheng <chengzhihao1@huawei.com> wrote on Tue, 10 Aug 2021 19:35:02 +0800: > 在 2021/8/7 18:32, Miquel Raynal 写道: > Hi Miquel, > > Hi Zhihao, > > > > Zhihao Cheng <chengzhihao1@huawei.com> wrote on Sat, 7 Aug 2021 > > 10:15:46 +0800: > > > >> 在 2021/8/7 3:28, Miquel Raynal 写道: > >> Hi Miquel, > >>> Hi Zhihao, > >>> > >>> Zhihao Cheng <chengzhihao1@huawei.com> wrote on Sat, 31 Jul 2021 > >>> 10:32:42 +0800: > >>> @@ -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? > >> I think both "!concat->mtd._read_oob != !subdev[i]->_read_oob" and "!concat->mtd._write_oob != !subdev[i]->_write_oob" need to be modified otherwise concatenating goes failure. > >> > >> I thought there exists two problems: > >> > >> 1. Wrong callback fetching in mtd partition device > >> > >> 2. Warning for existence of _read and _read_oob at the same time > >> > >> so I solved them in two steps to make history commit logs a bit clear. > >> > >> Though these two patches can be combined to one. > > No please keep the split. > > > > What I mean here is that I don't think your fix is valid. Maybe we > > should propagate these callbacks as well instead of trying to hack into > > this condition. I don't see why you should check against subdev[i] for > > half of the callbacks and check for subdev_master for the last two. > > I have the following understanding of mtd: > > 1. The existence of mtd partition device's callbacks(what can mtd do, _read, _write, _read_oob, etc.) is decided by mtd master device. All mtd common functions (mtd_read, mtd_read_oob) pass master mtd device rather than partition mtd device as parameter, because nand_chip(initialized as master mtd device) process requests. So there are two steps in mtd common function: fetch master mtd device; invoke master mtd devices's callback and pass in master mtd device. > > Propogating callbacks to partition mtd device may bring some imperfections: > > A. No adaptions to mtd common functions: partition mtd device's callbacks will never be invoked, they are only used to judge whether assigin concatenated device's callback while concatenating. Looks weird. > > @@ -86,6 +86,61 @@ static struct mtd_info *allocate_partition(struct mtd_info *parent, > child->part.offset = part->offset; > INIT_LIST_HEAD(&child->partitions); > > + if (parent->_read) > + child->_read = parent->_read; > + if (parent->_write) > + child->_write = parent->_write; > [...] > + if (parent->_read_oob) > + child->_read_oob = parent->_read_oob; > + if (parent->_write_oob) > > > B. Re-import removed partition mtd device's callbacks and adapt mtd common functions: Current implemention is simplier than the version before 46b5889cc2c54("mtd: implement proper partition handling"). Adapting mtd common functions is a risky thing, which could effect other modules, should we do that? > > +static int part_read(struct mtd_info *mtd, loff_t from, size_t len, > + size_t *retlen, u_char *buf) > +{ > + struct mtd_part *part = mtd_to_part(mtd); > + struct mtd_ecc_stats stats; > + int res; > + > + stats = part->parent->ecc_stats; > + res = part->parent->_read(part->parent, from + part->offset, len, > + retlen, buf); > + if (unlikely(mtd_is_eccerr(res))) > + mtd->ecc_stats.failed += > + part->parent->ecc_stats.failed - stats.failed; > + else > + mtd->ecc_stats.corrected += > + part->parent->ecc_stats.corrected - stats.corrected; > + return res; > +} > > static int mtd_read_oob_std(struct mtd_info *mtd, loff_t from, > struct mtd_oob_ops *ops) > { > - struct mtd_info *master = mtd_get_master(mtd); > int ret; > > - from = mtd_get_master_ofs(mtd, from); > - if (master->_read_oob) > - ret = master->_read_oob(master, from, ops); > + if (mtd->_read_oob) > + ret = mtd->_read_oob(mtd, from, ops); > else > - ret = master->_read(master, from, ops->len, &ops->retlen, > + ret = mtd->_read(mtd, from, ops->len, &ops->retlen, > ops->datbuf); > > > 2. Checking against subdev[i] for data members and check against subdev_master for the callbacks looks weird. I modified it based on the assumption that partition mtd device' callbacks inherit from master mtd device but data members(name, size) may not. Maybe I should add some comment to explain why checking against subdev[i] for data members and checking against subdev_master for the callbacks. > > > So, which method is better? Any other method? > I see your point, actually my concern was about checking half of the *callbacks* from a particular device, and the other half from another device (eg. the master) but as you stated it here, we check the callbacks of the master but the "data members" as you call them from the subdevice, which I think is fine. So I guess I'll take these changes as they currently are. Thanks, Miquèl
On Sat, 2021-07-31 at 02:32:42 UTC, Zhihao Cheng wrote: > 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> Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/next, thanks. Miquel
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) { kfree(concat); printk("Incompatible OOB or ECC data on \"%s\"\n", subdev[i]->name);
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(-)