* [Patch] mtd: fix writing incorrect ECC parity data in OOB region. @ 2016-08-29 3:58 mtk04561 2016-08-29 8:10 ` Boris Brezillon 0 siblings, 1 reply; 7+ 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] 7+ messages in thread
* Re: [Patch] mtd: fix writing incorrect ECC parity data in OOB region. 2016-08-29 3:58 [Patch] mtd: fix writing incorrect ECC parity data in OOB region mtk04561 @ 2016-08-29 8:10 ` Boris Brezillon 2016-08-29 8:56 ` Jorge Ramirez ` (3 more replies) 0 siblings, 4 replies; 7+ messages in thread From: Boris Brezillon @ 2016-08-29 8:10 UTC (permalink / raw) To: mtk04561 Cc: xiaolei.li, steven.liu, robh, computersforpeace, daniel.thompson, linux-mtd, matthias.bgg, linux-mediatek, dwmw2, blogic, Jorge Ramirez-Ortiz +Jorge Hi Roger, On Mon, 29 Aug 2016 11:58:13 +0800 mtk04561 <rogercc.lin@mediatek.com> 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@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); 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; > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Patch] mtd: fix writing incorrect ECC parity data in OOB region. 2016-08-29 8:10 ` Boris Brezillon @ 2016-08-29 8:56 ` Jorge Ramirez 2016-08-29 11:40 ` [PATCH v2 0/2] mtd: nand: " RogerCC.Lin ` (2 subsequent siblings) 3 siblings, 0 replies; 7+ messages in thread From: Jorge Ramirez @ 2016-08-29 8:56 UTC (permalink / raw) To: Boris Brezillon, mtk04561 Cc: xiaolei.li, steven.liu, robh, computersforpeace, daniel.thompson, linux-mtd, matthias.bgg, linux-mediatek, dwmw2, blogic On 08/29/2016 10:10 AM, Boris Brezillon wrote: > +Jorge Thanks Boris. Xiaolei is looking into this - he is aware of the patch. ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 0/2] mtd: nand: fix writing incorrect ECC parity data in OOB region. 2016-08-29 8:10 ` Boris Brezillon 2016-08-29 8:56 ` Jorge Ramirez @ 2016-08-29 11:40 ` 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 3 siblings, 1 reply; 7+ messages in thread From: RogerCC.Lin @ 2016-08-29 11:40 UTC (permalink / raw) To: Boris Brezillon Cc: xiaolei.li, steven.liu, robh, computersforpeace, daniel.thompson, linux-mtd, matthias.bgg, linux-mediatek, dwmw2, blogic, Jorge Ramirez-Ortiz This series fix chances to write incorrect ECC data which may cause uncorrectable ECC error when reading. changes since v1: - separate patches into 2. - use shift operator with byte access to avoid endianness conversion problems. - follow linux coding style. 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). RogerCC Lin (2): mtd: nand: fix generating over-boundary ECC data when writing. mtd: nand: fix chances to create incomplete ECC data when writing. drivers/mtd/nand/mtk_ecc.c | 12 ++++++++---- drivers/mtd/nand/mtk_nand.c | 8 ++++---- 2 files changed, 12 insertions(+), 8 deletions(-) ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 0/2] mtd: nand: fix writing incorrect ECC parity data in OOB region. 2016-08-29 11:40 ` [PATCH v2 0/2] mtd: nand: " RogerCC.Lin @ 2016-08-29 11:53 ` RogerCC.Lin 0 siblings, 0 replies; 7+ messages in thread From: RogerCC.Lin @ 2016-08-29 11:53 UTC (permalink / raw) To: Boris Brezillon Cc: xiaolei.li, steven.liu, robh, computersforpeace, daniel.thompson, linux-mtd, matthias.bgg, linux-mediatek, dwmw2, blogic, Jorge Ramirez-Ortiz Hi Boris and all, I am sorry that though I have change the sender name in the email, but It seems the patchwork still show the wrong name, I will check what wrong with my email, sorry for the inconvenience. Roger ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 1/2] mtd: nand: fix generating over-boundary ECC data when writing. 2016-08-29 8:10 ` 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:40 ` RogerCC.Lin 2016-08-29 11:40 ` [PATCH v2 2/2] mtd: nand: fix chances to create incomplete " RogerCC.Lin 3 siblings, 0 replies; 7+ messages in thread From: RogerCC.Lin @ 2016-08-29 11:40 UTC (permalink / raw) To: Boris Brezillon Cc: xiaolei.li, steven.liu, robh, computersforpeace, daniel.thompson, linux-mtd, matthias.bgg, linux-mediatek, dwmw2, blogic, Jorge Ramirez-Ortiz 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. Signed-off-by: RogerCC Lin <rogercc.lin@mediatek.com> --- drivers/mtd/nand/mtk_ecc.c | 12 ++++++++---- 1 files changed, 8 insertions(+), 4 deletions(-) diff --git a/drivers/mtd/nand/mtk_ecc.c b/drivers/mtd/nand/mtk_ecc.c index 25a4fbd..495538e 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); @@ -392,11 +393,14 @@ int mtk_ecc_encode(struct mtk_ecc *ecc, struct mtk_ecc_config *config, /* Program ECC bytes to OOB: per sector oob = FDM + ECC + SPARE */ len = (config->strength * ECC_PARITY_BITS + 7) >> 3; - p = (u32 *)(data + bytes); + 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] = (val >> ((i % 4) * 8)) & 0xff; + } timeout: dma_unmap_single(ecc->dev, addr, bytes, DMA_TO_DEVICE); -- 1.7.0.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v2 2/2] mtd: nand: fix chances to create incomplete ECC data when writing. 2016-08-29 8:10 ` Boris Brezillon ` (2 preceding siblings ...) 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 ` RogerCC.Lin 3 siblings, 0 replies; 7+ messages in thread From: RogerCC.Lin @ 2016-08-29 11:40 UTC (permalink / raw) To: Boris Brezillon Cc: xiaolei.li, steven.liu, robh, computersforpeace, daniel.thompson, linux-mtd, matthias.bgg, linux-mediatek, dwmw2, blogic, Jorge Ramirez-Ortiz 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. Signed-off-by: RogerCC Lin <rogercc.lin@mediatek.com> --- drivers/mtd/nand/mtk_nand.c | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/mtd/nand/mtk_nand.c b/drivers/mtd/nand/mtk_nand.c index ddaa2ac..5e0727c 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; -- 1.7.0.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-08-29 11:53 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-08-29 3:58 [Patch] mtd: fix writing incorrect ECC parity data in OOB region mtk04561 2016-08-29 8:10 ` 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
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).