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