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

[Ahmed Tiba <ahmed.tiba@arm.com>]

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.

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

>> + * 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
>> +
>> +#include <linux/workqueue.h>
>> +
>> +#include <acpi/ghes.h>
>> +
>> +#define GHES_PFX	"GHES: "
>> +
>> +#define GHES_ESTATUS_MAX_SIZE		65536
>> +#define GHES_ESOURCE_PREALLOC_MAX_SIZE	65536
>> +
>> +#define GHES_ESTATUS_POOL_MIN_ALLOC_ORDER 3
>> +
>> +/* This is just an estimation for memory pool allocation */
>> +#define GHES_ESTATUS_CACHE_AVG_SIZE	512
>> +
>> +#define GHES_ESTATUS_CACHES_SIZE	4
>> +
>> +#define GHES_ESTATUS_IN_CACHE_MAX_NSEC	10000000000ULL
>> +/* Prevent too many caches are allocated because of RCU */
>> +#define GHES_ESTATUS_CACHE_ALLOCED_MAX	(GHES_ESTATUS_CACHES_SIZE * 3 / 2)
>> +
>> +#define GHES_ESTATUS_CACHE_LEN(estatus_len)			\
>> +	(sizeof(struct ghes_estatus_cache) + (estatus_len))
>> +#define GHES_ESTATUS_FROM_CACHE(estatus_cache)			\
>> +	((struct acpi_hest_generic_status *)				\
>> +	 ((struct ghes_estatus_cache *)(estatus_cache) + 1))
>> +
>> +#define GHES_ESTATUS_NODE_LEN(estatus_len)			\
>> +	(sizeof(struct ghes_estatus_node) + (estatus_len))
>> +#define GHES_ESTATUS_FROM_NODE(estatus_node)			\
>> +	((struct acpi_hest_generic_status *)				\
>> +	 ((struct ghes_estatus_node *)(estatus_node) + 1))
>> +
>> +#define GHES_VENDOR_ENTRY_LEN(gdata_len)                               \
>> +	(sizeof(struct ghes_vendor_record_entry) + (gdata_len))
>> +#define GHES_GDATA_FROM_VENDOR_ENTRY(vendor_entry)                     \
>> +	((struct acpi_hest_generic_data *)                              \
>> +	((struct ghes_vendor_record_entry *)(vendor_entry) + 1))
>> +
>> +static inline bool is_hest_type_generic_v2(struct ghes *ghes)
>> +{
>> +	return ghes->generic->header.type == ACPI_HEST_TYPE_GENERIC_ERROR_V2;
>> +}
>> +
>> +/*
>> + * A platform may describe one error source for the handling of synchronous
>> + * errors (e.g. MCE or SEA), or for handling asynchronous errors (e.g. SCI
>> + * or External Interrupt). On x86, the HEST notifications are always
>> + * asynchronous, so only SEA on ARM is delivered as a synchronous
>> + * notification.
>> + */
>> +static inline bool is_hest_sync_notify(struct ghes *ghes)
>> +{
>> +	u8 notify_type = ghes->generic->notify.type;
>> +
>> +	return notify_type == ACPI_HEST_NOTIFY_SEA;
>> +}
>> +
>> +struct ghes_vendor_record_entry {
>> +	struct work_struct work;
>> +	int error_severity;
>> +	char vendor_record[];
>> +};
>> +
>> +static struct ghes *ghes_new(struct acpi_hest_generic *generic);
>> +static void ghes_fini(struct ghes *ghes);
>> +
>> +static int ghes_read_estatus(struct ghes *ghes,
>> +		      struct acpi_hest_generic_status *estatus,
>> +		      u64 *buf_paddr, enum fixed_addresses fixmap_idx);
>> +static void ghes_clear_estatus(struct ghes *ghes,
>> +			struct acpi_hest_generic_status *estatus,
>> +			u64 buf_paddr, enum fixed_addresses fixmap_idx);
> 
> I'm not sure some of this makes sense in a file named ghes_cper.h
> Maybe we just need a different intro comment though.
> 
>> +static int __ghes_peek_estatus(struct ghes *ghes,
>> +			struct acpi_hest_generic_status *estatus,
>> +			u64 *buf_paddr, enum fixed_addresses fixmap_idx);
>> +static int __ghes_check_estatus(struct ghes *ghes,
>> +			 struct acpi_hest_generic_status *estatus);
>> +static int __ghes_read_estatus(struct acpi_hest_generic_status *estatus,
>> +			u64 buf_paddr, enum fixed_addresses fixmap_idx,
>> +			size_t buf_len);
>> +
>> +#endif /* ACPI_APEI_GHES_CPER_H */
>>
>