Attention is currently required from: Matti Finder, Peter Marheine.
7 comments:
Patchset:
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:
Patch Set #3, Line 131: Unsupported
Maybe `Unknown` instead of `Unsupported`, because it's not even in our enum.
You don't need { } when the body of if conditional is only one line.
(same in few more places in the code)
Patch Set #3, 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:
Patch Set #3, 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` ?
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)
It's in hex few lines above (0x01 on line 448) and here is decimal. Same number, but let's use the same base.
To view, visit change 84934. To unsubscribe, or for help writing mail filters, visit settings.