[flashrom-gerrit] [L] Change in flashrom[main]: rpmc: add rpmc commands feature

[Matti Finder (Code Review)]

Attention is currently required from: Anastasia Klimchuk, Peter Marheine.

Matti Finder 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 5:

(7 comments)

File include/rpmc.h:

https://review.coreboot.org/c/flashrom/+/84934/comment/08dcfa13_6490f47f?usp... :
PS4, Line 71: dependant
>

Done

https://review.coreboot.org/c/flashrom/+/84934/comment/78b96543_dbc24fb3?usp... :
PS4, Line 71: dependant
>

Done

https://review.coreboot.org/c/flashrom/+/84934/comment/dfb12815_7d20d2a9?usp... :
PS4, Line 90: struct flashrom_flashctx * flash
> Sorry I just noticed! we usually have asterisk sticking to the variable name (struct flashrom_flashc […]

Done

File rpmc.c:

https://review.coreboot.org/c/flashrom/+/84934/comment/5a8154a3_3ad5aaa0?usp... :
PS4, Line 131: 				if (res != RPMC_SUCCESS) {
             : 					return res;
             : 				}
> Found another places here (rpmc_poll_until_finished) where { } not needed because conditional body i […]

Done

https://review.coreboot.org/c/flashrom/+/84934/comment/1fce12c7_fee71ca7?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

Done

https://review.coreboot.org/c/flashrom/+/84934/comment/e629f8c4_be003353?usp... :
PS4, Line 497: no error string defined for this value
> Maybe […]

Done

File sfdp.c:

https://review.coreboot.org/c/flashrom/+/84934/comment/f33b9fd7_264fc3c3?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).

I don't know exactly but from looking at the code it only seems to set information about erase and write calls.
> 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.

Yes for safety and also consistency I agree.

Failure to parse the rpmc page is no problem since the feature bit is only set when the function can't fail anymore. I have removed it's influence on the return value of the function.