| Message ID | 20180521135003.32459-3-mr.nuke.me@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
On 5/21/2018 9:49 AM, Alexandru Gagniuc wrote: > +/* PCIe errors should not cause a panic. */ > +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; > + > + 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. > + */ > +static int ghes_severity(struct ghes *ghes) > +{ > + int worst_sev, sec_sev; > + struct acpi_hest_generic_data *gdata; > + const guid_t *section_type; > + const struct acpi_hest_generic_status *estatus = ghes->estatus; > + > + worst_sev = GHES_SEV_NO; > + apei_estatus_for_each_section(estatus, gdata) { > + section_type = (guid_t *)gdata->section_type; > + sec_sev = ghes_cper_severity(gdata->error_severity); > + > + if (guid_equal(section_type, &CPER_SEC_PCIE)) > + sec_sev = ghes_sec_pcie_severity(gdata); > + > + worst_sev = max(worst_sev, sec_sev); > + } > + > + return worst_sev; > +} > + > static void ghes_do_proc(struct ghes *ghes, > const struct acpi_hest_generic_status *estatus) > { > @@ -944,7 +986,7 @@ static int ghes_notify_nmi(unsigned int cmd, struct pt_regs *regs) > ret = NMI_HANDLED; > } > > - sev = ghes_cper_severity(ghes->estatus->error_severity); > + sev = ghes_severity(ghes); Hello Alex, There is a compile warning if CONFIG_HAVE_ACPI_APEI_NMI is not selected. CC drivers/acpi/apei/ghes.o drivers/acpi/apei/ghes.c:483:12: warning: ‘ghes_severity’ defined but not used [-Wunused-function] static int ghes_severity(struct ghes *ghes) ^~~~~~~~~~~~~ Thanks, Tyler
On 05/21/2018 09:27 AM, Tyler Baicar wrote: > On 5/21/2018 9:49 AM, Alexandru Gagniuc wrote: (snip) > Hello Alex, > > There is a compile warning if CONFIG_HAVE_ACPI_APEI_NMI is not selected. > > CC drivers/acpi/apei/ghes.o > drivers/acpi/apei/ghes.c:483:12: warning: ‘ghes_severity’ defined but > not used [-Wunused-function] > static int ghes_severity(struct ghes *ghes) > ^~~~~~~~~~~~~ Thanks for finding that. It's an easy fix. Staged for v7. > Thanks, > Tyler > -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, May 21, 2018 at 3:49 PM, Alexandru Gagniuc <mr.nuke.me@gmail.com> 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. > 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? 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? > Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com> > --- > drivers/acpi/apei/ghes.c | 48 +++++++++++++++++++++++++++++++++++++++++++++--- > 1 file changed, 45 insertions(+), 3 deletions(-) > > diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c > index 7c1a16b106ba..9baaab798020 100644 > --- a/drivers/acpi/apei/ghes.c > +++ b/drivers/acpi/apei/ghes.c > @@ -425,8 +425,7 @@ static void ghes_handle_memory_failure(struct acpi_hest_generic_data *gdata, int > * GHES_SEV_RECOVERABLE -> AER_NONFATAL > * GHES_SEV_RECOVERABLE && CPER_SEC_RESET -> AER_FATAL > * These both need to be reported and recovered from by the AER driver. > - * GHES_SEV_PANIC does not make it to this handling since the kernel must > - * panic. > + * GHES_SEV_PANIC -> AER_FATAL > */ > static void ghes_handle_aer(struct acpi_hest_generic_data *gdata) > { > @@ -459,6 +458,49 @@ static void ghes_handle_aer(struct acpi_hest_generic_data *gdata) > #endif > } > > +/* PCIe errors should not cause a panic. */ This comment is not sufficient and it should go inside of the function. > +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. > + > + 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. > + */ > +static int ghes_severity(struct ghes *ghes) > +{ > + int worst_sev, sec_sev; > + struct acpi_hest_generic_data *gdata; > + const guid_t *section_type; > + const struct acpi_hest_generic_status *estatus = ghes->estatus; > + > + worst_sev = GHES_SEV_NO; > + apei_estatus_for_each_section(estatus, gdata) { > + section_type = (guid_t *)gdata->section_type; > + sec_sev = ghes_cper_severity(gdata->error_severity); > + > + if (guid_equal(section_type, &CPER_SEC_PCIE)) > + sec_sev = ghes_sec_pcie_severity(gdata); > + > + worst_sev = max(worst_sev, sec_sev); > + } > + > + return worst_sev; > +} > + > static void ghes_do_proc(struct ghes *ghes, > const struct acpi_hest_generic_status *estatus) > { > @@ -944,7 +986,7 @@ static int ghes_notify_nmi(unsigned int cmd, struct pt_regs *regs) > ret = NMI_HANDLED; > } > > - sev = ghes_cper_severity(ghes->estatus->error_severity); > + sev = ghes_severity(ghes); > if (sev >= GHES_SEV_PANIC) { > oops_begin(); > ghes_print_queued_estatus(); > -- -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@gmail.com> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 5/22/2018 10:32 AM, Alex G. wrote: > 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. > You can get around that panic and still have the error handled as AER_FATAL in the current code. Your FW needs to mark the error as RECOVERABLE and then set the CPER_SEC_RESET flag. https://elixir.bootlin.com/linux/v4.17-rc6/source/drivers/acpi/apei/ghes.c#L450 Thanks, Tyler
On 05/22/2018 10:15 AM, Tyler Baicar wrote: > On 5/22/2018 10:32 AM, Alex G. wrote: >> 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. >> > You can get around that panic and still have the error handled as > AER_FATAL in > the current code. Your FW needs to mark the error as RECOVERABLE and then > set the CPER_SEC_RESET flag. Of course, that would be ideal. But experience shows that firmware doesn't do this. That's the whole point: firmware sends questionable data. Alex > https://elixir.bootlin.com/linux/v4.17-rc6/source/drivers/acpi/apei/ghes.c#L450 > > > Thanks, > Tyler > -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c index 7c1a16b106ba..9baaab798020 100644 --- a/drivers/acpi/apei/ghes.c +++ b/drivers/acpi/apei/ghes.c @@ -425,8 +425,7 @@ static void ghes_handle_memory_failure(struct acpi_hest_generic_data *gdata, int * GHES_SEV_RECOVERABLE -> AER_NONFATAL * GHES_SEV_RECOVERABLE && CPER_SEC_RESET -> AER_FATAL * These both need to be reported and recovered from by the AER driver. - * GHES_SEV_PANIC does not make it to this handling since the kernel must - * panic. + * GHES_SEV_PANIC -> AER_FATAL */ static void ghes_handle_aer(struct acpi_hest_generic_data *gdata) { @@ -459,6 +458,49 @@ static void ghes_handle_aer(struct acpi_hest_generic_data *gdata) #endif } +/* PCIe errors should not cause a panic. */ +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; + + 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. + */ +static int ghes_severity(struct ghes *ghes) +{ + int worst_sev, sec_sev; + struct acpi_hest_generic_data *gdata; + const guid_t *section_type; + const struct acpi_hest_generic_status *estatus = ghes->estatus; + + worst_sev = GHES_SEV_NO; + apei_estatus_for_each_section(estatus, gdata) { + section_type = (guid_t *)gdata->section_type; + sec_sev = ghes_cper_severity(gdata->error_severity); + + if (guid_equal(section_type, &CPER_SEC_PCIE)) + sec_sev = ghes_sec_pcie_severity(gdata); + + worst_sev = max(worst_sev, sec_sev); + } + + return worst_sev; +} + static void ghes_do_proc(struct ghes *ghes, const struct acpi_hest_generic_status *estatus) { @@ -944,7 +986,7 @@ static int ghes_notify_nmi(unsigned int cmd, struct pt_regs *regs) ret = NMI_HANDLED; } - sev = ghes_cper_severity(ghes->estatus->error_severity); + sev = ghes_severity(ghes); if (sev >= GHES_SEV_PANIC) { oops_begin(); ghes_print_queued_estatus();
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, 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). Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com> --- drivers/acpi/apei/ghes.c | 48 +++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 45 insertions(+), 3 deletions(-)