Attention is currently required from: Matti Finder.

View Change

9 comments:

• Patchset:

  • Patch Set #4:

    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:

  • Patch Set #4, 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:

  • Patch Set #3, 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:

  • Patch Set #4, 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?

  • Patch Set #4, 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

  • Patch Set #4, 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

  • Patch Set #4, 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:

  • Patch Set #3, 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:

  • Patch Set #4, 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.

To view, visit change 84934. To unsubscribe, or for help writing mail filters, visit settings.