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

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

New issues:
- [High] Blindly clearing the shared `PS_MISC` severity register in a shared interrupt handler creates a race condition that can permanently drop the peer controller's interrupts.
- [High] The interrupt handler checks the raw status (`ev_raw`) instead of the masked status (`val`) for pending interrupts, breaking spurious IRQ detection and inappropriately clearing masked bits.
- [High] The driver fails to mask unhandled interrupt sources and leaves the sticky aggregator uncleared on boot, leading to an unrecoverable interrupt storm on CPM6.

Pre-existing issues:
- [High] On MDB5, INT B, C, and D interrupts are enabled in hardware but not mapped to the Linux IRQ domain, causing them to be silently dropped.
--

--- Patch [2]: [PATCH v7 2/2] PCI: amd-mdb: Add CPM6 host controller support ---
commit 23a947213bc9887ee94f4ea8b025fb7bc8ad21bd
Author: Sai Krishna Musham <[email protected]>

PCI: amd-mdb: Add CPM6 host controller support

This patch adds support for AMD Versal2 CPM6 host controllers. It adapts
the driver to handle the differing interrupt register layout of CPM6 and
manages the shared event interrupt line.

Link: https://lore.kernel.org/all/[email protected]/

> diff --git a/drivers/pci/controller/dwc/pcie-amd-mdb.c b/drivers/pci/controller/dwc/pcie-amd-mdb.c
> --- a/drivers/pci/controller/dwc/pcie-amd-mdb.c
> +++ b/drivers/pci/controller/dwc/pcie-amd-mdb.c

[ ... ]

> +static void amd_mdb_pcie_clear_aggregators(struct amd_mdb_pcie *pcie)
> +{
> +	if (pcie->variant->version == MDB5)
> +		return;
> +
> +	/* Clear this host's serviced MISC_EVENT contribution from MERGED. */
> +	writel_relaxed(amd_mdb_pcie_merged_host_mask(pcie),
> +		       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 clearing this shared register unconditionally create a race condition
that drops interrupts for the peer controller?

If Host 1's interrupt arrives and sets its MERGED_STATUS bit just before
Host 0's interrupt handler executes this W1C write to the shared 
AMD_CPM6_PS_MISC_IR_STATUS register, the aggregator is cleared while Host 1's
event is still pending. Because the aggregator does not re-assert for
already-latched MERGED bits or loses the edge, does this mean the GIC line
drops and Host 1's interrupt is permanently lost?

[ ... ]

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

[ ... ]

> +	val &= pcie->variant->misc_mask_all;
> +
> +	/*
> +	 * The misc GIC line is shared with the peer host controller. If this
> +	 * host has nothing pending, report the interrupt as not ours so the
> +	 * shared line's spurious detection keeps working and the peer's sticky
> +	 * aggregator state is left untouched.
> +	 */
> +	if (!ev_raw)
> +		return IRQ_NONE;

[Severity: High]
Should this check use the masked status (val) rather than the raw status
(ev_raw) to determine if the interrupt was spurious?

If this controller has a masked status bit active (such as an AER error left
to native AER), ev_raw will be non-zero even when the peer controller asserted
the shared GIC line. The check if (!ev_raw) will pass, proceeding to falsely
return IRQ_HANDLED while unconditionally clearing the masked status bit and
the shared aggregator. Could this break AER error reporting?

[ ... ]

> -static void amd_mdb_pcie_free_irq_domains(struct amd_mdb_pcie *pcie)
> +static void amd_mdb_pcie_disable_interrupts(struct amd_mdb_pcie *pcie)
>  {
> +	u32 misc_mask_all = pcie->variant->misc_mask_all;
> +	u32 val;
> +
> +	/* Mask all leaf TLP interrupts. */
> +	writel_relaxed(misc_mask_all,
> +		       pcie->intr_base + pcie->variant->misc_disable_reg);

[Severity: High]
Does this sequence leave unhandled interrupt sources unmasked and the sticky
aggregator uncleared?

For CPM6, misc_mask_all only includes the INTx bit, which leaves other
interrupt sources like PCIe errors unmasked. It also fails to clear the
sticky MERGED_STATUS aggregator. 

When amd_mdb_pcie_init_port() is called later and enables the shared
aggregator via AMD_CPM6_MERGED_ENABLE, any pending unhandled leaf interrupts
or stale aggregator state will immediately assert the GIC line. If the
handler then returns IRQ_NONE or clears the leaf without resolving the
underlying error, will this cause an unrecoverable interrupt storm on boot?

[ ... ]

>  static int amd_mdb_setup_irq(struct amd_mdb_pcie *pcie,
>  			     struct platform_device *pdev)
>  {
>  	struct dw_pcie *pci = &pcie->pci;
>  	struct dw_pcie_rp *pp = &pci->pp;
>  	struct device *dev = &pdev->dev;
> +	unsigned long event_flags = IRQF_NO_THREAD;
>  	int i, irq, err;

[ ... ]

>  	pcie->intx_irq = irq_create_mapping(pcie->mdb_domain,
> -					    AMD_MDB_PCIE_INTR_INTX);
> +				    pcie->variant->intx_hwirq);

[Severity: High]
This isn't a bug introduced by this patch, but are the INT B, C, and D
interrupts missing from the IRQ domain mapping on MDB5?

In amd_mdb_setup_irq(), only the intx_hwirq (which corresponds to INT A,
bit 16 on MDB5) is mapped into the mdb_domain. However, the hardware is
configured to unmask all four INTx sources (bits 16, 18, 20, 22) in
amd_mdb_intx_irq_unmask(). 

When INT B, C, or D fire, amd_mdb_pcie_event() will call
generic_handle_domain_irq() for an unmapped hwirq, which returns -EINVAL.
Does this mean legacy PCI devices using INT B, C, or D behind the MDB5
controller will have their interrupts silently dropped?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2