Re: [PATCH v2 03/11] ACPI: APEI: GHES: move CPER read helpers

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

On Fri, 20 Feb 2026 13:42:21 +0000
Ahmed Tiba <ahmed.tiba@arm.com> wrote:

> Relocate the CPER buffer mapping, peek, and clear helpers from ghes.c into
> ghes_cper.c so they can be shared with other firmware-first providers.
> This commit only shuffles code; behavior stays the same.
> 
> Signed-off-by: Ahmed Tiba <ahmed.tiba@arm.com>
Hi Ahmed,

Most of the comments in here are about changing the patch break up.
Basic suggest approach is move stuff as it is needed, not in advance of
that need.  So when you move the function to the c file, only then add what
it needs to the includes / header.

Jonathan

> diff --git a/drivers/acpi/apei/ghes_cper.c b/drivers/acpi/apei/ghes_cper.c
> index 63047322a3d9..7e0015e960c1 100644
> --- a/drivers/acpi/apei/ghes_cper.c
> +++ b/drivers/acpi/apei/ghes_cper.c
> @@ -1,7 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0
>  /*
>   *
> - * APEI GHES CPER helper translation unit - staging file for helper moves
> + * APEI GHES CPER helper translation unit - code mechanically moved from ghes.c

In the long run, no interest in where it came from. People can
look at the git history for that. 

>   *
>   * Copyright (C) 2026 ARM Ltd.
>   * Author: Ahmed Tiba <ahmed.tiba@arm.com>
> @@ -17,10 +17,176 @@
>  #include <linux/slab.h>
>  
>  #include <acpi/apei.h>
> +#include <acpi/ghes_cper.h>
>  
>  #include <asm/fixmap.h>
>  #include <asm/tlbflush.h>
>  
>  #include "apei-internal.h"
>  
> -/* Helper bodies will be moved here in follow-up commits. */

If you just do the file creation with this first move, then we don't get churn of
comments like this one.

> +/* Read the CPER block, returning its address, and header in estatus. */
> +int __ghes_peek_estatus(struct ghes *ghes,
> +			       struct acpi_hest_generic_status *estatus,
> +			       u64 *buf_paddr, enum fixed_addresses fixmap_idx)
> +{
> +	struct acpi_hest_generic *g = ghes->generic;
> +	int rc;
> +
> +	rc = apei_read(buf_paddr, &g->error_status_address);
> +	if (rc) {
> +		*buf_paddr = 0;
> +		pr_warn_ratelimited(FW_WARN GHES_PFX
> +"Failed to read error status block address for hardware error source: %d.\n",

Unusual indenting.  I'd just fix that whilst you are here. Don't worry about long line.

> +				   g->header.source_id);
> +		return -EIO;

> diff --git a/include/acpi/ghes_cper.h b/include/acpi/ghes_cper.h
> index 2597fbadc4f3..2e3919f0c3e7 100644
> --- a/include/acpi/ghes_cper.h
> +++ b/include/acpi/ghes_cper.h
> @@ -74,21 +74,21 @@ struct ghes_vendor_record_entry {
>  	char vendor_record[];
>  };
>  
> -static struct ghes *ghes_new(struct acpi_hest_generic *generic);
Huh. Static forward declarations in a header?  That never made sense. Fix it in the
earlier patch and remove the statics from the declarations.

Actually no, just bring them into the header only when you need to. So as part
of the patch that moves the caller or the function.



> -static void ghes_fini(struct ghes *ghes);
> +struct ghes *ghes_new(struct acpi_hest_generic *generic);
> +void ghes_fini(struct ghes *ghes);
>  
> -static int ghes_read_estatus(struct ghes *ghes,
> +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,
> +void ghes_clear_estatus(struct ghes *ghes,
>  			struct acpi_hest_generic_status *estatus,
>  			u64 buf_paddr, enum fixed_addresses fixmap_idx);
> -static int __ghes_peek_estatus(struct ghes *ghes,
> +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,
> +int __ghes_check_estatus(struct ghes *ghes,
>  			 struct acpi_hest_generic_status *estatus);
> -static int __ghes_read_estatus(struct acpi_hest_generic_status *estatus,
> +int __ghes_read_estatus(struct acpi_hest_generic_status *estatus,
>  			u64 buf_paddr, enum fixed_addresses fixmap_idx,
>  			size_t buf_len);
>  
>