From mboxrd@z Thu Jan 1 00:00:00 1970 From: Pierre AUBERT Date: Fri, 18 Apr 2014 08:39:56 +0200 Subject: [U-Boot] [PATCH V2 2/2] eMMC: cmd_mmc.c adds the 'rpmb' sub-command for the 'mmc' command In-Reply-To: <20140417195619.09D8538040B@gemini.denx.de> References: <1397218337-27204-1-git-send-email-p.aubert@staubli.com> <1397747435-24042-1-git-send-email-p.aubert@staubli.com> <1397747435-24042-3-git-send-email-p.aubert@staubli.com> <20140417195619.09D8538040B@gemini.denx.de> Message-ID: <5350C8BC.9000607@staubli.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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@staubli.com> you wrote: >> This sub-command adds support for the RPMB partition of an eMMC: >> * mmc rpmb key
>> Programs the authentication key in the eMMC This key can not >> be overwritten. >> * mmc rpmb read
<#count> [address of key] >> Reads <#count> blocks of 256 bytes in the RPMB partition >> beginning at block number . If the optionnal >> address of the authentication key is provided, the >> Message Authentication Code (MAC) is verified on each >> block. >> * mmc rpmb write
<#count>
>> Writes <#count> blocks of 256 bytes in the RPMB partition >> beginning at block number . 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 >> CC: Pantelis Antoniou >> --- >> 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