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

[Peter Pan]

On Fri, Mar 17, 2017 at 7:12 PM, Boris Brezillon
<boris.brezillon at free-electrons.com> wrote:
> 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.

I see,