[PATCH 0/3] Improve mtdpart robustness

[PATCH 0/3] Improve mtdpart robustness

[Michal Suchanek]

Hello,

I hit a problem with ofpart getting confused by dt nodes unrelated to
parttioning and refusing the dt which in turn led to mtd refusing access to the
device altogether.

Compiling in a partitioning scheme should not prevent you access to the
unpartitioned device.

Please apply at least the functional patches.

Thanks

Michal

Michal Suchanek (3):
  mtd: mtdpart: add debug prints to partition parser.
  mtd: ofpart: do not fail probe when no partitions exist
  mtd: mtdpart: Do not fail mtd probe when parsing partitions fails.

 drivers/mtd/mtdpart.c | 9 ++++++---
 drivers/mtd/ofpart.c  | 4 ++--
 2 files changed, 8 insertions(+), 5 deletions(-)

-- 
2.1.4

[PATCH 3/3] mtd: mtdpart: Do not fail mtd probe when parsing partitions fails.

[Michal Suchanek]

Due to wrong assumption in ofpart ofpart fails on Exynos on SPI chips
with no partitions because the subnode containing controller data
confuses the ofpart parser.

Thus compiling in ofpart support automatically fails probing any SPI NOR
flash without partitions on Exynos. Compiling in a partitioning scheme
should not cause probe of otherwise valid device to fail.

Remove that failure possibility.

Signed-off-by: Michal Suchanek <hramrach@gmail.com>
---
 drivers/mtd/mtdpart.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
index a79c4f7..37a2b79 100644
--- a/drivers/mtd/mtdpart.c
+++ b/drivers/mtd/mtdpart.c
@@ -743,7 +743,6 @@ static const char * const default_mtd_part_types[] = {
  * partitions parsed out by the first parser.
  *
  * This function may return:
- * o a negative error code in case of failure
  * o zero if no partitions were found
  * o a positive number of found partitions, in which case on exit @pparts will
  *   point to an array containing this number of &struct mtd_info objects.
@@ -773,10 +772,10 @@ int parse_mtd_partitions(struct mtd_info *master, const char *const *types,
 		if (ret > 0) {
 			printk(KERN_NOTICE "%d %s partitions found on MTD device %s\n",
 			       ret, parser->name, master->name);
-			break;
+			return ret;
 		}
 	}
-	return ret;
+	return 0;
 }
 
 int mtd_is_partition(const struct mtd_info *mtd)
-- 
2.1.4

[PATCH 2/3] mtd: ofpart: do not fail probe when no partitions exist

[Michal Suchanek]

On Exynos it is necessary to set SPI controller parameters that apply to
a SPI slave in a DT subnode of the slave device.  Example:

flash: m25p80@0 {
        #address-cells = <1>;
        #size-cells = <1>;
        compatible = "jedec,spi-nor";
        reg = <0>;
        spi-max-frequency = <40000000>;
        m25p,fast-read;
        controller-data {
            samsung,spi-feedback-delay = <0>;
        };
};

The ofpart code checks if there are any subnodes on the MTD device DT
node and if so it tries to parse them as MTD partitions (example below)

flash@0 {
        #address-cells = <1>;
        #size-cells = <1>;

        partition@0 {
                label = "u-boot";
                reg = <0x0000000 0x100000>;
                read-only;
        };

        uimage@100000 {
                reg = <0x0100000 0x200000>;
        };
};

The controller-data node contains no partition information and no other
subnodes with partition information exist.

The ofpart code returns an error when there are subnodes of the flash DT
node but no partitions are found. This error is then propagated to
mtdpart which propagetes it to MTD probe which fails probing the flash
device.

Change this condition to a warning so that flash without partitions can
be accessed on Exynos with ofpart support compiled in.

Signed-off-by: Michal Suchanek <hramrach@gmail.com>

--
 - add more verbose explanation
---
 drivers/mtd/ofpart.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/ofpart.c b/drivers/mtd/ofpart.c
index aa26c32..a29d29f 100644
--- a/drivers/mtd/ofpart.c
+++ b/drivers/mtd/ofpart.c
@@ -94,10 +94,10 @@ static int parse_ofpart_partitions(struct mtd_info *master,
 
 	if (!i) {
 		of_node_put(pp);
-		pr_err("No valid partition found on %s\n", node->full_name);
+		pr_warn("No valid partition found on %s\n", node->full_name);
 		kfree(*pparts);
 		*pparts = NULL;
-		return -EINVAL;
+		return 0;
 	}
 
 	return nr_parts;
-- 
2.1.4

Re: [PATCH 2/3] mtd: ofpart: do not fail probe when no partitions exist

[Brian Norris]

On Mon, Jul 27, 2015 at 08:30:43PM -0000, Michal Suchanek wrote:
...
> The controller-data node contains no partition information and no other
> subnodes with partition information exist.
> 
> The ofpart code returns an error when there are subnodes of the flash DT
> node but no partitions are found. This error is then propagated to
> mtdpart which propagetes it to MTD probe which fails probing the flash
> device.
> 
> Change this condition to a warning so that flash without partitions can
> be accessed on Exynos with ofpart support compiled in.

You never replied to my suggestion here:

http://lists.infradead.org/pipermail/linux-arm-kernel/2015-June/352206.html

Particularly, "just define a proper compatibile property for [the
'controller-data'] subnode, and ofpart.c will naturally handle this".

> Signed-off-by: Michal Suchanek <hramrach@gmail.com>
> 
> --
>  - add more verbose explanation
> ---
>  drivers/mtd/ofpart.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/ofpart.c b/drivers/mtd/ofpart.c
> index aa26c32..a29d29f 100644
> --- a/drivers/mtd/ofpart.c
> +++ b/drivers/mtd/ofpart.c
> @@ -94,10 +94,10 @@ static int parse_ofpart_partitions(struct mtd_info *master,
>  
>  	if (!i) {
>  		of_node_put(pp);
> -		pr_err("No valid partition found on %s\n", node->full_name);
> +		pr_warn("No valid partition found on %s\n", node->full_name);
>  		kfree(*pparts);
>  		*pparts = NULL;
> -		return -EINVAL;
> +		return 0;

I don't really like this, since it can turn other invalid device trees
into a silent fallback. I'd really prefer we make it easy to tell the
difference between a MTD partition subnode and another foo-bar subnode.

>  	}
>  
>  	return nr_parts;

Brian

Re: [PATCH 2/3] mtd: ofpart: do not fail probe when no partitions exist

[Michal Suchanek]

On 27 July 2015 at 22:39, Brian Norris <computersforpeace@gmail.com> wrote:
> On Mon, Jul 27, 2015 at 08:30:43PM -0000, Michal Suchanek wrote:
> ...
>> The controller-data node contains no partition information and no other
>> subnodes with partition information exist.
>>
>> The ofpart code returns an error when there are subnodes of the flash DT
>> node but no partitions are found. This error is then propagated to
>> mtdpart which propagetes it to MTD probe which fails probing the flash
>> device.
>>
>> Change this condition to a warning so that flash without partitions can
>> be accessed on Exynos with ofpart support compiled in.
>
> You never replied to my suggestion here:
>
> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-June/352206.html
>
> Particularly, "just define a proper compatibile property for [the
> 'controller-data'] subnode, and ofpart.c will naturally handle this".
>
>> Signed-off-by: Michal Suchanek <hramrach@gmail.com>
>>
>> --
>>  - add more verbose explanation
>> ---
>>  drivers/mtd/ofpart.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mtd/ofpart.c b/drivers/mtd/ofpart.c
>> index aa26c32..a29d29f 100644
>> --- a/drivers/mtd/ofpart.c
>> +++ b/drivers/mtd/ofpart.c
>> @@ -94,10 +94,10 @@ static int parse_ofpart_partitions(struct mtd_info *master,
>>
>>       if (!i) {
>>               of_node_put(pp);
>> -             pr_err("No valid partition found on %s\n", node->full_name);
>> +             pr_warn("No valid partition found on %s\n", node->full_name);
>>               kfree(*pparts);
>>               *pparts = NULL;
>> -             return -EINVAL;
>> +             return 0;
>
> I don't really like this, since it can turn other invalid device trees
> into a silent fallback. I'd really prefer we make it easy to tell the
> difference between a MTD partition subnode and another foo-bar subnode.

Ok, so I don't find it reasonable to have one driver handle devicetree
nodes without compatible property and insist that no other driver ever
defines nodes without a compatible property.

So either drivers that parse nodes without a compatible property
should handle nodes that are meant to be used by other drivers or
*every* driver has to use a compatible property including ofpart and
then no collision can happen.

So this goes both ways - either deal with nodes that have no
compatible but are not for your driver or define a compatible for your
driver. The s3c64xx driver is ahead of ofpart here since it just tries
to read a property from a subnode of particular name and falls back to
default if not present.

All in all there is no 'foo-bar devicetree'. The fact that your driver
does not understand a particular part of a devicetree does not mean
the part is incorrect. You cannot know what drivers will emerge in the
future and what bindings will they use. In fact this very issue shows
that you make wrong assumptions about that.

Thanks

Michal

[PATCH 1/3] mtd: mtdpart: add debug prints to partition parser.

[Michal Suchanek]

The probe of a mtd device can fail when a partition parser returns
error. The failure due to partition parsing can be quite mysterious when
multiple partitioning schemes are comiled in and any of them can fail
the probe.

Add debug prints which show what parsers were tried and what they
returned.

Signed-off-by: Michal Suchanek <hramrach@gmail.com>
---
 drivers/mtd/mtdpart.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
index cafdb88..a79c4f7 100644
--- a/drivers/mtd/mtdpart.c
+++ b/drivers/mtd/mtdpart.c
@@ -759,12 +759,16 @@ int parse_mtd_partitions(struct mtd_info *master, const char *const *types,
 		types = default_mtd_part_types;
 
 	for ( ; ret <= 0 && *types; types++) {
+		pr_debug("Parsing partitions %s on %s", *types, master->name);
 		parser = get_partition_parser(*types);
 		if (!parser && !request_module("%s", *types))
 			parser = get_partition_parser(*types);
+		pr_debug("%s got parser %s", master->name,
+			 parser ? parser->name : NULL);
 		if (!parser)
 			continue;
 		ret = (*parser->parse_fn)(master, pparts, data);
+		pr_debug("%s parser %s: %i", master->name, parser->name, ret);
 		put_partition_parser(parser);
 		if (ret > 0) {
 			printk(KERN_NOTICE "%d %s partitions found on MTD device %s\n",
-- 
2.1.4