From b70c77691ba9a26989fc8922a2e1807f6f8bdd09 Mon Sep 17 00:00:00 2001 From: Jacob Garber Date: Mon, 25 Mar 2019 18:20:06 -0600 Subject: [PATCH] nb/intel/pineview: Correct lsbpos(0) and msbpos(0) lsbpos and msbpos have incorrect behaviour when given 0. lsbpos(0) returns 8, and msbpos(0) hangs. The latter is because the check i >= 0 is always true for an unsigned integer, causing it to loop indefinitely (this was flagged by Coverity). 0 doesn't have a lsb or msb position, so we change both functions to return -1 in this case to indicate an error. The code already guards against calling these functions with 0, but we make this more explicit to prevent errors in the future. Found-by: Coverity Scan, CID 1347356, 1347386 Signed-off-by: Jacob Garber Change-Id: Ic5be50846cc545dcd48593e5ed3fd6068a6104cb Reviewed-on: https://review.coreboot.org/c/coreboot/+/32054 Tested-by: build bot (Jenkins) Reviewed-by: Paul Menzel Reviewed-by: Patrick Georgi --- src/northbridge/intel/pineview/raminit.c | 34 +++++++++++++++--------- 1 file changed, 22 insertions(+), 12 deletions(-) diff --git a/src/northbridge/intel/pineview/raminit.c b/src/northbridge/intel/pineview/raminit.c index fa5122afc6..bf00099bab 100644 --- a/src/northbridge/intel/pineview/raminit.c +++ b/src/northbridge/intel/pineview/raminit.c @@ -326,18 +326,24 @@ static u32 ddr_reg_to_mhz(u32 speed) } #endif -static u8 lsbpos(u8 val) //Forward +// Return the position of the least significant set bit, 0-indexed. +// 0 does not have a lsb, so return -1 for error. +static int lsbpos(u8 val) { - u8 i; - for (i = 0; (i < 8) && ((val & (1 << i)) == 0); i++); - return i; + for (int i = 0; i < 8; i++) + if (val & (1 << i)) + return i; + return -1; } -static u8 msbpos(u8 val) //Reverse +// Return the position of the most significant set bit, 0-indexed. +// 0 does not have a msb, so return -1 for error. +static int msbpos(u8 val) { - u8 i; - for (i = 7; (i >= 0) && ((val & (1 << i)) == 0); i--); - return i; + for (int i = 7; i >= 0; i--) + if (val & (1 << i)) + return i; + return -1; } static void sdram_detect_smallest_params(struct sysinfo *s) @@ -443,10 +449,14 @@ static void sdram_detect_ram_speed(struct sysinfo *s) die("No common CAS among dimms\n"); } + // commoncas is nonzero, so these calls will not error + u8 msbp = (u8)msbpos(commoncas); + u8 lsbp = (u8)lsbpos(commoncas); + // Start with fastest common CAS cas = 0; - highcas = msbpos(commoncas); - lowcas = max(lsbpos(commoncas), 5); + highcas = msbp; + lowcas = max(lsbp, 5); while (cas == 0 && highcas >= lowcas) { FOR_EACH_POPULATED_DIMM(s->dimms, i) { @@ -484,8 +494,8 @@ static void sdram_detect_ram_speed(struct sysinfo *s) die("Timings not supported by MCH\n"); } cas = 0; - highcas = msbpos(commoncas); - lowcas = lsbpos(commoncas); + highcas = msbp; + lowcas = lsbp; while (cas == 0 && highcas >= lowcas) { FOR_EACH_POPULATED_DIMM(s->dimms, i) { switch (freq) {