Re: [PATCH 2/8] mtd: check for max_bitflips in mtd_read_oob()

From: Brian Norris <computersforpeace@gmail.com>
To: Mike Dunn <mikedunn@newsguy.com>
Cc: David Woodhouse <dwmw2@infradead.org>,
	linux-mtd@lists.infradead.org,
	Shmulik Ladkani <shmulik.ladkani@gmail.com>,
	Artem Bityutskiy <dedekind1@gmail.com>
Subject: Re: [PATCH 2/8] mtd: check for max_bitflips in mtd_read_oob()
Date: Tue, 10 Jul 2012 19:12:27 -0700	[thread overview]
Message-ID: <CAN8TOE8d73CMSuNeLDQ7EiBA0mR9rmA-fCf_QQvKbVvsO4X69w@mail.gmail.com> (raw)
In-Reply-To: <4FE9FE1A.9070801@newsguy.com>

On Tue, Jun 26, 2012 at 11:23 AM, Mike Dunn <mikedunn@newsguy.com> wrote:
> Reviewed-by Mike Dunn <mikedunn@newsguy.com>

I'll add your/Shmulik's tags in a resend. This is getting a bit late
in the rc cycle, but it should still go in on 3.5 right?

> This should fix the problem, but it's still confusing and inconsistent; see below...
>
> On 06/22/2012 04:35 PM, Brian Norris wrote:
>> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
>> index fcfce24..75288d3 100644
>> --- a/drivers/mtd/mtdcore.c
>> +++ b/drivers/mtd/mtdcore.c
>> @@ -860,10 +860,22 @@ EXPORT_SYMBOL_GPL(mtd_panic_write);
>>
>>  int mtd_read_oob(struct mtd_info *mtd, loff_t from, struct mtd_oob_ops *ops)
>>  {
>> +     int ret_code;
>>       ops->retlen = ops->oobretlen = 0;
>>       if (!mtd->_read_oob)
>>               return -EOPNOTSUPP;
>> -     return mtd->_read_oob(mtd, from, ops);
>> +     /*
>> +      * In cases where ops->datbuf != NULL, mtd->_read_oob() can have
>> +      * semantics similar to mtd->_read(), regarding max bitflips. In other
>> +      * cases, mtd->_read_oob() may return -EUCLEAN. In all cases, perform
>> +      * similar logic to mtd_read() (see above).
>> +      */
>> +     ret_code = mtd->_read_oob(mtd, from, ops);
>> +     if (unlikely(ret_code < 0))
>> +             return ret_code;
>
> As Brian explains in the comment, here the return code propagated up could be
> -EUCLEAN for an oob-only read (ops->databuf == NULL), which is unlike the
> mtd_read() case, where here only a hard error will be propagated up.  To be
> consistent, nand_do_read_oob(), and non-nand drivers' implementation of
> mtd->_read_oob(), should not return -EUCLEAN.  When (or if) a policy is decided
> for reporting bitflips within the oob area, this will need to be revisited.

Yeah, this seems to be a recurring theme: that no in-tree drivers
actually implement ECC for the OOB region. If no other drivers need to
report ECC flips/errors from OOB, then I can't really establish a
policy here. Perhaps I can rethink my driver sometime...

> Thanks Brian!

You're welcome,
Brian