Attention is currently required from: Matti Finder.
Anastasia Klimchuk has posted comments on this change by Matti Finder. ( https://review.coreboot.org/c/flashrom/+/84840?usp=email )
The change is no longer submittable: All-Comments-Resolved is unsatisfied now.
Change subject: cli_classic: Add rpmc command support ......................................................................
Patch Set 4:
(8 comments)
Patchset:
PS4: Matti, wow, that's so much work that you did! Thank you so much for your contribution!
Sorry I am joining the review a bit later. I added comments around the code, but also I have few comments to the patch in general:
1) What do you think splitting this into two commits: first can add the feature implementation (basically everything except of cli_classic). Second would be cli client which is using new feature, also manpage update (mentioned in the other comment). Also, see next point ->
2) This cool new feature needs to be added to "Recent development", so that whoever is interested knows what to expect in the next release, the doc is here: `doc/release_notes/devel.rst`
3) You are now the author of lots of code in flashrom, what do you think about becoming a maintainer for your feature? The commitment would be only for your feature, not for everything that is happening on flashrom We have a doc about the team https://flashrom.org/about_flashrom/team.html (but you are welcome to get involved into anything interested for you! :)
Commit Message:
https://review.coreboot.org/c/flashrom/+/84840/comment/a1878c01_09f775c9?usp... : PS4, Line 21: usefull usefull -> useful (typo)
(3 times in commit message :)
File cli_classic.c:
https://review.coreboot.org/c/flashrom/+/84840/comment/3cedd297_ca38f78d?usp... : PS4, Line 147: #if CONFIG_RPMC_ENABLED == 1 : "RPMC COMMANDS\n" : " --get-rpmc-status read the extended status\n" : " --write-root-key write the root key register\n" : " --update-hmac-key update the hmac key register\n" : " --increment-counter <previous>\n" : " increment rpmc counter\n" : " --get-counter get rpmc counter\n" : "RPMC OPTIONS\n" : " --counter-address <number> counter address (default: 0)\n" : " --rpmc-root-key <keyfile> rpmc root key file\n" : " --key-data <keydata> hex number to use as key data (default: 0)\n" : #endif // CONFIG_RPMC_ENABLED These new commands need to also be documented on the manpage.
Manpage source is here: `doc/classic_cli_manpage.rst` and from this source we generated the manpage and also a webpage (https://flashrom.org/classic_cli_manpage.html)
https://review.coreboot.org/c/flashrom/+/84840/comment/f238b960_f90f467b?usp... : PS4, Line 1323: if (options.rpmc_read_data) : ret = rpmc_read_data(fill_flash); I am thinking, and maybe this is more the question to Peter, what do you think, if we were too add this to libflashrom, would that be easy? Currently it's only cli feature, but it shouldn't be cli only in the long term: cli should be using libflashrom API as with everything else.
In ideal world :) we would add libflashrom API first, and then cli client which is using it. Not all in one commit of course.
File include/flash.h:
https://review.coreboot.org/c/flashrom/+/84840/comment/abd265b2_4fc3aab6?usp... : PS4, Line 553: implement I know it's a small thing, but if you start with uppercase it would look better.
https://review.coreboot.org/c/flashrom/+/84840/comment/195ab568_f5db14d5?usp... : PS4, Line 575: /* : * all times in microsecond (us) : */ Also small comment, but this can be 1-line comment, and starting with uppercase
`/* All times in microsecond (us) */`
https://review.coreboot.org/c/flashrom/+/84840/comment/100f756f_277db26f?usp... : PS4, Line 581: rpmc_ctx I understand this goes into a flashchip definition? and you mentioned you tested on one chip? It would be great if you could update the definition for the chip you tested, probably better in a separate commit.
It would be a very useful as an real example of how to do it.
File meson.build:
https://review.coreboot.org/c/flashrom/+/84840/comment/daa23c24_d1227966?usp... : PS4, Line 177: if libcrypto.found() : add_project_arguments('-DCONFIG_RPMC_ENABLED=1', language : 'c') This ignores the meson option you added. It is auto by default, however it is possible to disable it.
Even if libcrypto found, you also need to check that `rpmc` option is enabled or auto.
For example docs are doing this, in doc/meson.build , lines 9-10 are checking the library, and then the meson option too.