Re: [PATCH v2 01/11] ACPI: APEI: GHES: share macros via a private header

[Jonathan Cameron <jonathan.cameron@huawei.com>]

On Wed, 11 Mar 2026 11:39:38 +0000
Ahmed Tiba <ahmed.tiba@arm.com> wrote:

> On 24/02/2026 15:22, Jonathan Cameron wrote:
> > On Fri, 20 Feb 2026 13:42:19 +0000
> > Ahmed Tiba <ahmed.tiba@arm.com> wrote:
> >   
> >> Carve the CPER helper macros out of ghes.c and place them in a private
> >> header so they can be shared with upcoming helper files. This is a
> >> mechanical include change with no functional differences.
> >>
> >> Signed-off-by: Ahmed Tiba <ahmed.tiba@arm.com>  
> > +CC Mauro as he's been doing a lot of work on error injection recently so
> > can probably review the use of the various structures much more easily
> > than I can!
> > 
> > My main comment is on the naming of the new header.
> > 
> > Jonathan  
> 
> The content is intentionally GHES‑specific CPER handling,
> not generic UEFI CPER. It's the GHES view of CPER parsing/handling
> and is used by the shared GHES/DT path, so keeping it in ghes_cper.h 
> documents that boundary better than moving it to ghes.h (which also 
> contains non‑CPER GHES logic). The helpers moved there are the ones 
> needed by the shared CPER handling path.

Ok. So the intended meaning here is GHES and CPER, not stuff specific
to the CPER aspects of GHES.  Maybe, though I'm not sure why you
don't just name ghes.h in that case as GHES always incorporates CPER.
I guess because that file already exists and covers some ACPI specific parts
and HEST bits that aren't of use to you.

Ah well, one for the ACPI maintainers to perhaps suggest what makes
most sense to them.
> 
> >> ---
> >>   drivers/acpi/apei/ghes.c | 60 +-----------------------------
> >>   include/acpi/ghes_cper.h | 95 ++++++++++++++++++++++++++++++++++++++++++++++++
> >>   2 files changed, 96 insertions(+), 59 deletions(-)
> >>
> >> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> >> index f96aede5d9a3..07b70bcb8342 100644
> >> --- a/drivers/acpi/apei/ghes.c
> >> +++ b/drivers/acpi/apei/ghes.c  
> >   
> >>   
> >>   static struct ghes_estatus_cache __rcu *ghes_estatus_caches[GHES_ESTATUS_CACHES_SIZE];
> >> diff --git a/include/acpi/ghes_cper.h b/include/acpi/ghes_cper.h
> >> new file mode 100644
> >> index 000000000000..2597fbadc4f3
> >> --- /dev/null
> >> +++ b/include/acpi/ghes_cper.h
> >> @@ -0,0 +1,95 @@
> >> +/* SPDX-License-Identifier: GPL-2.0-only */
> >> +/*
> >> + * APEI Generic Hardware Error Source: CPER Helper  
> > 
> > There is other stuff in her usch as the GHES acks etc
> > in ghes_clear_estatus(). So I think this intro text
> > needs a bit more thought.  The boundary is already rather
> > blurred though as for example cper_estatus_len() is only
> > tangentially connected to cper.
> >   
> >> + *
> >> + * Copyright (C) 2026 ARM Ltd.  
> > 
> > Doesn't make sense to ad this copyright in this patch as so far
> > it's cut and paste of code from a file that you didn't write (at least
> > not in 2026!)
> > 
> > Might make sense after a few patches, in which case add the copyright
> > when it does.  
> 
> The file is new and maintained by Arm as part of this refactor,
> so I kept the header consistent with other newly introduced files.

It's code moved from elsewhere, so you need to at least also list
the copyright of the original file alongside the new Arm one.
Just moving it and dropping that copyright is inconsistent with
the license.


> 
> >> + * Author: Ahmed Tiba <ahmed.tiba@arm.com>
> >> + * Based on ACPI APEI GHES driver.
> >> + *
> >> + */
> >> +
> >> +#ifndef ACPI_APEI_GHES_CPER_H
> >> +#define ACPI_APEI_GHES_CPER_H