[RFC,v2,2/4] acpi: apei: Split GHES handlers outside of ghes_do_proc

[RFC,v2,2/4] acpi: apei: Split GHES handlers outside of ghes_do_proc

[Alexandru Gagniuc]

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(-)

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);
 

[RFC,v2,2/4] acpi: apei: Split GHES handlers outside of ghes_do_proc

[Borislav Petkov]

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.

[RFC,v2,2/4] acpi: apei: Split GHES handlers outside of ghes_do_proc

[Alexandru Gagniuc]

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-edac" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC,v2,2/4] acpi: apei: Split GHES handlers outside of ghes_do_proc

[Borislav Petkov]

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.

[RFC,v2,2/4] acpi: apei: Split GHES handlers outside of ghes_do_proc

[Alexandru Gagniuc]

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-edac" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC,v2,2/4] acpi: apei: Split GHES handlers outside of ghes_do_proc

[Borislav Petkov]

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?

[RFC,v2,2/4] acpi: apei: Split GHES handlers outside of ghes_do_proc

[Alexandru Gagniuc]

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-edac" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC,v2,2/4] acpi: apei: Split GHES handlers outside of ghes_do_proc

[Borislav Petkov]

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.