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

[poza@codeaurora.org]

On 2018-08-16 03:26, Benjamin Herrenschmidt wrote:
> (resent using my proper email address)
> 
> On Tue, 2018-08-14 at 14:52 +0530, poza@codeaurora.org wrote:
>> > >       if (result == PCI_ERS_RESULT_RECOVERED) {
>> > >               if (pcie_wait_for_link(udev, true))
>> > >                       pci_rescan_bus(udev->bus);
>> > > -            pci_info(dev, "Device recovery from fatal error successful\n");
>> > > +            /* find the pci_dev after rescanning the bus */
>> > > +            dev = pci_get_domain_bus_and_slot(domain, bus, devfn);
>> >
>> > one of the motivations was to remove and re-enumerate rather then
>> > going thorugh driver's recovery sequence
>> > was; it might be possible that hotplug capable bridge, the device
>> > might have changed.
>> > hence this check will fail
> 
> Under what circumstances do you actually "unplug" the device ? We are
> trying to cleanup/fix some of the PowerPC EEH code which is in a way
> similar to AER, and we found that this unplug/replug, which we do if
> the driver doesn't have recovery callbacks only, is causing more
> problems than it solves.
> 
> We are moving toward instead unbinding the driver, resetting the
> device, then re-binding the driver instead of unplug/replug.
> 
> Also why would you ever bypass the driver callbacks if the driver has
> some ? The whole point is to keep the driver bound while resetting the
> device (provided it has the right callbacks) so we don't lose the
> linkage between stroage devices and mounted filesystems.
> 

there are two different paths we are taking, one is for ERR_FATAL and 
the other is for ERR_NONFATAL.
while AER and DPC converge into ERR_FATAL path; ERR_NONFATAL path is the 
one where AER and driver callbacks would come into picture.

since DPC does hw recovery of the link, and handles only ERR_FATAL, we 
do remove devices and re-enumerate them.
but if the error is no fatal, we will fall back to driver callbacks to 
initiate link error handling and recovery process.

under normal circumstances I do not see the some downstream devices 
would have been replaced in case of FATAL errors, but code might not 
want to assume that
same device is there, since we are removing downstream devices 
completely. so taking reference of old BDF and keeping it for later 
reference is not good idea.
although I think there are some parts of pcie_do_fatal_recovery need a 
fix, which Thomas is fixing is anyway.

so yes, I agree with you and we are talking about same thing e.g.
"
  We are moving toward instead unbinding the driver, resetting the
  device, then re-binding the driver instead of unplug/replug.
"

driver callbacks are very much there, please have a look at 
pcie_do_nonfatal_recovery().

refering Sepc: 6.2.2.2.1 Fatal Errors, where link is unreliable and it 
might need AER style reset of link or DPC style HW recovery
In both cases, the shutdown callbacks are expected to be called,
e.g.  some driver handle errors ERR_NONFATAL or FATAL in similar ways
e.g.
ioat_pcie_error_detected();  calls  ioat_shutdown(); in case of 
ERR_NONFATAL
otherwise ioat_shutdown() in case of ERR_FATAL.


> Cheers,
> Ben.