Re: [PATCH] acpi: apei: call into AER handling regardless of severity

Re: [PATCH] acpi: apei: call into AER handling regardless of severity

[Sinan Kaya]

+linux-pci

On 8/29/2017 6:19 PM, Borislav Petkov wrote:
> if (entry.severity == AER_NONFATAL)
> 		do_recovery(pdev, entry.severity);

Providing input from PCI perspective only:

PCIe defines the the following error categories:

1. Correctable error
2. Uncorrectable non-fatal errors
3. Uncorrectable fatal errors

The do_recovery function needs to be called for both uncorrectable error
categories. (#2 and #3 above)

How these map to GHES error categories is out of know-how.

Below are references to some of the PCIe error docs:

https://www.kernel.org/doc/Documentation/PCI/pcieaer-howto.txt
https://www.kernel.org/doc/Documentation/PCI/pci-error-recovery.txt

-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH] acpi: apei: call into AER handling regardless of severity

[Borislav Petkov]

On Tue, Aug 29, 2017 at 06:34:49PM -0400, Sinan Kaya wrote:
> The do_recovery function needs to be called for both uncorrectable error
> categories. (#2 and #3 above)

Care to share why exactly that needs to happen?

Because I'm reading this in pcieaer-howto.txt:

"If an error message indicates a non-fatal error, performing link reset
at upstream is not required."

and

"If an error message indicates a fatal error, kernel will broadcast
error_detected(dev, pci_channel_io_frozen) to all drivers within a
hierarchy in question. Then, performing link reset at upstream is
necessary."

Now, pci-error-recovery.txt has link reset as step 3 so I'm assuming
recovery means link reset. And thus, non-fatal AER errors are not
required to do recovery but fatal are.

> How these map to GHES error categories is out of know-how.

        case CPER_SEV_INFORMATIONAL:
                return GHES_SEV_NO;
        case CPER_SEV_CORRECTED:
                return GHES_SEV_CORRECTED;
        case CPER_SEV_RECOVERABLE:
                return GHES_SEV_RECOVERABLE;
        case CPER_SEV_FATAL:
                return GHES_SEV_PANIC;

and

        case CPER_SEV_RECOVERABLE:
                return AER_NONFATAL;
        case CPER_SEV_FATAL:
                return AER_FATAL;
        default:
                return AER_CORRECTABLE;

So I see GHES_SEV_RECOVERABLE -> CPER_SEV_RECOVERABLE -> AER_NONFATAL.

Which means, we've never done error recovery for AER_FATAL errors. Which
we should've been doing in the first place! Unless...

... Error recovery for those fatal errors has been happening down the
other, PCI path:

aer_isr->aer_isr_one_error->...->do_recovery()

Which then makes me look at this contraption in the ghes code:

config ACPI_APEI_PCIEAER
        bool "APEI PCIe AER logging/recovering support"
        depends on ACPI_APEI && PCIEAER
        help
          PCIe AER errors may be reported via APEI firmware first mode.
          Turn on this option to enable the corresponding support.

So this says "may be" reported.

Now the question is, what kind of errors are being reported through here
and what exactly are we expected to do about them? Print them? Or do
more?

Hmmm.

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

Re: [PATCH] acpi: apei: call into AER handling regardless of severity

[Sinan Kaya]

On 8/30/2017 6:16 AM, Borislav Petkov wrote:
> On Tue, Aug 29, 2017 at 06:34:49PM -0400, Sinan Kaya wrote:
>> The do_recovery function needs to be called for both uncorrectable error
>> categories. (#2 and #3 above)
> 
> Care to share why exactly that needs to happen?

Let me try below:

> 
> Because I'm reading this in pcieaer-howto.txt:
> 
> "If an error message indicates a non-fatal error, performing link reset
> at upstream is not required."
> 

Link reset is not the only recovery mechanism. In the case
of nonfatal errors, it is assumed that the endpoint CSR is still reachable. 
Error is propagated the PCIe endpoint driver. Endpoint driver does a
re-initialization, we are back in business.

> and
> 
> "If an error message indicates a fatal error, kernel will broadcast
> error_detected(dev, pci_channel_io_frozen) to all drivers within a
> hierarchy in question. Then, performing link reset at upstream is
> necessary."
> 
> Now, pci-error-recovery.txt has link reset as step 3 so I'm assuming
> recovery means link reset. And thus, non-fatal AER errors are not
> required to do recovery but fatal are.

Nope, link reset is the second approach. Endpoint recovery won't happen
if the endpoint CSRs are unreachable. We have to recover the link by
doing a link reset in this case. 

Therefore, different behavior in do_recovery function based on the
severity of the error.

> 
>> How these map to GHES error categories is out of know-how.
> 
>         case CPER_SEV_INFORMATIONAL:
>                 return GHES_SEV_NO;
>         case CPER_SEV_CORRECTED:
>                 return GHES_SEV_CORRECTED;
>         case CPER_SEV_RECOVERABLE:
>                 return GHES_SEV_RECOVERABLE;
>         case CPER_SEV_FATAL:
>                 return GHES_SEV_PANIC;
> 
> and
> 
>         case CPER_SEV_RECOVERABLE:
>                 return AER_NONFATAL;
>         case CPER_SEV_FATAL:
>                 return AER_FATAL;
>         default:
>                 return AER_CORRECTABLE;
> 

Thanks for the mapping.

> So I see GHES_SEV_RECOVERABLE -> CPER_SEV_RECOVERABLE -> AER_NONFATAL.
> 
> Which means, we've never done error recovery for AER_FATAL errors. Which
> we should've been doing in the first place! Unless...
That's not true. The GHES code is changing the severity here before posting
to the AER driver in ghes_do_proc().

	if (gdata->flags & CPER_SEC_RESET)
		aer_severity = AER_FATAL;

A PCIe error could be AER_FATAL but it is recoverable. It does not need to
crash the system.

Here is another mapping below based on my understanding.

GHES_SEV_RECOVERABLE -> CPER_SEV_RECOVERABLE -> CPER_SEC_RESET-> AER_FATAL

> 
> ... Error recovery for those fatal errors has been happening down the
> other, PCI path:
> 
> aer_isr->aer_isr_one_error->...->do_recovery()

No, AER ISR is not set up if firmware first is enabled.

> 
> Which then makes me look at this contraption in the ghes code:
> 
> config ACPI_APEI_PCIEAER
>         bool "APEI PCIe AER logging/recovering support"
>         depends on ACPI_APEI && PCIEAER
>         help
>           PCIe AER errors may be reported via APEI firmware first mode.
>           Turn on this option to enable the corresponding support.
> 
> So this says "may be" reported.
> 
> Now the question is, what kind of errors are being reported through here
> and what exactly are we expected to do about them? Print them? Or do
> more?

The behavior should match non firmware-first case ideally.

1. Print all correctable errors.
2. Go to do_recovery for all uncorrectable errors including fatal and
non-fatal. 

This is also what AER driver does in the absence of firmware first via
handle_error_source().

> 
> Hmmm.
> 


-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH] acpi: apei: call into AER handling regardless of severity

[Borislav Petkov]

On Wed, Aug 30, 2017 at 10:05:44AM -0400, Sinan Kaya wrote:
> Link reset is not the only recovery mechanism. In the case of nonfatal
> errors, it is assumed that the endpoint CSR is still reachable.
> Error is propagated the PCIe endpoint driver. Endpoint driver does a
> re-initialization, we are back in business.

I'm assuming that's broadcast_error_message()'s job.

> That's not true. The GHES code is changing the severity here before posting
> to the AER driver in ghes_do_proc().
> 
> 	if (gdata->flags & CPER_SEC_RESET)
> 		aer_severity = AER_FATAL;

You're missing the point that we would walk into that if branch *only* for

                        if (sev == GHES_SEV_RECOVERABLE &&
                            sec_sev == GHES_SEV_RECOVERABLE

severities. So if you have an AER_FATAL error but ghes severities are
not GHES_SEV_RECOVERABLE, nothing happens.

> No, AER ISR is not set up if firmware first is enabled.

So then this is a major suckage. We do AER recovery on FF systems only
for GHES_SEV_RECOVERABLE severity.

> The behavior should match non firmware-first case ideally.
> 
> 1. Print all correctable errors.
> 2. Go to do_recovery for all uncorrectable errors including fatal and
> non-fatal. 
> 
> This is also what AER driver does in the absence of firmware first via
> handle_error_source().

Yes, that makes sense.

Which would mean that we'd call aer_recover_queue() regardless of GHES
severity but we'd do recovery only if GHES_SEV_RECOVERABLE is set
or CPER_SEC_RESET. I.e., we can communicate all that by setting the
correct AER severity before calling aer_recover_queue(). And then call
do_recovery() based on AER severity.

Hmmm?

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

Re: [PATCH] acpi: apei: call into AER handling regardless of severity

[Sinan Kaya]

On 8/30/2017 11:16 AM, Borislav Petkov wrote:
> On Wed, Aug 30, 2017 at 10:05:44AM -0400, Sinan Kaya wrote:
>> Link reset is not the only recovery mechanism. In the case of nonfatal
>> errors, it is assumed that the endpoint CSR is still reachable.
>> Error is propagated the PCIe endpoint driver. Endpoint driver does a
>> re-initialization, we are back in business.
> 
> I'm assuming that's broadcast_error_message()'s job.
> 

That's right. Each driver provides an err_handler hook. broadcast function
calls these. 

static struct pci_driver e1000_driver = {
	..
	.err_handler = &e1000_err_handler
};

struct pci_error_handlers {
	...
	pci_ers_result_t (*error_detected)(struct pci_dev *dev,
					   enum pci_channel_state error);
}


>> That's not true. The GHES code is changing the severity here before posting
>> to the AER driver in ghes_do_proc().
>>
>> 	if (gdata->flags & CPER_SEC_RESET)
>> 		aer_severity = AER_FATAL;
> 
> You're missing the point that we would walk into that if branch *only* for
> 
>                         if (sev == GHES_SEV_RECOVERABLE &&
>                             sec_sev == GHES_SEV_RECOVERABLE
> 
> severities. So if you have an AER_FATAL error but ghes severities are
> not GHES_SEV_RECOVERABLE, nothing happens.

I see. We should probably try to do something only if GHES_SEV_CORRECTED or
GHES_SEV_RECOVERABLE.

If somebody wants to crash the system with GHES_SEV_PANIC, there is no point
in doing additional work.

> 
>> No, AER ISR is not set up if firmware first is enabled.
> 
> So then this is a major suckage. We do AER recovery on FF systems only
> for GHES_SEV_RECOVERABLE severity.
> 
>> The behavior should match non firmware-first case ideally.
>>
>> 1. Print all correctable errors.
>> 2. Go to do_recovery for all uncorrectable errors including fatal and
>> non-fatal. 
>>
>> This is also what AER driver does in the absence of firmware first via
>> handle_error_source().
> 
> Yes, that makes sense.
> 
> Which would mean that we'd call aer_recover_queue() regardless of GHES
> severity but we'd do recovery only if GHES_SEV_RECOVERABLE is set
> or CPER_SEC_RESET. I.e., we can communicate all that by setting the
> correct AER severity before calling aer_recover_queue(). And then call
> do_recovery() based on AER severity.
> 
> Hmmm?
> 

Sounds good. Do you still want to do PCIe recovery in the case of
GHES_SEV_PANIC or if some FW returns GHES_SEV_NO?

-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH] acpi: apei: call into AER handling regardless of severity

[Baicar, Tyler]

On 8/30/2017 9:31 AM, Sinan Kaya wrote:
> On 8/30/2017 11:16 AM, Borislav Petkov wrote:
>> On Wed, Aug 30, 2017 at 10:05:44AM -0400, Sinan Kaya wrote:
>>> Link reset is not the only recovery mechanism. In the case of nonfatal
>>> errors, it is assumed that the endpoint CSR is still reachable.
>>> Error is propagated the PCIe endpoint driver. Endpoint driver does a
>>> re-initialization, we are back in business.
>> I'm assuming that's broadcast_error_message()'s job.
>>
> That's right. Each driver provides an err_handler hook. broadcast function
> calls these.
>
> static struct pci_driver e1000_driver = {
> 	..
> 	.err_handler = &e1000_err_handler
> };
>
> struct pci_error_handlers {
> 	...
> 	pci_ers_result_t (*error_detected)(struct pci_dev *dev,
> 					   enum pci_channel_state error);
> }
>
>
>>> That's not true. The GHES code is changing the severity here before posting
>>> to the AER driver in ghes_do_proc().
>>>
>>> 	if (gdata->flags & CPER_SEC_RESET)
>>> 		aer_severity = AER_FATAL;
>> You're missing the point that we would walk into that if branch *only* for
>>
>>                          if (sev == GHES_SEV_RECOVERABLE &&
>>                              sec_sev == GHES_SEV_RECOVERABLE
>>
>> severities. So if you have an AER_FATAL error but ghes severities are
>> not GHES_SEV_RECOVERABLE, nothing happens.
> I see. We should probably try to do something only if GHES_SEV_CORRECTED or
> GHES_SEV_RECOVERABLE.
>
> If somebody wants to crash the system with GHES_SEV_PANIC, there is no point
> in doing additional work.
See below.
>>> No, AER ISR is not set up if firmware first is enabled.
>> So then this is a major suckage. We do AER recovery on FF systems only
>> for GHES_SEV_RECOVERABLE severity.
>>
>>> The behavior should match non firmware-first case ideally.
>>>
>>> 1. Print all correctable errors.
>>> 2. Go to do_recovery for all uncorrectable errors including fatal and
>>> non-fatal.
>>>
>>> This is also what AER driver does in the absence of firmware first via
>>> handle_error_source().
>> Yes, that makes sense.
>>
>> Which would mean that we'd call aer_recover_queue() regardless of GHES
>> severity but we'd do recovery only if GHES_SEV_RECOVERABLE is set
>> or CPER_SEC_RESET. I.e., we can communicate all that by setting the
>> correct AER severity before calling aer_recover_queue(). And then call
>> do_recovery() based on AER severity.
>>
>> Hmmm?
>>
> Sounds good. Do you still want to do PCIe recovery in the case of
> GHES_SEV_PANIC or if some FW returns GHES_SEV_NO?
>
We do not need to worry about the GHES_SEV_PANIC case. Those get sent to 
__ghes_panic() in ghes_proc() without even making it to ghes_do_proc(). 
Those errors are just printed and then the kernel panics.

I think with my two patches we will have the desired functionality:

GHES_SEV_CORRECTABLE -> AER_CORRECTABLE -> Print AER info, but do not 
call do_recovery

GHES_SEV_RECOVERABLE -> AER_NONFATAL -> Print AER info and do_recovery

GHES_RECOVERABLE and CPER_SEC_RESET -> AER_FATAL -> Print AER info and 
do_recover

Thanks,
Tyler

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

Re: [PATCH] acpi: apei: call into AER handling regardless of severity

[Borislav Petkov]

On Wed, Aug 30, 2017 at 09:42:08AM -0600, Baicar, Tyler wrote:
> I think with my two patches we will have the desired functionality:
> 
> GHES_SEV_CORRECTABLE -> AER_CORRECTABLE -> Print AER info, but do not call
> do_recovery
> 
> GHES_SEV_RECOVERABLE -> AER_NONFATAL -> Print AER info and do_recovery
> 
> GHES_RECOVERABLE and CPER_SEC_RESET -> AER_FATAL -> Print AER info and
> do_recover

Right, so I'd like to you create a separate function ghes_do_proc_aer()
or ghes_handle_aer() or so and carve out all the code inside #ifdef
CONFIG_ACPI_APEI_PCIEAER into it, add your two changes to the patch and
slap a big fat comment above the new function explaining *why* we're
doing what we're doing and how we're mapping all the severities to AER
severity in order to do recovery and/or only to print the error.

So that it is known and people can see in the future why we're doing
this and what the logic has been and what kind of policy we're chasing
and so on and so on...

Ok?

Thanks.

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

Re: [PATCH] acpi: apei: call into AER handling regardless of severity

[Baicar, Tyler]

On 8/30/2017 11:14 AM, Borislav Petkov wrote:
> On Wed, Aug 30, 2017 at 09:42:08AM -0600, Baicar, Tyler wrote:
>> I think with my two patches we will have the desired functionality:
>>
>> GHES_SEV_CORRECTABLE -> AER_CORRECTABLE -> Print AER info, but do not call
>> do_recovery
>>
>> GHES_SEV_RECOVERABLE -> AER_NONFATAL -> Print AER info and do_recovery
>>
>> GHES_RECOVERABLE and CPER_SEC_RESET -> AER_FATAL -> Print AER info and
>> do_recover
> Right, so I'd like to you create a separate function ghes_do_proc_aer()
> or ghes_handle_aer() or so and carve out all the code inside #ifdef
> CONFIG_ACPI_APEI_PCIEAER into it, add your two changes to the patch and
> slap a big fat comment above the new function explaining *why* we're
> doing what we're doing and how we're mapping all the severities to AER
> severity in order to do recovery and/or only to print the error.
>
> So that it is known and people can see in the future why we're doing
> this and what the logic has been and what kind of policy we're chasing
> and so on and so on...
>
> Ok?
>
> Thanks.
Yes, I can do that.

Thanks,
Tyler

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

Re: [PATCH] acpi: apei: call into AER handling regardless of severity

[Borislav Petkov]

On Wed, Aug 30, 2017 at 11:31:06AM -0400, Sinan Kaya wrote:
> I see. We should probably try to do something only if GHES_SEV_CORRECTED or
> GHES_SEV_RECOVERABLE.
> 
> If somebody wants to crash the system with GHES_SEV_PANIC, there is no point
> in doing additional work.

Makes sense.

Whatever we do, I'd like to have this all nicely documented *why* we're
doing the recovery policy we're doing.

> Sounds good. Do you still want to do PCIe recovery in the case of
> GHES_SEV_PANIC or if some FW returns GHES_SEV_NO?

So I read GHES_SEV_PANIC as: we should panic and stop any processing
whatsoever ASAP in order to avoid further error propagation. So doing
recovery there might *actually* be a bad idea.

GHES_SEV_NO would map to AER_CORRECTABLE and I think that would mean,
print the error to let the user know but no need to recover because no
harm was done.

I *think*.

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--