[RFC PATCH] of: platform: Skip mapping of interrupts in of_device_alloc()

[RFC PATCH] of: platform: Skip mapping of interrupts in of_device_alloc()

[Lad Prabhakar]

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 <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
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/87pmqrck2m.wl-maz@kernel.org/
    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

Re: [RFC PATCH] of: platform: Skip mapping of interrupts in of_device_alloc()

[Rob Herring]

On Wed, Dec 8, 2021 at 6:11 PM Lad Prabhakar
<prabhakar.mahadev-lad.rj@bp.renesas.com> 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 <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
> 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/87pmqrck2m.wl-maz@kernel.org/
>     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

Re: [RFC PATCH] of: platform: Skip mapping of interrupts in of_device_alloc()

[Lad, Prabhakar]

Hi Rob,

Thank you for the review.

On Thu, Dec 9, 2021 at 3:08 AM Rob Herring <robh+dt@kernel.org> wrote:
>
> On Wed, Dec 8, 2021 at 6:11 PM Lad Prabhakar
> <prabhakar.mahadev-lad.rj@bp.renesas.com> 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 <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> > 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/87pmqrck2m.wl-maz@kernel.org/
> >     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

Re: [RFC PATCH] of: platform: Skip mapping of interrupts in of_device_alloc()

[Marc Zyngier]

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 <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
> 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/87pmqrck2m.wl-maz@kernel.org/
>     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...

Re: [RFC PATCH] of: platform: Skip mapping of interrupts in of_device_alloc()

[Lad, Prabhakar]

Hi Marc,

Thank you for the review.

On Thu, Dec 9, 2021 at 8:07 AM Marc Zyngier <maz@kernel.org> 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 <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> > 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/87pmqrck2m.wl-maz@kernel.org/
> >     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...

Re: [RFC PATCH] of: platform: Skip mapping of interrupts in of_device_alloc()

[Marc Zyngier]

On Thu, 09 Dec 2021 10:00:44 +0000,
"Lad, Prabhakar" <prabhakar.csengg@gmail.com> 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.

Re: [RFC PATCH] of: platform: Skip mapping of interrupts in of_device_alloc()

[Lad, Prabhakar]

Hi Rob and Marc,

On Thu, Dec 9, 2021 at 10:33 AM Marc Zyngier <maz@kernel.org> wrote:
>
> On Thu, 09 Dec 2021 10:00:44 +0000,
> "Lad, Prabhakar" <prabhakar.csengg@gmail.com> 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.

Re: [RFC PATCH] of: platform: Skip mapping of interrupts in of_device_alloc()

[Rob Herring]

On Thu, Dec 9, 2021 at 5:35 AM Lad, Prabhakar
<prabhakar.csengg@gmail.com> wrote:
>
> Hi Rob and Marc,
>
> On Thu, Dec 9, 2021 at 10:33 AM Marc Zyngier <maz@kernel.org> wrote:
> >
> > On Thu, 09 Dec 2021 10:00:44 +0000,
> > "Lad, Prabhakar" <prabhakar.csengg@gmail.com> 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

Re: [RFC PATCH] of: platform: Skip mapping of interrupts in of_device_alloc()

[Lad, Prabhakar]

On Thu, Dec 9, 2021 at 8:34 PM Rob Herring <robh+dt@kernel.org> wrote:
>
> On Thu, Dec 9, 2021 at 5:35 AM Lad, Prabhakar
> <prabhakar.csengg@gmail.com> wrote:
> >
> > Hi Rob and Marc,
> >
> > On Thu, Dec 9, 2021 at 10:33 AM Marc Zyngier <maz@kernel.org> wrote:
> > >
> > > On Thu, 09 Dec 2021 10:00:44 +0000,
> > > "Lad, Prabhakar" <prabhakar.csengg@gmail.com> 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

Re: [RFC PATCH] of: platform: Skip mapping of interrupts in of_device_alloc()

[Rob Herring]

On Thu, Dec 9, 2021 at 7:16 PM Lad, Prabhakar
<prabhakar.csengg@gmail.com> wrote:
>
> On Thu, Dec 9, 2021 at 8:34 PM Rob Herring <robh+dt@kernel.org> wrote:
> >
> > On Thu, Dec 9, 2021 at 5:35 AM Lad, Prabhakar
> > <prabhakar.csengg@gmail.com> wrote:
> > >
> > > Hi Rob and Marc,
> > >
> > > On Thu, Dec 9, 2021 at 10:33 AM Marc Zyngier <maz@kernel.org> wrote:
> > > >
> > > > On Thu, 09 Dec 2021 10:00:44 +0000,
> > > > "Lad, Prabhakar" <prabhakar.csengg@gmail.com> 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

Re: [RFC PATCH] of: platform: Skip mapping of interrupts in of_device_alloc()

[Lad, Prabhakar]

Hi Marc,

On Thu, Dec 9, 2021 at 10:33 AM Marc Zyngier <maz@kernel.org> wrote:
>
> On Thu, 09 Dec 2021 10:00:44 +0000,
> "Lad, Prabhakar" <prabhakar.csengg@gmail.com> 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