Messages in this thread | | | Date | Wed, 29 Mar 2023 13:55:29 +0300 | Subject | Re: [PATCH v2] mtd: rawnand: meson: fix bitmask for length in command word | From | Arseniy Krasnov <> |
| |
On 29.03.2023 12:00, Miquel Raynal wrote: > 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). >
Ok, I see.
Thanks, Arseniy
>> >>> >>> 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
| |