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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] Asynchronous pm_runtime_put() is used immediately before device_del(), causing race conditions on device teardown.
- [High] Spurious command failure (-EOPNOTSUPP) in pci_lmr_run_cmd() due to a fast-path NAK check racing with hardware state updates.
- [High] Graceful LMR suspend on reset device causes timeout delays and state corruption.
- [High] Direct modification of ASPM Link Control registers bypasses the PCI core ASPM subsystem.
- [High] Runtime PM violations with unprotected config space reads.
- [Medium] Mixing guard() lock acquisition with goto-based error handling in margin_enable_write().
- [Medium] Incorrect bitmask used when extracting the maximum number of timing steps.
--

--- Patch [1]: [PATCH v5] PCI/pcie: Add PCIe Lane Margining at Receiver (LMR) support ---
commit 067656b09520551d8f1bf1f71cfd2550c4c5e0ba
Author: Priyank Rathod <rathodpriyank@google.com>

PCI/pcie: Add PCIe Lane Margining at Receiver (LMR) support

This commit introduces driver and debugfs support for PCIe Lane Margining at
Receiver (LMR). It exposes per-device interfaces to evaluate high-speed link
margins by controlling receiver selection and margin steps.

> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -5145,8 +5145,10 @@ int __pci_reset_function_locked(struct pci_dev *dev)
>  		method = &pci_reset_fn_methods[m];
>  		pci_dbg(dev, "reset via %s\n", method->name);
>  		rc = method->reset_fn(dev, PCI_RESET_DO_RESET);
> -		if (!rc)
> +		if (!rc) {
> +			pci_reset_lmr(dev);
> +			return 0;
> +		}

[Severity: High]
Does calling pci_reset_lmr() here attempt to communicate with hardware that has
just been reset?

If the hardware state is already cleared by reset, delegating to the graceful
suspend routine might time out when trying to issue demargin commands to
inactive hardware.

> diff --git a/drivers/pci/pcie/margin.c b/drivers/pci/pcie/margin.c
> --- a/drivers/pci/pcie/margin.c
> +++ b/drivers/pci/pcie/margin.c
[ ... ]
> @@ -183,6 +183,23 @@ static struct dentry *get_pci_debugfs_root(void)
>  
>  /*
>   * pci_lmr_disable_aspm() - Temporarily disable ASPM L0s/L1 during active
>   * margining per PCIe Base Spec r6.0 sec 8.4.4, saving original ASPMC bits.
>   */
>  static void pci_lmr_disable_aspm(struct pci_margin_dev *mdev)
>  {
>  	struct pci_dev *dev = mdev->dev;
>  	struct pci_dev *parent = pci_upstream_bridge(dev);
>  	u16 ctl;
>  
>  	if (mdev->aspm_saved)
>  		return;
>  
>  	if (!pcie_capability_read_word(dev, PCI_EXP_LNKCTL, &ctl)) {
>  		mdev->saved_aspm = ctl & PCI_EXP_LNKCTL_ASPMC;
>  		pcie_capability_clear_word(dev, PCI_EXP_LNKCTL, PCI_EXP_LNKCTL_ASPMC);
>  	}
>  
>  	if (parent && pci_is_pcie(parent)) {
>  		if (!pcie_capability_read_word(parent, PCI_EXP_LNKCTL, &ctl)) {
>  			mdev->saved_parent_aspm = ctl & PCI_EXP_LNKCTL_ASPMC;
>  			pcie_capability_clear_word(parent, PCI_EXP_LNKCTL, PCI_EXP_LNKCTL_ASPMC);
>  		}

[Severity: High]
Does directly modifying the ASPM Link Control registers here bypass the PCI
core ASPM subsystem's refcounting and synchronization?

If multiple child devices on the same bridge enable LMR concurrently, they
might race when manipulating the shared parent upstream bridge config space.
When one child restores its saved state, it might inadvertently re-enable
ASPM while a sibling is still actively margining.

[ ... ]
> @@ -240,6 +240,55 @@ static int pci_lmr_run_cmd(struct pci_dev *dev, int lane, u8 rx, u8 type,
[ ... ]
>  	timeout = jiffies + msecs_to_jiffies(LMR_CMD_TIMEOUT_MS);
>  	while (1) {
>  		if (pci_read_config_word(dev, sts_offset, &sts))
>  			return -EIO;
>  		if (PCI_POSSIBLE_ERROR(sts))
>  			return -ENODEV;
>  
>  		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;
>  		}
>  
>  		/*
>  		 * Per PCIe Base Spec r6.0 sec 8.4.4, if receiver echoes NO_CMD (0x7)
>  		 * after a command was sent, it indicates command rejection/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;

[Severity: High]
Is there a risk that this fast-path NAK check will read a stale NO_CMD status
from the previous step?

Hardware state machine updates are typically not instantaneous. If the status
register is polled immediately after writing the new command, it may still
contain the NO_CMD state from the prior handshake.

This might cause valid margining commands to spuriously fail with -EOPNOTSUPP
before they have a chance to complete.

[ ... ]
> @@ -279,6 +279,11 @@ static int pci_lmr_cache_rx_info(struct pci_margin_lane *plane, u8 rx)
>  	if (ret)
>  		return ret;
>  	info->caps = pci_lmr_sts_payload(sts);
>  
>  	ret = pci_lmr_run_cmd(plane->mdev->dev, plane->lane, rx,
>  			      LMR_TYPE_REPORT_TIMING_STEPS, 0, 0, &sts);
>  	if (ret)
>  		return ret;
>  	info->num_timing_steps = FIELD_GET(LMR_STEPS_MASK, pci_lmr_sts_payload(sts));

[Severity: Medium]
Should this use LMR_TIMING_STEP_MASK instead of LMR_STEPS_MASK?

The PCIe Base Specification defines the Max Timing Margin payload as 6 bits.
Since LMR_STEPS_MASK uses a 7-bit mask (GENMASK(6,0)), the driver might parse
an incorrectly large step limit if the device sets bit 6.

[ ... ]
> @@ -406,6 +406,17 @@ static void __pci_suspend_lmr_locked(struct pci_dev *dev)
>  	if (pci_read_config_word(dev, mdev->cap + PCI_LMR_PORT_STS, &sts) == 0) {
>  		sts &= ~PCI_LMR_PORT_STS_SW_READY;
>  		pci_write_config_word(dev, mdev->cap + PCI_LMR_PORT_STS, sts);
>  	}
>  	pci_lmr_restore_aspm(mdev);
>  	pm_runtime_put(&dev->dev);
>  	mdev->enabled = false;
>  }

[Severity: High]
Does this asynchronous pm_runtime_put() create a race condition during device
teardown?

In pci_destroy_dev(), device_del() is called immediately after pci_lmr_exit():

drivers/pci/remove.c:pci_destroy_dev() {
    ...
	pci_lmr_exit(dev);
	/* ... */
	pci_tsm_destroy(dev);
	device_del(&dev->dev);
    ...
}

Using pm_runtime_put_sync() instead might ensure the device reaches the proper
power state and that pending idle/suspend work is completed before the device
structures are removed.

[ ... ]
> @@ -428,6 +428,74 @@ static ssize_t margin_enable_write(struct file *file, const char __user *user_buf,
>  			       size_t count, loff_t *ppos)
>  {
[ ... ]
>  	guard(mutex)(&mdev->lock);
>  
>  	if (mdev->enabled == enable)
>  		return count;
>  
>  	if (!enable) {
>  		__pci_suspend_lmr_locked(dev);
>  		return count;
>  	}
>  
>  	/* PCIe r6.0 sec 8.4.4: LMR is physically undefined below 16.0 GT/s (Gen4) */
>  	pcie_capability_read_word(dev, PCI_EXP_LNKSTA, &lnksta);
>  	if ((lnksta & PCI_EXP_LNKSTA_CLS) < PCI_EXP_LNKSTA_CLS_16_0GB)
>  		return -EOPNOTSUPP;
>  
>  	if (pci_read_config_word(dev, mdev->cap + PCI_LMR_PORT_CAP, &cap))
>  		return -EIO;
>  
>  	/* Prevent D3hot runtime PM transition during active margining */
>  	ret = pm_runtime_resume_and_get(&dev->dev);

[Severity: High]
Can these configuration space reads return invalid data if the device is
currently in the D3hot power state?

Reading PCI_EXP_LNKSTA and PCI_LMR_PORT_CAP before calling
pm_runtime_resume_and_get() could return 0xFFFF or 0. This might cause the
link speed check to fail incorrectly, or cause the driver to erroneously see
USES_SW_READY as true.

>  	if (ret < 0)
>  		return ret;
[ ... ]
>  	if (cap & PCI_LMR_PORT_CAP_USES_SW_READY) {
>  		if (pci_read_config_word(dev, mdev->cap + PCI_LMR_PORT_STS, &sts)) {
>  			ret = -EIO;
>  			goto err_enable;
>  		}
>  		sts |= PCI_LMR_PORT_STS_SW_READY;
>  		pci_write_config_word(dev, mdev->cap + PCI_LMR_PORT_STS, sts);
>  	}
>  
>  	timeout = jiffies + msecs_to_jiffies(LMR_ENABLE_TIMEOUT_MS);
>  	while (1) {
>  		if (pci_read_config_word(dev, mdev->cap + PCI_LMR_PORT_STS, &sts)) {
>  			ret = -EIO;
>  			goto err_enable;
>  		}
[ ... ]
>  err_enable:
>  	if (cap & PCI_LMR_PORT_CAP_USES_SW_READY) {
>  		if (pci_read_config_word(dev, mdev->cap + PCI_LMR_PORT_STS, &sts) == 0) {

[Severity: Medium]
Is it safe to mix goto-based error handling with scope-based lock acquisition
in the same function?

While the label is at the end of the scope here, mixing the goto pattern
with guard(mutex)(&mdev->lock) makes ownership semantics less clear and might
increase the risk of resource leaks or locking issues during future updates.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260820-pcie-lmt-v5-1-943b3b0e18bf@google.com?part=1