Dear Marek Vasut,
thank you for your comments, please see below:
On Sat, Oct 15, 2011 at 11:02 AM, Marek Vasut marek.vasut@gmail.com wrote:
On Saturday, October 15, 2011 05:39:08 AM Vadim Bendebury wrote:
The command gets an arbitrary number of arguments (up to 30), which are interpreted as byte values and are feed into the TPM device after proper initialization. Then the return value and data of the TPM driver is examined.
Dear Vadim Bendebury,
[...]
diff --git a/common/cmd_tpm.c b/common/cmd_tpm.c new file mode 100644 index 0000000..e008a78 --- /dev/null +++ b/common/cmd_tpm.c @@ -0,0 +1,111 @@ +/*
- Copyright (c) 2011 The Chromium OS Authors. All rights reserved.
- Released under the 2-clause BSD license.
Are we ok with this ? Also, you say something about GPL in the same comment?
Can someone please tell me what needs to be put in the license headers and I will do it. I hear different suggestions from different people.
- See file CREDITS for list of people who contributed to this
- project.
- This program is free software; you can redistribute it and/or
- modify it under the terms of the GNU General Public License as
- published by the Free Software Foundation; either version 2 of
- the License, or (at your option) any later version.
- This program is distributed in the hope that it will be useful,
- but WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- GNU General Public License for more details.
- You should have received a copy of the GNU General Public License
- along with this program; if not, write to the Free Software
- Foundation, Inc., 59 Temple Place, Suite 330, Boston,
- MA 02111-1307 USA
- */
[...]
- puts("Got TPM response:\n");
- for (i = 0; i < read_size; i++)
- printf(" %2.2x", tpm_buffer[i]);
- puts("\n");
- rv = 0;
- } else {
- puts("tpm command failed\n");
- }
- return rv;
+}
You might want to use debug() where fitting ?
I don't expect failures and if happen prefer them to be printed always, not only if debug is enabled.
+static int do_tpm(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) +{
- int rv = 0;
- /*
- * Verify that in case it is present, the first argument, it is
- * exactly one character in size.
- */
- if (argc < 7) {
- puts("command should be at least six bytes in size\n");
- return ~0;
Ugh, return 1 isn't ok ? Using ~0 on int type is weird.
I was under impression that any nonzero value is good. I see sometimes -1 returned for error in other u-boot sources. Also, I am sorry, I am new to this, when someone says "it is weird" - does this mean that it has to be changed?
- }
- if (tis_init()) {
- puts("tis_init() failed!\n");
- return ~0;
- }
- if (tis_open()) {
- puts("tis_open() failed!\n");
- return ~0;
- }
- rv = tpm_process(argc - 1, argv + 1);
- if (!rv && tis_close()) {
- puts("tis_close() failed!\n");
- rv = ~0;
This doesn't make much sense, tis_close() might not be called under all circumstances, is it ok ?
good point, I thought about this, but left untouched. It does not matter with this driver, but is better to call tis_close() no matter what. I'll fix it.
- }
- return rv;
+}
+U_BOOT_CMD(tpm, MAX_TRANSACTION_SIZE, 1, do_tpm,
- "tpm <data> [<data>] - write data and read ressponse\n",
- "send arbitrary data to the TPM and read the response"
+);
+static void report_error(const char *msg) +{
- if (msg && *msg)
Uhm, you also check if first character is non-zero? why ?
To avoid printing an empty string if someone calls this with an empty message?
- printf("%s\n", msg);
- cmd_usage(&__u_boot_cmd_tpm);
Two underscores aren't a good practice.
I did this as a result of a previous review. Do you have a suggestion how this should be done instead?
cheers, /vb
+}
Cheers