On Wed, May 7, 2014 at 1:51 AM, Frank Rowand <frowand.list@xxxxxxxxx> wrote: > From: Frank Rowand <frank.rowand@xxxxxxxxxxxxxx> > > 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@xxxxxxxxxxxxxx> > --- > 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
