Re: [PATCH v6 07/10] mhi: pci_generic: Add PCI error handlers

[Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>]

On Thu, Dec 31, 2020 at 10:27:01AM +0100, Loic Poulain wrote:
> Hi Mani,
> 
> On Thu, 31 Dec 2020 at 08:18, Manivannan Sadhasivam
> <manivannan.sadhasivam@linaro.org> wrote:
> >
> > On Tue, Dec 29, 2020 at 09:43:48AM +0100, Loic Poulain wrote:
> > > In AER capable root complex, errors are reported to the host which
> > > can then act accordingly and perform PCI recovering procedure.
> > >
> > > This patch enables error reporting and implements error_detected,
> > > slot_reset and resume callbacks.
> > >
> > > Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
> > > Reviewed-by Hemant Kumar <hemantk@codeaurora.org>
> > > ---
> > >  drivers/bus/mhi/pci_generic.c | 50 +++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 50 insertions(+)
> 
> > > +static pci_ers_result_t mhi_pci_slot_reset(struct pci_dev *pdev)
> > > +{
> > > +     if (pci_enable_device(pdev)) {
> > > +             dev_err(&pdev->dev, "Cannot re-enable PCI device after reset.\n");
> > > +             return PCI_ERS_RESULT_DISCONNECT;
> > > +     }
> > > +
> >
> > This callback will be called after PCI slot reset, so we should also be resetting
> > the device after enabling it.
> 
> Yes, but that is done in mhi_pci_io_resume.
> 
> From the PCI error recovery documentation, "drivers should not restart
> normal I/O processing operations at this point (in slot_reset) If all
> device drivers report success on this callback, the platform will call
> resume() to complete the sequence, and let the driver restart normal
> I/O processing."
> The actual MHI PCI/recovery is then done in resume (mhi_pci_io_resume).
> 

If you read one paragraph above,

"This call gives drivers the chance to re-initialize the hardware
(re-download firmware, etc.).  At this point, the driver may assume
that the card is in a fresh state and is fully functional. The slot
is unfrozen and the driver has full access to PCI config space,
memory mapped I/O space and DMA."

So at the end of this call, the device is assumed to be functional (then only
PCI_ERS_RESULT_RECOVERED makes sense).

IMO, you should call mhi_pci_reset_prepare() in this callback and
mhi_pci_reset_done() in resume(). No need to schedule the recovery work.

Thanks,
Mani

> Regards,
> Loic