Messages in this thread | | | Date | Wed, 29 Mar 2023 11:00:54 +0200 | From | Miquel Raynal <> | Subject | Re: [PATCH v2] mtd: rawnand: meson: fix bitmask for length in command word |
| |
Hi Arseniy,
avkrasnov@sberdevices.ru wrote on Wed, 29 Mar 2023 11:33:38 +0300:
> On 29.03.2023 11:09, Tudor Ambarus wrote: > > > > > > On 3/29/23 08:47, Arseniy Krasnov wrote: > >> Valid mask is 0x3FFF, without this patch the following problems were > >> found: > >> > >> 1) [ 0.938914] Could not find a valid ONFI parameter page, trying > >> bit-wise majority to recover it > >> [ 0.947384] ONFI parameter recovery failed, aborting > >> > >> 2) Read with disabled ECC mode was broken. > >> > >> Fixes: 8fae856c5350 ("mtd: rawnand: meson: add support for Amlogic NAND flash controller") > >> Cc: <Stable@vger.kernel.org> > >> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru> > >> --- > >> drivers/mtd/nand/raw/meson_nand.c | 6 +++--- > >> 1 file changed, 3 insertions(+), 3 deletions(-) > >> > >> diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c > >> index a28574c00900..074e14225c06 100644 > >> --- a/drivers/mtd/nand/raw/meson_nand.c > >> +++ b/drivers/mtd/nand/raw/meson_nand.c > >> @@ -280,7 +280,7 @@ static void meson_nfc_cmd_access(struct nand_chip *nand, int raw, bool dir, > >> > >> if (raw) { > >> len = mtd->writesize + mtd->oobsize; > >> - cmd = (len & GENMASK(5, 0)) | scrambler | DMA_DIR(dir); > >> + cmd = (len & GENMASK(13, 0)) | scrambler | DMA_DIR(dir); > > > > What happens when len > GENMASK(13, 0)? Do you check this somewhere? > > 'len' will be trimmed. I'm not sure that this case is possible here, because GENMASK(13, 0) > is hardware limit for this NAND controller, so 'writesize' and 'oobsize' will be initialized > to fit this value. Moreover GENMASK(13, 0) is 16Kb - i think it is big enough for single > read. Also i'm not sure that it is good approach to check 'len' here - we are in the middle > of NAND read processing.
No, you should check the page size will not exceed this limit in the attach hook, likely.
You should also refuse exec_op operations with a data length bigger than 16k (either with a manual check in your own parser or just by providing the max size to the parser table, depending on what's used).
> > > > > Please introduce a macro/field for GENMASK(13, 0), having such mask > > scattered along the code looks hackish and doesn't help readability. > > You'll get to use FIELD_PREP as well. > > Ack, i'll do it in v3 > > Thanks, Arseniy > > > > >> writel(cmd, nfc->reg_base + NFC_REG_CMD); > >> return; > >> } > >> @@ -544,7 +544,7 @@ static int meson_nfc_read_buf(struct nand_chip *nand, u8 *buf, int len) > >> if (ret) > >> goto out; > >> > >> - cmd = NFC_CMD_N2M | (len & GENMASK(5, 0)); > >> + cmd = NFC_CMD_N2M | (len & GENMASK(13, 0)); > >> writel(cmd, nfc->reg_base + NFC_REG_CMD); > >> > >> meson_nfc_drain_cmd(nfc); > >> @@ -568,7 +568,7 @@ static int meson_nfc_write_buf(struct nand_chip *nand, u8 *buf, int len) > >> if (ret) > >> return ret; > >> > >> - cmd = NFC_CMD_M2N | (len & GENMASK(5, 0)); > >> + cmd = NFC_CMD_M2N | (len & GENMASK(13, 0)); > >> writel(cmd, nfc->reg_base + NFC_REG_CMD); > >> > >> meson_nfc_drain_cmd(nfc);
Thanks, Miquèl
| |