Patchwork flash bbt broken due to unitialized bitflip_threshold?

login
register
mail settings
Submitter Brian Norris
Date June 22, 2012, 8:39 p.m.
Message ID <CAN8TOE90sB0-ZHZPX4+ASUDBCSRHgumVqDu0BOrcphyijm6Xgw@mail.gmail.com>
Download mbox | patch
Permalink /patch/166676/
State New
Headers show

Comments

Brian Norris - June 22, 2012, 8:39 p.m.
On Sun, Jun 10, 2012 at 12:08 AM, Shmulik Ladkani
<shmulik.ladkani@gmail.com> wrote:
> To summarize, return value of the 'mtd_read_oob()' API is
> inconsistent (sometimes max_bitflips, other times EUCLEAN), and does not
> adhere to return code policy of 'mtd_read()' - return EUCLEAN to the users,
> they're not interested with internals such as max_bitflips.
>
> This might affect users of 'mtd_read_oob()' which might falsely think
> the OOB read has failed.
>
> Mike, care to take a look and amend if necessary?
> I think this needs to be fixed pre v3.5 as well.

Mike, are you planning on handling this?

I think it would be safe to basically C&P the max_bitflips code from
mtd_read() into mtd_read_oob(). This would simply pass through the
EUCLEAN that may be returned for OOB-only case (only for my driver?),
and it would translate the 'ret_code > 0' case properly for data+OOB
reads. I'll resend officially if this looks OK. Or I'll defer to Mike
if he's working on this.

Brian

 static inline int mtd_write_oob(struct mtd_info *mtd, loff_t to,
                                struct mtd_oob_ops *ops)
Mike Dunn - June 25, 2012, 5:44 p.m.
On 06/22/2012 01:39 PM, Brian Norris wrote:
> On Sun, Jun 10, 2012 at 12:08 AM, Shmulik Ladkani
> <shmulik.ladkani@gmail.com> wrote:
>> To summarize, return value of the 'mtd_read_oob()' API is
>> inconsistent (sometimes max_bitflips, other times EUCLEAN), and does not
>> adhere to return code policy of 'mtd_read()' - return EUCLEAN to the users,
>> they're not interested with internals such as max_bitflips.
>>
>> This might affect users of 'mtd_read_oob()' which might falsely think
>> the OOB read has failed.
>>
>> Mike, care to take a look and amend if necessary?
>> I think this needs to be fixed pre v3.5 as well.
> 
> Mike, are you planning on handling this?
> 
> I think it would be safe to basically C&P the max_bitflips code from
> mtd_read() into mtd_read_oob(). This would simply pass through the
> EUCLEAN that may be returned for OOB-only case (only for my driver?),
> and it would translate the 'ret_code > 0' case properly for data+OOB
> reads. I'll resend officially if this looks OK. Or I'll defer to Mike
> if he's working on this.


I'm sorry Brian, I missed Shmulik's post and thought this issue was put to bed
with the patch that initialized bitflip_threshold before the nand_scan_tail()
call.

I will take a look at your patch set that addresses this.  Sorry for dropping
the ball.

Mike

Patch

diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 5757307..37c9820 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -821,6 +821,27 @@  int mtd_read(struct mtd_info *mtd, loff_t from,
size_t len, size_t *retlen,
 }
 EXPORT_SYMBOL_GPL(mtd_read);

+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;
+       /*
+        * 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);
+
 int mtd_write(struct mtd_info *mtd, loff_t to, size_t len, size_t *retlen,
              const u_char *buf)
 {
diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
index 63dadc0..81d61e7 100644
--- a/include/linux/mtd/mtd.h
+++ b/include/linux/mtd/mtd.h
@@ -265,14 +265,7 @@  int mtd_write(struct mtd_info *mtd, loff_t to,
size_t len, size_t *retlen,
 int mtd_panic_write(struct mtd_info *mtd, loff_t to, size_t len,
size_t *retlen,
                    const u_char *buf);

-static inline int mtd_read_oob(struct mtd_info *mtd, loff_t from,
-                              struct mtd_oob_ops *ops)
-{
-       ops->retlen = ops->oobretlen = 0;
-       if (!mtd->_read_oob)
-               return -EOPNOTSUPP;
-       return mtd->_read_oob(mtd, from, ops);
-}
+int mtd_read_oob(struct mtd_info *mtd, loff_t from, struct mtd_oob_ops *ops);