* [PATCH] irqchip/renesas-intc-irqpin: Use platform_get_irq_optional() to get the interrupt @ 2021-12-15 23:44 Lad Prabhakar 2021-12-16 8:45 ` Geert Uytterhoeven 0 siblings, 1 reply; 5+ messages in thread From: Lad Prabhakar @ 2021-12-15 23:44 UTC (permalink / raw) To: Thomas Gleixner, Marc Zyngier Cc: Rob Herring, Geert Uytterhoeven, linux-kernel, Prabhakar, 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] irqchip/renesas-intc-irqpin: Use platform_get_irq_optional() to get the interrupt 2021-12-15 23:44 [PATCH] irqchip/renesas-intc-irqpin: Use platform_get_irq_optional() to get the interrupt Lad Prabhakar @ 2021-12-16 8:45 ` Geert Uytterhoeven 2021-12-16 14:22 ` Lad, Prabhakar 0 siblings, 1 reply; 5+ messages in thread From: Geert Uytterhoeven @ 2021-12-16 8:45 UTC (permalink / raw) To: Lad Prabhakar Cc: Thomas Gleixner, Marc Zyngier, Rob Herring, Linux Kernel Mailing List, Prabhakar 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] irqchip/renesas-intc-irqpin: Use platform_get_irq_optional() to get the interrupt 2021-12-16 8:45 ` Geert Uytterhoeven @ 2021-12-16 14:22 ` Lad, Prabhakar 2021-12-16 14:31 ` Geert Uytterhoeven 0 siblings, 1 reply; 5+ messages in thread From: Lad, Prabhakar @ 2021-12-16 14:22 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Lad Prabhakar, Thomas Gleixner, Marc Zyngier, Rob Herring, Linux Kernel Mailing List 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] irqchip/renesas-intc-irqpin: Use platform_get_irq_optional() to get the interrupt 2021-12-16 14:22 ` Lad, Prabhakar @ 2021-12-16 14:31 ` Geert Uytterhoeven 2021-12-16 17:36 ` Lad, Prabhakar 0 siblings, 1 reply; 5+ messages in thread From: Geert Uytterhoeven @ 2021-12-16 14:31 UTC (permalink / raw) To: Lad, Prabhakar Cc: Lad Prabhakar, Thomas Gleixner, Marc Zyngier, Rob Herring, Linux Kernel Mailing List, Linux-Renesas 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] irqchip/renesas-intc-irqpin: Use platform_get_irq_optional() to get the interrupt 2021-12-16 14:31 ` Geert Uytterhoeven @ 2021-12-16 17:36 ` Lad, Prabhakar 0 siblings, 0 replies; 5+ messages in thread From: Lad, Prabhakar @ 2021-12-16 17:36 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Lad Prabhakar, Thomas Gleixner, Marc Zyngier, Rob Herring, Linux Kernel Mailing List, Linux-Renesas 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-12-16 17:37 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-12-15 23:44 [PATCH] irqchip/renesas-intc-irqpin: Use platform_get_irq_optional() to get the interrupt Lad Prabhakar 2021-12-16 8:45 ` Geert Uytterhoeven 2021-12-16 14:22 ` Lad, Prabhakar 2021-12-16 14:31 ` Geert Uytterhoeven 2021-12-16 17:36 ` Lad, Prabhakar
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).