RE: [PATCH] edac,ghes,cper: Add Row Extension to Memory Error Record

[Kluver, Alex]

Yes, I am working on a resubmit. The updated patch will be resubmitted in a series of patches that include other updates to the cper memory record.

Thanks,
--Alex Kluver



-----Original Message-----
From: Borislav Petkov <bp@xxxxxxxxx> 
Sent: Saturday, August 15, 2020 4:34 AM
To: Kluver, Alex <alex.kluver@xxxxxxx>
Cc: linux-edac@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; ardb@xxxxxxxxxx; mchehab@xxxxxxxxxx; Anderson, Russ <russ.anderson@xxxxxxx>; Sivanich, Dimitri <dimitri.sivanich@xxxxxxx>
Subject: Re: [PATCH] edac,ghes,cper: Add Row Extension to Memory Error Record

On Mon, Jul 27, 2020 at 01:14:45PM -0500, Alex Kluver wrote:
> Memory errors could be printed with incorrect row values since the 
> DIMM size has outgrown the 16 bit row field in the CPER structure. 
> UEFI Specification Version 2.8 has increased the size of row by 
> allowing it to use the first 2 bits from a previously reserved space within the structure.
> 
> When needed, add the extension bits to the row value printed.
> 
> Based on UEFI 2.8 Table 299. Memory Error Record
> 
> Tested-by: Russ Anderson <russ.anderson@xxxxxxx>
> Signed-off-by: Alex Kluver <alex.kluver@xxxxxxx>
> ---
>  drivers/edac/ghes_edac.c    | 10 ++++++++--
>  drivers/firmware/efi/cper.c | 11 +++++++++--
>  include/linux/cper.h        |  9 +++++++--
>  3 files changed, 24 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c index 
> cb3dab56a875..cfa3156300f5 100644
> --- a/drivers/edac/ghes_edac.c
> +++ b/drivers/edac/ghes_edac.c
> @@ -337,8 +337,14 @@ void ghes_edac_report_mem_error(int sev, struct cper_sec_mem_err *mem_err)
>  		p += sprintf(p, "rank:%d ", mem_err->rank);
>  	if (mem_err->validation_bits & CPER_MEM_VALID_BANK)
>  		p += sprintf(p, "bank:%d ", mem_err->bank);
> -	if (mem_err->validation_bits & CPER_MEM_VALID_ROW)
> -		p += sprintf(p, "row:%d ", mem_err->row);
> +	if (mem_err->validation_bits & (CPER_MEM_VALID_ROW | CPER_MEM_VALID_ROW_EXT)) {
> +		u32 row_extended = 0;
> +		if (mem_err->validation_bits & CPER_MEM_VALID_ROW_EXT)
> +			row_extended = (mem_err->extended & CPER_MEM_EXT_ROW_MASK)
> +				<<CPER_MEM_EXT_ROW_SHIFT;
> +		row_extended |= mem_err->row;
> +		p += sprintf(p, "row:%d ", row_extended);
> +	}
>  	if (mem_err->validation_bits & CPER_MEM_VALID_COLUMN)
>  		p += sprintf(p, "col:%d ", mem_err->column);
>  	if (mem_err->validation_bits & CPER_MEM_VALID_BIT_POSITION) diff 
> --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c 
> index f564e15fbc7e..5faaf6ecd659 100644
> --- a/drivers/firmware/efi/cper.c
> +++ b/drivers/firmware/efi/cper.c
> @@ -234,8 +234,14 @@ static int cper_mem_err_location(struct cper_mem_err_compact *mem, char *msg)
>  		n += scnprintf(msg + n, len - n, "bank: %d ", mem->bank);
>  	if (mem->validation_bits & CPER_MEM_VALID_DEVICE)
>  		n += scnprintf(msg + n, len - n, "device: %d ", mem->device);
> -	if (mem->validation_bits & CPER_MEM_VALID_ROW)
> -		n += scnprintf(msg + n, len - n, "row: %d ", mem->row);
> +	if (mem->validation_bits & (CPER_MEM_VALID_ROW | CPER_MEM_VALID_ROW_EXT)) {
> +		u32 row_extended = 0;
> +		if (mem->validation_bits & CPER_MEM_VALID_ROW_EXT)
> +			row_extended = (mem->extended & CPER_MEM_EXT_ROW_MASK)
> +				<<CPER_MEM_EXT_ROW_SHIFT;

This is not very readable.

> +		row_extended |= mem->row;
> +		n += scnprintf(msg + n, len - n, "row: %d ", row_extended);
> +	}

Both those hunks contain duplicated code which kinda wants to be an inline function in cper.h which returns row_extended and gets called by both sites. And then the call site can look very simple:

        if (mem_err->validation_bits & CPER_MEM_VALID_ROW)
                row = mem_err->row;

        /* add row extension */
        row |= cper_get_mem_extension();

        p += sprintf(p, "row:%d ", row);

with

static inline u32 cper_get_mem_extension(void) {
	if (!(mem_err->validation_bits & CPER_MEM_VALID_ROW_EXT))
		return 0;

	return (mem_err->extended & CPER_MEM_EXT_ROW_MASK) << CPER_MEM_EXT_ROW_SHIFT; }

Something along those lines...

Thx.

--
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette