Jacob Garber has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33409
Change subject: nb/intel/x4x: Die on unknown memory speed ......................................................................
nb/intel/x4x: Die on unknown memory speed
The speed argument should be one of the six values from the mem_clk enum, so something is very wrong if this is not the case. Better to die() now than return 0, which will cause a division-by-zero error later on where this function is called.
Change-Id: Ib628c0eed3d6571bdde1df27ae213ca0691ec256 Signed-off-by: Jacob Garber jgarber1@ualberta.ca Found-by: Coverity CID 1391088 --- M src/northbridge/intel/x4x/raminit_ddr23.c 1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/09/33409/1
diff --git a/src/northbridge/intel/x4x/raminit_ddr23.c b/src/northbridge/intel/x4x/raminit_ddr23.c index 422b0ff..dae10cf 100644 --- a/src/northbridge/intel/x4x/raminit_ddr23.c +++ b/src/northbridge/intel/x4x/raminit_ddr23.c @@ -42,7 +42,7 @@ static const u16 mhz[] = { 0, 0, 667, 800, 1067, 1333 };
if (speed >= ARRAY_SIZE(mhz)) - return 0; + die("RAM init: unknown DDR2 speed %u\n", speed);
return mhz[speed]; }
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33409 )
Change subject: nb/intel/x4x: Die on unknown memory speed ......................................................................
Patch Set 1: Code-Review-1
We still have the same problem when 'u32 speed' is either zero or one.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33409 )
Change subject: nb/intel/x4x: Die on unknown memory speed ......................................................................
Patch Set 1: Code-Review-1
(1 comment)
https://review.coreboot.org/#/c/33409/1/src/northbridge/intel/x4x/raminit_dd... File src/northbridge/intel/x4x/raminit_ddr23.c:
https://review.coreboot.org/#/c/33409/1/src/northbridge/intel/x4x/raminit_dd... PS1, Line 45: DDR2 this function has nothing to do with DDR2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33409 )
Change subject: nb/intel/x4x: Die on unknown memory speed ......................................................................
Patch Set 1:
Patch Set 1: Code-Review-1
We still have the same problem when 'u32 speed' is either zero or one.
What's more, since 'u32 speed' is actually an 'enum mem_clock' in both cases 'ddr2mhz(u32 speed)' is called, the 'die()' would never run.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33409 )
Change subject: nb/intel/x4x: Die on unknown memory speed ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/33409/1/src/northbridge/intel/x4x/raminit_dd... File src/northbridge/intel/x4x/raminit_ddr23.c:
https://review.coreboot.org/#/c/33409/1/src/northbridge/intel/x4x/raminit_dd... PS1, Line 45: DDR2
this function has nothing to do with DDR2
I guess the confusion may have arisen because of the function name. I am pretty sure it means "DDR to MHz".
Jacob Garber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33409 )
Change subject: nb/intel/x4x: Die on unknown memory speed ......................................................................
Patch Set 1:
Patch Set 1:
Patch Set 1: Code-Review-1
We still have the same problem when 'u32 speed' is either zero or one.
What's more, since 'u32 speed' is actually an 'enum mem_clock' in both cases 'ddr2mhz(u32 speed)' is called, the 'die()' would never run.
Well, enums in C are weakly typed, so it's possible to assign an enum a value that is not one of the declared enumerations (hence the "very wrong").
Jacob Garber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33409 )
Change subject: nb/intel/x4x: Die on unknown memory speed ......................................................................
Patch Set 1:
Patch Set 1: Code-Review-1
We still have the same problem when 'u32 speed' is either zero or one.
Looking at the enum, those values should be 400 and 533. Do you think we could fill those in?
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33409 )
Change subject: nb/intel/x4x: Die on unknown memory speed ......................................................................
Patch Set 1:
Patch Set 1:
Patch Set 1: Code-Review-1
We still have the same problem when 'u32 speed' is either zero or one.
Looking at the enum, those values should be 400 and 533. Do you think we could fill those in?
Those values are not supported by the code.
Hello Angel Pons, Arthur Heymans, David Hendricks, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33409
to look at the new patch set (#2).
Change subject: nb/intel/x4x: Die on invalid memory speeds ......................................................................
nb/intel/x4x: Die on invalid memory speeds
The speed argument should be one of the six values from the mem_clock enum, so something is very wrong if this is not the case. Better to die now than return 0, which will cause a division-by-zero error later on where this function is called. The first two speeds are also unsupported and have the same problem with returning 0, so die on those as well.
Change-Id: Ib628c0eed3d6571bdde1df27ae213ca0691ec256 Signed-off-by: Jacob Garber jgarber1@ualberta.ca Found-by: Coverity CID 1391088 --- M src/northbridge/intel/x4x/raminit_ddr23.c 1 file changed, 2 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/09/33409/2
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33409 )
Change subject: nb/intel/x4x: Die on invalid memory speeds ......................................................................
Patch Set 2: Code-Review+1
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33409 )
Change subject: nb/intel/x4x: Die on invalid memory speeds ......................................................................
Patch Set 2: Code-Review+1
I wouldn't expect a conversion function to die(), but I guess it's better than dividing by zero.
David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33409 )
Change subject: nb/intel/x4x: Die on invalid memory speeds ......................................................................
Patch Set 3: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/33409/1/src/northbridge/intel/x4x/r... File src/northbridge/intel/x4x/raminit_ddr23.c:
https://review.coreboot.org/c/coreboot/+/33409/1/src/northbridge/intel/x4x/r... PS1, Line 45: DDR2
I guess the confusion may have arisen because of the function name. […]
Function was renamed in CB:33491 (thanks Elyes!)
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33409 )
Change subject: nb/intel/x4x: Die on invalid memory speeds ......................................................................
Patch Set 3: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/33409/1/src/northbridge/intel/x4x/r... File src/northbridge/intel/x4x/raminit_ddr23.c:
https://review.coreboot.org/c/coreboot/+/33409/1/src/northbridge/intel/x4x/r... PS1, Line 45: DDR2
Function was renamed in CB:33491 (thanks Elyes!)
Done
Martin Roth has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/33409 )
Change subject: nb/intel/x4x: Die on invalid memory speeds ......................................................................
nb/intel/x4x: Die on invalid memory speeds
The speed argument should be one of the six values from the mem_clock enum, so something is very wrong if this is not the case. Better to die now than return 0, which will cause a division-by-zero error later on where this function is called. The first two speeds are also unsupported and have the same problem with returning 0, so die on those as well.
Change-Id: Ib628c0eed3d6571bdde1df27ae213ca0691ec256 Signed-off-by: Jacob Garber jgarber1@ualberta.ca Found-by: Coverity CID 1391088 Reviewed-on: https://review.coreboot.org/c/coreboot/+/33409 Reviewed-by: David Hendricks david.hendricks@gmail.com Reviewed-by: Angel Pons th3fanbus@gmail.com Reviewed-by: HAOUAS Elyes ehaouas@noos.fr Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/northbridge/intel/x4x/raminit_ddr23.c 1 file changed, 2 insertions(+), 2 deletions(-)
Approvals: build bot (Jenkins): Verified David Hendricks: Looks good to me, approved HAOUAS Elyes: Looks good to me, but someone else must approve Angel Pons: Looks good to me, approved
diff --git a/src/northbridge/intel/x4x/raminit_ddr23.c b/src/northbridge/intel/x4x/raminit_ddr23.c index 32618e8..1e40b9c 100644 --- a/src/northbridge/intel/x4x/raminit_ddr23.c +++ b/src/northbridge/intel/x4x/raminit_ddr23.c @@ -41,8 +41,8 @@ { static const u16 mhz[] = { 0, 0, 667, 800, 1067, 1333 };
- if (speed >= ARRAY_SIZE(mhz)) - return 0; + if (speed <= 1 || speed >= ARRAY_SIZE(mhz)) + die("RAM init: invalid memory speed %u\n", speed);
return mhz[speed]; }
-
Angel Pons (Code Review) -
Arthur Heymans (Code Review)
-
David Hendricks (Code Review)
-
HAOUAS Elyes (Code Review)
-
Jacob Garber (Code Review)
-
Martin Roth (Code Review)