Hello Wolfgang,
Le 17/04/2014 21:56, Wolfgang Denk a écrit :
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.
I will add it in a V3
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?
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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
