2017-09-06 13:01:33

by Nayna Jain

Subject: [PATCH v2 0/4] additional TPM performance improvements

After further discussions with the Device Driver working group (ddwg),
the following changes were made:

* Check for burstcount at least once to confirm the TPM is ready to accept
the data. Similarly, query for the TPM Expect status as sanity check at
the end.

* Make the sleep for status check during send() in the loop less than
5msec.

* Make the sleep in the loop while querying for burstcount less than
5msec.

Below is the list of patches along with the performance improvements
seen with a TPM 1.2 with an 8 byte burstcount for 1000 extends:

Patch |Improvement(time in sec)

tpm: ignore burstcount to improve tpm_tis | ~41 - ~14
send() performance.

tpm: define __wait_for_tpm_stat to specify | ~14 - ~10
variable polling sleep time

tpm: reduce tpm_msleep() time in | ~10 - ~9
get_burstcount()

tpm: modify tpm_msleep() function to have | ~9 - ~8
max range

Changelog v2:

* Add module parameter to handle ignoring of burst count during
tpm tis send() operation.
* Add improvements over sleep time to reduce delays.

Nayna Jain (4):
tpm: ignore burstcount to improve tpm_tis send() performance.
tpm: define __wait_for_tpm_stat to specify variable polling sleep time
tpm: reduce tpm_msleep() time in get_burstcount()
tpm: use tpm_msleep() value as max delay

Documentation/admin-guide/kernel-parameters.txt | 8 ++++++
drivers/char/tpm/tpm-interface.c | 15 ++++++++--
drivers/char/tpm/tpm.h | 7 +++--
drivers/char/tpm/tpm_tis_core.c | 37 +++++++++++++++++++------
4 files changed, 53 insertions(+), 14 deletions(-)

--
2.13.3

2017-09-06 12:57:28

by Nayna Jain

Subject: [PATCH v2 3/4] tpm: reduce tpm_msleep() time in get_burstcount()

Currently, get_burstcount() function sleeps for 5msec in a loop
before retrying for next query to burstcount. However, if it takes
lesser time for TPM to return, this 5 msec delay is longer
than necessary.

This patch replaces the tpm_msleep time from 5msec to 1msec.

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

Signed-off-by: Nayna Jain <[email protected]>
Acked-by: Mimi Zohar <[email protected]>
---
drivers/char/tpm/tpm_tis_core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index d1eab29cb447..d710bbc4608b 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -169,7 +169,7 @@ static int get_burstcount(struct tpm_chip *chip)
burstcnt = (value >> 8) & 0xFFFF;
if (burstcnt)
return burstcnt;
- tpm_msleep(TPM_TIMEOUT);
+ tpm_msleep(1);
} while (time_before(jiffies, stop));
return -EBUSY;
}
--
2.13.3

2017-09-06 12:57:38

by Nayna Jain

Subject: [PATCH 1/4] tpm: ignore burstcount to improve tpm_tis send() performance.

The TPM burstcount status indicates the number of bytes that can
be sent to the TPM without causing bus wait states. Effectively,
it is the number of empty bytes in the command FIFO. Further,
some TPMs have a static burstcount, when the value remains zero
until the entire FIFO is empty.

This patch adds an optimization to check for burstcount only once.
And if it is valid, it writes all the bytes at once, permitting
wait states. The performance of a 34 byte extend on a TPM 1.2 with
an 8 byte burstcount improved from 41 msec to 14 msec.

This functionality is enabled only by passing module
parameter ignore_burst_count=1. By default, this parameter
is disabled.

Suggested-by: Ken Goldman <[email protected]> in
conjunction with the TPM Device Driver work group.
Signed-off-by: Nayna Jain <[email protected]>
Acked-by: Mimi Zohar <[email protected]>
---
Documentation/admin-guide/kernel-parameters.txt | 8 ++++++++
drivers/char/tpm/tpm_tis_core.c | 24 +++++++++++++++++++++---
2 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 4e303be83df6..3c59bb91e1ee 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -1465,6 +1465,14 @@
mode generally follows that for the NaN encoding,
except where unsupported by hardware.

+ ignore_burst_count [TPM_TIS_CORE]
+ tpm_tis_core driver queries for the burstcount before
+ every send call in a loop. However, it causes delay to
+ the send command for TPMs with low burstcount value.
+ Setting this value to 1, will make driver to query for
+ burstcount only once in the loop to improve the
+ performance. By default, its value is set to 0.
+
ignore_loglevel [KNL]
Ignore loglevel setting - this will print /all/
kernel messages to the console. Useful for debugging.
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 63bc6c3b949e..6b9bf4c4d434 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -31,6 +31,11 @@
#include "tpm.h"
#include "tpm_tis_core.h"

+static bool ignore_burst_count = false;
+module_param(ignore_burst_count, bool, 0444);
+MODULE_PARM_DESC(ignore_burst_count,
+ "Ignore burstcount value while writing data");
+
/* Before we attempt to access the TPM we must see that the valid bit is set.
* The specification says that this bit is 0 at reset and remains 0 until the
* 'TPM has gone through its self test and initialization and has established
@@ -256,6 +261,7 @@ static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t len)
{
struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
int rc, status, burstcnt;
+ int sendcnt;
size_t count = 0;
bool itpm = priv->flags & TPM_TIS_ITPM_WORKAROUND;

@@ -271,19 +277,31 @@ static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t len)
}

while (count < len - 1) {
+
+ /*
+ * Get the initial burstcount to ensure TPM is ready to
+ * accept data, even when waiting for burstcount is disabled.
+ */
burstcnt = get_burstcount(chip);
if (burstcnt < 0) {
dev_err(&chip->dev, "Unable to read burstcount\n");
rc = burstcnt;
goto out_err;
}
- burstcnt = min_t(int, burstcnt, len - count - 1);
+
+ if (ignore_burst_count)
+ sendcnt = len - 1;
+ else
+ sendcnt = min_t(int, burstcnt, len - count - 1);
+
rc = tpm_tis_write_bytes(priv, TPM_DATA_FIFO(priv->locality),
- burstcnt, buf + count);
+ sendcnt, buf + count);
if (rc < 0)
goto out_err;

- count += burstcnt;
+ count += sendcnt;
+ if (ignore_burst_count)
+ continue;

if (wait_for_tpm_stat(chip, TPM_STS_VALID, chip->timeout_c,
&priv->int_queue, false) < 0) {
--
2.13.3

2017-09-06 12:57:47

by Nayna Jain

Subject: [PATCH 2/4] tpm: define __wait_for_tpm_stat to specify variable polling sleep time

The existing wait_for_tpm_stat() checks the chip status before
sleeping for 5 msec in a polling loop. For some functions although
the status isn't ready immediately, the status returns extremely
quickly. Waiting for 5 msec causes an unnecessary delay. An
example is the send() call in the tpms_tis driver.

This patch defines __wait_for_tpm_stat(), allowing the caller
to specify the polling sleep timeout value within the loop.
The existing wait_for_tpm_stat() becomes a wrapper for this
function.

Signed-off-by: Nayna Jain <[email protected]>
---
drivers/char/tpm/tpm-interface.c | 15 ++++++++++++---
drivers/char/tpm/tpm.h | 3 +++
drivers/char/tpm/tpm_tis_core.c | 11 ++++++-----
3 files changed, 21 insertions(+), 8 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 1d6729be4cd6..b23d006243b7 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -1050,8 +1050,9 @@ static bool wait_for_tpm_stat_cond(struct tpm_chip *chip, u8 mask,
return false;
}

-int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask, unsigned long timeout,
- wait_queue_head_t *queue, bool check_cancel)
+int __wait_for_tpm_stat(struct tpm_chip *chip, u8 mask, unsigned long timeout,
+ unsigned int poll_sleep, wait_queue_head_t *queue,
+ bool check_cancel)
{
unsigned long stop;
long rc;
@@ -1085,7 +1086,7 @@ int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask, unsigned long timeout,
}
} else {
do {
- tpm_msleep(TPM_TIMEOUT);
+ tpm_msleep(poll_sleep);
status = chip->ops->status(chip);
if ((status & mask) == mask)
return 0;
@@ -1093,6 +1094,14 @@ int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask, unsigned long timeout,
}
return -ETIME;
}
+EXPORT_SYMBOL_GPL(__wait_for_tpm_stat);
+
+int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask, unsigned long timeout,
+ wait_queue_head_t *queue, bool check_cancel)
+{
+ return __wait_for_tpm_stat(chip, mask, timeout, TPM_TIMEOUT,
+ queue, check_cancel);
+}
EXPORT_SYMBOL_GPL(wait_for_tpm_stat);

#define TPM_ORD_SAVESTATE 152
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 2d5466a72e40..eb2f8818eded 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -525,6 +525,9 @@ int tpm_do_selftest(struct tpm_chip *chip);
unsigned long tpm_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal);
int tpm_pm_suspend(struct device *dev);
int tpm_pm_resume(struct device *dev);
+int __wait_for_tpm_stat(struct tpm_chip *chip, u8 mask,
+ unsigned long timeout, unsigned int poll_sleep,
+ wait_queue_head_t *queue, bool check_cancel);
int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask, unsigned long timeout,
wait_queue_head_t *queue, bool check_cancel);

diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 6b9bf4c4d434..d1eab29cb447 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -268,8 +268,8 @@ static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t len)
status = tpm_tis_status(chip);
if ((status & TPM_STS_COMMAND_READY) == 0) {
tpm_tis_ready(chip);
- if (wait_for_tpm_stat
- (chip, TPM_STS_COMMAND_READY, chip->timeout_b,
+ if (__wait_for_tpm_stat
+ (chip, TPM_STS_COMMAND_READY, chip->timeout_b, 1,
&priv->int_queue, false) < 0) {
rc = -ETIME;
goto out_err;
@@ -303,7 +303,8 @@ static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t len)
if (ignore_burst_count)
continue;

- if (wait_for_tpm_stat(chip, TPM_STS_VALID, chip->timeout_c,
+ if (__wait_for_tpm_stat(chip, TPM_STS_VALID,
+ chip->timeout_c, 1,
&priv->int_queue, false) < 0) {
rc = -ETIME;
goto out_err;
@@ -320,8 +321,8 @@ static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t len)
if (rc < 0)
goto out_err;

- if (wait_for_tpm_stat(chip, TPM_STS_VALID, chip->timeout_c,
- &priv->int_queue, false) < 0) {
+ if (__wait_for_tpm_stat(chip, TPM_STS_VALID, chip->timeout_c,
+ 1, &priv->int_queue, false) < 0) {
rc = -ETIME;
goto out_err;
}
--
2.13.3

2017-09-06 12:58:30

by Nayna Jain

Subject: [PATCH v2 1/4] tpm: ignore burstcount to improve tpm_tis send() performance.

The TPM burstcount status indicates the number of bytes that can
be sent to the TPM without causing bus wait states. Effectively,
it is the number of empty bytes in the command FIFO. Further,
some TPMs have a static burstcount, when the value remains zero
until the entire FIFO is empty.

This patch adds an optimization to check for burstcount only once.
And if it is valid, it writes all the bytes at once, permitting
wait states. The performance of a 34 byte extend on a TPM 1.2 with
an 8 byte burstcount improved from 41 msec to 14 msec.

This functionality is enabled only by passing module
parameter ignore_burst_count=1. By default, this parameter
is disabled.

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

Suggested-by: Ken Goldman <[email protected]> in
conjunction with the TPM Device Driver work group.
Signed-off-by: Nayna Jain <[email protected]>
Acked-by: Mimi Zohar <[email protected]>
---
Documentation/admin-guide/kernel-parameters.txt | 8 ++++++++
drivers/char/tpm/tpm_tis_core.c | 24 +++++++++++++++++++++---
2 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 4e303be83df6..3c59bb91e1ee 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -1465,6 +1465,14 @@
mode generally follows that for the NaN encoding,
except where unsupported by hardware.

+ ignore_burst_count [TPM_TIS_CORE]
+ tpm_tis_core driver queries for the burstcount before
+ every send call in a loop. However, it causes delay to
+ the send command for TPMs with low burstcount value.
+ Setting this value to 1, will make driver to query for
+ burstcount only once in the loop to improve the
+ performance. By default, its value is set to 0.
+
ignore_loglevel [KNL]
Ignore loglevel setting - this will print /all/
kernel messages to the console. Useful for debugging.
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 63bc6c3b949e..6b9bf4c4d434 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -31,6 +31,11 @@
#include "tpm.h"
#include "tpm_tis_core.h"

+static bool ignore_burst_count = false;
+module_param(ignore_burst_count, bool, 0444);
+MODULE_PARM_DESC(ignore_burst_count,
+ "Ignore burstcount value while writing data");
+
/* Before we attempt to access the TPM we must see that the valid bit is set.
* The specification says that this bit is 0 at reset and remains 0 until the
* 'TPM has gone through its self test and initialization and has established
@@ -256,6 +261,7 @@ static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t len)
{
struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
int rc, status, burstcnt;
+ int sendcnt;
size_t count = 0;
bool itpm = priv->flags & TPM_TIS_ITPM_WORKAROUND;

@@ -271,19 +277,31 @@ static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t len)
}

while (count < len - 1) {
+
+ /*
+ * Get the initial burstcount to ensure TPM is ready to
+ * accept data, even when waiting for burstcount is disabled.
+ */
burstcnt = get_burstcount(chip);
if (burstcnt < 0) {
dev_err(&chip->dev, "Unable to read burstcount\n");
rc = burstcnt;
goto out_err;
}
- burstcnt = min_t(int, burstcnt, len - count - 1);
+
+ if (ignore_burst_count)
+ sendcnt = len - 1;
+ else
+ sendcnt = min_t(int, burstcnt, len - count - 1);
+
rc = tpm_tis_write_bytes(priv, TPM_DATA_FIFO(priv->locality),
- burstcnt, buf + count);
+ sendcnt, buf + count);
if (rc < 0)
goto out_err;

- count += burstcnt;
+ count += sendcnt;
+ if (ignore_burst_count)
+ continue;

if (wait_for_tpm_stat(chip, TPM_STS_VALID, chip->timeout_c,
&priv->int_queue, false) < 0) {
--
2.13.3

2017-09-06 12:58:38

by Nayna Jain

Subject: [PATCH v2 2/4] tpm: define __wait_for_tpm_stat to specify variable polling sleep time

The existing wait_for_tpm_stat() checks the chip status before
sleeping for 5 msec in a polling loop. For some functions although
the status isn't ready immediately, the status returns extremely
quickly. Waiting for 5 msec causes an unnecessary delay. An
example is the send() call in the tpms_tis driver.

This patch defines __wait_for_tpm_stat(), allowing the caller
to specify the polling sleep timeout value within the loop.
The existing wait_for_tpm_stat() becomes a wrapper for this
function.

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

Signed-off-by: Nayna Jain <[email protected]>
Acked-by: Mimi Zohar <[email protected]>
---
drivers/char/tpm/tpm-interface.c | 15 ++++++++++++---
drivers/char/tpm/tpm.h | 3 +++
drivers/char/tpm/tpm_tis_core.c | 11 ++++++-----
3 files changed, 21 insertions(+), 8 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 1d6729be4cd6..b23d006243b7 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -1050,8 +1050,9 @@ static bool wait_for_tpm_stat_cond(struct tpm_chip *chip, u8 mask,
return false;
}

-int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask, unsigned long timeout,
- wait_queue_head_t *queue, bool check_cancel)
+int __wait_for_tpm_stat(struct tpm_chip *chip, u8 mask, unsigned long timeout,
+ unsigned int poll_sleep, wait_queue_head_t *queue,
+ bool check_cancel)
{
unsigned long stop;
long rc;
@@ -1085,7 +1086,7 @@ int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask, unsigned long timeout,
}
} else {
do {
- tpm_msleep(TPM_TIMEOUT);
+ tpm_msleep(poll_sleep);
status = chip->ops->status(chip);
if ((status & mask) == mask)
return 0;
@@ -1093,6 +1094,14 @@ int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask, unsigned long timeout,
}
return -ETIME;
}
+EXPORT_SYMBOL_GPL(__wait_for_tpm_stat);
+
+int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask, unsigned long timeout,
+ wait_queue_head_t *queue, bool check_cancel)
+{
+ return __wait_for_tpm_stat(chip, mask, timeout, TPM_TIMEOUT,
+ queue, check_cancel);
+}
EXPORT_SYMBOL_GPL(wait_for_tpm_stat);

#define TPM_ORD_SAVESTATE 152
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 2d5466a72e40..eb2f8818eded 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -525,6 +525,9 @@ int tpm_do_selftest(struct tpm_chip *chip);
unsigned long tpm_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal);
int tpm_pm_suspend(struct device *dev);
int tpm_pm_resume(struct device *dev);
+int __wait_for_tpm_stat(struct tpm_chip *chip, u8 mask,
+ unsigned long timeout, unsigned int poll_sleep,
+ wait_queue_head_t *queue, bool check_cancel);
int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask, unsigned long timeout,
wait_queue_head_t *queue, bool check_cancel);

diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 6b9bf4c4d434..d1eab29cb447 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -268,8 +268,8 @@ static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t len)
status = tpm_tis_status(chip);
if ((status & TPM_STS_COMMAND_READY) == 0) {
tpm_tis_ready(chip);
- if (wait_for_tpm_stat
- (chip, TPM_STS_COMMAND_READY, chip->timeout_b,
+ if (__wait_for_tpm_stat
+ (chip, TPM_STS_COMMAND_READY, chip->timeout_b, 1,
&priv->int_queue, false) < 0) {
rc = -ETIME;
goto out_err;
@@ -303,7 +303,8 @@ static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t len)
if (ignore_burst_count)
continue;

- if (wait_for_tpm_stat(chip, TPM_STS_VALID, chip->timeout_c,
+ if (__wait_for_tpm_stat(chip, TPM_STS_VALID,
+ chip->timeout_c, 1,
&priv->int_queue, false) < 0) {
rc = -ETIME;
goto out_err;
@@ -320,8 +321,8 @@ static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t len)
if (rc < 0)
goto out_err;

- if (wait_for_tpm_stat(chip, TPM_STS_VALID, chip->timeout_c,
- &priv->int_queue, false) < 0) {
+ if (__wait_for_tpm_stat(chip, TPM_STS_VALID, chip->timeout_c,
+ 1, &priv->int_queue, false) < 0) {
rc = -ETIME;
goto out_err;
}
--
2.13.3

2017-09-06 12:59:00

by Nayna Jain

Subject: Re: [PATCH 1/4] tpm: ignore burstcount to improve tpm_tis send() performance.

Please ignore these one.. My command took patches recursively from
directory also.

Sorry for this.

Thanks & Regards,
- Nayna

On 09/06/2017 06:26 PM, Nayna Jain wrote:
> The TPM burstcount status indicates the number of bytes that can
> be sent to the TPM without causing bus wait states. Effectively,
> it is the number of empty bytes in the command FIFO. Further,
> some TPMs have a static burstcount, when the value remains zero
> until the entire FIFO is empty.
>
> This patch adds an optimization to check for burstcount only once.
> And if it is valid, it writes all the bytes at once, permitting
> wait states. The performance of a 34 byte extend on a TPM 1.2 with
> an 8 byte burstcount improved from 41 msec to 14 msec.
>
> This functionality is enabled only by passing module
> parameter ignore_burst_count=1. By default, this parameter
> is disabled.
>
> Suggested-by: Ken Goldman <[email protected]> in
> conjunction with the TPM Device Driver work group.
> Signed-off-by: Nayna Jain <[email protected]>
> Acked-by: Mimi Zohar <[email protected]>
> ---
> Documentation/admin-guide/kernel-parameters.txt | 8 ++++++++
> drivers/char/tpm/tpm_tis_core.c | 24 +++++++++++++++++++++---
> 2 files changed, 29 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 4e303be83df6..3c59bb91e1ee 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -1465,6 +1465,14 @@
> mode generally follows that for the NaN encoding,
> except where unsupported by hardware.
>
> + ignore_burst_count [TPM_TIS_CORE]
> + tpm_tis_core driver queries for the burstcount before
> + every send call in a loop. However, it causes delay to
> + the send command for TPMs with low burstcount value.
> + Setting this value to 1, will make driver to query for
> + burstcount only once in the loop to improve the
> + performance. By default, its value is set to 0.
> +
> ignore_loglevel [KNL]
> Ignore loglevel setting - this will print /all/
> kernel messages to the console. Useful for debugging.
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index 63bc6c3b949e..6b9bf4c4d434 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -31,6 +31,11 @@
> #include "tpm.h"
> #include "tpm_tis_core.h"
>
> +static bool ignore_burst_count = false;
> +module_param(ignore_burst_count, bool, 0444);
> +MODULE_PARM_DESC(ignore_burst_count,
> + "Ignore burstcount value while writing data");
> +
> /* Before we attempt to access the TPM we must see that the valid bit is set.
> * The specification says that this bit is 0 at reset and remains 0 until the
> * 'TPM has gone through its self test and initialization and has established
> @@ -256,6 +261,7 @@ static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t len)
> {
> struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
> int rc, status, burstcnt;
> + int sendcnt;
> size_t count = 0;
> bool itpm = priv->flags & TPM_TIS_ITPM_WORKAROUND;
>
> @@ -271,19 +277,31 @@ static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t len)
> }
>
> while (count < len - 1) {
> +
> + /*
> + * Get the initial burstcount to ensure TPM is ready to
> + * accept data, even when waiting for burstcount is disabled.
> + */
> burstcnt = get_burstcount(chip);
> if (burstcnt < 0) {
> dev_err(&chip->dev, "Unable to read burstcount\n");
> rc = burstcnt;
> goto out_err;
> }
> - burstcnt = min_t(int, burstcnt, len - count - 1);
> +
> + if (ignore_burst_count)
> + sendcnt = len - 1;
> + else
> + sendcnt = min_t(int, burstcnt, len - count - 1);
> +
> rc = tpm_tis_write_bytes(priv, TPM_DATA_FIFO(priv->locality),
> - burstcnt, buf + count);
> + sendcnt, buf + count);
> if (rc < 0)
> goto out_err;
>
> - count += burstcnt;
> + count += sendcnt;
> + if (ignore_burst_count)
> + continue;
>
> if (wait_for_tpm_stat(chip, TPM_STS_VALID, chip->timeout_c,
> &priv->int_queue, false) < 0) {
>

2017-09-06 12:59:09

by Nayna Jain

Subject: Re: [PATCH 2/4] tpm: define __wait_for_tpm_stat to specify variable polling sleep time

Please ignore these one.. My command took patches recursively from
directory also.

Sorry for this.

Thanks & Regards,
- Nayna

On 09/06/2017 06:26 PM, Nayna Jain wrote:
> The existing wait_for_tpm_stat() checks the chip status before
> sleeping for 5 msec in a polling loop. For some functions although
> the status isn't ready immediately, the status returns extremely
> quickly. Waiting for 5 msec causes an unnecessary delay. An
> example is the send() call in the tpms_tis driver.
>
> This patch defines __wait_for_tpm_stat(), allowing the caller
> to specify the polling sleep timeout value within the loop.
> The existing wait_for_tpm_stat() becomes a wrapper for this
> function.
>
> Signed-off-by: Nayna Jain <[email protected]>
> ---
> drivers/char/tpm/tpm-interface.c | 15 ++++++++++++---
> drivers/char/tpm/tpm.h | 3 +++
> drivers/char/tpm/tpm_tis_core.c | 11 ++++++-----
> 3 files changed, 21 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index 1d6729be4cd6..b23d006243b7 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -1050,8 +1050,9 @@ static bool wait_for_tpm_stat_cond(struct tpm_chip *chip, u8 mask,
> return false;
> }
>
> -int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask, unsigned long timeout,
> - wait_queue_head_t *queue, bool check_cancel)
> +int __wait_for_tpm_stat(struct tpm_chip *chip, u8 mask, unsigned long timeout,
> + unsigned int poll_sleep, wait_queue_head_t *queue,
> + bool check_cancel)
> {
> unsigned long stop;
> long rc;
> @@ -1085,7 +1086,7 @@ int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask, unsigned long timeout,
> }
> } else {
> do {
> - tpm_msleep(TPM_TIMEOUT);
> + tpm_msleep(poll_sleep);
> status = chip->ops->status(chip);
> if ((status & mask) == mask)
> return 0;
> @@ -1093,6 +1094,14 @@ int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask, unsigned long timeout,
> }
> return -ETIME;
> }
> +EXPORT_SYMBOL_GPL(__wait_for_tpm_stat);
> +
> +int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask, unsigned long timeout,
> + wait_queue_head_t *queue, bool check_cancel)
> +{
> + return __wait_for_tpm_stat(chip, mask, timeout, TPM_TIMEOUT,
> + queue, check_cancel);
> +}
> EXPORT_SYMBOL_GPL(wait_for_tpm_stat);
>
> #define TPM_ORD_SAVESTATE 152
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index 2d5466a72e40..eb2f8818eded 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -525,6 +525,9 @@ int tpm_do_selftest(struct tpm_chip *chip);
> unsigned long tpm_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal);
> int tpm_pm_suspend(struct device *dev);
> int tpm_pm_resume(struct device *dev);
> +int __wait_for_tpm_stat(struct tpm_chip *chip, u8 mask,
> + unsigned long timeout, unsigned int poll_sleep,
> + wait_queue_head_t *queue, bool check_cancel);
> int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask, unsigned long timeout,
> wait_queue_head_t *queue, bool check_cancel);
>
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index 6b9bf4c4d434..d1eab29cb447 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -268,8 +268,8 @@ static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t len)
> status = tpm_tis_status(chip);
> if ((status & TPM_STS_COMMAND_READY) == 0) {
> tpm_tis_ready(chip);
> - if (wait_for_tpm_stat
> - (chip, TPM_STS_COMMAND_READY, chip->timeout_b,
> + if (__wait_for_tpm_stat
> + (chip, TPM_STS_COMMAND_READY, chip->timeout_b, 1,
> &priv->int_queue, false) < 0) {
> rc = -ETIME;
> goto out_err;
> @@ -303,7 +303,8 @@ static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t len)
> if (ignore_burst_count)
> continue;
>
> - if (wait_for_tpm_stat(chip, TPM_STS_VALID, chip->timeout_c,
> + if (__wait_for_tpm_stat(chip, TPM_STS_VALID,
> + chip->timeout_c, 1,
> &priv->int_queue, false) < 0) {
> rc = -ETIME;
> goto out_err;
> @@ -320,8 +321,8 @@ static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t len)
> if (rc < 0)
> goto out_err;
>
> - if (wait_for_tpm_stat(chip, TPM_STS_VALID, chip->timeout_c,
> - &priv->int_queue, false) < 0) {
> + if (__wait_for_tpm_stat(chip, TPM_STS_VALID, chip->timeout_c,
> + 1, &priv->int_queue, false) < 0) {
> rc = -ETIME;
> goto out_err;
> }
>

2017-09-06 13:00:39

by Nayna Jain

Subject: [PATCH v2 4/4] tpm: use tpm_msleep() value as max delay

Currently, tpm_msleep() uses delay_msec as the minimum value in
usleep_range. However, that is the maximum time we want to wait.
The function is modified to use the delay_msec as the maximum
value, not the minimum value.

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

Signed-off-by: Nayna Jain <[email protected]>
Acked-by: Mimi Zohar <[email protected]>
---
drivers/char/tpm/tpm.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index eb2f8818eded..ff5a8b7b80b9 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -533,8 +533,8 @@ int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask, unsigned long timeout,

static inline void tpm_msleep(unsigned int delay_msec)
{
- usleep_range(delay_msec * 1000,
- (delay_msec * 1000) + TPM_TIMEOUT_RANGE_US);
+ usleep_range((delay_msec * 1000) - TPM_TIMEOUT_RANGE_US,
+ delay_msec * 1000);
};

struct tpm_chip *tpm_chip_find_get(int chip_num);
--
2.13.3

2017-09-06 16:12:56

by Jason Gunthorpe

Subject: Re: [PATCH v2 1/4] tpm: ignore burstcount to improve tpm_tis send() performance.

On Wed, Sep 06, 2017 at 08:56:36AM -0400, Nayna Jain wrote:

> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 4e303be83df6..3c59bb91e1ee 100644
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -1465,6 +1465,14 @@
> mode generally follows that for the NaN encoding,
> except where unsupported by hardware.
>
> + ignore_burst_count [TPM_TIS_CORE]
> + tpm_tis_core driver queries for the burstcount before
> + every send call in a loop. However, it causes delay to
> + the send command for TPMs with low burstcount value.
> + Setting this value to 1, will make driver to query for
> + burstcount only once in the loop to improve the
> + performance. By default, its value is set to 0.

Really don't want to see a kernel command line parameter for this..

Please figure out a different approach or at least a better name..

> + /*
> + * Get the initial burstcount to ensure TPM is ready to
> + * accept data, even when waiting for burstcount is disabled.
> + */
> burstcnt = get_burstcount(chip);
> if (burstcnt < 0) {
> dev_err(&chip->dev, "Unable to read burstcount\n");
> rc = burstcnt;
> goto out_err;
> }
> - burstcnt = min_t(int, burstcnt, len - count - 1);
> +
> + if (ignore_burst_count)
> + sendcnt = len - 1;
> + else
> + sendcnt = min_t(int, burstcnt, len - count - 1);
> +
> rc = tpm_tis_write_bytes(priv, TPM_DATA_FIFO(priv->locality),
> - burstcnt, buf + count);
> + sendcnt, buf + count);

The problem with this approach is that the TPM could totally block the CPU for
very long periods of time.

It seems very risky to enable..

Jason

2017-09-07 16:19:11

by Jarkko Sakkinen

Subject: Re: [PATCH v2 0/4] additional TPM performance improvements

On Wed, Sep 06, 2017 at 08:56:35AM -0400, Nayna Jain wrote:
> After further discussions with the Device Driver working group (ddwg),
> the following changes were made:
>
> * Check for burstcount at least once to confirm the TPM is ready to accept
> the data. Similarly, query for the TPM Expect status as sanity check at
> the end.
>
> * Make the sleep for status check during send() in the loop less than
> 5msec.
>
> * Make the sleep in the loop while querying for burstcount less than
> 5msec.
>
> Below is the list of patches along with the performance improvements
> seen with a TPM 1.2 with an 8 byte burstcount for 1000 extends:
>
> Patch |Improvement(time in sec)
>
> tpm: ignore burstcount to improve tpm_tis | ~41 - ~14
> send() performance.
>
> tpm: define __wait_for_tpm_stat to specify | ~14 - ~10
> variable polling sleep time
>
> tpm: reduce tpm_msleep() time in | ~10 - ~9
> get_burstcount()
>
> tpm: modify tpm_msleep() function to have | ~9 - ~8
> max range
>
> Changelog v2:
>
> * Add module parameter to handle ignoring of burst count during
> tpm tis send() operation.
> * Add improvements over sleep time to reduce delays.
>
> Nayna Jain (4):
> tpm: ignore burstcount to improve tpm_tis send() performance.
> tpm: define __wait_for_tpm_stat to specify variable polling sleep time
> tpm: reduce tpm_msleep() time in get_burstcount()
> tpm: use tpm_msleep() value as max delay
>
> Documentation/admin-guide/kernel-parameters.txt | 8 ++++++
> drivers/char/tpm/tpm-interface.c | 15 ++++++++--
> drivers/char/tpm/tpm.h | 7 +++--
> drivers/char/tpm/tpm_tis_core.c | 37 +++++++++++++++++++------
> 4 files changed, 53 insertions(+), 14 deletions(-)
>
> --
> 2.13.3
>

I'll look into this after next week.

/Jarkko

2017-09-11 15:20:15

by Alexander Steffen

Subject: RE: [tpmdd-devel] [PATCH v2 0/4] additional TPM performance improvements

> After further discussions with the Device Driver working group (ddwg),
> the following changes were made:
>
> * Check for burstcount at least once to confirm the TPM is ready to accept
> the data. Similarly, query for the TPM Expect status as sanity check at
> the end.
>
> * Make the sleep for status check during send() in the loop less than
> 5msec.
>
> * Make the sleep in the loop while querying for burstcount less than
> 5msec.
>
> Below is the list of patches along with the performance improvements
> seen with a TPM 1.2 with an 8 byte burstcount for 1000 extends:
>
> Patch |Improvement(time in sec)
>
> tpm: ignore burstcount to improve tpm_tis | ~41 - ~14
> send() performance.
>
> tpm: define __wait_for_tpm_stat to specify | ~14 - ~10
> variable polling sleep time
>
> tpm: reduce tpm_msleep() time in | ~10 - ~9
> get_burstcount()
>
> tpm: modify tpm_msleep() function to have | ~9 - ~8
> max range
>
> Changelog v2:
>
> * Add module parameter to handle ignoring of burst count during
> tpm tis send() operation.
> * Add improvements over sleep time to reduce delays.
>
> Nayna Jain (4):
> tpm: ignore burstcount to improve tpm_tis send() performance.
> tpm: define __wait_for_tpm_stat to specify variable polling sleep time
> tpm: reduce tpm_msleep() time in get_burstcount()
> tpm: use tpm_msleep() value as max delay
>
> Documentation/admin-guide/kernel-parameters.txt | 8 ++++++
> drivers/char/tpm/tpm-interface.c | 15 ++++++++--
> drivers/char/tpm/tpm.h | 7 +++--
> drivers/char/tpm/tpm_tis_core.c | 37 +++++++++++++++++++------
> 4 files changed, 53 insertions(+), 14 deletions(-)
>
> --
> 2.13.3

I haven't looked at the changes in detail yet, but some initial comments:

The ignore_burst_count functionality has already received some negative feedback and probably needs more iterations, if it can be accepted at all, so you might want to split it off from the rest of the series.

Also, did you explore any alternative ways to activate that functionality besides a command line parameter? If it is off by default, then most users will never see the benefits, and if they do activate it manually, then they might hit some obscure bugs because those code paths receive only little testing. I could imagine for example activating this functionality automatically for certain safe drivers like tpm_tis_spi, where wait states cannot block us (or anyone else on the bus) indefinitely.

I ran all your patches through my tests (though without making any changes, so ignore_burst_count was off, since that is the default) and did not see any failures. My test bench does not contain performance tests (yet), so I cannot confirm your measurements. checkpatch.pl has a couple of complaints though.

Alexander

2017-09-13 00:45:20

by Jarkko Sakkinen

Subject: Re: [PATCH v2 1/4] tpm: ignore burstcount to improve tpm_tis send() performance.

On Wed, Sep 06, 2017 at 08:56:36AM -0400, Nayna Jain wrote:
> The TPM burstcount status indicates the number of bytes that can
> be sent to the TPM without causing bus wait states. Effectively,
> it is the number of empty bytes in the command FIFO. Further,
> some TPMs have a static burstcount, when the value remains zero
> until the entire FIFO is empty.
>
> This patch adds an optimization to check for burstcount only once.
> And if it is valid, it writes all the bytes at once, permitting
> wait states. The performance of a 34 byte extend on a TPM 1.2 with
> an 8 byte burstcount improved from 41 msec to 14 msec.
>
> This functionality is enabled only by passing module
> parameter ignore_burst_count=1. By default, this parameter
> is disabled.
>
> After this change, performance on a TPM 1.2 with an 8 byte
> burstcount for 1000 extends improved from ~41sec to ~14sec.
>
> Suggested-by: Ken Goldman <[email protected]> in
> conjunction with the TPM Device Driver work group.
> Signed-off-by: Nayna Jain <[email protected]>
> Acked-by: Mimi Zohar <[email protected]>
> ---
> Documentation/admin-guide/kernel-parameters.txt | 8 ++++++++
> drivers/char/tpm/tpm_tis_core.c | 24 +++++++++++++++++++++---
> 2 files changed, 29 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 4e303be83df6..3c59bb91e1ee 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -1465,6 +1465,14 @@
> mode generally follows that for the NaN encoding,
> except where unsupported by hardware.
>
> + ignore_burst_count [TPM_TIS_CORE]
> + tpm_tis_core driver queries for the burstcount before
> + every send call in a loop. However, it causes delay to
> + the send command for TPMs with low burstcount value.
> + Setting this value to 1, will make driver to query for
> + burstcount only once in the loop to improve the
> + performance. By default, its value is set to 0.
> +
> ignore_loglevel [KNL]
> Ignore loglevel setting - this will print /all/
> kernel messages to the console. Useful for debugging.
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index 63bc6c3b949e..6b9bf4c4d434 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -31,6 +31,11 @@
> #include "tpm.h"
> #include "tpm_tis_core.h"
>
> +static bool ignore_burst_count = false;
> +module_param(ignore_burst_count, bool, 0444);
> +MODULE_PARM_DESC(ignore_burst_count,
> + "Ignore burstcount value while writing data");
> +
> /* Before we attempt to access the TPM we must see that the valid bit is set.
> * The specification says that this bit is 0 at reset and remains 0 until the
> * 'TPM has gone through its self test and initialization and has established
> @@ -256,6 +261,7 @@ static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t len)
> {
> struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
> int rc, status, burstcnt;
> + int sendcnt;
> size_t count = 0;
> bool itpm = priv->flags & TPM_TIS_ITPM_WORKAROUND;
>
> @@ -271,19 +277,31 @@ static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t len)
> }
>
> while (count < len - 1) {
> +
> + /*
> + * Get the initial burstcount to ensure TPM is ready to
> + * accept data, even when waiting for burstcount is disabled.
> + */
> burstcnt = get_burstcount(chip);
> if (burstcnt < 0) {
> dev_err(&chip->dev, "Unable to read burstcount\n");
> rc = burstcnt;
> goto out_err;
> }
> - burstcnt = min_t(int, burstcnt, len - count - 1);
> +
> + if (ignore_burst_count)
> + sendcnt = len - 1;
> + else
> + sendcnt = min_t(int, burstcnt, len - count - 1);
> +
> rc = tpm_tis_write_bytes(priv, TPM_DATA_FIFO(priv->locality),
> - burstcnt, buf + count);
> + sendcnt, buf + count);
> if (rc < 0)
> goto out_err;
>
> - count += burstcnt;
> + count += sendcnt;
> + if (ignore_burst_count)
> + continue;
>
> if (wait_for_tpm_stat(chip, TPM_STS_VALID, chip->timeout_c,
> &priv->int_queue, false) < 0) {
> --
> 2.13.3
>

Makes sense to discuss whether to have the kernel command-line
parameter or not before applying this.

To fuel the discussion, alternative to this would be:

1. Have this always on i.e. no command-line parameter.
2. If someone yells, we add the command-line parameter later on.

/Jarkko

2017-09-13 00:47:14

by Jarkko Sakkinen

Subject: Re: [PATCH v2 4/4] tpm: use tpm_msleep() value as max delay

On Wed, Sep 06, 2017 at 08:56:39AM -0400, Nayna Jain wrote:
> Currently, tpm_msleep() uses delay_msec as the minimum value in
> usleep_range. However, that is the maximum time we want to wait.
> The function is modified to use the delay_msec as the maximum
> value, not the minimum value.
>
> After this change, performance on a TPM 1.2 with an 8 byte
> burstcount for 1000 extends improved from ~9sec to ~8sec.
>
> Signed-off-by: Nayna Jain <[email protected]>
> Acked-by: Mimi Zohar <[email protected]>
> ---
> drivers/char/tpm/tpm.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index eb2f8818eded..ff5a8b7b80b9 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -533,8 +533,8 @@ int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask, unsigned long timeout,
>
> static inline void tpm_msleep(unsigned int delay_msec)
> {
> - usleep_range(delay_msec * 1000,
> - (delay_msec * 1000) + TPM_TIMEOUT_RANGE_US);
> + usleep_range((delay_msec * 1000) - TPM_TIMEOUT_RANGE_US,
> + delay_msec * 1000);
> };
>
> struct tpm_chip *tpm_chip_find_get(int chip_num);
> --
> 2.13.3
>

Doesn't this need a Fixes tag?

/Jarkko

2017-09-13 00:58:35

by Jarkko Sakkinen

Subject: Re: [PATCH v2 2/4] tpm: define __wait_for_tpm_stat to specify variable polling sleep time

On Wed, Sep 06, 2017 at 08:56:37AM -0400, Nayna Jain wrote:
> The existing wait_for_tpm_stat() checks the chip status before
> sleeping for 5 msec in a polling loop. For some functions although
> the status isn't ready immediately, the status returns extremely
> quickly. Waiting for 5 msec causes an unnecessary delay. An
> example is the send() call in the tpms_tis driver.
>
> This patch defines __wait_for_tpm_stat(), allowing the caller
> to specify the polling sleep timeout value within the loop.
> The existing wait_for_tpm_stat() becomes a wrapper for this
> function.
>
> After this change, performance on a TPM 1.2 with an 8 byte
> burstcount for 1000 extends improved from ~14sec to ~10sec.
>
> Signed-off-by: Nayna Jain <[email protected]>
> Acked-by: Mimi Zohar <[email protected]>

Please get rid of wait_for_tpm_stat() rather than further making it more
complex. It's hardware specific stuff. This function should not exist in
tpm-interface.c.

/Jarkko

> ---
> drivers/char/tpm/tpm-interface.c | 15 ++++++++++++---
> drivers/char/tpm/tpm.h | 3 +++
> drivers/char/tpm/tpm_tis_core.c | 11 ++++++-----
> 3 files changed, 21 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index 1d6729be4cd6..b23d006243b7 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -1050,8 +1050,9 @@ static bool wait_for_tpm_stat_cond(struct tpm_chip *chip, u8 mask,
> return false;
> }
>
> -int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask, unsigned long timeout,
> - wait_queue_head_t *queue, bool check_cancel)
> +int __wait_for_tpm_stat(struct tpm_chip *chip, u8 mask, unsigned long timeout,
> + unsigned int poll_sleep, wait_queue_head_t *queue,
> + bool check_cancel)
> {
> unsigned long stop;
> long rc;
> @@ -1085,7 +1086,7 @@ int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask, unsigned long timeout,
> }
> } else {
> do {
> - tpm_msleep(TPM_TIMEOUT);
> + tpm_msleep(poll_sleep);
> status = chip->ops->status(chip);
> if ((status & mask) == mask)
> return 0;
> @@ -1093,6 +1094,14 @@ int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask, unsigned long timeout,
> }
> return -ETIME;
> }
> +EXPORT_SYMBOL_GPL(__wait_for_tpm_stat);
> +
> +int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask, unsigned long timeout,
> + wait_queue_head_t *queue, bool check_cancel)
> +{
> + return __wait_for_tpm_stat(chip, mask, timeout, TPM_TIMEOUT,
> + queue, check_cancel);
> +}
> EXPORT_SYMBOL_GPL(wait_for_tpm_stat);
>
> #define TPM_ORD_SAVESTATE 152
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index 2d5466a72e40..eb2f8818eded 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -525,6 +525,9 @@ int tpm_do_selftest(struct tpm_chip *chip);
> unsigned long tpm_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal);
> int tpm_pm_suspend(struct device *dev);
> int tpm_pm_resume(struct device *dev);
> +int __wait_for_tpm_stat(struct tpm_chip *chip, u8 mask,
> + unsigned long timeout, unsigned int poll_sleep,
> + wait_queue_head_t *queue, bool check_cancel);
> int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask, unsigned long timeout,
> wait_queue_head_t *queue, bool check_cancel);
>
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index 6b9bf4c4d434..d1eab29cb447 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -268,8 +268,8 @@ static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t len)
> status = tpm_tis_status(chip);
> if ((status & TPM_STS_COMMAND_READY) == 0) {
> tpm_tis_ready(chip);
> - if (wait_for_tpm_stat
> - (chip, TPM_STS_COMMAND_READY, chip->timeout_b,
> + if (__wait_for_tpm_stat
> + (chip, TPM_STS_COMMAND_READY, chip->timeout_b, 1,
> &priv->int_queue, false) < 0) {
> rc = -ETIME;
> goto out_err;
> @@ -303,7 +303,8 @@ static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t len)
> if (ignore_burst_count)
> continue;
>
> - if (wait_for_tpm_stat(chip, TPM_STS_VALID, chip->timeout_c,
> + if (__wait_for_tpm_stat(chip, TPM_STS_VALID,
> + chip->timeout_c, 1,
> &priv->int_queue, false) < 0) {
> rc = -ETIME;
> goto out_err;
> @@ -320,8 +321,8 @@ static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t len)
> if (rc < 0)
> goto out_err;
>
> - if (wait_for_tpm_stat(chip, TPM_STS_VALID, chip->timeout_c,
> - &priv->int_queue, false) < 0) {
> + if (__wait_for_tpm_stat(chip, TPM_STS_VALID, chip->timeout_c,
> + 1, &priv->int_queue, false) < 0) {
> rc = -ETIME;
> goto out_err;
> }
> --
> 2.13.3
>

2017-09-13 01:00:23

by Jarkko Sakkinen

Subject: Re: [PATCH v2 3/4] tpm: reduce tpm_msleep() time in get_burstcount()

On Wed, Sep 06, 2017 at 08:56:38AM -0400, Nayna Jain wrote:
> Currently, get_burstcount() function sleeps for 5msec in a loop
> before retrying for next query to burstcount. However, if it takes
> lesser time for TPM to return, this 5 msec delay is longer
> than necessary.
>
> This patch replaces the tpm_msleep time from 5msec to 1msec.
>
> After this change, performance on a TPM 1.2 with an 8 byte
> burstcount for 1000 extends improved from ~10sec to ~9sec.
>
> Signed-off-by: Nayna Jain <[email protected]>
> Acked-by: Mimi Zohar <[email protected]>
> ---
> drivers/char/tpm/tpm_tis_core.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index d1eab29cb447..d710bbc4608b 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -169,7 +169,7 @@ static int get_burstcount(struct tpm_chip *chip)
> burstcnt = (value >> 8) & 0xFFFF;
> if (burstcnt)
> return burstcnt;
> - tpm_msleep(TPM_TIMEOUT);
> + tpm_msleep(1);
> } while (time_before(jiffies, stop));
> return -EBUSY;
> }
> --
> 2.13.3

How did you pick 1 ms delay? Should there be a constant defining it?

/Jarkko

2017-09-13 18:39:44

by Peter Huewe

Subject: Re: [PATCH v2 1/4] tpm: ignore burstcount to improve tpm_tis send() performance.

Am 12. September 2017 17:45:08 GMT-07:00 schrieb Jarkko Sakkinen <[email protected]>:
>On Wed, Sep 06, 2017 at 08:56:36AM -0400, Nayna Jain wrote:
>> The TPM burstcount status indicates the number of bytes that can
>> be sent to the TPM without causing bus wait states. Effectively,
>> it is the number of empty bytes in the command FIFO. Further,
>> some TPMs have a static burstcount, when the value remains zero
>> until the entire FIFO is empty.
>>
>> This patch adds an optimization to check for burstcount only once.
>> And if it is valid, it writes all the bytes at once, permitting
>> wait states. The performance of a 34 byte extend on a TPM 1.2 with
>> an 8 byte burstcount improved from 41 msec to 14 msec.
>>
>> This functionality is enabled only by passing module
>> parameter ignore_burst_count=1. By default, this parameter
>> is disabled.
>>
>> After this change, performance on a TPM 1.2 with an 8 byte
>> burstcount for 1000 extends improved from ~41sec to ~14sec.
>>
>> Suggested-by: Ken Goldman <[email protected]> in
>> conjunction with the TPM Device Driver work group.
>> Signed-off-by: Nayna Jain <[email protected]>
>> Acked-by: Mimi Zohar <[email protected]>
>> ---
>> Documentation/admin-guide/kernel-parameters.txt | 8 ++++++++
>> drivers/char/tpm/tpm_tis_core.c | 24
>+++++++++++++++++++++---
>> 2 files changed, 29 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/admin-guide/kernel-parameters.txt
>b/Documentation/admin-guide/kernel-parameters.txt
>> index 4e303be83df6..3c59bb91e1ee 100644
>> --- a/Documentation/admin-guide/kernel-parameters.txt
>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>> @@ -1465,6 +1465,14 @@
>> mode generally follows that for the NaN encoding,
>> except where unsupported by hardware.
>>
>> + ignore_burst_count [TPM_TIS_CORE]
>> + tpm_tis_core driver queries for the burstcount before
>> + every send call in a loop. However, it causes delay to
>> + the send command for TPMs with low burstcount value.
>> + Setting this value to 1, will make driver to query for
>> + burstcount only once in the loop to improve the
>> + performance. By default, its value is set to 0.
>> +
>> ignore_loglevel [KNL]
>> Ignore loglevel setting - this will print /all/
>> kernel messages to the console. Useful for debugging.
>> diff --git a/drivers/char/tpm/tpm_tis_core.c
>b/drivers/char/tpm/tpm_tis_core.c
>> index 63bc6c3b949e..6b9bf4c4d434 100644
>> --- a/drivers/char/tpm/tpm_tis_core.c
>> +++ b/drivers/char/tpm/tpm_tis_core.c
>> @@ -31,6 +31,11 @@
>> #include "tpm.h"
>> #include "tpm_tis_core.h"
>>
>> +static bool ignore_burst_count = false;
>> +module_param(ignore_burst_count, bool, 0444);
>> +MODULE_PARM_DESC(ignore_burst_count,
>> + "Ignore burstcount value while writing data");
>> +
>> /* Before we attempt to access the TPM we must see that the valid
>bit is set.
>> * The specification says that this bit is 0 at reset and remains 0
>until the
>> * 'TPM has gone through its self test and initialization and has
>established
>> @@ -256,6 +261,7 @@ static int tpm_tis_send_data(struct tpm_chip
>*chip, u8 *buf, size_t len)
>> {
>> struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
>> int rc, status, burstcnt;
>> + int sendcnt;
>> size_t count = 0;
>> bool itpm = priv->flags & TPM_TIS_ITPM_WORKAROUND;
>>
>> @@ -271,19 +277,31 @@ static int tpm_tis_send_data(struct tpm_chip
>*chip, u8 *buf, size_t len)
>> }
>>
>> while (count < len - 1) {
>> +
>> + /*
>> + * Get the initial burstcount to ensure TPM is ready to
>> + * accept data, even when waiting for burstcount is disabled.
>> + */
>> burstcnt = get_burstcount(chip);
>> if (burstcnt < 0) {
>> dev_err(&chip->dev, "Unable to read burstcount\n");
>> rc = burstcnt;
>> goto out_err;
>> }
>> - burstcnt = min_t(int, burstcnt, len - count - 1);
>> +
>> + if (ignore_burst_count)
>> + sendcnt = len - 1;
>> + else
>> + sendcnt = min_t(int, burstcnt, len - count - 1);
>> +
>> rc = tpm_tis_write_bytes(priv, TPM_DATA_FIFO(priv->locality),
>> - burstcnt, buf + count);
>> + sendcnt, buf + count);
>> if (rc < 0)
>> goto out_err;
>>
>> - count += burstcnt;
>> + count += sendcnt;
>> + if (ignore_burst_count)
>> + continue;
>>
>> if (wait_for_tpm_stat(chip, TPM_STS_VALID, chip->timeout_c,
>> &priv->int_queue, false) < 0) {
>> --
>> 2.13.3
>>
>
>Makes sense to discuss whether to have the kernel command-line
>parameter or not before applying this.
>
>To fuel the discussion, alternative to this would be:
>
>1. Have this always on i.e. no command-line parameter.
>2. If someone yells, we add the command-line parameter later on.
>
According to what I've read in the tcg ddwg group this patch should not cause problems on _sane_ tpms.

I'm not 100%convinced that all tpms are sane all the time, but I think we do not want yet another cmdline parameter.

So if we want to pull it in (and ddwg does not see an issue, so yes) it should be on by default, without a kernel parameter.

If there is a kernel parameter, then it should only be one called "failsafe" - which includes the force behavior and maybe the "broken" tpm path.

But I agree with Alex, every additonal code path reduces testing coverage.


We would be happy to test a "default on" patch.

Peter

>/Jarkko

--
Sent from my mobile

2017-09-13 18:52:20

by Ken Goldman

Subject: Re: [tpmdd-devel] [PATCH v2 1/4] tpm: ignore burstcount to improve tpm_tis send() performance.

On 9/6/2017 12:12 PM, Jason Gunthorpe wrote:
>
> The problem with this approach is that the TPM could totally block
> the CPU for very long periods of time.
>
> It seems very risky to enable..
>

How would you characterize "very long"?

The TPM vendors confirm that they empty the FIFO at internal speeds that
are comparable to the bus speed. Thus, any stall will be sub-usec. Is
that an issue?

In addition, new TPMs have ever larger FIFO's, making stalls less likely
going forward.

2017-09-13 19:01:45

by Peter Huewe

Subject: Re: [tpmdd-devel] [PATCH v2 1/4] tpm: ignore burstcount to improve tpm_tis send() performance.

Am 13. September 2017 11:52:12 GMT-07:00 schrieb Ken Goldman <[email protected]>:
>On 9/6/2017 12:12 PM, Jason Gunthorpe wrote:
>>
>> The problem with this approach is that the TPM could totally block
>> the CPU for very long periods of time.
>>
>> It seems very risky to enable..
>>
>
>How would you characterize "very long"?
>
>The TPM vendors confirm that they empty the FIFO at internal speeds
>that
>are comparable to the bus speed. Thus, any stall will be sub-usec. Is
>that an issue?

If the tpm does behave correctly, this is fine.
If the tpm hangs for whatever reason, your machine is frozen and you will never figure out why.

That's my concern there.
However ddwg seems fine.

>
>In addition, new TPMs have ever larger FIFO's, making stalls less
>likely
>going forward.
But also reduced the polling loops that introduce the performance penalty ;)

>
>
>------------------------------------------------------------------------------
>Check out the vibrant tech community on one of the world's most
>engaging tech sites, Slashdot.org! http://sdm.link/slashdot
>_______________________________________________
>tpmdd-devel mailing list
>[email protected]
>https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

--
Sent from my mobile

2017-09-13 23:10:20

by Jarkko Sakkinen

Subject: Re: [PATCH v2 1/4] tpm: ignore burstcount to improve tpm_tis send() performance.

On Wed, Sep 13, 2017 at 11:39:03AM -0700, Peter Huewe wrote:
>
>
> Am 12. September 2017 17:45:08 GMT-07:00 schrieb Jarkko Sakkinen <[email protected]>:
> >On Wed, Sep 06, 2017 at 08:56:36AM -0400, Nayna Jain wrote:
> >> The TPM burstcount status indicates the number of bytes that can
> >> be sent to the TPM without causing bus wait states. Effectively,
> >> it is the number of empty bytes in the command FIFO. Further,
> >> some TPMs have a static burstcount, when the value remains zero
> >> until the entire FIFO is empty.
> >>
> >> This patch adds an optimization to check for burstcount only once.
> >> And if it is valid, it writes all the bytes at once, permitting
> >> wait states. The performance of a 34 byte extend on a TPM 1.2 with
> >> an 8 byte burstcount improved from 41 msec to 14 msec.
> >>
> >> This functionality is enabled only by passing module
> >> parameter ignore_burst_count=1. By default, this parameter
> >> is disabled.
> >>
> >> After this change, performance on a TPM 1.2 with an 8 byte
> >> burstcount for 1000 extends improved from ~41sec to ~14sec.
> >>
> >> Suggested-by: Ken Goldman <[email protected]> in
> >> conjunction with the TPM Device Driver work group.
> >> Signed-off-by: Nayna Jain <[email protected]>
> >> Acked-by: Mimi Zohar <[email protected]>
> >> ---
> >> Documentation/admin-guide/kernel-parameters.txt | 8 ++++++++
> >> drivers/char/tpm/tpm_tis_core.c | 24
> >+++++++++++++++++++++---
> >> 2 files changed, 29 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/Documentation/admin-guide/kernel-parameters.txt
> >b/Documentation/admin-guide/kernel-parameters.txt
> >> index 4e303be83df6..3c59bb91e1ee 100644
> >> --- a/Documentation/admin-guide/kernel-parameters.txt
> >> +++ b/Documentation/admin-guide/kernel-parameters.txt
> >> @@ -1465,6 +1465,14 @@
> >> mode generally follows that for the NaN encoding,
> >> except where unsupported by hardware.
> >>
> >> + ignore_burst_count [TPM_TIS_CORE]
> >> + tpm_tis_core driver queries for the burstcount before
> >> + every send call in a loop. However, it causes delay to
> >> + the send command for TPMs with low burstcount value.
> >> + Setting this value to 1, will make driver to query for
> >> + burstcount only once in the loop to improve the
> >> + performance. By default, its value is set to 0.
> >> +
> >> ignore_loglevel [KNL]
> >> Ignore loglevel setting - this will print /all/
> >> kernel messages to the console. Useful for debugging.
> >> diff --git a/drivers/char/tpm/tpm_tis_core.c
> >b/drivers/char/tpm/tpm_tis_core.c
> >> index 63bc6c3b949e..6b9bf4c4d434 100644
> >> --- a/drivers/char/tpm/tpm_tis_core.c
> >> +++ b/drivers/char/tpm/tpm_tis_core.c
> >> @@ -31,6 +31,11 @@
> >> #include "tpm.h"
> >> #include "tpm_tis_core.h"
> >>
> >> +static bool ignore_burst_count = false;
> >> +module_param(ignore_burst_count, bool, 0444);
> >> +MODULE_PARM_DESC(ignore_burst_count,
> >> + "Ignore burstcount value while writing data");
> >> +
> >> /* Before we attempt to access the TPM we must see that the valid
> >bit is set.
> >> * The specification says that this bit is 0 at reset and remains 0
> >until the
> >> * 'TPM has gone through its self test and initialization and has
> >established
> >> @@ -256,6 +261,7 @@ static int tpm_tis_send_data(struct tpm_chip
> >*chip, u8 *buf, size_t len)
> >> {
> >> struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
> >> int rc, status, burstcnt;
> >> + int sendcnt;
> >> size_t count = 0;
> >> bool itpm = priv->flags & TPM_TIS_ITPM_WORKAROUND;
> >>
> >> @@ -271,19 +277,31 @@ static int tpm_tis_send_data(struct tpm_chip
> >*chip, u8 *buf, size_t len)
> >> }
> >>
> >> while (count < len - 1) {
> >> +
> >> + /*
> >> + * Get the initial burstcount to ensure TPM is ready to
> >> + * accept data, even when waiting for burstcount is disabled.
> >> + */
> >> burstcnt = get_burstcount(chip);
> >> if (burstcnt < 0) {
> >> dev_err(&chip->dev, "Unable to read burstcount\n");
> >> rc = burstcnt;
> >> goto out_err;
> >> }
> >> - burstcnt = min_t(int, burstcnt, len - count - 1);
> >> +
> >> + if (ignore_burst_count)
> >> + sendcnt = len - 1;
> >> + else
> >> + sendcnt = min_t(int, burstcnt, len - count - 1);
> >> +
> >> rc = tpm_tis_write_bytes(priv, TPM_DATA_FIFO(priv->locality),
> >> - burstcnt, buf + count);
> >> + sendcnt, buf + count);
> >> if (rc < 0)
> >> goto out_err;
> >>
> >> - count += burstcnt;
> >> + count += sendcnt;
> >> + if (ignore_burst_count)
> >> + continue;
> >>
> >> if (wait_for_tpm_stat(chip, TPM_STS_VALID, chip->timeout_c,
> >> &priv->int_queue, false) < 0) {
> >> --
> >> 2.13.3
> >>
> >
> >Makes sense to discuss whether to have the kernel command-line
> >parameter or not before applying this.
> >
> >To fuel the discussion, alternative to this would be:
> >
> >1. Have this always on i.e. no command-line parameter.
> >2. If someone yells, we add the command-line parameter later on.
> >
> According to what I've read in the tcg ddwg group this patch should
> not cause problems on _sane_ tpms.
>
> I'm not 100%convinced that all tpms are sane all the time, but I think
> we do not want yet another cmdline parameter.
>
> So if we want to pull it in (and ddwg does not see an issue, so yes)
> it should be on by default, without a kernel parameter.
>
> If there is a kernel parameter, then it should only be one called
> "failsafe" - which includes the force behavior and maybe the "broken"
> tpm path.
>
> But I agree with Alex, every additonal code path reduces testing coverage.
>
>
> We would be happy to test a "default on" patch.
>
> Peter
>
> >/Jarkko

I'm starting to dilate to this direction.

It is hard to believe that any such TPM would be in active use anywhere
assuming that there exist a TPM where this causes issues. This combined
to the assumption that you would run the latest mainline on it makes it
a pretty insignificant scenario.

/Jarkko

2017-09-14 09:25:50

by Nayna Jain

Subject: Re: [PATCH v2 4/4] tpm: use tpm_msleep() value as max delay

On 09/13/2017 06:17 AM, Jarkko Sakkinen wrote:
> On Wed, Sep 06, 2017 at 08:56:39AM -0400, Nayna Jain wrote:
>> Currently, tpm_msleep() uses delay_msec as the minimum value in
>> usleep_range. However, that is the maximum time we want to wait.
>> The function is modified to use the delay_msec as the maximum
>> value, not the minimum value.
>>
>> After this change, performance on a TPM 1.2 with an 8 byte
>> burstcount for 1000 extends improved from ~9sec to ~8sec.
>>
>> Signed-off-by: Nayna Jain <[email protected]>
>> Acked-by: Mimi Zohar <[email protected]>
>> ---
>> drivers/char/tpm/tpm.h | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
>> index eb2f8818eded..ff5a8b7b80b9 100644
>> --- a/drivers/char/tpm/tpm.h
>> +++ b/drivers/char/tpm/tpm.h
>> @@ -533,8 +533,8 @@ int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask, unsigned long timeout,
>>
>> static inline void tpm_msleep(unsigned int delay_msec)
>> {
>> - usleep_range(delay_msec * 1000,
>> - (delay_msec * 1000) + TPM_TIMEOUT_RANGE_US);
>> + usleep_range((delay_msec * 1000) - TPM_TIMEOUT_RANGE_US,
>> + delay_msec * 1000);
>> };
>>
>> struct tpm_chip *tpm_chip_find_get(int chip_num);
>> --
>> 2.13.3
>>
> Doesn't this need a Fixes tag?
Yeah.. will add.

   - Nayna
> /Jarkko
>

2017-09-14 12:29:06

by Jarkko Sakkinen

Subject: Re: [PATCH v2 4/4] tpm: use tpm_msleep() value as max delay

On Thu, Sep 14, 2017 at 02:55:34PM +0530, Nayna Jain wrote:
>
>
> On 09/13/2017 06:17 AM, Jarkko Sakkinen wrote:
> > On Wed, Sep 06, 2017 at 08:56:39AM -0400, Nayna Jain wrote:
> > > Currently, tpm_msleep() uses delay_msec as the minimum value in
> > > usleep_range. However, that is the maximum time we want to wait.
> > > The function is modified to use the delay_msec as the maximum
> > > value, not the minimum value.
> > >
> > > After this change, performance on a TPM 1.2 with an 8 byte
> > > burstcount for 1000 extends improved from ~9sec to ~8sec.
> > >
> > > Signed-off-by: Nayna Jain <[email protected]>
> > > Acked-by: Mimi Zohar <[email protected]>
> > > ---
> > > drivers/char/tpm/tpm.h | 4 ++--
> > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> > > index eb2f8818eded..ff5a8b7b80b9 100644
> > > --- a/drivers/char/tpm/tpm.h
> > > +++ b/drivers/char/tpm/tpm.h
> > > @@ -533,8 +533,8 @@ int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask, unsigned long timeout,
> > > static inline void tpm_msleep(unsigned int delay_msec)
> > > {
> > > - usleep_range(delay_msec * 1000,
> > > - (delay_msec * 1000) + TPM_TIMEOUT_RANGE_US);
> > > + usleep_range((delay_msec * 1000) - TPM_TIMEOUT_RANGE_US,
> > > + delay_msec * 1000);
> > > };
> > > struct tpm_chip *tpm_chip_find_get(int chip_num);
> > > --
> > > 2.13.3
> > >
> > Doesn't this need a Fixes tag?
> Yeah.. will add.

No need just for that. I'll test this when I'm back in Finland.
It was a question just to check that I'm in the same page :-)

/Jarkko

2017-09-15 12:29:39

by Nayna Jain

Subject: Re: [PATCH v2 1/4] tpm: ignore burstcount to improve tpm_tis send() performance.

On 09/14/2017 04:40 AM, Jarkko Sakkinen wrote:
> On Wed, Sep 13, 2017 at 11:39:03AM -0700, Peter Huewe wrote:
>>
>> Am 12. September 2017 17:45:08 GMT-07:00 schrieb Jarkko Sakkinen <[email protected]>:
>>> On Wed, Sep 06, 2017 at 08:56:36AM -0400, Nayna Jain wrote:
>>>> The TPM burstcount status indicates the number of bytes that can
>>>> be sent to the TPM without causing bus wait states. Effectively,
>>>> it is the number of empty bytes in the command FIFO. Further,
>>>> some TPMs have a static burstcount, when the value remains zero
>>>> until the entire FIFO is empty.
>>>>
>>>> This patch adds an optimization to check for burstcount only once.
>>>> And if it is valid, it writes all the bytes at once, permitting
>>>> wait states. The performance of a 34 byte extend on a TPM 1.2 with
>>>> an 8 byte burstcount improved from 41 msec to 14 msec.
>>>>
>>>> This functionality is enabled only by passing module
>>>> parameter ignore_burst_count=1. By default, this parameter
>>>> is disabled.
>>>>
>>>> After this change, performance on a TPM 1.2 with an 8 byte
>>>> burstcount for 1000 extends improved from ~41sec to ~14sec.
>>>>
>>>> Suggested-by: Ken Goldman <[email protected]> in
>>>> conjunction with the TPM Device Driver work group.
>>>> Signed-off-by: Nayna Jain <[email protected]>
>>>> Acked-by: Mimi Zohar <[email protected]>
>>>> ---
>>>> Documentation/admin-guide/kernel-parameters.txt | 8 ++++++++
>>>> drivers/char/tpm/tpm_tis_core.c | 24
>>> +++++++++++++++++++++---
>>>> 2 files changed, 29 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/Documentation/admin-guide/kernel-parameters.txt
>>> b/Documentation/admin-guide/kernel-parameters.txt
>>>> index 4e303be83df6..3c59bb91e1ee 100644
>>>> --- a/Documentation/admin-guide/kernel-parameters.txt
>>>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>>>> @@ -1465,6 +1465,14 @@
>>>> mode generally follows that for the NaN encoding,
>>>> except where unsupported by hardware.
>>>>
>>>> + ignore_burst_count [TPM_TIS_CORE]
>>>> + tpm_tis_core driver queries for the burstcount before
>>>> + every send call in a loop. However, it causes delay to
>>>> + the send command for TPMs with low burstcount value.
>>>> + Setting this value to 1, will make driver to query for
>>>> + burstcount only once in the loop to improve the
>>>> + performance. By default, its value is set to 0.
>>>> +
>>>> ignore_loglevel [KNL]
>>>> Ignore loglevel setting - this will print /all/
>>>> kernel messages to the console. Useful for debugging.
>>>> diff --git a/drivers/char/tpm/tpm_tis_core.c
>>> b/drivers/char/tpm/tpm_tis_core.c
>>>> index 63bc6c3b949e..6b9bf4c4d434 100644
>>>> --- a/drivers/char/tpm/tpm_tis_core.c
>>>> +++ b/drivers/char/tpm/tpm_tis_core.c
>>>> @@ -31,6 +31,11 @@
>>>> #include "tpm.h"
>>>> #include "tpm_tis_core.h"
>>>>
>>>> +static bool ignore_burst_count = false;
>>>> +module_param(ignore_burst_count, bool, 0444);
>>>> +MODULE_PARM_DESC(ignore_burst_count,
>>>> + "Ignore burstcount value while writing data");
>>>> +
>>>> /* Before we attempt to access the TPM we must see that the valid
>>> bit is set.
>>>> * The specification says that this bit is 0 at reset and remains 0
>>> until the
>>>> * 'TPM has gone through its self test and initialization and has
>>> established
>>>> @@ -256,6 +261,7 @@ static int tpm_tis_send_data(struct tpm_chip
>>> *chip, u8 *buf, size_t len)
>>>> {
>>>> struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
>>>> int rc, status, burstcnt;
>>>> + int sendcnt;
>>>> size_t count = 0;
>>>> bool itpm = priv->flags & TPM_TIS_ITPM_WORKAROUND;
>>>>
>>>> @@ -271,19 +277,31 @@ static int tpm_tis_send_data(struct tpm_chip
>>> *chip, u8 *buf, size_t len)
>>>> }
>>>>
>>>> while (count < len - 1) {
>>>> +
>>>> + /*
>>>> + * Get the initial burstcount to ensure TPM is ready to
>>>> + * accept data, even when waiting for burstcount is disabled.
>>>> + */
>>>> burstcnt = get_burstcount(chip);
>>>> if (burstcnt < 0) {
>>>> dev_err(&chip->dev, "Unable to read burstcount\n");
>>>> rc = burstcnt;
>>>> goto out_err;
>>>> }
>>>> - burstcnt = min_t(int, burstcnt, len - count - 1);
>>>> +
>>>> + if (ignore_burst_count)
>>>> + sendcnt = len - 1;
>>>> + else
>>>> + sendcnt = min_t(int, burstcnt, len - count - 1);
>>>> +
>>>> rc = tpm_tis_write_bytes(priv, TPM_DATA_FIFO(priv->locality),
>>>> - burstcnt, buf + count);
>>>> + sendcnt, buf + count);
>>>> if (rc < 0)
>>>> goto out_err;
>>>>
>>>> - count += burstcnt;
>>>> + count += sendcnt;
>>>> + if (ignore_burst_count)
>>>> + continue;
>>>>
>>>> if (wait_for_tpm_stat(chip, TPM_STS_VALID, chip->timeout_c,
>>>> &priv->int_queue, false) < 0) {
>>>> --
>>>> 2.13.3
>>>>
>>> Makes sense to discuss whether to have the kernel command-line
>>> parameter or not before applying this.
>>>
>>> To fuel the discussion, alternative to this would be:
>>>
>>> 1. Have this always on i.e. no command-line parameter.
>>> 2. If someone yells, we add the command-line parameter later on.
>>>
>> According to what I've read in the tcg ddwg group this patch should
>> not cause problems on _sane_ tpms.
>>
>> I'm not 100%convinced that all tpms are sane all the time, but I think
>> we do not want yet another cmdline parameter.
>>
>> So if we want to pull it in (and ddwg does not see an issue, so yes)
>> it should be on by default, without a kernel parameter.
>>
>> If there is a kernel parameter, then it should only be one called
>> "failsafe" - which includes the force behavior and maybe the "broken"
>> tpm path.
>>
>> But I agree with Alex, every additonal code path reduces testing coverage.
>>
>>
>> We would be happy to test a "default on" patch.
>>
>> Peter
>>
>>> /Jarkko
> I'm starting to dilate to this direction.
>
> It is hard to believe that any such TPM would be in active use anywhere
> assuming that there exist a TPM where this causes issues. This combined
> to the assumption that you would run the latest mainline on it makes it
> a pretty insignificant scenario.

It sounds like we are getting in direction to have this change by default.
Before removing the ignore_burst_count parameter, I will post a test version of this
patch which enables ignore_burst_count by default, for testing purposes only.

Thanks Peter and Alex for testing.

Thanks & Regards,
- Nayna


>
> /Jarkko
>

2017-09-15 12:37:42

by Nayna Jain

Subject: Re: [PATCH v2 2/4] tpm: define __wait_for_tpm_stat to specify variable polling sleep time

On 09/13/2017 06:28 AM, Jarkko Sakkinen wrote:
> On Wed, Sep 06, 2017 at 08:56:37AM -0400, Nayna Jain wrote:
>> The existing wait_for_tpm_stat() checks the chip status before
>> sleeping for 5 msec in a polling loop. For some functions although
>> the status isn't ready immediately, the status returns extremely
>> quickly. Waiting for 5 msec causes an unnecessary delay. An
>> example is the send() call in the tpms_tis driver.
>>
>> This patch defines __wait_for_tpm_stat(), allowing the caller
>> to specify the polling sleep timeout value within the loop.
>> The existing wait_for_tpm_stat() becomes a wrapper for this
>> function.
>>
>> After this change, performance on a TPM 1.2 with an 8 byte
>> burstcount for 1000 extends improved from ~14sec to ~10sec.
>>
>> Signed-off-by: Nayna Jain <[email protected]>
>> Acked-by: Mimi Zohar <[email protected]>
> Please get rid of wait_for_tpm_stat() rather than further making it more
> complex. It's hardware specific stuff. This function should not exist in
> tpm-interface.c.

I think I didn't understand the meaning of "get rid of wait_for_tpm_stat()".
Do you mean to take care of it in driver specific file ?
Can you please elaborate it ?

Thanks & Regards,
    - Nayna

>
> /Jarkko
>
>> ---
>> drivers/char/tpm/tpm-interface.c | 15 ++++++++++++---
>> drivers/char/tpm/tpm.h | 3 +++
>> drivers/char/tpm/tpm_tis_core.c | 11 ++++++-----
>> 3 files changed, 21 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
>> index 1d6729be4cd6..b23d006243b7 100644
>> --- a/drivers/char/tpm/tpm-interface.c
>> +++ b/drivers/char/tpm/tpm-interface.c
>> @@ -1050,8 +1050,9 @@ static bool wait_for_tpm_stat_cond(struct tpm_chip *chip, u8 mask,
>> return false;
>> }
>>
>> -int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask, unsigned long timeout,
>> - wait_queue_head_t *queue, bool check_cancel)
>> +int __wait_for_tpm_stat(struct tpm_chip *chip, u8 mask, unsigned long timeout,
>> + unsigned int poll_sleep, wait_queue_head_t *queue,
>> + bool check_cancel)
>> {
>> unsigned long stop;
>> long rc;
>> @@ -1085,7 +1086,7 @@ int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask, unsigned long timeout,
>> }
>> } else {
>> do {
>> - tpm_msleep(TPM_TIMEOUT);
>> + tpm_msleep(poll_sleep);
>> status = chip->ops->status(chip);
>> if ((status & mask) == mask)
>> return 0;
>> @@ -1093,6 +1094,14 @@ int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask, unsigned long timeout,
>> }
>> return -ETIME;
>> }
>> +EXPORT_SYMBOL_GPL(__wait_for_tpm_stat);
>> +
>> +int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask, unsigned long timeout,
>> + wait_queue_head_t *queue, bool check_cancel)
>> +{
>> + return __wait_for_tpm_stat(chip, mask, timeout, TPM_TIMEOUT,
>> + queue, check_cancel);
>> +}
>> EXPORT_SYMBOL_GPL(wait_for_tpm_stat);
>>
>> #define TPM_ORD_SAVESTATE 152
>> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
>> index 2d5466a72e40..eb2f8818eded 100644
>> --- a/drivers/char/tpm/tpm.h
>> +++ b/drivers/char/tpm/tpm.h
>> @@ -525,6 +525,9 @@ int tpm_do_selftest(struct tpm_chip *chip);
>> unsigned long tpm_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal);
>> int tpm_pm_suspend(struct device *dev);
>> int tpm_pm_resume(struct device *dev);
>> +int __wait_for_tpm_stat(struct tpm_chip *chip, u8 mask,
>> + unsigned long timeout, unsigned int poll_sleep,
>> + wait_queue_head_t *queue, bool check_cancel);
>> int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask, unsigned long timeout,
>> wait_queue_head_t *queue, bool check_cancel);
>>
>> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
>> index 6b9bf4c4d434..d1eab29cb447 100644
>> --- a/drivers/char/tpm/tpm_tis_core.c
>> +++ b/drivers/char/tpm/tpm_tis_core.c
>> @@ -268,8 +268,8 @@ static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t len)
>> status = tpm_tis_status(chip);
>> if ((status & TPM_STS_COMMAND_READY) == 0) {
>> tpm_tis_ready(chip);
>> - if (wait_for_tpm_stat
>> - (chip, TPM_STS_COMMAND_READY, chip->timeout_b,
>> + if (__wait_for_tpm_stat
>> + (chip, TPM_STS_COMMAND_READY, chip->timeout_b, 1,
>> &priv->int_queue, false) < 0) {
>> rc = -ETIME;
>> goto out_err;
>> @@ -303,7 +303,8 @@ static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t len)
>> if (ignore_burst_count)
>> continue;
>>
>> - if (wait_for_tpm_stat(chip, TPM_STS_VALID, chip->timeout_c,
>> + if (__wait_for_tpm_stat(chip, TPM_STS_VALID,
>> + chip->timeout_c, 1,
>> &priv->int_queue, false) < 0) {
>> rc = -ETIME;
>> goto out_err;
>> @@ -320,8 +321,8 @@ static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t len)
>> if (rc < 0)
>> goto out_err;
>>
>> - if (wait_for_tpm_stat(chip, TPM_STS_VALID, chip->timeout_c,
>> - &priv->int_queue, false) < 0) {
>> + if (__wait_for_tpm_stat(chip, TPM_STS_VALID, chip->timeout_c,
>> + 1, &priv->int_queue, false) < 0) {
>> rc = -ETIME;
>> goto out_err;
>> }
>> --
>> 2.13.3
>>

2017-09-15 12:41:06

by Nayna Jain

Subject: Re: [PATCH v2 3/4] tpm: reduce tpm_msleep() time in get_burstcount()

On 09/13/2017 06:30 AM, Jarkko Sakkinen wrote:
> On Wed, Sep 06, 2017 at 08:56:38AM -0400, Nayna Jain wrote:
>> Currently, get_burstcount() function sleeps for 5msec in a loop
>> before retrying for next query to burstcount. However, if it takes
>> lesser time for TPM to return, this 5 msec delay is longer
>> than necessary.
>>
>> This patch replaces the tpm_msleep time from 5msec to 1msec.
>>
>> After this change, performance on a TPM 1.2 with an 8 byte
>> burstcount for 1000 extends improved from ~10sec to ~9sec.
>>
>> Signed-off-by: Nayna Jain <[email protected]>
>> Acked-by: Mimi Zohar <[email protected]>
>> ---
>> drivers/char/tpm/tpm_tis_core.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
>> index d1eab29cb447..d710bbc4608b 100644
>> --- a/drivers/char/tpm/tpm_tis_core.c
>> +++ b/drivers/char/tpm/tpm_tis_core.c
>> @@ -169,7 +169,7 @@ static int get_burstcount(struct tpm_chip *chip)
>> burstcnt = (value >> 8) & 0xFFFF;
>> if (burstcnt)
>> return burstcnt;
>> - tpm_msleep(TPM_TIMEOUT);
>> + tpm_msleep(1);
>> } while (time_before(jiffies, stop));
>> return -EBUSY;
>> }
>> --
>> 2.13.3
> How did you pick 1 ms delay? Should there be a constant defining it?

As per ddwg input, the command may not take more than a few
microseconds. The minimum tpm_msleep() value is 1 msec, so we really
don't have a choice.  (We're working on a patch set to lower this
value even more.)

Thanks & Regards,
- Nayna

>
> /Jarkko
>

2017-09-15 15:19:45

by Jarkko Sakkinen

Subject: Re: [PATCH v2 1/4] tpm: ignore burstcount to improve tpm_tis send() performance.

On Fri, Sep 15, 2017 at 05:59:21PM +0530, Nayna Jain wrote:
>
>
> On 09/14/2017 04:40 AM, Jarkko Sakkinen wrote:
> > On Wed, Sep 13, 2017 at 11:39:03AM -0700, Peter Huewe wrote:
> > >
> > > Am 12. September 2017 17:45:08 GMT-07:00 schrieb Jarkko Sakkinen <[email protected]>:
> > > > On Wed, Sep 06, 2017 at 08:56:36AM -0400, Nayna Jain wrote:
> > > > > The TPM burstcount status indicates the number of bytes that can
> > > > > be sent to the TPM without causing bus wait states. Effectively,
> > > > > it is the number of empty bytes in the command FIFO. Further,
> > > > > some TPMs have a static burstcount, when the value remains zero
> > > > > until the entire FIFO is empty.
> > > > >
> > > > > This patch adds an optimization to check for burstcount only once.
> > > > > And if it is valid, it writes all the bytes at once, permitting
> > > > > wait states. The performance of a 34 byte extend on a TPM 1.2 with
> > > > > an 8 byte burstcount improved from 41 msec to 14 msec.
> > > > >
> > > > > This functionality is enabled only by passing module
> > > > > parameter ignore_burst_count=1. By default, this parameter
> > > > > is disabled.
> > > > >
> > > > > After this change, performance on a TPM 1.2 with an 8 byte
> > > > > burstcount for 1000 extends improved from ~41sec to ~14sec.
> > > > >
> > > > > Suggested-by: Ken Goldman <[email protected]> in
> > > > > conjunction with the TPM Device Driver work group.
> > > > > Signed-off-by: Nayna Jain <[email protected]>
> > > > > Acked-by: Mimi Zohar <[email protected]>
> > > > > ---
> > > > > Documentation/admin-guide/kernel-parameters.txt | 8 ++++++++
> > > > > drivers/char/tpm/tpm_tis_core.c | 24
> > > > +++++++++++++++++++++---
> > > > > 2 files changed, 29 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/Documentation/admin-guide/kernel-parameters.txt
> > > > b/Documentation/admin-guide/kernel-parameters.txt
> > > > > index 4e303be83df6..3c59bb91e1ee 100644
> > > > > --- a/Documentation/admin-guide/kernel-parameters.txt
> > > > > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > > > > @@ -1465,6 +1465,14 @@
> > > > > mode generally follows that for the NaN encoding,
> > > > > except where unsupported by hardware.
> > > > > + ignore_burst_count [TPM_TIS_CORE]
> > > > > + tpm_tis_core driver queries for the burstcount before
> > > > > + every send call in a loop. However, it causes delay to
> > > > > + the send command for TPMs with low burstcount value.
> > > > > + Setting this value to 1, will make driver to query for
> > > > > + burstcount only once in the loop to improve the
> > > > > + performance. By default, its value is set to 0.
> > > > > +
> > > > > ignore_loglevel [KNL]
> > > > > Ignore loglevel setting - this will print /all/
> > > > > kernel messages to the console. Useful for debugging.
> > > > > diff --git a/drivers/char/tpm/tpm_tis_core.c
> > > > b/drivers/char/tpm/tpm_tis_core.c
> > > > > index 63bc6c3b949e..6b9bf4c4d434 100644
> > > > > --- a/drivers/char/tpm/tpm_tis_core.c
> > > > > +++ b/drivers/char/tpm/tpm_tis_core.c
> > > > > @@ -31,6 +31,11 @@
> > > > > #include "tpm.h"
> > > > > #include "tpm_tis_core.h"
> > > > > +static bool ignore_burst_count = false;
> > > > > +module_param(ignore_burst_count, bool, 0444);
> > > > > +MODULE_PARM_DESC(ignore_burst_count,
> > > > > + "Ignore burstcount value while writing data");
> > > > > +
> > > > > /* Before we attempt to access the TPM we must see that the valid
> > > > bit is set.
> > > > > * The specification says that this bit is 0 at reset and remains 0
> > > > until the
> > > > > * 'TPM has gone through its self test and initialization and has
> > > > established
> > > > > @@ -256,6 +261,7 @@ static int tpm_tis_send_data(struct tpm_chip
> > > > *chip, u8 *buf, size_t len)
> > > > > {
> > > > > struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
> > > > > int rc, status, burstcnt;
> > > > > + int sendcnt;
> > > > > size_t count = 0;
> > > > > bool itpm = priv->flags & TPM_TIS_ITPM_WORKAROUND;
> > > > > @@ -271,19 +277,31 @@ static int tpm_tis_send_data(struct tpm_chip
> > > > *chip, u8 *buf, size_t len)
> > > > > }
> > > > > while (count < len - 1) {
> > > > > +
> > > > > + /*
> > > > > + * Get the initial burstcount to ensure TPM is ready to
> > > > > + * accept data, even when waiting for burstcount is disabled.
> > > > > + */
> > > > > burstcnt = get_burstcount(chip);
> > > > > if (burstcnt < 0) {
> > > > > dev_err(&chip->dev, "Unable to read burstcount\n");
> > > > > rc = burstcnt;
> > > > > goto out_err;
> > > > > }
> > > > > - burstcnt = min_t(int, burstcnt, len - count - 1);
> > > > > +
> > > > > + if (ignore_burst_count)
> > > > > + sendcnt = len - 1;
> > > > > + else
> > > > > + sendcnt = min_t(int, burstcnt, len - count - 1);
> > > > > +
> > > > > rc = tpm_tis_write_bytes(priv, TPM_DATA_FIFO(priv->locality),
> > > > > - burstcnt, buf + count);
> > > > > + sendcnt, buf + count);
> > > > > if (rc < 0)
> > > > > goto out_err;
> > > > > - count += burstcnt;
> > > > > + count += sendcnt;
> > > > > + if (ignore_burst_count)
> > > > > + continue;
> > > > > if (wait_for_tpm_stat(chip, TPM_STS_VALID, chip->timeout_c,
> > > > > &priv->int_queue, false) < 0) {
> > > > > --
> > > > > 2.13.3
> > > > >
> > > > Makes sense to discuss whether to have the kernel command-line
> > > > parameter or not before applying this.
> > > >
> > > > To fuel the discussion, alternative to this would be:
> > > >
> > > > 1. Have this always on i.e. no command-line parameter.
> > > > 2. If someone yells, we add the command-line parameter later on.
> > > >
> > > According to what I've read in the tcg ddwg group this patch should
> > > not cause problems on _sane_ tpms.
> > >
> > > I'm not 100%convinced that all tpms are sane all the time, but I think
> > > we do not want yet another cmdline parameter.
> > >
> > > So if we want to pull it in (and ddwg does not see an issue, so yes)
> > > it should be on by default, without a kernel parameter.
> > >
> > > If there is a kernel parameter, then it should only be one called
> > > "failsafe" - which includes the force behavior and maybe the "broken"
> > > tpm path.
> > >
> > > But I agree with Alex, every additonal code path reduces testing coverage.
> > >
> > >
> > > We would be happy to test a "default on" patch.
> > >
> > > Peter
> > >
> > > > /Jarkko
> > I'm starting to dilate to this direction.
> >
> > It is hard to believe that any such TPM would be in active use anywhere
> > assuming that there exist a TPM where this causes issues. This combined
> > to the assumption that you would run the latest mainline on it makes it
> > a pretty insignificant scenario.
>
> It sounds like we are getting in direction to have this change by default.
> Before removing the ignore_burst_count parameter, I will post a test version of this
> patch which enables ignore_burst_count by default, for testing purposes only.
>
> Thanks Peter and Alex for testing.
>
> Thanks & Regards,
> - Nayna

I could apply it immediately after some testing to my next branch where
it gets pulled to linux-next. There's still a lot of time before next
pull request so many people would get exposed. If it cause problems,
we reconsider.

/Jarkko

2017-09-15 15:20:52

by Jarkko Sakkinen

Subject: Re: [PATCH v2 2/4] tpm: define __wait_for_tpm_stat to specify variable polling sleep time

On Fri, Sep 15, 2017 at 06:07:26PM +0530, Nayna Jain wrote:
>
>
> On 09/13/2017 06:28 AM, Jarkko Sakkinen wrote:
> > On Wed, Sep 06, 2017 at 08:56:37AM -0400, Nayna Jain wrote:
> > > The existing wait_for_tpm_stat() checks the chip status before
> > > sleeping for 5 msec in a polling loop. For some functions although
> > > the status isn't ready immediately, the status returns extremely
> > > quickly. Waiting for 5 msec causes an unnecessary delay. An
> > > example is the send() call in the tpms_tis driver.
> > >
> > > This patch defines __wait_for_tpm_stat(), allowing the caller
> > > to specify the polling sleep timeout value within the loop.
> > > The existing wait_for_tpm_stat() becomes a wrapper for this
> > > function.
> > >
> > > After this change, performance on a TPM 1.2 with an 8 byte
> > > burstcount for 1000 extends improved from ~14sec to ~10sec.
> > >
> > > Signed-off-by: Nayna Jain <[email protected]>
> > > Acked-by: Mimi Zohar <[email protected]>
> > Please get rid of wait_for_tpm_stat() rather than further making it more
> > complex. It's hardware specific stuff. This function should not exist in
> > tpm-interface.c.
>
> I think I didn't understand the meaning of "get rid of wait_for_tpm_stat()".
> Do you mean to take care of it in driver specific file ?
> Can you please elaborate it ?
>
> Thanks & Regards,
> ??? - Nayna

It's not a generic function. It's used only in tpm_tis and tpm-xenfront.
They should have their owen private functions for this. Here the sharing
of code makes zero sense.

/Jarkko