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

Hi Claudiu,

Thank you for the review.

On Fri, Jul 31, 2026 at 5:02 PM Claudiu Beznea <[email protected]> wrote:
>
> Hi, Prabhakar,
>
> On 7/27/26 12:58, Prabhakar wrote:
> > From: Lad Prabhakar <[email protected]>
> >
> > 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: [email protected]
> > Signed-off-by: Lad Prabhakar <[email protected]>
> > ---
> > v1->v2:
> > - Restricted the code to max 80 columns.
> > - Moved calling irq_domain_remove() under err_cleanup_intx label.
> > ---
> >   drivers/pci/controller/pcie-rzg3s-host.c | 49 +++++++++++++++++-------
> >   1 file changed, 36 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/pci/controller/pcie-rzg3s-host.c b/drivers/pci/controller/pcie-rzg3s-host.c
> > index f51b5d7c1998..70f1faf7e422 100644
> > --- a/drivers/pci/controller/pcie-rzg3s-host.c
> > +++ b/drivers/pci/controller/pcie-rzg3s-host.c
> > @@ -1006,22 +1006,35 @@ 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);
> > +     }
> > +}
> > +
> >   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 +1046,29 @@ 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);
> > -                     return ret;
> > -             }
> > +             if (ret)
> > +                     goto err_cleanup_intx;
> >       }
> >
> >       return 0;
> > +
> > +err_cleanup_intx:
> > +     if (host->intx_domain)
> > +             irq_domain_remove(host->intx_domain);
> > +     rzg3s_pcie_clear_intx_handlers(host, i);
>
> If you have it like this it worth squashing these lines in the same function, e.g.:
>
>         rzg3s_pcie_teardown_intx(host, i);
>
> defined something like:
Ok.

>
> void rzg3s_pcie_teardown_intx(struct rzg3s_pcie_host *host, int count)
> {
>         if (host->intx_domain)
>                 irq_domain_remove(host->intx_domain);
>
This needs to be done after the handler has been set to NULL as the
handler does use intx_domain (pointed by Sashiko).

>
>         while (--count >= 0) {
>                 irq_set_chained_handler_and_data(host->intx_irqs[count], NULL,
>                                                  NULL);
>         }
> }
>
> > +
> > +     return ret;
> >   }
> >
> >   static void rzg3s_pcie_teardown_irqdomain(struct rzg3s_pcie_host *host)
> > @@ -1055,6 +1076,8 @@ static void rzg3s_pcie_teardown_irqdomain(struct rzg3s_pcie_host *host)
> >       if (IS_ENABLED(CONFIG_PCI_MSI))
> >               rzg3s_pcie_teardown_msi(host);
> >
> > +     rzg3s_pcie_clear_intx_handlers(host, PCI_NUM_INTX);
> > +
> >       irq_domain_remove(host->intx_domain);
>
> And use the same function here, replacing:
>
> -       rzg3s_pcie_clear_intx_handlers(host, PCI_NUM_INTX);
> -
> -       irq_domain_remove(host->intx_domain);
> +       rzg3s_pcie_teardown_intx(host, PCI_NUM_INTX);
>
> Btw, the order b/w rzg3s_pcie_clear_intx_heandlers() and irq_domain_remove() was
> revered here compared with the order from rzg3s_pcie_init_irqdomain().
I will do the below:

static void rzg3s_pcie_teardown_intx(struct rzg3s_pcie_host *host,
                     int count)
{
    while (--count >= 0) {
        irq_set_chained_handler_and_data(host->intx_irqs[count], NULL,
                         NULL);
    }

    if (host->intx_domain)
        irq_domain_remove(host->intx_domain);
}

Cheers,
Prabhakar