[PATCH] irqchip/renesas-intc-irqpin: Use platform_get_irq_optional() to get the interrupt

[PATCH] irqchip/renesas-intc-irqpin: Use platform_get_irq_optional() to get the interrupt

[Lad Prabhakar]

platform_get_resource(pdev, IORESOURCE_IRQ, ..) relies on static
allocation of IRQ resources in DT core code, this causes an issue
when using hierarchical interrupt domains using "interrupts" property
in the node as this bypassed the hierarchical setup and messed up the
irq chaining.

In preparation for removal of static setup of IRQ resource from DT core
code use platform_get_irq_optional().

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
Hi,

Usage of platform_get_irq/_optional was agreed based on
the discussion [0].

[0] https://patchwork.kernel.org/project/linux-renesas-soc/
patch/20211209001056.29774-1-prabhakar.mahadev-lad.rj@bp.renesas.com/

Cheers,
Prabhakar
---
 drivers/irqchip/irq-renesas-intc-irqpin.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/irqchip/irq-renesas-intc-irqpin.c b/drivers/irqchip/irq-renesas-intc-irqpin.c
index cb7f60b3b4a9..c35d9fbcda5c 100644
--- a/drivers/irqchip/irq-renesas-intc-irqpin.c
+++ b/drivers/irqchip/irq-renesas-intc-irqpin.c
@@ -375,7 +375,6 @@ static int intc_irqpin_probe(struct platform_device *pdev)
 	struct intc_irqpin_priv *p;
 	struct intc_irqpin_iomem *i;
 	struct resource *io[INTC_IRQPIN_REG_NR];
-	struct resource *irq;
 	struct irq_chip *irq_chip;
 	void (*enable_fn)(struct irq_data *d);
 	void (*disable_fn)(struct irq_data *d);
@@ -418,12 +417,14 @@ static int intc_irqpin_probe(struct platform_device *pdev)
 
 	/* allow any number of IRQs between 1 and INTC_IRQPIN_MAX */
 	for (k = 0; k < INTC_IRQPIN_MAX; k++) {
-		irq = platform_get_resource(pdev, IORESOURCE_IRQ, k);
-		if (!irq)
+		ret = platform_get_irq_optional(pdev, k);
+		if (ret == -EPROBE_DEFER)
+			goto err0;
+		if (ret < 0)
 			break;
 
 		p->irq[k].p = p;
-		p->irq[k].requested_irq = irq->start;
+		p->irq[k].requested_irq = ret;
 	}
 
 	nirqs = k;
-- 
2.17.1

Re: [PATCH] irqchip/renesas-intc-irqpin: Use platform_get_irq_optional() to get the interrupt

[Geert Uytterhoeven]

Hi Prabhakar,

On Thu, Dec 16, 2021 at 12:45 AM Lad Prabhakar
<prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
> platform_get_resource(pdev, IORESOURCE_IRQ, ..) relies on static
> allocation of IRQ resources in DT core code, this causes an issue
> when using hierarchical interrupt domains using "interrupts" property
> in the node as this bypassed the hierarchical setup and messed up the
> irq chaining.
>
> In preparation for removal of static setup of IRQ resource from DT core
> code use platform_get_irq_optional().
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

> --- a/drivers/irqchip/irq-renesas-intc-irqpin.c
> +++ b/drivers/irqchip/irq-renesas-intc-irqpin.c

> @@ -418,12 +417,14 @@ static int intc_irqpin_probe(struct platform_device *pdev)
>
>         /* allow any number of IRQs between 1 and INTC_IRQPIN_MAX */
>         for (k = 0; k < INTC_IRQPIN_MAX; k++) {
> -               irq = platform_get_resource(pdev, IORESOURCE_IRQ, k);
> -               if (!irq)
> +               ret = platform_get_irq_optional(pdev, k);
> +               if (ret == -EPROBE_DEFER)
> +                       goto err0;
> +               if (ret < 0)
>                         break;

Shouldn't all errors be considered fatal, except for -ENXIO (no
interrupt)?

>
>                 p->irq[k].p = p;
> -               p->irq[k].requested_irq = irq->start;
> +               p->irq[k].requested_irq = ret;
>         }
>
>         nirqs = k;

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] irqchip/renesas-intc-irqpin: Use platform_get_irq_optional() to get the interrupt

[Lad, Prabhakar]

Hi Geert,

Thank you for the review.

On Thu, Dec 16, 2021 at 8:45 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> On Thu, Dec 16, 2021 at 12:45 AM Lad Prabhakar
> <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
> > platform_get_resource(pdev, IORESOURCE_IRQ, ..) relies on static
> > allocation of IRQ resources in DT core code, this causes an issue
> > when using hierarchical interrupt domains using "interrupts" property
> > in the node as this bypassed the hierarchical setup and messed up the
> > irq chaining.
> >
> > In preparation for removal of static setup of IRQ resource from DT core
> > code use platform_get_irq_optional().
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> > --- a/drivers/irqchip/irq-renesas-intc-irqpin.c
> > +++ b/drivers/irqchip/irq-renesas-intc-irqpin.c
>
> > @@ -418,12 +417,14 @@ static int intc_irqpin_probe(struct platform_device *pdev)
> >
> >         /* allow any number of IRQs between 1 and INTC_IRQPIN_MAX */
> >         for (k = 0; k < INTC_IRQPIN_MAX; k++) {
> > -               irq = platform_get_resource(pdev, IORESOURCE_IRQ, k);
> > -               if (!irq)
> > +               ret = platform_get_irq_optional(pdev, k);
> > +               if (ret == -EPROBE_DEFER)
> > +                       goto err0;
> > +               if (ret < 0)
> >                         break;
>
> Shouldn't all errors be considered fatal, except for -ENXIO (no
> interrupt)?
>
Initial behavior of this driver was even if one
platform_get_resource() succeeded the probe continued further, this is
the same behavior which I wanted to keep with this code while using
platform_get_irq_optional(). But as you pointed out I'll change it to below:

+               ret = platform_get_irq(pdev, k);
+               if (ret < 0)
+                       goto err0;

We bail out any error case and will also drop the check for (nirqs < 1).

Cheers,
Prabhakar
> >
> >                 p->irq[k].p = p;
> > -               p->irq[k].requested_irq = irq->start;
> > +               p->irq[k].requested_irq = ret;
> >         }
> >
> >         nirqs = k;
>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

Re: [PATCH] irqchip/renesas-intc-irqpin: Use platform_get_irq_optional() to get the interrupt

[Geert Uytterhoeven]

Hi Prabhakar,

On Thu, Dec 16, 2021 at 3:23 PM Lad, Prabhakar
<prabhakar.csengg@gmail.com> wrote:
> On Thu, Dec 16, 2021 at 8:45 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Thu, Dec 16, 2021 at 12:45 AM Lad Prabhakar
> > <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
> > > platform_get_resource(pdev, IORESOURCE_IRQ, ..) relies on static
> > > allocation of IRQ resources in DT core code, this causes an issue
> > > when using hierarchical interrupt domains using "interrupts" property
> > > in the node as this bypassed the hierarchical setup and messed up the
> > > irq chaining.
> > >
> > > In preparation for removal of static setup of IRQ resource from DT core
> > > code use platform_get_irq_optional().
> > >
> > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > > --- a/drivers/irqchip/irq-renesas-intc-irqpin.c
> > > +++ b/drivers/irqchip/irq-renesas-intc-irqpin.c
> >
> > > @@ -418,12 +417,14 @@ static int intc_irqpin_probe(struct platform_device *pdev)
> > >
> > >         /* allow any number of IRQs between 1 and INTC_IRQPIN_MAX */
> > >         for (k = 0; k < INTC_IRQPIN_MAX; k++) {
> > > -               irq = platform_get_resource(pdev, IORESOURCE_IRQ, k);
> > > -               if (!irq)
> > > +               ret = platform_get_irq_optional(pdev, k);
> > > +               if (ret == -EPROBE_DEFER)
> > > +                       goto err0;
> > > +               if (ret < 0)
> > >                         break;
> >
> > Shouldn't all errors be considered fatal, except for -ENXIO (no
> > interrupt)?
> >
> Initial behavior of this driver was even if one
> platform_get_resource() succeeded the probe continued further, this is

Indeed, the loop obtained all interrupts present, until no more are to found.
In the old logic, it would return a NULL resource for the first
non-existing one.
In the new logic, it would return -ENXIO.
Hence you need to check for -ENXIO in the loop, to distinguish "no more
interrupts" from actual errors.

> the same behavior which I wanted to keep with this code while using
> platform_get_irq_optional(). But as you pointed out I'll change it to below:
>
> +               ret = platform_get_irq(pdev, k);
> +               if (ret < 0)
> +                       goto err0;
>
> We bail out any error case and will also drop the check for (nirqs < 1).

I think that check should stay: there should be at least one interrupt.

> > >                 p->irq[k].p = p;
> > > -               p->irq[k].requested_irq = irq->start;
> > > +               p->irq[k].requested_irq = ret;
> > >         }
> > >
> > >         nirqs = k;

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] irqchip/renesas-intc-irqpin: Use platform_get_irq_optional() to get the interrupt

[Lad, Prabhakar]

Hi Geert,

On Thu, Dec 16, 2021 at 2:31 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> On Thu, Dec 16, 2021 at 3:23 PM Lad, Prabhakar
> <prabhakar.csengg@gmail.com> wrote:
> > On Thu, Dec 16, 2021 at 8:45 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > On Thu, Dec 16, 2021 at 12:45 AM Lad Prabhakar
> > > <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
> > > > platform_get_resource(pdev, IORESOURCE_IRQ, ..) relies on static
> > > > allocation of IRQ resources in DT core code, this causes an issue
> > > > when using hierarchical interrupt domains using "interrupts" property
> > > > in the node as this bypassed the hierarchical setup and messed up the
> > > > irq chaining.
> > > >
> > > > In preparation for removal of static setup of IRQ resource from DT core
> > > > code use platform_get_irq_optional().
> > > >
> > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > >
> > > > --- a/drivers/irqchip/irq-renesas-intc-irqpin.c
> > > > +++ b/drivers/irqchip/irq-renesas-intc-irqpin.c
> > >
> > > > @@ -418,12 +417,14 @@ static int intc_irqpin_probe(struct platform_device *pdev)
> > > >
> > > >         /* allow any number of IRQs between 1 and INTC_IRQPIN_MAX */
> > > >         for (k = 0; k < INTC_IRQPIN_MAX; k++) {
> > > > -               irq = platform_get_resource(pdev, IORESOURCE_IRQ, k);
> > > > -               if (!irq)
> > > > +               ret = platform_get_irq_optional(pdev, k);
> > > > +               if (ret == -EPROBE_DEFER)
> > > > +                       goto err0;
> > > > +               if (ret < 0)
> > > >                         break;
> > >
> > > Shouldn't all errors be considered fatal, except for -ENXIO (no
> > > interrupt)?
> > >
> > Initial behavior of this driver was even if one
> > platform_get_resource() succeeded the probe continued further, this is
>
> Indeed, the loop obtained all interrupts present, until no more are to found.
> In the old logic, it would return a NULL resource for the first
> non-existing one.
> In the new logic, it would return -ENXIO.
> Hence you need to check for -ENXIO in the loop, to distinguish "no more
> interrupts" from actual errors.
>
Thanks for the clarification, I'll post a v2 with the changes.

> > the same behavior which I wanted to keep with this code while using
> > platform_get_irq_optional(). But as you pointed out I'll change it to below:
> >
> > +               ret = platform_get_irq(pdev, k);
> > +               if (ret < 0)
> > +                       goto err0;
> >
> > We bail out any error case and will also drop the check for (nirqs < 1).
>
> I think that check should stay: there should be at least one interrupt.
>
Agreed.

Cheers,
Prabhakar
> > > >                 p->irq[k].p = p;
> > > > -               p->irq[k].requested_irq = irq->start;
> > > > +               p->irq[k].requested_irq = ret;
> > > >         }
> > > >
> > > >         nirqs = k;
>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds