Re: [PATCH v2 4/6] EDAC/ghes: Extract error report initialization

[Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>]

On Mon, 19 Oct 2026 22:38:43 +0200
Pierre Gondois <pierre.gondois@arm.com> wrote:

> Extract a ghes_edac_init_error() function to prepare for
> Memory Error Section 2 support (and re-usage).
> 
> Also use a guard in the caller ghes_edac_report_mem_error()
> function for code simplification.
> 
> Signed-off-by: Pierre Gondois <pierre.gondois@arm.com>
> ---
>  drivers/edac/ghes_edac.c | 66 +++++++++++++++++++++-------------------
>  1 file changed, 35 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c
> index d80c88818691..b90150c67f4b 100644
> --- a/drivers/edac/ghes_edac.c
> +++ b/drivers/edac/ghes_edac.c
> @@ -267,37 +267,14 @@ static int print_mem_error_other_detail(const struct cper_sec_mem_err *mem, char
>  	return n;
>  }
>  
> -static int ghes_edac_report_mem_error(struct notifier_block *nb,
> -				      unsigned long val, void *data)
> +/* Caller must hold ghes_lock. */
> +static struct edac_raw_error_desc *
> +ghes_edac_init_error(struct ghes_pvt *pvt, unsigned long sev)

Slightly unusual to have an init function that doesn't directly take the thing
being initialized. I'd pass it in. The clearly of pvt->msg and pvt->other_detail
is also a bit of a surprise for a function that claims to be initializing something
else so I'd do those outside of the helper.

Or maybe rename helper to something like
ghes_edac_fill_error_from_pvt() 
Not sure that really conveys the modification of pvt data hidden in here though.

Perhaps this refactor was a bad suggestion from me and better to just duplicate
the code.  What do ACPI maintainers think?



>  {
> -	struct cper_sec_mem_err *mem_err = (struct cper_sec_mem_err *)data;
> -	struct cper_mem_err_compact cmem;
> -	struct edac_raw_error_desc *e;
> -	struct mem_ctl_info *mci;
> -	unsigned long sev = val;
> -	struct ghes_pvt *pvt;
> -	unsigned long flags;
> -	char *p;
> -
> -	/*
> -	 * We can do the locking below because GHES defers error processing
> -	 * from NMI to IRQ context. Whenever that changes, we'd at least
> -	 * know.
> -	 */
> -	if (WARN_ON_ONCE(in_nmi()))
> -		return NOTIFY_OK;
> -
> -	spin_lock_irqsave(&ghes_lock, flags);
> -
> -	pvt = ghes_pvt;
> -	if (!pvt)
> -		goto unlock;
> -
> -	mci = pvt->mci;
> -	e = &mci->error_desc;
> +	struct edac_raw_error_desc *e = &pvt->mci->error_desc;
>  
>  	/* Cleans the error report buffer */
> -	memset(e, 0, sizeof (*e));
> +	memset(e, 0, sizeof(*e));
>  	e->error_count = 1;
>  	e->grain = 1;
>  	e->msg = pvt->msg;
> @@ -323,6 +300,36 @@ static int ghes_edac_report_mem_error(struct notifier_block *nb,
>  		e->type = HW_EVENT_ERR_INFO;
>  	}
>  
> +	return e;
> +}
> +
> +static int ghes_edac_report_mem_error(struct notifier_block *nb,
> +				      unsigned long val, void *data)
> +{
> +	struct cper_sec_mem_err *mem_err = (struct cper_sec_mem_err *)data;
> +	struct cper_mem_err_compact cmem;
> +	struct edac_raw_error_desc *e;
> +	struct mem_ctl_info *mci;
> +	struct ghes_pvt *pvt;
> +	char *p;
> +
> +	/*
> +	 * We can do the locking below because GHES defers error processing
> +	 * from NMI to IRQ context. Whenever that changes, we'd at least
> +	 * know.
> +	 */
> +	if (WARN_ON_ONCE(in_nmi()))
> +		return NOTIFY_OK;
> +
> +	guard(spinlock_irqsave)(&ghes_lock);
> +
> +	pvt = ghes_pvt;
> +	if (!pvt)
> +		return NOTIFY_OK;
> +
> +	mci = pvt->mci;
> +	e = ghes_edac_init_error(pvt, val);
> +
>  	edac_dbg(1, "error validation_bits: 0x%08llx\n",
>  		 (long long)mem_err->validation_bits);
>  
> @@ -375,9 +382,6 @@ static int ghes_edac_report_mem_error(struct notifier_block *nb,
>  
>  	edac_raw_mc_handle_error(e);
>  
> -unlock:
> -	spin_unlock_irqrestore(&ghes_lock, flags);
> -
>  	return NOTIFY_OK;
>  }
>