From mboxrd@z Thu Jan 1 00:00:00 1970 Date: Mon, 29 Aug 2016 10:10:06 +0200 From: Boris Brezillon To: mtk04561 Cc: , , , , , , , , , , Jorge Ramirez-Ortiz Subject: Re: [Patch] mtd: fix writing incorrect ECC parity data in OOB region. Message-ID: <20160829101006.2b200e0a@bbrezillon> In-Reply-To: <1472443093.27061.4.camel@mtkswgap22> References: <1472443093.27061.4.camel@mtkswgap22> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit List-Id: Linux MTD discussion mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , +Jorge Hi Roger, On Mon, 29 Aug 2016 11:58:13 +0800 mtk04561 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 > --- > 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; > >