Change in ...coreboot[master]: nb/intel/pineview: Correct unsigned integer check in msbpos

[Jacob Garber (Code Review)]

Jacob Garber has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32054

Change subject: nb/intel/pineview: Correct unsigned integer check in msbpos
......................................................................

nb/intel/pineview: Correct unsigned integer check in msbpos

The check i >= 0 is always true for an unsigned integer, causing
msbpos(0) to hang. We correct it to i != 0.

Note this has no material change since the code guards against
finding the msb of 0 anyway, but it fixes Coverity CID 1347386.

Signed-off-by: Jacob Garber 
Change-Id: Ic5be50846cc545dcd48593e5ed3fd6068a6104cb
---
M src/northbridge/intel/pineview/raminit.c
1 file changed, 1 insertion(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/32054/1

diff --git a/src/northbridge/intel/pineview/raminit.c b/src/northbridge/intel/pineview/raminit.c
index fa5122a..4f9a0b4 100644
--- a/src/northbridge/intel/pineview/raminit.c
+++ b/src/northbridge/intel/pineview/raminit.c
@@ -336,7 +336,7 @@
 static u8 msbpos(u8 val) //Reverse
 {
    u8 i;
-	for (i = 7; (i >= 0) && ((val & (1 << i)) == 0); i--);
+	for (i = 7; (i != 0) && ((val & (1 << i)) == 0); i--);
    return i;
 }

[build bot (Jenkins) (Code Review)]

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32054 )

Change subject: nb/intel/pineview: Correct unsigned integer check in msbpos
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/32054/1/src/northbridge/intel/pineview/ramin... 
File src/northbridge/intel/pineview/raminit.c:

https://review.coreboot.org/#/c/32054/1/src/northbridge/intel/pineview/ramin... 
PS1, Line 339: 	for (i = 7; (i != 0) && ((val & (1 << i)) == 0); i--);
trailing statements should be on next line

[Jacob Garber (Code Review)]

Hello Patrick Rudolph, build bot (Jenkins), Damien Zammit,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/32054

to look at the new patch set (#2).

Change subject: nb/intel/pineview: Correct unsigned integer check in msbpos
......................................................................

nb/intel/pineview: Correct unsigned integer check in msbpos

The check i >= 0 is always true for an unsigned integer, causing
msbpos(0) to hang. We correct it to i != 0.

Note this has no material change since the code guards against
finding the msb of 0 anyway, but it fixes CID 1347356, 1347386.

Also fix checkpatch trailing statement lints.

Signed-off-by: Jacob Garber 
Change-Id: Ic5be50846cc545dcd48593e5ed3fd6068a6104cb
---
M src/northbridge/intel/pineview/raminit.c
1 file changed, 4 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/32054/2

[Arthur Heymans (Code Review)]

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32054 )

Change subject: nb/intel/pineview: Correct unsigned integer check in msbpos
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/#/c/32054/2/src/northbridge/intel/pineview/ramin... 
File src/northbridge/intel/pineview/raminit.c:

https://review.coreboot.org/#/c/32054/2/src/northbridge/intel/pineview/ramin... 
PS2, Line 329: static u8 lsbpos(u8 val) //Forward
             : {
             : 	u8 i;
             : 	for (i = 0; (i < 8) && ((val & (1 << i)) == 0); i++)
             : 		;
             : 	return i;
             : }
             : 
             : static u8 msbpos(u8 val) //Reverse
             : {
             : 	u8 i;
             : 	for (i = 7; (i != 0) && ((val & (1 << i)) == 0); i--)
             : 		;
             : 	return i;
             : }
Your return value is still meaningless when you input 0. Better properly guard against it instead just avoiding infinite loops.

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32054 )

Change subject: nb/intel/pineview: Correct unsigned integer check in msbpos
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/#/c/32054/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/32054/2//COMMIT_MSG@15 
PS2, Line 15: Also fix checkpatch trailing statement lints.
Next time a separate commit would be appreciated.

[Jacob Garber (Code Review)]

Jacob Garber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32054 )

Change subject: nb/intel/pineview: Correct unsigned integer check in msbpos
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/#/c/32054/2/src/northbridge/intel/pineview/ramin... 
File src/northbridge/intel/pineview/raminit.c:

https://review.coreboot.org/#/c/32054/2/src/northbridge/intel/pineview/ramin... 
PS2, Line 329: static u8 lsbpos(u8 val) //Forward
             : {
             : 	u8 i;
             : 	for (i = 0; (i < 8) && ((val & (1 << i)) == 0); i++)
             : 		;
             : 	return i;
             : }
             : 
             : static u8 msbpos(u8 val) //Reverse
             : {
             : 	u8 i;
             : 	for (i = 7; (i != 0) && ((val & (1 << i)) == 0); i--)
             : 		;
             : 	return i;
             : }
> Your return value is still meaningless when you input 0. […]

Alright. lsbpos(0) also returns 8, so I should probably fix that too. Should that be done in a separate commit?

Change in ...coreboot[master]: nb/intel/pineview: Correct lsbpos(0) and msbpos(0)

[Jacob Garber (Code Review)]

Hello Patrick Rudolph, build bot (Jenkins), Damien Zammit,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/32054

to look at the new patch set (#3).

Change subject: nb/intel/pineview: Correct lsbpos(0) and msbpos(0)
......................................................................

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 as CID 1347356, 1347386.
0 doesn't have a lsb or msb position, so we change both to return
-1 on this case to indicate an error.

Signed-off-by: Jacob Garber 
Change-Id: Ic5be50846cc545dcd48593e5ed3fd6068a6104cb
---
M src/northbridge/intel/pineview/raminit.c
1 file changed, 12 insertions(+), 8 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/32054/3

Change in ...coreboot[master]: nb/intel/pineview: Correct lsbpos(0) and msbpos(0)

[Jacob Garber (Code Review)]

Hello Patrick Rudolph, build bot (Jenkins), Damien Zammit,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/32054

to look at the new patch set (#4).

Change subject: nb/intel/pineview: Correct lsbpos(0) and msbpos(0)
......................................................................

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
---
M src/northbridge/intel/pineview/raminit.c
1 file changed, 22 insertions(+), 12 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/32054/4

Change in ...coreboot[master]: nb/intel/pineview: Correct lsbpos(0) and msbpos(0)

[Jacob Garber (Code Review)]

Jacob Garber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32054 )

Change subject: nb/intel/pineview: Correct lsbpos(0) and msbpos(0)
......................................................................

Patch Set 4:

(2 comments)

https://review.coreboot.org/#/c/32054/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/32054/2//COMMIT_MSG@15 
PS2, Line 15: Also fix checkpatch trailing statement lints.
> Next time a separate commit would be appreciated.

Done

https://review.coreboot.org/#/c/32054/2/src/northbridge/intel/pineview/ramin... 
File src/northbridge/intel/pineview/raminit.c:

https://review.coreboot.org/#/c/32054/2/src/northbridge/intel/pineview/ramin... 
PS2, Line 329: static u8 lsbpos(u8 val) //Forward
             : {
             : 	u8 i;
             : 	for (i = 0; (i < 8) && ((val & (1 << i)) == 0); i++)
             : 		;
             : 	return i;
             : }
             : 
             : static u8 msbpos(u8 val) //Reverse
             : {
             : 	u8 i;
             : 	for (i = 7; (i != 0) && ((val & (1 << i)) == 0); i--)
             : 		;
             : 	return i;
             : }
> Alright. lsbpos(0) also returns 8, so I should probably fix that too. […]

Done

Change in ...coreboot[master]: nb/intel/pineview: Correct lsbpos(0) and msbpos(0)

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32054 )

Change subject: nb/intel/pineview: Correct lsbpos(0) and msbpos(0)
......................................................................

Patch Set 4: Code-Review+1

Change in ...coreboot[master]: nb/intel/pineview: Correct lsbpos(0) and msbpos(0)

[Patrick Georgi (Code Review)]

Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32054 )

Change subject: nb/intel/pineview: Correct lsbpos(0) and msbpos(0)
......................................................................

Patch Set 4: Code-Review+2

Change in ...coreboot[master]: nb/intel/pineview: Correct lsbpos(0) and msbpos(0)

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/32054 )

Change subject: nb/intel/pineview: Correct lsbpos(0) and msbpos(0)
......................................................................

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 
---
M src/northbridge/intel/pineview/raminit.c
1 file changed, 22 insertions(+), 12 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Patrick Georgi: Looks good to me, approved
  Paul Menzel: Looks good to me, but someone else must approve

diff --git a/src/northbridge/intel/pineview/raminit.c b/src/northbridge/intel/pineview/raminit.c
index fa5122a..bf00099 100644
--- a/src/northbridge/intel/pineview/raminit.c
+++ b/src/northbridge/intel/pineview/raminit.c
@@ -326,18 +326,24 @@
 }
 #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 @@
    	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 @@
    		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) {