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

[Marek Vasut]

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?

> + *
> + * 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 ?

> +
> +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.

> +	}
> +
> +	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 ?

> +	}
> +
> +	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 ?

> +		printf("%s\n", msg);
> +	cmd_usage(&__u_boot_cmd_tpm);

Two underscores aren't a good practice.

> +}

Cheers