Re: [Xen-devel] [PATCH] vTPM: Fix Atmel timeout bug.

[Emil Condrea <emilcondrea@xxxxxxxxx>]

Of course we can use max, but I thought that it might be useful to have a prink to inform the user that the timeout was adjusted.

InÂinit_tpm_tis the default timeouts are set using:

/* Set default timeouts */ tpm->timeout_a = MILLISECS(TIS_SHORT_TIMEOUT);//750*1000000UL tpm->timeout_b = MILLISECS(TIS_LONG_TIMEOUT);//2000*1000000UL tpm->timeout_c = MILLISECS(TIS_SHORT_TIMEOUT); tpm->timeout_d = MILLISECS(TIS_SHORT_TIMEOUT);

But in kernel fix they are set as 750*1000 instead ofÂ750*1000000UL : https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/char/tpm/tpm_tis.c#n381

So if we want to integrate kernel changes I think we should use MICROSECS(TIS_SHORT_TIMEOUT) which is 750000

Also in kernel the default timeouts are initialized using msecs_to_jiffies which is different from MILLISECS macro.:Âhttps://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/char/tpm/tpm_tis.c#n548

Is there a certain reason for not usingÂmsecs_to_jiffiesÂ?

On Thu, Oct 30, 2014 at 2:58 PM, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:

On 30/10/14 13:05, Emil Condrea wrote:
> Some versions of Atmel TPMs provide invalid values for TPM_CAP_PROP_TIS_TIMEOUT query.
> Because timeouts are invalid, every other command after tpm_get_timeouts will fail.
> It is a known issue and it was fixed recently in linux kernel tpm_tis.c on 2014-07-29.
> This patch does not allow timeouts to be less than standard values.
> I tested it on a Dell Latitude E5520 and after making the changes I was able to start vtpmmgr-stubdom.
>
> Signed-off-by: Emil Condrea <emilcondrea@xxxxxxxxx>
> ---
>Â extras/mini-os/tpm_tis.c | 12 ++++++++++++
>Â 1 file changed, 12 insertions(+)
>
> diff --git a/extras/mini-os/tpm_tis.c b/extras/mini-os/tpm_tis.c
> index b067cb7..81d426a 100644
> --- a/extras/mini-os/tpm_tis.c
> +++ b/extras/mini-os/tpm_tis.c
> @@ -33,6 +33,11 @@
>Â #ifndef min
>Â Â Â Â#define min( a, b ) ( ((a) < (b)) ? (a) : (b) )
>Â #endif
> +#define ADJUST_TIMEOUTS_TO_STANDARD(initial,standard,timeout_no)Â Â Â Â Â Â Â Â Â Â Â\
> +Â Â Âif((initial) < (standard)){Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â\
> +Â Â Â Â Â Â Â(initial) = (standard);Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â\
> +Â Â Â Â Â Â Âprintk("Timeout %c was adjusted to standard value.\n",timeout_no);Â Â Â \
> +Â Â Â}
>
>Â #define TPM_HEADER_SIZE 10
>
> @@ -997,15 +1002,22 @@ int tpm_get_timeouts(struct tpm_chip *chip)
>Â Â Â}
>Â Â Âif (timeout)
>Â Â Â Â chip->timeout_a = MICROSECS(timeout * scale); /*Convert to msec */
> +Â ÂADJUST_TIMEOUTS_TO_STANDARD(chip->timeout_a,MILLISECS(TIS_SHORT_TIMEOUT),'a');

Surely each of these are just

chip->timeout_a = max(MICROSECS(timeout * scale),
MILLISECS(TIS_SHORT_TIMEOUT))

Also, are you certain that the units here are correct? The micro vs
milli seconds looks odd.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel