[PATCH v2 0/4] PCI: Rework error reporting with PCIe failed link retraining

[PATCH v2 0/4] PCI: Rework error reporting with PCIe failed link retraining

[Maciej W. Rozycki]

Hi,

 This is v2 superseding a patch series originally posted here: 
<https://lore.kernel.org/r/alpine.DEB.2.21.2402092125070.2376@angie.orcam.me.uk/>.

 This patch series addresses issues observed by Ilpo as reported here: 
<https://lore.kernel.org/r/aa2d1c4e-9961-d54a-00c7-ddf8e858a9b0@linux.intel.com/>, 
one with excessive delays happening when `pcie_failed_link_retrain' is 
called, but link retraining has not been actually attempted, and another 
one with an API misuse caused by a merge mistake.

 It also addresses an issue observed by Matthew as discussed here: 
<https://lore.kernel.org/r/20240806000659.30859-1-mattc@purestorage.com/> 
and here: 
<https://lore.kernel.org/r/20240722193407.23255-1-mattc@purestorage.com/>. 
where a stale LBMS bit state causes `pcie_failed_link_retrain', in the 
absence of a downstream device, to leave the link stuck at the 2.5GT/s 
speed rate, which then negatively impacts devices plugged in in the 
future.

 See individual change description for further details; 1/4 and 2/4 are 
new changes, 3/4 supersedes: 
<https://patchwork.kernel.org/project/linux-pci/patch/alpine.DEB.2.21.2402100045590.2376@angie.orcam.me.uk/>, 
and 4/4 supersedes: 
<https://patchwork.kernel.org/project/linux-pci/patch/alpine.DEB.2.21.2402100048440.2376@angie.orcam.me.uk/>.

 These changes have been verified with a SiFive HiFive Unmatched system, 
also using a small debug change to verify that the state of the LBMS bit 
is clear at the exit from `pcie_failed_link_retrain'.

 Ilpo, since 3/4 and 4/4 have only been trivially updated and their 
combined effect is not changed even I chose to retain your Reviewed-by 
tags from v1.  Let me know if you disagree and what to do so you don't.

 I apologise to take so long, it's been a tough period to me load-wise.

  Maciej

[PATCH v2 1/4] PCI: Clear the LBMS bit after a link retrain

[Maciej W. Rozycki]

The LBMS bit, where implemented, is set by hardware either in response 
to the completion of retraining caused by writing 1 to the Retrain Link 
bit or whenever hardware has changed the link speed or width in attempt 
to correct unreliable link operation.  It is never cleared by hardware 
other than by software writing 1 to the bit position in the Link Status 
register and we never do such a write.

We currently have two places, namely `apply_bad_link_workaround' and 
`pcie_failed_link_retrain' in drivers/pci/controller/dwc/pcie-tegra194.c 
and drivers/pci/quirks.c respectively where we check the state of the 
LBMS bit and neither is interested in the state of the bit resulting 
from the completion of retraining, both check for a link fault.

And in particular `pcie_failed_link_retrain' causes issues consequently, 
by trying to retrain a link where there's no downstream device anymore 
and the state of 1 in the LBMS bit has been retained from when there was 
a device downstream that has since been removed.

Clear the LBMS bit then at the conclusion of `pcie_retrain_link', so 
that we have a single place that controls it and that our code can track 
link speed or width changes resulting from unreliable link operation.

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+
---
New change in v2.
---
 drivers/pci/pci.c |   10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

linux-pcie-retrain-link-lbms-clear.diff
Index: linux-macro/drivers/pci/pci.c
===================================================================
--- linux-macro.orig/drivers/pci/pci.c
+++ linux-macro/drivers/pci/pci.c
@@ -4718,7 +4718,15 @@ int pcie_retrain_link(struct pci_dev *pd
 		pcie_capability_clear_word(pdev, PCI_EXP_LNKCTL, PCI_EXP_LNKCTL_RL);
 	}
 
-	return pcie_wait_for_link_status(pdev, use_lt, !use_lt);
+	rc = pcie_wait_for_link_status(pdev, use_lt, !use_lt);
+
+	/*
+	 * Clear LBMS after a manual retrain so that the bit can be used
+	 * to track link speed or width changes made by hardware itself
+	 * in attempt to correct unreliable link operation.
+	 */
+	pcie_capability_write_word(pdev, PCI_EXP_LNKSTA, PCI_EXP_LNKSTA_LBMS);
+	return rc;
 }
 
 /**

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

[Maciej W. Rozycki]

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@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+
---
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);
 			return false;
 		}
 

[PATCH v2 3/4] PCI: Correct error reporting with PCIe failed link retraining

[Maciej W. Rozycki]

Only return successful completion status from `pcie_failed_link_retrain' 
if retraining has actually been done, preventing excessive delays from 
being triggered at call sites in a hope that communication will finally 
be established with the downstream device where in fact nothing has been 
done about the link in question that would justify such a hope.

Fixes: a89c82249c37 ("PCI: Work around PCIe link training failures")
Reported-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Link: https://lore.kernel.org/r/aa2d1c4e-9961-d54a-00c7-ddf8e858a9b0@linux.intel.com/
Signed-off-by: Maciej W. Rozycki <macro@orcam.me.uk>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Cc: stable@vger.kernel.org # v6.5+
---
Changes from v1 (superseding 1/2):

- Regenerate on top of 2/4.

- Reword the comment update for clarity.

- Go back to returning explicit FALSE rather than `ret' where it is known 
  that we failed.
---
 drivers/pci/quirks.c |   12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

linux-pcie-failed-link-retrain-status-fix.diff
Index: linux-macro/drivers/pci/quirks.c
===================================================================
--- linux-macro.orig/drivers/pci/quirks.c
+++ linux-macro/drivers/pci/quirks.c
@@ -78,7 +78,8 @@
  * 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.
+ * Return TRUE if the link has been successfully retrained.  Return FALSE
+ * if retraining was not needed or we attempted a retrain and it failed.
  */
 bool pcie_failed_link_retrain(struct pci_dev *dev)
 {
@@ -87,6 +88,7 @@ bool pcie_failed_link_retrain(struct pci
 		{}
 	};
 	u16 lnksta, lnkctl2;
+	bool ret = false;
 
 	if (!pci_is_pcie(dev) || !pcie_downstream_port(dev) ||
 	    !pcie_cap_has_lnkctl2(dev) || !dev->link_active_reporting)
@@ -104,7 +106,8 @@ bool pcie_failed_link_retrain(struct pci
 		lnkctl2 |= PCI_EXP_LNKCTL2_TLS_2_5GT;
 		pcie_capability_write_word(dev, PCI_EXP_LNKCTL2, lnkctl2);
 
-		if (pcie_retrain_link(dev, false)) {
+		ret = pcie_retrain_link(dev, false) == 0;
+		if (!ret) {
 			pci_info(dev, "retraining failed\n");
 			pcie_capability_write_word(dev, PCI_EXP_LNKCTL2,
 						   oldlnkctl2);
@@ -126,13 +129,14 @@ bool pcie_failed_link_retrain(struct pci
 		lnkctl2 |= lnkcap & PCI_EXP_LNKCAP_SLS;
 		pcie_capability_write_word(dev, PCI_EXP_LNKCTL2, lnkctl2);
 
-		if (pcie_retrain_link(dev, false)) {
+		ret = pcie_retrain_link(dev, false) == 0;
+		if (!ret) {
 			pci_info(dev, "retraining failed\n");
 			return false;
 		}
 	}
 
-	return true;
+	return ret;
 }
 
 static ktime_t fixup_debug_start(struct pci_dev *dev,

[PATCH v2 4/4] PCI: Use an error code with PCIe failed link retraining

[Maciej W. Rozycki]

Given how the call place in `pcie_wait_for_link_delay' got structured 
now, and that `pcie_retrain_link' returns a potentially useful error 
code, convert `pcie_failed_link_retrain' to return an error code rather 
than a boolean status, fixing handling at the call site mentioned.  
Update the other call site accordingly.

Fixes: 1abb47390350 ("Merge branch 'pci/enumeration'")
Reported-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Link: https://lore.kernel.org/r/aa2d1c4e-9961-d54a-00c7-ddf8e858a9b0@linux.intel.com/
Signed-off-by: Maciej W. Rozycki <macro@orcam.me.uk>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Cc: stable@vger.kernel.org # v6.5+
---
Changes from v1 (superseding 2/2):

- Regenerate following changes to 3/4, no functional change.
---
 drivers/pci/pci.c    |    2 +-
 drivers/pci/pci.h    |    6 +++---
 drivers/pci/quirks.c |   20 ++++++++++----------
 3 files changed, 14 insertions(+), 14 deletions(-)

linux-pcie-failed-link-retrain-status-int.diff
Index: linux-macro/drivers/pci/pci.c
===================================================================
--- linux-macro.orig/drivers/pci/pci.c
+++ linux-macro/drivers/pci/pci.c
@@ -1324,7 +1324,7 @@ static int pci_dev_wait(struct pci_dev *
 		if (delay > PCI_RESET_WAIT) {
 			if (retrain) {
 				retrain = false;
-				if (pcie_failed_link_retrain(bridge)) {
+				if (pcie_failed_link_retrain(bridge) == 0) {
 					delay = 1;
 					continue;
 				}
Index: linux-macro/drivers/pci/pci.h
===================================================================
--- linux-macro.orig/drivers/pci/pci.h
+++ linux-macro/drivers/pci/pci.h
@@ -606,7 +606,7 @@ void pci_acs_init(struct pci_dev *dev);
 int pci_dev_specific_acs_enabled(struct pci_dev *dev, u16 acs_flags);
 int pci_dev_specific_enable_acs(struct pci_dev *dev);
 int pci_dev_specific_disable_acs_redir(struct pci_dev *dev);
-bool pcie_failed_link_retrain(struct pci_dev *dev);
+int pcie_failed_link_retrain(struct pci_dev *dev);
 #else
 static inline int pci_dev_specific_acs_enabled(struct pci_dev *dev,
 					       u16 acs_flags)
@@ -621,9 +621,9 @@ static inline int pci_dev_specific_disab
 {
 	return -ENOTTY;
 }
-static inline bool pcie_failed_link_retrain(struct pci_dev *dev)
+static inline int pcie_failed_link_retrain(struct pci_dev *dev)
 {
-	return false;
+	return -ENOTTY;
 }
 #endif
 
Index: linux-macro/drivers/pci/quirks.c
===================================================================
--- linux-macro.orig/drivers/pci/quirks.c
+++ linux-macro/drivers/pci/quirks.c
@@ -78,21 +78,21 @@
  * 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.  Return FALSE
+ * Return 0 if the link has been successfully retrained.  Return an error
  * if retraining was not needed or we attempted a retrain and it failed.
  */
-bool pcie_failed_link_retrain(struct pci_dev *dev)
+int pcie_failed_link_retrain(struct pci_dev *dev)
 {
 	static const struct pci_device_id ids[] = {
 		{ PCI_VDEVICE(ASMEDIA, 0x2824) }, /* ASMedia ASM2824 */
 		{}
 	};
 	u16 lnksta, lnkctl2;
-	bool ret = false;
+	int ret = -ENOTTY;
 
 	if (!pci_is_pcie(dev) || !pcie_downstream_port(dev) ||
 	    !pcie_cap_has_lnkctl2(dev) || !dev->link_active_reporting)
-		return false;
+		return ret;
 
 	pcie_capability_read_word(dev, PCI_EXP_LNKCTL2, &lnkctl2);
 	pcie_capability_read_word(dev, PCI_EXP_LNKSTA, &lnksta);
@@ -106,13 +106,13 @@ bool pcie_failed_link_retrain(struct pci
 		lnkctl2 |= PCI_EXP_LNKCTL2_TLS_2_5GT;
 		pcie_capability_write_word(dev, PCI_EXP_LNKCTL2, lnkctl2);
 
-		ret = pcie_retrain_link(dev, false) == 0;
-		if (!ret) {
+		ret = pcie_retrain_link(dev, false);
+		if (ret) {
 			pci_info(dev, "retraining failed\n");
 			pcie_capability_write_word(dev, PCI_EXP_LNKCTL2,
 						   oldlnkctl2);
 			pcie_retrain_link(dev, false);
-			return false;
+			return ret;
 		}
 
 		pcie_capability_read_word(dev, PCI_EXP_LNKSTA, &lnksta);
@@ -129,10 +129,10 @@ bool pcie_failed_link_retrain(struct pci
 		lnkctl2 |= lnkcap & PCI_EXP_LNKCAP_SLS;
 		pcie_capability_write_word(dev, PCI_EXP_LNKCTL2, lnkctl2);
 
-		ret = pcie_retrain_link(dev, false) == 0;
-		if (!ret) {
+		ret = pcie_retrain_link(dev, false);
+		if (ret) {
 			pci_info(dev, "retraining failed\n");
-			return false;
+			return ret;
 		}
 	}
 

Re: [PATCH v2 0/4] PCI: Rework error reporting with PCIe failed link retraining

[Bjorn Helgaas]

On Fri, Aug 09, 2024 at 02:24:40PM +0100, Maciej W. Rozycki wrote:
> Hi,
> 
>  This is v2 superseding a patch series originally posted here: 
> <https://lore.kernel.org/r/alpine.DEB.2.21.2402092125070.2376@angie.orcam.me.uk/>.
> 
>  This patch series addresses issues observed by Ilpo as reported here: 
> <https://lore.kernel.org/r/aa2d1c4e-9961-d54a-00c7-ddf8e858a9b0@linux.intel.com/>, 
> one with excessive delays happening when `pcie_failed_link_retrain' is 
> called, but link retraining has not been actually attempted, and another 
> one with an API misuse caused by a merge mistake.
> 
>  It also addresses an issue observed by Matthew as discussed here: 
> <https://lore.kernel.org/r/20240806000659.30859-1-mattc@purestorage.com/> 
> and here: 
> <https://lore.kernel.org/r/20240722193407.23255-1-mattc@purestorage.com/>. 
> where a stale LBMS bit state causes `pcie_failed_link_retrain', in the 
> absence of a downstream device, to leave the link stuck at the 2.5GT/s 
> speed rate, which then negatively impacts devices plugged in in the 
> future.
> 
>  See individual change description for further details; 1/4 and 2/4 are 
> new changes, 3/4 supersedes: 
> <https://patchwork.kernel.org/project/linux-pci/patch/alpine.DEB.2.21.2402100045590.2376@angie.orcam.me.uk/>, 
> and 4/4 supersedes: 
> <https://patchwork.kernel.org/project/linux-pci/patch/alpine.DEB.2.21.2402100048440.2376@angie.orcam.me.uk/>.
> 
>  These changes have been verified with a SiFive HiFive Unmatched system, 
> also using a small debug change to verify that the state of the LBMS bit 
> is clear at the exit from `pcie_failed_link_retrain'.
> 
>  Ilpo, since 3/4 and 4/4 have only been trivially updated and their 
> combined effect is not changed even I chose to retain your Reviewed-by 
> tags from v1.  Let me know if you disagree and what to do so you don't.
> 
>  I apologise to take so long, it's been a tough period to me load-wise.

Applied to pci/enumeration for v6.12, thanks for all this debugging
and work.

Matthew, let me know if this addresses the problems you saw, and I can
add your tested-by if appropriate.

Bjorn

Re: [PATCH v2 1/4] PCI: Clear the LBMS bit after a link retrain

[Ilpo Järvinen]

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

> The LBMS bit, where implemented, is set by hardware either in response 
> to the completion of retraining caused by writing 1 to the Retrain Link 
> bit or whenever hardware has changed the link speed or width in attempt 
> to correct unreliable link operation.  It is never cleared by hardware 
> other than by software writing 1 to the bit position in the Link Status 
> register and we never do such a write.
> 
> We currently have two places, namely `apply_bad_link_workaround' and 
> `pcie_failed_link_retrain' in drivers/pci/controller/dwc/pcie-tegra194.c 
> and drivers/pci/quirks.c respectively where we check the state of the 
> LBMS bit and neither is interested in the state of the bit resulting 
> from the completion of retraining, both check for a link fault.
> 
> And in particular `pcie_failed_link_retrain' causes issues consequently, 
> by trying to retrain a link where there's no downstream device anymore 
> and the state of 1 in the LBMS bit has been retained from when there was 
> a device downstream that has since been removed.
> 
> Clear the LBMS bit then at the conclusion of `pcie_retrain_link', so 
> that we have a single place that controls it and that our code can track 
> link speed or width changes resulting from unreliable link operation.
> 
> 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+
> ---
> New change in v2.
> ---
>  drivers/pci/pci.c |   10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> linux-pcie-retrain-link-lbms-clear.diff
> Index: linux-macro/drivers/pci/pci.c
> ===================================================================
> --- linux-macro.orig/drivers/pci/pci.c
> +++ linux-macro/drivers/pci/pci.c
> @@ -4718,7 +4718,15 @@ int pcie_retrain_link(struct pci_dev *pd
>  		pcie_capability_clear_word(pdev, PCI_EXP_LNKCTL, PCI_EXP_LNKCTL_RL);
>  	}
>  
> -	return pcie_wait_for_link_status(pdev, use_lt, !use_lt);
> +	rc = pcie_wait_for_link_status(pdev, use_lt, !use_lt);
> +
> +	/*
> +	 * Clear LBMS after a manual retrain so that the bit can be used
> +	 * to track link speed or width changes made by hardware itself
> +	 * in attempt to correct unreliable link operation.
> +	 */
> +	pcie_capability_write_word(pdev, PCI_EXP_LNKSTA, PCI_EXP_LNKSTA_LBMS);

I see Bjorn already took this series in which is good, it's long overdue 
we finally fix the bugs addressed by this series. :-)

I'm slightly worried this particular change could interfere with the 
intent of pcie_failed_link_retrain() because LBMS is cleared also in the 
failure cases.

In the case of your HW, there's retraining loop by HW so LBMS gets set 
again but if the HW would not retrain in a loop and needs similar gen1 
bootstrap, it's very non-obvious to me how things will end up interacting 
with pcie_retrain_link() call from pcie_aspm_configure_common_clock(). 
That is, this could clear the LBMS indication and another is not going to 
be asserted (and even in case of with the retraining loop, it would be 
racy to get LBMS re-asserted soon enough).

My impression is that things seem to work with the current ordering of the 
code but it seems quite fragile (however, the callchains are quite 
complicated to track so I might have missed something). Perhaps that won't 
matter much in the end with the bandwidth controller coming to rework all 
this anyway but wanted to note this may have caveats.

> +	return rc;
>  }
>  
>  /**
> 

-- 
 i.

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

[Ilpo Järvinen]

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

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@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+
> ---
> 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);
>  			return false;
>  		}

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.

Re: [PATCH v2 1/4] PCI: Clear the LBMS bit after a link retrain

[Maciej W. Rozycki]

On Mon, 12 Aug 2024, Ilpo Järvinen wrote:

> I'm slightly worried this particular change could interfere with the 
> intent of pcie_failed_link_retrain() because LBMS is cleared also in the 
> failure cases.

 I think it doesn't really matter, because in a correctly operating system 
LBMS is not supposed to be set at the point `pcie_failed_link_retrain' is 
called in the first place.  We don't want to respond to LBMS being set 
just as a consequence of writing 1 to the Retrain Link bit, because it is 
always set in this scenario even for open links and we know we've did the 
retraining anyway, so we can communicate it via other means if we need to.

> In the case of your HW, there's retraining loop by HW so LBMS gets set 
> again but if the HW would not retrain in a loop and needs similar gen1 
> bootstrap, it's very non-obvious to me how things will end up interacting 
> with pcie_retrain_link() call from pcie_aspm_configure_common_clock(). 
> That is, this could clear the LBMS indication and another is not going to 
> be asserted (and even in case of with the retraining loop, it would be 
> racy to get LBMS re-asserted soon enough).

 Yes, and it is an intended effect.  We only want to trigger for LBMS set 
by hardware in an attempt to correct unreliable link operation.

> My impression is that things seem to work with the current ordering of the 
> code but it seems quite fragile (however, the callchains are quite 
> complicated to track so I might have missed something). Perhaps that won't 
> matter much in the end with the bandwidth controller coming to rework all 
> this anyway but wanted to note this may have caveats.

 I look forward to the outcome of your effort.  I expect you'll remove 
this part then and handle the clearing of the LBMS bit in the bandwidth 
controller interrupt handler.

  Maciej

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

[Matthew W Carlis]

Sorry past few days have been struggling to buy some time to look at these. Every
time I go into the store and ask for more time they try to give me phone cards.
Its an improvement to restore the "capable" link speed & in this case I think we're
also enabling a user to have over-ridden it if they were to write the register
by hand. Its nice to give every potentially "new" device a chance at achieving
its full potential.
A little outside of the scope of this patch, but I was wondering if the logline
should be a warn level logline? I don't honestly know what a "normal" level is
for the pci system. Also, instead of saying  "retraining ... at 2.5GT/s\n",
would it be more clear if it said "forcing downstream link to 2.5GT/s". In my mind
its more clear that an action has been taken which produces a potentially less
than unexpected speed.

I can test this patch on a couple of systems, but I need a week or so to
get it done...

Reviewed-by: Matthew Carlis <mattc@purestorage.com>

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

[Ilpo Järvinen]

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@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+
> ---
> 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.

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

[Maciej W. Rozycki]

On Thu, 22 Aug 2024, Ilpo Järvinen wrote:

> > 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
> > @@ -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.

 Good point, I haven't given this part much thought and just pasted the 
same call to `pcie_retrain_link' as above.  At worst we'll hit the same 
timeout, but as you write we don't expect DLLLA to ever come up at this 
point and LT may come down long before the timeout (and if it oscillates 
and we probe at the wrong moments, then we'll hit the timeout anyway).

> Do you agree?

 I do.  Thank you for your suggestion, I have posted v3 now with your
suggested update only.

  Maciej