| Message ID | dfb76ad5-b62a-3f1d-494e-cd17d57945ae@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fixes for Rockchip NAND controller driver | expand |
Hi Johan, Would you mind reducing the subjets with s/rockchip-nand-controller/rockchip/? jbx6244@gmail.com wrote on Thu, 15 Jun 2023 19:34:01 +0200: > The MTD framework reserves 1 or 2 bytes for the bad block marker > depending on the bus size. The rockchip-nand-controller driver > currently only supports a 8 bit bus, but reserves standard 2 bytes > for the BBM in the chip->oob_poi buffer. The first free OOB byte is > therefore OOB2 at offset 2. Again, I don't think it's ever been a single byte. Please drop this paragraph, it does not justify anything below. What is important here is the PA thing I guess. > Page Address (PA) bytes are located at the Maybe you can briefly explain what the PA bytes are above. > last 4 positions before ECC. The current advertised free OOB area has > an offset that starts at OOB6 and a length that overlaps with the space Let me try to rephrase this: " The currently advertised free OOB area starts at offset 6, like if 4 PA bytes were located right after the BBM. This is wrong as the PA bytes are located right before the ECC bytes. Fix the layout by allowing access to all bytes between the BBM and the PA bytes instead of reserving 4 bytes right after the BBM. > reserved for the PA bytes. Writing unrelated data to a reserved space > with a specific task can corrupt our boot block page read order. "our"? > Fix by changing the free OOB offset to 2. > > This change breaks existing jffs2 users. > > Signed-off-by: Johan Jonker <jbx6244@gmail.com> Please add a Fixes tag, now that I look at it again, it looks like the original author did try to protect these PA bytes, but failed. > --- > > Changed V3: > Change prefixes > Reword > State break existing users. > > --- > > Example: > > Wrong free OOB offset starts at OOB6: > oob_region->offset = NFC_SYS_DATA_SIZE + 2; > = 4 + 2 > = 6 > > oob_region->length = rknand->metadata_size - NFC_SYS_DATA_SIZE - 2; > = 32 - 4 - 2 > = 26 > > Together with this length above it overlaps a reserved space for the > boot blocks Page Address(PA) > > chip->oob_poi buffer layout for 8 steps: > > BBM0 BBM1 OOB2 OOB3 | OOB4 OOB5 OOB6 OOB7 > > OOB8 OOB9 OOB10 OOB11 | OOB12 OOB13 OOB15 OOB15 > OOB16 OOB17 OOB18 OOB19 | OOB20 OOB21 OOB22 OOB23 > > OOB24 OOB25 OOB26 OOB27 | PA0 PA1 PA2 PA3 > > ECC0 ECC1 ECC2 ECC3 | ... ... ... ... > > Fix by new offset at OOB2: > oob_region->offset = 2; > > The full range of free OOB with 8 steps runs from OOB2 > till/including OOB27. > --- > drivers/mtd/nand/raw/rockchip-nand-controller.c | 11 ++++------- > 1 file changed, 4 insertions(+), 7 deletions(-) > > diff --git a/drivers/mtd/nand/raw/rockchip-nand-controller.c b/drivers/mtd/nand/raw/rockchip-nand-controller.c > index 2312e2736..37fc07ba5 100644 > --- a/drivers/mtd/nand/raw/rockchip-nand-controller.c > +++ b/drivers/mtd/nand/raw/rockchip-nand-controller.c > @@ -562,9 +562,10 @@ static int rk_nfc_write_page_raw(struct nand_chip *chip, const u8 *buf, > * BBM OOB1 OOB2 OOB3 |......| PA0 PA1 PA2 PA3 > * > * The rk_nfc_ooblayout_free() function already has reserved > - * these 4 bytes with: > + * these 4 bytes together with 2 bytes for BBM > + * by reducing it's length: > * > - * oob_region->offset = NFC_SYS_DATA_SIZE + 2; > + * oob_region->length = rknand->metadata_size - NFC_SYS_DATA_SIZE - 2; > */ > if (!i) > memcpy(rk_nfc_oob_ptr(chip, i), > @@ -933,12 +934,8 @@ static int rk_nfc_ooblayout_free(struct mtd_info *mtd, int section, > if (section) > return -ERANGE; > > - /* > - * The beginning of the OOB area stores the reserved data for the NFC, > - * the size of the reserved data is NFC_SYS_DATA_SIZE bytes. > - */ > oob_region->length = rknand->metadata_size - NFC_SYS_DATA_SIZE - 2; > - oob_region->offset = NFC_SYS_DATA_SIZE + 2; > + oob_region->offset = 2; > > return 0; > } > -- > 2.30.2 > Thanks, Miquèl
diff --git a/drivers/mtd/nand/raw/rockchip-nand-controller.c b/drivers/mtd/nand/raw/rockchip-nand-controller.c index 2312e2736..37fc07ba5 100644 --- a/drivers/mtd/nand/raw/rockchip-nand-controller.c +++ b/drivers/mtd/nand/raw/rockchip-nand-controller.c @@ -562,9 +562,10 @@ static int rk_nfc_write_page_raw(struct nand_chip *chip, const u8 *buf, * BBM OOB1 OOB2 OOB3 |......| PA0 PA1 PA2 PA3 * * The rk_nfc_ooblayout_free() function already has reserved - * these 4 bytes with: + * these 4 bytes together with 2 bytes for BBM + * by reducing it's length: * - * oob_region->offset = NFC_SYS_DATA_SIZE + 2; + * oob_region->length = rknand->metadata_size - NFC_SYS_DATA_SIZE - 2; */ if (!i) memcpy(rk_nfc_oob_ptr(chip, i), @@ -933,12 +934,8 @@ static int rk_nfc_ooblayout_free(struct mtd_info *mtd, int section, if (section) return -ERANGE; - /* - * The beginning of the OOB area stores the reserved data for the NFC, - * the size of the reserved data is NFC_SYS_DATA_SIZE bytes. - */ oob_region->length = rknand->metadata_size - NFC_SYS_DATA_SIZE - 2; - oob_region->offset = NFC_SYS_DATA_SIZE + 2; + oob_region->offset = 2; return 0; }
The MTD framework reserves 1 or 2 bytes for the bad block marker depending on the bus size. The rockchip-nand-controller driver currently only supports a 8 bit bus, but reserves standard 2 bytes for the BBM in the chip->oob_poi buffer. The first free OOB byte is therefore OOB2 at offset 2. Page Address (PA) bytes are located at the last 4 positions before ECC. The current advertised free OOB area has an offset that starts at OOB6 and a length that overlaps with the space reserved for the PA bytes. Writing unrelated data to a reserved space with a specific task can corrupt our boot block page read order. Fix by changing the free OOB offset to 2. This change breaks existing jffs2 users. Signed-off-by: Johan Jonker <jbx6244@gmail.com> --- Changed V3: Change prefixes Reword State break existing users. --- Example: Wrong free OOB offset starts at OOB6: oob_region->offset = NFC_SYS_DATA_SIZE + 2; = 4 + 2 = 6 oob_region->length = rknand->metadata_size - NFC_SYS_DATA_SIZE - 2; = 32 - 4 - 2 = 26 Together with this length above it overlaps a reserved space for the boot blocks Page Address(PA) chip->oob_poi buffer layout for 8 steps: BBM0 BBM1 OOB2 OOB3 | OOB4 OOB5 OOB6 OOB7 OOB8 OOB9 OOB10 OOB11 | OOB12 OOB13 OOB15 OOB15 OOB16 OOB17 OOB18 OOB19 | OOB20 OOB21 OOB22 OOB23 OOB24 OOB25 OOB26 OOB27 | PA0 PA1 PA2 PA3 ECC0 ECC1 ECC2 ECC3 | ... ... ... ... Fix by new offset at OOB2: oob_region->offset = 2; The full range of free OOB with 8 steps runs from OOB2 till/including OOB27. --- drivers/mtd/nand/raw/rockchip-nand-controller.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) -- 2.30.2