linux-mtd.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
From: mtk04561 <rogercc.lin@mediatek.com>
To: <boris.brezillon@free-electrons.com>
Cc: <xiaolei.li@mediatek.com>, <steven.liu@mediatek.com>,
	<robh@kernel.org>, <computersforpeace@gmail.com>,
	<daniel.thompson@linaro.org>, <linux-mtd@lists.infradead.org>,
	<matthias.bgg@gmail.com>, <linux-mediatek@lists.infradead.org>,
	<dwmw2@infradead.org>, <blogic@openwrt.org>
Subject: [Patch] mtd: fix writing incorrect ECC parity data in OOB region.
Date: Mon, 29 Aug 2016 11:58:13 +0800	[thread overview]
Message-ID: <1472443093.27061.4.camel@mtkswgap22> (raw)

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;

             reply	other threads:[~2016-08-29  3:58 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-08-29  3:58 mtk04561 [this message]
2016-08-29  8:10 ` [Patch] mtd: fix writing incorrect ECC parity data in OOB region Boris Brezillon
2016-08-29  8:56   ` Jorge Ramirez
2016-08-29 11:40   ` [PATCH v2 0/2] mtd: nand: " RogerCC.Lin
2016-08-29 11:53     ` RogerCC.Lin
2016-08-29 11:40   ` [PATCH v2 1/2] mtd: nand: fix generating over-boundary ECC data when writing RogerCC.Lin
2016-08-29 11:40   ` [PATCH v2 2/2] mtd: nand: fix chances to create incomplete " RogerCC.Lin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1472443093.27061.4.camel@mtkswgap22 \
    --to=rogercc.lin@mediatek.com \
    --cc=blogic@openwrt.org \
    --cc=boris.brezillon@free-electrons.com \
    --cc=computersforpeace@gmail.com \
    --cc=daniel.thompson@linaro.org \
    --cc=dwmw2@infradead.org \
    --cc=linux-mediatek@lists.infradead.org \
    --cc=linux-mtd@lists.infradead.org \
    --cc=matthias.bgg@gmail.com \
    --cc=robh@kernel.org \
    --cc=steven.liu@mediatek.com \
    --cc=xiaolei.li@mediatek.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).