* [PATCH REVIEW 1/2] tpm: provide a way to override the chip returned durations
@ 2018-12-14 13:21 Alexey Klimov
2018-12-14 13:21 ` [PATCH REVIEW 2/2] tpm_tis: override durations for STM tpm with firmware 1.2.8.28 Alexey Klimov
2019-01-03 13:05 ` [PATCH REVIEW 1/2] tpm: provide a way to override the chip returned durations Jarkko Sakkinen
0 siblings, 2 replies; 8+ messages in thread
From: Alexey Klimov @ 2018-12-14 13:21 UTC (permalink / raw)
To: linux-integrity; +Cc: jsnitsel, jarkko.sakkinen, peterhuewe, jgg, aklimov
Patch adds method ->update_durations to override returned
durations in case TPM chip misbehaves for TPM 1.2 drivers.
Cc: Jerry Snitselaar <jsnitsel@redhat.com>
Signed-off-by: Alexey Klimov <aklimov@redhat.com>
---
drivers/char/tpm/tpm-interface.c | 18 ++++++++++++++++++
include/linux/tpm.h | 2 ++
2 files changed, 20 insertions(+)
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 129f640424b7..5cb90918938d 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -768,6 +768,7 @@ int tpm_get_timeouts(struct tpm_chip *chip)
{
cap_t cap;
unsigned long timeout_old[4], timeout_chip[4], timeout_eff[4];
+ unsigned long durations[3];
ssize_t rc;
if (chip->flags & TPM_CHIP_FLAG_HAVE_TIMEOUTS)
@@ -877,6 +878,23 @@ int tpm_get_timeouts(struct tpm_chip *chip)
usecs_to_jiffies(be32_to_cpu(cap.duration.tpm_long));
chip->duration[TPM_LONG_LONG] = 0; /* not used under 1.2 */
+ /*
+ * Provide ability for vendor overrides of duration values in case
+ * of misreporting.
+ */
+ if (chip->ops->update_durations != NULL)
+ chip->duration_adjusted =
+ chip->ops->update_durations(chip, durations);
+
+ /* Report and set adjusted timeouts */
+ if (chip->duration_adjusted) {
+
+ dev_info(&chip->dev, HW_ERR "Adjusting reported durations\n");
+ chip->duration[TPM_SHORT] = durations[0];
+ chip->duration[TPM_MEDIUM] = durations[1];
+ chip->duration[TPM_LONG] = durations[2];
+ }
+
/* 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.
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index 4609b94142d4..d6018431b478 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -43,6 +43,8 @@ struct tpm_class_ops {
u8 (*status) (struct tpm_chip *chip);
bool (*update_timeouts)(struct tpm_chip *chip,
unsigned long *timeout_cap);
+ bool (*update_durations)(struct tpm_chip *chip,
+ unsigned long *durations_cap);
int (*go_idle)(struct tpm_chip *chip);
int (*cmd_ready)(struct tpm_chip *chip);
int (*request_locality)(struct tpm_chip *chip, int loc);
--
2.14.4
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH REVIEW 2/2] tpm_tis: override durations for STM tpm with firmware 1.2.8.28
2018-12-14 13:21 [PATCH REVIEW 1/2] tpm: provide a way to override the chip returned durations Alexey Klimov
@ 2018-12-14 13:21 ` Alexey Klimov
2019-01-03 13:14 ` Jarkko Sakkinen
2019-01-03 13:05 ` [PATCH REVIEW 1/2] tpm: provide a way to override the chip returned durations Jarkko Sakkinen
1 sibling, 1 reply; 8+ messages in thread
From: Alexey Klimov @ 2018-12-14 13:21 UTC (permalink / raw)
To: linux-integrity; +Cc: jsnitsel, jarkko.sakkinen, peterhuewe, jgg, aklimov
There was revealed a bug in the STM TPM chipset used in Dell R415s.
Bug is observed so far only on chipset firmware 1.2.8.28
(1.2 TPM, device-id 0x0, rev-id 78). After some number of
operations chipset hangs and stays in inconsistent state:
tpm_tis 00:09: Operation Timed out
tpm_tis 00:09: tpm_transmit: tpm_send: error -5
Durations returned by the chip are the same like on other
firmware revisions but apparently with specifically 1.2.8.28 fw
durations should be reset to 2 minutes to enable tpm chip work
properly. No working way of updating firmware was found.
This patch adds implementation of ->update_durations method
that matches only STM devices with specific firmware version.
Cc: Jerry Snitselaar <jsnitsel@redhat.com>
Signed-off-by: Alexey Klimov <aklimov@redhat.com>
---
drivers/char/tpm/tpm_tis_core.c | 90 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 90 insertions(+)
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index d2345d9fd7b5..e0bdca647460 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -514,6 +514,95 @@ static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
return rc;
}
+struct tis_vendor_durations_override {
+ u32 did_vid;
+ struct tpm_version_t tpm_version;
+ unsigned long durs[3];
+};
+
+static
+const struct tis_vendor_durations_override vendor_dur_overrides[] = {
+ /* STMicroelectronics 0x104a */
+ { 0x0000104A,
+ { 1, 2, 8, 28 },
+ { (2 * 60 * HZ), (2 * 60 * HZ), (2 * 60 * HZ) } },
+};
+
+static bool tpm_tis_update_durations(struct tpm_chip *chip,
+ unsigned long *duration_cap)
+{
+ struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
+ u32 did_vid;
+ int i, rc;
+ cap_t cap;
+
+ if (chip->ops->clk_enable != NULL)
+ chip->ops->clk_enable(chip, true);
+
+ rc = tpm_tis_read32(priv, TPM_DID_VID(0), &did_vid);
+ if (rc < 0)
+ goto out;
+
+ for (i = 0; i != ARRAY_SIZE(vendor_dur_overrides); i++) {
+ if (vendor_dur_overrides[i].did_vid != did_vid)
+ continue;
+
+ /* Try to get a TPM version 1.2 TPM_CAP_VERSION_INFO */
+ rc = tpm_getcap(chip, TPM_CAP_VERSION_1_2, &cap,
+ "attempting to determine the 1.2 version",
+ sizeof(cap.tpm_version_1_2));
+ if (!rc) {
+ if ((cap.tpm_version_1_2.Major ==
+ vendor_dur_overrides[i].tpm_version.Major) &&
+ (cap.tpm_version_1_2.Minor ==
+ vendor_dur_overrides[i].tpm_version.Minor) &&
+ (cap.tpm_version_1_2.revMajor ==
+ vendor_dur_overrides[i].tpm_version.revMajor) &&
+ (cap.tpm_version_1_2.revMinor ==
+ vendor_dur_overrides[i].tpm_version.revMinor)) {
+
+ memcpy(duration_cap,
+ vendor_dur_overrides[i].durs,
+ sizeof(vendor_dur_overrides[i].durs));
+ rc = true;
+ goto out;
+ }
+ } else {
+ rc = tpm_getcap(chip, TPM_CAP_VERSION_1_1, &cap,
+ "attempting to determine the 1.1 version",
+ sizeof(cap.tpm_version));
+ if (rc) {
+ rc = false;
+ goto out;
+ }
+ if ((cap.tpm_version.Major ==
+ vendor_dur_overrides[i].tpm_version.Major) &&
+ (cap.tpm_version.Minor ==
+ vendor_dur_overrides[i].tpm_version.Minor) &&
+ (cap.tpm_version.revMajor ==
+ vendor_dur_overrides[i].tpm_version.revMajor) &&
+ (cap.tpm_version.revMinor ==
+ vendor_dur_overrides[i].tpm_version.revMinor)) {
+
+ memcpy(duration_cap,
+ vendor_dur_overrides[i].durs,
+ sizeof(vendor_dur_overrides[i].durs));
+ rc = true;
+ goto out;
+ }
+ }
+ }
+
+ rc = false;
+
+out:
+ if (chip->ops->clk_enable != NULL)
+ chip->ops->clk_enable(chip, false);
+
+ return rc;
+}
+
+
struct tis_vendor_timeout_override {
u32 did_vid;
unsigned long timeout_us[4];
@@ -847,6 +936,7 @@ static const struct tpm_class_ops tpm_tis = {
.send = tpm_tis_send,
.cancel = tpm_tis_ready,
.update_timeouts = tpm_tis_update_timeouts,
+ .update_durations = tpm_tis_update_durations,
.req_complete_mask = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
.req_complete_val = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
.req_canceled = tpm_tis_req_canceled,
--
2.14.4
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH REVIEW 1/2] tpm: provide a way to override the chip returned durations
2018-12-14 13:21 [PATCH REVIEW 1/2] tpm: provide a way to override the chip returned durations Alexey Klimov
2018-12-14 13:21 ` [PATCH REVIEW 2/2] tpm_tis: override durations for STM tpm with firmware 1.2.8.28 Alexey Klimov
@ 2019-01-03 13:05 ` Jarkko Sakkinen
1 sibling, 0 replies; 8+ messages in thread
From: Jarkko Sakkinen @ 2019-01-03 13:05 UTC (permalink / raw)
To: Alexey Klimov; +Cc: linux-integrity, jsnitsel, peterhuewe, jgg
On Fri, Dec 14, 2018 at 01:21:14PM +0000, Alexey Klimov wrote:
> Patch adds method ->update_durations to override returned
> durations in case TPM chip misbehaves for TPM 1.2 drivers.
>
> Cc: Jerry Snitselaar <jsnitsel@redhat.com>
> Signed-off-by: Alexey Klimov <aklimov@redhat.com>
> ---
> drivers/char/tpm/tpm-interface.c | 18 ++++++++++++++++++
> include/linux/tpm.h | 2 ++
> 2 files changed, 20 insertions(+)
>
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index 129f640424b7..5cb90918938d 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -768,6 +768,7 @@ int tpm_get_timeouts(struct tpm_chip *chip)
> {
> cap_t cap;
> unsigned long timeout_old[4], timeout_chip[4], timeout_eff[4];
> + unsigned long durations[3];
> ssize_t rc;
>
> if (chip->flags & TPM_CHIP_FLAG_HAVE_TIMEOUTS)
> @@ -877,6 +878,23 @@ int tpm_get_timeouts(struct tpm_chip *chip)
> usecs_to_jiffies(be32_to_cpu(cap.duration.tpm_long));
> chip->duration[TPM_LONG_LONG] = 0; /* not used under 1.2 */
>
> + /*
> + * Provide ability for vendor overrides of duration values in case
> + * of misreporting.
> + */
> + if (chip->ops->update_durations != NULL)
> + chip->duration_adjusted =
> + chip->ops->update_durations(chip, durations);
> +
> + /* Report and set adjusted timeouts */
> + if (chip->duration_adjusted) {
> +
Can you remove the extra newline?
> + dev_info(&chip->dev, HW_ERR "Adjusting reported durations\n");
> + chip->duration[TPM_SHORT] = durations[0];
> + chip->duration[TPM_MEDIUM] = durations[1];
> + chip->duration[TPM_LONG] = durations[2];
> + }
> +
> /* 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.
> diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> index 4609b94142d4..d6018431b478 100644
> --- a/include/linux/tpm.h
> +++ b/include/linux/tpm.h
> @@ -43,6 +43,8 @@ struct tpm_class_ops {
> u8 (*status) (struct tpm_chip *chip);
> bool (*update_timeouts)(struct tpm_chip *chip,
> unsigned long *timeout_cap);
> + bool (*update_durations)(struct tpm_chip *chip,
> + unsigned long *durations_cap);
> int (*go_idle)(struct tpm_chip *chip);
> int (*cmd_ready)(struct tpm_chip *chip);
> int (*request_locality)(struct tpm_chip *chip, int loc);
> --
> 2.14.4
>
/Jarkko
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH REVIEW 2/2] tpm_tis: override durations for STM tpm with firmware 1.2.8.28
2018-12-14 13:21 ` [PATCH REVIEW 2/2] tpm_tis: override durations for STM tpm with firmware 1.2.8.28 Alexey Klimov
@ 2019-01-03 13:14 ` Jarkko Sakkinen
2019-01-14 19:39 ` Jerry Snitselaar
0 siblings, 1 reply; 8+ messages in thread
From: Jarkko Sakkinen @ 2019-01-03 13:14 UTC (permalink / raw)
To: Alexey Klimov; +Cc: linux-integrity, jsnitsel, peterhuewe, jgg
On Fri, Dec 14, 2018 at 01:21:15PM +0000, Alexey Klimov wrote:
> There was revealed a bug in the STM TPM chipset used in Dell R415s.
> Bug is observed so far only on chipset firmware 1.2.8.28
> (1.2 TPM, device-id 0x0, rev-id 78). After some number of
> operations chipset hangs and stays in inconsistent state:
>
> tpm_tis 00:09: Operation Timed out
> tpm_tis 00:09: tpm_transmit: tpm_send: error -5
>
> Durations returned by the chip are the same like on other
> firmware revisions but apparently with specifically 1.2.8.28 fw
> durations should be reset to 2 minutes to enable tpm chip work
> properly. No working way of updating firmware was found.
>
> This patch adds implementation of ->update_durations method
> that matches only STM devices with specific firmware version.
>
> Cc: Jerry Snitselaar <jsnitsel@redhat.com>
> Signed-off-by: Alexey Klimov <aklimov@redhat.com>
> ---
> drivers/char/tpm/tpm_tis_core.c | 90 +++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 90 insertions(+)
>
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index d2345d9fd7b5..e0bdca647460 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -514,6 +514,95 @@ static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
> return rc;
> }
>
> +struct tis_vendor_durations_override {
> + u32 did_vid;
> + struct tpm_version_t tpm_version;
> + unsigned long durs[3];
I would rather have just "unsigned long durations[3];".
> +};
> +
> +static
> +const struct tis_vendor_durations_override vendor_dur_overrides[] = {
> + /* STMicroelectronics 0x104a */
> + { 0x0000104A,
> + { 1, 2, 8, 28 },
> + { (2 * 60 * HZ), (2 * 60 * HZ), (2 * 60 * HZ) } },
> +};
> +
> +static bool tpm_tis_update_durations(struct tpm_chip *chip,
> + unsigned long *duration_cap)
> +{
> + struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
> + u32 did_vid;
> + int i, rc;
> + cap_t cap;
> +
> + if (chip->ops->clk_enable != NULL)
> + chip->ops->clk_enable(chip, true);
> +
> + rc = tpm_tis_read32(priv, TPM_DID_VID(0), &did_vid);
> + if (rc < 0)
> + goto out;
> +
> + for (i = 0; i != ARRAY_SIZE(vendor_dur_overrides); i++) {
> + if (vendor_dur_overrides[i].did_vid != did_vid)
> + continue;
> +
> + /* Try to get a TPM version 1.2 TPM_CAP_VERSION_INFO */
> + rc = tpm_getcap(chip, TPM_CAP_VERSION_1_2, &cap,
> + "attempting to determine the 1.2 version",
> + sizeof(cap.tpm_version_1_2));
Not properly aligned.
> + if (!rc) {
> + if ((cap.tpm_version_1_2.Major ==
> + vendor_dur_overrides[i].tpm_version.Major) &&
> + (cap.tpm_version_1_2.Minor ==
> + vendor_dur_overrides[i].tpm_version.Minor) &&
> + (cap.tpm_version_1_2.revMajor ==
> + vendor_dur_overrides[i].tpm_version.revMajor) &&
> + (cap.tpm_version_1_2.revMinor ==
> + vendor_dur_overrides[i].tpm_version.revMinor)) {
Same.
> +
> + memcpy(duration_cap,
> + vendor_dur_overrides[i].durs,
> + sizeof(vendor_dur_overrides[i].durs));
Same.
> + rc = true;
> + goto out;
> + }
> + } else {
> + rc = tpm_getcap(chip, TPM_CAP_VERSION_1_1, &cap,
> + "attempting to determine the 1.1 version",
> + sizeof(cap.tpm_version));
Same.
> + if (rc) {
> + rc = false;
> + goto out;
> + }
> + if ((cap.tpm_version.Major ==
> + vendor_dur_overrides[i].tpm_version.Major) &&
> + (cap.tpm_version.Minor ==
> + vendor_dur_overrides[i].tpm_version.Minor) &&
> + (cap.tpm_version.revMajor ==
> + vendor_dur_overrides[i].tpm_version.revMajor) &&
> + (cap.tpm_version.revMinor ==
> + vendor_dur_overrides[i].tpm_version.revMinor)) {
Same.
> +
> + memcpy(duration_cap,
> + vendor_dur_overrides[i].durs,
> + sizeof(vendor_dur_overrides[i].durs));
Same.
> + rc = true;
> + goto out;
> + }
> + }
> + }
> +
> + rc = false;
Shoud return proper rc instead of bool.
> +
> +out:
> + if (chip->ops->clk_enable != NULL)
> + chip->ops->clk_enable(chip, false);
> +
> + return rc;
> +}
> +
> +
> struct tis_vendor_timeout_override {
> u32 did_vid;
> unsigned long timeout_us[4];
> @@ -847,6 +936,7 @@ static const struct tpm_class_ops tpm_tis = {
> .send = tpm_tis_send,
> .cancel = tpm_tis_ready,
> .update_timeouts = tpm_tis_update_timeouts,
> + .update_durations = tpm_tis_update_durations,
> .req_complete_mask = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
> .req_complete_val = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
> .req_canceled = tpm_tis_req_canceled,
> --
> 2.14.4
>
/Jarkko
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH REVIEW 2/2] tpm_tis: override durations for STM tpm with firmware 1.2.8.28
2019-01-03 13:14 ` Jarkko Sakkinen
@ 2019-01-14 19:39 ` Jerry Snitselaar
2019-01-18 14:59 ` Jarkko Sakkinen
0 siblings, 1 reply; 8+ messages in thread
From: Jerry Snitselaar @ 2019-01-14 19:39 UTC (permalink / raw)
To: Jarkko Sakkinen; +Cc: Alexey Klimov, linux-integrity, peterhuewe, jgg
On Thu Jan 03 19, Jarkko Sakkinen wrote:
>On Fri, Dec 14, 2018 at 01:21:15PM +0000, Alexey Klimov wrote:
>> There was revealed a bug in the STM TPM chipset used in Dell R415s.
>> Bug is observed so far only on chipset firmware 1.2.8.28
>> (1.2 TPM, device-id 0x0, rev-id 78). After some number of
>> operations chipset hangs and stays in inconsistent state:
>>
>> tpm_tis 00:09: Operation Timed out
>> tpm_tis 00:09: tpm_transmit: tpm_send: error -5
>>
>> Durations returned by the chip are the same like on other
>> firmware revisions but apparently with specifically 1.2.8.28 fw
>> durations should be reset to 2 minutes to enable tpm chip work
>> properly. No working way of updating firmware was found.
>>
>> This patch adds implementation of ->update_durations method
>> that matches only STM devices with specific firmware version.
>>
>> Cc: Jerry Snitselaar <jsnitsel@redhat.com>
>> Signed-off-by: Alexey Klimov <aklimov@redhat.com>
>> ---
>> drivers/char/tpm/tpm_tis_core.c | 90 +++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 90 insertions(+)
>>
>> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
>> index d2345d9fd7b5..e0bdca647460 100644
>> --- a/drivers/char/tpm/tpm_tis_core.c
>> +++ b/drivers/char/tpm/tpm_tis_core.c
>> @@ -514,6 +514,95 @@ static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
>> return rc;
>> }
>>
>> +struct tis_vendor_durations_override {
>> + u32 did_vid;
>> + struct tpm_version_t tpm_version;
>> + unsigned long durs[3];
>
>I would rather have just "unsigned long durations[3];".
>
>> +};
>> +
>> +static
>> +const struct tis_vendor_durations_override vendor_dur_overrides[] = {
>> + /* STMicroelectronics 0x104a */
>> + { 0x0000104A,
>> + { 1, 2, 8, 28 },
>> + { (2 * 60 * HZ), (2 * 60 * HZ), (2 * 60 * HZ) } },
>> +};
>> +
>> +static bool tpm_tis_update_durations(struct tpm_chip *chip,
>> + unsigned long *duration_cap)
>> +{
>> + struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
>> + u32 did_vid;
>> + int i, rc;
>> + cap_t cap;
>> +
>> + if (chip->ops->clk_enable != NULL)
>> + chip->ops->clk_enable(chip, true);
>> +
>> + rc = tpm_tis_read32(priv, TPM_DID_VID(0), &did_vid);
>> + if (rc < 0)
>> + goto out;
>> +
>> + for (i = 0; i != ARRAY_SIZE(vendor_dur_overrides); i++) {
>> + if (vendor_dur_overrides[i].did_vid != did_vid)
>> + continue;
>> +
>> + /* Try to get a TPM version 1.2 TPM_CAP_VERSION_INFO */
>> + rc = tpm_getcap(chip, TPM_CAP_VERSION_1_2, &cap,
>> + "attempting to determine the 1.2 version",
>> + sizeof(cap.tpm_version_1_2));
>
>Not properly aligned.
>
>> + if (!rc) {
>> + if ((cap.tpm_version_1_2.Major ==
>> + vendor_dur_overrides[i].tpm_version.Major) &&
>> + (cap.tpm_version_1_2.Minor ==
>> + vendor_dur_overrides[i].tpm_version.Minor) &&
>> + (cap.tpm_version_1_2.revMajor ==
>> + vendor_dur_overrides[i].tpm_version.revMajor) &&
>> + (cap.tpm_version_1_2.revMinor ==
>> + vendor_dur_overrides[i].tpm_version.revMinor)) {
>
>Same.
>
>> +
>> + memcpy(duration_cap,
>> + vendor_dur_overrides[i].durs,
>> + sizeof(vendor_dur_overrides[i].durs));
>
>Same.
>
>> + rc = true;
>> + goto out;
>> + }
>> + } else {
>> + rc = tpm_getcap(chip, TPM_CAP_VERSION_1_1, &cap,
>> + "attempting to determine the 1.1 version",
>> + sizeof(cap.tpm_version));
>
>Same.
>
>> + if (rc) {
>> + rc = false;
>> + goto out;
>> + }
>> + if ((cap.tpm_version.Major ==
>> + vendor_dur_overrides[i].tpm_version.Major) &&
>> + (cap.tpm_version.Minor ==
>> + vendor_dur_overrides[i].tpm_version.Minor) &&
>> + (cap.tpm_version.revMajor ==
>> + vendor_dur_overrides[i].tpm_version.revMajor) &&
>> + (cap.tpm_version.revMinor ==
>> + vendor_dur_overrides[i].tpm_version.revMinor)) {
>
>Same.
>
>> +
>> + memcpy(duration_cap,
>> + vendor_dur_overrides[i].durs,
>> + sizeof(vendor_dur_overrides[i].durs));
>
>Same.
>
>> + rc = true;
>> + goto out;
>> + }
>> + }
>> + }
>> +
>> + rc = false;
>
>Shoud return proper rc instead of bool.
>
Alexey was following the example of tpm_tis_update_timeouts() which
returns true if the timeouts were updated, and otherwise returns
false. The bool here makes sense to me, but what rc would you suggest
in this case?
Regards,
Jerry
>> +
>> +out:
>> + if (chip->ops->clk_enable != NULL)
>> + chip->ops->clk_enable(chip, false);
>> +
>> + return rc;
>> +}
>> +
>> +
>> struct tis_vendor_timeout_override {
>> u32 did_vid;
>> unsigned long timeout_us[4];
>> @@ -847,6 +936,7 @@ static const struct tpm_class_ops tpm_tis = {
>> .send = tpm_tis_send,
>> .cancel = tpm_tis_ready,
>> .update_timeouts = tpm_tis_update_timeouts,
>> + .update_durations = tpm_tis_update_durations,
>> .req_complete_mask = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
>> .req_complete_val = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
>> .req_canceled = tpm_tis_req_canceled,
>> --
>> 2.14.4
>>
>
>/Jarkko
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH REVIEW 2/2] tpm_tis: override durations for STM tpm with firmware 1.2.8.28
2019-01-14 19:39 ` Jerry Snitselaar
@ 2019-01-18 14:59 ` Jarkko Sakkinen
2019-01-20 21:30 ` Jerry Snitselaar
0 siblings, 1 reply; 8+ messages in thread
From: Jarkko Sakkinen @ 2019-01-18 14:59 UTC (permalink / raw)
To: Jerry Snitselaar; +Cc: Alexey Klimov, linux-integrity, peterhuewe, jgg
On Mon, Jan 14, 2019 at 12:39:40PM -0700, Jerry Snitselaar wrote:
> Alexey was following the example of tpm_tis_update_timeouts() which
> returns true if the timeouts were updated, and otherwise returns
> false. The bool here makes sense to me, but what rc would you suggest
> in this case?
Maybe the pattern used there is not that great then.
The callback should simply be update_durations(chip), and it would do
whatever updates needed and either return zero or -errno. And of course
update durations_adjusted flag because that is needed in sysfs.
/Jarkko
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH REVIEW 2/2] tpm_tis: override durations for STM tpm with firmware 1.2.8.28
2019-01-18 14:59 ` Jarkko Sakkinen
@ 2019-01-20 21:30 ` Jerry Snitselaar
2019-01-21 12:29 ` Jarkko Sakkinen
0 siblings, 1 reply; 8+ messages in thread
From: Jerry Snitselaar @ 2019-01-20 21:30 UTC (permalink / raw)
To: Jarkko Sakkinen; +Cc: Alexey Klimov, linux-integrity, peterhuewe, jgg
On Fri Jan 18 19, Jarkko Sakkinen wrote:
>On Mon, Jan 14, 2019 at 12:39:40PM -0700, Jerry Snitselaar wrote:
>> Alexey was following the example of tpm_tis_update_timeouts() which
>> returns true if the timeouts were updated, and otherwise returns
>> false. The bool here makes sense to me, but what rc would you suggest
>> in this case?
>
>Maybe the pattern used there is not that great then.
>
>The callback should simply be update_durations(chip), and it would do
>whatever updates needed and either return zero or -errno. And of course
>update durations_adjusted flag because that is needed in sysfs.
>
>/Jarkko
Taking a quick look, they already track whether the adjustment
occurred in the tpm_chip struct, so that could be used instead for
what the bool return was being used for. I'll post a patch for the
timeout updates code, and work with Alexey to rework his patchset.
Regards,
Jerry
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH REVIEW 2/2] tpm_tis: override durations for STM tpm with firmware 1.2.8.28
2019-01-20 21:30 ` Jerry Snitselaar
@ 2019-01-21 12:29 ` Jarkko Sakkinen
0 siblings, 0 replies; 8+ messages in thread
From: Jarkko Sakkinen @ 2019-01-21 12:29 UTC (permalink / raw)
To: Jerry Snitselaar; +Cc: Alexey Klimov, linux-integrity, peterhuewe, jgg
On Sun, Jan 20, 2019 at 02:30:12PM -0700, Jerry Snitselaar wrote:
> On Fri Jan 18 19, Jarkko Sakkinen wrote:
> > On Mon, Jan 14, 2019 at 12:39:40PM -0700, Jerry Snitselaar wrote:
> > > Alexey was following the example of tpm_tis_update_timeouts() which
> > > returns true if the timeouts were updated, and otherwise returns
> > > false. The bool here makes sense to me, but what rc would you suggest
> > > in this case?
> >
> > Maybe the pattern used there is not that great then.
> >
> > The callback should simply be update_durations(chip), and it would do
> > whatever updates needed and either return zero or -errno. And of course
> > update durations_adjusted flag because that is needed in sysfs.
> >
> > /Jarkko
>
> Taking a quick look, they already track whether the adjustment
> occurred in the tpm_chip struct, so that could be used instead for
> what the bool return was being used for. I'll post a patch for the
> timeout updates code, and work with Alexey to rework his patchset.
Awesome, thank you!
/Jarkko
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-01-21 12:29 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-14 13:21 [PATCH REVIEW 1/2] tpm: provide a way to override the chip returned durations Alexey Klimov
2018-12-14 13:21 ` [PATCH REVIEW 2/2] tpm_tis: override durations for STM tpm with firmware 1.2.8.28 Alexey Klimov
2019-01-03 13:14 ` Jarkko Sakkinen
2019-01-14 19:39 ` Jerry Snitselaar
2019-01-18 14:59 ` Jarkko Sakkinen
2019-01-20 21:30 ` Jerry Snitselaar
2019-01-21 12:29 ` Jarkko Sakkinen
2019-01-03 13:05 ` [PATCH REVIEW 1/2] tpm: provide a way to override the chip returned durations Jarkko Sakkinen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).