| Message ID | 20230310085452.1368716-1-miquel.raynal@bootlin.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | mtd: rawnand: nandsim: Artificially prevent sequential page reads | expand |
Hi Miquel, > The continuous read support added recently makes nandsim > unhappy. Indeed, all the supported commands should be re-encoded into > internal commands, so of course there is currently no support for the > commands and patterns needed for continuous reads to work. > > I tried to add support for them but nandsim (which is more a tool to > develop/debug upper layers rather than the raw NAND core) suffers from a > big limitation: it's internal parser needs to know what exact operation > is happening when the address cycles are performed. The research is then > sequential from the start up to the address cycles, but does not check > what's coming next even though the information is available. This is a > limitation which is related to the old API used by the core which kind > of forced the controllers to guess what operation was being performed > rather early. Today the core uses a more transparent API called > ->exec_op() which no longer requires controller drivers to do any more > guessing, but despite being updated to ->exec_op(), nandsim is still a > bit constrained on this regard and thus cannot handle sequential page > reads because the start sequence beginning is identical to a regular > page read. > > If the internal algorithm is updated some day, it should be possible to > make it support sequential page reads by adding something like: > > /* Large page devices continuous read page start */ > {OPT_LARGEPAGE, {STATE_CMD_READ0, STATE_ADDR_PAGE, STATE_CMD_READSTART, > STATE_CMD_READCACHESEQ | ACTION_CPY, STATE_DATAOUT, > STATE_READY}}, > /* Large page devices continuous read page continue */ > {OPT_LARGEPAGE, {STATE_CMD_READCACHESEQ | ACTION_CPY_NEXT, STATE_DATAOUT, > STATE_READY}}, > /* Large page devices continuous read page end */ > {OPT_LARGEPAGE, {STATE_CMD_READCACHEEND | ACTION_CPY_NEXT, STATE_DATAOUT, > STATE_READY}}, > > For now, we just return -EOPNOTSUPP when the core asks controller > drivers if they support the feature in order to prevent any further use > of these opcodes. > > Note: This is a hack, ->exec_op() is not supposed to check against the > COMMAND opcodes unless _really_ needed. > > Fixes: 003fe4b9545b ("mtd: rawnand: Support for sequential cache reads") > Reported-by: Zhihao Cheng <chengzhihao1@huawei.com> > Link: https://lore.kernel.org/linux-mtd/fd34fe55-7f4a-030d-8653-9bb9cf08410d@huawei.com/ > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > --- > drivers/mtd/nand/raw/nandsim.c | 17 ++++++++++++++++- > 1 file changed, 16 insertions(+), 1 deletion(-) > This patch works! Thanks for you help. Tested-by: Zhihao Cheng <chengzhihao1@huawei.com> > diff --git a/drivers/mtd/nand/raw/nandsim.c b/drivers/mtd/nand/raw/nandsim.c > index c21abf748948..179b28459b4b 100644 > --- a/drivers/mtd/nand/raw/nandsim.c > +++ b/drivers/mtd/nand/raw/nandsim.c > @@ -2160,8 +2160,23 @@ static int ns_exec_op(struct nand_chip *chip, const struct nand_operation *op, > const struct nand_op_instr *instr = NULL; > struct nandsim *ns = nand_get_controller_data(chip); > > - if (check_only) > + if (check_only) { > + /* The current implementation of nandsim needs to know the > + * ongoing operation when performing the address cycles. This > + * means it cannot make the difference between a regular read > + * and a continuous read. Hence, this hack to manually refuse > + * supporting sequential cached operations. > + */ > + for (op_id = 0; op_id < op->ninstrs; op_id++) { > + instr = &op->instrs[op_id]; > + if (instr->type == NAND_OP_CMD_INSTR && > + (instr->ctx.cmd.opcode == NAND_CMD_READCACHEEND || > + instr->ctx.cmd.opcode == NAND_CMD_READCACHESEQ)) > + return -EOPNOTSUPP; > + } > + > return 0; > + } > > ns->lines.ce = 1; > >
----- Ursprüngliche Mail ----- > Von: "Miquel Raynal" <miquel.raynal@bootlin.com> > The continuous read support added recently makes nandsim > unhappy. Indeed, all the supported commands should be re-encoded into > internal commands, so of course there is currently no support for the > commands and patterns needed for continuous reads to work. > > I tried to add support for them but nandsim (which is more a tool to > develop/debug upper layers rather than the raw NAND core) suffers from a > big limitation: it's internal parser needs to know what exact operation > is happening when the address cycles are performed. The research is then > sequential from the start up to the address cycles, but does not check > what's coming next even though the information is available. This is a > limitation which is related to the old API used by the core which kind > of forced the controllers to guess what operation was being performed > rather early. Today the core uses a more transparent API called > ->exec_op() which no longer requires controller drivers to do any more > guessing, but despite being updated to ->exec_op(), nandsim is still a > bit constrained on this regard and thus cannot handle sequential page > reads because the start sequence beginning is identical to a regular > page read. > > If the internal algorithm is updated some day, it should be possible to > make it support sequential page reads by adding something like: > > /* Large page devices continuous read page start */ > {OPT_LARGEPAGE, {STATE_CMD_READ0, STATE_ADDR_PAGE, STATE_CMD_READSTART, > STATE_CMD_READCACHESEQ | ACTION_CPY, STATE_DATAOUT, > STATE_READY}}, > /* Large page devices continuous read page continue */ > {OPT_LARGEPAGE, {STATE_CMD_READCACHESEQ | ACTION_CPY_NEXT, STATE_DATAOUT, > STATE_READY}}, > /* Large page devices continuous read page end */ > {OPT_LARGEPAGE, {STATE_CMD_READCACHEEND | ACTION_CPY_NEXT, STATE_DATAOUT, > STATE_READY}}, > > For now, we just return -EOPNOTSUPP when the core asks controller > drivers if they support the feature in order to prevent any further use > of these opcodes. > > Note: This is a hack, ->exec_op() is not supposed to check against the > COMMAND opcodes unless _really_ needed. > > Fixes: 003fe4b9545b ("mtd: rawnand: Support for sequential cache reads") > Reported-by: Zhihao Cheng <chengzhihao1@huawei.com> > Link: > https://lore.kernel.org/linux-mtd/fd34fe55-7f4a-030d-8653-9bb9cf08410d@huawei.com/ > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > --- > drivers/mtd/nand/raw/nandsim.c | 17 ++++++++++++++++- > 1 file changed, 16 insertions(+), 1 deletion(-) > > diff --git a/drivers/mtd/nand/raw/nandsim.c b/drivers/mtd/nand/raw/nandsim.c > index c21abf748948..179b28459b4b 100644 > --- a/drivers/mtd/nand/raw/nandsim.c > +++ b/drivers/mtd/nand/raw/nandsim.c > @@ -2160,8 +2160,23 @@ static int ns_exec_op(struct nand_chip *chip, const > struct nand_operation *op, > const struct nand_op_instr *instr = NULL; > struct nandsim *ns = nand_get_controller_data(chip); > > - if (check_only) > + if (check_only) { > + /* The current implementation of nandsim needs to know the > + * ongoing operation when performing the address cycles. This > + * means it cannot make the difference between a regular read > + * and a continuous read. Hence, this hack to manually refuse > + * supporting sequential cached operations. > + */ > + for (op_id = 0; op_id < op->ninstrs; op_id++) { > + instr = &op->instrs[op_id]; > + if (instr->type == NAND_OP_CMD_INSTR && > + (instr->ctx.cmd.opcode == NAND_CMD_READCACHEEND || > + instr->ctx.cmd.opcode == NAND_CMD_READCACHESEQ)) > + return -EOPNOTSUPP; > + } > + > return 0; > + } > > ns->lines.ce = 1; Acked-by: Richard Weinberger <richard@nod.at> Thanks, //richard
On Fri, 2023-03-10 at 08:54:52 UTC, Miquel Raynal wrote: > The continuous read support added recently makes nandsim > unhappy. Indeed, all the supported commands should be re-encoded into > internal commands, so of course there is currently no support for the > commands and patterns needed for continuous reads to work. > > I tried to add support for them but nandsim (which is more a tool to > develop/debug upper layers rather than the raw NAND core) suffers from a > big limitation: it's internal parser needs to know what exact operation > is happening when the address cycles are performed. The research is then > sequential from the start up to the address cycles, but does not check > what's coming next even though the information is available. This is a > limitation which is related to the old API used by the core which kind > of forced the controllers to guess what operation was being performed > rather early. Today the core uses a more transparent API called > ->exec_op() which no longer requires controller drivers to do any more > guessing, but despite being updated to ->exec_op(), nandsim is still a > bit constrained on this regard and thus cannot handle sequential page > reads because the start sequence beginning is identical to a regular > page read. > > If the internal algorithm is updated some day, it should be possible to > make it support sequential page reads by adding something like: > > /* Large page devices continuous read page start */ > {OPT_LARGEPAGE, {STATE_CMD_READ0, STATE_ADDR_PAGE, STATE_CMD_READSTART, > STATE_CMD_READCACHESEQ | ACTION_CPY, STATE_DATAOUT, > STATE_READY}}, > /* Large page devices continuous read page continue */ > {OPT_LARGEPAGE, {STATE_CMD_READCACHESEQ | ACTION_CPY_NEXT, STATE_DATAOUT, > STATE_READY}}, > /* Large page devices continuous read page end */ > {OPT_LARGEPAGE, {STATE_CMD_READCACHEEND | ACTION_CPY_NEXT, STATE_DATAOUT, > STATE_READY}}, > > For now, we just return -EOPNOTSUPP when the core asks controller > drivers if they support the feature in order to prevent any further use > of these opcodes. > > Note: This is a hack, ->exec_op() is not supposed to check against the > COMMAND opcodes unless _really_ needed. > > Fixes: 003fe4b9545b ("mtd: rawnand: Support for sequential cache reads") > Reported-by: Zhihao Cheng <chengzhihao1@huawei.com> > Link: https://lore.kernel.org/linux-mtd/fd34fe55-7f4a-030d-8653-9bb9cf08410d@huawei.com/ > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > Tested-by: Zhihao Cheng <chengzhihao1@huawei.com> > Acked-by: Richard Weinberger <richard@nod.at> Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/fixes. Miquel
diff --git a/drivers/mtd/nand/raw/nandsim.c b/drivers/mtd/nand/raw/nandsim.c index c21abf748948..179b28459b4b 100644 --- a/drivers/mtd/nand/raw/nandsim.c +++ b/drivers/mtd/nand/raw/nandsim.c @@ -2160,8 +2160,23 @@ static int ns_exec_op(struct nand_chip *chip, const struct nand_operation *op, const struct nand_op_instr *instr = NULL; struct nandsim *ns = nand_get_controller_data(chip); - if (check_only) + if (check_only) { + /* The current implementation of nandsim needs to know the + * ongoing operation when performing the address cycles. This + * means it cannot make the difference between a regular read + * and a continuous read. Hence, this hack to manually refuse + * supporting sequential cached operations. + */ + for (op_id = 0; op_id < op->ninstrs; op_id++) { + instr = &op->instrs[op_id]; + if (instr->type == NAND_OP_CMD_INSTR && + (instr->ctx.cmd.opcode == NAND_CMD_READCACHEEND || + instr->ctx.cmd.opcode == NAND_CMD_READCACHESEQ)) + return -EOPNOTSUPP; + } + return 0; + } ns->lines.ce = 1;
The continuous read support added recently makes nandsim unhappy. Indeed, all the supported commands should be re-encoded into internal commands, so of course there is currently no support for the commands and patterns needed for continuous reads to work. I tried to add support for them but nandsim (which is more a tool to develop/debug upper layers rather than the raw NAND core) suffers from a big limitation: it's internal parser needs to know what exact operation is happening when the address cycles are performed. The research is then sequential from the start up to the address cycles, but does not check what's coming next even though the information is available. This is a limitation which is related to the old API used by the core which kind of forced the controllers to guess what operation was being performed rather early. Today the core uses a more transparent API called ->exec_op() which no longer requires controller drivers to do any more guessing, but despite being updated to ->exec_op(), nandsim is still a bit constrained on this regard and thus cannot handle sequential page reads because the start sequence beginning is identical to a regular page read. If the internal algorithm is updated some day, it should be possible to make it support sequential page reads by adding something like: /* Large page devices continuous read page start */ {OPT_LARGEPAGE, {STATE_CMD_READ0, STATE_ADDR_PAGE, STATE_CMD_READSTART, STATE_CMD_READCACHESEQ | ACTION_CPY, STATE_DATAOUT, STATE_READY}}, /* Large page devices continuous read page continue */ {OPT_LARGEPAGE, {STATE_CMD_READCACHESEQ | ACTION_CPY_NEXT, STATE_DATAOUT, STATE_READY}}, /* Large page devices continuous read page end */ {OPT_LARGEPAGE, {STATE_CMD_READCACHEEND | ACTION_CPY_NEXT, STATE_DATAOUT, STATE_READY}}, For now, we just return -EOPNOTSUPP when the core asks controller drivers if they support the feature in order to prevent any further use of these opcodes. Note: This is a hack, ->exec_op() is not supposed to check against the COMMAND opcodes unless _really_ needed. Fixes: 003fe4b9545b ("mtd: rawnand: Support for sequential cache reads") Reported-by: Zhihao Cheng <chengzhihao1@huawei.com> Link: https://lore.kernel.org/linux-mtd/fd34fe55-7f4a-030d-8653-9bb9cf08410d@huawei.com/ Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> --- drivers/mtd/nand/raw/nandsim.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)