Re: [PATCH v9 00/14] pci: Work around ASMedia ASM2824 PCIe link training failures

[Maciej W. Rozycki]

On Fri, 16 Jun 2023, Bjorn Helgaas wrote:

> I agree that as I rearranged it, the workaround doesn't apply in all
> cases simultaneously.  Maybe not ideal, but maybe not terrible either.
> Looking at it again, maybe it would have made more sense to move the
> pcie_wait_for_link_delay() change to the last patch along with the
> pci_dev_wait() change.  I dunno.

 I think the order of the changes is not important enough to justify 
spending a lot of time and mental effort on it.  You decided, so be it.  
Thank you for your effort made with this review.

 With this series out of the way I have now posted a small clean-up for 
SBR code duplication between PCI core and an InfiniBand driver I came 
across in the course of working on this series.  See 
.

 Please have a look at your convenience.

  Maciej

Re: [PATCH v9 00/14] pci: Work around ASMedia ASM2824 PCIe link training failures

[Bjorn Helgaas]

On Fri, Jun 16, 2023 at 01:27:52PM +0100, Maciej W. Rozycki wrote:
> On Thu, 15 Jun 2023, Bjorn Helgaas wrote:

>  As per my earlier remark:
> 
> > I think making a system halfway-fixed would make little sense, but with
> > the actual fix actually made last as you suggested I think this can be
> > split off, because it'll make no functional change by itself.
> 
> I am not perfectly happy with your rearrangement to fold the !PCI_QUIRKS 
> stub into the change carrying the actual workaround and then have the 
> reset path update with a follow-up change only, but I won't fight over it.  
> It's only one tree revision that will be in this halfway-fixed state and 
> I'll trust your judgement here.

Thanks for raising this.  Here's my thought process:

  12 PCI: Provide stub failed link recovery for device probing and hot plug
  13 PCI: Add failed link recovery for device reset events
  14 PCI: Work around PCIe link training failures

Patch 12 [1] adds calls to pcie_failed_link_retrain(), which does
nothing and returns false.  Functionally, it's a no-op, but the
structure is important later.

Patch 13 [2] claims to request failed link recovery after resets, but
actually doesn't do anything yet because pcie_failed_link_retrain() is
still a no-op, so this was a bit confusing.

Patch 14 [3] implements pcie_failed_link_retrain(), so the recovery
mentioned in 12 and 13 actually happens.  But this patch doesn't add
the call to pcie_failed_link_retrain(), so it's a little bit hard to
connect the dots.

I agree that as I rearranged it, the workaround doesn't apply in all
cases simultaneously.  Maybe not ideal, but maybe not terrible either.
Looking at it again, maybe it would have made more sense to move the
pcie_wait_for_link_delay() change to the last patch along with the
pci_dev_wait() change.  I dunno.

Bjorn

[1] 12 
https://lore.kernel.org/r/alpine.deb.2.21.2306111619570.64...@angie.orcam.me.uk
[2] 13 
https://lore.kernel.org/r/alpine.deb.2.21.2306111631050.64...@angie.orcam.me.uk
[3] 14 
https://lore.kernel.org/r/alpine.deb.2.21.2305310038540.59...@angie.orcam.me.uk

Re: [PATCH v9 00/14] pci: Work around ASMedia ASM2824 PCIe link training failures

[Maciej W. Rozycki]

On Thu, 15 Jun 2023, Bjorn Helgaas wrote:

> >  If doing it this way, which I actually like, I think it would be a little 
> > bit better performance- and style-wise if this was written as:
> > 
> > if (pci_is_pcie(dev)) {
> > bridge = pci_upstream_bridge(dev);
> > retrain = !!bridge;
> > }
> > 
> > (or "retrain = bridge != NULL" if you prefer this style), and then we 
> > don't have to repeatedly check two variables iff (pcie && !bridge) in the 
> > loop below:
> 
> Done, thanks, I do like that better.  I did:
> 
>   bridge = pci_upstream_bridge(dev);
>   if (bridge)
> retrain = true;
> 
> because it seems like it flows more naturally when reading.

 Perfect, and good timing too, as I have just started checking your tree 
as your message arrived.  I ran my usual tests with and w/o PCI_QUIRKS 
enabled and results were as expected.  As before I didn't check hot plug 
and reset paths as these features are awkward with the HiFive Unmatched 
system involved.

 I have skimmed over the changes as committed to pci/enumeration and found 
nothing suspicious.  I have verified that the tree builds as at each of 
them with my configuration.

 As per my earlier remark:

> I think making a system halfway-fixed would make little sense, but with
> the actual fix actually made last as you suggested I think this can be
> split off, because it'll make no functional change by itself.

I am not perfectly happy with your rearrangement to fold the !PCI_QUIRKS 
stub into the change carrying the actual workaround and then have the 
reset path update with a follow-up change only, but I won't fight over it.  
It's only one tree revision that will be in this halfway-fixed state and 
I'll trust your judgement here.

 Let me know if anything pops up related to these changes anytime and I'll 
be happy to look into it.  The system involved is nearing two years since 
its deployment already, but hopefully it has many years to go yet and will 
continue being ready to verify things.  It's not that there's lots of real 
RISC-V hardware available, let alone with PCI/e connectivity.

 Thank you for staying with me and reviewing this patch series through all 
the iterations.

  Maciej

Re: [PATCH v9 00/14] pci: Work around ASMedia ASM2824 PCIe link training failures

[Bjorn Helgaas]

On Thu, Jun 15, 2023 at 01:41:10AM +0100, Maciej W. Rozycki wrote:
> On Wed, 14 Jun 2023, Bjorn Helgaas wrote:
> 
> > >  This is v9 of the change to work around a PCIe link training phenomenon 
> > > where a pair of devices both capable of operating at a link speed above 
> > > 2.5GT/s seems unable to negotiate the link speed and continues training 
> > > indefinitely with the Link Training bit switching on and off repeatedly 
> > > and the data link layer never reaching the active state.
> > > 
> > >  With several requests addressed and a few extra issues spotted this
> > > version has now grown to 14 patches.  It has been verified for device 
> > > enumeration with and without PCI_QUIRKS enabled, using the same piece of 
> > > RISC-V hardware as previously.  Hot plug or reset events have not been 
> > > verified, as this is difficult if at all feasible with hardware in 
> > > question.

> >  static int pci_dev_wait(struct pci_dev *dev, char *reset_type, int timeout)
> >  {
> > -   bool retrain = true;
> > int delay = 1;
> > +   bool retrain = false;
> > +   struct pci_dev *bridge;
> > +
> > +   if (pci_is_pcie(dev)) {
> > +   retrain = true;
> > +   bridge = pci_upstream_bridge(dev);
> > +   }
> 
>  If doing it this way, which I actually like, I think it would be a little 
> bit better performance- and style-wise if this was written as:
> 
>   if (pci_is_pcie(dev)) {
>   bridge = pci_upstream_bridge(dev);
>   retrain = !!bridge;
>   }
> 
> (or "retrain = bridge != NULL" if you prefer this style), and then we 
> don't have to repeatedly check two variables iff (pcie && !bridge) in the 
> loop below:

Done, thanks, I do like that better.  I did:

  bridge = pci_upstream_bridge(dev);
  if (bridge)
retrain = true;

because it seems like it flows more naturally when reading.

Bjorn

Re: [PATCH v9 00/14] pci: Work around ASMedia ASM2824 PCIe link training failures

[Maciej W. Rozycki]

On Wed, 14 Jun 2023, Bjorn Helgaas wrote:

> >  This is v9 of the change to work around a PCIe link training phenomenon 
> > where a pair of devices both capable of operating at a link speed above 
> > 2.5GT/s seems unable to negotiate the link speed and continues training 
> > indefinitely with the Link Training bit switching on and off repeatedly 
> > and the data link layer never reaching the active state.
> > 
> >  With several requests addressed and a few extra issues spotted this
> > version has now grown to 14 patches.  It has been verified for device 
> > enumeration with and without PCI_QUIRKS enabled, using the same piece of 
> > RISC-V hardware as previously.  Hot plug or reset events have not been 
> > verified, as this is difficult if at all feasible with hardware in 
> > question.
> > 
> >  Last iteration: 
> > ,
> >  
> > and my input to it:
> > .
> 
> Thanks, I applied these to pci/enumeration for v6.5.

 Great, thanks!

> I tweaked a few things, so double-check to be sure I didn't break
> something:
> 
>   - Moved dev->link_active_reporting init to set_pcie_port_type()
> because it does other PCIe-related stuff.
> 
>   - Reordered to keep all the link_active_reporting things together.
> 
>   - Reordered to clean up & factor pcie_retrain_link() before exposing
> it to the rest of the PCI core.
> 
>   - Moved pcie_retrain_link() a little earlier to keep it next to
> pcie_wait_for_link_status().
> 
>   - Squashed the stubs into the actual quirk so we don't have the
> intermediate state where we call the stubs but they never do
> anything (let me know if there's a reason we need your order).
> 
>   - Inline pcie_parent_link_retrain(), which seemed like it didn't add
> enough to be worthwhile.

 Ack, I'll double-check and report back.  A minor nit I've spotted below:

>  static int pci_dev_wait(struct pci_dev *dev, char *reset_type, int timeout)
>  {
> - bool retrain = true;
>   int delay = 1;
> + bool retrain = false;
> + struct pci_dev *bridge;
> +
> + if (pci_is_pcie(dev)) {
> + retrain = true;
> + bridge = pci_upstream_bridge(dev);
> + }

 If doing it this way, which I actually like, I think it would be a little 
bit better performance- and style-wise if this was written as:

if (pci_is_pcie(dev)) {
bridge = pci_upstream_bridge(dev);
retrain = !!bridge;
}

(or "retrain = bridge != NULL" if you prefer this style), and then we 
don't have to repeatedly check two variables iff (pcie && !bridge) in the 
loop below:

> @@ -1201,9 +1190,9 @@ static int pci_dev_wait(struct pci_dev *dev, char 
> *reset_type, int timeout)
>   }
>  
>   if (delay > PCI_RESET_WAIT) {
> - if (retrain) {
> + if (retrain && bridge) {

-- i.e. code can stay then as:

if (retrain) {

here.  I hope you find this observation rather obvious, so will you amend 
your tree, or shall I send an incremental update?

 Otherwise I don't find anything suspicious with the interdiff itself 
(thanks for posting it, that's really useful indeed!), but as I say I'll 
yet double-check how things look and work with your tree.  Hopefully 
tomorrow (Thu), as I have other stuff yet to complete tonight.

  Maciej

Re: [PATCH v9 00/14] pci: Work around ASMedia ASM2824 PCIe link training failures

[Bjorn Helgaas]

On Sun, Jun 11, 2023 at 06:19:08PM +0100, Maciej W. Rozycki wrote:
> Hi,
> 
>  This is v9 of the change to work around a PCIe link training phenomenon 
> where a pair of devices both capable of operating at a link speed above 
> 2.5GT/s seems unable to negotiate the link speed and continues training 
> indefinitely with the Link Training bit switching on and off repeatedly 
> and the data link layer never reaching the active state.
> 
>  With several requests addressed and a few extra issues spotted this
> version has now grown to 14 patches.  It has been verified for device 
> enumeration with and without PCI_QUIRKS enabled, using the same piece of 
> RISC-V hardware as previously.  Hot plug or reset events have not been 
> verified, as this is difficult if at all feasible with hardware in 
> question.
> 
>  Last iteration: 
> ,
>  
> and my input to it:
> .

Thanks, I applied these to pci/enumeration for v6.5.

I tweaked a few things, so double-check to be sure I didn't break
something:

  - Moved dev->link_active_reporting init to set_pcie_port_type()
because it does other PCIe-related stuff.

  - Reordered to keep all the link_active_reporting things together.

  - Reordered to clean up & factor pcie_retrain_link() before exposing
it to the rest of the PCI core.

  - Moved pcie_retrain_link() a little earlier to keep it next to
pcie_wait_for_link_status().

  - Squashed the stubs into the actual quirk so we don't have the
intermediate state where we call the stubs but they never do
anything (let me know if there's a reason we need your order).

  - Inline pcie_parent_link_retrain(), which seemed like it didn't add
enough to be worthwhile.

Interdiff below:

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 80694e2574b8..f11268924c8f 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1153,27 +1153,16 @@ void pci_resume_bus(struct pci_bus *bus)
pci_walk_bus(bus, pci_resume_one, NULL);
 }
 
-/**
- * pcie_parent_link_retrain - Check and retrain link we are downstream from
- * @dev: PCI device to handle.
- *
- * Return TRUE if the link was retrained, FALSE otherwise.
- */
-static bool pcie_parent_link_retrain(struct pci_dev *dev)
-{
-   struct pci_dev *bridge;
-
-   bridge = pci_upstream_bridge(dev);
-   if (bridge)
-   return pcie_failed_link_retrain(bridge);
-   else
-   return false;
-}
-
 static int pci_dev_wait(struct pci_dev *dev, char *reset_type, int timeout)
 {
-   bool retrain = true;
int delay = 1;
+   bool retrain = false;
+   struct pci_dev *bridge;
+
+   if (pci_is_pcie(dev)) {
+   retrain = true;
+   bridge = pci_upstream_bridge(dev);
+   }
 
/*
 * After reset, the device should not silently discard config
@@ -1201,9 +1190,9 @@ static int pci_dev_wait(struct pci_dev *dev, char 
*reset_type, int timeout)
}
 
if (delay > PCI_RESET_WAIT) {
-   if (retrain) {
+   if (retrain && bridge) {
retrain = false;
-   if (pcie_parent_link_retrain(dev)) {
+   if (pcie_failed_link_retrain(bridge)) {
delay = 1;
continue;
}
@@ -4914,6 +4903,38 @@ static bool pcie_wait_for_link_status(struct pci_dev 
*pdev,
return (lnksta & lnksta_mask) == lnksta_match;
 }
 
+/**
+ * pcie_retrain_link - Request a link retrain and wait for it to complete
+ * @pdev: Device whose link to retrain.
+ * @use_lt: Use the LT bit if TRUE, or the DLLLA bit if FALSE, for status.
+ *
+ * Retrain completion status is retrieved from the Link Status Register
+ * according to @use_lt.  It is not verified whether the use of the DLLLA
+ * bit is valid.
+ *
+ * Return TRUE if successful, or FALSE if training has not completed
+ * within PCIE_LINK_RETRAIN_TIMEOUT_MS milliseconds.
+ */
+bool pcie_retrain_link(struct pci_dev *pdev, bool use_lt)
+{
+   u16 lnkctl;
+
+   pcie_capability_read_word(pdev, PCI_EXP_LNKCTL, );
+   lnkctl |= PCI_EXP_LNKCTL_RL;
+   pcie_capability_write_word(pdev, PCI_EXP_LNKCTL, lnkctl);
+   if (pdev->clear_retrain_link) {
+   /*
+* Due to an erratum in some devices the Retrain Link bit
+* needs to be cleared again manually to allow the link
+* training to succeed.
+*/
+   lnkctl &= ~PCI_EXP_LNKCTL_RL;
+   pcie_capability_write_word(pdev, PCI_EXP_LNKCTL, lnkctl);
+   }
+
+   return pcie_wait_for_link_status(pdev, use_lt, !use_lt);
+}
+
 /**
  * pcie_wait_for_link_delay - Wait until link is active or 

[PATCH v9 00/14] pci: Work around ASMedia ASM2824 PCIe link training failures

[Maciej W. Rozycki]

Hi,

 This is v9 of the change to work around a PCIe link training phenomenon 
where a pair of devices both capable of operating at a link speed above 
2.5GT/s seems unable to negotiate the link speed and continues training 
indefinitely with the Link Training bit switching on and off repeatedly 
and the data link layer never reaching the active state.

 With several requests addressed and a few extra issues spotted this
version has now grown to 14 patches.  It has been verified for device 
enumeration with and without PCI_QUIRKS enabled, using the same piece of 
RISC-V hardware as previously.  Hot plug or reset events have not been 
verified, as this is difficult if at all feasible with hardware in 
question.

 Last iteration: 
,
 
and my input to it:
.

  Maciej