@Markus and all, please review it, thanks Subject: [PATCH][V3] EDAC/i10nm: Fix an inappropriate shift exponen A surprising value was determined after a read failure from a DIMM. Software analyses pointed a data processing issue out. `UBSAN: shift-out-of-bounds in drivers/edac/skx_common.c:369:16 shift exponent -66 is negative` when get rows, cols and ranks, A special value combination could not be handled so far. Check if an invalid value was detected by a call of the function “skx_get_dimm_attr”. * Print a corresponding error message in this case. * Return zero then directly from the function “skx_get_dimm_info”. Fixes: 4ec656bdf43a13 (EDAC, skx_edac: Add EDAC driver for Skylake) Signed-off-by: Koba Ko <koba.ko@xxxxxxxxxxxxx> --- V3: simplify and polish the summary and add `Fixes:` V2: make error-print explicitly Thanks Koba Ko On Tue, Jul 4, 2023 at 3:16 PM Markus Elfring <Markus.Elfring@xxxxxx> wrote: > > > As per suggestions, i modified V3. > > could you please take a look > > > > Subject: [PATCH][V3] EDAC/i10nm: shift exponent is negative > > Would you like to put the text “[PATCH v4] EDAC/i10nm: Fix an inappropriate shift exponent” > into a subsequent patch? > > I would find further wording variants nicer. > > > > Because failed to read from DIMM, get the negative value for shift > > operation. > > A surprising value was determined after a read failure from a DIMM. > > > … > > UBSAN complains this error > > Software analyses pointed a data processing issue out. > > > > `UBSAN: shift-out-of-bounds in drivers/edac/skx_common.c:369:16 > > shift exponent -66 is negative` > > > > when get rows, cols and ranks, the returned error value doesn't be > > handled. > > A special value combination could not be handled so far. > > > > check the return value is EINVAL, if yes, directly return 0 and > > show error message explicitly. > > Check if an invalid value was detected by a call of the function “skx_get_dimm_attr”. > > * Print a corresponding error message in this case. > > * Return zero then directly from the function “skx_get_dimm_info”. > > > … > @@ -351,6 +351,8 @@ int skx_get_dimm_info(u32 mtr, u32 mcmtr, u32 amap, struct dimm_info *dimm, > ranks = numrank(mtr); > rows = numrow(mtr); > cols = imc->hbm_mc ? 6 : numcol(mtr); > + if (ranks == -EINVAL || rows == -EINVAL || cols == -EINVAL) > + return 0; > … > > > Can it be nicer to perform a check for such an error code directly > after each variable assignment? > (May this condition check be split?) > > > > Fixes: 4ec656bdf43a13) EDAC, skx_edac: Add EDAC driver for Skylake > > Please properly apply parentheses and double quotes for this tag. > > See also: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.4#n145 > > > > V2 -> V3: simplify the summary and add 'Fixes:' > > V1 -> V2: make error-print explicitly > > How do you think about to use more succinct version identifiers > for such descriptions? > > V4: > … > > V3: > … > > > Regards, > Markus
