Attention is currently required from: Angel Pons, Paul Menzel.
Máté Kukri has posted comments on this change by Máté Kukri. ( https://review.coreboot.org/c/coreboot/+/82272?usp=email )
Change subject: util/ifdtool: Support the DCI enable bit on Skylake/Kaby Lake ......................................................................
Patch Set 4:
(10 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/82272/comment/d53962e3_01ee322b?usp... : PS3, Line 14: CCT
CCA
Done
https://review.coreboot.org/c/coreboot/+/82272/comment/ed50bb61_59d8b410?usp... : PS3, Line 19: to be honored
unneeded words
Done
https://review.coreboot.org/c/coreboot/+/82272/comment/ceab156a_ec62cde6?usp... : PS3, Line 21: do
does ("the *state* of FPFs" is singular)
Done
https://review.coreboot.org/c/coreboot/+/82272/comment/f74c19c9_07798e24?usp... : PS3, Line 22:
Maybe paste the lines on the system you tested this with?
Done
File util/ifdtool/ifdtool.c:
https://review.coreboot.org/c/coreboot/+/82272/comment/bdc58549_9da8ba24?usp... : PS3, Line 1891: "DCI enable not supported for chipset! (Might be because the bit location" : "is unknown or because platform is too old)\n"
nit: Try to reflow these lines so that they fit in 96 characters (move "location" to second line?).
Done
https://review.coreboot.org/c/coreboot/+/82272/comment/ec6b101a_3a2ddd6d?usp... : PS3, Line 2182: for skylake or newer platform
well it's just skylake and kabylake for now (but newer ones can be added)
Done
https://review.coreboot.org/c/coreboot/+/82272/comment/0629e934_78374b17?usp... : PS3, Line 2237: /*[11]=*/
I like that idea
Done
https://review.coreboot.org/c/coreboot/+/82272/comment/820a76c4_61897e6f?usp... : PS3, Line 2511: dcienable > 1
What if `dcienable` is less than 0? […]
this isn't the only problem: - strtol could reporti an internal under/overflow? (at least that results in LONG_{MIN,MAX}) - without giving it an endp, and checking *endp != '\0', strol could silently ignore trailing garbage at the end of the optarg string - and i guess the truncation from long to int can also hide invalid arguments (and trigger "implmentation defined behaviour")
I've changed it to strtoul to catch the obvious case, but this program is so full of the usual C issues, I'd probably be rewriting the entire thing if wanted to fix all such problems.
https://review.coreboot.org/c/coreboot/+/82272/comment/7fabc2be_80eb3d5e?usp... : PS3, Line 2512: fprintf(stderr, "error: Illegal value\n");
Please be more descriptive, e.g. […]
Done
https://review.coreboot.org/c/coreboot/+/82272/comment/e53fbef8_b5db2a09?usp... : PS3, Line 2518: // fallthrough
Actually, this should exit with failure... […]
Done