Re: nand_op_parser_exec_op should use longest pattern

[Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>]

On Fri, 29 Mar 2019 14:35:13 +0100
Sascha Hauer <s.hauer@xxxxxxxxxxxxxx> wrote:

> Hi All,
> 
> I just played with the new exec_op interface for the first time and
> together with Boris we found a problem in the pattern table parser.
> 
> The vf610 driver uses this pattern table:
> 
> static const struct nand_op_parser vf610_nfc_op_parser = NAND_OP_PARSER(
> 	NAND_OP_PARSER_PATTERN(vf610_nfc_cmd,
> 		NAND_OP_PARSER_PAT_CMD_ELEM(true),
> 		NAND_OP_PARSER_PAT_ADDR_ELEM(true, 5),
> 		NAND_OP_PARSER_PAT_DATA_OUT_ELEM(true, PAGE_2K + OOB_MAX),
> 		NAND_OP_PARSER_PAT_CMD_ELEM(true),
> 		NAND_OP_PARSER_PAT_WAITRDY_ELEM(true)),
> 	NAND_OP_PARSER_PATTERN(vf610_nfc_cmd,
> 		NAND_OP_PARSER_PAT_CMD_ELEM(true),
> 		NAND_OP_PARSER_PAT_ADDR_ELEM(true, 5),
> 		NAND_OP_PARSER_PAT_CMD_ELEM(true),
> 		NAND_OP_PARSER_PAT_WAITRDY_ELEM(true),
> 		NAND_OP_PARSER_PAT_DATA_IN_ELEM(true, PAGE_2K + OOB_MAX)),
> 	);
> 
> It has two patterns, one supposed for writing and one for reading. All elements
> are optional. Now with a typical page read we'll get this:
> 
> [   33.932464] nand:     ->CMD      [0x00]
> [   33.936338] nand:     ->ADDR     [5 cyc: 00 00 00 0a 00]
> [   33.941755] nand:     ->CMD      [0x30]
> [   33.945628] nand:     ->WAITRDY  [max 1 ms]
> [   33.949909] nand:       DATA_IN  [2176 B]
> 
> Only the first four elements are executed in one go, the fifth is
> exectuted separately. This is because the pattern table parser finds
> that the first pattern (supposed for writing) already matches for the
> first four elements and then uses it instead of realizing that the
> second pattern matches the whole operation.
> 
> I have no fix for this, just wanted to let you know. It turned out that
> in my case for the GPMI nand driver I probably won't need any pattern
> table.

This is a completely untested fix, just in case someone has time to
test/debug it.

--->8---
diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
index ddd396e93e32..e0ba9f0b1fdc 100644
--- a/drivers/mtd/nand/raw/nand_base.c
+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -2165,30 +2165,38 @@ int nand_op_parser_exec_op(struct nand_chip *chip,
        unsigned int i;
 
        while (ctx.subop.instrs < op->instrs + op->ninstrs) {
-               int ret;
+               const struct nand_op_parser_pattern *pattern;
+               struct nand_op_parser_ctx best_ctx;
+               int ret, best_pattern = -1;
 
                for (i = 0; i < parser->npatterns; i++) {
-                       const struct nand_op_parser_pattern *pattern;
+                       struct nand_op_parser_ctx test_ctx = ctx;
 
-                       pattern = &parser->patterns[i];
-                       if (!nand_op_parser_match_pat(pattern, &ctx))
+                       if (!nand_op_parser_match_pat(pattern, &test_ctx))
                                continue;
 
-                       nand_op_parser_trace(&ctx);
+                       if (best_pattern >= 0 &&
+                           (test_ctx.subop.ninstrs < best_ctx.subop.ninstrs ||
+                            (test_ctx.subop.ninstrs == best_ctx.subop.ninstrs &&
+                             test_ctx.last_instr_end_off <= best_ctx.last_instr_end_off)))
+                               continue;
 
-                       if (check_only)
-                               break;
+                       best_pattern = i;
+                       best_ctx = test_ctx;
+               }
 
+               if (best_pattern < 0) {
+                       pr_debug("->exec_op() parser: pattern not found!\n");
+                       return -ENOTSUPP;
+               }
+
+               ctx = best_ctx;
+               nand_op_parser_trace(&ctx);
+
+               if (!check_only) {
                        ret = pattern->exec(chip, &ctx.subop);
                        if (ret)
                                return ret;
-
-                       break;
-               }
-
-               if (i == parser->npatterns) {
-                       pr_debug("->exec_op() parser: pattern not found!\n");
-                       return -ENOTSUPP;
                }
 
                /*

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/