Re: [PATCH 2/3] mtd: rawnand: Check the CHANGE_READ_COLUMN from nand_read_subpage() is supported

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

Hi Boris,

boris.brezillon@collabora.com wrote on Mon, 6 Sep 2021 17:08:27 +0200:

> On Mon,  6 Sep 2021 15:29:41 +0200
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> > There are constraint controllers which do not support
> > CHANGE_READ_COLUMNs at any offset. In particular, the offset alignment
> > might be an issue. Ensure that the operation is supported before trying
> > it.
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  drivers/mtd/nand/raw/nand_base.c | 17 +++++++++++++----
> >  1 file changed, 13 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
> > index 7f29f27bb6fd..8175635b9802 100644
> > --- a/drivers/mtd/nand/raw/nand_base.c
> > +++ b/drivers/mtd/nand/raw/nand_base.c
> > @@ -3065,10 +3065,19 @@ static int nand_read_subpage(struct nand_chip *chip, uint32_t data_offs,
> >  		    (busw - 1))
> >  			aligned_len++;
> >  
> > -		ret = nand_change_read_column_op(chip,
> > -						 mtd->writesize + aligned_pos,
> > -						 &chip->oob_poi[aligned_pos],
> > -						 aligned_len, false);
> > +		ret = nand_check_change_read_column_op(chip,
> > +						       mtd->writesize + aligned_pos,
> > +						       &chip->oob_poi[aligned_pos],
> > +						       aligned_len, false, true);
> > +		if (!ret)
> > +			ret = nand_change_read_column_op(chip,
> > +							 mtd->writesize + aligned_pos,
> > +							 &chip->oob_poi[aligned_pos],
> > +							 aligned_len, false);
> > +		else
> > +			ret = nand_change_read_column_op(chip, mtd->writesize,
> > +							 chip->oob_poi,
> > +							 mtd->oobsize, false);
> >  		if (ret)
> >  			return ret;  
> 


The previous behavior was:

/*
 * gaps == true means we need to request the entire OOB area and we
 * will call an OOB layout helper to extract the ECC bytes.
 * gaps == false means there is no data interleaved, we can just read
 * the number of ECC bytes by starting at the right offset and no
 * extracting operation will be needed.
 */
gaps = ...;
if (!gaps)
	nand_change_read_column(at a specific offset and a specific
				length just to match the ECC bytes);
else
	nand_change_read_column(the entire OOB);

While the new behavior is:

if (!gaps) {
	op_supported = nand_check_change_read_column();
	if (!op_supported)
		// same operation as if gaps == true
		nand_change_read_column(the entire OOB);
	else
		nand_change_read_column(at a specific offset);
} else {
	nand_change_read_column(the entire OOB)
}

> So you just fail if the CHANGE column op is not supported? Looks like
> this check should be done before we assign ecc->read_subpage to
> nand_read_subpage in
> nand_set_ecc_on_host_ops()/nand_set_ecc_soft_ops()...

So I actually don't understand the above comment as the code is not
simply failing, it's actually falling back to second method which is
maybe slightly slower but still valid. Do you think it's wrong?

Thanks,
Miquèl

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