[PATCH V3 1/3] dt-bindings: mtd: document linux,part-probe property

[PATCH V3 1/3] dt-bindings: mtd: document linux,part-probe property

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

Support for this property has been introduced in 2010 with commit
9d5da3a9b849 ("mtd: extend physmap_of to let the device tree specify the
parition probe") but it was never documented. Fix this by adding a
proper description and example.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 Documentation/devicetree/bindings/mtd/common.txt | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/mtd/common.txt b/Documentation/devicetree/bindings/mtd/common.txt
index fc068b923d7a..1ada70e718b2 100644
--- a/Documentation/devicetree/bindings/mtd/common.txt
+++ b/Documentation/devicetree/bindings/mtd/common.txt
@@ -6,10 +6,17 @@ Optional properties:
   controller based name) in order to ease flash device identification
   and/or describe what they are used for.
 
+- linux,part-probe: if present, this property should contain a list of strings
+  with partition probes to be used for the flash device. A role of partition
+  probe (parser) is to read/construct partition table and register found
+  partitions. Getting partition table may be platform or device specific so
+  various devices may use various Linux drivers for this purpose.
+
 Example:
 
 	flash@0 {
 		label = "System-firmware";
+		linux,part-probe = "cmdlinepart", "ofpart";
 
 		/* flash type specific properties */
 	};
-- 
2.11.0

[PATCH V3 2/3] mtd: add core code reading DT specified part probes

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

Handling (creating) partitions for flash devices requires using a proper
driver that will read partition table (out of somewhere). We can't
simply try all existing drivers one by one:
1) It would increase boot time
2) The order could be a problem
3) In some corner cases parsers could misinterpret some data as a table
Due to this MTD subsystem allows drivers to specify a list of applicable
part probes.

So far physmap_of was the only driver with support for linux,part-probe
DT property. Other ones had list or probes hardcoded which wasn't making
them really flexible. It prevented using common flash drivers on
platforms that required some specific partition table access.

This commit adds support for mentioned DT property directly to the MTD
core. It's a rewritten implementation of physmap_of's code and it makes
original code obsolete. Thanks to calling it on device parse
registration (as suggested by Boris) all drivers gain support for it for
free.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 drivers/mtd/mtdcore.c | 36 +++++++++++++++++++++++++++++++++++-
 1 file changed, 35 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 66a9dedd1062..917def28c756 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -664,6 +664,32 @@ static void mtd_set_dev_defaults(struct mtd_info *mtd)
 	}
 }
 
+static const char * const *mtd_of_get_probes(struct device_node *np)
+{
+	const char **res;
+	int count;
+
+	count = of_property_count_strings(np, "linux,part-probe");
+	if (count < 0)
+		return NULL;
+
+	res = kzalloc((count + 1) * sizeof(*res), GFP_KERNEL);
+	if (!res)
+		return NULL;
+
+	count = of_property_read_string_array(np, "linux,part-probe", res,
+					      count);
+	if (count < 0)
+		return NULL;
+
+	return res;
+}
+
+static inline void mtd_of_free_probes(const char * const *probes)
+{
+	kfree(probes);
+}
+
 /**
  * mtd_device_parse_register - parse partitions and register an MTD device.
  *
@@ -698,14 +724,19 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
 			      const struct mtd_partition *parts,
 			      int nr_parts)
 {
+	const char * const *part_probe_types;
 	struct mtd_partitions parsed;
 	int ret;
 
 	mtd_set_dev_defaults(mtd);
 
+	part_probe_types = mtd_of_get_probes(mtd_get_of_node(mtd));
+	if (!part_probe_types)
+		part_probe_types = types;
+
 	memset(&parsed, 0, sizeof(parsed));
 
-	ret = parse_mtd_partitions(mtd, types, &parsed, parser_data);
+	ret = parse_mtd_partitions(mtd, part_probe_types, &parsed, parser_data);
 	if ((ret < 0 || parsed.nr_parts == 0) && parts && nr_parts) {
 		/* Fall back to driver-provided partitions */
 		parsed = (struct mtd_partitions){
@@ -720,6 +751,9 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
 		memset(&parsed, 0, sizeof(parsed));
 	}
 
+	if (part_probe_types != types)
+		mtd_of_free_probes(part_probe_types);
+
 	ret = mtd_add_device_partitions(mtd, &parsed);
 	if (ret)
 		goto out;
-- 
2.11.0

Re: [PATCH V3 2/3] mtd: add core code reading DT specified part probes

[Boris Brezillon]

Hi Rafal,

On Fri, 31 Mar 2017 13:40:15 +0200
Rafał Miłecki <zajec5@gmail.com> wrote:

> From: Rafał Miłecki <rafal@milecki.pl>
> 
> Handling (creating) partitions for flash devices requires using a proper
> driver that will read partition table (out of somewhere). We can't
> simply try all existing drivers one by one:
> 1) It would increase boot time
> 2) The order could be a problem
> 3) In some corner cases parsers could misinterpret some data as a table
> Due to this MTD subsystem allows drivers to specify a list of applicable
> part probes.
> 
> So far physmap_of was the only driver with support for linux,part-probe
> DT property. Other ones had list or probes hardcoded which wasn't making
> them really flexible. It prevented using common flash drivers on
> platforms that required some specific partition table access.
> 
> This commit adds support for mentioned DT property directly to the MTD
> core. It's a rewritten implementation of physmap_of's code and it makes
> original code obsolete. Thanks to calling it on device parse
> registration (as suggested by Boris) all drivers gain support for it for
> free.
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
>  drivers/mtd/mtdcore.c | 36 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 35 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index 66a9dedd1062..917def28c756 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -664,6 +664,32 @@ static void mtd_set_dev_defaults(struct mtd_info *mtd)
>  	}
>  }
>  
> +static const char * const *mtd_of_get_probes(struct device_node *np)

To be consistent with NAND DT helpers, can you put the of_get_ prefix
at the beginning: of_get_mtd_probes().
And get_probes() is a bit too generic IMHO, how about
of_get_mtd_part_probes() (or any other name clearly showing that this
is about partition parsers/probes).

> +{
> +	const char **res;
> +	int count;
> +
> +	count = of_property_count_strings(np, "linux,part-probe");
> +	if (count < 0)
> +		return NULL;
> +
> +	res = kzalloc((count + 1) * sizeof(*res), GFP_KERNEL);
> +	if (!res)
> +		return NULL;
> +
> +	count = of_property_read_string_array(np, "linux,part-probe", res,
> +					      count);
> +	if (count < 0)
> +		return NULL;
> +
> +	return res;
> +}
> +
> +static inline void mtd_of_free_probes(const char * const *probes)

Drop the inline, the compiler is smart enough to decide by itself.

> +{
> +	kfree(probes);
> +}

This is not really DT specific, and we might want to extract the list
of probes by other means in the future (cmdline, ACPI?).

How about declaring these 2 functions:

static char **mtd_alloc_part_type_table(int nentries)
{
	return kzalloc((nentries + 1) * sizeof(*res), GFP_KERNEL);
}

static void mtd_free_part_type_table(const char * const *table)
{
	kfree(table);
}

You can then use mtd_alloc_part_type_table() in
of_get_mtd_part_probes() to allocate your partition type table.

> +
>  /**
>   * mtd_device_parse_register - parse partitions and register an MTD device.
>   *
> @@ -698,14 +724,19 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
>  			      const struct mtd_partition *parts,
>  			      int nr_parts)
>  {
> +	const char * const *part_probe_types;
>  	struct mtd_partitions parsed;
>  	int ret;
>  
>  	mtd_set_dev_defaults(mtd);
>  
> +	part_probe_types = mtd_of_get_probes(mtd_get_of_node(mtd));
> +	if (!part_probe_types)
> +		part_probe_types = types;
> +

How about doing it the other way around:

	if (part_probe_types)
		types = part_probe_types;

>  	memset(&parsed, 0, sizeof(parsed));
>  
> -	ret = parse_mtd_partitions(mtd, types, &parsed, parser_data);
> +	ret = parse_mtd_partitions(mtd, part_probe_types, &parsed, parser_data);

this way you don't need this change

>  	if ((ret < 0 || parsed.nr_parts == 0) && parts && nr_parts) {
>  		/* Fall back to driver-provided partitions */
>  		parsed = (struct mtd_partitions){
> @@ -720,6 +751,9 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
>  		memset(&parsed, 0, sizeof(parsed));
>  	}
>  
> +	if (part_probe_types != types)
> +		mtd_of_free_probes(part_probe_types);

and here you simply have:

	mtd_of_free_probes(part_probe_types);


> +
>  	ret = mtd_add_device_partitions(mtd, &parsed);
>  	if (ret)
>  		goto out;

Re: [PATCH V3 2/3] mtd: add core code reading DT specified part probes

[Boris Brezillon]

On Sun, 9 Apr 2017 13:04:06 +0200
Boris Brezillon <boris.brezillon@free-electrons.com> wrote:

> static char **mtd_alloc_part_type_table(int nentries)

Oops, s/char **/const char **/

> {
> 	return kzalloc((nentries + 1) * sizeof(*res), GFP_KERNEL);
> }
> 
> static void mtd_free_part_type_table(const char * const *table)
> {
> 	kfree(table);
> }

I realize this might not be suitable for all kind of part-probes
definitions. Some might need to dynamically allocate each string and
expect the core to free them in mtd_free_part_type_table() (the one I
have in mind is the cmdline part-probes parser). Others already have
the strings statically defined or allocated and maintained somewhere
else (this is the case with DT which provides direct access to string
definitions), which means the core shouldn't free them.

I see 3 solutions to this problem:

1/ go back to your initial solution with DT specific functions, and
   wait until someone decides to implement another way to define
   part-probes (cmdline or ACPI) before considering a more complex
   solution
2/ always allocate strings dynamically and let
   mtd_free_part_type_table() free them. This implies using kstrdup() on
   strings returned by of_property_read_string_array()
3/ use something smarter to let the part-probes table creator free it,
   for example, by using something like:

	struct mtd_part_probes {
		const char * const *types;
		void (*free)(const char * const *types);
	}

#3 is overkill IMO. I'm fine with #1 and #2, pick the one you prefer.

[PATCH V3 3/3] mtd: physmap_of: drop duplicated support for linux, part-probe property

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

Now support for linux,part-probe has been added to the MTD core there is
no need to duplicate support for it in physmap_of.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 drivers/mtd/maps/physmap_of.c | 46 +------------------------------------------
 1 file changed, 1 insertion(+), 45 deletions(-)

diff --git a/drivers/mtd/maps/physmap_of.c b/drivers/mtd/maps/physmap_of.c
index 14e8909c9955..49dbb7235848 100644
--- a/drivers/mtd/maps/physmap_of.c
+++ b/drivers/mtd/maps/physmap_of.c
@@ -114,47 +114,9 @@ static struct mtd_info *obsolete_probe(struct platform_device *dev,
 static const char * const part_probe_types_def[] = {
 	"cmdlinepart", "RedBoot", "ofpart", "ofoldpart", NULL };
 
-static const char * const *of_get_probes(struct device_node *dp)
-{
-	const char *cp;
-	int cplen;
-	unsigned int l;
-	unsigned int count;
-	const char **res;
-
-	cp = of_get_property(dp, "linux,part-probe", &cplen);
-	if (cp == NULL)
-		return part_probe_types_def;
-
-	count = 0;
-	for (l = 0; l != cplen; l++)
-		if (cp[l] == 0)
-			count++;
-
-	res = kzalloc((count + 1)*sizeof(*res), GFP_KERNEL);
-	if (!res)
-		return NULL;
-	count = 0;
-	while (cplen > 0) {
-		res[count] = cp;
-		l = strlen(cp) + 1;
-		cp += l;
-		cplen -= l;
-		count++;
-	}
-	return res;
-}
-
-static void of_free_probes(const char * const *probes)
-{
-	if (probes != part_probe_types_def)
-		kfree(probes);
-}
-
 static const struct of_device_id of_flash_match[];
 static int of_flash_probe(struct platform_device *dev)
 {
-	const char * const *part_probe_types;
 	const struct of_device_id *match;
 	struct device_node *dp = dev->dev.of_node;
 	struct resource res;
@@ -320,14 +282,8 @@ static int of_flash_probe(struct platform_device *dev)
 
 	info->cmtd->dev.parent = &dev->dev;
 	mtd_set_of_node(info->cmtd, dp);
-	part_probe_types = of_get_probes(dp);
-	if (!part_probe_types) {
-		err = -ENOMEM;
-		goto err_out;
-	}
-	mtd_device_parse_register(info->cmtd, part_probe_types, NULL,
+	mtd_device_parse_register(info->cmtd, part_probe_types_def, NULL,
 			NULL, 0);
-	of_free_probes(part_probe_types);
 
 	kfree(mtd_list);
 
-- 
2.11.0

Re: [PATCH V3 3/3] mtd: physmap_of: drop duplicated support for linux,part-probe property

[Boris Brezillon]

On Fri, 31 Mar 2017 13:40:16 +0200
Rafał Miłecki <zajec5@gmail.com> wrote:

> From: Rafał Miłecki <rafal@milecki.pl>
> 
> Now support for linux,part-probe has been added to the MTD core there is
> no need to duplicate support for it in physmap_of.
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>

Acked-by: Boris Brezillon <boris.brezillon@free-electrons.com>

> ---
>  drivers/mtd/maps/physmap_of.c | 46 +------------------------------------------
>  1 file changed, 1 insertion(+), 45 deletions(-)
> 
> diff --git a/drivers/mtd/maps/physmap_of.c b/drivers/mtd/maps/physmap_of.c
> index 14e8909c9955..49dbb7235848 100644
> --- a/drivers/mtd/maps/physmap_of.c
> +++ b/drivers/mtd/maps/physmap_of.c
> @@ -114,47 +114,9 @@ static struct mtd_info *obsolete_probe(struct platform_device *dev,
>  static const char * const part_probe_types_def[] = {
>  	"cmdlinepart", "RedBoot", "ofpart", "ofoldpart", NULL };
>  
> -static const char * const *of_get_probes(struct device_node *dp)
> -{
> -	const char *cp;
> -	int cplen;
> -	unsigned int l;
> -	unsigned int count;
> -	const char **res;
> -
> -	cp = of_get_property(dp, "linux,part-probe", &cplen);
> -	if (cp == NULL)
> -		return part_probe_types_def;
> -
> -	count = 0;
> -	for (l = 0; l != cplen; l++)
> -		if (cp[l] == 0)
> -			count++;
> -
> -	res = kzalloc((count + 1)*sizeof(*res), GFP_KERNEL);
> -	if (!res)
> -		return NULL;
> -	count = 0;
> -	while (cplen > 0) {
> -		res[count] = cp;
> -		l = strlen(cp) + 1;
> -		cp += l;
> -		cplen -= l;
> -		count++;
> -	}
> -	return res;
> -}
> -
> -static void of_free_probes(const char * const *probes)
> -{
> -	if (probes != part_probe_types_def)
> -		kfree(probes);
> -}
> -
>  static const struct of_device_id of_flash_match[];
>  static int of_flash_probe(struct platform_device *dev)
>  {
> -	const char * const *part_probe_types;
>  	const struct of_device_id *match;
>  	struct device_node *dp = dev->dev.of_node;
>  	struct resource res;
> @@ -320,14 +282,8 @@ static int of_flash_probe(struct platform_device *dev)
>  
>  	info->cmtd->dev.parent = &dev->dev;
>  	mtd_set_of_node(info->cmtd, dp);
> -	part_probe_types = of_get_probes(dp);
> -	if (!part_probe_types) {
> -		err = -ENOMEM;
> -		goto err_out;
> -	}
> -	mtd_device_parse_register(info->cmtd, part_probe_types, NULL,
> +	mtd_device_parse_register(info->cmtd, part_probe_types_def, NULL,
>  			NULL, 0);
> -	of_free_probes(part_probe_types);
>  
>  	kfree(mtd_list);
>  

Re: [PATCH V3 1/3] dt-bindings: mtd: document linux,part-probe property

[Rob Herring]

On Fri, Mar 31, 2017 at 01:40:14PM +0200, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> Support for this property has been introduced in 2010 with commit
> 9d5da3a9b849 ("mtd: extend physmap_of to let the device tree specify the
> parition probe") but it was never documented. Fix this by adding a
> proper description and example.
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
>  Documentation/devicetree/bindings/mtd/common.txt | 7 +++++++
>  1 file changed, 7 insertions(+)

Acked-by: Rob Herring <robh@kernel.org> 

Re: [PATCH V3 1/3] dt-bindings: mtd: document linux,part-probe property

[Boris Brezillon]

Hi Rafal,

On Fri, 31 Mar 2017 13:40:14 +0200
Rafał Miłecki <zajec5@gmail.com> wrote:

> From: Rafał Miłecki <rafal@milecki.pl>
> 
> Support for this property has been introduced in 2010 with commit
> 9d5da3a9b849 ("mtd: extend physmap_of to let the device tree specify the
> parition probe") but it was never documented. Fix this by adding a
> proper description and example.
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>

Acked-by: Boris Brezillon <boris.brezillon@free-electrons.com>

> ---
>  Documentation/devicetree/bindings/mtd/common.txt | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mtd/common.txt b/Documentation/devicetree/bindings/mtd/common.txt
> index fc068b923d7a..1ada70e718b2 100644
> --- a/Documentation/devicetree/bindings/mtd/common.txt
> +++ b/Documentation/devicetree/bindings/mtd/common.txt
> @@ -6,10 +6,17 @@ Optional properties:
>    controller based name) in order to ease flash device identification
>    and/or describe what they are used for.
>  
> +- linux,part-probe: if present, this property should contain a list of strings
> +  with partition probes to be used for the flash device. A role of partition
> +  probe (parser) is to read/construct partition table and register found
> +  partitions. Getting partition table may be platform or device specific so
> +  various devices may use various Linux drivers for this purpose.
> +
>  Example:
>  
>  	flash@0 {
>  		label = "System-firmware";
> +		linux,part-probe = "cmdlinepart", "ofpart";
>  
>  		/* flash type specific properties */
>  	};

[PATCH V4 1/3] dt-bindings: mtd: document linux,part-probe property

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

Support for this property has been introduced in 2010 with commit
9d5da3a9b849 ("mtd: extend physmap_of to let the device tree specify the
parition probe") but it was never documented. Fix this by adding a
proper description and example.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
Acked-by: Rob Herring <robh@kernel.org>
Acked-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 Documentation/devicetree/bindings/mtd/common.txt | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/mtd/common.txt b/Documentation/devicetree/bindings/mtd/common.txt
index fc068b923d7a..1ada70e718b2 100644
--- a/Documentation/devicetree/bindings/mtd/common.txt
+++ b/Documentation/devicetree/bindings/mtd/common.txt
@@ -6,10 +6,17 @@ Optional properties:
   controller based name) in order to ease flash device identification
   and/or describe what they are used for.
 
+- linux,part-probe: if present, this property should contain a list of strings
+  with partition probes to be used for the flash device. A role of partition
+  probe (parser) is to read/construct partition table and register found
+  partitions. Getting partition table may be platform or device specific so
+  various devices may use various Linux drivers for this purpose.
+
 Example:
 
 	flash@0 {
 		label = "System-firmware";
+		linux,part-probe = "cmdlinepart", "ofpart";
 
 		/* flash type specific properties */
 	};
-- 
2.11.0

[PATCH V4 2/3] mtd: add core code reading DT specified part probes

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

Handling (creating) partitions for flash devices requires using a proper
driver that will read partition table (out of somewhere). We can't
simply try all existing drivers one by one:
1) It would increase boot time
2) The order could be a problem
3) In some corner cases parsers could misinterpret some data as a table
Due to this MTD subsystem allows drivers to specify a list of applicable
part probes.

So far physmap_of was the only driver with support for linux,part-probe
DT property. Other ones had list or probes hardcoded which wasn't making
them really flexible. It prevented using common flash drivers on
platforms that required some specific partition table access.

This commit adds support for mentioned DT property directly to the MTD
core. It's a rewritten implementation of physmap_of's code and it makes
original code obsolete. Thanks to calling it on device parse
registration (as suggested by Boris) all drivers gain support for it for
free.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
V4: Rename of functions
    Add comment to the of_get_mtd_part_probes
    Simplify mtd_device_parse_register changes
    Thanks Boris for help on all above!
---
 drivers/mtd/mtdcore.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 66a9dedd1062..07b07de7e080 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -664,6 +664,33 @@ static void mtd_set_dev_defaults(struct mtd_info *mtd)
 	}
 }
 
+static const char * const *of_get_mtd_part_probes(struct device_node *np)
+{
+	const char **res;
+	int count;
+
+	count = of_property_count_strings(np, "linux,part-probe");
+	if (count < 0)
+		return NULL;
+
+	/* This looks common: consider helper function if copying */
+	res = kzalloc((count + 1) * sizeof(*res), GFP_KERNEL);
+	if (!res)
+		return NULL;
+
+	count = of_property_read_string_array(np, "linux,part-probe", res,
+					      count);
+	if (count < 0)
+		return NULL;
+
+	return res;
+}
+
+static void of_free_mtd_part_probes(const char * const *probes)
+{
+	kfree(probes);
+}
+
 /**
  * mtd_device_parse_register - parse partitions and register an MTD device.
  *
@@ -698,11 +725,16 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
 			      const struct mtd_partition *parts,
 			      int nr_parts)
 {
+	const char * const *part_probe_types;
 	struct mtd_partitions parsed;
 	int ret;
 
 	mtd_set_dev_defaults(mtd);
 
+	part_probe_types = of_get_mtd_part_probes(mtd_get_of_node(mtd));
+	if (part_probe_types)
+		types = part_probe_types;
+
 	memset(&parsed, 0, sizeof(parsed));
 
 	ret = parse_mtd_partitions(mtd, types, &parsed, parser_data);
@@ -720,6 +752,8 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
 		memset(&parsed, 0, sizeof(parsed));
 	}
 
+	of_free_mtd_part_probes(part_probe_types);
+
 	ret = mtd_add_device_partitions(mtd, &parsed);
 	if (ret)
 		goto out;
-- 
2.11.0

Re: [PATCH V4 2/3] mtd: add core code reading DT specified part probes

[Boris Brezillon]

On Mon, 17 Apr 2017 20:28:52 +0200
Rafał Miłecki <zajec5@gmail.com> wrote:

> From: Rafał Miłecki <rafal@milecki.pl>
> 
> Handling (creating) partitions for flash devices requires using a proper
> driver that will read partition table (out of somewhere). We can't
> simply try all existing drivers one by one:
> 1) It would increase boot time
> 2) The order could be a problem
> 3) In some corner cases parsers could misinterpret some data as a table
> Due to this MTD subsystem allows drivers to specify a list of applicable
> part probes.
> 
> So far physmap_of was the only driver with support for linux,part-probe
> DT property. Other ones had list or probes hardcoded which wasn't making
> them really flexible. It prevented using common flash drivers on
> platforms that required some specific partition table access.
> 
> This commit adds support for mentioned DT property directly to the MTD
> core. It's a rewritten implementation of physmap_of's code and it makes
> original code obsolete. Thanks to calling it on device parse
> registration (as suggested by Boris) all drivers gain support for it for
> free.
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
> V4: Rename of functions
>     Add comment to the of_get_mtd_part_probes
>     Simplify mtd_device_parse_register changes
>     Thanks Boris for help on all above!
> ---
>  drivers/mtd/mtdcore.c | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index 66a9dedd1062..07b07de7e080 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -664,6 +664,33 @@ static void mtd_set_dev_defaults(struct mtd_info *mtd)
>  	}
>  }
>  
> +static const char * const *of_get_mtd_part_probes(struct device_node *np)
> +{
> +	const char **res;
> +	int count;
> +
> +	count = of_property_count_strings(np, "linux,part-probe");
> +	if (count < 0)
> +		return NULL;
> +
> +	/* This looks common: consider helper function if copying */
> +	res = kzalloc((count + 1) * sizeof(*res), GFP_KERNEL);
> +	if (!res)
> +		return NULL;
> +
> +	count = of_property_read_string_array(np, "linux,part-probe", res,
> +					      count);
> +	if (count < 0)
> +		return NULL;
> +
> +	return res;
> +}
> +
> +static void of_free_mtd_part_probes(const char * const *probes)
> +{
> +	kfree(probes);
> +}
> +
>  /**
>   * mtd_device_parse_register - parse partitions and register an MTD device.
>   *
> @@ -698,11 +725,16 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
>  			      const struct mtd_partition *parts,
>  			      int nr_parts)
>  {
> +	const char * const *part_probe_types;

One last thing: can we rename this variable of_part_probe_types?

With this addressed,

Acked-by: Boris Brezillon <boris.brezillon@free-electrons.com>

Thanks,

Boris

>  	struct mtd_partitions parsed;
>  	int ret;
>  
>  	mtd_set_dev_defaults(mtd);
>  
> +	part_probe_types = of_get_mtd_part_probes(mtd_get_of_node(mtd));
> +	if (part_probe_types)
> +		types = part_probe_types;
> +
>  	memset(&parsed, 0, sizeof(parsed));
>  
>  	ret = parse_mtd_partitions(mtd, types, &parsed, parser_data);
> @@ -720,6 +752,8 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
>  		memset(&parsed, 0, sizeof(parsed));
>  	}
>  
> +	of_free_mtd_part_probes(part_probe_types);
> +
>  	ret = mtd_add_device_partitions(mtd, &parsed);
>  	if (ret)
>  		goto out;

[PATCH V4 3/3] mtd: physmap_of: drop duplicated support for linux, part-probe property

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

Now support for linux,part-probe has been added to the MTD core there is
no need to duplicate support for it in physmap_of.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
Acked-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 drivers/mtd/maps/physmap_of.c | 46 +------------------------------------------
 1 file changed, 1 insertion(+), 45 deletions(-)

diff --git a/drivers/mtd/maps/physmap_of.c b/drivers/mtd/maps/physmap_of.c
index 14e8909c9955..49dbb7235848 100644
--- a/drivers/mtd/maps/physmap_of.c
+++ b/drivers/mtd/maps/physmap_of.c
@@ -114,47 +114,9 @@ static struct mtd_info *obsolete_probe(struct platform_device *dev,
 static const char * const part_probe_types_def[] = {
 	"cmdlinepart", "RedBoot", "ofpart", "ofoldpart", NULL };
 
-static const char * const *of_get_probes(struct device_node *dp)
-{
-	const char *cp;
-	int cplen;
-	unsigned int l;
-	unsigned int count;
-	const char **res;
-
-	cp = of_get_property(dp, "linux,part-probe", &cplen);
-	if (cp == NULL)
-		return part_probe_types_def;
-
-	count = 0;
-	for (l = 0; l != cplen; l++)
-		if (cp[l] == 0)
-			count++;
-
-	res = kzalloc((count + 1)*sizeof(*res), GFP_KERNEL);
-	if (!res)
-		return NULL;
-	count = 0;
-	while (cplen > 0) {
-		res[count] = cp;
-		l = strlen(cp) + 1;
-		cp += l;
-		cplen -= l;
-		count++;
-	}
-	return res;
-}
-
-static void of_free_probes(const char * const *probes)
-{
-	if (probes != part_probe_types_def)
-		kfree(probes);
-}
-
 static const struct of_device_id of_flash_match[];
 static int of_flash_probe(struct platform_device *dev)
 {
-	const char * const *part_probe_types;
 	const struct of_device_id *match;
 	struct device_node *dp = dev->dev.of_node;
 	struct resource res;
@@ -320,14 +282,8 @@ static int of_flash_probe(struct platform_device *dev)
 
 	info->cmtd->dev.parent = &dev->dev;
 	mtd_set_of_node(info->cmtd, dp);
-	part_probe_types = of_get_probes(dp);
-	if (!part_probe_types) {
-		err = -ENOMEM;
-		goto err_out;
-	}
-	mtd_device_parse_register(info->cmtd, part_probe_types, NULL,
+	mtd_device_parse_register(info->cmtd, part_probe_types_def, NULL,
 			NULL, 0);
-	of_free_probes(part_probe_types);
 
 	kfree(mtd_list);
 
-- 
2.11.0

[PATCH V5 1/3] dt-bindings: mtd: document linux,part-probe property

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

Support for this property has been introduced in 2010 with commit
9d5da3a9b849 ("mtd: extend physmap_of to let the device tree specify the
parition probe") but it was never documented. Fix this by adding a
proper description and example.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
Acked-by: Rob Herring <robh@kernel.org>
Acked-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 Documentation/devicetree/bindings/mtd/common.txt | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/mtd/common.txt b/Documentation/devicetree/bindings/mtd/common.txt
index fc068b923d7a..1ada70e718b2 100644
--- a/Documentation/devicetree/bindings/mtd/common.txt
+++ b/Documentation/devicetree/bindings/mtd/common.txt
@@ -6,10 +6,17 @@ Optional properties:
   controller based name) in order to ease flash device identification
   and/or describe what they are used for.
 
+- linux,part-probe: if present, this property should contain a list of strings
+  with partition probes to be used for the flash device. A role of partition
+  probe (parser) is to read/construct partition table and register found
+  partitions. Getting partition table may be platform or device specific so
+  various devices may use various Linux drivers for this purpose.
+
 Example:
 
 	flash@0 {
 		label = "System-firmware";
+		linux,part-probe = "cmdlinepart", "ofpart";
 
 		/* flash type specific properties */
 	};
-- 
2.11.0

[PATCH V5 2/3] mtd: add core code reading DT specified part probes

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

Handling (creating) partitions for flash devices requires using a proper
driver that will read partition table (out of somewhere). We can't
simply try all existing drivers one by one:
1) It would increase boot time
2) The order could be a problem
3) In some corner cases parsers could misinterpret some data as a table
Due to this MTD subsystem allows drivers to specify a list of applicable
part probes.

So far physmap_of was the only driver with support for linux,part-probe
DT property. Other ones had list or probes hardcoded which wasn't making
them really flexible. It prevented using common flash drivers on
platforms that required some specific partition table access.

This commit adds support for mentioned DT property directly to the MTD
core. It's a rewritten implementation of physmap_of's code and it makes
original code obsolete. Thanks to calling it on device parse
registration (as suggested by Boris) all drivers gain support for it for
free.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
Acked-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
V4: Rename of functions
    Add comment to the of_get_mtd_part_probes
    Simplify mtd_device_parse_register changes
    Thanks Boris for help on all above!
V5: Rename variable to of_part_probe_types
---
 drivers/mtd/mtdcore.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 66a9dedd1062..228bcfd03cf0 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -664,6 +664,33 @@ static void mtd_set_dev_defaults(struct mtd_info *mtd)
 	}
 }
 
+static const char * const *of_get_mtd_part_probes(struct device_node *np)
+{
+	const char **res;
+	int count;
+
+	count = of_property_count_strings(np, "linux,part-probe");
+	if (count < 0)
+		return NULL;
+
+	/* This looks common: consider helper function if copying */
+	res = kzalloc((count + 1) * sizeof(*res), GFP_KERNEL);
+	if (!res)
+		return NULL;
+
+	count = of_property_read_string_array(np, "linux,part-probe", res,
+					      count);
+	if (count < 0)
+		return NULL;
+
+	return res;
+}
+
+static void of_free_mtd_part_probes(const char * const *probes)
+{
+	kfree(probes);
+}
+
 /**
  * mtd_device_parse_register - parse partitions and register an MTD device.
  *
@@ -698,11 +725,16 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
 			      const struct mtd_partition *parts,
 			      int nr_parts)
 {
+	const char * const *of_part_probe_types;
 	struct mtd_partitions parsed;
 	int ret;
 
 	mtd_set_dev_defaults(mtd);
 
+	of_part_probe_types = of_get_mtd_part_probes(mtd_get_of_node(mtd));
+	if (of_part_probe_types)
+		types = of_part_probe_types;
+
 	memset(&parsed, 0, sizeof(parsed));
 
 	ret = parse_mtd_partitions(mtd, types, &parsed, parser_data);
@@ -720,6 +752,8 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
 		memset(&parsed, 0, sizeof(parsed));
 	}
 
+	of_free_mtd_part_probes(of_part_probe_types);
+
 	ret = mtd_add_device_partitions(mtd, &parsed);
 	if (ret)
 		goto out;
-- 
2.11.0

[PATCH V5 3/3] mtd: physmap_of: drop duplicated support for linux, part-probe property

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

Now support for linux,part-probe has been added to the MTD core there is
no need to duplicate support for it in physmap_of.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
Acked-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 drivers/mtd/maps/physmap_of.c | 46 +------------------------------------------
 1 file changed, 1 insertion(+), 45 deletions(-)

diff --git a/drivers/mtd/maps/physmap_of.c b/drivers/mtd/maps/physmap_of.c
index 14e8909c9955..49dbb7235848 100644
--- a/drivers/mtd/maps/physmap_of.c
+++ b/drivers/mtd/maps/physmap_of.c
@@ -114,47 +114,9 @@ static struct mtd_info *obsolete_probe(struct platform_device *dev,
 static const char * const part_probe_types_def[] = {
 	"cmdlinepart", "RedBoot", "ofpart", "ofoldpart", NULL };
 
-static const char * const *of_get_probes(struct device_node *dp)
-{
-	const char *cp;
-	int cplen;
-	unsigned int l;
-	unsigned int count;
-	const char **res;
-
-	cp = of_get_property(dp, "linux,part-probe", &cplen);
-	if (cp == NULL)
-		return part_probe_types_def;
-
-	count = 0;
-	for (l = 0; l != cplen; l++)
-		if (cp[l] == 0)
-			count++;
-
-	res = kzalloc((count + 1)*sizeof(*res), GFP_KERNEL);
-	if (!res)
-		return NULL;
-	count = 0;
-	while (cplen > 0) {
-		res[count] = cp;
-		l = strlen(cp) + 1;
-		cp += l;
-		cplen -= l;
-		count++;
-	}
-	return res;
-}
-
-static void of_free_probes(const char * const *probes)
-{
-	if (probes != part_probe_types_def)
-		kfree(probes);
-}
-
 static const struct of_device_id of_flash_match[];
 static int of_flash_probe(struct platform_device *dev)
 {
-	const char * const *part_probe_types;
 	const struct of_device_id *match;
 	struct device_node *dp = dev->dev.of_node;
 	struct resource res;
@@ -320,14 +282,8 @@ static int of_flash_probe(struct platform_device *dev)
 
 	info->cmtd->dev.parent = &dev->dev;
 	mtd_set_of_node(info->cmtd, dp);
-	part_probe_types = of_get_probes(dp);
-	if (!part_probe_types) {
-		err = -ENOMEM;
-		goto err_out;
-	}
-	mtd_device_parse_register(info->cmtd, part_probe_types, NULL,
+	mtd_device_parse_register(info->cmtd, part_probe_types_def, NULL,
 			NULL, 0);
-	of_free_probes(part_probe_types);
 
 	kfree(mtd_list);
 
-- 
2.11.0

Re: [PATCH V5 1/3] dt-bindings: mtd: document linux,part-probe property

[Brian Norris]

Hi,

On Mon, Apr 17, 2017 at 09:47:44PM +0200, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> Support for this property has been introduced in 2010 with commit
> 9d5da3a9b849 ("mtd: extend physmap_of to let the device tree specify the
> parition probe") but it was never documented. Fix this by adding a
> proper description and example.
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> Acked-by: Rob Herring <robh@kernel.org>
> Acked-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---
>  Documentation/devicetree/bindings/mtd/common.txt | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mtd/common.txt b/Documentation/devicetree/bindings/mtd/common.txt
> index fc068b923d7a..1ada70e718b2 100644
> --- a/Documentation/devicetree/bindings/mtd/common.txt
> +++ b/Documentation/devicetree/bindings/mtd/common.txt
> @@ -6,10 +6,17 @@ Optional properties:
>    controller based name) in order to ease flash device identification
>    and/or describe what they are used for.
>  
> +- linux,part-probe: if present, this property should contain a list of strings
> +  with partition probes to be used for the flash device. A role of partition
> +  probe (parser) is to read/construct partition table and register found
> +  partitions. Getting partition table may be platform or device specific so
> +  various devices may use various Linux drivers for this purpose.

Have you reviewed the old threads about this? I thought I already
discouraged extending this binding to be generic. Particularly, we do
*not* want to codify things like 'linux,part-probe = "bcm47xxpart"',
which doesn't follow any accepted practice about naming. And you've also
failed to document any actual strings to put here, which hides the fact
that you're opening a can of worms.

Piece of a previous thread:
http://lists.infradead.org/pipermail/linux-mtd/2015-October/062613.html
[PATCH] mtd: document linux-specific partition parser DT binding

I attempted to resolve that here, but never made time to address the few
not-so-constructive comments I received:

https://mail-archive.com/linux-kernel@vger.kernel.org/msg1035539.html
[RFC PATCH 0/7] mtd: partitions: add of_match_table support

I'd still prefer that approach to supporting this shortcut for all MTDs.

Brian

> +
>  Example:
>  
>  	flash@0 {
>  		label = "System-firmware";
> +		linux,part-probe = "cmdlinepart", "ofpart";
>  
>  		/* flash type specific properties */
>  	};
> -- 
> 2.11.0
> 

Re: [PATCH V5 1/3] dt-bindings: mtd: document linux,part-probe property

[Boris Brezillon]

On Wed, 19 Apr 2017 13:37:05 -0700
Brian Norris <computersforpeace@gmail.com> wrote:

> Hi,
> 
> On Mon, Apr 17, 2017 at 09:47:44PM +0200, Rafał Miłecki wrote:
> > From: Rafał Miłecki <rafal@milecki.pl>
> > 
> > Support for this property has been introduced in 2010 with commit
> > 9d5da3a9b849 ("mtd: extend physmap_of to let the device tree specify the
> > parition probe") but it was never documented. Fix this by adding a
> > proper description and example.
> > 
> > Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> > Acked-by: Rob Herring <robh@kernel.org>
> > Acked-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > ---
> >  Documentation/devicetree/bindings/mtd/common.txt | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/mtd/common.txt b/Documentation/devicetree/bindings/mtd/common.txt
> > index fc068b923d7a..1ada70e718b2 100644
> > --- a/Documentation/devicetree/bindings/mtd/common.txt
> > +++ b/Documentation/devicetree/bindings/mtd/common.txt
> > @@ -6,10 +6,17 @@ Optional properties:
> >    controller based name) in order to ease flash device identification
> >    and/or describe what they are used for.
> >  
> > +- linux,part-probe: if present, this property should contain a list of strings
> > +  with partition probes to be used for the flash device. A role of partition
> > +  probe (parser) is to read/construct partition table and register found
> > +  partitions. Getting partition table may be platform or device specific so
> > +  various devices may use various Linux drivers for this purpose.  
> 
> Have you reviewed the old threads about this? I thought I already
> discouraged extending this binding to be generic. Particularly, we do
> *not* want to codify things like 'linux,part-probe = "bcm47xxpart"',
> which doesn't follow any accepted practice about naming. And you've also
> failed to document any actual strings to put here, which hides the fact
> that you're opening a can of worms.
> 
> Piece of a previous thread:
> http://lists.infradead.org/pipermail/linux-mtd/2015-October/062613.html
> [PATCH] mtd: document linux-specific partition parser DT binding
> 
> I attempted to resolve that here, but never made time to address the few
> not-so-constructive comments I received:
> 
> https://mail-archive.com/linux-kernel@vger.kernel.org/msg1035539.html
> [RFC PATCH 0/7] mtd: partitions: add of_match_table support

Oops, completely forgot you had proposed an alternative solution. Sorry
about that.