Re: [PATCH V6] PCI: rcar: Add L1 link state fix into data abort hook

["Pali Rohár" <pali@kernel.org>]

On Monday 19 July 2021 20:39:13 Marek Vasut wrote:
> On 7/19/21 7:23 PM, Pali Rohár wrote:
> 
> [...]
> 
> > > > > The R-Car PCIe controller is capable of handling L0s/L1 link states.
> > > > > While the controller can enter and exit L0s link state, and exit L1
> > > > > link state, without any additional action from the driver, to enter
> > > > > L1 link state, the driver must complete the link state transition by
> > > > > issuing additional commands to the controller.
> > > > > 
> > > > > The problem is, this transition is not atomic. The controller sets
> > > > > PMEL1RX bit in PMSR register upon reception of PM_ENTER_L1 DLLP from
> > > > > the PCIe card, but then the controller enters some sort of inbetween
> > > > > state. The driver must detect this condition and complete the link
> > > > > state transition, by setting L1IATN bit in PMCTLR and waiting for
> > > > > the link state transition to complete.
> > > > > 
> > > > > If a PCIe access happens inside this window, where the controller
> > > > > is between L0 and L1 link states, the access generates a fault and
> > > > > the ARM 'imprecise external abort' handler is invoked.
> > 
> > And if PCIe MMIO access does not happen, what fixes this issue?
> 
> Then you have no problem because you don't hit this fault.

When controller stucks in some "unknown" state you have a problem. And
it does not matter if you are doing MMIO or not. If controller is in
Recovery or Configuration state then endpoint card cannot send neither
interrupt nor memory read / write messages to system.

Driver for endpoint card does not have to do active polling to check if
something in endpoint card happened. It can just wait for interrupt and
then do some stuff (which is IIRC preferred design if events are not too
frequent). And in this case card is in dead state and you have this
problem, right?

> > In this
> > patch is implemented only arm32 external abort hook handler (which is
> > called only when PCIe MMIO access happens and aborts).
> 
> Yes, for the aarch64 rcar the same fix is implemented in atf (see below).
> 
> > > > > Just like other PCI controller drivers, here we hook the fault handler,
> > > > > perform the fixup to help the controller enter L1 link state, and then
> > > > > restart the instruction which triggered the fault. Since the controller
> > > > > is in L1 link state now, the link can exit from L1 link state to L0 and
> > > > > successfully complete the access.
> > 
> > Link cannot directly goes to L0 from L1. It first goes to Recovery state
> > and in this state card can "disconnect" or reset...
> > 
> > What would happen if PCIe MMIO access is issued when link is not in some
> > L* state? (This can be manually triggered by PCIe Hot Reset - toggling
> > Secondary Bus Reset bit in Bridge Control register on parent PCIe Bridge
> > device) Is R-Car working in this case and does not crash?
> 
> This seems to be exactly the situation the commit message describes -- the
> controller is stuck between L states and needs manual register write to
> proceed.

No, I asked what happen when is *not* in L state. Commit message does
not describe it.

So what happen if you try to do MMIO e.g. during Hot Reset state? (This
state can be easily "forced", so easy to test) Does it crash too (and
therefore needs some other "hack")? Or it is working fine without any
crash? Read operation in most cases returns all-ones and write just do
nothing (as write has no response).

> [...]
> 
> > > > To be clear, I'm not objecting to the patch.  It's a hardware problem
> > > > and we should work around it as best we can.
> > 
> > I'm not sure if current API of hook_fault_code or rather whole usage of
> > it is prepared to expand into more and more drivers. Last time I looked
> > at this arm32 part, it was possible to register only one callback from
> > driver. So extending usage of this hook API can result that two drivers
> > start fighting who register it earlier...
> 
> There doesn't seem to be much ongoing HW development on the arm32 r-car, so
> I don't expect this list of hooks to grow much on this platform.

For R-Car it then fine. This was just general comment as arm32 is still
actively developed platform.