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

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 4050 bytes --]

Hi Bjorn,

You still seem to have the old version of this patch in enumeration 
branch.

Could you please consider replacing it with this v3 one that is slightly 
better (use_lt was changed to true because it makes more sense).

On Mon, 26 Aug 2024, Ilpo Järvinen wrote:
> On Sun, 25 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 so as to remove any 
> > residual state, waiting on LT rather than DLLLA to avoid an excessive 
> > delay and ignoring the result as this training is supposed to fail anyway.
> > 
> > Fixes: a89c82249c37 ("PCI: Work around PCIe link training failures")
> > Reported-by: Matthew W Carlis <mattc@purestorage.com>
> > Link: https://lore.kernel.org/r/20240806000659.30859-1-mattc@purestorage.com/
> > Link: https://lore.kernel.org/r/20240722193407.23255-1-mattc@purestorage.com/
> > Signed-off-by: Maciej W. Rozycki <macro@orcam.me.uk>
> > Cc: stable@vger.kernel.org # v6.5+
> > ---
> > Changes from v2:
> > 
> > - Wait on LT rather than DLLLA with clean-up retraining with the speed 
> >   restriction lifted, so as to avoid an excessive delay as it's supposed 
> >   to fail anyway.
> > 
> > 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, true);
> 
> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>


-- 
 i.