Re: [PATCH v5] cper, apei, mce: Pass x86 CPER through the MCA handling chain

[Punit Agrawal]

Smita Koralahalli Channabasappa <skoralah@xxxxxxx> 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@xxxxxxxxx> 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