| Message ID | 20180416215903.7318-3-mr.nuke.me@gmail.com (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
On Mon, Apr 16, 2018 at 04:59:01PM -0500, Alexandru Gagniuc wrote: > static void ghes_do_proc(struct ghes *ghes, > const struct acpi_hest_generic_status *estatus) > { > int sev, sec_sev; > struct acpi_hest_generic_data *gdata; > + const struct ghes_handler *handler; > guid_t *sec_type; > guid_t *fru_id = &NULL_UUID_LE; > char *fru_text = ""; > @@ -478,21 +537,10 @@ static void ghes_do_proc(struct ghes *ghes, > if (gdata->validation_bits & CPER_SEC_VALID_FRU_TEXT) > fru_text = gdata->fru_text; > > - if (guid_equal(sec_type, &CPER_SEC_PLATFORM_MEM)) { > - struct cper_sec_mem_err *mem_err = acpi_hest_get_payload(gdata); > - > - ghes_edac_report_mem_error(sev, mem_err); > - > - arch_apei_report_mem_error(sev, mem_err); > - ghes_handle_memory_failure(gdata, sev); > - } > - else if (guid_equal(sec_type, &CPER_SEC_PCIE)) { > - ghes_handle_aer(gdata); > - } > - else if (guid_equal(sec_type, &CPER_SEC_PROC_ARM)) { > - struct cper_sec_proc_arm *err = acpi_hest_get_payload(gdata); > > - log_arm_hw_error(err); > + handler = get_handler(sec_type); I don't like this - it was better and more readable before because I can follow which handler gets called. This change makes is less readable.
On 04/18/2018 12:52 PM, Borislav Petkov wrote: > On Mon, Apr 16, 2018 at 04:59:01PM -0500, Alexandru Gagniuc wrote: >> static void ghes_do_proc(struct ghes *ghes, >> const struct acpi_hest_generic_status *estatus) >> { >> int sev, sec_sev; >> struct acpi_hest_generic_data *gdata; >> + const struct ghes_handler *handler; >> guid_t *sec_type; >> guid_t *fru_id = &NULL_UUID_LE; >> char *fru_text = ""; >> @@ -478,21 +537,10 @@ static void ghes_do_proc(struct ghes *ghes, >> if (gdata->validation_bits & CPER_SEC_VALID_FRU_TEXT) >> fru_text = gdata->fru_text; >> >> - if (guid_equal(sec_type, &CPER_SEC_PLATFORM_MEM)) { >> - struct cper_sec_mem_err *mem_err = acpi_hest_get_payload(gdata); >> - >> - ghes_edac_report_mem_error(sev, mem_err); >> - >> - arch_apei_report_mem_error(sev, mem_err); >> - ghes_handle_memory_failure(gdata, sev); >> - } >> - else if (guid_equal(sec_type, &CPER_SEC_PCIE)) { >> - ghes_handle_aer(gdata); >> - } >> - else if (guid_equal(sec_type, &CPER_SEC_PROC_ARM)) { >> - struct cper_sec_proc_arm *err = acpi_hest_get_payload(gdata); >> >> - log_arm_hw_error(err); >> + handler = get_handler(sec_type); > > I don't like this - it was better and more readable before because I can > follow which handler gets called. This change makes is less readable. I agree with the readability argument in the current situation of three handlers. I guess I was thinking ahead and generalizing for an arbitrary number of handlers. On the other side, you lose readability as soon as you get a few more handlers and the function becomes too long. And more importantly, you lose generality: it's not obvious that there's ghes_edac_report_mem_error() which too wide a context. 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 Thu, Apr 19, 2018 at 09:19:03AM -0500, Alex G. wrote: > On the other side, you lose readability as soon as you get a few more > handlers and the function becomes too long. No you don't - you split it properly. > And more importantly, you lose generality: it's not obvious that > there's ghes_edac_report_mem_error() which too wide a context. I don't understand what that means.
On 04/19/2018 09:30 AM, Borislav Petkov wrote: > On Thu, Apr 19, 2018 at 09:19:03AM -0500, Alex G. wrote: >> On the other side, you lose readability as soon as you get a few more >> handlers and the function becomes too long. > > No you don't - you split it properly. And that was the motivation behind my splitting it in this patch. >> And more importantly, you lose generality: it's not obvious that >> there's ghes_edac_report_mem_error() which too wide a context. > > I don't understand what that means. My apologies, sometimes my thought is too far ahead of my typing fingers. For the purpose of handling _one_ error, you need the CPER entry for that one error -- narrow context. You don't need the entire GHES structure -- wide context. Individual handlers should not be able to access the entire ghes. When the handlers are restricted to a common signature --which doesn't include ghes--, it's obvious when functions try to bite more than they can chew. 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 Thu, Apr 19, 2018 at 09:57:08AM -0500, Alex G. wrote: > And that was the motivation behind my splitting it in this patch. By "split" I don't mean add a function pointer which gets selected and then called - if the function becomes too long, you simply split the function body properly. > You don't need the entire GHES structure -- wide context. Individual > handlers should not be able to access the entire ghes. But you remove the @ghes argument in patch 1. So what are we even talking about?
On 04/19/2018 10:29 AM, Borislav Petkov wrote: > On Thu, Apr 19, 2018 at 09:57:08AM -0500, Alex G. wrote: >> And that was the motivation behind my splitting it in this patch. > > By "split" I don't mean add a function pointer which gets selected and > then called - if the function becomes too long, you simply split the > function body properly. The bulk of the function is the if/else mapping from UUID to error handler. I don't see how that can be easily split up, hence why I originally resorted to the mapping. As you said, we'll keep it simple at first. >> You don't need the entire GHES structure -- wide context. Individual >> handlers should not be able to access the entire ghes. > > But you remove the @ghes argument in patch 1. So what are we even > talking about? You could say, by convention, handlers shouldn't access ghes directly, but that is not obvious when @ghes is in scope. The reason I bring it up is that, if [1/4] ends up being unneeded, then I will drop it from the series. 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 Thu, Apr 19, 2018 at 10:46:15AM -0500, Alex G. wrote: > The bulk of the function is the if/else mapping from UUID to error > handler. I don't see how that can be easily split up, hence why I > originally resorted to the mapping. As you said, we'll keep it simple at > first. So that function is 43 lines now. Why are we even talking about this?! Just add your UUID check to the if-else statement and be done with it already. No handlers no nothing.
diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c index f9b53a6f55f3..2119c51b4a9e 100644 --- a/drivers/acpi/apei/ghes.c +++ b/drivers/acpi/apei/ghes.c @@ -414,6 +414,25 @@ static void ghes_handle_memory_failure(struct acpi_hest_generic_data *gdata, int #endif } +static int ghes_handle_arm(struct acpi_hest_generic_data *gdata, int sev) +{ + struct cper_sec_proc_arm *err = acpi_hest_get_payload(gdata); + + log_arm_hw_error(err); + return ghes_severity(gdata->error_severity); +} + +static int ghes_handle_mem(struct acpi_hest_generic_data *gdata, int sev) +{ + struct cper_sec_mem_err *mem_err = acpi_hest_get_payload(gdata); + + ghes_edac_report_mem_error(sev, mem_err); + arch_apei_report_mem_error(sev, mem_err); + ghes_handle_memory_failure(gdata, sev); + + return ghes_severity(gdata->error_severity); +} + /* * PCIe AER errors need to be sent to the AER driver for reporting and * recovery. The GHES severities map to the following AER severities and @@ -428,7 +447,7 @@ static void ghes_handle_memory_failure(struct acpi_hest_generic_data *gdata, int * GHES_SEV_PANIC does not make it to this handling since the kernel must * panic. */ -static void ghes_handle_aer(struct acpi_hest_generic_data *gdata) +static int ghes_handle_aer(struct acpi_hest_generic_data *gdata, int sev) { #ifdef CONFIG_ACPI_APEI_PCIEAER struct cper_sec_pcie *pcie_err = acpi_hest_get_payload(gdata); @@ -456,14 +475,54 @@ static void ghes_handle_aer(struct acpi_hest_generic_data *gdata) (struct aer_capability_regs *) pcie_err->aer_info); } + + return GHES_SEV_CORRECTED; #endif + return ghes_severity(gdata->error_severity); } +/** + * ghes_handler - handler for ACPI APEI errors + * @error_uuid: UUID describing the error entry (See ACPI/EFI CPER for details) + * @handle: Handler for the GHES entry of type 'error_uuid'. The handler + * returns the severity of the error after handling. A handler is allowed + * to demote errors to correctable or corrected, as appropriate. + */ +static const struct ghes_handler { + const guid_t *error_uuid; + int (*handle_irqsafe)(struct acpi_hest_generic_data *gdata, int sev); + int (*handle)(struct acpi_hest_generic_data *gdata, int sev); +} ghes_handlers[] = { + { + .error_uuid = &CPER_SEC_PLATFORM_MEM, + .handle = ghes_handle_mem, + }, { + .error_uuid = &CPER_SEC_PCIE, + .handle = ghes_handle_aer, + }, { + .error_uuid = &CPER_SEC_PROC_ARM, + .handle = ghes_handle_arm, + } +}; + +static const struct ghes_handler *get_handler(const guid_t *type) +{ + size_t i; + + for (i = 0; i < ARRAY_SIZE(ghes_handlers); i++) { + if (guid_equal(type, ghes_handlers[i].error_uuid)) + return &ghes_handlers[i]; + } + return NULL; +} + + static void ghes_do_proc(struct ghes *ghes, const struct acpi_hest_generic_status *estatus) { int sev, sec_sev; struct acpi_hest_generic_data *gdata; + const struct ghes_handler *handler; guid_t *sec_type; guid_t *fru_id = &NULL_UUID_LE; char *fru_text = ""; @@ -478,21 +537,10 @@ static void ghes_do_proc(struct ghes *ghes, if (gdata->validation_bits & CPER_SEC_VALID_FRU_TEXT) fru_text = gdata->fru_text; - if (guid_equal(sec_type, &CPER_SEC_PLATFORM_MEM)) { - struct cper_sec_mem_err *mem_err = acpi_hest_get_payload(gdata); - - ghes_edac_report_mem_error(sev, mem_err); - - arch_apei_report_mem_error(sev, mem_err); - ghes_handle_memory_failure(gdata, sev); - } - else if (guid_equal(sec_type, &CPER_SEC_PCIE)) { - ghes_handle_aer(gdata); - } - else if (guid_equal(sec_type, &CPER_SEC_PROC_ARM)) { - struct cper_sec_proc_arm *err = acpi_hest_get_payload(gdata); - log_arm_hw_error(err); + handler = get_handler(sec_type); + if (handler) { + sec_sev = handler->handle(gdata, sev); } else { void *err = acpi_hest_get_payload(gdata);
Use a mapping from CPER UUID to get the correct handler for a given GHES error. This is in preparation of splitting some handlers into irq safe and regular parts. Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com> --- drivers/acpi/apei/ghes.c | 78 ++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 63 insertions(+), 15 deletions(-)