[PATCH 2/3] mtd: rawnand: Check the CHANGE_READ_COLUMN from nand_read_subpage() is supported
Boris Brezillon
boris.brezillon at collabora.com
Mon Sep 6 09:33:44 PDT 2021
On Mon, 6 Sep 2021 18:13:41 +0200
Miquel Raynal <miquel.raynal at bootlin.com> wrote:
> Hi Boris,
>
> boris.brezillon at 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 at 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 at 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;
More information about the linux-mtd
mailing list