On Wed, Jul 26, 2017 at 06:46:42PM +0100, Hamza Attak wrote:
> The patch simply replaces all msleep function calls with usleep_range calls
> in the generic drivers.
>
> Tested with an Infineon TPM 1.2, using the generic tpm-tis module, for a
> thousand PCR extends, we see results going from 1m57s unpatched to 40s
> with the new patch. We obtain similar results when using the original and
> patched tpm_infineon driver, which is also part of the patch.
> Similarly with a STM TPM 2.0, using the CRB driver, it takes about 20ms per
> extend unpatched and around 7ms with the new patch.
>
> Note that the PCR consistency is untouched with this patch, each TPM has
> been tested with 10 million extends and the aggregated PCR value is
> continuously verified to be correct.
>
> As an extension of this work, this could potentially and easily be applied
> to other vendor's drivers. Still, these changes are not included in the
> proposed patch as they are untested.
>
> Signed-off-by: Hamza Attak <ha...@hpe.com>
I think this would be a lot cleaner that we would have
void tpm_msleep(unsigned long msecs) function that would call usleep_range().
The change makes in high-level sense but the implementation makes the
driver code a mess.
> ---
> drivers/char/tpm/tpm-interface.c | 18 +++++++++++-------
> drivers/char/tpm/tpm.h | 11 +++++++++--
> drivers/char/tpm/tpm2-cmd.c | 6 +++---
> drivers/char/tpm/tpm_infineon.c | 17 ++++++++++-------
> drivers/char/tpm/tpm_tis_core.c | 11 +++++++----
> 5 files changed, 40 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm-interface.c
> b/drivers/char/tpm/tpm-interface.c
> index bd2128e..95276d1 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -395,7 +395,8 @@ ssize_t tpm_transmit(struct tpm_chip *chip, const u8
> *buf, size_t bufsiz,
> goto out;
> }
>
> - msleep(TPM_TIMEOUT); /* CHECK */
> + usleep_range(TPM_TIMEOUT_US,
> + TPM_TIMEOUT_US + TPM_TIMEOUT_RANGE_US);
> rmb();
> } while (time_before(jiffies, stop));
>
> @@ -836,13 +837,13 @@ int tpm_do_selftest(struct tpm_chip *chip)
> {
> int rc;
> unsigned int loops;
> - unsigned int delay_msec = 100;
> + unsigned int delay_usec = 100000;
> unsigned long duration;
> u8 dummy[TPM_DIGEST_SIZE];
>
> duration = tpm_calc_ordinal_duration(chip, TPM_ORD_CONTINUE_SELFTEST);
>
> - loops = jiffies_to_msecs(duration) / delay_msec;
> + loops = jiffies_to_usecs(duration) / delay_usec;
>
> rc = tpm_continue_selftest(chip);
> /* This may fail if there was no TPM driver during a suspend/resume
> @@ -862,7 +863,8 @@ int tpm_do_selftest(struct tpm_chip *chip)
> dev_info(
> &chip->dev, HW_ERR
> "TPM command timed out during continue self test");
> - msleep(delay_msec);
> + usleep_range(delay_usec,
> + delay_usec + TPM_TIMEOUT_RANGE_US);
> continue;
> }
>
> @@ -877,7 +879,7 @@ int tpm_do_selftest(struct tpm_chip *chip)
> }
> if (rc != TPM_WARN_DOING_SELFTEST)
> return rc;
> - msleep(delay_msec);
> + usleep_range(delay_usec, delay_usec + TPM_TIMEOUT_RANGE_US);
> } while (--loops > 0);
>
> return rc;
> @@ -977,7 +979,8 @@ again:
> }
> } else {
> do {
> - msleep(TPM_TIMEOUT);
> + usleep_range(TPM_TIMEOUT_US,
> + TPM_TIMEOUT_US + TPM_TIMEOUT_RANGE_US);
> status = chip->ops->status(chip);
> if ((status & mask) == mask)
> return 0;
> @@ -1045,7 +1048,8 @@ int tpm_pm_suspend(struct device *dev)
> */
> if (rc != TPM_WARN_RETRY)
> break;
> - msleep(TPM_TIMEOUT_RETRY);
> + usleep_range(TPM_TIMEOUT_RETRY_US,
> + TPM_TIMEOUT_RETRY_US + TPM_TIMEOUT_RANGE_US);
> }
>
> if (rc)
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index 4937b56..c1ace6c 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -45,8 +45,15 @@ enum tpm_const {
> };
>
> enum tpm_timeout {
> - TPM_TIMEOUT = 5, /* msecs */
> - TPM_TIMEOUT_RETRY = 100 /* msecs */
> + TPM_TIMEOUT = 5, /* msecs */
> + TPM_TIMEOUT_RETRY = 100, /* msecs */
> + /* Ideally all the drivers should be using the usecs values,
> + * the msecs values are only kept there for compatibility purposes
> + * with the remaining untested drivers.
> + */
> + TPM_TIMEOUT_US = TPM_TIMEOUT * 1000, /* usecs */
> + TPM_TIMEOUT_RETRY_US = TPM_TIMEOUT_RETRY * 1000, /* usecs */
> + TPM_TIMEOUT_RANGE_US = 300 /* usecs */
These renames are not needed and the comment doesn't make any sense
when you have _US postfix. Just keep them in msecs and implement the
wrapper. The exception is of course the new constant
TPM_TIMEOUT_RANGE_US, which you need to implement tpm_msleep().
> };
>
> /* TPM addresses */
> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
> index 881aea9..425caa4 100644
> --- a/drivers/char/tpm/tpm2-cmd.c
> +++ b/drivers/char/tpm/tpm2-cmd.c
> @@ -930,14 +930,14 @@ static int tpm2_do_selftest(struct tpm_chip *chip)
> {
> int rc;
> unsigned int loops;
> - unsigned int delay_msec = 100;
> + unsigned int delay_usec = 100000;
> unsigned long duration;
> struct tpm2_cmd cmd;
> int i;
>
> duration = tpm2_calc_ordinal_duration(chip, TPM2_CC_SELF_TEST);
>
> - loops = jiffies_to_msecs(duration) / delay_msec;
> + loops = jiffies_to_usecs(duration) / delay_usec;
>
> rc = tpm2_start_selftest(chip, true);
> if (rc)
> @@ -961,7 +961,7 @@ static int tpm2_do_selftest(struct tpm_chip *chip)
> if (rc != TPM2_RC_TESTING)
> break;
>
> - msleep(delay_msec);
> + usleep_range(delay_usec, delay_usec + TPM_TIMEOUT_RANGE_US);
> }
>
> return rc;
> diff --git a/drivers/char/tpm/tpm_infineon.c b/drivers/char/tpm/tpm_infineon.c
> index e3cf9f3..c9d2d13 100644
> --- a/drivers/char/tpm/tpm_infineon.c
> +++ b/drivers/char/tpm/tpm_infineon.c
> @@ -22,10 +22,10 @@
> /* Infineon specific definitions */
> /* maximum number of WTX-packages */
> #define TPM_MAX_WTX_PACKAGES 50
> -/* msleep-Time for WTX-packages */
> -#define TPM_WTX_MSLEEP_TIME 20
> -/* msleep-Time --> Interval to check status register */
> -#define TPM_MSLEEP_TIME 3
> +/* usleep-Time for WTX-packages */
> +#define TPM_WTX_USLEEP_TIME (20 * 1000)
> +/* usleep-Time --> Interval to check status register */
> +#define TPM_USLEEP_TIME (3 * 1000)
> /* gives number of max. msleep()-calls before throwing timeout */
> #define TPM_MAX_TRIES 5000
> #define TPM_INFINEON_DEV_VEN_VALUE 0x15D1
> @@ -191,7 +191,8 @@ static int wait(struct tpm_chip *chip, int wait_for_bit)
> /* check the status-register if wait_for_bit is set */
> if (status & 1 << wait_for_bit)
> break;
> - msleep(TPM_MSLEEP_TIME);
> + usleep_range(TPM_USLEEP_TIME,
> + TPM_USLEEP_TIME + TPM_TIMEOUT_RANGE_US);
> }
> if (i == TPM_MAX_TRIES) { /* timeout occurs */
> if (wait_for_bit == STAT_XFE)
> @@ -226,7 +227,8 @@ static void tpm_wtx(struct tpm_chip *chip)
> wait_and_send(chip, TPM_CTRL_WTX);
> wait_and_send(chip, 0x00);
> wait_and_send(chip, 0x00);
> - msleep(TPM_WTX_MSLEEP_TIME);
> + usleep_range(TPM_WTX_USLEEP_TIME,
> + TPM_WTX_USLEEP_TIME + TPM_TIMEOUT_RANGE_US);
> }
>
> static void tpm_wtx_abort(struct tpm_chip *chip)
> @@ -237,7 +239,8 @@ static void tpm_wtx_abort(struct tpm_chip *chip)
> wait_and_send(chip, 0x00);
> wait_and_send(chip, 0x00);
> number_of_wtx = 0;
> - msleep(TPM_WTX_MSLEEP_TIME);
> + usleep_range(TPM_WTX_USLEEP_TIME,
> + TPM_WTX_USLEEP_TIME + TPM_TIMEOUT_RANGE_US);
> }
>
> static int tpm_inf_recv(struct tpm_chip *chip, u8 * buf, size_t count)
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index c0f296b..93e3233 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -51,7 +51,8 @@ static int wait_startup(struct tpm_chip *chip, int l)
>
> if (access & TPM_ACCESS_VALID)
> return 0;
> - msleep(TPM_TIMEOUT);
> + usleep_range(TPM_TIMEOUT_US,
> + TPM_TIMEOUT_US + TPM_TIMEOUT_RANGE_US);
> } while (time_before(jiffies, stop));
> return -1;
> }
> @@ -125,7 +126,8 @@ again:
> do {
> if (check_locality(chip, l) >= 0)
> return l;
> - msleep(TPM_TIMEOUT);
> + usleep_range(TPM_TIMEOUT_US,
> + TPM_TIMEOUT_US + TPM_TIMEOUT_RANGE_US);
> } while (time_before(jiffies, stop));
> }
> return -1;
> @@ -170,7 +172,8 @@ static int get_burstcount(struct tpm_chip *chip)
> burstcnt = (value >> 8) & 0xFFFF;
> if (burstcnt)
> return burstcnt;
> - msleep(TPM_TIMEOUT);
> + usleep_range(TPM_TIMEOUT_US,
> + TPM_TIMEOUT_US + TPM_TIMEOUT_RANGE_US);
> } while (time_before(jiffies, stop));
> return -EBUSY;
> }
> @@ -408,7 +411,7 @@ static int tpm_tis_send(struct tpm_chip *chip, u8 *buf,
> size_t len)
> priv->irq = irq;
> chip->flags |= TPM_CHIP_FLAG_IRQ;
> if (!priv->irq_tested)
> - msleep(1);
> + usleep_range(1000, 1000 + TPM_TIMEOUT_RANGE_US);
> if (!priv->irq_tested)
> disable_interrupts(chip);
> priv->irq_tested = true;
> --
> 2.7.4
>
PS. Please add linux-kernel and linux-security-module to CCs for the
next version and thanks for working on this improvement.
/Jarkko
------------------------------------------------------------------------------
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
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel
