Re: [PATCH 3/3] PCI: rzg3s: Fix IRQ domain initialization error handling

[Lad, Prabhakar]

Hi Claudiu,

Thank you for the review.

On Thu, Jul 23, 2026 at 6:11 PM claudiu beznea <claudiu.beznea@xxxxxxxxx> wrote:
>
> Hi, Prabhakar,
>
> On 7/16/26 01:30, Prabhakar wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> >
> > rzg3s_pcie_init_irqdomain() installs chained handlers for the INTx
> > parent interrupts before creating the INTx irqdomain and initializing
> > MSI support.
> >
> > If any subsequent step fails, such as obtaining an INTx IRQ,
> > creating the irqdomain, or initializing MSI, the error path returns
> > without removing any chained handlers that were already installed.
> > This leaves stale handler and data pointers attached to the parent
> > interrupts.
> >
> > Fix the error path by unwinding the chained handler registration before
> > returning. Also clear the chained handlers during
> > rzg3s_pcie_teardown_irqdomain() so that teardown fully mirrors the
> > initialization sequence.
> >
> > Fixes: 7ef502fb35b28 ("PCI: Add Renesas RZ/G3S host controller driver")
> > Cc: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> > ---
> >   drivers/pci/controller/pcie-rzg3s-host.c | 37 +++++++++++++++++-------
> >   1 file changed, 27 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/pci/controller/pcie-rzg3s-host.c b/drivers/pci/controller/pcie-rzg3s-host.c
> > index f51b5d7c1998..5aeb030d43c8 100644
> > --- a/drivers/pci/controller/pcie-rzg3s-host.c
> > +++ b/drivers/pci/controller/pcie-rzg3s-host.c
> > @@ -1006,22 +1006,30 @@ static const struct irq_domain_ops rzg3s_pcie_intx_domain_ops = {
> >       .xlate = irq_domain_xlate_onetwocell,
> >   };
> >
> > +static void rzg3s_pcie_clear_intx_handlers(struct rzg3s_pcie_host *host, int count)
> > +{
> > +     while (--count >= 0)
> > +             irq_set_chained_handler_and_data(host->intx_irqs[count], NULL, NULL);
> > +}
>
> On PCI subsystem lines are still wrapped at 80 chars length. Could you please
> update it, adding also {} around multi-line blocks as the rest of the code in
> this file is doing, e.g.:
>
> static void rzg3s_pcie_clear_intx_handlers(struct rzg3s_pcie_host *host,
>                                            int count)
> {
>         while (--count >= 0) {
>                 irq_set_chained_handler_and_data(host->intx_irqs[count], NULL,
>                                                  NULL);
>         }
> }
>
Ok.

> > +
> >   static int rzg3s_pcie_init_irqdomain(struct rzg3s_pcie_host *host)
> >   {
> >       struct device *dev = host->dev;
> >       struct platform_device *pdev = to_platform_device(dev);
> > +     int i, ret;
> >
> > -     for (int i = 0; i < PCI_NUM_INTX; i++) {
> > +     for (i = 0; i < PCI_NUM_INTX; i++) {
> >               char irq_name[5] = {0};
> >               int irq;
> >
> >               scnprintf(irq_name, ARRAY_SIZE(irq_name), "int%c", 'a' + i);
> >
> >               irq = platform_get_irq_byname(pdev, irq_name);
> > -             if (irq < 0)
> > -                     return dev_err_probe(dev, irq,
> > -                                          "Failed to parse and map INT%c IRQ\n",
> > -                                          'A' + i);
> > +             if (irq < 0) {
> > +                     ret = irq;
> > +                     dev_err_probe(dev, ret, "Failed to parse and map INT%c IRQ\n", 'A' + i);
> > +                     goto err_cleanup_intx;
> > +             }
> >
> >               host->intx_irqs[i] = irq;
> >               irq_set_chained_handler_and_data(irq,
> > @@ -1033,21 +1041,28 @@ static int rzg3s_pcie_init_irqdomain(struct rzg3s_pcie_host *host)
> >                                                    PCI_NUM_INTX,
> >                                                    &rzg3s_pcie_intx_domain_ops,
> >                                                    host);
> > -     if (!host->intx_domain)
> > -             return dev_err_probe(dev, -EINVAL,
> > -                                  "Failed to add irq domain for INTx IRQs\n");
> > +     if (!host->intx_domain) {
> > +             ret = -EINVAL;
> > +             dev_err_probe(dev, ret, "Failed to add irq domain for INTx IRQs\n");
> > +             goto err_cleanup_intx;
> > +     }
> >       irq_domain_update_bus_token(host->intx_domain, DOMAIN_BUS_WIRED);
> >
> >       if (IS_ENABLED(CONFIG_PCI_MSI)) {
> > -             int ret = rzg3s_pcie_init_msi(host);
> > +             ret = rzg3s_pcie_init_msi(host);
> >
> >               if (ret) {
> >                       irq_domain_remove(host->intx_domain);
>
> Could you please drop this ^ from here and (see below)...
>
> > -                     return ret;
> > +                     goto err_cleanup_intx;
> >               }
> >       }
> >
> >       return 0;
> > +
> > +err_cleanup_intx:
>
> Add it here? To have everything grouped together. I am also working on fixing
> the hotplug support and having it like this would be useful. I can do it myself
> in a separate patch, but I think, logically, it belongs to this patch.
>
Ok, In that case I will need to add a check for host->intx_domain !=
NULL before calling irq_domain_remove().

Cheers,
Prabhakar