[RFC PATCH 0/3] devicetree, qcomm PMIC: fix node name conflict

[RFC PATCH 0/3] devicetree, qcomm PMIC: fix node name conflict

[Frank Rowand]

An issue with the path of SPMI nodes under /sys/bus/... was reported in
https://lkml.org/lkml/2014/4/23/312.  The symptom is that two different
grandchild nodes of the spmi with the same node-name@unit-address will
result in attempting to create duplicate links at
/sys/bus/platform/devices/unit-address.node-name.  It turns out that the
specific example provided might not be an expected configuration for
current hardware, but the reported trap remains an issue.

I have been poking at the problem, trying to figure out how to cleanly
fix the issue without breaking devicetree device creation.

The first patch in the series is the one that may be a very bad idea.  Or
it may help show the way forward to deal with what I think is the major
underlying problem.  I have not finished investigating the possible negative
side effects.  And I am still thinking whether this is a conceptually good
approach, or whether it is simply an expediant hack that hides the underlying
problem.  But I am throwing this out prematurely because I have mentioned
it to several people, and I want to make it visible to everyone involved.

The underlying architectural problem (in my opinion) is that a lot of devices
are created by the device tree infrastructure as platform devices, when they
truly should not be platform devices.  They should not be platform devices
because they are not physically on a platform bus, they are instead somewhere
below some other bus.  The first patch in this series is a hack which
results in the devices still being represented by "struct platform_device"
objects, but with a link to their parent's "struct bus_type" instead of
to &platform_bus_type.

The second patch does not require the first patch.  The second patch provides
a mechanism to allow subsystems to provide a method of naming devices to
avoid name collisions.

The third patch provides an example of a subsystem using the new feature
provided by the second patch.

The resulting device naming and soft links from applying all three patches,
or just the second and third patches are:


=====  no patches applied:

$ ls /sys/devices/
ARMv7 Krait  cpu-pmu.1    platform     software     tracepoint
breakpoint   cpus.0       soc.2        system       virtual

$ ls /sys/devices/soc.2/
f9000000.interrupt-controller  fc4281d0.qcom,mpm
f9011000.smem                  fc4ab000.restart
f9012000.regulator             fc4cf000.qcom,spmi
f9020000.timer                 fd484000.hwlock
f9088000.clock-controller      fd510000.pinctrl
f9098000.clock-controller      fd8c0000.clock-controller
f90a8000.clock-controller      gpio_keys.5
f90b8000.clock-controller      iio-thermal.4
f9824900.sdhc                  modalias
f991e000.serial                power
f9924000.i2c2                  subsystem
f9928000.i2c6                  timer.3
f9bff000.rng                   uevent
fc400000.clock-controller

$ ls /sys/devices/soc.2/fc4cf000.qcom,spmi/
driver     modalias   power      spmi-0     subsystem  uevent

$ ls /sys/devices/soc.2/fc4cf000.qcom,spmi/spmi-0/
0-00       0-01       0-04       power      subsystem  uevent

$ ls /sys/devices/soc.2/fc4cf000.qcom,spmi/spmi-0/0-00/
100.qcom,revid            driver                    subsystem
3100.qcom,pm8x41-adc-usr  gpios.18                  uevent
6000.qcom,rtc             power



$ ls /sys/bus/platform/devices/soc.2/
f9000000.interrupt-controller  fc4281d0.qcom,mpm
f9011000.smem                  fc4ab000.restart
f9012000.regulator             fc4cf000.qcom,spmi
f9020000.timer                 fd484000.hwlock
f9088000.clock-controller      fd510000.pinctrl
f9098000.clock-controller      fd8c0000.clock-controller
f90a8000.clock-controller      gpio_keys.5
f90b8000.clock-controller      iio-thermal.4
f9824900.sdhc                  modalias
f991e000.serial                power
f9924000.i2c2                  subsystem
f9928000.i2c6                  timer.3
f9bff000.rng                   uevent
fc400000.clock-controller

$ ls /sys/bus/platform/devices/soc.2/fc4cf000.qcom,spmi/
driver     modalias   power      spmi-0     subsystem  uevent

$ ls /sys/bus/platform/devices/soc.2/fc4cf000.qcom,spmi/spmi-0/
0-00       0-01       0-04       power      subsystem  uevent

$ ls /sys/bus/platform/devices/soc.2/fc4cf000.qcom,spmi/spmi-0/0-00/
100.qcom,revid            driver                    subsystem
3100.qcom,pm8x41-adc-usr  gpios.18                  uevent
6000.qcom,rtc             power

$ ls /sys/bus/platform/devices/soc.2/fc4cf000.qcom,spmi/spmi-0/0-01/
b040.pm8xxx-pwm      driver               uevent
d000.pm8xxx-pwm-led  power
d800.pm8xxx-wled     subsystem

$ ls /sys/bus/platform/devices/soc.2/fc4cf000.qcom,spmi/spmi-0/0-04/
driver     power      subsystem  uevent



$ ls /sys/bus/spmi/devices/
0-00    0-01    0-04    spmi-0

$ ls /sys/bus/platform/devices/
100.qcom,revid                 fc4cf000.qcom,spmi
3100.qcom,pm8x41-adc-usr       fd484000.hwlock
6000.qcom,rtc                  fd510000.pinctrl
alarmtimer                     fd8c0000.clock-controller
b040.pm8xxx-pwm                gpio_keys.5
cpu-pmu.1                      gpios.18
cpus.0                         iio-thermal.4
d000.pm8xxx-pwm-led            pm8841-s1.6
d800.pm8xxx-wled               pm8841-s2.7
f9000000.interrupt-controller  pm8941-l3.11
f9011000.smem                  pm8941-l6.12
f9012000.regulator             reg-dummy
f9020000.timer                 regulator-l11.14
f9088000.clock-controller      regulator-l19.15
f9098000.clock-controller      regulator-l20.16
f90a8000.clock-controller      regulator-l22.17
f90b8000.clock-controller      regulator-l9.13
f9824900.sdhc                  regulator-s1.8
f991e000.serial                regulator-s2.9
f9924000.i2c2                  regulator-s3.10
f9928000.i2c6                  regulatory.0
f9bff000.rng                   snd-soc-dummy
fc400000.clock-controller      soc.2
fc4281d0.qcom,mpm              timer.3
fc4ab000.restart


=====  all three patches applied:

$ ls /sys/devices/
ARMv7 Krait  cpu-pmu.1    platform     software     tracepoint
breakpoint   cpus.0       soc.2        system       virtual

$ ls /sys/devices/soc.2/
f9000000.interrupt-controller  fc4281d0.qcom,mpm
f9011000.smem                  fc4ab000.restart
f9012000.regulator             fc4cf000.qcom,spmi
f9020000.timer                 fd484000.hwlock
f9088000.clock-controller      fd510000.pinctrl
f9098000.clock-controller      fd8c0000.clock-controller
f90a8000.clock-controller      gpio_keys.5
f90b8000.clock-controller      iio-thermal.4
f9824900.sdhc                  modalias
f991e000.serial                power
f9924000.i2c2                  subsystem
f9928000.i2c6                  timer.3
f9bff000.rng                   uevent
fc400000.clock-controller

$ ls /sys/devices/soc.2/fc4cf000.qcom,spmi/
driver     modalias   power      spmi-0     subsystem  uevent

$ ls /sys/devices/soc.2/fc4cf000.qcom,spmi/spmi-0/
0-00       0-01       0-04       power      subsystem  uevent

$ ls /sys/devices/soc.2/fc4cf000.qcom,spmi/spmi-0/0-00/
0-00:100.qcom,revid            driver
0-00:3100.qcom,pm8x41-adc-usr  power
0-00:6000.qcom,rtc             subsystem
0-00:gpios                     uevent
 
 

$ ls /sys/bus/platform/devices/soc.2/
f9000000.interrupt-controller  fc4281d0.qcom,mpm
f9011000.smem                  fc4ab000.restart
f9012000.regulator             fc4cf000.qcom,spmi
f9020000.timer                 fd484000.hwlock
f9088000.clock-controller      fd510000.pinctrl
f9098000.clock-controller      fd8c0000.clock-controller
f90a8000.clock-controller      gpio_keys.5
f90b8000.clock-controller      iio-thermal.4
f9824900.sdhc                  modalias
f991e000.serial                power
f9924000.i2c2                  subsystem
f9928000.i2c6                  timer.3
f9bff000.rng                   uevent
fc400000.clock-controller

$ ls /sys/bus/platform/devices/soc.2/fc4cf000.qcom,spmi/
driver     modalias   power      spmi-0     subsystem  uevent

$ ls /sys/bus/platform/devices/soc.2/fc4cf000.qcom,spmi/spmi-0/
0-00       0-01       0-04       power      subsystem  uevent

$ ls /sys/bus/platform/devices/soc.2/fc4cf000.qcom,spmi/spmi-0/0-00/
0-00:100.qcom,revid            driver
0-00:3100.qcom,pm8x41-adc-usr  power
0-00:6000.qcom,rtc             subsystem
0-00:gpios                     uevent

$ ls /sys/bus/platform/devices/soc.2/fc4cf000.qcom,spmi/spmi-0/0-01/
0-01:b040.pm8xxx-pwm      driver                    uevent
0-01:d000.pm8xxx-pwm-led  power
0-01:d800.pm8xxx-wled     subsystem

$ ls /sys/bus/platform/devices/soc.2/fc4cf000.qcom,spmi/spmi-0/0-04/
0-04:100.qcom,revid  power                uevent
driver               subsystem



$ ls /sys/bus/spmi/devices/
0-00                           0-01:b040.pm8xxx-pwm
0-00:100.qcom,revid            0-01:d000.pm8xxx-pwm-led
0-00:3100.qcom,pm8x41-adc-usr  0-01:d800.pm8xxx-wled
0-00:6000.qcom,rtc             0-04
0-00:gpios                     0-04:100.qcom,revid
0-01                           spmi-0

$ ls /sys/bus/platform/devices/
alarmtimer                     f9bff000.rng
cpu-pmu.1                      fc400000.clock-controller
cpus.0                         fc4281d0.qcom,mpm
f9000000.interrupt-controller  fc4ab000.restart
f9011000.smem                  fc4cf000.qcom,spmi
f9012000.regulator             fd484000.hwlock
f9020000.timer                 fd510000.pinctrl
f9088000.clock-controller      fd8c0000.clock-controller
f9098000.clock-controller      gpio_keys.5
f90a8000.clock-controller      iio-thermal.4
f90b8000.clock-controller      reg-dummy
f9824900.sdhc                  regulatory.0
f991e000.serial                snd-soc-dummy
f9924000.i2c2                  soc.2
f9928000.i2c6                  timer.3


=====  patches 2 and 3 applied:

$ ls /sys/devices/
ARMv7 Krait  cpu-pmu.1    platform     software     tracepoint
breakpoint   cpus.0       soc.2        system       virtual

$ ls /sys/devices/soc.2/
f9000000.interrupt-controller  fc4281d0.qcom,mpm
f9011000.smem                  fc4ab000.restart
f9012000.regulator             fc4cf000.qcom,spmi
f9020000.timer                 fd484000.hwlock
f9088000.clock-controller      fd510000.pinctrl
f9098000.clock-controller      fd8c0000.clock-controller
f90a8000.clock-controller      gpio_keys.5
f90b8000.clock-controller      iio-thermal.4
f9824900.sdhc                  modalias
f991e000.serial                power
f9924000.i2c2                  subsystem
f9928000.i2c6                  timer.3
f9bff000.rng                   uevent
fc400000.clock-controller

$ ls /sys/devices/soc.2/fc4cf000.qcom,spmi/
driver     modalias   power      spmi-0     subsystem  uevent

$ ls /sys/devices/soc.2/fc4cf000.qcom,spmi/spmi-0/
0-00       0-01       0-04       power      subsystem  uevent

$ ls /sys/devices/soc.2/fc4cf000.qcom,spmi/spmi-0/0-00/
0-00:100.qcom,revid            driver
0-00:3100.qcom,pm8x41-adc-usr  power
0-00:6000.qcom,rtc             subsystem
0-00:gpios                     uevent
 
 
$ ls /sys/bus/platform/devices/soc.2/
f9000000.interrupt-controller  fc4281d0.qcom,mpm
f9011000.smem                  fc4ab000.restart
f9012000.regulator             fc4cf000.qcom,spmi
f9020000.timer                 fd484000.hwlock
f9088000.clock-controller      fd510000.pinctrl
f9098000.clock-controller      fd8c0000.clock-controller
f90a8000.clock-controller      gpio_keys.5
f90b8000.clock-controller      iio-thermal.4
f9824900.sdhc                  modalias
f991e000.serial                power
f9924000.i2c2                  subsystem
f9928000.i2c6                  timer.3
f9bff000.rng                   uevent
fc400000.clock-controller

$ ls /sys/bus/platform/devices/soc.2/fc4cf000.qcom,spmi/
driver     modalias   power      spmi-0     subsystem  uevent

$ ls /sys/bus/platform/devices/soc.2/fc4cf000.qcom,spmi/spmi-0/
0-00       0-01       0-04       power      subsystem  uevent

$ ls /sys/bus/platform/devices/soc.2/fc4cf000.qcom,spmi/spmi-0/0-00/
0-00:100.qcom,revid            driver
0-00:3100.qcom,pm8x41-adc-usr  power
0-00:6000.qcom,rtc             subsystem
0-00:gpios                     uevent

$ ls /sys/bus/platform/devices/soc.2/fc4cf000.qcom,spmi/spmi-0/0-01/
0-01:b040.pm8xxx-pwm      driver                    uevent
0-01:d000.pm8xxx-pwm-led  power
0-01:d800.pm8xxx-wled     subsystem

$ ls /sys/bus/platform/devices/soc.2/fc4cf000.qcom,spmi/spmi-0/0-04/
0-04:100.qcom,revid  power                uevent
driver               subsystem
 

$ ls /sys/bus/spmi/devices/
0-00    0-01    0-04    spmi-0

$ ls /sys/bus/platform/devices/
0-00:100.qcom,revid            fc4281d0.qcom,mpm
0-00:3100.qcom,pm8x41-adc-usr  fc4ab000.restart
0-00:6000.qcom,rtc             fc4cf000.qcom,spmi
0-00:gpios                     fd484000.hwlock
0-01:b040.pm8xxx-pwm           fd510000.pinctrl
0-01:d000.pm8xxx-pwm-led       fd8c0000.clock-controller
0-01:d800.pm8xxx-wled          gpio_keys.5
0-04:100.qcom,revid            iio-thermal.4
alarmtimer                     pm8841-s1.6
cpu-pmu.1                      pm8841-s2.7
cpus.0                         pm8941-l3.11
f9000000.interrupt-controller  pm8941-l6.12
f9011000.smem                  reg-dummy
f9012000.regulator             regulator-l11.14
f9020000.timer                 regulator-l19.15
f9088000.clock-controller      regulator-l20.16
f9098000.clock-controller      regulator-l22.17
f90a8000.clock-controller      regulator-l9.13
f90b8000.clock-controller      regulator-s1.8
f9824900.sdhc                  regulator-s2.9
f991e000.serial                regulator-s3.10
f9924000.i2c2                  regulatory.0
f9928000.i2c6                  snd-soc-dummy
f9bff000.rng                   soc.2
fc400000.clock-controller      timer.3


--
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

[RFC PATCH 2/3] devicetree: provide hook to allow setting devicetree device name

[Frank Rowand]

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

Optionally push devicetree device naming into a function called dynamically by
of_device_alloc().

TODO:
   Change made to of_device_alloc() could also be made to
   of_amba_device_create()

Signed-off-by: Frank Rowand <frank.rowand@sonymobile.com>
---
 drivers/of/platform.c |    2 ++
 include/linux/of.h    |    2 ++
 3 files changed, 43 insertions(+)

Index: b/drivers/of/platform.c
===================================================================
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -179,6 +179,8 @@ struct platform_device *of_device_alloc(
 
 	if (bus_id)
 		dev_set_name(&dev->dev, "%s", bus_id);
+	else if (np->parent->of_device_make_bus_id)
+		np->parent->of_device_make_bus_id(&dev->dev);
 	else
 		of_device_make_bus_id(&dev->dev);
 
Index: b/include/linux/of.h
===================================================================
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -17,6 +17,7 @@
  */
 #include <linux/types.h>
 #include <linux/bitops.h>
+#include <linux/device.h>
 #include <linux/errno.h>
 #include <linux/kobject.h>
 #include <linux/mod_devicetable.h>
@@ -60,6 +61,7 @@ struct device_node {
 	struct	kobject kobj;
 	unsigned long _flags;
 	void	*data;
+	void    (*of_device_make_bus_id)(struct device *dev);
 #if defined(CONFIG_SPARC)
 	const char *path_component_name;
 	unsigned int unique_id;

Re: [RFC PATCH 2/3] devicetree: provide hook to allow setting devicetree device name

[Grant Likely]

On Wed, May 7, 2014 at 1:52 AM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> From: Frank Rowand <frank.rowand-/MT0OVThwyLZJqsBc5GL+g@public.gmane.org>
>
> Optionally push devicetree device naming into a function called dynamically by
> of_device_alloc().
>
> TODO:
>    Change made to of_device_alloc() could also be made to
>    of_amba_device_create()
>
> Signed-off-by: Frank Rowand <frank.rowand-/MT0OVThwyLZJqsBc5GL+g@public.gmane.org>
> ---
>  drivers/of/platform.c |    2 ++
>  include/linux/of.h    |    2 ++
>  3 files changed, 43 insertions(+)
>
> Index: b/drivers/of/platform.c
> ===================================================================
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -179,6 +179,8 @@ struct platform_device *of_device_alloc(
>
>         if (bus_id)
>                 dev_set_name(&dev->dev, "%s", bus_id);
> +       else if (np->parent->of_device_make_bus_id)
> +               np->parent->of_device_make_bus_id(&dev->dev);

This isn't dangerous in the way the first patch is, but I don't want
to expose control over the naming scheme outside of the core code. It
would be too easy to get wrong and mess up other drivers. The problem
that is being hit is clearly a bug in the core DT code. It should be
naming devices so that there isn't a conflict. I think Rob's suggested
patch is the best fix.

g.
--
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

[RFC PATCH 3/3] devicetree, qcomm PMIC: use new hook to make PMIC device names unique

[Frank Rowand]

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

The previous patch in the series does:

   Optionally push device naming into a function called dynamically by
   of_device_alloc().

This patch adds an example of using that capability.

Signed-off-by: Frank Rowand <frank.rowand@sonymobile.com>
---
 drivers/mfd/pm8x41.c |   39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

Index: b/drivers/mfd/pm8x41.c
===================================================================
--- a/drivers/mfd/pm8x41.c
+++ b/drivers/mfd/pm8x41.c
@@ -13,6 +13,7 @@
 #include <linux/module.h>
 #include <linux/spmi.h>
 #include <linux/regmap.h>
+#include <linux/of_address.h>
 #include <linux/of_platform.h>
 
 static const struct regmap_config pm8x41_regmap_config = {
@@ -32,6 +33,43 @@ static void pm8x41_remove(struct spmi_de
 	device_for_each_child(&sdev->dev, NULL, pm8x41_remove_child);
 }
 
+static void spmi_of_device_make_bus_id(struct device *dev)
+{
+	struct device_node *node = dev->of_node;
+	const __be32 *reg;
+	u64 addr;
+	const __be32 *addrp;
+	struct spmi_device *sdev;
+
+	sdev = container_of(dev->parent, struct spmi_device, dev);
+
+	/*
+	 * For MMIO, get the physical address
+	 */
+	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;
+		}
+		if (addr != OF_BAD_ADDR) {
+			dev_set_name(dev, "%d-%02x:%llx.%s",
+				     sdev->ctrl->nr, sdev->usid,
+				     (unsigned long long)addr, node->name);
+			return;
+		}
+	}
+
+	dev_set_name(dev, "%d-%02x:%s",
+		     sdev->ctrl->nr, sdev->usid,
+		     node->name);
+}
+
 static int pm8x41_probe(struct spmi_device *sdev)
 {
 	struct regmap *regmap;
@@ -42,6 +80,7 @@ static int pm8x41_probe(struct spmi_devi
 		return PTR_ERR(regmap);
 	}
 
+	sdev->dev.of_node->of_device_make_bus_id = spmi_of_device_make_bus_id;
 	return of_platform_populate(sdev->dev.of_node, NULL, NULL, &sdev->dev);
 }
 

[RFC PATCH 1/3] devicetree: set bus type same as parent

[Frank Rowand]

From: Frank Rowand <frank.rowand-/MT0OVThwyLZJqsBc5GL+g@public.gmane.org>

This is a somewhat scary patch since it touches a path that is central to
device creation based on the device tree.  It should not be applied without
careful consideration.

I am not sure if this patch is a good idea, even if it does not break
anything.

An issue with the path of SPMI nodes under /sys/bus/... was reported in
https://lkml.org/lkml/2014/4/23/312.  The symptom is that two different
grandchild nodes of the spmi with the same node-name@unit-address will
result in attempting to create duplicate links at
/sys/bus/platform/devices/unit-address.node-name.  It turns out that the
specific example provided might not be an expected configuration for
current hardware, but the reported trap remains an issue.

The common pattern exposed is a driver probe function calling
of_platform_populate() to create child devices.  As the reporting
email noted, the devices are created with dev.bus set to
platform_bus_type.  Thus all devices created via this pattern will
result in a link in /sys/bus/platform/devices/, with the risk that
a name collision will occur.

This patch reduces the scope of possible name collisions to devices
on the same bus type.  This is still not ideal, because a legal
device tree source file can result in run time errors.  In the case
of SPMI nodes, the collisions will occur in /bus/spmi/devices/.

I have not investigated whether other drivers would be negatively impacted
by this change - there are 26 drivers in tree that call of_platform_populate().

Signed-off-by: Frank Rowand <frank.rowand-/MT0OVThwyLZJqsBc5GL+g@public.gmane.org>
---
 drivers/of/platform.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Index: b/drivers/of/platform.c
===================================================================
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -217,7 +217,10 @@ static struct platform_device *of_platfo
 	dev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
 	if (!dev->dev.dma_mask)
 		dev->dev.dma_mask = &dev->dev.coherent_dma_mask;
-	dev->dev.bus = &platform_bus_type;
+	if (parent && parent->bus)
+		dev->dev.bus = parent->bus;
+	else
+		dev->dev.bus = &platform_bus_type;
 	dev->dev.platform_data = platform_data;
 
 	/* We do not fill the DMA ops for platform devices by default.
--
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: [RFC PATCH 1/3] devicetree: set bus type same as parent

[Grant Likely]

On Wed, May 7, 2014 at 1:51 AM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> From: Frank Rowand <frank.rowand-/MT0OVThwyLZJqsBc5GL+g@public.gmane.org>
>
> This is a somewhat scary patch since it touches a path that is central to
> device creation based on the device tree.  It should not be applied without
> careful consideration.
>
> I am not sure if this patch is a good idea, even if it does not break
> anything.
>
> An issue with the path of SPMI nodes under /sys/bus/... was reported in
> https://lkml.org/lkml/2014/4/23/312.  The symptom is that two different
> grandchild nodes of the spmi with the same node-name@unit-address will
> result in attempting to create duplicate links at
> /sys/bus/platform/devices/unit-address.node-name.  It turns out that the
> specific example provided might not be an expected configuration for
> current hardware, but the reported trap remains an issue.
>
> The common pattern exposed is a driver probe function calling
> of_platform_populate() to create child devices.  As the reporting
> email noted, the devices are created with dev.bus set to
> platform_bus_type.  Thus all devices created via this pattern will
> result in a link in /sys/bus/platform/devices/, with the risk that
> a name collision will occur.
>
> This patch reduces the scope of possible name collisions to devices
> on the same bus type.  This is still not ideal, because a legal
> device tree source file can result in run time errors.  In the case
> of SPMI nodes, the collisions will occur in /bus/spmi/devices/.
>
> I have not investigated whether other drivers would be negatively impacted
> by this change - there are 26 drivers in tree that call of_platform_populate().
>
> Signed-off-by: Frank Rowand <frank.rowand-/MT0OVThwyLZJqsBc5GL+g@public.gmane.org>
> ---
>  drivers/of/platform.c |    5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> Index: b/drivers/of/platform.c
> ===================================================================
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -217,7 +217,10 @@ static struct platform_device *of_platfo
>         dev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
>         if (!dev->dev.dma_mask)
>                 dev->dev.dma_mask = &dev->dev.coherent_dma_mask;
> -       dev->dev.bus = &platform_bus_type;
> +       if (parent && parent->bus)
> +               dev->dev.bus = parent->bus;
> +       else
> +               dev->dev.bus = &platform_bus_type;

Yeah, this is a very wrong thing to do. It makes the device accessible
to the other bus types behaviour and most likely will result in
container_of() being used on the struct device to get the other bus
types container structure. It is a very bad idea.

I also suspect that you're conflating the concept of a bus_type and
the instance of a bus (like the platform bus) in the /sys/devices/
hierarchy. Among other things, a bus_type contains a list of drivers
and a list of devices that can potentially be bound together because
they use the same interface.

"platform_bus" on the other hand isn't really a device at all. It is a
bare struct device with no bus type and no behaviour. It exists merely
as a container for other devices. By default, any platform_device that
gets registered will be added as a child of platform_bus unless the
parent pointer is already assigned to something else.

The device hierarchy can pretty much be anything. A child can have any
bus type regardless of the bus type of the parent. Individual
subsystems will put restrictions on this (ie. all children of i2c bus
instances will use the i2c bus type), but the driver core is very
flexible.

The reason we use platform_bus_type so extensively is that there are a
very large number of devices that don't have any bus behaviour
expectations. No hot plug. No auto detection. Usually memory mapped.
The device just exists and is immediately accessible by the system.

g.

>         dev->dev.platform_data = platform_data;
>
>         /* We do not fill the DMA ops for platform devices by default.
--
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: [RFC PATCH 0/3] devicetree, qcomm PMIC: fix node name conflict

[Rob Herring]

On Tue, May 6, 2014 at 7:48 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> An issue with the path of SPMI nodes under /sys/bus/... was reported in
> https://lkml.org/lkml/2014/4/23/312.  The symptom is that two different
> grandchild nodes of the spmi with the same node-name@unit-address will
> result in attempting to create duplicate links at
> /sys/bus/platform/devices/unit-address.node-name.  It turns out that the
> specific example provided might not be an expected configuration for
> current hardware, but the reported trap remains an issue.
>
> I have been poking at the problem, trying to figure out how to cleanly
> fix the issue without breaking devicetree device creation.
>
> The first patch in the series is the one that may be a very bad idea.  Or
> it may help show the way forward to deal with what I think is the major
> underlying problem.  I have not finished investigating the possible negative
> side effects.  And I am still thinking whether this is a conceptually good
> approach, or whether it is simply an expediant hack that hides the underlying
> problem.  But I am throwing this out prematurely because I have mentioned
> it to several people, and I want to make it visible to everyone involved.
>
> The underlying architectural problem (in my opinion) is that a lot of devices
> are created by the device tree infrastructure as platform devices, when they
> truly should not be platform devices.  They should not be platform devices
> because they are not physically on a platform bus, they are instead somewhere
> below some other bus.  The first patch in this series is a hack which
> results in the devices still being represented by "struct platform_device"
> objects, but with a link to their parent's "struct bus_type" instead of
> to &platform_bus_type.
>
> The second patch does not require the first patch.  The second patch provides
> a mechanism to allow subsystems to provide a method of naming devices to
> avoid name collisions.
>
> The third patch provides an example of a subsystem using the new feature
> provided by the second patch.
>

I think the primary question to ask is there any added benefit to
having the additional hierarchy of devices. I don't think there is
much support to have more hierarchy from what I have seen of past
discussions.

Another approach could be to support having multiple platform bus
instances. Then drivers can easily create a new instance for each set
of sub-devices.

This can be solved in a much less invasive way just in the DT naming
algorithm. This is slightly different from what I had suggested of
just dropping the unit address. It keeps the unit address, but adds
the unique index on untranslate-able addresses. The diff is bigger due
to refactoring to reduce the indentation levels. It is untested and
whitespace corrupted:

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 404d1da..c77dd7a 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -105,23 +105,33 @@ void of_device_make_bus_id(struct device *dev)
         * For MMIO, get the physical address
         */
        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;
-               }
-               if (addr != OF_BAD_ADDR) {
-                       dev_set_name(dev, "%llx.%s",
-                                    (unsigned long long)addr, node->name);
-                       return;
-               }
+       if (!reg)
+               goto no_bus_id;
+
+       if (of_can_translate_address(node)) {
+               addr = of_translate_address(node, reg);
+               if (addr == OF_BAD_ADDR)
+                       goto no_bus_id;
+
+               dev_set_name(dev, "%llx.%s",
+                            (unsigned long long)addr, node->name);
+               return;
        }

+       addrp = of_get_address(node, 0, NULL, NULL);
+       if (!addrp)
+               goto no_bus_id;
+
+       addr = of_read_number(addrp, 1);
+       if (addr == OF_BAD_ADDR)
+               goto no_bus_id;
+
+       magic = atomic_add_return(1, &bus_no_reg_magic);
+       dev_set_name(dev, "%llx.%s.%d", (unsigned long long)addr,
+                    node->name, magic - 1);
+       return;
+
+no_bus_id:
        /*
         * No BusID, use the node name and add a globally incremented
         * counter (and pray...)
--
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: [RFC PATCH 0/3] devicetree, qcomm PMIC: fix node name conflict

[Frank Rowand]

On 5/6/2014 6:32 PM, Rob Herring wrote:
> On Tue, May 6, 2014 at 7:48 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> An issue with the path of SPMI nodes under /sys/bus/... was reported in
>> https://lkml.org/lkml/2014/4/23/312.  The symptom is that two different

< snip >

>>
> 
> I think the primary question to ask is there any added benefit to
> having the additional hierarchy of devices. I don't think there is
> much support to have more hierarchy from what I have seen of past
> discussions.

The hierarchy avoids the name space conflict.

It also maps the physical reality better than pretending all devices
are on the platform bus.

It follows the model that non-device tree systems use.  For example,
why should a usb device show up under /sys/bus/platform/ instead of
under /sys/bus/usb/ ?  (I'm not positive this actually happens, but
let me pretend it would.)

> Another approach could be to support having multiple platform bus
> instances. Then drivers can easily create a new instance for each set
> of sub-devices.
> 
> This can be solved in a much less invasive way just in the DT naming
> algorithm. This is slightly different from what I had suggested of
> just dropping the unit address. It keeps the unit address, but adds
> the unique index on untranslate-able addresses. The diff is bigger due
> to refactoring to reduce the indentation levels. It is untested and
> whitespace corrupted:
> 
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index 404d1da..c77dd7a 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -105,23 +105,33 @@ void of_device_make_bus_id(struct device *dev)
>          * For MMIO, get the physical address
>          */
>         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;
> -               }
> -               if (addr != OF_BAD_ADDR) {
> -                       dev_set_name(dev, "%llx.%s",
> -                                    (unsigned long long)addr, node->name);
> -                       return;
> -               }
> +       if (!reg)
> +               goto no_bus_id;
> +
> +       if (of_can_translate_address(node)) {
> +               addr = of_translate_address(node, reg);
> +               if (addr == OF_BAD_ADDR)
> +                       goto no_bus_id;
> +
> +               dev_set_name(dev, "%llx.%s",
> +                            (unsigned long long)addr, node->name);
> +               return;
>         }
> 
> +       addrp = of_get_address(node, 0, NULL, NULL);
> +       if (!addrp)
> +               goto no_bus_id;
> +
> +       addr = of_read_number(addrp, 1);
> +       if (addr == OF_BAD_ADDR)
> +               goto no_bus_id;
> +
> +       magic = atomic_add_return(1, &bus_no_reg_magic);
> +       dev_set_name(dev, "%llx.%s.%d", (unsigned long long)addr,
> +                    node->name, magic - 1);
> +       return;
> +
> +no_bus_id:
>         /*
>          * No BusID, use the node name and add a globally incremented
>          * counter (and pray...)
> 

I think the refactored code is easier to read.  (End of bike shed...)

The result of that patch is an even uglier set of device names.  I would love to get rid of
the bus_no_reg_magic instead of making more extensive use of it.  The new names for the
system that started this discussion are:

$ ls /sys/devices/soc.2/fc4cf000.qcom,spmi/spmi-0/0-00/
100.qcom,revid.19            gpios.21
3100.qcom,pm8x41-adc-usr.20  power
6000.qcom,rtc.18             subsystem
driver                       uevent

$ ls /sys/bus/platform/devices/soc.2/fc4cf000.qcom,spmi/spmi-0/0-00/
100.qcom,revid.19            gpios.21
3100.qcom,pm8x41-adc-usr.20  power
6000.qcom,rtc.18             subsystem
driver                       uevent

$ ls /sys/bus/platform/devices/soc.2/fc4cf000.qcom,spmi/spmi-0/0-01/
b040.pm8xxx-pwm.22      driver                  uevent
d000.pm8xxx-pwm-led.23  power
d800.pm8xxx-wled.24     subsystem

$ ls /sys/bus/platform/devices/soc.2/fc4cf000.qcom,spmi/spmi-0/0-04/
100.qcom,revid.25  power              uevent
driver             subsystem

$ ls /sys/bus/platform/devices/
100.qcom,revid.19              fc4ab000.restart
100.qcom,revid.25              fc4cf000.qcom,spmi
3100.qcom,pm8x41-adc-usr.20    fd484000.hwlock
6000.qcom,rtc.18               fd510000.pinctrl
alarmtimer                     fd8c0000.clock-controller
b040.pm8xxx-pwm.22             gpio_keys.5
cpu-pmu.1                      gpios.21
cpus.0                         iio-thermal.4
d000.pm8xxx-pwm-led.23         pm8841-s1.6
d800.pm8xxx-wled.24            pm8841-s2.7
f9000000.interrupt-controller  pm8941-l3.11
f9011000.smem                  pm8941-l6.12
f9012000.regulator             reg-dummy
f9020000.timer                 regulator-l11.14
f9088000.clock-controller      regulator-l19.15
f9098000.clock-controller      regulator-l20.16
f90a8000.clock-controller      regulator-l22.17
f90b8000.clock-controller      regulator-l9.13
f9824900.sdhc                  regulator-s1.8
f991e000.serial                regulator-s2.9
f9924000.i2c2                  regulator-s3.10
f9928000.i2c6                  regulatory.0
f9bff000.rng                   snd-soc-dummy
fc400000.clock-controller      soc.2
fc4281d0.qcom,mpm              timer.3

--
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: [RFC PATCH 0/3] devicetree, qcomm PMIC: fix node name conflict

[Grant Likely]

On Wed, May 7, 2014 at 2:32 AM, Rob Herring <robherring2@gmail.com> wrote:
> On Tue, May 6, 2014 at 7:48 PM, Frank Rowand <frowand.list@gmail.com> wrote:
>> An issue with the path of SPMI nodes under /sys/bus/... was reported in
>> https://lkml.org/lkml/2014/4/23/312.  The symptom is that two different
>> grandchild nodes of the spmi with the same node-name@unit-address will
>> result in attempting to create duplicate links at
>> /sys/bus/platform/devices/unit-address.node-name.  It turns out that the
>> specific example provided might not be an expected configuration for
>> current hardware, but the reported trap remains an issue.
>>
>> I have been poking at the problem, trying to figure out how to cleanly
>> fix the issue without breaking devicetree device creation.
>>
>> The first patch in the series is the one that may be a very bad idea.  Or
>> it may help show the way forward to deal with what I think is the major
>> underlying problem.  I have not finished investigating the possible negative
>> side effects.  And I am still thinking whether this is a conceptually good
>> approach, or whether it is simply an expediant hack that hides the underlying
>> problem.  But I am throwing this out prematurely because I have mentioned
>> it to several people, and I want to make it visible to everyone involved.
>>
>> The underlying architectural problem (in my opinion) is that a lot of devices
>> are created by the device tree infrastructure as platform devices, when they
>> truly should not be platform devices.  They should not be platform devices
>> because they are not physically on a platform bus, they are instead somewhere
>> below some other bus.  The first patch in this series is a hack which
>> results in the devices still being represented by "struct platform_device"
>> objects, but with a link to their parent's "struct bus_type" instead of
>> to &platform_bus_type.
>>
>> The second patch does not require the first patch.  The second patch provides
>> a mechanism to allow subsystems to provide a method of naming devices to
>> avoid name collisions.
>>
>> The third patch provides an example of a subsystem using the new feature
>> provided by the second patch.
>>
>
> I think the primary question to ask is there any added benefit to
> having the additional hierarchy of devices. I don't think there is
> much support to have more hierarchy from what I have seen of past
> discussions.
>
> Another approach could be to support having multiple platform bus
> instances. Then drivers can easily create a new instance for each set
> of sub-devices.
>

I fear that it will be more invasive that you think it will be. Right
now the bus_type abstraction is a mechanism for matching drivers and
devices. The bus type contains a bag of device drivers, and it tries
to match one of them to a device when a device gets registered to that
device (or when a driver gets registered, try to match it to one of
the devices it already knows about). You can see this in the
/sys/bus/<type>/drivers and /sys/bus/<type>/devices directories.

Splitting the platform bus type into multiple instances is not trivial
because the drivers will only be available to one instance. You'd need
to figure out how to make a device driver available to multiple
bus_type instances (ideally without having to manually add the driver
to each bus_type at module load time).

> This can be solved in a much less invasive way just in the DT naming
> algorithm. This is slightly different from what I had suggested of
> just dropping the unit address. It keeps the unit address, but adds
> the unique index on untranslate-able addresses. The diff is bigger due
> to refactoring to reduce the indentation levels. It is untested and
> whitespace corrupted:
>
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index 404d1da..c77dd7a 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -105,23 +105,33 @@ void of_device_make_bus_id(struct device *dev)
>          * For MMIO, get the physical address
>          */
>         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;
> -               }
> -               if (addr != OF_BAD_ADDR) {
> -                       dev_set_name(dev, "%llx.%s",
> -                                    (unsigned long long)addr, node->name);
> -                       return;
> -               }
> +       if (!reg)
> +               goto no_bus_id;
> +
> +       if (of_can_translate_address(node)) {
> +               addr = of_translate_address(node, reg);
> +               if (addr == OF_BAD_ADDR)
> +                       goto no_bus_id;
> +
> +               dev_set_name(dev, "%llx.%s",
> +                            (unsigned long long)addr, node->name);
> +               return;
>         }
>
> +       addrp = of_get_address(node, 0, NULL, NULL);
> +       if (!addrp)
> +               goto no_bus_id;
> +
> +       addr = of_read_number(addrp, 1);
> +       if (addr == OF_BAD_ADDR)
> +               goto no_bus_id;
> +
> +       magic = atomic_add_return(1, &bus_no_reg_magic);
> +       dev_set_name(dev, "%llx.%s.%d", (unsigned long long)addr,
> +                    node->name, magic - 1);
> +       return;
> +
> +no_bus_id:

Looks like a reasonable change to me.

g.

>         /*
>          * No BusID, use the node name and add a globally incremented
>          * counter (and pray...)

Re: [RFC PATCH 0/3] devicetree, qcomm PMIC: fix node name conflict

[Bjorn Andersson]

On Tue, May 6, 2014 at 6:32 PM, Rob Herring <robherring2@gmail.com> wrote:
> On Tue, May 6, 2014 at 7:48 PM, Frank Rowand <frowand.list@gmail.com> wrote:
>> An issue with the path of SPMI nodes under /sys/bus/... was reported in
>> https://lkml.org/lkml/2014/4/23/312.  The symptom is that two different
>> grandchild nodes of the spmi with the same node-name@unit-address will
>> result in attempting to create duplicate links at
>> /sys/bus/platform/devices/unit-address.node-name.  It turns out that the
>> specific example provided might not be an expected configuration for
>> current hardware, but the reported trap remains an issue.
>>
>> I have been poking at the problem, trying to figure out how to cleanly
>> fix the issue without breaking devicetree device creation.
>>
>> The first patch in the series is the one that may be a very bad idea.  Or
>> it may help show the way forward to deal with what I think is the major
>> underlying problem.  I have not finished investigating the possible negative
>> side effects.  And I am still thinking whether this is a conceptually good
>> approach, or whether it is simply an expediant hack that hides the underlying
>> problem.  But I am throwing this out prematurely because I have mentioned
>> it to several people, and I want to make it visible to everyone involved.
>>
>> The underlying architectural problem (in my opinion) is that a lot of devices
>> are created by the device tree infrastructure as platform devices, when they
>> truly should not be platform devices.  They should not be platform devices
>> because they are not physically on a platform bus, they are instead somewhere
>> below some other bus.  The first patch in this series is a hack which
>> results in the devices still being represented by "struct platform_device"
>> objects, but with a link to their parent's "struct bus_type" instead of
>> to &platform_bus_type.
>>
>> The second patch does not require the first patch.  The second patch provides
>> a mechanism to allow subsystems to provide a method of naming devices to
>> avoid name collisions.
>>
>> The third patch provides an example of a subsystem using the new feature
>> provided by the second patch.
>>
>
> I think the primary question to ask is there any added benefit to
> having the additional hierarchy of devices. I don't think there is
> much support to have more hierarchy from what I have seen of past
> discussions.
>
> Another approach could be to support having multiple platform bus
> instances. Then drivers can easily create a new instance for each set
> of sub-devices.
>
> This can be solved in a much less invasive way just in the DT naming
> algorithm. This is slightly different from what I had suggested of
> just dropping the unit address. It keeps the unit address, but adds
> the unique index on untranslate-able addresses. The diff is bigger due
> to refactoring to reduce the indentation levels. It is untested and
> whitespace corrupted:

The unique index leads to an interesting dependency between the order
of nodes in the DTB and userspace; paths to e.g. our the path to our
block devices contains soc.X where X changes now and then. Fortunately
soc.X won't change that often, but forcing more peripheral nodes to use
the same schema would show the problem quite often.

Does translatable/untranslatable refer to if this is an address translatable
my the cpu or that it's just a translatable address on this specific bus?
As far as I can see it's the latter and in our case (revid { reg =
<0x100, 0x100>; })
seem to be translatable with the current implementation.

Regards,
Bjorn

Re: [RFC PATCH 0/3] devicetree, qcomm PMIC: fix node name conflict

[Rob Herring]

On Wed, May 7, 2014 at 10:12 AM, Bjorn Andersson <bjorn-UYDU3/A3LUY@public.gmane.org> wrote:
> On Tue, May 6, 2014 at 6:32 PM, Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> On Tue, May 6, 2014 at 7:48 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>> An issue with the path of SPMI nodes under /sys/bus/... was reported in
>>> https://lkml.org/lkml/2014/4/23/312.  The symptom is that two different
>>> grandchild nodes of the spmi with the same node-name@unit-address will
>>> result in attempting to create duplicate links at
>>> /sys/bus/platform/devices/unit-address.node-name.  It turns out that the
>>> specific example provided might not be an expected configuration for
>>> current hardware, but the reported trap remains an issue.

[...]

>> This can be solved in a much less invasive way just in the DT naming
>> algorithm. This is slightly different from what I had suggested of
>> just dropping the unit address. It keeps the unit address, but adds
>> the unique index on untranslate-able addresses. The diff is bigger due
>> to refactoring to reduce the indentation levels. It is untested and
>> whitespace corrupted:
>
> The unique index leads to an interesting dependency between the order
> of nodes in the DTB and userspace; paths to e.g. our the path to our
> block devices contains soc.X where X changes now and then. Fortunately
> soc.X won't change that often, but forcing more peripheral nodes to use
> the same schema would show the problem quite often.

Userspace depending on the device names is broken and device names are
not considered part of the sysfs ABI. So devices having randomish
names is a feature. The names and location change when platforms
convert to DT. The location changes when someone decides to add an soc
device to a platform as well.

> Does translatable/untranslatable refer to if this is an address translatable
> my the cpu or that it's just a translatable address on this specific bus?
> As far as I can see it's the latter and in our case (revid { reg =
> <0x100, 0x100>; })
> seem to be translatable with the current implementation.

It should be the former. If the current behavior is the latter, then
the problem is in a different spot.

Rob
--
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