[PATCH 0/4] DT platform device name collision fixes

[PATCH 0/4] DT platform device name collision fixes

[Rob Herring]

From: Rob Herring <robh@kernel.org>

This series fixes the device naming collisions that can occur with 
nultiple devices having the same name and non-translatable unit 
addresses. This issue was raised in this thread[1]. I intend to merge 
this regardless of whether or not some hierarchy in sysfs is created. 
That is really a separate issue independent of these fixes.

I found and fix a couple of other issues in the process of testing the 
fix.

Rob

[1] https://lkml.org/lkml/2014/4/23/312

Rob Herring (4):
  of/selftest: add testcase for nodes with same name and address
  of/platform: return error on of_platform_device_create_pdata failure
  of/platform: fix device naming for non-translatable addresses
  of: kill off of_can_translate_address

 drivers/of/address.c                         | 22 +----------------
 drivers/of/platform.c                        | 20 +++++-----------
 drivers/of/selftest.c                        | 23 ++++++++++++++++++
 drivers/of/testcase-data/testcases.dtsi      |  1 +
 drivers/of/testcase-data/tests-platform.dtsi | 35 ++++++++++++++++++++++++++++
 include/linux/of_address.h                   |  1 -
 6 files changed, 66 insertions(+), 36 deletions(-)
 create mode 100644 drivers/of/testcase-data/tests-platform.dtsi

-- 
1.9.1

[PATCH 1/4] of/selftest: add testcase for nodes with same name and address

[Rob Herring]

From: Rob Herring <robh@kernel.org>

Add a test case for nodes which have the same name and same
non-translatable unit address.

Signed-off-by: Rob Herring <robh@kernel.org>
---
 drivers/of/selftest.c                        | 23 ++++++++++++++++++
 drivers/of/testcase-data/testcases.dtsi      |  1 +
 drivers/of/testcase-data/tests-platform.dtsi | 35 ++++++++++++++++++++++++++++
 3 files changed, 59 insertions(+)
 create mode 100644 drivers/of/testcase-data/tests-platform.dtsi

diff --git a/drivers/of/selftest.c b/drivers/of/selftest.c
index ae44500..c93cb4a 100644
--- a/drivers/of/selftest.c
+++ b/drivers/of/selftest.c
@@ -10,6 +10,7 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_irq.h>
+#include <linux/of_platform.h>
 #include <linux/list.h>
 #include <linux/mutex.h>
 #include <linux/slab.h>
@@ -427,6 +428,27 @@ static void __init of_selftest_match_node(void)
 	}
 }
 
+static void __init of_selftest_platform_populate(void)
+{
+	struct device_node *np, *child;
+	int rc;
+	struct of_device_id match[] = {
+		{ .compatible = "test-device", },
+		{}
+	};
+
+	np = of_find_node_by_path("/testcase-data/platform-tests");
+	if (!np) {
+		pr_err("No testcase data in device tree\n");
+		return;
+	}
+
+	for_each_child_of_node(np, child) {
+		rc = of_platform_populate(child, match, NULL, NULL);
+		selftest(!rc, "Could not create device for node '%s'\n", child->name);
+	}
+}
+
 static int __init of_selftest(void)
 {
 	struct device_node *np;
@@ -445,6 +467,7 @@ static int __init of_selftest(void)
 	of_selftest_parse_interrupts();
 	of_selftest_parse_interrupts_extended();
 	of_selftest_match_node();
+	of_selftest_platform_populate();
 	pr_info("end of selftest - %i passed, %i failed\n",
 		selftest_results.passed, selftest_results.failed);
 	return 0;
diff --git a/drivers/of/testcase-data/testcases.dtsi b/drivers/of/testcase-data/testcases.dtsi
index 3a5b75a..6d8d980a 100644
--- a/drivers/of/testcase-data/testcases.dtsi
+++ b/drivers/of/testcase-data/testcases.dtsi
@@ -1,3 +1,4 @@
 #include "tests-phandle.dtsi"
 #include "tests-interrupts.dtsi"
 #include "tests-match.dtsi"
+#include "tests-platform.dtsi"
diff --git a/drivers/of/testcase-data/tests-platform.dtsi b/drivers/of/testcase-data/tests-platform.dtsi
new file mode 100644
index 0000000..eb20eeb
--- /dev/null
+++ b/drivers/of/testcase-data/tests-platform.dtsi
@@ -0,0 +1,35 @@
+
+/ {
+	testcase-data {
+		platform-tests {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			test-device@0 {
+				compatible = "test-device";
+				reg = <0x0>;
+
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				dev@100 {
+					compatible = "test-sub-device";
+					reg = <0x100>;
+				};
+			};
+
+			test-device@1 {
+				compatible = "test-device";
+				reg = <0x1>;
+
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				dev@100 {
+					compatible = "test-sub-device";
+					reg = <0x100>;
+				};
+			};
+		};
+	};
+};
-- 
1.9.1

[PATCH 2/4] of/platform: return error on of_platform_device_create_pdata failure

[Rob Herring]

From: Rob Herring <robh@kernel.org>

of_platform_populate does not return an error if creating the platform
device fails. This means almost any error from driver core cannot be
detected by the caller. Fix this.

Signed-off-by: Rob Herring <robh@kernel.org>
---
 drivers/of/platform.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 404d1da..622aeb3 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -391,7 +391,9 @@ static int of_platform_bus_create(struct device_node *bus,
 	}
 
 	dev = of_platform_device_create_pdata(bus, bus_id, platform_data, parent);
-	if (!dev || !of_match_node(matches, bus))
+	if (!dev)
+		return -ENODEV;
+	if (!of_match_node(matches, bus))
 		return 0;
 
 	for_each_child_of_node(bus, child) {
-- 
1.9.1

[PATCH 3/4] of/platform: fix device naming for non-translatable addresses

[Rob Herring]

From: Rob Herring <robh@kernel.org>

Using non-translatable addresses in platform device names is wrong
because they may not be globally unique. Just use the default naming with
a global index if the address cannot be translated instead.

of_can_translate_address has the same checks as of_translate_address, so
we can remove it here as well.

Reported-by: "Ivan T. Ivanov" <iivanov@mm-sol.com>
Cc: Josh Cartwright <joshc@codeaurora.org>
Cc: Courtney Cavin <courtney.cavin@sonymobile.com>
Cc: Bjorn Andersson <bjorn@kryo.se>
Cc: Frank Rowand <frowand.list@gmail.com>
Cc: Grant Likely <grant.likely@linaro.org>
Signed-off-by: Rob Herring <robh@kernel.org>
---
 drivers/of/platform.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 622aeb3..d827ceb 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -78,7 +78,6 @@ void of_device_make_bus_id(struct device *dev)
 	struct device_node *node = dev->of_node;
 	const __be32 *reg;
 	u64 addr;
-	const __be32 *addrp;
 	int magic;
 
 #ifdef CONFIG_PPC_DCR
@@ -106,15 +105,7 @@ void of_device_make_bus_id(struct device *dev)
 	 */
 	reg = of_get_property(node, "reg", NULL);
 	if (reg) {
-		if (of_can_translate_address(node)) {
-			addr = of_translate_address(node, reg);
-		} else {
-			addrp = of_get_address(node, 0, NULL, NULL);
-			if (addrp)
-				addr = of_read_number(addrp, 1);
-			else
-				addr = OF_BAD_ADDR;
-		}
+		addr = of_translate_address(node, reg);
 		if (addr != OF_BAD_ADDR) {
 			dev_set_name(dev, "%llx.%s",
 				     (unsigned long long)addr, node->name);
-- 
1.9.1

[PATCH 4/4] of: kill off of_can_translate_address

[Rob Herring]

From: Rob Herring <robh@kernel.org>

of_can_translate_address only checks some conditions for address
translation, but does not check other conditions like having range
properties. The checks it does do are redundant with
__of_address_translate. The only difference is printing a message or
not. Since we only have a single caller that does the full translation
anyway, just remove of_can_translate_address and quiet the error
message.

Cc: Grant Likely <grant.likely@linaro.org>
Signed-off-by: Rob Herring <robh@kernel.org>
---
 drivers/of/address.c       | 22 +---------------------
 drivers/of/platform.c      |  5 ++---
 include/linux/of_address.h |  1 -
 3 files changed, 3 insertions(+), 25 deletions(-)

diff --git a/drivers/of/address.c b/drivers/of/address.c
index cb4242a..95351b2 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -498,8 +498,7 @@ static u64 __of_translate_address(struct device_node *dev,
 	/* Count address cells & copy address locally */
 	bus->count_cells(dev, &na, &ns);
 	if (!OF_CHECK_COUNTS(na, ns)) {
-		printk(KERN_ERR "prom_parse: Bad cell count for %s\n",
-		       of_node_full_name(dev));
+		pr_debug("OF: Bad cell count for %s\n", of_node_full_name(dev));
 		goto bail;
 	}
 	memcpy(addr, in_addr, na * 4);
@@ -564,25 +563,6 @@ u64 of_translate_dma_address(struct device_node *dev, const __be32 *in_addr)
 }
 EXPORT_SYMBOL(of_translate_dma_address);
 
-bool of_can_translate_address(struct device_node *dev)
-{
-	struct device_node *parent;
-	struct of_bus *bus;
-	int na, ns;
-
-	parent = of_get_parent(dev);
-	if (parent == NULL)
-		return false;
-
-	bus = of_match_bus(parent);
-	bus->count_cells(dev, &na, &ns);
-
-	of_node_put(parent);
-
-	return OF_CHECK_COUNTS(na, ns);
-}
-EXPORT_SYMBOL(of_can_translate_address);
-
 const __be32 *of_get_address(struct device_node *dev, int index, u64 *size,
 		    unsigned int *flags)
 {
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index d827ceb..07cfd1b 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -140,9 +140,8 @@ struct platform_device *of_device_alloc(struct device_node *np,
 		return NULL;
 
 	/* count the io and irq resources */
-	if (of_can_translate_address(np))
-		while (of_address_to_resource(np, num_reg, &temp_res) == 0)
-			num_reg++;
+	while (of_address_to_resource(np, num_reg, &temp_res) == 0)
+		num_reg++;
 	num_irq = of_irq_count(np);
 
 	/* Populate the resource table */
diff --git a/include/linux/of_address.h b/include/linux/of_address.h
index 5f6ed6b..906ca76 100644
--- a/include/linux/of_address.h
+++ b/include/linux/of_address.h
@@ -40,7 +40,6 @@ extern u64 of_translate_dma_address(struct device_node *dev,
 
 #ifdef CONFIG_OF_ADDRESS
 extern u64 of_translate_address(struct device_node *np, const __be32 *addr);
-extern bool of_can_translate_address(struct device_node *dev);
 extern int of_address_to_resource(struct device_node *dev, int index,
 				  struct resource *r);
 extern struct device_node *of_find_matching_node_by_address(
-- 
1.9.1

Re: [PATCH 0/4] DT platform device name collision fixes

[Frank Rowand]

On 5/7/2014 2:48 PM, Rob Herring wrote:
> From: Rob Herring <robh@kernel.org>
> 
> This series fixes the device naming collisions that can occur with 
> nultiple devices having the same name and non-translatable unit 
> addresses. This issue was raised in this thread[1]. I intend to merge 
> this regardless of whether or not some hierarchy in sysfs is created. 
> That is really a separate issue independent of these fixes.
> 
> I found and fix a couple of other issues in the process of testing the 
> fix.
> 
> Rob
> 
> [1] https://lkml.org/lkml/2014/4/23/312
> 
> Rob Herring (4):
>   of/selftest: add testcase for nodes with same name and address
>   of/platform: return error on of_platform_device_create_pdata failure
>   of/platform: fix device naming for non-translatable addresses
>   of: kill off of_can_translate_address

My opinion is that this approach is not a good approach to solving the
problem.  It is papering over a symptom, instead of dealing with the
root cause.

But despite my opinion, you can add to patches 2-4 (I did not test
the self-test added in patch 1):

   Tested-by: Frank Rowand <frank.rowand@sonymobile.com>

The patches resolve the name conflict originally reported for the
qcomm PMIC, tested on 3.15-rc1, with a bunch of out of tree
patches.

-Frank

Re: [PATCH 1/4] of/selftest: add testcase for nodes with same name and address

[Frank Rowand]

On 5/7/2014 2:48 PM, Rob Herring wrote:
> From: Rob Herring <robh@kernel.org>
> 
> Add a test case for nodes which have the same name and same
> non-translatable unit address.

If I apply patch 1 and 2 without applying 3 and 4 then console
warnings are printed, but from a different area of code than
the original problem reported.  This probably is not a big deal,
but I'm trying to figure out if I can modify the test to also
show the original problem.

The test case also properly reports the failure.

Once all 4 patches are applied, then the test case passes.

Thus:

   Tested-by: Frank Rowand <frank.rowand@sonymobile.com>

> 
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
>  drivers/of/selftest.c                        | 23 ++++++++++++++++++
>  drivers/of/testcase-data/testcases.dtsi      |  1 +
>  drivers/of/testcase-data/tests-platform.dtsi | 35 ++++++++++++++++++++++++++++
>  3 files changed, 59 insertions(+)
>  create mode 100644 drivers/of/testcase-data/tests-platform.dtsi
> 

< snip >

Re: [PATCH 0/4] DT platform device name collision fixes

[Frank Rowand]

On 5/7/2014 3:52 PM, Frank Rowand wrote:
> On 5/7/2014 2:48 PM, Rob Herring wrote:
>> From: Rob Herring <robh@kernel.org>
>>
>> This series fixes the device naming collisions that can occur with 
>> nultiple devices having the same name and non-translatable unit 
>> addresses. This issue was raised in this thread[1]. I intend to merge 
>> this regardless of whether or not some hierarchy in sysfs is created. 
>> That is really a separate issue independent of these fixes.
>>
>> I found and fix a couple of other issues in the process of testing the 
>> fix.
>>
>> Rob
>>
>> [1] https://lkml.org/lkml/2014/4/23/312
>>
>> Rob Herring (4):
>>   of/selftest: add testcase for nodes with same name and address
>>   of/platform: return error on of_platform_device_create_pdata failure
>>   of/platform: fix device naming for non-translatable addresses
>>   of: kill off of_can_translate_address
> 
> My opinion is that this approach is not a good approach to solving the
> problem.  It is papering over a symptom, instead of dealing with the
> root cause.
> 
> But despite my opinion, you can add to patches 2-4 (I did not test
> the self-test added in patch 1):
> 
>    Tested-by: frowand.list@gmail.com <frank.rowand@sonymobile.com>
> 
> The patches resolve the name conflict originally reported for the
> qcomm PMIC, tested on 3.15-rc1, with a bunch of out of tree
> patches.

And you can add to the 4 patches:

  Reviewed-by: Frank Rowand <frank.rowand@sonymobile.com>

-Frank

Re: [PATCH 1/4] of/selftest: add testcase for nodes with same name and address

[Grant Likely]

On Thu, May 8, 2014 at 3:51 AM, Frank Rowand <frowand.list@gmail.com> wrote:
> On 5/7/2014 2:48 PM, Rob Herring wrote:
>> From: Rob Herring <robh@kernel.org>
>>
>> Add a test case for nodes which have the same name and same
>> non-translatable unit address.
>
> If I apply patch 1 and 2 without applying 3 and 4 then console
> warnings are printed, but from a different area of code than
> the original problem reported.  This probably is not a big deal,
> but I'm trying to figure out if I can modify the test to also
> show the original problem.
>
> The test case also properly reports the failure.
>
> Once all 4 patches are applied, then the test case passes.

When adding testcases I do prefer the addition of the test to appear
before the fix while the patch is under review. Makes it easy to see
that, yes the bug exists, and yes the later patch resolves it.

When the patches are actually committed for merging then it is
probably a good idea to reverse them. Thanks for the testing.

g.

>
> Thus:
>
>    Tested-by: Frank Rowand <frank.rowand@sonymobile.com>
>
>>
>> Signed-off-by: Rob Herring <robh@kernel.org>
>> ---
>>  drivers/of/selftest.c                        | 23 ++++++++++++++++++
>>  drivers/of/testcase-data/testcases.dtsi      |  1 +
>>  drivers/of/testcase-data/tests-platform.dtsi | 35 ++++++++++++++++++++++++++++
>>  3 files changed, 59 insertions(+)
>>  create mode 100644 drivers/of/testcase-data/tests-platform.dtsi
>>
>
> < snip >
>

Re: [PATCH 1/4] of/selftest: add testcase for nodes with same name and address

[Grant Likely]

On Wed, May 7, 2014 at 10:48 PM, Rob Herring <robherring2@gmail.com> wrote:
> From: Rob Herring <robh@kernel.org>
>
> Add a test case for nodes which have the same name and same
> non-translatable unit address.
>
> Signed-off-by: Rob Herring <robh@kernel.org>

Looks good to me.

Reviewed-by: Grant Likely <grant.likely@linaro.org>

> ---
>  drivers/of/selftest.c                        | 23 ++++++++++++++++++
>  drivers/of/testcase-data/testcases.dtsi      |  1 +
>  drivers/of/testcase-data/tests-platform.dtsi | 35 ++++++++++++++++++++++++++++
>  3 files changed, 59 insertions(+)
>  create mode 100644 drivers/of/testcase-data/tests-platform.dtsi
>
> diff --git a/drivers/of/selftest.c b/drivers/of/selftest.c
> index ae44500..c93cb4a 100644
> --- a/drivers/of/selftest.c
> +++ b/drivers/of/selftest.c
> @@ -10,6 +10,7 @@
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/of_irq.h>
> +#include <linux/of_platform.h>
>  #include <linux/list.h>
>  #include <linux/mutex.h>
>  #include <linux/slab.h>
> @@ -427,6 +428,27 @@ static void __init of_selftest_match_node(void)
>         }
>  }
>
> +static void __init of_selftest_platform_populate(void)
> +{
> +       struct device_node *np, *child;
> +       int rc;
> +       struct of_device_id match[] = {
> +               { .compatible = "test-device", },
> +               {}
> +       };
> +
> +       np = of_find_node_by_path("/testcase-data/platform-tests");
> +       if (!np) {
> +               pr_err("No testcase data in device tree\n");
> +               return;
> +       }
> +
> +       for_each_child_of_node(np, child) {
> +               rc = of_platform_populate(child, match, NULL, NULL);
> +               selftest(!rc, "Could not create device for node '%s'\n", child->name);
> +       }
> +}
> +
>  static int __init of_selftest(void)
>  {
>         struct device_node *np;
> @@ -445,6 +467,7 @@ static int __init of_selftest(void)
>         of_selftest_parse_interrupts();
>         of_selftest_parse_interrupts_extended();
>         of_selftest_match_node();
> +       of_selftest_platform_populate();
>         pr_info("end of selftest - %i passed, %i failed\n",
>                 selftest_results.passed, selftest_results.failed);
>         return 0;
> diff --git a/drivers/of/testcase-data/testcases.dtsi b/drivers/of/testcase-data/testcases.dtsi
> index 3a5b75a..6d8d980a 100644
> --- a/drivers/of/testcase-data/testcases.dtsi
> +++ b/drivers/of/testcase-data/testcases.dtsi
> @@ -1,3 +1,4 @@
>  #include "tests-phandle.dtsi"
>  #include "tests-interrupts.dtsi"
>  #include "tests-match.dtsi"
> +#include "tests-platform.dtsi"
> diff --git a/drivers/of/testcase-data/tests-platform.dtsi b/drivers/of/testcase-data/tests-platform.dtsi
> new file mode 100644
> index 0000000..eb20eeb
> --- /dev/null
> +++ b/drivers/of/testcase-data/tests-platform.dtsi
> @@ -0,0 +1,35 @@
> +
> +/ {
> +       testcase-data {
> +               platform-tests {
> +                       #address-cells = <1>;
> +                       #size-cells = <0>;
> +
> +                       test-device@0 {
> +                               compatible = "test-device";
> +                               reg = <0x0>;
> +
> +                               #address-cells = <1>;
> +                               #size-cells = <0>;
> +
> +                               dev@100 {
> +                                       compatible = "test-sub-device";
> +                                       reg = <0x100>;
> +                               };
> +                       };
> +
> +                       test-device@1 {
> +                               compatible = "test-device";
> +                               reg = <0x1>;
> +
> +                               #address-cells = <1>;
> +                               #size-cells = <0>;
> +
> +                               dev@100 {
> +                                       compatible = "test-sub-device";
> +                                       reg = <0x100>;
> +                               };
> +                       };
> +               };
> +       };
> +};
> --
> 1.9.1
>

Re: [PATCH 3/4] of/platform: fix device naming for non-translatable addresses

[Arnd Bergmann]

On Wednesday 07 May 2014 16:48:17 Rob Herring wrote:
> From: Rob Herring <robh@kernel.org>
> 
> Using non-translatable addresses in platform device names is wrong
> because they may not be globally unique. Just use the default naming with
> a global index if the address cannot be translated instead.
> 
> of_can_translate_address has the same checks as of_translate_address, so
> we can remove it here as well.

Have you checked that this doesn't break platforms using clkdev lookup
based on names with local addresses? It's quite possible that nobody
does that, but it would be good to have someone confirm it.

	Arnd

Re: [PATCH 3/4] of/platform: fix device naming for non-translatable addresses

[Ivan T. Ivanov]

On Wed, 2014-05-07 at 16:48 -0500, Rob Herring wrote:
> From: Rob Herring <robh@kernel.org>
> 
> Using non-translatable addresses in platform device names is wrong
> because they may not be globally unique. Just use the default naming with
> a global index if the address cannot be translated instead.
> 
> of_can_translate_address has the same checks as of_translate_address, so
> we can remove it here as well.
> 
> Reported-by: "Ivan T. Ivanov" <iivanov@mm-sol.com>
> Cc: Josh Cartwright <joshc@codeaurora.org>
> Cc: Courtney Cavin <courtney.cavin@sonymobile.com>
> Cc: Bjorn Andersson <bjorn@kryo.se>
> Cc: Frank Rowand <frowand.list@gmail.com>
> Cc: Grant Likely <grant.likely@linaro.org>
> Signed-off-by: Rob Herring <robh@kernel.org>

Thanks. It works for me. 

Tested-by: Ivan T. Ivanov <iivanov@mm-sol.com>

Regards.

Re: [PATCH 3/4] of/platform: fix device naming for non-translatable addresses

[Rob Herring]

On Thu, May 8, 2014 at 5:41 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Wednesday 07 May 2014 16:48:17 Rob Herring wrote:
>> From: Rob Herring <robh@kernel.org>
>>
>> Using non-translatable addresses in platform device names is wrong
>> because they may not be globally unique. Just use the default naming with
>> a global index if the address cannot be translated instead.
>>
>> of_can_translate_address has the same checks as of_translate_address, so
>> we can remove it here as well.
>
> Have you checked that this doesn't break platforms using clkdev lookup
> based on names with local addresses? It's quite possible that nobody
> does that, but it would be good to have someone confirm it.

I had not, but now I have. There don't appear to be any cases of that.

Rob

Re: [PATCH 3/4] of/platform: fix device naming for non-translatable addresses

[Arnd Bergmann]

On Thursday 08 May 2014 07:55:06 Rob Herring wrote:
> On Thu, May 8, 2014 at 5:41 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Wednesday 07 May 2014 16:48:17 Rob Herring wrote:
> >> From: Rob Herring <robh@kernel.org>
> >>
> >> Using non-translatable addresses in platform device names is wrong
> >> because they may not be globally unique. Just use the default naming with
> >> a global index if the address cannot be translated instead.
> >>
> >> of_can_translate_address has the same checks as of_translate_address, so
> >> we can remove it here as well.
> >
> > Have you checked that this doesn't break platforms using clkdev lookup
> > based on names with local addresses? It's quite possible that nobody
> > does that, but it would be good to have someone confirm it.
> 
> I had not, but now I have. There don't appear to be any cases of that.

Ok, thanks for checking.

	Arnd

Re: [PATCH 1/4] of/selftest: add testcase for nodes with same name and address

[Frank Rowand]

On 5/8/2014 1:59 AM, Grant Likely wrote:
> On Thu, May 8, 2014 at 3:51 AM, Frank Rowand <frowand.list@gmail.com> wrote:
>> On 5/7/2014 2:48 PM, Rob Herring wrote:
>>> From: Rob Herring <robh@kernel.org>
>>>
>>> Add a test case for nodes which have the same name and same
>>> non-translatable unit address.
>>
>> If I apply patch 1 and 2 without applying 3 and 4 then console
>> warnings are printed, but from a different area of code than
>> the original problem reported.  This probably is not a big deal,
>> but I'm trying to figure out if I can modify the test to also
>> show the original problem.
>>
>> The test case also properly reports the failure.
>>
>> Once all 4 patches are applied, then the test case passes.
> 
> When adding testcases I do prefer the addition of the test to appear
> before the fix while the patch is under review. Makes it easy to see
> that, yes the bug exists, and yes the later patch resolves it.
> 
> When the patches are actually committed for merging then it is
> probably a good idea to reverse them. Thanks for the testing.

In this case, the test case has to be patch #2, if you want to be
able to verify that the test case catches the bug, because patch
#1 added code to allow the error return value to propagate all
the way out to the API called by the test code.  So the patch
ordering is correct.

> 
> g.
> 
>>
>> Thus:
>>
>>    Tested-by: Frank Rowand <frank.rowand@sonymobile.com>
>>
>>>
>>> Signed-off-by: Rob Herring <robh@kernel.org>
>>> ---
>>>  drivers/of/selftest.c                        | 23 ++++++++++++++++++
>>>  drivers/of/testcase-data/testcases.dtsi      |  1 +
>>>  drivers/of/testcase-data/tests-platform.dtsi | 35 ++++++++++++++++++++++++++++
>>>  3 files changed, 59 insertions(+)
>>>  create mode 100644 drivers/of/testcase-data/tests-platform.dtsi
>>>
>>
>> < snip >
>>
> .
> 

Re: [PATCH 1/4] of/selftest: add testcase for nodes with same name and address

[Frank Rowand]

On 5/7/2014 7:51 PM, Frank Rowand wrote:
> On 5/7/2014 2:48 PM, Rob Herring wrote:
>> From: Rob Herring <robh@kernel.org>
>>
>> Add a test case for nodes which have the same name and same
>> non-translatable unit address.
> 
> If I apply patch 1 and 2 without applying 3 and 4 then console
> warnings are printed, but from a different area of code than
> the original problem reported.  This probably is not a big deal,
> but I'm trying to figure out if I can modify the test to also
> show the original problem.

If you want to add a test that triggers the same stack trace as
the orginally reported problem, a patch is below.  It would apply
between your original patch 1 and patch 2.

> 
> The test case also properly reports the failure.
> 
> Once all 4 patches are applied, then the test case passes.
> 
> Thus:
> 
>    Tested-by: Frank Rowand <frank.rowand@sonymobile.com>
> 
>>
>> Signed-off-by: Rob Herring <robh@kernel.org>
>> ---
>>  drivers/of/selftest.c                        | 23 ++++++++++++++++++
>>  drivers/of/testcase-data/testcases.dtsi      |  1 +
>>  drivers/of/testcase-data/tests-platform.dtsi | 35 ++++++++++++++++++++++++++++
>>  3 files changed, 59 insertions(+)
>>  create mode 100644 drivers/of/testcase-data/tests-platform.dtsi
>>
> 
> < snip >
> 
> 




From: Frank Rowand <frank.rowand@sonymobile.com>

Add another test case to of_selftest_platform_populate().  This case
triggers the same stack trace from of_platform_populate() to sysfs_warn_dup()
as seen in the case reported by https://lkml.org/lkml/2014/4/23/312.

Signed-off-by: Frank Rowand <frank.rowand@sonymobile.com>
---
 drivers/of/selftest.c                        |   27 ++++++++++++++++++++++
 drivers/of/testcase-data/tests-platform.dtsi |   33 +++++++++++++++++++++++++++
 2 files changed, 60 insertions(+)

Index: b/drivers/of/testcase-data/tests-platform.dtsi
===================================================================
--- a/drivers/of/testcase-data/tests-platform.dtsi
+++ b/drivers/of/testcase-data/tests-platform.dtsi
@@ -31,5 +31,38 @@
 				};
 			};
 		};
+
+		test-master {
+			compatible = "test-master";
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			test-device@0 {
+				compatible = "test-device";
+				reg = <0x0>;
+
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				sub-dev@100 {
+					compatible = "test-sub-device";
+					reg = <0x100>;
+				};
+			};
+
+			test-device@1 {
+				compatible = "test-device";
+				reg = <0x1>;
+
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				sub-dev@100 {
+					compatible = "test-sub-device";
+					reg = <0x100>;
+				};
+			};
+		};
 	};
 };
+
Index: b/drivers/of/selftest.c
===================================================================
--- a/drivers/of/selftest.c
+++ b/drivers/of/selftest.c
@@ -436,6 +436,7 @@ static void __init of_selftest_platform_
 		{ .compatible = "test-device", },
 		{}
 	};
+	struct platform_device *pdev;
 
 	np = of_find_node_by_path("/testcase-data/platform-tests");
 	if (!np) {
@@ -447,6 +448,32 @@ static void __init of_selftest_platform_
 		rc = of_platform_populate(child, match, NULL, NULL);
 		selftest(!rc, "Could not create device for node '%s'\n", child->name);
 	}
+
+	np = of_find_node_by_path("/testcase-data/test-master");
+	if (!np) {
+		pr_err("No /testcase-data/test-master node in device tree\n");
+		return;
+	}
+
+	for_each_child_of_node(np, child) {
+		pdev = of_device_alloc(child, NULL, NULL);
+		if (pdev) {
+			pdev->dev.bus = &platform_bus_type;
+			if (of_device_add(pdev) != 0) {
+				platform_device_put(pdev);
+				pdev = NULL;
+			}
+		}
+		selftest(pdev, "Could not allocate device for node '%s'\n",
+			 child->full_name);
+
+		if (pdev) {
+			rc = of_platform_populate(child, NULL, NULL, &pdev->dev);
+			selftest(!rc, "Could not populate node '%s'\n",
+				 child->full_name);
+		}
+	}
+
 }
 
 static int __init of_selftest(void)

Re: [PATCH 2/4] of/platform: return error on of_platform_device_create_pdata failure

[Olof Johansson]

On Wed, May 07, 2014 at 04:48:16PM -0500, Rob Herring wrote:
> From: Rob Herring <robh@kernel.org>
> 
> of_platform_populate does not return an error if creating the platform
> device fails. This means almost any error from driver core cannot be
> detected by the caller. Fix this.
> 
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
>  drivers/of/platform.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index 404d1da..622aeb3 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -391,7 +391,9 @@ static int of_platform_bus_create(struct device_node *bus,
>  	}
>  
>  	dev = of_platform_device_create_pdata(bus, bus_id, platform_data, parent);
> -	if (!dev || !of_match_node(matches, bus))
> +	if (!dev)
> +		return -ENODEV;
> +	if (!of_match_node(matches, bus))
>  		return 0;
>  
>  	for_each_child_of_node(bus, child) {

This patch caused every single MMC driver to break on OF platforms, as far
as I can tell.  Reverting it on last night's -next seems to resolve it.

The problem seems to be that of_platform_populate() will bail on the
first device that fails this.

How did you test this code, Frank? On what platform? Practically all my
targets here failed to mount rootfs from eMMC or SD card...

Note that the below patch mostly undoes the original intent of letting an error
percolate up, since only the last rc value is actually returned. So I doubt
it's the right patch to pick up, but either this or a revert is needed right
now.


-Olof


>From f4cdc90a500a339e5b96ca06d108e1be7a808d76 Mon Sep 17 00:00:00 2001
From: Olof Johansson <olof@lixom.net>
Date: Tue, 13 May 2014 10:51:30 -0700
Subject: [PATCH] of/platform: Don't abort of_platform_populate() early

52c75b64a374 ('of/platform: return error on
of_platform_device_create_pdata failure') starts returning ENODEV on
some calls now, and that will make of_platform_populate terminate the
loop. Be tolerant of that particlar error return value instead.

Fixes: 52c75b64a374 ('of/platform: return error on of_platform_device_create_pdata failure')
Cc: Frank Rowand <frank.rowand@sonymobile.com>
Signed-off-by: Olof Johansson <olof@lixom.net>
---
 drivers/of/platform.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 8a6de3c..310de38 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -524,7 +524,7 @@ int of_platform_populate(struct device_node *root,
 
 	for_each_child_of_node(root, child) {
 		rc = of_platform_bus_create(child, matches, lookup, parent, true);
-		if (rc)
+		if (rc && rc != -ENODEV)
 			break;
 	}
 
-- 
1.7.10.4

Re: [PATCH 2/4] of/platform: return error on of_platform_device_create_pdata failure

[Rob Herring]

On Tue, May 13, 2014 at 12:56 PM, Olof Johansson <olof@lixom.net> wrote:
> On Wed, May 07, 2014 at 04:48:16PM -0500, Rob Herring wrote:
>> From: Rob Herring <robh@kernel.org>
>>
>> of_platform_populate does not return an error if creating the platform
>> device fails. This means almost any error from driver core cannot be
>> detected by the caller. Fix this.
>>
>> Signed-off-by: Rob Herring <robh@kernel.org>
>> ---
>>  drivers/of/platform.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
>> index 404d1da..622aeb3 100644
>> --- a/drivers/of/platform.c
>> +++ b/drivers/of/platform.c
>> @@ -391,7 +391,9 @@ static int of_platform_bus_create(struct device_node *bus,
>>       }
>>
>>       dev = of_platform_device_create_pdata(bus, bus_id, platform_data, parent);
>> -     if (!dev || !of_match_node(matches, bus))
>> +     if (!dev)
>> +             return -ENODEV;
>> +     if (!of_match_node(matches, bus))
>>               return 0;
>>
>>       for_each_child_of_node(bus, child) {
>
> This patch caused every single MMC driver to break on OF platforms, as far
> as I can tell.  Reverting it on last night's -next seems to resolve it.
>
> The problem seems to be that of_platform_populate() will bail on the
> first device that fails this.
>
> How did you test this code, Frank? On what platform? Practically all my
> targets here failed to mount rootfs from eMMC or SD card...

I tested on versatile. It looks like anything using 'status =
"disabled";' is broken.

> Note that the below patch mostly undoes the original intent of letting an error
> percolate up, since only the last rc value is actually returned. So I doubt
> it's the right patch to pick up, but either this or a revert is needed right
> now.

I will drop this (I'm rebasing this series anyway). ENODEV is the only
error code, so the below patch is not useful. I mainly added this as a
way to make the test self-checking, but can do this another way.
Proper propagation of errors will be more invasive.

Rob

> -Olof
>
>
> From f4cdc90a500a339e5b96ca06d108e1be7a808d76 Mon Sep 17 00:00:00 2001
> From: Olof Johansson <olof@lixom.net>
> Date: Tue, 13 May 2014 10:51:30 -0700
> Subject: [PATCH] of/platform: Don't abort of_platform_populate() early
>
> 52c75b64a374 ('of/platform: return error on
> of_platform_device_create_pdata failure') starts returning ENODEV on
> some calls now, and that will make of_platform_populate terminate the
> loop. Be tolerant of that particlar error return value instead.
>
> Fixes: 52c75b64a374 ('of/platform: return error on of_platform_device_create_pdata failure')
> Cc: Frank Rowand <frank.rowand@sonymobile.com>
> Signed-off-by: Olof Johansson <olof@lixom.net>
> ---
>  drivers/of/platform.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index 8a6de3c..310de38 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -524,7 +524,7 @@ int of_platform_populate(struct device_node *root,
>
>         for_each_child_of_node(root, child) {
>                 rc = of_platform_bus_create(child, matches, lookup, parent, true);
> -               if (rc)
> +               if (rc && rc != -ENODEV)
>                         break;
>         }
>
> --
> 1.7.10.4
>

Re: [PATCH 2/4] of/platform: return error on of_platform_device_create_pdata failure

[Frank Rowand]

On 5/13/2014 10:56 AM, Olof Johansson wrote:
> On Wed, May 07, 2014 at 04:48:16PM -0500, Rob Herring wrote:
>> From: Rob Herring <robh@kernel.org>
>>
>> of_platform_populate does not return an error if creating the platform
>> device fails. This means almost any error from driver core cannot be
>> detected by the caller. Fix this.
>>
>> Signed-off-by: Rob Herring <robh@kernel.org>
>> ---
>>  drivers/of/platform.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
>> index 404d1da..622aeb3 100644
>> --- a/drivers/of/platform.c
>> +++ b/drivers/of/platform.c
>> @@ -391,7 +391,9 @@ static int of_platform_bus_create(struct device_node *bus,
>>  	}
>>  
>>  	dev = of_platform_device_create_pdata(bus, bus_id, platform_data, parent);
>> -	if (!dev || !of_match_node(matches, bus))
>> +	if (!dev)
>> +		return -ENODEV;
>> +	if (!of_match_node(matches, bus))
>>  		return 0;
>>  
>>  	for_each_child_of_node(bus, child) {
> 
> This patch caused every single MMC driver to break on OF platforms, as far
> as I can tell.  Reverting it on last night's -next seems to resolve it.
> 
> The problem seems to be that of_platform_populate() will bail on the
> first device that fails this.
> 
> How did you test this code, Frank? On what platform? Practically all my
> targets here failed to mount rootfs from eMMC or SD card...

I tested on a dragonboard, but without a working sdhci driver.  Thus I would not
have hit the error.

> 
> Note that the below patch mostly undoes the original intent of letting an error
> percolate up, since only the last rc value is actually returned. So I doubt
> it's the right patch to pick up, but either this or a revert is needed right
> now.
> 
> 
> -Olof
> 
> 
>>From f4cdc90a500a339e5b96ca06d108e1be7a808d76 Mon Sep 17 00:00:00 2001
> From: Olof Johansson <olof@lixom.net>
> Date: Tue, 13 May 2014 10:51:30 -0700
> Subject: [PATCH] of/platform: Don't abort of_platform_populate() early
> 
> 52c75b64a374 ('of/platform: return error on
> of_platform_device_create_pdata failure') starts returning ENODEV on
> some calls now, and that will make of_platform_populate terminate the
> loop. Be tolerant of that particlar error return value instead.
> 
> Fixes: 52c75b64a374 ('of/platform: return error on of_platform_device_create_pdata failure')
> Cc: Frank Rowand <frank.rowand@sonymobile.com>
> Signed-off-by: Olof Johansson <olof@lixom.net>
> ---
>  drivers/of/platform.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index 8a6de3c..310de38 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -524,7 +524,7 @@ int of_platform_populate(struct device_node *root,
>  
>  	for_each_child_of_node(root, child) {
>  		rc = of_platform_bus_create(child, matches, lookup, parent, true);
> -		if (rc)
> +		if (rc && rc != -ENODEV)
>  			break;
>  	}
>  
> 

Re: [PATCH 2/4] of/platform: return error on of_platform_device_create_pdata failure

[Olof Johansson]

On Tue, May 13, 2014 at 1:32 PM, Frank Rowand <frowand.list@gmail.com> wrote:
> On 5/13/2014 10:56 AM, Olof Johansson wrote:
>> On Wed, May 07, 2014 at 04:48:16PM -0500, Rob Herring wrote:
>>> From: Rob Herring <robh@kernel.org>
>>>
>>> of_platform_populate does not return an error if creating the platform
>>> device fails. This means almost any error from driver core cannot be
>>> detected by the caller. Fix this.
>>>
>>> Signed-off-by: Rob Herring <robh@kernel.org>
>>> ---
>>>  drivers/of/platform.c | 4 +++-
>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
>>> index 404d1da..622aeb3 100644
>>> --- a/drivers/of/platform.c
>>> +++ b/drivers/of/platform.c
>>> @@ -391,7 +391,9 @@ static int of_platform_bus_create(struct device_node *bus,
>>>      }
>>>
>>>      dev = of_platform_device_create_pdata(bus, bus_id, platform_data, parent);
>>> -    if (!dev || !of_match_node(matches, bus))
>>> +    if (!dev)
>>> +            return -ENODEV;
>>> +    if (!of_match_node(matches, bus))
>>>              return 0;
>>>
>>>      for_each_child_of_node(bus, child) {
>>
>> This patch caused every single MMC driver to break on OF platforms, as far
>> as I can tell.  Reverting it on last night's -next seems to resolve it.
>>
>> The problem seems to be that of_platform_populate() will bail on the
>> first device that fails this.
>>
>> How did you test this code, Frank? On what platform? Practically all my
>> targets here failed to mount rootfs from eMMC or SD card...
>
> I tested on a dragonboard, but without a working sdhci driver.  Thus I would not
> have hit the error.

Yeah, or more generically: Without a device that would have been
probed after the first status = "disabled" device that was found.
Makes sense, especially since dragonboard support is still not quite
there.

Ah well, it means the -next process is working, after all. :)



-Olof

Re: [PATCH 2/4] of/platform: return error on of_platform_device_create_pdata failure

[Ivan T. Ivanov]

On Tue, 2014-05-13 at 13:32 -0700, Frank Rowand wrote:
> On 5/13/2014 10:56 AM, Olof Johansson wrote:
> > On Wed, May 07, 2014 at 04:48:16PM -0500, Rob Herring wrote:
> >> From: Rob Herring <robh@kernel.org>
> >>
> >> of_platform_populate does not return an error if creating the platform
> >> device fails. This means almost any error from driver core cannot be
> >> detected by the caller. Fix this.
> >>
> >> Signed-off-by: Rob Herring <robh@kernel.org>
> >> ---
> >>  drivers/of/platform.c | 4 +++-
> >>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> >> index 404d1da..622aeb3 100644
> >> --- a/drivers/of/platform.c
> >> +++ b/drivers/of/platform.c
> >> @@ -391,7 +391,9 @@ static int of_platform_bus_create(struct device_node *bus,
> >>  	}
> >>  
> >>  	dev = of_platform_device_create_pdata(bus, bus_id, platform_data, parent);
> >> -	if (!dev || !of_match_node(matches, bus))
> >> +	if (!dev)
> >> +		return -ENODEV;
> >> +	if (!of_match_node(matches, bus))
> >>  		return 0;
> >>  
> >>  	for_each_child_of_node(bus, child) {
> > 
> > This patch caused every single MMC driver to break on OF platforms, as far
> > as I can tell.  Reverting it on last night's -next seems to resolve it.
> > 
> > The problem seems to be that of_platform_populate() will bail on the
> > first device that fails this.
> > 
> > How did you test this code, Frank? On what platform? Practically all my
> > targets here failed to mount rootfs from eMMC or SD card...
> 
> I tested on a dragonboard, but without a working sdhci driver.  Thus I would not
> have hit the error.

Same here.

Regards,
Ivan