Dear Pierre Aubert, In message <1397747435-24042-3-git-send-email-p.aub...@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.aub...@staubli.com> > CC: Pantelis Antoniou <pa...@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 -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de "The computer programmer is a creator of universes for which he alone is responsible. Universes of virtually unlimited complexity can be created in the form of computer programs." - Joseph Weizenbaum, _Computer Power and Human Reason_ _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot