improve mtdpart robustness

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 <[email protected]>

---
v2:

- reformat debug messages

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 <[email protected]>

---
v2:

- only allow partition parsing failure when MTD_PARTITIONED_MASTER is set

---
 drivers/mtd/mtdpart.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

drivers/mtd/mtdpart.c +0 -0

index 31888c2..6eafbe9 100644

From 53b60f31a2a0f2a7e8220a4aff47457248bccbcf Mon Sep 17 00:00:00 2001

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 <[email protected]>
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 <[email protected]>

Signed-off-by Brian Norris <[email protected]>

---
 drivers/mtd/mtdcore.c |  6 ++++--
 drivers/mtd/mtdpart.c | 14 ++++++++++----
 2 files changed, 14 insertions(+), 6 deletions(-)

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 <[email protected]>

---
v3:

- rename DT node ofpart -> partitions

---
 .../devicetree/bindings/mtd/partition.txt          | 68 +++++++++++++---------
 1 file changed, 40 insertions(+), 28 deletions(-)

Documentation/devicetree/bindings/mtd/partition.txt +40 -28

index 8e5557d..8c2aff7 100644
@@ -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>;
+		};
 	};
 };

Signature · 1 line

2.1.4

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

Quoted 102 lines · 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 <[email protected]>

---
v3:

- rename DT node ofpart -> partitions

---
 .../devicetree/bindings/mtd/partition.txt          | 68 +++++++++++++---------
 1 file changed, 40 insertions(+), 28 deletions(-)

Documentation/devicetree/bindings/mtd/partition.txt +35 -24

index 8e5557d..8c2aff7 100644
@@ -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>;
+		};
 

Quoted 14 lines

Documentation/devicetree/bindings/mtd/partition.txt +5 -4

-	/* 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>;
+		};
 	};
 };

Signature · 1 line

2.1.4

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

Quoted 102 lines · 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 <[email protected]>

---
v3:

- rename DT node ofpart -> partitions

---
 .../devicetree/bindings/mtd/partition.txt          | 68 +++++++++++++---------
 1 file changed, 40 insertions(+), 28 deletions(-)

Documentation/devicetree/bindings/mtd/partition.txt +35 -24

index 8e5557d..8c2aff7 100644
@@ -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>;
+             };
 

Quoted 12 lines

Documentation/devicetree/bindings/mtd/partition.txt +5 -5

-     /* 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>;
+             };
      };
 };
--

Quoted 2 lines

2.1.4

Quoted 6 lines · 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.

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

Hi Rob,

Thanks for the review.

Quoted 9 lines · Rob Herring

Quoted 6 lines · 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.

Quoted 2 lines

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?

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

Signed-off-by Michal Suchanek <[email protected]>

---
 Documentation/devicetree/bindings/mtd/partition.txt | 1 +
 1 file changed, 1 insertion(+)

Documentation/devicetree/bindings/mtd/partition.txt +0 -0

index 8c2aff7..7fa9c08 100644

Quoted 1 line · Michal Suchanek

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.

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 <[email protected]>