On Fri, 10 Jan 2025, Jiwei Sun wrote:
> From: Jiwei Sun <sunjw10@xxxxxxxxxx>
>
> Since commit de9a6c8d5dbf ("PCI/bwctrl: Add pcie_set_target_speed() to set
> PCIe Link Speed"), there are two potential issues in the function
> pcie_failed_link_retrain().
>
> (1) The macro PCIE_LNKCTL2_TLS2SPEED() and PCIE_LNKCAP_SLS2SPEED() just
> uses the link speed field of the registers. However, there are many other
> different function fields in the Link Control 2 Register or the Link
> Capabilities Register. If the register value is directly used by the two
> macros, it may cause getting an error link speed value (PCI_SPEED_UNKNOWN).
>
> (2) In the pcie_failed_link_retrain(), the local variable lnkctl2 is not
> changed after reading from PCI_EXP_LNKCTL2. It might cause that the
> removing 2.5GT/s downstream link speed restriction codes are not executed.
Thanks for finding these issues and coming up with a patch.
These cases seem two different problems to me so I'd put them into
different patches, which would also make it easeir focus on
describing the issue in case 2 which is currently a bit vague (when
looking how de9a6c8d5dbf managed to break it by removing the lnkctl2
change prior to writing into the reg).
> In order to avoid the above-mentioned potential issues, only keep link
> speed field of the two registers before using by pcie_set_target_speed()
> and reread the Link Control 2 Register before using.
>
> Fixes: de9a6c8d5dbf ("PCI/bwctrl: Add pcie_set_target_speed() to set PCIe Link Speed")
> Signed-off-by: Jiwei Sun <sunjw10@xxxxxxxxxx>
> ---
> drivers/pci/quirks.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 76f4df75b08a..605628c810a5 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -118,11 +118,13 @@ int pcie_failed_link_retrain(struct pci_dev *dev)
> ret = pcie_set_target_speed(dev, PCIE_SPEED_2_5GT, false);
> if (ret) {
> pci_info(dev, "retraining failed\n");
> + oldlnkctl2 &= PCI_EXP_LNKCTL2_TLS;
> pcie_set_target_speed(dev, PCIE_LNKCTL2_TLS2SPEED(oldlnkctl2),
> true);
I'd prefer these get fixed inside the macros so that the callers don't
need to take handle what seem something that is always needed.
> return ret;
> }
>
> + pcie_capability_read_word(dev, PCI_EXP_LNKCTL2, &lnkctl2);
> pcie_capability_read_word(dev, PCI_EXP_LNKSTA, &lnksta);
> }
>
> @@ -133,6 +135,7 @@ int pcie_failed_link_retrain(struct pci_dev *dev)
>
> pci_info(dev, "removing 2.5GT/s downstream link speed restriction\n");
> pcie_capability_read_dword(dev, PCI_EXP_LNKCAP, &lnkcap);
> + lnkcap &= PCI_EXP_LNKCAP_SLS;
> ret = pcie_set_target_speed(dev, PCIE_LNKCAP_SLS2SPEED(lnkcap), false);
--
i.
