| Message ID | 20120509131334.2d0b7160@pixies.home.jungo.com |
|---|---|
| State | Accepted |
| Commit | 1951f2f710a621ae0bc4268617046a6c02c634d0 |
| Headers | show |
On Wed, 2012-05-09 at 13:13 +0300, Shmulik Ladkani wrote: > @@ -1826,9 +1827,12 @@ static int nand_do_read_oob(struct mtd_info *mtd, loff_t from, > > while (1) { > if (ops->mode == MTD_OPS_RAW) > - chip->ecc.read_oob_raw(mtd, chip, page); > + ret = chip->ecc.read_oob_raw(mtd, chip, page); > else > - chip->ecc.read_oob(mtd, chip, page); > + ret = chip->ecc.read_oob(mtd, chip, page); > + > + if (ret < 0) > + break; For page reading the convention is that we keep reading and try to read everything anyway, I guess it is reasonable thing to do for OOB as well?
Hi Artem, On Fri, 11 May 2012 14:48:11 +0300 Artem Bityutskiy <dedekind1@gmail.com> wrote: > On Wed, 2012-05-09 at 13:13 +0300, Shmulik Ladkani wrote: > > @@ -1826,9 +1827,12 @@ static int nand_do_read_oob(struct mtd_info *mtd, loff_t from, > > > > while (1) { > > if (ops->mode == MTD_OPS_RAW) > > - chip->ecc.read_oob_raw(mtd, chip, page); > > + ret = chip->ecc.read_oob_raw(mtd, chip, page); > > else > > - chip->ecc.read_oob(mtd, chip, page); > > + ret = chip->ecc.read_oob(mtd, chip, page); > > + > > + if (ret < 0) > > + break; > > For page reading the convention is that we keep reading and try to read > everything anyway, I guess it is reasonable thing to do for OOB as well? AFAIU, we actually _stop_ reading upon 'ecc.read_page()' error. And 'ops->retlen' is updated to reflect actual bytes sucessfully read. See snip from 'nand_do_read_ops': --------------------------------- while (1) { ... ... chip->cmdfunc(mtd, NAND_CMD_READ0, 0x00, page); /* * Now read the page into the buffer. Absent an error, * the read methods return max bitflips per ecc step. */ if (unlikely(ops->mode == MTD_OPS_RAW)) ret = chip->ecc.read_page_raw(mtd, chip, bufpoi, oob_required, page); else if (!aligned && NAND_SUBPAGE_READ(chip) && !oob) ret = chip->ecc.read_subpage(mtd, chip, col, bytes, bufpoi); else ret = chip->ecc.read_page(mtd, chip, bufpoi, oob_required, page); if (ret < 0) { if (!aligned) /* Invalidate page cache */ chip->pagebuf = -1; break; } ... ... } ops->retlen = ops->len - (size_t) readlen; if (oob) ops->oobretlen = ops->ooblen - oobreadlen; if (ret < 0) return ret; --------------------------------- The error is propagated back to 'nand_read' and to the 'mtd->_read' user. Have I misinterpreted your question? Did you mean something else? BTW, note that the patch does not intend to change existing behavior of '_read_oob' in case 'ecc.read_oob()' calls were succesful. The behavior is changed only if 'ecc.read_oob()' fails. In that case an error indication is returned and 'ops->oobretlen' is set appropriately. I havn't tested this, but it seems as the reaonable thing to report back to the '_read_oob' users. Regards, Shmulik
On Fri, 2012-05-11 at 15:54 +0300, Shmulik Ladkani wrote: > Hi Artem, > > On Fri, 11 May 2012 14:48:11 +0300 Artem Bityutskiy <dedekind1@gmail.com> wrote: > > On Wed, 2012-05-09 at 13:13 +0300, Shmulik Ladkani wrote: > > > @@ -1826,9 +1827,12 @@ static int nand_do_read_oob(struct mtd_info *mtd, loff_t from, > > > > > > while (1) { > > > if (ops->mode == MTD_OPS_RAW) > > > - chip->ecc.read_oob_raw(mtd, chip, page); > > > + ret = chip->ecc.read_oob_raw(mtd, chip, page); > > > else > > > - chip->ecc.read_oob(mtd, chip, page); > > > + ret = chip->ecc.read_oob(mtd, chip, page); > > > + > > > + if (ret < 0) > > > + break; > > > > For page reading the convention is that we keep reading and try to read > > everything anyway, I guess it is reasonable thing to do for OOB as well? > > AFAIU, we actually _stop_ reading upon 'ecc.read_page()' error. > And 'ops->retlen' is updated to reflect actual bytes sucessfully read. Yes, you are right. It should keep reading in case of an ECC error, not in case of any error.
On Wed, 2012-05-09 at 13:13 +0300, Shmulik Ladkani wrote: > Apparently, there is an implementor of 'read_oob' which may return an > error inidication (e.g. docg4_read_oob may return -EIO). > > Test the return value of 'read_oob/read_oob_raw', and if negative, > propagate the error, so it's returned by the '_read_oob' interface. > > Signed-off-by: Shmulik Ladkani <shmulik.ladkani@gmail.com> Pushed to l2-mtd.git, thanks!
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c index 4047d7c..d47586c 100644 --- a/drivers/mtd/nand/nand_base.c +++ b/drivers/mtd/nand/nand_base.c @@ -1791,6 +1791,7 @@ static int nand_do_read_oob(struct mtd_info *mtd, loff_t from, int readlen = ops->ooblen; int len; uint8_t *buf = ops->oobbuf; + int ret = 0; pr_debug("%s: from = 0x%08Lx, len = %i\n", __func__, (unsigned long long)from, readlen); @@ -1826,9 +1827,12 @@ static int nand_do_read_oob(struct mtd_info *mtd, loff_t from, while (1) { if (ops->mode == MTD_OPS_RAW) - chip->ecc.read_oob_raw(mtd, chip, page); + ret = chip->ecc.read_oob_raw(mtd, chip, page); else - chip->ecc.read_oob(mtd, chip, page); + ret = chip->ecc.read_oob(mtd, chip, page); + + if (ret < 0) + break; len = min(len, readlen); buf = nand_transfer_oob(chip, buf, ops, len); @@ -1857,7 +1861,10 @@ static int nand_do_read_oob(struct mtd_info *mtd, loff_t from, } } - ops->oobretlen = ops->ooblen; + ops->oobretlen = ops->ooblen - readlen; + + if (ret < 0) + return ret; if (mtd->ecc_stats.failed - stats.failed) return -EBADMSG;
Apparently, there is an implementor of 'read_oob' which may return an error inidication (e.g. docg4_read_oob may return -EIO). Test the return value of 'read_oob/read_oob_raw', and if negative, propagate the error, so it's returned by the '_read_oob' interface. Signed-off-by: Shmulik Ladkani <shmulik.ladkani@gmail.com> --- drivers/mtd/nand/nand_base.c | 13 ++++++++++--- 1 files changed, 10 insertions(+), 3 deletions(-)