Dear Pierre Aubert,
In message 1397747435-24042-3-git-send-email-p.aubert@staubli.com you wrote:
This sub-command adds support for the RPMB partition of an eMMC:
- mmc rpmb key <address of the authentication key> Programs the authentication key in the eMMC This key can not be overwritten.
- mmc rpmb read <address> <block> <#count> [address of key] Reads <#count> blocks of 256 bytes in the RPMB partition beginning at block number <block>. If the optionnal address of the authentication key is provided, the Message Authentication Code (MAC) is verified on each block.
- mmc rpmb write <address> <block> <#count> <address of key> Writes <#count> blocks of 256 bytes in the RPMB partition beginning at block number <block>. The datas are signed with the key provided.
- mmc rpmb counter Returns the 'Write counter' of the RPMB partition.
The sub-command is conditional on compilation flag CONFIG_SUPPORT_EMMC_RPMB
Such new options must be documented in the README.
Signed-off-by: Pierre Aubert p.aubert@staubli.com CC: Pantelis Antoniou panto@antoniou-consulting.com
common/cmd_mmc.c | 128 +++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 files changed, 127 insertions(+), 1 deletions(-)
diff --git a/common/cmd_mmc.c b/common/cmd_mmc.c index c1916c9..3cf11e7 100644 --- a/common/cmd_mmc.c +++ b/common/cmd_mmc.c @@ -130,7 +130,123 @@ U_BOOT_CMD( "display MMC info", "- display info of the current MMC device" ); +#ifdef CONFIG_SUPPORT_EMMC_RPMB +static int confirm_key_prog(void) +{
- puts("Warning: Programming authentication key can be done only once !\n"
" Use this command only if you are sure of what you are doing,\n""Really perform the key programming ? ");- if (getc() == 'y') {
Would it not makes sense to flush the input before reading the char, so that you don;t react on any type-ahead that might already be buffered?
int c;putc('y');c = getc();putc('\n');if (c == '\r')return 1;- }
Should we allow for 'Y"? And for "yes" / "Yes"?
We have getenv_yesno() - maybe we should provide a similar function that can be used in other places where such interactive confirmation is needed?
- if (state != RPMB_INVALID) {
Change this into
if (state == RPMB_INVALID) return CMD_RET_USAGE;
and avoid one level of indentation; this will make the code much easier to read.
if (IS_SD(mmc)) {
Is IS_SD() a reliable test for eMMC devics, or would that also return true in other cases?
if (confirm_key_prog()) {if (mmc_rpmb_set_key(mmc, key_addr)) {printf("ERROR - Key already programmed ?\n");ret = CMD_RET_FAILURE;}} else {ret = CMD_RET_FAILURE;}
You should really avoid deep nesting and take early exits. You can write this as:
if (!confirm_key_prog()) return CMD_RET_FAILURE;
if (mmc_rpmb_set_key(mmc, key_addr)) { printf("ERROR - Key already programmed ?\n"); ret = CMD_RET_FAILURE; }
Please fix globally.
} else if (state == RPMB_COUNTER) {unsigned long counter;if (mmc_rpmb_get_counter(mmc, &counter))
Please insert a blank line between declarations and code.
printf("%d RPMB blocks %s: %s\n",n, argv[2], (n == cnt) ? "OK" : "ERROR");
As the input is in hex, it is usually also a good idea to (also) print the count in hex.
#endif /* CONFIG_SUPPORT_EMMC_BOOT */ +#ifdef CONFIG_SUPPORT_EMMC_RPMB
- } else if (strcmp(argv[1], "rpmb") == 0) {
return do_mmcrpmb(argc, argv);+#endif /* CONFIG_SUPPORT_EMMC_RPMB */
I think that now, with more subcommands being added, we should convert the mmc code to proper subcommand handling. [It might even make sense to do so for "mmc rpmb", too.]
Best regards,
Wolfgang Denk