of_device_alloc() in early boot stage creates a interrupt mapping if
there exists a "interrupts" property in the node.
For hierarchical interrupt domains using "interrupts" property in the node
bypassed the hierarchical setup and messed up the irq setup.
This patch adds a check in of_device_alloc() to skip interrupt mapping if
"not-interrupt-producer" property is present in the node. This allows
nodes to describe the interrupts using "interrupts" property.
Signed-off-by: Lad Prabhakar <[email protected]>
---
Hi All,
Spawning from discussion [1], here is simple patch (not the ideal probably
welcome for suggestions) from stopping the OF code from creating a map for
the interrupts when using "interrupts" property.
[1] https://lore.kernel.org/lkml/[email protected]/
T/#mbd1e47c1981082aded4b32a52e2c04291e515508
Cheers,
Prabhakar
---
drivers/of/platform.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index b3faf89744aa..629776ca1721 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -114,7 +114,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 rc, i, num_reg = 0, num_irq = 0;
struct resource *res, temp_res;
dev = platform_device_alloc("", PLATFORM_DEVID_NONE);
@@ -124,7 +124,14 @@ struct platform_device *of_device_alloc(struct device_node *np,
/* count the io and irq resources */
while (of_address_to_resource(np, num_reg, &temp_res) == 0)
num_reg++;
- num_irq = of_irq_count(np);
+
+ /*
+ * we don't want to map the interrupts of hierarchical interrupt domain
+ * into the parent domain yet. This will be the job of the hierarchical
+ * interrupt driver code to map the interrupts as and when needed.
+ */
+ if (!of_property_read_bool(np, "not-interrupt-producer"))
+ num_irq = of_irq_count(np);
/* Populate the resource table */
if (num_irq || num_reg) {
@@ -140,7 +147,7 @@ struct platform_device *of_device_alloc(struct device_node *np,
rc = of_address_to_resource(np, i, res);
WARN_ON(rc);
}
- if (of_irq_to_resource_table(np, res, num_irq) != num_irq)
+ if (num_irq && of_irq_to_resource_table(np, res, num_irq) != num_irq)
pr_debug("not all legacy IRQ resources mapped for %pOFn\n",
np);
}
--
2.17.1
On Wed, Dec 8, 2021 at 6:11 PM Lad Prabhakar
<[email protected]> wrote:
>
> of_device_alloc() in early boot stage creates a interrupt mapping if
> there exists a "interrupts" property in the node.
>
> For hierarchical interrupt domains using "interrupts" property in the node
> bypassed the hierarchical setup and messed up the irq setup.
>
> This patch adds a check in of_device_alloc() to skip interrupt mapping if
> "not-interrupt-producer" property is present in the node. This allows
> nodes to describe the interrupts using "interrupts" property.
>
> Signed-off-by: Lad Prabhakar <[email protected]>
> ---
> Hi All,
>
> Spawning from discussion [1], here is simple patch (not the ideal probably
> welcome for suggestions) from stopping the OF code from creating a map for
> the interrupts when using "interrupts" property.
>
> [1] https://lore.kernel.org/lkml/[email protected]/
> T/#mbd1e47c1981082aded4b32a52e2c04291e515508
>
> Cheers,
> Prabhakar
> ---
> drivers/of/platform.c | 13 ++++++++++---
> 1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index b3faf89744aa..629776ca1721 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -114,7 +114,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 rc, i, num_reg = 0, num_irq = 0;
> struct resource *res, temp_res;
>
> dev = platform_device_alloc("", PLATFORM_DEVID_NONE);
> @@ -124,7 +124,14 @@ struct platform_device *of_device_alloc(struct device_node *np,
> /* count the io and irq resources */
> while (of_address_to_resource(np, num_reg, &temp_res) == 0)
> num_reg++;
> - num_irq = of_irq_count(np);
> +
> + /*
> + * we don't want to map the interrupts of hierarchical interrupt domain
> + * into the parent domain yet. This will be the job of the hierarchical
> + * interrupt driver code to map the interrupts as and when needed.
> + */
> + if (!of_property_read_bool(np, "not-interrupt-producer"))
> + num_irq = of_irq_count(np);
The property won't fly for sure. A compatible match table could work
here, but I don't really want another temporary solution.
> /* Populate the resource table */
> if (num_irq || num_reg) {
> @@ -140,7 +147,7 @@ struct platform_device *of_device_alloc(struct device_node *np,
> rc = of_address_to_resource(np, i, res);
> WARN_ON(rc);
> }
> - if (of_irq_to_resource_table(np, res, num_irq) != num_irq)
> + if (num_irq && of_irq_to_resource_table(np, res, num_irq) != num_irq)
You might want to look at commit 9ec36cafe43b ("of/irq: do irq
resolution in platform_get_irq"). The intent was to remove this code,
but looks like the cleanup has a ways to go 7 years on. Primarily,
it's convert any platform_get_resource(pdev, IORESOURCE_IRQ, n) call
to platform_get_irq(). There's ~169 of those.
There are probably some open coded accesses to pdev->resources too,
but I didn't spot any.
Rob
On 2021-12-09 00:10, Lad Prabhakar wrote:
> of_device_alloc() in early boot stage creates a interrupt mapping if
> there exists a "interrupts" property in the node.
>
> For hierarchical interrupt domains using "interrupts" property in the
> node
> bypassed the hierarchical setup and messed up the irq setup.
>
> This patch adds a check in of_device_alloc() to skip interrupt mapping
> if
> "not-interrupt-producer" property is present in the node. This allows
> nodes to describe the interrupts using "interrupts" property.
>
> Signed-off-by: Lad Prabhakar <[email protected]>
> ---
> Hi All,
>
> Spawning from discussion [1], here is simple patch (not the ideal
> probably
> welcome for suggestions) from stopping the OF code from creating a map
> for
> the interrupts when using "interrupts" property.
>
> [1] https://lore.kernel.org/lkml/[email protected]/
> T/#mbd1e47c1981082aded4b32a52e2c04291e515508
>
> Cheers,
> Prabhakar
> ---
> drivers/of/platform.c | 13 ++++++++++---
> 1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index b3faf89744aa..629776ca1721 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -114,7 +114,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 rc, i, num_reg = 0, num_irq = 0;
> struct resource *res, temp_res;
>
> dev = platform_device_alloc("", PLATFORM_DEVID_NONE);
> @@ -124,7 +124,14 @@ struct platform_device *of_device_alloc(struct
> device_node *np,
> /* count the io and irq resources */
> while (of_address_to_resource(np, num_reg, &temp_res) == 0)
> num_reg++;
> - num_irq = of_irq_count(np);
> +
> + /*
> + * we don't want to map the interrupts of hierarchical interrupt
> domain
> + * into the parent domain yet. This will be the job of the
> hierarchical
> + * interrupt driver code to map the interrupts as and when needed.
> + */
> + if (!of_property_read_bool(np, "not-interrupt-producer"))
I don't think this is right. If anything, the expected behaviour should
be
indicated by the driver and not the device node.
> + num_irq = of_irq_count(np);
>
> /* Populate the resource table */
> if (num_irq || num_reg) {
> @@ -140,7 +147,7 @@ struct platform_device *of_device_alloc(struct
> device_node *np,
> rc = of_address_to_resource(np, i, res);
> WARN_ON(rc);
> }
> - if (of_irq_to_resource_table(np, res, num_irq) != num_irq)
> + if (num_irq && of_irq_to_resource_table(np, res, num_irq) !=
> num_irq)
> pr_debug("not all legacy IRQ resources mapped for %pOFn\n",
> np);
> }
The root of the issue is that all the resource allocation is done
upfront,
way before we even have a driver that could potentially deal with this
device. This is a potential waste of resource, and it triggers the
issue you noticed.
If you delay the resource allocation until there is an actual match with
a
driver, you could have a per-driver flag telling you whether the IRQ
allocation should be performed before the probe() function is called.
M.
--
Jazz is not dead. It just smells funny...
Hi Rob,
Thank you for the review.
On Thu, Dec 9, 2021 at 3:08 AM Rob Herring <[email protected]> wrote:
>
> On Wed, Dec 8, 2021 at 6:11 PM Lad Prabhakar
> <[email protected]> wrote:
> >
> > of_device_alloc() in early boot stage creates a interrupt mapping if
> > there exists a "interrupts" property in the node.
> >
> > For hierarchical interrupt domains using "interrupts" property in the node
> > bypassed the hierarchical setup and messed up the irq setup.
> >
> > This patch adds a check in of_device_alloc() to skip interrupt mapping if
> > "not-interrupt-producer" property is present in the node. This allows
> > nodes to describe the interrupts using "interrupts" property.
> >
> > Signed-off-by: Lad Prabhakar <[email protected]>
> > ---
> > Hi All,
> >
> > Spawning from discussion [1], here is simple patch (not the ideal probably
> > welcome for suggestions) from stopping the OF code from creating a map for
> > the interrupts when using "interrupts" property.
> >
> > [1] https://lore.kernel.org/lkml/[email protected]/
> > T/#mbd1e47c1981082aded4b32a52e2c04291e515508
> >
> > Cheers,
> > Prabhakar
> > ---
> > drivers/of/platform.c | 13 ++++++++++---
> > 1 file changed, 10 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> > index b3faf89744aa..629776ca1721 100644
> > --- a/drivers/of/platform.c
> > +++ b/drivers/of/platform.c
> > @@ -114,7 +114,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 rc, i, num_reg = 0, num_irq = 0;
> > struct resource *res, temp_res;
> >
> > dev = platform_device_alloc("", PLATFORM_DEVID_NONE);
> > @@ -124,7 +124,14 @@ struct platform_device *of_device_alloc(struct device_node *np,
> > /* count the io and irq resources */
> > while (of_address_to_resource(np, num_reg, &temp_res) == 0)
> > num_reg++;
> > - num_irq = of_irq_count(np);
> > +
> > + /*
> > + * we don't want to map the interrupts of hierarchical interrupt domain
> > + * into the parent domain yet. This will be the job of the hierarchical
> > + * interrupt driver code to map the interrupts as and when needed.
> > + */
> > + if (!of_property_read_bool(np, "not-interrupt-producer"))
> > + num_irq = of_irq_count(np);
>
> The property won't fly for sure. A compatible match table could work
> here, but I don't really want another temporary solution.
>
Agreed.
> > /* Populate the resource table */
> > if (num_irq || num_reg) {
> > @@ -140,7 +147,7 @@ struct platform_device *of_device_alloc(struct device_node *np,
> > rc = of_address_to_resource(np, i, res);
> > WARN_ON(rc);
> > }
> > - if (of_irq_to_resource_table(np, res, num_irq) != num_irq)
> > + if (num_irq && of_irq_to_resource_table(np, res, num_irq) != num_irq)
>
> You might want to look at commit 9ec36cafe43b ("of/irq: do irq
> resolution in platform_get_irq"). The intent was to remove this code,
> but looks like the cleanup has a ways to go 7 years on. Primarily,
> it's convert any platform_get_resource(pdev, IORESOURCE_IRQ, n) call
> to platform_get_irq(). There's ~169 of those.
>
That's a good point converting all the drivers to use
platform_get_irq() so that the resource allocation happens on demand,
and the above code can be dropped.
> There are probably some open coded accesses to pdev->resources too,
> but I didn't spot any.
>
I'll have a closer look.
Cheers,
Prabhakar
> Rob
Hi Marc,
Thank you for the review.
On Thu, Dec 9, 2021 at 8:07 AM Marc Zyngier <[email protected]> wrote:
>
> On 2021-12-09 00:10, Lad Prabhakar wrote:
> > of_device_alloc() in early boot stage creates a interrupt mapping if
> > there exists a "interrupts" property in the node.
> >
> > For hierarchical interrupt domains using "interrupts" property in the
> > node
> > bypassed the hierarchical setup and messed up the irq setup.
> >
> > This patch adds a check in of_device_alloc() to skip interrupt mapping
> > if
> > "not-interrupt-producer" property is present in the node. This allows
> > nodes to describe the interrupts using "interrupts" property.
> >
> > Signed-off-by: Lad Prabhakar <[email protected]>
> > ---
> > Hi All,
> >
> > Spawning from discussion [1], here is simple patch (not the ideal
> > probably
> > welcome for suggestions) from stopping the OF code from creating a map
> > for
> > the interrupts when using "interrupts" property.
> >
> > [1] https://lore.kernel.org/lkml/[email protected]/
> > T/#mbd1e47c1981082aded4b32a52e2c04291e515508
> >
> > Cheers,
> > Prabhakar
> > ---
> > drivers/of/platform.c | 13 ++++++++++---
> > 1 file changed, 10 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> > index b3faf89744aa..629776ca1721 100644
> > --- a/drivers/of/platform.c
> > +++ b/drivers/of/platform.c
> > @@ -114,7 +114,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 rc, i, num_reg = 0, num_irq = 0;
> > struct resource *res, temp_res;
> >
> > dev = platform_device_alloc("", PLATFORM_DEVID_NONE);
> > @@ -124,7 +124,14 @@ struct platform_device *of_device_alloc(struct
> > device_node *np,
> > /* count the io and irq resources */
> > while (of_address_to_resource(np, num_reg, &temp_res) == 0)
> > num_reg++;
> > - num_irq = of_irq_count(np);
> > +
> > + /*
> > + * we don't want to map the interrupts of hierarchical interrupt
> > domain
> > + * into the parent domain yet. This will be the job of the
> > hierarchical
> > + * interrupt driver code to map the interrupts as and when needed.
> > + */
> > + if (!of_property_read_bool(np, "not-interrupt-producer"))
>
> I don't think this is right. If anything, the expected behaviour should
> be
> indicated by the driver and not the device node.
>
> > + num_irq = of_irq_count(np);
> >
> > /* Populate the resource table */
> > if (num_irq || num_reg) {
> > @@ -140,7 +147,7 @@ struct platform_device *of_device_alloc(struct
> > device_node *np,
> > rc = of_address_to_resource(np, i, res);
> > WARN_ON(rc);
> > }
> > - if (of_irq_to_resource_table(np, res, num_irq) != num_irq)
> > + if (num_irq && of_irq_to_resource_table(np, res, num_irq) !=
> > num_irq)
> > pr_debug("not all legacy IRQ resources mapped for %pOFn\n",
> > np);
> > }
>
> The root of the issue is that all the resource allocation is done
> upfront,
> way before we even have a driver that could potentially deal with this
> device. This is a potential waste of resource, and it triggers the
> issue you noticed.
>
> If you delay the resource allocation until there is an actual match with
> a
> driver, you could have a per-driver flag telling you whether the IRQ
> allocation should be performed before the probe() function is called.
>
As suggested by Rob, if we switch the drivers to use
platform_get_resource(pdev, IORESOURCE_IRQ, n) call with
platform_get_irq() this code should go away and with this switch the
resource allocation will happen demand. Is this approach OK?
Cheers,
Prabhakar
> M.
> --
> Jazz is not dead. It just smells funny...
On Thu, 09 Dec 2021 10:00:44 +0000,
"Lad, Prabhakar" <[email protected]> wrote:
>
> > The root of the issue is that all the resource allocation is done
> > upfront, way before we even have a driver that could potentially
> > deal with this device. This is a potential waste of resource, and
> > it triggers the issue you noticed.
> >
> > If you delay the resource allocation until there is an actual
> > match with a driver, you could have a per-driver flag telling you
> > whether the IRQ allocation should be performed before the probe()
> > function is called.
> >
> As suggested by Rob, if we switch the drivers to use
> platform_get_resource(pdev, IORESOURCE_IRQ, n) call with
> platform_get_irq() this code should go away and with this switch the
> resource allocation will happen demand. Is this approach OK?
If you get rid of of_irq_to_resource_table() altogether, then yes,
this has a fighting chance to work.
M.
--
Without deviation from the norm, progress is not possible.
Hi Rob and Marc,
On Thu, Dec 9, 2021 at 10:33 AM Marc Zyngier <[email protected]> wrote:
>
> On Thu, 09 Dec 2021 10:00:44 +0000,
> "Lad, Prabhakar" <[email protected]> wrote:
> >
> > > The root of the issue is that all the resource allocation is done
> > > upfront, way before we even have a driver that could potentially
> > > deal with this device. This is a potential waste of resource, and
> > > it triggers the issue you noticed.
> > >
> > > If you delay the resource allocation until there is an actual
> > > match with a driver, you could have a per-driver flag telling you
> > > whether the IRQ allocation should be performed before the probe()
> > > function is called.
> > >
> > As suggested by Rob, if we switch the drivers to use
> > platform_get_resource(pdev, IORESOURCE_IRQ, n) call with
> > platform_get_irq() this code should go away and with this switch the
> > resource allocation will happen demand. Is this approach OK?
>
> If you get rid of of_irq_to_resource_table() altogether, then yes,
> this has a fighting chance to work.
>
Yes, switching to platform_get_irq() will eventually cause
of_irq_to_resource_table() to go away.
On second thought, instead of touching all the drivers, if we update
platform_get_resource/platform_get_resource_byname to internally call
platform_get_irq() internally if it's a IORESOURCE_IRQ resource. Does
that sound good or should I just get on changing all the drivers to
use platform_get_irq() instead?
Cheers,
Prabhakar
> M.
>
>
> --
> Without deviation from the norm, progress is not possible.
On Thu, Dec 9, 2021 at 5:35 AM Lad, Prabhakar
<[email protected]> wrote:
>
> Hi Rob and Marc,
>
> On Thu, Dec 9, 2021 at 10:33 AM Marc Zyngier <[email protected]> wrote:
> >
> > On Thu, 09 Dec 2021 10:00:44 +0000,
> > "Lad, Prabhakar" <[email protected]> wrote:
> > >
> > > > The root of the issue is that all the resource allocation is done
> > > > upfront, way before we even have a driver that could potentially
> > > > deal with this device. This is a potential waste of resource, and
> > > > it triggers the issue you noticed.
> > > >
> > > > If you delay the resource allocation until there is an actual
> > > > match with a driver, you could have a per-driver flag telling you
> > > > whether the IRQ allocation should be performed before the probe()
> > > > function is called.
> > > >
> > > As suggested by Rob, if we switch the drivers to use
> > > platform_get_resource(pdev, IORESOURCE_IRQ, n) call with
> > > platform_get_irq() this code should go away and with this switch the
> > > resource allocation will happen demand. Is this approach OK?
> >
> > If you get rid of of_irq_to_resource_table() altogether, then yes,
> > this has a fighting chance to work.
> >
> Yes, switching to platform_get_irq() will eventually cause
> of_irq_to_resource_table() to go away.
>
> On second thought, instead of touching all the drivers, if we update
> platform_get_resource/platform_get_resource_byname to internally call
> platform_get_irq() internally if it's a IORESOURCE_IRQ resource. Does
> that sound good or should I just get on changing all the drivers to
> use platform_get_irq() instead?
Except that platform_get_irq() already internally calls
platform_get_resource()... I think changing the drivers is the right
way. Happy to do some if you want to divide it up.
Using coccigrep, I think I've found all the places using
platform_device.resource directly. A large swath are Sparc drivers
which don't matter. The few that do matter I've prepared patches for
here[1]. Most of what I found were DT based drivers that copy
resources to a child platform device. That case will not work with
platform_get_irq() callers either unless the child device has it's DT
node set to the parent node which is the change I made.
Rob
[1] git://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-kernelci
On Thu, Dec 9, 2021 at 8:34 PM Rob Herring <[email protected]> wrote:
>
> On Thu, Dec 9, 2021 at 5:35 AM Lad, Prabhakar
> <[email protected]> wrote:
> >
> > Hi Rob and Marc,
> >
> > On Thu, Dec 9, 2021 at 10:33 AM Marc Zyngier <[email protected]> wrote:
> > >
> > > On Thu, 09 Dec 2021 10:00:44 +0000,
> > > "Lad, Prabhakar" <[email protected]> wrote:
> > > >
> > > > > The root of the issue is that all the resource allocation is done
> > > > > upfront, way before we even have a driver that could potentially
> > > > > deal with this device. This is a potential waste of resource, and
> > > > > it triggers the issue you noticed.
> > > > >
> > > > > If you delay the resource allocation until there is an actual
> > > > > match with a driver, you could have a per-driver flag telling you
> > > > > whether the IRQ allocation should be performed before the probe()
> > > > > function is called.
> > > > >
> > > > As suggested by Rob, if we switch the drivers to use
> > > > platform_get_resource(pdev, IORESOURCE_IRQ, n) call with
> > > > platform_get_irq() this code should go away and with this switch the
> > > > resource allocation will happen demand. Is this approach OK?
> > >
> > > If you get rid of of_irq_to_resource_table() altogether, then yes,
> > > this has a fighting chance to work.
> > >
> > Yes, switching to platform_get_irq() will eventually cause
> > of_irq_to_resource_table() to go away.
> >
> > On second thought, instead of touching all the drivers, if we update
> > platform_get_resource/platform_get_resource_byname to internally call
> > platform_get_irq() internally if it's a IORESOURCE_IRQ resource. Does
> > that sound good or should I just get on changing all the drivers to
> > use platform_get_irq() instead?
>
> Except that platform_get_irq() already internally calls
> platform_get_resource()... I think changing the drivers is the right
> way. Happy to do some if you want to divide it up.
>
Thank you, I think I'll manage.
> Using coccigrep, I think I've found all the places using
> platform_device.resource directly. A large swath are Sparc drivers
> which don't matter. The few that do matter I've prepared patches for
> here[1]. Most of what I found were DT based drivers that copy
> resources to a child platform device. That case will not work with
> platform_get_irq() callers either unless the child device has it's DT
> node set to the parent node which is the change I made.
>
Thank you for getting this done. Do you want me to include those along
with my conversion patches?
Any reason why we dont care for Sparc drivers?
> Rob
>
> [1] git://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-kernelci
Cheers,
Prabhakar
On Thu, Dec 9, 2021 at 7:16 PM Lad, Prabhakar
<[email protected]> wrote:
>
> On Thu, Dec 9, 2021 at 8:34 PM Rob Herring <[email protected]> wrote:
> >
> > On Thu, Dec 9, 2021 at 5:35 AM Lad, Prabhakar
> > <[email protected]> wrote:
> > >
> > > Hi Rob and Marc,
> > >
> > > On Thu, Dec 9, 2021 at 10:33 AM Marc Zyngier <[email protected]> wrote:
> > > >
> > > > On Thu, 09 Dec 2021 10:00:44 +0000,
> > > > "Lad, Prabhakar" <[email protected]> wrote:
> > > > >
> > > > > > The root of the issue is that all the resource allocation is done
> > > > > > upfront, way before we even have a driver that could potentially
> > > > > > deal with this device. This is a potential waste of resource, and
> > > > > > it triggers the issue you noticed.
> > > > > >
> > > > > > If you delay the resource allocation until there is an actual
> > > > > > match with a driver, you could have a per-driver flag telling you
> > > > > > whether the IRQ allocation should be performed before the probe()
> > > > > > function is called.
> > > > > >
> > > > > As suggested by Rob, if we switch the drivers to use
> > > > > platform_get_resource(pdev, IORESOURCE_IRQ, n) call with
> > > > > platform_get_irq() this code should go away and with this switch the
> > > > > resource allocation will happen demand. Is this approach OK?
> > > >
> > > > If you get rid of of_irq_to_resource_table() altogether, then yes,
> > > > this has a fighting chance to work.
> > > >
> > > Yes, switching to platform_get_irq() will eventually cause
> > > of_irq_to_resource_table() to go away.
> > >
> > > On second thought, instead of touching all the drivers, if we update
> > > platform_get_resource/platform_get_resource_byname to internally call
> > > platform_get_irq() internally if it's a IORESOURCE_IRQ resource. Does
> > > that sound good or should I just get on changing all the drivers to
> > > use platform_get_irq() instead?
> >
> > Except that platform_get_irq() already internally calls
> > platform_get_resource()... I think changing the drivers is the right
> > way. Happy to do some if you want to divide it up.
> >
> Thank you, I think I'll manage.
>
> > Using coccigrep, I think I've found all the places using
> > platform_device.resource directly. A large swath are Sparc drivers
> > which don't matter. The few that do matter I've prepared patches for
> > here[1]. Most of what I found were DT based drivers that copy
> > resources to a child platform device. That case will not work with
> > platform_get_irq() callers either unless the child device has it's DT
> > node set to the parent node which is the change I made.
> >
> Thank you for getting this done. Do you want me to include those along
> with my conversion patches?
No, I'll send them out.
> Any reason why we dont care for Sparc drivers?
Sparc does its own thing and doesn't use drivers/of/platform.c to
create devices. I'm sure we could modernize a bunch of them, but
that's not a blocker.
Rob
Hi Marc,
On Thu, Dec 9, 2021 at 10:33 AM Marc Zyngier <[email protected]> wrote:
>
> On Thu, 09 Dec 2021 10:00:44 +0000,
> "Lad, Prabhakar" <[email protected]> wrote:
> >
> > > The root of the issue is that all the resource allocation is done
> > > upfront, way before we even have a driver that could potentially
> > > deal with this device. This is a potential waste of resource, and
> > > it triggers the issue you noticed.
> > >
> > > If you delay the resource allocation until there is an actual
> > > match with a driver, you could have a per-driver flag telling you
> > > whether the IRQ allocation should be performed before the probe()
> > > function is called.
> > >
> > As suggested by Rob, if we switch the drivers to use
> > platform_get_resource(pdev, IORESOURCE_IRQ, n) call with
> > platform_get_irq() this code should go away and with this switch the
> > resource allocation will happen demand. Is this approach OK?
>
> If you get rid of of_irq_to_resource_table() altogether, then yes,
> this has a fighting chance to work.
>
To clarify, did you mean to get rid of_irq_to_resource_table()
completely or just from the drivers/of/platform.c ([0])?
[0] https://elixir.bootlin.com/linux/latest/source/drivers/of/platform.c#L143
Cheers,
Prabhakar