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

From: Smita Koralahalli Channabasappa <skoralah@amd.com>
To: Punit Agrawal <punit1.agrawal@toshiba.co.jp>
Cc: x86@kernel.org, linux-kernel@vger.kernel.org,
	linux-pm@vger.kernel.org, linux-edac@vger.kernel.org,
	linux-efi@vger.kernel.org, linux-acpi@vger.kernel.org,
	devel@acpica.org, Borislav Petkov <bp@alien8.de>,
	Tony Luck <tony.luck@intel.com>,
	"Rafael J . Wysocki" <rjw@rjwysocki.net>,
	Len Brown <len.brown@intel.com>, Ard Biesheuvel <ardb@kernel.org>,
	Yazen Ghannam <yazen.ghannam@amd.com>
Subject: Re: [PATCH v2 1/2] cper, apei, mce: Pass x86 CPER through the MCA handling chain
Date: Wed, 2 Sep 2020 14:29:28 -0500	[thread overview]
Message-ID: <102d0c75-d642-8f8b-68c7-792499c2a62a@amd.com> (raw)
In-Reply-To: <878sdvv20h.fsf@kokedama.swc.toshiba.co.jp>

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

[...]