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

[claudiu beznea <claudiu.beznea@tuxon.dev>]

Hi, Prabhakar,

On 7/16/26 01:30, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> 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@vger.kernel.org
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
>   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);
	}
}

> +
>   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.

Thank you,
Claudiu

> +	rzg3s_pcie_clear_intx_handlers(host, i);
> +
> +	return ret;
>   }
>   
>   static void rzg3s_pcie_teardown_irqdomain(struct rzg3s_pcie_host *host)
> @@ -1055,6 +1070,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);
>   }
>