Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

On Thu, 2018-08-16 at 13:26 +0530, poza@codeaurora.org wrote:
> > Please, at the very least revert the spec changes. They are utterly
> > wrong.
> > 
> > The driver MUST remain active during the recovery process *including*
> > fatal errors.
> > 
> > Only if the recovery fails and the driver gives us may you chose to
> > unplug the device (though there is little point).
> > 
> > What you have designed will work fine for network drivers but will not
> > work for storage.
> > 
> > Ben.
> 
> Hi Ben,
> 
> If you look at original DPC driver it was doing exactly the same as it 
> is doing now.
> so with or without new changes DPC driver had the same behavior from 
> beginning.

Yes and that behaviour is utterly wrong.

> commit 26e515713342b6f7c553aa3c66b21c6ab7cf82af

Yup, I noticed. We don't use DPC, all of that is subsumed into our EEH
infrastructure, so we didn't notice.

But now that the wrongness is spreading it's starting to show,
especially the changes to the spec.

We shouldn't be too focused on ERR_FATAL vs NON_FATAL. HW has a knack
for misreporting these things anyway.

All we really care about is whether the device was isolated and data
was lost or whether it's just an informational corrected error. The
latter should just be reported without impact. The former should lead
to a recovery procedure.

The design of the error callback was specifially so that you *could*
reset the link (or even PERST the whole device & retrain) *without*
having to unbind the driver or worse, soft-unplug the device.
Specifically beacause once you do the latter, you lose the links to the
mounted filesystems and those cannot be re-established.

So the spec needs to be reverted, DPC fixed and AER as well to do the
right thing here.

In fact it would be nice if we could make some of that logic common
with EEH. Sam (CC) has been cleaning up some of our EEH code which is,
to be honest, a bloody mess, so we might be able to comprehend and
refactor it more easily.

The basic idea is that regardless of the type of error, you first
notify the driver that an error occured. At that point, the io channel
might have been frozen already, so the driver can make no expectation
that the device is still reachable, which matches what DPC does I
believe.

The driver can ask for re-enabling the channel if it thinks it can
attempt a recovery but the core doesn't have to honor it and can chose
to just reset the device.

Similarly, in case of re-enabling, the driver can decide that this
didn't work out and request an escalation to a reset.

So typically, a non-fatal could go through the re-enable phase (in the
case where DPC didn't actually block the channel at all), while non-
fatal would always go to reset.

The only case where we would "unplug" on the linux side is when the
driver doesn't have error handling callbacks.

Cheers,
Ben.