Re: [PATCH 4/6] cper: add helpers to handle Memory Error Section 2

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

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

> Add separate location and printing helpers for Memory Error
> Section 2:
> - cper_mem_err2_location()
> - cper_dimm_err2_location()
> - cper_print_mem2()
> 
> Similar functions/logic exists for Memory Error Section (1),
> but require a compact version of the record. To avoid relying
> and propagating a non-standard record format, the new functions
> diretly rely on struct cper_sec_mem_err2.
> 
> Signed-off-by: Pierre Gondois <pierre.gondois@arm.com>
A few comments but they are about spec confusion or trivial.

Reviewed-by: Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>

> ---
>  drivers/firmware/efi/cper.c | 108 ++++++++++++++++++++++++++++++++++++
>  include/linux/cper.h        |   3 +
>  2 files changed, 111 insertions(+)
> 
> diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
> index 06b4fdb59917..5c9b832b67a1 100644
> --- a/drivers/firmware/efi/cper.c
> +++ b/drivers/firmware/efi/cper.c
> @@ -445,6 +445,106 @@ static void cper_print_mem(const char *pfx, const struct cper_sec_mem_err *mem,
>  		printk("%s%s\n", pfx, rcd_decode_str);
>  }
>  
> +int cper_mem_err2_location(const struct cper_sec_mem_err2 *mem, char *msg)
> +{
> +	u32 len, n;
> +
> +	if (!msg)
> +		return 0;
> +
> +	n = 0;
> +	len = CPER_REC_LEN;
> +	if (mem->validation_bits & CPER_MEM2_VALID_NODE)
> +		n += scnprintf(msg + n, len - n, "node:%d ", mem->node);
> +	if (mem->validation_bits & CPER_MEM2_VALID_CARD)
> +		n += scnprintf(msg + n, len - n, "card:%d ", mem->card);
> +	if (mem->validation_bits & CPER_MEM2_VALID_MODULE)
> +		n += scnprintf(msg + n, len - n, "module:%d ", mem->module);
> +	if (mem->validation_bits & CPER_MEM2_VALID_RANK_NUMBER)
> +		n += scnprintf(msg + n, len - n, "rank:%u ", mem->rank);
> +	if (mem->validation_bits & CPER_MEM2_VALID_BANK)
> +		n += scnprintf(msg + n, len - n, "bank:%d ", mem->bank);
> +	if (mem->validation_bits & CPER_MEM2_VALID_BANK_GROUP)
> +		n += scnprintf(msg + n, len - n, "bank_group:%d ",
> +			       mem->bank >> CPER_MEM_BANK_GROUP_SHIFT);
> +	if (mem->validation_bits & CPER_MEM2_VALID_BANK_ADDRESS)
> +		n += scnprintf(msg + n, len - n, "bank_address:%d ",
> +			       mem->bank & CPER_MEM_BANK_ADDRESS_MASK);

I guess this makes sense. I couldn't figure out from the spec if there
are rules that say whether expected that VALID_BANK is not set when
VALID_BANK_GROUP and/or VALID_BANK_ADDRESS are set.
Safer to just over print than read stuff into the spec that isn't there.

> +	if (mem->validation_bits & CPER_MEM2_VALID_DEVICE)
> +		n += scnprintf(msg + n, len - n, "device:%u ", mem->device);
> +	if (mem->validation_bits & CPER_MEM2_VALID_ROW)
> +		n += scnprintf(msg + n, len - n, "row:%u ", mem->row);
> +	if (mem->validation_bits & CPER_MEM2_VALID_COLUMN)
> +		n += scnprintf(msg + n, len - n, "column:%u ", mem->column);
> +	if (mem->validation_bits & CPER_MEM2_VALID_BIT_POSITION)
> +		n += scnprintf(msg + n, len - n, "bit_position:%u ",
> +			       mem->bit_pos);
> +	if (mem->validation_bits & CPER_MEM2_VALID_REQUESTOR_ID)
> +		n += scnprintf(msg + n, len - n, "requestor_id:0x%016llx ",
> +			       mem->requestor_id);
> +	if (mem->validation_bits & CPER_MEM2_VALID_RESPONDER_ID)
> +		n += scnprintf(msg + n, len - n, "responder_id:0x%016llx ",
> +			       mem->responder_id);
> +	if (mem->validation_bits & CPER_MEM2_VALID_TARGET_ID)
> +		n += scnprintf(msg + n, len - n, "target_id:0x%016llx ",
> +			       mem->target_id);
> +	if (mem->validation_bits & CPER_MEM2_VALID_CHIP_ID)
> +		n += scnprintf(msg + n, len - n, "chip_id:%d ",
> +			       mem->chip_id);
> +
> +	return n;
> +}
> +EXPORT_SYMBOL_GPL(cper_mem_err2_location);
> +
> +int cper_dimm_err2_location(const struct cper_sec_mem_err2 *mem, char *msg)
> +{
> +	u32 len, n;
> +	const char *bank = NULL, *device = NULL;
> +
> +	if (!msg || !(mem->validation_bits & CPER_MEM2_VALID_MODULE_HANDLE))
> +		return 0;
> +
> +	len = CPER_REC_LEN;

I'd move this to...

> +	/* SMBIOS handles are 16 bits; never truncate a Section 2 handle. */
> +	if (mem->mem_dev_handle <= U16_MAX)
> +		dmi_memdev_name(mem->mem_dev_handle, &bank, &device);
> +	else
> +		pr_warn(FW_WARN "Memory Error Section 2 module handle 0x%x exceeds SMBIOS range\n",
> +			mem->mem_dev_handle);

here just because it isn't used until down here.

> +	if (bank && device)
> +		n = snprintf(msg, len, "DIMM location: %s %s ", bank, device);
> +	else
> +		n = snprintf(msg, len,
> +			     "DIMM location: not present. DMI handle: 0x%.8x ",
> +			     mem->mem_dev_handle);
> +
> +	return n;
> +}
> +EXPORT_SYMBOL_GPL(cper_dimm_err2_location);
> +
> +static void cper_print_mem2(const char *pfx, const struct cper_sec_mem_err2 *mem)
> +{
> +	char rcd_decode_str[CPER_REC_LEN];
> +
> +	if (mem->validation_bits & CPER_MEM2_VALID_ERROR_STATUS)
> +		printk("%serror_status: %s (0x%016llx)\n", pfx,
> +		       cper_mem_err_status_str(mem->error_status), mem->error_status);
> +	if (mem->validation_bits & CPER_MEM2_VALID_PA)
> +		printk("%sphysical_address: 0x%016llx\n", pfx, mem->physical_addr);
> +	if (mem->validation_bits & CPER_MEM2_VALID_PA_MASK)
> +		printk("%sphysical_address_mask: 0x%016llx\n", pfx,
> +		       mem->physical_addr_mask);
> +	if (cper_mem_err2_location(mem, rcd_decode_str))
> +		printk("%s%s\n", pfx, rcd_decode_str);
> +	if (mem->validation_bits & CPER_MEM2_VALID_ERROR_TYPE)
> +		printk("%serror_type: %u, %s\n", pfx, mem->error_type,
> +		       cper_mem_err_type_str(mem->error_type));
> +	if (mem->validation_bits & CPER_MEM2_VALID_STATUS)
> +		printk("%sstatus: 0x%02x\n", pfx, mem->status);
> +	if (cper_dimm_err2_location(mem, rcd_decode_str))
> +		printk("%s%s\n", pfx, rcd_decode_str);
> +}