Re: [PATCH] PCI: Avoid link retraining on empty ports when lifting speed restriction

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.

> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index b09f27f7846fc..d0c8b09712faf 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -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);

[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.

> +			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.

> +			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.

> +		}
> +
>  		ret = pcie_set_target_speed(dev, speed_cap, false);
>  		if (ret)
>  			goto err;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1