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

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:

void rzg3s_pcie_teardown_intx(struct rzg3s_pcie_host *host, int count)
{
	if (host->intx_domain)
		irq_domain_remove(host->intx_domain);	


	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().
Thank you,
Claudiu