Re: [PATCH 5/6] ACPI: APEI: add separate Memory Error Section 2 handlers

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

On Mon, 19 Oct 2026 16:56:12 +0200
Pierre Gondois <pierre.gondois@arm.com> wrote:

> Add dedicated GHES and EDAC callbacks for Memory Error Section 2
> records. Use a separate GHES notifier chain so existing consumers
> continue to receive legacy memory error sections.
> 
> Signed-off-by: Pierre Gondois <pierre.gondois@arm.com>
A few trivial things inline. 

Otherwise I'm fine with this though as you noted before the duplicating
is a bit annoying.

> ---
>  drivers/acpi/apei/ghes.c |  49 ++++++++++++-
>  drivers/edac/ghes_edac.c | 150 +++++++++++++++++++++++++++++++++++++++
>  include/acpi/ghes.h      |   2 +
>  3 files changed, 200 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index fe10ab0e02f6..c7fb15543b56 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c


> diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c
> index d80c88818691..b18460ba8f40 100644
> --- a/drivers/edac/ghes_edac.c
> +++ b/drivers/edac/ghes_edac.c

> +
> +static int ghes_edac_report_mem_error2(struct notifier_block *nb,
> +				       unsigned long val, void *data)
> +{
> +	struct cper_sec_mem_err2 *mem_err = data;
> +	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);

Perhaps use a guard so you can simplify error paths a little.

> +
> +	pvt = ghes_pvt;
> +	if (!pvt)
> +		goto unlock;
> +
> +	mci = pvt->mci;
> +	e = &mci->error_desc;
> +
> +	/* Cleans the error report buffer */
> +	memset(e, 0, sizeof(*e));
> +	e->error_count = 1;
> +	e->grain = 1;
> +	e->msg = pvt->msg;
> +	e->other_detail = pvt->other_detail;
> +	e->top_layer = -1;
> +	e->mid_layer = -1;
> +	e->low_layer = -1;
> +	*pvt->other_detail = '\0';
> +	*pvt->msg = '\0';
> +
> +	switch (sev) {
> +	case GHES_SEV_CORRECTED:
> +		e->type = HW_EVENT_ERR_CORRECTED;
> +		break;
> +	case GHES_SEV_RECOVERABLE:
> +		e->type = HW_EVENT_ERR_UNCORRECTED;
> +		break;
> +	case GHES_SEV_PANIC:
> +		e->type = HW_EVENT_ERR_FATAL;
> +		break;
> +	default:
> +	case GHES_SEV_NO:
> +		e->type = HW_EVENT_ERR_INFO;
> +	}
> +

Almost everything down to here is not memerr2 specific maybe factor
out an initialization helper for the edac structure?

> +	edac_dbg(1, "error validation_bits: 0x%08llx\n",
> +		 (long long)mem_err->validation_bits);
> +
> +	/* Error type, mapped on e->msg */
> +	if (mem_err->validation_bits & CPER_MEM2_VALID_ERROR_TYPE) {
> +		u8 etype = mem_err->error_type;
> +
> +		p = pvt->msg;
> +		p += snprintf(p, sizeof(pvt->msg), "%s", cper_mem_err_type_str(etype));
> +	} else {
> +		strscpy(pvt->msg, "unknown error");
> +	}
> +
> +	/* Error address */
> +	if (mem_err->validation_bits & CPER_MEM2_VALID_PA) {
> +		e->page_frame_number = PHYS_PFN(mem_err->physical_addr);
> +		e->offset_in_page = offset_in_page(mem_err->physical_addr);
> +	}
> +
> +	/* Error grain */
> +	if (mem_err->validation_bits & CPER_MEM2_VALID_PA_MASK)
> +		e->grain = ~mem_err->physical_addr_mask + 1;
> +
> +	/* Memory error location, mapped on e->location */
> +	p = e->location;
> +	p += cper_mem_err2_location(mem_err, p);
> +
> +	if (mem_err->validation_bits & CPER_MEM2_VALID_MODULE_HANDLE) {
> +		struct dimm_info *dimm = NULL;
> +
> +		p += cper_dimm_err2_location(mem_err, p);
> +		if (mem_err->mem_dev_handle <= U16_MAX)
> +			dimm = find_dimm_by_handle(mci, mem_err->mem_dev_handle);
> +		else
> +			pr_warn(FW_WARN "Memory Error Section 2 module handle 0x%x exceeds SMBIOS range\n",
> +				mem_err->mem_dev_handle);
> +		if (dimm) {
> +			e->top_layer = dimm->idx;
> +			strscpy(e->label, dimm->label);
> +		}
> +	}
> +	if (p > e->location)
> +		*(p - 1) = '\0';
> +
> +	if (!*e->label)
> +		strscpy(e->label, "unknown memory");
> +
> +	/* All other fields are mapped on e->other_detail */
> +	p = pvt->other_detail;
> +	p += print_mem_error2_other_detail(mem_err, p, e->location, OTHER_DETAIL_LEN);
> +	if (p > pvt->other_detail)
> +		*(p - 1) = '\0';
> +
> +	edac_raw_mc_handle_error(e);
> +
> +unlock:
> +	spin_unlock_irqrestore(&ghes_lock, flags);
> +
> +	return NOTIFY_OK;
> +}