[PATCH v5 08/10] ACPI: APEI: share GHES CPER helpers

[Ahmed Tiba]

On 29/05/2026 17:32, Jonathan Cameron wrote:
> On Fri, 29 May 2026 10:50:48 +0100
> Ahmed Tiba <ahmed.tiba at arm.com> wrote:
> 
>> Wire GHES up to the helper routines in ghes_cper.c and remove the local
>> copies from ghes.c. This keeps the control flow identical while letting
>> the helpers be shared with other firmware-first providers.
>>
>> Signed-off-by: Ahmed Tiba <ahmed.tiba at arm.com>
> Mostly looks fine.  The one bit that rather makes this exercise of breaking
> out generic code look dodgy is the ifdefs in the generic file.
> 

As below.

> 
>> ---
>>   drivers/acpi/apei/ghes.c      | 416 +--------------------------------------
>>   drivers/acpi/apei/ghes_cper.c | 438 +++++++++++++++++++++++++++++++++++++++++-
>>   include/acpi/ghes_cper.h      |  20 ++
>>   3 files changed, 459 insertions(+), 415 deletions(-)
>>
>> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
>> index 85be2ebf4d3e..f85b97c4db4c 100644
>> --- a/drivers/acpi/apei/ghes.c
>> +++ b/drivers/acpi/apei/ghes.c
> 
>>   
>>   static void __ghes_panic(struct ghes *ghes,
>> diff --git a/drivers/acpi/apei/ghes_cper.c b/drivers/acpi/apei/ghes_cper.c
>> index d7a666a163c3..0ff9d06eb78f 100644
>> --- a/drivers/acpi/apei/ghes_cper.c
>> +++ b/drivers/acpi/apei/ghes_cper.c
>> @@ -13,22 +13,32 @@
> 
>>   
>>   #include "apei-internal.h"
>>   
>> +ATOMIC_NOTIFIER_HEAD(ghes_report_chain);
>> +
>> +#ifndef CONFIG_ACPI_APEI
>> +void __weak arch_apei_report_mem_error(int sev, struct cper_sec_mem_err *mem_err) { }
>> +#endif
> This is non obvious enough that the reasoning for a new weak function should be mentioned in
> the patch description.  Why not stub it in include/acpi/apei.h?
> 

Agreed. I should have explained that in the changelog.

The weak arch_apei_report_mem_error() fallback was only meant to keep
the shared helper buildable when GHES_CPER_HELPERS is enabled without
ACPI_APEI, while preserving the current GHES behaviour when ACPI_APEI
is enabled.

I kept it local because this fallback is only needed by this helper 
split and I did not want to widen the APEI header API for that.

>> +
>>   static struct ghes_estatus_cache __rcu *ghes_estatus_caches[GHES_ESTATUS_CACHES_SIZE];
>>   static atomic_t ghes_estatus_cache_alloced;
> 
>> +void __ghes_print_estatus(const char *pfx,
>> +			  const struct acpi_hest_generic *generic,
>> +			  const struct acpi_hest_generic_status *estatus)
>> +{
>> +	static atomic_t seqno;
>> +	unsigned int curr_seqno;
>> +	char pfx_seq[64];
>> +
>> +	if (!pfx) {
>> +		if (ghes_severity(estatus->error_severity) <=
>> +		    GHES_SEV_CORRECTED)
>> +			pfx = KERN_WARNING;
>> +		else
>> +			pfx = KERN_ERR;
>> +	}
>> +	curr_seqno = atomic_inc_return(&seqno);
>> +	snprintf(pfx_seq, sizeof(pfx_seq), "%s{%u}" HW_ERR, pfx, curr_seqno);
>> +	printk("%sHardware error from APEI Generic Hardware Error Source: %d\n",
>> +	       pfx_seq, generic->header.source_id);
>> +	cper_estatus_print(pfx_seq, estatus);
>> +}
>> +
>> +int ghes_print_estatus(const char *pfx,
>> +		       const struct acpi_hest_generic *generic,
>> +		       const struct acpi_hest_generic_status *estatus)
>> +{
>> +	/* Not more than 2 messages every 5 seconds */
>> +	static DEFINE_RATELIMIT_STATE(ratelimit_corrected, 5 * HZ, 2);
>> +	static DEFINE_RATELIMIT_STATE(ratelimit_uncorrected, 5 * HZ, 2);
>> +	struct ratelimit_state *ratelimit;
>> +
>> +	if (ghes_severity(estatus->error_severity) <= GHES_SEV_CORRECTED)
>> +		ratelimit = &ratelimit_corrected;
>> +	else
>> +		ratelimit = &ratelimit_uncorrected;
>> +	if (__ratelimit(ratelimit)) {
>> +		__ghes_print_estatus(pfx, generic, estatus);
>> +		return 1;
>> +	}
>> +	return 0;
>> +}
>> +
>> +#ifdef CONFIG_ACPI_APEI
> 
> So after the effort to break the the generic stuff we end up with non generic
> bits in the broken out file?  Is there no way to avoid this?
> 
The intent here was not to create a new generic estatus core
but to keep the existing GHES control flow and lift the CPER helper flow
for reuse by the DT provider.

Splitting the remaining ACPI GHES specific read/clear path back out into
ghes.c would break that flow across files again. The CONFIG_ACPI_APEI
guard keeps that ACPI specific piece local while the DT side reuses the
same CPER parsing and status-dispatch path.

Best regards,
Ahmed