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

[Pierre AUBERT]

Hello Wolfgang,

Le 17/04/2014 21:56, Wolfgang Denk a écrit :
> 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.
I will add it in a V3
>
>> 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?
I have found such a confirmation in cmd_fuse, cmd_otp and cmd_mmc. It 
makes sense to provide
a global function. I will try to submit a patch for that.
>> +	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?
You're right, the test must be more restrictive. The RPMB partition is 
available only since the release 4.41 of the Jedec
standard. I will fix it in a V3.
>
>> +			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.
No problem, it will be fixed globally in V3
>
>> +		} else if (state == RPMB_COUNTER) {
>> +			unsigned long counter;
>> +			if (mmc_rpmb_get_counter(mmc, &counter))
> Please insert a blank line between declarations and code.
Ok
>
>> +			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.
For coherency with the mmc read and mmc write, I kept the same output. 
But it can be changed, of course.
>
>>   #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.]
Do you think about the use of the macro U_BOOT_CMD_MKENT ?
>
> Best regards,
>
> Wolfgang Denk
>
Best regards
Pierre Aubert