[U-Boot] [PATCH v2 1/2] Introduce generic TPM support in u-boot
TPM (Trusted Platform Module) is an integrated circuit and software platform that provides computer manufacturers with the core components of a subsystem used to assure authenticity, integrity and confidentiality.
This driver supports version 1.2 of the TCG (Trusted Computing Group) specifications.
The TCG specification defines several so called localities in a TPM chip, to be controlled by different software layers. When used on a typical x86 platform during the firmware phase, only locality 0 can be accessed by the CPU, so this driver even while supporting the locality concept presumes that only locality zero is used.
This implementation is loosely based on the article "Writing a TPM Device Driver" published on http://ptgmedia.pearsoncmg.com
Compiling this driver with DEBUG defined will generate trace of all accesses to TMP registers.
This driver has been tested and is being used in three different functional ChromeOS machines (Pinetrail and Sandy Bridge Intel chipsets) all using the same Infineon SLB 9635 TT 1.2 device.
A u-boot cli command allowing access to the TPM was also implemented and is being submitted as a second patch.
Signed-off-by: Vadim Bendebury vbendeb@chromium.org CC: Wolfgang Denk wd@denx.de --- Makefile | 3 + README | 11 + drivers/tpm/Makefile | 27 +++ drivers/tpm/generic_lpc_tpm.c | 488 +++++++++++++++++++++++++++++++++++++++++ include/tpm.h | 54 +++++ 5 files changed, 583 insertions(+), 0 deletions(-) create mode 100644 drivers/tpm/Makefile create mode 100644 drivers/tpm/generic_lpc_tpm.c create mode 100644 include/tpm.h
diff --git a/Makefile b/Makefile index cd6fc8c..76f780a 100644 --- a/Makefile +++ b/Makefile @@ -268,6 +268,9 @@ LIBS += arch/powerpc/cpu/mpc8xxx/lib8xxx.o endif LIBS += drivers/rtc/librtc.o LIBS += drivers/serial/libserial.o +ifeq ($(CONFIG_GENERIC_LPC_TPM),y) +LIBS += drivers/tpm/libtpm.o +endif LIBS += drivers/twserial/libtws.o LIBS += drivers/usb/eth/libusb_eth.o LIBS += drivers/usb/gadget/libusb_gadget.o diff --git a/README b/README index 0868531..1dced5a 100644 --- a/README +++ b/README @@ -1010,12 +1010,23 @@ The following options need to be configured: CONFIG_SH_ETHER_USE_PORT Define the number of ports to be used
+ CONFIG_SH_ETHER_PHY_ADDR Define the ETH PHY's address
CONFIG_SH_ETHER_CACHE_WRITEBACK If this option is set, the driver enables cache flush.
+- TPM Support: + CONFIG_GENERIC_LPC_TPM + Support for generic parallel port TPM devices. Only one device + per system is supported at this time. + + CONFIG_TPM_TIS_BASE_ADDRESS + Base address where the generic TPM device is mapped + to. Contemporary x86 systems usually map it at + 0xfed40000. + - USB Support: At the moment only the UHCI host controller is supported (PIP405, MIP405, MPC5200); define diff --git a/drivers/tpm/Makefile b/drivers/tpm/Makefile new file mode 100644 index 0000000..7df586f --- /dev/null +++ b/drivers/tpm/Makefile @@ -0,0 +1,27 @@ +# Copyright (c) 2011 The Chromium OS Authors. All rights reserved. +# Use of this source code is governed by a BSD-style license that can be +# found in the LICENSE file. +# + +include $(TOPDIR)/config.mk + +LIB := $(obj)libtpm.o + +COBJS-$(CONFIG_GENERIC_LPC_TPM) = generic_lpc_tpm.o + +COBJS := $(COBJS-y) +SRCS := $(COBJS:.o=.c) +OBJS := $(addprefix $(obj),$(COBJS)) + +all: $(LIB) + +$(LIB): $(obj).depend $(OBJS) + $(call cmd_link_o_target, $(OBJS)) + +######################################################################### + +include $(SRCTREE)/rules.mk + +sinclude $(obj).depend + +######################################################################### diff --git a/drivers/tpm/generic_lpc_tpm.c b/drivers/tpm/generic_lpc_tpm.c new file mode 100644 index 0000000..8319286 --- /dev/null +++ b/drivers/tpm/generic_lpc_tpm.c @@ -0,0 +1,488 @@ +/* + * Copyright (c) 2011 The Chromium OS Authors. + * Released under the 2-clause BSD license. + * + * 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 + */ + +/* + * The code in this file is based on the article "Writing a TPM Device Driver" + * published on http://ptgmedia.pearsoncmg.com. + * + * One principal difference is that in the simplest config the other than 0 + * TPM localities do not get mapped by some devices (for instance, by Infineon + * slb9635), so this driver provides access to locality 0 only. + */ + +#include <common.h> +#include <asm/io.h> +#include <tpm.h> + +#define PREFIX "lpc_tpm: " + +#define TPM_TOTAL_LOCALITIES 5 +struct tpm_locality { + u32 access; + u8 padding0[4]; + u32 int_enable; + u8 vector; + u8 padding1[3]; + u32 int_status; + u32 int_capability; + u32 tpm_status; + u8 padding2[8]; + u8 data; + u8 padding3[3803]; + u32 did_vid; + u8 rid; + u8 padding4[251]; +}; + +struct lpc_tpm { + struct tpm_locality locality[TPM_TOTAL_LOCALITIES]; +}; + +static struct lpc_tpm *lpc_tpm_dev = + (struct lpc_tpm *)CONFIG_TPM_TIS_BASE_ADDRESS; + +/* Some registers' bit field definitions */ +#define TIS_STS_VALID (1 << 7) /* 0x80 */ +#define TIS_STS_COMMAND_READY (1 << 6) /* 0x40 */ +#define TIS_STS_TPM_GO (1 << 5) /* 0x20 */ +#define TIS_STS_DATA_AVAILABLE (1 << 4) /* 0x10 */ +#define TIS_STS_EXPECT (1 << 3) /* 0x08 */ +#define TIS_STS_RESPONSE_RETRY (1 << 1) /* 0x02 */ + +#define TIS_ACCESS_TPM_REG_VALID_STS (1 << 7) /* 0x80 */ +#define TIS_ACCESS_ACTIVE_LOCALITY (1 << 5) /* 0x20 */ +#define TIS_ACCESS_BEEN_SEIZED (1 << 4) /* 0x10 */ +#define TIS_ACCESS_SEIZE (1 << 3) /* 0x08 */ +#define TIS_ACCESS_PENDING_REQUEST (1 << 2) /* 0x04 */ +#define TIS_ACCESS_REQUEST_USE (1 << 1) /* 0x02 */ +#define TIS_ACCESS_TPM_ESTABLISHMENT (1 << 0) /* 0x01 */ + +#define TIS_STS_BURST_COUNT_MASK (0xffff) +#define TIS_STS_BURST_COUNT_SHIFT (8) + +/* + * Error value returned if a tpm register does not enter the expected state + * after continuous polling. No actual TPM register reading ever returns ~0, + * so this value is a safe error indication to be mixed with possible status + * register values. + */ +#define TPM_TIMEOUT_ERR (~0) + +/* Error value returned on various TPM driver errors */ +#define TPM_DRIVER_ERR (-1) + + /* 1 second is plenty for anything TPM does.*/ +#define MAX_DELAY_US (1000 * 1000) + +/* Retrieve burst count value out of the status register contents. */ +#define BURST_COUNT(status) ((u16)(((status) >> TIS_STS_BURST_COUNT_SHIFT) & \ + TIS_STS_BURST_COUNT_MASK)) + +/* + * Structures defined below allow creating descriptions of TPM vendor/device + * ID information for run time discovery. The only device the system knows + * about at this time is Infineon slb9635 + */ +struct device_name { + u16 dev_id; + const char * const dev_name; +}; + +struct vendor_name { + u16 vendor_id; + const char *vendor_name; + const struct device_name *dev_names; +}; + +static const struct device_name infineon_devices[] = { + {0xb, "SLB9635 TT 1.2"}, + {0} +}; + +static const struct vendor_name vendor_names[] = { + {0x15d1, "Infineon", infineon_devices}, +}; + +/* + * Cached vendor/device ID pair to indicate that the device has been already + * discovered + */ +static u32 vendor_dev_id; + +/* TPM access going through macros to make tracing easier. */ +#define tpm_read(ptr) ({ \ + u32 __ret; \ + __ret = (sizeof(*ptr) == 1) ? readb(ptr) : readl(ptr); \ + debug(PREFIX "Read reg 0x%x returns 0x%x\n", \ + (u32)ptr - (u32)lpc_tpm_dev, __ret); \ + __ret; }) + +#define tpm_write(value, ptr) ({ \ + u32 __v = value; \ + debug(PREFIX "Write reg 0x%x with 0x%x\n", \ + (u32)ptr - (u32)lpc_tpm_dev, __v); \ + if (sizeof(*ptr) == 1) \ + writeb(__v, ptr); \ + else \ + writel(__v, ptr); }) + +/* + * tis_wait_reg() + * + * Wait for at least a second for a register to change its state to match the + * expected state. Normally the transition happens within microseconds. + * + * @reg - the TPM register offset + * @locality - locality + * @mask - bitmask for the bitfield(s) to watch + * @expected - value the field(s) are supposed to be set to + * + * Returns the register contents in case the expected value was found in the + * appropriate register bits, or TPM_TIMEOUT_ERR on timeout. + */ +static u32 tis_wait_reg(u32 *reg, u8 mask, u8 expected) +{ + u32 time_us = MAX_DELAY_US; + + while (time_us > 0) { + u32 value = tpm_read(reg); + if ((value & mask) == expected) + return value; + udelay(1); /* 1 us */ + time_us--; + } + return TPM_TIMEOUT_ERR; +} + +/* + * Probe the TPM device and try determining its manufacturer/device name. + * + * Returns 0 on success (the device is found or was found during an earlier + * invocation) or TPM_DRIVER_ERR if the device is not found. + */ +static u32 tis_probe(void) +{ + u32 didvid = tpm_read(&lpc_tpm_dev->locality[0].did_vid); + int i; + const char *device_name = "unknown"; + const char *vendor_name = device_name; + u16 vid, did; + + if (vendor_dev_id) + return 0; /* Already probed. */ + + if (!didvid || (didvid == 0xffffffff)) { + printf("%s: No TPM device found\n", __func__); + return TPM_DRIVER_ERR; + } + + vendor_dev_id = didvid; + + vid = didvid & 0xffff; + did = (didvid >> 16) & 0xffff; + for (i = 0; i < ARRAY_SIZE(vendor_names); i++) { + int j = 0; + u16 known_did; + if (vid == vendor_names[i].vendor_id) + vendor_name = vendor_names[i].vendor_name; + + while ((known_did = vendor_names[i].dev_names[j].dev_id) != 0) { + if (known_did == did) { + device_name = + vendor_names[i].dev_names[j].dev_name; + break; + } + j++; + } + break; + } + + printf("Found TPM %s by %s\n", device_name, vendor_name); + return 0; +} + +/* + * tis_senddata() + * + * send the passed in data to the TPM device. + * + * @data - address of the data to send, byte by byte + * @len - length of the data to send + * + * Returns 0 on success, TPM_DRIVER_ERR on error (in case the device does + * not accept the entire command). + */ +static u32 tis_senddata(const u8 * const data, u32 len) +{ + u32 offset = 0; + u16 burst = 0; + u32 max_cycles = 0; + u8 locality = 0; + u32 value; + + value = tis_wait_reg(&lpc_tpm_dev->locality[locality].tpm_status, + TIS_STS_COMMAND_READY, TIS_STS_COMMAND_READY); + if (value == TPM_TIMEOUT_ERR) { + printf("%s:%d - failed to get 'command_ready' status\n", + __FILE__, __LINE__); + return TPM_DRIVER_ERR; + } + burst = BURST_COUNT(value); + + while (1) { + unsigned count; + + /* Wait till the device is ready to accept more data. */ + while (!burst) { + if (max_cycles++ == MAX_DELAY_US) { + printf("%s:%d failed to feed %d bytes of %d\n", + __FILE__, __LINE__, len - offset, len); + return TPM_DRIVER_ERR; + } + udelay(1); + burst = BURST_COUNT(tpm_read(&lpc_tpm_dev->locality + [locality].tpm_status)); + } + + max_cycles = 0; + + /* + * Calculate number of bytes the TPM is ready to accept in one + * shot. + * + * We want to send the last byte outside of the loop (hence + * the -1 below) to make sure that the 'expected' status bit + * changes to zero exactly after the last byte is fed into the + * FIFO. + */ + count = min(burst, len - offset - 1); + while (count--) + tpm_write(data[offset++], + &lpc_tpm_dev->locality[locality].data); + + value = tis_wait_reg(&lpc_tpm_dev->locality + [locality].tpm_status, + TIS_STS_VALID, TIS_STS_VALID); + + if ((value == TPM_TIMEOUT_ERR) || !(value & TIS_STS_EXPECT)) { + printf("%s:%d TPM command feed overflow\n", + __FILE__, __LINE__); + return TPM_DRIVER_ERR; + } + + burst = BURST_COUNT(value); + if ((offset == (len - 1)) && burst) + /* + * We need to be able to send the last byte to the + * device, so burst size must be nonzero before we + * break out. + */ + break; + } + + /* Send the last byte. */ + tpm_write(data[offset++], &lpc_tpm_dev->locality[locality].data); + /* + * Verify that TPM does not expect any more data as part of this + * command. + */ + value = tis_wait_reg(&lpc_tpm_dev->locality[locality].tpm_status, + TIS_STS_VALID, TIS_STS_VALID); + if ((value == TPM_TIMEOUT_ERR) || (value & TIS_STS_EXPECT)) { + printf("%s:%d unexpected TPM status 0x%x\n", + __FILE__, __LINE__, value); + return TPM_DRIVER_ERR; + } + + /* OK, sitting pretty, let's start the command execution. */ + tpm_write(TIS_STS_TPM_GO, &lpc_tpm_dev->locality[locality].tpm_status); + return 0; +} + +/* + * tis_readresponse() + * + * read the TPM device response after a command was issued. + * + * @buffer - address where to read the response, byte by byte. + * @len - pointer to the size of buffer + * + * On success stores the number of received bytes to len and returns 0. On + * errors (misformatted TPM data or synchronization problems) returns + * TPM_DRIVER_ERR. + */ +static u32 tis_readresponse(u8 *buffer, u32 *len) +{ + u16 burst_count; + u32 status; + u32 offset = 0; + u8 locality = 0; + const u32 has_data = TIS_STS_DATA_AVAILABLE | TIS_STS_VALID; + u32 expected_count = *len; + int max_cycles = 0; + + /* Wait for the TPM to process the command */ + status = tis_wait_reg(&lpc_tpm_dev->locality[locality].tpm_status, + has_data, has_data); + if (status == TPM_TIMEOUT_ERR) { + printf("%s:%d failed processing command\n", + __FILE__, __LINE__); + return TPM_DRIVER_ERR; + } + + do { + while ((burst_count = BURST_COUNT(status)) == 0) { + if (max_cycles++ == MAX_DELAY_US) { + printf("%s:%d TPM stuck on read\n", + __FILE__, __LINE__); + return TPM_DRIVER_ERR; + } + udelay(1); + status = tpm_read(&lpc_tpm_dev->locality + [locality].tpm_status); + } + + max_cycles = 0; + + while (burst_count-- && (offset < expected_count)) { + buffer[offset++] = (u8) tpm_read(&lpc_tpm_dev->locality + [locality].data); + + if (offset == 6) { + /* + * We got the first six bytes of the reply, + * let's figure out how many bytes to expect + * total - it is stored as a 4 byte number in + * network order, starting with offset 2 into + * the body of the reply. + */ + u32 real_length; + memcpy(&real_length, + buffer + 2, + sizeof(real_length)); + expected_count = be32_to_cpu(real_length); + + if ((expected_count < offset) || + (expected_count > *len)) { + printf("%s:%d bad response size %d\n", + __FILE__, __LINE__, + expected_count); + return TPM_DRIVER_ERR; + } + } + } + + /* Wait for the next portion */ + status = tis_wait_reg(&lpc_tpm_dev->locality + [locality].tpm_status, + TIS_STS_VALID, TIS_STS_VALID); + if (status == TPM_TIMEOUT_ERR) { + printf("%s:%d failed to read response\n", + __FILE__, __LINE__); + return TPM_DRIVER_ERR; + } + + if (offset == expected_count) + break; /* We got all we need */ + + } while ((status & has_data) == has_data); + + /* + * Make sure we indeed read all there was. The TIS_STS_VALID bit is + * known to be set. + */ + if (status & TIS_STS_DATA_AVAILABLE) { + printf("%s:%d wrong receive status %x\n", + __FILE__, __LINE__, status); + return TPM_DRIVER_ERR; + } + + /* Tell the TPM that we are done. */ + tpm_write(TIS_STS_COMMAND_READY, &lpc_tpm_dev->locality + [locality].tpm_status); + *len = offset; + return 0; +} + +int tis_init(void) +{ + return tis_probe(); +} + +int tis_open(void) +{ + u8 locality = 0; /* we use locality zero for everything */ + + if (tis_close()) + return TPM_DRIVER_ERR; + + /* now request access to locality */ + tpm_write(TIS_ACCESS_REQUEST_USE, + &lpc_tpm_dev->locality[locality].access); + + + /* did we get a lock? */ + if (tis_wait_reg(&lpc_tpm_dev->locality[locality].access, + TIS_ACCESS_ACTIVE_LOCALITY, + TIS_ACCESS_ACTIVE_LOCALITY) == TPM_TIMEOUT_ERR) { + printf("%s:%d - failed to lock locality %d\n", + __FILE__, __LINE__, locality); + return TPM_DRIVER_ERR; + } + + tpm_write(TIS_STS_COMMAND_READY, + &lpc_tpm_dev->locality[locality].tpm_status); + return 0; +} + +int tis_close(void) +{ + u8 locality = 0; + + if (tpm_read(&lpc_tpm_dev->locality[locality].access) & + TIS_ACCESS_ACTIVE_LOCALITY) { + tpm_write(TIS_ACCESS_ACTIVE_LOCALITY, + &lpc_tpm_dev->locality[locality].access); + + if (tis_wait_reg(&lpc_tpm_dev->locality[locality].access, + TIS_ACCESS_ACTIVE_LOCALITY, 0) == + TPM_TIMEOUT_ERR) { + printf("%s:%d - failed to release locality %d\n", + __FILE__, __LINE__, locality); + return TPM_DRIVER_ERR; + } + } + return 0; +} + +int tis_sendrecv(const u8 *sendbuf, size_t send_size, + u8 *recvbuf, size_t *recv_len) +{ + if (tis_senddata(sendbuf, send_size)) { + printf("%s:%d failed sending data to TPM\n", + __FILE__, __LINE__); + return TPM_DRIVER_ERR; + } + + return tis_readresponse(recvbuf, recv_len); +} diff --git a/include/tpm.h b/include/tpm.h new file mode 100644 index 0000000..febf248 --- /dev/null +++ b/include/tpm.h @@ -0,0 +1,54 @@ +/* Copyright (c) 2011 The Chromium OS Authors. All rights reserved. + * Use of this source code is governed by a BSD-style license that can be + * found in the LICENSE file. + */ + +#ifndef _INCLUDE_TPM_H_ +#define _INCLUDE_TPM_H_ + +#include <common.h> + +/* + * tis_init() + * + * Initialize the TPM device. Returns 0 on success or -1 on + * failure (in case device probing did not succeed). + */ +int tis_init(void); + +/* + * tis_open() + * + * Requests access to locality 0 for the caller. After all commands have been + * completed the caller is supposed to call tis_close(). + * + * Returns 0 on success, -1 on failure. + */ +int tis_open(void); + +/* + * tis_close() + * + * terminate the currect session with the TPM by releasing the locked + * locality. Returns 0 on success of -1 on failure (in case lock + * removal did not succeed). + */ +int tis_close(void); + +/* + * tis_sendrecv() + * + * Send the requested data to the TPM and then try to get its response + * + * @sendbuf - buffer of the data to send + * @send_size size of the data to send + * @recvbuf - memory to save the response to + * @recv_len - pointer to the size of the response buffer + * + * Returns 0 on success (and places the number of response bytes at recv_len) + * or -1 on failure. + */ +int tis_sendrecv(const uint8_t *sendbuf, size_t send_size, uint8_t *recvbuf, + size_t *recv_len); + +#endif /* _INCLUDE_TPM_H_ */
On Saturday, October 15, 2011 05:38:50 AM Vadim Bendebury wrote:
TPM (Trusted Platform Module) is an integrated circuit and software platform that provides computer manufacturers with the core components of a subsystem used to assure authenticity, integrity and confidentiality.
[...]
Quick points: * The license * Use debug() when fitting
diff --git a/drivers/tpm/generic_lpc_tpm.c b/drivers/tpm/generic_lpc_tpm.c new file mode 100644 index 0000000..8319286 --- /dev/null +++ b/drivers/tpm/generic_lpc_tpm.c
[...]
+struct lpc_tpm {
- struct tpm_locality locality[TPM_TOTAL_LOCALITIES];
+};
Do you need such envelope ?
+static struct lpc_tpm *lpc_tpm_dev =
- (struct lpc_tpm *)CONFIG_TPM_TIS_BASE_ADDRESS;
+/* Some registers' bit field definitions */ +#define TIS_STS_VALID (1 << 7) /* 0x80 */ +#define TIS_STS_COMMAND_READY (1 << 6) /* 0x40 */ +#define TIS_STS_TPM_GO (1 << 5) /* 0x20 */ +#define TIS_STS_DATA_AVAILABLE (1 << 4) /* 0x10 */ +#define TIS_STS_EXPECT (1 << 3) /* 0x08 */ +#define TIS_STS_RESPONSE_RETRY (1 << 1) /* 0x02 */
+#define TIS_ACCESS_TPM_REG_VALID_STS (1 << 7) /* 0x80 */ +#define TIS_ACCESS_ACTIVE_LOCALITY (1 << 5) /* 0x20 */ +#define TIS_ACCESS_BEEN_SEIZED (1 << 4) /* 0x10 */ +#define TIS_ACCESS_SEIZE (1 << 3) /* 0x08 */ +#define TIS_ACCESS_PENDING_REQUEST (1 << 2) /* 0x04 */ +#define TIS_ACCESS_REQUEST_USE (1 << 1) /* 0x02 */ +#define TIS_ACCESS_TPM_ESTABLISHMENT (1 << 0) /* 0x01 */
+#define TIS_STS_BURST_COUNT_MASK (0xffff) +#define TIS_STS_BURST_COUNT_SHIFT (8)
+/*
- Error value returned if a tpm register does not enter the expected
state + * after continuous polling. No actual TPM register reading ever returns ~0, + * so this value is a safe error indication to be mixed with possible status + * register values.
- */
+#define TPM_TIMEOUT_ERR (~0)
+/* Error value returned on various TPM driver errors */
Dot missing at the end.
+#define TPM_DRIVER_ERR (-1)
- /* 1 second is plenty for anything TPM does.*/
+#define MAX_DELAY_US (1000 * 1000)
+/* Retrieve burst count value out of the status register contents. */ +#define BURST_COUNT(status) ((u16)(((status) >> TIS_STS_BURST_COUNT_SHIFT) & \ + TIS_STS_BURST_COUNT_MASK))
Do you need the cast ?
+/*
- Structures defined below allow creating descriptions of TPM
vendor/device + * ID information for run time discovery. The only device the system knows + * about at this time is Infineon slb9635
- */
+struct device_name {
- u16 dev_id;
- const char * const dev_name;
+};
+struct vendor_name {
- u16 vendor_id;
- const char *vendor_name;
- const struct device_name *dev_names;
+};
+static const struct device_name infineon_devices[] = {
- {0xb, "SLB9635 TT 1.2"},
- {0}
+};
+static const struct vendor_name vendor_names[] = {
- {0x15d1, "Infineon", infineon_devices},
+};
+/*
- Cached vendor/device ID pair to indicate that the device has been
already + * discovered
- */
+static u32 vendor_dev_id;
+/* TPM access going through macros to make tracing easier. */ +#define tpm_read(ptr) ({ \
- u32 __ret; \
- __ret = (sizeof(*ptr) == 1) ? readb(ptr) : readl(ptr); \
debug(PREFIX "Read reg 0x%x returns 0x%x\n", \(u32)ptr - (u32)lpc_tpm_dev, __ret); \__ret; })
Make this inline function
+#define tpm_write(value, ptr) ({ \
- u32 __v = value; \
- debug(PREFIX "Write reg 0x%x with 0x%x\n", \
(u32)ptr - (u32)lpc_tpm_dev, __v); \- if (sizeof(*ptr) == 1) \
writeb(__v, ptr); \- else \
writel(__v, ptr); })
DTTO
[...]
+static u32 tis_senddata(const u8 * const data, u32 len) +{
- u32 offset = 0;
- u16 burst = 0;
- u32 max_cycles = 0;
- u8 locality = 0;
- u32 value;
- value = tis_wait_reg(&lpc_tpm_dev->locality[locality].tpm_status,
TIS_STS_COMMAND_READY, TIS_STS_COMMAND_READY);- if (value == TPM_TIMEOUT_ERR) {
printf("%s:%d - failed to get 'command_ready' status\n",__FILE__, __LINE__);return TPM_DRIVER_ERR;- }
- burst = BURST_COUNT(value);
- while (1) {
No endless loops please.
unsigned count;/* Wait till the device is ready to accept more data. */while (!burst) {if (max_cycles++ == MAX_DELAY_US) {printf("%s:%d failed to feed %d bytes of %d\n",__FILE__, __LINE__, len - offset, len);return TPM_DRIVER_ERR;}udelay(1);burst = BURST_COUNT(tpm_read(&lpc_tpm_dev->locality[locality].tpm_status));}max_cycles = 0;/** Calculate number of bytes the TPM is ready to accept in one* shot.** We want to send the last byte outside of the loop (hence* the -1 below) to make sure that the 'expected' status bit* changes to zero exactly after the last byte is fed into the* FIFO.*/count = min(burst, len - offset - 1);while (count--)tpm_write(data[offset++],&lpc_tpm_dev->locality[locality].data);value = tis_wait_reg(&lpc_tpm_dev->locality[locality].tpm_status,TIS_STS_VALID, TIS_STS_VALID);if ((value == TPM_TIMEOUT_ERR) || !(value & TIS_STS_EXPECT)) {printf("%s:%d TPM command feed overflow\n",__FILE__, __LINE__);return TPM_DRIVER_ERR;}burst = BURST_COUNT(value);if ((offset == (len - 1)) && burst)/** We need to be able to send the last byte to the* device, so burst size must be nonzero before we* break out.*/break;- }
- /* Send the last byte. */
- tpm_write(data[offset++], &lpc_tpm_dev->locality[locality].data);
- /*
* Verify that TPM does not expect any more data as part of this* command.*/- value = tis_wait_reg(&lpc_tpm_dev->locality[locality].tpm_status,
TIS_STS_VALID, TIS_STS_VALID);- if ((value == TPM_TIMEOUT_ERR) || (value & TIS_STS_EXPECT)) {
printf("%s:%d unexpected TPM status 0x%x\n",__FILE__, __LINE__, value);return TPM_DRIVER_ERR;- }
- /* OK, sitting pretty, let's start the command execution. */
- tpm_write(TIS_STS_TPM_GO, &lpc_tpm_dev->locality[locality].tpm_status);
- return 0;
+}
+/*
- tis_readresponse()
- read the TPM device response after a command was issued.
- @buffer - address where to read the response, byte by byte.
- @len - pointer to the size of buffer
- On success stores the number of received bytes to len and returns 0. On
- errors (misformatted TPM data or synchronization problems) returns
- TPM_DRIVER_ERR.
- */
+static u32 tis_readresponse(u8 *buffer, u32 *len) +{
- u16 burst_count;
- u32 status;
- u32 offset = 0;
- u8 locality = 0;
- const u32 has_data = TIS_STS_DATA_AVAILABLE | TIS_STS_VALID;
- u32 expected_count = *len;
- int max_cycles = 0;
- /* Wait for the TPM to process the command */
- status = tis_wait_reg(&lpc_tpm_dev->locality[locality].tpm_status,
has_data, has_data);- if (status == TPM_TIMEOUT_ERR) {
Just make it return non-zero for timeout and check for non-zero. And unify the variable names please.
printf("%s:%d failed processing command\n",__FILE__, __LINE__);return TPM_DRIVER_ERR;- }
- do {
while ((burst_count = BURST_COUNT(status)) == 0) {if (max_cycles++ == MAX_DELAY_US) {printf("%s:%d TPM stuck on read\n",__FILE__, __LINE__);return TPM_DRIVER_ERR;}udelay(1);status = tpm_read(&lpc_tpm_dev->locality[locality].tpm_status);}max_cycles = 0;while (burst_count-- && (offset < expected_count)) {buffer[offset++] = (u8) tpm_read(&lpc_tpm_dev->locality[locality].data);if (offset == 6) {/** We got the first six bytes of the reply,* let's figure out how many bytes to expect* total - it is stored as a 4 byte number in* network order, starting with offset 2 into* the body of the reply.*/u32 real_length;memcpy(&real_length,buffer + 2,sizeof(real_length));expected_count = be32_to_cpu(real_length);if ((expected_count < offset) ||(expected_count > *len)) {printf("%s:%d bad response size %d\n",__FILE__, __LINE__,expected_count);return TPM_DRIVER_ERR;}}}/* Wait for the next portion */status = tis_wait_reg(&lpc_tpm_dev->locality[locality].tpm_status,TIS_STS_VALID, TIS_STS_VALID);if (status == TPM_TIMEOUT_ERR) {printf("%s:%d failed to read response\n",__FILE__, __LINE__);return TPM_DRIVER_ERR;}if (offset == expected_count)break; /* We got all we need */- } while ((status & has_data) == has_data);
No endless loop please.
- /*
* Make sure we indeed read all there was. The TIS_STS_VALID bit is* known to be set.*/- if (status & TIS_STS_DATA_AVAILABLE) {
printf("%s:%d wrong receive status %x\n",__FILE__, __LINE__, status);return TPM_DRIVER_ERR;- }
- /* Tell the TPM that we are done. */
- tpm_write(TIS_STS_COMMAND_READY, &lpc_tpm_dev->locality
[locality].tpm_status);- *len = offset;
- return 0;
+}
+int tis_init(void) +{
- return tis_probe();
+}
Make tis_probe into tis_init and be done with it ?
+int tis_open(void) +{
- u8 locality = 0; /* we use locality zero for everything */
- if (tis_close())
return TPM_DRIVER_ERR;- /* now request access to locality */
- tpm_write(TIS_ACCESS_REQUEST_USE,
&lpc_tpm_dev->locality[locality].access);- /* did we get a lock? */
- if (tis_wait_reg(&lpc_tpm_dev->locality[locality].access,
TIS_ACCESS_ACTIVE_LOCALITY,TIS_ACCESS_ACTIVE_LOCALITY) == TPM_TIMEOUT_ERR) {printf("%s:%d - failed to lock locality %d\n",__FILE__, __LINE__, locality);return TPM_DRIVER_ERR;- }
- tpm_write(TIS_STS_COMMAND_READY,
&lpc_tpm_dev->locality[locality].tpm_status);- return 0;
+}
[...]
Cheers
Dear Marek Vasut,
thank you for your comments, please see below:
On Sat, Oct 15, 2011 at 11:08 AM, Marek Vasut marek.vasut@gmail.com wrote:
On Saturday, October 15, 2011 05:38:50 AM Vadim Bendebury wrote:
TPM (Trusted Platform Module) is an integrated circuit and software platform that provides computer manufacturers with the core components of a subsystem used to assure authenticity, integrity and confidentiality.
[...]
Quick points:
- The license
Please suggest the appropriate file header text.
- Use debug() when fitting
It seems to me debug/puts/printf are used where appropriate - do you have some cases in mind which need to be changed?
diff --git a/drivers/tpm/generic_lpc_tpm.c b/drivers/tpm/generic_lpc_tpm.c new file mode 100644 index 0000000..8319286 --- /dev/null +++ b/drivers/tpm/generic_lpc_tpm.c
[...]
+struct lpc_tpm {
- struct tpm_locality locality[TPM_TOTAL_LOCALITIES];
+};
Do you need such envelope ?
I think I do - this accurately describes the structure of the chip.
+static struct lpc_tpm *lpc_tpm_dev =
- (struct lpc_tpm *)CONFIG_TPM_TIS_BASE_ADDRESS;
+/* Some registers' bit field definitions */ +#define TIS_STS_VALID (1 << 7) /* 0x80 */ +#define TIS_STS_COMMAND_READY (1 << 6) /* 0x40 */ +#define TIS_STS_TPM_GO (1 << 5) /* 0x20 */ +#define TIS_STS_DATA_AVAILABLE (1 << 4) /* 0x10 */ +#define TIS_STS_EXPECT (1 << 3) /* 0x08 */ +#define TIS_STS_RESPONSE_RETRY (1 << 1) /* 0x02 */
+#define TIS_ACCESS_TPM_REG_VALID_STS (1 << 7) /* 0x80 */ +#define TIS_ACCESS_ACTIVE_LOCALITY (1 << 5) /* 0x20 */ +#define TIS_ACCESS_BEEN_SEIZED (1 << 4) /* 0x10 */ +#define TIS_ACCESS_SEIZE (1 << 3) /* 0x08 */ +#define TIS_ACCESS_PENDING_REQUEST (1 << 2) /* 0x04 */ +#define TIS_ACCESS_REQUEST_USE (1 << 1) /* 0x02 */ +#define TIS_ACCESS_TPM_ESTABLISHMENT (1 << 0) /* 0x01 */
+#define TIS_STS_BURST_COUNT_MASK (0xffff) +#define TIS_STS_BURST_COUNT_SHIFT (8)
+/*
- Error value returned if a tpm register does not enter the expected
state + * after continuous polling. No actual TPM register reading ever returns ~0, + * so this value is a safe error indication to be mixed with possible status + * register values.
- */
+#define TPM_TIMEOUT_ERR (~0)
+/* Error value returned on various TPM driver errors */
Dot missing at the end.
ok.
+#define TPM_DRIVER_ERR (-1)
- /* 1 second is plenty for anything TPM does.*/
+#define MAX_DELAY_US (1000 * 1000)
+/* Retrieve burst count value out of the status register contents. */ +#define BURST_COUNT(status) ((u16)(((status) >> TIS_STS_BURST_COUNT_SHIFT) & \ + TIS_STS_BURST_COUNT_MASK))
Do you need the cast ?
I think it demonstrates the intentional truncation of the value, it gets assigned to u16 values down the road, prevents compiler warnings about assigning incompatible values in some cases.
+/*
- Structures defined below allow creating descriptions of TPM
vendor/device + * ID information for run time discovery. The only device the system knows + * about at this time is Infineon slb9635
- */
+struct device_name {
- u16 dev_id;
- const char * const dev_name;
+};
+struct vendor_name {
- u16 vendor_id;
- const char *vendor_name;
- const struct device_name *dev_names;
+};
+static const struct device_name infineon_devices[] = {
- {0xb, "SLB9635 TT 1.2"},
- {0}
+};
+static const struct vendor_name vendor_names[] = {
- {0x15d1, "Infineon", infineon_devices},
+};
+/*
- Cached vendor/device ID pair to indicate that the device has been
already + * discovered
- */
+static u32 vendor_dev_id;
+/* TPM access going through macros to make tracing easier. */ +#define tpm_read(ptr) ({ \
- u32 __ret; \
- __ret = (sizeof(*ptr) == 1) ? readb(ptr) : readl(ptr); \
- debug(PREFIX "Read reg 0x%x returns 0x%x\n", \
- (u32)ptr - (u32)lpc_tpm_dev, __ret); \
- __ret; })
Make this inline function
+#define tpm_write(value, ptr) ({ \
- u32 __v = value; \
- debug(PREFIX "Write reg 0x%x with 0x%x\n", \
- (u32)ptr - (u32)lpc_tpm_dev, __v); \
- if (sizeof(*ptr) == 1) \
- writeb(__v, ptr); \
- else \
- writel(__v, ptr); })
DTTO
Are you sure these will work as inline functions?
[...]
+static u32 tis_senddata(const u8 * const data, u32 len) +{
- u32 offset = 0;
- u16 burst = 0;
- u32 max_cycles = 0;
- u8 locality = 0;
- u32 value;
- value = tis_wait_reg(&lpc_tpm_dev->locality[locality].tpm_status,
- TIS_STS_COMMAND_READY, TIS_STS_COMMAND_READY);
- if (value == TPM_TIMEOUT_ERR) {
- printf("%s:%d - failed to get 'command_ready' status\n",
- __FILE__, __LINE__);
- return TPM_DRIVER_ERR;
- }
- burst = BURST_COUNT(value);
- while (1) {
No endless loops please.
You are the third person looking at this, but the first one uncomfortable with this. Obviously this is not an endless loop, there are three points where the loop terminates, two on timeout errors and one on success.
- unsigned count;
- /* Wait till the device is ready to accept more data. */
- while (!burst) {
- if (max_cycles++ == MAX_DELAY_US) {
- printf("%s:%d failed to feed %d bytes of %d\n",
- __FILE__, __LINE__, len - offset, len);
- return TPM_DRIVER_ERR;
- }
- udelay(1);
- burst = BURST_COUNT(tpm_read(&lpc_tpm_dev->locality
- [locality].tpm_status));
- }
- max_cycles = 0;
- /*
- * Calculate number of bytes the TPM is ready to accept in one
- * shot.
- *
- * We want to send the last byte outside of the loop (hence
- * the -1 below) to make sure that the 'expected' status bit
- * changes to zero exactly after the last byte is fed into the
- * FIFO.
- */
- count = min(burst, len - offset - 1);
- while (count--)
- tpm_write(data[offset++],
- &lpc_tpm_dev->locality[locality].data);
- value = tis_wait_reg(&lpc_tpm_dev->locality
- [locality].tpm_status,
- TIS_STS_VALID, TIS_STS_VALID);
- if ((value == TPM_TIMEOUT_ERR) || !(value & TIS_STS_EXPECT)) {
- printf("%s:%d TPM command feed overflow\n",
- __FILE__, __LINE__);
- return TPM_DRIVER_ERR;
- }
- burst = BURST_COUNT(value);
- if ((offset == (len - 1)) && burst)
- /*
- * We need to be able to send the last byte to the
- * device, so burst size must be nonzero before we
- * break out.
- */
- break;
- }
- /* Send the last byte. */
- tpm_write(data[offset++], &lpc_tpm_dev->locality[locality].data);
- /*
- * Verify that TPM does not expect any more data as part of this
- * command.
- */
- value = tis_wait_reg(&lpc_tpm_dev->locality[locality].tpm_status,
- TIS_STS_VALID, TIS_STS_VALID);
- if ((value == TPM_TIMEOUT_ERR) || (value & TIS_STS_EXPECT)) {
- printf("%s:%d unexpected TPM status 0x%x\n",
- __FILE__, __LINE__, value);
- return TPM_DRIVER_ERR;
- }
- /* OK, sitting pretty, let's start the command execution. */
- tpm_write(TIS_STS_TPM_GO, &lpc_tpm_dev->locality[locality].tpm_status);
- return 0;
+}
+/*
- tis_readresponse()
- read the TPM device response after a command was issued.
- @buffer - address where to read the response, byte by byte.
- @len - pointer to the size of buffer
- On success stores the number of received bytes to len and returns 0. On
- errors (misformatted TPM data or synchronization problems) returns
- TPM_DRIVER_ERR.
- */
+static u32 tis_readresponse(u8 *buffer, u32 *len) +{
- u16 burst_count;
- u32 status;
- u32 offset = 0;
- u8 locality = 0;
- const u32 has_data = TIS_STS_DATA_AVAILABLE | TIS_STS_VALID;
- u32 expected_count = *len;
- int max_cycles = 0;
- /* Wait for the TPM to process the command */
- status = tis_wait_reg(&lpc_tpm_dev->locality[locality].tpm_status,
- has_data, has_data);
- if (status == TPM_TIMEOUT_ERR) {
Just make it return non-zero for timeout and check for non-zero. And unify the variable names please.
What variable names are you referring to, can you please elaborate.
- printf("%s:%d failed processing command\n",
- __FILE__, __LINE__);
- return TPM_DRIVER_ERR;
- }
- do {
- while ((burst_count = BURST_COUNT(status)) == 0) {
- if (max_cycles++ == MAX_DELAY_US) {
- printf("%s:%d TPM stuck on read\n",
- __FILE__, __LINE__);
- return TPM_DRIVER_ERR;
- }
- udelay(1);
- status = tpm_read(&lpc_tpm_dev->locality
- [locality].tpm_status);
- }
- max_cycles = 0;
- while (burst_count-- && (offset < expected_count)) {
- buffer[offset++] = (u8) tpm_read(&lpc_tpm_dev->locality
- [locality].data);
- if (offset == 6) {
- /*
- * We got the first six bytes of the reply,
- * let's figure out how many bytes to expect
- * total - it is stored as a 4 byte number in
- * network order, starting with offset 2 into
- * the body of the reply.
- */
- u32 real_length;
- memcpy(&real_length,
- buffer + 2,
- sizeof(real_length));
- expected_count = be32_to_cpu(real_length);
- if ((expected_count < offset) ||
- (expected_count > *len)) {
- printf("%s:%d bad response size %d\n",
- __FILE__, __LINE__,
- expected_count);
- return TPM_DRIVER_ERR;
- }
- }
- }
- /* Wait for the next portion */
- status = tis_wait_reg(&lpc_tpm_dev->locality
- [locality].tpm_status,
- TIS_STS_VALID, TIS_STS_VALID);
- if (status == TPM_TIMEOUT_ERR) {
- printf("%s:%d failed to read response\n",
- __FILE__, __LINE__);
- return TPM_DRIVER_ERR;
- }
- if (offset == expected_count)
- break; /* We got all we need */
- } while ((status & has_data) == has_data);
No endless loop please.
I am not sure why you call this endless - it terminates on a few events. The thing is that it is not even known in advance how many cycles the loop will have to run, but it has necessary stop gaps to prevent it from running forever.
- /*
- * Make sure we indeed read all there was. The TIS_STS_VALID bit is
- * known to be set.
- */
- if (status & TIS_STS_DATA_AVAILABLE) {
- printf("%s:%d wrong receive status %x\n",
- __FILE__, __LINE__, status);
- return TPM_DRIVER_ERR;
- }
- /* Tell the TPM that we are done. */
- tpm_write(TIS_STS_COMMAND_READY, &lpc_tpm_dev->locality
- [locality].tpm_status);
- *len = offset;
- return 0;
+}
+int tis_init(void) +{
- return tis_probe();
+}
Make tis_probe into tis_init and be done with it ?
ok
+int tis_open(void) +{
- u8 locality = 0; /* we use locality zero for everything */
- if (tis_close())
- return TPM_DRIVER_ERR;
- /* now request access to locality */
- tpm_write(TIS_ACCESS_REQUEST_USE,
- &lpc_tpm_dev->locality[locality].access);
- /* did we get a lock? */
- if (tis_wait_reg(&lpc_tpm_dev->locality[locality].access,
- TIS_ACCESS_ACTIVE_LOCALITY,
- TIS_ACCESS_ACTIVE_LOCALITY) == TPM_TIMEOUT_ERR) {
- printf("%s:%d - failed to lock locality %d\n",
- __FILE__, __LINE__, locality);
- return TPM_DRIVER_ERR;
- }
- tpm_write(TIS_STS_COMMAND_READY,
- &lpc_tpm_dev->locality[locality].tpm_status);
- return 0;
+}
[...]
Cheers
cheers, /vb
On Saturday, October 15, 2011 08:47:39 PM Vadim Bendebury wrote:
Dear Marek Vasut,
thank you for your comments, please see below:
On Sat, Oct 15, 2011 at 11:08 AM, Marek Vasut marek.vasut@gmail.com wrote:
On Saturday, October 15, 2011 05:38:50 AM Vadim Bendebury wrote:
TPM (Trusted Platform Module) is an integrated circuit and software platform that provides computer manufacturers with the core components of a subsystem used to assure authenticity, integrity and confidentiality.
[...]
Quick points:
- The license
Please suggest the appropriate file header text.
Uh ... you should know the license !!!
- Use debug() when fitting
It seems to me debug/puts/printf are used where appropriate - do you have some cases in mind which need to be changed?
Ok
diff --git a/drivers/tpm/generic_lpc_tpm.c b/drivers/tpm/generic_lpc_tpm.c new file mode 100644 index 0000000..8319286 --- /dev/null +++ b/drivers/tpm/generic_lpc_tpm.c
[...]
+struct lpc_tpm {
struct tpm_locality locality[TPM_TOTAL_LOCALITIES];+};
Do you need such envelope ?
I think I do - this accurately describes the structure of the chip.
There's just one member ... it's weird?
+static struct lpc_tpm *lpc_tpm_dev =
(struct lpc_tpm *)CONFIG_TPM_TIS_BASE_ADDRESS;+/* Some registers' bit field definitions */ +#define TIS_STS_VALID (1 << 7) /* 0x80 */ +#define TIS_STS_COMMAND_READY (1 << 6) /* 0x40 */ +#define TIS_STS_TPM_GO (1 << 5) /* 0x20 */ +#define TIS_STS_DATA_AVAILABLE (1 << 4) /* 0x10 */ +#define TIS_STS_EXPECT (1 << 3) /* 0x08 */ +#define TIS_STS_RESPONSE_RETRY (1 << 1) /* 0x02 */
+#define TIS_ACCESS_TPM_REG_VALID_STS (1 << 7) /* 0x80 */ +#define TIS_ACCESS_ACTIVE_LOCALITY (1 << 5) /* 0x20 */ +#define TIS_ACCESS_BEEN_SEIZED (1 << 4) /* 0x10 */ +#define TIS_ACCESS_SEIZE (1 << 3) /* 0x08 */ +#define TIS_ACCESS_PENDING_REQUEST (1 << 2) /* 0x04 */ +#define TIS_ACCESS_REQUEST_USE (1 << 1) /* 0x02 */ +#define TIS_ACCESS_TPM_ESTABLISHMENT (1 << 0) /* 0x01 */
+#define TIS_STS_BURST_COUNT_MASK (0xffff) +#define TIS_STS_BURST_COUNT_SHIFT (8)
+/*
- Error value returned if a tpm register does not enter the expected
state + * after continuous polling. No actual TPM register reading ever returns ~0, + * so this value is a safe error indication to be mixed with possible status + * register values.
- */
+#define TPM_TIMEOUT_ERR (~0)
+/* Error value returned on various TPM driver errors */
Dot missing at the end.
ok.
Please fix globally.
+#define TPM_DRIVER_ERR (-1)
- /* 1 second is plenty for anything TPM does.*/
+#define MAX_DELAY_US (1000 * 1000)
+/* Retrieve burst count value out of the status register contents. */ +#define BURST_COUNT(status) ((u16)(((status) >> TIS_STS_BURST_COUNT_SHIFT) & \ + TIS_STS_BURST_COUNT_MASK))
Do you need the cast ?
I think it demonstrates the intentional truncation of the value, it gets assigned to u16 values down the road, prevents compiler warnings about assigning incompatible values in some cases.
Make it an inline function then, this will do the typechecking for you.
+/*
- Structures defined below allow creating descriptions of TPM
vendor/device + * ID information for run time discovery. The only device the system knows + * about at this time is Infineon slb9635
- */
+struct device_name {
u16 dev_id;const char * const dev_name;+};
+struct vendor_name {
u16 vendor_id;const char *vendor_name;const struct device_name *dev_names;+};
+static const struct device_name infineon_devices[] = {
{0xb, "SLB9635 TT 1.2"},{0}+};
+static const struct vendor_name vendor_names[] = {
{0x15d1, "Infineon", infineon_devices},+};
+/*
- Cached vendor/device ID pair to indicate that the device has been
already + * discovered
- */
+static u32 vendor_dev_id;
+/* TPM access going through macros to make tracing easier. */ +#define tpm_read(ptr) ({ \
u32 __ret; \__ret = (sizeof(*ptr) == 1) ? readb(ptr) : readl(ptr); \debug(PREFIX "Read reg 0x%x returns 0x%x\n", \(u32)ptr - (u32)lpc_tpm_dev, __ret); \__ret; })Make this inline function
+#define tpm_write(value, ptr) ({ \
u32 __v = value; \debug(PREFIX "Write reg 0x%x with 0x%x\n", \(u32)ptr - (u32)lpc_tpm_dev, __v); \if (sizeof(*ptr) == 1) \writeb(__v, ptr); \else \writel(__v, ptr); })DTTO
Are you sure these will work as inline functions?
Why not ? Also, why do you introduce the __v ?
[...]
+static u32 tis_senddata(const u8 * const data, u32 len) +{
u32 offset = 0;u16 burst = 0;u32 max_cycles = 0;u8 locality = 0;u32 value;value = tis_wait_reg(&lpc_tpm_dev->locality[locality].tpm_status,TIS_STS_COMMAND_READY,TIS_STS_COMMAND_READY); + if (value == TPM_TIMEOUT_ERR) {
printf("%s:%d - failed to get 'command_ready' status\n",__FILE__, __LINE__);return TPM_DRIVER_ERR;}burst = BURST_COUNT(value);while (1) {No endless loops please.
You are the third person looking at this, but the first one uncomfortable with this. Obviously this is not an endless loop, there are three points where the loop terminates, two on timeout errors and one on success.
So there's no case where this can loop endlessly ? fine.
unsigned count;/* Wait till the device is ready to accept more data. */while (!burst) {if (max_cycles++ == MAX_DELAY_US) {printf("%s:%d failed to feed %d bytes of%d\n", + __FILE__, __LINE__, len - offset, len); + return TPM_DRIVER_ERR;
}udelay(1);burst =BURST_COUNT(tpm_read(&lpc_tpm_dev->locality + [locality].tpm_status)); + }
max_cycles = 0;/** Calculate number of bytes the TPM is ready to accept inone + * shot.
** We want to send the last byte outside of the loop(hence + * the -1 below) to make sure that the 'expected' status bit + * changes to zero exactly after the last byte is fed into the + * FIFO.
*/count = min(burst, len - offset - 1);while (count--)tpm_write(data[offset++],&lpc_tpm_dev->locality[locality].data);value = tis_wait_reg(&lpc_tpm_dev->locality[locality].tpm_status,TIS_STS_VALID, TIS_STS_VALID);if ((value == TPM_TIMEOUT_ERR) || !(value &TIS_STS_EXPECT)) { + printf("%s:%d TPM command feed overflow\n", + __FILE__, __LINE__);
return TPM_DRIVER_ERR;}burst = BURST_COUNT(value);if ((offset == (len - 1)) && burst)/** We need to be able to send the last byte to the* device, so burst size must be nonzero before we* break out.*/break;}/* Send the last byte. */tpm_write(data[offset++], &lpc_tpm_dev->locality[locality].data);/** Verify that TPM does not expect any more data as part of this* command.*/value = tis_wait_reg(&lpc_tpm_dev->locality[locality].tpm_status,TIS_STS_VALID, TIS_STS_VALID);if ((value == TPM_TIMEOUT_ERR) || (value & TIS_STS_EXPECT)) {printf("%s:%d unexpected TPM status 0x%x\n",__FILE__, __LINE__, value);return TPM_DRIVER_ERR;}/* OK, sitting pretty, let's start the command execution. */tpm_write(TIS_STS_TPM_GO,&lpc_tpm_dev->locality[locality].tpm_status); + return 0; +}
+/*
- tis_readresponse()
- read the TPM device response after a command was issued.
- @buffer - address where to read the response, byte by byte.
- @len - pointer to the size of buffer
- On success stores the number of received bytes to len and returns 0.
On + * errors (misformatted TPM data or synchronization problems) returns + * TPM_DRIVER_ERR.
- */
+static u32 tis_readresponse(u8 *buffer, u32 *len) +{
u16 burst_count;u32 status;u32 offset = 0;u8 locality = 0;const u32 has_data = TIS_STS_DATA_AVAILABLE | TIS_STS_VALID;u32 expected_count = *len;int max_cycles = 0;/* Wait for the TPM to process the command */status = tis_wait_reg(&lpc_tpm_dev->locality[locality].tpm_status,has_data, has_data);if (status == TPM_TIMEOUT_ERR) {Just make it return non-zero for timeout and check for non-zero. And unify the variable names please.
What variable names are you referring to, can you please elaborate.
"value" in the previous function, "status" in this one. Why the inconsistence ?
printf("%s:%d failed processing command\n",__FILE__, __LINE__);return TPM_DRIVER_ERR;}do {while ((burst_count = BURST_COUNT(status)) == 0) {if (max_cycles++ == MAX_DELAY_US) {printf("%s:%d TPM stuck on read\n",__FILE__, __LINE__);return TPM_DRIVER_ERR;}udelay(1);status = tpm_read(&lpc_tpm_dev->locality[locality].tpm_status);}max_cycles = 0;while (burst_count-- && (offset < expected_count)) {buffer[offset++] = (u8)tpm_read(&lpc_tpm_dev->locality + [locality].data); +
if (offset == 6) {/** We got the first six bytes of thereply, + * let's figure out how many bytes to expect + * total - it is stored as a 4 byte number in + * network order, starting with offset 2 into + * the body of the reply.
*/u32 real_length;memcpy(&real_length,buffer + 2,sizeof(real_length));expected_count = be32_to_cpu(real_length);if ((expected_count < offset) ||(expected_count > *len)) {printf("%s:%d bad response size%d\n", + __FILE__, __LINE__,
expected_count);return TPM_DRIVER_ERR;}}}/* Wait for the next portion */status = tis_wait_reg(&lpc_tpm_dev->locality[locality].tpm_status,TIS_STS_VALID, TIS_STS_VALID);if (status == TPM_TIMEOUT_ERR) {printf("%s:%d failed to read response\n",__FILE__, __LINE__);return TPM_DRIVER_ERR;}if (offset == expected_count)break; /* We got all we need */} while ((status & has_data) == has_data);No endless loop please.
I am not sure why you call this endless - it terminates on a few events. The thing is that it is not even known in advance how many cycles the loop will have to run, but it has necessary stop gaps to prevent it from running forever.
See above, is it possible for this to hang ? If not, ok.
/** Make sure we indeed read all there was. The TIS_STS_VALID bitis + * known to be set.
*/if (status & TIS_STS_DATA_AVAILABLE) {printf("%s:%d wrong receive status %x\n",__FILE__, __LINE__, status);return TPM_DRIVER_ERR;}/* Tell the TPM that we are done. */tpm_write(TIS_STS_COMMAND_READY, &lpc_tpm_dev->locality[locality].tpm_status);*len = offset;return 0;+}
+int tis_init(void) +{
return tis_probe();+}
Make tis_probe into tis_init and be done with it ?
ok
+int tis_open(void) +{
u8 locality = 0; /* we use locality zero for everything */if (tis_close())return TPM_DRIVER_ERR;/* now request access to locality */tpm_write(TIS_ACCESS_REQUEST_USE,&lpc_tpm_dev->locality[locality].access);/* did we get a lock? */if (tis_wait_reg(&lpc_tpm_dev->locality[locality].access,TIS_ACCESS_ACTIVE_LOCALITY,TIS_ACCESS_ACTIVE_LOCALITY) == TPM_TIMEOUT_ERR) {printf("%s:%d - failed to lock locality %d\n",__FILE__, __LINE__, locality);return TPM_DRIVER_ERR;}tpm_write(TIS_STS_COMMAND_READY,&lpc_tpm_dev->locality[locality].tpm_status);return 0;+}
[...]
Cheers
cheers, /vb
Cheers
On Sat, Oct 15, 2011 at 2:09 PM, Marek Vasut marek.vasut@gmail.com wrote:
On Saturday, October 15, 2011 08:47:39 PM Vadim Bendebury wrote:
Dear Marek Vasut,
thank you for your comments, please see below:
On Sat, Oct 15, 2011 at 11:08 AM, Marek Vasut marek.vasut@gmail.com wrote:
On Saturday, October 15, 2011 05:38:50 AM Vadim Bendebury wrote:
TPM (Trusted Platform Module) is an integrated circuit and software platform that provides computer manufacturers with the core components of a subsystem used to assure authenticity, integrity and confidentiality.
[...]
Quick points:
- The license
Please suggest the appropriate file header text.
Uh ... you should know the license !!!
removed the BSD part
[..]
+struct lpc_tpm {
- struct tpm_locality locality[TPM_TOTAL_LOCALITIES];
+};
Do you need such envelope ?
I think I do - this accurately describes the structure of the chip.
There's just one member ... it's weird?
I think it is appropriate in this case to encapsulate the entire chip description in a structure. Among other things makes it easier to pass a pointer to the chip description around.
[..]
Dot missing at the end.
ok.
Please fix globally.
done
+#define TPM_DRIVER_ERR (-1)
- /* 1 second is plenty for anything TPM does.*/
+#define MAX_DELAY_US (1000 * 1000)
+/* Retrieve burst count value out of the status register contents. */ +#define BURST_COUNT(status) ((u16)(((status) >> TIS_STS_BURST_COUNT_SHIFT) & \ + TIS_STS_BURST_COUNT_MASK))
Do you need the cast ?
I think it demonstrates the intentional truncation of the value, it gets assigned to u16 values down the road, prevents compiler warnings about assigning incompatible values in some cases.
Make it an inline function then, this will do the typechecking for you.
I am not sure what is wrong with a short macro in this case - is this against the coding style?
+/*
- Structures defined below allow creating descriptions of TPM
vendor/device + * ID information for run time discovery. The only device the system knows + * about at this time is Infineon slb9635
- */
+struct device_name {
- u16 dev_id;
- const char * const dev_name;
+};
+struct vendor_name {
- u16 vendor_id;
- const char *vendor_name;
- const struct device_name *dev_names;
+};
+static const struct device_name infineon_devices[] = {
- {0xb, "SLB9635 TT 1.2"},
- {0}
+};
+static const struct vendor_name vendor_names[] = {
- {0x15d1, "Infineon", infineon_devices},
+};
+/*
- Cached vendor/device ID pair to indicate that the device has been
already + * discovered
- */
+static u32 vendor_dev_id;
+/* TPM access going through macros to make tracing easier. */ +#define tpm_read(ptr) ({ \
- u32 __ret; \
- __ret = (sizeof(*ptr) == 1) ? readb(ptr) : readl(ptr); \
- debug(PREFIX "Read reg 0x%x returns 0x%x\n", \
- (u32)ptr - (u32)lpc_tpm_dev, __ret); \
- __ret; })
Make this inline function
+#define tpm_write(value, ptr) ({ \
- u32 __v = value; \
- debug(PREFIX "Write reg 0x%x with 0x%x\n", \
- (u32)ptr - (u32)lpc_tpm_dev, __v); \
- if (sizeof(*ptr) == 1) \
- writeb(__v, ptr); \
- else \
- writel(__v, ptr); })
DTTO
Are you sure these will work as inline functions?
Why not ? Also, why do you introduce the __v ?
macro vs function: need to be able to tell the pointed object size at run time.
__v is needed to avoid side effects when invoking the macro.
[...]
+static u32 tis_senddata(const u8 * const data, u32 len) +{
- u32 offset = 0;
- u16 burst = 0;
- u32 max_cycles = 0;
- u8 locality = 0;
- u32 value;
- value = tis_wait_reg(&lpc_tpm_dev->locality[locality].tpm_status,
- TIS_STS_COMMAND_READY,
TIS_STS_COMMAND_READY); + if (value == TPM_TIMEOUT_ERR) {
- printf("%s:%d - failed to get 'command_ready' status\n",
- __FILE__, __LINE__);
- return TPM_DRIVER_ERR;
- }
- burst = BURST_COUNT(value);
- while (1) {
No endless loops please.
You are the third person looking at this, but the first one uncomfortable with this. Obviously this is not an endless loop, there are three points where the loop terminates, two on timeout errors and one on success.
So there's no case where this can loop endlessly ? fine.
- unsigned count;
- /* Wait till the device is ready to accept more data. */
- while (!burst) {
- if (max_cycles++ == MAX_DELAY_US) {
- printf("%s:%d failed to feed %d bytes of
%d\n", + __FILE__, __LINE__, len - offset, len); + return TPM_DRIVER_ERR;
- }
- udelay(1);
- burst =
BURST_COUNT(tpm_read(&lpc_tpm_dev->locality + [locality].tpm_status)); + }
- max_cycles = 0;
- /*
- * Calculate number of bytes the TPM is ready to accept in
one + * shot.
- *
- * We want to send the last byte outside of the loop
(hence + * the -1 below) to make sure that the 'expected' status bit + * changes to zero exactly after the last byte is fed into the + * FIFO.
- */
- count = min(burst, len - offset - 1);
- while (count--)
- tpm_write(data[offset++],
- &lpc_tpm_dev->locality[locality].data);
- value = tis_wait_reg(&lpc_tpm_dev->locality
- [locality].tpm_status,
- TIS_STS_VALID, TIS_STS_VALID);
- if ((value == TPM_TIMEOUT_ERR) || !(value &
TIS_STS_EXPECT)) { + printf("%s:%d TPM command feed overflow\n", + __FILE__, __LINE__);
- return TPM_DRIVER_ERR;
- }
- burst = BURST_COUNT(value);
- if ((offset == (len - 1)) && burst)
- /*
- * We need to be able to send the last byte to the
- * device, so burst size must be nonzero before we
- * break out.
- */
- break;
- }
- /* Send the last byte. */
- tpm_write(data[offset++], &lpc_tpm_dev->locality[locality].data);
- /*
- * Verify that TPM does not expect any more data as part of this
- * command.
- */
- value = tis_wait_reg(&lpc_tpm_dev->locality[locality].tpm_status,
- TIS_STS_VALID, TIS_STS_VALID);
- if ((value == TPM_TIMEOUT_ERR) || (value & TIS_STS_EXPECT)) {
- printf("%s:%d unexpected TPM status 0x%x\n",
- __FILE__, __LINE__, value);
- return TPM_DRIVER_ERR;
- }
- /* OK, sitting pretty, let's start the command execution. */
- tpm_write(TIS_STS_TPM_GO,
&lpc_tpm_dev->locality[locality].tpm_status); + return 0; +}
+/*
- tis_readresponse()
- read the TPM device response after a command was issued.
- @buffer - address where to read the response, byte by byte.
- @len - pointer to the size of buffer
- On success stores the number of received bytes to len and returns 0.
On + * errors (misformatted TPM data or synchronization problems) returns + * TPM_DRIVER_ERR.
- */
+static u32 tis_readresponse(u8 *buffer, u32 *len) +{
- u16 burst_count;
- u32 status;
- u32 offset = 0;
- u8 locality = 0;
- const u32 has_data = TIS_STS_DATA_AVAILABLE | TIS_STS_VALID;
- u32 expected_count = *len;
- int max_cycles = 0;
- /* Wait for the TPM to process the command */
- status = tis_wait_reg(&lpc_tpm_dev->locality[locality].tpm_status,
- has_data, has_data);
- if (status == TPM_TIMEOUT_ERR) {
Just make it return non-zero for timeout and check for non-zero. And unify the variable names please.
What variable names are you referring to, can you please elaborate.
"value" in the previous function, "status" in this one. Why the inconsistence ?
done
- printf("%s:%d failed processing command\n",
- __FILE__, __LINE__);
- return TPM_DRIVER_ERR;
- }
- do {
- while ((burst_count = BURST_COUNT(status)) == 0) {
- if (max_cycles++ == MAX_DELAY_US) {
- printf("%s:%d TPM stuck on read\n",
- __FILE__, __LINE__);
- return TPM_DRIVER_ERR;
- }
- udelay(1);
- status = tpm_read(&lpc_tpm_dev->locality
- [locality].tpm_status);
- }
- max_cycles = 0;
- while (burst_count-- && (offset < expected_count)) {
- buffer[offset++] = (u8)
tpm_read(&lpc_tpm_dev->locality + [locality].data); +
- if (offset == 6) {
- /*
- * We got the first six bytes of the
reply, + * let's figure out how many bytes to expect + * total - it is stored as a 4 byte number in + * network order, starting with offset 2 into + * the body of the reply.
- */
- u32 real_length;
- memcpy(&real_length,
- buffer + 2,
- sizeof(real_length));
- expected_count = be32_to_cpu(real_length);
- if ((expected_count < offset) ||
- (expected_count > *len)) {
- printf("%s:%d bad response size
%d\n", + __FILE__, __LINE__,
- expected_count);
- return TPM_DRIVER_ERR;
- }
- }
- }
- /* Wait for the next portion */
- status = tis_wait_reg(&lpc_tpm_dev->locality
- [locality].tpm_status,
- TIS_STS_VALID, TIS_STS_VALID);
- if (status == TPM_TIMEOUT_ERR) {
- printf("%s:%d failed to read response\n",
- __FILE__, __LINE__);
- return TPM_DRIVER_ERR;
- }
- if (offset == expected_count)
- break; /* We got all we need */
- } while ((status & has_data) == has_data);
No endless loop please.
I am not sure why you call this endless - it terminates on a few events. The thing is that it is not even known in advance how many cycles the loop will have to run, but it has necessary stop gaps to prevent it from running forever.
See above, is it possible for this to hang ? If not, ok.
- /*
- * Make sure we indeed read all there was. The TIS_STS_VALID bit
is + * known to be set.
- */
- if (status & TIS_STS_DATA_AVAILABLE) {
- printf("%s:%d wrong receive status %x\n",
- __FILE__, __LINE__, status);
- return TPM_DRIVER_ERR;
- }
- /* Tell the TPM that we are done. */
- tpm_write(TIS_STS_COMMAND_READY, &lpc_tpm_dev->locality
- [locality].tpm_status);
- *len = offset;
- return 0;
+}
+int tis_init(void) +{
- return tis_probe();
+}
Make tis_probe into tis_init and be done with it ?
ok
+int tis_open(void) +{
- u8 locality = 0; /* we use locality zero for everything */
- if (tis_close())
- return TPM_DRIVER_ERR;
- /* now request access to locality */
- tpm_write(TIS_ACCESS_REQUEST_USE,
- &lpc_tpm_dev->locality[locality].access);
- /* did we get a lock? */
- if (tis_wait_reg(&lpc_tpm_dev->locality[locality].access,
- TIS_ACCESS_ACTIVE_LOCALITY,
- TIS_ACCESS_ACTIVE_LOCALITY) == TPM_TIMEOUT_ERR) {
- printf("%s:%d - failed to lock locality %d\n",
- __FILE__, __LINE__, locality);
- return TPM_DRIVER_ERR;
- }
- tpm_write(TIS_STS_COMMAND_READY,
- &lpc_tpm_dev->locality[locality].tpm_status);
- return 0;
+}
[...]
Cheers
cheers, /vb
Cheers
cheers, /vb
On Sunday, October 16, 2011 03:04:33 AM Vadim Bendebury wrote:
On Sat, Oct 15, 2011 at 2:09 PM, Marek Vasut marek.vasut@gmail.com wrote:
On Saturday, October 15, 2011 08:47:39 PM Vadim Bendebury wrote:
Dear Marek Vasut,
thank you for your comments, please see below:
On Sat, Oct 15, 2011 at 11:08 AM, Marek Vasut marek.vasut@gmail.com
wrote:
On Saturday, October 15, 2011 05:38:50 AM Vadim Bendebury wrote:
TPM (Trusted Platform Module) is an integrated circuit and software platform that provides computer manufacturers with the core components of a subsystem used to assure authenticity, integrity and confidentiality.
[...]
Quick points:
- The license
Please suggest the appropriate file header text.
Uh ... you should know the license !!!
removed the BSD part
Are you sure you're not relicensing code you don't own ? I'm just curious, what's the origin of the code ? I'd prefer to avoid legal crap.
[..]
+struct lpc_tpm {
struct tpm_locality locality[TPM_TOTAL_LOCALITIES];+};
Do you need such envelope ?
I think I do - this accurately describes the structure of the chip.
There's just one member ... it's weird?
I think it is appropriate in this case to encapsulate the entire chip description in a structure. Among other things makes it easier to pass a pointer to the chip description around.
can't you pass the locality array ?
[..]
Dot missing at the end.
ok.
Please fix globally.
done
+#define TPM_DRIVER_ERR (-1)
- /* 1 second is plenty for anything TPM does.*/
+#define MAX_DELAY_US (1000 * 1000)
+/* Retrieve burst count value out of the status register contents. */ +#define BURST_COUNT(status) ((u16)(((status) >> TIS_STS_BURST_COUNT_SHIFT) & \ + TIS_STS_BURST_COUNT_MASK))
Do you need the cast ?
I think it demonstrates the intentional truncation of the value, it gets assigned to u16 values down the road, prevents compiler warnings about assigning incompatible values in some cases.
Make it an inline function then, this will do the typechecking for you.
I am not sure what is wrong with a short macro in this case - is this against the coding style?
It doesn't do typechecking.
+/*
- Structures defined below allow creating descriptions of TPM
vendor/device + * ID information for run time discovery. The only device the system knows + * about at this time is Infineon slb9635
- */
+struct device_name {
u16 dev_id;const char * const dev_name;+};
+struct vendor_name {
u16 vendor_id;const char *vendor_name;const struct device_name *dev_names;+};
+static const struct device_name infineon_devices[] = {
{0xb, "SLB9635 TT 1.2"},{0}+};
+static const struct vendor_name vendor_names[] = {
{0x15d1, "Infineon", infineon_devices},+};
+/*
- Cached vendor/device ID pair to indicate that the device has been
already + * discovered
- */
+static u32 vendor_dev_id;
+/* TPM access going through macros to make tracing easier. */ +#define tpm_read(ptr) ({ \
u32 __ret; \__ret = (sizeof(*ptr) == 1) ? readb(ptr) : readl(ptr); \debug(PREFIX "Read reg 0x%x returns 0x%x\n", \(u32)ptr - (u32)lpc_tpm_dev, __ret); \__ret; })Make this inline function
+#define tpm_write(value, ptr) ({ \
u32 __v = value; \debug(PREFIX "Write reg 0x%x with 0x%x\n", \(u32)ptr - (u32)lpc_tpm_dev, __v); \if (sizeof(*ptr) == 1) \writeb(__v, ptr); \else \writel(__v, ptr); })DTTO
Are you sure these will work as inline functions?
Why not ? Also, why do you introduce the __v ?
macro vs function: need to be able to tell the pointed object size at run time.
This seems wrong like hell.
__v is needed to avoid side effects when invoking the macro.
Side effects ? What side effects ?
[...]
Cheers
On Sat, Oct 15, 2011 at 8:31 PM, Marek Vasut marek.vasut@gmail.com wrote:
On Sunday, October 16, 2011 03:04:33 AM Vadim Bendebury wrote:
On Sat, Oct 15, 2011 at 2:09 PM, Marek Vasut marek.vasut@gmail.com wrote:
On Saturday, October 15, 2011 08:47:39 PM Vadim Bendebury wrote:
Dear Marek Vasut,
thank you for your comments, please see below:
On Sat, Oct 15, 2011 at 11:08 AM, Marek Vasut marek.vasut@gmail.com
wrote:
On Saturday, October 15, 2011 05:38:50 AM Vadim Bendebury wrote:
TPM (Trusted Platform Module) is an integrated circuit and software platform that provides computer manufacturers with the core components of a subsystem used to assure authenticity, integrity and confidentiality.
[...]
Quick points:
- The license
Please suggest the appropriate file header text.
Uh ... you should know the license !!!
removed the BSD part
Are you sure you're not relicensing code you don't own ? I'm just curious, what's the origin of the code ? I'd prefer to avoid legal crap.
I am sure.
[..]
+struct lpc_tpm {
- struct tpm_locality locality[TPM_TOTAL_LOCALITIES];
+};
Do you need such envelope ?
I think I do - this accurately describes the structure of the chip.
There's just one member ... it's weird?
I think it is appropriate in this case to encapsulate the entire chip description in a structure. Among other things makes it easier to pass a pointer to the chip description around.
can't you pass the locality array ?
no, because it would not be clear how big the array is.
[..]
Dot missing at the end.
ok.
Please fix globally.
done
+#define TPM_DRIVER_ERR (-1)
- /* 1 second is plenty for anything TPM does.*/
+#define MAX_DELAY_US (1000 * 1000)
+/* Retrieve burst count value out of the status register contents. */ +#define BURST_COUNT(status) ((u16)(((status) >> TIS_STS_BURST_COUNT_SHIFT) & \ + TIS_STS_BURST_COUNT_MASK))
Do you need the cast ?
I think it demonstrates the intentional truncation of the value, it gets assigned to u16 values down the road, prevents compiler warnings about assigning incompatible values in some cases.
Make it an inline function then, this will do the typechecking for you.
I am not sure what is wrong with a short macro in this case - is this against the coding style?
It doesn't do typechecking.
but the code around it does, doesn't it?
Sorry, as I said, I am new here: how does this work on this project - does the submitter have to agree to all reviewer's comments? Can I ask somebody else to confirm that using a macro in this case in inappropriate?
+/*
- Structures defined below allow creating descriptions of TPM
vendor/device + * ID information for run time discovery. The only device the system knows + * about at this time is Infineon slb9635
- */
+struct device_name {
- u16 dev_id;
- const char * const dev_name;
+};
+struct vendor_name {
- u16 vendor_id;
- const char *vendor_name;
- const struct device_name *dev_names;
+};
+static const struct device_name infineon_devices[] = {
- {0xb, "SLB9635 TT 1.2"},
- {0}
+};
+static const struct vendor_name vendor_names[] = {
- {0x15d1, "Infineon", infineon_devices},
+};
+/*
- Cached vendor/device ID pair to indicate that the device has been
already + * discovered
- */
+static u32 vendor_dev_id;
+/* TPM access going through macros to make tracing easier. */ +#define tpm_read(ptr) ({ \
- u32 __ret; \
- __ret = (sizeof(*ptr) == 1) ? readb(ptr) : readl(ptr); \
- debug(PREFIX "Read reg 0x%x returns 0x%x\n", \
- (u32)ptr - (u32)lpc_tpm_dev, __ret); \
- __ret; })
Make this inline function
+#define tpm_write(value, ptr) ({ \
- u32 __v = value; \
- debug(PREFIX "Write reg 0x%x with 0x%x\n", \
- (u32)ptr - (u32)lpc_tpm_dev, __v); \
- if (sizeof(*ptr) == 1) \
- writeb(__v, ptr); \
- else \
- writel(__v, ptr); })
DTTO
Are you sure these will work as inline functions?
Why not ? Also, why do you introduce the __v ?
macro vs function: need to be able to tell the pointed object size at run time.
This seems wrong like hell.
You are entitled to your opinion, but you should not be suggesting to change this code to inline functions, because it would break it.
__v is needed to avoid side effects when invoking the macro.
Side effects ? What side effects ?
https://www.securecoding.cert.org/confluence/display/seccode/PRE31-C.+Avoid+...
[...]
Cheers _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
cheers, /vb
Dear Vadim Bendebury,
In message CANy1bu+DZmD_Z=AU8LdTUy7ewHfwMU-8PvH1OA1jba9Tz1xuxA@mail.gmail.com you wrote:
I am not sure what is wrong with a short macro in this case - is this against the coding style?
It doesn't do typechecking.
but the code around it does, doesn't it?
I explained this yesterday, too. Functions are preferred over macros. In this case ther eis no reason not to use a function.
Sorry, as I said, I am new here: how does this work on this project - does the submitter have to agree to all reviewer's comments? Can I ask
No, you don't have to agree. But we also don't have to accept code that we don't like ;-)
somebody else to confirm that using a macro in this case in inappropriate?
I already did.
Best regards,
Wolfgang Denk
Wolfgang Denk,
On Sun, Oct 16, 2011 at 12:35 AM, Wolfgang Denk wd@denx.de wrote:
Dear Vadim Bendebury,
In message CANy1bu+DZmD_Z=AU8LdTUy7ewHfwMU-8PvH1OA1jba9Tz1xuxA@mail.gmail.com you wrote:
I am not sure what is wrong with a short macro in this case - is this against the coding style?
It doesn't do typechecking.
but the code around it does, doesn't it?
I explained this yesterday, too. Functions are preferred over macros. In this case ther eis no reason not to use a function.
Sorry, as I said, I am new here: how does this work on this project - does the submitter have to agree to all reviewer's comments? Can I ask
No, you don't have to agree. But we also don't have to accept code that we don't like ;-)
certainly, but wouldn't you risk to throw the baby out with the bath water this way? ;-)
Also, what about situations when one reviewer requests a certain implementation and another one finds it inappropriate?
somebody else to confirm that using a macro in this case in inappropriate?
I already did.
Thank you, I will fix this.
Can you please also confirm that having a structure with a single element as an array is "weird" and must be changed to passing around a pointer to a single element without the size (or maybe the idea is that the pointer AND the size need to be passed around)?
Are macros acceptable to wrap input output with debug messages, as was suggested earlier on this list, or should I replace each macro with two inline functions?
Please advise, cheers, /vb
Best regards,
Wolfgang Denk
-- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de The average woman would rather have beauty than brains, because the average man can see better than he can think. _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Dear Vadim Bendebury,
In message CANy1buJCjdQnpQ6EHTjJ3jOvGe5fKqDoDpevofhu6_-2Y7L6gg@mail.gmail.com you wrote:
Also, what about situations when one reviewer requests a certain implementation and another one finds it inappropriate?
Here we had several people (Marek and me) asking the same thing. And actually my message was intended to tell you that I agree with Marek.
In case of doubt, someone has to make a final decision.
In this specific case I already decided (and told you) that I want to see a function instead of a macro.
Can you please also confirm that having a structure with a single element as an array is "weird" and must be changed to passing around a pointer to a single element without the size (or maybe the idea is that the pointer AND the size need to be passed around)?
Yes, I consider this weird, too. And you failed to provide a good explanation why you think this would be needed so far.
Are macros acceptable to wrap input output with debug messages, as was suggested earlier on this list, or should I replace each macro with two inline functions?
Sorry, I don't remember which code you are referring to here.
Best regards,
Wolfgang Denk
Dear Wolfgang Denk,
On Sun, Oct 16, 2011 at 1:04 PM, Wolfgang Denk wd@denx.de wrote:
Dear Vadim Bendebury,
In message CANy1buJCjdQnpQ6EHTjJ3jOvGe5fKqDoDpevofhu6_-2Y7L6gg@mail.gmail.com you wrote:
Also, what about situations when one reviewer requests a certain implementation and another one finds it inappropriate?
Here we had several people (Marek and me) asking the same thing. And actually my message was intended to tell you that I agree with Marek.
In case of doubt, someone has to make a final decision.
In this specific case I already decided (and told you) that I want to see a function instead of a macro.
yes, I saw it and have already changed the implementation.
Can you please also confirm that having a structure with a single element as an array is "weird" and must be changed to passing around a pointer to a single element without the size (or maybe the idea is that the pointer AND the size need to be passed around)?
Yes, I consider this weird, too. And you failed to provide a good explanation why you think this would be needed so far.
My explanation is that it is better readable when the entire information about a chip is contained in one type, as opposed to having the size separate and needed to be carried around (instead of using ARRAY_SIZE() when needed). When the pointer to the chip structure is passed around, the recipient does not have to be aware of a separate definition, all information about the chip is contained in a structure.
Sorry, I am restating the reasons here because I am not sure if you wrote your previous reply before seeing them.
Please let me know if you don't find this explanation convincing, I will change the code as you suggest.
Are macros acceptable to wrap input output with debug messages, as was suggested earlier on this list, or should I replace each macro with two inline functions?
Sorry, I don't remember which code you are referring to here.
I am referring to this message:
http://lists.denx.de/pipermail/u-boot/2011-October/104780.html
which is a part of this thread:
http://lists.denx.de/pipermail/u-boot/2011-October/104665.html
cheers, /vb
Best regards,
Wolfgang Denk
-- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de "I'm growing older, but not up." - Jimmy Buffett
Dear Vadim Bendebury,
In message CAC3GErH1GpLTEfH3ELi5ebtJPz0HLcJxj3YX3GGUU9HgcHErGA@mail.gmail.com you wrote:
Yes, I consider this weird, too. And you failed to provide a good explanation why you think this would be needed so far.
My explanation is that it is better readable when the entire information about a chip is contained in one type, as opposed to having the size separate and needed to be carried around (instead of using ARRAY_SIZE() when needed). When the pointer to the chip structure is passed around, the recipient does not have to be aware of a separate definition, all information about the chip is contained in a structure.
I read your explanation, but I still fail to uinderstand why we need an additinal wrapper around the internal strcut, which is the only element.
Sorry, I am restating the reasons here because I am not sure if you wrote your previous reply before seeing them.
I did. That's why I wrote you failed to provide a _good_ explanation.
Please let me know if you don't find this explanation convincing, I will change the code as you suggest.
Are macros acceptable to wrap input output with debug messages, as was suggested earlier on this list, or should I replace each macro with two inline functions?
Sorry, I don't remember which code you are referring to here.
I am referring to this message:
http://lists.denx.de/pipermail/u-boot/2011-October/104780.html
Yes, I know that. But why would one macro turn into two inline functions?
Best regards,
Wolfgang Denk
Dear Wolfgang Denk,
On Sun, Oct 16, 2011 at 1:31 PM, Wolfgang Denk wd@denx.de wrote:
Dear Vadim Bendebury,
In message CAC3GErH1GpLTEfH3ELi5ebtJPz0HLcJxj3YX3GGUU9HgcHErGA@mail.gmail.com you wrote:
Yes, I consider this weird, too. And you failed to provide a good explanation why you think this would be needed so far.
My explanation is that it is better readable when the entire information about a chip is contained in one type, as opposed to having the size separate and needed to be carried around (instead of using ARRAY_SIZE() when needed). When the pointer to the chip structure is passed around, the recipient does not have to be aware of a separate definition, all information about the chip is contained in a structure.
I read your explanation, but I still fail to uinderstand why we need an additinal wrapper around the internal strcut, which is the only element.
Sorry, I am restating the reasons here because I am not sure if you wrote your previous reply before seeing them.
I did. That's why I wrote you failed to provide a _good_ explanation.
ok.
Please let me know if you don't find this explanation convincing, I will change the code as you suggest.
Are macros acceptable to wrap input output with debug messages, as was suggested earlier on this list, or should I replace each macro with two inline functions?
Sorry, I don't remember which code you are referring to here.
I am referring to this message:
http://lists.denx.de/pipermail/u-boot/2011-October/104780.html
Yes, I know that. But why would one macro turn into two inline functions?
because this chip is sensitive to the access cycle size (byte versus word).
When using macros one can rely on the passed in pointer type to decide which access to use (byte vs word). If using inline functions, there would be two separate functions required for read (byte and u32) and write (byte and u32).
cheers, /vb
Best regards,
Wolfgang Denk
-- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de Alan Turing thought about criteria to settle the question of whether machines can think, a question of which we now know that it is about as relevant as the question of whether submarines can swim. -- Edsger Dijkstra _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Dear Vadim Bendebury,
In message CANy1buJ0mhpDY948c+PM6u4EsWtoHw3+-u+m_FNiG3BztBinVA@mail.gmail.com you wrote:
because this chip is sensitive to the access cycle size (byte versus word).
When using macros one can rely on the passed in pointer type to decide which access to use (byte vs word). If using inline functions, there would be two separate functions required for read (byte and u32) and write (byte and u32).
This would then be an improvement in the code, as we then can see what the code is actually doing.
Best regards,
Wolfgang Denk
Dear Wolfgang Denk,
On Sun, Oct 16, 2011 at 1:53 PM, Wolfgang Denk wd@denx.de wrote:
Dear Vadim Bendebury,
In message CANy1buJ0mhpDY948c+PM6u4EsWtoHw3+-u+m_FNiG3BztBinVA@mail.gmail.com you wrote:
because this chip is sensitive to the access cycle size (byte versus word).
When using macros one can rely on the passed in pointer type to decide which access to use (byte vs word). If using inline functions, there would be two separate functions required for read (byte and u32) and write (byte and u32).
This would then be an improvement in the code, as we then can see what the code is actually doing.
please see the latest patch (v5) which addresses the chip representation and register access wrapper comments.
cheers, /vb
Best regards,
Wolfgang Denk
-- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de What the gods would destroy they first submit to an IEEE standards committee.
On Sunday, October 16, 2011 05:45:40 AM Vadim Bendebury wrote:
On Sat, Oct 15, 2011 at 8:31 PM, Marek Vasut marek.vasut@gmail.com wrote:
On Sunday, October 16, 2011 03:04:33 AM Vadim Bendebury wrote:
On Sat, Oct 15, 2011 at 2:09 PM, Marek Vasut marek.vasut@gmail.com wrote:
On Saturday, October 15, 2011 08:47:39 PM Vadim Bendebury wrote:
Dear Marek Vasut,
thank you for your comments, please see below:
On Sat, Oct 15, 2011 at 11:08 AM, Marek Vasut marek.vasut@gmail.com
wrote:
On Saturday, October 15, 2011 05:38:50 AM Vadim Bendebury wrote: > TPM (Trusted Platform Module) is an integrated circuit and > software platform that provides computer manufacturers with the > core components of a subsystem used to assure authenticity, > integrity and confidentiality.
[...]
Quick points:
- The license
Please suggest the appropriate file header text.
Uh ... you should know the license !!!
removed the BSD part
Are you sure you're not relicensing code you don't own ? I'm just curious, what's the origin of the code ? I'd prefer to avoid legal crap.
I am sure.
Would you mind answering my second question please ?
[..]
> + > +struct lpc_tpm { > + struct tpm_locality locality[TPM_TOTAL_LOCALITIES]; > +};
Do you need such envelope ?
I think I do - this accurately describes the structure of the chip.
There's just one member ... it's weird?
I think it is appropriate in this case to encapsulate the entire chip description in a structure. Among other things makes it easier to pass a pointer to the chip description around.
can't you pass the locality array ?
no, because it would not be clear how big the array is.
TPM_TOTAL_LOCALITIES big ?
[..]
Dot missing at the end.
ok.
Please fix globally.
done
> +#define TPM_DRIVER_ERR (-1) > + > + /* 1 second is plenty for anything TPM does.*/ > +#define MAX_DELAY_US (1000 * 1000) > + > +/* Retrieve burst count value out of the status register > contents. */ +#define BURST_COUNT(status) ((u16)(((status) >> > TIS_STS_BURST_COUNT_SHIFT) & \ + > TIS_STS_BURST_COUNT_MASK))
Do you need the cast ?
I think it demonstrates the intentional truncation of the value, it gets assigned to u16 values down the road, prevents compiler warnings about assigning incompatible values in some cases.
Make it an inline function then, this will do the typechecking for you.
I am not sure what is wrong with a short macro in this case - is this against the coding style?
It doesn't do typechecking.
but the code around it does, doesn't it?
Sorry, as I said, I am new here: how does this work on this project - does the submitter have to agree to all reviewer's comments? Can I ask somebody else to confirm that using a macro in this case in inappropriate?
> + > +/* > + * Structures defined below allow creating descriptions of TPM > vendor/device + * ID information for run time discovery. The only > device the system knows + * about at this time is Infineon slb9635 > + */ > +struct device_name { > + u16 dev_id; > + const char * const dev_name; > +}; > + > +struct vendor_name { > + u16 vendor_id; > + const char *vendor_name; > + const struct device_name *dev_names; > +}; > + > +static const struct device_name infineon_devices[] = { > + {0xb, "SLB9635 TT 1.2"}, > + {0} > +}; > + > +static const struct vendor_name vendor_names[] = { > + {0x15d1, "Infineon", infineon_devices}, > +}; > + > +/* > + * Cached vendor/device ID pair to indicate that the device has > been already + * discovered > + */ > +static u32 vendor_dev_id; > + > +/* TPM access going through macros to make tracing easier. */ > +#define tpm_read(ptr) ({ \ > + u32 __ret; \ > + __ret = (sizeof(*ptr) == 1) ? readb(ptr) : readl(ptr); \ > + debug(PREFIX "Read reg 0x%x returns 0x%x\n", \ > + (u32)ptr - (u32)lpc_tpm_dev, __ret); \ > + __ret; }) > +
Make this inline function
> +#define tpm_write(value, ptr) ({ \ > + u32 __v = value; \ > + debug(PREFIX "Write reg 0x%x with 0x%x\n", \ > + (u32)ptr - (u32)lpc_tpm_dev, __v); \ > + if (sizeof(*ptr) == 1) \ > + writeb(__v, ptr); \ > + else \ > + writel(__v, ptr); }) > +
DTTO
Are you sure these will work as inline functions?
Why not ? Also, why do you introduce the __v ?
macro vs function: need to be able to tell the pointed object size at run time.
This seems wrong like hell.
You are entitled to your opinion, but you should not be suggesting to change this code to inline functions, because it would break it.
Then write it so it won't break please.
__v is needed to avoid side effects when invoking the macro.
Side effects ? What side effects ?
https://www.securecoding.cert.org/confluence/display/seccode/PRE31-C.+Avoid +side-effects+in+arguments+to+unsafe+macros
I still don't see it. You use the variable in printf() and writeX(), neither of which change the variable ... so where's the sideeffect ?
Cheers
On Sun, Oct 16, 2011 at 5:28 AM, Marek Vasut marek.vasut@gmail.com wrote:
On Sunday, October 16, 2011 05:45:40 AM Vadim Bendebury wrote:
On Sat, Oct 15, 2011 at 8:31 PM, Marek Vasut marek.vasut@gmail.com wrote:
On Sunday, October 16, 2011 03:04:33 AM Vadim Bendebury wrote:
On Sat, Oct 15, 2011 at 2:09 PM, Marek Vasut marek.vasut@gmail.com wrote:
On Saturday, October 15, 2011 08:47:39 PM Vadim Bendebury wrote:
Dear Marek Vasut,
thank you for your comments, please see below:
On Sat, Oct 15, 2011 at 11:08 AM, Marek Vasut marek.vasut@gmail.com
wrote:
> On Saturday, October 15, 2011 05:38:50 AM Vadim Bendebury wrote: >> TPM (Trusted Platform Module) is an integrated circuit and >> software platform that provides computer manufacturers with the >> core components of a subsystem used to assure authenticity, >> integrity and confidentiality. > > [...] > > Quick points: > * The license
Please suggest the appropriate file header text.
Uh ... you should know the license !!!
removed the BSD part
Are you sure you're not relicensing code you don't own ? I'm just curious, what's the origin of the code ? I'd prefer to avoid legal crap.
I am sure.
Would you mind answering my second question please ?
I wrote this from scratch.
[..]
>> + >> +struct lpc_tpm { >> + struct tpm_locality locality[TPM_TOTAL_LOCALITIES]; >> +}; > > Do you need such envelope ?
I think I do - this accurately describes the structure of the chip.
There's just one member ... it's weird?
I think it is appropriate in this case to encapsulate the entire chip description in a structure. Among other things makes it easier to pass a pointer to the chip description around.
can't you pass the locality array ?
no, because it would not be clear how big the array is.
TPM_TOTAL_LOCALITIES big ?
I believe it is clearer when this information is included in a structure describing the chip (as opposed to the array size being a separate #define)
[..]
> Dot missing at the end.
ok.
Please fix globally.
done
>> +#define TPM_DRIVER_ERR (-1) >> + >> + /* 1 second is plenty for anything TPM does.*/ >> +#define MAX_DELAY_US (1000 * 1000) >> + >> +/* Retrieve burst count value out of the status register >> contents. */ +#define BURST_COUNT(status) ((u16)(((status) >> >> TIS_STS_BURST_COUNT_SHIFT) & \ + >> TIS_STS_BURST_COUNT_MASK)) > > Do you need the cast ?
I think it demonstrates the intentional truncation of the value, it gets assigned to u16 values down the road, prevents compiler warnings about assigning incompatible values in some cases.
Make it an inline function then, this will do the typechecking for you.
I am not sure what is wrong with a short macro in this case - is this against the coding style?
It doesn't do typechecking.
but the code around it does, doesn't it?
Sorry, as I said, I am new here: how does this work on this project - does the submitter have to agree to all reviewer's comments? Can I ask somebody else to confirm that using a macro in this case in inappropriate?
converted BURST_COUNT into a function.
>> + >> +/* >> + * Structures defined below allow creating descriptions of TPM >> vendor/device + * ID information for run time discovery. The only >> device the system knows + * about at this time is Infineon slb9635 >> + */ >> +struct device_name { >> + u16 dev_id; >> + const char * const dev_name; >> +}; >> + >> +struct vendor_name { >> + u16 vendor_id; >> + const char *vendor_name; >> + const struct device_name *dev_names; >> +}; >> + >> +static const struct device_name infineon_devices[] = { >> + {0xb, "SLB9635 TT 1.2"}, >> + {0} >> +}; >> + >> +static const struct vendor_name vendor_names[] = { >> + {0x15d1, "Infineon", infineon_devices}, >> +}; >> + >> +/* >> + * Cached vendor/device ID pair to indicate that the device has >> been already + * discovered >> + */ >> +static u32 vendor_dev_id; >> + >> +/* TPM access going through macros to make tracing easier. */ >> +#define tpm_read(ptr) ({ \ >> + u32 __ret; \ >> + __ret = (sizeof(*ptr) == 1) ? readb(ptr) : readl(ptr); \ >> + debug(PREFIX "Read reg 0x%x returns 0x%x\n", \ >> + (u32)ptr - (u32)lpc_tpm_dev, __ret); \ >> + __ret; }) >> + > > Make this inline function > >> +#define tpm_write(value, ptr) ({ \ >> + u32 __v = value; \ >> + debug(PREFIX "Write reg 0x%x with 0x%x\n", \ >> + (u32)ptr - (u32)lpc_tpm_dev, __v); \ >> + if (sizeof(*ptr) == 1) \ >> + writeb(__v, ptr); \ >> + else \ >> + writel(__v, ptr); }) >> + > > DTTO
Are you sure these will work as inline functions?
Why not ? Also, why do you introduce the __v ?
macro vs function: need to be able to tell the pointed object size at run time.
This seems wrong like hell.
You are entitled to your opinion, but you should not be suggesting to change this code to inline functions, because it would break it.
Then write it so it won't break please.
it is not broken as of now and IMO this is a good case for using macros.
There is at least one other custodian who supports this, and Wolfgang Denk does not seem to mind.
__v is needed to avoid side effects when invoking the macro.
Side effects ? What side effects ?
https://www.securecoding.cert.org/confluence/display/seccode/PRE31-C.+Avoid +side-effects+in+arguments+to+unsafe+macros
I still don't see it. You use the variable in printf() and writeX(), neither of which change the variable ... so where's the sideeffect ?
The side effect comes from the calling site.
When data[count++] is used as a macro argument, if there is no intermediate variable defined in the macro declaration, macro expansion inserts data[count++] in the code several times (as many times as the parameter is used in the macro declaration), and in this particular case gets executed twice, resulting in `count' advancing by 2 and wrong `data' values used.
Cheers
cheers, /vb
On Sunday, October 16, 2011 09:49:19 PM Vadim Bendebury wrote:
On Sun, Oct 16, 2011 at 5:28 AM, Marek Vasut marek.vasut@gmail.com wrote:
On Sunday, October 16, 2011 05:45:40 AM Vadim Bendebury wrote:
On Sat, Oct 15, 2011 at 8:31 PM, Marek Vasut marek.vasut@gmail.com wrote:
On Sunday, October 16, 2011 03:04:33 AM Vadim Bendebury wrote:
On Sat, Oct 15, 2011 at 2:09 PM, Marek Vasut marek.vasut@gmail.com
wrote:
On Saturday, October 15, 2011 08:47:39 PM Vadim Bendebury wrote: > Dear Marek Vasut, > > thank you for your comments, please see below: > > On Sat, Oct 15, 2011 at 11:08 AM, Marek Vasut > marek.vasut@gmail.com
wrote:
> > On Saturday, October 15, 2011 05:38:50 AM Vadim Bendebury wrote: > >> TPM (Trusted Platform Module) is an integrated circuit and > >> software platform that provides computer manufacturers with the > >> core components of a subsystem used to assure authenticity, > >> integrity and confidentiality. > > > > [...] > > > > Quick points: > > * The license > > Please suggest the appropriate file header text.
Uh ... you should know the license !!!
removed the BSD part
Are you sure you're not relicensing code you don't own ? I'm just curious, what's the origin of the code ? I'd prefer to avoid legal crap.
I am sure.
Would you mind answering my second question please ?
I wrote this from scratch.
Thanks!
[...]
__v is needed to avoid side effects when invoking the macro.
Side effects ? What side effects ?
https://www.securecoding.cert.org/confluence/display/seccode/PRE31-C.+Av oid +side-effects+in+arguments+to+unsafe+macros
I still don't see it. You use the variable in printf() and writeX(), neither of which change the variable ... so where's the sideeffect ?
The side effect comes from the calling site.
When data[count++] is used as a macro argument, if there is no intermediate variable defined in the macro declaration, macro expansion inserts data[count++] in the code several times (as many times as the parameter is used in the macro declaration), and in this particular case gets executed twice, resulting in `count' advancing by 2 and wrong `data' values used.
Thanks for clearing this
Cheers
cheers, /vb
Dear Vadim Bendebury,
In message CAC3GErGRvV0PjWt47d1dQ=D5B3kv+Bge0jsxdZSYWEPSvbekLw@mail.gmail.com you wrote:
Make it an inline function then, this will do the typechecking for you.
I am not sure what is wrong with a short macro in this case - is this against the coding style?
"Documentation/CodingStyle":
Generally, inline functions are preferable to macros resembling functions.
Marek listed one (important) reason why this is the case.
So please change this.
Best regards,
Wolfgang Denk
On Saturday 15 October 2011 17:09:55 Marek Vasut wrote:
On Saturday, October 15, 2011 08:47:39 PM Vadim Bendebury wrote:
On Sat, Oct 15, 2011 at 11:08 AM, Marek Vasut wrote:
On Saturday, October 15, 2011 05:38:50 AM Vadim Bendebury wrote:
--- /dev/null +++ b/drivers/tpm/generic_lpc_tpm.c
[...]
+struct lpc_tpm {
struct tpm_locality locality[TPM_TOTAL_LOCALITIES];+};
Do you need such envelope ?
I think I do - this accurately describes the structure of the chip.
There's just one member ... it's weird?
i don't think so. if the hardware is really that way, then a single struct that represents it makes sense to me.
+/* Error value returned on various TPM driver errors */
Dot missing at the end.
ok.
Please fix globally.
this isn't really a standard. for single sentences/fragments, they often get dropped. left up to the relevant writer what they want to do.
+/* TPM access going through macros to make tracing easier. */ +#define tpm_read(ptr) ({ \
u32 __ret; \__ret = (sizeof(*ptr) == 1) ? readb(ptr) : readl(ptr); \debug(PREFIX "Read reg 0x%x returns 0x%x\n", \(u32)ptr - (u32)lpc_tpm_dev, __ret); \__ret; })Make this inline function
+#define tpm_write(value, ptr) ({ \
u32 __v = value; \debug(PREFIX "Write reg 0x%x with 0x%x\n", \(u32)ptr - (u32)lpc_tpm_dev, __v); \if (sizeof(*ptr) == 1) \writeb(__v, ptr); \else \writel(__v, ptr); })DTTO
Are you sure these will work as inline functions?
Why not ? Also, why do you introduce the __v ?
the sizeof() magic requires it be a macro. and the __v indirection is required (there should be a __p too, but that'd be a little more work, and is less likely to have side effects). -mike
Dear Vadim Bendebury,
In message 20111015033850.74AD5419FF@eskimo.mtv.corp.google.com you wrote:
TPM (Trusted Platform Module) is an integrated circuit and software platform that provides computer manufacturers with the core components of a subsystem used to assure authenticity, integrity and confidentiality.
...
--- /dev/null +++ b/drivers/tpm/Makefile @@ -0,0 +1,27 @@ +# Copyright (c) 2011 The Chromium OS Authors. All rights reserved. +# Use of this source code is governed by a BSD-style license that can be +# found in the LICENSE file.
Please fix this. This has been pointed out before. Please work a bit more careful. Thanks.
...
+++ b/drivers/tpm/generic_lpc_tpm.c @@ -0,0 +1,488 @@ +/*
- Copyright (c) 2011 The Chromium OS Authors.
- Released under the 2-clause BSD license.
...
- 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.
If we can have it under GPLv2+, then we don't need the BSD part. Please drop these lines.
...
+/* TPM access going through macros to make tracing easier. */ +#define tpm_read(ptr) ({ \
- u32 __ret; \
- __ret = (sizeof(*ptr) == 1) ? readb(ptr) : readl(ptr); \
debug(PREFIX "Read reg 0x%x returns 0x%x\n", \(u32)ptr - (u32)lpc_tpm_dev, __ret); \__ret; })
Please test with something like this instead:
debug("%sRead reg 0x%x returns 0x%x\n", PREFIX, ...
It might result in smaller code. Please verify. If so, please fix globally.
...
- vid = didvid & 0xffff;
- did = (didvid >> 16) & 0xffff;
- for (i = 0; i < ARRAY_SIZE(vendor_names); i++) {
int j = 0;u16 known_did;if (vid == vendor_names[i].vendor_id)vendor_name = vendor_names[i].vendor_name;
Please separate declarations and code by a blank line.
burst = BURST_COUNT(value);if ((offset == (len - 1)) && burst)/** We need to be able to send the last byte to the* device, so burst size must be nonzero before we* break out.*/break;
We require braces around multiline statements.
Best regards,
Wolfgang Denk
On Sat, Oct 15, 2011 at 12:25 PM, Wolfgang Denk wd@denx.de wrote:
Dear Vadim Bendebury,
In message 20111015033850.74AD5419FF@eskimo.mtv.corp.google.com you wrote:
TPM (Trusted Platform Module) is an integrated circuit and software platform that provides computer manufacturers with the core components of a subsystem used to assure authenticity, integrity and confidentiality.
...
--- /dev/null +++ b/drivers/tpm/Makefile @@ -0,0 +1,27 @@ +# Copyright (c) 2011 The Chromium OS Authors. All rights reserved. +# Use of this source code is governed by a BSD-style license that can be +# found in the LICENSE file.
Please fix this. This has been pointed out before. Please work a bit more careful. Thanks.
done
...
+++ b/drivers/tpm/generic_lpc_tpm.c @@ -0,0 +1,488 @@ +/*
- Copyright (c) 2011 The Chromium OS Authors.
- Released under the 2-clause BSD license.
...
- 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.
If we can have it under GPLv2+, then we don't need the BSD part. Please drop these lines.
done
...
+/* TPM access going through macros to make tracing easier. */ +#define tpm_read(ptr) ({ \
- u32 __ret; \
- __ret = (sizeof(*ptr) == 1) ? readb(ptr) : readl(ptr); \
- debug(PREFIX "Read reg 0x%x returns 0x%x\n", \
- (u32)ptr - (u32)lpc_tpm_dev, __ret); \
- __ret; })
Please test with something like this instead:
debug("%sRead reg 0x%x returns 0x%x\n", PREFIX, ...
It might result in smaller code. Please verify. If so, please fix globally.
did not see any difference in the code layout as reported by objdump of the module
...
- vid = didvid & 0xffff;
- did = (didvid >> 16) & 0xffff;
- for (i = 0; i < ARRAY_SIZE(vendor_names); i++) {
- int j = 0;
- u16 known_did;
- if (vid == vendor_names[i].vendor_id)
- vendor_name = vendor_names[i].vendor_name;
Please separate declarations and code by a blank line.
done
- burst = BURST_COUNT(value);
- if ((offset == (len - 1)) && burst)
- /*
- * We need to be able to send the last byte to the
- * device, so burst size must be nonzero before we
- * break out.
- */
- break;
We require braces around multiline statements.
done
Best regards,
Wolfgang Denk
-- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de "Deliver yesterday, code today, think tomorrow."
chrrs, /vb
On Friday 14 October 2011 23:38:50 Vadim Bendebury wrote:
--- /dev/null +++ b/drivers/tpm/generic_lpc_tpm.c
+#define TPM_TIMEOUT_ERR (~0) +#define TPM_DRIVER_ERR (-1)
these are the same thing. another reason why you shouldn't mix ~ with normal values. use -2 or something.
+/* TPM access going through macros to make tracing easier. */ +#define tpm_read(ptr) ({ \
- u32 __ret; \
- __ret = (sizeof(*ptr) == 1) ? readb(ptr) : readl(ptr); \
debug(PREFIX "Read reg 0x%x returns 0x%x\n", \(u32)ptr - (u32)lpc_tpm_dev, __ret); \__ret; })
that last "__ret;" is indented way too far. it should be on the same level as "u32 __ret;" and such.
+#define tpm_write(value, ptr) ({ \
- u32 __v = value; \
- debug(PREFIX "Write reg 0x%x with 0x%x\n", \
(u32)ptr - (u32)lpc_tpm_dev, __v); \- if (sizeof(*ptr) == 1) \
writeb(__v, ptr); \- else \
writel(__v, ptr); })
({...}) doesn't make sense here. this should be a do{...}while(0).
printf("%s:%d - failed to get 'command_ready' status\n",__FILE__, __LINE__);
replace __FILE__/__LINE__ with __func__ here and everywhere else in the file -mike
On Sat, Oct 15, 2011 at 12:42 PM, Mike Frysinger vapier@gentoo.org wrote:
On Friday 14 October 2011 23:38:50 Vadim Bendebury wrote:
--- /dev/null +++ b/drivers/tpm/generic_lpc_tpm.c
+#define TPM_TIMEOUT_ERR (~0) +#define TPM_DRIVER_ERR (-1)
these are the same thing. another reason why you shouldn't mix ~ with normal values. use -2 or something.
ok
+/* TPM access going through macros to make tracing easier. */ +#define tpm_read(ptr) ({ \
- u32 __ret; \
- __ret = (sizeof(*ptr) == 1) ? readb(ptr) : readl(ptr); \
- debug(PREFIX "Read reg 0x%x returns 0x%x\n", \
- (u32)ptr - (u32)lpc_tpm_dev, __ret); \
- __ret; })
that last "__ret;" is indented way too far. it should be on the same level as "u32 __ret;" and such.
ok
+#define tpm_write(value, ptr) ({ \
- u32 __v = value; \
- debug(PREFIX "Write reg 0x%x with 0x%x\n", \
- (u32)ptr - (u32)lpc_tpm_dev, __v); \
- if (sizeof(*ptr) == 1) \
- writeb(__v, ptr); \
- else \
- writel(__v, ptr); })
({...}) doesn't make sense here. this should be a do{...}while(0).
ok
- printf("%s:%d - failed to get 'command_ready' status\n",
- __FILE__, __LINE__);
replace __FILE__/__LINE__ with __func__ here and everywhere else in the file -mike
Mike, you seem the only one concerned with this. As I described in our previous exchange, I believe specifying file and line number is better. So, I would like to hear a second opinion on this.
cheers, /vb
On Sat, Oct 15, 2011 at 12:42 PM, Mike Frysinger vapier@gentoo.org wrote:
On Friday 14 October 2011 23:38:50 Vadim Bendebury wrote:
--- /dev/null +++ b/drivers/tpm/generic_lpc_tpm.c
+#define TPM_TIMEOUT_ERR (~0) +#define TPM_DRIVER_ERR (-1)
these are the same thing. another reason why you shouldn't mix ~ with normal values. use -2 or something.
done
+/* TPM access going through macros to make tracing easier. */ +#define tpm_read(ptr) ({ \
- u32 __ret; \
- __ret = (sizeof(*ptr) == 1) ? readb(ptr) : readl(ptr); \
- debug(PREFIX "Read reg 0x%x returns 0x%x\n", \
- (u32)ptr - (u32)lpc_tpm_dev, __ret); \
- __ret; })
that last "__ret;" is indented way too far. it should be on the same level as "u32 __ret;" and such.
done
+#define tpm_write(value, ptr) ({ \
- u32 __v = value; \
- debug(PREFIX "Write reg 0x%x with 0x%x\n", \
- (u32)ptr - (u32)lpc_tpm_dev, __v); \
- if (sizeof(*ptr) == 1) \
- writeb(__v, ptr); \
- else \
- writel(__v, ptr); })
({...}) doesn't make sense here. this should be a do{...}while(0).
done [..] cheers, /vb
participants (4)
-
Marek Vasut -
Mike Frysinger -
Vadim Bendebury -
Wolfgang Denk