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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <aCH65bOzLKMdMzQR@wunner.de>
Date: Mon, 12 May 2025 15:43:01 +0200
From: Lukas Wunner <lukas@...ner.de>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: Bjorn Helgaas <bhelgaas@...gle.com>, linux-pci@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/1] PCI: Update Link Speed after retraining

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@...ner.de>
> Link: https://lore.kernel.org/linux-pci/aBCjpfyYmlkJ12AZ@wunner.de/
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>

Code change itself is fine and

Reviewed-by: Lukas Wunner <lukas@...ner.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