[PATCH] PCI: altera: Convert root port IRQ to a requested handler

[PATCH] PCI: altera: Convert root port IRQ to a requested handler

[Mahesh Vaidya]

altera_pcie_irq_teardown() removes the chained handler before removing
the INTx domain. Removing the handler prevents new invocations, but does
not wait for one already running on another CPU. Such a handler can
therefore access pcie or pcie->irq_domain after they have been freed.

Chained handlers bypass handle_irq_event(), so synchronize_irq() cannot
rely on IRQD_IRQ_INPROGRESS and may return without waiting if the parent
irqchip does not report IRQCHIP_STATE_ACTIVE.

Convert the chained handler to a requested hard IRQ. Request it only
after creating the INTx domain and call free_irq() before removing the
domain. free_irq() then synchronizes against any in-flight handler.

Use IRQF_NO_THREAD because generic_handle_domain_irq() requires hardirq
context. Use IRQF_NO_SUSPEND to preserve the suspend behavior of the
previous chained cascade. Request and free the IRQ explicitly because
automatic devres cleanup happens after remove() returns.

Fixes: ec15c4d0d5d2 ("PCI: altera: Allow building as module")
Reported-by: Bjorn Helgaas <bhelgaas@google.com>
Link: https://lore.kernel.org/linux-pci/20260430204330.3121003-3-mahesh.vaidya@altera.com/
Suggested-by: Dinh Nguyen <dinguyen@kernel.org>
Assisted-by: ChatGPT:GPT-5
Signed-off-by: Mahesh Vaidya <mahesh.vaidya@altera.com>
---
This converts the driver as Dinh proposed in [1], where sashiko raised
the use-after-free while Bjorn was considering 7a94138caeb2 ("PCI:
altera: Fix resource leaks on probe failure") for backport. Dinh offered
to write the conversion there; we agreed I would send it, since I have
V2 and V3 hardware to test it on.

[1] https://lore.kernel.org/linux-pci/20260430204330.3121003-3-mahesh.vaidya@altera.com/

Note for reviewers, on IRQF_NO_SUSPEND:

Converting the parent from a chained handler to a requested one changes
its suspend behavior, because suspend_device_irqs() skips chained
descriptors. I used IRQF_NO_SUSPEND to preserve the existing behavior
rather than change it in a fix. I would appreciate confirmation from the
IRQ maintainers that this is the right call. The alternative is to omit
the flag and accept the behavior change.

Tested on Stratix 10 (V2) with an igb-driven Ethernet endpoint using
INTx and on Agilex 7 (V3, M-series R-tile) with an NVMe endpoint using
INTx. On both, INTx delivery remained correct under sustained load with
no spurious interrupts reported, and repeated driver bind/unbind cycles
with interrupts in flight produced no warnings and no crashes. V1 shares
altera_pcie_isr() with V2, so the converted handler is identical there,
but I have no Cyclone V hardware to confirm it.

On backporting: I have not added Cc: stable because the conversion
affects all three supported generations and V1 remains untested. If you
would prefer this to accompany 7a94138caeb2 into stable, I can send
targeted backports.

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

diff --git a/drivers/pci/controller/pcie-altera.c b/drivers/pci/controller/pcie-altera.c
index 8ef56e2fabcf..c5a67aa3be79 100644
--- a/drivers/pci/controller/pcie-altera.c
+++ b/drivers/pci/controller/pcie-altera.c
@@ -9,7 +9,7 @@
 #include <linux/bitfield.h>
 #include <linux/delay.h>
 #include <linux/interrupt.h>
-#include <linux/irqchip/chained_irq.h>
+#include <linux/irq.h>
 #include <linux/irqdomain.h>
 #include <linux/init.h>
 #include <linux/module.h>
@@ -123,7 +123,7 @@ struct altera_pcie_ops {
 			   unsigned int devfn, int where, int size, u32 *value);
 	int (*ep_write_cfg)(struct altera_pcie *pcie, u8 busno,
 			    unsigned int devfn, int where, int size, u32 value);
-	void (*rp_isr)(struct irq_desc *desc);
+	irqreturn_t (*rp_isr)(int irq, void *data);
 };
 
 struct altera_pcie_data {
@@ -796,19 +796,15 @@ static const struct irq_domain_ops intx_domain_ops = {
 	.xlate = pci_irqd_intx_xlate,
 };
 
-static void altera_pcie_isr(struct irq_desc *desc)
+static irqreturn_t altera_pcie_isr(int irq, void *data)
 {
-	struct irq_chip *chip = irq_desc_get_chip(desc);
-	struct altera_pcie *pcie;
-	struct device *dev;
+	struct altera_pcie *pcie = data;
+	struct device *dev = &pcie->pdev->dev;
+	irqreturn_t handled = IRQ_NONE;
 	unsigned long status;
 	u32 bit;
 	int ret;
 
-	chained_irq_enter(chip, desc);
-	pcie = irq_desc_get_handler_data(desc);
-	dev = &pcie->pdev->dev;
-
 	while ((status = cra_readl(pcie, P2A_INT_STATUS)
 		& P2A_INT_STS_ALL) != 0) {
 		for_each_set_bit(bit, &status, PCI_NUM_INTX) {
@@ -819,34 +815,33 @@ static void altera_pcie_isr(struct irq_desc *desc)
 			if (ret)
 				dev_err_ratelimited(dev, "unexpected IRQ, INT%d\n", bit);
 		}
+		handled = IRQ_HANDLED;
 	}
-	chained_irq_exit(chip, desc);
+
+	return handled;
 }
 
-static void aglx_isr(struct irq_desc *desc)
+static irqreturn_t aglx_isr(int irq, void *data)
 {
-	struct irq_chip *chip = irq_desc_get_chip(desc);
-	struct altera_pcie *pcie;
-	struct device *dev;
+	struct altera_pcie *pcie = data;
+	struct device *dev = &pcie->pdev->dev;
 	u32 status;
 	int ret;
 
-	chained_irq_enter(chip, desc);
-	pcie = irq_desc_get_handler_data(desc);
-	dev = &pcie->pdev->dev;
-
 	status = readl(pcie->hip_base + pcie->pcie_data->port_conf_offset +
 		       pcie->pcie_data->port_irq_status_offset);
 
-	if (status & CFG_AER) {
-		writel(CFG_AER, (pcie->hip_base + pcie->pcie_data->port_conf_offset +
-				 pcie->pcie_data->port_irq_status_offset));
+	if (!(status & CFG_AER))
+		return IRQ_NONE;
 
-		ret = generic_handle_domain_irq(pcie->irq_domain, 0);
-		if (ret)
-			dev_err_ratelimited(dev, "unexpected IRQ %d\n", pcie->irq);
-	}
-	chained_irq_exit(chip, desc);
+	writel(CFG_AER, (pcie->hip_base + pcie->pcie_data->port_conf_offset +
+			 pcie->pcie_data->port_irq_status_offset));
+
+	ret = generic_handle_domain_irq(pcie->irq_domain, 0);
+	if (ret)
+		dev_err_ratelimited(dev, "unexpected IRQ %d\n", pcie->irq);
+
+	return IRQ_HANDLED;
 }
 
 static int altera_pcie_init_irq_domain(struct altera_pcie *pcie)
@@ -881,7 +876,12 @@ static void altera_pcie_disable_irq(struct altera_pcie *pcie)
 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);
+
+	/*
+	 * free_irq() waits for any in-flight parent handler, so it can no
+	 * longer dispatch through pcie->irq_domain below.
+	 */
+	free_irq(pcie->irq, pcie);
 	irq_domain_remove(pcie->irq_domain);
 }
 
@@ -1034,12 +1034,19 @@ static int altera_pcie_probe(struct platform_device *pdev)
 	}
 
 	/*
-	 * The chained handler uses pcie->irq_domain, so set it only after the
-	 * INTx domain has been created.
+	 * The handler demultiplexes into pcie->irq_domain, so request the IRQ
+	 * only after the INTx domain has been created. IRQF_NO_THREAD keeps it
+	 * in hardirq context, which generic_handle_domain_irq() requires, and
+	 * IRQF_NO_SUSPEND keeps the cascade available across suspend as the
+	 * chained handler was.
 	 */
-	irq_set_chained_handler_and_data(pcie->irq,
-					 pcie->pcie_data->ops->rp_isr,
-					 pcie);
+	ret = request_irq(pcie->irq, pcie->pcie_data->ops->rp_isr,
+			  IRQF_NO_THREAD | IRQF_NO_SUSPEND, dev_name(dev),
+			  pcie);
+	if (ret) {
+		dev_err(dev, "Failed to request IRQ %d\n", pcie->irq);
+		goto err_remove_domain;
+	}
 
 	if (pcie->pcie_data->version == ALTERA_PCIE_V1 ||
 	    pcie->pcie_data->version == ALTERA_PCIE_V2) {
@@ -1067,6 +1074,11 @@ static int altera_pcie_probe(struct platform_device *pdev)
 err_teardown_irq:
 	altera_pcie_irq_teardown(pcie);
 
+	return ret;
+
+err_remove_domain:
+	irq_domain_remove(pcie->irq_domain);
+
 	return ret;
 }
 
-- 
2.34.1

Re: [PATCH] PCI: altera: Convert root port IRQ to a requested handler

[sashiko-bot]

> altera_pcie_irq_teardown() removes the chained handler before removing
> the INTx domain. Removing the handler prevents new invocations, but does
> not wait for one already running on another CPU. Such a handler can
> therefore access pcie or pcie->irq_domain after they have been freed.
> 
> Chained handlers bypass handle_irq_event(), so synchronize_irq() cannot
> rely on IRQD_IRQ_INPROGRESS and may return without waiting if the parent
> irqchip does not report IRQCHIP_STATE_ACTIVE.
> 
> Convert the chained handler to a requested hard IRQ. Request it only
> after creating the INTx domain and call free_irq() before removing the
> domain. free_irq() then synchronizes against any in-flight handler.
> 
> Use IRQF_NO_THREAD because generic_handle_domain_irq() requires hardirq
> context. Use IRQF_NO_SUSPEND to preserve the suspend behavior of the
> previous chained cascade. Request and free the IRQ explicitly because
> automatic devres cleanup happens after remove() returns.
> 
> Fixes: ec15c4d0d5d2 ("PCI: altera: Allow building as module")
> Reported-by: Bjorn Helgaas <bhelgaas@google.com>
> Link: https://lore.kernel.org/linux-pci/20260430204330.3121003-3-mahesh.vaidya@altera.com/
> Suggested-by: Dinh Nguyen <dinguyen@kernel.org>
> Assisted-by: ChatGPT:GPT-5
> Signed-off-by: Mahesh Vaidya <mahesh.vaidya@altera.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261014100723.1643089-1-mahesh.vaidya@altera.com?part=1