[PATCH v3 0/5] improve mtdpart robustness

[PATCH v3 0/5] improve mtdpart robustness

[Michal Suchanek]

Hello,

this patch series addresses

 - compiling in a mtd partitioning scheme may render a mtd device inacessible
 - ofpart can fail when subnodes of mtd device node exist
     eg. s3c64xx spi controller-data

v3 updates the last patch
  - address possible memory leak
  - use more generic name for partition node

Thanks

Michal

Michal Suchanek (5):
  mtd: mtdpart: add debug prints to partition parser.
  mtd: mtdpart: Do not fail mtd probe when parsing partitions fails.
  mtd: ofpart: update devicetree binding specification
  mtd: ofpart: document the lock flag.
  mtd: ofpart: move ofpart partitions to a dedicated dt node

 .../devicetree/bindings/mtd/partition.txt          | 69 +++++++++++++---------
 drivers/mtd/mtdpart.c                              | 14 ++++-
 drivers/mtd/ofpart.c                               | 63 ++++++++++++++------
 3 files changed, 99 insertions(+), 47 deletions(-)

-- 
2.1.4


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

[PATCH v3 2/5] 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 when MTD_PARTITIONED_MASTER is set.

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

---
v2:

 - only allow partition parsing failure when MTD_PARTITIONED_MASTER is
   set
---
 drivers/mtd/mtdpart.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
index 31888c2..6eafbe9 100644
--- a/drivers/mtd/mtdpart.c
+++ b/drivers/mtd/mtdpart.c
@@ -774,10 +774,15 @@ 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;
+		}
+		if (!IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER) && (ret < 0)) {
+			pr_err("Error parsing %s partitions on %s\n",
+			       parser->name, master->name);
+			return ret;
 		}
 	}
-	return ret;
+	return 0;
 }
 
 int mtd_is_partition(const struct mtd_info *mtd)
-- 
2.1.4


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

Re: [PATCH v3 2/5] mtd: mtdpart: Do not fail mtd probe when parsing partitions fails.

[Brian Norris]

Hi Michal,

Sorry for the very long waits here. Unfortunately, I've been saying that
a lot lately, as I haven't had a lot of time and have generated a lot of
backlog. I really like that you've been working on this though, since
there are definitely problems here.

On Tue, Aug 18, 2015 at 03:34:07PM -0000, Michal Suchanek wrote:
> 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 when MTD_PARTITIONED_MASTER is set.
> 
> Signed-off-by: Michal Suchanek <hramrach-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> 
> ---
> v2:
> 
>  - only allow partition parsing failure when MTD_PARTITIONED_MASTER is
>    set

Why the special case for MTD_PARTITIONED_MASTER? I don't think this case
should be much different. In either case, we should actually be falling
back to just exposing the unpartitioned master device. We already do
that when no partitions are found (but no error codes) in either
MTD_PARTITIONED_MASTER={y,n}. We just need to do that for error cases
too, I think.

> ---
>  drivers/mtd/mtdpart.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
> index 31888c2..6eafbe9 100644
> --- a/drivers/mtd/mtdpart.c
> +++ b/drivers/mtd/mtdpart.c
> @@ -774,10 +774,15 @@ 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;
> +		}
> +		if (!IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER) && (ret < 0)) {
> +			pr_err("Error parsing %s partitions on %s\n",
> +			       parser->name, master->name);
> +			return ret;

I think this hunk should be more like the hunk from the previous
versions; don't do a separate CONFIG_xxx special case.

I also think this has one other flaw: it's best if we don't completely
ignore all failures. If we have one or more partitioning failures, this
should show up somehow in the log at least, and not only with the
pr_debug() messages from patch 1. I like the logic for the block
subsystem for failures (see check_partition() in
block/partitions/check.c). Skip a parser quietly if a later one
succeeds, but report an error with a warning printk if none succeed.

>  		}
>  	}
> -	return ret;
> +	return 0;
>  }
>  
>  int mtd_is_partition(const struct mtd_info *mtd)

All in all, I think my suggestions would look something like the
following alternative patch. I haven't tested it yet.

Brian

(git-format-patch pasted below)

>From 53b60f31a2a0f2a7e8220a4aff47457248bccbcf Mon Sep 17 00:00:00 2001
From: Brian Norris <computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Date: Sun, 11 Oct 2015 10:25:23 -0700
Subject: [PATCH] mtd: mtdpart: Do not fail mtd probe when parsing partitions
 fails.

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.

Instead, let's do the following:
 * try parsers until one succeeds
 * if no parser succeeds, report the first error we saw
 * even in the failure case, allow MTD to probe, with fallback
   partitions or no partitions at all -- the master device will still be
   registered

Issue report and comments initially by Michal Suchanek.

Reported-by: Michal Suchanek <hramrach-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Signed-off-by: Brian Norris <computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/mtd/mtdcore.c |  6 ++++--
 drivers/mtd/mtdpart.c | 14 ++++++++++----
 2 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 8ec4f81e9190..bb50e486e064 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -589,8 +589,10 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
 	}
 	/* Didn't come up with either parsed OR fallback partitions */
 	if (ret < 0) {
-		pr_info("mtd: failed to find partitions\n");
-		goto out;
+		pr_info("mtd: failed to find partitions; one or more parsers reports errors (%d)\n",
+			ret);
+		/* Don't abort on errors; we can still use unpartitioned MTD */
+		ret = 0;
 	}
 
 	ret = mtd_add_device_partitions(mtd, real_parts, ret);
diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
index f5279ea6dc87..f8ba153f63bf 100644
--- a/drivers/mtd/mtdpart.c
+++ b/drivers/mtd/mtdpart.c
@@ -755,12 +755,12 @@ int parse_mtd_partitions(struct mtd_info *master, const char *const *types,
 			 struct mtd_part_parser_data *data)
 {
 	struct mtd_part_parser *parser;
-	int ret = 0;
+	int ret, err = 0;
 
 	if (!types)
 		types = default_mtd_part_types;
 
-	for ( ; ret <= 0 && *types; types++) {
+	for ( ; *types; types++) {
 		pr_debug("%s: parsing partitions %s\n", master->name, *types);
 		parser = get_partition_parser(*types);
 		if (!parser && !request_module("%s", *types))
@@ -776,10 +776,16 @@ 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;
 		}
+		/*
+		 * Stash the first error we see; only report it if no parser
+		 * succeeds
+		 */
+		if (ret < 0 && !err)
+			err = ret;
 	}
-	return ret;
+	return err;
 }
 
 int mtd_is_partition(const struct mtd_info *mtd)
-- 
2.6.0.rc2.230.g3dd15c0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/5] mtd: mtdpart: Do not fail mtd probe when parsing partitions fails.

[Brian Norris]

On Sun, Oct 11, 2015 at 01:03:47PM -0700, Brian Norris wrote:
> All in all, I think my suggestions would look something like the
> following alternative patch. I haven't tested it yet.
> 
> Brian
> 
> (git-format-patch pasted below)
> 
> From 53b60f31a2a0f2a7e8220a4aff47457248bccbcf Mon Sep 17 00:00:00 2001
> From: Brian Norris <computersforpeace@gmail.com>
> Date: Sun, 11 Oct 2015 10:25:23 -0700
> Subject: [PATCH] mtd: mtdpart: Do not fail mtd probe when parsing partitions
>  fails.
> 
> 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.
> 
> Instead, let's do the following:
>  * try parsers until one succeeds
>  * if no parser succeeds, report the first error we saw
>  * even in the failure case, allow MTD to probe, with fallback
>    partitions or no partitions at all -- the master device will still be
>    registered
> 
> Issue report and comments initially by Michal Suchanek.
> 
> Reported-by: Michal Suchanek <hramrach@gmail.com>
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> ---
>  drivers/mtd/mtdcore.c |  6 ++++--
>  drivers/mtd/mtdpart.c | 14 ++++++++++----
>  2 files changed, 14 insertions(+), 6 deletions(-)

Tested, and applied this version to l2-mtd.git

[PATCH v3 1/5] 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>
---
v2:

 - reformat debug messages
---
 drivers/mtd/mtdpart.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
index cafdb88..31888c2 100644
--- a/drivers/mtd/mtdpart.c
+++ b/drivers/mtd/mtdpart.c
@@ -759,12 +759,17 @@ int parse_mtd_partitions(struct mtd_info *master, const char *const *types,
 		types = default_mtd_part_types;
 
 	for ( ; ret <= 0 && *types; types++) {
+		pr_debug("%s: parsing partitions %s\n", master->name, *types);
 		parser = get_partition_parser(*types);
 		if (!parser && !request_module("%s", *types))
 			parser = get_partition_parser(*types);
+		pr_debug("%s: got parser %s\n", master->name,
+			 parser ? parser->name : NULL);
 		if (!parser)
 			continue;
 		ret = (*parser->parse_fn)(master, pparts, data);
+		pr_debug("%s: parser %s: %i\n",
+			 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


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

Re: [PATCH v3 1/5] mtd: mtdpart: add debug prints to partition parser.

[Brian Norris]

On Tue, Aug 18, 2015 at 03:34:07PM -0000, Michal Suchanek wrote:
> 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>
> ---
> v2:
> 
>  - reformat debug messages

Looks OK to me. Applied to l2-mtd.git.

[PATCH v3 3/5] mtd: ofpart: update devicetree binding specification

[Michal Suchanek]

To avoid conflict with other drivers using subnodes of the mtd device
create only one ofpart-specific node rather than any number of
arbitrary partition subnodes.

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

 - rename DT node ofpart -> partitions
---
 .../devicetree/bindings/mtd/partition.txt          | 68 +++++++++++++---------
 1 file changed, 40 insertions(+), 28 deletions(-)

diff --git a/Documentation/devicetree/bindings/mtd/partition.txt b/Documentation/devicetree/bindings/mtd/partition.txt
index 8e5557d..8c2aff7 100644
--- a/Documentation/devicetree/bindings/mtd/partition.txt
+++ b/Documentation/devicetree/bindings/mtd/partition.txt
@@ -4,10 +4,16 @@ Partitions can be represented by sub-nodes of an mtd device. This can be used
 on platforms which have strong conventions about which portions of a flash are
 used for what purposes, but which don't use an on-flash partition table such
 as RedBoot.
+
+The partition table should be partitions subnode of the mtd node. Partitions are
+defined in subnodes of the partitions node.
+
+For backwards compatibility partitions as direct subnodes of the mtd device are
+supported. This use is discouraged.
 NOTE: if the sub-node has a compatible string, then it is not a partition.
 
-#address-cells & #size-cells must both be present in the mtd device. There are
-two valid values for both:
+#address-cells & #size-cells must both be present in the partitions subnode of the
+mtd device. There are two valid values for both:
 <1>: for partitions that require a single 32-bit cell to represent their
      size/address (aka the value is below 4 GiB)
 <2>: for partitions that require two 32-bit cells to represent their
@@ -28,44 +34,50 @@ Examples:
 
 
 flash@0 {
-	#address-cells = <1>;
-	#size-cells = <1>;
+	partitions {
+		#address-cells = <1>;
+		#size-cells = <1>;
 
-	partition@0 {
-		label = "u-boot";
-		reg = <0x0000000 0x100000>;
-		read-only;
-	};
+		partition@0 {
+			label = "u-boot";
+			reg = <0x0000000 0x100000>;
+			read-only;
+		};
 
-	uimage@100000 {
-		reg = <0x0100000 0x200000>;
+		uimage@100000 {
+			reg = <0x0100000 0x200000>;
+		};
 	};
 };
 
 flash@1 {
-	#address-cells = <1>;
-	#size-cells = <2>;
+	partitions {
+		#address-cells = <1>;
+		#size-cells = <2>;
 
-	/* a 4 GiB partition */
-	partition@0 {
-		label = "filesystem";
-		reg = <0x00000000 0x1 0x00000000>;
+		/* a 4 GiB partition */
+		partition@0 {
+			label = "filesystem";
+			reg = <0x00000000 0x1 0x00000000>;
+		};
 	};
 };
 
 flash@2 {
-	#address-cells = <2>;
-	#size-cells = <2>;
+	partitions {
+		#address-cells = <2>;
+		#size-cells = <2>;
 
-	/* an 8 GiB partition */
-	partition@0 {
-		label = "filesystem #1";
-		reg = <0x0 0x00000000 0x2 0x00000000>;
-	};
+		/* an 8 GiB partition */
+		partition@0 {
+			label = "filesystem #1";
+			reg = <0x0 0x00000000 0x2 0x00000000>;
+		};
 
-	/* a 4 GiB partition */
-	partition@200000000 {
-		label = "filesystem #2";
-		reg = <0x2 0x00000000 0x1 0x00000000>;
+		/* a 4 GiB partition */
+		partition@200000000 {
+			label = "filesystem #2";
+			reg = <0x2 0x00000000 0x1 0x00000000>;
+		};
 	};
 };
-- 
2.1.4


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

Re: [PATCH v3 3/5] mtd: ofpart: update devicetree binding specification

[Brian Norris]

Hi DT maintainers,

It's a bit hypocritical of me, since I've been a slow reviewer as well,
but... can we get some review on this one? Usually, I'm comfortable
taking driver DT bindings without your review, but this one is a bit
more generic and is more far-reaching than the average driver.

I'm not a big fan of this change, and I don't quite understand why the
bus driver (the SPI bus, which is a level up from the SPI device / MTD
node) can specify its grandchildren (see spi-samsung.txt). But given the
constraints, I think Michal's solution is OK. And I do agree that MTD's
ofpart should be bit more specific.

Anyway, a quick look and an Ack/Nak would be appreciated.

Thanks,
Brian

On Tue, Aug 18, 2015 at 03:34:08PM -0000, Michal Suchanek wrote:
> To avoid conflict with other drivers using subnodes of the mtd device
> create only one ofpart-specific node rather than any number of
> arbitrary partition subnodes.
> 
> Signed-off-by: Michal Suchanek <hramrach-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
> v3:
> 
>  - rename DT node ofpart -> partitions
> ---
>  .../devicetree/bindings/mtd/partition.txt          | 68 +++++++++++++---------
>  1 file changed, 40 insertions(+), 28 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mtd/partition.txt b/Documentation/devicetree/bindings/mtd/partition.txt
> index 8e5557d..8c2aff7 100644
> --- a/Documentation/devicetree/bindings/mtd/partition.txt
> +++ b/Documentation/devicetree/bindings/mtd/partition.txt
> @@ -4,10 +4,16 @@ Partitions can be represented by sub-nodes of an mtd device. This can be used
>  on platforms which have strong conventions about which portions of a flash are
>  used for what purposes, but which don't use an on-flash partition table such
>  as RedBoot.
> +
> +The partition table should be partitions subnode of the mtd node. Partitions are
> +defined in subnodes of the partitions node.
> +
> +For backwards compatibility partitions as direct subnodes of the mtd device are
> +supported. This use is discouraged.
>  NOTE: if the sub-node has a compatible string, then it is not a partition.
>  
> -#address-cells & #size-cells must both be present in the mtd device. There are
> -two valid values for both:
> +#address-cells & #size-cells must both be present in the partitions subnode of the
> +mtd device. There are two valid values for both:
>  <1>: for partitions that require a single 32-bit cell to represent their
>       size/address (aka the value is below 4 GiB)
>  <2>: for partitions that require two 32-bit cells to represent their
> @@ -28,44 +34,50 @@ Examples:
>  
>  
>  flash@0 {
> -	#address-cells = <1>;
> -	#size-cells = <1>;
> +	partitions {
> +		#address-cells = <1>;
> +		#size-cells = <1>;
>  
> -	partition@0 {
> -		label = "u-boot";
> -		reg = <0x0000000 0x100000>;
> -		read-only;
> -	};
> +		partition@0 {
> +			label = "u-boot";
> +			reg = <0x0000000 0x100000>;
> +			read-only;
> +		};
>  
> -	uimage@100000 {
> -		reg = <0x0100000 0x200000>;
> +		uimage@100000 {
> +			reg = <0x0100000 0x200000>;
> +		};
>  	};
>  };
>  
>  flash@1 {
> -	#address-cells = <1>;
> -	#size-cells = <2>;
> +	partitions {
> +		#address-cells = <1>;
> +		#size-cells = <2>;
>  
> -	/* a 4 GiB partition */
> -	partition@0 {
> -		label = "filesystem";
> -		reg = <0x00000000 0x1 0x00000000>;
> +		/* a 4 GiB partition */
> +		partition@0 {
> +			label = "filesystem";
> +			reg = <0x00000000 0x1 0x00000000>;
> +		};
>  	};
>  };
>  
>  flash@2 {
> -	#address-cells = <2>;
> -	#size-cells = <2>;
> +	partitions {
> +		#address-cells = <2>;
> +		#size-cells = <2>;
>  
> -	/* an 8 GiB partition */
> -	partition@0 {
> -		label = "filesystem #1";
> -		reg = <0x0 0x00000000 0x2 0x00000000>;
> -	};
> +		/* an 8 GiB partition */
> +		partition@0 {
> +			label = "filesystem #1";
> +			reg = <0x0 0x00000000 0x2 0x00000000>;
> +		};
>  
> -	/* a 4 GiB partition */
> -	partition@200000000 {
> -		label = "filesystem #2";
> -		reg = <0x2 0x00000000 0x1 0x00000000>;
> +		/* a 4 GiB partition */
> +		partition@200000000 {
> +			label = "filesystem #2";
> +			reg = <0x2 0x00000000 0x1 0x00000000>;
> +		};
>  	};
>  };
> -- 
> 2.1.4
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 3/5] mtd: ofpart: update devicetree binding specification

[Brian Norris]

On Sun, Oct 11, 2015 at 01:04:02PM -0700, Brian Norris wrote:
> Hi DT maintainers,

Last call: I'd like a reviewer that
(1) does DT reviews somewhat regularly and
(2) is not me.

If I can't get one soon, I'll take this for 4.4.

Thanks,
Brian

> It's a bit hypocritical of me, since I've been a slow reviewer as well,
> but... can we get some review on this one? Usually, I'm comfortable
> taking driver DT bindings without your review, but this one is a bit
> more generic and is more far-reaching than the average driver.
> 
> I'm not a big fan of this change, and I don't quite understand why the
> bus driver (the SPI bus, which is a level up from the SPI device / MTD
> node) can specify its grandchildren (see spi-samsung.txt). But given the
> constraints, I think Michal's solution is OK. And I do agree that MTD's
> ofpart should be bit more specific.
> 
> Anyway, a quick look and an Ack/Nak would be appreciated.
> 
> Thanks,
> Brian
> 
> On Tue, Aug 18, 2015 at 03:34:08PM -0000, Michal Suchanek wrote:
> > To avoid conflict with other drivers using subnodes of the mtd device
> > create only one ofpart-specific node rather than any number of
> > arbitrary partition subnodes.
> > 
> > Signed-off-by: Michal Suchanek <hramrach-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > ---
> > v3:
> > 
> >  - rename DT node ofpart -> partitions
> > ---
> >  .../devicetree/bindings/mtd/partition.txt          | 68 +++++++++++++---------
> >  1 file changed, 40 insertions(+), 28 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/mtd/partition.txt b/Documentation/devicetree/bindings/mtd/partition.txt
> > index 8e5557d..8c2aff7 100644
> > --- a/Documentation/devicetree/bindings/mtd/partition.txt
> > +++ b/Documentation/devicetree/bindings/mtd/partition.txt
> > @@ -4,10 +4,16 @@ Partitions can be represented by sub-nodes of an mtd device. This can be used
> >  on platforms which have strong conventions about which portions of a flash are
> >  used for what purposes, but which don't use an on-flash partition table such
> >  as RedBoot.
> > +
> > +The partition table should be partitions subnode of the mtd node. Partitions are
> > +defined in subnodes of the partitions node.
> > +
> > +For backwards compatibility partitions as direct subnodes of the mtd device are
> > +supported. This use is discouraged.
> >  NOTE: if the sub-node has a compatible string, then it is not a partition.
> >  
> > -#address-cells & #size-cells must both be present in the mtd device. There are
> > -two valid values for both:
> > +#address-cells & #size-cells must both be present in the partitions subnode of the
> > +mtd device. There are two valid values for both:
> >  <1>: for partitions that require a single 32-bit cell to represent their
> >       size/address (aka the value is below 4 GiB)
> >  <2>: for partitions that require two 32-bit cells to represent their
> > @@ -28,44 +34,50 @@ Examples:
> >  
> >  
> >  flash@0 {
> > -	#address-cells = <1>;
> > -	#size-cells = <1>;
> > +	partitions {
> > +		#address-cells = <1>;
> > +		#size-cells = <1>;
> >  
> > -	partition@0 {
> > -		label = "u-boot";
> > -		reg = <0x0000000 0x100000>;
> > -		read-only;
> > -	};
> > +		partition@0 {
> > +			label = "u-boot";
> > +			reg = <0x0000000 0x100000>;
> > +			read-only;
> > +		};
> >  
> > -	uimage@100000 {
> > -		reg = <0x0100000 0x200000>;
> > +		uimage@100000 {
> > +			reg = <0x0100000 0x200000>;
> > +		};
> >  	};
> >  };
> >  
> >  flash@1 {
> > -	#address-cells = <1>;
> > -	#size-cells = <2>;
> > +	partitions {
> > +		#address-cells = <1>;
> > +		#size-cells = <2>;
> >  
> > -	/* a 4 GiB partition */
> > -	partition@0 {
> > -		label = "filesystem";
> > -		reg = <0x00000000 0x1 0x00000000>;
> > +		/* a 4 GiB partition */
> > +		partition@0 {
> > +			label = "filesystem";
> > +			reg = <0x00000000 0x1 0x00000000>;
> > +		};
> >  	};
> >  };
> >  
> >  flash@2 {
> > -	#address-cells = <2>;
> > -	#size-cells = <2>;
> > +	partitions {
> > +		#address-cells = <2>;
> > +		#size-cells = <2>;
> >  
> > -	/* an 8 GiB partition */
> > -	partition@0 {
> > -		label = "filesystem #1";
> > -		reg = <0x0 0x00000000 0x2 0x00000000>;
> > -	};
> > +		/* an 8 GiB partition */
> > +		partition@0 {
> > +			label = "filesystem #1";
> > +			reg = <0x0 0x00000000 0x2 0x00000000>;
> > +		};
> >  
> > -	/* a 4 GiB partition */
> > -	partition@200000000 {
> > -		label = "filesystem #2";
> > -		reg = <0x2 0x00000000 0x1 0x00000000>;
> > +		/* a 4 GiB partition */
> > +		partition@200000000 {
> > +			label = "filesystem #2";
> > +			reg = <0x2 0x00000000 0x1 0x00000000>;
> > +		};
> >  	};
> >  };
> > -- 
> > 2.1.4
> > 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 3/5] mtd: ofpart: update devicetree binding specification

[Rob Herring]

On Sun, Oct 11, 2015 at 3:04 PM, Brian Norris
<computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> Hi DT maintainers,
>
> It's a bit hypocritical of me, since I've been a slow reviewer as well,
> but... can we get some review on this one? Usually, I'm comfortable
> taking driver DT bindings without your review, but this one is a bit
> more generic and is more far-reaching than the average driver.

Sorry, missed this one. This would be a good one to send to
devicetree-spec to BTW.

> I'm not a big fan of this change, and I don't quite understand why the
> bus driver (the SPI bus, which is a level up from the SPI device / MTD
> node) can specify its grandchildren (see spi-samsung.txt). But given the

That's just an example. I just would change it.

> constraints, I think Michal's solution is OK. And I do agree that MTD's
> ofpart should be bit more specific.
>
> Anyway, a quick look and an Ack/Nak would be appreciated.

Looks fine to me:

Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

>
> Thanks,
> Brian
>
> On Tue, Aug 18, 2015 at 03:34:08PM -0000, Michal Suchanek wrote:
>> To avoid conflict with other drivers using subnodes of the mtd device
>> create only one ofpart-specific node rather than any number of
>> arbitrary partition subnodes.
>>
>> Signed-off-by: Michal Suchanek <hramrach-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>> v3:
>>
>>  - rename DT node ofpart -> partitions
>> ---
>>  .../devicetree/bindings/mtd/partition.txt          | 68 +++++++++++++---------
>>  1 file changed, 40 insertions(+), 28 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/mtd/partition.txt b/Documentation/devicetree/bindings/mtd/partition.txt
>> index 8e5557d..8c2aff7 100644
>> --- a/Documentation/devicetree/bindings/mtd/partition.txt
>> +++ b/Documentation/devicetree/bindings/mtd/partition.txt
>> @@ -4,10 +4,16 @@ Partitions can be represented by sub-nodes of an mtd device. This can be used
>>  on platforms which have strong conventions about which portions of a flash are
>>  used for what purposes, but which don't use an on-flash partition table such
>>  as RedBoot.
>> +
>> +The partition table should be partitions subnode of the mtd node. Partitions are
>> +defined in subnodes of the partitions node.
>> +
>> +For backwards compatibility partitions as direct subnodes of the mtd device are
>> +supported. This use is discouraged.
>>  NOTE: if the sub-node has a compatible string, then it is not a partition.
>>
>> -#address-cells & #size-cells must both be present in the mtd device. There are
>> -two valid values for both:
>> +#address-cells & #size-cells must both be present in the partitions subnode of the
>> +mtd device. There are two valid values for both:
>>  <1>: for partitions that require a single 32-bit cell to represent their
>>       size/address (aka the value is below 4 GiB)
>>  <2>: for partitions that require two 32-bit cells to represent their
>> @@ -28,44 +34,50 @@ Examples:
>>
>>
>>  flash@0 {
>> -     #address-cells = <1>;
>> -     #size-cells = <1>;
>> +     partitions {
>> +             #address-cells = <1>;
>> +             #size-cells = <1>;
>>
>> -     partition@0 {
>> -             label = "u-boot";
>> -             reg = <0x0000000 0x100000>;
>> -             read-only;
>> -     };
>> +             partition@0 {
>> +                     label = "u-boot";
>> +                     reg = <0x0000000 0x100000>;
>> +                     read-only;
>> +             };
>>
>> -     uimage@100000 {
>> -             reg = <0x0100000 0x200000>;
>> +             uimage@100000 {
>> +                     reg = <0x0100000 0x200000>;
>> +             };
>>       };
>>  };
>>
>>  flash@1 {
>> -     #address-cells = <1>;
>> -     #size-cells = <2>;
>> +     partitions {
>> +             #address-cells = <1>;
>> +             #size-cells = <2>;
>>
>> -     /* a 4 GiB partition */
>> -     partition@0 {
>> -             label = "filesystem";
>> -             reg = <0x00000000 0x1 0x00000000>;
>> +             /* a 4 GiB partition */
>> +             partition@0 {
>> +                     label = "filesystem";
>> +                     reg = <0x00000000 0x1 0x00000000>;
>> +             };
>>       };
>>  };
>>
>>  flash@2 {
>> -     #address-cells = <2>;
>> -     #size-cells = <2>;
>> +     partitions {
>> +             #address-cells = <2>;
>> +             #size-cells = <2>;
>>
>> -     /* an 8 GiB partition */
>> -     partition@0 {
>> -             label = "filesystem #1";
>> -             reg = <0x0 0x00000000 0x2 0x00000000>;
>> -     };
>> +             /* an 8 GiB partition */
>> +             partition@0 {
>> +                     label = "filesystem #1";
>> +                     reg = <0x0 0x00000000 0x2 0x00000000>;
>> +             };
>>
>> -     /* a 4 GiB partition */
>> -     partition@200000000 {
>> -             label = "filesystem #2";
>> -             reg = <0x2 0x00000000 0x1 0x00000000>;
>> +             /* a 4 GiB partition */
>> +             partition@200000000 {
>> +                     label = "filesystem #2";
>> +                     reg = <0x2 0x00000000 0x1 0x00000000>;
>> +             };
>>       };
>>  };
>> --
>> 2.1.4
>>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 3/5] mtd: ofpart: update devicetree binding specification

[Brian Norris]

Hi Rob,

Thanks for the review.

On Mon, Oct 26, 2015 at 11:35:24PM -0500, Rob Herring wrote:
> On Sun, Oct 11, 2015 at 3:04 PM, Brian Norris
> <computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > Hi DT maintainers,
> >
> > It's a bit hypocritical of me, since I've been a slow reviewer as well,
> > but... can we get some review on this one? Usually, I'm comfortable
> > taking driver DT bindings without your review, but this one is a bit
> > more generic and is more far-reaching than the average driver.
> 
> Sorry, missed this one. This would be a good one to send to
> devicetree-spec to BTW.

I'm not very familiar with that list. With the intention of getting into
an ePAPR (or similar) spec? Or just for additional review? If the
former, would you suggest codifying both the old and the new, or just
the new?

Brian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 3/5] mtd: ofpart: update devicetree binding specification

[Rob Herring]

On Tue, Oct 27, 2015 at 5:50 PM, Brian Norris
<computersforpeace@gmail.com> wrote:
> Hi Rob,
>
> Thanks for the review.
>
> On Mon, Oct 26, 2015 at 11:35:24PM -0500, Rob Herring wrote:
>> On Sun, Oct 11, 2015 at 3:04 PM, Brian Norris
>> <computersforpeace@gmail.com> wrote:
>> > Hi DT maintainers,
>> >
>> > It's a bit hypocritical of me, since I've been a slow reviewer as well,
>> > but... can we get some review on this one? Usually, I'm comfortable
>> > taking driver DT bindings without your review, but this one is a bit
>> > more generic and is more far-reaching than the average driver.
>>
>> Sorry, missed this one. This would be a good one to send to
>> devicetree-spec to BTW.
>
> I'm not very familiar with that list. With the intention of getting into
> an ePAPR (or similar) spec? Or just for additional review? If the
> former, would you suggest codifying both the old and the new, or just
> the new?

I would say it is for anything not driver specific. It was created to
separate out the driver binding firehose from the common bindings and
get more non-Linux participation on those. Perhaps it was poorly
named. I want to improve the split in docs so the appropriate list is
used.

Sending to both devicetree and devicetree-spec is fine.

Rob

[PATCH v3 4/5] mtd: ofpart: document the lock flag.

[Michal Suchanek]

The lock flag of ofpart is undocumented. Add to binding doc.

Signed-off-by: Michal Suchanek <hramrach@gmail.com>
---
 Documentation/devicetree/bindings/mtd/partition.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/mtd/partition.txt b/Documentation/devicetree/bindings/mtd/partition.txt
index 8c2aff7..7fa9c08 100644
--- a/Documentation/devicetree/bindings/mtd/partition.txt
+++ b/Documentation/devicetree/bindings/mtd/partition.txt
@@ -29,6 +29,7 @@ Optional properties:
   partition should only be mounted read-only. This is usually used for flash
   partitions containing early-boot firmware images or data which should not be
   clobbered.
+- lock : Clear always locked after reset flag
 
 Examples:
 
-- 
2.1.4


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

Re: [PATCH v3 4/5] mtd: ofpart: document the lock flag.

[Brian Norris]

On Tue, Aug 18, 2015 at 03:34:08PM -0000, Michal Suchanek wrote:
> The lock flag of ofpart is undocumented. Add to binding doc.

Good catch. There are a lot of small corners of very old code that never
really got reviewed properly, I expect...

(And the flag looks very odd. Why exactly is it in the partitions?)

And now that I'm looking further...does this flag even *do* anything?
AFAICT, it doesn't set the master device flags -- only the partition
flags. But MTD drivers currently never see the partition flags -- they
only see the master struct mtd_info. I think the only way anyone could
observe the effect of this flag is to read the MTD flags from sysfs. And
that's pretty useless.

If my understanding is correct, then I'd rather completely remove the
code that "handles" this flag, rather than codify it in the docs.

> Signed-off-by: Michal Suchanek <hramrach@gmail.com>
> ---
>  Documentation/devicetree/bindings/mtd/partition.txt | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/mtd/partition.txt b/Documentation/devicetree/bindings/mtd/partition.txt
> index 8c2aff7..7fa9c08 100644
> --- a/Documentation/devicetree/bindings/mtd/partition.txt
> +++ b/Documentation/devicetree/bindings/mtd/partition.txt
> @@ -29,6 +29,7 @@ Optional properties:
>    partition should only be mounted read-only. This is usually used for flash
>    partitions containing early-boot firmware images or data which should not be
>    clobbered.
> +- lock : Clear always locked after reset flag

This seems more like a SW description. I've reworded slightly locally.

Is my above analysis sensible? If so, I'll patch this to kill the DT
parsing code for "lock" entirely. If I'm wrong, then I can push this
patch. Let me know what you think.

Brian

P.S. IIUC, then most of the 'mask_flags' stuff for partitioned devices
really does nothing. These flags don't seem to ever be checked. Ugh.

Re: [PATCH v3 4/5] mtd: ofpart: document the lock flag.

[Brian Norris]

On Sun, Oct 11, 2015 at 01:04:12PM -0700, Brian Norris wrote:
> On Tue, Aug 18, 2015 at 03:34:08PM -0000, Michal Suchanek wrote:
> > The lock flag of ofpart is undocumented. Add to binding doc.
> 
> Good catch. There are a lot of small corners of very old code that never
> really got reviewed properly, I expect...
> 
> (And the flag looks very odd. Why exactly is it in the partitions?)
> 
> And now that I'm looking further...does this flag even *do* anything?
> AFAICT, it doesn't set the master device flags -- only the partition
> flags. But MTD drivers currently never see the partition flags -- they
> only see the master struct mtd_info. I think the only way anyone could
> observe the effect of this flag is to read the MTD flags from sysfs. And
> that's pretty useless.
> 
> If my understanding is correct, then I'd rather completely remove the
> code that "handles" this flag, rather than codify it in the docs.

I've tested and confirmed: this only sets the flags for the partition
(*NOT* for the master device), so the only visible effect of this
property is to change sysfs flags. I'll send out a patch to kill this
property entirely.

Brian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 5/5] mtd: ofpart: move ofpart partitions to a dedicated dt node

[Michal Suchanek]

Parsing direct subnodes of a mtd device as partitions is unreliable
since the mtd device is also part of its bus subsystem and can contain
bus data in subnodes.

Move ofpart data to a subnode of its own so it is clear which data is
part of the partition layout.

Signed-off-by: Michal Suchanek <hramrach@gmail.com>
---
v2:
 - fix indent
v3:
 - fix possible memory leak when parittions are allocated but no valid
   partition is found
 - rename dt node ofpart -> partitions
---
 drivers/mtd/ofpart.c | 63 ++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 46 insertions(+), 17 deletions(-)

diff --git a/drivers/mtd/ofpart.c b/drivers/mtd/ofpart.c
index aa26c32..669c345 100644
--- a/drivers/mtd/ofpart.c
+++ b/drivers/mtd/ofpart.c
@@ -29,23 +29,33 @@ static int parse_ofpart_partitions(struct mtd_info *master,
 				   struct mtd_partition **pparts,
 				   struct mtd_part_parser_data *data)
 {
-	struct device_node *node;
+	struct device_node *mtd_node;
+	struct device_node *ofpart_node;
 	const char *partname;
 	struct device_node *pp;
-	int nr_parts, i;
+	int nr_parts, i, ret = 0;
+	bool dedicated = true;
 
 
 	if (!data)
 		return 0;
 
-	node = data->of_node;
-	if (!node)
+	mtd_node = data->of_node;
+	if (!mtd_node)
 		return 0;
 
+	ofpart_node = of_get_child_by_name(mtd_node, "partitions");
+	if (!ofpart_node) {
+		pr_warn("%s: 'partitions' subnode not found on %s. Trying to parse direct subnodes as partitions.\n",
+			master->name, mtd_node->full_name);
+		ofpart_node = mtd_node;
+		dedicated = false;
+	}
+
 	/* First count the subnodes */
 	nr_parts = 0;
-	for_each_child_of_node(node,  pp) {
-		if (node_has_compatible(pp))
+	for_each_child_of_node(ofpart_node,  pp) {
+		if (!dedicated && node_has_compatible(pp))
 			continue;
 
 		nr_parts++;
@@ -59,22 +69,36 @@ static int parse_ofpart_partitions(struct mtd_info *master,
 		return -ENOMEM;
 
 	i = 0;
-	for_each_child_of_node(node,  pp) {
+	for_each_child_of_node(ofpart_node,  pp) {
 		const __be32 *reg;
 		int len;
 		int a_cells, s_cells;
 
-		if (node_has_compatible(pp))
+		if (!dedicated && node_has_compatible(pp))
 			continue;
 
 		reg = of_get_property(pp, "reg", &len);
 		if (!reg) {
-			nr_parts--;
-			continue;
+			if (dedicated) {
+				pr_debug("%s: ofpart partition %s (%s) missing reg property.\n",
+					 master->name, pp->full_name,
+					 mtd_node->full_name);
+				goto ofpart_fail;
+			} else {
+				nr_parts--;
+				continue;
+			}
 		}
 
 		a_cells = of_n_addr_cells(pp);
 		s_cells = of_n_size_cells(pp);
+		if (len / 4 != a_cells + s_cells) {
+			pr_debug("%s: ofpart partition %s (%s) error parsing reg property.\n",
+				 master->name, pp->full_name,
+				 mtd_node->full_name);
+			goto ofpart_fail;
+		}
+
 		(*pparts)[i].offset = of_read_number(reg, a_cells);
 		(*pparts)[i].size = of_read_number(reg + a_cells, s_cells);
 
@@ -92,15 +116,20 @@ static int parse_ofpart_partitions(struct mtd_info *master,
 		i++;
 	}
 
-	if (!i) {
-		of_node_put(pp);
-		pr_err("No valid partition found on %s\n", node->full_name);
-		kfree(*pparts);
-		*pparts = NULL;
-		return -EINVAL;
-	}
+	if (!nr_parts)
+		goto ofpart_none;
 
 	return nr_parts;
+
+ofpart_fail:
+	pr_err("%s: error parsing ofpart partition %s (%s)\n",
+	       master->name, pp->full_name, mtd_node->full_name);
+	ret = -EINVAL;
+ofpart_none:
+	of_node_put(pp);
+	kfree(*pparts);
+	*pparts = NULL;
+	return ret;
 }
 
 static struct mtd_part_parser ofpart_parser = {
-- 
2.1.4


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

Re: [PATCH v3 5/5] mtd: ofpart: move ofpart partitions to a dedicated dt node

[Brian Norris]

On Tue, Aug 18, 2015 at 03:34:09PM -0000, Michal Suchanek wrote:
> Parsing direct subnodes of a mtd device as partitions is unreliable
> since the mtd device is also part of its bus subsystem and can contain
> bus data in subnodes.
> 
> Move ofpart data to a subnode of its own so it is clear which data is
> part of the partition layout.
> 
> Signed-off-by: Michal Suchanek <hramrach-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Applied this patch to l2-mtd.git. Thanks!
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html