Re: nand_op_parser_exec_op should use longest pattern

[Stefan Agner <stefan@xxxxxxxx>]

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.

--
Stefan


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