Attention is currently required from: Patrick Rudolph.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/78133?usp=email )
Change subject: cpu/intel/model_206ax: Print supported C-states ......................................................................
Patch Set 5:
(7 comments)
Patchset:
PS5: might also be an idea to split the patch into the part that introduces the common code functionality and the part that uses that in the intel cpu code. didn't do a full in-depth review when i commented the last time, but i've done a much more in-depth review now
File src/arch/x86/cpu_common.c:
https://review.coreboot.org/c/coreboot/+/78133/comment/735a820e_1e1153a0 : PS5, Line 196: int cpu_get_c_state_support(const int lvl) hmm, i'd make both return value and parameter unsigned integers, since those won't and shouldn't be negative. should the value range of the lvl parameter also be checked?
https://review.coreboot.org/c/coreboot/+/78133/comment/d7364a83_33a34241 : PS5, Line 201: 1 might be good to have a define for this bit
File src/cpu/intel/model_206ax/acpi.c:
https://review.coreboot.org/c/coreboot/+/78133/comment/80807737_44c1ca25 : PS5, Line 103: h, l it would make the code easier to read if you'd use 'state' and 'substate' as variable names
https://review.coreboot.org/c/coreboot/+/78133/comment/73fd868c_4c344b02 : PS5, Line 104: maybe add a comment that C0 is always supported; was wondering why i was initialized as 1, but that seems to be the reason
https://review.coreboot.org/c/coreboot/+/78133/comment/1223326b_12ef286c : PS5, Line 109: > l i'd drop this or make it != 0 and drop the + 1 in line 106 too; at least for me that would make the code easier to read
https://review.coreboot.org/c/coreboot/+/78133/comment/455852b1_51ba28ca : PS5, Line 110: strcpy(&str[strlen(str)], c_state_names[i]); : strcpy(&str[strlen(str)], " "); i'd just use printk directly instead of assembling a string and then printing it. the advantage of not assembling a string would be that that will eliminate the possibility of out of bounds writes