Re: [PATCH v6 2/2] acpi: apei: Do not panic() on PCIe errors reported through GHES

[Alex G.]

On 05/22/2018 04:02 AM, Rafael J. Wysocki wrote:
> On Mon, May 21, 2018 at 3:49 PM, Alexandru Gagniuc <mr.nuke.me@xxxxxxxxx> wrote:
>> The policy was to panic() when GHES said that an error is "Fatal".
>> This logic is wrong for several reasons, as it doesn't account for the
>> cause of the error.
>>
>> PCIe fatal errors indicate that the link to a device is either
>> unstable or unusable. They don't indicate that the machine is on fire,
> 
> But they very well may indicate just that AFAICS.

I guess it's possible to set a machine on fire, and get a PCIe error as
one of the links melts. Although in that case, I doubt how we try to
handle the error makes a difference. Sarcasm aside, my point is that it
makes little sense to crash a machine when we lose a PCIe link.


>> and they are not severe enough to justify a panic(). Do not blindly
>> rely on firmware to evaluate the severity for us. Instead, look at
>> the error severity based on what caused the error (GHES subsections).
> 
> Which bit also comes from the firmware, right?  So why is it regarded
> as a better source of information?

It's less bad (not using 'better') because it relates more closely to
the error than the specific mechanism through which it is reported. The
header severity is an artificial concept that firmware has to make up,
whereas the subsection severity usually comes directly from hardware.

> Or are you trying to say that both of the pieces of information in
> question should be consistent with each other?  But if they aren't,
> which one should we trust more and why?

The header severity is letting someone else make the decisions for you.

(snip)
>> +/* PCIe errors should not cause a panic. */
> 
> This comment is not sufficient and it should go inside of the function.

What would make a "sufficient" comment?

>> +static int ghes_sec_pcie_severity(struct acpi_hest_generic_data *gdata)
>> +{
>> +       struct cper_sec_pcie *pcie_err = acpi_hest_get_payload(gdata);
>> +
>> +       if (pcie_err->validation_bits & CPER_PCIE_VALID_DEVICE_ID &&
>> +           pcie_err->validation_bits & CPER_PCIE_VALID_AER_INFO &&
>> +           IS_ENABLED(CONFIG_ACPI_APEI_PCIEAER))
>> +               return GHES_SEV_RECOVERABLE;
> 
> You have not explained convincingly enough why the above condition
> makes sense at all.

Is this a test? I think it's self-explanatory:
How can you invoke a handler when you don't have a source for the error?
Or how can you invoke a handler when you don't have that handler?

>> +
>> +       return ghes_cper_severity(gdata->error_severity);
>> +}
>> +
>> +/*
>> + * The severity field in the status block is an unreliable metric for the
>> + * severity. A more reliable way is to look at each subsection and see how safe
>> + * it is to call the approproate error handler.
>> + * We're not conerned with handling the error. We're concerned with being able
>> + * to notify an error handler by crossing the NMI/IRQ boundary, being able to
>> + * schedule_work, and so forth.
>> + *   - SEC_PCIE: All PCIe errors can be handled by AER.
> 
> Make this comment a proper kerneldoc or move it inside of the function.

I don't like moving long comments inside a function, as it breaks code
flow. Above-function explanation is also consistent with how
ghes_handle_aer() is documented.

Rafael, thank you very much for taking the time to review these patches.
Although, after reading through your emails, I'm at a loss on how you
want to to solve the problem. It appears everyone has a very strong and
different opinion how to proceed.

I think the biggest problem is having a policy to panic on "fatal"
errors, instead of letting the error handler make that decision. I'd
much rather kill that stupid policy, but people seem to like it for some
reason.

Alex