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

[Peter Pan <peterpansjtu@gmail.com>]

Hi Arnaud,

On Fri, Mar 17, 2017 at 6:33 PM, Arnaud Mouiche
<arnaud.mouiche@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?

Peter Pan

>
> Arnaud
>
>> +       nand_for_each_page(nand, from, ops->len, ops->ooboffs,
>> ops->ooblen,
>> +                          ooblen, &iter) {
>> +               ret = spinand_do_read_page(mtd, iter.page, ecc_off,
>> +                                          &corrected, oob_only);
>> +               if (ret)
>> +                       break;
>> +               *max_bitflips = max(*max_bitflips, corrected);
>> +               if (ops->datbuf) {
>> +                       size = min_t(int, from + ops->len - iter.offs,
>> +                                    nand_page_size(nand) -
>> iter.pageoffs);
>> +                       memcpy(ops->datbuf + ops->retlen,
>> +                              chip->buf + iter.pageoffs, size);
>> +                       ops->retlen += size;
>> +               }
>> +               if (ops->oobbuf) {
>> +                       size = min_t(int, iter.oobleft, ooblen);
>> +                       ret = spinand_transfer_oob(chip,
>> +                                                  ops->oobbuf +
>> ops->oobretlen,
>> +                                                  ops, size);
>> +                       if (ret) {
>> +                               pr_err("Transfer oob error %d\n", ret);
>> +                               return ret;
>> +                       }
>> +                       ops->oobretlen += size;
>> +               }
>> +       }
>> +
>> +       return ret;
>> +}
>> +
>> +/*
>> + * spinand_do_read_ops - read data from device to buffer
>> + * @mtd: MTD device structure
>> + * @from: offset to read from
>> + * @ops: oob operations description structure
>> + */
>> +static int spinand_do_read_ops(struct mtd_info *mtd, loff_t from,
>> +                              struct mtd_oob_ops *ops)
>> +{
>> +       struct spinand_device *chip = mtd_to_spinand(mtd);
>> +       struct nand_device *nand = mtd_to_nand(mtd);
>> +       int ret;
>> +       struct mtd_ecc_stats stats;
>> +       unsigned int max_bitflips = 0;
>> +       bool ecc_off = ops->mode == MTD_OPS_RAW;
>> +
>> +       if (!valid_nand_address(nand, from)) {
>> +               pr_err("%s: invalid read address\n", __func__);
>> +               return -EINVAL;
>> +       }
>> +       if ((ops->ooblen > 0) && !valid_nand_oob_ops(nand, from, ops)) {
>> +               pr_err("%s: invalid oob operation input\n", __func__);
>> +               return -EINVAL;
>> +       }
>> +       mutex_lock(&chip->lock);
>> +       stats = mtd->ecc_stats;
>> +       if (ecc_off)
>> +               spinand_disable_ecc(chip);
>> +       ret = spinand_read_pages(mtd, from, ops, &max_bitflips);
>> +       if (ecc_off)
>> +               spinand_enable_ecc(chip);
>> +       if (ret)
>> +               goto out;
>> +
>> +       if (mtd->ecc_stats.failed - stats.failed) {
>> +               ret = -EBADMSG;
>> +               goto out;
>> +       }
>> +       ret = max_bitflips;
>> +
>> +out:
>> +       mutex_unlock(&chip->lock);
>> +       return ret;
>> +}
>> +
>
>