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

[poza@codeaurora.org]

On 2018-08-17 05:00, Benjamin Herrenschmidt wrote:
> 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.

The existing design is; framework dictating drivers what it should do 
rather than driver deciding.
let me explain.

aer_isr
     aer_isr_one_error
          get_device_error_info   >> this checks
               {
                 /* Link is still healthy for IO reads */    >> Bjorn 
this looks like a very strange thing to me, if link is not healthy we 
are not even going for pcie_do_fatal_recovery()
		pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_STATUS,
			&info->status);
		pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_MASK,
			&info->mask);
		if (!(info->status & ~info->mask))
			return 0;

               {
         handle_error_source
         pcie_do_nonfatal_recovery or pcie_do_fatal_recovery

now it does not seem to me that driver has some way of knowing if it has 
to return PCI_ERS_RESULT_CAN_RECOVER ? or PCI_ERS_RESULT_NEED_RESET?

let us take an example of storage driver here e.g. NVME
nvme_error_detected()  relies heavily on pci_channel_state_t which is 
passed by error framework  (err.c)

I understand your point of view and original intention of design that 
let driver dictate whether it wants slot_reset ?
but driver relies on framework to pass that information in order to take 
decision.

In case of pci_channel_io_frozen  (which is ERR_FATAL), most of the 
drivers demand link reset.

but in case of ERR_NONFATAL, the link is supposed to be functional and 
there is no need for link reset.
and exactly all the PCI based drivers returns 
PCI_ERS_RESULT_CAN_RECOVER, which is valid.


so I think to conclude, you are referring more of your views towards
pcie_do_fatal_recovery()


which does

> remove_devices from the downstream link
> reset_link()   (Secondary bus reset)
> re-enumerate.

and this is what DPC defined, and AER is just following that.

Regards,
Oza.

> 
> 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.