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

[Punit Agrawal]

Hi Smita,

Smita Koralahalli Channabasappa <skoralah@xxxxxxx> writes:

> On 8/31/20 12:05 AM, Punit Agrawal wrote:
>
>> Hi Smita,
>>
>> A couple of comments below -
>>
>> Smita Koralahalli <Smita.KoralahalliChannabasappa@xxxxxxx> 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
>
> [...]