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:helg...@kernel.org]
> Sent: 25 September 2017 19:34
> To: Gabriele Paoloni
> Cc: Linuxarm; liudongdong (C); linux-...@vger.kernel.org; linux-
> ker...@vger.kernel.org
> Subject: Re: [PATCH v2] PCIe AER: report uncorrectable errors only to
> the functions that logged the errors
> 
> On Wed, Sep 06, 2017 at 10:56:42AM +, Gabriele Paoloni wrote:
> > Hi Bjorn
> >
> > > -Original Message-
> > > From: Bjorn Helgaas [mailto:helg...@kernel.org]
> > > Sent: 02 September 2017 18:33
> > > To: Gabriele Paoloni
> > > Cc: Linuxarm; liudongdong (C); linux-...@vger.kernel.org; linux-
> > > ker...@vger.kernel.org
> > > 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 +, Gabriele Paoloni wrote:
> > > > Hi Bjorn
> > > >
> > > > Many thanks for looking at this
> > > >
> > > > > -Original Message-
> > > > > From: Bjorn Helgaas [mailto:helg...@kernel.org]
> > > > > Sent: 01 September 2017 05:43
> > > > > To: Gabriele Paoloni
> > > > > Cc: Linuxarm; liudongdong (C); linux-...@vger.kernel.org;
> linux-
> > > > > ker...@vger.kernel.org
> > > > > 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
> > > > 

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:helg...@kernel.org]
> Sent: 25 September 2017 19:34
> To: Gabriele Paoloni
> Cc: Linuxarm; liudongdong (C); linux-...@vger.kernel.org; linux-
> ker...@vger.kernel.org
> Subject: Re: [PATCH v2] PCIe AER: report uncorrectable errors only to
> the functions that logged the errors
> 
> On Wed, Sep 06, 2017 at 10:56:42AM +, Gabriele Paoloni wrote:
> > Hi Bjorn
> >
> > > -Original Message-
> > > From: Bjorn Helgaas [mailto:helg...@kernel.org]
> > > Sent: 02 September 2017 18:33
> > > To: Gabriele Paoloni
> > > Cc: Linuxarm; liudongdong (C); linux-...@vger.kernel.org; linux-
> > > ker...@vger.kernel.org
> > > 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 +, Gabriele Paoloni wrote:
> > > > Hi Bjorn
> > > >
> > > > Many thanks for looking at this
> > > >
> > > > > -Original Message-
> > > > > From: Bjorn Helgaas [mailto:helg...@kernel.org]
> > > > > Sent: 01 September 2017 05:43
> > > > > To: Gabriele Paoloni
> > > > > Cc: Linuxarm; liudongdong (C); linux-...@vger.kernel.org;
> linux-
> > > > > ker...@vger.kernel.org
> > > > > 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
> > > > 

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

[Bjorn Helgaas]

On Wed, Sep 06, 2017 at 10:56:42AM +, Gabriele Paoloni wrote:
> Hi Bjorn
> 
> > -Original Message-
> > From: Bjorn Helgaas [mailto:helg...@kernel.org]
> > Sent: 02 September 2017 18:33
> > To: Gabriele Paoloni
> > Cc: Linuxarm; liudongdong (C); linux-...@vger.kernel.org; linux-
> > ker...@vger.kernel.org
> > 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 +, Gabriele Paoloni wrote:
> > > Hi Bjorn
> > >
> > > Many thanks for looking at this
> > >
> > > > -Original Message-
> > > > From: Bjorn Helgaas [mailto:helg...@kernel.org]
> > > > Sent: 01 September 2017 05:43
> > > > To: Gabriele Paoloni
> > > > Cc: Linuxarm; liudongdong (C); linux-...@vger.kernel.org; linux-
> > > > ker...@vger.kernel.org
> > > > 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
> 

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

[Bjorn Helgaas]

On Wed, Sep 06, 2017 at 10:56:42AM +, Gabriele Paoloni wrote:
> Hi Bjorn
> 
> > -Original Message-
> > From: Bjorn Helgaas [mailto:helg...@kernel.org]
> > Sent: 02 September 2017 18:33
> > To: Gabriele Paoloni
> > Cc: Linuxarm; liudongdong (C); linux-...@vger.kernel.org; linux-
> > ker...@vger.kernel.org
> > 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 +, Gabriele Paoloni wrote:
> > > Hi Bjorn
> > >
> > > Many thanks for looking at this
> > >
> > > > -Original Message-
> > > > From: Bjorn Helgaas [mailto:helg...@kernel.org]
> > > > Sent: 01 September 2017 05:43
> > > > To: Gabriele Paoloni
> > > > Cc: Linuxarm; liudongdong (C); linux-...@vger.kernel.org; linux-
> > > > ker...@vger.kernel.org
> > > > 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 PC

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:helg...@kernel.org]
> Sent: 02 September 2017 18:33
> To: Gabriele Paoloni
> Cc: Linuxarm; liudongdong (C); linux-...@vger.kernel.org; linux-
> ker...@vger.kernel.org
> 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 +, Gabriele Paoloni wrote:
> > Hi Bjorn
> >
> > Many thanks for looking at this
> >
> > > -Original Message-
> > > From: Bjorn Helgaas [mailto:helg...@kernel.org]
> > > Sent: 01 September 2017 05:43
> > > To: Gabriele Paoloni
> > > Cc: Linuxarm; liudongdong (C); linux-...@vger.kernel.org; linux-
> > > ker...@vger.kernel.org
> > > 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
> "l

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:helg...@kernel.org]
> Sent: 02 September 2017 18:33
> To: Gabriele Paoloni
> Cc: Linuxarm; liudongdong (C); linux-...@vger.kernel.org; linux-
> ker...@vger.kernel.org
> 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 +, Gabriele Paoloni wrote:
> > Hi Bjorn
> >
> > Many thanks for looking at this
> >
> > > -Original Message-
> > > From: Bjorn Helgaas [mailto:helg...@kernel.org]
> > > Sent: 01 September 2017 05:43
> > > To: Gabriele Paoloni
> > > Cc: Linuxarm; liudongdong (C); linux-...@vger.kernel.org; linux-
> > > ker...@vger.kernel.org
> > > 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&q

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

[Bjorn Helgaas]

On Fri, Sep 01, 2017 at 11:39:35AM +, Gabriele Paoloni wrote:
> Hi Bjorn
> 
> Many thanks for looking at this
> 
> > -Original Message-
> > From: Bjorn Helgaas [mailto:helg...@kernel.org]
> > Sent: 01 September 2017 05:43
> > To: Gabriele Paoloni
> > Cc: Linuxarm; liudongdong (C); linux-...@vger.kernel.org; linux-
> > ker...@vger.kernel.org
> > 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?)

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.

> > > > This patch implements this new behaviour for non fatal errors.
> > > >
> > > > Signed-off-by: Gabriele Paoloni <gabriele.paol...@huawei.com>
> > > > Signed-off-by: Dongdong Liu <liudongdo...@huawei.com>
> > > > ---
> > > > 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 @@ s

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

[Bjorn Helgaas]

On Fri, Sep 01, 2017 at 11:39:35AM +, Gabriele Paoloni wrote:
> Hi Bjorn
> 
> Many thanks for looking at this
> 
> > -Original Message-
> > From: Bjorn Helgaas [mailto:helg...@kernel.org]
> > Sent: 01 September 2017 05:43
> > To: Gabriele Paoloni
> > Cc: Linuxarm; liudongdong (C); linux-...@vger.kernel.org; linux-
> > ker...@vger.kernel.org
> > 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?)

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.

> > > > This patch implements this new behaviour for non fatal errors.
> > > >
> > > > Signed-off-by: Gabriele Paoloni 
> > > > Signed-off-by: Dongdong Liu 
> > > > ---
> > > > 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,
> > > &g

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

[Gabriele Paoloni]

Hi Bjorn

Many thanks for looking at this

> -Original Message-
> From: Bjorn Helgaas [mailto:helg...@kernel.org]
> Sent: 01 September 2017 05:43
> To: Gabriele Paoloni
> Cc: Linuxarm; liudongdong (C); linux-...@vger.kernel.org; linux-
> ker...@vger.kernel.org
> 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?

> 
> > > This patch implements this new behaviour for non fatal errors.
> > >
> > > Signed-off-by: Gabriele Paoloni <gabriele.paol...@huawei.com>
> > > Signed-off-by: Dongdong Liu <liudongdo...@huawei.com>
> > > ---
> > > 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, _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, _data);
> > > + else
> > > + pci_walk_bus(dev->bus, cb, _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 thi

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

[Gabriele Paoloni]

Hi Bjorn

Many thanks for looking at this

> -Original Message-
> From: Bjorn Helgaas [mailto:helg...@kernel.org]
> Sent: 01 September 2017 05:43
> To: Gabriele Paoloni
> Cc: Linuxarm; liudongdong (C); linux-...@vger.kernel.org; linux-
> ker...@vger.kernel.org
> 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?

> 
> > > This patch implements this new behaviour for non fatal errors.
> > >
> > > Signed-off-by: Gabriele Paoloni 
> > > Signed-off-by: Dongdong Liu 
> > > ---
> > > 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, _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, _data);
> > > + else
> > > + pci_walk_bus(dev->bus, cb, _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
>

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

[Bjorn Helgaas]

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?

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.

> > This patch implements this new behaviour for non fatal errors.
> > 
> > Signed-off-by: Gabriele Paoloni 
> > Signed-off-by: Dongdong Liu 
> > ---
> > 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, _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, _data);
> > +   else
> > +   pci_walk_bus(dev->bus, cb, _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.
> 
> Bjorn

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

[Bjorn Helgaas]

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?

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.

> > This patch implements this new behaviour for non fatal errors.
> > 
> > Signed-off-by: Gabriele Paoloni 
> > Signed-off-by: Dongdong Liu 
> > ---
> > 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, _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, _data);
> > +   else
> > +   pci_walk_bus(dev->bus, cb, _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.
> 
> Bjorn

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

[Bjorn Helgaas]

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?

> This patch implements this new behaviour for non fatal errors.
> 
> Signed-off-by: Gabriele Paoloni 
> Signed-off-by: Dongdong Liu 
> ---
> 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, _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, _data);
> + else
> + pci_walk_bus(dev->bus, cb, _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.

Bjorn

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

[Bjorn Helgaas]

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?

> This patch implements this new behaviour for non fatal errors.
> 
> Signed-off-by: Gabriele Paoloni 
> Signed-off-by: Dongdong Liu 
> ---
> 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, _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, _data);
> + else
> + pci_walk_bus(dev->bus, cb, _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.

Bjorn

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

[Gabriele Paoloni]

ping...

> -Original Message-
> From: Gabriele Paoloni
> Sent: 18 August 2017 12:02
> To: helg...@kernel.org
> Cc: Gabriele Paoloni; Linuxarm; liudongdong (C); linux-
> p...@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: [PATCH v2] PCIe AER: report uncorrectable errors only to the
> functions that logged the errors
> 
> 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.
> This patch implements this new behaviour for non fatal errors.
> 
> Signed-off-by: Gabriele Paoloni <gabriele.paol...@huawei.com>
> Signed-off-by: Dongdong Liu <liudongdo...@huawei.com>
> ---
> 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, _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, _data);
> + else
> + pci_walk_bus(dev->bus, cb, _data);
>   }
> 
>   return result_data.result;
> --
> 2.7.4
> 

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

[Gabriele Paoloni]

ping...

> -Original Message-
> From: Gabriele Paoloni
> Sent: 18 August 2017 12:02
> To: helg...@kernel.org
> Cc: Gabriele Paoloni; Linuxarm; liudongdong (C); linux-
> p...@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: [PATCH v2] PCIe AER: report uncorrectable errors only to the
> functions that logged the errors
> 
> 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.
> This patch implements this new behaviour for non fatal errors.
> 
> Signed-off-by: Gabriele Paoloni 
> Signed-off-by: Dongdong Liu 
> ---
> 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, _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, _data);
> + else
> + pci_walk_bus(dev->bus, cb, _data);
>   }
> 
>   return result_data.result;
> --
> 2.7.4
> 

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

[Gabriele Paoloni]

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.
This patch implements this new behaviour for non fatal errors.

Signed-off-by: Gabriele Paoloni 
Signed-off-by: Dongdong Liu 
---
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, _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, _data);
+   else
+   pci_walk_bus(dev->bus, cb, _data);
}
 
return result_data.result;
-- 
2.7.4

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

[Gabriele Paoloni]

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.
This patch implements this new behaviour for non fatal errors.

Signed-off-by: Gabriele Paoloni 
Signed-off-by: Dongdong Liu 
---
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, _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, _data);
+   else
+   pci_walk_bus(dev->bus, cb, _data);
}
 
return result_data.result;
-- 
2.7.4