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 (mailing list archive)
State Handled Elsewhere, archived
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."

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