| Message ID | 1334615755-15418-3-git-send-email-computersforpeace@gmail.com |
|---|---|
| State | New, archived |
| Headers | show |
Hi Brian, On Mon, 16 Apr 2012 15:35:55 -0700 Brian Norris <computersforpeace@gmail.com> wrote: > Now that we have a function parameter for the OOB buffer, we can pass the OOB > buffer as an argument to the nand_ecc_ctrl functions. This allows drivers to > know when OOB data must be returned to the upper layers and when it is simply > needed for internal calculations, potentially saving time for NAND HW/SW that > can simply avoid reading the OOB data. I think for consistency sake, existing chip->ecc.{read,write}_page_xxx methods do need to be ported to support the new 'oob' parameter. > @@ -2272,12 +2272,14 @@ static int nand_do_write_ops(struct mtd_info *mtd, loff_t to, > size_t len = min(oobwritelen, oobmaxlen); > oob = nand_fill_oob(mtd, oob, len, ops); > oobwritelen -= len; > + oobpoi = chip->oob_poi; > } else { > + oobpoi = NULL; > /* We still need to erase leftover OOB data */ > memset(chip->oob_poi, 0xff, mtd->oobsize); > } > > - ret = chip->write_page(mtd, chip, wbuf, NULL, page, cached, > + ret = chip->write_page(mtd, chip, wbuf, oobpoi, page, cached, > (ops->mode == MTD_OPS_RAW)); > if (ret) > break; The 'write_page' interface is problematic, as the meaning of 'oob' parameter is a bit inconsistent: - A NULL 'oob' actually states "no OOB buffer to write" - Your driver instructs HW to write the page (ECC taken care of by HW) - However default chip->ecc.write_page_xxx methods do need a temp buffer for OOB ECC calculation (hence will probably use the internal chip->oob_poi buffer) - But when non-null 'oob' is passed to the default methods, they should probably use the given 'oob' buffer (and not a temp buffer) (This is same for the read interface.) So the 'oob' parameter is more of a boolean than an actual buffer to be used by the various ecc.{read,write}_page implementors. Any reason not to pass a boolean instead? Regards, Shmulik
On 4/18/2012 4:52 AM, Shmulik Ladkani wrote: > On Mon, 16 Apr 2012 15:35:55 -0700 Brian Norris<computersforpeace@gmail.com> wrote: >> Now that we have a function parameter for the OOB buffer, we can pass the OOB >> buffer as an argument to the nand_ecc_ctrl functions. This allows drivers to >> know when OOB data must be returned to the upper layers and when it is simply >> needed for internal calculations, potentially saving time for NAND HW/SW that >> can simply avoid reading the OOB data. > > I think for consistency sake, existing chip->ecc.{read,write}_page_xxx > methods do need to be ported to support the new 'oob' parameter. OK, but it's difficult to tell sometimes what is and isn't needed; some drivers might expect OOB data in chip->oob_poi unconditionally so they can perform correction, whereas others might fill up buffers that won't be used in the end. >> @@ -2272,12 +2272,14 @@ static int nand_do_write_ops(struct mtd_info *mtd, loff_t to, >> size_t len = min(oobwritelen, oobmaxlen); >> oob = nand_fill_oob(mtd, oob, len, ops); >> oobwritelen -= len; >> + oobpoi = chip->oob_poi; >> } else { >> + oobpoi = NULL; >> /* We still need to erase leftover OOB data */ >> memset(chip->oob_poi, 0xff, mtd->oobsize); >> } >> >> - ret = chip->write_page(mtd, chip, wbuf, NULL, page, cached, >> + ret = chip->write_page(mtd, chip, wbuf, oobpoi, page, cached, >> (ops->mode == MTD_OPS_RAW)); >> if (ret) >> break; > > The 'write_page' interface is problematic, as the meaning of 'oob' > parameter is a bit inconsistent: > - A NULL 'oob' actually states "no OOB buffer to write" > - Your driver instructs HW to write the page (ECC taken care of by HW) > - However default chip->ecc.write_page_xxx methods do need a temp buffer > for OOB ECC calculation (hence will probably use the internal > chip->oob_poi buffer) Right, this is a trouble spot for 'porting them to support the new oob parameter', since many driver-users still need a buffer even when OOB is not needed for the higher levels. > - But when non-null 'oob' is passed to the default methods, they should > probably use the given 'oob' buffer (and not a temp buffer) Yes. This gets strange and potentially ugly, with code snippets like below. > (This is same for the read interface.) > > So the 'oob' parameter is more of a boolean than an actual buffer to be > used by the various ecc.{read,write}_page implementors. Yes, I suppose so. I naturally used 'oob' as a buffer, since that's very straightforward and logical from a 'layers' perspective and because my driver doesn't need any buffer when oob is not required. But I see that it essentially would become a boolean flag for many of the other interfaces, and so a boolean can work just as well. > Any reason not to pass a boolean instead? Only reason I'm thinking of: a cleaner interface. To me, the interface is rather non-obvious and ugly when data is constantly shuttled back and forth behind the scenes (i.e., not via function arguments or ret values) by using chip->oob_poi. However, this sense of "ugliness" competes with the ugliness of needing a buffer even when the interface might otherwise say "no OOB." Many {read,write}_page functions would need something like: uint8_t *oobbuf = oob ? oob : chip->oob_poi; which is not pretty. I'm open to either way, I guess, but I'm now leaning a little toward 'oob' as a boolean. Brian
Hi Brian, On Wed, 18 Apr 2012 09:13:38 -0700 Brian Norris <computersforpeace@gmail.com> wrote: > > So the 'oob' parameter is more of a boolean than an actual buffer to be > > used by the various ecc.{read,write}_page implementors. > > Yes, I suppose so. I naturally used 'oob' as a buffer, since that's very > straightforward and logical from a 'layers' perspective and because my > driver doesn't need any buffer when oob is not required. But I see that > it essentially would become a boolean flag for many of the other > interfaces, and so a boolean can work just as well. > > > Any reason not to pass a boolean instead? > > Only reason I'm thinking of: a cleaner interface. > > To me, the interface is rather non-obvious and ugly when data is > constantly shuttled back and forth behind the scenes (i.e., not via > function arguments or ret values) by using chip->oob_poi. > > However, this sense of "ugliness" competes with the ugliness of needing > a buffer even when the interface might otherwise say "no OOB." Many > {read,write}_page functions would need something like: > > uint8_t *oobbuf = oob ? oob : chip->oob_poi; > > which is not pretty. > > I'm open to either way, I guess, but I'm now leaning a little toward > 'oob' as a boolean. Yes, both approaches aren't perfect. However a 'int has_user_oob' (or 'has_oob') has a precise, consistent meaning: the mtd user explicitly provided an OOB buffer (or in the read case, the mtd user expects the OOB to be returned). (need help with the boolean's name, though) Also, I think it will result in less changes (makes the whole s/oob/oobbuf/ collateral damage of 1st patch irrelevant). So IMO a boolean is better. Regards, Shmulik
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c index 95ba987..a206f43 100644 --- a/drivers/mtd/nand/nand_base.c +++ b/drivers/mtd/nand/nand_base.c @@ -1475,7 +1475,7 @@ static int nand_do_read_ops(struct mtd_info *mtd, loff_t from, uint32_t max_oobsize = ops->mode == MTD_OPS_AUTO_OOB ? mtd->oobavail : mtd->oobsize; - uint8_t *bufpoi, *oob, *buf; + uint8_t *bufpoi, *oobpoi, *oob, *buf; stats = mtd->ecc_stats; @@ -1489,6 +1489,7 @@ static int nand_do_read_ops(struct mtd_info *mtd, loff_t from, buf = ops->datbuf; oob = ops->oobbuf; + oobpoi = oob ? chip->oob_poi : NULL; while (1) { bytes = min(mtd->writesize - col, readlen); @@ -1506,13 +1507,13 @@ static int nand_do_read_ops(struct mtd_info *mtd, loff_t from, /* Now read the page into the buffer */ if (unlikely(ops->mode == MTD_OPS_RAW)) ret = chip->ecc.read_page_raw(mtd, chip, bufpoi, - NULL, page); + oobpoi, 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, - NULL, page); + oobpoi, page); if (ret < 0) { if (!aligned) /* Invalidate page cache */ @@ -1535,7 +1536,6 @@ static int nand_do_read_ops(struct mtd_info *mtd, loff_t from, buf += bytes; if (unlikely(oob)) { - int toread = min(oobreadlen, max_oobsize); if (toread) { @@ -2256,7 +2256,7 @@ static int nand_do_write_ops(struct mtd_info *mtd, loff_t to, while (1) { int bytes = mtd->writesize; int cached = writelen > bytes && page != blockmask; - uint8_t *wbuf = buf; + uint8_t *wbuf = buf, *oobpoi; /* Partial page write? */ if (unlikely(column || writelen < (mtd->writesize - 1))) { @@ -2272,12 +2272,14 @@ static int nand_do_write_ops(struct mtd_info *mtd, loff_t to, size_t len = min(oobwritelen, oobmaxlen); oob = nand_fill_oob(mtd, oob, len, ops); oobwritelen -= len; + oobpoi = chip->oob_poi; } else { + oobpoi = NULL; /* We still need to erase leftover OOB data */ memset(chip->oob_poi, 0xff, mtd->oobsize); } - ret = chip->write_page(mtd, chip, wbuf, NULL, page, cached, + ret = chip->write_page(mtd, chip, wbuf, oobpoi, page, cached, (ops->mode == MTD_OPS_RAW)); if (ret) break;
Now that we have a function parameter for the OOB buffer, we can pass the OOB buffer as an argument to the nand_ecc_ctrl functions. This allows drivers to know when OOB data must be returned to the upper layers and when it is simply needed for internal calculations, potentially saving time for NAND HW/SW that can simply avoid reading the OOB data. Signed-off-by: Brian Norris <computersforpeace@gmail.com> --- drivers/mtd/nand/nand_base.c | 14 ++++++++------ 1 files changed, 8 insertions(+), 6 deletions(-)