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.ander...@hpe.com>
> Signed-off-by: Alex Kluver <alex.klu...@hpe.com>
> ---
> 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
