Re: [PATCH v7] PCI/pcie: Add PCIe Lane Margining at Receiver (LMR) support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Fast-path NAK detection is nested inside the timeout block, forcing a 150ms delay on every NAK.
- [High] Lock mismatch in `margin_enable_write` when disabling margining leaves the remote target device unprotected.
- [High] Direct hardware modification of ASPM and link control bypasses the kernel ASPM subsystem and remote device locks, causing state corruption.
--

commit ec623c281254474c94a2e37656d0693bdc62d283
Author: Priyank Rathod <rathodpriyank@google.com>
PCI/pcie: Add PCIe Lane Margining at Receiver (LMR) support

This commit adds driver and debugfs support for PCIe Lane Margining at
Receiver (LMR) on Gen4+ links. It introduces Kconfig options and debugfs
interfaces to control margining, configure target receivers, and run timing
and voltage margin steps.

> diff --git a/drivers/pci/pcie/margin.c b/drivers/pci/pcie/margin.c
> new file mode 100644
> index 0000000000000..a428726c500db
> --- /dev/null
> +++ b/drivers/pci/pcie/margin.c
[ ... ]
> +static void pci_lmr_aspm_inhibit(struct pci_margin_dev *mdev, bool inhibit)
> +{
[ ... ]
> +		/*
> +		 * 1. Downstream Component (upstream_port) must be disabled
> +		 * FIRST per sec 7.5.3.7.
> +		 */
> +		if (upstream_port && pci_is_pcie(upstream_port) &&
> +		    upstream_port->current_state == PCI_D0) {
> +			if (!pcie_capability_read_word(upstream_port, PCI_EXP_LNKCTL, &ctl)) {
> +				mdev->saved_usp_aspm = ctl & PCI_EXP_LNKCTL_ASPMC;
> +				pcie_capability_clear_word(upstream_port, PCI_EXP_LNKCTL,
> +							   PCI_EXP_LNKCTL_ASPMC);
> +			}
> +		}
> +
> +		/*
> +		 * 2. Upstream Component (downstream_port) must be disabled
> +		 * SECOND per sec 7.5.3.7.
> +		 */
> +		if (downstream_port && pci_is_pcie(downstream_port) &&
> +		    downstream_port->current_state == PCI_D0) {
> +			if (!pcie_capability_read_word(downstream_port, PCI_EXP_LNKCTL, &ctl)) {
> +				mdev->saved_dsp_aspm = ctl & PCI_EXP_LNKCTL_ASPMC;
> +				pcie_capability_clear_word(downstream_port, PCI_EXP_LNKCTL,
> +							   PCI_EXP_LNKCTL_ASPMC);
> +			}
> +		}

[Severity: High]
Does this direct hardware modification of ASPM registers bypass the kernel
ASPM subsystem and the remote device's lock?

By clearing ASPM bits directly in the config space without acquiring the
remote partner's pci_dev_lock, this could lead to data races and state
corruption if the core ASPM tracking state diverges from the physical
hardware.

[ ... ]
> +static void pci_lmr_ensure_aspm_inhibited(struct pci_margin_dev *mdev)
> +{
[ ... ]
> +	if (re_inhibit) {
> +		pci_info_ratelimited(mdev->dev,
> +				     "ASPM re-enabled unexpectedly; re-enforcing ASPM inhibit for LMR\n");
> +		/* Disable Downstream Component first, Upstream Component second per sec 7.5.3.7 */
> +		if (upstream_port && pci_is_pcie(upstream_port))
> +			pcie_capability_clear_word(upstream_port, PCI_EXP_LNKCTL,
> +						   PCI_EXP_LNKCTL_ASPMC);

[Severity: High]
Is it safe to repair the ASPM mismatch with naked configuration writes here?

This TOCTOU gap modifies the remote partner's ASPM registers while entirely
omitting the remote pci_dev_lock and bypassing the core aspm_lock.

[ ... ]
> +static int pci_lmr_run_cmd(struct pci_margin_dev *mdev, int lane, u8 rx, u8 type,
> +			   u8 usage, u8 payload, u16 *status_val)
> +{
[ ... ]
> +		if (FIELD_GET(PCI_LMR_LANE_STS_MTYPE, sts) == type &&
> +		    FIELD_GET(PCI_LMR_LANE_STS_RX_NUM, sts) == rx) {
> +			if (status_val)
> +				*status_val = sts;
> +			return 0;
> +		}
> +
> +		if (time_after(jiffies, timeout)) {
> +			/*
> +			 * Per PCIe Base Specification Revision 7.0 sec 4.2.18.2
> +			 * & Table 4-77 (r6.0 Table 4-73), if receiver echoes
> +			 * NO_CMD (0x7) after command issuance, it indicates NAK.
> +			 */
> +			if (FIELD_GET(PCI_LMR_LANE_STS_MTYPE, sts) == LMR_TYPE_NO_CMD &&
> +			    FIELD_GET(PCI_LMR_LANE_STS_RX_NUM, sts) == rx)
> +				return -EOPNOTSUPP;
> +			break;
> +		}
> +
> +		usleep_range(LMR_CMD_SLEEP_MIN_US, LMR_CMD_SLEEP_MAX_US);
> +	}

[Severity: High]
Does nesting the NAK check inside the timeout block defeat the fast-path?

If the target receiver responds with NO_CMD (0x7) immediately to indicate a
NAK, this logic appears to force the loop to wait the full 150ms timeout
before ever evaluating the NAK condition. Can this check be moved outside
the time_after() block?

[ ... ]
> +static ssize_t margin_enable_write(struct file *file,
> +				   const char __user *user_buf, size_t count,
> +				   loff_t *ppos)
> +{
[ ... ]
> +	pci_lmr_get_link_partners(dev, &downstream_port, &upstream_port);
> +
> +	/* Strict hierarchical lock order: Downstream Port (parent) before Upstream Port (child) */
> +	if (downstream_port)
> +		pci_dev_lock(downstream_port);
> +	if (upstream_port && upstream_port != downstream_port)
> +		pci_dev_lock(upstream_port);
> +
> +	mutex_lock(&mdev->lock);
> +
> +	if (mdev->enabled == enable) {
> +		ret = count;
> +	} else if (!enable) {
> +		pci_lmr_disable_locked(mdev);
> +		ret = count;
> +	}

[Severity: High]
Can a topology change between margining enable and disable cause a lock
mismatch here?

When disabling margining, pci_dev_lock() is acquired on the dynamically
fetched upstream_port and downstream_port, but the teardown configuration
writes inside pci_lmr_disable_locked(mdev) operate on the globally cached
mdev->partner.

If the link partner has changed, does this leave the remote target device's
runtime PM and config state unprotected?

[ ... ]
> +void pci_reset_lmr(struct pci_dev *dev)
> +{
[ ... ]
> +		/* Restore original hardware ASPM before saved states can seal the leak */
> +		pci_lmr_aspm_inhibit(mdev, false);
> +		pci_lmr_restore_autonomous(mdev);

[Severity: High]
Does pci_reset_lmr() invoke config space mutations on the remote partner
without acquiring the remote partner's pci_dev_lock?

It appears that this reset path triggers pci_lmr_aspm_inhibit() without
locking the remote device, potentially causing data races during reset.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260828-pcie-lmt-v7-1-6012e9e0940a@google.com?part=1