Re: [PATCH 2/5] PCI/FLIT: Add support for Flit Logging Extended Capability

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Multiple execution contexts perform Read-Modify-Write (RMW) operations on the `PCI_FLIT_ERR_CNTR_CTRL` register without mutual exclusion, creating a race condition that can leave hardware interrupts enabled after driver removal or suspend, resulting in an unhandled IRQ storm.
- [High] Failure to properly mask level-triggered INTx interrupts in the hard IRQ handler leads to an IRQ storm and CPU lockup.
- [High] Infinite loop in threaded IRQ handler on surprise device removal or PCI error response.
- [Medium] Platform-configured Flit Error Counter settings are lost across D3cold suspend/resume transitions.
- [Medium] Unbounded loop in `flit_isr` drains hardware error logs, risking Denial of Service (DoS) and soft lockups.
--

--- Patch [2]: [PATCH 2/5] PCI/FLIT: Add support for Flit Logging Extended Capability ---
commit 2eae6403f78aaec770133667c1980b90da860010
Author: Avadhut Naik <avadhut.naik@amd.com>

PCI/FLIT: Add support for Flit Logging Extended Capability

This commit introduces a port service driver to support the PCI Express Flit
Logging Extended Capability. This feature allows logging of erroneous flits
on ports operating in PCIe Gen6 Flit Mode.

> diff --git a/drivers/pci/pcie/flit.c b/drivers/pci/pcie/flit.c
> new file mode 100644
> index 0000000000000..12f2b6fbf1de0
> --- /dev/null
> +++ b/drivers/pci/pcie/flit.c
> @@ -0,0 +1,170 @@
[ ... ]
> +static void flit_cntr_disable(struct pcie_device *dev)
> +{
> +	struct pci_dev *pdev = dev->port;
> +	u16 ctrl, flit = pdev->flit_cap;
> +
> +	/* Disable both Error Counter and Interrupt generation */
> +	pci_read_config_word(pdev, flit + PCI_FLIT_ERR_CNTR_CTRL, &ctrl);
> +	ctrl &= ~(PCI_FLIT_ERR_CNTR_EN | PCI_FLIT_ERR_CNTR_INTR_EN);
> +	pci_write_config_word(pdev, flit + PCI_FLIT_ERR_CNTR_CTRL, ctrl);
> +}

[Severity: High]
Multiple execution contexts perform Read-Modify-Write (RMW) operations on the
PCI_FLIT_ERR_CNTR_CTRL register without mutual exclusion.

If flit_cntr_disable() runs concurrently with flit_isr() during driver
unbind or hot-unplug, could the lockless clear of the EN and INTR_EN bits
be silently overwritten by flit_isr() writing back its stale cached value
with INTR_EN set?

Since flit_irq() also performs a lockless RMW from hardirq context, should
all accesses to this control register be protected by a spinlock to prevent
leaving hardware interrupts enabled after driver removal?

[ ... ]
> +static irqreturn_t flit_isr(int irq, void *context)
> +{
[ ... ]
> +	do {
> +		pci_read_config_dword(pdev, flit + PCI_FLIT_ERR_LOG1, &err_log1);
> +
> +		if (!(err_log1 & PCI_FLIT_ERR_LOG_VALID))
> +			break;
> +
> +		pci_read_config_dword(pdev, flit + PCI_FLIT_ERR_LOG2, &err_log2);
> +		pci_info(pdev, HW_ERR "  Error Log1: 0x%08x Error Log2: 0x%08x\n", err_log1, err_log2);
> +
> +		pci_write_config_dword(pdev, flit + PCI_FLIT_ERR_LOG1, err_log1);
> +	} while (err_log1 & PCI_FLIT_ERR_LOG_MORE);

[Severity: High]
If the PCIe link goes down or the device is surprise-removed,
pci_read_config_dword() will return an error response of 0xFFFFFFFF (~0).

Since (~0 & PCI_FLIT_ERR_LOG_VALID) and (~0 & PCI_FLIT_ERR_LOG_MORE) are
both true, will this trap the threaded handler in an infinite loop?

[Severity: Medium]
Does this do/while loop need a maximum iteration limit?

If a faulty or malicious PCIe device streams flit errors and continually
asserts the MORE bit faster than the CPU can process them, could this
indefinitely stall the CPU in the threaded IRQ handler and cause a soft
lockup?

[ ... ]
> +static irqreturn_t flit_irq(int irq, void *context)
> +{
> +	struct pcie_device *dev = (struct pcie_device *)context;
> +	struct pci_dev *pdev = dev->port;
> +	u16 flit = pdev->flit_cap;
> +	u16 cntr_sta, cntr_ctrl;
> +
> +	pci_read_config_word(pdev, flit + PCI_FLIT_ERR_CNTR_STA, &cntr_sta);
> +	if (!(cntr_sta & PCI_FLIT_INTR_GEN_CNTR))
> +		return IRQ_NONE;

[Severity: High]
Similar to the loop in flit_isr() above, if the device is disconnected and
pci_read_config_word() sets cntr_sta to 0xFFFF, will this erroneously skip
the IRQ_NONE return and wake the thread?

> +
> +	pci_read_config_word(pdev, flit + PCI_FLIT_ERR_CNTR_CTRL, &cntr_ctrl);
> +	cntr_ctrl &= ~PCI_FLIT_ERR_CNTR_EN;
> +	pci_write_config_word(pdev, flit + PCI_FLIT_ERR_CNTR_CTRL, cntr_ctrl);
> +	return IRQ_WAKE_THREAD;
> +}

[Severity: High]
When using shared legacy INTx interrupts, the hard IRQ handler clears
PCI_FLIT_ERR_CNTR_EN but fails to clear PCI_FLIT_ERR_CNTR_INTR_EN.

Since the interrupt status bit is only cleared on a 0 to 1 transition of
the EN bit (which happens later in the threaded handler), the INTx line
remains physically asserted.

Because flit_probe() does not use IRQF_ONESHOT, won't returning
IRQ_WAKE_THREAD unmask the IRQ line and immediately re-trigger the hard
IRQ handler, starving the threaded handler and creating an infinite IRQ
storm?

[ ... ]
> +static int flit_cntr_suspend(struct pcie_device *dev)
> +{
> +	flit_cntr_disable(dev);
> +	return 0;
> +}

[Severity: Medium]
The suspend hook disables the counter but does not read or save the
platform-configured "Set Events to Count" and "Trigger Event Count"
fields.

When flit_cntr_enable() is called on resume, it reads the hardware-reset
value (typically 0) and writes it back with the enable bits set. Will this
permanently erase the platform-specific error logging thresholds across a
D3cold suspend and resume cycle?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714183950.1620016-1-yazen.ghannam@amd.com?part=2