diff mbox series

mtd: rawnand: use longest matching pattern

Message ID 20190419074717.22576-1-stefan@agner.ch
State Accepted
Delegated to: Miquel Raynal
Headers show
Series mtd: rawnand: use longest matching pattern | expand

Commit Message

Stefan Agner April 19, 2019, 7:47 a.m. UTC 
Sometimes the exec_op parser does not choose the optimal pattern if
multiple patterns with optional elements are available. Since the stack
automatically splits operations in multiple exec_op calls, a non-optimal
pattern gets broken up into multiple calls. E.g. an OOB read using the
vf610 driver:
  nand: executing subop:
  nand:     ->CMD      [0x00]
  nand:     ->ADDR     [5 cyc: 00 08 ea 94 02]
  nand:     ->CMD      [0x30]
  nand:     ->WAITRDY  [max 200000 ms]
  nand:       DATA_IN  [64 B]
  nand: executing subop:
  nand:       CMD      [0x00]
  nand:       ADDR     [5 cyc: 00 08 ea 94 02]
  nand:       CMD      [0x30]
  nand:       WAITRDY  [max 200000 ms]
  nand:     ->DATA_IN  [64 B]

However, the vf610 driver has a pattern which can execute the complete
command in a single go...

This patch makes sure that the longest matching pattern is chosen
instead of the first (potentially only partial) match. With this
change the vf610 reads the OOB in a single exec_op call:
  nand: executing subop:
  nand:     ->CMD      [0x00]
  nand:     ->ADDR     [5 cyc: 00 08 c0 1d 00]
  nand:     ->CMD      [0x30]
  nand:     ->WAITRDY  [max 200000 ms]
  nand:     ->DATA_IN  [64 B]

Reported-by: Sascha Hauer <s.hauer@pengutronix.de>
Suggested-by: Boris Brezillon <boris.brezillon@collabora.com>
Tested-by: Stefan Agner <stefan@agner.ch>
Signed-off-by: Stefan Agner <stefan@agner.ch>
---
 drivers/mtd/nand/raw/nand_base.c | 50 +++++++++++++++++++++++---------
 1 file changed, 37 insertions(+), 13 deletions(-)

Comments

Miquel Raynal May 20, 2019, 2:31 p.m. UTC | #1 
Hi Stefan,

Stefan Agner <stefan@agner.ch> wrote on Fri, 19 Apr 2019 09:47:17 +0200:

> Sometimes the exec_op parser does not choose the optimal pattern if
> multiple patterns with optional elements are available. Since the stack
> automatically splits operations in multiple exec_op calls, a non-optimal
> pattern gets broken up into multiple calls. E.g. an OOB read using the
> vf610 driver:
>   nand: executing subop:
>   nand:     ->CMD      [0x00]
>   nand:     ->ADDR     [5 cyc: 00 08 ea 94 02]
>   nand:     ->CMD      [0x30]
>   nand:     ->WAITRDY  [max 200000 ms]
>   nand:       DATA_IN  [64 B]
>   nand: executing subop:
>   nand:       CMD      [0x00]
>   nand:       ADDR     [5 cyc: 00 08 ea 94 02]
>   nand:       CMD      [0x30]
>   nand:       WAITRDY  [max 200000 ms]
>   nand:     ->DATA_IN  [64 B]
> 
> However, the vf610 driver has a pattern which can execute the complete
> command in a single go...
> 
> This patch makes sure that the longest matching pattern is chosen
> instead of the first (potentially only partial) match. With this
> change the vf610 reads the OOB in a single exec_op call:
>   nand: executing subop:
>   nand:     ->CMD      [0x00]
>   nand:     ->ADDR     [5 cyc: 00 08 c0 1d 00]
>   nand:     ->CMD      [0x30]
>   nand:     ->WAITRDY  [max 200000 ms]
>   nand:     ->DATA_IN  [64 B]
> 
> Reported-by: Sascha Hauer <s.hauer@pengutronix.de>
> Suggested-by: Boris Brezillon <boris.brezillon@collabora.com>
> Tested-by: Stefan Agner <stefan@agner.ch>
> Signed-off-by: Stefan Agner <stefan@agner.ch>
> ---

Applied, thanks.

Miquèl
diff mbox series

Patch

diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
index ddd396e93e32..ebf219c05853 100644
--- a/drivers/mtd/nand/raw/nand_base.c
+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -2131,6 +2131,22 @@  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,32 +2181,40 @@  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 (check_only)
-				break;
-
-			ret = pattern->exec(chip, &ctx.subop);
-			if (ret)
-				return ret;
+			if (best_pattern >= 0 &&
+			    nand_op_parser_cmp_ctx(&test_ctx, &best_ctx) <= 0)
+				continue;
 
-			break;
+			best_pattern = i;
+			best_ctx = test_ctx;
 		}
 
-		if (i == parser->npatterns) {
+		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;
+		}
+
 		/*
 		 * Update the context structure by pointing to the start of the
 		 * next subop.