of/platform: Fix no irq domain found errors when populating interrupts

PATCH In review Tony Lindgren 4 participants 8 messages Apr 10, 2014 – Apr 18, 2014

Subsystems (2)

Driver core Device tree

lore

Share & tools

lore

Tony Lindgren

author

2014-04-10 21:38 UTC #0 lore raw

PATCH 1/1 of/platform: Fix no irq domain found errors when populating interrupts

Currently we get the following kind of errors if we try to use interrupt phandles to irqchips that have not yet initialized:

irq: no irq domain found for /ocp/pinmux@48002030 !

------------[ cut here ]------------
WARNING: CPU: 0 PID: 1 at drivers/of/platform.c:171 of_device_alloc+0x144/0x184()
Modules linked in:
CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.12.0-00038-g42a9708 #1012
(show_stack+0x14/0x1c)
(dump_stack+0x6c/0xa0)
(warn_slowpath_common+0x64/0x84)
(warn_slowpath_null+0x1c/0x24)
(of_device_alloc+0x144/0x184)
(of_platform_device_create_pdata+0x44/0x9c)
(of_platform_bus_create+0xd0/0x170)
(of_platform_bus_create+0x12c/0x170)
(of_platform_populate+0x60/0x98)

This is because we're wrongly trying to populate resources that are not yet available. It's perfectly valid to create irqchips dynamically, so let's fix up the issue by populating the interrupt resources at the driver probe time instead.

Note that at least currently we cannot dynamically allocate the resources as bus specific code may add legacy resources with platform_device_add_resources() before the driver probe. At least omap_device_alloc() currently relies on num_resources to determine if legacy resources should be added. Some of these will clear automatically when mach-omap2 boots with DT only, but there are probably other places too where platform_device_add_resources() modifies things before driver probe.

This patch was discussed quite a bit earlier, but so far it seems we don't have any better options to fix the problem. For the earlier discussion, please see:

https://lkml.org/lkml/2013/11/22/520

The addition of of_platform_probe() is based on patches posted earlier by Thierry Reding <[email protected]>.

Signed-off-by Tony Lindgren <[email protected]>

---

Greg, this is still broken, can you please pick this up?

drivers/base/platform.c +4 -0

@@ -483,6 +483,10 @@ static int platform_drv_probe(struct device *_dev)
 
 	acpi_dev_pm_attach(_dev, true);
 
+	ret = of_platform_probe(dev);
+	if (ret)
+		return ret;
+
 	ret = drv->probe(dev);
 	if (ret)
 		acpi_dev_pm_detach(_dev, true);

drivers/of/platform.c +85 -7

120 lines not on this page · raw message

include/linux/of_platform.h +7 -0

@@ -72,6 +72,8 @@ extern int of_platform_populate(struct device_node *root,
 				const struct of_device_id *matches,
 				const struct of_dev_auxdata *lookup,
 				struct device *parent);
+
+extern int of_platform_probe(struct platform_device *pdev);
 #else
 static inline int of_platform_populate(struct device_node *root,
 					const struct of_device_id *matches,

@@ -80,6 +82,11 @@ static inline int of_platform_populate(struct device_node *root,
 {
 	return -ENODEV;
 }
+
+static inline int of_platform_probe(struct platform_device *pdev)
+{
+	return 0;
+}
 #endif
 
 #endif	/* _LINUX_OF_PLATFORM_H */

Rob Herring

2014-04-11 00:29 UTC #1 lore raw

Quoted 31 lines · Tony Lindgren

Currently we get the following kind of errors if we try to use interrupt phandles to irqchips that have not yet initialized:
irq: no irq domain found for /ocp/pinmux@48002030 !
------------[ cut here ]------------
WARNING: CPU: 0 PID: 1 at drivers/of/platform.c:171 of_device_alloc+0x144/0x184()
Modules linked in:
CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.12.0-00038-g42a9708 #1012
(show_stack+0x14/0x1c)
(dump_stack+0x6c/0xa0)
(warn_slowpath_common+0x64/0x84)
(warn_slowpath_null+0x1c/0x24)
(of_device_alloc+0x144/0x184)
(of_platform_device_create_pdata+0x44/0x9c)
(of_platform_bus_create+0xd0/0x170)
(of_platform_bus_create+0x12c/0x170)
(of_platform_populate+0x60/0x98)
This is because we're wrongly trying to populate resources that are not yet available. It's perfectly valid to create irqchips dynamically, so let's fix up the issue by populating the interrupt resources at the driver probe time instead.
Note that at least currently we cannot dynamically allocate the resources as bus specific code may add legacy resources with platform_device_add_resources() before the driver probe. At least omap_device_alloc() currently relies on num_resources to determine if legacy resources should be added. Some of these will clear automatically when mach-omap2 boots with DT only, but there are probably other places too where platform_device_add_resources() modifies things before driver probe.

Quoted 5 lines

This patch was discussed quite a bit earlier, but so far it seems we don't have any better options to fix the problem. For the earlier discussion, please see:
https://lkml.org/lkml/2013/11/22/520

There is a newer solution here which Grant seemed happier with:

http://lkml.iu.edu/hypermail/linux/kernel/1403.2/03666.html

Does it work for you?

Quoted 2 lines

The addition of of_platform_probe() is based on patches posted earlier by Thierry Reding <[email protected]>.

I fail to see how this patch does EPROBE_DEFER and solves the problem. Don't you need Thierry's other patches for that? I'm probably missing something.

Rob

Thierry Reding

2014-04-11 18:43 UTC #2 lore raw

Quoted 36 lines · Rob Herring

Quoted 35 lines · Tony Lindgren
Currently we get the following kind of errors if we try to use interrupt phandles to irqchips that have not yet initialized:
irq: no irq domain found for /ocp/pinmux@48002030 !
------------[ cut here ]------------
WARNING: CPU: 0 PID: 1 at drivers/of/platform.c:171 of_device_alloc+0x144/0x184()
Modules linked in:
CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.12.0-00038-g42a9708 #1012
(show_stack+0x14/0x1c)
(dump_stack+0x6c/0xa0)
(warn_slowpath_common+0x64/0x84)
(warn_slowpath_null+0x1c/0x24)
(of_device_alloc+0x144/0x184)
(of_platform_device_create_pdata+0x44/0x9c)
(of_platform_bus_create+0xd0/0x170)
(of_platform_bus_create+0x12c/0x170)
(of_platform_populate+0x60/0x98)
This is because we're wrongly trying to populate resources that are not yet available. It's perfectly valid to create irqchips dynamically, so let's fix up the issue by populating the interrupt resources at the driver probe time instead.
Note that at least currently we cannot dynamically allocate the resources as bus specific code may add legacy resources with platform_device_add_resources() before the driver probe. At least omap_device_alloc() currently relies on num_resources to determine if legacy resources should be added. Some of these will clear automatically when mach-omap2 boots with DT only, but there are probably other places too where platform_device_add_resources() modifies things before driver probe.
This patch was discussed quite a bit earlier, but so far it seems we don't have any better options to fix the problem. For the earlier discussion, please see:

Quoted 5 lines

Quoted 1 line
https://lkml.org/lkml/2013/11/22/520
There is a newer solution here which Grant seemed happier with:
http://lkml.iu.edu/hypermail/linux/kernel/1403.2/03666.html

I wonder why Grant seems to be happier with that solution than with my original proposal. That new solution does essentially the same thing. One of the main issues raised during review of my original proposal was that it had to modify the core, but this new solution does that as well.

Another thing that people weren't happy about was that my solution was more intrusive because it required a bunch of changes to the of_irq_*() helpers to make them propagate a proper error code. The new solution doesn't do that, but instead works around the lack of proper error propagation by trying to find an IRQ domain (which the of_irq_*() helpers will do anyway).

Thierry

Tony Lindgren

author

2014-04-18 20:55 UTC #3 lore raw

Quoted 48 lines · Thierry Reding

Quoted 36 lines · Rob Herring
Quoted 35 lines · Tony Lindgren
Currently we get the following kind of errors if we try to use interrupt phandles to irqchips that have not yet initialized:
irq: no irq domain found for /ocp/pinmux@48002030 !
------------[ cut here ]------------
WARNING: CPU: 0 PID: 1 at drivers/of/platform.c:171 of_device_alloc+0x144/0x184()
Modules linked in:
CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.12.0-00038-g42a9708 #1012
(show_stack+0x14/0x1c)
(dump_stack+0x6c/0xa0)
(warn_slowpath_common+0x64/0x84)
(warn_slowpath_null+0x1c/0x24)
(of_device_alloc+0x144/0x184)
(of_platform_device_create_pdata+0x44/0x9c)
(of_platform_bus_create+0xd0/0x170)
(of_platform_bus_create+0x12c/0x170)
(of_platform_populate+0x60/0x98)
This is because we're wrongly trying to populate resources that are not yet available. It's perfectly valid to create irqchips dynamically, so let's fix up the issue by populating the interrupt resources at the driver probe time instead.
Note that at least currently we cannot dynamically allocate the resources as bus specific code may add legacy resources with platform_device_add_resources() before the driver probe. At least omap_device_alloc() currently relies on num_resources to determine if legacy resources should be added. Some of these will clear automatically when mach-omap2 boots with DT only, but there are probably other places too where platform_device_add_resources() modifies things before driver probe.
This patch was discussed quite a bit earlier, but so far it seems we don't have any better options to fix the problem. For the earlier discussion, please see:
Quoted 5 lines
Quoted 1 line
https://lkml.org/lkml/2013/11/22/520
There is a newer solution here which Grant seemed happier with:
http://lkml.iu.edu/hypermail/linux/kernel/1403.2/03666.html
I wonder why Grant seems to be happier with that solution than with my original proposal. That new solution does essentially the same thing. One of the main issues raised during review of my original proposal was that it had to modify the core, but this new solution does that as well.

Quoted 6 lines

Another thing that people weren't happy about was that my solution was more intrusive because it required a bunch of changes to the of_irq_*() helpers to make them propagate a proper error code. The new solution doesn't do that, but instead works around the lack of proper error propagation by trying to find an IRQ domain (which the of_irq_*() helpers will do anyway).

Yeah the problem I see with the patches above is that there's nothing wrong with the irq subsystem. Replied along those lines to the thread above.

Cheers,

Tony

Russell King - ARM Linux

2014-04-11 09:26 UTC #4 lore raw

In reply to Tony Lindgren

Quoted 35 lines · Tony Lindgren

Currently we get the following kind of errors if we try to use interrupt phandles to irqchips that have not yet initialized:
irq: no irq domain found for /ocp/pinmux@48002030 !
------------[ cut here ]------------
WARNING: CPU: 0 PID: 1 at drivers/of/platform.c:171 of_device_alloc+0x144/0x184()
Modules linked in:
CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.12.0-00038-g42a9708 #1012
(show_stack+0x14/0x1c)
(dump_stack+0x6c/0xa0)
(warn_slowpath_common+0x64/0x84)
(warn_slowpath_null+0x1c/0x24)
(of_device_alloc+0x144/0x184)
(of_platform_device_create_pdata+0x44/0x9c)
(of_platform_bus_create+0xd0/0x170)
(of_platform_bus_create+0x12c/0x170)
(of_platform_populate+0x60/0x98)
This is because we're wrongly trying to populate resources that are not yet available. It's perfectly valid to create irqchips dynamically, so let's fix up the issue by populating the interrupt resources at the driver probe time instead.
Note that at least currently we cannot dynamically allocate the resources as bus specific code may add legacy resources with platform_device_add_resources() before the driver probe. At least omap_device_alloc() currently relies on num_resources to determine if legacy resources should be added. Some of these will clear automatically when mach-omap2 boots with DT only, but there are probably other places too where platform_device_add_resources() modifies things before driver probe.
This patch was discussed quite a bit earlier, but so far it seems we don't have any better options to fix the problem. For the earlier discussion, please see:

Quoted 6 lines

https://lkml.org/lkml/2013/11/22/520
The addition of of_platform_probe() is based on patches posted earlier by Thierry Reding <[email protected]>.
Signed-off-by Tony Lindgren <[email protected]>

So what happens if a device driver probe function:

- creates a new platform device
- copies the resources from the original to the new device
- copies the of_node from the original to the new device
- registers the new device

Yes, it's broken (because it can result in the same driver being re-probed by the new device) but we *do* have stuff in the kernel tree which does this.

Signature · 2 lines

FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

Thierry Reding

2014-04-11 18:36 UTC #5 lore raw

Quoted 45 lines · Russell King - ARM Linux

Quoted 35 lines · Tony Lindgren
Currently we get the following kind of errors if we try to use interrupt phandles to irqchips that have not yet initialized:
irq: no irq domain found for /ocp/pinmux@48002030 !
------------[ cut here ]------------
WARNING: CPU: 0 PID: 1 at drivers/of/platform.c:171 of_device_alloc+0x144/0x184()
Modules linked in:
CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.12.0-00038-g42a9708 #1012
(show_stack+0x14/0x1c)
(dump_stack+0x6c/0xa0)
(warn_slowpath_common+0x64/0x84)
(warn_slowpath_null+0x1c/0x24)
(of_device_alloc+0x144/0x184)
(of_platform_device_create_pdata+0x44/0x9c)
(of_platform_bus_create+0xd0/0x170)
(of_platform_bus_create+0x12c/0x170)
(of_platform_populate+0x60/0x98)
This is because we're wrongly trying to populate resources that are not yet available. It's perfectly valid to create irqchips dynamically, so let's fix up the issue by populating the interrupt resources at the driver probe time instead.
Note that at least currently we cannot dynamically allocate the resources as bus specific code may add legacy resources with platform_device_add_resources() before the driver probe. At least omap_device_alloc() currently relies on num_resources to determine if legacy resources should be added. Some of these will clear automatically when mach-omap2 boots with DT only, but there are probably other places too where platform_device_add_resources() modifies things before driver probe.
This patch was discussed quite a bit earlier, but so far it seems we don't have any better options to fix the problem. For the earlier discussion, please see:
Quoted 6 lines
https://lkml.org/lkml/2013/11/22/520
The addition of of_platform_probe() is based on patches posted earlier by Thierry Reding <[email protected]>.
Signed-off-by Tony Lindgren <[email protected]>
So what happens if a device driver probe function:

Quoted 8 lines

- creates a new platform device
- copies the resources from the original to the new device
- copies the of_node from the original to the new device
- registers the new device
Yes, it's broken (because it can result in the same driver being re-probed by the new device) but we *do* have stuff in the kernel tree which does this.

From what I can tell the only clean solution would be to allow the OF functions to properly propagate errors. My earlier attempt was exactly that, but was deemed too invasive.

But that doesn't really solve the case you describe above either. So I think the only good generic solution would be for all resources to be resolved by the driver's .probe() function so that resources aren't "cached" in the device node.

Thierry

Tony Lindgren

author

2014-04-18 21:38 UTC #6 lore raw

Quoted 46 lines · Thierry Reding

Quoted 45 lines · Russell King - ARM Linux
Quoted 35 lines · Tony Lindgren
Currently we get the following kind of errors if we try to use interrupt phandles to irqchips that have not yet initialized:
irq: no irq domain found for /ocp/pinmux@48002030 !
------------[ cut here ]------------
WARNING: CPU: 0 PID: 1 at drivers/of/platform.c:171 of_device_alloc+0x144/0x184()
Modules linked in:
CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.12.0-00038-g42a9708 #1012
(show_stack+0x14/0x1c)
(dump_stack+0x6c/0xa0)
(warn_slowpath_common+0x64/0x84)
(warn_slowpath_null+0x1c/0x24)
(of_device_alloc+0x144/0x184)
(of_platform_device_create_pdata+0x44/0x9c)
(of_platform_bus_create+0xd0/0x170)
(of_platform_bus_create+0x12c/0x170)
(of_platform_populate+0x60/0x98)
This is because we're wrongly trying to populate resources that are not yet available. It's perfectly valid to create irqchips dynamically, so let's fix up the issue by populating the interrupt resources at the driver probe time instead.
Note that at least currently we cannot dynamically allocate the resources as bus specific code may add legacy resources with platform_device_add_resources() before the driver probe. At least omap_device_alloc() currently relies on num_resources to determine if legacy resources should be added. Some of these will clear automatically when mach-omap2 boots with DT only, but there are probably other places too where platform_device_add_resources() modifies things before driver probe.
This patch was discussed quite a bit earlier, but so far it seems we don't have any better options to fix the problem. For the earlier discussion, please see:
Quoted 6 lines
https://lkml.org/lkml/2013/11/22/520
The addition of of_platform_probe() is based on patches posted earlier by Thierry Reding <[email protected]>.
Signed-off-by Tony Lindgren <[email protected]>
So what happens if a device driver probe function:

Quoted 8 lines

Quoted 8 lines
- creates a new platform device
- copies the resources from the original to the new device
- copies the of_node from the original to the new device
- registers the new device
Yes, it's broken (because it can result in the same driver being re-probed by the new device) but we *do* have stuff in the kernel tree which does this.

Grr. Care to list some examples? See also if what I'm suggesting below if that might work for the cases you're describing.

Quoted 3 lines

From what I can tell the only clean solution would be to allow the OF functions to properly propagate errors. My earlier attempt was exactly that, but was deemed too invasive.

Frankly, I think sprinkling new of_* functions all over the subsystems is going to be a never ending task to try to fix this and other similar issues. For a long term solution it makes sense to not probe the driver at all until all it's resources are in place.

Quoted 4 lines

But that doesn't really solve the case you describe above either. So I think the only good generic solution would be for all resources to be resolved by the driver's .probe() function so that resources aren't "cached" in the device node.

For the other resources than interrupts I think what Russell describes can be worked around by keeping the initial populating of the resources except for the interrupts. Then we can just overwrite the resources in of_device_resource_populate().

No idea which drivers Russell refers to above on the copied devices. But presumably the copied device does not need interrupts without a driver probe?

Regards,

Tony

8< ----------------------
From: Tony Lindgren <[email protected]>
Date: Fri, 11 Apr 2014 07:52:00 -0700
Subject: [PATCH] of/platform: Fix no irq domain found errors when populating interrupts

Currently we get the following kind of errors if we try to use interrupt phandles to irqchips that have not yet initialized:

irq: no irq domain found for /ocp/pinmux@48002030 !

------------[ cut here ]------------
WARNING: CPU: 0 PID: 1 at drivers/of/platform.c:171 of_device_alloc+0x144/0x184()
Modules linked in:
CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.12.0-00038-g42a9708 #1012
(show_stack+0x14/0x1c)
(dump_stack+0x6c/0xa0)
(warn_slowpath_common+0x64/0x84)
(warn_slowpath_null+0x1c/0x24)
(of_device_alloc+0x144/0x184)
(of_platform_device_create_pdata+0x44/0x9c)
(of_platform_bus_create+0xd0/0x170)
(of_platform_bus_create+0x12c/0x170)
(of_platform_populate+0x60/0x98)

This patch was discussed quite a bit earlier, but so far it seems we don't have any better options to fix the problem. For the earlier discussion, please see:

https://lkml.org/lkml/2013/11/22/520

The addition of of_platform_probe() is based on patches posted earlier by Thierry Reding <[email protected]>.

Signed-off-by Tony Lindgren <[email protected]>

drivers/base/platform.c +4 -0

@@ -484,6 +484,10 @@ static int platform_drv_probe(struct device *_dev)
 	if (ACPI_HANDLE(_dev))
 		acpi_dev_pm_attach(_dev, true);
 
+	ret = of_platform_probe(dev);
+	if (ret)
+		return ret;
+
 	ret = drv->probe(dev);
 	if (ret && ACPI_HANDLE(_dev))
 		acpi_dev_pm_detach(_dev, true);

drivers/of/platform.c +77 -2

101 lines not on this page · raw message

include/linux/of_platform.h +7 -0

@@ -72,6 +72,8 @@ extern int of_platform_populate(struct device_node *root,
 				const struct of_device_id *matches,
 				const struct of_dev_auxdata *lookup,
 				struct device *parent);
+
+extern int of_platform_probe(struct platform_device *pdev);
 #else
 static inline int of_platform_populate(struct device_node *root,
 					const struct of_device_id *matches,

@@ -80,6 +82,11 @@ static inline int of_platform_populate(struct device_node *root,
 {
 	return -ENODEV;
 }
+
+static inline int of_platform_probe(struct platform_device *pdev)
+{
+	return 0;
+}
 #endif
 
 #endif	/* _LINUX_OF_PLATFORM_H */

Russell King - ARM Linux

2014-04-18 23:19 UTC #7 lore raw

Quoted 9 lines · Tony Lindgren

Quoted 8 lines · Thierry Reding
Quoted 7 lines · Russell King - ARM Linux
So what happens if a device driver probe function:
- creates a new platform device
- copies the resources from the original to the new device
- copies the of_node from the original to the new device
- registers the new device

Quoted 6 lines

Quoted 3 lines
Quoted 3 lines
Yes, it's broken (because it can result in the same driver being re-probed by the new device) but we *do* have stuff in the kernel tree which does this.
Grr. Care to list some examples? See also if what I'm suggesting below if that might work for the cases you're describing.

Until recently, ahci_imx did exactly this, but that's been fixed now (by reworking ahci into a library.) I'm not immediately aware of any other drivers pulling this trick.

Signature · 2 lines

FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.