Re: [tpmdd-devel] [PATCH resend] TPM: Issue TPM_STARTUP at driver load if the TPM has not been started

[<Pet...@in...>]

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 <pet...@in...>

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 <pet...@in...>

Thanks,
Peter