[U-Boot] [PATCH V2 2/2] eMMC: cmd_mmc.c adds the 'rpmb' sub-command for the 'mmc' command

[Wolfgang Denk]

Dear Pierre Aubert,

In message <1397747435-24042-3-git-send-email-p.aubert at 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 at staubli.com>
> CC: Pantelis Antoniou <panto at 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: wd at 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_