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
