[U-Boot] [PATCH v2 2/2] Add a cli command to test the TPM device.

[Vadim Bendebury]

Dear Marek Vasut,

thank you for your comments, please see below:

On Sat, Oct 15, 2011 at 11:02 AM, Marek Vasut <marek.vasut at 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
>