Re: [PATCH v2 2/4] PCI: Revert to the original speed after PCIe failed link retraining

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, 9 Aug 2024, Maciej W. Rozycki wrote:

> When `pcie_failed_link_retrain' has failed to retrain the link by hand 
> it leaves the link speed restricted to 2.5GT/s, which will then affect 
> any device that has been plugged in later on, which may not suffer from 
> the problem that caused the speed restriction to have been attempted.  
> Consequently such a downstream device will suffer from an unnecessary 
> communication throughput limitation and therefore performance loss.
> 
> Remove the speed restriction then and revert the Link Control 2 register 
> to its original state if link retraining with the speed restriction in 
> place has failed.  Retrain the link again afterwards to remove any 
> residual state, ignoring the result as it's supposed to fail anyway.
> 
> Fixes: a89c82249c37 ("PCI: Work around PCIe link training failures")
> Reported-by: Matthew W Carlis <mattc@xxxxxxxxxxxxxxx>
> Link: https://lore.kernel.org/r/20240806000659.30859-1-mattc@xxxxxxxxxxxxxxx/
> Link: https://lore.kernel.org/r/20240722193407.23255-1-mattc@xxxxxxxxxxxxxxx/
> Signed-off-by: Maciej W. Rozycki <macro@xxxxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx # v6.5+
> ---
> New change in v2.
> ---
>  drivers/pci/quirks.c |   11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> linux-pcie-failed-link-retrain-fail-unclamp.diff
> Index: linux-macro/drivers/pci/quirks.c
> ===================================================================
> --- linux-macro.orig/drivers/pci/quirks.c
> +++ linux-macro/drivers/pci/quirks.c
> @@ -66,7 +66,7 @@
>   * apply this erratum workaround to any downstream ports as long as they
>   * support Link Active reporting and have the Link Control 2 register.
>   * Restrict the speed to 2.5GT/s then with the Target Link Speed field,
> - * request a retrain and wait 200ms for the data link to go up.
> + * request a retrain and check the result.
>   *
>   * If this turns out successful and we know by the Vendor:Device ID it is
>   * safe to do so, then lift the restriction, letting the devices negotiate
> @@ -74,6 +74,10 @@
>   * firmware may have already arranged and lift it with ports that already
>   * report their data link being up.
>   *
> + * Otherwise revert the speed to the original setting and request a retrain
> + * again to remove any residual state, ignoring the result as it's supposed
> + * to fail anyway.
> + *
>   * Return TRUE if the link has been successfully retrained, otherwise FALSE.
>   */
>  bool pcie_failed_link_retrain(struct pci_dev *dev)
> @@ -92,6 +96,8 @@ bool pcie_failed_link_retrain(struct pci
>  	pcie_capability_read_word(dev, PCI_EXP_LNKSTA, &lnksta);
>  	if ((lnksta & (PCI_EXP_LNKSTA_LBMS | PCI_EXP_LNKSTA_DLLLA)) ==
>  	    PCI_EXP_LNKSTA_LBMS) {
> +		u16 oldlnkctl2 = lnkctl2;
> +
>  		pci_info(dev, "broken device, retraining non-functional downstream link at 2.5GT/s\n");
>  
>  		lnkctl2 &= ~PCI_EXP_LNKCTL2_TLS;
> @@ -100,6 +106,9 @@ bool pcie_failed_link_retrain(struct pci
>  
>  		if (pcie_retrain_link(dev, false)) {
>  			pci_info(dev, "retraining failed\n");
> +			pcie_capability_write_word(dev, PCI_EXP_LNKCTL2,
> +						   oldlnkctl2);
> +			pcie_retrain_link(dev, false);

Hi again all,

While rebasing the bandwidth controller patches, I revisited this line and 
realized using false for use_lt is not optimal here.

It would definitely seem better to use LT (true) in this case because it 
likely results in much shorter wait. In hotplug cases w/o a peer device, 
DLLLA will just make the wait last until the timeout, whereas LT would 
short-circuit the training almost right away I think (mostly guessing with 
limited knowledge about LTSSM). We are no longer even expecting the link 
to come up at this point so using DLLLA seems illogical.

Do you agree?

-- 
 i.





[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux