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

[Ilpo Järvinen]

On Tue, 30 Jan 2024, Maciej W. Rozycki wrote:

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

I already wrote a patch which addresses this together with the caller 
site changes.

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

Indeed, it looks the step by step changed didn't yield good result here.

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

How do you get the LBMS set in the first place? Isn't that because the 
link tries to come up so shouldn't it reassert that bit again before the 
code ends up into the target speed quirk? That is, I assumed you actually 
wanted to detect LBMS getting set during pcie_wait_for_link_status() call 
preceeding pcie_failed_link_retrain() call?

There's always an option to store it into pci_dev for later use when the 
quirk is found to be working for the first time if other solutions don't 
work.

In any case and unrelated to this patch, the way this quirk monopolizes 
LBMS bit is going to have to be changed because it won't be reliable with 
the PCIe BW controller that sets up and irq for LBMS (and clears the bit).
In bwctrl v5 (yet to be posted) I'll add LBMS counter into bwctrl to allow 
this quirk to keep working (which will need to be confirmed).

>  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?

Yes, when resuming the device is already gone but the bridge still has 
LBMS set. My understanding is that it was set because it was there
from pre-suspend time but I've not really taken a deep look into it 
because the problem and fix seemed obvious.

I read that "without the Port transitioning through DL_Down status" 
differently than you, I only interpret that it relates to the two 
bullets following it. ...So if retrain bit is set, and link then goes 
down, the bullet no longer applies and LBMS should not be set because 
there was transition through DL_Down. But I could well be wrong...

-- 
 i.