[PATCH v3 4/8] nand: spi: add basic operations support

[Boris Brezillon]

On Fri, 17 Mar 2017 19:09:38 +0800
Peter Pan <peterpansjtu at gmail.com> wrote:

> Hi Boris,
> 
> On Fri, Mar 17, 2017 at 7:02 PM, Boris Brezillon
> <boris.brezillon at free-electrons.com> wrote:
> > On Fri, 17 Mar 2017 18:49:08 +0800
> > Peter Pan <peterpansjtu at gmail.com> wrote:
> >  
> >> Hi Arnaud,
> >>
> >> On Fri, Mar 17, 2017 at 6:33 PM, Arnaud Mouiche
> >> <arnaud.mouiche at gmail.com> wrote:  
> >> >
> >> >
> >> > On 16/03/2017 07:47, Peter Pan wrote:  
> >> >>
> >> >> [...]
> >> >>
> >> >> +
> >> >> +/*
> >> >> + * spinand_read_pages - read data from device to buffer
> >> >> + * @mtd: MTD device structure
> >> >> + * @from: offset to read from
> >> >> + * @ops: oob operations description structure
> >> >> + * @max_bitflips: maximum bitflip count
> >> >> + */
> >> >> +static int spinand_read_pages(struct mtd_info *mtd, loff_t from,
> >> >> +                             struct mtd_oob_ops *ops,
> >> >> +                             unsigned int *max_bitflips)
> >> >> +{
> >> >> +       struct spinand_device *chip = mtd_to_spinand(mtd);
> >> >> +       struct nand_device *nand = mtd_to_nand(mtd);
> >> >> +       int size, ret;
> >> >> +       unsigned int corrected = 0;
> >> >> +       bool ecc_off = ops->mode == MTD_OPS_RAW;
> >> >> +       int ooblen = ops->mode == MTD_OPS_AUTO_OOB ?
> >> >> +                    mtd->oobavail : mtd->oobsize;
> >> >> +       bool oob_only = !ops->datbuf;
> >> >> +       struct nand_page_iter iter;
> >> >> +
> >> >> +       ops->retlen = 0;
> >> >> +       ops->oobretlen = 0;
> >> >> +       *max_bitflips = 0;
> >> >> +  
> >> >
> >> >
> >> > I'm stuck in a infinite bad block scan on the very first nand bad block mark
> >> > read attempt.
> >> > Indeed, here I'm in the first page read attempt of scan_block_fast() where
> >> > scan_block_fast() fills "struct mtd_oob_ops ops" the following way
> >> >     struct mtd_oob_ops ops;
> >> >
> >> >     ops.ooblen = nand_per_page_oobsize(this);   <= 64
> >> >     ops.oobbuf = buf;
> >> >     ops.ooboffs = 0;
> >> >     ops.datbuf = NULL;
> >> >     ops.mode = MTD_OPS_PLACE_OOB;
> >> >
> >> > It just forget to also set ops.len which is left to its uninitialized value,
> >> > and is equal to 0xFFFFFFFF in my case.
> >> > Since we only try to read from oob, obviously retlen is never increased, and
> >> > we never except the loop.
> >> > But more obviously, either ops.len should have been set to zero somewhere
> >> > because we only read oob, either nand_for_each_page() should take in count
> >> > this fact.  
> >>
> >> Yes, you are right. I added some code to assign ops->len to 0 when ops->datbuf
> >> is NULL. It is lost somehow...  Thanks for your quick debug.
> >> I'm still thinking whether the caller should guarantee ops->len is 0
> >> when reading
> >> oob only or core.c guarantee it. What's your opinion Boris and Arnaud?  
> >
> > Hm, we could add something to the core to check the mtd op consistency,
> > but, in any case, we should probably fix the nand-bbt code to
> > initialize the ops object to 0:
> >
> >         struct mtd_oob_ops ops = { };  
> 
> I will fix the bbt code in v4 if you like. Add warning message in core.c?

Add an error message, and return an error. Not sure silently fixing the
problem is the right thing to do.