Re: [PATCH v3] 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] Enabling Lane Margining permanently disables ASPM L0s/L1 without a restoration path, resulting in a persistent power regression.
- [High] The driver determines the number of lanes using Maximum Link Width instead of Negotiated Link Width, leading to massive command timeouts on inactive lanes.
- [Medium] Ignoring the return value of `pci_lmr_demargin_lane()` causes software state desynchronization and leaked hardware margins.
- [Medium] Incomplete LMR state teardown in `pci_lmr_exit()` fails to clear `SW_READY`, leaking hardware state.
- [Medium] The lazy initialization of `pci_debugfs_root_dir` is not thread-safe, allowing concurrent device probes to corrupt the root dentry pointer.
- [Medium] Missing runtime PM protection allows the device to enter D3hot while margining is active, destroying the hardware margining state.
- [Low] Enabling LMR silently clobbers user-selected lane receivers, forcing them back to LMR_RX_LOCAL.
- [Low] Incorrect error check for debugfs_create_dir() return value in pci_margin_debugfs_init().
--

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

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

This patch adds support for PCIe Lane Margining at Receiver (LMR) per the PCIe
6.0 base specification. It introduces driver structures, debugfs exposure for
manual margining configuration, and hooks into the device probe and remove
paths.

> diff --git a/drivers/pci/pcie/margin.c b/drivers/pci/pcie/margin.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/pci/pcie/margin.c
> @@ -0,0 +1,890 @@

[ ... ]

> +static struct dentry *get_pci_debugfs_root(void)
> +{
> +	if (!pci_debugfs_root_dir)
> +		pci_debugfs_root_dir = debugfs_lookup("pci", NULL);
> +	if (!pci_debugfs_root_dir)
> +		pci_debugfs_root_dir = debugfs_create_dir("pci", NULL);
> +	return pci_debugfs_root_dir;
> +}

[Severity: Medium]
Could this concurrent lockless check corrupt the global debugfs root pointer?

If two threads concurrently probe devices, both might see a NULL pointer and
attempt to create the directory, leading one to overwrite the global
variable with an error pointer from debugfs_create_dir().

[ ... ]

> +static int pci_lmr_cache_rx_info(struct pci_margin_lane *plane, u8 rx)
> +{
> +	struct pci_margin_rx_info *info = &plane->rx_info[rx];
> +	u16 sts;
> +	int ret;
> +
> +	if (info->caps_cached)
> +		return 0;
> +
> +	/* Issuing REPORT_CAPS aborts any active margin per PCIe spec */
> +	pci_lmr_demargin_lane(plane);
> +
> +	ret = pci_lmr_run_cmd(plane->mdev->dev, plane->lane, rx,
> +			      LMR_TYPE_REPORT_CAPS, 0, 0, &sts);

[Severity: Medium]
Should the return value of pci_lmr_demargin_lane() be checked here?

If the demargining command fails due to a timeout, proceeding to issue
REPORT_CAPS resets the hardware state without software properly handling the
error, which might cause the hardware and software states to fall out of sync.

[ ... ]

> +static ssize_t margin_enable_write(struct file *file, const char __user *user_buf,
> +			       size_t count, loff_t *ppos)
> +{

[ ... ]

> +	if (enable) {
> +		if (pci_read_config_word(dev, mdev->cap + PCI_LMR_PORT_CAP, &cap)) {
> +			ret = -EIO;
> +			goto out;
> +		}
> +
> +		/*
> +		 * PCIe Base Spec r6.0: Link must be maintained in L0 during
> +		 * active margining. Disable ASPM L0s/L1.
> +		 */
> +		pci_disable_link_state(dev, PCIE_LINK_STATE_L0S | PCIE_LINK_STATE_L1);

[Severity: High]
Does pci_disable_link_state() permanently disable ASPM for this device?

There does not appear to be a restoration path when the enable flag is toggled
off. This could result in a persistent power efficiency regression for the
system until the next reboot.

[Severity: Medium]
Is a runtime PM reference needed here, such as pm_runtime_get_sync()?

Without taking a runtime PM reference, the PM core could transition an idle
device to D3hot. This would drop the link from L0, violating the active
margining state machine requirements and potentially destroying the hardware
margining state.

> +
> +		if (cap & PCI_LMR_PORT_CAP_USES_SW_READY) {

[ ... ]

> +		/* Cache capabilities for local receiver on all lanes */
> +		for (i = 0; i < mdev->num_lanes; i++) {
> +			mdev->lanes[i].rx = LMR_RX_LOCAL;
> +			pci_lmr_cache_rx_info(&mdev->lanes[i], LMR_RX_LOCAL);
> +		}
> +		mdev->enabled = true;

[Severity: Low]
Does enabling margining intentionally clobber user-selected lane receivers?

If a user configures specific receivers for lanes via debugfs while margining
is disabled, that configuration will be silently overwritten and forced back
to LMR_RX_LOCAL.

[ ... ]

> +static ssize_t margin_lane_receiver_write(struct file *file, const char __user *user_buf,
> +				      size_t count, loff_t *ppos)
> +{

[ ... ]

> +	if (mdev->enabled) {
> +		/* Demargin previous receiver per single-receiver spec rule */
> +		pci_lmr_demargin_lane(plane);
> +		ret = pci_lmr_cache_rx_info(plane, rx);
> +		if (ret)
> +			goto out;
> +	}
> +
> +	plane->rx = rx;

[Severity: Medium]
What happens if pci_lmr_demargin_lane() fails here?

If the error is ignored, plane->rx is updated to the new receiver anyway. This
could leave the previous hardware receiver permanently margined while software
variables lose track of it.

[ ... ]

> +static void pci_margin_debugfs_init(struct pci_margin_dev *mdev)
> +{
> +	struct pci_dev *dev = mdev->dev;
> +	struct dentry *parent;
> +	char dirname[64];
> +	int i;
> +
> +	parent = get_pci_debugfs_root();
> +	snprintf(dirname, sizeof(dirname), "pcie_lmr_%s", dev_name(&dev->dev));
> +	mdev->debugfs = debugfs_create_dir(dirname, parent);
> +	if (!mdev->debugfs)
> +		return;

[Severity: Low]
Does debugfs_create_dir() return an error pointer rather than NULL on failure?

Checking for NULL here will evaluate to false on an error pointer, causing the
driver to proceed with an invalid debugfs dentry instead of cleanly aborting.

[ ... ]

> +void pci_lmr_init(struct pci_dev *dev)
> +{
> +	struct pci_margin_dev *mdev;
> +	enum pci_bus_speed speed;
> +	u16 lmr;
> +	u16 lnkcap;
> +	int i;

[ ... ]

> +	mutex_init(&mdev->lock);
> +
> +	pcie_capability_read_word(dev, PCI_EXP_LNKCAP, &lnkcap);
> +	mdev->num_lanes = FIELD_GET(PCI_EXP_LNKCAP_MLW, lnkcap);
> +
> +	if (mdev->num_lanes == 0 || mdev->num_lanes > LMR_MAX_LANES) {
> +		pci_warn(dev, "Invalid link width %d for LMR\n", mdev->num_lanes);
> +		goto err_free_mdev;
> +	}

[Severity: High]
Should this use Negotiated Link Width (PCI_EXP_LNKSTA_NLW) instead of Maximum
Link Width?

If a device like a x16 card is inserted into a slot with fewer active lanes,
the maximum link width will overestimate the active lanes. When margining is
enabled, commands sent to the inactive lanes might time out, causing massive
stalls during interaction.

[ ... ]

> +void pci_lmr_exit(struct pci_dev *dev)
> +{
> +	struct pci_margin_dev *mdev = dev->lmr;
> +	int i;
> +
> +	if (!mdev)
> +		return;
> +
> +	mutex_lock(&mdev->lock);
> +	if (mdev->enabled) {
> +		for (i = 0; i < mdev->num_lanes; i++)
> +			pci_lmr_demargin_lane(&mdev->lanes[i]);
> +	}
> +	mutex_unlock(&mdev->lock);

[Severity: Medium]
Should this teardown path also clear the PCI_LMR_PORT_STS_SW_READY bit and
set mdev->enabled to false?

Unlike the suspend path, this function leaves SW_READY asserted in hardware.
During device unbind or teardown, this abandons the hardware in an active
margining state expectation.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260818-pcie-lmt-v3-1-d5df983a5d53@google.com?part=1