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 19:41 +0530, poza@codeaurora.org wrote:
> 
> > See above.
> > > 
> > > okay, so here is what current pcie_do_nonfatal_recovery() doe.
> > > 
> > > pcie_do_nonfatal_recovery
> > >      report_error_detected()    >> calls driver callbacks
> > >      report_mmio_enabled()
> > >      report_slot_reset()  >> if PCI_ERS_RESULT_NEED_RESET
> > 
> > Above if the driver returned "NEED RESET" we should not just "report" a
> > slot reset, we should *perform* one :-) Unless the AER code does it in
> > a place I missed...
> 
> I am willing work on this if Bjorn agrees.
> but I am still trying to figure out missing piece.
> 
> so Ben,
> you are suggesting
> 
> ERR_NONFATAL handling
> pcie_do_nonfatal_recovery
>       report_error_detected()    >> calls driver callbacks
>        report_mmio_enabled()
>        report_slot_reset()  >> if PCI_ERS_RESULT_NEED_RESET
> Here along with calling slot_reset, you are suggesting to do Secondary 
> Bus Reset ?
> 
> but this is ERR_NONFATAL and according to definition the link is still 
> good, so that the transcriptions on PCIe link can happen.
> so my question is why do we want to reset the link ?

The driver responded to you from error_detected() or mmio_enabled()
that it wants the slot to be reset. So you should do so. This comes
from the driver deciding that whatever happens, it doesn't trust the
device state anymore.

report_slot_reset() iirc is about telling the driver that the reset has
been performed and it can reconfigure the device.

To be frank I haven't looked at this in a while, so we should refer to
the document before ou patched it :-) But the basic design we created
back then was precisely that the driver would have the ultimate say in
the matter.

Also because multiple devices, at least on power, can share a domain, 
we can get into a situation where one device requires a reset, so all
will get reset, and their respective drivers need to be notified.

Note: it's not so much about resetting the *link* than about resetting
the *device*.

> although
> I see following note in the code as well.
>                  /*
> 		 * TODO: Should call platform-specific
> 		 * functions to reset slot before calling
> 		 * drivers' slot_reset callbacks?
> 		 */

Yup :-)

Cheers,
Ben.

> Regards,
> Oza.
> 
> > 
> > Also we should do a hot reset at least, not just a link reset.
> > 
> > >      report_resume()
> > > 
> > > If you suggest how it is broken, it will help me to understand.
> > > probably you might want to point out what are the calls need to be 
> > > added
> > > or removed or differently handled, specially storage point of view.
> > 
> > 
> > > Regards,
> > > Oza.
> > > 
> > > > 
> > > > Keep in mind that those callbacks were designed originally for EEH
> > > > (which predates AER), and so was the spec written.
> > > > 
> > > > We don't actually use the AER code on POWER today, so we didn't notice
> > > > how broken the implementation was :-)
> > > > 
> > > > We should fix that.
> > > > 
> > > > Either we can sort all that out by email, or we should plan some kind
> > > > of catch-up, either at Plumbers (provided I can go there) or maybe a
> > > > webex call.
> > > > 
> > > > Cheers,
> > > > Ben.