| Message ID | 5659FF4A.7080203@simon.arlott.org.uk |
|---|---|
| State | Superseded |
| Headers | show |
Hi, On Sat, Nov 28, 2015 at 8:23 PM, Simon Arlott <simon@fire.lp0.eu> wrote: > Workaround false ECC uncorrectable errors by checking if the data > has been erased and the OOB data indicates that the data has been > erased. The v4.0 controller on the BCM63168 incorrectly handles > these as uncorrectable errors. > > I don't know which version of the controller handles this scenario > correctly. I'm only able to test this in Hamming mode, so the BCH > version needs to be checked. > > This code is quite complicated because the fact that either the data > buffer or the OOB data may not have been read from the controller, and > both of them are required to determine if the error is incorrect. > > Signed-off-by: Simon Arlott <simon@fire.lp0.eu> > --- > drivers/mtd/nand/brcmnand/brcmnand.c | 107 ++++++++++++++++++++++++++++++++++- > 1 file changed, 106 insertions(+), 1 deletion(-) > > diff --git a/drivers/mtd/nand/brcmnand/brcmnand.c b/drivers/mtd/nand/brcmnand/brcmnand.c > index 5f26b8a..0857af7 100644 > --- a/drivers/mtd/nand/brcmnand/brcmnand.c > +++ b/drivers/mtd/nand/brcmnand/brcmnand.c > @@ -1387,6 +1387,102 @@ static int brcmnand_dma_trans(struct brcmnand_host *host, u64 addr, u32 *buf, > } > > /* > + * Workaround false ECC uncorrectable errors by checking if the data > + * has been erased and the OOB data indicates that the data has been > + * erased. The controller incorrectly handles these as uncorrectable > + * errors. > + */ > +static void brcmnand_read_ecc_unc_err(struct mtd_info *mtd, > + struct nand_chip *chip, > + int idx, unsigned int trans, > + u32 *buf, u8 *oob_begin, u8 *oob_end, > + u64 *err_addr) > +{ > + struct brcmnand_host *host = chip->priv; > + struct brcmnand_controller *ctrl = host->ctrl; > + u32 *buf_tmp = NULL; > + u8 *oob_tmp = NULL; > + bool buf_erased = false; > + bool oob_erased = false; > + int j; > + > + /* Assume this is fixed in v5.0+ */ > + if (ctrl->nand_version >= 0x0500) > + return; > + > + /* Read OOB data if not already read */ > + if (!oob_begin) { > + oob_tmp = kmalloc(ctrl->max_oob, GFP_KERNEL); > + if (!oob_tmp) > + goto out_free; > + > + oob_begin = oob_tmp; > + oob_end = oob_tmp; > + > + oob_end += read_oob_from_regs(ctrl, idx, oob_tmp, > + mtd->oobsize / trans, > + host->hwcfg.sector_size_1k); > + } > + > + if (is_hamming_ecc(&host->hwcfg)) { > + u8 *oob_offset = oob_begin + 6; > + > + if (oob_offset + 3 < oob_end) > + /* Erased if ECC bytes are all 0xFF, or the data bytes > + * are all 0xFF which should have Hamming codes of 0x00 > + */ > + oob_erased = memchr_inv(oob_offset, 0xFF, 3) == NULL || > + memchr_inv(oob_offset, 0x00, 3) == NULL; > + } else { /* BCH */ > + u8 *oob_offset = oob_end - ctrl->max_oob; > + > + if (oob_offset >= oob_begin) > + /* Erased if ECC bytes are all 0xFF */ > + oob_erased = memchr_inv(oob_offset, 0xFF, > + oob_end - oob_offset) == NULL; > + } > + > + if (!oob_erased) > + goto out_free; > + > + /* Read data buffer if not already read */ > + if (!buf) { > + buf_tmp = kmalloc_array(FC_WORDS, sizeof(*buf_tmp), GFP_KERNEL); > + if (!buf_tmp) > + goto out_free; > + > + buf = buf_tmp; > + > + brcmnand_soc_data_bus_prepare(ctrl->soc); > + > + for (j = 0; j < FC_WORDS; j++, buf++) > + *buf = brcmnand_read_fc(ctrl, j); > + > + brcmnand_soc_data_bus_unprepare(ctrl->soc); > + } > + > + /* Go to start of buffer */ > + buf -= FC_WORDS; > + > + /* Erased if all data bytes are 0xFF */ > + buf_erased = memchr_inv(buf, 0xFF, FC_WORDS) == NULL; > + > + if (!buf_erased) > + goto out_free; We now have a function exactly for that use case in 4.4, nand_check_erased_buf [1], consider using that. This also has the benefit of treating bit flips as correctable as long as the ECC scheme is strong enough. Jonas [1] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/mtd/nand/nand_base.c#n1110
On 01/12/15 10:41, Jonas Gorski wrote: > On Sat, Nov 28, 2015 at 8:23 PM, Simon Arlott <simon@fire.lp0.eu> wrote: >> + >> + /* Go to start of buffer */ >> + buf -= FC_WORDS; >> + >> + /* Erased if all data bytes are 0xFF */ >> + buf_erased = memchr_inv(buf, 0xFF, FC_WORDS) == NULL; >> + >> + if (!buf_erased) >> + goto out_free; > > We now have a function exactly for that use case in 4.4, > nand_check_erased_buf [1], consider using that. This also has the > benefit of treating bit flips as correctable as long as the ECC scheme > is strong enough. I have no idea whether or not it's appropriate to specify bitflips_threshold > 0 so it'd just be a more complex way to do a memchr_inv() search for 0xFF. The code also has to check for the hamming code bytes being all 0x00, because according to the comments [2], the controller also has difficulty with the non-erased all-0xFFs scenario too. [2] https://github.com/lp0/bcm963xx_4.12L.06B_consumer/blob/dd8fcb13046f738c311507dc2fcfd3e5d57a88e0/kernel/linux/drivers/mtd/brcmnand/brcmnand_base.c#L2459 > > Jonas > > [1] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/mtd/nand/nand_base.c#n1110 >
On Wed, Dec 2, 2015 at 9:17 PM, Simon Arlott <simon@fire.lp0.eu> wrote: > On 01/12/15 10:41, Jonas Gorski wrote: >> On Sat, Nov 28, 2015 at 8:23 PM, Simon Arlott <simon@fire.lp0.eu> wrote: >>> + >>> + /* Go to start of buffer */ >>> + buf -= FC_WORDS; >>> + >>> + /* Erased if all data bytes are 0xFF */ >>> + buf_erased = memchr_inv(buf, 0xFF, FC_WORDS) == NULL; >>> + >>> + if (!buf_erased) >>> + goto out_free; >> >> We now have a function exactly for that use case in 4.4, >> nand_check_erased_buf [1], consider using that. This also has the >> benefit of treating bit flips as correctable as long as the ECC scheme >> is strong enough. > > I have no idea whether or not it's appropriate to specify > bitflips_threshold > 0 so it'd just be a more complex way to do > a memchr_inv() search for 0xFF. The threshold would be the amount of bitflips the code can correct, so basically ecc.strength (at least that is my understanding). > The code also has to check for the hamming code bytes being all 0x00, > because according to the comments [2], the controller also has > difficulty with the non-erased all-0xFFs scenario too. According to brcmnand.c hamming can fix up to fifteen bitflips, but in the current code you would fail a hamming protected all-0xff-page for even a single bitflip in the data or in the ecc bytes, which means that all-0xff-pages wouldn't be protected at all. Jonas
Hi, On Wed, Dec 02, 2015 at 09:44:04PM +0100, Jonas Gorski wrote: > On Wed, Dec 2, 2015 at 9:17 PM, Simon Arlott <simon@fire.lp0.eu> wrote: > > On 01/12/15 10:41, Jonas Gorski wrote: > >> On Sat, Nov 28, 2015 at 8:23 PM, Simon Arlott <simon@fire.lp0.eu> wrote: > >>> + > >>> + /* Go to start of buffer */ > >>> + buf -= FC_WORDS; > >>> + > >>> + /* Erased if all data bytes are 0xFF */ > >>> + buf_erased = memchr_inv(buf, 0xFF, FC_WORDS) == NULL; > >>> + > >>> + if (!buf_erased) > >>> + goto out_free; > >> > >> We now have a function exactly for that use case in 4.4, > >> nand_check_erased_buf [1], consider using that. This also has the > >> benefit of treating bit flips as correctable as long as the ECC scheme > >> is strong enough. > > > > I have no idea whether or not it's appropriate to specify > > bitflips_threshold > 0 so it'd just be a more complex way to do > > a memchr_inv() search for 0xFF. > > The threshold would be the amount of bitflips the code can correct, so > basically ecc.strength (at least that is my understanding). > > > The code also has to check for the hamming code bytes being all 0x00, > > because according to the comments [2], the controller also has > > difficulty with the non-erased all-0xFFs scenario too. > > According to brcmnand.c hamming can fix up to fifteen bitflips, but in Hamming only protects 1 bitflip. The '15' is the value used by the controller to represent Hamming (i.e., there is no BCH-15). > the current code you would fail a hamming protected all-0xff-page for > even a single bitflip in the data or in the ecc bytes, which means > that all-0xff-pages wouldn't be protected at all. BTW, I think Kamal had code to handle protecting bitflips in erased pages code in the Broadcom STB Linux BSP. Perhaps he can port that to upstream with nand_check_erased_ecc_chunk()? IIUC, that would probably handle your case too, Simon, although it wouldn't be optimal for an all-0xff check (i.e., bitflip_threshold == 0). If that's really an issue (i.e., we have an implementation + data), I'm sure we could add optimization to nand_check_erased_ecc_chunk() to support the bitflip_threshold == 0 case. Brian
On Wed, Dec 02, 2015 at 12:54:15PM -0800, Brian Norris wrote: > BTW, I think Kamal had code to handle protecting bitflips in erased > pages code in the Broadcom STB Linux BSP. Perhaps he can port that to > upstream with nand_check_erased_ecc_chunk()? IIUC, that would probably > handle your case too, Simon, although it wouldn't be optimal for an > all-0xff check (i.e., bitflip_threshold == 0). I think this was for BCH only, actually. Hamming wasn't used as much on STB. So the above would only help part of this patch. I'm less sure of the behavior for the Hamming engine. Brian
Hi, On Wed, Dec 2, 2015 at 9:54 PM, Brian Norris <computersforpeace@gmail.com> wrote: > Hi, > > On Wed, Dec 02, 2015 at 09:44:04PM +0100, Jonas Gorski wrote: >> On Wed, Dec 2, 2015 at 9:17 PM, Simon Arlott <simon@fire.lp0.eu> wrote: >> > On 01/12/15 10:41, Jonas Gorski wrote: >> >> On Sat, Nov 28, 2015 at 8:23 PM, Simon Arlott <simon@fire.lp0.eu> wrote: >> >>> + >> >>> + /* Go to start of buffer */ >> >>> + buf -= FC_WORDS; >> >>> + >> >>> + /* Erased if all data bytes are 0xFF */ >> >>> + buf_erased = memchr_inv(buf, 0xFF, FC_WORDS) == NULL; >> >>> + >> >>> + if (!buf_erased) >> >>> + goto out_free; >> >> >> >> We now have a function exactly for that use case in 4.4, >> >> nand_check_erased_buf [1], consider using that. This also has the >> >> benefit of treating bit flips as correctable as long as the ECC scheme >> >> is strong enough. >> > >> > I have no idea whether or not it's appropriate to specify >> > bitflips_threshold > 0 so it'd just be a more complex way to do >> > a memchr_inv() search for 0xFF. >> >> The threshold would be the amount of bitflips the code can correct, so >> basically ecc.strength (at least that is my understanding). >> >> > The code also has to check for the hamming code bytes being all 0x00, >> > because according to the comments [2], the controller also has >> > difficulty with the non-erased all-0xFFs scenario too. >> >> According to brcmnand.c hamming can fix up to fifteen bitflips, but in > > Hamming only protects 1 bitflip. The '15' is the value used by the > controller to represent Hamming (i.e., there is no BCH-15). Ah, yeah that confused me because I also vaguely remembered hamming only providing protection for 1, but then saw the ecc_level = 15 assignment. Still, that means that even hamming protected erased pages with a single bitflip should be treated as readable / all-0xff, but with correctable bitflips, and not as uncorrectable. >> the current code you would fail a hamming protected all-0xff-page for >> even a single bitflip in the data or in the ecc bytes, which means >> that all-0xff-pages wouldn't be protected at all. > > BTW, I think Kamal had code to handle protecting bitflips in erased > pages code in the Broadcom STB Linux BSP. Perhaps he can port that to > upstream with nand_check_erased_ecc_chunk()? IIUC, that would probably > handle your case too, Simon, although it wouldn't be optimal for an > all-0xff check (i.e., bitflip_threshold == 0). > > If that's really an issue (i.e., we have an implementation + data), I'm > sure we could add optimization to nand_check_erased_ecc_chunk() to > support the bitflip_threshold == 0 case. Maybe I'm missing something, but wasn't the point of introducing nand_check_erased_ecc_chunk that bitflips in erased pages should be treated as bitflips corrected by the ecc, and therefore fixed up before passing the data further on? So having a theshold of 0 would be wrong / no protection at all, and could be quite destructive on MLC nand, where bitflips in erased pages are rather common. Jonas
On 02/12/15 21:08, Jonas Gorski wrote: > On Wed, Dec 2, 2015 at 9:54 PM, Brian Norris > <computersforpeace@gmail.com> wrote: >> >> BTW, I think Kamal had code to handle protecting bitflips in erased >> pages code in the Broadcom STB Linux BSP. Perhaps he can port that to >> upstream with nand_check_erased_ecc_chunk()? IIUC, that would probably >> handle your case too, Simon, although it wouldn't be optimal for an >> all-0xff check (i.e., bitflip_threshold == 0). >> >> If that's really an issue (i.e., we have an implementation + data), I'm >> sure we could add optimization to nand_check_erased_ecc_chunk() to >> support the bitflip_threshold == 0 case. > > Maybe I'm missing something, but wasn't the point of introducing > nand_check_erased_ecc_chunk that bitflips in erased pages should be > treated as bitflips corrected by the ecc, and therefore fixed up > before passing the data further on? So having a theshold of 0 would be > wrong / no protection at all, and could be quite destructive on MLC > nand, where bitflips in erased pages are rather common. Without this patch I can't access erased pages at all. I don't know if the controller will still return an uncorrectable error if the page is erased but has 1 or more bit flips.
On Wed, Dec 02, 2015 at 10:08:20PM +0100, Jonas Gorski wrote: > On Wed, Dec 2, 2015 at 9:54 PM, Brian Norris > <computersforpeace@gmail.com> wrote: > > If that's really an issue (i.e., we have an implementation + data), I'm > > sure we could add optimization to nand_check_erased_ecc_chunk() to > > support the bitflip_threshold == 0 case. > > Maybe I'm missing something, but wasn't the point of introducing > nand_check_erased_ecc_chunk that bitflips in erased pages should be > treated as bitflips corrected by the ecc, and therefore fixed up > before passing the data further on? So having a theshold of 0 would be > wrong / no protection at all, and could be quite destructive on MLC > nand, where bitflips in erased pages are rather common. Yes, that would be pretty destructive on MLC NAND. But there might be cases where this is reasonable. For instance, we might have: (a) SLC NAND that specifies up to 1 bitflip per 512 bytes (b) a Hamming ECC engine that can correct 1 bitflip (c) said Hamming engine does not handle all-0xff pages correctly, nor does it handle near-0xff pages (e.g., with a single 0 bit) correctly Now, we might (for instance) allow up to ecc_strength / 2 bitflips in erased pages [1], as a general rule, but for the above case, that means we don't allow any on Hamming ECC. Hence, bitflip_threshold = 1 / 2 = 0. This is kind of a degenerate case, so we might consider supporting it differently than we would for BCH. e.g., don't use nand_check_erased_ecc_chunk() at all. Brian [1] We might do this because we can expect that if an unprogrammed page has N bitflips, there might be even more than N bitflips after programming the page.
diff --git a/drivers/mtd/nand/brcmnand/brcmnand.c b/drivers/mtd/nand/brcmnand/brcmnand.c index 5f26b8a..0857af7 100644 --- a/drivers/mtd/nand/brcmnand/brcmnand.c +++ b/drivers/mtd/nand/brcmnand/brcmnand.c @@ -1387,6 +1387,102 @@ static int brcmnand_dma_trans(struct brcmnand_host *host, u64 addr, u32 *buf, } /* + * Workaround false ECC uncorrectable errors by checking if the data + * has been erased and the OOB data indicates that the data has been + * erased. The controller incorrectly handles these as uncorrectable + * errors. + */ +static void brcmnand_read_ecc_unc_err(struct mtd_info *mtd, + struct nand_chip *chip, + int idx, unsigned int trans, + u32 *buf, u8 *oob_begin, u8 *oob_end, + u64 *err_addr) +{ + struct brcmnand_host *host = chip->priv; + struct brcmnand_controller *ctrl = host->ctrl; + u32 *buf_tmp = NULL; + u8 *oob_tmp = NULL; + bool buf_erased = false; + bool oob_erased = false; + int j; + + /* Assume this is fixed in v5.0+ */ + if (ctrl->nand_version >= 0x0500) + return; + + /* Read OOB data if not already read */ + if (!oob_begin) { + oob_tmp = kmalloc(ctrl->max_oob, GFP_KERNEL); + if (!oob_tmp) + goto out_free; + + oob_begin = oob_tmp; + oob_end = oob_tmp; + + oob_end += read_oob_from_regs(ctrl, idx, oob_tmp, + mtd->oobsize / trans, + host->hwcfg.sector_size_1k); + } + + if (is_hamming_ecc(&host->hwcfg)) { + u8 *oob_offset = oob_begin + 6; + + if (oob_offset + 3 < oob_end) + /* Erased if ECC bytes are all 0xFF, or the data bytes + * are all 0xFF which should have Hamming codes of 0x00 + */ + oob_erased = memchr_inv(oob_offset, 0xFF, 3) == NULL || + memchr_inv(oob_offset, 0x00, 3) == NULL; + } else { /* BCH */ + u8 *oob_offset = oob_end - ctrl->max_oob; + + if (oob_offset >= oob_begin) + /* Erased if ECC bytes are all 0xFF */ + oob_erased = memchr_inv(oob_offset, 0xFF, + oob_end - oob_offset) == NULL; + } + + if (!oob_erased) + goto out_free; + + /* Read data buffer if not already read */ + if (!buf) { + buf_tmp = kmalloc_array(FC_WORDS, sizeof(*buf_tmp), GFP_KERNEL); + if (!buf_tmp) + goto out_free; + + buf = buf_tmp; + + brcmnand_soc_data_bus_prepare(ctrl->soc); + + for (j = 0; j < FC_WORDS; j++, buf++) + *buf = brcmnand_read_fc(ctrl, j); + + brcmnand_soc_data_bus_unprepare(ctrl->soc); + } + + /* Go to start of buffer */ + buf -= FC_WORDS; + + /* Erased if all data bytes are 0xFF */ + buf_erased = memchr_inv(buf, 0xFF, FC_WORDS) == NULL; + + if (!buf_erased) + goto out_free; + + /* Clear error addresses */ + brcmnand_write_reg(ctrl, BRCMNAND_UNCORR_ADDR, 0); + brcmnand_write_reg(ctrl, BRCMNAND_CORR_ADDR, 0); + brcmnand_write_reg(ctrl, BRCMNAND_UNCORR_EXT_ADDR, 0); + brcmnand_write_reg(ctrl, BRCMNAND_CORR_EXT_ADDR, 0); + *err_addr = 0; + +out_free: + kfree(buf_tmp); + kfree(oob_tmp); +} + +/* * Assumes proper CS is already set */ static int brcmnand_read_by_pio(struct mtd_info *mtd, struct nand_chip *chip, @@ -1396,6 +1492,7 @@ static int brcmnand_read_by_pio(struct mtd_info *mtd, struct nand_chip *chip, struct brcmnand_host *host = chip->priv; struct brcmnand_controller *ctrl = host->ctrl; int i, j, ret = 0; + u8 *prev_oob = NULL; /* Clear error addresses */ brcmnand_write_reg(ctrl, BRCMNAND_UNCORR_ADDR, 0); @@ -1424,10 +1521,12 @@ static int brcmnand_read_by_pio(struct mtd_info *mtd, struct nand_chip *chip, brcmnand_soc_data_bus_unprepare(ctrl->soc); } - if (oob) + if (oob) { + prev_oob = oob; oob += read_oob_from_regs(ctrl, i, oob, mtd->oobsize / trans, host->hwcfg.sector_size_1k); + } if (!ret) { *err_addr = brcmnand_read_reg(ctrl, @@ -1435,6 +1534,12 @@ static int brcmnand_read_by_pio(struct mtd_info *mtd, struct nand_chip *chip, ((u64)(brcmnand_read_reg(ctrl, BRCMNAND_UNCORR_EXT_ADDR) & 0xffff) << 32); + + if (*err_addr) + brcmnand_read_ecc_unc_err(mtd, chip, + i, trans, buf, prev_oob, oob, + err_addr); + if (*err_addr) ret = -EBADMSG; }
Workaround false ECC uncorrectable errors by checking if the data has been erased and the OOB data indicates that the data has been erased. The v4.0 controller on the BCM63168 incorrectly handles these as uncorrectable errors. I don't know which version of the controller handles this scenario correctly. I'm only able to test this in Hamming mode, so the BCH version needs to be checked. This code is quite complicated because the fact that either the data buffer or the OOB data may not have been read from the controller, and both of them are required to determine if the error is incorrect. Signed-off-by: Simon Arlott <simon@fire.lp0.eu> --- drivers/mtd/nand/brcmnand/brcmnand.c | 107 ++++++++++++++++++++++++++++++++++- 1 file changed, 106 insertions(+), 1 deletion(-)