linux-edac.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v5] cper, apei, mce: Pass x86 CPER through the MCA handling chain
@ 2020-11-03 16:49 Smita Koralahalli
  2020-11-06  5:36 ` Punit Agrawal
  2020-11-06 12:09 ` Borislav Petkov
  0 siblings, 2 replies; 9+ messages in thread
From: Smita Koralahalli @ 2020-11-03 16:49 UTC (permalink / raw)
  To: x86, linux-kernel, linux-pm, linux-edac, linux-efi, linux-acpi
  Cc: Borislav Petkov, Tony Luck, Rafael J . Wysocki, Len Brown,
	Ard Biesheuvel, Punit Agrawal, Yazen Ghannam,
	Smita.KoralahalliChannabasappa

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.

[ Fix a build breakage in patch v1. ]
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>
---
Link:
https://lkml.kernel.org/r/20200904140444.161291-1-Smita.KoralahalliChannabasappa@amd.com

v5:
	Included check to determine if register array size is large
	enough to hold all the registers which we want to extract.
	Used already defined MSR_AMD64_SMCA_MC0_STATUS.
v4:
	Included what kernel test robot reported.
	Changed function name from apei_mce_report_x86_error ->
	apei_smca_report_x86_error.
	Added comment for MASK_MCA_STATUS definition.
	Wrapped apei_smca_report_x86_error() with CONFIG_X86_MCE in
	arch/x86/include/asm/mce.h
v3:
	Moved arch specific declarations from generic headers to arch
	specific headers.
	Cleaned additional declarations which are unnecessary.
	Included the check for context type.
	Added additional check to verify for appropriate MSR address in
	the register layout.
v2:
	Fixed build error reported by kernel test robot.
	Passed struct variable as function argument instead of entire struct.
---
 arch/x86/include/asm/acpi.h     | 11 +++++++
 arch/x86/include/asm/mce.h      |  6 ++++
 arch/x86/kernel/acpi/apei.c     |  5 +++
 arch/x86/kernel/cpu/mce/apei.c  | 56 +++++++++++++++++++++++++++++++++
 drivers/firmware/efi/cper-x86.c | 11 +++++--
 5 files changed, 86 insertions(+), 3 deletions(-)

diff --git a/arch/x86/include/asm/acpi.h b/arch/x86/include/asm/acpi.h
index 6d2df1ee427b..65064d9f7fa6 100644
--- a/arch/x86/include/asm/acpi.h
+++ b/arch/x86/include/asm/acpi.h
@@ -159,6 +159,8 @@ static inline u64 x86_default_get_root_pointer(void)
 extern int x86_acpi_numa_init(void);
 #endif /* CONFIG_ACPI_NUMA */
 
+struct cper_ia_proc_ctx;
+
 #ifdef CONFIG_ACPI_APEI
 static inline pgprot_t arch_apei_get_mem_attribute(phys_addr_t addr)
 {
@@ -177,6 +179,15 @@ static inline pgprot_t arch_apei_get_mem_attribute(phys_addr_t addr)
 	 */
 	return PAGE_KERNEL_NOENC;
 }
+
+int arch_apei_report_x86_error(struct cper_ia_proc_ctx *ctx_info,
+			       u64 lapic_id);
+#else
+static inline int arch_apei_report_x86_error(struct cper_ia_proc_ctx *ctx_info,
+					     u64 lapic_id)
+{
+	return -EINVAL;
+}
 #endif
 
 #define ACPI_TABLE_UPGRADE_MAX_PHYS (max_low_pfn_mapped << PAGE_SHIFT)
diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
index a0f147893a04..07eedfd6ec38 100644
--- a/arch/x86/include/asm/mce.h
+++ b/arch/x86/include/asm/mce.h
@@ -198,16 +198,22 @@ static inline void enable_copy_mc_fragile(void)
 }
 #endif
 
+struct cper_ia_proc_ctx;
+
 #ifdef CONFIG_X86_MCE
 int mcheck_init(void);
 void mcheck_cpu_init(struct cpuinfo_x86 *c);
 void mcheck_cpu_clear(struct cpuinfo_x86 *c);
 void mcheck_vendor_init_severity(void);
+int apei_smca_report_x86_error(struct cper_ia_proc_ctx *ctx_info,
+			       u64 lapic_id);
 #else
 static inline int mcheck_init(void) { return 0; }
 static inline void mcheck_cpu_init(struct cpuinfo_x86 *c) {}
 static inline void mcheck_cpu_clear(struct cpuinfo_x86 *c) {}
 static inline void mcheck_vendor_init_severity(void) {}
+static inline int apei_smca_report_x86_error(struct cper_ia_proc_ctx *ctx_info,
+					     u64 lapic_id) { return -EINVAL; }
 #endif
 
 #ifdef CONFIG_X86_ANCIENT_MCE
diff --git a/arch/x86/kernel/acpi/apei.c b/arch/x86/kernel/acpi/apei.c
index c22fb55abcfd..0916f00a992e 100644
--- a/arch/x86/kernel/acpi/apei.c
+++ b/arch/x86/kernel/acpi/apei.c
@@ -43,3 +43,8 @@ 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)
+{
+	return apei_smca_report_x86_error(ctx_info, lapic_id);
+}
diff --git a/arch/x86/kernel/cpu/mce/apei.c b/arch/x86/kernel/cpu/mce/apei.c
index af8d37962586..f56f0bc147e2 100644
--- a/arch/x86/kernel/cpu/mce/apei.c
+++ b/arch/x86/kernel/cpu/mce/apei.c
@@ -51,6 +51,62 @@ 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_smca_report_x86_error(struct cper_ia_proc_ctx *ctx_info, u64 lapic_id)
+{
+	const u64 *i_mce = ((const u64 *) (ctx_info + 1));
+	unsigned int cpu;
+	struct mce m;
+
+	if (!boot_cpu_has(X86_FEATURE_SMCA))
+		return -EINVAL;
+
+	/*
+	 * The starting address of the Register Array extracted from BERT
+	 * must match with the first expected register in the register
+	 * layout of MCAX address space. In SMCA systems this address
+	 * corresponds to banks's MCA_STATUS register.
+	 *
+	 * The Register array size must be large enough to include all
+	 * the SMCA registers which we want to extract.
+	 *
+	 * The number of registers in the Register Array is determined
+	 * by Register Array Size/8 as defined in UEFI spec v2.8, sec
+	 * N.2.4.2.2. The register layout is fixed and currently the raw
+	 * data in the register array includes 6 SMCA registers which the
+	 * kernel can extract.
+	 */
+
+	if ((ctx_info->msr_addr & MSR_AMD64_SMCA_MC0_STATUS) !=
+	    MSR_AMD64_SMCA_MC0_STATUS || ctx_info->reg_arr_size < 48)
+		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;
+}
+
 #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..438ed9eff6d0 100644
--- a/drivers/firmware/efi/cper-x86.c
+++ b/drivers/firmware/efi/cper-x86.c
@@ -2,6 +2,7 @@
 // Copyright (C) 2018, Advanced Micro Devices, Inc.
 
 #include <linux/cper.h>
+#include <linux/acpi.h>
 
 /*
  * We don't need a "CPER_IA" prefix since these are all locally defined.
@@ -347,9 +348,13 @@ 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 (ctx_info->reg_ctx_type != CTX_TYPE_MSR ||
+		    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);
 	}
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v5] cper, apei, mce: Pass x86 CPER through the MCA handling chain
  2020-11-03 16:49 [PATCH v5] cper, apei, mce: Pass x86 CPER through the MCA handling chain Smita Koralahalli
@ 2020-11-06  5:36 ` Punit Agrawal
  2020-11-06 12:09   ` Borislav Petkov
  2020-11-06 12:09 ` Borislav Petkov
  1 sibling, 1 reply; 9+ messages in thread
From: Punit Agrawal @ 2020-11-06  5:36 UTC (permalink / raw)
  To: Smita Koralahalli
  Cc: x86, linux-kernel, linux-pm, linux-edac, linux-efi, linux-acpi,
	Borislav Petkov, Tony Luck, Rafael J . Wysocki, Len Brown,
	Ard Biesheuvel, Yazen Ghannam

Hi Smita,

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.
>
> [ Fix a build breakage in patch v1. ]
> 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..438ed9eff6d0 100644
> --- a/drivers/firmware/efi/cper-x86.c
> +++ b/drivers/firmware/efi/cper-x86.c
> @@ -2,6 +2,7 @@
>  // Copyright (C) 2018, Advanced Micro Devices, Inc.
>  
>  #include <linux/cper.h>
> +#include <linux/acpi.h>

Did you mean to include <asm/acpi.h>?

>  
>  /*
>   * We don't need a "CPER_IA" prefix since these are all locally defined.
> @@ -347,9 +348,13 @@ 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 (ctx_info->reg_ctx_type != CTX_TYPE_MSR ||
> +		    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);
>  	}

With that addressed,

FWIW,

Reviewed-by: Punit Agrawal <punit1.agrawal@toshiba.co.jp>

Thanks,
Punit

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v5] cper, apei, mce: Pass x86 CPER through the MCA handling chain
  2020-11-03 16:49 [PATCH v5] cper, apei, mce: Pass x86 CPER through the MCA handling chain Smita Koralahalli
  2020-11-06  5:36 ` Punit Agrawal
@ 2020-11-06 12:09 ` Borislav Petkov
  2020-11-09 18:36   ` Smita Koralahalli Channabasappa
  1 sibling, 1 reply; 9+ messages in thread
From: Borislav Petkov @ 2020-11-06 12:09 UTC (permalink / raw)
  To: Smita Koralahalli
  Cc: x86, linux-kernel, linux-pm, linux-edac, linux-efi, linux-acpi,
	Tony Luck, Rafael J . Wysocki, Len Brown, Ard Biesheuvel,
	Punit Agrawal, Yazen Ghannam

On Tue, Nov 03, 2020 at 10:49:52AM -0600, Smita Koralahalli wrote:
> diff --git a/arch/x86/kernel/cpu/mce/apei.c b/arch/x86/kernel/cpu/mce/apei.c
> index af8d37962586..f56f0bc147e2 100644
> --- a/arch/x86/kernel/cpu/mce/apei.c
> +++ b/arch/x86/kernel/cpu/mce/apei.c
> @@ -51,6 +51,62 @@ 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_smca_report_x86_error(struct cper_ia_proc_ctx *ctx_info, u64 lapic_id)
> +{
> +	const u64 *i_mce = ((const u64 *) (ctx_info + 1));
> +	unsigned int cpu;
> +	struct mce m;
> +
> +	if (!boot_cpu_has(X86_FEATURE_SMCA))
> +		return -EINVAL;
> +
> +	/*
> +	 * The starting address of the Register Array extracted from BERT
> +	 * must match with the first expected register in the register
> +	 * layout of MCAX address space. In SMCA systems this address
> +	 * corresponds to banks's MCA_STATUS register.

So which is it "MCAX" or "SMCA"? They both denote the same thing but
let's stick to one to avoid unnecessary confusion. I'm guessing to
"SMCA" because it is more wide-spread in the kernel...

> +	 *
> +	 * The Register array size must be large enough to include all
> +	 * the SMCA registers which we want to extract.
> +	 *
> +	 * The number of registers in the Register Array is determined
> +	 * by Register Array Size/8 as defined in UEFI spec v2.8, sec
> +	 * N.2.4.2.2. The register layout is fixed and currently the raw
> +	 * data in the register array includes 6 SMCA registers which the
> +	 * kernel can extract.
> +	 */
> +
> +	if ((ctx_info->msr_addr & MSR_AMD64_SMCA_MC0_STATUS) !=
> +	    MSR_AMD64_SMCA_MC0_STATUS || ctx_info->reg_arr_size < 48)
> +		return -EINVAL;

Split that if in two consecutive if-statements.

Also, why the ANDing and not simply do:

	if (ctx_info->msr_addr == MSR_AMD64_SMCA_MC0_STATUS)

?

I'm guessing you wanna match *all* MCi_STATUS MSRs - not only MC0, yes?

If so, document that with in the comment above it.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v5] cper, apei, mce: Pass x86 CPER through the MCA handling chain
  2020-11-06  5:36 ` Punit Agrawal
@ 2020-11-06 12:09   ` Borislav Petkov
  2020-11-09  1:18     ` Punit Agrawal
  0 siblings, 1 reply; 9+ messages in thread
From: Borislav Petkov @ 2020-11-06 12:09 UTC (permalink / raw)
  To: Punit Agrawal
  Cc: Smita Koralahalli, x86, linux-kernel, linux-pm, linux-edac,
	linux-efi, linux-acpi, Tony Luck, Rafael J . Wysocki, Len Brown,
	Ard Biesheuvel, Yazen Ghannam

On Fri, Nov 06, 2020 at 02:36:46PM +0900, Punit Agrawal wrote:
> > diff --git a/drivers/firmware/efi/cper-x86.c b/drivers/firmware/efi/cper-x86.c
> > index 2531de49f56c..438ed9eff6d0 100644
> > --- a/drivers/firmware/efi/cper-x86.c
> > +++ b/drivers/firmware/efi/cper-x86.c
> > @@ -2,6 +2,7 @@
> >  // Copyright (C) 2018, Advanced Micro Devices, Inc.
> >  
> >  #include <linux/cper.h>
> > +#include <linux/acpi.h>
> 
> Did you mean to include <asm/acpi.h>?

Why?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v5] cper, apei, mce: Pass x86 CPER through the MCA handling chain
  2020-11-06 12:09   ` Borislav Petkov
@ 2020-11-09  1:18     ` Punit Agrawal
  2020-11-09 19:05       ` Smita Koralahalli Channabasappa
  0 siblings, 1 reply; 9+ messages in thread
From: Punit Agrawal @ 2020-11-09  1:18 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Smita Koralahalli, x86, linux-kernel, linux-pm, linux-edac,
	linux-efi, linux-acpi, Tony Luck, Rafael J . Wysocki, Len Brown,
	Ard Biesheuvel, Yazen Ghannam

Borislav Petkov <bp@alien8.de> writes:

> On Fri, Nov 06, 2020 at 02:36:46PM +0900, Punit Agrawal wrote:
>> > diff --git a/drivers/firmware/efi/cper-x86.c b/drivers/firmware/efi/cper-x86.c
>> > index 2531de49f56c..438ed9eff6d0 100644
>> > --- a/drivers/firmware/efi/cper-x86.c
>> > +++ b/drivers/firmware/efi/cper-x86.c
>> > @@ -2,6 +2,7 @@
>> >  // Copyright (C) 2018, Advanced Micro Devices, Inc.
>> >  
>> >  #include <linux/cper.h>
>> > +#include <linux/acpi.h>
>> 
>> Did you mean to include <asm/acpi.h>?
>
> Why?

Because arch_apei_report_x86_error() used in the patch is defined
there. The indirect include works but pulls in additional definitions
not needed by the patch.

Do you prefer the more generic include?

Thanks,
Punit

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v5] cper, apei, mce: Pass x86 CPER through the MCA handling chain
  2020-11-06 12:09 ` Borislav Petkov
@ 2020-11-09 18:36   ` Smita Koralahalli Channabasappa
  0 siblings, 0 replies; 9+ messages in thread
From: Smita Koralahalli Channabasappa @ 2020-11-09 18:36 UTC (permalink / raw)
  To: Borislav Petkov, Smita Koralahalli
  Cc: x86, linux-kernel, linux-pm, linux-edac, linux-efi, linux-acpi,
	Tony Luck, Rafael J . Wysocki, Len Brown, Ard Biesheuvel,
	Punit Agrawal, Yazen Ghannam

On 11/6/20 6:09 AM, Borislav Petkov wrote:

> On Tue, Nov 03, 2020 at 10:49:52AM -0600, Smita Koralahalli wrote:
>> diff --git a/arch/x86/kernel/cpu/mce/apei.c b/arch/x86/kernel/cpu/mce/apei.c
>> index af8d37962586..f56f0bc147e2 100644
>> --- a/arch/x86/kernel/cpu/mce/apei.c
>> +++ b/arch/x86/kernel/cpu/mce/apei.c
>> @@ -51,6 +51,62 @@ 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_smca_report_x86_error(struct cper_ia_proc_ctx *ctx_info, u64 lapic_id)
>> +{
>> +	const u64 *i_mce = ((const u64 *) (ctx_info + 1));
>> +	unsigned int cpu;
>> +	struct mce m;
>> +
>> +	if (!boot_cpu_has(X86_FEATURE_SMCA))
>> +		return -EINVAL;
>> +
>> +	/*
>> +	 * The starting address of the Register Array extracted from BERT
>> +	 * must match with the first expected register in the register
>> +	 * layout of MCAX address space. In SMCA systems this address
>> +	 * corresponds to banks's MCA_STATUS register.
> So which is it "MCAX" or "SMCA"? They both denote the same thing but
> let's stick to one to avoid unnecessary confusion. I'm guessing to
> "SMCA" because it is more wide-spread in the kernel...

Okay I will change it to SMCA.

>> +	 *
>> +	 * The Register array size must be large enough to include all
>> +	 * the SMCA registers which we want to extract.
>> +	 *
>> +	 * The number of registers in the Register Array is determined
>> +	 * by Register Array Size/8 as defined in UEFI spec v2.8, sec
>> +	 * N.2.4.2.2. The register layout is fixed and currently the raw
>> +	 * data in the register array includes 6 SMCA registers which the
>> +	 * kernel can extract.
>> +	 */
>> +
>> +	if ((ctx_info->msr_addr & MSR_AMD64_SMCA_MC0_STATUS) !=
>> +	    MSR_AMD64_SMCA_MC0_STATUS || ctx_info->reg_arr_size < 48)
>> +		return -EINVAL;
> Split that if in two consecutive if-statements.

Okay.

>
> Also, why the ANDing and not simply do:
>
> 	if (ctx_info->msr_addr == MSR_AMD64_SMCA_MC0_STATUS)
>
> ?
>
> I'm guessing you wanna match *all* MCi_STATUS MSRs - not only MC0, yes?
>
> If so, document that with in the comment above it.
>
> Thx.

ANDing with MC0 avoids bank check by turning off the bits corresponding
to the bank number.

For example: MCA_STATUS in SMCA space is 0xC0002YY1 where YY is the bank
number. The bank number is "Dont care" so ANDing with MC0_STATUS
(0xC0002001) should match on any MCi_STATUS MSR.

I will include that in comments above it.

Thanks,

>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v5] cper, apei, mce: Pass x86 CPER through the MCA handling chain
  2020-11-09  1:18     ` Punit Agrawal
@ 2020-11-09 19:05       ` Smita Koralahalli Channabasappa
  2020-11-11 20:37         ` Smita Koralahalli Channabasappa
  0 siblings, 1 reply; 9+ messages in thread
From: Smita Koralahalli Channabasappa @ 2020-11-09 19:05 UTC (permalink / raw)
  To: Punit Agrawal, Borislav Petkov
  Cc: Smita Koralahalli, x86, linux-kernel, linux-pm, linux-edac,
	linux-efi, linux-acpi, Tony Luck, Rafael J . Wysocki, Len Brown,
	Ard Biesheuvel, Yazen Ghannam

On 11/8/20 7:18 PM, Punit Agrawal wrote:

> Borislav Petkov <bp@alien8.de> writes:
>
>> On Fri, Nov 06, 2020 at 02:36:46PM +0900, Punit Agrawal wrote:
>>>> diff --git a/drivers/firmware/efi/cper-x86.c b/drivers/firmware/efi/cper-x86.c
>>>> index 2531de49f56c..438ed9eff6d0 100644
>>>> --- a/drivers/firmware/efi/cper-x86.c
>>>> +++ b/drivers/firmware/efi/cper-x86.c
>>>> @@ -2,6 +2,7 @@
>>>>   // Copyright (C) 2018, Advanced Micro Devices, Inc.
>>>>   
>>>>   #include <linux/cper.h>
>>>> +#include <linux/acpi.h>
>>> Did you mean to include <asm/acpi.h>?
>> Why?
> Because arch_apei_report_x86_error() used in the patch is defined
> there. The indirect include works but pulls in additional definitions
> not needed by the patch.
>
> Do you prefer the more generic include?

Okay.

I agree, it's generally a good practice to avoid pulling up additional
definitions. I had this when I made the declaration in generic header
file and may be I did not consider it changing initially as my build
didn't break after moving the declaration from generic header to arch
specific header file.

I will take care henceforth and make the changes as required.

Thanks,


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v5] cper, apei, mce: Pass x86 CPER through the MCA handling chain
  2020-11-09 19:05       ` Smita Koralahalli Channabasappa
@ 2020-11-11 20:37         ` Smita Koralahalli Channabasappa
  2020-11-13  1:40           ` Punit Agrawal
  0 siblings, 1 reply; 9+ messages in thread
From: Smita Koralahalli Channabasappa @ 2020-11-11 20:37 UTC (permalink / raw)
  To: Punit Agrawal, Borislav Petkov
  Cc: Smita Koralahalli, x86, linux-kernel, linux-pm, linux-edac,
	linux-efi, linux-acpi, Tony Luck, Rafael J . Wysocki, Len Brown,
	Ard Biesheuvel, Yazen Ghannam

Punit,

On 11/9/20 1:05 PM, Smita Koralahalli Channabasappa wrote:

> On 11/8/20 7:18 PM, Punit Agrawal wrote:
>> Borislav Petkov <bp@alien8.de> writes:
>>> On Fri, Nov 06, 2020 at 02:36:46PM +0900, Punit Agrawal wrote:
>>>>> diff --git a/drivers/firmware/efi/cper-x86.c b/drivers/firmware/efi/cper-x86.c
>>>>> index 2531de49f56c..438ed9eff6d0 100644
>>>>> --- a/drivers/firmware/efi/cper-x86.c
>>>>> +++ b/drivers/firmware/efi/cper-x86.c
>>>>> @@ -2,6 +2,7 @@
>>>>>    // Copyright (C) 2018, Advanced Micro Devices, Inc.
>>>>>      #include <linux/cper.h>
>>>>> +#include <linux/acpi.h>
>>>> Did you mean to include <asm/acpi.h>?
>>> Why?
>> Because arch_apei_report_x86_error() used in the patch is defined
>> there. The indirect include works but pulls in additional definitions
>> not needed by the patch.
>>
>> Do you prefer the more generic include?
> I agree, it's generally a good practice to avoid pulling up additional
> definitions. I had this when I made the declaration in generic header
> file and may be I did not consider it changing initially as my build
> didn't break after moving the declaration from generic header to arch
> specific header file.
> I will take care henceforth and make the changes as required.

The asm specific include throws out a warning when I run checkpatch.pl

WARNING: Use #include <linux/acpi.h> instead of <asm/acpi.h>
#215: FILE: drivers/firmware/efi/cper-x86.c:5:
+#include <asm/acpi.h>

Should I just keep the generic include?

Thanks,
Smita


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v5] cper, apei, mce: Pass x86 CPER through the MCA handling chain
  2020-11-11 20:37         ` Smita Koralahalli Channabasappa
@ 2020-11-13  1:40           ` Punit Agrawal
  0 siblings, 0 replies; 9+ messages in thread
From: Punit Agrawal @ 2020-11-13  1:40 UTC (permalink / raw)
  To: Smita Koralahalli Channabasappa
  Cc: Borislav Petkov, Smita Koralahalli, x86, linux-kernel, linux-pm,
	linux-edac, linux-efi, linux-acpi, Tony Luck, Rafael J . Wysocki,
	Len Brown, Ard Biesheuvel, Yazen Ghannam

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

> Punit,
>
> On 11/9/20 1:05 PM, Smita Koralahalli Channabasappa wrote:
>
>> On 11/8/20 7:18 PM, Punit Agrawal wrote:
>>> Borislav Petkov <bp@alien8.de> writes:
>>>> On Fri, Nov 06, 2020 at 02:36:46PM +0900, Punit Agrawal wrote:
>>>>>> diff --git a/drivers/firmware/efi/cper-x86.c b/drivers/firmware/efi/cper-x86.c
>>>>>> index 2531de49f56c..438ed9eff6d0 100644
>>>>>> --- a/drivers/firmware/efi/cper-x86.c
>>>>>> +++ b/drivers/firmware/efi/cper-x86.c
>>>>>> @@ -2,6 +2,7 @@
>>>>>>    // Copyright (C) 2018, Advanced Micro Devices, Inc.
>>>>>>      #include <linux/cper.h>
>>>>>> +#include <linux/acpi.h>
>>>>> Did you mean to include <asm/acpi.h>?
>>>> Why?
>>> Because arch_apei_report_x86_error() used in the patch is defined
>>> there. The indirect include works but pulls in additional definitions
>>> not needed by the patch.
>>>
>>> Do you prefer the more generic include?
>> I agree, it's generally a good practice to avoid pulling up additional
>> definitions. I had this when I made the declaration in generic header
>> file and may be I did not consider it changing initially as my build
>> didn't break after moving the declaration from generic header to arch
>> specific header file.
>> I will take care henceforth and make the changes as required.
>
> The asm specific include throws out a warning when I run checkpatch.pl
>
> WARNING: Use #include <linux/acpi.h> instead of <asm/acpi.h>
> #215: FILE: drivers/firmware/efi/cper-x86.c:5:
> +#include <asm/acpi.h>
>
> Should I just keep the generic include?

Thanks for checking.

I had a quick look at checkpatch to understand the reason for the
warning. It seems to warn when "asm" includes are used when a suitable
"linux" include exists[0].

I am not convinced that the rationale for that check applies in this
case as the function being used is indeed an architecture specific one
but also don't feel strongly enough to object.

Feel free to pick up the "Reviewed-by" tag in either case.

Thanks,
Punit

[0] https://github.com/torvalds/linux/blob/master/scripts/checkpatch.pl#L5333

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2020-11-13  1:41 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-03 16:49 [PATCH v5] cper, apei, mce: Pass x86 CPER through the MCA handling chain Smita Koralahalli
2020-11-06  5:36 ` Punit Agrawal
2020-11-06 12:09   ` Borislav Petkov
2020-11-09  1:18     ` Punit Agrawal
2020-11-09 19:05       ` Smita Koralahalli Channabasappa
2020-11-11 20:37         ` Smita Koralahalli Channabasappa
2020-11-13  1:40           ` Punit Agrawal
2020-11-06 12:09 ` Borislav Petkov
2020-11-09 18:36   ` Smita Koralahalli Channabasappa

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).