[PATCH 1/3] tpm: move TPM_POLL_SLEEP from tpm_tis_core.c to tpm.h

[PATCH 1/3] tpm: move TPM_POLL_SLEEP from tpm_tis_core.c to tpm.h

[Nayna Jain]

This patch moves TPM_POLL_SLEEP from tpm_tis_core.c to tpm.h, renaming
it to TPM_TIMEOUT_POLL, to follow the existing enum naming
conventions.

Signed-off-by: Nayna Jain <nayna@linux.vnet.ibm.com>
---
 drivers/char/tpm/tpm.h          |  3 ++-
 drivers/char/tpm/tpm_tis_core.c | 10 ++--------
 2 files changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index f895fba4e20d..7e797377e1eb 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -53,7 +53,8 @@ enum tpm_const {
 enum tpm_timeout {
 	TPM_TIMEOUT = 5,	/* msecs */
 	TPM_TIMEOUT_RETRY = 100, /* msecs */
-	TPM_TIMEOUT_RANGE_US = 300	/* usecs */
+	TPM_TIMEOUT_RANGE_US = 300,	/* usecs */
+	TPM_TIMEOUT_POLL = 1	/* msecs */
 };
 
 /* TPM addresses */
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 183a5f54d875..dc474e7244a6 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -31,12 +31,6 @@
 #include "tpm.h"
 #include "tpm_tis_core.h"
 
-/* This is a polling delay to check for status and burstcount.
- * As per ddwg input, expectation is that status check and burstcount
- * check should return within few usecs.
- */
-#define TPM_POLL_SLEEP	1  /* msec */
-
 static void tpm_tis_clkrun_enable(struct tpm_chip *chip, bool value);
 
 static bool wait_for_tpm_stat_cond(struct tpm_chip *chip, u8 mask,
@@ -90,7 +84,7 @@ static int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask,
 		}
 	} else {
 		do {
-			tpm_msleep(TPM_POLL_SLEEP);
+			tpm_msleep(TPM_TIMEOUT_POLL);
 			status = chip->ops->status(chip);
 			if ((status & mask) == mask)
 				return 0;
@@ -232,7 +226,7 @@ static int get_burstcount(struct tpm_chip *chip)
 		burstcnt = (value >> 8) & 0xFFFF;
 		if (burstcnt)
 			return burstcnt;
-		tpm_msleep(TPM_POLL_SLEEP);
+		tpm_msleep(TPM_TIMEOUT_POLL);
 	} while (time_before(jiffies, stop));
 	return -EBUSY;
 }
-- 
2.13.3

[PATCH 2/3] tpm: reduce poll sleep time between send() and recv() in tpm_transmit()

[Nayna Jain]

In tpm_transmit, after send(), the code checks for status in a loop
with polling every 5msec. It is expected that the tpm might return
earlier than 5msec, so it might be adding to unnecessary delay.

This patch reduces the polling sleep time from 5msec to 1msec.

After this change, performance on a TPM 1.2 with an 8 byte
burstcount for 1000 extends improved from ~14sec to ~10.7sec.

Signed-off-by: Nayna Jain <nayna@linux.vnet.ibm.com>
---
 drivers/char/tpm/tpm-interface.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 76df4fbcf089..2d22f981f0c9 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -470,7 +470,7 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
 			goto out;
 		}
 
-		tpm_msleep(TPM_TIMEOUT);
+		tpm_msleep(TPM_TIMEOUT_POLL);
 		rmb();
 	} while (time_before(jiffies, stop));
 
-- 
2.13.3

[RFC PATCH 3/3] tpm: tpm_msleep() with finer granularity improves performance

[Nayna Jain]

When 'commit 9f3fc7bcddcb ("tpm: replace msleep() with  usleep_range()
in TPM 1.2/2.0 generic drivers")' was upstreamed, it replaced the
msleep() calls with usleep_range(), but did not change the
granularity of the calls. They're still defined in terms of msec.
Test results show that refining the granularity further improves
the performance. We're posting this patch as an RFC to show that there
needs to be another function which allows finer granularity.

After this change, performance on a TPM 1.2 with an 8 byte
burstcount for 1000 extends improved from ~10.7sec to ~6.9sec.

Signed-off-by: Nayna Jain <nayna@linux.vnet.ibm.com>
---
 drivers/char/tpm/tpm.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 7e797377e1eb..8cad6bfc5f46 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -522,8 +522,7 @@ int tpm_pm_resume(struct device *dev);
 
 static inline void tpm_msleep(unsigned int delay_msec)
 {
-	usleep_range((delay_msec * 1000) - TPM_TIMEOUT_RANGE_US,
-		     delay_msec * 1000);
+	usleep_range((delay_msec * 1000) / 10, (delay_msec * 1000) / 2);
 };
 
 struct tpm_chip *tpm_chip_find_get(struct tpm_chip *chip);
-- 
2.13.3

Re: [PATCH 1/3] tpm: move TPM_POLL_SLEEP from tpm_tis_core.c to tpm.h

[Jarkko Sakkinen]

Hi

On Wed, Feb 28, 2018 at 02:18:26PM -0500, Nayna Jain wrote:
> This patch moves TPM_POLL_SLEEP from tpm_tis_core.c to tpm.h, renaming
> it to TPM_TIMEOUT_POLL, to follow the existing enum naming
> conventions.
> 
> Signed-off-by: Nayna Jain <nayna@linux.vnet.ibm.com>

The cover letter is missing. Are this meant to be a patch set or
individual patches? I'll check these anyway.

> ---
>  drivers/char/tpm/tpm.h          |  3 ++-
>  drivers/char/tpm/tpm_tis_core.c | 10 ++--------
>  2 files changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index f895fba4e20d..7e797377e1eb 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -53,7 +53,8 @@ enum tpm_const {
>  enum tpm_timeout {
>  	TPM_TIMEOUT = 5,	/* msecs */
>  	TPM_TIMEOUT_RETRY = 100, /* msecs */
> -	TPM_TIMEOUT_RANGE_US = 300	/* usecs */
> +	TPM_TIMEOUT_RANGE_US = 300,	/* usecs */

What is happening here?

> +	TPM_TIMEOUT_POLL = 1	/* msecs */
>  };
>  
>  /* TPM addresses */
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index 183a5f54d875..dc474e7244a6 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -31,12 +31,6 @@
>  #include "tpm.h"
>  #include "tpm_tis_core.h"
>  
> -/* This is a polling delay to check for status and burstcount.
> - * As per ddwg input, expectation is that status check and burstcount
> - * check should return within few usecs.
> - */
> -#define TPM_POLL_SLEEP	1  /* msec */
> -
>  static void tpm_tis_clkrun_enable(struct tpm_chip *chip, bool value);
>  
>  static bool wait_for_tpm_stat_cond(struct tpm_chip *chip, u8 mask,
> @@ -90,7 +84,7 @@ static int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask,
>  		}
>  	} else {
>  		do {
> -			tpm_msleep(TPM_POLL_SLEEP);
> +			tpm_msleep(TPM_TIMEOUT_POLL);
>  			status = chip->ops->status(chip);
>  			if ((status & mask) == mask)
>  				return 0;
> @@ -232,7 +226,7 @@ static int get_burstcount(struct tpm_chip *chip)
>  		burstcnt = (value >> 8) & 0xFFFF;
>  		if (burstcnt)
>  			return burstcnt;
> -		tpm_msleep(TPM_POLL_SLEEP);
> +		tpm_msleep(TPM_TIMEOUT_POLL);
>  	} while (time_before(jiffies, stop));
>  	return -EBUSY;
>  }
> -- 
> 2.13.3
> 

Otherwise, looks fine.

/Jarkko

Re: [PATCH 2/3] tpm: reduce poll sleep time between send() and recv() in tpm_transmit()

[Jarkko Sakkinen]

On Wed, Feb 28, 2018 at 02:18:27PM -0500, Nayna Jain wrote:
> In tpm_transmit, after send(), the code checks for status in a loop

Maybe cutting hairs now but please just use the actual function name
instead of send(). Just makes the commit log more useful asset.

> -		tpm_msleep(TPM_TIMEOUT);
> +		tpm_msleep(TPM_TIMEOUT_POLL);

What about just calling schedule()?

/Jarkko

Re: [RFC PATCH 3/3] tpm: tpm_msleep() with finer granularity improves performance

[Jarkko Sakkinen]

On Wed, Feb 28, 2018 at 02:18:28PM -0500, Nayna Jain wrote:
> When 'commit 9f3fc7bcddcb ("tpm: replace msleep() with  usleep_range()
> in TPM 1.2/2.0 generic drivers")' was upstreamed, it replaced the

"was upstreamed" is redundant information. If you speak about commit ID,
it is expected to be in the mainline. Why there is "'" before the word
'commit'?

Just write

  In commit 9f3fc7bcddcb ("tpm: replace msleep() with  usleep_range()
  in TPM 1.2/2.0 generic drivers")' msleep() was replaced with
  usleep_range().

> msleep() calls with usleep_range(), but did not change the
> granularity of the calls. They're still defined in terms of msec.
> Test results show that refining the granularity further improves
> the performance. We're posting this patch as an RFC to show that there
> needs to be another function which allows finer granularity.
> 
> After this change, performance on a TPM 1.2 with an 8 byte
> burstcount for 1000 extends improved from ~10.7sec to ~6.9sec.

Environment where this result was achieved would be mandatory.

> Signed-off-by: Nayna Jain <nayna@linux.vnet.ibm.com>
> ---
>  drivers/char/tpm/tpm.h | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index 7e797377e1eb..8cad6bfc5f46 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -522,8 +522,7 @@ int tpm_pm_resume(struct device *dev);
>  
>  static inline void tpm_msleep(unsigned int delay_msec)
>  {
> -	usleep_range((delay_msec * 1000) - TPM_TIMEOUT_RANGE_US,
> -		     delay_msec * 1000);
> +	usleep_range((delay_msec * 1000) / 10, (delay_msec * 1000) / 2);

Shouldn't the max be 'delay_msec * 1000'? Where do these numbers
come from?

/Jarkko

Re: [PATCH 1/3] tpm: move TPM_POLL_SLEEP from tpm_tis_core.c to tpm.h

[Nayna Jain]

On 03/01/2018 02:07 PM, Jarkko Sakkinen wrote:
> Hi
>
> On Wed, Feb 28, 2018 at 02:18:26PM -0500, Nayna Jain wrote:
>> This patch moves TPM_POLL_SLEEP from tpm_tis_core.c to tpm.h, renaming
>> it to TPM_TIMEOUT_POLL, to follow the existing enum naming
>> conventions.
>>
>> Signed-off-by: Nayna Jain <nayna@linux.vnet.ibm.com>
> The cover letter is missing. Are this meant to be a patch set or
> individual patches? I'll check these anyway.

These patches continue to improve TPM performance.
The first patch exports and renames the timeout enum for polling, that 
is subsequently used in the second patch.
The third patch is posted as an RFC  for further performance improvement 
by improving granularity.
>
>> ---
>>   drivers/char/tpm/tpm.h          |  3 ++-
>>   drivers/char/tpm/tpm_tis_core.c | 10 ++--------
>>   2 files changed, 4 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
>> index f895fba4e20d..7e797377e1eb 100644
>> --- a/drivers/char/tpm/tpm.h
>> +++ b/drivers/char/tpm/tpm.h
>> @@ -53,7 +53,8 @@ enum tpm_const {
>>   enum tpm_timeout {
>>   	TPM_TIMEOUT = 5,	/* msecs */
>>   	TPM_TIMEOUT_RETRY = 100, /* msecs */
>> -	TPM_TIMEOUT_RANGE_US = 300	/* usecs */
>> +	TPM_TIMEOUT_RANGE_US = 300,	/* usecs */
> What is happening here?
Addition of comma to add next enum value.

Thanks & Regards,
     - Nayna

>
>> +	TPM_TIMEOUT_POLL = 1	/* msecs */
>>   };
>>   
>>   /* TPM addresses */
>> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
>> index 183a5f54d875..dc474e7244a6 100644
>> --- a/drivers/char/tpm/tpm_tis_core.c
>> +++ b/drivers/char/tpm/tpm_tis_core.c
>> @@ -31,12 +31,6 @@
>>   #include "tpm.h"
>>   #include "tpm_tis_core.h"
>>   
>> -/* This is a polling delay to check for status and burstcount.
>> - * As per ddwg input, expectation is that status check and burstcount
>> - * check should return within few usecs.
>> - */
>> -#define TPM_POLL_SLEEP	1  /* msec */
>> -
>>   static void tpm_tis_clkrun_enable(struct tpm_chip *chip, bool value);
>>   
>>   static bool wait_for_tpm_stat_cond(struct tpm_chip *chip, u8 mask,
>> @@ -90,7 +84,7 @@ static int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask,
>>   		}
>>   	} else {
>>   		do {
>> -			tpm_msleep(TPM_POLL_SLEEP);
>> +			tpm_msleep(TPM_TIMEOUT_POLL);
>>   			status = chip->ops->status(chip);
>>   			if ((status & mask) == mask)
>>   				return 0;
>> @@ -232,7 +226,7 @@ static int get_burstcount(struct tpm_chip *chip)
>>   		burstcnt = (value >> 8) & 0xFFFF;
>>   		if (burstcnt)
>>   			return burstcnt;
>> -		tpm_msleep(TPM_POLL_SLEEP);
>> +		tpm_msleep(TPM_TIMEOUT_POLL);
>>   	} while (time_before(jiffies, stop));
>>   	return -EBUSY;
>>   }
>> -- 
>> 2.13.3
>>
> Otherwise, looks fine.
> /Jarkko
>

Re: [PATCH 2/3] tpm: reduce poll sleep time between send() and recv() in tpm_transmit()

[Nayna Jain]

On 03/01/2018 02:52 PM, Jarkko Sakkinen wrote:
> On Wed, Feb 28, 2018 at 02:18:27PM -0500, Nayna Jain wrote:
>> In tpm_transmit, after send(), the code checks for status in a loop
> Maybe cutting hairs now but please just use the actual function name
> instead of send(). Just makes the commit log more useful asset.
Sure, will do.
>
>> -		tpm_msleep(TPM_TIMEOUT);
>> +		tpm_msleep(TPM_TIMEOUT_POLL);
> What about just calling schedule()?
I'm not sure what you mean by "schedule()".  Are you suggesting instead 
of using usleep_range(),  using something with an even finer grain 
construct?

Thanks & Regards,
      - Nayna

>
> /Jarkko
>

Re: [RFC PATCH 3/3] tpm: tpm_msleep() with finer granularity improves performance

[Nayna Jain]

On 03/01/2018 03:28 PM, Jarkko Sakkinen wrote:
> On Wed, Feb 28, 2018 at 02:18:28PM -0500, Nayna Jain wrote:
>> When 'commit 9f3fc7bcddcb ("tpm: replace msleep() with  usleep_range()
>> in TPM 1.2/2.0 generic drivers")' was upstreamed, it replaced the
> "was upstreamed" is redundant information. If you speak about commit ID,
> it is expected to be in the mainline. Why there is "'" before the word
> 'commit'?
>
> Just write
>
>    In commit 9f3fc7bcddcb ("tpm: replace msleep() with  usleep_range()
>    in TPM 1.2/2.0 generic drivers")' msleep() was replaced with
>    usleep_range().
Yeah. Sure. Will do.
>
>> msleep() calls with usleep_range(), but did not change the
>> granularity of the calls. They're still defined in terms of msec.
>> Test results show that refining the granularity further improves
>> the performance. We're posting this patch as an RFC to show that there
>> needs to be another function which allows finer granularity.
>>
>> After this change, performance on a TPM 1.2 with an 8 byte
>> burstcount for 1000 extends improved from ~10.7sec to ~6.9sec.
> Environment where this result was achieved would be mandatory.
Sure.
It is an x86 based, locked down, single purpose closed system.
It has Infineon TPM 1.2 using LPC Bus.
>
>> Signed-off-by: Nayna Jain <nayna@linux.vnet.ibm.com>
>> ---
>>   drivers/char/tpm/tpm.h | 3 +--
>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
>> index 7e797377e1eb..8cad6bfc5f46 100644
>> --- a/drivers/char/tpm/tpm.h
>> +++ b/drivers/char/tpm/tpm.h
>> @@ -522,8 +522,7 @@ int tpm_pm_resume(struct device *dev);
>>   
>>   static inline void tpm_msleep(unsigned int delay_msec)
>>   {
>> -	usleep_range((delay_msec * 1000) - TPM_TIMEOUT_RANGE_US,
>> -		     delay_msec * 1000);
>> +	usleep_range((delay_msec * 1000) / 10, (delay_msec * 1000) / 2);
> Shouldn't the max be 'delay_msec * 1000'? Where do these numbers
> come from?
We don’t expect the patch to be upstreamed as is with the /10 and /2. 
Our point in posting
this was to show that msec is the wrong granularity for polling. And so 
we suggest to have another
sleep() function which can take timeouts in usecs.

The way timeouts are used in the driver is to sleep between polling for 
a specified amount of time.
Since not all TPM commands take the same time to execute, some of them 
might return much
earlier than others. In such cases, having those TPM commands use a 
polling granularity of
msecs is wrong, and adds cumulative delays. Since the polling loops for 
a specified amount
of time, which is defined by TCG Specification for each command, 
changing the granularity for
polling should not cause problems.

To obtain the performance improvements in the specified environment, 
minimizing the minimum
value of usleep_range() wasn’t enough. We found that changing the 
maximum value by /2 gave a
dramatic improvement, and pointed us in the direction of using a smaller 
granularity.

Thanks & Regards,
      - Nayna

>
> /Jarkko
>

Re: [PATCH 2/3] tpm: reduce poll sleep time between send() and recv() in tpm_transmit()

[Jarkko Sakkinen]

On Fri, Mar 02, 2018 at 12:26:35AM +0530, Nayna Jain wrote:
> 
> 
> On 03/01/2018 02:52 PM, Jarkko Sakkinen wrote:
> > On Wed, Feb 28, 2018 at 02:18:27PM -0500, Nayna Jain wrote:
> > > In tpm_transmit, after send(), the code checks for status in a loop
> > Maybe cutting hairs now but please just use the actual function name
> > instead of send(). Just makes the commit log more useful asset.
> Sure, will do.
> > 
> > > -		tpm_msleep(TPM_TIMEOUT);
> > > +		tpm_msleep(TPM_TIMEOUT_POLL);
> > What about just calling schedule()?
> I'm not sure what you mean by "schedule()".  Are you suggesting instead of
> using usleep_range(),  using something with an even finer grain construct?
> 
> Thanks & Regards,
>      - Nayna

kernel/sched/core.c

/Jarkko

Re: [PATCH 2/3] tpm: reduce poll sleep time between send() and recv() in tpm_transmit()

[Jarkko Sakkinen]

On Mon, Mar 05, 2018 at 12:56:33PM +0200, Jarkko Sakkinen wrote:
> On Fri, Mar 02, 2018 at 12:26:35AM +0530, Nayna Jain wrote:
> > 
> > 
> > On 03/01/2018 02:52 PM, Jarkko Sakkinen wrote:
> > > On Wed, Feb 28, 2018 at 02:18:27PM -0500, Nayna Jain wrote:
> > > > In tpm_transmit, after send(), the code checks for status in a loop
> > > Maybe cutting hairs now but please just use the actual function name
> > > instead of send(). Just makes the commit log more useful asset.
> > Sure, will do.
> > > 
> > > > -		tpm_msleep(TPM_TIMEOUT);
> > > > +		tpm_msleep(TPM_TIMEOUT_POLL);
> > > What about just calling schedule()?
> > I'm not sure what you mean by "schedule()".  Are you suggesting instead of
> > using usleep_range(),  using something with an even finer grain construct?
> > 
> > Thanks & Regards,
> >      - Nayna
> 
> kernel/sched/core.c

The question I'm trying ask to is: is it better to sleep such a short
time or just ask scheduler to schedule something else after each
iteration?

/Jarkko

Re: [PATCH 2/3] tpm: reduce poll sleep time between send() and recv() in tpm_transmit()

[Mimi Zohar]

On Mon, 2018-03-05 at 20:01 +0200, Jarkko Sakkinen wrote:
> On Mon, Mar 05, 2018 at 12:56:33PM +0200, Jarkko Sakkinen wrote:
> > On Fri, Mar 02, 2018 at 12:26:35AM +0530, Nayna Jain wrote:
> > > 
> > > 
> > > On 03/01/2018 02:52 PM, Jarkko Sakkinen wrote:
> > > > On Wed, Feb 28, 2018 at 02:18:27PM -0500, Nayna Jain wrote:
> > > > > In tpm_transmit, after send(), the code checks for status in a loop
> > > > Maybe cutting hairs now but please just use the actual function name
> > > > instead of send(). Just makes the commit log more useful asset.
> > > Sure, will do.
> > > > 
> > > > > -		tpm_msleep(TPM_TIMEOUT);
> > > > > +		tpm_msleep(TPM_TIMEOUT_POLL);
> > > > What about just calling schedule()?
> > > I'm not sure what you mean by "schedule()".  Are you suggesting instead of
> > > using usleep_range(),  using something with an even finer grain construct?
> > > 
> > > Thanks & Regards,
> > >      - Nayna
> > 
> > kernel/sched/core.c
> 
> The question I'm trying ask to is: is it better to sleep such a short
> time or just ask scheduler to schedule something else after each
> iteration?

I still don't understand why scheduling some work would be an
improvement.  We still need to loop, testing for the TPM command to
complete.

According to the schedule_hrtimeout_range() function comment,
schedule_hrtimeout_range() is both power and performance friendly.
 What we didn't realize is that the hrtimer *uses* the maximum range
to calculate the sleep time, but *may* return earlier based on the
minimum time.

This patch minimizes the tpm_msleep().  The subsequent patch in this
patch set shows that 1 msec isn't fine enough granularity.  I still
think calling usleep_range() is the right solution, but it needs to be
at a finer granularity than tpm_msleep() provides.

Mimi

Re: [PATCH 2/3] tpm: reduce poll sleep time between send() and recv() in tpm_transmit()

[Jarkko Sakkinen]

On Mon, 2018-03-05 at 14:07 -0500, Mimi Zohar wrote:
> On Mon, 2018-03-05 at 20:01 +0200, Jarkko Sakkinen wrote:
> > On Mon, Mar 05, 2018 at 12:56:33PM +0200, Jarkko Sakkinen wrote:
> > > On Fri, Mar 02, 2018 at 12:26:35AM +0530, Nayna Jain wrote:
> > > > 
> > > > 
> > > > On 03/01/2018 02:52 PM, Jarkko Sakkinen wrote:
> > > > > On Wed, Feb 28, 2018 at 02:18:27PM -0500, Nayna Jain wrote:
> > > > > > In tpm_transmit, after send(), the code checks for status in a loop
> > > > > 
> > > > > Maybe cutting hairs now but please just use the actual function name
> > > > > instead of send(). Just makes the commit log more useful asset.
> > > > 
> > > > Sure, will do.
> > > > > 
> > > > > > -		tpm_msleep(TPM_TIMEOUT);
> > > > > > +		tpm_msleep(TPM_TIMEOUT_POLL);
> > > > > 
> > > > > What about just calling schedule()?
> > > > 
> > > > I'm not sure what you mean by "schedule()".  Are you suggesting instead
> > > > of
> > > > using usleep_range(),  using something with an even finer grain
> > > > construct?
> > > > 
> > > > Thanks & Regards,
> > > >      - Nayna
> > > 
> > > kernel/sched/core.c
> > 
> > The question I'm trying ask to is: is it better to sleep such a short
> > time or just ask scheduler to schedule something else after each
> > iteration?
> 
> I still don't understand why scheduling some work would be an
> improvement.  We still need to loop, testing for the TPM command to
> complete.
> 
> According to the schedule_hrtimeout_range() function comment,
> schedule_hrtimeout_range() is both power and performance friendly.
>  What we didn't realize is that the hrtimer *uses* the maximum range
> to calculate the sleep time, but *may* return earlier based on the
> minimum time.
> 
> This patch minimizes the tpm_msleep().  The subsequent patch in this
> patch set shows that 1 msec isn't fine enough granularity.  I still
> think calling usleep_range() is the right solution, but it needs to be
> at a finer granularity than tpm_msleep() provides.
> 
> Mimi

We can move to usleep_range() in call sites where it makes sense instead
of adjusting tpm_msleep() implementation...

/Jarkko