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

login
register
mail settings
Submitter Brian Norris
Date June 22, 2012, 11:35 p.m.
Message ID <1340408145-24531-3-git-send-email-computersforpeace@gmail.com>
Download mbox | patch
Permalink /patch/166692/
State New
Headers show

Comments

Brian Norris - June 22, 2012, 11:35 p.m.
mtd_read_oob() has some unexpected similarities to mtd_read(). For
instance, when ops->datbuf != NULL, nand_base.c might return max_bitflips;
however, when ops->datbuf == NULL, nand_base's code potentially could
return -EUCLEAN (no in-tree drivers do this yet). In any case where the
driver might return max_bitflips, we should translate this into an
appropriate return code using the bitflip_threshold.

Essentially, mtd_read_oob() duplicates the logic from mtd_read().

This prevents users of mtd_read_oob() from receiving a positive return
value (i.e., from max_bitflips) and interpreting it as an unknown error.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
Cc: Shmulik Ladkani <shmulik.ladkani@gmail.com>
Cc: Mike Dunn <mikedunn@newsguy.com>
---
 drivers/mtd/mtdcore.c |   14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)
Shmulik Ladkani - June 26, 2012, 12:11 p.m.
Hi Brian,

On Fri, 22 Jun 2012 16:35:39 -0700 Brian Norris <computersforpeace@gmail.com> 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

s/can have/has/

> +	 * semantics similar to mtd->_read(), regarding max bitflips. In other

Please consider:
s/regarding max bitflips/returning a non-negative integer representing max bitflips/

Other than these tiny comment amendments,

Reviewed-by: Shmulik Ladkani <shmulik.ladkani@gmail.com>

Thanks,
Shmulik
Mike Dunn - June 26, 2012, 6:23 p.m.
On 06/22/2012 04:35 PM, Brian Norris wrote:
> mtd_read_oob() has some unexpected similarities to mtd_read(). For
> instance, when ops->datbuf != NULL, nand_base.c might return max_bitflips;
> however, when ops->datbuf == NULL, nand_base's code potentially could
> return -EUCLEAN (no in-tree drivers do this yet). In any case where the
> driver might return max_bitflips, we should translate this into an
> appropriate return code using the bitflip_threshold.
> 
> Essentially, mtd_read_oob() duplicates the logic from mtd_read().
> 
> This prevents users of mtd_read_oob() from receiving a positive return
> value (i.e., from max_bitflips) and interpreting it as an unknown error.


Reviewed-by Mike Dunn <mikedunn@newsguy.com>

This should fix the problem, but it's still confusing and inconsistent; see below...


> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> Cc: Shmulik Ladkani <shmulik.ladkani@gmail.com>
> Cc: Mike Dunn <mikedunn@newsguy.com>
> ---
>  drivers/mtd/mtdcore.c |   14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> 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.

Thanks Brian!


> +	if (mtd->ecc_strength == 0)
> +		return 0;	/* device lacks ecc */
> +	return ret_code >= mtd->bitflip_threshold ? -EUCLEAN : 0;
>  }
>  EXPORT_SYMBOL_GPL(mtd_read_oob);
>
Brian Norris - July 11, 2012, 2:12 a.m.
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
Artem Bityutskiy - Aug. 15, 2012, 11:24 a.m.
On Fri, 2012-06-22 at 16:35 -0700, Brian Norris wrote:
> mtd_read_oob() has some unexpected similarities to mtd_read(). For
> instance, when ops->datbuf != NULL, nand_base.c might return max_bitflips;
> however, when ops->datbuf == NULL, nand_base's code potentially could
> return -EUCLEAN (no in-tree drivers do this yet). In any case where the
> driver might return max_bitflips, we should translate this into an
> appropriate return code using the bitflip_threshold.
> 
> Essentially, mtd_read_oob() duplicates the logic from mtd_read().
> 
> This prevents users of mtd_read_oob() from receiving a positive return
> value (i.e., from max_bitflips) and interpreting it as an unknown error.

Pushed this patch to l2-mtd.git, thanks. I've amended the commentary as
Smulik requested, and also removed "(see above)", because "above" is (a)
unclear, and (b) if it means 'mtd_read()', we'd need to make sure
'mtd_read()' is always above 'mtd_read_oob()'.

Thanks!
Brian Norris - Aug. 15, 2012, 7:15 p.m.
On Wed, Aug 15, 2012 at 4:24 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> Pushed this patch to l2-mtd.git, thanks. I've amended the commentary as
> Smulik requested, and also removed "(see above)", because "above" is (a)
> unclear, and (b) if it means 'mtd_read()', we'd need to make sure
> 'mtd_read()' is always above 'mtd_read_oob()'.

Good idea, removing "(see above)". Readers should be smart enough to
locate mtd_read() and its accompanying comments themselves anyway.

Note that there was a v2 of this patch, although the difference was
very minor (the comments and the Reviewed-by's). The end result is
almost the same as your amendments.

Thanks,
Brian
Artem Bityutskiy - Aug. 16, 2012, 10:48 a.m.
On Wed, 2012-08-15 at 12:15 -0700, Brian Norris wrote:
> Note that there was a v2 of this patch, although the difference was
> very minor (the comments and the Reviewed-by's). The end result is
> almost the same as your amendments.

Reviewed-by tags are important for me, because people spent time and
they should be credited in some form in git log. I actually collected
all the tags from the thread and added them when applying your patches. 

Thanks!
Brian Norris - Aug. 17, 2012, 10:58 p.m.
On Thu, Aug 16, 2012 at 3:48 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> On Wed, 2012-08-15 at 12:15 -0700, Brian Norris wrote:
>> Note that there was a v2 of this patch, although the difference was
>> very minor (the comments and the Reviewed-by's). The end result is
>> almost the same as your amendments.
>
> Reviewed-by tags are important for me, because people spent time and
> they should be credited in some form in git log.

Right, I understood this, and those tags were reflected in my v2.

> I actually collected
> all the tags from the thread and added them when applying your patches.

Yes, I was only commenting that while you took and amended my v1, it
ended up being nearly identical to my v2 - including the Reviewed-by
tags. They only differed by some punctuation or something else
trivial.

Thanks!

Brian

Patch

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;
+	if (mtd->ecc_strength == 0)
+		return 0;	/* device lacks ecc */
+	return ret_code >= mtd->bitflip_threshold ? -EUCLEAN : 0;
 }
 EXPORT_SYMBOL_GPL(mtd_read_oob);