Skip to contentSkip to conversation

PCI: Avoid link retraining on empty ports when lifting speed restriction

PATCH Superseded Andreas Wild 2 participants 2 messages Aug 1 PCI

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 PCI: Avoid link retraining on empty ports when lifting speed restriction

Since commit 72780f796468 ("PCI: Always lift 2.5GT/s restriction in PCIe failed link retraining") the Target Speed quirk lifts a firmware-imposed 2.5GT/s restriction on any downstream port, without checking whether the link is up. Where nothing is plugged in, the retraining that follows can never complete, so each attempt costs PCIE_LINK_RETRAIN_TIMEOUT_MS. The quirk makes two of them -- the initial one and the restore on the error path -- adding a fixed 2 s to every boot.

On an MSI PRO Z690-A WIFI DDR4 (Intel 600 Series PCH) with one empty x1 slot, running v7.2-rc5:

0.541 pci 0000:00:1c.0: removing 2.5GT/s downstream link speed restriction 1.541 pci 0000:00:1c.0: retraining failed
2.541 pci 0000:00:1c.2: [8086:7aba] type 01 class 0x060400

The kerneldoc above the quirk already contemplates this case ("For a port that has been left unconnected both bits will be clear") and describes lifting the restriction where firmware arranged it "and the port reports its link already being up". The code stopped checking the latter.

Program the Target Link Speed directly when Data Link Layer Link Active is clear, and skip the retraining. The restriction is still lifted, so a device plugged into the port later trains at the full speed the port supports, which is what the original change set out to ensure.

The check cannot be hoisted down into pcie_bwctrl_change_speed(): the ASM2824 workaround earlier in this function deliberately retrains a link whose DLLLA is clear, and would break.

Measured on v7.2-rc5, same tree and same config with and without this
patch: the interval after the quirk logs drops from 1000 ms to 3 ms, the second timeout disappears along with "retraining failed", and the systemd "kernel" boot phase goes from 3.011 s to 1.024 s. The restriction is still lifted -- on the still-empty port afterwards:

LnkCap: Port #1, Speed 8GT/s, Width x1
LnkSta: Speed 2.5GT/s, Width x0
LnkCtl2: Target Link Speed: 8GT/s

Fixes 72780f796468 ("PCI: Always lift 2.5GT/s restriction in PCIe failed link retraining")
---
 drivers/pci/quirks.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)
drivers/pci/quirks.c +17 -0
index b09f27f..d0c8b09 100644
@@ -97,6 +97,7 @@ int pcie_failed_link_retrain(struct pci_dev *dev)
 	u16 lnksta, lnkctl2, oldlnkctl2;
 	enum pci_bus_speed speed_cap;
 	int ret = -ENOTTY;
+	u32 lnkcap;
 
 	if (!pci_is_pcie(dev) || !pcie_downstream_port(dev) ||
 	    !pcie_cap_has_lnkctl2(dev) || !dev->link_active_reporting)
@@ -115,9 +116,25 @@ int pcie_failed_link_retrain(struct pci_dev *dev)
 			goto err;
 	}
 
+	pcie_capability_read_word(dev, PCI_EXP_LNKSTA, &lnksta);
 	pcie_capability_read_word(dev, PCI_EXP_LNKCTL2, &lnkctl2);
 	if ((lnkctl2 & PCI_EXP_LNKCTL2_TLS) == PCI_EXP_LNKCTL2_TLS_2_5GT) {
 		pci_info(dev, "removing 2.5GT/s downstream link speed restriction\n");
+
+		/*
+		 * With no link partner the retraining can never complete and
+		 * every attempt costs PCIE_LINK_RETRAIN_TIMEOUT_MS.  Program
+		 * the Target Link Speed directly and skip the retraining; the
+		 * link will train at that speed once a device shows up.
+		 */
+		if (!(lnksta & PCI_EXP_LNKSTA_DLLLA)) {
+			pcie_capability_read_dword(dev, PCI_EXP_LNKCAP, &lnkcap);
+			pcie_capability_clear_and_set_word(dev, PCI_EXP_LNKCTL2,
+							   PCI_EXP_LNKCTL2_TLS,
+							   lnkcap & PCI_EXP_LNKCAP_SLS);
+			return ret;
+		}
+
 		ret = pcie_set_target_speed(dev, speed_cap, false);
 		if (ret)
 			goto err;
Signature · 1 line
2.55.0
#1 lore raw
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] Deriving Target Link Speed directly from LNKCAP ignores LNKCAP2 and software constraints. - [High]…

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Deriving Target Link Speed directly from LNKCAP ignores LNKCAP2 and software constraints.
- [High] Lockless modification of Target Link Speed races with bandwidth controllers and misses LTSSM state synchronization.
- [Medium] Unhandled error from pcie_capability_read_dword() causes writing a Reserved value to Target Link Speed.
- [Medium] Reusing the ret variable can cause a false positive success return if the link drops concurrently. --

--- Patch [1]: [PATCH] PCI: Avoid link retraining on empty ports when lifting speed restriction --- commit 8c8f91b0ee4d3449b1bc98ead3ff414abb7453a9
Author: Andreas Wild <[email protected]>

PCI: Avoid link retraining on empty ports when lifting speed restriction

This commit speeds up boot times by skipping link retraining on unconnected downstream ports when lifting a 2.5GT/s firmware restriction. It directly programs the Target Link Speed instead of initiating retraining if the Data Link Layer Link Active is clear.

Quoted 12 lines
drivers/pci/quirks.c +1 -0
index b09f27f7846fc..d0c8b09712faf 100644
@@ -97,6 +97,7 @@ int pcie_failed_link_retrain(struct pci_dev *dev)
 	u16 lnksta, lnkctl2, oldlnkctl2;
 	enum pci_bus_speed speed_cap;
 	int ret = -ENOTTY;
+	u32 lnkcap;
 
 	if (!pci_is_pcie(dev) || !pcie_downstream_port(dev) ||
 	    !pcie_cap_has_lnkctl2(dev) || !dev->link_active_reporting)

[ ... ]

Quoted 7 lines
drivers/pci/quirks.c +1 -0
@@ -115,9 +116,25 @@ int pcie_failed_link_retrain(struct pci_dev *dev)
 			goto err;
 	}
 
+	pcie_capability_read_word(dev, PCI_EXP_LNKSTA, &lnksta);
 	pcie_capability_read_word(dev, PCI_EXP_LNKCTL2, &lnkctl2);
 	if ((lnkctl2 & PCI_EXP_LNKCTL2_TLS) == PCI_EXP_LNKCTL2_TLS_2_5GT) {

drivers/pci/quirks.c int pcie_failed_link_retrain(struct pci_dev *dev) · line 131

Quoted 10 lines
drivers/pci/quirks.c +9 -0
 		pci_info(dev, "removing 2.5GT/s downstream link speed restriction\n");
+
+		/*
+		 * With no link partner the retraining can never complete and
+		 * every attempt costs PCIE_LINK_RETRAIN_TIMEOUT_MS.  Program
+		 * the Target Link Speed directly and skip the retraining; the
+		 * link will train at that speed once a device shows up.
+		 */
+		if (!(lnksta & PCI_EXP_LNKSTA_DLLLA)) {
+			pcie_capability_read_dword(dev, PCI_EXP_LNKCAP, &lnkcap);

[Severity: Medium]
Could this result in writing a reserved value to Target Link Speed?

If pcie_capability_read_dword() encounters a transient bus error, it returns an error code and leaves lnkcap as 0. Because the return value isn't checked, lnkcap & PCI_EXP_LNKCAP_SLS would evaluate to 0000b (Reserved), which is then written to the PCI_EXP_LNKCTL2_TLS field.

drivers/pci/quirks.c int pcie_failed_link_retrain(struct pci_dev *dev) · line 134

Quoted 3 lines
drivers/pci/quirks.c +3 -0
+			pcie_capability_clear_and_set_word(dev, PCI_EXP_LNKCTL2,
+							   PCI_EXP_LNKCTL2_TLS,
+							   lnkcap & PCI_EXP_LNKCAP_SLS);

[Severity: High]
Does this bypass software speed constraints and PCIe 3.0+ requirements?

By deriving the Target Link Speed directly from LNKCAP, the code ignores PCI_EXP_LNKCAP2, which is mandatory for determining supported speeds on newer devices.

The PCI core already evaluates and caches the correct maximum speed in dev->supported_speeds, which is retrieved earlier in this function as speed_cap.

[Severity: High]
Is it safe to modify the Target Link Speed without subsystem locking or hardware retraining synchronization?

By calling pcie_capability_clear_and_set_word() directly, this bypasses pcie_set_target_speed(). This skips acquiring pcie_bwctrl_setspeed_rwsem and data->set_speed_mutex, potentially racing with thermal governors.

Furthermore, missing the pcie_retrain_link() call means that if a device is hot-plugged concurrently, the hardware LTSSM might not recognize the updated Target Link Speed.

drivers/pci/quirks.c int pcie_failed_link_retrain(struct pci_dev *dev) · line 135

Quoted 1 line
drivers/pci/quirks.c +1 -0
+			return ret;

[Severity: Medium]
Does returning ret here introduce a race condition if the link drops concurrently?

If the initial pcie_lbms_seen() workaround block earlier in this function successfully retrains the link, ret is set to 0.

If the link is unstable and drops immediately after, this second block reads DLLLA == 0. This early return will then execute with ret == 0, falsely signaling success when the link has actually dropped.

Quoted 5 lines
drivers/pci/quirks.c +2 -0
+		}
+
 		ret = pcie_set_target_speed(dev, speed_cap, false);
 		if (ret)
 			goto err;
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1