Re: [PATCH v4 2/2] PCI: amd-mdb: Add CPM6 host controller support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [High] The shared event IRQ handler unconditionally returns IRQ_HANDLED and clears shared aggregators, breaking spurious interrupt detection.
- [High] Probe error paths fail to disable hardware interrupts, leading to unrecoverable IRQ storms on the shared interrupt line.
- [High] Hardware interrupts are unmasked before the interrupt handler is registered, creating a race window for an IRQ storm.

Pre-existing issues:
- [High] Probe failure cleanup frees IRQ domains while devm-managed handlers are still active, leading to Use-After-Free.
- [High] MDB5 INTB, INTC, and INTD interrupts trigger a WARN_ON_ONCE due to attempting to dispatch unmapped hwirqs.
--

--- Patch [2]: [PATCH v4 2/2] PCI: amd-mdb: Add CPM6 host controller support ---
commit b10c5fc8b35403027eb649f10e64e4ddee1b0920
Author: Sai Krishna Musham <sai.krishna.musham@amd.com>
Subject: PCI: amd-mdb: Add CPM6 host controller support

This commit extends the AMD MDB PCIe driver to support AMD Versal2 CPM6
variants. It adds variant-specific register layouts and implements
aggregator clearing for the new shared interrupt architecture.

Link: https://lore.kernel.org/all/20260402180006.486229-3-sai.krishna.musham@amd.com/
Link: https://lore.kernel.org/all/20260728202044.1785986-3-sai.krishna.musham@amd.com/
Link: https://lore.kernel.org/all/20260803144412.713639-3-sai.krishna.musham@amd.com/

> diff --git a/drivers/pci/controller/dwc/pcie-amd-mdb.c b/drivers/pci/controller/dwc/pcie-amd-mdb.c
> index dee295f281a30..3e8e02ab96bce 100644
> --- a/drivers/pci/controller/dwc/pcie-amd-mdb.c
> +++ b/drivers/pci/controller/dwc/pcie-amd-mdb.c
[ ... ]
> @@ -226,13 +387,44 @@ static irqreturn_t amd_mdb_pcie_event(int irq, void *args)
>  {
>  	struct amd_mdb_pcie *pcie = args;
>  	unsigned long val;
> +	u32 ev_raw, err;
>  	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);
> +
> +	if (pcie->variant->version == MDB5) {
> +		for_each_set_bit(i, &val, 32)
> +			generic_handle_domain_irq(pcie->mdb_domain, i);

[Severity: High]
This isn't a bug introduced by this patch, but does this code cause a
WARN_ON_ONCE if INTB, INTC, or INTD is asserted on MDB5?

The mask AMD_MDB_TLP_PCIE_INTX_MASK covers bits 16 to 23 of misc_status_reg.
This loop dispatches all set bits to generic_handle_domain_irq(). However,
in amd_mdb_setup_irq(), only INTA (hwirq 16) is mapped via
irq_create_mapping().

If INTB (bit 18) asserts, the handler will dispatch hwirq 18, which is
unmapped, failing to invoke the handler and triggering a warning in the IRQ
core.

> +		writel_relaxed(val, pcie->intr_base + pcie->variant->misc_status_reg);
> +		return IRQ_HANDLED;
> +	}
> +
> +	err = readl_relaxed(pcie->intr_base + AMD_CPM6_PCIE_ERR_STATUS);
> +
> +	val &= pcie->variant->misc_mask_all;
> +
>  	for_each_set_bit(i, &val, 32)
>  		generic_handle_domain_irq(pcie->mdb_domain, i);
> -	writel_relaxed(val, pcie->slcr + AMD_MDB_TLP_IR_STATUS_MISC);
> +
> +	/* Clear handled + any unhandled sticky bits to avoid IRQ storms. */
> +	writel_relaxed(ev_raw, pcie->intr_base + pcie->variant->misc_status_reg);
> +
> +	/* On CPM6 completion timeout is reported via PCIE_ERR. */
> +	if (err) {
> +		u32 pending = err & ~readl_relaxed(pcie->intr_base +
> +						   AMD_CPM6_PCIE_ERR_MASK);
> +
> +		if (pending & AMD_CPM6_PCIE_ERR_MASK_ALL)
> +			generic_handle_domain_irq(pcie->mdb_domain,
> +						  AMD_CPM6_PCIE_INTR_CMPL_TIMEOUT);
> +		/* Clear every asserted bit so the leaf and MERGED de-assert. */
> +		writel_relaxed(err, pcie->intr_base + AMD_CPM6_PCIE_ERR_STATUS);
> +	}
> +
> +	/* Sticky aggregation bits; clear each pass or the IRQ re-fires */
> +	amd_mdb_pcie_clear_aggregators(pcie);
>  
>  	return IRQ_HANDLED;
>  }

[Severity: High]
Could returning IRQ_HANDLED unconditionally here break the kernel's spurious
interrupt detection?

Since this IRQ is shared (as seen by the IRQF_SHARED flag), any interrupt on
the shared line invokes this handler. If the interrupt was generated by the
other host controller and this one is idle (ev_raw == 0 and err == 0), this
code returns IRQ_HANDLED without doing any work.

Additionally, unconditionally calling amd_mdb_pcie_clear_aggregators() clears
the shared PS_MISC_IR_STATUS aggregator, which could drop interrupts for the
other host controller.

[ ... ]
> @@ -351,15 +569,20 @@ static int amd_mdb_setup_irq(struct amd_mdb_pcie *pcie,
>  	struct dw_pcie *pci = &pcie->pci;
>  	struct dw_pcie_rp *pp = &pci->pp;
>  	struct device *dev = &pdev->dev;
> +	const struct amd_mdb_intr_cause *intr_cause;
> +	unsigned long event_flags = IRQF_NO_THREAD;
>  	int i, irq, err;
>  
> +	intr_cause = pcie->variant->version == MDB5 ?
> +		     mdb5_intr_cause : cpm6_intr_cause;
> +
>  	amd_mdb_pcie_init_port(pcie);

[Severity: High]
Is there a race condition leading to an IRQ storm by enabling hardware
interrupts before registering the handler?

amd_mdb_pcie_init_port() enables and unmasks hardware interrupts and shared
aggregators. The devm_request_irq() for the handler happens later in this
function.

For CPM6's shared IRQ, if the hardware asserts an interrupt during this window,
the shared line fires. The other host's handler will spin endlessly because it
cannot clear this un-probed host's internal hardware state.

>  
>  	pp->irq = platform_get_irq(pdev, 0);
>  	if (pp->irq < 0)
>  		return pp->irq;
[ ... ]
> +	/*
> +	 * On CPM6 the misc GIC line is shared between both host controllers,
> +	 * so the event IRQ must allow sharing.
> +	 */
> +	if (pcie->variant->version != MDB5)
> +		event_flags |= IRQF_SHARED;
> +
>  	/* Plug the main event handler. */
> -	err = devm_request_irq(dev, pp->irq, amd_mdb_pcie_event, IRQF_NO_THREAD,
> +	err = devm_request_irq(dev, pp->irq, amd_mdb_pcie_event, event_flags,
>  			       "amd_mdb pcie_irq", pcie);
>  	if (err) {
>  		dev_err(dev, "Failed to request event IRQ %d, err=%d\n",
> -			pp->irq, err);
> +			pp->irq, err);
>  		return err;
>  	}

[Severity: High]
Will the probe error path leave hardware interrupts enabled and unmasked?

If a failure occurs after this point (e.g., in dw_pcie_host_init()), the error
path in amd_mdb_add_pcie_port() only cleans up the IRQ domains. It fails to
mask the hardware interrupts and shared aggregators that were just enabled in
amd_mdb_pcie_init_port().

Because the IRQ is shared on CPM6, if the uninitialized hardware asserts an
interrupt, the other host controller's handler will be continuously invoked but
unable to clear the source, causing an unrecoverable IRQ storm.

[Severity: High]
This isn't a bug introduced by this patch, but can the probe error path cause
a use-after-free of the IRQ domain?

If a failure occurs after devm_request_irq() succeeds, amd_mdb_add_pcie_port()
jumps to its error path and calls amd_mdb_pcie_free_irq_domains(), which
immediately destroys and frees pcie->mdb_domain. However, the IRQ handlers
were registered with devm-managed functions and remain active until devres
cleanup runs after the probe function returns.

If an interrupt fires in this window (which is likely if hardware interrupts
are left enabled), amd_mdb_pcie_event() will dereference the freed domain via:

amd_mdb_pcie_event() {
    ...
    generic_handle_domain_irq(pcie->mdb_domain, i);
    ...
}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260808105256.3705889-1-sai.krishna.musham@amd.com?part=2