| Message ID | 1340408145-24531-3-git-send-email-computersforpeace@gmail.com |
|---|---|
| State | New, archived |
| Headers | show |
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
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); >
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
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!
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
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!
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
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);
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(-)