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

* [Patch] mtd: fix writing incorrect ECC parity data in OOB region.
@ 2016-08-29  3:58 ` mtk04561
  0 siblings, 0 replies; 14+ messages in thread
From: mtk04561 @ 2016-08-29  3:58 UTC (permalink / raw)
  To: boris.brezillon
  Cc: xiaolei.li, steven.liu, robh, computersforpeace, daniel.thompson,
	linux-mtd, matthias.bgg, linux-mediatek, dwmw2, blogic

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



Description:
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.

Test:
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@mediatek.com>
---

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);

        /* 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));
+       }
 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,
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;

^ permalink raw reply related	[flat|nested] 14+ messages in thread