[PATCH v2 0/2] Decode raw MSR values of MCA registers in BERT

[PATCH v2 0/2] Decode raw MSR values of MCA registers in BERT

[Smita Koralahalli]

This series provides better decoding for SMCA specific raw MSR values in
BERT reported using x86 Processor Error Common Platform Error Record
(CPER) format.

Patch 1 extracts the raw MSR values of MCA registers in BERT and passes
it through the MCA handling chain.

Patch 2 provides a fix of missing error logs as observed in Patch 1.

Patch v2 provides a fix for the build error observed in Patch v1.
As reported by kernel test robot, Patch 1 failed stating undefined
reference to arch_apei_report_x86_error() in function cper_print_proc_ia.
The failure is noticed when CONFIG_ACPI_APEI is not compiled into the
kernel. Fix the error by returning error code if CONFIG_ACPI_APEI is not
installed into the kernel.

Links:
https://lkml.kernel.org/r/20200825144710.23584-1-Smita.KoralahalliChannabasappa@amd.com
https://lkml.kernel.org/r/20200825144710.23584-2-Smita.KoralahalliChannabasappa@amd.com
https://lkml.kernel.org/r/20200825144710.23584-3-Smita.KoralahalliChannabasappa@amd.com

Smita Koralahalli (2):
  cper, apei, mce: Pass x86 CPER through the MCA handling chain
  x86/mce/dev-mcelog: Fix updating kflags in AMD systems

 arch/x86/include/asm/mce.h           |  3 +++
 arch/x86/kernel/acpi/apei.c          |  9 +++++++
 arch/x86/kernel/cpu/mce/apei.c       | 37 ++++++++++++++++++++++++++++
 arch/x86/kernel/cpu/mce/dev-mcelog.c |  4 ++-
 drivers/firmware/efi/cper-x86.c      | 10 +++++---
 include/acpi/apei.h                  |  9 +++++++
 6 files changed, 67 insertions(+), 5 deletions(-)

-- 
2.17.1

[PATCH v2 1/2] cper, apei, mce: Pass x86 CPER through the MCA handling chain

[Smita Koralahalli]

Linux Kernel uses ACPI Boot Error Record Table (BERT) to report fatal
errors that occurred in a previous boot. The MCA errors in the BERT are
reported using the x86 Processor Error Common Platform Error Record (CPER)
format. Currently, the record prints out the raw MSR values and AMD relies
on the raw record to provide MCA information.

Extract the raw MSR values of MCA registers from the BERT and feed it into
the standard mce_log() function through the existing x86/MCA RAS
infrastructure. This will result in better decoding from the EDAC MCE
decoder or the default notifier.

The implementation is SMCA specific as the raw MCA register values are
given in the register offset order of the MCAX address space.

Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>
---
v2:
	Fixed build error reported by kernel test robot.
	Passed struct variable as function argument instead of entire struct

 arch/x86/include/asm/mce.h      |  3 +++
 arch/x86/kernel/acpi/apei.c     |  9 ++++++++
 arch/x86/kernel/cpu/mce/apei.c  | 37 +++++++++++++++++++++++++++++++++
 drivers/firmware/efi/cper-x86.c | 10 +++++----
 include/acpi/apei.h             |  9 ++++++++
 5 files changed, 64 insertions(+), 4 deletions(-)

diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
index cf503824529c..139a89945225 100644
--- a/arch/x86/include/asm/mce.h
+++ b/arch/x86/include/asm/mce.h
@@ -291,6 +291,9 @@ struct cper_sec_mem_err;
 extern void apei_mce_report_mem_error(int corrected,
 				      struct cper_sec_mem_err *mem_err);
 
+struct cper_ia_proc_ctx;
+int apei_mce_report_x86_error(struct cper_ia_proc_ctx *ctx_info, u64 lapic_id);
+
 /*
  * Enumerate new IP types and HWID values in AMD processors which support
  * Scalable MCA.
diff --git a/arch/x86/kernel/acpi/apei.c b/arch/x86/kernel/acpi/apei.c
index c22fb55abcfd..13d60a91eaa0 100644
--- a/arch/x86/kernel/acpi/apei.c
+++ b/arch/x86/kernel/acpi/apei.c
@@ -43,3 +43,12 @@ void arch_apei_report_mem_error(int sev, struct cper_sec_mem_err *mem_err)
 	apei_mce_report_mem_error(sev, mem_err);
 #endif
 }
+
+int arch_apei_report_x86_error(struct cper_ia_proc_ctx *ctx_info, u64 lapic_id)
+{
+	int err = -EINVAL;
+#ifdef CONFIG_X86_MCE
+	err = apei_mce_report_x86_error(ctx_info, lapic_id);
+#endif
+	return err;
+}
diff --git a/arch/x86/kernel/cpu/mce/apei.c b/arch/x86/kernel/cpu/mce/apei.c
index af8d37962586..836bd9296116 100644
--- a/arch/x86/kernel/cpu/mce/apei.c
+++ b/arch/x86/kernel/cpu/mce/apei.c
@@ -51,6 +51,43 @@ void apei_mce_report_mem_error(int severity, struct cper_sec_mem_err *mem_err)
 }
 EXPORT_SYMBOL_GPL(apei_mce_report_mem_error);
 
+int apei_mce_report_x86_error(struct cper_ia_proc_ctx *ctx_info, u64 lapic_id)
+{
+	const u64 *i_mce = ((const void *) (ctx_info + 1));
+	unsigned int cpu;
+	struct mce m;
+
+	if (!boot_cpu_has(X86_FEATURE_SMCA))
+		return -EINVAL;
+
+	mce_setup(&m);
+
+	m.extcpu = -1;
+	m.socketid = -1;
+
+	for_each_possible_cpu(cpu) {
+		if (cpu_data(cpu).initial_apicid == lapic_id) {
+			m.extcpu = cpu;
+			m.socketid = cpu_data(m.extcpu).phys_proc_id;
+			break;
+		}
+	}
+
+	m.apicid = lapic_id;
+	m.bank = (ctx_info->msr_addr >> 4) & 0xFF;
+	m.status = *i_mce;
+	m.addr = *(i_mce + 1);
+	m.misc = *(i_mce + 2);
+	/* Skipping MCA_CONFIG */
+	m.ipid = *(i_mce + 4);
+	m.synd = *(i_mce + 5);
+
+	mce_log(&m);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(apei_mce_report_x86_error);
+
 #define CPER_CREATOR_MCE						\
 	GUID_INIT(0x75a574e3, 0x5052, 0x4b29, 0x8a, 0x8e, 0xbe, 0x2c,	\
 		  0x64, 0x90, 0xb8, 0x9d)
diff --git a/drivers/firmware/efi/cper-x86.c b/drivers/firmware/efi/cper-x86.c
index 2531de49f56c..374b8e18552a 100644
--- a/drivers/firmware/efi/cper-x86.c
+++ b/drivers/firmware/efi/cper-x86.c
@@ -1,7 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
 // Copyright (C) 2018, Advanced Micro Devices, Inc.
 
-#include <linux/cper.h>
+#include <acpi/apei.h>
 
 /*
  * We don't need a "CPER_IA" prefix since these are all locally defined.
@@ -347,9 +347,11 @@ void cper_print_proc_ia(const char *pfx, const struct cper_sec_proc_ia *proc)
 			       ctx_info->mm_reg_addr);
 		}
 
-		printk("%sRegister Array:\n", newpfx);
-		print_hex_dump(newpfx, "", DUMP_PREFIX_OFFSET, 16, groupsize,
-			       (ctx_info + 1), ctx_info->reg_arr_size, 0);
+		if (arch_apei_report_x86_error(ctx_info, proc->lapic_id)) {
+			printk("%sRegister Array:\n", newpfx);
+			print_hex_dump(newpfx, "", DUMP_PREFIX_OFFSET, 16, groupsize,
+				       (ctx_info + 1), ctx_info->reg_arr_size, 0);
+		}
 
 		ctx_info = (struct cper_ia_proc_ctx *)((long)ctx_info + size);
 	}
diff --git a/include/acpi/apei.h b/include/acpi/apei.h
index 680f80960c3d..44d4d08acce0 100644
--- a/include/acpi/apei.h
+++ b/include/acpi/apei.h
@@ -33,8 +33,15 @@ extern bool ghes_disable;
 
 #ifdef CONFIG_ACPI_APEI
 void __init acpi_hest_init(void);
+int arch_apei_report_x86_error(struct cper_ia_proc_ctx *ctx_info,
+			       u64 lapic_id);
 #else
 static inline void acpi_hest_init(void) { return; }
+static inline int arch_apei_report_x86_error(struct cper_ia_proc_ctx *ctx_info,
+					     u64 lapic_id)
+{
+	return -EINVAL;
+}
 #endif
 
 typedef int (*apei_hest_func_t)(struct acpi_hest_header *hest_hdr, void *data);
@@ -51,6 +58,8 @@ int erst_clear(u64 record_id);
 
 int arch_apei_enable_cmcff(struct acpi_hest_header *hest_hdr, void *data);
 void arch_apei_report_mem_error(int sev, struct cper_sec_mem_err *mem_err);
+int arch_apei_report_x86_error(struct cper_ia_proc_ctx *ctx_info,
+			       u64 lapic_id);
 
 #endif
 #endif
-- 
2.17.1

[PATCH v2 2/2] x86/mce/dev-mcelog: Fix updating kflags in AMD systems

[Smita Koralahalli]

The mcelog utility is not commonly used on AMD systems. Therefore, errors
logged only by the dev_mce_log() notifier will be missed. This may occur
if the EDAC modules are not loaded in which case it's preferable to print
the error record by the default notifier.

However, the mce->kflags set by dev_mce_log() notifier makes the default
notifier to skip over the errors assuming they are processed by
dev_mce_log().

Do not update kflags in the dev_mce_log() notifier on AMD systems.

Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>
---
v2:
	No change

 arch/x86/kernel/cpu/mce/dev-mcelog.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/mce/dev-mcelog.c b/arch/x86/kernel/cpu/mce/dev-mcelog.c
index 03e51053592a..100fbeebdc72 100644
--- a/arch/x86/kernel/cpu/mce/dev-mcelog.c
+++ b/arch/x86/kernel/cpu/mce/dev-mcelog.c
@@ -67,7 +67,9 @@ static int dev_mce_log(struct notifier_block *nb, unsigned long val,
 unlock:
 	mutex_unlock(&mce_chrdev_read_mutex);
 
-	mce->kflags |= MCE_HANDLED_MCELOG;
+	if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD)
+		mce->kflags |= MCE_HANDLED_MCELOG;
+
 	return NOTIFY_OK;
 }
 
-- 
2.17.1

Re: [PATCH v2 1/2] cper, apei, mce: Pass x86 CPER through the MCA handling chain

[Punit Agrawal]

Hi Smita,

A couple of comments below -

Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com> writes:

> Linux Kernel uses ACPI Boot Error Record Table (BERT) to report fatal
> errors that occurred in a previous boot. The MCA errors in the BERT are
> reported using the x86 Processor Error Common Platform Error Record (CPER)
> format. Currently, the record prints out the raw MSR values and AMD relies
> on the raw record to provide MCA information.
>
> Extract the raw MSR values of MCA registers from the BERT and feed it into
> the standard mce_log() function through the existing x86/MCA RAS
> infrastructure. This will result in better decoding from the EDAC MCE
> decoder or the default notifier.
>
> The implementation is SMCA specific as the raw MCA register values are
> given in the register offset order of the MCAX address space.
>
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>
> ---

[...]


> diff --git a/drivers/firmware/efi/cper-x86.c b/drivers/firmware/efi/cper-x86.c
> index 2531de49f56c..374b8e18552a 100644
> --- a/drivers/firmware/efi/cper-x86.c
> +++ b/drivers/firmware/efi/cper-x86.c
> @@ -1,7 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0
>  // Copyright (C) 2018, Advanced Micro Devices, Inc.
>  
> -#include <linux/cper.h>

Why is the include dropped? AFAICT, the definitions from there are still
being used after this patch.

> +#include <acpi/apei.h>
>  
>  /*
>   * We don't need a "CPER_IA" prefix since these are all locally defined.
> @@ -347,9 +347,11 @@ void cper_print_proc_ia(const char *pfx, const struct cper_sec_proc_ia *proc)
>  			       ctx_info->mm_reg_addr);
>  		}
>  
> -		printk("%sRegister Array:\n", newpfx);
> -		print_hex_dump(newpfx, "", DUMP_PREFIX_OFFSET, 16, groupsize,
> -			       (ctx_info + 1), ctx_info->reg_arr_size, 0);
> +		if (arch_apei_report_x86_error(ctx_info, proc->lapic_id)) {
> +			printk("%sRegister Array:\n", newpfx);
> +			print_hex_dump(newpfx, "", DUMP_PREFIX_OFFSET, 16, groupsize,
> +				       (ctx_info + 1), ctx_info->reg_arr_size, 0);
> +		}
>  
>  		ctx_info = (struct cper_ia_proc_ctx *)((long)ctx_info + size);
>  	}
> diff --git a/include/acpi/apei.h b/include/acpi/apei.h
> index 680f80960c3d..44d4d08acce0 100644
> --- a/include/acpi/apei.h
> +++ b/include/acpi/apei.h
> @@ -33,8 +33,15 @@ extern bool ghes_disable;
>  
>  #ifdef CONFIG_ACPI_APEI
>  void __init acpi_hest_init(void);
> +int arch_apei_report_x86_error(struct cper_ia_proc_ctx *ctx_info,
> +			       u64 lapic_id);
>  #else
>  static inline void acpi_hest_init(void) { return; }
> +static inline int arch_apei_report_x86_error(struct cper_ia_proc_ctx *ctx_info,
> +					     u64 lapic_id)
> +{
> +	return -EINVAL;
> +}
>  #endif

Adding the declaration to this include violates the separation of
generic and architecture specific code.

Can this be moved to the appropriate architecture specific header?
Perhaps arch/x86/include/asm/apei.h.

>  typedef int (*apei_hest_func_t)(struct acpi_hest_header *hest_hdr, void *data);
> @@ -51,6 +58,8 @@ int erst_clear(u64 record_id);
>  
>  int arch_apei_enable_cmcff(struct acpi_hest_header *hest_hdr, void *data);
>  void arch_apei_report_mem_error(int sev, struct cper_sec_mem_err *mem_err);
> +int arch_apei_report_x86_error(struct cper_ia_proc_ctx *ctx_info,
> +			       u64 lapic_id);


Why is the additional declaration needed?

Thanks,
Punit

>  
>  #endif
>  #endif

Re: [PATCH v2 1/2] cper, apei, mce: Pass x86 CPER through the MCA handling chain

[Yazen Ghannam]

On Fri, Aug 28, 2020 at 03:33:31PM -0500, Smita Koralahalli wrote:
...
> +int apei_mce_report_x86_error(struct cper_ia_proc_ctx *ctx_info, u64 lapic_id)
> +{
> +	const u64 *i_mce = ((const void *) (ctx_info + 1));
> +	unsigned int cpu;
> +	struct mce m;
> +
> +	if (!boot_cpu_has(X86_FEATURE_SMCA))
> +		return -EINVAL;
> +

This function is called on any context type, but it can only decode
"MSR" types that follow the MCAX register layout used on Scalable MCA
systems.

So I think there should be a couple of checks added:
1) Context type is "MSR".
2) Register layout follows what is expected below. There's no explict
way to do this, since the data is implemenation-specific. But at least
there can be a check that the starting MSR address matches the first
expected register: Bank's MCA_STATUS in MCAX space (0xC0002XX1).

For example:

	(ctx_info->msr_addr & 0xC0002001) == 0xC0002001

The raw value in the example should be defined with a name.

> +	mce_setup(&m);
> +
> +	m.extcpu = -1;
> +	m.socketid = -1;
> +
> +	for_each_possible_cpu(cpu) {
> +		if (cpu_data(cpu).initial_apicid == lapic_id) {
> +			m.extcpu = cpu;
> +			m.socketid = cpu_data(m.extcpu).phys_proc_id;
> +			break;
> +		}
> +	}
> +
> +	m.apicid = lapic_id;
> +	m.bank = (ctx_info->msr_addr >> 4) & 0xFF;
> +	m.status = *i_mce;
> +	m.addr = *(i_mce + 1);
> +	m.misc = *(i_mce + 2);
> +	/* Skipping MCA_CONFIG */
> +	m.ipid = *(i_mce + 4);
> +	m.synd = *(i_mce + 5);
> +
> +	mce_log(&m);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(apei_mce_report_x86_error);
> +

Thanks,
Yazen

Re: [PATCH v2 1/2] cper, apei, mce: Pass x86 CPER through the MCA handling chain

[Smita Koralahalli Channabasappa]

On 8/31/20 12:05 AM, Punit Agrawal wrote:

> Hi Smita,
>
> A couple of comments below -
>
> Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com> writes:
>
> [...]
>
>
>> diff --git a/drivers/firmware/efi/cper-x86.c b/drivers/firmware/efi/cper-x86.c
>> index 2531de49f56c..374b8e18552a 100644
>> --- a/drivers/firmware/efi/cper-x86.c
>> +++ b/drivers/firmware/efi/cper-x86.c
>> @@ -1,7 +1,7 @@
>>   // SPDX-License-Identifier: GPL-2.0
>>   // Copyright (C) 2018, Advanced Micro Devices, Inc.
>>   
>> -#include <linux/cper.h>
> Why is the include dropped? AFAICT, the definitions from there are still
> being used after this patch.

Dropped because <acpi/apei.h> already includes <linux/cper.h>

>> +#include <acpi/apei.h>

[...]

>> diff --git a/include/acpi/apei.h b/include/acpi/apei.h
>> index 680f80960c3d..44d4d08acce0 100644
>> --- a/include/acpi/apei.h
>> +++ b/include/acpi/apei.h
>> @@ -33,8 +33,15 @@ extern bool ghes_disable;
>>   
>>   #ifdef CONFIG_ACPI_APEI
>>   void __init acpi_hest_init(void);
>> +int arch_apei_report_x86_error(struct cper_ia_proc_ctx *ctx_info,
>> +			       u64 lapic_id);
>>   #else
>>   static inline void acpi_hest_init(void) { return; }
>> +static inline int arch_apei_report_x86_error(struct cper_ia_proc_ctx *ctx_info,
>> +					     u64 lapic_id)
>> +{
>> +	return -EINVAL;
>> +}
>>   #endif
> Adding the declaration to this include violates the separation of
> generic and architecture specific code.
>
> Can this be moved to the appropriate architecture specific header?
> Perhaps arch/x86/include/asm/apei.h.

Yes, I have fixed this and moved into arch/x86/include/asm/acpi.h.

>>   typedef int (*apei_hest_func_t)(struct acpi_hest_header *hest_hdr, void *data);
>> @@ -51,6 +58,8 @@ int erst_clear(u64 record_id);
>>   
>>   int arch_apei_enable_cmcff(struct acpi_hest_header *hest_hdr, void *data);
>>   void arch_apei_report_mem_error(int sev, struct cper_sec_mem_err *mem_err);
>> +int arch_apei_report_x86_error(struct cper_ia_proc_ctx *ctx_info,
>> +			       u64 lapic_id);
>
> Why is the additional declaration needed?

Will fix in the next revision.

Thanks,
Smita

[...]

Re: [PATCH v2 1/2] cper, apei, mce: Pass x86 CPER through the MCA handling chain

[Punit Agrawal]

Hi Smita,

Smita Koralahalli Channabasappa <skoralah@amd.com> writes:

> On 8/31/20 12:05 AM, Punit Agrawal wrote:
>
>> Hi Smita,
>>
>> A couple of comments below -
>>
>> Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com> writes:
>>
>> [...]
>>
>>
>>> diff --git a/drivers/firmware/efi/cper-x86.c b/drivers/firmware/efi/cper-x86.c
>>> index 2531de49f56c..374b8e18552a 100644
>>> --- a/drivers/firmware/efi/cper-x86.c
>>> +++ b/drivers/firmware/efi/cper-x86.c
>>> @@ -1,7 +1,7 @@
>>>   // SPDX-License-Identifier: GPL-2.0
>>>   // Copyright (C) 2018, Advanced Micro Devices, Inc.
>>>   -#include <linux/cper.h>
>> Why is the include dropped? AFAICT, the definitions from there are still
>> being used after this patch.
>
> Dropped because <acpi/apei.h> already includes <linux/cper.h>

Generally, you want to follow the rule that if a declaration from a
header file is being used, it should show up in the includes. The same
applies to both source as well as header files.

It doesn't matter if another include in the source file in turn ends up
including the same header again; the #ifdef guards are there to prevent
duplicate declarations.

The rationale is that if future changes remove the usage of
<acpi/apei.h>, the C file can still be compiled after dropping the
include; there should be no need to then re-introduce <linux/cper.h> at
that point.

Hope that makes sense.

Thanks,
Punit

>>> +#include <acpi/apei.h>
>
> [...]
>
>>> diff --git a/include/acpi/apei.h b/include/acpi/apei.h
>>> index 680f80960c3d..44d4d08acce0 100644
>>> --- a/include/acpi/apei.h
>>> +++ b/include/acpi/apei.h
>>> @@ -33,8 +33,15 @@ extern bool ghes_disable;
>>>     #ifdef CONFIG_ACPI_APEI
>>>   void __init acpi_hest_init(void);
>>> +int arch_apei_report_x86_error(struct cper_ia_proc_ctx *ctx_info,
>>> +			       u64 lapic_id);
>>>   #else
>>>   static inline void acpi_hest_init(void) { return; }
>>> +static inline int arch_apei_report_x86_error(struct cper_ia_proc_ctx *ctx_info,
>>> +					     u64 lapic_id)
>>> +{
>>> +	return -EINVAL;
>>> +}
>>>   #endif
>> Adding the declaration to this include violates the separation of
>> generic and architecture specific code.
>>
>> Can this be moved to the appropriate architecture specific header?
>> Perhaps arch/x86/include/asm/apei.h.
>
> Yes, I have fixed this and moved into arch/x86/include/asm/acpi.h.
>
>>>   typedef int (*apei_hest_func_t)(struct acpi_hest_header *hest_hdr, void *data);
>>> @@ -51,6 +58,8 @@ int erst_clear(u64 record_id);
>>>     int arch_apei_enable_cmcff(struct acpi_hest_header *hest_hdr,
>>> void *data);
>>>   void arch_apei_report_mem_error(int sev, struct cper_sec_mem_err *mem_err);
>>> +int arch_apei_report_x86_error(struct cper_ia_proc_ctx *ctx_info,
>>> +			       u64 lapic_id);
>>
>> Why is the additional declaration needed?
>
> Will fix in the next revision.
>
> Thanks,
> Smita
>
> [...]

Re: [PATCH v2 1/2] cper, apei, mce: Pass x86 CPER through the MCA handling chain

[Ard Biesheuvel]

On Thu, 3 Sep 2020 at 09:34, Punit Agrawal <punit1.agrawal@toshiba.co.jp> wrote:
>
> Hi Smita,
>
> Smita Koralahalli Channabasappa <skoralah@amd.com> writes:
>
> > On 8/31/20 12:05 AM, Punit Agrawal wrote:
> >
> >> Hi Smita,
> >>
> >> A couple of comments below -
> >>
> >> Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com> writes:
> >>
> >> [...]
> >>
> >>
> >>> diff --git a/drivers/firmware/efi/cper-x86.c b/drivers/firmware/efi/cper-x86.c
> >>> index 2531de49f56c..374b8e18552a 100644
> >>> --- a/drivers/firmware/efi/cper-x86.c
> >>> +++ b/drivers/firmware/efi/cper-x86.c
> >>> @@ -1,7 +1,7 @@
> >>>   // SPDX-License-Identifier: GPL-2.0
> >>>   // Copyright (C) 2018, Advanced Micro Devices, Inc.
> >>>   -#include <linux/cper.h>
> >> Why is the include dropped? AFAICT, the definitions from there are still
> >> being used after this patch.
> >
> > Dropped because <acpi/apei.h> already includes <linux/cper.h>
>
> Generally, you want to follow the rule that if a declaration from a
> header file is being used, it should show up in the includes. The same
> applies to both source as well as header files.
>
> It doesn't matter if another include in the source file in turn ends up
> including the same header again; the #ifdef guards are there to prevent
> duplicate declarations.
>
> The rationale is that if future changes remove the usage of
> <acpi/apei.h>, the C file can still be compiled after dropping the
> include; there should be no need to then re-introduce <linux/cper.h> at
> that point.
>
> Hope that makes sense.
>

Agreed. If the code still uses declarations from linux/cper.h after
the patch, the #include should remain.

Re: [PATCH v2 1/2] cper, apei, mce: Pass x86 CPER through the MCA handling chain

[Smita Koralahalli Channabasappa]

On 9/11/20 1:23 PM, Ard Biesheuvel wrote:

> On Thu, 3 Sep 2020 at 09:34, Punit Agrawal <punit1.agrawal@toshiba.co.jp> wrote:
>> Hi Smita,
>>
>> Smita Koralahalli Channabasappa <skoralah@amd.com> writes:
>>
>>> On 8/31/20 12:05 AM, Punit Agrawal wrote:
>>>
>>>> Hi Smita,
>>>>
>>>> A couple of comments below -
>>>>
>>>> Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com> writes:
>>>>
>>>> [...]
>>>>
>>>>
>>>>> diff --git a/drivers/firmware/efi/cper-x86.c b/drivers/firmware/efi/cper-x86.c
>>>>> index 2531de49f56c..374b8e18552a 100644
>>>>> --- a/drivers/firmware/efi/cper-x86.c
>>>>> +++ b/drivers/firmware/efi/cper-x86.c
>>>>> @@ -1,7 +1,7 @@
>>>>>    // SPDX-License-Identifier: GPL-2.0
>>>>>    // Copyright (C) 2018, Advanced Micro Devices, Inc.
>>>>>    -#include <linux/cper.h>
>>>> Why is the include dropped? AFAICT, the definitions from there are still
>>>> being used after this patch.
>>> Dropped because <acpi/apei.h> already includes <linux/cper.h>
>> Generally, you want to follow the rule that if a declaration from a
>> header file is being used, it should show up in the includes. The same
>> applies to both source as well as header files.
>>
>> It doesn't matter if another include in the source file in turn ends up
>> including the same header again; the #ifdef guards are there to prevent
>> duplicate declarations.
>>
>> The rationale is that if future changes remove the usage of
>> <acpi/apei.h>, the C file can still be compiled after dropping the
>> include; there should be no need to then re-introduce <linux/cper.h> at
>> that point.
>>
>> Hope that makes sense.
>>
> Agreed. If the code still uses declarations from linux/cper.h after
> the patch, the #include should remain.

Thanks, I have taken care of the comments and sent out the next revision now.
https://lkml.kernel.org/r/20200903234531.162484-2-Smita.KoralahalliChannabasappa@amd.com.
Please ignore my previous email.

Thanks,
Smita