Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1442615 > unrolled thread

[PATCH v9 0/5] tpm: Command duration logging and chip-specific override

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v9 0/5] tpm: Command duration logging and chip-specific override Ed Swierk <eswierk@skyportsystems.com> - 2016-07-13 18:30 +0200
    [PATCH v9 3/5] tpm: Clean up reading of timeout and duration capabilities Ed Swierk <eswierk@skyportsystems.com> - 2016-07-13 18:30 +0200
      Re: [PATCH v9 3/5] tpm: Clean up reading of timeout and duration  capabilities Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2016-07-18 20:20 +0200
      Re: [PATCH v9 3/5] tpm: Clean up reading of timeout and duration  capabilities Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2016-07-18 20:30 +0200
        Re: [PATCH v9 3/5] tpm: Clean up reading of timeout and duration  capabilities Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2016-07-18 20:30 +0200
    [PATCH v9 1/5] tpm_tis: Improve reporting of IO errors Ed Swierk <eswierk@skyportsystems.com> - 2016-07-13 18:30 +0200
    [PATCH v9 2/5] tpm: Add optional logging of TPM command durations Ed Swierk <eswierk@skyportsystems.com> - 2016-07-13 18:30 +0200
    [PATCH v9 4/5] tpm: Allow TPM chip drivers to override reported command durations Ed Swierk <eswierk@skyportsystems.com> - 2016-07-13 18:30 +0200
      Re: [PATCH v9 4/5] tpm: Allow TPM chip drivers to override reported  command durations kbuild test robot <lkp@intel.com> - 2016-07-13 19:10 +0200
      Re: [PATCH v9 4/5] tpm: Allow TPM chip drivers to override reported  command durations Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2016-07-18 20:50 +0200
    Re: [PATCH v9 0/5] tpm: Command duration logging and chip-specific override Ed Swierk <eswierk@skyportsystems.com> - 2016-07-13 18:50 +0200
      Re: [PATCH v9 0/5] tpm: Command duration logging and chip-specific  override Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2016-07-13 19:40 +0200
        Re: [PATCH v9 0/5] tpm: Command duration logging and chip-specific override Ed Swierk <eswierk@skyportsystems.com> - 2016-07-13 22:10 +0200
          Re: [PATCH v9 0/5] tpm: Command duration logging and chip-specific  override Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2016-07-13 23:00 +0200
          Re: [PATCH v9 0/5] tpm: Command duration logging and chip-specific override ebiederm@xmission.com (Eric W. Biederman) - 2016-07-13 23:20 +0200
    Re: [PATCH v9 0/5] tpm: Command duration logging and chip-specific  override Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2016-07-18 20:10 +0200

#1442615 — [PATCH v9 0/5] tpm: Command duration logging and chip-specific override

v9: Include command duration in existing error messages rather than
logging an extra debug message. Rebase onto Jarkko's tree.

v8: Fix v7 goof-up in tpm_getcap().

v7: Use tpm_getcap() instead of a redundant new function.

v6: Split tpm_get_cap_prop() out of tpm_get_timeouts(); always return
error on TPM command failure.

v5: Use msecs_to_jiffies() instead of * HZ / 1000.

v4: Rework tpm_get_timeouts() to allow overriding both timeouts and
durations via a single callback.

This series
- improves TPM command error reporting
- adds optional logging of TPM command durations
- allows chip-specific override of command durations as well as protocol
  timeouts
- overrides ST19NP18 TPM command duration to avoid lockups

Ed Swierk (5):
  tpm_tis: Improve reporting of IO errors
  tpm: Add optional logging of TPM command durations
  tpm: Clean up reading of timeout and duration capabilities
  tpm: Allow TPM chip drivers to override reported command durations
  tpm_tis: Increase ST19NP18 TPM command duration to avoid chip lockup

 drivers/char/tpm/tpm-interface.c | 219 ++++++++++++++++++++-------------------
 drivers/char/tpm/tpm_tis_core.c  |  46 ++++----
 include/linux/tpm.h              |   3 +-
 3 files changed, 136 insertions(+), 132 deletions(-)

-- 
1.9.1

[toc] | [next] | [standalone]

#1442616 — [PATCH v9 3/5] tpm: Clean up reading of timeout and duration capabilities

Call tpm_getcap() from tpm_get_timeouts() to eliminate redundant
code. Return all errors to the caller rather than swallowing them
(e.g. when tpm_transmit_cmd() returns nonzero).

Signed-off-by: Ed Swierk <eswierk@skyportsystems.com>
---
 drivers/char/tpm/tpm-interface.c | 74 +++++++++++++++-------------------------
 1 file changed, 27 insertions(+), 47 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index a4beb53..dc492ee 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -460,9 +460,19 @@ ssize_t tpm_getcap(struct tpm_chip *chip, __be32 subcap_id, cap_t *cap,
 		tpm_cmd.params.getcap_in.subcap_size = cpu_to_be32(4);
 		tpm_cmd.params.getcap_in.subcap = subcap_id;
 	}
+
 	rc = tpm_transmit_cmd(chip, &tpm_cmd, TPM_INTERNAL_RESULT_SIZE, desc);
+
+	if (!rc &&
+	    ((subcap_id == TPM_CAP_PROP_TIS_TIMEOUT &&
+	      be32_to_cpu(tpm_cmd.header.out.length) != TPM_HEADER_SIZE + 20) ||
+	     (subcap_id == TPM_CAP_PROP_TIS_DURATION &&
+	      be32_to_cpu(tpm_cmd.header.out.length) != TPM_HEADER_SIZE + 16)))
+		rc = -EINVAL;
+
 	if (!rc)
 		*cap = tpm_cmd.params.getcap_out.cap;
+
 	return rc;
 }
 
@@ -503,10 +513,9 @@ static int tpm_startup(struct tpm_chip *chip, __be16 startup_type)
 
 int tpm_get_timeouts(struct tpm_chip *chip)
 {
-	struct tpm_cmd_t tpm_cmd;
+	cap_t cap;
 	unsigned long new_timeout[4];
 	unsigned long old_timeout[4];
-	struct duration_t *duration_cap;
 	ssize_t rc;
 
 	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
@@ -524,42 +533,25 @@ int tpm_get_timeouts(struct tpm_chip *chip)
 		return 0;
 	}
 
-	tpm_cmd.header.in = tpm_getcap_header;
-	tpm_cmd.params.getcap_in.cap = TPM_CAP_PROP;
-	tpm_cmd.params.getcap_in.subcap_size = cpu_to_be32(4);
-	tpm_cmd.params.getcap_in.subcap = TPM_CAP_PROP_TIS_TIMEOUT;
-	rc = tpm_transmit_cmd(chip, &tpm_cmd, TPM_INTERNAL_RESULT_SIZE, NULL);
-
+	rc = tpm_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, &cap,
+			"attempting to determine the timeouts");
 	if (rc == TPM_ERR_INVALID_POSTINIT) {
 		/* The TPM is not started, we are the first to talk to it.
 		   Execute a startup command. */
-		dev_info(&chip->dev, "Issuing TPM_STARTUP");
+		dev_info(&chip->dev, "Issuing TPM_STARTUP\n");
 		if (tpm_startup(chip, TPM_ST_CLEAR))
 			return rc;
 
-		tpm_cmd.header.in = tpm_getcap_header;
-		tpm_cmd.params.getcap_in.cap = TPM_CAP_PROP;
-		tpm_cmd.params.getcap_in.subcap_size = cpu_to_be32(4);
-		tpm_cmd.params.getcap_in.subcap = TPM_CAP_PROP_TIS_TIMEOUT;
-		rc = tpm_transmit_cmd(chip, &tpm_cmd, TPM_INTERNAL_RESULT_SIZE,
-				  NULL);
-	}
-	if (rc) {
-		dev_err(&chip->dev,
-			"A TPM error (%zd) occurred attempting to determine the timeouts\n",
-			rc);
-		goto duration;
+		rc = tpm_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, &cap,
+				"attempting to determine the timeouts");
 	}
+	if (rc)
+		return rc;
 
-	if (be32_to_cpu(tpm_cmd.header.out.return_code) != 0 ||
-	    be32_to_cpu(tpm_cmd.header.out.length)
-	    != sizeof(tpm_cmd.header.out) + sizeof(u32) + 4 * sizeof(u32))
-		return -EINVAL;
-
-	old_timeout[0] = be32_to_cpu(tpm_cmd.params.getcap_out.cap.timeout.a);
-	old_timeout[1] = be32_to_cpu(tpm_cmd.params.getcap_out.cap.timeout.b);
-	old_timeout[2] = be32_to_cpu(tpm_cmd.params.getcap_out.cap.timeout.c);
-	old_timeout[3] = be32_to_cpu(tpm_cmd.params.getcap_out.cap.timeout.d);
+	old_timeout[0] = be32_to_cpu(cap.timeout.a);
+	old_timeout[1] = be32_to_cpu(cap.timeout.b);
+	old_timeout[2] = be32_to_cpu(cap.timeout.c);
+	old_timeout[3] = be32_to_cpu(cap.timeout.d);
 	memcpy(new_timeout, old_timeout, sizeof(new_timeout));
 
 	/*
@@ -597,29 +589,17 @@ int tpm_get_timeouts(struct tpm_chip *chip)
 	chip->timeout_c = usecs_to_jiffies(new_timeout[2]);
 	chip->timeout_d = usecs_to_jiffies(new_timeout[3]);
 
-duration:
-	tpm_cmd.header.in = tpm_getcap_header;
-	tpm_cmd.params.getcap_in.cap = TPM_CAP_PROP;
-	tpm_cmd.params.getcap_in.subcap_size = cpu_to_be32(4);
-	tpm_cmd.params.getcap_in.subcap = TPM_CAP_PROP_TIS_DURATION;
-
-	rc = tpm_transmit_cmd(chip, &tpm_cmd, TPM_INTERNAL_RESULT_SIZE,
-			      "attempting to determine the durations");
+	rc = tpm_getcap(chip, TPM_CAP_PROP_TIS_DURATION, &cap,
+			"attempting to determine the durations");
 	if (rc)
 		return rc;
 
-	if (be32_to_cpu(tpm_cmd.header.out.return_code) != 0 ||
-	    be32_to_cpu(tpm_cmd.header.out.length)
-	    != sizeof(tpm_cmd.header.out) + sizeof(u32) + 3 * sizeof(u32))
-		return -EINVAL;
-
-	duration_cap = &tpm_cmd.params.getcap_out.cap.duration;
 	chip->duration[TPM_SHORT] =
-	    usecs_to_jiffies(be32_to_cpu(duration_cap->tpm_short));
+		usecs_to_jiffies(be32_to_cpu(cap.duration.tpm_short));
 	chip->duration[TPM_MEDIUM] =
-	    usecs_to_jiffies(be32_to_cpu(duration_cap->tpm_medium));
+		usecs_to_jiffies(be32_to_cpu(cap.duration.tpm_medium));
 	chip->duration[TPM_LONG] =
-	    usecs_to_jiffies(be32_to_cpu(duration_cap->tpm_long));
+		usecs_to_jiffies(be32_to_cpu(cap.duration.tpm_long));
 
 	/* The Broadcom BCM0102 chipset in a Dell Latitude D820 gets the above
 	 * value wrong and apparently reports msecs rather than usecs. So we
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1445688 — Re: [PATCH v9 3/5] tpm: Clean up reading of timeout and duration capabilities

On Wed, Jul 13, 2016 at 09:19:34AM -0700, Ed Swierk wrote:
> Call tpm_getcap() from tpm_get_timeouts() to eliminate redundant
> code. Return all errors to the caller rather than swallowing them
> (e.g. when tpm_transmit_cmd() returns nonzero).
> 
> Signed-off-by: Ed Swierk <eswierk@skyportsystems.com>

Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

/Jarkko

> ---
>  drivers/char/tpm/tpm-interface.c | 74 +++++++++++++++-------------------------
>  1 file changed, 27 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index a4beb53..dc492ee 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -460,9 +460,19 @@ ssize_t tpm_getcap(struct tpm_chip *chip, __be32 subcap_id, cap_t *cap,
>  		tpm_cmd.params.getcap_in.subcap_size = cpu_to_be32(4);
>  		tpm_cmd.params.getcap_in.subcap = subcap_id;
>  	}
> +
>  	rc = tpm_transmit_cmd(chip, &tpm_cmd, TPM_INTERNAL_RESULT_SIZE, desc);
> +
> +	if (!rc &&
> +	    ((subcap_id == TPM_CAP_PROP_TIS_TIMEOUT &&
> +	      be32_to_cpu(tpm_cmd.header.out.length) != TPM_HEADER_SIZE + 20) ||
> +	     (subcap_id == TPM_CAP_PROP_TIS_DURATION &&
> +	      be32_to_cpu(tpm_cmd.header.out.length) != TPM_HEADER_SIZE + 16)))
> +		rc = -EINVAL;
> +
>  	if (!rc)
>  		*cap = tpm_cmd.params.getcap_out.cap;
> +
>  	return rc;
>  }
>  
> @@ -503,10 +513,9 @@ static int tpm_startup(struct tpm_chip *chip, __be16 startup_type)
>  
>  int tpm_get_timeouts(struct tpm_chip *chip)
>  {
> -	struct tpm_cmd_t tpm_cmd;
> +	cap_t cap;
>  	unsigned long new_timeout[4];
>  	unsigned long old_timeout[4];
> -	struct duration_t *duration_cap;
>  	ssize_t rc;
>  
>  	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
> @@ -524,42 +533,25 @@ int tpm_get_timeouts(struct tpm_chip *chip)
>  		return 0;
>  	}
>  
> -	tpm_cmd.header.in = tpm_getcap_header;
> -	tpm_cmd.params.getcap_in.cap = TPM_CAP_PROP;
> -	tpm_cmd.params.getcap_in.subcap_size = cpu_to_be32(4);
> -	tpm_cmd.params.getcap_in.subcap = TPM_CAP_PROP_TIS_TIMEOUT;
> -	rc = tpm_transmit_cmd(chip, &tpm_cmd, TPM_INTERNAL_RESULT_SIZE, NULL);
> -
> +	rc = tpm_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, &cap,
> +			"attempting to determine the timeouts");
>  	if (rc == TPM_ERR_INVALID_POSTINIT) {
>  		/* The TPM is not started, we are the first to talk to it.
>  		   Execute a startup command. */
> -		dev_info(&chip->dev, "Issuing TPM_STARTUP");
> +		dev_info(&chip->dev, "Issuing TPM_STARTUP\n");
>  		if (tpm_startup(chip, TPM_ST_CLEAR))
>  			return rc;
>  
> -		tpm_cmd.header.in = tpm_getcap_header;
> -		tpm_cmd.params.getcap_in.cap = TPM_CAP_PROP;
> -		tpm_cmd.params.getcap_in.subcap_size = cpu_to_be32(4);
> -		tpm_cmd.params.getcap_in.subcap = TPM_CAP_PROP_TIS_TIMEOUT;
> -		rc = tpm_transmit_cmd(chip, &tpm_cmd, TPM_INTERNAL_RESULT_SIZE,
> -				  NULL);
> -	}
> -	if (rc) {
> -		dev_err(&chip->dev,
> -			"A TPM error (%zd) occurred attempting to determine the timeouts\n",
> -			rc);
> -		goto duration;
> +		rc = tpm_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, &cap,
> +				"attempting to determine the timeouts");
>  	}
> +	if (rc)
> +		return rc;
>  
> -	if (be32_to_cpu(tpm_cmd.header.out.return_code) != 0 ||
> -	    be32_to_cpu(tpm_cmd.header.out.length)
> -	    != sizeof(tpm_cmd.header.out) + sizeof(u32) + 4 * sizeof(u32))
> -		return -EINVAL;
> -
> -	old_timeout[0] = be32_to_cpu(tpm_cmd.params.getcap_out.cap.timeout.a);
> -	old_timeout[1] = be32_to_cpu(tpm_cmd.params.getcap_out.cap.timeout.b);
> -	old_timeout[2] = be32_to_cpu(tpm_cmd.params.getcap_out.cap.timeout.c);
> -	old_timeout[3] = be32_to_cpu(tpm_cmd.params.getcap_out.cap.timeout.d);
> +	old_timeout[0] = be32_to_cpu(cap.timeout.a);
> +	old_timeout[1] = be32_to_cpu(cap.timeout.b);
> +	old_timeout[2] = be32_to_cpu(cap.timeout.c);
> +	old_timeout[3] = be32_to_cpu(cap.timeout.d);
>  	memcpy(new_timeout, old_timeout, sizeof(new_timeout));
>  
>  	/*
> @@ -597,29 +589,17 @@ int tpm_get_timeouts(struct tpm_chip *chip)
>  	chip->timeout_c = usecs_to_jiffies(new_timeout[2]);
>  	chip->timeout_d = usecs_to_jiffies(new_timeout[3]);
>  
> -duration:
> -	tpm_cmd.header.in = tpm_getcap_header;
> -	tpm_cmd.params.getcap_in.cap = TPM_CAP_PROP;
> -	tpm_cmd.params.getcap_in.subcap_size = cpu_to_be32(4);
> -	tpm_cmd.params.getcap_in.subcap = TPM_CAP_PROP_TIS_DURATION;
> -
> -	rc = tpm_transmit_cmd(chip, &tpm_cmd, TPM_INTERNAL_RESULT_SIZE,
> -			      "attempting to determine the durations");
> +	rc = tpm_getcap(chip, TPM_CAP_PROP_TIS_DURATION, &cap,
> +			"attempting to determine the durations");
>  	if (rc)
>  		return rc;
>  
> -	if (be32_to_cpu(tpm_cmd.header.out.return_code) != 0 ||
> -	    be32_to_cpu(tpm_cmd.header.out.length)
> -	    != sizeof(tpm_cmd.header.out) + sizeof(u32) + 3 * sizeof(u32))
> -		return -EINVAL;
> -
> -	duration_cap = &tpm_cmd.params.getcap_out.cap.duration;
>  	chip->duration[TPM_SHORT] =
> -	    usecs_to_jiffies(be32_to_cpu(duration_cap->tpm_short));
> +		usecs_to_jiffies(be32_to_cpu(cap.duration.tpm_short));
>  	chip->duration[TPM_MEDIUM] =
> -	    usecs_to_jiffies(be32_to_cpu(duration_cap->tpm_medium));
> +		usecs_to_jiffies(be32_to_cpu(cap.duration.tpm_medium));
>  	chip->duration[TPM_LONG] =
> -	    usecs_to_jiffies(be32_to_cpu(duration_cap->tpm_long));
> +		usecs_to_jiffies(be32_to_cpu(cap.duration.tpm_long));
>  
>  	/* The Broadcom BCM0102 chipset in a Dell Latitude D820 gets the above
>  	 * value wrong and apparently reports msecs rather than usecs. So we
> -- 
> 1.9.1
> 

[toc] | [prev] | [next] | [standalone]

#1445692 — Re: [PATCH v9 3/5] tpm: Clean up reading of timeout and duration capabilities

On Wed, Jul 13, 2016 at 09:19:34AM -0700, Ed Swierk wrote:
> Call tpm_getcap() from tpm_get_timeouts() to eliminate redundant
> code. Return all errors to the caller rather than swallowing them
> (e.g. when tpm_transmit_cmd() returns nonzero).
> 
> Signed-off-by: Ed Swierk <eswierk@skyportsystems.com>

You have to fix the reported kbuild errors.

/Jarkko

> ---
>  drivers/char/tpm/tpm-interface.c | 74 +++++++++++++++-------------------------
>  1 file changed, 27 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index a4beb53..dc492ee 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -460,9 +460,19 @@ ssize_t tpm_getcap(struct tpm_chip *chip, __be32 subcap_id, cap_t *cap,
>  		tpm_cmd.params.getcap_in.subcap_size = cpu_to_be32(4);
>  		tpm_cmd.params.getcap_in.subcap = subcap_id;
>  	}
> +
>  	rc = tpm_transmit_cmd(chip, &tpm_cmd, TPM_INTERNAL_RESULT_SIZE, desc);
> +
> +	if (!rc &&
> +	    ((subcap_id == TPM_CAP_PROP_TIS_TIMEOUT &&
> +	      be32_to_cpu(tpm_cmd.header.out.length) != TPM_HEADER_SIZE + 20) ||
> +	     (subcap_id == TPM_CAP_PROP_TIS_DURATION &&
> +	      be32_to_cpu(tpm_cmd.header.out.length) != TPM_HEADER_SIZE + 16)))
> +		rc = -EINVAL;
> +
>  	if (!rc)
>  		*cap = tpm_cmd.params.getcap_out.cap;
> +
>  	return rc;
>  }
>  
> @@ -503,10 +513,9 @@ static int tpm_startup(struct tpm_chip *chip, __be16 startup_type)
>  
>  int tpm_get_timeouts(struct tpm_chip *chip)
>  {
> -	struct tpm_cmd_t tpm_cmd;
> +	cap_t cap;
>  	unsigned long new_timeout[4];
>  	unsigned long old_timeout[4];
> -	struct duration_t *duration_cap;
>  	ssize_t rc;
>  
>  	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
> @@ -524,42 +533,25 @@ int tpm_get_timeouts(struct tpm_chip *chip)
>  		return 0;
>  	}
>  
> -	tpm_cmd.header.in = tpm_getcap_header;
> -	tpm_cmd.params.getcap_in.cap = TPM_CAP_PROP;
> -	tpm_cmd.params.getcap_in.subcap_size = cpu_to_be32(4);
> -	tpm_cmd.params.getcap_in.subcap = TPM_CAP_PROP_TIS_TIMEOUT;
> -	rc = tpm_transmit_cmd(chip, &tpm_cmd, TPM_INTERNAL_RESULT_SIZE, NULL);
> -
> +	rc = tpm_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, &cap,
> +			"attempting to determine the timeouts");
>  	if (rc == TPM_ERR_INVALID_POSTINIT) {
>  		/* The TPM is not started, we are the first to talk to it.
>  		   Execute a startup command. */
> -		dev_info(&chip->dev, "Issuing TPM_STARTUP");
> +		dev_info(&chip->dev, "Issuing TPM_STARTUP\n");
>  		if (tpm_startup(chip, TPM_ST_CLEAR))
>  			return rc;
>  
> -		tpm_cmd.header.in = tpm_getcap_header;
> -		tpm_cmd.params.getcap_in.cap = TPM_CAP_PROP;
> -		tpm_cmd.params.getcap_in.subcap_size = cpu_to_be32(4);
> -		tpm_cmd.params.getcap_in.subcap = TPM_CAP_PROP_TIS_TIMEOUT;
> -		rc = tpm_transmit_cmd(chip, &tpm_cmd, TPM_INTERNAL_RESULT_SIZE,
> -				  NULL);
> -	}
> -	if (rc) {
> -		dev_err(&chip->dev,
> -			"A TPM error (%zd) occurred attempting to determine the timeouts\n",
> -			rc);
> -		goto duration;
> +		rc = tpm_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, &cap,
> +				"attempting to determine the timeouts");
>  	}
> +	if (rc)
> +		return rc;
>  
> -	if (be32_to_cpu(tpm_cmd.header.out.return_code) != 0 ||
> -	    be32_to_cpu(tpm_cmd.header.out.length)
> -	    != sizeof(tpm_cmd.header.out) + sizeof(u32) + 4 * sizeof(u32))
> -		return -EINVAL;
> -
> -	old_timeout[0] = be32_to_cpu(tpm_cmd.params.getcap_out.cap.timeout.a);
> -	old_timeout[1] = be32_to_cpu(tpm_cmd.params.getcap_out.cap.timeout.b);
> -	old_timeout[2] = be32_to_cpu(tpm_cmd.params.getcap_out.cap.timeout.c);
> -	old_timeout[3] = be32_to_cpu(tpm_cmd.params.getcap_out.cap.timeout.d);
> +	old_timeout[0] = be32_to_cpu(cap.timeout.a);
> +	old_timeout[1] = be32_to_cpu(cap.timeout.b);
> +	old_timeout[2] = be32_to_cpu(cap.timeout.c);
> +	old_timeout[3] = be32_to_cpu(cap.timeout.d);
>  	memcpy(new_timeout, old_timeout, sizeof(new_timeout));
>  
>  	/*
> @@ -597,29 +589,17 @@ int tpm_get_timeouts(struct tpm_chip *chip)
>  	chip->timeout_c = usecs_to_jiffies(new_timeout[2]);
>  	chip->timeout_d = usecs_to_jiffies(new_timeout[3]);
>  
> -duration:
> -	tpm_cmd.header.in = tpm_getcap_header;
> -	tpm_cmd.params.getcap_in.cap = TPM_CAP_PROP;
> -	tpm_cmd.params.getcap_in.subcap_size = cpu_to_be32(4);
> -	tpm_cmd.params.getcap_in.subcap = TPM_CAP_PROP_TIS_DURATION;
> -
> -	rc = tpm_transmit_cmd(chip, &tpm_cmd, TPM_INTERNAL_RESULT_SIZE,
> -			      "attempting to determine the durations");
> +	rc = tpm_getcap(chip, TPM_CAP_PROP_TIS_DURATION, &cap,
> +			"attempting to determine the durations");
>  	if (rc)
>  		return rc;
>  
> -	if (be32_to_cpu(tpm_cmd.header.out.return_code) != 0 ||
> -	    be32_to_cpu(tpm_cmd.header.out.length)
> -	    != sizeof(tpm_cmd.header.out) + sizeof(u32) + 3 * sizeof(u32))
> -		return -EINVAL;
> -
> -	duration_cap = &tpm_cmd.params.getcap_out.cap.duration;
>  	chip->duration[TPM_SHORT] =
> -	    usecs_to_jiffies(be32_to_cpu(duration_cap->tpm_short));
> +		usecs_to_jiffies(be32_to_cpu(cap.duration.tpm_short));
>  	chip->duration[TPM_MEDIUM] =
> -	    usecs_to_jiffies(be32_to_cpu(duration_cap->tpm_medium));
> +		usecs_to_jiffies(be32_to_cpu(cap.duration.tpm_medium));
>  	chip->duration[TPM_LONG] =
> -	    usecs_to_jiffies(be32_to_cpu(duration_cap->tpm_long));
> +		usecs_to_jiffies(be32_to_cpu(cap.duration.tpm_long));
>  
>  	/* The Broadcom BCM0102 chipset in a Dell Latitude D820 gets the above
>  	 * value wrong and apparently reports msecs rather than usecs. So we
> -- 
> 1.9.1
> 

[toc] | [prev] | [next] | [standalone]

#1445694 — Re: [PATCH v9 3/5] tpm: Clean up reading of timeout and duration capabilities

On Mon, Jul 18, 2016 at 09:19:53PM +0300, Jarkko Sakkinen wrote:
> On Wed, Jul 13, 2016 at 09:19:34AM -0700, Ed Swierk wrote:
> > Call tpm_getcap() from tpm_get_timeouts() to eliminate redundant
> > code. Return all errors to the caller rather than swallowing them
> > (e.g. when tpm_transmit_cmd() returns nonzero).
> > 
> > Signed-off-by: Ed Swierk <eswierk@skyportsystems.com>
> 
> You have to fix the reported kbuild errors.

Please ignore this :) Pressed send button by mistake in mutt.

/Jarkko

[toc] | [prev] | [next] | [standalone]

#1442617 — [PATCH v9 1/5] tpm_tis: Improve reporting of IO errors

Mysterious TPM behavior can be difficult to track down through all the
layers of software. Add error messages for conditions that should
never happen. Also include the manufacturer ID along with other chip
data printed during init.

Signed-off-by: Ed Swierk <eswierk@skyportsystems.com>
Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 drivers/char/tpm/tpm_tis_core.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 8110b52..e62fdeb 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -217,6 +217,8 @@ static int tpm_tis_recv(struct tpm_chip *chip, u8 *buf, size_t count)
 
 	expected = be32_to_cpu(*(__be32 *) (buf + 2));
 	if (expected > count) {
+		dev_err(&chip->dev, "Response too long (wanted %zd, got %d)\n",
+			count, expected);
 		size = -EIO;
 		goto out;
 	}
@@ -283,6 +285,8 @@ static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t len)
 				  &priv->int_queue, false);
 		status = tpm_tis_status(chip);
 		if (!itpm && (status & TPM_STS_DATA_EXPECT) == 0) {
+			dev_err(&chip->dev, "Chip not accepting %zd bytes\n",
+				len - count);
 			rc = -EIO;
 			goto out_err;
 		}
@@ -297,6 +301,7 @@ static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t len)
 			  &priv->int_queue, false);
 	status = tpm_tis_status(chip);
 	if (!itpm && (status & TPM_STS_DATA_EXPECT) != 0) {
+		dev_err(&chip->dev, "Chip not accepting last byte\n");
 		rc = -EIO;
 		goto out_err;
 	}
@@ -707,8 +712,9 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
 	if (rc < 0)
 		goto out_err;
 
-	dev_info(dev, "%s TPM (device-id 0x%X, rev-id %d)\n",
+	dev_info(dev, "%s TPM (manufacturer-id 0x%X, device-id 0x%X, rev-id %d)\n",
 		 (chip->flags & TPM_CHIP_FLAG_TPM2) ? "2.0" : "1.2",
+		 priv->manufacturer_id,
 		 vendor >> 16, rid);
 
 	if (!(priv->flags & TPM_TIS_ITPM_POSSIBLE)) {
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1442618 — [PATCH v9 2/5] tpm: Add optional logging of TPM command durations

Some TPMs violate their own advertised command durations. This is much
easier to debug with data about how long each command actually takes
to complete. Add debug messages that can be enabled by running

  echo -n 'module tpm +p' >/sys/kernel/debug/dynamic_debug/control

on a kernel configured with DYNAMIC_DEBUG=y.

Signed-off-by: Ed Swierk <eswierk@skyportsystems.com>
Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 drivers/char/tpm/tpm-interface.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 5e3c1b6..a4beb53 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -335,13 +335,14 @@ ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,
 {
 	ssize_t rc;
 	u32 count, ordinal;
-	unsigned long stop;
+	unsigned long start, stop;
 
 	if (bufsiz > TPM_BUFSIZE)
 		bufsiz = TPM_BUFSIZE;
 
 	count = be32_to_cpu(*((__be32 *) (buf + 2)));
 	ordinal = be32_to_cpu(*((__be32 *) (buf + 6)));
+	dev_dbg(&chip->dev, "starting command %d count %d\n", ordinal, count);
 	if (count == 0)
 		return -ENODATA;
 	if (count > bufsiz) {
@@ -362,18 +363,23 @@ ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,
 	if (chip->flags & TPM_CHIP_FLAG_IRQ)
 		goto out_recv;
 
+	start = jiffies;
 	if (chip->flags & TPM_CHIP_FLAG_TPM2)
-		stop = jiffies + tpm2_calc_ordinal_duration(chip, ordinal);
+		stop = start + tpm2_calc_ordinal_duration(chip, ordinal);
 	else
-		stop = jiffies + tpm_calc_ordinal_duration(chip, ordinal);
+		stop = start + tpm_calc_ordinal_duration(chip, ordinal);
 	do {
 		u8 status = chip->ops->status(chip);
 		if ((status & chip->ops->req_complete_mask) ==
-		    chip->ops->req_complete_val)
+		    chip->ops->req_complete_val) {
+			dev_dbg(&chip->dev, "completed command %d in %d ms\n",
+				ordinal, jiffies_to_msecs(jiffies - start));
 			goto out_recv;
+		}
 
 		if (chip->ops->req_canceled(chip, status)) {
-			dev_err(&chip->dev, "Operation Canceled\n");
+			dev_err(&chip->dev, "canceled command %d after %d ms\n",
+				ordinal, jiffies_to_msecs(jiffies - start));
 			rc = -ECANCELED;
 			goto out;
 		}
@@ -383,7 +389,8 @@ ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,
 	} while (time_before(jiffies, stop));
 
 	chip->ops->cancel(chip);
-	dev_err(&chip->dev, "Operation Timed out\n");
+	dev_err(&chip->dev, "command %d timed out after %d ms\n", ordinal,
+		jiffies_to_msecs(jiffies - start));
 	rc = -ETIME;
 	goto out;
 
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1442620 — [PATCH v9 4/5] tpm: Allow TPM chip drivers to override reported command durations

Some TPM chips report bogus command durations in their capabilities,
just as others report incorrect timeouts. Rework tpm_get_timeouts() to
allow chip drivers to override either via a single callback. Also
clean up handling of TPMs that report milliseconds instead of
microseconds.

Signed-off-by: Ed Swierk <eswierk@skyportsystems.com>
---
 drivers/char/tpm/tpm-interface.c | 148 ++++++++++++++++++++++-----------------
 drivers/char/tpm/tpm_tis_core.c  |  32 +++------
 include/linux/tpm.h              |   3 +-
 3 files changed, 94 insertions(+), 89 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index dc492ee..9dafc25 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -513,10 +513,10 @@ static int tpm_startup(struct tpm_chip *chip, __be16 startup_type)
 
 int tpm_get_timeouts(struct tpm_chip *chip)
 {
-	cap_t cap;
-	unsigned long new_timeout[4];
-	unsigned long old_timeout[4];
-	ssize_t rc;
+	cap_t cap1, cap2;
+	int rc;
+	unsigned long orig_timeout_a, orig_timeout_b, orig_timeout_c,
+		orig_timeout_d, orig_duration[3];
 
 	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
 		/* Fixed timeouts for TPM2 */
@@ -533,7 +533,7 @@ int tpm_get_timeouts(struct tpm_chip *chip)
 		return 0;
 	}
 
-	rc = tpm_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, &cap,
+	rc = tpm_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, &cap1,
 			"attempting to determine the timeouts");
 	if (rc == TPM_ERR_INVALID_POSTINIT) {
 		/* The TPM is not started, we are the first to talk to it.
@@ -542,77 +542,95 @@ int tpm_get_timeouts(struct tpm_chip *chip)
 		if (tpm_startup(chip, TPM_ST_CLEAR))
 			return rc;
 
-		rc = tpm_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, &cap,
+		rc = tpm_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, &cap1,
 				"attempting to determine the timeouts");
 	}
 	if (rc)
 		return rc;
 
-	old_timeout[0] = be32_to_cpu(cap.timeout.a);
-	old_timeout[1] = be32_to_cpu(cap.timeout.b);
-	old_timeout[2] = be32_to_cpu(cap.timeout.c);
-	old_timeout[3] = be32_to_cpu(cap.timeout.d);
-	memcpy(new_timeout, old_timeout, sizeof(new_timeout));
-
-	/*
-	 * Provide ability for vendor overrides of timeout values in case
-	 * of misreporting.
-	 */
-	if (chip->ops->update_timeouts != NULL)
-		chip->timeout_adjusted =
-			chip->ops->update_timeouts(chip, new_timeout);
-
-	if (!chip->timeout_adjusted) {
-		/* Don't overwrite default if value is 0 */
-		if (new_timeout[0] != 0 && new_timeout[0] < 1000) {
-			int i;
-
-			/* timeouts in msec rather usec */
-			for (i = 0; i != ARRAY_SIZE(new_timeout); i++)
-				new_timeout[i] *= 1000;
-			chip->timeout_adjusted = true;
-		}
-	}
-
-	/* Report adjusted timeouts */
-	if (chip->timeout_adjusted) {
-		dev_info(&chip->dev,
-			 HW_ERR "Adjusting reported timeouts: A %lu->%luus B %lu->%luus C %lu->%luus D %lu->%luus\n",
-			 old_timeout[0], new_timeout[0],
-			 old_timeout[1], new_timeout[1],
-			 old_timeout[2], new_timeout[2],
-			 old_timeout[3], new_timeout[3]);
-	}
-
-	chip->timeout_a = usecs_to_jiffies(new_timeout[0]);
-	chip->timeout_b = usecs_to_jiffies(new_timeout[1]);
-	chip->timeout_c = usecs_to_jiffies(new_timeout[2]);
-	chip->timeout_d = usecs_to_jiffies(new_timeout[3]);
-
-	rc = tpm_getcap(chip, TPM_CAP_PROP_TIS_DURATION, &cap,
+	rc = tpm_getcap(chip, TPM_CAP_PROP_TIS_DURATION, &cap2,
 			"attempting to determine the durations");
 	if (rc)
 		return rc;
 
-	chip->duration[TPM_SHORT] =
-		usecs_to_jiffies(be32_to_cpu(cap.duration.tpm_short));
-	chip->duration[TPM_MEDIUM] =
-		usecs_to_jiffies(be32_to_cpu(cap.duration.tpm_medium));
-	chip->duration[TPM_LONG] =
-		usecs_to_jiffies(be32_to_cpu(cap.duration.tpm_long));
+	be32_to_cpus(&cap1.timeout.a);
+	be32_to_cpus(&cap1.timeout.b);
+	be32_to_cpus(&cap1.timeout.c);
+	be32_to_cpus(&cap1.timeout.d);
+	chip->timeout_a = usecs_to_jiffies(cap1.timeout.a);
+	chip->timeout_b = usecs_to_jiffies(cap1.timeout.b);
+	chip->timeout_c = usecs_to_jiffies(cap1.timeout.c);
+	chip->timeout_d = usecs_to_jiffies(cap1.timeout.d);
 
-	/* The Broadcom BCM0102 chipset in a Dell Latitude D820 gets the above
-	 * value wrong and apparently reports msecs rather than usecs. So we
-	 * fix up the resulting too-small TPM_SHORT value to make things work.
-	 * We also scale the TPM_MEDIUM and -_LONG values by 1000.
-	 */
-	if (chip->duration[TPM_SHORT] < (HZ / 100)) {
-		chip->duration[TPM_SHORT] = HZ;
-		chip->duration[TPM_MEDIUM] *= 1000;
-		chip->duration[TPM_LONG] *= 1000;
-		chip->duration_adjusted = true;
-		dev_info(&chip->dev, "Adjusting TPM timeout parameters.");
+	/* Some TPMs report timeouts in milliseconds rather than
+	   microseconds. Use a value between 1 and 1000 as an
+	   indication that this is the case. */
+	if (cap1.timeout.a > 0 && cap1.timeout.a < 1000) {
+		chip->timeout_a = msecs_to_jiffies(cap1.timeout.a);
+		chip->timeout_b = msecs_to_jiffies(cap1.timeout.b);
+		chip->timeout_c = msecs_to_jiffies(cap1.timeout.c);
+		chip->timeout_d = msecs_to_jiffies(cap1.timeout.d);
+		chip->timeout_adjusted = true;
 	}
+
+	be32_to_cpus(&cap2.duration.tpm_short);
+	be32_to_cpus(&cap2.duration.tpm_medium);
+	be32_to_cpus(&cap2.duration.tpm_long);
+	chip->duration[TPM_SHORT] =
+		usecs_to_jiffies(cap2.duration.tpm_short);
+	chip->duration[TPM_MEDIUM] =
+		usecs_to_jiffies(cap2.duration.tpm_medium);
+	chip->duration[TPM_LONG] =
+		usecs_to_jiffies(cap2.duration.tpm_long);
+
+	orig_timeout_a = chip->timeout_a;
+	orig_timeout_b = chip->timeout_b;
+	orig_timeout_c = chip->timeout_c;
+	orig_timeout_d = chip->timeout_d;
+	memcpy(orig_duration, chip->duration, 3 * sizeof(unsigned long));
+
+	/* Interpret duration values between 1 and 10000 as
+	   milliseconds to deal with TPMs like the Broadcom BCM0102 in
+	   the Dell Latitude D820. */
+	if (cap2.duration.tpm_short > 0 && cap2.duration.tpm_short < 10000) {
+		chip->duration[TPM_SHORT] =
+			msecs_to_jiffies(cap2.duration.tpm_short);
+		chip->duration[TPM_MEDIUM] =
+			msecs_to_jiffies(cap2.duration.tpm_medium);
+		chip->duration[TPM_LONG] =
+			msecs_to_jiffies(cap2.duration.tpm_long);
+		chip->duration_adjusted = true;
+	}
+
+	if (chip->ops->update_timeouts != NULL)
+		chip->ops->update_timeouts(chip);
+
+	if (chip->timeout_adjusted) {
+		dev_info(&chip->dev,
+			 HW_ERR "Adjusted timeouts: A %u->%uus B %u->%uus"
+			 " C %u->%uus D %u->%uus\n",
+			 jiffies_to_usecs(orig_timeout_a),
+			 jiffies_to_usecs(chip->timeout_a),
+			 jiffies_to_usecs(orig_timeout_b),
+			 jiffies_to_usecs(chip->timeout_b),
+			 jiffies_to_usecs(orig_timeout_c),
+			 jiffies_to_usecs(chip->timeout_c),
+			 jiffies_to_usecs(orig_timeout_d),
+			 jiffies_to_usecs(chip->timeout_d));
+	}
+
+	if (chip->duration_adjusted) {
+		dev_info(&chip->dev,
+			 HW_ERR "Adjusted durations: short %u->%uus"
+			 " medium %u->%uus long %u->%uus\n",
+			 jiffies_to_usecs(orig_duration[TPM_SHORT]),
+			 jiffies_to_usecs(chip->duration[TPM_SHORT]),
+			 jiffies_to_usecs(orig_duration[TPM_MEDIUM]),
+			 jiffies_to_usecs(chip->duration[TPM_MEDIUM]),
+			 jiffies_to_usecs(orig_duration[TPM_LONG]),
+			 jiffies_to_usecs(chip->duration[TPM_LONG]));
+	}
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(tpm_get_timeouts);
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index e62fdeb..f013664 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -398,37 +398,25 @@ static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
 	return rc;
 }
 
-struct tis_vendor_timeout_override {
-	u32 did_vid;
-	unsigned long timeout_us[4];
-};
-
-static const struct tis_vendor_timeout_override vendor_timeout_overrides[] = {
-	/* Atmel 3204 */
-	{ 0x32041114, { (TIS_SHORT_TIMEOUT*1000), (TIS_LONG_TIMEOUT*1000),
-			(TIS_SHORT_TIMEOUT*1000), (TIS_SHORT_TIMEOUT*1000) } },
-};
-
-static bool tpm_tis_update_timeouts(struct tpm_chip *chip,
-				    unsigned long *timeout_cap)
+static void tpm_tis_update_timeouts(struct tpm_chip *chip)
 {
 	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
-	int i, rc;
+	int rc;
 	u32 did_vid;
 
 	rc = tpm_tis_read32(priv, TPM_DID_VID(0), &did_vid);
 	if (rc < 0)
 		return rc;
 
-	for (i = 0; i != ARRAY_SIZE(vendor_timeout_overrides); i++) {
-		if (vendor_timeout_overrides[i].did_vid != did_vid)
-			continue;
-		memcpy(timeout_cap, vendor_timeout_overrides[i].timeout_us,
-		       sizeof(vendor_timeout_overrides[i].timeout_us));
-		return true;
+	switch (did_vid) {
+	case 0x32041114: /* Atmel 3204 */
+		chip->timeout_a = msecs_to_jiffies(TIS_SHORT_TIMEOUT);
+		chip->timeout_b = msecs_to_jiffies(TIS_LONG_TIMEOUT);
+		chip->timeout_c = msecs_to_jiffies(TIS_SHORT_TIMEOUT);
+		chip->timeout_d = msecs_to_jiffies(TIS_SHORT_TIMEOUT);
+		chip->timeout_adjusted = true;
+		break;
 	}
-
-	return false;
 }
 
 /*
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index 706e63e..2380ebf 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -41,8 +41,7 @@ struct tpm_class_ops {
 	int (*send) (struct tpm_chip *chip, u8 *buf, size_t len);
 	void (*cancel) (struct tpm_chip *chip);
 	u8 (*status) (struct tpm_chip *chip);
-	bool (*update_timeouts)(struct tpm_chip *chip,
-				unsigned long *timeout_cap);
+	void (*update_timeouts)(struct tpm_chip *chip);
 
 };
 
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1442638 — Re: [PATCH v9 4/5] tpm: Allow TPM chip drivers to override reported command durations

[Multipart message — attachments visible in raw view] — view raw

Hi,

[auto build test WARNING on next-20160712]
[cannot apply to char-misc/char-misc-testing v4.7-rc7 v4.7-rc6 v4.7-rc5 v4.7-rc7]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Ed-Swierk/tpm-Command-duration-logging-and-chip-specific-override/20160714-002547
config: i386-randconfig-s1-201628 (attached as .config)
compiler: gcc-6 (Debian 6.1.1-1) 6.1.1 20160430
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All warnings (new ones prefixed by >>):

   drivers/char/tpm/tpm_tis_core.c: In function 'tpm_tis_update_timeouts':
>> drivers/char/tpm/tpm_tis_core.c:414:10: warning: 'return' with a value, in function returning void
      return rc;
             ^~
   drivers/char/tpm/tpm_tis_core.c:406:13: note: declared here
    static void tpm_tis_update_timeouts(struct tpm_chip *chip)
                ^~~~~~~~~~~~~~~~~~~~~~~

vim +/return +414 drivers/char/tpm/tpm_tis_core.c

41a5e1cf Christophe Ricard 2016-05-19  398  	if (!priv->irq_tested)
41a5e1cf Christophe Ricard 2016-05-19  399  		msleep(1);
41a5e1cf Christophe Ricard 2016-05-19  400  	if (!priv->irq_tested)
41a5e1cf Christophe Ricard 2016-05-19  401  		disable_interrupts(chip);
41a5e1cf Christophe Ricard 2016-05-19  402  	priv->irq_tested = true;
41a5e1cf Christophe Ricard 2016-05-19  403  	return rc;
41a5e1cf Christophe Ricard 2016-05-19  404  }
41a5e1cf Christophe Ricard 2016-05-19  405  
a7da7fe7 Ed Swierk         2016-07-13  406  static void tpm_tis_update_timeouts(struct tpm_chip *chip)
41a5e1cf Christophe Ricard 2016-05-19  407  {
41a5e1cf Christophe Ricard 2016-05-19  408  	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
a7da7fe7 Ed Swierk         2016-07-13  409  	int rc;
41a5e1cf Christophe Ricard 2016-05-19  410  	u32 did_vid;
41a5e1cf Christophe Ricard 2016-05-19  411  
41a5e1cf Christophe Ricard 2016-05-19  412  	rc = tpm_tis_read32(priv, TPM_DID_VID(0), &did_vid);
41a5e1cf Christophe Ricard 2016-05-19  413  	if (rc < 0)
41a5e1cf Christophe Ricard 2016-05-19 @414  		return rc;
41a5e1cf Christophe Ricard 2016-05-19  415  
a7da7fe7 Ed Swierk         2016-07-13  416  	switch (did_vid) {
a7da7fe7 Ed Swierk         2016-07-13  417  	case 0x32041114: /* Atmel 3204 */
a7da7fe7 Ed Swierk         2016-07-13  418  		chip->timeout_a = msecs_to_jiffies(TIS_SHORT_TIMEOUT);
a7da7fe7 Ed Swierk         2016-07-13  419  		chip->timeout_b = msecs_to_jiffies(TIS_LONG_TIMEOUT);
a7da7fe7 Ed Swierk         2016-07-13  420  		chip->timeout_c = msecs_to_jiffies(TIS_SHORT_TIMEOUT);
a7da7fe7 Ed Swierk         2016-07-13  421  		chip->timeout_d = msecs_to_jiffies(TIS_SHORT_TIMEOUT);
a7da7fe7 Ed Swierk         2016-07-13  422  		chip->timeout_adjusted = true;

:::::: The code at line 414 was first introduced by commit
:::::: 41a5e1cf1fe151ed48b4b3106c748d03a85133ce tpm/tpm_tis: Split tpm_tis driver into a core and TCG TIS compliant phy

:::::: TO: Christophe Ricard <christophe.ricard@gmail.com>
:::::: CC: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[toc] | [prev] | [next] | [standalone]

#1445707 — Re: [PATCH v9 4/5] tpm: Allow TPM chip drivers to override reported command durations

On Wed, Jul 13, 2016 at 09:19:35AM -0700, Ed Swierk wrote:
> Some TPM chips report bogus command durations in their capabilities,
> just as others report incorrect timeouts. Rework tpm_get_timeouts() to
> allow chip drivers to override either via a single callback. Also
> clean up handling of TPMs that report milliseconds instead of
> microseconds.

I don't really undestand why you need to turn things so much over.
There's too much noise in this commit to reasonably evaluate it.

> Signed-off-by: Ed Swierk <eswierk@skyportsystems.com>
> ---
>  drivers/char/tpm/tpm-interface.c | 148 ++++++++++++++++++++++-----------------
>  drivers/char/tpm/tpm_tis_core.c  |  32 +++------
>  include/linux/tpm.h              |   3 +-
>  3 files changed, 94 insertions(+), 89 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index dc492ee..9dafc25 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -513,10 +513,10 @@ static int tpm_startup(struct tpm_chip *chip, __be16 startup_type)
>  
>  int tpm_get_timeouts(struct tpm_chip *chip)
>  {
> -	cap_t cap;

cap_t cap2;

> -	unsigned long new_timeout[4];
> -	unsigned long old_timeout[4];
> -	ssize_t rc;
> +	cap_t cap1, cap2;

Do not use cap1.

> +	int rc;
> +	unsigned long orig_timeout_a, orig_timeout_b, orig_timeout_c,
> +		orig_timeout_d, orig_duration[3];

Use old_timeout array just to reduce the diff.

>  
>  	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
>  		/* Fixed timeouts for TPM2 */
> @@ -533,7 +533,7 @@ int tpm_get_timeouts(struct tpm_chip *chip)
>  		return 0;
>  	}
>  
> -	rc = tpm_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, &cap,
> +	rc = tpm_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, &cap1,
>  			"attempting to determine the timeouts");
>  	if (rc == TPM_ERR_INVALID_POSTINIT) {
>  		/* The TPM is not started, we are the first to talk to it.
> @@ -542,77 +542,95 @@ int tpm_get_timeouts(struct tpm_chip *chip)
>  		if (tpm_startup(chip, TPM_ST_CLEAR))
>  			return rc;
>  
> -		rc = tpm_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, &cap,
> +		rc = tpm_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, &cap1,
>  				"attempting to determine the timeouts");
>  	}
>  	if (rc)
>  		return rc;
>  
> -	old_timeout[0] = be32_to_cpu(cap.timeout.a);
> -	old_timeout[1] = be32_to_cpu(cap.timeout.b);
> -	old_timeout[2] = be32_to_cpu(cap.timeout.c);
> -	old_timeout[3] = be32_to_cpu(cap.timeout.d);
> -	memcpy(new_timeout, old_timeout, sizeof(new_timeout));
> -
> -	/*
> -	 * Provide ability for vendor overrides of timeout values in case
> -	 * of misreporting.
> -	 */
> -	if (chip->ops->update_timeouts != NULL)
> -		chip->timeout_adjusted =
> -			chip->ops->update_timeouts(chip, new_timeout);
> -
> -	if (!chip->timeout_adjusted) {
> -		/* Don't overwrite default if value is 0 */
> -		if (new_timeout[0] != 0 && new_timeout[0] < 1000) {
> -			int i;
> -
> -			/* timeouts in msec rather usec */
> -			for (i = 0; i != ARRAY_SIZE(new_timeout); i++)
> -				new_timeout[i] *= 1000;
> -			chip->timeout_adjusted = true;
> -		}
> -	}
> -
> -	/* Report adjusted timeouts */
> -	if (chip->timeout_adjusted) {
> -		dev_info(&chip->dev,
> -			 HW_ERR "Adjusting reported timeouts: A %lu->%luus B %lu->%luus C %lu->%luus D %lu->%luus\n",
> -			 old_timeout[0], new_timeout[0],
> -			 old_timeout[1], new_timeout[1],
> -			 old_timeout[2], new_timeout[2],
> -			 old_timeout[3], new_timeout[3]);
> -	}
> -
> -	chip->timeout_a = usecs_to_jiffies(new_timeout[0]);
> -	chip->timeout_b = usecs_to_jiffies(new_timeout[1]);
> -	chip->timeout_c = usecs_to_jiffies(new_timeout[2]);
> -	chip->timeout_d = usecs_to_jiffies(new_timeout[3]);
> -
> -	rc = tpm_getcap(chip, TPM_CAP_PROP_TIS_DURATION, &cap,
> +	rc = tpm_getcap(chip, TPM_CAP_PROP_TIS_DURATION, &cap2,
>  			"attempting to determine the durations");
>  	if (rc)
>  		return rc;
>  
> -	chip->duration[TPM_SHORT] =
> -		usecs_to_jiffies(be32_to_cpu(cap.duration.tpm_short));
> -	chip->duration[TPM_MEDIUM] =
> -		usecs_to_jiffies(be32_to_cpu(cap.duration.tpm_medium));
> -	chip->duration[TPM_LONG] =
> -		usecs_to_jiffies(be32_to_cpu(cap.duration.tpm_long));
> +	be32_to_cpus(&cap1.timeout.a);
> +	be32_to_cpus(&cap1.timeout.b);
> +	be32_to_cpus(&cap1.timeout.c);
> +	be32_to_cpus(&cap1.timeout.d);
> +	chip->timeout_a = usecs_to_jiffies(cap1.timeout.a);
> +	chip->timeout_b = usecs_to_jiffies(cap1.timeout.b);
> +	chip->timeout_c = usecs_to_jiffies(cap1.timeout.c);
> +	chip->timeout_d = usecs_to_jiffies(cap1.timeout.d);
>  
> -	/* The Broadcom BCM0102 chipset in a Dell Latitude D820 gets the above
> -	 * value wrong and apparently reports msecs rather than usecs. So we
> -	 * fix up the resulting too-small TPM_SHORT value to make things work.
> -	 * We also scale the TPM_MEDIUM and -_LONG values by 1000.
> -	 */
> -	if (chip->duration[TPM_SHORT] < (HZ / 100)) {
> -		chip->duration[TPM_SHORT] = HZ;
> -		chip->duration[TPM_MEDIUM] *= 1000;
> -		chip->duration[TPM_LONG] *= 1000;
> -		chip->duration_adjusted = true;
> -		dev_info(&chip->dev, "Adjusting TPM timeout parameters.");
> +	/* Some TPMs report timeouts in milliseconds rather than
> +	   microseconds. Use a value between 1 and 1000 as an
> +	   indication that this is the case. */
> +	if (cap1.timeout.a > 0 && cap1.timeout.a < 1000) {
> +		chip->timeout_a = msecs_to_jiffies(cap1.timeout.a);
> +		chip->timeout_b = msecs_to_jiffies(cap1.timeout.b);
> +		chip->timeout_c = msecs_to_jiffies(cap1.timeout.c);
> +		chip->timeout_d = msecs_to_jiffies(cap1.timeout.d);
> +		chip->timeout_adjusted = true;
>  	}
> +
> +	be32_to_cpus(&cap2.duration.tpm_short);
> +	be32_to_cpus(&cap2.duration.tpm_medium);
> +	be32_to_cpus(&cap2.duration.tpm_long);
> +	chip->duration[TPM_SHORT] =
> +		usecs_to_jiffies(cap2.duration.tpm_short);
> +	chip->duration[TPM_MEDIUM] =
> +		usecs_to_jiffies(cap2.duration.tpm_medium);
> +	chip->duration[TPM_LONG] =
> +		usecs_to_jiffies(cap2.duration.tpm_long);
> +
> +	orig_timeout_a = chip->timeout_a;
> +	orig_timeout_b = chip->timeout_b;
> +	orig_timeout_c = chip->timeout_c;
> +	orig_timeout_d = chip->timeout_d;
> +	memcpy(orig_duration, chip->duration, 3 * sizeof(unsigned long));
> +
> +	/* Interpret duration values between 1 and 10000 as
> +	   milliseconds to deal with TPMs like the Broadcom BCM0102 in
> +	   the Dell Latitude D820. */
> +	if (cap2.duration.tpm_short > 0 && cap2.duration.tpm_short < 10000) {
> +		chip->duration[TPM_SHORT] =
> +			msecs_to_jiffies(cap2.duration.tpm_short);
> +		chip->duration[TPM_MEDIUM] =
> +			msecs_to_jiffies(cap2.duration.tpm_medium);
> +		chip->duration[TPM_LONG] =
> +			msecs_to_jiffies(cap2.duration.tpm_long);
> +		chip->duration_adjusted = true;
> +	}
> +
> +	if (chip->ops->update_timeouts != NULL)
> +		chip->ops->update_timeouts(chip);
> +
> +	if (chip->timeout_adjusted) {
> +		dev_info(&chip->dev,
> +			 HW_ERR "Adjusted timeouts: A %u->%uus B %u->%uus"
> +			 " C %u->%uus D %u->%uus\n",
> +			 jiffies_to_usecs(orig_timeout_a),
> +			 jiffies_to_usecs(chip->timeout_a),
> +			 jiffies_to_usecs(orig_timeout_b),
> +			 jiffies_to_usecs(chip->timeout_b),
> +			 jiffies_to_usecs(orig_timeout_c),
> +			 jiffies_to_usecs(chip->timeout_c),
> +			 jiffies_to_usecs(orig_timeout_d),
> +			 jiffies_to_usecs(chip->timeout_d));
> +	}
> +
> +	if (chip->duration_adjusted) {
> +		dev_info(&chip->dev,
> +			 HW_ERR "Adjusted durations: short %u->%uus"
> +			 " medium %u->%uus long %u->%uus\n",
> +			 jiffies_to_usecs(orig_duration[TPM_SHORT]),
> +			 jiffies_to_usecs(chip->duration[TPM_SHORT]),
> +			 jiffies_to_usecs(orig_duration[TPM_MEDIUM]),
> +			 jiffies_to_usecs(chip->duration[TPM_MEDIUM]),
> +			 jiffies_to_usecs(orig_duration[TPM_LONG]),
> +			 jiffies_to_usecs(chip->duration[TPM_LONG]));
> +	}
> +
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(tpm_get_timeouts);
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index e62fdeb..f013664 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -398,37 +398,25 @@ static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
>  	return rc;
>  }
>  
> -struct tis_vendor_timeout_override {
> -	u32 did_vid;
> -	unsigned long timeout_us[4];
> -};
> -
> -static const struct tis_vendor_timeout_override vendor_timeout_overrides[] = {
> -	/* Atmel 3204 */
> -	{ 0x32041114, { (TIS_SHORT_TIMEOUT*1000), (TIS_LONG_TIMEOUT*1000),
> -			(TIS_SHORT_TIMEOUT*1000), (TIS_SHORT_TIMEOUT*1000) } },
> -};
> -
> -static bool tpm_tis_update_timeouts(struct tpm_chip *chip,
> -				    unsigned long *timeout_cap)
> +static void tpm_tis_update_timeouts(struct tpm_chip *chip)
>  {
>  	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
> -	int i, rc;
> +	int rc;
>  	u32 did_vid;
>  
>  	rc = tpm_tis_read32(priv, TPM_DID_VID(0), &did_vid);
>  	if (rc < 0)
>  		return rc;
>  
> -	for (i = 0; i != ARRAY_SIZE(vendor_timeout_overrides); i++) {
> -		if (vendor_timeout_overrides[i].did_vid != did_vid)
> -			continue;
> -		memcpy(timeout_cap, vendor_timeout_overrides[i].timeout_us,
> -		       sizeof(vendor_timeout_overrides[i].timeout_us));
> -		return true;
> +	switch (did_vid) {
> +	case 0x32041114: /* Atmel 3204 */
> +		chip->timeout_a = msecs_to_jiffies(TIS_SHORT_TIMEOUT);
> +		chip->timeout_b = msecs_to_jiffies(TIS_LONG_TIMEOUT);
> +		chip->timeout_c = msecs_to_jiffies(TIS_SHORT_TIMEOUT);
> +		chip->timeout_d = msecs_to_jiffies(TIS_SHORT_TIMEOUT);
> +		chip->timeout_adjusted = true;
> +		break;

Is this needed if we have the special case for milliseconds in
tpm_get_timeouts?

>  	}
> -
> -	return false;
>  }
>  
>  /*
> diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> index 706e63e..2380ebf 100644
> --- a/include/linux/tpm.h
> +++ b/include/linux/tpm.h
> @@ -41,8 +41,7 @@ struct tpm_class_ops {
>  	int (*send) (struct tpm_chip *chip, u8 *buf, size_t len);
>  	void (*cancel) (struct tpm_chip *chip);
>  	u8 (*status) (struct tpm_chip *chip);
> -	bool (*update_timeouts)(struct tpm_chip *chip,
> -				unsigned long *timeout_cap);
> +	void (*update_timeouts)(struct tpm_chip *chip);
>  
>  };
>  
> -- 
> 1.9.1
> 

/Jarkko

[toc] | [prev] | [next] | [standalone]

#1442626

On Wed, Jul 13, 2016 at 9:19 AM, Ed Swierk <eswierk@skyportsystems.com> wrote:
> v9: Include command duration in existing error messages rather than
> logging an extra debug message. Rebase onto Jarkko's tree.

Incidentally, with Jarkko's tree the tpm_tis module refuses to
initialize (with or without force=1):

tpm_tis 00:03: can't request region for resource [mem 0xfed40000-0xfed44fff]
tpm_tis: probe of 00:03 failed with error -16

The memory region is not marked reserved by the BIOS:

e820: BIOS-provided physical RAM map:
Xen: [mem 0x0000000000000000-0x000000000005ffff] usable
Xen: [mem 0x0000000000060000-0x0000000000067fff] reserved
Xen: [mem 0x0000000000068000-0x000000000009afff] usable
Xen: [mem 0x00000000000a0000-0x00000000000fffff] reserved
Xen: [mem 0x0000000000100000-0x0000000075b01fff] usable
Xen: [mem 0x0000000075baf000-0x000000007a1f5fff] usable
Xen: [mem 0x000000007b7d7000-0x000000007b7fffff] usable
Xen: [mem 0x000000007bf00000-0x000000007bffffff] usable
Xen: [mem 0x00000000c7ffc000-0x00000000c7ffcfff] reserved
Xen: [mem 0x00000000fbffc000-0x00000000fbffcfff] reserved
Xen: [mem 0x00000000fec00000-0x00000000fec01fff] reserved
Xen: [mem 0x00000000fec40000-0x00000000fec40fff] reserved
Xen: [mem 0x00000000fed20000-0x00000000fed2ffff] usable
Xen: [mem 0x00000000fee00000-0x00000000feefffff] reserved
Xen: [mem 0x0000000100000000-0x0000000505deafff] usable

With force=1, /proc/iomem shows:

fed30000-fedfffff : RAM buffer
  fed40000-fed44fff : tpm_tis

I can work around this problem by passing memmap=0x5000$0xfed40000 on
the kernel command line.

--Ed

[toc] | [prev] | [next] | [standalone]

#1442690 — Re: [PATCH v9 0/5] tpm: Command duration logging and chip-specific override

On Wed, Jul 13, 2016 at 09:44:05AM -0700, Ed Swierk wrote:
> On Wed, Jul 13, 2016 at 9:19 AM, Ed Swierk <eswierk@skyportsystems.com> wrote:
> > v9: Include command duration in existing error messages rather than
> > logging an extra debug message. Rebase onto Jarkko's tree.
> 
> Incidentally, with Jarkko's tree the tpm_tis module refuses to
> initialize (with or without force=1):
> 
> tpm_tis 00:03: can't request region for resource [mem 0xfed40000-0xfed44fff]
> tpm_tis: probe of 00:03 failed with error -16
>
> The memory region is not marked reserved by the BIOS:
> fed30000-fedfffff : RAM buffer

I think your bios is broken?

A working BIOS will look like this:

 $ cat /proc/iomem  | grep -i fed400
 fed40000-fed44fff : pnp 00:00

It sets aside the struct resource during pnp:

 [    0.097318] pnp: PnP ACPI init
 [    0.097366] system 00:00: [mem 0xfed40000-0xfed44fff] has been reserved

What did your system do?

You should see prints like this:

                printk(KERN_DEBUG
                       "e820: reserve RAM buffer [mem %#010llx-%#010llx]\n",
                       start, end);

Which only happen if E820_RAM is set, which is certainly not right for
TPM memory.

I don't know what kernel convention is to handle these sorts of
defects?

Is the use of the memmap kernel command line an appropriate work
around?

Jason

[toc] | [prev] | [next] | [standalone]

#1442804

On Wed, Jul 13, 2016 at 10:36 AM, Jason Gunthorpe
<jgunthorpe@obsidianresearch.com> wrote:
> I think your bios is broken?

The BIOS is broken in many ways. I already have to pass
memmap=256M$0x80000000, otherwise PCIe extended config space
(MMCONFIG) is inaccessible. Also I found memmap=0x7000$0x7a7d0000
works around "APEI: Can not request [mem 0x7a7d0018-0x7a7d0067] for
APEI ERST registers", as the BIOS seems to be mistakenly reserving
0x7b7d0000-7b7d7000 instead.

> A working BIOS will look like this:
>
>  $ cat /proc/iomem  | grep -i fed400
>  fed40000-fed44fff : pnp 00:00
>
> It sets aside the struct resource during pnp:
>
>  [    0.097318] pnp: PnP ACPI init
>  [    0.097366] system 00:00: [mem 0xfed40000-0xfed44fff] has been reserved
>
> What did your system do?
>
> You should see prints like this:
>
>                 printk(KERN_DEBUG
>                        "e820: reserve RAM buffer [mem %#010llx-%#010llx]\n",
>                        start, end);
>
> Which only happen if E820_RAM is set, which is certainly not right for
> TPM memory.

On my system I see

e820: reserve RAM buffer [mem 0x0009b000-0x0009ffff]
e820: reserve RAM buffer [mem 0x75b02000-0x77ffffff]
e820: reserve RAM buffer [mem 0x7a1f6000-0x7bffffff]
e820: reserve RAM buffer [mem 0x7b800000-0x7bffffff]
e820: reserve RAM buffer [mem 0xfed30000-0xffffffff]
e820: reserve RAM buffer [mem 0x505deb000-0x507ffffff]

which doesn't make a whole lot of sense, as several of those areas
overlap each other, never mind devices.

> I don't know what kernel convention is to handle these sorts of
> defects?
>
> Is the use of the memmap kernel command line an appropriate work
> around?

It works for me, though I would like to know if there's another approach.

--Ed

[toc] | [prev] | [next] | [standalone]

#1442847 — Re: [PATCH v9 0/5] tpm: Command duration logging and chip-specific override

On Wed, Jul 13, 2016 at 01:00:28PM -0700, Ed Swierk wrote:
> On Wed, Jul 13, 2016 at 10:36 AM, Jason Gunthorpe
> <jgunthorpe@obsidianresearch.com> wrote:
> > I think your bios is broken?
> 
> The BIOS is broken in many ways. I already have to pass
> memmap=256M$0x80000000, otherwise PCIe extended config space
> (MMCONFIG) is inaccessible. Also I found memmap=0x7000$0x7a7d0000
> works around "APEI: Can not request [mem 0x7a7d0018-0x7a7d0067] for
> APEI ERST registers", as the BIOS seems to be mistakenly reserving
> 0x7b7d0000-7b7d7000 instead.

Is it is possible whatever causes the 'reserve RAM buffer' behavior in
the kernel is wonky with your BIOS? That seems to be a special Linux
action..

Could it be Xen related? The Xen hypervisor replaces the physical ram
map for the dom0 guest, which is why you get these sorts of prints:
 Xen: [mem 0x0000000000000000-0x000000000005ffff] usable
and not:
[    0.000000] BIOS-e820: [mem 0x0000000000000000-0x0000000000057fff] usable

Jason

[toc] | [prev] | [next] | [standalone]

#1442869

Ed Swierk <eswierk@skyportsystems.com> writes:

> On Wed, Jul 13, 2016 at 10:36 AM, Jason Gunthorpe
> <jgunthorpe@obsidianresearch.com> wrote:
>> I think your bios is broken?
>
> The BIOS is broken in many ways. I already have to pass
> memmap=256M$0x80000000, otherwise PCIe extended config space
> (MMCONFIG) is inaccessible. Also I found memmap=0x7000$0x7a7d0000
> works around "APEI: Can not request [mem 0x7a7d0018-0x7a7d0067] for
> APEI ERST registers", as the BIOS seems to be mistakenly reserving
> 0x7b7d0000-7b7d7000 instead.
>
>> A working BIOS will look like this:
>>
>>  $ cat /proc/iomem  | grep -i fed400
>>  fed40000-fed44fff : pnp 00:00
>>
>> It sets aside the struct resource during pnp:
>>
>>  [    0.097318] pnp: PnP ACPI init
>>  [    0.097366] system 00:00: [mem 0xfed40000-0xfed44fff] has been reserved
>>
>> What did your system do?
>>
>> You should see prints like this:
>>
>>                 printk(KERN_DEBUG
>>                        "e820: reserve RAM buffer [mem %#010llx-%#010llx]\n",
>>                        start, end);
>>
>> Which only happen if E820_RAM is set, which is certainly not right for
>> TPM memory.
>
> On my system I see
>
> e820: reserve RAM buffer [mem 0x0009b000-0x0009ffff]
> e820: reserve RAM buffer [mem 0x75b02000-0x77ffffff]
> e820: reserve RAM buffer [mem 0x7a1f6000-0x7bffffff]
> e820: reserve RAM buffer [mem 0x7b800000-0x7bffffff]
> e820: reserve RAM buffer [mem 0xfed30000-0xffffffff]
> e820: reserve RAM buffer [mem 0x505deb000-0x507ffffff]
>
> which doesn't make a whole lot of sense, as several of those areas
> overlap each other, never mind devices.
>
>> I don't know what kernel convention is to handle these sorts of
>> defects?
>>
>> Is the use of the memmap kernel command line an appropriate work
>> around?
>
> It works for me, though I would like to know if there's another
> approach.

There is always poke your BIOS vendor until they deliver code that is
not so b0rked it can not be used.

You can also add a quirk based on the BIOS's mainboard identification
string that fixes up the data provided by the BIOS.  I remember a fair
number of those dealing with reboot behavior and the like.

Eric

[toc] | [prev] | [next] | [standalone]

#1445680 — Re: [PATCH v9 0/5] tpm: Command duration logging and chip-specific override

On Wed, Jul 13, 2016 at 09:19:31AM -0700, Ed Swierk wrote:
> v9: Include command duration in existing error messages rather than
> logging an extra debug message. Rebase onto Jarkko's tree.
> 
> v8: Fix v7 goof-up in tpm_getcap().
> 
> v7: Use tpm_getcap() instead of a redundant new function.
> 
> v6: Split tpm_get_cap_prop() out of tpm_get_timeouts(); always return
> error on TPM command failure.
> 
> v5: Use msecs_to_jiffies() instead of * HZ / 1000.
> 
> v4: Rework tpm_get_timeouts() to allow overriding both timeouts and
> durations via a single callback.
> 
> This series
> - improves TPM command error reporting
> - adds optional logging of TPM command durations
> - allows chip-specific override of command durations as well as protocol
>   timeouts
> - overrides ST19NP18 TPM command duration to avoid lockups

For the next version please write something more readable. Describe the
motivation and solution in big picture, why and how. This list does no
good because it just mimics the list of patches.

/Jarkko

> Ed Swierk (5):
>   tpm_tis: Improve reporting of IO errors
>   tpm: Add optional logging of TPM command durations
>   tpm: Clean up reading of timeout and duration capabilities
>   tpm: Allow TPM chip drivers to override reported command durations
>   tpm_tis: Increase ST19NP18 TPM command duration to avoid chip lockup
> 
>  drivers/char/tpm/tpm-interface.c | 219 ++++++++++++++++++++-------------------
>  drivers/char/tpm/tpm_tis_core.c  |  46 ++++----
>  include/linux/tpm.h              |   3 +-
>  3 files changed, 136 insertions(+), 132 deletions(-)
> 
> -- 
> 1.9.1
> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web