Re: [PATCH v6 2/2] PCI: amd-mdb: Add CPM6 host controller support
From: sashiko-bot@kernel.org
Date: 2026-08-19 17:32:45
Also in:
linux-pci
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] The shared event interrupt handler `amd_mdb_pcie_event` unconditionally returns `IRQ_HANDLED` and clears shared aggregators even when no events are pending, breaking spurious interrupt detection and potentially disrupting the peer host. - [High] The CPM6 hardware INTx enable bit is unconditionally unmasked during initialization and probe, completely defeating the newly added `intx_refmask` refcount logic and leaving the system vulnerable to unhandled interrupt storms. - [High] Probe error path and teardown destroy IRQ domains while devm-managed IRQs and mappings are still active, leading to leaked mappings, kernel warnings, and potential use-after-free via the shared IRQ line. - [Medium] Failing to disable hardware interrupts on an early probe error path leaves the shared line asserted, potentially causing an endless IRQ storm. --
--- Patch [2]: [PATCH v6 2/2] PCI: amd-mdb: Add CPM6 host controller support ---commit 35b39748e651a7f764a655c28114103b16705e80 Author: Sai Krishna Musham [off-list ref] Subject: PCI: amd-mdb: Add CPM6 host controller support This commit adds support for the AMD Versal2 CPM6 host controller variants. It introduces a new interrupt register layout and handles a shared GIC event interrupt line by clearing the controller's aggregator contribution.
quoted hunk ↗ jump to hunk
diff --git a/drivers/pci/controller/dwc/pcie-amd-mdb.c b/drivers/pci/controller/dwc/pcie-amd-mdb.c index dee295f281a30..4355fa6d48a39 100644 --- a/drivers/pci/controller/dwc/pcie-amd-mdb.c +++ b/drivers/pci/controller/dwc/pcie-amd-mdb.c
[ ... ]
quoted hunk ↗ jump to hunk
@@ -193,11 +285,10 @@ static void amd_mdb_event_irq_unmask(struct irq_data *d) struct dw_pcie *pci = &pcie->pci; struct dw_pcie_rp *port = &pci->pp; unsigned long flags; - u32 val; raw_spin_lock_irqsave(&port->lock, flags); - val = BIT(d->hwirq); - writel_relaxed(val, pcie->slcr + AMD_MDB_TLP_IR_ENABLE_MISC); + writel_relaxed(BIT(d->hwirq), + pcie->intr_base + pcie->variant->misc_enable_reg); raw_spin_unlock_irqrestore(&port->lock, flags); }
[Severity: High] Does this unconditionally unmask the INTx enable bit in the hardware register? Since devm_request_irq() will call amd_mdb_event_irq_unmask(), it appears the parent IRQ subsystem will unconditionally write the hardware bit here. Would this completely defeat the intx_refmask refcount logic added in amd_mdb_intx_irq_unmask(), and leave the system vulnerable to an unhandled interrupt storm if a downstream device asserts INTx early? [ ... ]
quoted hunk ↗ jump to hunk
@@ -226,13 +317,30 @@ static irqreturn_t amd_mdb_pcie_event(int irq, void *args) { struct amd_mdb_pcie *pcie = args; unsigned long val; + u32 ev_raw; int i; - val = readl_relaxed(pcie->slcr + AMD_MDB_TLP_IR_STATUS_MISC); - val &= ~readl_relaxed(pcie->slcr + AMD_MDB_TLP_IR_MASK_MISC); + ev_raw = readl_relaxed(pcie->intr_base + pcie->variant->misc_status_reg); + val = ev_raw; + val &= ~readl_relaxed(pcie->intr_base + pcie->variant->misc_mask_reg);
[ ... ]
quoted hunk ↗ jump to hunk
+ /* Clear handled + any unhandled sticky bits to avoid IRQ storms. */ + writel_relaxed(ev_raw, pcie->intr_base + pcie->variant->misc_status_reg); + + /* Sticky aggregation bits; clear each pass or the IRQ re-fires */ + amd_mdb_pcie_clear_aggregators(pcie); return IRQ_HANDLED; }
[Severity: High] Could this function unconditionally return IRQ_HANDLED and clear the shared aggregators even when there are no events pending (when ev_raw == 0)? The IRQ_NONE return logic seems to have been placed in dw_pcie_rp_intx() instead of here. Does returning IRQ_HANDLED unconditionally break spurious interrupt detection for the shared line? Also, if the peer host triggered the interrupt, will this clear the shared AMD_CPM6_PS_MISC_IR_STATUS aggregator before the peer can process it? [ ... ]
quoted hunk ↗ jump to hunk
@@ -250,24 +358,59 @@ static void amd_mdb_pcie_free_irq_domains(struct amd_mdb_pcie *pcie)
[ ... ]
quoted hunk ↗ jump to hunk
+static void amd_mdb_pcie_init_port(struct amd_mdb_pcie *pcie) +{ + u32 misc_mask_all; + u32 val; + + misc_mask_all = pcie->variant->misc_mask_all; /* Disable all TLP interrupts. */ - writel_relaxed(AMD_MDB_PCIE_IMR_ALL_MASK, - pcie->slcr + AMD_MDB_TLP_IR_DISABLE_MISC); + writel_relaxed(misc_mask_all, + pcie->intr_base + pcie->variant->misc_disable_reg); /* Clear pending TLP interrupts. */ - val = readl_relaxed(pcie->slcr + AMD_MDB_TLP_IR_STATUS_MISC); - val &= AMD_MDB_PCIE_IMR_ALL_MASK; - writel_relaxed(val, pcie->slcr + AMD_MDB_TLP_IR_STATUS_MISC); + val = readl_relaxed(pcie->intr_base + pcie->variant->misc_status_reg) & + misc_mask_all; + writel_relaxed(val, pcie->intr_base + pcie->variant->misc_status_reg); /* Enable all TLP interrupts. */ - writel_relaxed(AMD_MDB_PCIE_IMR_ALL_MASK, - pcie->slcr + AMD_MDB_TLP_IR_ENABLE_MISC); + writel_relaxed(misc_mask_all, + pcie->intr_base + pcie->variant->misc_enable_reg);
[Severity: High] Does this unconditionally write misc_mask_all (which contains the INTx enable bit) to the hardware enable register? Similar to the issue in amd_mdb_event_irq_unmask(), does this bypass the dynamic intx_refmask refcounting logic intended for the child domain? [ ... ]
quoted hunk ↗ jump to hunk
@@ -435,9 +601,36 @@ static int amd_mdb_add_pcie_port(struct amd_mdb_pcie *pcie, struct device *dev = &pdev->dev; int err; - pcie->slcr = devm_platform_ioremap_resource_byname(pdev, "slcr"); - if (IS_ERR(pcie->slcr)) - return PTR_ERR(pcie->slcr); + if (pcie->variant->version == MDB5) {
[ ... ]
quoted hunk ↗ jump to hunk
+ pcie->intr_base = devm_platform_ioremap_resource_byname(pdev, "intr"); + if (IS_ERR(pcie->intr_base)) + return PTR_ERR(pcie->intr_base); + } err = amd_mdb_pcie_init_irq_domains(pcie, pdev); if (err) - return err;
[Severity: Medium] If amd_mdb_pcie_init_irq_domains() fails, this returns the error directly and bypasses the out label. Does this skip the newly added amd_mdb_pcie_disable_interrupts() call, potentially leaving the shared line asserted and causing an interrupt storm? [ ... ]
quoted hunk ↗ jump to hunk
@@ -466,6 +659,11 @@ static int amd_mdb_add_pcie_port(struct amd_mdb_pcie *pcie, return 0; out: + /* + * Mask the hardware interrupts before tearing down so a stale source + * cannot storm the shared CPM6 GIC line once this host is unwound. + */ + amd_mdb_pcie_disable_interrupts(pcie); amd_mdb_pcie_free_irq_domains(pcie); return err; }
[Severity: High] Does this error cleanup path destroy the IRQ domains while the devm-managed IRQs and mappings are still active? Because devres frees the requested handlers after the probe function returns, the newly shared IRQ handler remains active while pcie->mdb_domain is NULL. Can an asynchronous interrupt from the peer controller hitting the handler during this window cause a NULL pointer dereference? Should the mappings be disposed of via irq_dispose_mapping() or the handlers freed explicitly before destroying the domains? -- Sashiko AI review · https://sashiko.dev/#/patchset/20260819172057.3636214-1-sai.krishna.musham@amd.com?part=2