Re: [RFC,v3 5/5] mtd: spinand: skip set/get oob data bytes when interleaved case

From: Miquel Raynal <miquel.raynal@bootlin.com>
To: Xiangsheng Hou <xiangsheng.hou@mediatek.com>
Cc: <broonie@kernel.org>, <benliang.zhao@mediatek.com>,
	<dandan.he@mediatek.com>, <guochun.mao@mediatek.com>,
	<bin.zhang@mediatek.com>, <sanny.chen@mediatek.com>,
	<mao.zhong@mediatek.com>, <yingjoe.chen@mediatek.com>,
	<donghunt@amazon.com>, <rdlee@amazon.com>,
	<linux-mtd@lists.infradead.org>,
	<linux-mediatek@lists.infradead.org>,
	<srv_heupstream@mediatek.com>
Subject: Re: [RFC,v3 5/5] mtd: spinand: skip set/get oob data bytes when interleaved case
Date: Tue, 9 Nov 2021 13:05:18 +0100	[thread overview]
Message-ID: <20211109130518.70db0d5d@xps13> (raw)
In-Reply-To: <20211022024021.14665-6-xiangsheng.hou@mediatek.com>

Hi Xiangsheng,

Has we discussed a while ago:

	...it is possible that I did not test with MTD_OPS_AUTO_OOB
	recently and indeed the ECC bytes will missing in this case. In
	the write path, maybe the ->prepare_io hooks should spread the
	user data following req->mode in req.oobbuf before computing
	the codes. Similar logic should be applied to the read path.

Can you please add a patch for this situation in your next iteration?
It does not look like this situation has been handled.

xiangsheng.hou@mediatek.com wrote on Fri, 22 Oct 2021 10:40:21 +0800:

> For syndrome layout, ECC/free byte in oob layout and main
> data are interleaved. For this case, it is better to set/get
> oob data bytes in ECC engine.

I don't understand the last sentence

> 
> For MTK ECC engine, although it can auto place data as sector +
> oob free + oob ecc for one page in pipelined. However, the bad
> mark will be not fit with nand spec. Therefore, there have bad
> mark swap operation in ecc engine.
> 
> But, the swap opeation(between bbm 0xff with 1byte main data) will
> lead to more one byte than oobavailable.

Sorry but again, I don't understand what you mean.

> Set oob databytes after
> bad mark swap will lead to lost one oob free byte.

We don't care about free OOB bytes really.

> 
> Therefore, just try to modify the spi nand framework for review.
> And this may be common for the interleaved case.

I don't get how falling back to a memcpy will solve your problem? Can
you please provide an anscii figure or something more visual to let us
understand?

> 
> Signed-off-by: Xiangsheng Hou <xiangsheng.hou@mediatek.com>
> ---
>  drivers/mtd/nand/spi/core.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
> index 2c8685f1f2fa..32a4707982c5 100644
> --- a/drivers/mtd/nand/spi/core.c
> +++ b/drivers/mtd/nand/spi/core.c
> @@ -401,7 +401,8 @@ static int spinand_read_from_cache_op(struct spinand_device *spinand,
>  		       req->datalen);
>  
>  	if (req->ooblen) {
> -		if (req->mode == MTD_OPS_AUTO_OOB)
> +		if (req->mode == MTD_OPS_AUTO_OOB &&
> +			nand->ecc.user_conf.placement != NAND_ECC_PLACEMENT_INTERLEAVED)
>  			mtd_ooblayout_get_databytes(mtd, req->oobbuf.in,
>  						    spinand->oobbuf,
>  						    req->ooboffs,
> @@ -442,7 +443,8 @@ static int spinand_write_to_cache_op(struct spinand_device *spinand,
>  		       req->datalen);
>  
>  	if (req->ooblen) {
> -		if (req->mode == MTD_OPS_AUTO_OOB)
> +		if (req->mode == MTD_OPS_AUTO_OOB &&
> +			nand->ecc.user_conf.placement != NAND_ECC_PLACEMENT_INTERLEAVED)
>  			mtd_ooblayout_set_databytes(mtd, req->oobbuf.out,
>  						    spinand->oobbuf,
>  						    req->ooboffs,

Thanks,
Miquèl

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek