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

[sashiko-bot@kernel.org]

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 <sai.krishna.musham@amd.com>
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.

> 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

[ ... ]

> @@ -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?

[ ... ]

> @@ -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);

[ ... ]

> +	/* 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?

[ ... ]

> @@ -250,24 +358,59 @@ static void amd_mdb_pcie_free_irq_domains(struct amd_mdb_pcie *pcie)

[ ... ]

> +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?

[ ... ]

> @@ -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) {

[ ... ]

> +		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?

[ ... ]

> @@ -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