Messages in this thread | | | From | <> | Subject | RE: [PATCH resend] TPM: Issue TPM_STARTUP at driver load if the TPM has not been started | Date | Wed, 21 Nov 2012 08:59:24 +0000 |
| |
Hi Jason, Hi Kent, > >Discussion on this got a bit sidetracked talking about >suspend/resume.. To be clear, this fixes a real, serious, problem on >normal embedded cases where the kernel refuses to attach the TPM driver >at all. > >Key, are you happy with this as-is for the next merge window? > >This version is rebased and retested to 3.7-rc6. Tested on Atmel >and Nuvoton LPC TPMs on PPC32.
Sorry I was tied up at work with other stuff, so I couldn't try out your initial patch. Thanks for resubmitting!
I just gave the new version a run on my beagleboard with our Infineon SLB9635 TT 1.2 Soft I2C TPM and it seems to work as expected. (Tested with and without previous startup).
Tested-by: Peter Huewe <peter.huewe@infineon.com>
I just have some minor comments - please see below.
>+++ b/drivers/char/tpm/tpm.c >+#define TPM_ORD_STARTUP cpu_to_be32(153) >+#define TPM_ST_CLEAR cpu_to_be16(1) >+#define TPM_ST_STATE cpu_to_be16(2) >+#define TPM_ST_DEACTIVATED cpu_to_be16(3) >+static const struct tpm_input_header tpm_startup_header = { >+ .tag = TPM_TAG_RQU_COMMAND, >+ .length = cpu_to_be32(12), >+ .ordinal = TPM_ORD_STARTUP >+}; >+ > ssize_t tpm_getcap(struct device *dev, __be32 subcap_id, cap_t *cap, > const char *desc) > {
Purely cosmetic question, but why did you define this before the tpm_getcap and not tpm_startup? All the other definitions are made before they are used - so this should perhaps better be moved directly before tpm_startup. (Maybe we should move out these definitions to a common location? Header?)
>@@ -541,11 +560,27 @@ int tpm_get_timeouts(struct tpm_chip *chip) > tpm_cmd.params.getcap_in.cap = TPM_CAP_PROP; > tpm_cmd.params.getcap_in.subcap_size = cpu_to_be32(4); > tpm_cmd.params.getcap_in.subcap = TPM_CAP_PROP_TIS_TIMEOUT; >+ rc = transmit_cmd(chip, &tpm_cmd, TPM_INTERNAL_RESULT_SIZE, 0);
Please use NULL instead of 0, otherwise sparse complains - so - rc = transmit_cmd(chip, &tpm_cmd, TPM_INTERNAL_RESULT_SIZE, 0); + rc = transmit_cmd(chip, &tpm_cmd, TPM_INTERNAL_RESULT_SIZE, NULL);
>+ if (rc == TPM_ERR_INVALID_POSTINIT) { > ... >+ rc = transmit_cmd(chip, &tpm_cmd, TPM_INTERNAL_RESULT_SIZE, 0);
Same here please use NULL instead of 0, otherwise sparse complains - so - rc = transmit_cmd(chip, &tpm_cmd, TPM_INTERNAL_RESULT_SIZE, 0); + rc = transmit_cmd(chip, &tpm_cmd, TPM_INTERNAL_RESULT_SIZE, NULL);
>diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h > extern ssize_t tpm_show_pubek(struct device *, struct device_attribute *attr, >@@ -291,6 +292,10 @@ struct tpm_getrandom_in { > __be32 num_bytes; > }__attribute__((packed)); > >+struct tpm_startup_in { >+ __be16 startup_type; >+} __packed;
All the other user __attribute__((packed)); Care to change to be consistent?
Apart from these three minor topics also a Reviewed-by: Peter Huewe <peter.huewe@infineon.com>
Thanks, Peter
| |