| Message ID | 1396308537-16013-6-git-send-email-davidm@egauge.net |
|---|---|
| State | Rejected |
| Headers | show |
Hi David, >From: David Mosberger > >When NAND_STATUS_REWRITE is set, there is no direct indication as to >how many bits have were flipped. This patch improves the existing >code by manually counting the number of bitflips, rather than just >assuming the worst-case of mtd->bitflip_threshold. This avoids >unnecessary rewrites, e.g., due to a single stuck bit. > >Signed-off-by: David Mosberger <davidm@egauge.net> >--- > drivers/mtd/nand/nand_base.c | 90 ++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 86 insertions(+), 4 deletions(-) > >diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c >index 834352a..80cfaa8 100644 >--- a/drivers/mtd/nand/nand_base.c >+++ b/drivers/mtd/nand/nand_base.c >@@ -1272,6 +1272,84 @@ static int nand_read_subpage(struct mtd_info *mtd, struct nand_chip *chip, > return max_bitflips; > } > >+static int >+set_on_die_ecc(struct mtd_info *mtd, struct nand_chip *chip, int on) >+{ >+ u8 data[ONFI_SUBFEATURE_PARAM_LEN] = { 0, }; >+ >+ if (chip->ecc.mode != NAND_ECC_HW_ON_DIE) >+ return 0; >+ >+ if (on) >+ data[0] = ONFI_FEATURE_ARRAY_OP_MODE_ENABLE_ON_DIE_ECC; >+ >+ return chip->onfi_set_features(mtd, chip, >+ ONFI_FEATURE_ADDR_ARRAY_OP_MODE, data); >+} >+ >+/* >+ * Return the number of bits that differ between buffers SRC1 and >+ * SRC2, both of which are LEN bytes long. >+ * >+ * This code could be optimized for, but it only gets called on pages >+ * with bitflips and compared to the cost of migrating an eraseblock, >+ * the execution time here is trivial... >+ */ >+static int >+bitdiff(const void *s1, const void *s2, size_t len) >+{ >+ const uint8_t *src1 = s1, *src2 = s2; >+ int count = 0, i; >+ >+ for (i = 0; i < len; ++i) >+ count += hweight8(*src1++ ^ *src2++); >+ return count; hweight8() might not be good option, how about using hweight32 ? >+} >+ >+static int >+check_for_bitflips(struct mtd_info *mtd, struct nand_chip *chip, int page) >+{ >+ int flips = 0, max_bitflips = 0, i, j, read_size; >+ uint8_t *chkbuf, *rawbuf, *chkoob, *rawoob; >+ uint32_t *eccpos; >+ >+ chkbuf = chip->buffers->chkbuf; >+ rawbuf = chip->buffers->rawbuf; >+ read_size = mtd->writesize + mtd->oobsize; >+ >+ /* Read entire page w/OOB area with on-die ECC on: */ >+ chip->read_buf(mtd, chkbuf, read_size); >+ >+ /* Re-read page with on-die ECC off: */ >+ set_on_die_ecc(mtd, chip, 0); >+ chip->cmdfunc(mtd, NAND_CMD_READ0, 0x00, page); >+ chip->read_buf(mtd, rawbuf, read_size); >+ set_on_die_ecc(mtd, chip, 1); >+ >+ chkoob = chkbuf + mtd->writesize; >+ rawoob = rawbuf + mtd->writesize; >+ eccpos = chip->ecc.layout->eccpos; >+ for (i = 0; i < chip->ecc.steps; ++i) { >+ /* Count bit flips in the actual data area: */ >+ flips = bitdiff(chkbuf, rawbuf, chip->ecc.size); Do you really need this as separate function call ? You could make it inline, like you did below of OOB. Also, you may stop checking bitflips, once count > chip->ecc.strength. Some of the discussion on different patch may be helpful here, please see Brian's comments on [1]. >+ /* Count bit flips in the ECC bytes: */ >+ for (j = 0; j < chip->ecc.bytes; ++j) { Again, why checking only for ecc.bytes ? should have been. >+ flips += hweight8(chkoob[*eccpos] ^ rawoob[*eccpos]); >+ ++eccpos; >+ } >+ if (flips > 0) >+ mtd->ecc_stats.corrected += flips; >+ max_bitflips = max_t(int, max_bitflips, flips); >+ chkbuf += chip->ecc.size; >+ rawbuf += chip->ecc.size; >+ } >+ >+ /* Re-issue the READ command for the actual data read that follows. */ >+ chip->cmdfunc(mtd, NAND_CMD_READ0, 0x00, page); >+ If you refer my previous feedbacks, you could avoided this extra "chip->read_buf(mtd, chkbuf, read_size);" by calling it in nand_page_read_on_die(); >+ return max_bitflips; >+} >+ > static int check_read_status_on_die(struct mtd_info *mtd, > struct nand_chip *chip, int page) > { >@@ -1290,11 +1368,15 @@ static int check_read_status_on_die(struct mtd_info *mtd, > mtd->ecc_stats.failed++; > } else if (status & NAND_STATUS_REWRITE) { > /* >- * Simple but suboptimal: any page with a single stuck >- * bit will be unusable since it'll be rewritten on >- * each read... >+ * The Micron chips turn on the REWRITE status bit for >+ * ANY bit flips. Some pages have stuck bits, so we >+ * don't want to migrate a block just because of >+ * single bit errors because otherwise, that block >+ * would effectively become unusable. So, work out in >+ * software what the max number of flipped bits is for >+ * all subpages in a page: > */ >- max_bitflips = mtd->bitflip_threshold; >+ max_bitflips = check_for_bitflips(mtd, chip, page); We usually don't delete the changes, introduced in previous patches of same series. So, take one approach and then logically divide the patches. > } > return max_bitflips; > } >-- >1.7.9.5 > Again, please review all the comments given by Gerhard and myself on previous patches. Don't hurry in sending newer revisions, as it would just increase your iterations. [1] http://lists.infradead.org/pipermail/linux-mtd/2014-March/052625.html with regards, pekon
On Mon, Mar 31, 2014 at 05:28:57PM -0600, David Mosberger wrote: > When NAND_STATUS_REWRITE is set, there is no direct indication as to > how many bits have were flipped. This patch improves the existing > code by manually counting the number of bitflips, rather than just > assuming the worst-case of mtd->bitflip_threshold. This avoids > unnecessary rewrites, e.g., due to a single stuck bit. > > Signed-off-by: David Mosberger <davidm@egauge.net> > --- > drivers/mtd/nand/nand_base.c | 90 ++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 86 insertions(+), 4 deletions(-) > > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c > index 834352a..80cfaa8 100644 > --- a/drivers/mtd/nand/nand_base.c > +++ b/drivers/mtd/nand/nand_base.c > @@ -1272,6 +1272,84 @@ static int nand_read_subpage(struct mtd_info *mtd, struct nand_chip *chip, > return max_bitflips; > } > > +static int > +set_on_die_ecc(struct mtd_info *mtd, struct nand_chip *chip, int on) > +{ > + u8 data[ONFI_SUBFEATURE_PARAM_LEN] = { 0, }; > + > + if (chip->ecc.mode != NAND_ECC_HW_ON_DIE) > + return 0; I think this check is unnecessary, and probably wrong. The caller should make sure not to call this for devices that don't support it. Or else, there should at least be an error code, like -EOPNOTSUPP. > + > + if (on) > + data[0] = ONFI_FEATURE_ARRAY_OP_MODE_ENABLE_ON_DIE_ECC; > + > + return chip->onfi_set_features(mtd, chip, > + ONFI_FEATURE_ADDR_ARRAY_OP_MODE, data); > +} This should be implemented on a per-vendor basis and provided as a callback (perhaps chip->set_internal_ecc()?). Then, you would only make chip->set_internal_ecc non-NULL for flash that support it. > + > +/* > + * Return the number of bits that differ between buffers SRC1 and > + * SRC2, both of which are LEN bytes long. > + * > + * This code could be optimized for, but it only gets called on pages > + * with bitflips and compared to the cost of migrating an eraseblock, > + * the execution time here is trivial... > + */ > +static int > +bitdiff(const void *s1, const void *s2, size_t len) Please join the above 2 lines. > +{ > + const uint8_t *src1 = s1, *src2 = s2; Kill the local variables. > + int count = 0, i; > + > + for (i = 0; i < len; ++i) > + count += hweight8(*src1++ ^ *src2++); It's rather unfortunate that we have to resort to this... but anyway, we might as well use hweight32() or hweight_long(), right? And any leftover odd bytes can be caught up with hweight8(). > + return count; > +} > + > +static int > +check_for_bitflips(struct mtd_info *mtd, struct nand_chip *chip, int page) > +{ > + int flips = 0, max_bitflips = 0, i, j, read_size; > + uint8_t *chkbuf, *rawbuf, *chkoob, *rawoob; > + uint32_t *eccpos; > + > + chkbuf = chip->buffers->chkbuf; > + rawbuf = chip->buffers->rawbuf; > + read_size = mtd->writesize + mtd->oobsize; > + > + /* Read entire page w/OOB area with on-die ECC on: */ > + chip->read_buf(mtd, chkbuf, read_size); Do you actually need to re-read, or can you use the existing data? Or at least, you could overwrite the databuf, instead of using a new chkbuf. > + > + /* Re-read page with on-die ECC off: */ > + set_on_die_ecc(mtd, chip, 0); > + chip->cmdfunc(mtd, NAND_CMD_READ0, 0x00, page); > + chip->read_buf(mtd, rawbuf, read_size); > + set_on_die_ecc(mtd, chip, 1); > + > + chkoob = chkbuf + mtd->writesize; > + rawoob = rawbuf + mtd->writesize; > + eccpos = chip->ecc.layout->eccpos; > + for (i = 0; i < chip->ecc.steps; ++i) { > + /* Count bit flips in the actual data area: */ > + flips = bitdiff(chkbuf, rawbuf, chip->ecc.size); > + /* Count bit flips in the ECC bytes: */ > + for (j = 0; j < chip->ecc.bytes; ++j) { > + flips += hweight8(chkoob[*eccpos] ^ rawoob[*eccpos]); Why didn't you use bitdiff() here too? > + ++eccpos; > + } > + if (flips > 0) > + mtd->ecc_stats.corrected += flips; > + max_bitflips = max_t(int, max_bitflips, flips); > + chkbuf += chip->ecc.size; > + rawbuf += chip->ecc.size; > + } > + > + /* Re-issue the READ command for the actual data read that follows. */ > + chip->cmdfunc(mtd, NAND_CMD_READ0, 0x00, page); > + > + return max_bitflips; > +} > + > static int check_read_status_on_die(struct mtd_info *mtd, > struct nand_chip *chip, int page) > { > @@ -1290,11 +1368,15 @@ static int check_read_status_on_die(struct mtd_info *mtd, > mtd->ecc_stats.failed++; > } else if (status & NAND_STATUS_REWRITE) { > /* > - * Simple but suboptimal: any page with a single stuck > - * bit will be unusable since it'll be rewritten on > - * each read... > + * The Micron chips turn on the REWRITE status bit for > + * ANY bit flips. Some pages have stuck bits, so we > + * don't want to migrate a block just because of > + * single bit errors because otherwise, that block > + * would effectively become unusable. So, work out in > + * software what the max number of flipped bits is for > + * all subpages in a page: Can you shorten this comment? It's rather verbose, and it's making assumptions about upper-layer "migrations". I think we can leave it at something much simpler, like: /* * Micron on-die ECC doesn't report the number of bitflips, so * we have to count them ourself to see if the error rate is too * high. */ > */ > - max_bitflips = mtd->bitflip_threshold; > + max_bitflips = check_for_bitflips(mtd, chip, page); > } > return max_bitflips; > } Brian
Pekon, On Tue, Apr 1, 2014 at 12:29 AM, Gupta, Pekon <pekon@ti.com> wrote: >>+/* >>+ * Return the number of bits that differ between buffers SRC1 and >>+ * SRC2, both of which are LEN bytes long. >>+ * >>+ * This code could be optimized for, but it only gets called on pages >>+ * with bitflips and compared to the cost of migrating an eraseblock, >>+ * the execution time here is trivial... >>+ */ >>+static int >>+bitdiff(const void *s1, const void *s2, size_t len) >>+{ >>+ const uint8_t *src1 = s1, *src2 = s2; >>+ int count = 0, i; >>+ >>+ for (i = 0; i < len; ++i) >>+ count += hweight8(*src1++ ^ *src2++); >>+ return count; > > hweight8() might not be good option, how about using hweight32 ? Why don't you read the comment above. Yes, you can add more code and make it faster and it will not matter one bit, so I chose to go with the shortest code possible. Sue me. >>+ flips += hweight8(chkoob[*eccpos] ^ rawoob[*eccpos]); >>+ ++eccpos; >>+ } >>+ if (flips > 0) >>+ mtd->ecc_stats.corrected += flips; >>+ max_bitflips = max_t(int, max_bitflips, flips); >>+ chkbuf += chip->ecc.size; >>+ rawbuf += chip->ecc.size; >>+ } >>+ >>+ /* Re-issue the READ command for the actual data read that follows. */ >>+ chip->cmdfunc(mtd, NAND_CMD_READ0, 0x00, page); >>+ > > If you refer my previous feedbacks, you could avoided this extra > "chip->read_buf(mtd, chkbuf, read_size);" by calling it in nand_page_read_on_die(); The code you quote has no chip->read_buf call so I'm not sure how to avoid something that isn't there. I'm more than happy to address your concern once I understand what it is. --david
On Tue, Apr 01, 2014 at 09:51:13AM -0600, David Mosberger wrote: > On Tue, Apr 1, 2014 at 12:29 AM, Gupta, Pekon <pekon@ti.com> wrote: > >>+/* > >>+ * Return the number of bits that differ between buffers SRC1 and > >>+ * SRC2, both of which are LEN bytes long. > >>+ * > >>+ * This code could be optimized for, but it only gets called on pages > >>+ * with bitflips and compared to the cost of migrating an eraseblock, > >>+ * the execution time here is trivial... > >>+ */ > >>+static int > >>+bitdiff(const void *s1, const void *s2, size_t len) > >>+{ > >>+ const uint8_t *src1 = s1, *src2 = s2; > >>+ int count = 0, i; > >>+ > >>+ for (i = 0; i < len; ++i) > >>+ count += hweight8(*src1++ ^ *src2++); > >>+ return count; > > > > hweight8() might not be good option, how about using hweight32 ? > > Why don't you read the comment above. Yes, you can add more code > and make it faster and it will not matter one bit, so I chose to go with > the shortest code possible. Sue me. I'm guilty of making the same comment, and it really isn't warranted in some cases where simplicity should win. I'm fine taking this piece as-is, I suppose, since it likely experiences a low bit-error rate. But I would caution that bit errors are becoming increasingly common, so your "trivial" comment may not age well. > >>+ flips += hweight8(chkoob[*eccpos] ^ rawoob[*eccpos]); > >>+ ++eccpos; > >>+ } > >>+ if (flips > 0) > >>+ mtd->ecc_stats.corrected += flips; > >>+ max_bitflips = max_t(int, max_bitflips, flips); > >>+ chkbuf += chip->ecc.size; > >>+ rawbuf += chip->ecc.size; > >>+ } Brian
(Re-constructing CC list and leaving message intact, since you missed the "Reply-All" button) On Tue, Apr 01, 2014 at 10:03:00AM -0600, David Mosberger wrote: > Brian, > > On Tue, Apr 1, 2014 at 1:50 AM, Brian Norris > <computersforpeace@gmail.com> wrote: > > On Mon, Mar 31, 2014 at 05:28:57PM -0600, David Mosberger wrote: > > >> +static int > >> +set_on_die_ecc(struct mtd_info *mtd, struct nand_chip *chip, int on) > >> +{ > >> + u8 data[ONFI_SUBFEATURE_PARAM_LEN] = { 0, }; > >> + > >> + if (chip->ecc.mode != NAND_ECC_HW_ON_DIE) > >> + return 0; > > > > I think this check is unnecessary, and probably wrong. The caller should > > make sure not to call this for devices that don't support it. Or else, > > there should at least be an error code, like -EOPNOTSUPP. > > Fair enough. I removed the check for ecc.mode. > > >> + > >> + if (on) > >> + data[0] = ONFI_FEATURE_ARRAY_OP_MODE_ENABLE_ON_DIE_ECC; > >> + > >> + return chip->onfi_set_features(mtd, chip, > >> + ONFI_FEATURE_ADDR_ARRAY_OP_MODE, data); > >> +} > > > > This should be implemented on a per-vendor basis and provided as a > > callback (perhaps chip->set_internal_ecc()?). Then, you would only make > > chip->set_internal_ecc non-NULL for flash that support it. > > It's not clear at all to me how (un-)standardized this stuff is. It > may be Micron specific, > but it may not be. I don't know. Since it's only called for Micron > chips with on-die enabled, > the code is safe as it is. > > > Do you actually need to re-read, or can you use the existing data? Or at > > least, you could overwrite the databuf, instead of using a new chkbuf. > > In general, you have to (re-)read. Consider read_oob or read_subpage. > > >> + > >> + /* Re-read page with on-die ECC off: */ > >> + set_on_die_ecc(mtd, chip, 0); > >> + chip->cmdfunc(mtd, NAND_CMD_READ0, 0x00, page); > >> + chip->read_buf(mtd, rawbuf, read_size); > >> + set_on_die_ecc(mtd, chip, 1); > >> + > >> + chkoob = chkbuf + mtd->writesize; > >> + rawoob = rawbuf + mtd->writesize; > >> + eccpos = chip->ecc.layout->eccpos; > >> + for (i = 0; i < chip->ecc.steps; ++i) { > >> + /* Count bit flips in the actual data area: */ > >> + flips = bitdiff(chkbuf, rawbuf, chip->ecc.size); > >> + /* Count bit flips in the ECC bytes: */ > >> + for (j = 0; j < chip->ecc.bytes; ++j) { > >> + flips += hweight8(chkoob[*eccpos] ^ rawoob[*eccpos]); > > > > Why didn't you use bitdiff() here too? > > Because the data is not contiguous and I didn't think the overhead > of an extra function call was warranted for individual bytes. But yeah, > we could certainly use bitdiff() here on individual bytes, if you prefer. > > >> /* > >> - * Simple but suboptimal: any page with a single stuck > >> - * bit will be unusable since it'll be rewritten on > >> - * each read... > >> + * The Micron chips turn on the REWRITE status bit for > >> + * ANY bit flips. Some pages have stuck bits, so we > >> + * don't want to migrate a block just because of > >> + * single bit errors because otherwise, that block > >> + * would effectively become unusable. So, work out in > >> + * software what the max number of flipped bits is for > >> + * all subpages in a page: > > > > Can you shorten this comment? It's rather verbose, and it's making > > assumptions about upper-layer "migrations". I think we can leave it at > > something much simpler, like: > > > > /* > > * Micron on-die ECC doesn't report the number of bitflips, so > > * we have to count them ourself to see if the error rate is too > > * high. > > */ > > Sure, I did add "This is particularly important for pages with stuck > bits." since > I think that is an important case to think about here. > > --david > -- > eGauge Systems LLC, http://egauge.net/, 1.877-EGAUGE1, fax 720.545.9768
On Tue, Apr 01, 2014 at 10:33:05AM -0700, Brian Norris wrote: > (Re-constructing CC list and leaving message intact, since you missed > the "Reply-All" button) > > On Tue, Apr 01, 2014 at 10:03:00AM -0600, David Mosberger wrote: > > On Tue, Apr 1, 2014 at 1:50 AM, Brian Norris <computersforpeace@gmail.com> wrote: > > > On Mon, Mar 31, 2014 at 05:28:57PM -0600, David Mosberger wrote: > > >> + > > >> + if (on) > > >> + data[0] = ONFI_FEATURE_ARRAY_OP_MODE_ENABLE_ON_DIE_ECC; > > >> + > > >> + return chip->onfi_set_features(mtd, chip, > > >> + ONFI_FEATURE_ADDR_ARRAY_OP_MODE, data); > > >> +} > > > > > > This should be implemented on a per-vendor basis and provided as a > > > callback (perhaps chip->set_internal_ecc()?). Then, you would only make > > > chip->set_internal_ecc non-NULL for flash that support it. > > > > It's not clear at all to me how (un-)standardized this stuff is. It > > may be Micron specific, > > but it may not be. I don't know. Since it's only called for Micron > > chips with on-die enabled, > > the code is safe as it is. The point is that we don't write code into the generic framework that assumes Micron is the only one to implement it, if possible. This type of replaceable feature is best left as a callback which can be set to NULL, I think. Or if you can find a better point at which the implementation specifics can be abstracted, that can work as well. But regardless, my high level comment must be addressed -- you wrote this code as if Micron is the only one to implement on-die ECC. FWIW, the Toshiba BENAND (Built-in ECC NAND) datasheet I saw doesn't advertise the ability to disable its ECC, but it does report per-sector bitflip information (nice!). Also, I think it hides the ECC syndrome bytes from the user, so most drivers could possibly ignore the built-in ECC entirely if desired. > > > Do you actually need to re-read, or can you use the existing data? Or at > > > least, you could overwrite the databuf, instead of using a new chkbuf. > > > > In general, you have to (re-)read. Consider read_oob or read_subpage. > > > > >> + > > >> + /* Re-read page with on-die ECC off: */ > > >> + set_on_die_ecc(mtd, chip, 0); > > >> + chip->cmdfunc(mtd, NAND_CMD_READ0, 0x00, page); > > >> + chip->read_buf(mtd, rawbuf, read_size); > > >> + set_on_die_ecc(mtd, chip, 1); > > >> + > > >> + chkoob = chkbuf + mtd->writesize; > > >> + rawoob = rawbuf + mtd->writesize; > > >> + eccpos = chip->ecc.layout->eccpos; > > >> + for (i = 0; i < chip->ecc.steps; ++i) { > > >> + /* Count bit flips in the actual data area: */ > > >> + flips = bitdiff(chkbuf, rawbuf, chip->ecc.size); > > >> + /* Count bit flips in the ECC bytes: */ > > >> + for (j = 0; j < chip->ecc.bytes; ++j) { > > >> + flips += hweight8(chkoob[*eccpos] ^ rawoob[*eccpos]); > > > > > > Why didn't you use bitdiff() here too? > > > > Because the data is not contiguous and I didn't think the overhead > > of an extra function call was warranted for individual bytes. But yeah, > > we could certainly use bitdiff() here on individual bytes, if you prefer. Sorry, I misread the loop. Never mind. (Although it does then suggest that maybe the bitdiff() function doesn't really need to stand alone, for symmetry. Your call.) > > >> /* > > >> - * Simple but suboptimal: any page with a single stuck > > >> - * bit will be unusable since it'll be rewritten on > > >> - * each read... > > >> + * The Micron chips turn on the REWRITE status bit for > > >> + * ANY bit flips. Some pages have stuck bits, so we > > >> + * don't want to migrate a block just because of > > >> + * single bit errors because otherwise, that block > > >> + * would effectively become unusable. So, work out in > > >> + * software what the max number of flipped bits is for > > >> + * all subpages in a page: > > > > > > Can you shorten this comment? It's rather verbose, and it's making > > > assumptions about upper-layer "migrations". I think we can leave it at > > > something much simpler, like: > > > > > > /* > > > * Micron on-die ECC doesn't report the number of bitflips, so > > > * we have to count them ourself to see if the error rate is too > > > * high. > > > */ > > > > Sure, I did add "This is particularly important for pages with stuck > > bits." since > > I think that is an important case to think about here. Use your judgment, but please at least kill the migration comment. Brian
Brian, On Tue, Apr 1, 2014 at 12:01 PM, Brian Norris <computersforpeace@gmail.com> wrote: > The point is that we don't write code into the generic framework that > assumes Micron is the only one to implement it, if possible. This type > of replaceable feature is best left as a callback which can be set to > NULL, I think. Or if you can find a better point at which the > implementation specifics can be abstracted, that can work as well. But > regardless, my high level comment must be addressed -- you wrote this > code as if Micron is the only one to implement on-die ECC. > > FWIW, the Toshiba BENAND (Built-in ECC NAND) datasheet I saw doesn't > advertise the ability to disable its ECC, but it does report per-sector > bitflip information (nice!). Also, I think it hides the ECC syndrome > bytes from the user, so most drivers could possibly ignore the built-in > ECC entirely if desired. I can only make the patch as good as I know how to. If you need a perfect patch before doing *anything*, chances are the situation won't improve at all. I really don't understand when Linux kernel development stopped accepting improvements just because they're not perfect. Incremental improvements are part and parcel of Linux development, at least that's what I always thought. The patch as it stands is safe and it improves on the status quo. If that's not good enough, fine, I tried to do my part. Really, there is no direct benefit for me to spend time getting this merged. I'm doing it because I thought it's the right thing to do to contribute back a little bit since my company by relying on Linux is really is standing on the shoulders of giants. I'm sorry if I have sent too many patch revisions in too short amount of time. I tried to incorporate feedback and avoid duplicate comments by providing updated patches. Also, I don't have much time available, so the longer this process takes, the less likely I'll be able to finish it (please don't take that as a threat, it's really just my reality). --david
Hi David, >From: dmosberger@gmail.com >Sent: Tuesday, April 01, 2014 11:44 PM >>On Tue, Apr 1, 2014 at 12:01 PM, Brian Norris ><computersforpeace@gmail.com> wrote: >> The point is that we don't write code into the generic framework that >> assumes Micron is the only one to implement it, if possible. This type >> of replaceable feature is best left as a callback which can be set to >> NULL, I think. Or if you can find a better point at which the >> implementation specifics can be abstracted, that can work as well. But >> regardless, my high level comment must be addressed -- you wrote this >> code as if Micron is the only one to implement on-die ECC. >> >> FWIW, the Toshiba BENAND (Built-in ECC NAND) datasheet I saw doesn't >> advertise the ability to disable its ECC, but it does report per-sector >> bitflip information (nice!). Also, I think it hides the ECC syndrome >> bytes from the user, so most drivers could possibly ignore the built-in >> ECC entirely if desired. > >I can only make the patch as good as I know how to. >If you need a perfect patch before doing *anything*, chances are >the situation won't improve at all. > >I really don't understand when Linux kernel development stopped >accepting improvements just because they're not perfect. Incremental >improvements are part and parcel of Linux development, at least that's >what I always thought. > >The patch as it stands is safe and it improves on the status quo. >If that's not good enough, fine, I tried to do my part. > >Really, there is no direct benefit for me to spend time getting this merged. >I'm doing it because I thought it's the right thing to do to contribute back >a little bit since my company by relying on Linux is really is standing on >the shoulders of giants. > Some ground realities .. Most developers (including me) are working for companies, and so our implicit priorities become to maintain controller drivers first. Like you we also get limited bandwidth to work on generic frameworks (like nand_base.c) and review patches. So the actual attention given to generic or core framework components is quite less. (1) Thus usually whatever gets accepted first time in generic-framework remains as-it-is unless there is a bug reported or new feature addition. (2) Changing a generic framework also means regress testing on multiple Platform, as you might accidently break some old platform or driver which is already in production. So it's always better to get it correct first time. So, Brian's and my feedbacks were constructive to get things right at the first introduction of this feature. Once it's accepted this might remain untouched even it is sub-optimal. And Maintainer can always take a call when a patch is good enough for acceptance. However, there was never an intention to discourage you from submitting patches. >I'm sorry if I have sent too many patch revisions in too short amount >of time. I tried to incorporate feedback and avoid duplicate comments >by providing updated patches. Also, I don't have much time available, >so the longer this process takes, the less likely I'll be able to finish it >(please don't take that as a threat, it's really just my reality). > Having too many patch versions would un-necessary burden you only with re-testing and cleaning. Therefore I suggested to discuss and iron-out differences, so that there is no duplication of effort on you side. I'll try to help you out with some patches from your next version, as there are number of comments already to fix for this one. But I don't have Micron device with on-die ECC support, so you might have to help in testing. Hope that helps.. > --david with regards, pekon
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c index 834352a..80cfaa8 100644 --- a/drivers/mtd/nand/nand_base.c +++ b/drivers/mtd/nand/nand_base.c @@ -1272,6 +1272,84 @@ static int nand_read_subpage(struct mtd_info *mtd, struct nand_chip *chip, return max_bitflips; } +static int +set_on_die_ecc(struct mtd_info *mtd, struct nand_chip *chip, int on) +{ + u8 data[ONFI_SUBFEATURE_PARAM_LEN] = { 0, }; + + if (chip->ecc.mode != NAND_ECC_HW_ON_DIE) + return 0; + + if (on) + data[0] = ONFI_FEATURE_ARRAY_OP_MODE_ENABLE_ON_DIE_ECC; + + return chip->onfi_set_features(mtd, chip, + ONFI_FEATURE_ADDR_ARRAY_OP_MODE, data); +} + +/* + * Return the number of bits that differ between buffers SRC1 and + * SRC2, both of which are LEN bytes long. + * + * This code could be optimized for, but it only gets called on pages + * with bitflips and compared to the cost of migrating an eraseblock, + * the execution time here is trivial... + */ +static int +bitdiff(const void *s1, const void *s2, size_t len) +{ + const uint8_t *src1 = s1, *src2 = s2; + int count = 0, i; + + for (i = 0; i < len; ++i) + count += hweight8(*src1++ ^ *src2++); + return count; +} + +static int +check_for_bitflips(struct mtd_info *mtd, struct nand_chip *chip, int page) +{ + int flips = 0, max_bitflips = 0, i, j, read_size; + uint8_t *chkbuf, *rawbuf, *chkoob, *rawoob; + uint32_t *eccpos; + + chkbuf = chip->buffers->chkbuf; + rawbuf = chip->buffers->rawbuf; + read_size = mtd->writesize + mtd->oobsize; + + /* Read entire page w/OOB area with on-die ECC on: */ + chip->read_buf(mtd, chkbuf, read_size); + + /* Re-read page with on-die ECC off: */ + set_on_die_ecc(mtd, chip, 0); + chip->cmdfunc(mtd, NAND_CMD_READ0, 0x00, page); + chip->read_buf(mtd, rawbuf, read_size); + set_on_die_ecc(mtd, chip, 1); + + chkoob = chkbuf + mtd->writesize; + rawoob = rawbuf + mtd->writesize; + eccpos = chip->ecc.layout->eccpos; + for (i = 0; i < chip->ecc.steps; ++i) { + /* Count bit flips in the actual data area: */ + flips = bitdiff(chkbuf, rawbuf, chip->ecc.size); + /* Count bit flips in the ECC bytes: */ + for (j = 0; j < chip->ecc.bytes; ++j) { + flips += hweight8(chkoob[*eccpos] ^ rawoob[*eccpos]); + ++eccpos; + } + if (flips > 0) + mtd->ecc_stats.corrected += flips; + max_bitflips = max_t(int, max_bitflips, flips); + chkbuf += chip->ecc.size; + rawbuf += chip->ecc.size; + } + + /* Re-issue the READ command for the actual data read that follows. */ + chip->cmdfunc(mtd, NAND_CMD_READ0, 0x00, page); + + return max_bitflips; +} + static int check_read_status_on_die(struct mtd_info *mtd, struct nand_chip *chip, int page) { @@ -1290,11 +1368,15 @@ static int check_read_status_on_die(struct mtd_info *mtd, mtd->ecc_stats.failed++; } else if (status & NAND_STATUS_REWRITE) { /* - * Simple but suboptimal: any page with a single stuck - * bit will be unusable since it'll be rewritten on - * each read... + * The Micron chips turn on the REWRITE status bit for + * ANY bit flips. Some pages have stuck bits, so we + * don't want to migrate a block just because of + * single bit errors because otherwise, that block + * would effectively become unusable. So, work out in + * software what the max number of flipped bits is for + * all subpages in a page: */ - max_bitflips = mtd->bitflip_threshold; + max_bitflips = check_for_bitflips(mtd, chip, page); } return max_bitflips; }
When NAND_STATUS_REWRITE is set, there is no direct indication as to how many bits have were flipped. This patch improves the existing code by manually counting the number of bitflips, rather than just assuming the worst-case of mtd->bitflip_threshold. This avoids unnecessary rewrites, e.g., due to a single stuck bit. Signed-off-by: David Mosberger <davidm@egauge.net> --- drivers/mtd/nand/nand_base.c | 90 ++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 86 insertions(+), 4 deletions(-)