[PATCH] driver-core: platform: Resolve DT interrupt references late
Thierry Reding
thierry.reding at gmail.com
Wed Jan 8 14:28:32 EST 2014
On Wed, Jan 08, 2014 at 08:40:41AM -0800, Tony Lindgren wrote:
> * Thierry Reding <thierry.reding at gmail.com> [140108 04:55]:
> > When devices are probed from the device tree, any interrupts that they
> > reference are resolved at device creation time. This causes problems if
> > the interrupt provider hasn't been registered yet at that time, which
> > results in the interrupt being set to 0.
> >
> > This is especially bad for platform devices because they are created at
> > a very early stage during boot when the majority of interrupt providers
> > haven't had a chance to be probed yet. One of the platform where this
> > causes major issues is OMAP.
> >
> > Note that this patch is the easy way out to fix a large part of the
> > problems for now. A more proper solution for the long term would be to
> > transition drivers to an API that always resolves resources of any kind
> > (not only interrupts) at probe time.
> >
> > For some background and discussion on possible solutions, see:
> >
> > https://lkml.org/lkml/2013/11/22/520
> >
> > Acked-by: Rob Herring <robherring2 at gmail.com>
> > Signed-off-by: Thierry Reding <treding at nvidia.com>
> > ---
> > Note that this is somewhat urgent and should if at all possible go into
> > v3.13 before the release.
> >
> > drivers/base/platform.c | 8 +++++++-
> > 1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> > index 3a94b799f166..c894d1af3a5e 100644
> > --- a/drivers/base/platform.c
> > +++ b/drivers/base/platform.c
> > @@ -13,6 +13,7 @@
> > #include <linux/string.h>
> > #include <linux/platform_device.h>
> > #include <linux/of_device.h>
> > +#include <linux/of_irq.h>
> > #include <linux/module.h>
> > #include <linux/init.h>
> > #include <linux/dma-mapping.h>
> > @@ -87,7 +88,12 @@ int platform_get_irq(struct platform_device *dev, unsigned int num)
> > return -ENXIO;
> > return dev->archdata.irqs[num];
> > #else
> > - struct resource *r = platform_get_resource(dev, IORESOURCE_IRQ, num);
> > + struct resource *r;
> > +
> > + if (IS_ENABLED(CONFIG_OF) && dev->dev.of_node)
> > + return irq_of_parse_and_map(dev->dev.of_node, num);
> > +
> > + r = platform_get_resource(dev, IORESOURCE_IRQ, num);
> >
> > return r ? r->start : -ENXIO;
> > #endif
>
> Hmm actually testing this patch, it does not fix fix the $Subject bug :(
>
> irq: no irq domain found for /ocp/pinmux at 48002030 !
> [ 0.301361] ------------[ cut here ]------------
> [ 0.301391] WARNING: CPU: 0 PID: 1 at drivers/of/platform.c:171 of_device_alloc+0x144/0x184()
> [ 0.301422] Modules linked in:
> [ 0.301452] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.13.0-rc7-00002-g4d998a6 #17
> [ 0.301513] [<c0015c04>] (unwind_backtrace+0x0/0xf4) from [<c00127b0>] (show_stack+0x14/0x1c)
> [ 0.301544] [<c00127b0>] (show_stack+0x14/0x1c) from [<c05685a4>] (dump_stack+0x6c/0xa0)
> [ 0.301574] [<c05685a4>] (dump_stack+0x6c/0xa0) from [<c00425b4>] (warn_slowpath_common+0x64/0x84)
> [ 0.301605] [<c00425b4>] (warn_slowpath_common+0x64/0x84) from [<c00425f0>] (warn_slowpath_null+0x1c/0x24)
> [ 0.301635] [<c00425f0>] (warn_slowpath_null+0x1c/0x24) from [<c0485210>] (of_device_alloc+0x144/0x184)
> [ 0.301635] [<c0485210>] (of_device_alloc+0x144/0x184) from [<c0485294>] (of_platform_device_create_pdata+0x44/0x9c)
> [ 0.301666] [<c0485294>] (of_platform_device_create_pdata+0x44/0x9c) from [<c04853bc>] (of_platform_bus_create+0xd0/0x170)
> [ 0.301696] [<c04853bc>] (of_platform_bus_create+0xd0/0x170) from [<c0485418>] (of_platform_bus_create+0x12c/0x170)
> [ 0.301727] [<c0485418>] (of_platform_bus_create+0x12c/0x170) from [<c04854bc>] (of_platform_populate+0x60/0x98)
> [ 0.301757] [<c04854bc>] (of_platform_populate+0x60/0x98) from [<c07ca53c>] (pdata_quirks_init+0x28/0x78)
> [ 0.301788] [<c07ca53c>] (pdata_quirks_init+0x28/0x78) from [<c07bab20>] (customize_machine+0x20/0x48)
> [ 0.301818] [<c07bab20>] (customize_machine+0x20/0x48) from [<c000882c>] (do_one_initcall+0x2c/0x150)
> [ 0.301849] [<c000882c>] (do_one_initcall+0x2c/0x150) from [<c07b75d8>] (do_basic_setup+0x94/0xd4)
> [ 0.301879] [<c07b75d8>] (do_basic_setup+0x94/0xd4) from [<c07b7694>] (kernel_init_freeable+0x7c/0x120)
> [ 0.301910] [<c07b7694>] (kernel_init_freeable+0x7c/0x120) from [<c05667ec>] (kernel_init+0x8/0x120)
> [ 0.301940] [<c05667ec>] (kernel_init+0x8/0x120) from [<c000e908>] (ret_from_fork+0x14/0x2c)
> [ 0.302124] ---[ end trace 2b87f5de2f86f809 ]---
> ...
>
> There's nothing wrong with the interrupt related code paths, we're just
> trying to call the functions at a wrong time when thing are not yet
> initialized.
The patch won't get rid of that warning, but it should at least restore
things to a working state at runtime. At least for well-behaved drivers
that use platform_get_irq() rather than those that try to access the
resources directly.
> Below is a repost of what works for me without using notifiers. Anybody
> got any better ideas for a minimal fix?
That patch is somewhat big for something that should be a minimal fix.
Being the size that it is it might have undesired side-effects that may
not get noticed until it's way too late, so I'm hesitant to have
something like this merged at this point in the release cycle.
Thierry
> 8< -------------------------------
> From: Tony Lindgren <tony at atomide.com>
> Date: Tue, 7 Jan 2014 17:07:18 -0800
> 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 at 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. This issue
> will get fixed 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.
>
> The addition of of_platform_probe() is based on patches posted earlier by
> Thierry Reding <thierry.reding at gmail.com>.
>
> Signed-off-by: Tony Lindgren <tony at atomide.com>
>
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -481,6 +481,10 @@ static int platform_drv_probe(struct device *_dev)
> struct platform_device *dev = to_platform_device(_dev);
> int ret;
>
> + ret = of_platform_probe(dev);
> + if (ret)
> + return ret;
> +
> if (ACPI_HANDLE(_dev))
> acpi_dev_pm_attach(_dev, true);
>
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -141,7 +141,7 @@ struct platform_device *of_device_alloc(struct device_node *np,
> struct device *parent)
> {
> struct platform_device *dev;
> - int rc, i, num_reg = 0, num_irq;
> + int num_reg = 0, num_irq;
> struct resource *res, temp_res;
>
> dev = platform_device_alloc("", -1);
> @@ -154,7 +154,14 @@ struct platform_device *of_device_alloc(struct device_node *np,
> num_reg++;
> num_irq = of_irq_count(np);
>
> - /* Populate the resource table */
> + /*
> + * Only allocate the resources for us to use later on. Note that bus
> + * specific code may also add in additional legacy resources using
> + * platform_device_add_resources(), and may even rely on us allocating
> + * the basic resources here to do so. So we cannot allocate the
> + * resources lazily until the legacy code has been fixed to not rely
> + * on allocating resources here.
> + */
> if (num_irq || num_reg) {
> res = kzalloc(sizeof(*res) * (num_irq + num_reg), GFP_KERNEL);
> if (!res) {
> @@ -164,11 +171,7 @@ struct platform_device *of_device_alloc(struct device_node *np,
>
> dev->num_resources = num_reg + num_irq;
> dev->resource = res;
> - for (i = 0; i < num_reg; i++, res++) {
> - rc = of_address_to_resource(np, i, res);
> - WARN_ON(rc);
> - }
> - WARN_ON(of_irq_to_resource_table(np, res, num_irq) != num_irq);
> + /* See of_device_resource_populate for populating the data */
> }
>
> dev->dev.of_node = of_node_get(np);
> @@ -187,6 +190,50 @@ struct platform_device *of_device_alloc(struct device_node *np,
> EXPORT_SYMBOL(of_device_alloc);
>
> /**
> + * of_device_resource_populate - Populate device resources from device tree
> + * @dev: pointer to platform device
> + *
> + * The device interrupts are not necessarily available for all
> + * irqdomains initially so we need to populate them lazily at
> + * device probe time from of_platform_populate.
> + */
> +static int of_device_resource_populate(struct platform_device *pdev)
> +{
> + struct device_node *np = pdev->dev.of_node;
> + int rc, i, num_reg = 0, num_irq;
> + struct resource *res, temp_res;
> +
> + res = pdev->resource;
> +
> + /*
> + * Count the io and irq resources again. Currently we cannot rely on
> + * pdev->num_resources as bus specific code may have changed that
> + * with platform_device_add_resources(). But the resources we allocated
> + * earlier are still there and available for us to populate.
> + */
> + if (of_can_translate_address(np))
> + while (of_address_to_resource(np, num_reg, &temp_res) == 0)
> + num_reg++;
> + num_irq = of_irq_count(np);
> +
> + if (pdev->num_resources < num_reg + num_irq) {
> + dev_WARN(&pdev->dev, "not enough resources %i < %i\n",
> + pdev->num_resources, num_reg + num_irq);
> + return -EINVAL;
> + }
> +
> + for (i = 0; i < num_reg; i++, res++) {
> + rc = of_address_to_resource(np, i, res);
> + WARN_ON(rc);
> + }
> +
> + if (num_irq)
> + WARN_ON(of_irq_to_resource_table(np, res, num_irq) != num_irq);
> +
> + return 0;
> +}
> +
> +/**
> * of_platform_device_create_pdata - Alloc, initialize and register an of_device
> * @np: pointer to node to create device for
> * @bus_id: name to assign device
> @@ -485,4 +532,35 @@ int of_platform_populate(struct device_node *root,
> return rc;
> }
> EXPORT_SYMBOL_GPL(of_platform_populate);
> +
> +/**
> + * of_platform_probe() - OF specific initialization at probe time
> + * @pdev: pointer to a platform device
> + *
> + * This function is called by the driver core to perform devicetree-specific
> + * setup for a given platform device at probe time. If a device's resources
> + * as specified in the device tree are not available yet, this function can
> + * return -EPROBE_DEFER and cause the device to be probed again later, when
> + * other drivers that potentially provide the missing resources have been
> + * probed in turn.
> + *
> + * Note that because of the above, all code executed by this function must
> + * be prepared to be run multiple times on the same device (i.e. it must be
> + * idempotent).
> + *
> + * Returns 0 on success or a negative error code on failure.
> + */
> +int of_platform_probe(struct platform_device *pdev)
> +{
> + int ret;
> +
> + if (!pdev->dev.of_node)
> + return 0;
> +
> + ret = of_device_resource_populate(pdev);
> + if (ret < 0)
> + return ret;
> +
> + return 0;
> +}
> #endif /* CONFIG_OF_ADDRESS */
> --- a/include/linux/of_platform.h
> +++ b/include/linux/of_platform.h
> @@ -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 */
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140108/37d11f14/attachment-0001.sig>
More information about the linux-arm-kernel
mailing list