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 Mon, 2018-08-20 at 18:56 +0530, poza@codeaurora.org wrote:
> Hi Ben,
> 
> I get the idea and get your points. and when I started implementation of 
> this, I did ask these questions to myself.
> but then we all fell aligned to what DPC was doing, because thats how 
> the driver was dealing with ERR_FATAL.
> 
> I can put together a patch adhering to the idea, and modify err.c.
> let me know if you are okay with that.
> I have both DPC and AER capable root port, so I can easily validate the 
> changes as we improve upon this.
> 
> besides We have to come to some common ground on policy.
> 1) if device driver dictates the reset we should agree to do SBR. 
> (irrespective of the type of error)

It's not just "the driver dictates the reset". It's a combination of
all agents. Either the driver or the framework, *both* can request a
reset. Also if you have multiple devices involved (for example multiple
functions), then any of the drivers can request the reset.

> 2) under what circumstances framework will impose the reset, even if 
> device driver did not !

Well ERR_FATAL typically would be one. Make it an argument to the
framework, it's possible that EEH might always do, I have to look into
it.

> probably changes might be simpler; although it will require to exercise 
> some tests cases for both DPC and AER.
> let me know if anything I missed here, but let me attempt to make patch 
> and we can go form there.
> 
> Let me know you opinion.

I agree. Hopefully Sam will have a chance to chime in (he's caught up
with something else at the moment) as well.

Cheers,
Ben.

> Regards,
> Oza.
> 
> > 
> > > Also I am very much interested in knowing original intention of DPC
> > > driver to unplug/plug devices,
> > > all I remember in some conversation was:
> > > hotplug capable bridge might have see devices changed, so it is safer 
> > > to
> > > remove/unplug the devices and during which .shutdown methods of driver
> > > is called, in case of ERR_FATAL.
> > 
> > The device being "swapped" during an error recovery operation is ...
> > unlikely. Do the bridges have a way to latch that the presence detect
> > changed ?
> > 
> > > although DPC is HW recovery while AER is sw recovery both should
> > > fundamentally act in the same way as far as device drivers callbacks 
> > > are
> > > concerned.
> > > (again I really dont know real motivation behind this)
> > 
> > The main problem with unplug/replug (as I mentioned earlier) is that it
> > just does NOT work for storage controllers (or similar type of
> > devices). The links between the storage controller and the mounted
> > filesystems is lost permanently, you'll most likely have to reboot the
> > machine.
> > 
> > With our current EEH implementation we can successfully recover from
> > fatal errors with the storage controller by resetting it.
> > 
> > Finally as for PERST vs. Hot Reset, this is an implementation detail.
> > Not all our platforms can control PERST on all devices either, we
> > generally prefer PERST as it provides a more reliable reset mechanism
> > in practice (talking from experience) but will fallback to hot reset.
> > 
> > Cheers,
> > Ben.
> > 
> > > 
> > > Regards,
> > > Oza.
> > > 
> > > >  - Figure out what hooks might be needed to be able to plumb EEH into
> > > > it, possibly removing a bunch of crap in arch/powerpc (yay !)
> > > > 
> > > > I don't think having a webex will be that practical with the timezones
> > > > involved. I'm trying to get approval to go to Plumbers in which case we
> > > > could setup a BOF but I have no guarantee at this point that I can make
> > > > it happen.
> > > > 
> > > > So let's try using email as much possible for now.
> > > > 
> > > > Cheers,
> > > > Ben.