[V4,1/2] mtd: partitions: add of_match_table parser matching

Message ID 20170624231025.23292-1-zajec5@gmail.com
State New
Delegated to: Brian Norris
Headers show

Commit Message

Rafał Miłecki June 24, 2017, 11:10 p.m.
From: Brian Norris <computersforpeace@gmail.com>

Partition parsers can now provide an of_match_table to enable
flash<-->parser matching via device tree.

This support is currently limited to built-in parsers as it uses
request_module() and friends. This should be sufficient for most cases
though as compiling parsers as modules isn't a common choice.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
This is based on Brian's patches:
[RFC PATCH 4/7] mtd: add of_match_mtd_parser() and of_mtd_match_mtd_parser() helpers
[RFC PATCH 6/7] RFC: mtd: partitions: enable of_match_table matching

V1: Put helpers in mtdpart.c instead of drivers/of/of_mtd.c
    Merge helpers into a single of_mtd_match_mtd_parser
V3: Add a simple comment to note we will need the best match in the future
V4: Rework new functions to pick parser with the best match
    Move new code in parse_mtd_partitions up so it has precedence over flash
    driver defaults and MTD defaults
---
 drivers/mtd/mtdpart.c          | 65 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/mtd/partitions.h |  1 +
 2 files changed, 66 insertions(+)

Comments

Jonas Gorski June 28, 2017, 9:19 p.m. | #1
Hi,

On 25 June 2017 at 01:10, Rafał Miłecki <zajec5@gmail.com> wrote:
> From: Brian Norris <computersforpeace@gmail.com>
>
> Partition parsers can now provide an of_match_table to enable
> flash<-->parser matching via device tree.
>
> This support is currently limited to built-in parsers as it uses
> request_module() and friends. This should be sufficient for most cases
> though as compiling parsers as modules isn't a common choice.
>
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>

I gave this and Rafał's recent patches a spin and split out
bcm63xxpart's imagetag parsing into its own parser as a partition
parser assigned to the imagetag partition, then added an
of_match_table to bcm63xxpart, and it worked beautifully:

[    1.111302] m25p80 spi1.0: s25fl129p1 (16384 Kbytes)
[    1.117222] bcm63xxpart: Partition 0 is CFE offset 0 and length 10000
[    1.127313] bcm63xxpart: Partition 1 is nvram offset ff0000 and length 10000
[    1.134604] bcm63xxpart: Partition 2 is linux offset 10000 and length fe0000
[    1.141927] 3 bcm63xxpart partitions found on MTD device spi1.0
[    1.148032] Creating 3 MTD partitions on "spi1.0":
[    1.152945] 0x000000000000-0x000000010000 : "CFE"
[    1.161004] 0x000000ff0000-0x000001000000 : "nvram"
[    1.169777] 0x000000010000-0x000000ff0000 : "linux"
[    1.179489] parser_bcm63xx_imagetag: rootfs: CFE image tag found at
0x0 with version 6, board type 96328avng
[    1.189671] parser_bcm63xx_imagetag: Partition 0 is kernel offset
100 and length 151f52
[    1.197902] parser_bcm63xx_imagetag: Partition 1 is rootfs offset
152052 and length e8dfae
[    1.206405] parser_bcm63xx_imagetag: Spare partition is offset
350004 and length c8fffc
[    1.214738] 2 bcm63xx-imagetag partitions found on MTD device linux
[    1.221201] Creating 2 MTD partitions on "linux":
[    1.226032] 0x000000000100-0x000000152052 : "kernel"
[    1.234445] 0x000000152052-0x000000fe0000 : "rootfs"

Things I did:
1) having just the compatible for bcm63xxpart: bcm63xxpart was used.
2) bcm63xxpart, then fixed-partitions as two compatible strings:
bcm63xxpart was used.
3) fixed-partitons, then bcm63xxpart: ofpart was used.
4) non-existent compatible, then fixed-partitons: ofpart was used.

then I ran out of ideas to test.

I guess this might be enough for a

Tested-by: Jonas Gorski <jonas.gorski@gmail.com>


Regards
Jonas
Brian Norris Aug. 11, 2017, 4:19 p.m. | #2
Hi,

On Sun, Jun 25, 2017 at 01:10:24AM +0200, Rafał Miłecki wrote:
> From: Brian Norris <computersforpeace@gmail.com>
> 
> Partition parsers can now provide an of_match_table to enable
> flash<-->parser matching via device tree.
> 
> This support is currently limited to built-in parsers as it uses
> request_module() and friends. This should be sufficient for most cases
> though as compiling parsers as modules isn't a common choice.
> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
> This is based on Brian's patches:
> [RFC PATCH 4/7] mtd: add of_match_mtd_parser() and of_mtd_match_mtd_parser() helpers
> [RFC PATCH 6/7] RFC: mtd: partitions: enable of_match_table matching
> 
> V1: Put helpers in mtdpart.c instead of drivers/of/of_mtd.c
>     Merge helpers into a single of_mtd_match_mtd_parser
> V3: Add a simple comment to note we will need the best match in the future
> V4: Rework new functions to pick parser with the best match
>     Move new code in parse_mtd_partitions up so it has precedence over flash
>     driver defaults and MTD defaults
> ---
>  drivers/mtd/mtdpart.c          | 65 ++++++++++++++++++++++++++++++++++++++++++
>  include/linux/mtd/partitions.h |  1 +
>  2 files changed, 66 insertions(+)
> 
> diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
> index 2ad9493703f9..239d5e6e62ed 100644
> --- a/drivers/mtd/mtdpart.c
> +++ b/drivers/mtd/mtdpart.c
> @@ -30,6 +30,7 @@
>  #include <linux/mtd/mtd.h>
>  #include <linux/mtd/partitions.h>
>  #include <linux/err.h>
> +#include <linux/of.h>
>  
>  #include "mtdcore.h"
>  
> @@ -885,6 +886,60 @@ static int mtd_part_do_parse(struct mtd_part_parser *parser,
>  	return ret;
>  }
>  
> +static int of_mtd_match_mtd_parser(struct mtd_info *mtd,
> +				   struct mtd_part_parser *parser)
> +{
> +	const struct of_device_id *matches;
> +	struct device_node *np;
> +	int score = 0;
> +
> +	matches = parser->of_match_table;
> +	if (!matches)
> +		return false;
> +
> +	np = mtd_get_of_node(mtd);
> +	np = of_get_child_by_name(np, "partitions");
> +	if (!np)
> +		return false;

This function returns int, but you're returning 'false'. Should probably
be 0, or negative.

> +
> +	for (; matches->name[0] || matches->type[0] || matches->compatible[0];
> +	     matches++) {
> +		if (!matches->compatible[0])
> +			continue;
> +		score = max(score,
> +			    of_device_is_compatible(np, matches->compatible));
> +	}
> +
> +	of_node_put(np);
> +
> +	return score;
> +}
> +
> +static struct mtd_part_parser *mtd_part_get_parser_by_of(struct mtd_info *mtd)
> +{
> +	struct mtd_part_parser *p, *ret = NULL;
> +	struct mtd_part_parser *best_parser = NULL;
> +	int best_score = 0;
> +
> +	spin_lock(&part_parser_lock);
> +
> +	list_for_each_entry(p, &part_parsers, list) {
> +		int score = of_mtd_match_mtd_parser(mtd, p);
> +
> +		if (score > best_score) {
> +			best_score = score;
> +			best_parser = p;
> +		}
> +	}
> +
> +	if (best_parser && try_module_get(best_parser->owner))
> +		ret = best_parser;

Unfortunately, this only tries a single parser (the "best" in the
compatible list). But what if that parser doesn't match? I thought the
idea was to fall back to the others. e.g., if this was like a block
device, we might have something like 'compatible = "gpt", "mbr";', and
if we don't find a GPT descriptor, we fall back to MBR.

I believe the correct algorithm for this is:

 for each c in compatible: // in order
   for each p in parsers:
     if p matches c:
       ret = do_parse(p)
       if ret > 0:
         terminate(success)

Which is essentially "try parsers that match each compatible property,
starting with the first". (You could also do this by sorting the parser
list, but that would just be extra complex.)

If I understand right, yours is like sorting the parser list and only
trying the first one (i.e., the "max score").

Brian

> +
> +	spin_unlock(&part_parser_lock);
> +
> +	return ret;
> +}
> +
>  /**
>   * parse_mtd_partitions - parse MTD partitions
>   * @master: the master partition (describes whole MTD device)
> @@ -913,6 +968,16 @@ int parse_mtd_partitions(struct mtd_info *master, const char *const *types,
>  	struct mtd_part_parser *parser;
>  	int ret, err = 0;
>  
> +	parser = mtd_part_get_parser_by_of(master);
> +	if (parser) {
> +		ret = mtd_part_do_parse(parser, master, pparts, data);
> +		if (ret > 0)
> +			return 0;
> +		mtd_part_parser_put(parser);
> +		if (ret < 0)
> +			err = ret;
> +	}
> +
>  	if (!types)
>  		types = default_mtd_part_types;
>  
> diff --git a/include/linux/mtd/partitions.h b/include/linux/mtd/partitions.h
> index c4beb70dacbd..11cb0c50cd84 100644
> --- a/include/linux/mtd/partitions.h
> +++ b/include/linux/mtd/partitions.h
> @@ -77,6 +77,7 @@ struct mtd_part_parser {
>  	struct list_head list;
>  	struct module *owner;
>  	const char *name;
> +	const struct of_device_id *of_match_table;
>  	int (*parse_fn)(struct mtd_info *, const struct mtd_partition **,
>  			struct mtd_part_parser_data *);
>  	void (*cleanup)(const struct mtd_partition *pparts, int nr_parts);
> -- 
> 2.11.0
>

Patch

diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
index 2ad9493703f9..239d5e6e62ed 100644
--- a/drivers/mtd/mtdpart.c
+++ b/drivers/mtd/mtdpart.c
@@ -30,6 +30,7 @@ 
 #include <linux/mtd/mtd.h>
 #include <linux/mtd/partitions.h>
 #include <linux/err.h>
+#include <linux/of.h>
 
 #include "mtdcore.h"
 
@@ -885,6 +886,60 @@  static int mtd_part_do_parse(struct mtd_part_parser *parser,
 	return ret;
 }
 
+static int of_mtd_match_mtd_parser(struct mtd_info *mtd,
+				   struct mtd_part_parser *parser)
+{
+	const struct of_device_id *matches;
+	struct device_node *np;
+	int score = 0;
+
+	matches = parser->of_match_table;
+	if (!matches)
+		return false;
+
+	np = mtd_get_of_node(mtd);
+	np = of_get_child_by_name(np, "partitions");
+	if (!np)
+		return false;
+
+	for (; matches->name[0] || matches->type[0] || matches->compatible[0];
+	     matches++) {
+		if (!matches->compatible[0])
+			continue;
+		score = max(score,
+			    of_device_is_compatible(np, matches->compatible));
+	}
+
+	of_node_put(np);
+
+	return score;
+}
+
+static struct mtd_part_parser *mtd_part_get_parser_by_of(struct mtd_info *mtd)
+{
+	struct mtd_part_parser *p, *ret = NULL;
+	struct mtd_part_parser *best_parser = NULL;
+	int best_score = 0;
+
+	spin_lock(&part_parser_lock);
+
+	list_for_each_entry(p, &part_parsers, list) {
+		int score = of_mtd_match_mtd_parser(mtd, p);
+
+		if (score > best_score) {
+			best_score = score;
+			best_parser = p;
+		}
+	}
+
+	if (best_parser && try_module_get(best_parser->owner))
+		ret = best_parser;
+
+	spin_unlock(&part_parser_lock);
+
+	return ret;
+}
+
 /**
  * parse_mtd_partitions - parse MTD partitions
  * @master: the master partition (describes whole MTD device)
@@ -913,6 +968,16 @@  int parse_mtd_partitions(struct mtd_info *master, const char *const *types,
 	struct mtd_part_parser *parser;
 	int ret, err = 0;
 
+	parser = mtd_part_get_parser_by_of(master);
+	if (parser) {
+		ret = mtd_part_do_parse(parser, master, pparts, data);
+		if (ret > 0)
+			return 0;
+		mtd_part_parser_put(parser);
+		if (ret < 0)
+			err = ret;
+	}
+
 	if (!types)
 		types = default_mtd_part_types;
 
diff --git a/include/linux/mtd/partitions.h b/include/linux/mtd/partitions.h
index c4beb70dacbd..11cb0c50cd84 100644
--- a/include/linux/mtd/partitions.h
+++ b/include/linux/mtd/partitions.h
@@ -77,6 +77,7 @@  struct mtd_part_parser {
 	struct list_head list;
 	struct module *owner;
 	const char *name;
+	const struct of_device_id *of_match_table;
 	int (*parse_fn)(struct mtd_info *, const struct mtd_partition **,
 			struct mtd_part_parser_data *);
 	void (*cleanup)(const struct mtd_partition *pparts, int nr_parts);