[PATCH 0/2] mtd: mtdconcat: Fix callback functions check

[PATCH 0/2] mtd: mtdconcat: Fix callback functions check

[Zhihao Cheng]

Patch 1: Check subdev's master mtd device's callback function while
making concatenated device.
Patch 2: Remove bad callback function assignments.

Zhihao Cheng (2):
  mtd: mtdconcat: Judge callback function existence getting from master
    for each partition
  mtd: mtdconcat: Remove concat_{read|write}_oob

 drivers/mtd/mtdconcat.c | 123 +++-------------------------------------
 1 file changed, 8 insertions(+), 115 deletions(-)

-- 
2.31.1

[PATCH 1/2] mtd: mtdconcat: Judge callback function existence getting from master for each partition

[Zhihao Cheng]

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) {
 			kfree(concat);
 			printk("Incompatible OOB or ECC data on \"%s\"\n",
 			       subdev[i]->name);
-- 
2.31.1

[PATCH 2/2] mtd: mtdconcat: Remove concat_{read|write}_oob

[Zhihao Cheng]

Since 2431c4f5b46c3("mtd: Implement mtd_{read,write}() as wrappers
around mtd_{read,write}_oob()") don't allow _write|_read and
_write_oob|_read_oob existing at the same time. We should stop these
two callback functions assigning, otherwise following warning occurs
while making concatenated device:

  WARNING: CPU: 2 PID: 6728 at drivers/mtd/mtdcore.c:595
  add_mtd_device+0x7f/0x7b0

Fixes: 2431c4f5b46c3("mtd: Implement mtd_{read,write}() around ...")
Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
---
 drivers/mtd/mtdconcat.c | 113 +---------------------------------------
 1 file changed, 1 insertion(+), 112 deletions(-)

diff --git a/drivers/mtd/mtdconcat.c b/drivers/mtd/mtdconcat.c
index ea130eeb54d5..98d1c79cf51d 100644
--- a/drivers/mtd/mtdconcat.c
+++ b/drivers/mtd/mtdconcat.c
@@ -256,110 +256,6 @@ concat_writev(struct mtd_info *mtd, const struct kvec *vecs,
 	return err;
 }
 
-static int
-concat_read_oob(struct mtd_info *mtd, loff_t from, struct mtd_oob_ops *ops)
-{
-	struct mtd_concat *concat = CONCAT(mtd);
-	struct mtd_oob_ops devops = *ops;
-	int i, err, ret = 0;
-
-	ops->retlen = ops->oobretlen = 0;
-
-	for (i = 0; i < concat->num_subdev; i++) {
-		struct mtd_info *subdev = concat->subdev[i];
-
-		if (from >= subdev->size) {
-			from -= subdev->size;
-			continue;
-		}
-
-		/* partial read ? */
-		if (from + devops.len > subdev->size)
-			devops.len = subdev->size - from;
-
-		err = mtd_read_oob(subdev, from, &devops);
-		ops->retlen += devops.retlen;
-		ops->oobretlen += devops.oobretlen;
-
-		/* Save information about bitflips! */
-		if (unlikely(err)) {
-			if (mtd_is_eccerr(err)) {
-				mtd->ecc_stats.failed++;
-				ret = err;
-			} else if (mtd_is_bitflip(err)) {
-				mtd->ecc_stats.corrected++;
-				/* Do not overwrite -EBADMSG !! */
-				if (!ret)
-					ret = err;
-			} else
-				return err;
-		}
-
-		if (devops.datbuf) {
-			devops.len = ops->len - ops->retlen;
-			if (!devops.len)
-				return ret;
-			devops.datbuf += devops.retlen;
-		}
-		if (devops.oobbuf) {
-			devops.ooblen = ops->ooblen - ops->oobretlen;
-			if (!devops.ooblen)
-				return ret;
-			devops.oobbuf += ops->oobretlen;
-		}
-
-		from = 0;
-	}
-	return -EINVAL;
-}
-
-static int
-concat_write_oob(struct mtd_info *mtd, loff_t to, struct mtd_oob_ops *ops)
-{
-	struct mtd_concat *concat = CONCAT(mtd);
-	struct mtd_oob_ops devops = *ops;
-	int i, err;
-
-	if (!(mtd->flags & MTD_WRITEABLE))
-		return -EROFS;
-
-	ops->retlen = ops->oobretlen = 0;
-
-	for (i = 0; i < concat->num_subdev; i++) {
-		struct mtd_info *subdev = concat->subdev[i];
-
-		if (to >= subdev->size) {
-			to -= subdev->size;
-			continue;
-		}
-
-		/* partial write ? */
-		if (to + devops.len > subdev->size)
-			devops.len = subdev->size - to;
-
-		err = mtd_write_oob(subdev, to, &devops);
-		ops->retlen += devops.retlen;
-		ops->oobretlen += devops.oobretlen;
-		if (err)
-			return err;
-
-		if (devops.datbuf) {
-			devops.len = ops->len - ops->retlen;
-			if (!devops.len)
-				return 0;
-			devops.datbuf += devops.retlen;
-		}
-		if (devops.oobbuf) {
-			devops.ooblen = ops->ooblen - ops->oobretlen;
-			if (!devops.ooblen)
-				return 0;
-			devops.oobbuf += devops.oobretlen;
-		}
-		to = 0;
-	}
-	return -EINVAL;
-}
-
 static int concat_erase(struct mtd_info *mtd, struct erase_info *instr)
 {
 	struct mtd_concat *concat = CONCAT(mtd);
@@ -684,10 +580,6 @@ struct mtd_info *mtd_concat_create(struct mtd_info *subdev[],	/* subdevices to c
 	subdev_master = mtd_get_master(subdev[0]);
 	if (subdev_master->_writev)
 		concat->mtd._writev = concat_writev;
-	if (subdev_master->_read_oob)
-		concat->mtd._read_oob = concat_read_oob;
-	if (subdev_master->_write_oob)
-		concat->mtd._write_oob = concat_write_oob;
 	if (subdev_master->_block_isbad)
 		concat->mtd._block_isbad = concat_block_isbad;
 	if (subdev_master->_block_markbad)
@@ -724,15 +616,12 @@ 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_master->_read_oob ||
-		    !concat->mtd._write_oob != !subdev_master->_write_oob) {
+		    concat->mtd.oobsize    !=  subdev[i]->oobsize) {
 			kfree(concat);
 			printk("Incompatible OOB or ECC data on \"%s\"\n",
 			       subdev[i]->name);
-- 
2.31.1

Re: [PATCH 2/2] mtd: mtdconcat: Remove concat_{read|write}_oob

[Miquel Raynal]

Hi Zhihao,

Richard, a question for you below :)

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

> Since 2431c4f5b46c3("mtd: Implement mtd_{read,write}() as wrappers
> around mtd_{read,write}_oob()") don't allow _write|_read and
> _write_oob|_read_oob existing at the same time. We should stop these
> two callback functions assigning, otherwise following warning occurs
> while making concatenated device:
> 
>   WARNING: CPU: 2 PID: 6728 at drivers/mtd/mtdcore.c:595
>   add_mtd_device+0x7f/0x7b0
> 
> Fixes: 2431c4f5b46c3("mtd: Implement mtd_{read,write}() around ...")
> Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
> ---
>  drivers/mtd/mtdconcat.c | 113 +---------------------------------------
>  1 file changed, 1 insertion(+), 112 deletions(-)
> 
> diff --git a/drivers/mtd/mtdconcat.c b/drivers/mtd/mtdconcat.c
> index ea130eeb54d5..98d1c79cf51d 100644
> --- a/drivers/mtd/mtdconcat.c
> +++ b/drivers/mtd/mtdconcat.c
> @@ -256,110 +256,6 @@ concat_writev(struct mtd_info *mtd, const struct kvec *vecs,
>  	return err;
>  }
>  
> -static int
> -concat_read_oob(struct mtd_info *mtd, loff_t from, struct mtd_oob_ops *ops)
> -{
> -	struct mtd_concat *concat = CONCAT(mtd);
> -	struct mtd_oob_ops devops = *ops;
> -	int i, err, ret = 0;
> -
> -	ops->retlen = ops->oobretlen = 0;
> -
> -	for (i = 0; i < concat->num_subdev; i++) {
> -		struct mtd_info *subdev = concat->subdev[i];
> -
> -		if (from >= subdev->size) {
> -			from -= subdev->size;
> -			continue;
> -		}
> -
> -		/* partial read ? */
> -		if (from + devops.len > subdev->size)
> -			devops.len = subdev->size - from;
> -
> -		err = mtd_read_oob(subdev, from, &devops);
> -		ops->retlen += devops.retlen;
> -		ops->oobretlen += devops.oobretlen;
> -
> -		/* Save information about bitflips! */
> -		if (unlikely(err)) {
> -			if (mtd_is_eccerr(err)) {
> -				mtd->ecc_stats.failed++;
> -				ret = err;
> -			} else if (mtd_is_bitflip(err)) {
> -				mtd->ecc_stats.corrected++;
> -				/* Do not overwrite -EBADMSG !! */
> -				if (!ret)
> -					ret = err;
> -			} else
> -				return err;
> -		}
> -
> -		if (devops.datbuf) {
> -			devops.len = ops->len - ops->retlen;
> -			if (!devops.len)
> -				return ret;
> -			devops.datbuf += devops.retlen;
> -		}
> -		if (devops.oobbuf) {
> -			devops.ooblen = ops->ooblen - ops->oobretlen;
> -			if (!devops.ooblen)
> -				return ret;
> -			devops.oobbuf += ops->oobretlen;
> -		}
> -
> -		from = 0;
> -	}
> -	return -EINVAL;
> -}
> -
> -static int
> -concat_write_oob(struct mtd_info *mtd, loff_t to, struct mtd_oob_ops *ops)
> -{
> -	struct mtd_concat *concat = CONCAT(mtd);
> -	struct mtd_oob_ops devops = *ops;
> -	int i, err;
> -
> -	if (!(mtd->flags & MTD_WRITEABLE))
> -		return -EROFS;
> -
> -	ops->retlen = ops->oobretlen = 0;
> -
> -	for (i = 0; i < concat->num_subdev; i++) {
> -		struct mtd_info *subdev = concat->subdev[i];
> -
> -		if (to >= subdev->size) {
> -			to -= subdev->size;
> -			continue;
> -		}
> -
> -		/* partial write ? */
> -		if (to + devops.len > subdev->size)
> -			devops.len = subdev->size - to;
> -
> -		err = mtd_write_oob(subdev, to, &devops);
> -		ops->retlen += devops.retlen;
> -		ops->oobretlen += devops.oobretlen;
> -		if (err)
> -			return err;
> -
> -		if (devops.datbuf) {
> -			devops.len = ops->len - ops->retlen;
> -			if (!devops.len)
> -				return 0;
> -			devops.datbuf += devops.retlen;
> -		}
> -		if (devops.oobbuf) {
> -			devops.ooblen = ops->ooblen - ops->oobretlen;
> -			if (!devops.ooblen)
> -				return 0;
> -			devops.oobbuf += devops.oobretlen;
> -		}
> -		to = 0;
> -	}
> -	return -EINVAL;
> -}
> -
>  static int concat_erase(struct mtd_info *mtd, struct erase_info *instr)
>  {
>  	struct mtd_concat *concat = CONCAT(mtd);
> @@ -684,10 +580,6 @@ struct mtd_info *mtd_concat_create(struct mtd_info *subdev[],	/* subdevices to c
>  	subdev_master = mtd_get_master(subdev[0]);
>  	if (subdev_master->_writev)
>  		concat->mtd._writev = concat_writev;
> -	if (subdev_master->_read_oob)
> -		concat->mtd._read_oob = concat_read_oob;
> -	if (subdev_master->_write_oob)
> -		concat->mtd._write_oob = concat_write_oob;

Actually I am not sure _read|write_oob() is the right callback to
remove.

Richard, what is your input on this? Shall we remove _read|write()
instead? I don't remember the exact rationale behind these two helpers.

>  	if (subdev_master->_block_isbad)
>  		concat->mtd._block_isbad = concat_block_isbad;
>  	if (subdev_master->_block_markbad)
> @@ -724,15 +616,12 @@ 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_master->_read_oob ||
> -		    !concat->mtd._write_oob != !subdev_master->_write_oob) {
> +		    concat->mtd.oobsize    !=  subdev[i]->oobsize) {
>  			kfree(concat);
>  			printk("Incompatible OOB or ECC data on \"%s\"\n",
>  			       subdev[i]->name);

Thanks,
Miquèl

Re: [PATCH 1/2] mtd: mtdconcat: Judge callback function existence getting from master for each partition

[Miquel Raynal]

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

Re: [PATCH 1/2] mtd: mtdconcat: Judge callback function existence getting from master for each partition

[Zhihao Cheng]

在 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.

Re: [PATCH 2/2] mtd: mtdconcat: Remove concat_{read|write}_oob

[Zhihao Cheng]

在 2021/8/7 3:26, Miquel Raynal 写道:
Hi Miquel,
>>   static int concat_erase(struct mtd_info *mtd, struct erase_info *instr)
>>   {
>>   	struct mtd_concat *concat = CONCAT(mtd);
>> @@ -684,10 +580,6 @@ struct mtd_info *mtd_concat_create(struct mtd_info *subdev[],	/* subdevices to c
>>   	subdev_master = mtd_get_master(subdev[0]);
>>   	if (subdev_master->_writev)
>>   		concat->mtd._writev = concat_writev;
>> -	if (subdev_master->_read_oob)
>> -		concat->mtd._read_oob = concat_read_oob;
>> -	if (subdev_master->_write_oob)
>> -		concat->mtd._write_oob = concat_write_oob;
> Actually I am not sure _read|write_oob() is the right callback to
> remove.
>
> Richard, what is your input on this? Shall we remove _read|write()
> instead? I don't remember the exact rationale behind these two helpers.

Oh, I guess I made a mistake. It looks like that reserving _{read|write}_oob is a better method in my limited knowledge to nand driver. For example, nand_do_read_oob() behaves different from nand_do_read_ops(), and calling which function is decided by mtd_oob_ops.databuf.
Callback _read_oobs() can support both functions, but callback _read() don't support nand_do_read_oob(). So mtd_read_oobs() covers mtd_read()?
Is my understand right?

Re: [PATCH 2/2] mtd: mtdconcat: Remove concat_{read|write}_oob

[Miquel Raynal]

Hi Zhihao,

Zhihao Cheng <chengzhihao1@huawei.com> wrote on Sat, 7 Aug 2021
10:59:32 +0800:

> 在 2021/8/7 3:26, Miquel Raynal 写道:
> Hi Miquel,
> >>   static int concat_erase(struct mtd_info *mtd, struct erase_info *instr)
> >>   {
> >>   	struct mtd_concat *concat = CONCAT(mtd);
> >> @@ -684,10 +580,6 @@ struct mtd_info *mtd_concat_create(struct mtd_info *subdev[],	/* subdevices to c
> >>   	subdev_master = mtd_get_master(subdev[0]);
> >>   	if (subdev_master->_writev)
> >>   		concat->mtd._writev = concat_writev;
> >> -	if (subdev_master->_read_oob)
> >> -		concat->mtd._read_oob = concat_read_oob;
> >> -	if (subdev_master->_write_oob)
> >> -		concat->mtd._write_oob = concat_write_oob;  
> > Actually I am not sure _read|write_oob() is the right callback to
> > remove.
> >
> > Richard, what is your input on this? Shall we remove _read|write()
> > instead? I don't remember the exact rationale behind these two helpers.  
> 
> Oh, I guess I made a mistake. It looks like that reserving _{read|write}_oob is a better method in my limited knowledge to nand driver. For example, nand_do_read_oob() behaves different from nand_do_read_ops(), and calling which function is decided by mtd_oob_ops.databuf.
> Callback _read_oobs() can support both functions, but callback _read() don't support nand_do_read_oob(). So mtd_read_oobs() covers mtd_read()?
> Is my understand right?
> 

Yes please let's drop _read|write() instead.

Thanks,
Miquèl

Re: [PATCH 1/2] mtd: mtdconcat: Judge callback function existence getting from master for each partition

[Miquel Raynal]

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

Re: [PATCH 1/2] mtd: mtdconcat: Judge callback function existence getting from master for each partition

[Zhihao Cheng]

在 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?

Re: [PATCH 1/2] mtd: mtdconcat: Judge callback function existence getting from master for each partition

[Miquel Raynal]

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

Re: [PATCH 2/2] mtd: mtdconcat: Remove concat_{read|write}_oob

[Miquel Raynal]

On Sat, 2021-07-31 at 02:32:43 UTC, Zhihao Cheng wrote:
> Since 2431c4f5b46c3("mtd: Implement mtd_{read,write}() as wrappers
> around mtd_{read,write}_oob()") don't allow _write|_read and
> _write_oob|_read_oob existing at the same time. We should stop these
> two callback functions assigning, otherwise following warning occurs
> while making concatenated device:
> 
>   WARNING: CPU: 2 PID: 6728 at drivers/mtd/mtdcore.c:595
>   add_mtd_device+0x7f/0x7b0
> 
> Fixes: 2431c4f5b46c3("mtd: Implement mtd_{read,write}() around ...")
> 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

Re: [PATCH 1/2] mtd: mtdconcat: Judge callback function existence getting from master for each partition

[Miquel Raynal]

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