[PATCH REVIEW 1/2] tpm: provide a way to override the chip returned durations

[PATCH REVIEW 1/2] tpm: provide a way to override the chip returned durations

[Alexey Klimov]

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

[PATCH REVIEW 2/2] tpm_tis: override durations for STM tpm with firmware 1.2.8.28

[Alexey Klimov]

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

Re: [PATCH REVIEW 1/2] tpm: provide a way to override the chip returned durations

[Jarkko Sakkinen]

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

Re: [PATCH REVIEW 2/2] tpm_tis: override durations for STM tpm with firmware 1.2.8.28

[Jarkko Sakkinen]

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

Re: [PATCH REVIEW 2/2] tpm_tis: override durations for STM tpm with firmware 1.2.8.28

[Jerry Snitselaar]

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

Re: [PATCH REVIEW 2/2] tpm_tis: override durations for STM tpm with firmware 1.2.8.28

[Jarkko Sakkinen]

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

Re: [PATCH REVIEW 2/2] tpm_tis: override durations for STM tpm with firmware 1.2.8.28

[Jerry Snitselaar]

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

Re: [PATCH REVIEW 2/2] tpm_tis: override durations for STM tpm with firmware 1.2.8.28

[Jarkko Sakkinen]

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