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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [High] Device ASPM state is permanently disabled after a PCI reset because `pci_restore_state()` clobbers the correctly restored LMR ASPM configuration.
- [High] Concurrent execution of sysfs device reset and device removal leads to a Use-After-Free of `dev->lmr` and its mutex.
- [High] Incorrect teardown ordering in `pci_lmr_exit()` allows concurrent debugfs writes to permanently leak an RPM reference and leave upstream ASPM disabled.
- [Medium] LMR software state is not suspended during hibernation, leading to a mismatch between software tracking and hardware state upon thaw/restore.
- [Medium] The driver violates PCIe specification requirements by failing to automatically issue a Demargin command before reversing margin directions.
- [Medium] The error cleanup path in `margin_enable_write()` clobbers the original negative error code, causing the function to silently return success.
- [Medium] Secondary Bus Resets (SBR) bypass device-specific reset handlers, permanently desynchronizing LMR software state and leaking the runtime PM reference.
- [Medium] The driver writes 1 to reserved direction bits when configuring devices that only support symmetric margining.
--

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

This patch introduces support for PCIe Lane Margining at Receiver (LMR). It
adds debugfs controls to allow manual execution of timing and voltage margin
steps on capable receivers, along with basic state management.

Link: https://lore.kernel.org/r/20260820-pcie-lmt-v5-1-943b3b0e18bf@google.com

> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -821,6 +821,7 @@ static int pci_pm_suspend(struct device *dev)
>  	 * since Coffee Lake, to enter a lower-power PM state.
>  	 */
>  	pci_suspend_ptm(pci_dev);
> +	pci_suspend_lmr(pci_dev);
>  
>  	if (pci_has_legacy_pm_support(pci_dev))
>  		return pci_legacy_suspend(dev, PMSG_SUSPEND);

[Severity: Medium]
Does this miss hibernation (S4) support? 

The LMR suspend is hooked into pci_pm_suspend() for S3 sleep, but 
pci_pm_freeze() and pci_pm_poweroff() don't appear to call pci_suspend_lmr(). 
If the system hibernates, the device loses power and the margining state 
resets, but upon resume, mdev->enabled would still be true, leaving ASPM 
tracking desynchronized and a leaked runtime PM reference.

> --- 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;
> +		}
>  
>  		pci_dbg(dev, "%s failed with %d\n", method->name, rc);

[Severity: High]
Will the ASPM restoration in pci_reset_lmr() be immediately clobbered here? 

The pci_reset_function() sequence calls pci_save_state() before this, and 
pci_restore_state() afterwards. If pci_reset_lmr() restores the ASPM hardware 
state to its original configuration, won't pci_restore_state() subsequently 
blindly overwrite it with the saved config space (where ASPM was disabled by 
LMR), leaving ASPM permanently disabled?

[Severity: Medium]
By hooking LMR reset cleanup exclusively here in __pci_reset_function_locked(), 
do we miss Secondary Bus Resets (SBR)? 

If a root user triggers a bus reset via the parent bridge, the downstream 
devices are reset using a different path. The hardware LMR state would be 
wiped, but pci_reset_lmr() is never invoked, leaving mdev->enabled as true 
and failing to drop the runtime PM reference.

> --- a/drivers/pci/pcie/margin.c
> +++ b/drivers/pci/pcie/margin.c
[ ... ]
> @@ -557,6 +557,13 @@ static ssize_t margin_enable_write(struct file *file, const char __user *user_buf,
>  	mdev->enabled = true;
>  	return count;
>  
> +err_sw_ready:
> +	if (cap & PCI_LMR_PORT_CAP_USES_SW_READY) {
> +		ret = pci_read_config_word(dev, mdev->cap + PCI_LMR_PORT_STS, &sts);
> +		if (ret == PCIBIOS_SUCCESSFUL) {
> +			sts &= ~PCI_LMR_PORT_STS_SW_READY;
> +			pci_write_config_word(dev, mdev->cap + PCI_LMR_PORT_STS, sts);
> +		}
> +	}
> +err_aspm:

[Severity: Medium]
Does this error cleanup path accidentally clobber the original negative error 
code? 

If an error like -ETIMEDOUT occurs above, the jump to err_sw_ready issues a 
pci_read_config_word(). If that read succeeds, ret is overwritten with 
PCIBIOS_SUCCESSFUL (0). The function will then silently return 0 instead of 
propagating the actual failure to userspace.

[ ... ]
> @@ -732,6 +732,10 @@ static ssize_t margin_lane_step_write(struct file *file, const char __user *user_buf,
>  	if (!mdev->enabled)
>  		return -EBUSY;
>  
> +	if (val == 0) {
> +		ret = pci_lmr_demargin_lane(plane);
> +		return ret ? ret : count;
> +	}
> +
> +	ret = pci_lmr_cache_rx_info(plane, plane->rx);

[Severity: Medium]
Are we missing an automatic demargin when changing margin directions? 

The code issues a demargin command if the requested value is exactly 0, but 
if a user sequentially writes a positive offset and then a negative offset (or 
vice versa), the driver issues the new step command immediately. Per the PCIe 
Base Specification, software must issue a Demargin (or Clear Margin) command 
before changing margin directions to avoid undefined behavior or NAKs.

[ ... ]
> @@ -754,6 +758,11 @@ static ssize_t margin_lane_step_write(struct file *file, const char __user *user_buf,
>  		if (val < 0) {
>  			if (!(caps & LMR_CAP_IND_LEFT_RIGHT_TIMING))
>  				return -EINVAL;
>  			step = -val;
>  			dir = LMR_STEP_DIR_DECREASE;
>  		} else {
>  			step = val;
>  			dir = LMR_STEP_DIR_INCREASE;
>  		}

[Severity: Medium]
Does this write 1 to a reserved direction bit on symmetric-only receivers? 

For positive margins, the code unconditionally assigns 
dir = LMR_STEP_DIR_INCREASE (1). If the receiver doesn't support independent 
left/right or up/down margining, the PCIe Base Specification designates the 
direction bits as Reserved, so they should be written as 0.

[ ... ]
> @@ -983,6 +983,16 @@ void pci_lmr_exit(struct pci_dev *dev)
>  {
>  	struct pci_margin_dev *mdev = dev->lmr;
>  
> +	if (!dev || !mdev)
> +		return;
> +
> +	pci_suspend_lmr(dev);
> +
> +	pci_margin_debugfs_remove(mdev);
> +	mutex_destroy(&mdev->lock);
> +	kfree(mdev);
> +	dev->lmr = NULL;
>  }

[Severity: High]
Does this teardown ordering allow concurrent debugfs writes to leak a runtime 
PM reference? 

By calling pci_suspend_lmr() before pci_margin_debugfs_remove(), a concurrent 
write to the enable file could be blocked on mdev->lock. Once 
pci_suspend_lmr() sets enabled = false and releases the lock, the write thread 
wakes up, sees enabled is false, enables LMR again, acquires a runtime PM 
reference, and disables ASPM. Then pci_margin_debugfs_remove() completes and 
the structure is freed, abandoning the newly created hardware and RPM state.

[ ... ]
> @@ -1029,6 +1039,11 @@ void pci_reset_lmr(struct pci_dev *dev)
>  {
>  	struct pci_margin_dev *mdev = dev->lmr;
>  
> +	if (!dev || !mdev)
> +		return;
> +
> +	guard(mutex)(&mdev->lock);
> +	pci_lmr_reset_software_state_locked(mdev);
>  }

[Severity: High]
Can a concurrent device removal and sysfs reset trigger a use-after-free here? 

In pci_lmr_exit(), mdev is freed and dev->lmr is set to NULL without holding 
the device_lock. If a concurrent sysfs reset triggers pci_reset_function() 
(which acquires device_lock) and calls pci_reset_lmr(), it might read the 
dev->lmr pointer just before pci_lmr_exit() nullifies it. It would then 
attempt to lock the mutex of the already freed mdev structure.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260824-pcie-lmt-v6-1-86ada768b0c8@google.com?part=1