Re: [PATCH v5 05/10] ACPI: APEI: GHES: move vendor record helpers

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 29 May 2026 10:50:45 +0100
Ahmed Tiba <ahmed.tiba@arm.com> wrote:

> Shift the vendor record workqueue helpers into ghes_cper.c so both GHES
> and future DT-based providers can use the same implementation. The change
> is mechanical and keeps the notifier behavior identical.
> 
> Signed-off-by: Ahmed Tiba <ahmed.tiba@arm.com>
A few questions  / comments inline

J
> ---
>  drivers/acpi/apei/ghes.c      | 86 +++++++++----------------------------------
>  drivers/acpi/apei/ghes_cper.c | 55 +++++++++++++++++++++++++++
>  include/acpi/ghes_cper.h      |  2 +
>  3 files changed, 75 insertions(+), 68 deletions(-)
> 
> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index adab7404310e..81ac51632f21 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
...

> -
> -static void ghes_vendor_record_notifier_destroy(void *nb)
> -{
> -	ghes_unregister_vendor_record_notifier(nb);
> -}
> -
> -int devm_ghes_register_vendor_record_notifier(struct device *dev,
> -					      struct notifier_block *nb)
> -{
> -	int ret;
> -
> -	ret = ghes_register_vendor_record_notifier(nb);
> -	if (ret)
> -		return ret;
> -
> -	return devm_add_action_or_reset(dev, ghes_vendor_record_notifier_destroy, nb);
> -}
> -EXPORT_SYMBOL_GPL(devm_ghes_register_vendor_record_notifier);

>  #define CXL_CPER_PROT_ERR_FIFO_DEPTH 8
>  static DEFINE_KFIFO(cxl_cper_prot_err_fifo, struct cxl_cper_prot_err_work_data,
> @@ -514,6 +446,24 @@ int cxl_cper_prot_err_kfifo_get(struct cxl_cper_prot_err_work_data *wd)
>  }
>  EXPORT_SYMBOL_NS_GPL(cxl_cper_prot_err_kfifo_get, "CXL");
>  
> +static void ghes_vendor_record_notifier_destroy(void *nb)
> +{
> +	ghes_unregister_vendor_record_notifier(nb);
> +}
> +
> +int devm_ghes_register_vendor_record_notifier(struct device *dev,
> +					      struct notifier_block *nb)
> +{
> +	int ret;
> +
> +	ret = ghes_register_vendor_record_notifier(nb);
> +	if (ret)
> +		return ret;
> +
> +	return devm_add_action_or_reset(dev, ghes_vendor_record_notifier_destroy, nb);
> +}
> +EXPORT_SYMBOL_GPL(devm_ghes_register_vendor_record_notifier);
> +

Why did these two move inside the file?  It is a bit odd to leave the devm calls in
a different place to what they are wrapping. I guess someone argued for that in an
earlier version? (hopefully not me ;)  
If the move puts them in an ifdef block then I'd not bother - it's tiny code and
to me doing this is more confusing than just leaving them where they were.


>  /* Room for 8 entries for each of the 4 event log queues */
>  #define CXL_CPER_FIFO_DEPTH 32
>  DEFINE_KFIFO(cxl_cper_fifo, struct cxl_cper_work_data, CXL_CPER_FIFO_DEPTH);
> diff --git a/drivers/acpi/apei/ghes_cper.c b/drivers/acpi/apei/ghes_cper.c
> index 0a117f478afb..131980d36064 100644
> --- a/drivers/acpi/apei/ghes_cper.c
> +++ b/drivers/acpi/apei/ghes_cper.c
> @@ -14,12 +14,17 @@
>  
>  #include <linux/err.h>
>  #include <linux/genalloc.h>
> +#include <linux/irq_work.h>
>  #include <linux/io.h>
>  #include <linux/kernel.h>
> +#include <linux/list.h>
>  #include <linux/math64.h>
>  #include <linux/mm.h>
> +#include <linux/notifier.h>
> +#include <linux/llist.h>
>  #include <linux/ratelimit.h>
>  #include <linux/rcupdate.h>
> +#include <linux/rculist.h>

I'm not seeing anything reason for most of these new includes.
Probably in the wrong patch


>  #include <linux/sched/clock.h>
>  #include <linux/slab.h>
>  
> @@ -266,6 +271,56 @@ void ghes_clear_estatus(struct ghes *ghes,
>  		ghes_ack_error(ghes->generic_v2);
>  }
>  
> +static BLOCKING_NOTIFIER_HEAD(vendor_record_notify_list);
> +
> +int ghes_register_vendor_record_notifier(struct notifier_block *nb)
> +{
> +	return blocking_notifier_chain_register(&vendor_record_notify_list, nb);
> +}
> +EXPORT_SYMBOL_GPL(ghes_register_vendor_record_notifier);
> +
> +void ghes_unregister_vendor_record_notifier(struct notifier_block *nb)
> +{
> +	blocking_notifier_chain_unregister(&vendor_record_notify_list, nb);
> +}
> +EXPORT_SYMBOL_GPL(ghes_unregister_vendor_record_notifier);
> +
> +static void ghes_vendor_record_work_func(struct work_struct *work)
> +{
> +	struct ghes_vendor_record_entry *entry;
> +	struct acpi_hest_generic_data *gdata;
> +	u32 len;
> +
> +	entry = container_of(work, struct ghes_vendor_record_entry, work);
> +	gdata = GHES_GDATA_FROM_VENDOR_ENTRY(entry);
> +
> +	blocking_notifier_call_chain(&vendor_record_notify_list,
> +				     entry->error_severity, gdata);
> +
> +	len = GHES_VENDOR_ENTRY_LEN(acpi_hest_get_record_size(gdata));
> +	gen_pool_free(ghes_estatus_pool, (unsigned long)entry, len);
> +}
> +
> +void ghes_defer_non_standard_event(struct acpi_hest_generic_data *gdata,
> +				   int sev)
> +{
> +	struct acpi_hest_generic_data *copied_gdata;
> +	struct ghes_vendor_record_entry *entry;
> +	u32 len;
> +
> +	len = GHES_VENDOR_ENTRY_LEN(acpi_hest_get_record_size(gdata));
> +	entry = (void *)gen_pool_alloc(ghes_estatus_pool, len);
> +	if (!entry)
> +		return;
> +
> +	copied_gdata = GHES_GDATA_FROM_VENDOR_ENTRY(entry);
> +	memcpy(copied_gdata, gdata, acpi_hest_get_record_size(gdata));
> +	entry->error_severity = sev;
> +
> +	INIT_WORK(&entry->work, ghes_vendor_record_work_func);
> +	schedule_work(&entry->work);
> +}
> +
>  /*
>   * GHES error status reporting throttle, to report more kinds of
>   * errors, instead of just most frequently occurred errors.
> diff --git a/include/acpi/ghes_cper.h b/include/acpi/ghes_cper.h
> index 1b5dbeca9bb6..51725f25c516 100644
> --- a/include/acpi/ghes_cper.h
> +++ b/include/acpi/ghes_cper.h
> @@ -104,5 +104,7 @@ int __ghes_read_estatus(struct acpi_hest_generic_status *estatus,
>  int ghes_estatus_cached(struct acpi_hest_generic_status *estatus);
>  void ghes_estatus_cache_add(struct acpi_hest_generic *generic,
>  			    struct acpi_hest_generic_status *estatus);
> +void ghes_defer_non_standard_event(struct acpi_hest_generic_data *gdata,
> +				   int sev);
>  
>  #endif /* ACPI_APEI_GHES_CPER_H */
>