diff mbox series

[1/6] RAS: ACPI: APEI: add conditional compilation to ARM error report functions

Message ID f520f2529bb27d452a2dee762b6968939df42f45.1720436039.git.mchehab+huawei@kernel.org
State New
Headers show
Series Fix issues with ARM Processor CPER records | expand

Commit Message

Mauro Carvalho Chehab July 8, 2024, 11:18 a.m. UTC 
From: Daniel Ferguson <danielf@os.amperecomputing.com>

This prevents the unnecessary inclusion of ARM specific RAS error
handling routines in non-ARM platforms.

[mchehab: avoid unneeded ifdefs and fix coding style issues]
Signed-off-by: Daniel Ferguson <danielf@os.amperecomputing.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/acpi/apei/ghes.c | 13 ++++++-------
 drivers/ras/ras.c        |  2 ++
 2 files changed, 8 insertions(+), 7 deletions(-)

Comments

Borislav Petkov July 8, 2024, 11:32 a.m. UTC | #1 
On Mon, Jul 08, 2024 at 01:18:10PM +0200, Mauro Carvalho Chehab wrote:
> From: Daniel Ferguson <danielf@os.amperecomputing.com>
> 
> This prevents the unnecessary inclusion of ARM specific RAS error

s/This prevents/Prevent/

Avoid having "This patch" or "This commit" or "This does <bla>" in the commit
message. It is tautologically useless.

"Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy
to do frotz", as if you are giving orders to the codebase to change
its behaviour."
Mauro Carvalho Chehab July 8, 2024, 12:10 p.m. UTC | #2 
Em Mon, 8 Jul 2024 13:32:34 +0200
Borislav Petkov <bp@alien8.de> escreveu:

> On Mon, Jul 08, 2024 at 01:18:10PM +0200, Mauro Carvalho Chehab wrote:
> > From: Daniel Ferguson <danielf@os.amperecomputing.com>
> > 
> > This prevents the unnecessary inclusion of ARM specific RAS error  
> 
> s/This prevents/Prevent/
> 
> Avoid having "This patch" or "This commit" or "This does <bla>" in the commit
> message. It is tautologically useless.
> 
> "Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
> instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy
> to do frotz", as if you are giving orders to the codebase to change
> its behaviour."
> 
> From Documentation/process/submitting-patches.rst
> 
> > handling routines in non-ARM platforms.  
> 
> Ok, this does "something". Why does it do it?
> 
> Otherwise it won't build on other architectures or is it going to cause code
> bloat or why are we doing this?

Probably a better description would be:

    RAS: ACPI: APEI: add conditional compilation to ARM error report functions
    
    Don't include ARM Processor specific error handling routines in 
    non-ARM platforms, preparing it to the next patch, as arm-specific
    kAPI symbols will be used, thus avoiding build breakages when ARM
    is not selected.
    
    [mchehab: avoid unneeded ifdefs and fix coding style issues]
    Signed-off-by: Daniel Ferguson <danielf@os.amperecomputing.com>
    Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

This patch itself just add conditionals to optimize out code on
non-ARM architectures. The next one will add some ARM-specific bits
inside ARM processor CPER trace, thus causing compilation breakages
on non-ARM, due to arm-specific kAPI bits that will be used then.

Thanks,
Mauro
Borislav Petkov July 8, 2024, 2:43 p.m. UTC | #3 
On Mon, Jul 08, 2024 at 02:10:25PM +0200, Mauro Carvalho Chehab wrote:
> This patch itself just add conditionals to optimize out code on
> non-ARM architectures. The next one will add some ARM-specific bits
> inside ARM processor CPER trace, thus causing compilation breakages
> on non-ARM, due to arm-specific kAPI bits that will be used then.

Are you sure?

I have both patches applied and then practically reverting the second one
builds an allmodconfig just fine.

diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index 90efca025d27..524fea3f4f76 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -532,7 +532,6 @@ static bool ghes_handle_arm_hw_error(struct acpi_hest_generic_data *gdata,
 				     int sev, bool sync)
 {
 	bool queued = false;
-#if defined(CONFIG_ARM) || defined (CONFIG_ARM64)
 	struct cper_sec_proc_arm *err = acpi_hest_get_payload(gdata);
 	int flags = sync ? MF_ACTION_REQUIRED : 0;
 	int sec_sev, i;
@@ -570,7 +569,6 @@ static bool ghes_handle_arm_hw_error(struct acpi_hest_generic_data *gdata,
 				    error_type);
 		p += err_info->length;
 	}
-#endif
 	return queued;
 }
 
diff --git a/drivers/ras/ras.c b/drivers/ras/ras.c
index 75acc09bc96a..359bb163aee0 100644
--- a/drivers/ras/ras.c
+++ b/drivers/ras/ras.c
@@ -54,7 +54,6 @@ void log_non_standard_event(const guid_t *sec_type, const guid_t *fru_id,
 
 void log_arm_hw_error(struct cper_sec_proc_arm *err, const u8 sev)
 {
-#if defined(CONFIG_ARM) || defined(CONFIG_ARM64)
 	struct cper_arm_err_info *err_info;
 	struct cper_arm_ctx_info *ctx_info;
 	u8 *ven_err_data;
@@ -97,7 +96,6 @@ void log_arm_hw_error(struct cper_sec_proc_arm *err, const u8 sev)
 
 	trace_arm_event(err, pei_err, pei_len, ctx_err, ctx_len,
 			ven_err_data, (u32)vsei_len, sev, cpu);
-#endif
 }
 
 static int __init ras_init(void)
Jonathan Cameron July 8, 2024, 2:55 p.m. UTC | #4 
On Mon, 8 Jul 2024 14:10:25 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:

> Em Mon, 8 Jul 2024 13:32:34 +0200
> Borislav Petkov <bp@alien8.de> escreveu:
> 
> > On Mon, Jul 08, 2024 at 01:18:10PM +0200, Mauro Carvalho Chehab wrote:  
> > > From: Daniel Ferguson <danielf@os.amperecomputing.com>
> > > 
> > > This prevents the unnecessary inclusion of ARM specific RAS error    
> > 
> > s/This prevents/Prevent/
> > 
> > Avoid having "This patch" or "This commit" or "This does <bla>" in the commit
> > message. It is tautologically useless.
> > 
> > "Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
> > instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy
> > to do frotz", as if you are giving orders to the codebase to change
> > its behaviour."
> > 
> > From Documentation/process/submitting-patches.rst
> >   
> > > handling routines in non-ARM platforms.    
> > 
> > Ok, this does "something". Why does it do it?
> > 
> > Otherwise it won't build on other architectures or is it going to cause code
> > bloat or why are we doing this?  
> 
> Probably a better description would be:
> 
>     RAS: ACPI: APEI: add conditional compilation to ARM error report functions
>     
>     Don't include ARM Processor specific error handling routines in 
>     non-ARM platforms, preparing it to the next patch, as arm-specific
>     kAPI symbols will be used, thus avoiding build breakages when ARM
>     is not selected.
>     
>     [mchehab: avoid unneeded ifdefs and fix coding style issues]
>     Signed-off-by: Daniel Ferguson <danielf@os.amperecomputing.com>
>     Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
With that change log seems fine to me.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> 
> This patch itself just add conditionals to optimize out code on
> non-ARM architectures. The next one will add some ARM-specific bits
> inside ARM processor CPER trace, thus causing compilation breakages
> on non-ARM, due to arm-specific kAPI bits that will be used then.
> 
> Thanks,
> Mauro
Mauro Carvalho Chehab July 11, 2024, 5:26 a.m. UTC | #5 
Em Mon, 8 Jul 2024 16:43:12 +0200
Borislav Petkov <bp@alien8.de> escreveu:

> On Mon, Jul 08, 2024 at 02:10:25PM +0200, Mauro Carvalho Chehab wrote:
> > This patch itself just add conditionals to optimize out code on
> > non-ARM architectures. The next one will add some ARM-specific bits
> > inside ARM processor CPER trace, thus causing compilation breakages
> > on non-ARM, due to arm-specific kAPI bits that will be used then.  
> 
> Are you sure?

That is what reviews to past attempts to merge patch 2 implied. 

> I have both patches applied and then practically reverting the second one
> builds an allmodconfig just fine.

I double-checked the logic: I noticed just one kABI symbol that
it is arm-specific (CPU midr), and there is has already a wrapper 
for it.

I also did a cross-compilation for both x86_64 and s390 to verify,
and indeed it is building fine without the ifdefs.

So, I'll drop patch 1.

Thanks,
Mauro
diff mbox series

Patch

diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index 623cc0cb4a65..2589a3536d91 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -529,11 +529,12 @@  static bool ghes_handle_memory_failure(struct acpi_hest_generic_data *gdata,
 }
 
 static bool ghes_handle_arm_hw_error(struct acpi_hest_generic_data *gdata,
-				       int sev, bool sync)
+				     int sev, bool sync)
 {
+	bool queued = false;
+#if defined(CONFIG_ARM) || defined (CONFIG_ARM64)
 	struct cper_sec_proc_arm *err = acpi_hest_get_payload(gdata);
 	int flags = sync ? MF_ACTION_REQUIRED : 0;
-	bool queued = false;
 	int sec_sev, i;
 	char *p;
 
@@ -570,7 +571,7 @@  static bool ghes_handle_arm_hw_error(struct acpi_hest_generic_data *gdata,
 				    error_type);
 		p += err_info->length;
 	}
-
+#endif
 	return queued;
 }
 
@@ -773,11 +774,9 @@  static bool ghes_do_proc(struct ghes *ghes,
 
 			arch_apei_report_mem_error(sev, mem_err);
 			queued = ghes_handle_memory_failure(gdata, sev, sync);
-		}
-		else if (guid_equal(sec_type, &CPER_SEC_PCIE)) {
+		} else if (guid_equal(sec_type, &CPER_SEC_PCIE)) {
 			ghes_handle_aer(gdata);
-		}
-		else if (guid_equal(sec_type, &CPER_SEC_PROC_ARM)) {
+		} else if (guid_equal(sec_type, &CPER_SEC_PROC_ARM)) {
 			queued = ghes_handle_arm_hw_error(gdata, sev, sync);
 		} else if (guid_equal(sec_type, &CPER_SEC_CXL_GEN_MEDIA_GUID)) {
 			struct cxl_cper_event_rec *rec = acpi_hest_get_payload(gdata);
diff --git a/drivers/ras/ras.c b/drivers/ras/ras.c
index a6e4792a1b2e..5d94ab79c8c3 100644
--- a/drivers/ras/ras.c
+++ b/drivers/ras/ras.c
@@ -54,7 +54,9 @@  void log_non_standard_event(const guid_t *sec_type, const guid_t *fru_id,
 
 void log_arm_hw_error(struct cper_sec_proc_arm *err)
 {
+#if defined(CONFIG_ARM) || defined(CONFIG_ARM64)
 	trace_arm_event(err);
+#endif
 }
 
 static int __init ras_init(void)