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

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 <redacted>
---
 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

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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 <redacted>
---
 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

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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 <redacted>
---
 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

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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 <redacted>

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

What is happening here?

Otherwise, looks fine.

/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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.

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

/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 03/01/2018 02:07 PM, Jarkko Sakkinen wrote:

Hi

On Wed, Feb 28, 2018 at 02:18:26PM -0500, Nayna Jain wrote:

quoted

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 <redacted>

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.

quoted

---
  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

quoted

+	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

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 03/01/2018 02:52 PM, Jarkko Sakkinen wrote:

On Wed, Feb 28, 2018 at 02:18:27PM -0500, Nayna Jain wrote:

quoted

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.

quoted

-		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

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 03/01/2018 03:28 PM, Jarkko Sakkinen wrote:

On Wed, Feb 28, 2018 at 02:18:28PM -0500, Nayna Jain wrote:

quoted

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.

quoted

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.

quoted

Signed-off-by: Nayna Jain <redacted>
---
  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

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Fri, Mar 02, 2018 at 12:26:35AM +0530, Nayna Jain wrote:

On 03/01/2018 02:52 PM, Jarkko Sakkinen wrote:

quoted

On Wed, Feb 28, 2018 at 02:18:27PM -0500, Nayna Jain wrote:

quoted

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.

quoted

quoted

-		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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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:

quoted

On 03/01/2018 02:52 PM, Jarkko Sakkinen wrote:

quoted

On Wed, Feb 28, 2018 at 02:18:27PM -0500, Nayna Jain wrote:

quoted

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.

quoted

quoted

-		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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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:

quoted

On Fri, Mar 02, 2018 at 12:26:35AM +0530, Nayna Jain wrote:

quoted

On 03/01/2018 02:52 PM, Jarkko Sakkinen wrote:

quoted

On Wed, Feb 28, 2018 at 02:18:27PM -0500, Nayna Jain wrote:

quoted

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.

quoted

quoted

-		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

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Mon, 2018-03-05 at 14:07 -0500, Mimi Zohar wrote:

On Mon, 2018-03-05 at 20:01 +0200, Jarkko Sakkinen wrote:

quoted

On Mon, Mar 05, 2018 at 12:56:33PM +0200, Jarkko Sakkinen wrote:

quoted

On Fri, Mar 02, 2018 at 12:26:35AM +0530, Nayna Jain wrote:

quoted

On 03/01/2018 02:52 PM, Jarkko Sakkinen wrote:

quoted

On Wed, Feb 28, 2018 at 02:18:27PM -0500, Nayna Jain wrote:

quoted

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.

quoted

quoted

-		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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html