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

[Wolfgang Denk]

Dear Pierre Aubert,

In message <1398182087-14913-4-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
> 
> Signed-off-by: Pierre Aubert <p.aubert at staubli.com>
> CC: Pantelis Antoniou <panto at antoniou-consulting.com>
> ---
>  README           |   10 ++++
>  common/cmd_mmc.c |  123 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 132 insertions(+), 1 deletions(-)

Unfortunatley there is no changelog here.  Please note that this is
mandatory for updated patch versions, see [1] 

[1] http://www.denx.de/wiki/view/U-Boot/Patches#Sending_updated_patch_versions

A changelog in the cover letter is useless, as this does not get
recorded in patchwork.  The changelog must always be in the patch
itself.


...
> +	if (argc == 4 && strcmp(argv[2], "key") == 0)
> +		state = RPMB_KEY;
> +	if ((argc == 6 || argc == 7) && strcmp(argv[2], "read") == 0)
> +		state = RPMB_READ;
> +	else if (argc == 7 && strcmp(argv[2], "write") == 0)
> +		state = RPMB_WRITE;
> +	else if (argc == 3 && strcmp(argv[2], "counter") == 0)
> +		state = RPMB_COUNTER;

As mentiuoned before, these repeated strcmp() should probably be
replaced by a subcommand table.

> +	ret = CMD_RET_SUCCESS;
> +	if (state == RPMB_KEY) {
> +		key_addr = (void *)simple_strtoul(argv[3], NULL, 16);
> +		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;

Do a return here, or use some "goto errout" and place the label errout
before the needed cleanup acrtions.

> +		}
> +	} else if (state == RPMB_COUNTER) {

You can then get rid of this "else" level.

> +		unsigned long counter;
> +
> +		if (mmc_rpmb_get_counter(mmc, &counter))
> +			ret = CMD_RET_FAILURE;
> +		else

Ditto.

> +			printf("Write counter= %lx\n", counter);

Should this be a debug() ?

> +#ifdef CONFIG_SUPPORT_EMMC_RPMB
> +	} else if (strcmp(argv[1], "rpmb") == 0) {
> +		return do_mmcrpmb(argc, argv);
> +#endif /*  CONFIG_SUPPORT_EMMC_RPMB */
>  	}

Again, it would be better to switch the code to regular subcommand
processing.

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
"I knew then (in 1970) that a 4-kbyte minicomputer would cost as much
as a house. So I reasoned  that  after  college,  I'd  have  to  live
cheaply in an apartment and put all my money into owning a computer."
      - Apple co-founder Steve Wozniak, EE Times, June 6, 1988, pg 45