Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1416790 > unrolled thread
| Started by | Ed Swierk <eswierk@skyportsystems.com> |
|---|---|
| First post | 2016-06-08 02:50 +0200 |
| Last post | 2016-06-22 03:20 +0200 |
| Articles | 20 on this page of 37 — 2 participants |
Back to article view | Back to linux.kernel
[PATCH v4 0/4] tpm: Command duration logging and chip-specific override Ed Swierk <eswierk@skyportsystems.com> - 2016-06-08 02:50 +0200
[PATCH v4 1/4] tpm_tis: Improve reporting of IO errors Ed Swierk <eswierk@skyportsystems.com> - 2016-06-08 02:50 +0200
[PATCH v5 0/4] tpm: Command duration logging and chip-specific override Ed Swierk <eswierk@skyportsystems.com> - 2016-06-09 01:10 +0200
[PATCH v5 4/4] tpm_tis: Increase ST19NP18 TPM command duration to avoid chip lockup Ed Swierk <eswierk@skyportsystems.com> - 2016-06-09 01:10 +0200
[PATCH v5 3/4] tpm: Allow TPM chip drivers to override reported command durations Ed Swierk <eswierk@skyportsystems.com> - 2016-06-09 01:10 +0200
Re: [PATCH v5 3/4] tpm: Allow TPM chip drivers to override reported command durations Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2016-06-10 14:30 +0200
Re: [PATCH v5 3/4] tpm: Allow TPM chip drivers to override reported command durations Ed Swierk <eswierk@skyportsystems.com> - 2016-06-10 19:40 +0200
Re: [PATCH v5 3/4] tpm: Allow TPM chip drivers to override reported command durations Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2016-06-10 21:50 +0200
Re: [PATCH v5 3/4] tpm: Allow TPM chip drivers to override reported command durations Ed Swierk <eswierk@skyportsystems.com> - 2016-06-11 04:00 +0200
[PATCH v5 2/4] tpm: Add optional logging of TPM command durations Ed Swierk <eswierk@skyportsystems.com> - 2016-06-09 01:10 +0200
[PATCH v5 1/4] tpm_tis: Improve reporting of IO errors Ed Swierk <eswierk@skyportsystems.com> - 2016-06-09 01:10 +0200
[PATCH v6 2/5] tpm: Add optional logging of TPM command durations Ed Swierk <eswierk@skyportsystems.com> - 2016-06-11 04:00 +0200
[PATCH v6 4/5] tpm: Allow TPM chip drivers to override reported command durations Ed Swierk <eswierk@skyportsystems.com> - 2016-06-11 04:00 +0200
Re: [PATCH v6 4/5] tpm: Allow TPM chip drivers to override reported command durations Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2016-06-16 22:30 +0200
[PATCH v6 5/5] tpm_tis: Increase ST19NP18 TPM command duration to avoid chip lockup Ed Swierk <eswierk@skyportsystems.com> - 2016-06-11 04:00 +0200
[PATCH v6 0/5] tpm: Command duration logging and chip-specific override Ed Swierk <eswierk@skyportsystems.com> - 2016-06-11 04:00 +0200
[PATCH v6 1/5] tpm_tis: Improve reporting of IO errors Ed Swierk <eswierk@skyportsystems.com> - 2016-06-11 04:00 +0200
[PATCH v6 3/5] tpm: Factor out reading of timeout and duration capabilities Ed Swierk <eswierk@skyportsystems.com> - 2016-06-11 04:00 +0200
Re: [PATCH v6 3/5] tpm: Factor out reading of timeout and duration capabilities Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2016-06-16 22:30 +0200
Re: [PATCH v6 3/5] tpm: Factor out reading of timeout and duration capabilities Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2016-06-19 14:20 +0200
[PATCH v7 2/5] tpm: Add optional logging of TPM command durations Ed Swierk <eswierk@skyportsystems.com> - 2016-06-21 04:00 +0200
[PATCH v7 5/5] tpm_tis: Increase ST19NP18 TPM command duration to avoid chip lockup Ed Swierk <eswierk@skyportsystems.com> - 2016-06-21 04:00 +0200
Re: [PATCH v7 5/5] tpm_tis: Increase ST19NP18 TPM command duration to avoid chip lockup Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2016-06-21 23:00 +0200
[PATCH v7 4/5] tpm: Allow TPM chip drivers to override reported command durations Ed Swierk <eswierk@skyportsystems.com> - 2016-06-21 04:00 +0200
Re: [PATCH v7 4/5] tpm: Allow TPM chip drivers to override reported command durations Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2016-06-21 23:00 +0200
[PATCH v7 0/5] tpm: Command duration logging and chip-specific override Ed Swierk <eswierk@skyportsystems.com> - 2016-06-21 04:10 +0200
[PATCH v7 1/5] tpm_tis: Improve reporting of IO errors Ed Swierk <eswierk@skyportsystems.com> - 2016-06-21 04:10 +0200
[PATCH v7 3/5] tpm: Clean up reading of timeout and duration capabilities Ed Swierk <eswierk@skyportsystems.com> - 2016-06-21 05:00 +0200
Re: [PATCH v7 3/5] tpm: Clean up reading of timeout and duration capabilities Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2016-06-21 23:40 +0200
Re: [PATCH v7 3/5] tpm: Clean up reading of timeout and duration capabilities Ed Swierk <eswierk@skyportsystems.com> - 2016-06-22 02:30 +0200
Re: [PATCH v7 3/5] tpm: Clean up reading of timeout and duration capabilities Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2016-06-22 13:00 +0200
[PATCH v8 4/5] tpm: Allow TPM chip drivers to override reported command durations Ed Swierk <eswierk@skyportsystems.com> - 2016-06-22 03:20 +0200
[PATCH v8 1/5] tpm_tis: Improve reporting of IO errors Ed Swierk <eswierk@skyportsystems.com> - 2016-06-22 03:20 +0200
[PATCH v8 2/5] tpm: Add optional logging of TPM command durations Ed Swierk <eswierk@skyportsystems.com> - 2016-06-22 03:20 +0200
[PATCH v8 3/5] tpm: Clean up reading of timeout and duration capabilities Ed Swierk <eswierk@skyportsystems.com> - 2016-06-22 03:20 +0200
[PATCH v8 0/5] tpm: Command duration logging and chip-specific override Ed Swierk <eswierk@skyportsystems.com> - 2016-06-22 03:20 +0200
[PATCH v8 5/5] tpm_tis: Increase ST19NP18 TPM command duration to avoid chip lockup Ed Swierk <eswierk@skyportsystems.com> - 2016-06-22 03:20 +0200
Page 1 of 2 [1] 2 Next page →
| From | Ed Swierk <eswierk@skyportsystems.com> |
|---|---|
| Date | 2016-06-08 02:50 +0200 |
| Subject | [PATCH v4 0/4] tpm: Command duration logging and chip-specific override |
| Message-ID | <rHzQZ-1kp-9@gated-at.bofh.it> |
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 (4): tpm_tis: Improve reporting of IO errors tpm: Add optional logging of TPM command durations 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 | 194 ++++++++++++++++++++++----------------- drivers/char/tpm/tpm_tis.c | 48 +++++----- include/linux/tpm.h | 3 +- 3 files changed, 131 insertions(+), 114 deletions(-) -- 1.9.1
[toc] | [next] | [standalone]
| From | Ed Swierk <eswierk@skyportsystems.com> |
|---|---|
| Date | 2016-06-08 02:50 +0200 |
| Subject | [PATCH v4 1/4] tpm_tis: Improve reporting of IO errors |
| Message-ID | <rHzR0-1kp-19@gated-at.bofh.it> |
| In reply to | #1416790 |
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.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index 65f7eec..088fa86 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -299,6 +299,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->pdev, "Response too long (wanted %zd, got %d)\n",
+ count, expected);
size = -EIO;
goto out;
}
@@ -366,6 +368,8 @@ static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t len)
&chip->vendor.int_queue, false);
status = tpm_tis_status(chip);
if (!itpm && (status & TPM_STS_DATA_EXPECT) == 0) {
+ dev_err(chip->pdev, "Chip not accepting %zd bytes\n",
+ len - count);
rc = -EIO;
goto out_err;
}
@@ -378,6 +382,7 @@ static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t len)
&chip->vendor.int_queue, false);
status = tpm_tis_status(chip);
if ((status & TPM_STS_DATA_EXPECT) != 0) {
+ dev_err(chip->pdev, "Chip not accepting last byte\n");
rc = -EIO;
goto out_err;
}
@@ -689,8 +694,9 @@ static int tpm_tis_init(struct device *dev, struct tpm_info *tpm_info,
vendor = ioread32(chip->vendor.iobase + TPM_DID_VID(0));
chip->vendor.manufacturer_id = vendor;
- 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",
+ chip->vendor.manufacturer_id,
vendor >> 16, ioread8(chip->vendor.iobase + TPM_RID(0)));
if (!itpm) {
--
1.9.1
[toc] | [prev] | [next] | [standalone]
| From | Ed Swierk <eswierk@skyportsystems.com> |
|---|---|
| Date | 2016-06-09 01:10 +0200 |
| Subject | [PATCH v5 0/4] tpm: Command duration logging and chip-specific override |
| Message-ID | <rHULM-6Am-3@gated-at.bofh.it> |
| In reply to | #1416790 |
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 (4): tpm_tis: Improve reporting of IO errors tpm: Add optional logging of TPM command durations 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 | 194 ++++++++++++++++++++++----------------- drivers/char/tpm/tpm_tis.c | 48 +++++----- include/linux/tpm.h | 3 +- 3 files changed, 131 insertions(+), 114 deletions(-) -- 1.9.1
[toc] | [prev] | [next] | [standalone]
| From | Ed Swierk <eswierk@skyportsystems.com> |
|---|---|
| Date | 2016-06-09 01:10 +0200 |
| Subject | [PATCH v5 4/4] tpm_tis: Increase ST19NP18 TPM command duration to avoid chip lockup |
| Message-ID | <rHULM-6Am-7@gated-at.bofh.it> |
| In reply to | #1417943 |
The STMicro ST19NP18-TPM sometimes takes much longer to execute commands than it reports in its capabilities. For example, command 186 (TPM_FlushSpecific) has been observed to take 14560 msec to complete, far longer than the 3000 msec limit for "short" commands reported by the chip. The behavior has also been seen with command 101 (TPM_GetCapability). Worse, when the tpm_tis driver attempts to cancel the current command (by writing commandReady = 1 to TPM_STS_x), the chip locks up completely, returning all-1s from all memory-mapped register reads. The lockup can be cleared only by resetting the system. The occurrence of this excessive command duration depends on the sequence of commands preceding it. One sequence is creating at least 2 new keys via TPM_CreateWrapKey, then letting the TPM idle for at least 30 seconds, then loading a key via TPM_LoadKey2. The next TPM_FlushSpecific occasionally takes tens of seconds to complete. Another sequence is creating many keys in a row without pause. The TPM_CreateWrapKey operation gets much slower after the first few iterations, as one would expect when the pool of precomputed keys is exhausted. Then after a 35-second pause, the same TPM_LoadKey2 followed by TPM_FlushSpecific sequence triggers the behavior. Our working theory is that this older TPM sometimes pauses to precompute keys, which modern chips implement as a background process. Without access to the chip's implementation details it's impossible to know whether any commands are immune to being blocked by this process. So it seems safest to ignore the chip's reported command durations, and use a value much higher than any observed duration, like 180 sec (which is the duration this chip reports for "long" commands). Signed-off-by: Ed Swierk <eswierk@skyportsystems.com> --- drivers/char/tpm/tpm_tis.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c index caf7278..862c502 100644 --- a/drivers/char/tpm/tpm_tis.c +++ b/drivers/char/tpm/tpm_tis.c @@ -485,6 +485,11 @@ static void tpm_tis_update_timeouts(struct tpm_chip *chip) chip->vendor.timeout_d = msecs_to_jiffies(TIS_SHORT_TIMEOUT); chip->vendor.timeout_adjusted = true; break; + case 0x0000104a: /* STMicro ST19NP18-TPM */ + chip->vendor.duration[TPM_SHORT] = 180 * HZ; + chip->vendor.duration[TPM_MEDIUM] = 180 * HZ; + chip->vendor.duration[TPM_LONG] = 180 * HZ; + chip->vendor.duration_adjusted = true; } } -- 1.9.1
[toc] | [prev] | [next] | [standalone]
| From | Ed Swierk <eswierk@skyportsystems.com> |
|---|---|
| Date | 2016-06-09 01:10 +0200 |
| Subject | [PATCH v5 3/4] tpm: Allow TPM chip drivers to override reported command durations |
| Message-ID | <rHULM-6Am-9@gated-at.bofh.it> |
| In reply to | #1417943 |
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 | 177 +++++++++++++++++++++------------------
drivers/char/tpm/tpm_tis.c | 35 ++------
include/linux/tpm.h | 3 +-
3 files changed, 106 insertions(+), 109 deletions(-)
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index cc1e5bc..b8a08bb 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -502,123 +502,138 @@ static int tpm_startup(struct tpm_chip *chip, __be16 startup_type)
"attempting to start the TPM");
}
-int tpm_get_timeouts(struct tpm_chip *chip)
+static int tpm_get_cap_prop(struct tpm_chip *chip, __be32 type, int size,
+ cap_t *cap, char *desc)
{
struct tpm_cmd_t tpm_cmd;
- unsigned long new_timeout[4];
- unsigned long old_timeout[4];
- struct duration_t *duration_cap;
ssize_t 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;
+ tpm_cmd.params.getcap_in.subcap = type;
rc = tpm_transmit_cmd(chip, &tpm_cmd, TPM_INTERNAL_RESULT_SIZE, NULL);
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->pdev, "Issuing TPM_STARTUP");
+ dev_info(chip->pdev, "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;
+ tpm_cmd.params.getcap_in.subcap = type;
rc = tpm_transmit_cmd(chip, &tpm_cmd, TPM_INTERNAL_RESULT_SIZE,
NULL);
}
+
if (rc) {
dev_err(chip->pdev,
- "A TPM error (%zd) occurred attempting to determine the timeouts\n",
- rc);
- goto duration;
+ "Error %zd reading %s\n", rc, desc);
+ return -EINVAL;
}
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))
+ != sizeof(tpm_cmd.header.out) + sizeof(u32) + size * sizeof(u32)) {
+ dev_err(chip->pdev,
+ "Bad return code or length reading %s\n", desc);
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);
- 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->vendor.timeout_adjusted =
- chip->ops->update_timeouts(chip, new_timeout);
-
- if (!chip->vendor.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->vendor.timeout_adjusted = true;
- }
}
- /* Report adjusted timeouts */
- if (chip->vendor.timeout_adjusted) {
- dev_info(chip->pdev,
- 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]);
- }
+ memcpy(cap, &tpm_cmd.params.getcap_out.cap, sizeof(cap_t));
- chip->vendor.timeout_a = usecs_to_jiffies(new_timeout[0]);
- chip->vendor.timeout_b = usecs_to_jiffies(new_timeout[1]);
- chip->vendor.timeout_c = usecs_to_jiffies(new_timeout[2]);
- chip->vendor.timeout_d = usecs_to_jiffies(new_timeout[3]);
+ return 0;
+}
-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;
+int tpm_get_timeouts(struct tpm_chip *chip)
+{
+ cap_t cap1, cap2;
+ int rc1, rc2;
+ struct tpm_vendor_specific orig_vendor;
+
+ rc1 = tpm_get_cap_prop(chip, TPM_CAP_PROP_TIS_TIMEOUT, 4, &cap1,
+ "timeouts");
+ if (rc1 == 0) {
+ 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->vendor.timeout_a = usecs_to_jiffies(cap1.timeout.a);
+ chip->vendor.timeout_b = usecs_to_jiffies(cap1.timeout.b);
+ chip->vendor.timeout_c = usecs_to_jiffies(cap1.timeout.c);
+ chip->vendor.timeout_d = usecs_to_jiffies(cap1.timeout.d);
+ }
+ rc2 = tpm_get_cap_prop(chip, TPM_CAP_PROP_TIS_DURATION, 3, &cap2,
+ "durations");
+ if (rc2 == 0) {
+ be32_to_cpus(&cap2.duration.tpm_short);
+ be32_to_cpus(&cap2.duration.tpm_medium);
+ be32_to_cpus(&cap2.duration.tpm_long);
+ chip->vendor.duration[TPM_SHORT] =
+ usecs_to_jiffies(cap2.duration.tpm_short);
+ chip->vendor.duration[TPM_MEDIUM] =
+ usecs_to_jiffies(cap2.duration.tpm_medium);
+ chip->vendor.duration[TPM_LONG] =
+ usecs_to_jiffies(cap2.duration.tpm_long);
+ }
- rc = tpm_transmit_cmd(chip, &tpm_cmd, TPM_INTERNAL_RESULT_SIZE,
- "attempting to determine the durations");
- if (rc)
- return rc;
+ memcpy(&orig_vendor, &chip->vendor, sizeof(orig_vendor));
+
+ /* 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 (rc1 == 0 && cap1.timeout.a > 0 && cap1.timeout.a < 1000) {
+ chip->vendor.timeout_a = msecs_to_jiffies(cap1.timeout.a);
+ chip->vendor.timeout_b = msecs_to_jiffies(cap1.timeout.b);
+ chip->vendor.timeout_c = msecs_to_jiffies(cap1.timeout.c);
+ chip->vendor.timeout_d = msecs_to_jiffies(cap1.timeout.d);
+ chip->vendor.timeout_adjusted = true;
+ }
+ /* Interpret duration values between 1 and 10000 as
+ milliseconds to deal with TPMs like the Broadcom BCM0102 in
+ the Dell Latitude D820. */
+ if (rc2 == 0 && cap2.duration.tpm_short > 0 &&
+ cap2.duration.tpm_short < 10000) {
+ chip->vendor.duration[TPM_SHORT] =
+ msecs_to_jiffies(cap2.duration.tpm_short);
+ chip->vendor.duration[TPM_MEDIUM] =
+ msecs_to_jiffies(cap2.duration.tpm_medium);
+ chip->vendor.duration[TPM_LONG] =
+ msecs_to_jiffies(cap2.duration.tpm_long);
+ chip->vendor.duration_adjusted = true;
+ }
- 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;
+ if (chip->ops->update_timeouts != NULL)
+ chip->ops->update_timeouts(chip);
- duration_cap = &tpm_cmd.params.getcap_out.cap.duration;
- chip->vendor.duration[TPM_SHORT] =
- usecs_to_jiffies(be32_to_cpu(duration_cap->tpm_short));
- chip->vendor.duration[TPM_MEDIUM] =
- usecs_to_jiffies(be32_to_cpu(duration_cap->tpm_medium));
- chip->vendor.duration[TPM_LONG] =
- usecs_to_jiffies(be32_to_cpu(duration_cap->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
- * 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->vendor.duration[TPM_SHORT] < (HZ / 100)) {
- chip->vendor.duration[TPM_SHORT] = HZ;
- chip->vendor.duration[TPM_MEDIUM] *= 1000;
- chip->vendor.duration[TPM_LONG] *= 1000;
- chip->vendor.duration_adjusted = true;
- dev_info(chip->pdev, "Adjusting TPM timeout parameters.");
+ if (chip->vendor.timeout_adjusted) {
+ dev_info(chip->pdev,
+ HW_ERR "Adjusted timeouts: A %u->%uus B %u->%uus"
+ " C %u->%uus D %u->%uus\n",
+ jiffies_to_usecs(orig_vendor.timeout_a),
+ jiffies_to_usecs(chip->vendor.timeout_a),
+ jiffies_to_usecs(orig_vendor.timeout_b),
+ jiffies_to_usecs(chip->vendor.timeout_b),
+ jiffies_to_usecs(orig_vendor.timeout_c),
+ jiffies_to_usecs(chip->vendor.timeout_c),
+ jiffies_to_usecs(orig_vendor.timeout_d),
+ jiffies_to_usecs(chip->vendor.timeout_d));
+ }
+ if (chip->vendor.duration_adjusted) {
+ dev_info(chip->pdev,
+ HW_ERR "Adjusted durations: short %u->%uus"
+ " medium %u->%uus long %u->%uus\n",
+ jiffies_to_usecs(orig_vendor.duration[TPM_SHORT]),
+ jiffies_to_usecs(chip->vendor.duration[TPM_SHORT]),
+ jiffies_to_usecs(orig_vendor.duration[TPM_MEDIUM]),
+ jiffies_to_usecs(chip->vendor.duration[TPM_MEDIUM]),
+ jiffies_to_usecs(orig_vendor.duration[TPM_LONG]),
+ jiffies_to_usecs(chip->vendor.duration[TPM_LONG]));
}
+
return 0;
}
EXPORT_SYMBOL_GPL(tpm_get_timeouts);
diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index 088fa86..caf7278 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -475,34 +475,17 @@ 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)
{
- int i;
- u32 did_vid;
-
- did_vid = ioread32(chip->vendor.iobase + TPM_DID_VID(0));
-
- 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 (ioread32(chip->vendor.iobase + TPM_DID_VID(0))) {
+ case 0x32041114: /* Atmel 3204 */
+ chip->vendor.timeout_a = msecs_to_jiffies(TIS_SHORT_TIMEOUT);
+ chip->vendor.timeout_b = msecs_to_jiffies(TIS_LONG_TIMEOUT);
+ chip->vendor.timeout_c = msecs_to_jiffies(TIS_SHORT_TIMEOUT);
+ chip->vendor.timeout_d = msecs_to_jiffies(TIS_SHORT_TIMEOUT);
+ chip->vendor.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]
| From | Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> |
|---|---|
| Date | 2016-06-10 14:30 +0200 |
| Subject | Re: [PATCH v5 3/4] tpm: Allow TPM chip drivers to override reported command durations |
| Message-ID | <rItJw-4w2-13@gated-at.bofh.it> |
| In reply to | #1417945 |
On Wed, Jun 08, 2016 at 04:00:17PM -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.
>
> Signed-off-by: Ed Swierk <eswierk@skyportsystems.com>
> ---
> drivers/char/tpm/tpm-interface.c | 177 +++++++++++++++++++++------------------
> drivers/char/tpm/tpm_tis.c | 35 ++------
> include/linux/tpm.h | 3 +-
> 3 files changed, 106 insertions(+), 109 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index cc1e5bc..b8a08bb 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -502,123 +502,138 @@ static int tpm_startup(struct tpm_chip *chip, __be16 startup_type)
> "attempting to start the TPM");
> }
>
> -int tpm_get_timeouts(struct tpm_chip *chip)
> +static int tpm_get_cap_prop(struct tpm_chip *chip, __be32 type, int size,
> + cap_t *cap, char *desc)
> {
> struct tpm_cmd_t tpm_cmd;
> - unsigned long new_timeout[4];
> - unsigned long old_timeout[4];
> - struct duration_t *duration_cap;
> ssize_t 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;
> + tpm_cmd.params.getcap_in.subcap = type;
> rc = tpm_transmit_cmd(chip, &tpm_cmd, TPM_INTERNAL_RESULT_SIZE, NULL);
>
> 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->pdev, "Issuing TPM_STARTUP");
> + dev_info(chip->pdev, "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;
> + tpm_cmd.params.getcap_in.subcap = type;
> rc = tpm_transmit_cmd(chip, &tpm_cmd, TPM_INTERNAL_RESULT_SIZE,
> NULL);
> }
> +
> if (rc) {
> dev_err(chip->pdev,
> - "A TPM error (%zd) occurred attempting to determine the timeouts\n",
> - rc);
> - goto duration;
> + "Error %zd reading %s\n", rc, desc);
> + return -EINVAL;
> }
>
> 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))
> + != sizeof(tpm_cmd.header.out) + sizeof(u32) + size * sizeof(u32)) {
> + dev_err(chip->pdev,
> + "Bad return code or length reading %s\n", desc);
> 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);
> - 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->vendor.timeout_adjusted =
> - chip->ops->update_timeouts(chip, new_timeout);
> -
> - if (!chip->vendor.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->vendor.timeout_adjusted = true;
> - }
> }
>
> - /* Report adjusted timeouts */
> - if (chip->vendor.timeout_adjusted) {
> - dev_info(chip->pdev,
> - 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]);
> - }
> + memcpy(cap, &tpm_cmd.params.getcap_out.cap, sizeof(cap_t));
>
> - chip->vendor.timeout_a = usecs_to_jiffies(new_timeout[0]);
> - chip->vendor.timeout_b = usecs_to_jiffies(new_timeout[1]);
> - chip->vendor.timeout_c = usecs_to_jiffies(new_timeout[2]);
> - chip->vendor.timeout_d = usecs_to_jiffies(new_timeout[3]);
> + return 0;
> +}
>
> -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;
> +int tpm_get_timeouts(struct tpm_chip *chip)
> +{
> + cap_t cap1, cap2;
> + int rc1, rc2;
> + struct tpm_vendor_specific orig_vendor;
> +
> + rc1 = tpm_get_cap_prop(chip, TPM_CAP_PROP_TIS_TIMEOUT, 4, &cap1,
> + "timeouts");
> + if (rc1 == 0) {
> + 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->vendor.timeout_a = usecs_to_jiffies(cap1.timeout.a);
> + chip->vendor.timeout_b = usecs_to_jiffies(cap1.timeout.b);
> + chip->vendor.timeout_c = usecs_to_jiffies(cap1.timeout.c);
> + chip->vendor.timeout_d = usecs_to_jiffies(cap1.timeout.d);
> + }
> + rc2 = tpm_get_cap_prop(chip, TPM_CAP_PROP_TIS_DURATION, 3, &cap2,
> + "durations");
> + if (rc2 == 0) {
> + be32_to_cpus(&cap2.duration.tpm_short);
> + be32_to_cpus(&cap2.duration.tpm_medium);
> + be32_to_cpus(&cap2.duration.tpm_long);
> + chip->vendor.duration[TPM_SHORT] =
> + usecs_to_jiffies(cap2.duration.tpm_short);
> + chip->vendor.duration[TPM_MEDIUM] =
> + usecs_to_jiffies(cap2.duration.tpm_medium);
> + chip->vendor.duration[TPM_LONG] =
> + usecs_to_jiffies(cap2.duration.tpm_long);
> + }
This is major change to the semantics. Before -EINVAL would have been
return on error condition.
PS If you want to encapsulate tpm_get_cap_prop(), that step should be
a separate commit (prepend this one).
/Jarkko
[toc] | [prev] | [next] | [standalone]
| From | Ed Swierk <eswierk@skyportsystems.com> |
|---|---|
| Date | 2016-06-10 19:40 +0200 |
| Subject | Re: [PATCH v5 3/4] tpm: Allow TPM chip drivers to override reported command durations |
| Message-ID | <rIyzv-7xq-3@gated-at.bofh.it> |
| In reply to | #1419326 |
On Fri, Jun 10, 2016 at 5:19 AM, Jarkko Sakkinen
<jarkko.sakkinen@linux.intel.com> wrote:
> On Wed, Jun 08, 2016 at 04:00:17PM -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.
>>
>> Signed-off-by: Ed Swierk <eswierk@skyportsystems.com>
>> ---
>> drivers/char/tpm/tpm-interface.c | 177 +++++++++++++++++++++------------------
>> drivers/char/tpm/tpm_tis.c | 35 ++------
>> include/linux/tpm.h | 3 +-
>> 3 files changed, 106 insertions(+), 109 deletions(-)
>>
>> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
>> index cc1e5bc..b8a08bb 100644
>> --- a/drivers/char/tpm/tpm-interface.c
>> +++ b/drivers/char/tpm/tpm-interface.c
>> @@ -502,123 +502,138 @@ static int tpm_startup(struct tpm_chip *chip, __be16 startup_type)
>> "attempting to start the TPM");
>> }
>>
>> -int tpm_get_timeouts(struct tpm_chip *chip)
>> +static int tpm_get_cap_prop(struct tpm_chip *chip, __be32 type, int size,
>> + cap_t *cap, char *desc)
>> {
>> struct tpm_cmd_t tpm_cmd;
>> - unsigned long new_timeout[4];
>> - unsigned long old_timeout[4];
>> - struct duration_t *duration_cap;
>> ssize_t 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;
>> + tpm_cmd.params.getcap_in.subcap = type;
>> rc = tpm_transmit_cmd(chip, &tpm_cmd, TPM_INTERNAL_RESULT_SIZE, NULL);
>>
>> 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->pdev, "Issuing TPM_STARTUP");
>> + dev_info(chip->pdev, "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;
>> + tpm_cmd.params.getcap_in.subcap = type;
>> rc = tpm_transmit_cmd(chip, &tpm_cmd, TPM_INTERNAL_RESULT_SIZE,
>> NULL);
>> }
>> +
>> if (rc) {
>> dev_err(chip->pdev,
>> - "A TPM error (%zd) occurred attempting to determine the timeouts\n",
>> - rc);
>> - goto duration;
>> + "Error %zd reading %s\n", rc, desc);
>> + return -EINVAL;
>> }
>>
>> 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))
>> + != sizeof(tpm_cmd.header.out) + sizeof(u32) + size * sizeof(u32)) {
>> + dev_err(chip->pdev,
>> + "Bad return code or length reading %s\n", desc);
>> 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);
>> - 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->vendor.timeout_adjusted =
>> - chip->ops->update_timeouts(chip, new_timeout);
>> -
>> - if (!chip->vendor.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->vendor.timeout_adjusted = true;
>> - }
>> }
>>
>> - /* Report adjusted timeouts */
>> - if (chip->vendor.timeout_adjusted) {
>> - dev_info(chip->pdev,
>> - 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]);
>> - }
>> + memcpy(cap, &tpm_cmd.params.getcap_out.cap, sizeof(cap_t));
>>
>> - chip->vendor.timeout_a = usecs_to_jiffies(new_timeout[0]);
>> - chip->vendor.timeout_b = usecs_to_jiffies(new_timeout[1]);
>> - chip->vendor.timeout_c = usecs_to_jiffies(new_timeout[2]);
>> - chip->vendor.timeout_d = usecs_to_jiffies(new_timeout[3]);
>> + return 0;
>> +}
>>
>> -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;
>> +int tpm_get_timeouts(struct tpm_chip *chip)
>> +{
>> + cap_t cap1, cap2;
>> + int rc1, rc2;
>> + struct tpm_vendor_specific orig_vendor;
>> +
>> + rc1 = tpm_get_cap_prop(chip, TPM_CAP_PROP_TIS_TIMEOUT, 4, &cap1,
>> + "timeouts");
>> + if (rc1 == 0) {
>> + 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->vendor.timeout_a = usecs_to_jiffies(cap1.timeout.a);
>> + chip->vendor.timeout_b = usecs_to_jiffies(cap1.timeout.b);
>> + chip->vendor.timeout_c = usecs_to_jiffies(cap1.timeout.c);
>> + chip->vendor.timeout_d = usecs_to_jiffies(cap1.timeout.d);
>> + }
>> + rc2 = tpm_get_cap_prop(chip, TPM_CAP_PROP_TIS_DURATION, 3, &cap2,
>> + "durations");
>> + if (rc2 == 0) {
>> + be32_to_cpus(&cap2.duration.tpm_short);
>> + be32_to_cpus(&cap2.duration.tpm_medium);
>> + be32_to_cpus(&cap2.duration.tpm_long);
>> + chip->vendor.duration[TPM_SHORT] =
>> + usecs_to_jiffies(cap2.duration.tpm_short);
>> + chip->vendor.duration[TPM_MEDIUM] =
>> + usecs_to_jiffies(cap2.duration.tpm_medium);
>> + chip->vendor.duration[TPM_LONG] =
>> + usecs_to_jiffies(cap2.duration.tpm_long);
>> + }
>
> This is major change to the semantics. Before -EINVAL would have been
> return on error condition.
>
> PS If you want to encapsulate tpm_get_cap_prop(), that step should be
> a separate commit (prepend this one).
Good points.
I'm confused about the error semantics in the first (timeouts) part of
tpm_get_timeouts(). If tpm_transmit_cmd() returns zero and a header
check fails, it returns -EINVAL. But if tpm_transmit_cmd() returns
nonzero, it swallows the error.
In contrast, in the second (durations) part, an error is returned in
either case.
Is this difference intentional, or should tpm_get_timeouts() return
errors immediately in all cases?
--Ed
[toc] | [prev] | [next] | [standalone]
| From | Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> |
|---|---|
| Date | 2016-06-10 21:50 +0200 |
| Subject | Re: [PATCH v5 3/4] tpm: Allow TPM chip drivers to override reported command durations |
| Message-ID | <rIABj-Hr-7@gated-at.bofh.it> |
| In reply to | #1419613 |
On Fri, Jun 10, 2016 at 10:34:15AM -0700, Ed Swierk wrote:
> On Fri, Jun 10, 2016 at 5:19 AM, Jarkko Sakkinen
> <jarkko.sakkinen@linux.intel.com> wrote:
> > On Wed, Jun 08, 2016 at 04:00:17PM -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.
> >>
> >> Signed-off-by: Ed Swierk <eswierk@skyportsystems.com>
> >> ---
> >> drivers/char/tpm/tpm-interface.c | 177 +++++++++++++++++++++------------------
> >> drivers/char/tpm/tpm_tis.c | 35 ++------
> >> include/linux/tpm.h | 3 +-
> >> 3 files changed, 106 insertions(+), 109 deletions(-)
> >>
> >> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> >> index cc1e5bc..b8a08bb 100644
> >> --- a/drivers/char/tpm/tpm-interface.c
> >> +++ b/drivers/char/tpm/tpm-interface.c
> >> @@ -502,123 +502,138 @@ static int tpm_startup(struct tpm_chip *chip, __be16 startup_type)
> >> "attempting to start the TPM");
> >> }
> >>
> >> -int tpm_get_timeouts(struct tpm_chip *chip)
> >> +static int tpm_get_cap_prop(struct tpm_chip *chip, __be32 type, int size,
> >> + cap_t *cap, char *desc)
> >> {
> >> struct tpm_cmd_t tpm_cmd;
> >> - unsigned long new_timeout[4];
> >> - unsigned long old_timeout[4];
> >> - struct duration_t *duration_cap;
> >> ssize_t 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;
> >> + tpm_cmd.params.getcap_in.subcap = type;
> >> rc = tpm_transmit_cmd(chip, &tpm_cmd, TPM_INTERNAL_RESULT_SIZE, NULL);
> >>
> >> 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->pdev, "Issuing TPM_STARTUP");
> >> + dev_info(chip->pdev, "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;
> >> + tpm_cmd.params.getcap_in.subcap = type;
> >> rc = tpm_transmit_cmd(chip, &tpm_cmd, TPM_INTERNAL_RESULT_SIZE,
> >> NULL);
> >> }
> >> +
> >> if (rc) {
> >> dev_err(chip->pdev,
> >> - "A TPM error (%zd) occurred attempting to determine the timeouts\n",
> >> - rc);
> >> - goto duration;
> >> + "Error %zd reading %s\n", rc, desc);
> >> + return -EINVAL;
> >> }
> >>
> >> 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))
> >> + != sizeof(tpm_cmd.header.out) + sizeof(u32) + size * sizeof(u32)) {
> >> + dev_err(chip->pdev,
> >> + "Bad return code or length reading %s\n", desc);
> >> 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);
> >> - 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->vendor.timeout_adjusted =
> >> - chip->ops->update_timeouts(chip, new_timeout);
> >> -
> >> - if (!chip->vendor.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->vendor.timeout_adjusted = true;
> >> - }
> >> }
> >>
> >> - /* Report adjusted timeouts */
> >> - if (chip->vendor.timeout_adjusted) {
> >> - dev_info(chip->pdev,
> >> - 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]);
> >> - }
> >> + memcpy(cap, &tpm_cmd.params.getcap_out.cap, sizeof(cap_t));
> >>
> >> - chip->vendor.timeout_a = usecs_to_jiffies(new_timeout[0]);
> >> - chip->vendor.timeout_b = usecs_to_jiffies(new_timeout[1]);
> >> - chip->vendor.timeout_c = usecs_to_jiffies(new_timeout[2]);
> >> - chip->vendor.timeout_d = usecs_to_jiffies(new_timeout[3]);
> >> + return 0;
> >> +}
> >>
> >> -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;
> >> +int tpm_get_timeouts(struct tpm_chip *chip)
> >> +{
> >> + cap_t cap1, cap2;
> >> + int rc1, rc2;
> >> + struct tpm_vendor_specific orig_vendor;
> >> +
> >> + rc1 = tpm_get_cap_prop(chip, TPM_CAP_PROP_TIS_TIMEOUT, 4, &cap1,
> >> + "timeouts");
> >> + if (rc1 == 0) {
> >> + 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->vendor.timeout_a = usecs_to_jiffies(cap1.timeout.a);
> >> + chip->vendor.timeout_b = usecs_to_jiffies(cap1.timeout.b);
> >> + chip->vendor.timeout_c = usecs_to_jiffies(cap1.timeout.c);
> >> + chip->vendor.timeout_d = usecs_to_jiffies(cap1.timeout.d);
> >> + }
> >> + rc2 = tpm_get_cap_prop(chip, TPM_CAP_PROP_TIS_DURATION, 3, &cap2,
> >> + "durations");
> >> + if (rc2 == 0) {
> >> + be32_to_cpus(&cap2.duration.tpm_short);
> >> + be32_to_cpus(&cap2.duration.tpm_medium);
> >> + be32_to_cpus(&cap2.duration.tpm_long);
> >> + chip->vendor.duration[TPM_SHORT] =
> >> + usecs_to_jiffies(cap2.duration.tpm_short);
> >> + chip->vendor.duration[TPM_MEDIUM] =
> >> + usecs_to_jiffies(cap2.duration.tpm_medium);
> >> + chip->vendor.duration[TPM_LONG] =
> >> + usecs_to_jiffies(cap2.duration.tpm_long);
> >> + }
> >
> > This is major change to the semantics. Before -EINVAL would have been
> > return on error condition.
> >
> > PS If you want to encapsulate tpm_get_cap_prop(), that step should be
> > a separate commit (prepend this one).
>
> Good points.
>
> I'm confused about the error semantics in the first (timeouts) part of
> tpm_get_timeouts(). If tpm_transmit_cmd() returns zero and a header
> check fails, it returns -EINVAL. But if tpm_transmit_cmd() returns
> nonzero, it swallows the error.
>
> In contrast, in the second (durations) part, an error is returned in
> either case.
>
> Is this difference intentional, or should tpm_get_timeouts() return
> errors immediately in all cases?
Sometimes these kinds of things are just "evolutional" :)
There are also couple of other things that don't look right:
* tpm_transmit_cmd() already prints the TPM error
* If you use dev_err(), you must fail. If we ought to continue, it
should be at most dev_warn().
In my opinion it is just plain wrong to continue to the durations part
in the case of TPM error. I would like to hear a second opinion, though.
> --Ed
/Jarkko
[toc] | [prev] | [next] | [standalone]
| From | Ed Swierk <eswierk@skyportsystems.com> |
|---|---|
| Date | 2016-06-11 04:00 +0200 |
| Subject | Re: [PATCH v5 3/4] tpm: Allow TPM chip drivers to override reported command durations |
| Message-ID | <rIGnn-55z-9@gated-at.bofh.it> |
| In reply to | #1419679 |
On Fri, Jun 10, 2016 at 12:42 PM, Jarkko Sakkinen
<jarkko.sakkinen@linux.intel.com> wrote:
> On Fri, Jun 10, 2016 at 10:34:15AM -0700, Ed Swierk wrote:
>> On Fri, Jun 10, 2016 at 5:19 AM, Jarkko Sakkinen
>> <jarkko.sakkinen@linux.intel.com> wrote:
>> > On Wed, Jun 08, 2016 at 04:00:17PM -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.
>> >>
>> >> Signed-off-by: Ed Swierk <eswierk@skyportsystems.com>
>> >> ---
>> >> drivers/char/tpm/tpm-interface.c | 177 +++++++++++++++++++++------------------
>> >> drivers/char/tpm/tpm_tis.c | 35 ++------
>> >> include/linux/tpm.h | 3 +-
>> >> 3 files changed, 106 insertions(+), 109 deletions(-)
>> >>
>> >> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
>> >> index cc1e5bc..b8a08bb 100644
>> >> --- a/drivers/char/tpm/tpm-interface.c
>> >> +++ b/drivers/char/tpm/tpm-interface.c
>> >> @@ -502,123 +502,138 @@ static int tpm_startup(struct tpm_chip *chip, __be16 startup_type)
>> >> "attempting to start the TPM");
>> >> }
>> >>
>> >> -int tpm_get_timeouts(struct tpm_chip *chip)
>> >> +static int tpm_get_cap_prop(struct tpm_chip *chip, __be32 type, int size,
>> >> + cap_t *cap, char *desc)
>> >> {
>> >> struct tpm_cmd_t tpm_cmd;
>> >> - unsigned long new_timeout[4];
>> >> - unsigned long old_timeout[4];
>> >> - struct duration_t *duration_cap;
>> >> ssize_t 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;
>> >> + tpm_cmd.params.getcap_in.subcap = type;
>> >> rc = tpm_transmit_cmd(chip, &tpm_cmd, TPM_INTERNAL_RESULT_SIZE, NULL);
>> >>
>> >> 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->pdev, "Issuing TPM_STARTUP");
>> >> + dev_info(chip->pdev, "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;
>> >> + tpm_cmd.params.getcap_in.subcap = type;
>> >> rc = tpm_transmit_cmd(chip, &tpm_cmd, TPM_INTERNAL_RESULT_SIZE,
>> >> NULL);
>> >> }
>> >> +
>> >> if (rc) {
>> >> dev_err(chip->pdev,
>> >> - "A TPM error (%zd) occurred attempting to determine the timeouts\n",
>> >> - rc);
>> >> - goto duration;
>> >> + "Error %zd reading %s\n", rc, desc);
>> >> + return -EINVAL;
>> >> }
>> >>
>> >> 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))
>> >> + != sizeof(tpm_cmd.header.out) + sizeof(u32) + size * sizeof(u32)) {
>> >> + dev_err(chip->pdev,
>> >> + "Bad return code or length reading %s\n", desc);
>> >> 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);
>> >> - 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->vendor.timeout_adjusted =
>> >> - chip->ops->update_timeouts(chip, new_timeout);
>> >> -
>> >> - if (!chip->vendor.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->vendor.timeout_adjusted = true;
>> >> - }
>> >> }
>> >>
>> >> - /* Report adjusted timeouts */
>> >> - if (chip->vendor.timeout_adjusted) {
>> >> - dev_info(chip->pdev,
>> >> - 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]);
>> >> - }
>> >> + memcpy(cap, &tpm_cmd.params.getcap_out.cap, sizeof(cap_t));
>> >>
>> >> - chip->vendor.timeout_a = usecs_to_jiffies(new_timeout[0]);
>> >> - chip->vendor.timeout_b = usecs_to_jiffies(new_timeout[1]);
>> >> - chip->vendor.timeout_c = usecs_to_jiffies(new_timeout[2]);
>> >> - chip->vendor.timeout_d = usecs_to_jiffies(new_timeout[3]);
>> >> + return 0;
>> >> +}
>> >>
>> >> -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;
>> >> +int tpm_get_timeouts(struct tpm_chip *chip)
>> >> +{
>> >> + cap_t cap1, cap2;
>> >> + int rc1, rc2;
>> >> + struct tpm_vendor_specific orig_vendor;
>> >> +
>> >> + rc1 = tpm_get_cap_prop(chip, TPM_CAP_PROP_TIS_TIMEOUT, 4, &cap1,
>> >> + "timeouts");
>> >> + if (rc1 == 0) {
>> >> + 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->vendor.timeout_a = usecs_to_jiffies(cap1.timeout.a);
>> >> + chip->vendor.timeout_b = usecs_to_jiffies(cap1.timeout.b);
>> >> + chip->vendor.timeout_c = usecs_to_jiffies(cap1.timeout.c);
>> >> + chip->vendor.timeout_d = usecs_to_jiffies(cap1.timeout.d);
>> >> + }
>> >> + rc2 = tpm_get_cap_prop(chip, TPM_CAP_PROP_TIS_DURATION, 3, &cap2,
>> >> + "durations");
>> >> + if (rc2 == 0) {
>> >> + be32_to_cpus(&cap2.duration.tpm_short);
>> >> + be32_to_cpus(&cap2.duration.tpm_medium);
>> >> + be32_to_cpus(&cap2.duration.tpm_long);
>> >> + chip->vendor.duration[TPM_SHORT] =
>> >> + usecs_to_jiffies(cap2.duration.tpm_short);
>> >> + chip->vendor.duration[TPM_MEDIUM] =
>> >> + usecs_to_jiffies(cap2.duration.tpm_medium);
>> >> + chip->vendor.duration[TPM_LONG] =
>> >> + usecs_to_jiffies(cap2.duration.tpm_long);
>> >> + }
>> >
>> > This is major change to the semantics. Before -EINVAL would have been
>> > return on error condition.
>> >
>> > PS If you want to encapsulate tpm_get_cap_prop(), that step should be
>> > a separate commit (prepend this one).
>>
>> Good points.
>>
>> I'm confused about the error semantics in the first (timeouts) part of
>> tpm_get_timeouts(). If tpm_transmit_cmd() returns zero and a header
>> check fails, it returns -EINVAL. But if tpm_transmit_cmd() returns
>> nonzero, it swallows the error.
>>
>> In contrast, in the second (durations) part, an error is returned in
>> either case.
>>
>> Is this difference intentional, or should tpm_get_timeouts() return
>> errors immediately in all cases?
>
> Sometimes these kinds of things are just "evolutional" :)
>
> There are also couple of other things that don't look right:
>
> * tpm_transmit_cmd() already prints the TPM error
> * If you use dev_err(), you must fail. If we ought to continue, it
> should be at most dev_warn().
>
> In my opinion it is just plain wrong to continue to the durations part
> in the case of TPM error. I would like to hear a second opinion, though.
>
>> --Ed
>
> /Jarkko
I'll split tpm_get_cap_prop() out from tpm_get_timeouts() in a
separate commit, and adjust the error handling on the assumption that
TPM errors should always be propagated.
--Ed
[toc] | [prev] | [next] | [standalone]
| From | Ed Swierk <eswierk@skyportsystems.com> |
|---|---|
| Date | 2016-06-09 01:10 +0200 |
| Subject | [PATCH v5 2/4] tpm: Add optional logging of TPM command durations |
| Message-ID | <rHULM-6Am-11@gated-at.bofh.it> |
| In reply to | #1417943 |
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 | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index c50637d..cc1e5bc 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -333,13 +333,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->pdev, "starting command %d count %d\n", ordinal, count);
if (count == 0)
return -ENODATA;
if (count > bufsiz) {
@@ -360,18 +361,24 @@ ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,
if (chip->vendor.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->pdev, "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->pdev, "Operation Canceled\n");
+ dev_dbg(chip->pdev, "canceled command %d after %d ms\n",
+ ordinal, jiffies_to_msecs(jiffies - start));
rc = -ECANCELED;
goto out;
}
@@ -382,6 +389,8 @@ ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,
chip->ops->cancel(chip);
dev_err(chip->pdev, "Operation Timed out\n");
+ dev_dbg(chip->pdev, "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]
| From | Ed Swierk <eswierk@skyportsystems.com> |
|---|---|
| Date | 2016-06-09 01:10 +0200 |
| Subject | [PATCH v5 1/4] tpm_tis: Improve reporting of IO errors |
| Message-ID | <rHULM-6Am-19@gated-at.bofh.it> |
| In reply to | #1417943 |
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.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index 65f7eec..088fa86 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -299,6 +299,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->pdev, "Response too long (wanted %zd, got %d)\n",
+ count, expected);
size = -EIO;
goto out;
}
@@ -366,6 +368,8 @@ static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t len)
&chip->vendor.int_queue, false);
status = tpm_tis_status(chip);
if (!itpm && (status & TPM_STS_DATA_EXPECT) == 0) {
+ dev_err(chip->pdev, "Chip not accepting %zd bytes\n",
+ len - count);
rc = -EIO;
goto out_err;
}
@@ -378,6 +382,7 @@ static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t len)
&chip->vendor.int_queue, false);
status = tpm_tis_status(chip);
if ((status & TPM_STS_DATA_EXPECT) != 0) {
+ dev_err(chip->pdev, "Chip not accepting last byte\n");
rc = -EIO;
goto out_err;
}
@@ -689,8 +694,9 @@ static int tpm_tis_init(struct device *dev, struct tpm_info *tpm_info,
vendor = ioread32(chip->vendor.iobase + TPM_DID_VID(0));
chip->vendor.manufacturer_id = vendor;
- 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",
+ chip->vendor.manufacturer_id,
vendor >> 16, ioread8(chip->vendor.iobase + TPM_RID(0)));
if (!itpm) {
--
1.9.1
[toc] | [prev] | [next] | [standalone]
| From | Ed Swierk <eswierk@skyportsystems.com> |
|---|---|
| Date | 2016-06-11 04:00 +0200 |
| Subject | [PATCH v6 2/5] tpm: Add optional logging of TPM command durations |
| Message-ID | <rIGnn-55z-3@gated-at.bofh.it> |
| In reply to | #1417943 |
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 | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index c50637d..cc1e5bc 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -333,13 +333,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->pdev, "starting command %d count %d\n", ordinal, count);
if (count == 0)
return -ENODATA;
if (count > bufsiz) {
@@ -360,18 +361,24 @@ ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,
if (chip->vendor.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->pdev, "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->pdev, "Operation Canceled\n");
+ dev_dbg(chip->pdev, "canceled command %d after %d ms\n",
+ ordinal, jiffies_to_msecs(jiffies - start));
rc = -ECANCELED;
goto out;
}
@@ -382,6 +389,8 @@ ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,
chip->ops->cancel(chip);
dev_err(chip->pdev, "Operation Timed out\n");
+ dev_dbg(chip->pdev, "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]
| From | Ed Swierk <eswierk@skyportsystems.com> |
|---|---|
| Date | 2016-06-11 04:00 +0200 |
| Subject | [PATCH v6 4/5] tpm: Allow TPM chip drivers to override reported command durations |
| Message-ID | <rIGnn-55z-15@gated-at.bofh.it> |
| In reply to | #1417943 |
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 | 139 +++++++++++++++++++++------------------
drivers/char/tpm/tpm_tis.c | 35 +++-------
include/linux/tpm.h | 3 +-
3 files changed, 85 insertions(+), 92 deletions(-)
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 4d1f62c..a14adfd 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -550,83 +550,94 @@ static int tpm_get_cap_prop(struct tpm_chip *chip, __be32 type, int size,
int tpm_get_timeouts(struct tpm_chip *chip)
{
- struct tpm_cmd_t tpm_cmd;
- unsigned long new_timeout[4];
- unsigned long old_timeout[4];
- struct duration_t *duration_cap;
- ssize_t rc;
+ cap_t cap1, cap2;
+ int rc;
+ struct tpm_vendor_specific orig_vendor;
- rc = tpm_get_cap_prop(chip, TPM_CAP_PROP_TIS_TIMEOUT, 4,
- &tpm_cmd.params.getcap_out.cap, "timeouts");
+ rc = tpm_get_cap_prop(chip, TPM_CAP_PROP_TIS_TIMEOUT, 4, &cap1,
+ "timeouts");
if (rc)
return rc;
- 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);
- memcpy(new_timeout, old_timeout, sizeof(new_timeout));
+ rc = tpm_get_cap_prop(chip, TPM_CAP_PROP_TIS_DURATION, 3, &cap2,
+ "durations");
+ if (rc)
+ return rc;
- /*
- * Provide ability for vendor overrides of timeout values in case
- * of misreporting.
- */
- if (chip->ops->update_timeouts != NULL)
- chip->vendor.timeout_adjusted =
- chip->ops->update_timeouts(chip, new_timeout);
+ 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->vendor.timeout_a = usecs_to_jiffies(cap1.timeout.a);
+ chip->vendor.timeout_b = usecs_to_jiffies(cap1.timeout.b);
+ chip->vendor.timeout_c = usecs_to_jiffies(cap1.timeout.c);
+ chip->vendor.timeout_d = usecs_to_jiffies(cap1.timeout.d);
- if (!chip->vendor.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->vendor.timeout_adjusted = true;
- }
+ /* 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->vendor.timeout_a = msecs_to_jiffies(cap1.timeout.a);
+ chip->vendor.timeout_b = msecs_to_jiffies(cap1.timeout.b);
+ chip->vendor.timeout_c = msecs_to_jiffies(cap1.timeout.c);
+ chip->vendor.timeout_d = msecs_to_jiffies(cap1.timeout.d);
+ chip->vendor.timeout_adjusted = true;
}
- /* Report adjusted timeouts */
+ be32_to_cpus(&cap2.duration.tpm_short);
+ be32_to_cpus(&cap2.duration.tpm_medium);
+ be32_to_cpus(&cap2.duration.tpm_long);
+ chip->vendor.duration[TPM_SHORT] =
+ usecs_to_jiffies(cap2.duration.tpm_short);
+ chip->vendor.duration[TPM_MEDIUM] =
+ usecs_to_jiffies(cap2.duration.tpm_medium);
+ chip->vendor.duration[TPM_LONG] =
+ usecs_to_jiffies(cap2.duration.tpm_long);
+
+ memcpy(&orig_vendor, &chip->vendor, sizeof(orig_vendor));
+
+ /* 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->vendor.duration[TPM_SHORT] =
+ msecs_to_jiffies(cap2.duration.tpm_short);
+ chip->vendor.duration[TPM_MEDIUM] =
+ msecs_to_jiffies(cap2.duration.tpm_medium);
+ chip->vendor.duration[TPM_LONG] =
+ msecs_to_jiffies(cap2.duration.tpm_long);
+ chip->vendor.duration_adjusted = true;
+ }
+
+ if (chip->ops->update_timeouts != NULL)
+ chip->ops->update_timeouts(chip);
+
if (chip->vendor.timeout_adjusted) {
dev_info(chip->pdev,
- 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]);
+ HW_ERR "Adjusted timeouts: A %u->%uus B %u->%uus"
+ " C %u->%uus D %u->%uus\n",
+ jiffies_to_usecs(orig_vendor.timeout_a),
+ jiffies_to_usecs(chip->vendor.timeout_a),
+ jiffies_to_usecs(orig_vendor.timeout_b),
+ jiffies_to_usecs(chip->vendor.timeout_b),
+ jiffies_to_usecs(orig_vendor.timeout_c),
+ jiffies_to_usecs(chip->vendor.timeout_c),
+ jiffies_to_usecs(orig_vendor.timeout_d),
+ jiffies_to_usecs(chip->vendor.timeout_d));
}
- chip->vendor.timeout_a = usecs_to_jiffies(new_timeout[0]);
- chip->vendor.timeout_b = usecs_to_jiffies(new_timeout[1]);
- chip->vendor.timeout_c = usecs_to_jiffies(new_timeout[2]);
- chip->vendor.timeout_d = usecs_to_jiffies(new_timeout[3]);
-
- rc = tpm_get_cap_prop(chip, TPM_CAP_PROP_TIS_DURATION, 3,
- &tpm_cmd.params.getcap_out.cap, "durations");
- if (rc)
- return rc;
-
- duration_cap = &tpm_cmd.params.getcap_out.cap.duration;
- chip->vendor.duration[TPM_SHORT] =
- usecs_to_jiffies(be32_to_cpu(duration_cap->tpm_short));
- chip->vendor.duration[TPM_MEDIUM] =
- usecs_to_jiffies(be32_to_cpu(duration_cap->tpm_medium));
- chip->vendor.duration[TPM_LONG] =
- usecs_to_jiffies(be32_to_cpu(duration_cap->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
- * 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->vendor.duration[TPM_SHORT] < (HZ / 100)) {
- chip->vendor.duration[TPM_SHORT] = HZ;
- chip->vendor.duration[TPM_MEDIUM] *= 1000;
- chip->vendor.duration[TPM_LONG] *= 1000;
- chip->vendor.duration_adjusted = true;
- dev_info(chip->pdev, "Adjusting TPM timeout parameters.");
+ if (chip->vendor.duration_adjusted) {
+ dev_info(chip->pdev,
+ HW_ERR "Adjusted durations: short %u->%uus"
+ " medium %u->%uus long %u->%uus\n",
+ jiffies_to_usecs(orig_vendor.duration[TPM_SHORT]),
+ jiffies_to_usecs(chip->vendor.duration[TPM_SHORT]),
+ jiffies_to_usecs(orig_vendor.duration[TPM_MEDIUM]),
+ jiffies_to_usecs(chip->vendor.duration[TPM_MEDIUM]),
+ jiffies_to_usecs(orig_vendor.duration[TPM_LONG]),
+ jiffies_to_usecs(chip->vendor.duration[TPM_LONG]));
}
+
return 0;
}
EXPORT_SYMBOL_GPL(tpm_get_timeouts);
diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index 088fa86..caf7278 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -475,34 +475,17 @@ 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)
{
- int i;
- u32 did_vid;
-
- did_vid = ioread32(chip->vendor.iobase + TPM_DID_VID(0));
-
- 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 (ioread32(chip->vendor.iobase + TPM_DID_VID(0))) {
+ case 0x32041114: /* Atmel 3204 */
+ chip->vendor.timeout_a = msecs_to_jiffies(TIS_SHORT_TIMEOUT);
+ chip->vendor.timeout_b = msecs_to_jiffies(TIS_LONG_TIMEOUT);
+ chip->vendor.timeout_c = msecs_to_jiffies(TIS_SHORT_TIMEOUT);
+ chip->vendor.timeout_d = msecs_to_jiffies(TIS_SHORT_TIMEOUT);
+ chip->vendor.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]
| From | Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> |
|---|---|
| Date | 2016-06-16 22:30 +0200 |
| Subject | Re: [PATCH v6 4/5] tpm: Allow TPM chip drivers to override reported command durations |
| Message-ID | <rKM5j-4rD-15@gated-at.bofh.it> |
| In reply to | #1419882 |
On Fri, Jun 10, 2016 at 06:55:06PM -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. > > Signed-off-by: Ed Swierk <eswierk@skyportsystems.com> Again, LGTM but have to test :) > --- > drivers/char/tpm/tpm-interface.c | 139 +++++++++++++++++++++------------------ > drivers/char/tpm/tpm_tis.c | 35 +++------- > include/linux/tpm.h | 3 +- > 3 files changed, 85 insertions(+), 92 deletions(-) /Jarkko
[toc] | [prev] | [next] | [standalone]
| From | Ed Swierk <eswierk@skyportsystems.com> |
|---|---|
| Date | 2016-06-11 04:00 +0200 |
| Subject | [PATCH v6 5/5] tpm_tis: Increase ST19NP18 TPM command duration to avoid chip lockup |
| Message-ID | <rIGnn-55z-19@gated-at.bofh.it> |
| In reply to | #1417943 |
The STMicro ST19NP18-TPM sometimes takes much longer to execute commands than it reports in its capabilities. For example, command 186 (TPM_FlushSpecific) has been observed to take 14560 msec to complete, far longer than the 3000 msec limit for "short" commands reported by the chip. The behavior has also been seen with command 101 (TPM_GetCapability). Worse, when the tpm_tis driver attempts to cancel the current command (by writing commandReady = 1 to TPM_STS_x), the chip locks up completely, returning all-1s from all memory-mapped register reads. The lockup can be cleared only by resetting the system. The occurrence of this excessive command duration depends on the sequence of commands preceding it. One sequence is creating at least 2 new keys via TPM_CreateWrapKey, then letting the TPM idle for at least 30 seconds, then loading a key via TPM_LoadKey2. The next TPM_FlushSpecific occasionally takes tens of seconds to complete. Another sequence is creating many keys in a row without pause. The TPM_CreateWrapKey operation gets much slower after the first few iterations, as one would expect when the pool of precomputed keys is exhausted. Then after a 35-second pause, the same TPM_LoadKey2 followed by TPM_FlushSpecific sequence triggers the behavior. Our working theory is that this older TPM sometimes pauses to precompute keys, which modern chips implement as a background process. Without access to the chip's implementation details it's impossible to know whether any commands are immune to being blocked by this process. So it seems safest to ignore the chip's reported command durations, and use a value much higher than any observed duration, like 180 sec (which is the duration this chip reports for "long" commands). Signed-off-by: Ed Swierk <eswierk@skyportsystems.com> --- drivers/char/tpm/tpm_tis.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c index caf7278..862c502 100644 --- a/drivers/char/tpm/tpm_tis.c +++ b/drivers/char/tpm/tpm_tis.c @@ -485,6 +485,11 @@ static void tpm_tis_update_timeouts(struct tpm_chip *chip) chip->vendor.timeout_d = msecs_to_jiffies(TIS_SHORT_TIMEOUT); chip->vendor.timeout_adjusted = true; break; + case 0x0000104a: /* STMicro ST19NP18-TPM */ + chip->vendor.duration[TPM_SHORT] = 180 * HZ; + chip->vendor.duration[TPM_MEDIUM] = 180 * HZ; + chip->vendor.duration[TPM_LONG] = 180 * HZ; + chip->vendor.duration_adjusted = true; } } -- 1.9.1
[toc] | [prev] | [next] | [standalone]
| From | Ed Swierk <eswierk@skyportsystems.com> |
|---|---|
| Date | 2016-06-11 04:00 +0200 |
| Subject | [PATCH v6 0/5] tpm: Command duration logging and chip-specific override |
| Message-ID | <rIGnn-55z-5@gated-at.bofh.it> |
| In reply to | #1417943 |
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: Factor out 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 | 192 ++++++++++++++++++++++----------------- drivers/char/tpm/tpm_tis.c | 48 +++++----- include/linux/tpm.h | 3 +- 3 files changed, 132 insertions(+), 111 deletions(-) -- 1.9.1
[toc] | [prev] | [next] | [standalone]
| From | Ed Swierk <eswierk@skyportsystems.com> |
|---|---|
| Date | 2016-06-11 04:00 +0200 |
| Subject | [PATCH v6 1/5] tpm_tis: Improve reporting of IO errors |
| Message-ID | <rIGno-55z-23@gated-at.bofh.it> |
| In reply to | #1419886 |
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.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index 65f7eec..088fa86 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -299,6 +299,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->pdev, "Response too long (wanted %zd, got %d)\n",
+ count, expected);
size = -EIO;
goto out;
}
@@ -366,6 +368,8 @@ static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t len)
&chip->vendor.int_queue, false);
status = tpm_tis_status(chip);
if (!itpm && (status & TPM_STS_DATA_EXPECT) == 0) {
+ dev_err(chip->pdev, "Chip not accepting %zd bytes\n",
+ len - count);
rc = -EIO;
goto out_err;
}
@@ -378,6 +382,7 @@ static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t len)
&chip->vendor.int_queue, false);
status = tpm_tis_status(chip);
if ((status & TPM_STS_DATA_EXPECT) != 0) {
+ dev_err(chip->pdev, "Chip not accepting last byte\n");
rc = -EIO;
goto out_err;
}
@@ -689,8 +694,9 @@ static int tpm_tis_init(struct device *dev, struct tpm_info *tpm_info,
vendor = ioread32(chip->vendor.iobase + TPM_DID_VID(0));
chip->vendor.manufacturer_id = vendor;
- 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",
+ chip->vendor.manufacturer_id,
vendor >> 16, ioread8(chip->vendor.iobase + TPM_RID(0)));
if (!itpm) {
--
1.9.1
[toc] | [prev] | [next] | [standalone]
| From | Ed Swierk <eswierk@skyportsystems.com> |
|---|---|
| Date | 2016-06-11 04:00 +0200 |
| Subject | [PATCH v6 3/5] tpm: Factor out reading of timeout and duration capabilities |
| Message-ID | <rIGno-55z-25@gated-at.bofh.it> |
| In reply to | #1419886 |
Factor sending the TPM_GetCapability command and validating the result
from tpm_get_timeouts() into a new function. 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 | 96 ++++++++++++++++++++++------------------
1 file changed, 52 insertions(+), 44 deletions(-)
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index cc1e5bc..4d1f62c 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -502,6 +502,52 @@ static int tpm_startup(struct tpm_chip *chip, __be16 startup_type)
"attempting to start the TPM");
}
+static int tpm_get_cap_prop(struct tpm_chip *chip, __be32 type, int size,
+ cap_t *cap, char *desc)
+{
+ struct tpm_cmd_t tpm_cmd;
+ ssize_t 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 = type;
+ rc = tpm_transmit_cmd(chip, &tpm_cmd, TPM_INTERNAL_RESULT_SIZE, NULL);
+
+ 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->pdev, "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 = type;
+ rc = tpm_transmit_cmd(chip, &tpm_cmd, TPM_INTERNAL_RESULT_SIZE,
+ NULL);
+ }
+
+ if (rc) {
+ dev_err(chip->pdev,
+ "Error %zd reading %s\n", rc, desc);
+ 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) + size * sizeof(u32)) {
+ dev_err(chip->pdev,
+ "Bad return code or length reading %s\n", desc);
+ return -EINVAL;
+ }
+
+ memcpy(cap, &tpm_cmd.params.getcap_out.cap, sizeof(cap_t));
+
+ return 0;
+}
+
int tpm_get_timeouts(struct tpm_chip *chip)
{
struct tpm_cmd_t tpm_cmd;
@@ -510,37 +556,10 @@ int tpm_get_timeouts(struct tpm_chip *chip)
struct duration_t *duration_cap;
ssize_t 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 == TPM_ERR_INVALID_POSTINIT) {
- /* The TPM is not started, we are the first to talk to it.
- Execute a startup command. */
- dev_info(chip->pdev, "Issuing TPM_STARTUP");
- 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->pdev,
- "A TPM error (%zd) occurred attempting to determine the timeouts\n",
- rc);
- goto duration;
- }
-
- 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;
+ rc = tpm_get_cap_prop(chip, TPM_CAP_PROP_TIS_TIMEOUT, 4,
+ &tpm_cmd.params.getcap_out.cap, "timeouts");
+ if (rc)
+ return rc;
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);
@@ -583,22 +602,11 @@ int tpm_get_timeouts(struct tpm_chip *chip)
chip->vendor.timeout_c = usecs_to_jiffies(new_timeout[2]);
chip->vendor.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_get_cap_prop(chip, TPM_CAP_PROP_TIS_DURATION, 3,
+ &tpm_cmd.params.getcap_out.cap, "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->vendor.duration[TPM_SHORT] =
usecs_to_jiffies(be32_to_cpu(duration_cap->tpm_short));
--
1.9.1
[toc] | [prev] | [next] | [standalone]
| From | Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> |
|---|---|
| Date | 2016-06-16 22:30 +0200 |
| Subject | Re: [PATCH v6 3/5] tpm: Factor out reading of timeout and duration capabilities |
| Message-ID | <rKM5j-4rD-11@gated-at.bofh.it> |
| In reply to | #1419888 |
On Fri, Jun 10, 2016 at 06:55:05PM -0700, Ed Swierk wrote: > Factor sending the TPM_GetCapability command and validating the result > from tpm_get_timeouts() into a new function. Return all errors to the > caller rather than swallowing them (e.g. when tpm_transmit_cmd() > returns nonzero). LGTM but I have to test this on next week. I'll give final Reviewed/Tested-by after that. Thanks for the good work. > Signed-off-by: Ed Swierk <eswierk@skyportsystems.com> > --- > drivers/char/tpm/tpm-interface.c | 96 ++++++++++++++++++++++------------------ > 1 file changed, 52 insertions(+), 44 deletions(-) /Jarkko
[toc] | [prev] | [next] | [standalone]
| From | Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> |
|---|---|
| Date | 2016-06-19 14:20 +0200 |
| Subject | Re: [PATCH v6 3/5] tpm: Factor out reading of timeout and duration capabilities |
| Message-ID | <rLJRL-2fZ-9@gated-at.bofh.it> |
| In reply to | #1419888 |
On Fri, Jun 10, 2016 at 06:55:05PM -0700, Ed Swierk wrote:
> Factor sending the TPM_GetCapability command and validating the result
> from tpm_get_timeouts() into a new function. 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 | 96 ++++++++++++++++++++++------------------
> 1 file changed, 52 insertions(+), 44 deletions(-)
I'm sorry but just now that I started applying these patches this patch
started to bother me.
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index cc1e5bc..4d1f62c 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -502,6 +502,52 @@ static int tpm_startup(struct tpm_chip *chip, __be16 startup_type)
> "attempting to start the TPM");
> }
>
> +static int tpm_get_cap_prop(struct tpm_chip *chip, __be32 type, int size,
> + cap_t *cap, char *desc)
> +{
> + struct tpm_cmd_t tpm_cmd;
> + ssize_t 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 = type;
> + rc = tpm_transmit_cmd(chip, &tpm_cmd, TPM_INTERNAL_RESULT_SIZE, NULL);
> +
> + 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->pdev, "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 = type;
> + rc = tpm_transmit_cmd(chip, &tpm_cmd, TPM_INTERNAL_RESULT_SIZE,
> + NULL);
> + }
I think inside tpm_get_timeouts() I'd rather something along the lines
(with error handling and such details taken away):
rc = tpm_getcap(...);
if (rc == TPM_ERR_INVALID_POSTINIT) {
tpm_startup(...);
tpm_getca(...);
}
> + if (rc) {
> + dev_err(chip->pdev,
> + "Error %zd reading %s\n", rc, desc);
> + 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) + size * sizeof(u32)) {
> + dev_err(chip->pdev,
> + "Bad return code or length reading %s\n", desc);
> + return -EINVAL;
> + }
This is bogus code. All this kind of checks should be contained in
tpm_transmit_cmd(). This is easily "fixed" by moving tpm_getcap() :)
/Jarkko
[toc] | [prev] | [next] | [standalone]
Page 1 of 2 [1] 2 Next page →
Back to top | Article view | linux.kernel
csiph-web