xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
* Re: [PATCH] vTPM: Fix Atmel timeout bug.
  2014-10-30 13:05 [PATCH] vTPM: Fix Atmel timeout bug Emil Condrea
@ 2014-10-30 12:58 ` Andrew Cooper
  2014-10-30 13:48   ` Emil Condrea
  0 siblings, 1 reply; 10+ messages in thread
From: Andrew Cooper @ 2014-10-30 12:58 UTC (permalink / raw)
  To: Emil Condrea, xen-devel; +Cc: dgdegra

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@gmail.com>
> ---
>  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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH] vTPM: Fix Atmel timeout bug.
@ 2014-10-30 13:05 Emil Condrea
  2014-10-30 12:58 ` Andrew Cooper
  0 siblings, 1 reply; 10+ messages in thread
From: Emil Condrea @ 2014-10-30 13:05 UTC (permalink / raw)
  To: xen-devel; +Cc: dgdegra, emilcondrea

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@gmail.com>
---
 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');
+   
    timeout = be32_to_cpu(timeout_cap->b);
    if (timeout)
       chip->timeout_b = MICROSECS(timeout * scale); /*Convert to msec */
+   ADJUST_TIMEOUTS_TO_STANDARD(chip->timeout_b,MILLISECS(TIS_LONG_TIMEOUT),'b');
+
    timeout = be32_to_cpu(timeout_cap->c);
    if (timeout)
       chip->timeout_c = MICROSECS(timeout * scale); /*Convert to msec */
+   ADJUST_TIMEOUTS_TO_STANDARD(chip->timeout_c,MILLISECS(TIS_SHORT_TIMEOUT),'c');
+
    timeout = be32_to_cpu(timeout_cap->d);
    if (timeout)
       chip->timeout_d = MICROSECS(timeout * scale); /*Convert to msec */
+   ADJUST_TIMEOUTS_TO_STANDARD(chip->timeout_d,MILLISECS(TIS_SHORT_TIMEOUT),'d');
 
 duration:
    tpm_cmd.header.in = tpm_getcap_header;
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH] vTPM: Fix Atmel timeout bug.
  2014-10-30 12:58 ` Andrew Cooper
@ 2014-10-30 13:48   ` Emil Condrea
  2014-11-04 10:15     ` Ian Campbell
  0 siblings, 1 reply; 10+ messages in thread
From: Emil Condrea @ 2014-10-30 13:48 UTC (permalink / raw)
  To: Andrew Cooper; +Cc: dgdegra, xen-devel


[-- Attachment #1.1: Type: text/plain, Size: 2860 bytes --]

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@citrix.com>
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@gmail.com>
> > ---
> >  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
>
>

[-- Attachment #1.2: Type: text/html, Size: 4584 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] vTPM: Fix Atmel timeout bug.
  2014-10-30 13:48   ` Emil Condrea
@ 2014-11-04 10:15     ` Ian Campbell
  2014-11-06 22:01       ` Daniel De Graaf
  0 siblings, 1 reply; 10+ messages in thread
From: Ian Campbell @ 2014-11-04 10:15 UTC (permalink / raw)
  To: Emil Condrea; +Cc: Andrew Cooper, dgdegra, xen-devel

On Thu, 2014-10-30 at 15:48 +0200, Emil Condrea wrote:
> 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 ?

jiffies are a Linux specific concept which mini-os doesn't share.

Daniel, do you have any opinion on this patch?

It seems like the Linux fix is made only for the specifically broken
platform. That seems to make sense to me since presumably other systems
report short timeouts which they can indeed cope with. It's only Atmel
which brokenly reports something it cannot handle.

Ian.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] vTPM: Fix Atmel timeout bug.
  2014-11-04 10:15     ` Ian Campbell
@ 2014-11-06 22:01       ` Daniel De Graaf
  2014-11-07 10:45         ` Emil Condrea
  2014-11-10 12:01         ` Ian Campbell
  0 siblings, 2 replies; 10+ messages in thread
From: Daniel De Graaf @ 2014-11-06 22:01 UTC (permalink / raw)
  To: Ian Campbell, Emil Condrea; +Cc: Andrew Cooper, xen-devel

On 11/04/2014 05:15 AM, Ian Campbell wrote:
> On Thu, 2014-10-30 at 15:48 +0200, Emil Condrea wrote:
>> 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 ?
>
> jiffies are a Linux specific concept which mini-os doesn't share.
>
> Daniel, do you have any opinion on this patch?
>
> It seems like the Linux fix is made only for the specifically broken
> platform. That seems to make sense to me since presumably other systems
> report short timeouts which they can indeed cope with. It's only Atmel
> which brokenly reports something it cannot handle.
>
> Ian.

I agree that an adjustment is needed when values are too short.  Adjusting
in all cases is not quite as nice as only fixing the broken TPMs, but it
is a lot simpler.  It also doesn't seem harmful to have the timeouts be
too large in the driver: a properly functioning TPM will not time out its
requests in any case, so the user won't notice normally, and the default
short timeout is 0.75 seconds - very few people will complain if they have
to wait that long to get a timeout instead of what their TPM actually uses.

-- 
Daniel De Graaf
National Security Agency

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] vTPM: Fix Atmel timeout bug.
  2014-11-06 22:01       ` Daniel De Graaf
@ 2014-11-07 10:45         ` Emil Condrea
  2014-11-10 12:01         ` Ian Campbell
  1 sibling, 0 replies; 10+ messages in thread
From: Emil Condrea @ 2014-11-07 10:45 UTC (permalink / raw)
  To: Daniel De Graaf; +Cc: Andrew Cooper, Ian Campbell, xen-devel


[-- Attachment #1.1: Type: text/plain, Size: 2374 bytes --]

Daniel,
So, is the patch going to be approved(adjusting all short timeouts) or it
needs to be modified?

On Fri, Nov 7, 2014 at 12:01 AM, Daniel De Graaf <dgdegra@tycho.nsa.gov>
wrote:

> On 11/04/2014 05:15 AM, Ian Campbell wrote:
>
>> On Thu, 2014-10-30 at 15:48 +0200, Emil Condrea wrote:
>>
>>> 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 ?
>>>
>>
>> jiffies are a Linux specific concept which mini-os doesn't share.
>>
>> Daniel, do you have any opinion on this patch?
>>
>> It seems like the Linux fix is made only for the specifically broken
>> platform. That seems to make sense to me since presumably other systems
>> report short timeouts which they can indeed cope with. It's only Atmel
>> which brokenly reports something it cannot handle.
>>
>> Ian.
>>
>
> I agree that an adjustment is needed when values are too short.  Adjusting
> in all cases is not quite as nice as only fixing the broken TPMs, but it
> is a lot simpler.  It also doesn't seem harmful to have the timeouts be
> too large in the driver: a properly functioning TPM will not time out its
> requests in any case, so the user won't notice normally, and the default
> short timeout is 0.75 seconds - very few people will complain if they have
> to wait that long to get a timeout instead of what their TPM actually uses.
>
> --
> Daniel De Graaf
> National Security Agency
>

[-- Attachment #1.2: Type: text/html, Size: 3439 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] vTPM: Fix Atmel timeout bug.
  2014-11-06 22:01       ` Daniel De Graaf
  2014-11-07 10:45         ` Emil Condrea
@ 2014-11-10 12:01         ` Ian Campbell
  2014-11-10 14:35           ` Konrad Rzeszutek Wilk
  2014-11-10 22:10           ` Daniel De Graaf
  1 sibling, 2 replies; 10+ messages in thread
From: Ian Campbell @ 2014-11-10 12:01 UTC (permalink / raw)
  To: Daniel De Graaf, Konrad Rzeszutek Wilk
  Cc: Andrew Cooper, Emil Condrea, xen-devel

On Thu, 2014-11-06 at 17:01 -0500, Daniel De Graaf wrote:
> On 11/04/2014 05:15 AM, Ian Campbell wrote:
> > On Thu, 2014-10-30 at 15:48 +0200, Emil Condrea wrote:
> >> 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 ?
> >
> > jiffies are a Linux specific concept which mini-os doesn't share.
> >
> > Daniel, do you have any opinion on this patch?
> >
> > It seems like the Linux fix is made only for the specifically broken
> > platform. That seems to make sense to me since presumably other systems
> > report short timeouts which they can indeed cope with. It's only Atmel
> > which brokenly reports something it cannot handle.
> >
> > Ian.
> 
> I agree that an adjustment is needed when values are too short.  Adjusting
> in all cases is not quite as nice as only fixing the broken TPMs, but it
> is a lot simpler.  It also doesn't seem harmful to have the timeouts be
> too large in the driver: a properly functioning TPM will not time out its
> requests in any case, so the user won't notice normally, and the default
> short timeout is 0.75 seconds - very few people will complain if they have
> to wait that long to get a timeout instead of what their TPM actually uses.

Can we take that as an ack?

Also needs the ok from Konrad as release manager. AIUI this is a bugfix
for a particular piece of h/w and as Daniel explains above the downside
is that sometimes someone might need to wait 0.75s for a timeout instead
of something shorter.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] vTPM: Fix Atmel timeout bug.
  2014-11-10 12:01         ` Ian Campbell
@ 2014-11-10 14:35           ` Konrad Rzeszutek Wilk
  2014-11-14 10:34             ` Ian Campbell
  2014-11-10 22:10           ` Daniel De Graaf
  1 sibling, 1 reply; 10+ messages in thread
From: Konrad Rzeszutek Wilk @ 2014-11-10 14:35 UTC (permalink / raw)
  To: Ian Campbell; +Cc: Andrew Cooper, Daniel De Graaf, Emil Condrea, xen-devel

On Mon, Nov 10, 2014 at 12:01:59PM +0000, Ian Campbell wrote:
> On Thu, 2014-11-06 at 17:01 -0500, Daniel De Graaf wrote:
> > On 11/04/2014 05:15 AM, Ian Campbell wrote:
> > > On Thu, 2014-10-30 at 15:48 +0200, Emil Condrea wrote:
> > >> 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 ?
> > >
> > > jiffies are a Linux specific concept which mini-os doesn't share.
> > >
> > > Daniel, do you have any opinion on this patch?
> > >
> > > It seems like the Linux fix is made only for the specifically broken
> > > platform. That seems to make sense to me since presumably other systems
> > > report short timeouts which they can indeed cope with. It's only Atmel
> > > which brokenly reports something it cannot handle.
> > >
> > > Ian.
> > 
> > I agree that an adjustment is needed when values are too short.  Adjusting
> > in all cases is not quite as nice as only fixing the broken TPMs, but it
> > is a lot simpler.  It also doesn't seem harmful to have the timeouts be
> > too large in the driver: a properly functioning TPM will not time out its
> > requests in any case, so the user won't notice normally, and the default
> > short timeout is 0.75 seconds - very few people will complain if they have
> > to wait that long to get a timeout instead of what their TPM actually uses.
> 
> Can we take that as an ack?
> 
> Also needs the ok from Konrad as release manager. AIUI this is a bugfix
> for a particular piece of h/w and as Daniel explains above the downside
> is that sometimes someone might need to wait 0.75s for a timeout instead
> of something shorter.

Release-Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] vTPM: Fix Atmel timeout bug.
  2014-11-10 12:01         ` Ian Campbell
  2014-11-10 14:35           ` Konrad Rzeszutek Wilk
@ 2014-11-10 22:10           ` Daniel De Graaf
  1 sibling, 0 replies; 10+ messages in thread
From: Daniel De Graaf @ 2014-11-10 22:10 UTC (permalink / raw)
  To: Ian Campbell, Konrad Rzeszutek Wilk
  Cc: Andrew Cooper, Emil Condrea, xen-devel

On 11/10/2014 07:01 AM, Ian Campbell wrote:
> On Thu, 2014-11-06 at 17:01 -0500, Daniel De Graaf wrote:
>> On 11/04/2014 05:15 AM, Ian Campbell wrote:
>>> On Thu, 2014-10-30 at 15:48 +0200, Emil Condrea wrote:
>>>> 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 ?
>>>
>>> jiffies are a Linux specific concept which mini-os doesn't share.
>>>
>>> Daniel, do you have any opinion on this patch?
>>>
>>> It seems like the Linux fix is made only for the specifically broken
>>> platform. That seems to make sense to me since presumably other systems
>>> report short timeouts which they can indeed cope with. It's only Atmel
>>> which brokenly reports something it cannot handle.
>>>
>>> Ian.
>>
>> I agree that an adjustment is needed when values are too short.  Adjusting
>> in all cases is not quite as nice as only fixing the broken TPMs, but it
>> is a lot simpler.  It also doesn't seem harmful to have the timeouts be
>> too large in the driver: a properly functioning TPM will not time out its
>> requests in any case, so the user won't notice normally, and the default
>> short timeout is 0.75 seconds - very few people will complain if they have
>> to wait that long to get a timeout instead of what their TPM actually uses.
>
> Can we take that as an ack?

Yes.  I was going to check to see if the printks would cause people to see
problems where the timeouts were already reasonable, but since the mini-os
driver is already quite verbose, this should not be a problem.  It might
be nice to output the old/new value of the timeout, but that's mostly for
the curious rather than actually necessary.

> Also needs the ok from Konrad as release manager. AIUI this is a bugfix
> for a particular piece of h/w and as Daniel explains above the downside
> is that sometimes someone might need to wait 0.75s for a timeout instead
> of something shorter.

-- 
Daniel De Graaf
National Security Agency

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] vTPM: Fix Atmel timeout bug.
  2014-11-10 14:35           ` Konrad Rzeszutek Wilk
@ 2014-11-14 10:34             ` Ian Campbell
  0 siblings, 0 replies; 10+ messages in thread
From: Ian Campbell @ 2014-11-14 10:34 UTC (permalink / raw)
  To: Konrad Rzeszutek Wilk
  Cc: Andrew Cooper, Daniel De Graaf, Emil Condrea, xen-devel

On Mon, 2014-11-10 at 09:35 -0500, Konrad Rzeszutek Wilk wrote:
> Release-Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Applied with Daniel's ack.

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2014-11-14 10:34 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-10-30 13:05 [PATCH] vTPM: Fix Atmel timeout bug Emil Condrea
2014-10-30 12:58 ` Andrew Cooper
2014-10-30 13:48   ` Emil Condrea
2014-11-04 10:15     ` Ian Campbell
2014-11-06 22:01       ` Daniel De Graaf
2014-11-07 10:45         ` Emil Condrea
2014-11-10 12:01         ` Ian Campbell
2014-11-10 14:35           ` Konrad Rzeszutek Wilk
2014-11-14 10:34             ` Ian Campbell
2014-11-10 22:10           ` Daniel De Graaf

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).