Re: nand_op_parser_exec_op should use longest pattern

[Stefan Agner <stefan@xxxxxxxx>]

Hi Boris,

On 30.03.2019 10:21, Boris Brezillon wrote:
> +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]

Hm, since I use HW ECC (which uses custom function calls), I do not get
such large data reads. But I do have OOB reads from time to time:

[    4.603585] nand: executing subop:
[    4.603616] nand:     ->CMD      [0x00]
[    4.603646] nand:     ->ADDR     [5 cyc: 00 08 ea 94 02]
[    4.603673] nand:     ->CMD      [0x30]
[    4.603700] nand:     ->WAITRDY  [max 200000 ms]
[    4.603727] nand:       DATA_IN  [64 B]
[    4.603881] nand: executing subop:
[    4.603912] nand:       CMD      [0x00]
[    4.603941] nand:       ADDR     [5 cyc: 00 08 ea 94 02]
[    4.603968] nand:       CMD      [0x30]
[    4.603994] nand:       WAITRDY  [max 200000 ms]
[    4.604022] nand:     ->DATA_IN  [64 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.

Tested it here, seems to boot a rootfs from flash just fine!

The OOB reads look like this now:
[   74.478469] nand: executing subop:
[   74.478500] nand:     ->CMD      [0x00]
[   74.478529] nand:     ->ADDR     [5 cyc: 00 08 c0 1d 00]
[   74.478555] nand:     ->CMD      [0x30]
[   74.478580] nand:     ->WAITRDY  [max 200000 ms]
[   74.478606] nand:     ->DATA_IN  [64 B]

So the patch below seems to do what we intend.

--
Stefan

> 
> --->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/