RE: [PATCH v2 2/2] pci: AMD: Add CPM6 host controller support to AMD MDB PCIe driver

AMD General

> -----Original Message-----
> From: [email protected] <[email protected]>
> Sent: Wednesday, July 29, 2026 2:04 AM
> To: Musham, Sai Krishna <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]
> Subject: Re: [PATCH v2 2/2] pci: AMD: Add CPM6 host controller support to
> AMD MDB PCIe driver
>
> Thank you for your contribution! Sashiko AI review found 8 potential issue(s)
> to consider:
>
> New issues:
> - [High] Double-free and Use-After-Free of devm-managed interrupts on
> probe failure.
> - [High] Masking a single virtual INTx line on CPM6 unconditionally disables
> the aggregate hardware INTx interrupt, breaking other devices.
> - [High] Unhandled bits in `AMD_CPM6_PCIE_ERR_STATUS` are never cleared,
> leading to an unrecoverable infinite IRQ storm.
> - [High] Exclusive ioremap of the shared `slcr` region will cause the second
> CPM6 host controller to fail probing.
> - [High] Uncoordinated W1C clearing of the shared sticky aggregator bit
> causes lost interrupts for the other host.
>
> Pre-existing issues:
> - [High] INTxB, INTxC, and INTxD interrupts on MDB5 are silently dropped
> because their hardware IRQs are never mapped.
> - [High] Use-After-Free of irq_domain triggered by concurrent hardware
> interrupt during probe error path.
> - [High] Clearing sticky W1C status registers at the end of the interrupt handler
> creates a race condition that loses concurrent edges.
> --
>
> --- Patch [2]: [PATCH v2 2/2] pci: AMD: Add CPM6 host controller support to
> AMD MDB PCIe driver ---
> commit 72da33718ab9f6a5d8466cccef118aefc6c26f76
> Author: Sai Krishna Musham <[email protected]>
> Subject: pci: AMD: Add CPM6 host controller support to AMD MDB PCIe
> driver
>
> This patch extends the AMD MDB PCIe driver to support AMD Versal2 CPM6
> variants. It adds variant-specific register layouts and implements a shared
> aggregator clearing mechanism for the new interrupt architecture.
>
> Link: https://lore.kernel.org/all/20260402180006.486229-3-
> [email protected]/
>
> > diff --git a/drivers/pci/controller/dwc/pcie-amd-mdb.c
> b/drivers/pci/controller/dwc/pcie-amd-mdb.c
> > index dee295f281a30..dbf36ce9902c8 100644
> > --- a/drivers/pci/controller/dwc/pcie-amd-mdb.c
> > +++ b/drivers/pci/controller/dwc/pcie-amd-mdb.c
> [ ... ]
> > @@ -148,42 +287,34 @@ static void
> amd_mdb_pcie_clear_aggregators(struct amd_mdb_pcie *pcie)
> >     writel_relaxed(merged_mask, pcie->slcr +
> AMD_CPM6_MERGED_STATUS);
> >
> >     /*
> >      * Clear MERGED in the PS_MISC severity register so the misc GIC line
> >      * de-asserts.
> >      */
> >     writel_relaxed(AMD_CPM6_PS_IR_MERGED,
> >                    pcie->slcr + AMD_CPM6_PS_MISC_IR_STATUS);
> >  }
>
> [Severity: High]
> Does blindly clearing the shared sticky aggregator bit drop concurrent
> interrupts from the other host controller?
>
> The commit mentions this aggregator is shared between controllers. If Host 0
> clears this W1C bit while Host 1 has a pending unserviced interrupt in the
> MERGED register, Host 1's edge might be swallowed and never serviced.
>
> Does this need coordinated locking to ensure it is only cleared when no
> other hosts have pending interrupts?
>

No coordinated locking is needed. The W1C clears only this host's MERGED
Bits (HOST0: 13/14, HOST1: 16/17), the peer's bits are untouched.
PS_MISC bit 21 (merged_interrupts_0) is a sticky W1C aggregate of
MERGED_INTERRUPTS. where both hosts live; since the peer's MERGED
bits stay set, bit 21 re-asserts and the peer's interrupt still fires.

> [ ... ]
> > @@ -213,11 +237,15 @@ static void amd_mdb_intx_irq_mask(struct
> irq_data *data)
> >     if (pcie->variant->version == MDB5)
> >             val = FIELD_PREP(AMD_MDB_TLP_PCIE_INTX_MASK,
> >                              AMD_MDB_PCIE_INTR_INTX_ASSERT(data-
> >hwirq));
> >     else
> >             val = pcie->variant->intx_mask;
> >
> >     /*
> >      * Writing '1' to a bit in AMD_MDB_TLP_IR_DISABLE_MISC disables
> that
> >      * interrupt, writing '0' has no effect.
> >      */
> > -   writel_relaxed(val, pcie->slcr + AMD_MDB_TLP_IR_DISABLE_MISC);
> > +   writel_relaxed(val, pcie->intr_base + pcie->variant-
> >misc_disable_reg);
>
> [Severity: High]
> Will masking a single virtual INTx line on CPM6 unconditionally disable the
> aggregate hardware INTx interrupt?
>
> On CPM6, there appears to be only a single aggregate INTx hardware interrupt
> bit. If an endpoint driver unbinds or calls disable_irq() on INTA, this
> unconditionally writes the intx_mask bit to the disable register.
>
> Does this disable the hardware aggregate interrupt completely, potentially
> starving other endpoint devices relying on INTB, INTC, or INTD?
>
> [ ... ]
> > @@ -354,16 +375,19 @@ static irqreturn_t amd_mdb_pcie_event(int irq,
> void *args)
> >     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 is a pre-existing issue, but are INTxB, INTxC, and INTxD interrupts
> silently dropped on MDB5?
>
> When an INTx occurs, amd_mdb_pcie_event() extracts the asserted bit and
> calls generic_handle_domain_irq(). However, amd_mdb_setup_irq() only
> creates a mapping for the single intx_hwirq (INTxA).
>
> Bits corresponding to INTxB, INTxC, and INTxD are never mapped, so
> generic_handle_domain_irq() will return -EINVAL and the interrupts will be
> lost.
>
> >             writel_relaxed(val, pcie->intr_base + pcie->variant-
> >misc_status_reg);
> >             return IRQ_HANDLED;
> >     }
> >
> >     val &= pcie->variant->misc_mask_all;
> >
> >     for_each_set_bit(i, &val, 32)
> >             generic_handle_domain_irq(pcie->mdb_domain, i);
> >
> >     /* Clear handled + any unhandled sticky bits to avoid IRQ storms. */
> > -   writel_relaxed(val, pcie->slcr + AMD_MDB_TLP_IR_STATUS_MISC);
> > +   writel_relaxed(ev_raw, pcie->intr_base + pcie->variant-
> >misc_status_reg);
>
> [Severity: High]
> This isn't a bug introduced by this patch, but does clearing the sticky W1C
> status register at the end of the interrupt handler create a race condition?
>
> If a second edge of the same interrupt type occurs during the handling
> phase, writing ev_raw back to the W1C register at the very end will clear
> the new edge without it ever being processed.
>
> Should the status register be ACKed before handling the events so that new
> edges can latch correctly?
>

Agreed. This is pre-existing MDB5 behavior and ACK-before-handle is the right
hardening. I will send it as a separate patch (per Bjorn), rather than mixing it
with CPM6 support.

> > +   /* On CPM6 the completion timeout is reported via the PCIE_ERR
> register. */
> > +   err = readl_relaxed(pcie->intr_base +
> AMD_CPM6_PCIE_ERR_STATUS);
> > +   err &= ~readl_relaxed(pcie->intr_base +
> AMD_CPM6_PCIE_ERR_MASK);
> > +   err &= AMD_CPM6_PCIE_ERR_MASK_ALL;
> > +   if (err) {
> > +           dev_warn(pcie->pci.dev, "Completion timeout\n");
> > +           writel_relaxed(err, pcie->intr_base +
> AMD_CPM6_PCIE_ERR_STATUS);
> > +   }
>
> [Severity: High]
> Are unhandled bits in the PCIE_ERR_STATUS register masked out and never
> cleared?
>
> The driver masks the read value with AMD_CPM6_PCIE_ERR_MASK_ALL and
> only
> writes back the known bits to the W1C register. If any unhandled bit is
> set, it remains uncleared.
>
> Since this register feeds into the sticky MERGED aggregator, will this cause
> the hardware to immediately re-assert the main parent interrupt and create
> an unrecoverable IRQ storm?
>
> [ ... ]
> > @@ -400,10 +421,30 @@ static void
> amd_mdb_pcie_free_irq_domains(struct amd_mdb_pcie *pcie)
> > +           const struct amd_mdb_intr_cause *intr_cause =
> > +                   pcie->variant->version == MDB5 ?
> > +                   mdb5_intr_cause : cpm6_intr_cause;
> > +           int i, irq;
> > +
> > +           for (i = 0; i < ARRAY_SIZE(mdb5_intr_cause); i++) {
> > +                   if (!intr_cause[i].str)
> > +                           continue;
> > +                   irq = irq_find_mapping(pcie->mdb_domain, i);
> > +                   if (irq) {
> > +                           disable_irq(irq);
> > +                           free_irq(irq, pcie);
> > +                           irq_dispose_mapping(irq);
> > +                   }
> > +           }
>
> [Severity: High]
> Is it safe to manually call free_irq() on an interrupt allocated with
> devm_request_irq()?
>
> In amd_mdb_setup_irq(), these interrupts are allocated under devres
> management. On probe failure, this error path manually frees them. When
> the
> driver unbinds, devres will attempt to free the same IRQs again.
>
> Can this cause a double-free on a disposed virtual IRQ mapping?
>
> [ ... ]
> >             irq_domain_remove(pcie->mdb_domain);
> >             pcie->mdb_domain = NULL;
>
> [Severity: High]
> This is a pre-existing issue, but can this lead to a use-after-free of the
> irq_domain if a hardware interrupt arrives concurrently?
>
> The error path destroys and frees pcie->mdb_domain, but fails to disable
> the main event interrupt first. If an interrupt fires concurrently,
> amd_mdb_pcie_event() might dereference the freed domain pointer to
> dispatch
> child interrupts.
>

Agreed, this is a pre-existing problem. Removing the manual free loop above
makes the window smaller, but the real fix - stopping the parent event IRQ
before removing the domains - is generic MDB5 code. I'll do that in a
separate patch so it can be backported on its own, as Bjorn suggested.

> [ ... ]
> > @@ -639,12 +660,26 @@ static int amd_mdb_add_pcie_port(struct
> amd_mdb_pcie *pcie,
> >     pcie->slcr = devm_platform_ioremap_resource_byname(pdev, "slcr");
> >     if (IS_ERR(pcie->slcr))
> >             return PTR_ERR(pcie->slcr);
>
> [Severity: High]
> Will an exclusive ioremap of the shared slcr region cause the second CPM6
> host controller to fail probing?
>
> The commit message notes the aggregation layer is shared between the two
> controllers. Requesting exclusive access to the memory region here means the
> first device claims it, and the second device might fail to probe with a
> resource conflict.
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260728202044.1785986-
> [email protected]?part=2

Thanks,
Sai Krishna