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

[Peter Pan]

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?

Peter Pan