* [RFC PATCH 1/1] tpm: don't return bool from update_timeouts
2019-01-29 17:41 [RFC PATCH 0/1] tpm: don't return bool from update_timeouts Jerry Snitselaar
@ 2019-01-29 17:41 ` Jerry Snitselaar
2019-01-29 19:22 ` Jarkko Sakkinen
2019-01-29 18:31 ` [RFC PATCH 0/1] " Jarkko Sakkinen
2019-01-29 20:14 ` [RFC PATCH v2 1/1] " Jerry Snitselaar
2 siblings, 1 reply; 9+ messages in thread
From: Jerry Snitselaar @ 2019-01-29 17:41 UTC (permalink / raw)
To: Jarkko Sakkinen, linux-integrity; +Cc: Jerry Snitselaar
Set tpm_chip->timeouts_adjusted directly in the update_timeouts
code instead of returning bool. Return rc of tpm read instead,
which can be return an error in the case of the spi driver.
Signed-off-by: Jerry Snitselaar <jsnitsel@redhat.com>
---
drivers/char/tpm/tpm1-cmd.c | 3 +--
drivers/char/tpm/tpm_tis_core.c | 8 ++++----
include/linux/tpm.h | 2 +-
3 files changed, 6 insertions(+), 7 deletions(-)
diff --git a/drivers/char/tpm/tpm1-cmd.c b/drivers/char/tpm/tpm1-cmd.c
index 6f306338953b..bda9a16b44f6 100644
--- a/drivers/char/tpm/tpm1-cmd.c
+++ b/drivers/char/tpm/tpm1-cmd.c
@@ -380,8 +380,7 @@ int tpm1_get_timeouts(struct tpm_chip *chip)
* of misreporting.
*/
if (chip->ops->update_timeouts)
- chip->timeout_adjusted =
- chip->ops->update_timeouts(chip, timeout_eff);
+ chip->ops->update_timeouts(chip, timeout_eff);
if (!chip->timeout_adjusted) {
/* Restore default if chip reported 0 */
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index bf7e49cfa643..57c5b6712563 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -521,13 +521,15 @@ static const struct tis_vendor_timeout_override vendor_timeout_overrides[] = {
(TIS_SHORT_TIMEOUT*1000), (TIS_SHORT_TIMEOUT*1000) } },
};
-static bool tpm_tis_update_timeouts(struct tpm_chip *chip,
+static int tpm_tis_update_timeouts(struct tpm_chip *chip,
unsigned long *timeout_cap)
{
struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
int i, rc;
u32 did_vid;
+ chip->timeout_adjusted = false;
+
if (chip->ops->clk_enable != NULL)
chip->ops->clk_enable(chip, true);
@@ -540,11 +542,9 @@ static bool tpm_tis_update_timeouts(struct tpm_chip *chip,
continue;
memcpy(timeout_cap, vendor_timeout_overrides[i].timeout_us,
sizeof(vendor_timeout_overrides[i].timeout_us));
- rc = true;
+ chip->timeout_adjusted = true;
}
- rc = false;
-
out:
if (chip->ops->clk_enable != NULL)
chip->ops->clk_enable(chip, false);
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index b49a55cf775f..d18df4b6247f 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -41,7 +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,
+ int (*update_timeouts)(struct tpm_chip *chip,
unsigned long *timeout_cap);
int (*go_idle)(struct tpm_chip *chip);
int (*cmd_ready)(struct tpm_chip *chip);
--
2.20.1.98.gecbdaf0899
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [RFC PATCH 1/1] tpm: don't return bool from update_timeouts
2019-01-29 17:41 ` [RFC PATCH 1/1] " Jerry Snitselaar
@ 2019-01-29 19:22 ` Jarkko Sakkinen
2019-01-29 19:50 ` Jerry Snitselaar
0 siblings, 1 reply; 9+ messages in thread
From: Jarkko Sakkinen @ 2019-01-29 19:22 UTC (permalink / raw)
To: Jerry Snitselaar; +Cc: linux-integrity
On Tue, Jan 29, 2019 at 10:41:45AM -0700, Jerry Snitselaar wrote:
> Set tpm_chip->timeouts_adjusted directly in the update_timeouts
> code instead of returning bool. Return rc of tpm read instead,
> which can be return an error in the case of the spi driver.
>
> Signed-off-by: Jerry Snitselaar <jsnitsel@redhat.com>
Couple of remarks:
1. Shouldn't it then be just void?
2. Why do we even need timeout_adjusted in struct tpm_chip [1]?
[1] Only used locally in tpm1_get_timeouts().
/Jarkko
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC PATCH 1/1] tpm: don't return bool from update_timeouts
2019-01-29 19:22 ` Jarkko Sakkinen
@ 2019-01-29 19:50 ` Jerry Snitselaar
2019-01-29 21:12 ` Jarkko Sakkinen
0 siblings, 1 reply; 9+ messages in thread
From: Jerry Snitselaar @ 2019-01-29 19:50 UTC (permalink / raw)
To: Jarkko Sakkinen; +Cc: linux-integrity
On Tue Jan 29 19, Jarkko Sakkinen wrote:
>On Tue, Jan 29, 2019 at 10:41:45AM -0700, Jerry Snitselaar wrote:
>> Set tpm_chip->timeouts_adjusted directly in the update_timeouts
>> code instead of returning bool. Return rc of tpm read instead,
>> which can be return an error in the case of the spi driver.
>>
>> Signed-off-by: Jerry Snitselaar <jsnitsel@redhat.com>
>
>Couple of remarks:
>
>1. Shouldn't it then be just void?
Yes I think if we don't really care about the tpm read failing
it can just be void, and print out a warning when the read fail
occurs.
>2. Why do we even need timeout_adjusted in struct tpm_chip [1]?
It gets used by timeouts_show in tpm-sysfs.c as well.
>
>[1] Only used locally in tpm1_get_timeouts().
>
>/Jarkko
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC PATCH 1/1] tpm: don't return bool from update_timeouts
2019-01-29 19:50 ` Jerry Snitselaar
@ 2019-01-29 21:12 ` Jarkko Sakkinen
0 siblings, 0 replies; 9+ messages in thread
From: Jarkko Sakkinen @ 2019-01-29 21:12 UTC (permalink / raw)
To: Jerry Snitselaar; +Cc: linux-integrity
On Tue, Jan 29, 2019 at 12:50:31PM -0700, Jerry Snitselaar wrote:
> On Tue Jan 29 19, Jarkko Sakkinen wrote:
> > On Tue, Jan 29, 2019 at 10:41:45AM -0700, Jerry Snitselaar wrote:
> > > Set tpm_chip->timeouts_adjusted directly in the update_timeouts
> > > code instead of returning bool. Return rc of tpm read instead,
> > > which can be return an error in the case of the spi driver.
> > >
> > > Signed-off-by: Jerry Snitselaar <jsnitsel@redhat.com>
> >
> > Couple of remarks:
> >
> > 1. Shouldn't it then be just void?
>
> Yes I think if we don't really care about the tpm read failing
> it can just be void, and print out a warning when the read fail
> occurs.
I don't think we care.
> > 2. Why do we even need timeout_adjusted in struct tpm_chip [1]?
>
> It gets used by timeouts_show in tpm-sysfs.c as well.
Ugh, dejavu :-) I've made this same false remark at least once
before.
/Jarkko
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC PATCH 0/1] tpm: don't return bool from update_timeouts
2019-01-29 17:41 [RFC PATCH 0/1] tpm: don't return bool from update_timeouts Jerry Snitselaar
2019-01-29 17:41 ` [RFC PATCH 1/1] " Jerry Snitselaar
@ 2019-01-29 18:31 ` Jarkko Sakkinen
2019-01-29 20:14 ` [RFC PATCH v2 1/1] " Jerry Snitselaar
2 siblings, 0 replies; 9+ messages in thread
From: Jarkko Sakkinen @ 2019-01-29 18:31 UTC (permalink / raw)
To: Jerry Snitselaar; +Cc: linux-integrity
On Tue, Jan 29, 2019 at 10:41:44AM -0700, Jerry Snitselaar wrote:
> Sorry, I've been tied up tracking down some iommu issues. I just took
> a quick stab at this. It looks like the only failure case is the tpm
> read of the did/vid failing for the spi driver. My question is do we
> want to fail the probe in that case, spit out a message, or just
> continue on? Thoughts?
>
> Regards,
> Jerry
Probably printing warning would be enough. I mean do selftests and
everything.
/Jarkko
^ permalink raw reply [flat|nested] 9+ messages in thread
* [RFC PATCH v2 1/1] tpm: don't return bool from update_timeouts
2019-01-29 17:41 [RFC PATCH 0/1] tpm: don't return bool from update_timeouts Jerry Snitselaar
2019-01-29 17:41 ` [RFC PATCH 1/1] " Jerry Snitselaar
2019-01-29 18:31 ` [RFC PATCH 0/1] " Jarkko Sakkinen
@ 2019-01-29 20:14 ` Jerry Snitselaar
2019-01-30 21:23 ` Jarkko Sakkinen
2 siblings, 1 reply; 9+ messages in thread
From: Jerry Snitselaar @ 2019-01-29 20:14 UTC (permalink / raw)
To: linux-integrity; +Cc: Jarkko Sakkinen, Jerry Snitselaar
Set tpm_chip->timeouts_adjusted directly in the update_timeouts
code instead of returning bool. In case of tpm read failing
print warning that the read failed and continue on.
Signed-off-by: Jerry Snitselaar <jsnitsel@redhat.com>
---
v2: changed update_timeouts to void and print warning in case
of tpm read failing.
drivers/char/tpm/tpm1-cmd.c | 3 +--
drivers/char/tpm/tpm_tis_core.c | 14 ++++++++------
include/linux/tpm.h | 2 +-
3 files changed, 10 insertions(+), 9 deletions(-)
diff --git a/drivers/char/tpm/tpm1-cmd.c b/drivers/char/tpm/tpm1-cmd.c
index 6f306338953b..bda9a16b44f6 100644
--- a/drivers/char/tpm/tpm1-cmd.c
+++ b/drivers/char/tpm/tpm1-cmd.c
@@ -380,8 +380,7 @@ int tpm1_get_timeouts(struct tpm_chip *chip)
* of misreporting.
*/
if (chip->ops->update_timeouts)
- chip->timeout_adjusted =
- chip->ops->update_timeouts(chip, timeout_eff);
+ chip->ops->update_timeouts(chip, timeout_eff);
if (!chip->timeout_adjusted) {
/* Restore default if chip reported 0 */
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index bf7e49cfa643..aec57bd25dd2 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -521,35 +521,37 @@ static const struct tis_vendor_timeout_override vendor_timeout_overrides[] = {
(TIS_SHORT_TIMEOUT*1000), (TIS_SHORT_TIMEOUT*1000) } },
};
-static bool tpm_tis_update_timeouts(struct tpm_chip *chip,
+static void tpm_tis_update_timeouts(struct tpm_chip *chip,
unsigned long *timeout_cap)
{
struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
int i, rc;
u32 did_vid;
+ chip->timeout_adjusted = false;
+
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)
+ if (rc < 0) {
+ dev_warn(&chip->dev, "%s: failed to read did_vid: %d\n", __func__, rc);
goto out;
+ }
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));
- rc = true;
+ chip->timeout_adjusted = true;
}
- rc = false;
-
out:
if (chip->ops->clk_enable != NULL)
chip->ops->clk_enable(chip, false);
- return rc;
+ return;
}
/*
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index b49a55cf775f..13563b8c0c3a 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -41,7 +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,
+ void (*update_timeouts)(struct tpm_chip *chip,
unsigned long *timeout_cap);
int (*go_idle)(struct tpm_chip *chip);
int (*cmd_ready)(struct tpm_chip *chip);
--
2.20.1.98.gecbdaf0899
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [RFC PATCH v2 1/1] tpm: don't return bool from update_timeouts
2019-01-29 20:14 ` [RFC PATCH v2 1/1] " Jerry Snitselaar
@ 2019-01-30 21:23 ` Jarkko Sakkinen
2019-01-30 21:24 ` Jarkko Sakkinen
0 siblings, 1 reply; 9+ messages in thread
From: Jarkko Sakkinen @ 2019-01-30 21:23 UTC (permalink / raw)
To: Jerry Snitselaar; +Cc: linux-integrity
On Tue, Jan 29, 2019 at 01:14:15PM -0700, Jerry Snitselaar wrote:
> Set tpm_chip->timeouts_adjusted directly in the update_timeouts
> code instead of returning bool. In case of tpm read failing
> print warning that the read failed and continue on.
>
> Signed-off-by: Jerry Snitselaar <jsnitsel@redhat.com>
Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
/Jarkko
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC PATCH v2 1/1] tpm: don't return bool from update_timeouts
2019-01-30 21:23 ` Jarkko Sakkinen
@ 2019-01-30 21:24 ` Jarkko Sakkinen
0 siblings, 0 replies; 9+ messages in thread
From: Jarkko Sakkinen @ 2019-01-30 21:24 UTC (permalink / raw)
To: Jerry Snitselaar; +Cc: linux-integrity
On Wed, Jan 30, 2019 at 11:23:03PM +0200, Jarkko Sakkinen wrote:
> On Tue, Jan 29, 2019 at 01:14:15PM -0700, Jerry Snitselaar wrote:
> > Set tpm_chip->timeouts_adjusted directly in the update_timeouts
> > code instead of returning bool. In case of tpm read failing
> > print warning that the read failed and continue on.
> >
> > Signed-off-by: Jerry Snitselaar <jsnitsel@redhat.com>
>
> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Hey, can you-re-send this without RFC and the earlier that I already
applied to my tree, with CC to linux-kernel and linux-security-module?
Keep my reviewed-by in both.
Otherwise, I cannot include these into my next PR.
/Jarkko
^ permalink raw reply [flat|nested] 9+ messages in thread