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 19:32:55 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> > > > > 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?  
> 
> If this one is not supported either I guess the entire software ECC
> support cannot be used. But what I am trying to achieve here is more
> complicated: the controller does not support one thing: reading the ECC
> bytes until the OOB end minus 2. Only a call to
> nand_change_read_column with these particular parameters would fail
> because it depends on the actual offset and length that are requested.

I suspect those parameters can be extracted from the NAND page size/ECC
config, which are known before the ECC initialization IIRC.

> 
> > > 	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.  
> 
> Well, yes it may fail on other controllers but as I said, if
> controllers do not support any type of nand_change_read_column() then
> they cannot use software ECC at all (and are close to be almost
> useless).

Which would be good to detect at probe time, with a probe failure when
this happens.

> 
> > 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.  
> 
> Agreed. I can certainly make something like this in theory, but what
> would be the condition? I am describing it in patch 3/3 : there is
> really a tiny set of conditions where this will fail so in the end I
> would end-up writing a condition that precisely matches the Arasan
> limitation.

I see. Maybe it's simpler to add NAND_NO_SUBPAGE_READ flag until you
find a better way to describe such limitations.

> 
> Not mentioning that it only fails with NV-DDR enabled (also something
> that I missed to capture in patch 3/3). The data interface being
> initialized after the read_subpage() hook it means I couldn't use this
> information.

Duh, this is nasty, indeed.

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