Change in coreboot[master]: vc/amd/agesa/[...]/Proc/Mem: Fix uninitialized scalar variable
awokd@danwin1210.me has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/38495 )
Change subject: vc/amd/agesa/[...]/Proc/Mem: Fix uninitialized scalar variable ......................................................................
vc/amd/agesa/[...]/Proc/Mem: Fix uninitialized scalar variable
FinalRxEnCycle[ByteLane] does not get set in mtthrcSeedTrain.c if (((UINT16) FinalRxEnCycle[ByteLane] >= NBPtr->MinRxEnSeedGross) && \ ((UINT16) FinalRxEnCycle[ByteLane] <= NBPtr->MaxRxEnSeedTotal)) evaluates FALSE. According to Coverity, this could result in cases where RxEnDlyTargetValue[ByteLane] = FinalRxEnCycle[ByteLane] is used with an uninitialized value. Move the FinalRxEnCycle[ByteLane] initial assignment up a line so it gets initialized regardless. Tested on Lenovo G505s.
Change-Id: Id1d3580c8915ba31e87059851fec2ae4b8b0e1da Signed-off-by: Joe Moore awokd@danwin1210.me Found-by: Coverity CID 1370599 --- M src/vendorcode/amd/agesa/f15tn/Proc/Mem/Tech/mtthrcSeedTrain.c M src/vendorcode/amd/agesa/f16kb/Proc/Mem/Tech/mtthrcSeedTrain.c 2 files changed, 2 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/95/38495/1
diff --git a/src/vendorcode/amd/agesa/f15tn/Proc/Mem/Tech/mtthrcSeedTrain.c b/src/vendorcode/amd/agesa/f15tn/Proc/Mem/Tech/mtthrcSeedTrain.c index 991667b..d21c246 100644 --- a/src/vendorcode/amd/agesa/f15tn/Proc/Mem/Tech/mtthrcSeedTrain.c +++ b/src/vendorcode/amd/agesa/f15tn/Proc/Mem/Tech/mtthrcSeedTrain.c @@ -504,8 +504,8 @@ for (ByteLane = 0; ByteLane < MaxByteLanes; ByteLane++) { if (RxEnDlyTargetFound[ByteLane] == FALSE) { MemClkCycle = (UINT8) (FailTestRxEnDly[ByteLane] >> 5); + FinalRxEnCycle[ByteLane] = PassTestRxEnDly[ByteLane] - 0x10; if (RxEnMemClkTested[ByteLane][MemClkCycle] ? RxEnMemClkSt[ByteLane][MemClkCycle] == TRUE : (TechPtr->ByteLaneError[ByteLane] && DlyWrittenToReg[ByteLane])) { - FinalRxEnCycle[ByteLane] = PassTestRxEnDly[ByteLane] - 0x10; if (((UINT16) FinalRxEnCycle[ByteLane] >= NBPtr->MinRxEnSeedGross) && ((UINT16) FinalRxEnCycle[ByteLane] <= NBPtr->MaxRxEnSeedTotal)) { // Since FailTestRxEnDly, we can set FinalRxEnCycle MemTRdPosRxEnSeedSetDly3 (TechPtr, (UINT16) FinalRxEnCycle[ByteLane], ByteLane); diff --git a/src/vendorcode/amd/agesa/f16kb/Proc/Mem/Tech/mtthrcSeedTrain.c b/src/vendorcode/amd/agesa/f16kb/Proc/Mem/Tech/mtthrcSeedTrain.c index 1446f3e..7ea7c56 100644 --- a/src/vendorcode/amd/agesa/f16kb/Proc/Mem/Tech/mtthrcSeedTrain.c +++ b/src/vendorcode/amd/agesa/f16kb/Proc/Mem/Tech/mtthrcSeedTrain.c @@ -505,8 +505,8 @@ for (ByteLane = 0; ByteLane < MaxByteLanes; ByteLane++) { if (RxEnDlyTargetFound[ByteLane] == FALSE) { MemClkCycle = (UINT8) (FailTestRxEnDly[ByteLane] >> 5); + FinalRxEnCycle[ByteLane] = PassTestRxEnDly[ByteLane] - 0x10; if (RxEnMemClkTested[ByteLane][MemClkCycle] ? RxEnMemClkSt[ByteLane][MemClkCycle] == TRUE : (TechPtr->ByteLaneError[ByteLane] && DlyWrittenToReg[ByteLane])) { - FinalRxEnCycle[ByteLane] = PassTestRxEnDly[ByteLane] - 0x10; if (((UINT16) FinalRxEnCycle[ByteLane] >= NBPtr->MinRxEnSeedGross) && ((UINT16) FinalRxEnCycle[ByteLane] <= NBPtr->MaxRxEnSeedTotal)) { // Since FailTestRxEnDly, we can set FinalRxEnCycle MemTRdPosRxEnSeedSetDly3 (TechPtr, (UINT16) FinalRxEnCycle[ByteLane], ByteLane);
awokd@danwin1210.me has uploaded a new patch set (#2). ( https://review.coreboot.org/c/coreboot/+/38495 )
Change subject: vc/amd/agesa/[...]/Proc/Mem: Fix uninitialized scalar variable ......................................................................
vc/amd/agesa/[...]/Proc/Mem: Fix uninitialized scalar variable
FinalRxEnCycle[ByteLane] does not get set in mtthrcSeedTrain.c if (((UINT16) FinalRxEnCycle[ByteLane] >= NBPtr->MinRxEnSeedGross) && \ ((UINT16) FinalRxEnCycle[ByteLane] <= NBPtr->MaxRxEnSeedTotal)) evaluates FALSE. According to Coverity, this could result in cases where RxEnDlyTargetValue[ByteLane] = FinalRxEnCycle[ByteLane] is used with an uninitialized value. Move the FinalRxEnCycle[ByteLane] initial assignment up a line so it gets initialized regardless. Tested on Lenovo G505s.
Change-Id: Id1d3580c8915ba31e87059851fec2ae4b8b0e1da Signed-off-by: Joe Moore awokd@danwin1210.me Found-by: Coverity CID 1370599 --- M src/vendorcode/amd/agesa/f15tn/Proc/Mem/Tech/mtthrcSeedTrain.c M src/vendorcode/amd/agesa/f16kb/Proc/Mem/Tech/mtthrcSeedTrain.c 2 files changed, 2 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/95/38495/2
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38495 )
Change subject: vc/amd/agesa/[...]/Proc/Mem: Fix uninitialized scalar variable ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/38495/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38495/2//COMMIT_MSG@15 PS2, Line 15: Tested on : Lenovo G505s. Please put it on a separate line (with blank line between paragraphs).
https://review.coreboot.org/c/coreboot/+/38495/2/src/vendorcode/amd/agesa/f1... File src/vendorcode/amd/agesa/f15tn/Proc/Mem/Tech/mtthrcSeedTrain.c:
https://review.coreboot.org/c/coreboot/+/38495/2/src/vendorcode/amd/agesa/f1... PS2, Line 508: (RxEnMemClkTested[ByteLane][MemClkCycle] ? RxEnMemClkSt[ByteLane][MemClkCycle] == TRUE : (TechPtr->ByteLaneError[ByteLane] && DlyWrittenToReg[ByteLane])) Do you know what this condition does? Moving the assignment out of the if statement changes logic, but I do not know if it’s intended. Why is 0x10 subtracted for example?
awokd@danwin1210.me has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38495 )
Change subject: [WIP] vc/amd/agesa/[...]/Proc/Mem: Fix uninitialized scalar variable ......................................................................
Patch Set 3:
(2 comments)
This change is ready for review.
https://review.coreboot.org/c/coreboot/+/38495/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38495/2//COMMIT_MSG@15 PS2, Line 15: Tested on : Lenovo G505s.
Please put it on a separate line (with blank line between paragraphs).
Done
https://review.coreboot.org/c/coreboot/+/38495/2/src/vendorcode/amd/agesa/f1... File src/vendorcode/amd/agesa/f15tn/Proc/Mem/Tech/mtthrcSeedTrain.c:
https://review.coreboot.org/c/coreboot/+/38495/2/src/vendorcode/amd/agesa/f1... PS2, Line 508: (RxEnMemClkTested[ByteLane][MemClkCycle] ? RxEnMemClkSt[ByteLane][MemClkCycle] == TRUE : (TechPtr->ByteLaneError[ByteLane] && DlyWrittenToReg[ByteLane]))
Do you know what this condition does? Moving the assignment out of the if statement changes logic, b […]
In no particular order, 0x10 is subtracted per 5b of 2.9.5.9.3 in Fam 15h BKDG: 5. Process the array of results and determine a pass-to-fail transition. A. DqsRcvEnCycle = the total delay value of the pass result. B. Program D18F2x9C_x0000_00[2A:10]_dct[1:0]_mp[1:0][DqsRcvEnGrossDelay, DqsRcvEnFineDe- lay] = DqsRcvEnCycle - 10h.
I am struggling following the cited condition. I think it has something to do with this part in 2.9.5.9.4: 3. For each read DQS delay value in D18F2x9C_x0000_0[3:0]0[6:5]_dct[1:0]_mp[1:0] from 0 to 1 UI: Program the read DQS delay value for the current lane. Read the DRAM training pattern from the test address. Record the result for the current settings as a pass or fail depending if the pattern is read correctly. Process the array of results and determine the longest string of consecutive passing read DQS delay values. If the read DQS delay results for the current lane contain three or more consecutive passing delay values, then program D18F2x9C_x0000_0[3:0]0[6:5]_dct[1:0]_mp[1:0] with the average value of the smallest and largest delay values in the string of consecutive passing results. If the average value of passing read DQS delay for the lane is negative, then adjust the input receiver DQ delay in D18F2x9C_x0D0F_0[F,7:0]1F_dct[1:0]_mp[1:0] for the lane as follows: IF (RxBypass3rd4thStg == 1) program RxBypass3rd4thStg=0 and repeat step 3 above for all ranks and lanes. ELSEIF (Rx4thStgEn == 0) program Rx4thStgEn=1 and repeat step 3 above for all ranks and lanes. ELSE program the read DQS delay for the lane with a value of zero.
However, the two places mentioned in the commit message are the only two that reference FinalRxEnCycle[]. Moving the assignment out of the if changes the logic only in the case Coverity was complaining about which currently results in it assigning an uninitialized value. My other thought was to put an initialization loop at the top and assign 0s to FinalRxEnCycle[], but if that was the intent it seemed to me it would be coded somewhere as an explicit assignment. I can test that approach and see what happens, though.
Marking this as a [WIP].
Mike Banon has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38495 )
Change subject: [WIP] vc/amd/agesa/[...]/Proc/Mem: Fix uninitialized scalar variable ......................................................................
Patch Set 3: Code-Review+1
AM1I-A (fam16h) also boots fine with this change applied.
Mike Banon has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38495 )
Change subject: [WIP] vc/amd/agesa/[...]/Proc/Mem: Fix uninitialized scalar variable ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38495/2/src/vendorcode/amd/agesa/f1... File src/vendorcode/amd/agesa/f15tn/Proc/Mem/Tech/mtthrcSeedTrain.c:
https://review.coreboot.org/c/coreboot/+/38495/2/src/vendorcode/amd/agesa/f1... PS2, Line 508: (RxEnMemClkTested[ByteLane][MemClkCycle] ? RxEnMemClkSt[ByteLane][MemClkCycle] == TRUE : (TechPtr->ByteLaneError[ByteLane] && DlyWrittenToReg[ByteLane]))
In no particular order, 0x10 is subtracted per 5b of 2.9.5.9.3 in Fam 15h BKDG: […]
Any news? Please also see a "DDR3 XMP support" change CB:40291 , since you're also digging that Proc/Mem AGESA code and maybe could share some ideas?
Mike Banon has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38495 )
Change subject: [WIP] vc/amd/agesa/[...]/Proc/Mem: Fix uninitialized scalar variable ......................................................................
Patch Set 3:
Please resolve the remaining comment to get this patch merged.
awokd@danwin1210.me has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38495 )
Change subject: [WIP] vc/amd/agesa/[...]/Proc/Mem: Fix uninitialized scalar variable ......................................................................
Patch Set 3:
Patch Set 3:
Please resolve the remaining comment to get this patch merged.
I'm not sure how to go about resolving it.
Attention is currently required from: awokd@danwin1210.me.
Mike Banon has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38495?usp=email )
Change subject: [WIP] vc/amd/agesa/[...]/Proc/Mem: Fix uninitialized scalar variable ......................................................................
Patch Set 3:
(1 comment)
Patchset:
PS3: awokd, please note that this patch is going to be "auto-abandoned" unless you rebase it on a similar parent or something
Attention is currently required from: Krystian Hebel, Michał Żygowski, awokd@danwin1210.me.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38495?usp=email )
Change subject: [WIP] vc/amd/agesa/[...]/Proc/Mem: Fix uninitialized scalar variable ......................................................................
Patch Set 3:
(1 comment)
Patchset:
PS3: 3mdeb folks, do you have any insights with your AGESA knowledge?
-
Mike Banon (Code Review) -
Name of user not set (Code Review)
-
Paul Menzel (Code Review)