Re: nand_op_parser_exec_op should use longest pattern

[Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>]

+Miquel

On Fri, 29 Mar 2019 15:37:56 +0100
Stefan Agner <stefan@xxxxxxxx> wrote:

> On 29.03.2019 14:35, Sascha Hauer 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.  
> 
> Hm, I do not remember noticing that during development. I wonder if it
> was the case already back then.
> 
> If yes, it did not seem to have a negative impact on performance
> compared to the old interface:
> https://linux-mtd.infradead.narkive.com/qZgEnPsC/patch-v6-0-3-mtd-rawnand-vf610-nfc-make-use-of-exec-op
> 
> > 
> > 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.  
> 
> Thanks for bringing it up! Will try it out when I come around.

Here is a new version of the proposed fix that compiles, at
least :-). Still not tested tested on a real HW though.

--->8---
diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
index ddd396e93e32..7a5178b5f13e 100644
--- a/drivers/mtd/nand/raw/nand_base.c
+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -2131,6 +2131,23 @@ static void nand_op_parser_trace(const struct nand_op_parser_ctx *ctx)
 }
 #endif
 
+static int nand_op_parser_cmp_ctx(const struct nand_op_parser_ctx *a,
+                                 const struct nand_op_parser_ctx *b)
+{
+
+       if (a->subop.ninstrs < b->subop.ninstrs)
+               return -1;
+       else if (a->subop.ninstrs > b->subop.ninstrs)
+               return 1;
+
+       if (a->subop.last_instr_end_off < b->subop.last_instr_end_off)
+               return -1;
+       else if (a->subop.last_instr_end_off > b->subop.last_instr_end_off)
+               return 1;
+
+       return 0;
+}
+
 /**
  * nand_op_parser_exec_op - exec_op parser
  * @chip: the NAND chip
@@ -2165,30 +2182,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 &&
+                           nand_op_parser_cmp_ctx(&test_ctx, &best_ctx) <= 0)
+                               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) {
+                       pattern = &parser->patterns[best_pattern];
                        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/