Attention is currently required from: Matti Finder.
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 4:
(9 comments)
Patchset:
PS4: Matti, thank you so much for your patience! I made another round of comments, but probably the last one.
There is one more thing, at the end when code is ready and all comments fixed, if you could re-test it again, on the latest version of the patch?
I am thinking about two scenarios to test:
1) chip model that supports rpmc, and handled as "SFDP-capable chip" 2) chip model that does not support rpmc, but is recognised and handled as a "SFDP-capable chip": to make sure baseline case is not broken.
File include/rpmc.h:
https://review.coreboot.org/c/flashrom/+/84934/comment/29424b16_61412a25?usp... : PS4, Line 90: struct flashrom_flashctx * flash Sorry I just noticed! we usually have asterisk sticking to the variable name (struct flashrom_flashctx *flash). As an example is sfdp.c.
I see that you used space between * and <name> consistently in all your new function declarations and definitions... but I need to ask you to change to align with our existing code style. Thank you so much! (hope you are not too upset)
File rpmc.c:
https://review.coreboot.org/c/flashrom/+/84934/comment/76c3bb63_6891df63?usp... : PS3, Line 224: return -1;
My original of how to handle the return values wasn't that good, and just lead to a lot of questions […]
Enum of return codes is good idea! Now it's all nice and readable.
File rpmc.c:
https://review.coreboot.org/c/flashrom/+/84934/comment/153afc03_2820beee?usp... : PS4, Line 117: usleep(flash->chip->rpmc_ctx.polling_long_delay_write_counter_us) Peter, I wanted to check, is this the right way to sleep? Are we supposed to be using `programmer_delay` everywhere?
https://review.coreboot.org/c/flashrom/+/84934/comment/6f8648e0_a80bd1fc?usp... : PS4, Line 131: if (res != RPMC_SUCCESS) { : return res; : } Found another places here (rpmc_poll_until_finished) where { } not needed because conditional body is one-line. And also above, when you call usleep
https://review.coreboot.org/c/flashrom/+/84934/comment/2fd0acbb_c9d89e30?usp... : PS4, Line 157: if (keyfile == NULL || read_buf_from_file(key, RPMC_HMAC_KEY_LENGTH, keyfile) != 0) { : return RPMC_ERROR_KEY_READ; : } And here, { } not needed, 1-line body
https://review.coreboot.org/c/flashrom/+/84934/comment/02e8f632_a82d2ae4?usp... : PS4, Line 497: no error string defined for this value Maybe
Unknown Internal Error: Error code not recognised
(because, the fact that we don't have a string is not a problem by itself, the problem we don't know what happened)
File sfdp.c:
https://review.coreboot.org/c/flashrom/+/84934/comment/062d90aa_49653796?usp... : PS3, Line 449: msg_cdbg("The chip contains an unknown " : "version of the JEDEC flash " : "parameters table, skipping it.\n");
Done
Thank you!
File sfdp.c:
https://review.coreboot.org/c/flashrom/+/84934/comment/acfe3672_a84de7bb?usp... : PS4, Line 457: if (sfdp_fill_flash(flash->chip, tbuf, len) == 0) { : ret = 1; : } I have a question: if for some reasons `sfdp_fill_flash` fails (i.e. returns non-zero), would RPMC feature work? (or any other feature). It seems like having `sfdp_fill_flash` successful is a pre-condition for doing anything with the chip? If we are handling the chip as generic SFDP-chip.
I am asking because right now failure of `sfdp_fill_flash` does not stop parsing the headers, and possibly below in line 468 `ret = 1` will mark probing successful, but it shouldn't be?
Maybe this should return error straight away if `sfdp_fill_flash` fails? Previously it wasn't important because only page 0 of headers was parsed.