Re: [PATCH v2] mtd: rawnand: meson: check buffer length
From: Liang Yang
Date: Thu Jun 08 2023 - 08:38:07 EST
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@xxxxxxxxxxx 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@xxxxxxxxxxxxxx>
---
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