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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Manual port type checking for downstream/upstream components misses PCI_EXP_TYPE_PCIE_BRIDGE, causing lock inversion deadlocks and violating PCIe spec initialization order.
--

commit 1a49529648026f2ee499ea16e34fdf520b1c02fb
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) capability
(mandatory for Gen 6+ links, optional for Gen 4 and 5). It provides a debugfs
interface to view capabilities and execute margin timing/voltage commands.

> diff --git a/drivers/pci/pcie/margin.c b/drivers/pci/pcie/margin.c
> new file mode 100644
> index 0000000000000..1dde5aa928c74
> --- /dev/null
> +++ b/drivers/pci/pcie/margin.c
[ ... ]
> +static void pci_lmr_get_ports(struct pci_margin_dev *mdev,
> +			      struct pci_dev **downstream_port,
> +			      struct pci_dev **upstream_port)
> +{
> +	struct pci_dev *dev = mdev->dev;
> +	struct pci_dev *partner = mdev->partner;
> +
> +	if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
> +	    pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM) {

[Severity: High]
Does this manual port type check miss PCI_EXP_TYPE_PCIE_BRIDGE?

If the device is a PCI-to-PCIe bridge, it acts as a downstream-facing port
but would fall into the else branch here. This incorrectly classifies the
bridge as an upstream-facing port, reversing the downstream_port and
upstream_port assignments.

> +		*downstream_port = dev;
> +		*upstream_port = partner;
> +	} else {
> +		*downstream_port = partner;
> +		*upstream_port = dev;
> +	}
> +}
[ ... ]
> +static ssize_t margin_enable_write(struct file *file,
> +				   const char __user *user_buf, size_t count,
> +				   loff_t *ppos)
> +{
[ ... ]
> +	} else {
> +		struct pci_dev *partner;
[ ... ]
> +		if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
> +		    pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM) {

[Severity: High]
Can this missing PCI_EXP_TYPE_PCIE_BRIDGE check cause an AB-BA lock
inversion?

When LMR is enabled via debugfs on a PCI-to-PCIe bridge,
pcie_get_link_endpoints() correctly identifies it as a downstream-facing port
and locks it first. During disable, the missing check reverses the
assignment, locking the link partner first and risking a deadlock if called
concurrently.

> +			downstream_port = pci_dev_get(dev);
> +			upstream_port = partner;
> +		} else {
> +			downstream_port = partner;
> +			upstream_port = pci_dev_get(dev);
> +		}
> +	}
[ ... ]
> +void pci_lmr_exit(struct pci_dev *dev)
> +{
> +	struct pci_margin_dev *mdev;
> +	struct pci_dev *partner;
> +	struct pci_dev *downstream_port, *upstream_port;
[ ... ]
> +	if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
> +	    pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM) {

[Severity: High]
Can this similarly cause a lock inversion deadlock during device removal?

If LMR is being concurrently manipulated, the missing
PCI_EXP_TYPE_PCIE_BRIDGE check reverses the locking order for the downstream
and upstream ports, potentially freezing the kernel.

> +		downstream_port = dev;
> +		upstream_port = partner;
> +	} else {
> +		downstream_port = partner;
> +		upstream_port = dev;
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260904-pcie-link-endpoints-v2-0-16fcb301a3e4@google.com?part=3