RE: [PATCH v2] PCIe AER: report uncorrectable errors only to the functions that logged the errors

[Gabriele Paoloni]

Hi Bjorn

> -----Original Message-----
> From: Bjorn Helgaas [mailto:helgaas@xxxxxxxxxx]
> Sent: 02 September 2017 18:33
> To: Gabriele Paoloni
> Cc: Linuxarm; liudongdong (C); linux-pci@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v2] PCIe AER: report uncorrectable errors only to
> the functions that logged the errors
> 
> On Fri, Sep 01, 2017 at 11:39:35AM +0000, Gabriele Paoloni wrote:
> > Hi Bjorn
> >
> > Many thanks for looking at this
> >
> > > -----Original Message-----
> > > From: Bjorn Helgaas [mailto:helgaas@xxxxxxxxxx]
> > > Sent: 01 September 2017 05:43
> > > To: Gabriele Paoloni
> > > Cc: Linuxarm; liudongdong (C); linux-pci@xxxxxxxxxxxxxxx; linux-
> > > kernel@xxxxxxxxxxxxxxx
> > > Subject: Re: [PATCH v2] PCIe AER: report uncorrectable errors only
> to
> > > the functions that logged the errors
> > >
> > > On Thu, Aug 31, 2017 at 03:03:44PM -0500, Bjorn Helgaas wrote:
> > > > On Fri, Aug 18, 2017 at 12:02:21PM +0100, Gabriele Paoloni wrote:
> > > > > Currently if an uncorrectable error is reported by an EP the
> AER
> > > > > driver walks over all the devices connected to the upstream
> port
> > > > > bus and in turns call the report_error_detected() callback.
> > > > > If any of the devices connected to the bus does not implement
> > > > > dev->driver->err_handler->error_detected() do_recovery() will
> fail
> > > > > leaving all the bus hierarchy devices unrecovered.
> > > > >
> > > > > However for non fatal errors the PCIe link should not be
> considered
> > > > > compromised, therefore it makes sense to report the error only
> to
> > > > > all the functions that logged an error.
> > > >
> > > > Can you include a pointer to the relevant part of the spec here?
> >
> > Sure
> > According to section "6.2.2.2.2. Non-Fatal Errors"
> > << Non-fatal errors are uncorrectable errors which cause a particular
> > transaction to be unreliable but the Link is otherwise fully
> functional.
> > Isolating Non-fatal from Fatal errors provides Requester/Receiver
> logic
> > in a device or system management software the opportunity to recover
> > from the error without resetting the components on the Link and
> > disturbing other transactions in progress. Devices not associated
> with
> > the transaction in error are not impacted by the error.>>
> >
> > I will add it to the commit msg:
> >
> > >
> > > Also, I forgot to ask: can you outline the problem this fixes?  I'm
> > > curious about why this hasn't been an issue in the past.  My guess
> is
> > > there's something new about your configuration, and the config and
> the
> > > symptoms might help connect this fix to similar problems.
> >
> > I already replied about this in the previous patch...
> > << In Hi1620 we have some integrated controllers that appear as PCIe
> EPs
> > under the same bus. Some of these controllers (e.g. the SATA
> > controller) are missing the err_handler callbacks.
> >
> > If one device reports a non-fatal uncorrectable error with the
> current
> > AER core code the callbacks for all the devices under the same bus
> will
> > be called and, if any of the devices is missing the callback all the
> > devices in the subtree are left in error state without recovery...
> > This patch is needed to sort out a situation like this one.>>
> >
> > Should I add this to the commit msg?
> 
> Thanks for the reminder.  I thought I remembered some details but
> hadn't dug them out again.  Yes, I was hoping for something we could
> include in the commit message.  I'm still not sure what specifically
> is *new* about this situation.  Maybe the particular mix of functions
> in a multi-function device?  Maybe the fact that you're seeing more
> AER errors than before (or maybe just doing more testing?)

Hi Bjorn as I said here we have some integrated controllers that appear
as PCIe EPs under the same bus. Usually PCIe is p2p (no more that 1
device per bus), but in our SoC we have different devices under the same
bus (not MF devices):

RC---bus0---|-SAS ctrl
            |
            |-SATA ctrl
            |
            |-XGE ctrl

Since this is unusual this is maybe the reason why this has not been
seen yet elsewhere...

> 
> Since this is actually a bug fix, this might be a good opportunity to
> open a bugzilla for it.  Then we have a place to attach the complete
> "lspci -vv" output, dmesg, etc., that are of interest but too much for
> the commit message.

Sure we can do this and add the bugzilla link in the commit msg 

> 
> > > > > This patch implements this new behaviour for non fatal errors.
> > > > >
> > > > > Signed-off-by: Gabriele Paoloni <gabriele.paoloni@xxxxxxxxxx>
> > > > > Signed-off-by: Dongdong Liu <liudongdong3@xxxxxxxxxx>
> > > > > ---
> > > > > Changes from v1:
> > > > >    - now errors are reported only to the fucntions that logged
> the
> > > error
> > > > >      instead of all the functions in the same device.
> > > > >    - the patch subject has changed to match the new
> implementation
> > > > > ---
> > > > >  drivers/pci/pcie/aer/aerdrv_core.c | 9 ++++++++-
> > > > >  1 file changed, 8 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/pci/pcie/aer/aerdrv_core.c
> > > b/drivers/pci/pcie/aer/aerdrv_core.c
> > > > > index b1303b3..057465ad 100644
> > > > > --- a/drivers/pci/pcie/aer/aerdrv_core.c
> > > > > +++ b/drivers/pci/pcie/aer/aerdrv_core.c
> > > > > @@ -390,7 +390,14 @@ static pci_ers_result_t
> > > broadcast_error_message(struct pci_dev *dev,
> > > > >  		 * If the error is reported by an end point, we think
> this
> > > > >  		 * error is related to the upstream link of the end
> point.
> > > > >  		 */
> > > > > -		pci_walk_bus(dev->bus, cb, &result_data);
> > > > > +		if (state == pci_channel_io_normal)
> > > > > +			/*
> > > > > +			 * the error is non fatal so the bus is ok,
> just
> > > invoke
> > > > > +			 * the callback for the function that logged
> the
> > > error.
> > > > > +			 */
> > > > > +			cb(dev, &result_data);
> > > > > +		else
> > > > > +			pci_walk_bus(dev->bus, cb, &result_data);
> > > >
> > > > I think the concept of this change makes sense, but I don't like
> the
> > > > implicit connection of PCI_ERR_ROOT_UNCOR_RCV -> AER_NONFATAL ->
> > > > pci_channel_io_normal.  That makes it harder than it should be to
> > > read
> > > > the code.
> > > >
> > > > What would you think of changing the signature of do_recovery()
> and
> > > > broadcast_error_message() so they take the struct aer_err_info
> > > pointer
> > > > instead of just the severity and pci_channel_state?  Then we
> could
> > > > check directly for AER_NONFATAL here.
> >
> > I think the main issue of this approach is
> > aer_recover_work_func()->do_recovery()
> > If we modify the signature of do_recovery() we also need to modify
> > struct aer_recover_entry to embed aer_err_info sub struct (and anyway
> > so far there is no hook to aer_err_info in ghes.c...so it seems to
> > me like unfeasible...)
> >
> > I think we can either add a severity field to
> broadcast_error_message()
> > or move
> >
> > 	enum pci_channel_state state;
> >
> > 	if (severity == AER_FATAL)
> > 		state = pci_channel_io_frozen;
> > 	else
> > 		state = pci_channel_io_normal;
> >
> > inside broadcast_error_message()
> >
> > In both cases we make the code more readable but we add redundancy...
> > Thoughts?
> 
> Hmmm, it's not as simple to solve as I hoped.
> 
> The "native" Linux AER driver reads registers directly from the AER
> capability, produces logs, and does some recovery.
> 
> From 100 miles away, APEI is basically a way for the platform (i.e.,
> firmware or manageability software) to insert itself in the middle:
> *it* reads the AER registers, does whatever it wants to do (e.g.,
> OS-independent logging), and then passes a copy of the AER capability
> (and the PCIe capability for good measure) on to the OS via APEI.
> 
> The Linux APEI code then stuffs that AER info into the native Linux
> AER path where we do our own recovery.
> 
> So we basically have two entry points to the Linux AER code: (1) the
> get_device_error_info() path that reads the AER registers directly,
> and (2) the APEI path that gets the AER register info from the
> firmware.
> 
> In my mind, these paths ought to be far more unified than they are.
> The transition from APEI to the native Linux AER code throws away
> almost all the information we got from the platform:
> aer_recover_work_func() in the APEI path gets a copy of the AER
> capability, but all it passes to do_recovery() in the native AER code
> is "severity" -- a single int.
> 
> I think we should revamp the native AER code so it collects the AER
> register info a little higher up, maybe in aer_isr_one_error() (it's
> kind of stupid that aer_process_err_devices() currently reads the AER
> registers *twice*).
> 
> Then we could make the APEI code copy the information we care about
> from the CPER into a struct aer_err_info, then stuff it into
> aer_process_err_devices().  That would have the nice side-effect of
> using exactly the same logging code for both paths (currently the APEI
> path uses cper_print_aer(), while the native path uses
> aer_print_error()).
> 
> This is a lot of work.

I agree (on what to do and, unfortunately, on a lot of work :) ).
However given that this patch fixes a bug I would ask if we can first
accept this patch first and then proceed with the rework...what do
you think?

Thanks
Gab

> 
> Bjorn