[PATCH 1/1] PCI: Update Link Speed after retraining

[PATCH 1/1] PCI: Update Link Speed after retraining

[Ilpo Järvinen]

PCIe Link Retraining can alter Link Speed. While bwctrl listens for
Link Bandwidth Management Status (LBMS) to pick up changes in Link
Speed, there is a race between pcie_reset_lbms() clearing LBMS after
the Link Training and pcie_bwnotif_irq() reading the Link Status
register. If LBMS is already cleared when the irq handler reads the
register, the interrupt handler will return early with IRQ_NONE and
won't update the Link Speed.

When Link Speed update originates from bwctrl,
pcie_bwctrl_change_speed() ensures Link Speed is update after the
retraining. ASPM driver, however, calls pcie_retrain_link() but does
not update the Link Speed after retraining which can result in stale
Link Speed.

To ensure Link Speed is not left state after Link Training, move the
call to pcie_update_link_speed() from pcie_bwctrl_change_speed() into
pcie_retrain_link().

Suggested-by: Lukas Wunner <lukas@wunner.de>
Link: https://lore.kernel.org/linux-pci/aBCjpfyYmlkJ12AZ@wunner.de/
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---

Based on top of pci/bwctrl.

 drivers/pci/pci.c         | 17 +++++++++++++++++
 drivers/pci/pcie/bwctrl.c | 13 +------------
 2 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 3d94cf33c1b6..eb0c55078d5e 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4718,6 +4718,11 @@ static int pcie_wait_for_link_status(struct pci_dev *pdev,
  * @pdev: Device whose link to retrain.
  * @use_lt: Use the LT bit if TRUE, or the DLLLA bit if FALSE, for status.
  *
+ * Trigger retraining of the PCIe Link and wait for the completion of the
+ * retraining. As link retraining is known to asserts LBMS and may change
+ * the Link Speed, LBMS is cleared after the retraining and the Link Speed
+ * of the subordinate bus is updated.
+ *
  * Retrain completion status is retrieved from the Link Status Register
  * according to @use_lt.  It is not verified whether the use of the DLLLA
  * bit is valid.
@@ -4758,6 +4763,18 @@ int pcie_retrain_link(struct pci_dev *pdev, bool use_lt)
 	 * in attempt to correct unreliable link operation.
 	 */
 	pcie_reset_lbms(pdev);
+
+	/*
+	 * Ensure the Link Speed updates after retraining in case the Link
+	 * Speed was changed because of the retraining. While the bwctrl's
+	 * IRQ handler normally picks up the new Link Speed, clearing LBMS
+	 * races with the IRQ handler reading the Link Status register and
+	 * can result in the handler returning early without updating the
+	 * Link Speed.
+	 */
+	if (pdev->subordinate)
+		pcie_update_link_speed(pdev->subordinate);
+
 	return rc;
 }
 
diff --git a/drivers/pci/pcie/bwctrl.c b/drivers/pci/pcie/bwctrl.c
index fdafa20e4587..790a935b34fd 100644
--- a/drivers/pci/pcie/bwctrl.c
+++ b/drivers/pci/pcie/bwctrl.c
@@ -125,18 +125,7 @@ static int pcie_bwctrl_change_speed(struct pci_dev *port, u16 target_speed, bool
 	if (ret != PCIBIOS_SUCCESSFUL)
 		return pcibios_err_to_errno(ret);
 
-	ret = pcie_retrain_link(port, use_lt);
-	if (ret < 0)
-		return ret;
-
-	/*
-	 * Ensure link speed updates also with platforms that have problems
-	 * with notifications.
-	 */
-	if (port->subordinate)
-		pcie_update_link_speed(port->subordinate);
-
-	return 0;
+	return pcie_retrain_link(port, use_lt);
 }
 
 /**

base-commit: 0238f352a63a075ac1f35ea565b5bec3057ec8bd
-- 
2.39.5

Re: [PATCH 1/1] PCI: Update Link Speed after retraining

[Lukas Wunner]

On Mon, May 12, 2025 at 04:01:24PM +0300, Ilpo Järvinen wrote:
> PCIe Link Retraining can alter Link Speed. While bwctrl listens for
> Link Bandwidth Management Status (LBMS) to pick up changes in Link
> Speed, there is a race between pcie_reset_lbms() clearing LBMS after
> the Link Training and pcie_bwnotif_irq() reading the Link Status
> register. If LBMS is already cleared when the irq handler reads the
> register, the interrupt handler will return early with IRQ_NONE and
> won't update the Link Speed.
> 
> When Link Speed update originates from bwctrl,
> pcie_bwctrl_change_speed() ensures Link Speed is update after the
                                                   ^^^^^^
Nit: updated

> retraining. ASPM driver, however, calls pcie_retrain_link() but does
> not update the Link Speed after retraining which can result in stale
> Link Speed.
> 
> To ensure Link Speed is not left state after Link Training, move the
                                   ^^^^^
Nit: stale

> call to pcie_update_link_speed() from pcie_bwctrl_change_speed() into
> pcie_retrain_link().

Actually aspm.c is compiled into the kernel even if CONFIG_PCIEPORTBUS=n,
so it's possible to have ASPM support without also having the bandwidth
controller.  And in that case it could likewise happen that retraining
by aspm.c results in a different link speed which needs to be updated.

Hence the discussion in the commit message and code comments about
a race with the bandwidth controller's IRQ handler seems somewhat
misleading.  Yes, if the bandwidth controller is enabled it will
pick up the new speed if aspm.c's retraining results in a speed change,
but may fail to do so because of the race.  But aspm.c needs to update
the link speed regardless because the bandwidth controller may not
even be enabled in the first place.

> Suggested-by: Lukas Wunner <lukas@wunner.de>
> Link: https://lore.kernel.org/linux-pci/aBCjpfyYmlkJ12AZ@wunner.de/
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

Code change itself is fine and

Reviewed-by: Lukas Wunner <lukas@wunner.de>

> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -4718,6 +4718,11 @@ static int pcie_wait_for_link_status(struct pci_dev *pdev,
>   * @pdev: Device whose link to retrain.
>   * @use_lt: Use the LT bit if TRUE, or the DLLLA bit if FALSE, for status.
>   *
> + * Trigger retraining of the PCIe Link and wait for the completion of the
> + * retraining. As link retraining is known to asserts LBMS and may change
                                                 ^^^^^^^
Nit: assert

Thanks,

Lukas