[PATCH] mtd: brcmnand: Workaround false ECC uncorrectable errors

[PATCH] mtd: brcmnand: Workaround false ECC uncorrectable errors

[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 <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;
+
+	/* 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;
 		}
-- 
2.1.4

-- 
Simon Arlott

Re: [PATCH] mtd: brcmnand: Workaround false ECC uncorrectable errors

[Jonas Gorski]

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

Re: [PATCH] mtd: brcmnand: Workaround false ECC uncorrectable errors

[Simon Arlott]

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
> 

-- 
Simon Arlott

Re: [PATCH] mtd: brcmnand: Workaround false ECC uncorrectable errors

[Jonas Gorski]

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

Re: [PATCH] mtd: brcmnand: Workaround false ECC uncorrectable errors

[Brian Norris]

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

Re: [PATCH] mtd: brcmnand: Workaround false ECC uncorrectable errors

[Brian Norris]

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

Re: [PATCH] mtd: brcmnand: Workaround false ECC uncorrectable errors

[Jonas Gorski]

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

Re: [PATCH] mtd: brcmnand: Workaround false ECC uncorrectable errors

[Simon Arlott]

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.

-- 
Simon Arlott

Re: [PATCH] mtd: brcmnand: Workaround false ECC uncorrectable errors

[Brian Norris]

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.