Re: [RFC,v4,4/5] mtd: spinand: Move set/get OOB databytes to each ECC engines

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Xiangsheng,

xiangsheng.hou@mediatek.com wrote on Tue, 30 Nov 2021 16:32:01 +0800:

> Move set/get OOB databytes to each ECC engines when in AUTO mode.
> For read/write in AUTO mode, the OOB bytes include not only free
> date bytes but also ECC data bytes.

This is more or less ok, I would rephrase it to give more details about
the issue:

Move the data bytes handling when in AUTO mode to the ECC drivers, only
them have the knowledge of what's in the OOB buffer and we should be
sure that the ECC bytes do not smash the data provided by the user in
this mode. So the ECC drivers must take care of any data that could be
present at the beginning of the buffer before generating the ECC bytes.

> And for some special ECC engine,
> the data bytes in OOB may be mixed with main data. For example,
> mediatek ECC engine will be one more main data byte swap with BBM.
> So, just put these operation in each ECC engine to distinguish
> the differentiation.

But his is not related to your change, please drop it.

Needs a Fixes here I believe.

> Signed-off-by: Xiangsheng Hou <xiangsheng.hou@mediatek.com>
> ---
>  drivers/mtd/nand/ecc-sw-bch.c           | 71 ++++++++++++++++---
>  drivers/mtd/nand/ecc-sw-hamming.c       | 71 ++++++++++++++++---
>  drivers/mtd/nand/spi/core.c             | 93 +++++++++++++++++--------
>  include/linux/mtd/nand-ecc-sw-bch.h     |  4 ++
>  include/linux/mtd/nand-ecc-sw-hamming.h |  4 ++
>  include/linux/mtd/spinand.h             |  4 ++
>  6 files changed, 198 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/mtd/nand/ecc-sw-bch.c b/drivers/mtd/nand/ecc-sw-bch.c
> index 405552d014a8..bda31ef8f0b8 100644
> --- a/drivers/mtd/nand/ecc-sw-bch.c
> +++ b/drivers/mtd/nand/ecc-sw-bch.c
> @@ -238,7 +238,9 @@ int nand_ecc_sw_bch_init_ctx(struct nand_device *nand)
>  	engine_conf->code_size = code_size;
>  	engine_conf->calc_buf = kzalloc(mtd->oobsize, GFP_KERNEL);
>  	engine_conf->code_buf = kzalloc(mtd->oobsize, GFP_KERNEL);
> -	if (!engine_conf->calc_buf || !engine_conf->code_buf) {
> +	engine_conf->oob_buf = kzalloc(mtd->oobsize, GFP_KERNEL);
> +	if (!engine_conf->calc_buf || !engine_conf->code_buf ||
> +	    !engine_conf->oob_buf) {
>  		ret = -ENOMEM;
>  		goto free_bufs;
>  	}
> @@ -267,6 +269,7 @@ int nand_ecc_sw_bch_init_ctx(struct nand_device *nand)
>  	nand_ecc_cleanup_req_tweaking(&engine_conf->req_ctx);
>  	kfree(engine_conf->calc_buf);
>  	kfree(engine_conf->code_buf);
> +	kfree(engine_conf->oob_buf);
>  free_engine_conf:
>  	kfree(engine_conf);
>  
> @@ -283,6 +286,7 @@ void nand_ecc_sw_bch_cleanup_ctx(struct nand_device *nand)
>  		nand_ecc_cleanup_req_tweaking(&engine_conf->req_ctx);
>  		kfree(engine_conf->calc_buf);
>  		kfree(engine_conf->code_buf);
> +		kfree(engine_conf->oob_buf);
>  		kfree(engine_conf);
>  	}
>  }
> @@ -299,22 +303,42 @@ static int nand_ecc_sw_bch_prepare_io_req(struct nand_device *nand,
>  	int total = nand->ecc.ctx.total;
>  	u8 *ecccalc = engine_conf->calc_buf;
>  	const u8 *data;
> -	int i;
> +	int i, ret = 0;

int i, ret;

>  
>  	/* Nothing to do for a raw operation */
>  	if (req->mode == MTD_OPS_RAW)
>  		return 0;
>  
> -	/* This engine does not provide BBM/free OOB bytes protection */
> -	if (!req->datalen)
> -		return 0;
> -

Why? Please drop this removal here and below, it has nothing to do with
the current fix, right?

>  	nand_ecc_tweak_req(&engine_conf->req_ctx, req);
>  
>  	/* No more preparation for page read */
>  	if (req->type == NAND_PAGE_READ)
>  		return 0;
>  
> +	if (req->ooblen) {
> +		memset(engine_conf->oob_buf, 0xff,
> +		       nanddev_per_page_oobsize(nand));

I think this is only needed in the AUTO case.

> +
> +		if (req->mode == MTD_OPS_AUTO_OOB) {
> +			ret = mtd_ooblayout_set_databytes(mtd, req->oobbuf.out,
> +							  engine_conf->oob_buf,
> +							  req->ooboffs,
> +							  mtd->oobavail);
> +			if (ret)
> +				return ret;
> +		} else {
> +			memcpy(engine_conf->oob_buf + req->ooboffs,
> +			       req->oobbuf.out, req->ooblen);
> +		}
> +
> +		engine_conf->src_oob_buf = (void *)req->oobbuf.out;
> +		req->oobbuf.out = engine_conf->oob_buf;
> +	}
> +
> +	/* This engine does not provide BBM/free OOB bytes protection */
> +	if (!req->datalen)
> +		return 0;
> +

I believe we can do something simpler:

if (req->ooblen && req->mode == AUTO) {
	memcpy(oobbuf, req->oobbuf.out...
	mtd_ooblayout_set_databytes(using oobbuf)
}

But I believe this should be done before tweaking the request so that
you can avoid messing with src_oob_buf, right?

>  	/* Preparation for page write: derive the ECC bytes and place them */
>  	for (i = 0, data = req->databuf.out;
>  	     eccsteps;
> @@ -344,12 +368,36 @@ static int nand_ecc_sw_bch_finish_io_req(struct nand_device *nand,
>  	if (req->mode == MTD_OPS_RAW)
>  		return 0;
>  
> -	/* This engine does not provide BBM/free OOB bytes protection */
> -	if (!req->datalen)
> -		return 0;
> -
>  	/* No more preparation for page write */
>  	if (req->type == NAND_PAGE_WRITE) {
> +		if (req->ooblen)
> +			req->oobbuf.out = engine_conf->src_oob_buf;
> +
> +		nand_ecc_restore_req(&engine_conf->req_ctx, req);
> +		return 0;
> +	}
> +
> +	if (req->ooblen) {
> +		memset(engine_conf->oob_buf, 0xff,
> +		       nanddev_per_page_oobsize(nand));
> +
> +		if (req->mode == MTD_OPS_AUTO_OOB) {
> +			ret = mtd_ooblayout_get_databytes(mtd,
> +							  engine_conf->oob_buf,
> +							  req->oobbuf.in,
> +							  req->ooboffs,
> +							  mtd->oobavail);
> +			if (ret)
> +				return ret;
> +		} else {
> +			memcpy(engine_conf->oob_buf,
> +			       req->oobbuf.in + req->ooboffs, req->ooblen);
> +		}
> +	}
> +
> +	/* This engine does not provide BBM/free OOB bytes protection */
> +	if (!req->datalen) {
> +		req->oobbuf.in = engine_conf->oob_buf;
>  		nand_ecc_restore_req(&engine_conf->req_ctx, req);
>  		return 0;
>  	}
> @@ -379,6 +427,9 @@ static int nand_ecc_sw_bch_finish_io_req(struct nand_device *nand,
>  		}
>  	}
>  
> +	if (req->ooblen)
> +		req->oobbuf.in = engine_conf->oob_buf;
> +
>  	nand_ecc_restore_req(&engine_conf->req_ctx, req);
>  
>  	return max_bitflips;

Same applies to the two other engines.

Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/