LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Miquel Raynal <miquel.raynal@bootlin.com>
To: Zhihao Cheng <chengzhihao1@huawei.com>
Cc: <richard@nod.at>, <vigneshr@ti.com>, <bbrezillon@kernel.org>,
	<linux-mtd@lists.infradead.org>, <linux-kernel@vger.kernel.org>,
	<yukuai3@huawei.com>
Subject: Re: [PATCH 1/2] mtd: mtdconcat: Judge callback function existence getting from master for each partition
Date: Fri, 6 Aug 2021 21:28:57 +0200	[thread overview]
Message-ID: <20210806212857.240e0c1f@xps13> (raw)
In-Reply-To: <20210731023243.3977104-2-chengzhihao1@huawei.com>

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

  reply	other threads:[~2021-08-06 19:29 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-31  2:32 [PATCH 0/2] mtd: mtdconcat: Fix callback functions check Zhihao Cheng
2021-07-31  2:32 ` [PATCH 1/2] mtd: mtdconcat: Judge callback function existence getting from master for each partition Zhihao Cheng
2021-08-06 19:28   ` Miquel Raynal [this message]
2021-08-07  2:15     ` Zhihao Cheng
2021-08-07 10:32       ` Miquel Raynal
2021-08-10 11:35         ` Zhihao Cheng
2021-08-16 13:43           ` Miquel Raynal
2021-08-16 14:27   ` Miquel Raynal
2021-07-31  2:32 ` [PATCH 2/2] mtd: mtdconcat: Remove concat_{read|write}_oob Zhihao Cheng
2021-08-06 19:26   ` Miquel Raynal
2021-08-07  2:59     ` Zhihao Cheng
2021-08-07 10:28       ` Miquel Raynal
2021-08-16 14:27   ` Miquel Raynal

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210806212857.240e0c1f@xps13 \
    --to=miquel.raynal@bootlin.com \
    --cc=bbrezillon@kernel.org \
    --cc=chengzhihao1@huawei.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mtd@lists.infradead.org \
    --cc=richard@nod.at \
    --cc=vigneshr@ti.com \
    --cc=yukuai3@huawei.com \
    --subject='Re: [PATCH 1/2] mtd: mtdconcat: Judge callback function existence getting from master for each partition' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).