Re: [Patch] mtd: fix writing incorrect ECC parity data in OOB region.

[Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>]

+Jorge

Hi Roger,

On Mon, 29 Aug 2016 11:58:13 +0800
mtk04561 <rogercc.lin-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org> wrote:

> Hi Boris,
> 
> I found a problem in current Mediatek upstream NAND driver (of nand/next
> branch). When testing the upstream driver with UBIFS file system, it has
> some chance to create incorrect ECC data in the second subpage, this
> makes UBIFS failed to mount. Below patch fixes this problem. Please help
> to review the patch and upstream. Thank you.
> 
> Best regards,
> Roger

Can you put this message after the '---' (below your SoB), so that it
does not appear when applying the patch.

> 
> 
> 
> Description:

Drop the 'Description:' line.

> When mtk_ecc_encode() is writing the ECC parity data to the OOB region,
> because each register is 4 bytes in length, but the len's unit is in
> bytes, the operation in the for loop will cross the ECC's boundary.
> 
> And when mtk_nfc_do_write_page() comparing the sector number, because
> the sector number field is at the 12th-bit position of NFI_BYTELEN
> register, the masked register should be shifted 12 bits before being
> compared. The result of this bug may cause the second subpage has
> incomplete ECC parity bytes.

Maybe you could split those changes in 2 patches (not a strong
requirement).

> 
> Test:

Drop the 'Test:' line.

> The patch passed the test of UBIFS file-system read/write on Mediatek's
> RFB. The tested driver is checked-out from LEDE OpenWRT project's
> upstream driver, which is pretty much same as nand/next branch upstream
> driver(git clone https://git.lede-project.org/source.git). 
> 
> 
> Signed-off-by: RogerCC Lin <rogercc.lin-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> ---
> diff --git a/drivers/mtd/nand/mtk_ecc.c b/drivers/mtd/nand/mtk_ecc.c
> index 25a4fbd..0a7ce8d 100644
> --- a/drivers/mtd/nand/mtk_ecc.c
> +++ b/drivers/mtd/nand/mtk_ecc.c
> @@ -366,7 +366,8 @@ int mtk_ecc_encode(struct mtk_ecc *ecc, struct
> mtk_ecc_config *config,
>                    u8 *data, u32 bytes)
>  {
>         dma_addr_t addr;
> -       u32 *p, len, i;
> +       u8 *p;
> +       u32 len, i, val;
>         int ret = 0;
> 
>         addr = dma_map_single(ecc->dev, data, bytes, DMA_TO_DEVICE);
> @@ -395,8 +396,11 @@ int mtk_ecc_encode(struct mtk_ecc *ecc, struct
> mtk_ecc_config *config,
>         p = (u32 *)(data + bytes);

Hm, you should now do:

	p = data + bytes;

> 
>         /* write the parity bytes generated by the ECC back to the OOB
> region */
> -       for (i = 0; i < len; i++)
> -               p[i] = readl(ecc->regs + ECC_ENCPAR(i));
> +       for (i = 0; i < len; i++) {
> +               if ((i % 4) == 0)
> +                       val = readl(ecc->regs + ECC_ENCPAR(i >> 2));
> +               p[i] = *((u8 *)&val + (i % 4));

Please, use a shift operator here:

		p[i] = (val >> ((i % 4) * 8)) & 0xff;

Otherwise you're exposed to endianness conversion problems (this works
fine in your case because you're using a little-endian kernel).

> +       }
>  timeout:
> 
>         dma_unmap_single(ecc->dev, addr, bytes, DMA_TO_DEVICE);
> diff --git a/drivers/mtd/nand/mtk_nand.c b/drivers/mtd/nand/mtk_nand.c
> index ddaa2ac..9c49121 100644
> --- a/drivers/mtd/nand/mtk_nand.c
> +++ b/drivers/mtd/nand/mtk_nand.c
> @@ -699,8 +699,8 @@ static int mtk_nfc_do_write_page(struct mtd_info
> *mtd, struct nand_chip *chip,
>         }
> 
>         ret = readl_poll_timeout_atomic(nfc->regs + NFI_ADDRCNTR, reg,
> -                                       (reg & CNTR_MASK) >=
> chip->ecc.steps,
> -                                       10, MTK_TIMEOUT);
> +                                       ((reg & CNTR_MASK) >> 12) >=
> +                                       chip->ecc.steps, 10,

Please keep '((reg & CNTR_MASK) >> 12) >= chip->ecc.steps' on the same
line.

If you want to comply with the 80 char lines, you can do:

	ret = readl_poll_timeout_atomic(nfc->regs + NFI_ADDRCNTR, reg,
				((reg & CNTR_MASK) >> 12) >= chip->ecc.steps,
				10, MTK_TIMEOUT);


>         if (ret)
>                 dev_err(dev, "hwecc write timeout\n");
> 
> @@ -902,8 +902,8 @@ static int mtk_nfc_read_subpage(struct mtd_info
> *mtd, struct nand_chip *chip,
>                 dev_warn(nfc->dev, "read ahb/dma done timeout\n");
> 
>         rc = readl_poll_timeout_atomic(nfc->regs + NFI_BYTELEN, reg,
> -                                      (reg & CNTR_MASK) >= sectors, 10,
> -                                      MTK_TIMEOUT);
> +                                      ((reg & CNTR_MASK) >> 12) >=
> sectors,
> +                                      10, MTK_TIMEOUT);
>         if (rc < 0) {
>                 dev_err(nfc->dev, "subpage done timeout\n");
>                 bitflips = -EIO;
> 
>