Re: [PATCH 1/2] PCI: Clear LBMS on resume to avoid Target Speed quirk

[Maciej W. Rozycki]

On Tue, 30 Jan 2024, Ilpo Järvinen wrote:

> First of all, this is not true for pcie_failed_link_retrain():
>  * Return TRUE if the link has been successfully retrained, otherwise FALSE.
> If LBMS is not set, the Target Speed quirk is not applied but the function 
> still returns true. I think that should be changed to early return false
> when no LBMS is present.

 I think there is a corner case here indeed.  If Link Active reporting is 
supported and neither DLLLA nor LBMS are set at entry, then the function 
indeed returns success even though the link is down and no attempt to 
retrain will have been made.  So this does indeed look a case for a return 
with the FALSE result.

 I think most easily this can be sorted by delegating the return result to 
a temporary variable, preset to FALSE and then only updated from results 
of the calls to `pcie_retrain_link'.  I can offer a patch as the author of 
the code and one who has means to verify it right away if that helped.

 Overall I guess it's all legacy of how this code evolved before it's been 
finally merged.

> > >   3. pci_bridge_wait_for_secondary_bus() then calls pci_dev_wait() which
> > >      cannot succeed (but waits ~1 minute, delaying the resume).
> > > 
> > > The Target Speed trick (in step 2) is only used if LBMS bit (PCIe r6.1
> > > sec 7.5.3.8) is set. For links that have been operational before
> > > suspend, it is well possible that LBMS has been set at the bridge and
> > > remains on. Thus, after resume, LBMS does not indicate the link needs
> > > the Target Speed quirk. Clear LBMS on resume for bridges to avoid the
> > > issue.

 This can be problematic AFAICT however.  While I am not able to verify 
suspend/resume operation with my devices, I expect the behaviour to be 
exactly the same after resume as after a bus reset: the link will fail to 
negotiate and the LBMS and DLLLA bits will be respectively set and clear.  
Consequently if you clear LBMS at resume, then the workaround won't 
trigger and the link will remain inoperational in its limbo state.

 What kind of scenario does the LBMS bit get set in that you have a 
trouble with?  You write that in your case the downstream device has been 
disconnected while the bug hierarchy was suspended and it is not present 
anymore at resume, is that correct?

 But in that case no link negotiation could have been possible and 
consequently the LBMS bit mustn't have been set by hardware (according to 
my understanding of PCIe), because (for compliant, non-broken devices 
anyway) it is only specified to be set for ports that can communicate with 
the other link end (the spec explicitly says there mustn't have been a 
transition through the DL_Down status for the port).

 Am I missing something?

  Maciej