[PATCH v2 0/2] PCI: altera: Fix IRQ cleanup on probe failure

[PATCH v2 0/2] PCI: altera: Fix IRQ cleanup on probe failure

[Mahesh Vaidya]

This series addresses review feedback from v1 of the Altera PCIe probe
failure cleanup fix.

Patch 1 removes irq_dispose_mapping(pcie->irq) from the IRQ teardown
path. pcie->irq is the parent IRQ returned by platform_get_irq(), not an
IRQ created by the Altera INTx irq_domain, so the driver should detach
the chained handler but not dispose the parent IRQ mapping.

Patch 2 fixes the original probe failure issue. The chained handler is
now installed only after the INTx domain is created, controller interrupts
are disabled during teardown, and the IRQ setup is torn down if
pci_host_probe() fails.

Tested on Agilex 7 and Stratix 10:
- Boot and fio read/write through a PCIe endpoint.
- Probe-failure cleanup path by injecting a failure before
  pci_host_probe().

Changes since v1:
- Removed irq_dispose_mapping(pcie->irq), since pcie->irq is the parent
  IRQ returned by platform_get_irq().
- Added controller interrupt disable helper.
- Disabled controller interrupts before tearing down the chained handler
  and INTx domain.
- Reused the teardown path when pci_host_probe() fails.

v1:
https://lore.kernel.org/linux-pci/20260427175302.570671-1-mahesh.vaidya@altera.com/

Mahesh Vaidya (2):
  PCI: altera: Do not dispose parent IRQ mapping
  PCI: altera: Fix resource leaks on probe failure

 drivers/pci/controller/pcie-altera.c | 36 +++++++++++++++++++++++++---
 1 file changed, 33 insertions(+), 3 deletions(-)


base-commit: 4224e91fea5695a89843b4c38283016616946307
-- 
2.34.1

[PATCH v2 1/2] PCI: altera: Do not dispose parent IRQ mapping

[Mahesh Vaidya]

altera_pcie_irq_teardown() calls irq_dispose_mapping() on pcie->irq.
However, pcie->irq is the parent IRQ returned by platform_get_irq(), not
an IRQ mapping created by the Altera INTx irq_domain.

The Altera driver only installs a chained handler on the parent IRQ. It
should detach that handler during teardown, but it should not dispose the
parent IRQ mapping, which belongs to the parent interrupt controller's
irq_domain.

Drop irq_dispose_mapping(pcie->irq) from the teardown path.

Fixes: ec15c4d0d5d2 ("PCI: altera: Allow building as module")
Cc: stable@vger.kernel.org
Reviewed-by: Subhransu S. Prusty <subhransu.sekhar.prusty@altera.com>
Signed-off-by: Mahesh Vaidya <mahesh.vaidya@altera.com>
---
 drivers/pci/controller/pcie-altera.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/pci/controller/pcie-altera.c b/drivers/pci/controller/pcie-altera.c
index 3dbb7adc421c..3d3519b8d88f 100644
--- a/drivers/pci/controller/pcie-altera.c
+++ b/drivers/pci/controller/pcie-altera.c
@@ -868,7 +868,6 @@ static void altera_pcie_irq_teardown(struct altera_pcie *pcie)
 {
 	irq_set_chained_handler_and_data(pcie->irq, NULL, NULL);
 	irq_domain_remove(pcie->irq_domain);
-	irq_dispose_mapping(pcie->irq);
 }
 
 static int altera_pcie_parse_dt(struct altera_pcie *pcie)
-- 
2.34.1

[PATCH v2 2/2] PCI: altera: Fix resource leaks on probe failure

[Mahesh Vaidya]

The chained IRQ handler is installed during probe but is only removed
from the remove path. If pci_host_probe() fails, the handler and INTx IRQ
domain remain installed even though the devm-managed host bridge storage
containing struct altera_pcie will be released, leaving the handler with
a stale data pointer.

Interrupts are also enabled before pci_host_probe() is called. If probe
fails after that point, the controller interrupt source should be disabled
before the chained handler and INTx domain are removed.

Install the chained handler only after the INTx domain has been created.
Disable controller interrupts during IRQ teardown, and tear the IRQ setup
down if pci_host_probe() fails.

Fixes: c63aed7334c2 ("PCI: altera: Use pci_host_probe() to register host")
Cc: stable@vger.kernel.org
Reviewed-by: Subhransu S. Prusty <subhransu.sekhar.prusty@altera.com>
Signed-off-by: Mahesh Vaidya <mahesh.vaidya@altera.com>
---
 drivers/pci/controller/pcie-altera.c | 35 ++++++++++++++++++++++++++--
 1 file changed, 33 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/pcie-altera.c b/drivers/pci/controller/pcie-altera.c
index 3d3519b8d88f..902ae2d81763 100644
--- a/drivers/pci/controller/pcie-altera.c
+++ b/drivers/pci/controller/pcie-altera.c
@@ -864,8 +864,23 @@ static int altera_pcie_init_irq_domain(struct altera_pcie *pcie)
 	return 0;
 }
 
+static void altera_pcie_disable_irq(struct altera_pcie *pcie)
+{
+	if (pcie->pcie_data->version == ALTERA_PCIE_V1 ||
+	    pcie->pcie_data->version == ALTERA_PCIE_V2) {
+		/* Disable all P2A interrupts */
+		cra_writel(pcie, 0, P2A_INT_ENABLE);
+	} else if (pcie->pcie_data->version == ALTERA_PCIE_V3) {
+		/* Disable port-level interrupts (CFG_AER, etc.) */
+		writel(0, pcie->hip_base +
+			  pcie->pcie_data->port_conf_offset +
+			  pcie->pcie_data->port_irq_enable_offset);
+	}
+}
+
 static void altera_pcie_irq_teardown(struct altera_pcie *pcie)
 {
+	altera_pcie_disable_irq(pcie);
 	irq_set_chained_handler_and_data(pcie->irq, NULL, NULL);
 	irq_domain_remove(pcie->irq_domain);
 }
@@ -890,7 +905,6 @@ static int altera_pcie_parse_dt(struct altera_pcie *pcie)
 	if (pcie->irq < 0)
 		return pcie->irq;
 
-	irq_set_chained_handler_and_data(pcie->irq, pcie->pcie_data->ops->rp_isr, pcie);
 	return 0;
 }
 
@@ -1019,6 +1033,14 @@ static int altera_pcie_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	/*
+	 * The chained handler uses pcie->irq_domain, so install it
+	 * only after the INTx domain has been created.
+	 */
+	irq_set_chained_handler_and_data(pcie->irq,
+					 pcie->pcie_data->ops->rp_isr,
+					 pcie);
+
 	if (pcie->pcie_data->version == ALTERA_PCIE_V1 ||
 	    pcie->pcie_data->version == ALTERA_PCIE_V2) {
 		/* clear all interrupts */
@@ -1036,7 +1058,16 @@ static int altera_pcie_probe(struct platform_device *pdev)
 	bridge->busnr = pcie->root_bus_nr;
 	bridge->ops = &altera_pcie_ops;
 
-	return pci_host_probe(bridge);
+	ret = pci_host_probe(bridge);
+	if (ret)
+		goto err_teardown_irq;
+
+	return 0;
+
+err_teardown_irq:
+	altera_pcie_irq_teardown(pcie);
+
+	return ret;
 }
 
 static void altera_pcie_remove(struct platform_device *pdev)
-- 
2.34.1

Re: [PATCH v2 0/2] PCI: altera: Fix IRQ cleanup on probe failure

[Manivannan Sadhasivam]

On Thu, 30 Apr 2026 13:43:28 -0700, Mahesh Vaidya wrote:
> This series addresses review feedback from v1 of the Altera PCIe probe
> failure cleanup fix.
> 
> Patch 1 removes irq_dispose_mapping(pcie->irq) from the IRQ teardown
> path. pcie->irq is the parent IRQ returned by platform_get_irq(), not an
> IRQ created by the Altera INTx irq_domain, so the driver should detach
> the chained handler but not dispose the parent IRQ mapping.
> 
> [...]

Applied, thanks!

[1/2] PCI: altera: Do not dispose parent IRQ mapping
      commit: 5ef4bac02189bee0b7c170e352d7a38e13fe9678
[2/2] PCI: altera: Fix resource leaks on probe failure
      commit: 7a94138caeb27f3c49c1dbd93bf422098925bb28

Best regards,
-- 
Manivannan Sadhasivam <mani@kernel.org>

Re: [PATCH v2 2/2] PCI: altera: Fix resource leaks on probe failure

[Bjorn Helgaas]

On Thu, Apr 30, 2026 at 01:43:30PM -0700, Mahesh Vaidya wrote:
> The chained IRQ handler is installed during probe but is only removed
> from the remove path. If pci_host_probe() fails, the handler and INTx IRQ
> domain remain installed even though the devm-managed host bridge storage
> containing struct altera_pcie will be released, leaving the handler with
> a stale data pointer.
> 
> Interrupts are also enabled before pci_host_probe() is called. If probe
> fails after that point, the controller interrupt source should be disabled
> before the chained handler and INTx domain are removed.
> 
> Install the chained handler only after the INTx domain has been created.
> Disable controller interrupts during IRQ teardown, and tear the IRQ setup
> down if pci_host_probe() fails.
> 
> Fixes: c63aed7334c2 ("PCI: altera: Use pci_host_probe() to register host")
> Cc: stable@vger.kernel.org
> Reviewed-by: Subhransu S. Prusty <subhransu.sekhar.prusty@altera.com>
> Signed-off-by: Mahesh Vaidya <mahesh.vaidya@altera.com>
> ---
>  drivers/pci/controller/pcie-altera.c | 35 ++++++++++++++++++++++++++--
>  1 file changed, 33 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-altera.c b/drivers/pci/controller/pcie-altera.c
> index 3d3519b8d88f..902ae2d81763 100644
> --- a/drivers/pci/controller/pcie-altera.c
> +++ b/drivers/pci/controller/pcie-altera.c
> @@ -864,8 +864,23 @@ static int altera_pcie_init_irq_domain(struct altera_pcie *pcie)
>  	return 0;
>  }
>  
> +static void altera_pcie_disable_irq(struct altera_pcie *pcie)
> +{
> +	if (pcie->pcie_data->version == ALTERA_PCIE_V1 ||
> +	    pcie->pcie_data->version == ALTERA_PCIE_V2) {
> +		/* Disable all P2A interrupts */
> +		cra_writel(pcie, 0, P2A_INT_ENABLE);
> +	} else if (pcie->pcie_data->version == ALTERA_PCIE_V3) {
> +		/* Disable port-level interrupts (CFG_AER, etc.) */
> +		writel(0, pcie->hip_base +
> +			  pcie->pcie_data->port_conf_offset +
> +			  pcie->pcie_data->port_irq_enable_offset);
> +	}
> +}
> +
>  static void altera_pcie_irq_teardown(struct altera_pcie *pcie)
>  {
> +	altera_pcie_disable_irq(pcie);
>  	irq_set_chained_handler_and_data(pcie->irq, NULL, NULL);
>  	irq_domain_remove(pcie->irq_domain);
>  }

This patch appeared in v7.2 as 7a94138caeb2 ("PCI: altera: Fix
resource leaks on probe failure").

While considering this for backporting, sashiko came up with the
questions below.  Can you take a look and see if they make sense?

  This is a pre-existing issue, but does altera_pcie_irq_teardown()
  lack synchronization with in-flight interrupts?

  If a device interrupt fires just before altera_pcie_disable_irq()
  masks it, could the chained ISR (altera_pcie_isr) execute
  concurrently with altera_pcie_irq_teardown() on another CPU?

  Because irq_set_chained_handler_and_data() unregisters the handler
  but does not wait for currently executing ISRs on other CPUs, could
  irq_domain_remove() free the domain while the ISR is still using it
  to call generic_handle_domain_irq(), leading to a use-after-free and
  a kernel panic?

  Would adding a call to synchronize_irq() before removing the IRQ
  domain prevent this race during driver removal or when
  pci_host_probe() fails?

Re: [PATCH v2 2/2] PCI: altera: Fix resource leaks on probe failure

[Dinh Nguyen]

On 9/3/26 01:56, Bjorn Helgaas wrote:
> On Thu, Apr 30, 2026 at 01:43:30PM -0700, Mahesh Vaidya wrote:
>> The chained IRQ handler is installed during probe but is only removed
>> from the remove path. If pci_host_probe() fails, the handler and INTx IRQ
>> domain remain installed even though the devm-managed host bridge storage
>> containing struct altera_pcie will be released, leaving the handler with
>> a stale data pointer.
>>
>> Interrupts are also enabled before pci_host_probe() is called. If probe
>> fails after that point, the controller interrupt source should be disabled
>> before the chained handler and INTx domain are removed.
>>
>> Install the chained handler only after the INTx domain has been created.
>> Disable controller interrupts during IRQ teardown, and tear the IRQ setup
>> down if pci_host_probe() fails.
>>
>> Fixes: c63aed7334c2 ("PCI: altera: Use pci_host_probe() to register host")
>> Cc: stable@vger.kernel.org
>> Reviewed-by: Subhransu S. Prusty <subhransu.sekhar.prusty@altera.com>
>> Signed-off-by: Mahesh Vaidya <mahesh.vaidya@altera.com>
>> ---
>>   drivers/pci/controller/pcie-altera.c | 35 ++++++++++++++++++++++++++--
>>   1 file changed, 33 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/pci/controller/pcie-altera.c b/drivers/pci/controller/pcie-altera.c
>> index 3d3519b8d88f..902ae2d81763 100644
>> --- a/drivers/pci/controller/pcie-altera.c
>> +++ b/drivers/pci/controller/pcie-altera.c
>> @@ -864,8 +864,23 @@ static int altera_pcie_init_irq_domain(struct altera_pcie *pcie)
>>   	return 0;
>>   }
>>   
>> +static void altera_pcie_disable_irq(struct altera_pcie *pcie)
>> +{
>> +	if (pcie->pcie_data->version == ALTERA_PCIE_V1 ||
>> +	    pcie->pcie_data->version == ALTERA_PCIE_V2) {
>> +		/* Disable all P2A interrupts */
>> +		cra_writel(pcie, 0, P2A_INT_ENABLE);
>> +	} else if (pcie->pcie_data->version == ALTERA_PCIE_V3) {
>> +		/* Disable port-level interrupts (CFG_AER, etc.) */
>> +		writel(0, pcie->hip_base +
>> +			  pcie->pcie_data->port_conf_offset +
>> +			  pcie->pcie_data->port_irq_enable_offset);
>> +	}
>> +}
>> +
>>   static void altera_pcie_irq_teardown(struct altera_pcie *pcie)
>>   {
>> +	altera_pcie_disable_irq(pcie);
>>   	irq_set_chained_handler_and_data(pcie->irq, NULL, NULL);
>>   	irq_domain_remove(pcie->irq_domain);
>>   }
> 
> This patch appeared in v7.2 as 7a94138caeb2 ("PCI: altera: Fix
> resource leaks on probe failure").
> 
> While considering this for backporting, sashiko came up with the
> questions below.  Can you take a look and see if they make sense?
> 
>    This is a pre-existing issue, but does altera_pcie_irq_teardown()
>    lack synchronization with in-flight interrupts?
> 
>    If a device interrupt fires just before altera_pcie_disable_irq()
>    masks it, could the chained ISR (altera_pcie_isr) execute
>    concurrently with altera_pcie_irq_teardown() on another CPU?
> 
>    Because irq_set_chained_handler_and_data() unregisters the handler
>    but does not wait for currently executing ISRs on other CPUs, could
>    irq_domain_remove() free the domain while the ISR is still using it
>    to call generic_handle_domain_irq(), leading to a use-after-free and
>    a kernel panic?
> 
>    Would adding a call to synchronize_irq() before removing the IRQ
>    domain prevent this race during driver removal or when
>    pci_host_probe() fails?

In this case, because the driver uses chained interrupts, 
synchronize_irq() is not effective.

Chained handlers execute directly via desc->handle_irq() and bypass the
generic handle_irq_event() function, which means the IRQD_IRQ_INPROGRESS
flag is never set for them. synchronize_irq() relies on this flag, so it 
will return immediately, effectively being a no-op.

A correct fix for this potential use-after-free would be to convert the 
driver to use a normal requested IRQ instead of the chained handler. 
I'll work on a patch for that.


Thanks,
Dinh