Attention is currently required from: Matti Finder, Peter Marheine.
Anastasia Klimchuk has posted comments on this change by Matti Finder. ( https://review.coreboot.org/c/flashrom/+/84934?usp=email )
Change subject: rpmc: add rpmc commands feature ......................................................................
Patch Set 3:
(7 comments)
Patchset:
PS3: Given that you agreed in the other comment, you can add yourself in MAINTAINERS file and include in this patch. Add your entry in the CORE section, and in alphabetical order. I think better have full name of the feature (REPLAY PROTECTED MONOTONIC COUNTER). The file paths are rpmc.c and include/rpmc.h
What you are subscribing for, is described here: https://flashrom.org/about_flashrom/team.html ("flashrom reviewers" section). Another doc to read (and maybe you read it already) is https://flashrom.org/dev_guide/development_guide.html
And once this patch is ready and approved, it will be submitted together with your addition to maintainers file! :)
File rpmc.c:
https://review.coreboot.org/c/flashrom/+/84934/comment/eaaa89b9_e2c2ba59?usp... : PS3, Line 131: Unsupported Maybe `Unknown` instead of `Unsupported`, because it's not even in our enum.
https://review.coreboot.org/c/flashrom/+/84934/comment/9659cdaf_b4bca503?usp... : PS3, Line 223: { You don't need { } when the body of if conditional is only one line. (same in few more places in the code)
https://review.coreboot.org/c/flashrom/+/84934/comment/3e040fdc_38f3da54?usp... : PS3, Line 224: return -1; I am adding comment here, as this is an example, but it applies to other code in this file.
Some functions in this file return -1 on error, and some return 1. Is there a reason for this inconsistency within the same file?
I haven't read the JESD260 document (but actually want to), but I understand the standard may require specific return values for operations. That's fine. But, for functions which are `static int` in here, these are yours, is this right? So you can decide how to represent the error code. Maybe if standard requires -1 on error, you can adopt this approach for all rpmc functions?
Also if -1 is required by standard, maybe create a macro for it RPMC_ERROR_CODE or something like that, and then use it everywhere.
File sfdp.c:
https://review.coreboot.org/c/flashrom/+/84934/comment/3758d1ed_fc51bb6c?usp... : PS3, Line 446: hdrs[i].id Can these header IDs ever repeat from one header to the other? (probably not?) For example, is id 0 always on header 0? Is there any relation between `i` and `hdrs[i].id` ?
https://review.coreboot.org/c/flashrom/+/84934/comment/301e4aef_df73f51e?usp... : PS3, Line 449: msg_cdbg("The chip contains an unknown " : "version of the JEDEC flash " : "parameters table, skipping it.\n"); I know it was like this before, but while you are here... maybe you can add `hdrs[i].id` and `hdrs[i].v_major` to this debug message? Otherwise it's just saying "something went wrong" without any actual debug info. (and same below)
https://review.coreboot.org/c/flashrom/+/84934/comment/559b34f2_5ca915fb?usp... : PS3, Line 461: 1 It's in hex few lines above (0x01 on line 448) and here is decimal. Same number, but let's use the same base.