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@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>

> > +#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

[...]