[PATCH] of: use platform_device_add

[PATCH] of: use platform_device_add

[Grant Likely]

This allows platform_device_add a chance to call insert_resource on all
of the resources from OF. At a minimum this fills in proc/iomem and
presumably makes resource tracking and conflict detection work better.
However, it has the side effect of moving all OF generated platform
devices from /sys/devices to /sys/devices/platform/. It /shouldn't/
break userspace because userspace is not supposed to depend on the full
path (because userspace always does what it is supposed to, right?).

It also has a backup call to of_device_add() when running on PowerPC to
catch any devices that have overlapping regions. It will complain about
them, but it will not fail to register the device.

Cc: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Rob Herring <rob.herring@calxeda.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
---

Greg, do you mind taking a look at this? The reason the OF code hasn't been
calling platform_device_add() directly to this point is:
a) there are some trees with resource overlays
b) I want the devices in /sys/devices not /sys/devices/platform.

I could easily add exceptions to platform_device_add() for both those cases, but
I don't like adding DT exceptions to the common code. However, I still need to
support the platforms that unfortunately have overlapping resources. This patch
does that by still calling the old path if platform_device_add() fails, but it
isn't nice either because of_device_add() has to duplicate
platform_device_add(). Blech. Plus the exception only applies for PowerPC.

So, how do you feel about having a 'relaxed' mode for platform_device_add()
which means it won't fail if resources overlap and maybe won't do the silly
platform_bus parent thing. Thoughts?

g.

 drivers/of/platform.c |   28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index b80891b..3d7ba40 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -203,6 +203,7 @@ struct platform_device *of_platform_device_create_pdata(
 					struct device *parent)
 {
 	struct platform_device *dev;
+	int rc;
 
 	if (!of_device_is_available(np))
 		return NULL;
@@ -214,16 +215,39 @@ struct platform_device *of_platform_device_create_pdata(
 #if defined(CONFIG_MICROBLAZE)
 	dev->archdata.dma_mask = 0xffffffffUL;
 #endif
+	dev->name = dev_name(&dev->dev);
 	dev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
-	dev->dev.bus = &platform_bus_type;
 	dev->dev.platform_data = platform_data;
+	dev->dev.id = PLATFORM_DEVID_NONE;
+	/* device_add will assume that this device is on the same node as
+	 * the parent. If there is no parent defined, set the node
+	 * explicitly */
+	if (!parent)
+		set_dev_node(&dev->dev, of_node_to_nid(np));
 
 	/* We do not fill the DMA ops for platform devices by default.
 	 * This is currently the responsibility of the platform code
 	 * to do such, possibly using a device notifier
 	 */
 
-	if (of_device_add(dev) != 0) {
+	rc = platform_device_add(dev);
+#ifdef CONFIG_POWERPC
+	/*
+	 * This POWERPC block isn't pretty, but the commit that adds it is a
+	 * little risky. There are possibly some powerpc platforms that have
+	 * overlapping resources in the device tree. If so, then I want to find
+	 * them, but I don't want to break support in the process. So, if
+	 * platform_device_add() fails, then register the device anyway, but
+	 * complain about it. Hopefully we can find and fix and problem
+	 * platforms before removing this code.
+	 */
+	if (rc == -EBUSY) {
+		dev_warn(&dev->dev, "WARNING: resource overlap in DT node %s\n",
+			np->full_name);
+		rc = of_device_add(dev);
+	}
+#endif
+	if (rc) {
 		platform_device_put(dev);
 		return NULL;
 	}
-- 
1.7.10.4

Re: [PATCH] of: use platform_device_add

[Greg Kroah-Hartman]

On Wed, Nov 21, 2012 at 06:15:59PM +0000, Grant Likely wrote:
> This allows platform_device_add a chance to call insert_resource on all
> of the resources from OF. At a minimum this fills in proc/iomem and
> presumably makes resource tracking and conflict detection work better.
> However, it has the side effect of moving all OF generated platform
> devices from /sys/devices to /sys/devices/platform/. It /shouldn't/
> break userspace because userspace is not supposed to depend on the full
> path (because userspace always does what it is supposed to, right?).
> 
> It also has a backup call to of_device_add() when running on PowerPC to
> catch any devices that have overlapping regions. It will complain about
> them, but it will not fail to register the device.
> 
> Cc: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Rob Herring <rob.herring@calxeda.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
> ---
> 
> Greg, do you mind taking a look at this? The reason the OF code hasn't been
> calling platform_device_add() directly to this point is:
> a) there are some trees with resource overlays
> b) I want the devices in /sys/devices not /sys/devices/platform.

Putting the devices all in the "flat" location of /sys/devices/ is a bit
worrisome to me.  What's wrong with platform/ ?  That is what they are,
right?  Why change this?

> I could easily add exceptions to platform_device_add() for both those cases, but
> I don't like adding DT exceptions to the common code. However, I still need to
> support the platforms that unfortunately have overlapping resources. This patch
> does that by still calling the old path if platform_device_add() fails, but it
> isn't nice either because of_device_add() has to duplicate
> platform_device_add(). Blech. Plus the exception only applies for PowerPC.
> 
> So, how do you feel about having a 'relaxed' mode for platform_device_add()
> which means it won't fail if resources overlap and maybe won't do the silly
> platform_bus parent thing. Thoughts?

I have no objection for the resource issue, if you assure me it will not
be abused :)

But the sysfs location is still an issue, sorry.

thanks,

greg k-h

Re: [PATCH] of: use platform_device_add

[Grant Likely]

On Wed, 21 Nov 2012 10:34:03 -0800, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> On Wed, Nov 21, 2012 at 06:15:59PM +0000, Grant Likely wrote:
> > This allows platform_device_add a chance to call insert_resource on all
> > of the resources from OF. At a minimum this fills in proc/iomem and
> > presumably makes resource tracking and conflict detection work better.
> > However, it has the side effect of moving all OF generated platform
> > devices from /sys/devices to /sys/devices/platform/. It /shouldn't/
> > break userspace because userspace is not supposed to depend on the full
> > path (because userspace always does what it is supposed to, right?).
> > 
> > It also has a backup call to of_device_add() when running on PowerPC to
> > catch any devices that have overlapping regions. It will complain about
> > them, but it will not fail to register the device.
> > 
> > Cc: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > Cc: Rob Herring <rob.herring@calxeda.com>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
> > ---
> > 
> > Greg, do you mind taking a look at this? The reason the OF code hasn't been
> > calling platform_device_add() directly to this point is:
> > a) there are some trees with resource overlays
> > b) I want the devices in /sys/devices not /sys/devices/platform.
> 
> Putting the devices all in the "flat" location of /sys/devices/ is a bit
> worrisome to me.  What's wrong with platform/ ?  That is what they are,
> right?  Why change this?

Hahaha. *You* encouraged me to write the patch to remove
/sys/devices/platform/ when I was waffling over whether or not it was a
good idea. Granted, that was well over a year ago, but it takes me a
while to get around to some of the things on my todo list. :-)

It's not so much that there is anything wrong with platform/ other than
it is nonsensical. For example, a core system bus is often represented by an
platform device of it's own with a bunch of peripherals as children of
that. For example a PCI host controller. It doesn't make much sense to me for
some core devices to be at /sys/devices and others to be gathered
together under /sys/devices/platform.

However, all that mildly feels 'wrong' to me but isn't that big deal. A
bigger problem with b) (which I didn't describe well) is that existing
PowerPC support roots the platform devices hierarchy at /sys/devices, not
/sys/devices/platform and I'm nervous that changing it will break
things. If I commit the change that makes the move, and somebody
complains that I broke their userspace, then I need to have an exception
for those system or revert the patch entirely.

Regardless, I'm no longer happy with DT and non-DT platform device
registration having separate code paths. I would /like/ for
sys/device/platform to disappear, but that is merely a side issue.
The real issue is whether or not existing PowerPC userspace breaks. If
it does, there needs to be an exception to keep things under
/sys/devices.

> > I could easily add exceptions to platform_device_add() for both those cases, but
> > I don't like adding DT exceptions to the common code. However, I still need to
> > support the platforms that unfortunately have overlapping resources. This patch
> > does that by still calling the old path if platform_device_add() fails, but it
> > isn't nice either because of_device_add() has to duplicate
> > platform_device_add(). Blech. Plus the exception only applies for PowerPC.
> > 
> > So, how do you feel about having a 'relaxed' mode for platform_device_add()
> > which means it won't fail if resources overlap and maybe won't do the silly
> > platform_bus parent thing. Thoughts?
> 
> I have no objection for the resource issue, if you assure me it will not
> be abused :)

I can make that assurance. It will be powerpc-only also.

g.