Re: [PATCH v7 07/10] ACPI: APEI: introduce GHES helper

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The decoupled `GHES_CPER_HELPERS` config relies on symbols defined in `ghes.o`, causing linker errors if enabled independently of `ACPI_APEI_GHES`.
- [Medium] `GHES_CPER_HELPERS` lacks dependencies on `ACPI_APEI` and architectural support, causing implicit declarations and linker errors if enabled independently.
--

--- Patch [7]: [PATCH v7 07/10] ACPI: APEI: introduce GHES helper ---
commit 0f74f9098911a0c1e3d2f8f68c8908787e6ae1b7
Author: Ahmed Tiba <ahmed.tiba@arm.com>

ACPI: APEI: introduce GHES helper
    
This commit introduces a new GHES_CPER_HELPERS Kconfig entry so the shared
helper code can be built even when ACPI_APEI_GHES is disabled. It updates
the build glue and headers to depend on the new symbol.

> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> index f165d14cf61a3..13ef0e99f840b 100644
> --- a/drivers/acpi/Kconfig
> +++ b/drivers/acpi/Kconfig
> @@ -6,6 +6,10 @@
>  config ARCH_SUPPORTS_ACPI
>  	bool
>  
> +config GHES_CPER_HELPERS
> +	bool
> +	select UEFI_CPER
> +

[Severity: Medium]
Since the commit message indicates this config is meant to be built even
when ACPI_APEI_GHES is disabled, should this have a depends on ACPI_APEI
rule?

If a future config selects GHES_CPER_HELPERS without ACPI_APEI, it looks
like ghes_cper.c will fail to compile. For example, ghes_map() calls
arch_apei_get_mem_attribute(), which lacks a fallback stub in
arch/x86/include/asm/acpi.h when CONFIG_ACPI_APEI is disabled.

Similarly, ghes_ack_error() calls apei_read(), which is only compiled
when CONFIG_ACPI_APEI is enabled.

>  menuconfig ACPI
>  	bool "ACPI (Advanced Configuration and Power Interface) Support"
>  	depends on ARCH_SUPPORTS_ACPI

[ ... ]

> diff --git a/include/acpi/ghes.h b/include/acpi/ghes.h
> index 8d7e5caef3f18..2ffab36b61541 100644
> --- a/include/acpi/ghes.h
> +++ b/include/acpi/ghes.h
> @@ -83,15 +83,17 @@ int devm_ghes_register_vendor_record_notifier(struct device *dev,
>  					      struct notifier_block *nb);
>  
>  struct list_head *ghes_get_devices(void);
> -
> -void ghes_estatus_pool_region_free(unsigned long addr, u32 size);
>  #else
>  static inline struct list_head *ghes_get_devices(void) { return NULL; }
> -
> -static inline void ghes_estatus_pool_region_free(unsigned long addr, u32 size) { return; }
>  #endif
>  
> +#ifdef CONFIG_GHES_CPER_HELPERS
>  int ghes_estatus_pool_init(unsigned int num_ghes);
> +void ghes_estatus_pool_region_free(unsigned long addr, u32 size);
> +#else
> +static inline int ghes_estatus_pool_init(unsigned int num_ghes) { return -ENODEV; }
> +static inline void ghes_estatus_pool_region_free(unsigned long addr, u32 size) { }
> +#endif

[Severity: Medium]
If GHES_CPER_HELPERS is enabled but ACPI_APEI_GHES is disabled, will this
cause linker errors?

It appears ghes_defer_non_standard_event() in drivers/acpi/apei/ghes_cper.c
allocates from ghes_estatus_pool:

drivers/acpi/apei/ghes_cper.c:ghes_defer_non_standard_event() {
    ...
    entry = (void *)gen_pool_alloc(ghes_estatus_pool, len);
    ...
}

However, ghes_estatus_pool is defined in drivers/acpi/apei/ghes.c, which is
entirely omitted when ACPI_APEI_GHES is disabled.

Additionally, exposing the prototype here for ghes_estatus_pool_init()
disables the inline stub, which could cause link errors for callers like
hest.c if ghes.o is missing.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-topics-ahmtib01-ras_ffh_arm_internal_review-v7-0-8b3a85216cef@arm.com?part=7