Re: [PATCH] msleep() delays - replace with usleep_range() in TPM 1.2/2.0 generic drivers

Re: [PATCH] msleep() delays - replace with usleep_range() in TPM 1.2/2.0 generic drivers

[Jarkko Sakkinen]

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 <hamza-ZPxbGqLxI0U@public.gmane.org>

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

Re: [PATCH] msleep() delays - replace with usleep_range() in TPM 1.2/2.0 generic drivers

[Jarkko Sakkinen]

Your subject line looks wrong. This cannot be cleanly applied with git
am.

Please go through https://kernelnewbies.org/FirstKernelPatch

On Wed, Jul 12, 2017 at 12:34:19PM +0000, Attak, Hamza wrote:
> msleep() delays - replace with usleep_range() in TPM 1.2/2.0 generic drivers
> 
> 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 <hamza-ZPxbGqLxI0U@public.gmane.org>
> ---
>  drivers/char/tpm/tpm-interface.c | 13 ++++++++-----
>  drivers/char/tpm/tpm2-cmd.c      |  2 +-
>  drivers/char/tpm/tpm_infineon.c  |  9 ++++++---
>  drivers/char/tpm/tpm_tis_core.c  |  9 +++++----
>  4 files changed, 20 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index bd2128e..513b801 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -395,7 +395,7 @@ ssize_t tpm_transmit(struct tpm_chip *chip, const u8 *buf, size_t bufsiz,
>  			goto out;
>  		}
>  
> -		msleep(TPM_TIMEOUT);	/* CHECK */
> +		usleep_range(TPM_TIMEOUT * 1000, (TPM_TIMEOUT * 1000) + 300);

Plese look into.

https://patchwork.kernel.org/patch/9617823/

Now you are sprinkling magic numbers everywhere.

>  		rmb();
>  	} while (time_before(jiffies, stop));
>  
> @@ -862,7 +862,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_msec * 1000,
> +				(delay_msec * 1000) + 300);

The second line dooes not look properly aligned.

There's probably similar style issues elsewhere in the patch (at least
spotted one after this).

/Jarkko

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Re: [PATCH] msleep() delays - replace with usleep_range() in TPM 1.2/2.0 generic drivers

[Attak, Hamza]

msleep() delays - replace with usleep_range() in TPM 1.2/2.0 generic drivers

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 <hamza-ZPxbGqLxI0U@public.gmane.org>
---
 drivers/char/tpm/tpm-interface.c | 13 ++++++++-----
 drivers/char/tpm/tpm2-cmd.c      |  2 +-
 drivers/char/tpm/tpm_infineon.c  |  9 ++++++---
 drivers/char/tpm/tpm_tis_core.c  |  9 +++++----
 4 files changed, 20 insertions(+), 13 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index bd2128e..513b801 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -395,7 +395,7 @@ ssize_t tpm_transmit(struct tpm_chip *chip, const u8 *buf, size_t bufsiz,
 			goto out;
 		}
 
-		msleep(TPM_TIMEOUT);	/* CHECK */
+		usleep_range(TPM_TIMEOUT * 1000, (TPM_TIMEOUT * 1000) + 300);
 		rmb();
 	} while (time_before(jiffies, stop));
 
@@ -862,7 +862,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_msec * 1000,
+				(delay_msec * 1000) + 300);
 			continue;
 		}
 
@@ -877,7 +878,7 @@ int tpm_do_selftest(struct tpm_chip *chip)
 		}
 		if (rc != TPM_WARN_DOING_SELFTEST)
 			return rc;
-		msleep(delay_msec);
+		usleep_range(delay_msec * 1000, (delay_msec * 1000) + 300);
 	} while (--loops > 0);
 
 	return rc;
@@ -977,7 +978,8 @@ again:
 		}
 	} else {
 		do {
-			msleep(TPM_TIMEOUT);
+			usleep_range(TPM_TIMEOUT * 1000,
+				(TPM_TIMEOUT * 1000) + 300);
 			status = chip->ops->status(chip);
 			if ((status & mask) == mask)
 				return 0;
@@ -1045,7 +1047,8 @@ int tpm_pm_suspend(struct device *dev)
 		 */
 		if (rc != TPM_WARN_RETRY)
 			break;
-		msleep(TPM_TIMEOUT_RETRY);
+		usleep_range(TPM_TIMEOUT_RETRY * 1000,
+			(TPM_TIMEOUT_RETRY * 1000) + 300);
 	}
 
 	if (rc)
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index 881aea9..bdae205 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -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_msec * 1000, (delay_msec * 1000) + 300);
 	}
 
 	return rc;
diff --git a/drivers/char/tpm/tpm_infineon.c b/drivers/char/tpm/tpm_infineon.c
index e3cf9f3..1846151 100644
--- a/drivers/char/tpm/tpm_infineon.c
+++ b/drivers/char/tpm/tpm_infineon.c
@@ -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_MSLEEP_TIME * 1000,
+			(TPM_MSLEEP_TIME * 1000) + 300);
 	}
 	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_MSLEEP_TIME * 1000,
+		(TPM_WTX_MSLEEP_TIME * 1000) + 300);
 }
 
 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_MSLEEP_TIME * 1000,
+		(TPM_WTX_MSLEEP_TIME * 1000) + 300);
 }
 
 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..62c0915 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -51,7 +51,7 @@ static int wait_startup(struct tpm_chip *chip, int l)
 
 		if (access & TPM_ACCESS_VALID)
 			return 0;
-		msleep(TPM_TIMEOUT);
+		usleep_range(TPM_TIMEOUT * 1000, (TPM_TIMEOUT * 1000) + 300);
 	} while (time_before(jiffies, stop));
 	return -1;
 }
@@ -125,7 +125,8 @@ again:
 		do {
 			if (check_locality(chip, l) >= 0)
 				return l;
-			msleep(TPM_TIMEOUT);
+			usleep_range(TPM_TIMEOUT * 1000,
+				(TPM_TIMEOUT * 1000) + 300);
 		} while (time_before(jiffies, stop));
 	}
 	return -1;
@@ -170,7 +171,7 @@ static int get_burstcount(struct tpm_chip *chip)
 		burstcnt = (value >> 8) & 0xFFFF;
 		if (burstcnt)
 			return burstcnt;
-		msleep(TPM_TIMEOUT);
+		usleep_range(TPM_TIMEOUT * 1000, (TPM_TIMEOUT * 1000) + 300);
 	} while (time_before(jiffies, stop));
 	return -EBUSY;
 }
@@ -408,7 +409,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, 1300);
 	if (!priv->irq_tested)
 		disable_interrupts(chip);
 	priv->irq_tested = true;
-- 
2.7.4


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Re: [PATCH] msleep() delays - replace with usleep_range() in TPM 1.2/2.0 generic drivers

[Attak, Hamza]

1. I guess you meant not to "base" my work on an unapplied patch. The patch we
   are submitting is not dependant on the referenced, earlier patch.
   It is also not including any of its changes. The former patch was only
   mentioned to give context, as it is a similar approach, however it is 
   addressing another problem (Nuvoton driver). This reference can as well be
   removed if misleading.
2. Linebreaks in commit message should be fixed now.
3. The URL has been removed, so as the reference to the former patch.

No, I didn't run checkpatch.pl when first submitting the patch. Testing it with
checkpatch.pl, the output gave me warning about lines over 80 chars. They are
now solved.
I am sending an updated version of the patch.

Thanks,
Hamza ATTAK.


-----Original Message-----
From: Jarkko Sakkinen [mailto:jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org] 
Sent: 10 July 2017 20:15
To: Attak, Hamza <hamza.attak-ZPxbGqLxI0U@public.gmane.org>
Cc: tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org; Jacquin, Ludovic <ludo-ZPxbGqLxI0U@public.gmane.org>; Edwards, Nigel <nigel.edwards-ZPxbGqLxI0U@public.gmane.org>
Subject: Re: [tpmdd-devel] [PATCH] msleep() delays - replace with usleep_range() in TPM 1.2/2.0 generic drivers

On Mon, Jul 10, 2017 at 01:26:51PM +0000, Attak, Hamza wrote:
> msleep() delays - replace with usleep_range() in TPM 1.2/2.0 generic 
> drivers
> 
> Following the recent Nuvoton patch submitted by Mimi Zohar:
> https://sourceforge.net/p/tpmdd/mailman/message/35685260/

1. Please do not your work on a patch that was never applied.
   This one was https://patchwork.kernel.org/patch/9617835/
2. Please break your lines properly in the commit message 3. Please use commit ID in the mainline instead of URL.

Have you run scripts/checkpatch.pl?

/Jarkko

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Re: [PATCH] msleep() delays - replace with usleep_range() in TPM 1.2/2.0 generic drivers

[Jarkko Sakkinen]

On Mon, Jul 10, 2017 at 01:26:51PM +0000, Attak, Hamza wrote:
> msleep() delays - replace with usleep_range() in TPM 1.2/2.0 generic drivers
> 
> Following the recent Nuvoton patch submitted by Mimi Zohar:
> https://sourceforge.net/p/tpmdd/mailman/message/35685260/

1. Please do not your work on a patch that was never applied.
   This one was https://patchwork.kernel.org/patch/9617835/ 
2. Please break your lines properly in the commit message
3. Please use commit ID in the mainline instead of URL.

Have you run scripts/checkpatch.pl?

/Jarkko

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

[PATCH] msleep() delays - replace with usleep_range() in TPM 1.2/2.0 generic drivers

[Attak, Hamza]

msleep() delays - replace with usleep_range() in TPM 1.2/2.0 generic drivers

Following the recent Nuvoton patch submitted by Mimi Zohar:
https://sourceforge.net/p/tpmdd/mailman/message/35685260/

Based on this work, I decided to apply a similar modification to the generic TPM drivers in the kernel.
The patch simply replaces all msleep function calls with usleep_range calls in the generic drivers. As a former patch already exists for the Nuvoton TPM, it is therefore not included in this new patch.

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 <hamza-ZPxbGqLxI0U@public.gmane.org>
---
 drivers/char/tpm/tpm-interface.c | 10 +++++-----
 drivers/char/tpm/tpm2-cmd.c      |  2 +-
 drivers/char/tpm/tpm_infineon.c  |  6 +++---
 drivers/char/tpm/tpm_tis_core.c  |  8 ++++----
 4 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index bd2128e..f03daf5 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -395,7 +395,7 @@ ssize_t tpm_transmit(struct tpm_chip *chip, const u8 *buf, size_t bufsiz,
 			goto out;
 		}
 
-		msleep(TPM_TIMEOUT);	/* CHECK */
+		usleep_range(TPM_TIMEOUT * 1000, (TPM_TIMEOUT * 1000) + 300);
 		rmb();
 	} while (time_before(jiffies, stop));
 
@@ -862,7 +862,7 @@ 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_msec * 1000, (delay_msec * 1000) + 300);
 			continue;
 		}
 
@@ -877,7 +877,7 @@ int tpm_do_selftest(struct tpm_chip *chip)
 		}
 		if (rc != TPM_WARN_DOING_SELFTEST)
 			return rc;
-		msleep(delay_msec);
+		usleep_range(delay_msec * 1000, (delay_msec * 1000) + 300);
 	} while (--loops > 0);
 
 	return rc;
@@ -977,7 +977,7 @@ again:
 		}
 	} else {
 		do {
-			msleep(TPM_TIMEOUT);
+			usleep_range(TPM_TIMEOUT * 1000, (TPM_TIMEOUT * 1000) + 300);
 			status = chip->ops->status(chip);
 			if ((status & mask) == mask)
 				return 0;
@@ -1045,7 +1045,7 @@ int tpm_pm_suspend(struct device *dev)
 		 */
 		if (rc != TPM_WARN_RETRY)
 			break;
-		msleep(TPM_TIMEOUT_RETRY);
+		usleep_range(TPM_TIMEOUT_RETRY * 1000, (TPM_TIMEOUT_RETRY * 1000) + 300);
 	}
 
 	if (rc)
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index 881aea9..bdae205 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -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_msec * 1000, (delay_msec * 1000) + 300);
 	}
 
 	return rc;
diff --git a/drivers/char/tpm/tpm_infineon.c b/drivers/char/tpm/tpm_infineon.c
index e3cf9f3..86f0f3e 100644
--- a/drivers/char/tpm/tpm_infineon.c
+++ b/drivers/char/tpm/tpm_infineon.c
@@ -191,7 +191,7 @@ 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_MSLEEP_TIME * 1000, (TPM_MSLEEP_TIME * 1000) + 300);
 	}
 	if (i == TPM_MAX_TRIES) {	/* timeout occurs */
 		if (wait_for_bit == STAT_XFE)
@@ -226,7 +226,7 @@ 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_MSLEEP_TIME * 1000, (TPM_WTX_MSLEEP_TIME * 1000) + 300);
 }
 
 static void tpm_wtx_abort(struct tpm_chip *chip)
@@ -237,7 +237,7 @@ 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_MSLEEP_TIME * 1000, (TPM_WTX_MSLEEP_TIME * 1000) + 300);
 }
 
 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..e7d3046 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -51,7 +51,7 @@ static int wait_startup(struct tpm_chip *chip, int l)
 
 		if (access & TPM_ACCESS_VALID)
 			return 0;
-		msleep(TPM_TIMEOUT);
+		usleep_range(TPM_TIMEOUT * 1000, (TPM_TIMEOUT * 1000) + 300);
 	} while (time_before(jiffies, stop));
 	return -1;
 }
@@ -125,7 +125,7 @@ again:
 		do {
 			if (check_locality(chip, l) >= 0)
 				return l;
-			msleep(TPM_TIMEOUT);
+			usleep_range(TPM_TIMEOUT * 1000, (TPM_TIMEOUT * 1000) + 300);
 		} while (time_before(jiffies, stop));
 	}
 	return -1;
@@ -170,7 +170,7 @@ static int get_burstcount(struct tpm_chip *chip)
 		burstcnt = (value >> 8) & 0xFFFF;
 		if (burstcnt)
 			return burstcnt;
-		msleep(TPM_TIMEOUT);
+		usleep_range(TPM_TIMEOUT * 1000, (TPM_TIMEOUT * 1000) + 300);
 	} while (time_before(jiffies, stop));
 	return -EBUSY;
 }
@@ -408,7 +408,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, 1300);
 	if (!priv->irq_tested)
 		disable_interrupts(chip);
 	priv->irq_tested = true;
-- 
2.7.4



------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

[PATCH] msleep() delays - replace with usleep_range() in TPM 1.2/2.0 generic drivers

[Attak, Hamza]

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

Following the recent Nuvoton patch submitted by Mimi Zohar:
https://sourceforge.net/p/tpmdd/mailman/message/35685260/

Based on this work, I decided to apply a similar modification to the generic TPM drivers in the kernel.
The patch simply replaces all msleep function calls with usleep_range calls in the generic drivers. As a former patch already exists for the Nuvoton TPM, it is therefore not included in this new patch.

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.

Thanks,
Hamza ATTAK.


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

[-- Attachment #2: tpm_usleep_range_tested.patch --]
[-- Type: application/octet-stream, Size: 4432 bytes --]

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index bd2128e..f03daf5 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -395,7 +395,7 @@ ssize_t tpm_transmit(struct tpm_chip *chip, const u8 *buf, size_t bufsiz,
 			goto out;
 		}
 
-		msleep(TPM_TIMEOUT);	/* CHECK */
+		usleep_range(TPM_TIMEOUT * 1000, (TPM_TIMEOUT * 1000) + 300);
 		rmb();
 	} while (time_before(jiffies, stop));
 
@@ -862,7 +862,7 @@ 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_msec * 1000, (delay_msec * 1000) + 300);
 			continue;
 		}
 
@@ -877,7 +877,7 @@ int tpm_do_selftest(struct tpm_chip *chip)
 		}
 		if (rc != TPM_WARN_DOING_SELFTEST)
 			return rc;
-		msleep(delay_msec);
+		usleep_range(delay_msec * 1000, (delay_msec * 1000) + 300);
 	} while (--loops > 0);
 
 	return rc;
@@ -977,7 +977,7 @@ int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask, unsigned long timeout,
 		}
 	} else {
 		do {
-			msleep(TPM_TIMEOUT);
+			usleep_range(TPM_TIMEOUT * 1000, (TPM_TIMEOUT * 1000) + 300);
 			status = chip->ops->status(chip);
 			if ((status & mask) == mask)
 				return 0;
@@ -1045,7 +1045,7 @@ int tpm_pm_suspend(struct device *dev)
 		 */
 		if (rc != TPM_WARN_RETRY)
 			break;
-		msleep(TPM_TIMEOUT_RETRY);
+		usleep_range(TPM_TIMEOUT_RETRY * 1000, (TPM_TIMEOUT_RETRY * 1000) + 300);
 	}
 
 	if (rc)
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index 881aea9..bdae205 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -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_msec * 1000, (delay_msec * 1000) + 300);
 	}
 
 	return rc;
diff --git a/drivers/char/tpm/tpm_infineon.c b/drivers/char/tpm/tpm_infineon.c
index e3cf9f3..86f0f3e 100644
--- a/drivers/char/tpm/tpm_infineon.c
+++ b/drivers/char/tpm/tpm_infineon.c
@@ -191,7 +191,7 @@ 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_MSLEEP_TIME * 1000, (TPM_MSLEEP_TIME * 1000) + 300);
 	}
 	if (i == TPM_MAX_TRIES) {	/* timeout occurs */
 		if (wait_for_bit == STAT_XFE)
@@ -226,7 +226,7 @@ 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_MSLEEP_TIME * 1000, (TPM_WTX_MSLEEP_TIME * 1000) + 300);
 }
 
 static void tpm_wtx_abort(struct tpm_chip *chip)
@@ -237,7 +237,7 @@ 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_MSLEEP_TIME * 1000, (TPM_WTX_MSLEEP_TIME * 1000) + 300);
 }
 
 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..e7d3046 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -51,7 +51,7 @@ static int wait_startup(struct tpm_chip *chip, int l)
 
 		if (access & TPM_ACCESS_VALID)
 			return 0;
-		msleep(TPM_TIMEOUT);
+		usleep_range(TPM_TIMEOUT * 1000, (TPM_TIMEOUT * 1000) + 300);
 	} while (time_before(jiffies, stop));
 	return -1;
 }
@@ -125,7 +125,7 @@ static int request_locality(struct tpm_chip *chip, int l)
 		do {
 			if (check_locality(chip, l) >= 0)
 				return l;
-			msleep(TPM_TIMEOUT);
+			usleep_range(TPM_TIMEOUT * 1000, (TPM_TIMEOUT * 1000) + 300);
 		} while (time_before(jiffies, stop));
 	}
 	return -1;
@@ -170,7 +170,7 @@ static int get_burstcount(struct tpm_chip *chip)
 		burstcnt = (value >> 8) & 0xFFFF;
 		if (burstcnt)
 			return burstcnt;
-		msleep(TPM_TIMEOUT);
+		usleep_range(TPM_TIMEOUT * 1000, (TPM_TIMEOUT * 1000) + 300);
 	} while (time_before(jiffies, stop));
 	return -EBUSY;
 }
@@ -408,7 +408,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, 1300);
 	if (!priv->irq_tested)
 		disable_interrupts(chip);
 	priv->irq_tested = true;

[-- Attachment #3: Type: text/plain, Size: 202 bytes --]

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

[-- Attachment #4: Type: text/plain, Size: 192 bytes --]

_______________________________________________
tpmdd-devel mailing list
tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel