LinuxLists.cc - [PATCH] tpm_tis: turn on TPM before calling tpm_get

[permalink] [raw]

Subject: [PATCH v2] tpm_tis: turn on TPM before calling tpm_get_timeouts

With power gating moved out of the tpm_transmit code we need
to power on the TPM prior to calling tpm_get_timeouts.

Cc: Jarkko Sakkinen <[email protected]>
Cc: Peter Huewe <[email protected]>
Cc: Jason Gunthorpe <[email protected]>
Cc: [email protected]
Cc: [email protected]
Fixes: a3fbfae82b4c ("tpm: take TPM chip power gating out of tpm_transmit()")
Reported-by: Christian Bundy <[email protected]>
Signed-off-by: Jerry Snitselaar <[email protected]>
---
v2: fix stable cc to correct address

drivers/char/tpm/tpm_tis_core.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 270f43acbb77..cb101cec8f8b 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -974,13 +974,14 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
* to make sure it works. May as well use that command to set the
* proper timeouts for the driver.
*/
+ tpm_chip_start(chip);
if (tpm_get_timeouts(chip)) {
dev_err(dev, "Could not get TPM timeouts and durations\n");
rc = -ENODEV;
+ tpm_stop_chip(chip);
goto out_err;
}

- tpm_chip_start(chip);
chip->flags |= TPM_CHIP_FLAG_IRQ;
if (irq) {
tpm_tis_probe_irq_single(chip, intmask, IRQF_SHARED,
--
2.24.0

2019-11-12 20:07:51

[permalink] [raw]

Subject: Re: [PATCH] tpm_tis: turn on TPM before calling tpm_get_timeouts

On Mon, Nov 11, 2019 at 04:34:18PM -0700, Jerry Snitselaar wrote:
> With power gating moved out of the tpm_transmit code we need
> to power on the TPM prior to calling tpm_get_timeouts.
>
> Cc: Jarkko Sakkinen <[email protected]>
> Cc: Peter Huewe <[email protected]>
> Cc: Jason Gunthorpe <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Fixes: a3fbfae82b4c ("tpm: take TPM chip power gating out of tpm_transmit()")
> Reported-by: Christian Bundy <[email protected]>
> Signed-off-by: Jerry Snitselaar <[email protected]>
> ---
> drivers/char/tpm/tpm_tis_core.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index 270f43acbb77..cb101cec8f8b 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -974,13 +974,14 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
> * to make sure it works. May as well use that command to set the
> * proper timeouts for the driver.
> */
> + tpm_chip_start(chip);
> if (tpm_get_timeouts(chip)) {
> dev_err(dev, "Could not get TPM timeouts and durations\n");
> rc = -ENODEV;
> + tpm_stop_chip(chip);
> goto out_err;
> }

Couldn't this call just be removed?

/Jarkko

2019-11-12 20:24:58

[permalink] [raw]

Subject: Re: [PATCH] tpm_tis: turn on TPM before calling tpm_get_timeouts

On Tue, Nov 12, 2019 at 1:03 PM Jarkko Sakkinen
<[email protected]> wrote:
>
> On Mon, Nov 11, 2019 at 04:34:18PM -0700, Jerry Snitselaar wrote:
> > With power gating moved out of the tpm_transmit code we need
> > to power on the TPM prior to calling tpm_get_timeouts.
> >
> > Cc: Jarkko Sakkinen <[email protected]>
> > Cc: Peter Huewe <[email protected]>
> > Cc: Jason Gunthorpe <[email protected]>
> > Cc: [email protected]
> > Cc: [email protected]
> > Fixes: a3fbfae82b4c ("tpm: take TPM chip power gating out of tpm_transmit()")
> > Reported-by: Christian Bundy <[email protected]>
> > Signed-off-by: Jerry Snitselaar <[email protected]>
> > ---
> > drivers/char/tpm/tpm_tis_core.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> > index 270f43acbb77..cb101cec8f8b 100644
> > --- a/drivers/char/tpm/tpm_tis_core.c
> > +++ b/drivers/char/tpm/tpm_tis_core.c
> > @@ -974,13 +974,14 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
> > * to make sure it works. May as well use that command to set the
> > * proper timeouts for the driver.
> > */
> > + tpm_chip_start(chip);
> > if (tpm_get_timeouts(chip)) {
> > dev_err(dev, "Could not get TPM timeouts and durations\n");
> > rc = -ENODEV;
> > + tpm_stop_chip(chip);
> > goto out_err;
> > }
>
> Couldn't this call just be removed?
>
> /Jarkko
>

Probably. It will eventually get called when tpm_chip_register
happens. I don't know what the reason was for trying it prior to the
irq probe.

2019-11-12 20:32:20

[permalink] [raw]

Subject: Re: [PATCH] tpm_tis: turn on TPM before calling tpm_get_timeouts

On Tue, Nov 12, 2019 at 01:23:33PM -0700, Jerry Snitselaar wrote:
> On Tue, Nov 12, 2019 at 1:03 PM Jarkko Sakkinen
> <[email protected]> wrote:
> >
> > On Mon, Nov 11, 2019 at 04:34:18PM -0700, Jerry Snitselaar wrote:
> > > With power gating moved out of the tpm_transmit code we need
> > > to power on the TPM prior to calling tpm_get_timeouts.
> > >
> > > Cc: Jarkko Sakkinen <[email protected]>
> > > Cc: Peter Huewe <[email protected]>
> > > Cc: Jason Gunthorpe <[email protected]>
> > > Cc: [email protected]
> > > Cc: [email protected]
> > > Fixes: a3fbfae82b4c ("tpm: take TPM chip power gating out of tpm_transmit()")
> > > Reported-by: Christian Bundy <[email protected]>
> > > Signed-off-by: Jerry Snitselaar <[email protected]>
> > > drivers/char/tpm/tpm_tis_core.c | 3 ++-
> > > 1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> > > index 270f43acbb77..cb101cec8f8b 100644
> > > +++ b/drivers/char/tpm/tpm_tis_core.c
> > > @@ -974,13 +974,14 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
> > > * to make sure it works. May as well use that command to set the
> > > * proper timeouts for the driver.
> > > */
> > > + tpm_chip_start(chip);
> > > if (tpm_get_timeouts(chip)) {
> > > dev_err(dev, "Could not get TPM timeouts and durations\n");
> > > rc = -ENODEV;
> > > + tpm_stop_chip(chip);
> > > goto out_err;
> > > }
> >
> > Couldn't this call just be removed?
> >
> > /Jarkko
> >
>
> Probably. It will eventually get called when tpm_chip_register
> happens. I don't know what the reason was for trying it prior to the
> irq probe.

At least tis once needed the timeouts before registration because it
was issuing TPM commands to complete its setup.

If timeouts have not been set then no TPM command should be executed.

Jason

2019-11-12 20:32:30

[permalink] [raw]

Subject: Re: [PATCH] tpm_tis: turn on TPM before calling tpm_get_timeouts

On Tue, Nov 12, 2019 at 1:26 PM Jason Gunthorpe <[email protected]> wrote:
>
> On Tue, Nov 12, 2019 at 01:23:33PM -0700, Jerry Snitselaar wrote:
> > On Tue, Nov 12, 2019 at 1:03 PM Jarkko Sakkinen
> > <[email protected]> wrote:
> > >
> > > On Mon, Nov 11, 2019 at 04:34:18PM -0700, Jerry Snitselaar wrote:
> > > > With power gating moved out of the tpm_transmit code we need
> > > > to power on the TPM prior to calling tpm_get_timeouts.
> > > >
> > > > Cc: Jarkko Sakkinen <[email protected]>
> > > > Cc: Peter Huewe <[email protected]>
> > > > Cc: Jason Gunthorpe <[email protected]>
> > > > Cc: [email protected]
> > > > Cc: [email protected]
> > > > Fixes: a3fbfae82b4c ("tpm: take TPM chip power gating out of tpm_transmit()")
> > > > Reported-by: Christian Bundy <[email protected]>
> > > > Signed-off-by: Jerry Snitselaar <[email protected]>
> > > > drivers/char/tpm/tpm_tis_core.c | 3 ++-
> > > > 1 file changed, 2 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> > > > index 270f43acbb77..cb101cec8f8b 100644
> > > > +++ b/drivers/char/tpm/tpm_tis_core.c
> > > > @@ -974,13 +974,14 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
> > > > * to make sure it works. May as well use that command to set the
> > > > * proper timeouts for the driver.
> > > > */
> > > > + tpm_chip_start(chip);
> > > > if (tpm_get_timeouts(chip)) {
> > > > dev_err(dev, "Could not get TPM timeouts and durations\n");
> > > > rc = -ENODEV;
> > > > + tpm_stop_chip(chip);
> > > > goto out_err;
> > > > }
> > >
> > > Couldn't this call just be removed?
> > >
> > > /Jarkko
> > >
> >
> > Probably. It will eventually get called when tpm_chip_register
> > happens. I don't know what the reason was for trying it prior to the
> > irq probe.
>
> At least tis once needed the timeouts before registration because it
> was issuing TPM commands to complete its setup.
>
> If timeouts have not been set then no TPM command should be executed.
>
> Jason
>

Would it function with the timeout values set at the beginning of
tpm_tis_core_init (max values)?

2019-11-12 20:35:20

[permalink] [raw]

Subject: Re: [PATCH] tpm_tis: turn on TPM before calling tpm_get_timeouts

On Tue, Nov 12, 2019 at 1:28 PM Jerry Snitselaar <[email protected]> wrote:
>
> On Tue, Nov 12, 2019 at 1:26 PM Jason Gunthorpe <[email protected]> wrote:
> >
> > On Tue, Nov 12, 2019 at 01:23:33PM -0700, Jerry Snitselaar wrote:
> > > On Tue, Nov 12, 2019 at 1:03 PM Jarkko Sakkinen
> > > <[email protected]> wrote:
> > > >
> > > > On Mon, Nov 11, 2019 at 04:34:18PM -0700, Jerry Snitselaar wrote:
> > > > > With power gating moved out of the tpm_transmit code we need
> > > > > to power on the TPM prior to calling tpm_get_timeouts.
> > > > >
> > > > > Cc: Jarkko Sakkinen <[email protected]>
> > > > > Cc: Peter Huewe <[email protected]>
> > > > > Cc: Jason Gunthorpe <[email protected]>
> > > > > Cc: [email protected]
> > > > > Cc: [email protected]
> > > > > Fixes: a3fbfae82b4c ("tpm: take TPM chip power gating out of tpm_transmit()")
> > > > > Reported-by: Christian Bundy <[email protected]>
> > > > > Signed-off-by: Jerry Snitselaar <[email protected]>
> > > > > drivers/char/tpm/tpm_tis_core.c | 3 ++-
> > > > > 1 file changed, 2 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> > > > > index 270f43acbb77..cb101cec8f8b 100644
> > > > > +++ b/drivers/char/tpm/tpm_tis_core.c
> > > > > @@ -974,13 +974,14 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
> > > > > * to make sure it works. May as well use that command to set the
> > > > > * proper timeouts for the driver.
> > > > > */
> > > > > + tpm_chip_start(chip);
> > > > > if (tpm_get_timeouts(chip)) {
> > > > > dev_err(dev, "Could not get TPM timeouts and durations\n");
> > > > > rc = -ENODEV;
> > > > > + tpm_stop_chip(chip);
> > > > > goto out_err;
> > > > > }
> > > >
> > > > Couldn't this call just be removed?
> > > >
> > > > /Jarkko
> > > >
> > >
> > > Probably. It will eventually get called when tpm_chip_register
> > > happens. I don't know what the reason was for trying it prior to the
> > > irq probe.
> >
> > At least tis once needed the timeouts before registration because it
> > was issuing TPM commands to complete its setup.
> >
> > If timeouts have not been set then no TPM command should be executed.
> >
> > Jason
> >
>
> Would it function with the timeout values set at the beginning of
> tpm_tis_core_init (max values)?

I guess that doesn't set the duration values though

2019-11-12 20:50:31

[permalink] [raw]

Subject: Re: [PATCH] tpm_tis: turn on TPM before calling tpm_get_timeouts

On Tue, Nov 12, 2019 at 01:31:09PM -0700, Jerry Snitselaar wrote:
> On Tue, Nov 12, 2019 at 1:28 PM Jerry Snitselaar <[email protected]> wrote:
> >
> > On Tue, Nov 12, 2019 at 1:26 PM Jason Gunthorpe <[email protected]> wrote:
> > >
> > > On Tue, Nov 12, 2019 at 01:23:33PM -0700, Jerry Snitselaar wrote:
> > > > On Tue, Nov 12, 2019 at 1:03 PM Jarkko Sakkinen
> > > > <[email protected]> wrote:
> > > > >
> > > > > On Mon, Nov 11, 2019 at 04:34:18PM -0700, Jerry Snitselaar wrote:
> > > > > > With power gating moved out of the tpm_transmit code we need
> > > > > > to power on the TPM prior to calling tpm_get_timeouts.
> > > > > >
> > > > > > Cc: Jarkko Sakkinen <[email protected]>
> > > > > > Cc: Peter Huewe <[email protected]>
> > > > > > Cc: Jason Gunthorpe <[email protected]>
> > > > > > Cc: [email protected]
> > > > > > Cc: [email protected]
> > > > > > Fixes: a3fbfae82b4c ("tpm: take TPM chip power gating out of tpm_transmit()")
> > > > > > Reported-by: Christian Bundy <[email protected]>
> > > > > > Signed-off-by: Jerry Snitselaar <[email protected]>
> > > > > > drivers/char/tpm/tpm_tis_core.c | 3 ++-
> > > > > > 1 file changed, 2 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> > > > > > index 270f43acbb77..cb101cec8f8b 100644
> > > > > > +++ b/drivers/char/tpm/tpm_tis_core.c
> > > > > > @@ -974,13 +974,14 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
> > > > > > * to make sure it works. May as well use that command to set the
> > > > > > * proper timeouts for the driver.
> > > > > > */
> > > > > > + tpm_chip_start(chip);
> > > > > > if (tpm_get_timeouts(chip)) {
> > > > > > dev_err(dev, "Could not get TPM timeouts and durations\n");
> > > > > > rc = -ENODEV;
> > > > > > + tpm_stop_chip(chip);
> > > > > > goto out_err;
> > > > > > }
> > > > >
> > > > > Couldn't this call just be removed?
> > > > >
> > > > > /Jarkko
> > > > >
> > > >
> > > > Probably. It will eventually get called when tpm_chip_register
> > > > happens. I don't know what the reason was for trying it prior to the
> > > > irq probe.
> > >
> > > At least tis once needed the timeouts before registration because it
> > > was issuing TPM commands to complete its setup.
> > >
> > > If timeouts have not been set then no TPM command should be executed.
> >
> > Would it function with the timeout values set at the beginning of
> > tpm_tis_core_init (max values)?
>
> I guess that doesn't set the duration values though

There is no reason to use anything but the correct timeouts, as read
from the device.

Jason

2019-11-12 21:17:40

[permalink] [raw]

Subject: Re: [PATCH] tpm_tis: turn on TPM before calling tpm_get_timeouts

On Tue Nov 12 19, Jason Gunthorpe wrote:
>On Tue, Nov 12, 2019 at 01:31:09PM -0700, Jerry Snitselaar wrote:
>> On Tue, Nov 12, 2019 at 1:28 PM Jerry Snitselaar <[email protected]> wrote:
>> >
>> > On Tue, Nov 12, 2019 at 1:26 PM Jason Gunthorpe <[email protected]> wrote:
>> > >
>> > > On Tue, Nov 12, 2019 at 01:23:33PM -0700, Jerry Snitselaar wrote:
>> > > > On Tue, Nov 12, 2019 at 1:03 PM Jarkko Sakkinen
>> > > > <[email protected]> wrote:
>> > > > >
>> > > > > On Mon, Nov 11, 2019 at 04:34:18PM -0700, Jerry Snitselaar wrote:
>> > > > > > With power gating moved out of the tpm_transmit code we need
>> > > > > > to power on the TPM prior to calling tpm_get_timeouts.
>> > > > > >
>> > > > > > Cc: Jarkko Sakkinen <[email protected]>
>> > > > > > Cc: Peter Huewe <[email protected]>
>> > > > > > Cc: Jason Gunthorpe <[email protected]>
>> > > > > > Cc: [email protected]
>> > > > > > Cc: [email protected]
>> > > > > > Fixes: a3fbfae82b4c ("tpm: take TPM chip power gating out of tpm_transmit()")
>> > > > > > Reported-by: Christian Bundy <[email protected]>
>> > > > > > Signed-off-by: Jerry Snitselaar <[email protected]>
>> > > > > > drivers/char/tpm/tpm_tis_core.c | 3 ++-
>> > > > > > 1 file changed, 2 insertions(+), 1 deletion(-)
>> > > > > >
>> > > > > > diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
>> > > > > > index 270f43acbb77..cb101cec8f8b 100644
>> > > > > > +++ b/drivers/char/tpm/tpm_tis_core.c
>> > > > > > @@ -974,13 +974,14 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
>> > > > > > * to make sure it works. May as well use that command to set the
>> > > > > > * proper timeouts for the driver.
>> > > > > > */
>> > > > > > + tpm_chip_start(chip);
>> > > > > > if (tpm_get_timeouts(chip)) {
>> > > > > > dev_err(dev, "Could not get TPM timeouts and durations\n");
>> > > > > > rc = -ENODEV;
>> > > > > > + tpm_stop_chip(chip);
>> > > > > > goto out_err;
>> > > > > > }
>> > > > >
>> > > > > Couldn't this call just be removed?
>> > > > >
>> > > > > /Jarkko
>> > > > >
>> > > >
>> > > > Probably. It will eventually get called when tpm_chip_register
>> > > > happens. I don't know what the reason was for trying it prior to the
>> > > > irq probe.
>> > >
>> > > At least tis once needed the timeouts before registration because it
>> > > was issuing TPM commands to complete its setup.
>> > >
>> > > If timeouts have not been set then no TPM command should be executed.
>> >
>> > Would it function with the timeout values set at the beginning of
>> > tpm_tis_core_init (max values)?
>>
>> I guess that doesn't set the duration values though
>
>There is no reason to use anything but the correct timeouts, as read
>from the device.
>
>Jason
>

Should there be a check in tpm1_get_timeouts and tpm2_get_timeouts:

if (chip->flags & TPM_CHIP_FLAG_HAVE_TIMEOUTS)
return 0;

to skip going through it again in the auto startup code if it was
already called and set?

2019-11-13 00:04:38

[permalink] [raw]

Subject: [PATCH v3] tpm_tis: turn on TPM before calling tpm_get_timeouts

With power gating moved out of the tpm_transmit code we need
to power on the TPM prior to calling tpm_get_timeouts.

Cc: Jarkko Sakkinen <[email protected]>
Cc: Peter Huewe <[email protected]>
Cc: Jason Gunthorpe <[email protected]>
Cc: [email protected]
Cc: [email protected]
Fixes: a3fbfae82b4c ("tpm: take TPM chip power gating out of tpm_transmit()")
Reported-by: Christian Bundy <[email protected]>
Signed-off-by: Jerry Snitselaar <[email protected]>
---
v3: call tpm_chip_stop in error path
v2: fix stable cc to correct address

drivers/char/tpm/tpm_tis_core.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 270f43acbb77..806acc666696 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -974,13 +974,14 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
* to make sure it works. May as well use that command to set the
* proper timeouts for the driver.
*/
+ tpm_chip_start(chip);
if (tpm_get_timeouts(chip)) {
dev_err(dev, "Could not get TPM timeouts and durations\n");
rc = -ENODEV;
+ tpm_chip_stop(chip);
goto out_err;
}

- tpm_chip_start(chip);
chip->flags |= TPM_CHIP_FLAG_IRQ;
if (irq) {
tpm_tis_probe_irq_single(chip, intmask, IRQF_SHARED,
--
2.24.0

2019-11-14 16:52:09

[permalink] [raw]

Subject: Re: [PATCH] tpm_tis: turn on TPM before calling tpm_get_timeouts

On Tue, Nov 12, 2019 at 04:26:23PM -0400, Jason Gunthorpe wrote:
> On Tue, Nov 12, 2019 at 01:23:33PM -0700, Jerry Snitselaar wrote:
> > On Tue, Nov 12, 2019 at 1:03 PM Jarkko Sakkinen
> > <[email protected]> wrote:
> > >
> > > On Mon, Nov 11, 2019 at 04:34:18PM -0700, Jerry Snitselaar wrote:
> > > > With power gating moved out of the tpm_transmit code we need
> > > > to power on the TPM prior to calling tpm_get_timeouts.
> > > >
> > > > Cc: Jarkko Sakkinen <[email protected]>
> > > > Cc: Peter Huewe <[email protected]>
> > > > Cc: Jason Gunthorpe <[email protected]>
> > > > Cc: [email protected]
> > > > Cc: [email protected]
> > > > Fixes: a3fbfae82b4c ("tpm: take TPM chip power gating out of tpm_transmit()")
> > > > Reported-by: Christian Bundy <[email protected]>
> > > > Signed-off-by: Jerry Snitselaar <[email protected]>
> > > > drivers/char/tpm/tpm_tis_core.c | 3 ++-
> > > > 1 file changed, 2 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> > > > index 270f43acbb77..cb101cec8f8b 100644
> > > > +++ b/drivers/char/tpm/tpm_tis_core.c
> > > > @@ -974,13 +974,14 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
> > > > * to make sure it works. May as well use that command to set the
> > > > * proper timeouts for the driver.
> > > > */
> > > > + tpm_chip_start(chip);
> > > > if (tpm_get_timeouts(chip)) {
> > > > dev_err(dev, "Could not get TPM timeouts and durations\n");
> > > > rc = -ENODEV;
> > > > + tpm_stop_chip(chip);
> > > > goto out_err;
> > > > }
> > >
> > > Couldn't this call just be removed?
> > >
> > > /Jarkko
> > >
> >
> > Probably. It will eventually get called when tpm_chip_register
> > happens. I don't know what the reason was for trying it prior to the
> > irq probe.
>
> At least tis once needed the timeouts before registration because it
> was issuing TPM commands to complete its setup.
>
> If timeouts have not been set then no TPM command should be executed.

Not true since you need a TPM command to set them. That is why they
have been set initially to maximum possible values.

/Jarkko

2019-11-14 16:53:39

[permalink] [raw]

Subject: Re: [PATCH] tpm_tis: turn on TPM before calling tpm_get_timeouts

On Thu, Nov 14, 2019 at 06:49:49PM +0200, Jarkko Sakkinen wrote:
> On Tue, Nov 12, 2019 at 04:26:23PM -0400, Jason Gunthorpe wrote:
> > On Tue, Nov 12, 2019 at 01:23:33PM -0700, Jerry Snitselaar wrote:
> > > On Tue, Nov 12, 2019 at 1:03 PM Jarkko Sakkinen
> > > <[email protected]> wrote:
> > > >
> > > > On Mon, Nov 11, 2019 at 04:34:18PM -0700, Jerry Snitselaar wrote:
> > > > > With power gating moved out of the tpm_transmit code we need
> > > > > to power on the TPM prior to calling tpm_get_timeouts.
> > > > >
> > > > > Cc: Jarkko Sakkinen <[email protected]>
> > > > > Cc: Peter Huewe <[email protected]>
> > > > > Cc: Jason Gunthorpe <[email protected]>
> > > > > Cc: [email protected]
> > > > > Cc: [email protected]
> > > > > Fixes: a3fbfae82b4c ("tpm: take TPM chip power gating out of tpm_transmit()")
> > > > > Reported-by: Christian Bundy <[email protected]>
> > > > > Signed-off-by: Jerry Snitselaar <[email protected]>
> > > > > drivers/char/tpm/tpm_tis_core.c | 3 ++-
> > > > > 1 file changed, 2 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> > > > > index 270f43acbb77..cb101cec8f8b 100644
> > > > > +++ b/drivers/char/tpm/tpm_tis_core.c
> > > > > @@ -974,13 +974,14 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
> > > > > * to make sure it works. May as well use that command to set the
> > > > > * proper timeouts for the driver.
> > > > > */
> > > > > + tpm_chip_start(chip);
> > > > > if (tpm_get_timeouts(chip)) {
> > > > > dev_err(dev, "Could not get TPM timeouts and durations\n");
> > > > > rc = -ENODEV;
> > > > > + tpm_stop_chip(chip);
> > > > > goto out_err;
> > > > > }
> > > >
> > > > Couldn't this call just be removed?
> > > >
> > > > /Jarkko
> > > >
> > >
> > > Probably. It will eventually get called when tpm_chip_register
> > > happens. I don't know what the reason was for trying it prior to the
> > > irq probe.
> >
> > At least tis once needed the timeouts before registration because it
> > was issuing TPM commands to complete its setup.
> >
> > If timeouts have not been set then no TPM command should be executed.
>
> Not true since you need a TPM command to set them. That is why they
> have been set initially to maximum possible values.

getting timeouts is the exception

Jason

2019-11-14 16:58:26

[permalink] [raw]

Subject: Re: [PATCH] tpm_tis: turn on TPM before calling tpm_get_timeouts

On Thu, Nov 14, 2019 at 06:55:06PM +0200, Jarkko Sakkinen wrote:
> > Would it function with the timeout values set at the beginning of
> > tpm_tis_core_init (max values)?
>
> tpm_get_timeouts() should be replaced with:
>
> if (tpm_chip_start()) {
> dev_err(dev, "Could not get TPM timeouts and durations\n");
> rc = -ENODEV;
> goto out_err;
> }
>
> tpm_stop_chip(chip);
>
> tpm_get_timeouts() is called by tpm_auto_startup(). Also the function
> should be moved to tpm_chip.c and converted to a static function so
> that it won't be called from random cal sites like above.

Careful, the design here was to allow a driver to do only
get_timeouts, then additional setup work, then do auto_startup()

Forcing a driver to do auto_startup too early may not be good.

Jason

2019-11-14 16:59:29

[permalink] [raw]

Subject: Re: [PATCH] tpm_tis: turn on TPM before calling tpm_get_timeouts

On Tue, Nov 12, 2019 at 01:28:49PM -0700, Jerry Snitselaar wrote:
> On Tue, Nov 12, 2019 at 1:26 PM Jason Gunthorpe <[email protected]> wrote:
> >
> > On Tue, Nov 12, 2019 at 01:23:33PM -0700, Jerry Snitselaar wrote:
> > > On Tue, Nov 12, 2019 at 1:03 PM Jarkko Sakkinen
> > > <[email protected]> wrote:
> > > >
> > > > On Mon, Nov 11, 2019 at 04:34:18PM -0700, Jerry Snitselaar wrote:
> > > > > With power gating moved out of the tpm_transmit code we need
> > > > > to power on the TPM prior to calling tpm_get_timeouts.
> > > > >
> > > > > Cc: Jarkko Sakkinen <[email protected]>
> > > > > Cc: Peter Huewe <[email protected]>
> > > > > Cc: Jason Gunthorpe <[email protected]>
> > > > > Cc: [email protected]
> > > > > Cc: [email protected]
> > > > > Fixes: a3fbfae82b4c ("tpm: take TPM chip power gating out of tpm_transmit()")
> > > > > Reported-by: Christian Bundy <[email protected]>
> > > > > Signed-off-by: Jerry Snitselaar <[email protected]>
> > > > > drivers/char/tpm/tpm_tis_core.c | 3 ++-
> > > > > 1 file changed, 2 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> > > > > index 270f43acbb77..cb101cec8f8b 100644
> > > > > +++ b/drivers/char/tpm/tpm_tis_core.c
> > > > > @@ -974,13 +974,14 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
> > > > > * to make sure it works. May as well use that command to set the
> > > > > * proper timeouts for the driver.
> > > > > */
> > > > > + tpm_chip_start(chip);
> > > > > if (tpm_get_timeouts(chip)) {
> > > > > dev_err(dev, "Could not get TPM timeouts and durations\n");
> > > > > rc = -ENODEV;
> > > > > + tpm_stop_chip(chip);
> > > > > goto out_err;
> > > > > }
> > > >
> > > > Couldn't this call just be removed?
> > > >
> > > > /Jarkko
> > > >
> > >
> > > Probably. It will eventually get called when tpm_chip_register
> > > happens. I don't know what the reason was for trying it prior to the
> > > irq probe.
> >
> > At least tis once needed the timeouts before registration because it
> > was issuing TPM commands to complete its setup.
> >
> > If timeouts have not been set then no TPM command should be executed.
> >
> > Jason
> >
>
> Would it function with the timeout values set at the beginning of
> tpm_tis_core_init (max values)?

tpm_get_timeouts() should be replaced with:

if (tpm_chip_start()) {
dev_err(dev, "Could not get TPM timeouts and durations\n");
rc = -ENODEV;
goto out_err;
}

tpm_stop_chip(chip);

tpm_get_timeouts() is called by tpm_auto_startup(). Also the function
should be moved to tpm_chip.c and converted to a static function so
that it won't be called from random cal sites like above.

/Jarkko

2019-11-14 17:03:28

[permalink] [raw]

Subject: Re: [PATCH v3] tpm_tis: turn on TPM before calling tpm_get_timeouts

On Tue, Nov 12, 2019 at 05:02:43PM -0700, Jerry Snitselaar wrote:
> With power gating moved out of the tpm_transmit code we need
> to power on the TPM prior to calling tpm_get_timeouts.
>
> Cc: Jarkko Sakkinen <[email protected]>
> Cc: Peter Huewe <[email protected]>
> Cc: Jason Gunthorpe <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Fixes: a3fbfae82b4c ("tpm: take TPM chip power gating out of tpm_transmit()")
> Reported-by: Christian Bundy <[email protected]>
> Signed-off-by: Jerry Snitselaar <[email protected]>
> ---
> v3: call tpm_chip_stop in error path
> v2: fix stable cc to correct address
>
> drivers/char/tpm/tpm_tis_core.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index 270f43acbb77..806acc666696 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -974,13 +974,14 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
> * to make sure it works. May as well use that command to set the
> * proper timeouts for the driver.
> */
> + tpm_chip_start(chip);
> if (tpm_get_timeouts(chip)) {
> dev_err(dev, "Could not get TPM timeouts and durations\n");
> rc = -ENODEV;
> + tpm_chip_stop(chip);
> goto out_err;
> }
>
> - tpm_chip_start(chip);

As the commit describes the call is not there for any other reason than
pinging the TPM.

/Jarkko

2019-11-15 17:45:01

[permalink] [raw]

Subject: Re: [PATCH] tpm_tis: turn on TPM before calling tpm_get_timeouts

On Thu, Nov 14, 2019 at 12:56:29PM -0400, Jason Gunthorpe wrote:
> On Thu, Nov 14, 2019 at 06:55:06PM +0200, Jarkko Sakkinen wrote:
> > > Would it function with the timeout values set at the beginning of
> > > tpm_tis_core_init (max values)?
> >
> > tpm_get_timeouts() should be replaced with:
> >
> > if (tpm_chip_start()) {
> > dev_err(dev, "Could not get TPM timeouts and durations\n");
> > rc = -ENODEV;
> > goto out_err;
> > }
> >
> > tpm_stop_chip(chip);
> >
> > tpm_get_timeouts() is called by tpm_auto_startup(). Also the function
> > should be moved to tpm_chip.c and converted to a static function so
> > that it won't be called from random cal sites like above.
>
> Careful, the design here was to allow a driver to do only
> get_timeouts, then additional setup work, then do auto_startup()
>
> Forcing a driver to do auto_startup too early may not be good.

All drivers always do it anyway because all drivers always call
tpm_chip_register().

/Jarkko

2019-11-15 18:38:39

[permalink] [raw]

Subject: Re: [PATCH] tpm_tis: turn on TPM before calling tpm_get_timeouts

On Fri, Nov 15, 2019 at 07:43:29PM +0200, Jarkko Sakkinen wrote:
> On Thu, Nov 14, 2019 at 12:56:29PM -0400, Jason Gunthorpe wrote:
> > On Thu, Nov 14, 2019 at 06:55:06PM +0200, Jarkko Sakkinen wrote:
> > > > Would it function with the timeout values set at the beginning of
> > > > tpm_tis_core_init (max values)?
> > >
> > > tpm_get_timeouts() should be replaced with:
> > >
> > > if (tpm_chip_start()) {
> > > dev_err(dev, "Could not get TPM timeouts and durations\n");
> > > rc = -ENODEV;
> > > goto out_err;
> > > }
> > >
> > > tpm_stop_chip(chip);
> > >
> > > tpm_get_timeouts() is called by tpm_auto_startup(). Also the function
> > > should be moved to tpm_chip.c and converted to a static function so
> > > that it won't be called from random cal sites like above.
> >
> > Careful, the design here was to allow a driver to do only
> > get_timeouts, then additional setup work, then do auto_startup()
> >
> > Forcing a driver to do auto_startup too early may not be good.
>
> All drivers always do it anyway because all drivers always call
> tpm_chip_register().

But chip_register is after the driver has done it's setup and after it
may have called get_timeouts

auto_setup should not be moved to before chip_register()

Jason

2019-11-15 22:42:34