mtd: brcmnand: Workaround false ECC uncorrectable errors
Subsystems (2)
PATCH 1/1 mtd: brcmnand: Workaround false ECC uncorrectable errors
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.
--- drivers/mtd/nand/brcmnand/brcmnand.c | 107 ++++++++++++++++++++++++++++++++++- 1 file changed, 106 insertions(+), 1 deletion(-)
drivers/mtd/nand/brcmnand/brcmnand.c +106 -1
@@ -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 ; }
Signature · 4 lines
2.1.4 -- Simon Arlott
Hi,
Quoted 98 lines · Simon Arlott
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 <[email protected]>--- drivers/mtd/nand/brcmnand/brcmnand.c | 107 ++++++++++++++++++++++++++++++++++- 1 file changed, 106 insertions(+), 1 deletion(-)drivers/mtd/nand/brcmnand/brcmnand.c +72
-0@@ -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);
drivers/mtd/nand/brcmnand/brcmnand.c static int brcmnand_dma_trans(struct brcmnand_host *host, u64 addr, u32 *buf, · line 1471
Quoted 10 lines
drivers/mtd/nand/brcmnand/brcmnand.c +10
-0+ } + +/* 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
Quoted 15 lines · Jonas Gorski
drivers/mtd/nand/brcmnand/brcmnand.c static int brcmnand_dma_trans(struct brcmnand_host *host, u64 addr, u32 *buf, · line 1471
Quoted 9 lines · Simon Arlott
drivers/mtd/nand/brcmnand/brcmnand.c +9
-0+ +/* 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.
Quoted 5 lines
Jonas
Signature · 1 line
Simon Arlott
Quoted 17 lines · Simon Arlott
Quoted 15 lines · Jonas Gorski
drivers/mtd/nand/brcmnand/brcmnand.c static int brcmnand_dma_trans(struct brcmnand_host *host, u64 addr, u32 *buf, · line 1471
Quoted 9 lines · Simon Arlott
drivers/mtd/nand/brcmnand/brcmnand.c +9
-0+ +/* 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.
Quoted 3 lines
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 amemchr_inv()search for0xFF.
The threshold would be the amount of bitflips the code can correct, so basically ecc.strength (at least that is my understanding).
Quoted 3 lines
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,
Quoted 22 lines · Jonas Gorski
Quoted 17 lines · Simon Arlott
Quoted 15 lines · Jonas Gorski
drivers/mtd/nand/brcmnand/brcmnand.c static int brcmnand_dma_trans(struct brcmnand_host *host, u64 addr, u32 *buf, · line 1471
Quoted 9 lines · Simon Arlott
drivers/mtd/nand/brcmnand/brcmnand.c +9
-0+ +/* 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.Quoted 3 lines
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 amemchr_inv()search for0xFF.
Quoted 8 lines
The threshold would be the amount of bitflips the code can correct, so basically ecc.strength (at least that is my understanding).
Quoted 3 lines
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.chamming 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).
Quoted 3 lines
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
Quoted 5 lines · Brian Norris
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,
Quoted 28 lines · Brian Norris
Hi,
Quoted 25 lines · Jonas Gorski
Quoted 17 lines · Simon Arlott
Quoted 15 lines · Jonas Gorski
drivers/mtd/nand/brcmnand/brcmnand.c static int brcmnand_dma_trans(struct brcmnand_host *host, u64 addr, u32 *buf, · line 1471
Quoted 9 lines · Simon Arlott
drivers/mtd/nand/brcmnand/brcmnand.c +9
-0+ +/* 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.Quoted 3 lines
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 amemchr_inv()search for0xFF.The threshold would be the amount of bitflips the code can correct, so basically ecc.strength (at least that is my understanding).
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
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.chamming can fix up to fifteen bitflips, but inHamming 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.
Quoted 10 lines
Quoted 3 lines
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).
Quoted 3 lines
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 thebitflip_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
Quoted 13 lines · Jonas Gorski
Quoted 10 lines · Brian Norris
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 thebitflip_threshold== 0 case.
Quoted 6 lines
Maybe I'm missing something, but wasn't the point of introducing
nand_check_erased_ecc_chunkthat 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.
Signature · 1 line
Simon Arlott
Quoted 6 lines · Jonas Gorski
Quoted 3 lines · Brian Norris
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 thebitflip_threshold== 0 case.
Quoted 6 lines
Maybe I'm missing something, but wasn't the point of introducing
nand_check_erased_ecc_chunkthat 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.