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

[Boris Brezillon <boris.brezillon@collabora.com>]

On Mon, 6 Sep 2021 18:13:41 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> 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);

What if this one is not supported either?

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

Well, it's falling back to a different method that might be unsupported
too, so it might still fail on other controllers. Moreover, I really
think this sort of things should be checked at probe time so you don't
spend time checking it every time you do a read.

Something like:

	if (nand_check_change_read_column(gaps=false))
		ecc->read_subpage = nand_read_subpage_nogaps;
	else if (nand_check_change_read_column(gaps=true))
		ecc->read_subpage = nand_read_subpage_gaps;
	else
		return -EOPNOTSUP;

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