* [PATCH v2] mtd: rawnand: meson: check buffer length @ 2023-06-06 10:16 Arseniy Krasnov 2023-06-06 16:29 ` Arseniy Krasnov 0 siblings, 1 reply; 12+ messages in thread From: Arseniy Krasnov @ 2023-06-06 10:16 UTC (permalink / raw) To: Liang Yang, Miquel Raynal, Richard Weinberger, Vignesh Raghavendra, Neil Armstrong, Kevin Hilman, Jerome Brunet, Martin Blumenstingl Cc: oxffffaa, kernel, Arseniy Krasnov, linux-mtd, linux-arm-kernel, linux-amlogic, linux-kernel Meson NAND controller has limited buffer length, so check it before command execution to avoid length trim. Also check MTD write size on chip attach. Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru> --- drivers/mtd/nand/raw/meson_nand.c | 47 +++++++++++++++++++++++++++---- 1 file changed, 42 insertions(+), 5 deletions(-) diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c index 23a73268421b..db6b18753071 100644 --- a/drivers/mtd/nand/raw/meson_nand.c +++ b/drivers/mtd/nand/raw/meson_nand.c @@ -111,6 +111,8 @@ #define PER_INFO_BYTE 8 +#define NFC_CMD_RAW_LEN GENMASK(13, 0) + struct meson_nfc_nand_chip { struct list_head node; struct nand_chip nand; @@ -284,7 +286,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(13, 0)) | scrambler | DMA_DIR(dir); + cmd = len | scrambler | DMA_DIR(dir); writel(cmd, nfc->reg_base + NFC_REG_CMD); return; } @@ -573,7 +575,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(13, 0)); + cmd = NFC_CMD_N2M | len; writel(cmd, nfc->reg_base + NFC_REG_CMD); meson_nfc_drain_cmd(nfc); @@ -597,7 +599,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(13, 0)); + cmd = NFC_CMD_M2N | len; writel(cmd, nfc->reg_base + NFC_REG_CMD); meson_nfc_drain_cmd(nfc); @@ -1007,6 +1009,31 @@ meson_nand_op_put_dma_safe_output_buf(const struct nand_op_instr *instr, kfree(buf); } +static int meson_nfc_check_op(struct nand_chip *chip, + const struct nand_operation *op) +{ + int op_id; + + for (op_id = 0; op_id < op->ninstrs; op_id++) { + const struct nand_op_instr *instr; + + instr = &op->instrs[op_id]; + + switch (instr->type) { + case NAND_OP_DATA_IN_INSTR: + case NAND_OP_DATA_OUT_INSTR: + if (instr->ctx.data.len > NFC_CMD_RAW_LEN) + return -ENOTSUPP; + + break; + default: + break; + } + } + + return 0; +} + static int meson_nfc_exec_op(struct nand_chip *nand, const struct nand_operation *op, bool check_only) { @@ -1015,10 +1042,12 @@ static int meson_nfc_exec_op(struct nand_chip *nand, const struct nand_op_instr *instr = NULL; void *buf; u32 op_id, delay_idle, cmd; + int err; int i; - if (check_only) - return 0; + err = meson_nfc_check_op(nand, op); + if (err || check_only) + return err; meson_nfc_select_chip(nand, op->cs); for (op_id = 0; op_id < op->ninstrs; op_id++) { @@ -1293,6 +1322,7 @@ static int meson_nand_attach_chip(struct nand_chip *nand) struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand); struct mtd_info *mtd = nand_to_mtd(nand); int nsectors = mtd->writesize / 1024; + int raw_writesize; int ret; if (!mtd->name) { @@ -1304,6 +1334,13 @@ static int meson_nand_attach_chip(struct nand_chip *nand) return -ENOMEM; } + raw_writesize = mtd->writesize + mtd->oobsize; + if (raw_writesize > NFC_CMD_RAW_LEN) { + dev_err(nfc->dev, "too big write size in raw mode: %d > %ld\n", + raw_writesize, NFC_CMD_RAW_LEN); + return -EINVAL; + } + if (nand->bbt_options & NAND_BBT_USE_FLASH) nand->bbt_options |= NAND_BBT_NO_OOB; -- 2.35.0 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2] mtd: rawnand: meson: check buffer length 2023-06-06 10:16 [PATCH v2] mtd: rawnand: meson: check buffer length Arseniy Krasnov @ 2023-06-06 16:29 ` Arseniy Krasnov 2023-06-07 21:17 ` Arseniy Krasnov 0 siblings, 1 reply; 12+ messages in thread From: Arseniy Krasnov @ 2023-06-06 16:29 UTC (permalink / raw) To: Liang Yang, Miquel Raynal, Richard Weinberger, Vignesh Raghavendra, Neil Armstrong, Kevin Hilman, Jerome Brunet, Martin Blumenstingl Cc: oxffffaa, kernel, linux-mtd, linux-arm-kernel, linux-amlogic, linux-kernel Sorry, here is changelog: v1 -> v2: * Move checks from 'switch/case' which executes commands in 'meson_nfc_exec_op()' to a special separated function 'meson_nfc_check_op()' which is called before commands processing. On 06.06.2023 13:16, Arseniy Krasnov wrote: > Meson NAND controller has limited buffer length, so check it before > command execution to avoid length trim. Also check MTD write size on > chip attach. > > Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru> > --- > drivers/mtd/nand/raw/meson_nand.c | 47 +++++++++++++++++++++++++++---- > 1 file changed, 42 insertions(+), 5 deletions(-) > > diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c > index 23a73268421b..db6b18753071 100644 > --- a/drivers/mtd/nand/raw/meson_nand.c > +++ b/drivers/mtd/nand/raw/meson_nand.c > @@ -111,6 +111,8 @@ > > #define PER_INFO_BYTE 8 > > +#define NFC_CMD_RAW_LEN GENMASK(13, 0) > + > struct meson_nfc_nand_chip { > struct list_head node; > struct nand_chip nand; > @@ -284,7 +286,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(13, 0)) | scrambler | DMA_DIR(dir); > + cmd = len | scrambler | DMA_DIR(dir); > writel(cmd, nfc->reg_base + NFC_REG_CMD); > return; > } > @@ -573,7 +575,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(13, 0)); > + cmd = NFC_CMD_N2M | len; > writel(cmd, nfc->reg_base + NFC_REG_CMD); > > meson_nfc_drain_cmd(nfc); > @@ -597,7 +599,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(13, 0)); > + cmd = NFC_CMD_M2N | len; > writel(cmd, nfc->reg_base + NFC_REG_CMD); > > meson_nfc_drain_cmd(nfc); > @@ -1007,6 +1009,31 @@ meson_nand_op_put_dma_safe_output_buf(const struct nand_op_instr *instr, > kfree(buf); > } > > +static int meson_nfc_check_op(struct nand_chip *chip, > + const struct nand_operation *op) > +{ > + int op_id; > + > + for (op_id = 0; op_id < op->ninstrs; op_id++) { > + const struct nand_op_instr *instr; > + > + instr = &op->instrs[op_id]; > + > + switch (instr->type) { > + case NAND_OP_DATA_IN_INSTR: > + case NAND_OP_DATA_OUT_INSTR: > + if (instr->ctx.data.len > NFC_CMD_RAW_LEN) > + return -ENOTSUPP; > + > + break; > + default: > + break; > + } > + } > + > + return 0; > +} > + > static int meson_nfc_exec_op(struct nand_chip *nand, > const struct nand_operation *op, bool check_only) > { > @@ -1015,10 +1042,12 @@ static int meson_nfc_exec_op(struct nand_chip *nand, > const struct nand_op_instr *instr = NULL; > void *buf; > u32 op_id, delay_idle, cmd; > + int err; > int i; > > - if (check_only) > - return 0; > + err = meson_nfc_check_op(nand, op); > + if (err || check_only) > + return err; > > meson_nfc_select_chip(nand, op->cs); > for (op_id = 0; op_id < op->ninstrs; op_id++) { > @@ -1293,6 +1322,7 @@ static int meson_nand_attach_chip(struct nand_chip *nand) > struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand); > struct mtd_info *mtd = nand_to_mtd(nand); > int nsectors = mtd->writesize / 1024; > + int raw_writesize; > int ret; > > if (!mtd->name) { > @@ -1304,6 +1334,13 @@ static int meson_nand_attach_chip(struct nand_chip *nand) > return -ENOMEM; > } > > + raw_writesize = mtd->writesize + mtd->oobsize; > + if (raw_writesize > NFC_CMD_RAW_LEN) { > + dev_err(nfc->dev, "too big write size in raw mode: %d > %ld\n", > + raw_writesize, NFC_CMD_RAW_LEN); > + return -EINVAL; > + } > + > if (nand->bbt_options & NAND_BBT_USE_FLASH) > nand->bbt_options |= NAND_BBT_NO_OOB; > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] mtd: rawnand: meson: check buffer length 2023-06-06 16:29 ` Arseniy Krasnov @ 2023-06-07 21:17 ` Arseniy Krasnov 2023-06-08 6:10 ` Miquel Raynal 2023-06-08 6:42 ` Liang Yang 0 siblings, 2 replies; 12+ messages in thread From: Arseniy Krasnov @ 2023-06-07 21:17 UTC (permalink / raw) To: Liang Yang, Miquel Raynal, Richard Weinberger, Vignesh Raghavendra, Neil Armstrong, Kevin Hilman, Jerome Brunet, Martin Blumenstingl Cc: oxffffaa, kernel, linux-mtd, linux-arm-kernel, linux-amlogic, linux-kernel Hi again Miquel, Liang! What do You think about this patch? Thanks, Arseniy On 06.06.2023 19:29, Arseniy Krasnov wrote: > Sorry, here is changelog: > v1 -> v2: > * Move checks from 'switch/case' which executes commands in 'meson_nfc_exec_op()' to a special > separated function 'meson_nfc_check_op()' which is called before commands processing. > > On 06.06.2023 13:16, Arseniy Krasnov wrote: >> Meson NAND controller has limited buffer length, so check it before >> command execution to avoid length trim. Also check MTD write size on >> chip attach. >> >> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru> >> --- >> drivers/mtd/nand/raw/meson_nand.c | 47 +++++++++++++++++++++++++++---- >> 1 file changed, 42 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c >> index 23a73268421b..db6b18753071 100644 >> --- a/drivers/mtd/nand/raw/meson_nand.c >> +++ b/drivers/mtd/nand/raw/meson_nand.c >> @@ -111,6 +111,8 @@ >> >> #define PER_INFO_BYTE 8 >> >> +#define NFC_CMD_RAW_LEN GENMASK(13, 0) >> + >> struct meson_nfc_nand_chip { >> struct list_head node; >> struct nand_chip nand; >> @@ -284,7 +286,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(13, 0)) | scrambler | DMA_DIR(dir); >> + cmd = len | scrambler | DMA_DIR(dir); >> writel(cmd, nfc->reg_base + NFC_REG_CMD); >> return; >> } >> @@ -573,7 +575,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(13, 0)); >> + cmd = NFC_CMD_N2M | len; >> writel(cmd, nfc->reg_base + NFC_REG_CMD); >> >> meson_nfc_drain_cmd(nfc); >> @@ -597,7 +599,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(13, 0)); >> + cmd = NFC_CMD_M2N | len; >> writel(cmd, nfc->reg_base + NFC_REG_CMD); >> >> meson_nfc_drain_cmd(nfc); >> @@ -1007,6 +1009,31 @@ meson_nand_op_put_dma_safe_output_buf(const struct nand_op_instr *instr, >> kfree(buf); >> } >> >> +static int meson_nfc_check_op(struct nand_chip *chip, >> + const struct nand_operation *op) >> +{ >> + int op_id; >> + >> + for (op_id = 0; op_id < op->ninstrs; op_id++) { >> + const struct nand_op_instr *instr; >> + >> + instr = &op->instrs[op_id]; >> + >> + switch (instr->type) { >> + case NAND_OP_DATA_IN_INSTR: >> + case NAND_OP_DATA_OUT_INSTR: >> + if (instr->ctx.data.len > NFC_CMD_RAW_LEN) >> + return -ENOTSUPP; >> + >> + break; >> + default: >> + break; >> + } >> + } >> + >> + return 0; >> +} >> + >> static int meson_nfc_exec_op(struct nand_chip *nand, >> const struct nand_operation *op, bool check_only) >> { >> @@ -1015,10 +1042,12 @@ static int meson_nfc_exec_op(struct nand_chip *nand, >> const struct nand_op_instr *instr = NULL; >> void *buf; >> u32 op_id, delay_idle, cmd; >> + int err; >> int i; >> >> - if (check_only) >> - return 0; >> + err = meson_nfc_check_op(nand, op); >> + if (err || check_only) >> + return err; >> >> meson_nfc_select_chip(nand, op->cs); >> for (op_id = 0; op_id < op->ninstrs; op_id++) { >> @@ -1293,6 +1322,7 @@ static int meson_nand_attach_chip(struct nand_chip *nand) >> struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand); >> struct mtd_info *mtd = nand_to_mtd(nand); >> int nsectors = mtd->writesize / 1024; >> + int raw_writesize; >> int ret; >> >> if (!mtd->name) { >> @@ -1304,6 +1334,13 @@ static int meson_nand_attach_chip(struct nand_chip *nand) >> return -ENOMEM; >> } >> >> + raw_writesize = mtd->writesize + mtd->oobsize; >> + if (raw_writesize > NFC_CMD_RAW_LEN) { >> + dev_err(nfc->dev, "too big write size in raw mode: %d > %ld\n", >> + raw_writesize, NFC_CMD_RAW_LEN); >> + return -EINVAL; >> + } >> + >> if (nand->bbt_options & NAND_BBT_USE_FLASH) >> nand->bbt_options |= NAND_BBT_NO_OOB; >> _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] mtd: rawnand: meson: check buffer length 2023-06-07 21:17 ` Arseniy Krasnov @ 2023-06-08 6:10 ` Miquel Raynal 2023-06-08 6:08 ` Arseniy Krasnov 2023-06-08 6:42 ` Liang Yang 1 sibling, 1 reply; 12+ messages in thread From: Miquel Raynal @ 2023-06-08 6:10 UTC (permalink / raw) To: Arseniy Krasnov Cc: Liang Yang, Richard Weinberger, Vignesh Raghavendra, Neil Armstrong, Kevin Hilman, Jerome Brunet, Martin Blumenstingl, oxffffaa, kernel, linux-mtd, linux-arm-kernel, linux-amlogic, linux-kernel Hi Arseniy, avkrasnov@sberdevices.ru wrote on Thu, 8 Jun 2023 00:17:35 +0300: > Hi again Miquel, Liang! > > What do You think about this patch? I really appreciate all the effort you are putting into this but please consider giving me a little bit of air as well, I'm already spending a lot of time reviewing all your patches. Please mind a ping is not relevant before a couple of weeks in general. In this case I had it in my "to apply" list but actually looking at it again I have a minor comment below. > > Thanks, Arseniy > > On 06.06.2023 19:29, Arseniy Krasnov wrote: > > Sorry, here is changelog: > > v1 -> v2: > > * Move checks from 'switch/case' which executes commands in 'meson_nfc_exec_op()' to a special > > separated function 'meson_nfc_check_op()' which is called before commands processing. > > > > On 06.06.2023 13:16, Arseniy Krasnov wrote: > >> Meson NAND controller has limited buffer length, so check it before > >> command execution to avoid length trim. Also check MTD write size on > >> chip attach. > >> > >> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru> > >> --- > >> drivers/mtd/nand/raw/meson_nand.c | 47 +++++++++++++++++++++++++++---- > >> 1 file changed, 42 insertions(+), 5 deletions(-) > >> > >> diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c > >> index 23a73268421b..db6b18753071 100644 > >> --- a/drivers/mtd/nand/raw/meson_nand.c > >> +++ b/drivers/mtd/nand/raw/meson_nand.c > >> @@ -111,6 +111,8 @@ > >> > >> #define PER_INFO_BYTE 8 > >> > >> +#define NFC_CMD_RAW_LEN GENMASK(13, 0) > >> + > >> struct meson_nfc_nand_chip { > >> struct list_head node; > >> struct nand_chip nand; > >> @@ -284,7 +286,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(13, 0)) | scrambler | DMA_DIR(dir); > >> + cmd = len | scrambler | DMA_DIR(dir); > >> writel(cmd, nfc->reg_base + NFC_REG_CMD); > >> return; > >> } > >> @@ -573,7 +575,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(13, 0)); > >> + cmd = NFC_CMD_N2M | len; > >> writel(cmd, nfc->reg_base + NFC_REG_CMD); > >> > >> meson_nfc_drain_cmd(nfc); > >> @@ -597,7 +599,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(13, 0)); > >> + cmd = NFC_CMD_M2N | len; > >> writel(cmd, nfc->reg_base + NFC_REG_CMD); > >> > >> meson_nfc_drain_cmd(nfc); > >> @@ -1007,6 +1009,31 @@ meson_nand_op_put_dma_safe_output_buf(const struct nand_op_instr *instr, > >> kfree(buf); > >> } > >> > >> +static int meson_nfc_check_op(struct nand_chip *chip, > >> + const struct nand_operation *op) > >> +{ > >> + int op_id; > >> + > >> + for (op_id = 0; op_id < op->ninstrs; op_id++) { > >> + const struct nand_op_instr *instr; > >> + > >> + instr = &op->instrs[op_id]; > >> + > >> + switch (instr->type) { > >> + case NAND_OP_DATA_IN_INSTR: > >> + case NAND_OP_DATA_OUT_INSTR: > >> + if (instr->ctx.data.len > NFC_CMD_RAW_LEN) > >> + return -ENOTSUPP; > >> + > >> + break; > >> + default: > >> + break; > >> + } > >> + } > >> + > >> + return 0; > >> +} > >> + > >> static int meson_nfc_exec_op(struct nand_chip *nand, > >> const struct nand_operation *op, bool check_only) > >> { > >> @@ -1015,10 +1042,12 @@ static int meson_nfc_exec_op(struct nand_chip *nand, > >> const struct nand_op_instr *instr = NULL; > >> void *buf; > >> u32 op_id, delay_idle, cmd; > >> + int err; > >> int i; > >> > >> - if (check_only) > >> - return 0; > >> + err = meson_nfc_check_op(nand, op); > >> + if (err || check_only) > >> + return err; For the sake of readability it is really important that we keep using standard constructions, so don't mix orthogonal checks and please use something like: err = check(); if (err) return err; if (check_only) return 0; > >> > >> meson_nfc_select_chip(nand, op->cs); > >> for (op_id = 0; op_id < op->ninstrs; op_id++) { > >> @@ -1293,6 +1322,7 @@ static int meson_nand_attach_chip(struct nand_chip *nand) > >> struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand); > >> struct mtd_info *mtd = nand_to_mtd(nand); > >> int nsectors = mtd->writesize / 1024; > >> + int raw_writesize; > >> int ret; > >> > >> if (!mtd->name) { > >> @@ -1304,6 +1334,13 @@ static int meson_nand_attach_chip(struct nand_chip *nand) > >> return -ENOMEM; > >> } > >> > >> + raw_writesize = mtd->writesize + mtd->oobsize; > >> + if (raw_writesize > NFC_CMD_RAW_LEN) { > >> + dev_err(nfc->dev, "too big write size in raw mode: %d > %ld\n", > >> + raw_writesize, NFC_CMD_RAW_LEN); > >> + return -EINVAL; > >> + } > >> + > >> if (nand->bbt_options & NAND_BBT_USE_FLASH) > >> nand->bbt_options |= NAND_BBT_NO_OOB; > >> Thanks, Miquèl _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] mtd: rawnand: meson: check buffer length 2023-06-08 6:10 ` Miquel Raynal @ 2023-06-08 6:08 ` Arseniy Krasnov 0 siblings, 0 replies; 12+ messages in thread From: Arseniy Krasnov @ 2023-06-08 6:08 UTC (permalink / raw) To: Miquel Raynal Cc: Liang Yang, Richard Weinberger, Vignesh Raghavendra, Neil Armstrong, Kevin Hilman, Jerome Brunet, Martin Blumenstingl, oxffffaa, kernel, linux-mtd, linux-arm-kernel, linux-amlogic, linux-kernel On 08.06.2023 09:10, Miquel Raynal wrote: > Hi Arseniy, > > avkrasnov@sberdevices.ru wrote on Thu, 8 Jun 2023 00:17:35 +0300: > >> Hi again Miquel, Liang! >> >> What do You think about this patch? > > I really appreciate all the effort you are putting into this but > please consider giving me a little bit of air as well, I'm already > spending a lot of time reviewing all your patches. Please mind a ping > is not relevant before a couple of weeks in general. Sorry for that! Sure, no problem! > > In this case I had it in my "to apply" list but actually looking at it > again I have a minor comment below. Got it! Thanks, Arseniy > >> >> Thanks, Arseniy >> >> On 06.06.2023 19:29, Arseniy Krasnov wrote: >>> Sorry, here is changelog: >>> v1 -> v2: >>> * Move checks from 'switch/case' which executes commands in 'meson_nfc_exec_op()' to a special >>> separated function 'meson_nfc_check_op()' which is called before commands processing. >>> >>> On 06.06.2023 13:16, Arseniy Krasnov wrote: >>>> Meson NAND controller has limited buffer length, so check it before >>>> command execution to avoid length trim. Also check MTD write size on >>>> chip attach. >>>> >>>> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru> >>>> --- >>>> drivers/mtd/nand/raw/meson_nand.c | 47 +++++++++++++++++++++++++++---- >>>> 1 file changed, 42 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c >>>> index 23a73268421b..db6b18753071 100644 >>>> --- a/drivers/mtd/nand/raw/meson_nand.c >>>> +++ b/drivers/mtd/nand/raw/meson_nand.c >>>> @@ -111,6 +111,8 @@ >>>> >>>> #define PER_INFO_BYTE 8 >>>> >>>> +#define NFC_CMD_RAW_LEN GENMASK(13, 0) >>>> + >>>> struct meson_nfc_nand_chip { >>>> struct list_head node; >>>> struct nand_chip nand; >>>> @@ -284,7 +286,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(13, 0)) | scrambler | DMA_DIR(dir); >>>> + cmd = len | scrambler | DMA_DIR(dir); >>>> writel(cmd, nfc->reg_base + NFC_REG_CMD); >>>> return; >>>> } >>>> @@ -573,7 +575,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(13, 0)); >>>> + cmd = NFC_CMD_N2M | len; >>>> writel(cmd, nfc->reg_base + NFC_REG_CMD); >>>> >>>> meson_nfc_drain_cmd(nfc); >>>> @@ -597,7 +599,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(13, 0)); >>>> + cmd = NFC_CMD_M2N | len; >>>> writel(cmd, nfc->reg_base + NFC_REG_CMD); >>>> >>>> meson_nfc_drain_cmd(nfc); >>>> @@ -1007,6 +1009,31 @@ meson_nand_op_put_dma_safe_output_buf(const struct nand_op_instr *instr, >>>> kfree(buf); >>>> } >>>> >>>> +static int meson_nfc_check_op(struct nand_chip *chip, >>>> + const struct nand_operation *op) >>>> +{ >>>> + int op_id; >>>> + >>>> + for (op_id = 0; op_id < op->ninstrs; op_id++) { >>>> + const struct nand_op_instr *instr; >>>> + >>>> + instr = &op->instrs[op_id]; >>>> + >>>> + switch (instr->type) { >>>> + case NAND_OP_DATA_IN_INSTR: >>>> + case NAND_OP_DATA_OUT_INSTR: >>>> + if (instr->ctx.data.len > NFC_CMD_RAW_LEN) >>>> + return -ENOTSUPP; >>>> + >>>> + break; >>>> + default: >>>> + break; >>>> + } >>>> + } >>>> + >>>> + return 0; >>>> +} >>>> + >>>> static int meson_nfc_exec_op(struct nand_chip *nand, >>>> const struct nand_operation *op, bool check_only) >>>> { >>>> @@ -1015,10 +1042,12 @@ static int meson_nfc_exec_op(struct nand_chip *nand, >>>> const struct nand_op_instr *instr = NULL; >>>> void *buf; >>>> u32 op_id, delay_idle, cmd; >>>> + int err; >>>> int i; >>>> >>>> - if (check_only) >>>> - return 0; >>>> + err = meson_nfc_check_op(nand, op); >>>> + if (err || check_only) >>>> + return err; > > For the sake of readability it is really important that we keep using > standard constructions, so don't mix orthogonal checks and please use > something like: > > err = check(); > if (err) > return err; > > if (check_only) > return 0; > >>>> >>>> meson_nfc_select_chip(nand, op->cs); >>>> for (op_id = 0; op_id < op->ninstrs; op_id++) { >>>> @@ -1293,6 +1322,7 @@ static int meson_nand_attach_chip(struct nand_chip *nand) >>>> struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand); >>>> struct mtd_info *mtd = nand_to_mtd(nand); >>>> int nsectors = mtd->writesize / 1024; >>>> + int raw_writesize; >>>> int ret; >>>> >>>> if (!mtd->name) { >>>> @@ -1304,6 +1334,13 @@ static int meson_nand_attach_chip(struct nand_chip *nand) >>>> return -ENOMEM; >>>> } >>>> >>>> + raw_writesize = mtd->writesize + mtd->oobsize; >>>> + if (raw_writesize > NFC_CMD_RAW_LEN) { >>>> + dev_err(nfc->dev, "too big write size in raw mode: %d > %ld\n", >>>> + raw_writesize, NFC_CMD_RAW_LEN); >>>> + return -EINVAL; >>>> + } >>>> + >>>> if (nand->bbt_options & NAND_BBT_USE_FLASH) >>>> nand->bbt_options |= NAND_BBT_NO_OOB; >>>> > > > Thanks, > Miquèl _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] mtd: rawnand: meson: check buffer length 2023-06-07 21:17 ` Arseniy Krasnov 2023-06-08 6:10 ` Miquel Raynal @ 2023-06-08 6:42 ` Liang Yang 2023-06-08 6:54 ` Miquel Raynal 1 sibling, 1 reply; 12+ messages in thread From: Liang Yang @ 2023-06-08 6:42 UTC (permalink / raw) To: Arseniy Krasnov, Miquel Raynal, Richard Weinberger, Vignesh Raghavendra, Neil Armstrong, Kevin Hilman, Jerome Brunet, Martin Blumenstingl Cc: oxffffaa, kernel, linux-mtd, linux-arm-kernel, linux-amlogic, linux-kernel Hi Arseniy, On 2023/6/8 5:17, Arseniy Krasnov wrote: > [ EXTERNAL EMAIL ] > > Hi again Miquel, Liang! > > What do You think about this patch? > > Thanks, Arseniy > > On 06.06.2023 19:29, Arseniy Krasnov wrote: >> Sorry, here is changelog: >> v1 -> v2: >> * Move checks from 'switch/case' which executes commands in 'meson_nfc_exec_op()' to a special >> separated function 'meson_nfc_check_op()' which is called before commands processing. >> >> On 06.06.2023 13:16, Arseniy Krasnov wrote: >>> Meson NAND controller has limited buffer length, so check it before >>> command execution to avoid length trim. Also check MTD write size on >>> chip attach. >>> >>> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru> >>> --- >>> drivers/mtd/nand/raw/meson_nand.c | 47 +++++++++++++++++++++++++++---- >>> 1 file changed, 42 insertions(+), 5 deletions(-) >>> >>> diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c >>> index 23a73268421b..db6b18753071 100644 >>> --- a/drivers/mtd/nand/raw/meson_nand.c >>> +++ b/drivers/mtd/nand/raw/meson_nand.c >>> @@ -111,6 +111,8 @@ >>> >>> #define PER_INFO_BYTE 8 >>> >>> +#define NFC_CMD_RAW_LEN GENMASK(13, 0) >>> + >>> struct meson_nfc_nand_chip { >>> struct list_head node; >>> struct nand_chip nand; >>> @@ -284,7 +286,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(13, 0)) | scrambler | DMA_DIR(dir); >>> + cmd = len | scrambler | DMA_DIR(dir); >>> writel(cmd, nfc->reg_base + NFC_REG_CMD); I think we could keep "& GENMASK(13, 0)". it is better here to indicate how many bits of length in this command and don't destory the command once we don't check the "len" outside of this function. or the code "if (len > NFC_CMD_RAW_LEN)" is better to put inside this function nearly. Thanks. >>> return; >>> } >>> @@ -573,7 +575,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(13, 0)); >>> + cmd = NFC_CMD_N2M | len; >>> writel(cmd, nfc->reg_base + NFC_REG_CMD); >>> >>> meson_nfc_drain_cmd(nfc); >>> @@ -597,7 +599,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(13, 0)); >>> + cmd = NFC_CMD_M2N | len; >>> writel(cmd, nfc->reg_base + NFC_REG_CMD); >>> >>> meson_nfc_drain_cmd(nfc); >>> @@ -1007,6 +1009,31 @@ meson_nand_op_put_dma_safe_output_buf(const struct nand_op_instr *instr, >>> kfree(buf); >>> } >>> >>> +static int meson_nfc_check_op(struct nand_chip *chip, >>> + const struct nand_operation *op) >>> +{ >>> + int op_id; >>> + >>> + for (op_id = 0; op_id < op->ninstrs; op_id++) { >>> + const struct nand_op_instr *instr; >>> + >>> + instr = &op->instrs[op_id]; >>> + >>> + switch (instr->type) { >>> + case NAND_OP_DATA_IN_INSTR: >>> + case NAND_OP_DATA_OUT_INSTR: >>> + if (instr->ctx.data.len > NFC_CMD_RAW_LEN) >>> + return -ENOTSUPP; >>> + >>> + break; >>> + default: >>> + break; >>> + } >>> + } >>> + >>> + return 0; >>> +} >>> + >>> static int meson_nfc_exec_op(struct nand_chip *nand, >>> const struct nand_operation *op, bool check_only) >>> { >>> @@ -1015,10 +1042,12 @@ static int meson_nfc_exec_op(struct nand_chip *nand, >>> const struct nand_op_instr *instr = NULL; >>> void *buf; >>> u32 op_id, delay_idle, cmd; >>> + int err; >>> int i; >>> >>> - if (check_only) >>> - return 0; >>> + err = meson_nfc_check_op(nand, op); >>> + if (err || check_only) >>> + return err; >>> >>> meson_nfc_select_chip(nand, op->cs); >>> for (op_id = 0; op_id < op->ninstrs; op_id++) { >>> @@ -1293,6 +1322,7 @@ static int meson_nand_attach_chip(struct nand_chip *nand) >>> struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand); >>> struct mtd_info *mtd = nand_to_mtd(nand); >>> int nsectors = mtd->writesize / 1024; >>> + int raw_writesize; >>> int ret; >>> >>> if (!mtd->name) { >>> @@ -1304,6 +1334,13 @@ static int meson_nand_attach_chip(struct nand_chip *nand) >>> return -ENOMEM; >>> } >>> >>> + raw_writesize = mtd->writesize + mtd->oobsize; >>> + if (raw_writesize > NFC_CMD_RAW_LEN) { >>> + dev_err(nfc->dev, "too big write size in raw mode: %d > %ld\n", >>> + raw_writesize, NFC_CMD_RAW_LEN); >>> + return -EINVAL; >>> + } >>> + >>> if (nand->bbt_options & NAND_BBT_USE_FLASH) >>> nand->bbt_options |= NAND_BBT_NO_OOB; >>> _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] mtd: rawnand: meson: check buffer length 2023-06-08 6:42 ` Liang Yang @ 2023-06-08 6:54 ` Miquel Raynal 2023-06-08 7:12 ` Liang Yang 0 siblings, 1 reply; 12+ messages in thread From: Miquel Raynal @ 2023-06-08 6:54 UTC (permalink / raw) To: Liang Yang Cc: Arseniy Krasnov, Richard Weinberger, Vignesh Raghavendra, Neil Armstrong, Kevin Hilman, Jerome Brunet, Martin Blumenstingl, oxffffaa, kernel, linux-mtd, linux-arm-kernel, linux-amlogic, linux-kernel Hi Liang, liang.yang@amlogic.com wrote on Thu, 8 Jun 2023 14:42:53 +0800: > Hi Arseniy, > > On 2023/6/8 5:17, Arseniy Krasnov wrote: > > [ EXTERNAL EMAIL ] > > > > Hi again Miquel, Liang! > > > > What do You think about this patch? > > > > Thanks, Arseniy > > > > On 06.06.2023 19:29, Arseniy Krasnov wrote: > >> Sorry, here is changelog: > >> v1 -> v2: > >> * Move checks from 'switch/case' which executes commands in 'meson_nfc_exec_op()' to a special > >> separated function 'meson_nfc_check_op()' which is called before commands processing. > >> > >> On 06.06.2023 13:16, Arseniy Krasnov wrote: > >>> Meson NAND controller has limited buffer length, so check it before > >>> command execution to avoid length trim. Also check MTD write size on > >>> chip attach. > >>> > >>> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru> > >>> --- > >>> drivers/mtd/nand/raw/meson_nand.c | 47 +++++++++++++++++++++++++++---- > >>> 1 file changed, 42 insertions(+), 5 deletions(-) > >>> > >>> diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c > >>> index 23a73268421b..db6b18753071 100644 > >>> --- a/drivers/mtd/nand/raw/meson_nand.c > >>> +++ b/drivers/mtd/nand/raw/meson_nand.c > >>> @@ -111,6 +111,8 @@ > >>> > >>> #define PER_INFO_BYTE 8 > >>> > >>> +#define NFC_CMD_RAW_LEN GENMASK(13, 0) > >>> + > >>> struct meson_nfc_nand_chip { > >>> struct list_head node; > >>> struct nand_chip nand; > >>> @@ -284,7 +286,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(13, 0)) | scrambler | DMA_DIR(dir); > >>> + cmd = len | scrambler | DMA_DIR(dir); > >>> writel(cmd, nfc->reg_base + NFC_REG_CMD); > > I think we could keep "& GENMASK(13, 0)". it is better here to indicate how many bits of length in this command and don't destory the command once we don't check the "len" outside of this function. or the code "if (len > NFC_CMD_RAW_LEN)" is better to put inside this function nearly. Thanks. It depends who calls this helper. If only used inside exec_op, it's not useful. If used outside, then if you want to clarify things you may want to use: #define NFC_CMD_OP_LEN(cmd) FIELD_PREP(GENMASK(13, 0), (cmd)) This is by far my favorite construction. Could apply to many other fields, like DMA_DIR, scrambler, etc. > >>> return; > >>> } > >>> @@ -573,7 +575,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(13, 0)); > >>> + cmd = NFC_CMD_N2M | len; > >>> writel(cmd, nfc->reg_base + NFC_REG_CMD); > >>> > >>> meson_nfc_drain_cmd(nfc); > >>> @@ -597,7 +599,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(13, 0)); > >>> + cmd = NFC_CMD_M2N | len; > >>> writel(cmd, nfc->reg_base + NFC_REG_CMD); > >>> > >>> meson_nfc_drain_cmd(nfc); > >>> @@ -1007,6 +1009,31 @@ meson_nand_op_put_dma_safe_output_buf(const struct nand_op_instr *instr, > >>> kfree(buf); > >>> } > >>> > >>> +static int meson_nfc_check_op(struct nand_chip *chip, > >>> + const struct nand_operation *op) > >>> +{ > >>> + int op_id; > >>> + > >>> + for (op_id = 0; op_id < op->ninstrs; op_id++) { > >>> + const struct nand_op_instr *instr; > >>> + > >>> + instr = &op->instrs[op_id]; > >>> + > >>> + switch (instr->type) { > >>> + case NAND_OP_DATA_IN_INSTR: > >>> + case NAND_OP_DATA_OUT_INSTR: > >>> + if (instr->ctx.data.len > NFC_CMD_RAW_LEN) > >>> + return -ENOTSUPP; > >>> + > >>> + break; > >>> + default: > >>> + break; > >>> + } > >>> + } > >>> + > >>> + return 0; > >>> +} > >>> + > >>> static int meson_nfc_exec_op(struct nand_chip *nand, > >>> const struct nand_operation *op, bool check_only) > >>> { > >>> @@ -1015,10 +1042,12 @@ static int meson_nfc_exec_op(struct nand_chip *nand, > >>> const struct nand_op_instr *instr = NULL; > >>> void *buf; > >>> u32 op_id, delay_idle, cmd; > >>> + int err; > >>> int i; > >>> > >>> - if (check_only) > >>> - return 0; > >>> + err = meson_nfc_check_op(nand, op); > >>> + if (err || check_only) > >>> + return err; > >>> > >>> meson_nfc_select_chip(nand, op->cs); > >>> for (op_id = 0; op_id < op->ninstrs; op_id++) { > >>> @@ -1293,6 +1322,7 @@ static int meson_nand_attach_chip(struct nand_chip *nand) > >>> struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand); > >>> struct mtd_info *mtd = nand_to_mtd(nand); > >>> int nsectors = mtd->writesize / 1024; > >>> + int raw_writesize; > >>> int ret; > >>> > >>> if (!mtd->name) { > >>> @@ -1304,6 +1334,13 @@ static int meson_nand_attach_chip(struct nand_chip *nand) > >>> return -ENOMEM; > >>> } > >>> > >>> + raw_writesize = mtd->writesize + mtd->oobsize; > >>> + if (raw_writesize > NFC_CMD_RAW_LEN) { > >>> + dev_err(nfc->dev, "too big write size in raw mode: %d > %ld\n", > >>> + raw_writesize, NFC_CMD_RAW_LEN); > >>> + return -EINVAL; > >>> + } > >>> + > >>> if (nand->bbt_options & NAND_BBT_USE_FLASH) > >>> nand->bbt_options |= NAND_BBT_NO_OOB; > >>> Thanks, Miquèl _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] mtd: rawnand: meson: check buffer length 2023-06-08 6:54 ` Miquel Raynal @ 2023-06-08 7:12 ` Liang Yang 2023-06-08 12:37 ` Liang Yang 0 siblings, 1 reply; 12+ messages in thread From: Liang Yang @ 2023-06-08 7:12 UTC (permalink / raw) To: Miquel Raynal Cc: Arseniy Krasnov, Richard Weinberger, Vignesh Raghavendra, Neil Armstrong, Kevin Hilman, Jerome Brunet, Martin Blumenstingl, oxffffaa, kernel, linux-mtd, linux-arm-kernel, linux-amlogic, linux-kernel Hi Miquel, On 2023/6/8 14:54, Miquel Raynal wrote: > [ EXTERNAL EMAIL ] > > Hi Liang, > > liang.yang@amlogic.com wrote on Thu, 8 Jun 2023 14:42:53 +0800: > >> Hi Arseniy, >> >> On 2023/6/8 5:17, Arseniy Krasnov wrote: >>> [ EXTERNAL EMAIL ] >>> >>> Hi again Miquel, Liang! >>> >>> What do You think about this patch? >>> >>> Thanks, Arseniy >>> >>> On 06.06.2023 19:29, Arseniy Krasnov wrote: >>>> Sorry, here is changelog: >>>> v1 -> v2: >>>> * Move checks from 'switch/case' which executes commands in 'meson_nfc_exec_op()' to a special >>>> separated function 'meson_nfc_check_op()' which is called before commands processing. >>>> >>>> On 06.06.2023 13:16, Arseniy Krasnov wrote: >>>>> Meson NAND controller has limited buffer length, so check it before >>>>> command execution to avoid length trim. Also check MTD write size on >>>>> chip attach. >>>>> >>>>> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru> >>>>> --- >>>>> drivers/mtd/nand/raw/meson_nand.c | 47 +++++++++++++++++++++++++++---- >>>>> 1 file changed, 42 insertions(+), 5 deletions(-) >>>>> >>>>> diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c >>>>> index 23a73268421b..db6b18753071 100644 >>>>> --- a/drivers/mtd/nand/raw/meson_nand.c >>>>> +++ b/drivers/mtd/nand/raw/meson_nand.c >>>>> @@ -111,6 +111,8 @@ >>>>> >>>>> #define PER_INFO_BYTE 8 >>>>> >>>>> +#define NFC_CMD_RAW_LEN GENMASK(13, 0) >>>>> + >>>>> struct meson_nfc_nand_chip { >>>>> struct list_head node; >>>>> struct nand_chip nand; >>>>> @@ -284,7 +286,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(13, 0)) | scrambler | DMA_DIR(dir); >>>>> + cmd = len | scrambler | DMA_DIR(dir); >>>>> writel(cmd, nfc->reg_base + NFC_REG_CMD); >> >> I think we could keep "& GENMASK(13, 0)". it is better here to indicate how many bits of length in this command and don't destory the command once we don't check the "len" outside of this function. or the code "if (len > NFC_CMD_RAW_LEN)" is better to put inside this function nearly. Thanks. > > It depends who calls this helper. If only used inside exec_op,write_page_raw() and read_page_raw() also call meson_nfc_cmd_access(), but i don't find where constrain the "len". > it's not useful. If used outside, then if you want to clarify > things you may want to use: > > #define NFC_CMD_OP_LEN(cmd) FIELD_PREP(GENMASK(13, 0), (cmd)) > > This is by far my favorite construction. Could apply to many other > fields, like DMA_DIR, scrambler, etc. > >>>>> return; >>>>> } >>>>> @@ -573,7 +575,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(13, 0)); >>>>> + cmd = NFC_CMD_N2M | len; >>>>> writel(cmd, nfc->reg_base + NFC_REG_CMD); >>>>> >>>>> meson_nfc_drain_cmd(nfc); >>>>> @@ -597,7 +599,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(13, 0)); >>>>> + cmd = NFC_CMD_M2N | len; >>>>> writel(cmd, nfc->reg_base + NFC_REG_CMD); >>>>> >>>>> meson_nfc_drain_cmd(nfc); >>>>> @@ -1007,6 +1009,31 @@ meson_nand_op_put_dma_safe_output_buf(const struct nand_op_instr *instr, >>>>> kfree(buf); >>>>> } >>>>> >>>>> +static int meson_nfc_check_op(struct nand_chip *chip, >>>>> + const struct nand_operation *op) >>>>> +{ >>>>> + int op_id; >>>>> + >>>>> + for (op_id = 0; op_id < op->ninstrs; op_id++) { >>>>> + const struct nand_op_instr *instr; >>>>> + >>>>> + instr = &op->instrs[op_id]; >>>>> + >>>>> + switch (instr->type) { >>>>> + case NAND_OP_DATA_IN_INSTR: >>>>> + case NAND_OP_DATA_OUT_INSTR: >>>>> + if (instr->ctx.data.len > NFC_CMD_RAW_LEN) >>>>> + return -ENOTSUPP; >>>>> + >>>>> + break; >>>>> + default: >>>>> + break; >>>>> + } >>>>> + } >>>>> + >>>>> + return 0; >>>>> +} >>>>> + >>>>> static int meson_nfc_exec_op(struct nand_chip *nand, >>>>> const struct nand_operation *op, bool check_only) >>>>> { >>>>> @@ -1015,10 +1042,12 @@ static int meson_nfc_exec_op(struct nand_chip *nand, >>>>> const struct nand_op_instr *instr = NULL; >>>>> void *buf; >>>>> u32 op_id, delay_idle, cmd; >>>>> + int err; >>>>> int i; >>>>> >>>>> - if (check_only) >>>>> - return 0; >>>>> + err = meson_nfc_check_op(nand, op); >>>>> + if (err || check_only) >>>>> + return err; >>>>> >>>>> meson_nfc_select_chip(nand, op->cs); >>>>> for (op_id = 0; op_id < op->ninstrs; op_id++) { >>>>> @@ -1293,6 +1322,7 @@ static int meson_nand_attach_chip(struct nand_chip *nand) >>>>> struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand); >>>>> struct mtd_info *mtd = nand_to_mtd(nand); >>>>> int nsectors = mtd->writesize / 1024; >>>>> + int raw_writesize; >>>>> int ret; >>>>> >>>>> if (!mtd->name) { >>>>> @@ -1304,6 +1334,13 @@ static int meson_nand_attach_chip(struct nand_chip *nand) >>>>> return -ENOMEM; >>>>> } >>>>> >>>>> + raw_writesize = mtd->writesize + mtd->oobsize; >>>>> + if (raw_writesize > NFC_CMD_RAW_LEN) { >>>>> + dev_err(nfc->dev, "too big write size in raw mode: %d > %ld\n", >>>>> + raw_writesize, NFC_CMD_RAW_LEN); >>>>> + return -EINVAL; >>>>> + } >>>>> + >>>>> if (nand->bbt_options & NAND_BBT_USE_FLASH) >>>>> nand->bbt_options |= NAND_BBT_NO_OOB; >>>>> > > > Thanks, > Miquèl _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] mtd: rawnand: meson: check buffer length 2023-06-08 7:12 ` Liang Yang @ 2023-06-08 12:37 ` Liang Yang 2023-06-08 13:21 ` Miquel Raynal 0 siblings, 1 reply; 12+ messages in thread From: Liang Yang @ 2023-06-08 12:37 UTC (permalink / raw) To: Miquel Raynal, Arseniy Krasnov Cc: Richard Weinberger, Vignesh Raghavendra, Neil Armstrong, Kevin Hilman, Jerome Brunet, Martin Blumenstingl, oxffffaa, kernel, linux-mtd, linux-arm-kernel, linux-amlogic, linux-kernel Hi Arseniy and Miquel, On 2023/6/8 15:12, Liang Yang wrote: > Hi Miquel, > > On 2023/6/8 14:54, Miquel Raynal wrote: >> [ EXTERNAL EMAIL ] >> >> Hi Liang, >> >> liang.yang@amlogic.com wrote on Thu, 8 Jun 2023 14:42:53 +0800: >> >>> Hi Arseniy, >>> >>> On 2023/6/8 5:17, Arseniy Krasnov wrote: >>>> [ EXTERNAL EMAIL ] >>>> >>>> Hi again Miquel, Liang! >>>> >>>> What do You think about this patch? >>>> >>>> Thanks, Arseniy >>>> >>>> On 06.06.2023 19:29, Arseniy Krasnov wrote: >>>>> Sorry, here is changelog: >>>>> v1 -> v2: >>>>> * Move checks from 'switch/case' which executes commands in >>>>> 'meson_nfc_exec_op()' to a special >>>>> separated function 'meson_nfc_check_op()' which is called >>>>> before commands processing. >>>>> >>>>> On 06.06.2023 13:16, Arseniy Krasnov wrote: >>>>>> Meson NAND controller has limited buffer length, so check it before >>>>>> command execution to avoid length trim. Also check MTD write size on >>>>>> chip attach. >>>>>> >>>>>> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru> >>>>>> --- >>>>>> drivers/mtd/nand/raw/meson_nand.c | 47 >>>>>> +++++++++++++++++++++++++++---- >>>>>> 1 file changed, 42 insertions(+), 5 deletions(-) >>>>>> >>>>>> diff --git a/drivers/mtd/nand/raw/meson_nand.c >>>>>> b/drivers/mtd/nand/raw/meson_nand.c >>>>>> index 23a73268421b..db6b18753071 100644 >>>>>> --- a/drivers/mtd/nand/raw/meson_nand.c >>>>>> +++ b/drivers/mtd/nand/raw/meson_nand.c >>>>>> @@ -111,6 +111,8 @@ >>>>>> >>>>>> #define PER_INFO_BYTE 8 >>>>>> >>>>>> +#define NFC_CMD_RAW_LEN GENMASK(13, 0) >>>>>> + >>>>>> struct meson_nfc_nand_chip { >>>>>> struct list_head node; >>>>>> struct nand_chip nand; >>>>>> @@ -284,7 +286,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(13, 0)) | scrambler | DMA_DIR(dir); >>>>>> + cmd = len | scrambler | DMA_DIR(dir); >>>>>> writel(cmd, nfc->reg_base + NFC_REG_CMD); >>> >>> I think we could keep "& GENMASK(13, 0)". it is better here to >>> indicate how many bits of length in this command and don't destory >>> the command once we don't check the "len" outside of this function. >>> or the code "if (len > NFC_CMD_RAW_LEN)" is better to put inside >>> this function nearly. Thanks. >> >> It depends who calls this helper. If only used inside >> exec_op,write_page_raw() and read_page_raw() also call >> meson_nfc_cmd_access(), > but i don't find where constrain the "len". Is the helper "meson_nfc_check_op()" needed by exec_op() after the constraint in attach_chip()? the length passed in exec_op() seems smaller than "mtd->writesize + mtd->oobsize" now. @Arseniy if it does need, I think the same constraint is needed by "meson_nfc_cmd_access()" > >> it's not useful. If used outside, then if you want to clarify >> things you may want to use: >> >> #define NFC_CMD_OP_LEN(cmd) FIELD_PREP(GENMASK(13, 0), (cmd)) >> >> This is by far my favorite construction. Could apply to many other >> fields, like DMA_DIR, scrambler, etc. @Miquel, FIELD_PREP() is better, but i have no idea whether we should add FIELD_PREP() in this patch, or keep the previous code. >> >>>>>> return; >>>>>> } >>>>>> @@ -573,7 +575,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(13, 0)); >>>>>> + cmd = NFC_CMD_N2M | len; >>>>>> writel(cmd, nfc->reg_base + NFC_REG_CMD); >>>>>> >>>>>> meson_nfc_drain_cmd(nfc); >>>>>> @@ -597,7 +599,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(13, 0)); >>>>>> + cmd = NFC_CMD_M2N | len; >>>>>> writel(cmd, nfc->reg_base + NFC_REG_CMD); >>>>>> >>>>>> meson_nfc_drain_cmd(nfc); >>>>>> @@ -1007,6 +1009,31 @@ meson_nand_op_put_dma_safe_output_buf(const >>>>>> struct nand_op_instr *instr, >>>>>> kfree(buf); >>>>>> } >>>>>> >>>>>> +static int meson_nfc_check_op(struct nand_chip *chip, >>>>>> + const struct nand_operation *op) >>>>>> +{ >>>>>> + int op_id; >>>>>> + >>>>>> + for (op_id = 0; op_id < op->ninstrs; op_id++) { >>>>>> + const struct nand_op_instr *instr; >>>>>> + >>>>>> + instr = &op->instrs[op_id]; >>>>>> + >>>>>> + switch (instr->type) { >>>>>> + case NAND_OP_DATA_IN_INSTR: >>>>>> + case NAND_OP_DATA_OUT_INSTR: >>>>>> + if (instr->ctx.data.len > NFC_CMD_RAW_LEN) >>>>>> + return -ENOTSUPP; >>>>>> + >>>>>> + break; >>>>>> + default: >>>>>> + break; >>>>>> + } >>>>>> + } >>>>>> + >>>>>> + return 0; >>>>>> +} >>>>>> + >>>>>> static int meson_nfc_exec_op(struct nand_chip *nand, >>>>>> const struct nand_operation *op, bool >>>>>> check_only) >>>>>> { >>>>>> @@ -1015,10 +1042,12 @@ static int meson_nfc_exec_op(struct >>>>>> nand_chip *nand, >>>>>> const struct nand_op_instr *instr = NULL; >>>>>> void *buf; >>>>>> u32 op_id, delay_idle, cmd; >>>>>> + int err; >>>>>> int i; >>>>>> >>>>>> - if (check_only) >>>>>> - return 0; >>>>>> + err = meson_nfc_check_op(nand, op); >>>>>> + if (err || check_only) >>>>>> + return err; >>>>>> >>>>>> meson_nfc_select_chip(nand, op->cs); >>>>>> for (op_id = 0; op_id < op->ninstrs; op_id++) { >>>>>> @@ -1293,6 +1322,7 @@ static int meson_nand_attach_chip(struct >>>>>> nand_chip *nand) >>>>>> struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand); >>>>>> struct mtd_info *mtd = nand_to_mtd(nand); >>>>>> int nsectors = mtd->writesize / 1024; >>>>>> + int raw_writesize; >>>>>> int ret; >>>>>> >>>>>> if (!mtd->name) { >>>>>> @@ -1304,6 +1334,13 @@ static int meson_nand_attach_chip(struct >>>>>> nand_chip *nand) >>>>>> return -ENOMEM; >>>>>> } >>>>>> >>>>>> + raw_writesize = mtd->writesize + mtd->oobsize; >>>>>> + if (raw_writesize > NFC_CMD_RAW_LEN) { >>>>>> + dev_err(nfc->dev, "too big write size in raw mode: %d >>>>>> > %ld\n", >>>>>> + raw_writesize, NFC_CMD_RAW_LEN); >>>>>> + return -EINVAL; >>>>>> + } >>>>>> + >>>>>> if (nand->bbt_options & NAND_BBT_USE_FLASH) >>>>>> nand->bbt_options |= NAND_BBT_NO_OOB; >>>>>> >> >> >> Thanks, >> Miquèl _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] mtd: rawnand: meson: check buffer length 2023-06-08 12:37 ` Liang Yang @ 2023-06-08 13:21 ` Miquel Raynal 2023-06-09 7:59 ` Liang Yang 0 siblings, 1 reply; 12+ messages in thread From: Miquel Raynal @ 2023-06-08 13:21 UTC (permalink / raw) To: Liang Yang Cc: Arseniy Krasnov, Richard Weinberger, Vignesh Raghavendra, Neil Armstrong, Kevin Hilman, Jerome Brunet, Martin Blumenstingl, oxffffaa, kernel, linux-mtd, linux-arm-kernel, linux-amlogic, linux-kernel Hi Liang, liang.yang@amlogic.com wrote on Thu, 8 Jun 2023 20:37:14 +0800: > Hi Arseniy and Miquel, > > On 2023/6/8 15:12, Liang Yang wrote: > > Hi Miquel, > > > > On 2023/6/8 14:54, Miquel Raynal wrote: > >> [ EXTERNAL EMAIL ] > >> > >> Hi Liang, > >> > >> liang.yang@amlogic.com wrote on Thu, 8 Jun 2023 14:42:53 +0800: > >> > >>> Hi Arseniy, > >>> > >>> On 2023/6/8 5:17, Arseniy Krasnov wrote: > >>>> [ EXTERNAL EMAIL ] > >>>> > >>>> Hi again Miquel, Liang! > >>>> > >>>> What do You think about this patch? > >>>> > >>>> Thanks, Arseniy > >>>> > >>>> On 06.06.2023 19:29, Arseniy Krasnov wrote: > >>>>> Sorry, here is changelog: > >>>>> v1 -> v2: > >>>>> * Move checks from 'switch/case' which executes commands in >>>>> 'meson_nfc_exec_op()' to a special > >>>>> separated function 'meson_nfc_check_op()' which is called >>>>> before commands processing. > >>>>> > >>>>> On 06.06.2023 13:16, Arseniy Krasnov wrote: > >>>>>> Meson NAND controller has limited buffer length, so check it before > >>>>>> command execution to avoid length trim. Also check MTD write size on > >>>>>> chip attach. > >>>>>> > >>>>>> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru> > >>>>>> --- > >>>>>> drivers/mtd/nand/raw/meson_nand.c | 47 >>>>>> +++++++++++++++++++++++++++---- > >>>>>> 1 file changed, 42 insertions(+), 5 deletions(-) > >>>>>> > >>>>>> diff --git a/drivers/mtd/nand/raw/meson_nand.c >>>>>> b/drivers/mtd/nand/raw/meson_nand.c > >>>>>> index 23a73268421b..db6b18753071 100644 > >>>>>> --- a/drivers/mtd/nand/raw/meson_nand.c > >>>>>> +++ b/drivers/mtd/nand/raw/meson_nand.c > >>>>>> @@ -111,6 +111,8 @@ > >>>>>> > >>>>>> #define PER_INFO_BYTE 8 > >>>>>> > >>>>>> +#define NFC_CMD_RAW_LEN GENMASK(13, 0) > >>>>>> + > >>>>>> struct meson_nfc_nand_chip { > >>>>>> struct list_head node; > >>>>>> struct nand_chip nand; > >>>>>> @@ -284,7 +286,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(13, 0)) | scrambler | DMA_DIR(dir); > >>>>>> + cmd = len | scrambler | DMA_DIR(dir); > >>>>>> writel(cmd, nfc->reg_base + NFC_REG_CMD); > >>> > >>> I think we could keep "& GENMASK(13, 0)". it is better here to >>> indicate how many bits of length in this command and don't destory >>> the command once we don't check the "len" outside of this function. >>> or the code "if (len > NFC_CMD_RAW_LEN)" is better to put inside >>> this function nearly. Thanks. > >> > >> It depends who calls this helper. If only used inside >> exec_op,write_page_raw() and read_page_raw() also call >> meson_nfc_cmd_access(), > but i don't find where constrain the "len". > > Is the helper "meson_nfc_check_op()" needed by exec_op() after the constraint in attach_chip()? the length passed in exec_op() seems smaller than "mtd->writesize + mtd->oobsize" now. exec_op() is primarily dedicated to performing side commands than page accesses, typically the parameter page is X bytes, you might want to read it 3 times, depending on the capabilities of the controller, you might need to split the operation or not, so the core checks what are the controller capabilities before doing the operation. So yes, the check_op() thing is necessary. > > @Arseniy if it does need, I think the same constraint is needed by > "meson_nfc_cmd_access()" > > > > >> it's not useful. If used outside, then if you want to clarify > >> things you may want to use: > >> > >> #define NFC_CMD_OP_LEN(cmd) FIELD_PREP(GENMASK(13, 0), (cmd)) > >> > >> This is by far my favorite construction. Could apply to many other > >> fields, like DMA_DIR, scrambler, etc. > > @Miquel, FIELD_PREP() is better, but i have no idea whether we should add FIELD_PREP() in this patch, or keep the previous code. > > >> > >>>>>> return; > >>>>>> } > >>>>>> @@ -573,7 +575,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(13, 0)); > >>>>>> + cmd = NFC_CMD_N2M | len; > >>>>>> writel(cmd, nfc->reg_base + NFC_REG_CMD); > >>>>>> > >>>>>> meson_nfc_drain_cmd(nfc); > >>>>>> @@ -597,7 +599,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(13, 0)); > >>>>>> + cmd = NFC_CMD_M2N | len; > >>>>>> writel(cmd, nfc->reg_base + NFC_REG_CMD); > >>>>>> > >>>>>> meson_nfc_drain_cmd(nfc); > >>>>>> @@ -1007,6 +1009,31 @@ meson_nand_op_put_dma_safe_output_buf(const >>>>>> struct nand_op_instr *instr, > >>>>>> kfree(buf); > >>>>>> } > >>>>>> > >>>>>> +static int meson_nfc_check_op(struct nand_chip *chip, > >>>>>> + const struct nand_operation *op) > >>>>>> +{ > >>>>>> + int op_id; > >>>>>> + > >>>>>> + for (op_id = 0; op_id < op->ninstrs; op_id++) { > >>>>>> + const struct nand_op_instr *instr; > >>>>>> + > >>>>>> + instr = &op->instrs[op_id]; > >>>>>> + > >>>>>> + switch (instr->type) { > >>>>>> + case NAND_OP_DATA_IN_INSTR: > >>>>>> + case NAND_OP_DATA_OUT_INSTR: > >>>>>> + if (instr->ctx.data.len > NFC_CMD_RAW_LEN) > >>>>>> + return -ENOTSUPP; > >>>>>> + > >>>>>> + break; > >>>>>> + default: > >>>>>> + break; > >>>>>> + } > >>>>>> + } > >>>>>> + > >>>>>> + return 0; > >>>>>> +} > >>>>>> + > >>>>>> static int meson_nfc_exec_op(struct nand_chip *nand, > >>>>>> const struct nand_operation *op, bool >>>>>> check_only) > >>>>>> { > >>>>>> @@ -1015,10 +1042,12 @@ static int meson_nfc_exec_op(struct >>>>>> nand_chip *nand, > >>>>>> const struct nand_op_instr *instr = NULL; > >>>>>> void *buf; > >>>>>> u32 op_id, delay_idle, cmd; > >>>>>> + int err; > >>>>>> int i; > >>>>>> > >>>>>> - if (check_only) > >>>>>> - return 0; > >>>>>> + err = meson_nfc_check_op(nand, op); > >>>>>> + if (err || check_only) > >>>>>> + return err; > >>>>>> > >>>>>> meson_nfc_select_chip(nand, op->cs); > >>>>>> for (op_id = 0; op_id < op->ninstrs; op_id++) { > >>>>>> @@ -1293,6 +1322,7 @@ static int meson_nand_attach_chip(struct >>>>>> nand_chip *nand) > >>>>>> struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand); > >>>>>> struct mtd_info *mtd = nand_to_mtd(nand); > >>>>>> int nsectors = mtd->writesize / 1024; > >>>>>> + int raw_writesize; > >>>>>> int ret; > >>>>>> > >>>>>> if (!mtd->name) { > >>>>>> @@ -1304,6 +1334,13 @@ static int meson_nand_attach_chip(struct >>>>>> nand_chip *nand) > >>>>>> return -ENOMEM; > >>>>>> } > >>>>>> > >>>>>> + raw_writesize = mtd->writesize + mtd->oobsize; > >>>>>> + if (raw_writesize > NFC_CMD_RAW_LEN) { > >>>>>> + dev_err(nfc->dev, "too big write size in raw mode: %d >>>>>> > %ld\n", > >>>>>> + raw_writesize, NFC_CMD_RAW_LEN); > >>>>>> + return -EINVAL; > >>>>>> + } > >>>>>> + > >>>>>> if (nand->bbt_options & NAND_BBT_USE_FLASH) > >>>>>> nand->bbt_options |= NAND_BBT_NO_OOB; > >>>>>> > >> > >> > >> Thanks, > >> Miquèl Thanks, Miquèl _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] mtd: rawnand: meson: check buffer length 2023-06-08 13:21 ` Miquel Raynal @ 2023-06-09 7:59 ` Liang Yang 2023-06-09 10:55 ` Arseniy Krasnov 0 siblings, 1 reply; 12+ messages in thread From: Liang Yang @ 2023-06-09 7:59 UTC (permalink / raw) To: Miquel Raynal Cc: Arseniy Krasnov, Richard Weinberger, Vignesh Raghavendra, Neil Armstrong, Kevin Hilman, Jerome Brunet, Martin Blumenstingl, oxffffaa, kernel, linux-mtd, linux-arm-kernel, linux-amlogic, linux-kernel Hi Miquel, On 2023/6/8 21:21, Miquel Raynal wrote: > [ EXTERNAL EMAIL ] > > Hi Liang, > > liang.yang@amlogic.com wrote on Thu, 8 Jun 2023 20:37:14 +0800: > >> Hi Arseniy and Miquel, >> >> On 2023/6/8 15:12, Liang Yang wrote: >>> Hi Miquel, >>> >>> On 2023/6/8 14:54, Miquel Raynal wrote: >>>> [ EXTERNAL EMAIL ] >>>> >>>> Hi Liang, >>>> >>>> liang.yang@amlogic.com wrote on Thu, 8 Jun 2023 14:42:53 +0800: >>>> >>>>> Hi Arseniy, >>>>> >>>>> On 2023/6/8 5:17, Arseniy Krasnov wrote: >>>>>> [ EXTERNAL EMAIL ] >>>>>> >>>>>> Hi again Miquel, Liang! >>>>>> >>>>>> What do You think about this patch? >>>>>> >>>>>> Thanks, Arseniy >>>>>> >>>>>> On 06.06.2023 19:29, Arseniy Krasnov wrote: >>>>>>> Sorry, here is changelog: >>>>>>> v1 -> v2: >>>>>>> * Move checks from 'switch/case' which executes commands in >>>>> 'meson_nfc_exec_op()' to a special >>>>>>> separated function 'meson_nfc_check_op()' which is called >>>>> before commands processing. >>>>>>> >>>>>>> On 06.06.2023 13:16, Arseniy Krasnov wrote: >>>>>>>> Meson NAND controller has limited buffer length, so check it before >>>>>>>> command execution to avoid length trim. Also check MTD write size on >>>>>>>> chip attach. >>>>>>>> >>>>>>>> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru> >>>>>>>> --- >>>>>>>> drivers/mtd/nand/raw/meson_nand.c | 47 >>>>>> +++++++++++++++++++++++++++---- >>>>>>>> 1 file changed, 42 insertions(+), 5 deletions(-) >>>>>>>> >>>>>>>> diff --git a/drivers/mtd/nand/raw/meson_nand.c >>>>>> b/drivers/mtd/nand/raw/meson_nand.c >>>>>>>> index 23a73268421b..db6b18753071 100644 >>>>>>>> --- a/drivers/mtd/nand/raw/meson_nand.c >>>>>>>> +++ b/drivers/mtd/nand/raw/meson_nand.c >>>>>>>> @@ -111,6 +111,8 @@ >>>>>>>> >>>>>>>> #define PER_INFO_BYTE 8 >>>>>>>> >>>>>>>> +#define NFC_CMD_RAW_LEN GENMASK(13, 0) >>>>>>>> + >>>>>>>> struct meson_nfc_nand_chip { >>>>>>>> struct list_head node; >>>>>>>> struct nand_chip nand; >>>>>>>> @@ -284,7 +286,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(13, 0)) | scrambler | DMA_DIR(dir); >>>>>>>> + cmd = len | scrambler | DMA_DIR(dir); >>>>>>>> writel(cmd, nfc->reg_base + NFC_REG_CMD); >>>>> >>>>> I think we could keep "& GENMASK(13, 0)". it is better here to >>> indicate how many bits of length in this command and don't destory >>> the command once we don't check the "len" outside of this function. >>> or the code "if (len > NFC_CMD_RAW_LEN)" is better to put inside >>> this function nearly. Thanks. >>>> >>>> It depends who calls this helper. If only used inside >> exec_op,write_page_raw() and read_page_raw() also call >> meson_nfc_cmd_access(), > but i don't find where constrain the "len". >> >> Is the helper "meson_nfc_check_op()" needed by exec_op() after the constraint in attach_chip()? the length passed in exec_op() seems smaller than "mtd->writesize + mtd->oobsize" now. > > exec_op() is primarily dedicated to performing side commands than page > accesses, typically the parameter page is X bytes, you might want to > read it 3 times, depending on the capabilities of the controller, you > might need to split the operation or not, so the core checks what are > the controller capabilities before doing the operation. So yes, the > check_op() thing is necessary. ok, i get it. thanks @Arseniy I have no other questions about this patch. > >> >> @Arseniy if it does need, I think the same constraint is needed by >> "meson_nfc_cmd_access()" >> >>> >>>> it's not useful. If used outside, then if you want to clarify >>>> things you may want to use: >>>> >>>> #define NFC_CMD_OP_LEN(cmd) FIELD_PREP(GENMASK(13, 0), (cmd)) >>>> >>>> This is by far my favorite construction. Could apply to many other >>>> fields, like DMA_DIR, scrambler, etc. >> >> @Miquel, FIELD_PREP() is better, but i have no idea whether we should add FIELD_PREP() in this patch, or keep the previous code. >> >>>> >>>>>>>> return; >>>>>>>> } >>>>>>>> @@ -573,7 +575,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(13, 0)); >>>>>>>> + cmd = NFC_CMD_N2M | len; >>>>>>>> writel(cmd, nfc->reg_base + NFC_REG_CMD); >>>>>>>> >>>>>>>> meson_nfc_drain_cmd(nfc); >>>>>>>> @@ -597,7 +599,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(13, 0)); >>>>>>>> + cmd = NFC_CMD_M2N | len; >>>>>>>> writel(cmd, nfc->reg_base + NFC_REG_CMD); >>>>>>>> >>>>>>>> meson_nfc_drain_cmd(nfc); >>>>>>>> @@ -1007,6 +1009,31 @@ meson_nand_op_put_dma_safe_output_buf(const >>>>>> struct nand_op_instr *instr, >>>>>>>> kfree(buf); >>>>>>>> } >>>>>>>> >>>>>>>> +static int meson_nfc_check_op(struct nand_chip *chip, >>>>>>>> + const struct nand_operation *op) >>>>>>>> +{ >>>>>>>> + int op_id; >>>>>>>> + >>>>>>>> + for (op_id = 0; op_id < op->ninstrs; op_id++) { >>>>>>>> + const struct nand_op_instr *instr; >>>>>>>> + >>>>>>>> + instr = &op->instrs[op_id]; >>>>>>>> + >>>>>>>> + switch (instr->type) { >>>>>>>> + case NAND_OP_DATA_IN_INSTR: >>>>>>>> + case NAND_OP_DATA_OUT_INSTR: >>>>>>>> + if (instr->ctx.data.len > NFC_CMD_RAW_LEN) >>>>>>>> + return -ENOTSUPP; >>>>>>>> + >>>>>>>> + break; >>>>>>>> + default: >>>>>>>> + break; >>>>>>>> + } >>>>>>>> + } >>>>>>>> + >>>>>>>> + return 0; >>>>>>>> +} >>>>>>>> + >>>>>>>> static int meson_nfc_exec_op(struct nand_chip *nand, >>>>>>>> const struct nand_operation *op, bool >>>>>> check_only) >>>>>>>> { >>>>>>>> @@ -1015,10 +1042,12 @@ static int meson_nfc_exec_op(struct >>>>>> nand_chip *nand, >>>>>>>> const struct nand_op_instr *instr = NULL; >>>>>>>> void *buf; >>>>>>>> u32 op_id, delay_idle, cmd; >>>>>>>> + int err; >>>>>>>> int i; >>>>>>>> >>>>>>>> - if (check_only) >>>>>>>> - return 0; >>>>>>>> + err = meson_nfc_check_op(nand, op); >>>>>>>> + if (err || check_only) >>>>>>>> + return err; >>>>>>>> >>>>>>>> meson_nfc_select_chip(nand, op->cs); >>>>>>>> for (op_id = 0; op_id < op->ninstrs; op_id++) { >>>>>>>> @@ -1293,6 +1322,7 @@ static int meson_nand_attach_chip(struct >>>>>> nand_chip *nand) >>>>>>>> struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand); >>>>>>>> struct mtd_info *mtd = nand_to_mtd(nand); >>>>>>>> int nsectors = mtd->writesize / 1024; >>>>>>>> + int raw_writesize; >>>>>>>> int ret; >>>>>>>> >>>>>>>> if (!mtd->name) { >>>>>>>> @@ -1304,6 +1334,13 @@ static int meson_nand_attach_chip(struct >>>>>> nand_chip *nand) >>>>>>>> return -ENOMEM; >>>>>>>> } >>>>>>>> >>>>>>>> + raw_writesize = mtd->writesize + mtd->oobsize; >>>>>>>> + if (raw_writesize > NFC_CMD_RAW_LEN) { >>>>>>>> + dev_err(nfc->dev, "too big write size in raw mode: %d >>>>>> > %ld\n", >>>>>>>> + raw_writesize, NFC_CMD_RAW_LEN); >>>>>>>> + return -EINVAL; >>>>>>>> + } >>>>>>>> + >>>>>>>> if (nand->bbt_options & NAND_BBT_USE_FLASH) >>>>>>>> nand->bbt_options |= NAND_BBT_NO_OOB; >>>>>>>> >>>> >>>> >>>> Thanks, >>>> Miquèl > > > Thanks, > Miquèl _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] mtd: rawnand: meson: check buffer length 2023-06-09 7:59 ` Liang Yang @ 2023-06-09 10:55 ` Arseniy Krasnov 0 siblings, 0 replies; 12+ messages in thread From: Arseniy Krasnov @ 2023-06-09 10:55 UTC (permalink / raw) To: Liang Yang, Miquel Raynal Cc: Richard Weinberger, Vignesh Raghavendra, Neil Armstrong, Kevin Hilman, Jerome Brunet, Martin Blumenstingl, oxffffaa, kernel, linux-mtd, linux-arm-kernel, linux-amlogic, linux-kernel On 09.06.2023 10:59, Liang Yang wrote: > Hi Miquel, > > On 2023/6/8 21:21, Miquel Raynal wrote: >> [ EXTERNAL EMAIL ] >> >> Hi Liang, >> >> liang.yang@amlogic.com wrote on Thu, 8 Jun 2023 20:37:14 +0800: >> >>> Hi Arseniy and Miquel, >>> >>> On 2023/6/8 15:12, Liang Yang wrote: >>>> Hi Miquel, >>>> >>>> On 2023/6/8 14:54, Miquel Raynal wrote: >>>>> [ EXTERNAL EMAIL ] >>>>> >>>>> Hi Liang, >>>>> >>>>> liang.yang@amlogic.com wrote on Thu, 8 Jun 2023 14:42:53 +0800: >>>>> >>>>>> Hi Arseniy, >>>>>> >>>>>> On 2023/6/8 5:17, Arseniy Krasnov wrote: >>>>>>> [ EXTERNAL EMAIL ] >>>>>>> >>>>>>> Hi again Miquel, Liang! >>>>>>> >>>>>>> What do You think about this patch? >>>>>>> >>>>>>> Thanks, Arseniy >>>>>>> >>>>>>> On 06.06.2023 19:29, Arseniy Krasnov wrote: >>>>>>>> Sorry, here is changelog: >>>>>>>> v1 -> v2: >>>>>>>> * Move checks from 'switch/case' which executes commands in >>>>> 'meson_nfc_exec_op()' to a special >>>>>>>> separated function 'meson_nfc_check_op()' which is called >>>>> before commands processing. >>>>>>>> >>>>>>>> On 06.06.2023 13:16, Arseniy Krasnov wrote: >>>>>>>>> Meson NAND controller has limited buffer length, so check it before >>>>>>>>> command execution to avoid length trim. Also check MTD write size on >>>>>>>>> chip attach. >>>>>>>>> >>>>>>>>> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru> >>>>>>>>> --- >>>>>>>>> drivers/mtd/nand/raw/meson_nand.c | 47 >>>>>> +++++++++++++++++++++++++++---- >>>>>>>>> 1 file changed, 42 insertions(+), 5 deletions(-) >>>>>>>>> >>>>>>>>> diff --git a/drivers/mtd/nand/raw/meson_nand.c >>>>>> b/drivers/mtd/nand/raw/meson_nand.c >>>>>>>>> index 23a73268421b..db6b18753071 100644 >>>>>>>>> --- a/drivers/mtd/nand/raw/meson_nand.c >>>>>>>>> +++ b/drivers/mtd/nand/raw/meson_nand.c >>>>>>>>> @@ -111,6 +111,8 @@ >>>>>>>>> >>>>>>>>> #define PER_INFO_BYTE 8 >>>>>>>>> >>>>>>>>> +#define NFC_CMD_RAW_LEN GENMASK(13, 0) >>>>>>>>> + >>>>>>>>> struct meson_nfc_nand_chip { >>>>>>>>> struct list_head node; >>>>>>>>> struct nand_chip nand; >>>>>>>>> @@ -284,7 +286,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(13, 0)) | scrambler | DMA_DIR(dir); >>>>>>>>> + cmd = len | scrambler | DMA_DIR(dir); >>>>>>>>> writel(cmd, nfc->reg_base + NFC_REG_CMD); >>>>>> >>>>>> I think we could keep "& GENMASK(13, 0)". it is better here to >>> indicate how many bits of length in this command and don't destory >>> the command once we don't check the "len" outside of this function. >>> or the code "if (len > NFC_CMD_RAW_LEN)" is better to put inside >>> this function nearly. Thanks. >>>>> >>>>> It depends who calls this helper. If only used inside >> exec_op,write_page_raw() and read_page_raw() also call >> meson_nfc_cmd_access(), > but i don't find where constrain the "len". >>> >>> Is the helper "meson_nfc_check_op()" needed by exec_op() after the constraint in attach_chip()? the length passed in exec_op() seems smaller than "mtd->writesize + mtd->oobsize" now. >> >> exec_op() is primarily dedicated to performing side commands than page >> accesses, typically the parameter page is X bytes, you might want to >> read it 3 times, depending on the capabilities of the controller, you >> might need to split the operation or not, so the core checks what are >> the controller capabilities before doing the operation. So yes, the >> check_op() thing is necessary. > > ok, i get it. thanks > > @Arseniy I have no other questions about this patch. Got it! Thanks, Arseniy >> >>> >>> @Arseniy if it does need, I think the same constraint is needed by >>> "meson_nfc_cmd_access()" >>> >>>> >>>>> it's not useful. If used outside, then if you want to clarify >>>>> things you may want to use: >>>>> >>>>> #define NFC_CMD_OP_LEN(cmd) FIELD_PREP(GENMASK(13, 0), (cmd)) >>>>> >>>>> This is by far my favorite construction. Could apply to many other >>>>> fields, like DMA_DIR, scrambler, etc. >>> >>> @Miquel, FIELD_PREP() is better, but i have no idea whether we should add FIELD_PREP() in this patch, or keep the previous code. >>> >>>>> >>>>>>>>> return; >>>>>>>>> } >>>>>>>>> @@ -573,7 +575,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(13, 0)); >>>>>>>>> + cmd = NFC_CMD_N2M | len; >>>>>>>>> writel(cmd, nfc->reg_base + NFC_REG_CMD); >>>>>>>>> >>>>>>>>> meson_nfc_drain_cmd(nfc); >>>>>>>>> @@ -597,7 +599,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(13, 0)); >>>>>>>>> + cmd = NFC_CMD_M2N | len; >>>>>>>>> writel(cmd, nfc->reg_base + NFC_REG_CMD); >>>>>>>>> >>>>>>>>> meson_nfc_drain_cmd(nfc); >>>>>>>>> @@ -1007,6 +1009,31 @@ meson_nand_op_put_dma_safe_output_buf(const >>>>>> struct nand_op_instr *instr, >>>>>>>>> kfree(buf); >>>>>>>>> } >>>>>>>>> >>>>>>>>> +static int meson_nfc_check_op(struct nand_chip *chip, >>>>>>>>> + const struct nand_operation *op) >>>>>>>>> +{ >>>>>>>>> + int op_id; >>>>>>>>> + >>>>>>>>> + for (op_id = 0; op_id < op->ninstrs; op_id++) { >>>>>>>>> + const struct nand_op_instr *instr; >>>>>>>>> + >>>>>>>>> + instr = &op->instrs[op_id]; >>>>>>>>> + >>>>>>>>> + switch (instr->type) { >>>>>>>>> + case NAND_OP_DATA_IN_INSTR: >>>>>>>>> + case NAND_OP_DATA_OUT_INSTR: >>>>>>>>> + if (instr->ctx.data.len > NFC_CMD_RAW_LEN) >>>>>>>>> + return -ENOTSUPP; >>>>>>>>> + >>>>>>>>> + break; >>>>>>>>> + default: >>>>>>>>> + break; >>>>>>>>> + } >>>>>>>>> + } >>>>>>>>> + >>>>>>>>> + return 0; >>>>>>>>> +} >>>>>>>>> + >>>>>>>>> static int meson_nfc_exec_op(struct nand_chip *nand, >>>>>>>>> const struct nand_operation *op, bool >>>>>> check_only) >>>>>>>>> { >>>>>>>>> @@ -1015,10 +1042,12 @@ static int meson_nfc_exec_op(struct >>>>>> nand_chip *nand, >>>>>>>>> const struct nand_op_instr *instr = NULL; >>>>>>>>> void *buf; >>>>>>>>> u32 op_id, delay_idle, cmd; >>>>>>>>> + int err; >>>>>>>>> int i; >>>>>>>>> >>>>>>>>> - if (check_only) >>>>>>>>> - return 0; >>>>>>>>> + err = meson_nfc_check_op(nand, op); >>>>>>>>> + if (err || check_only) >>>>>>>>> + return err; >>>>>>>>> >>>>>>>>> meson_nfc_select_chip(nand, op->cs); >>>>>>>>> for (op_id = 0; op_id < op->ninstrs; op_id++) { >>>>>>>>> @@ -1293,6 +1322,7 @@ static int meson_nand_attach_chip(struct >>>>>> nand_chip *nand) >>>>>>>>> struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand); >>>>>>>>> struct mtd_info *mtd = nand_to_mtd(nand); >>>>>>>>> int nsectors = mtd->writesize / 1024; >>>>>>>>> + int raw_writesize; >>>>>>>>> int ret; >>>>>>>>> >>>>>>>>> if (!mtd->name) { >>>>>>>>> @@ -1304,6 +1334,13 @@ static int meson_nand_attach_chip(struct >>>>>> nand_chip *nand) >>>>>>>>> return -ENOMEM; >>>>>>>>> } >>>>>>>>> >>>>>>>>> + raw_writesize = mtd->writesize + mtd->oobsize; >>>>>>>>> + if (raw_writesize > NFC_CMD_RAW_LEN) { >>>>>>>>> + dev_err(nfc->dev, "too big write size in raw mode: %d >>>>>> > %ld\n", >>>>>>>>> + raw_writesize, NFC_CMD_RAW_LEN); >>>>>>>>> + return -EINVAL; >>>>>>>>> + } >>>>>>>>> + >>>>>>>>> if (nand->bbt_options & NAND_BBT_USE_FLASH) >>>>>>>>> nand->bbt_options |= NAND_BBT_NO_OOB; >>>>>>>>> >>>>> >>>>> >>>>> Thanks, >>>>> Miquèl >> >> >> Thanks, >> Miquèl _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2023-06-09 11:00 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-06-06 10:16 [PATCH v2] mtd: rawnand: meson: check buffer length Arseniy Krasnov 2023-06-06 16:29 ` Arseniy Krasnov 2023-06-07 21:17 ` Arseniy Krasnov 2023-06-08 6:10 ` Miquel Raynal 2023-06-08 6:08 ` Arseniy Krasnov 2023-06-08 6:42 ` Liang Yang 2023-06-08 6:54 ` Miquel Raynal 2023-06-08 7:12 ` Liang Yang 2023-06-08 12:37 ` Liang Yang 2023-06-08 13:21 ` Miquel Raynal 2023-06-09 7:59 ` Liang Yang 2023-06-09 10:55 ` Arseniy Krasnov
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).