[PATCH] tpm_tis: turn on TPM before calling tpm_get_timeouts

[PATCH] tpm_tis: turn on TPM before calling tpm_get_timeouts

[Jerry Snitselaar]

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 <jarkko.sakkinen@linux.intel.com>
Cc: Peter Huewe <peterhuewe@gmx.de>
Cc: Jason Gunthorpe <jgg@ziepe.ca>
Cc: linux-kernel@vger.kernel.org
Cc: linux-stable@vger.kernel.org
Fixes: a3fbfae82b4c ("tpm: take TPM chip power gating out of tpm_transmit()")
Reported-by: Christian Bundy <christianbundy@fraction.io>
Signed-off-by: Jerry Snitselaar <jsnitsel@redhat.com>
---
 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

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

[Jerry Snitselaar]

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 <jarkko.sakkinen@linux.intel.com>
Cc: Peter Huewe <peterhuewe@gmx.de>
Cc: Jason Gunthorpe <jgg@ziepe.ca>
Cc: linux-kernel@vger.kernel.org
Cc: stable@vger.kernel.org
Fixes: a3fbfae82b4c ("tpm: take TPM chip power gating out of tpm_transmit()")
Reported-by: Christian Bundy <christianbundy@fraction.io>
Signed-off-by: Jerry Snitselaar <jsnitsel@redhat.com>
---
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

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

[Jarkko Sakkinen]

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 <jarkko.sakkinen@linux.intel.com>
> Cc: Peter Huewe <peterhuewe@gmx.de>
> Cc: Jason Gunthorpe <jgg@ziepe.ca>
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-stable@vger.kernel.org
> Fixes: a3fbfae82b4c ("tpm: take TPM chip power gating out of tpm_transmit()")
> Reported-by: Christian Bundy <christianbundy@fraction.io>
> Signed-off-by: Jerry Snitselaar <jsnitsel@redhat.com>
> ---
>  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

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

[Jerry Snitselaar]

On Tue, Nov 12, 2019 at 1:03 PM Jarkko Sakkinen
<jarkko.sakkinen@linux.intel.com> 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 <jarkko.sakkinen@linux.intel.com>
> > Cc: Peter Huewe <peterhuewe@gmx.de>
> > Cc: Jason Gunthorpe <jgg@ziepe.ca>
> > Cc: linux-kernel@vger.kernel.org
> > Cc: linux-stable@vger.kernel.org
> > Fixes: a3fbfae82b4c ("tpm: take TPM chip power gating out of tpm_transmit()")
> > Reported-by: Christian Bundy <christianbundy@fraction.io>
> > Signed-off-by: Jerry Snitselaar <jsnitsel@redhat.com>
> > ---
> >  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.

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

[Jason Gunthorpe]

On Tue, Nov 12, 2019 at 01:23:33PM -0700, Jerry Snitselaar wrote:
> On Tue, Nov 12, 2019 at 1:03 PM Jarkko Sakkinen
> <jarkko.sakkinen@linux.intel.com> 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 <jarkko.sakkinen@linux.intel.com>
> > > Cc: Peter Huewe <peterhuewe@gmx.de>
> > > Cc: Jason Gunthorpe <jgg@ziepe.ca>
> > > Cc: linux-kernel@vger.kernel.org
> > > Cc: linux-stable@vger.kernel.org
> > > Fixes: a3fbfae82b4c ("tpm: take TPM chip power gating out of tpm_transmit()")
> > > Reported-by: Christian Bundy <christianbundy@fraction.io>
> > > Signed-off-by: Jerry Snitselaar <jsnitsel@redhat.com>
> > >  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

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

[Jerry Snitselaar]

On Tue, Nov 12, 2019 at 1:26 PM Jason Gunthorpe <jgg@ziepe.ca> 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
> > <jarkko.sakkinen@linux.intel.com> 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 <jarkko.sakkinen@linux.intel.com>
> > > > Cc: Peter Huewe <peterhuewe@gmx.de>
> > > > Cc: Jason Gunthorpe <jgg@ziepe.ca>
> > > > Cc: linux-kernel@vger.kernel.org
> > > > Cc: linux-stable@vger.kernel.org
> > > > Fixes: a3fbfae82b4c ("tpm: take TPM chip power gating out of tpm_transmit()")
> > > > Reported-by: Christian Bundy <christianbundy@fraction.io>
> > > > Signed-off-by: Jerry Snitselaar <jsnitsel@redhat.com>
> > > >  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)?

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

[Jerry Snitselaar]

On Tue, Nov 12, 2019 at 1:28 PM Jerry Snitselaar <jsnitsel@redhat.com> wrote:
>
> On Tue, Nov 12, 2019 at 1:26 PM Jason Gunthorpe <jgg@ziepe.ca> 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
> > > <jarkko.sakkinen@linux.intel.com> 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 <jarkko.sakkinen@linux.intel.com>
> > > > > Cc: Peter Huewe <peterhuewe@gmx.de>
> > > > > Cc: Jason Gunthorpe <jgg@ziepe.ca>
> > > > > Cc: linux-kernel@vger.kernel.org
> > > > > Cc: linux-stable@vger.kernel.org
> > > > > Fixes: a3fbfae82b4c ("tpm: take TPM chip power gating out of tpm_transmit()")
> > > > > Reported-by: Christian Bundy <christianbundy@fraction.io>
> > > > > Signed-off-by: Jerry Snitselaar <jsnitsel@redhat.com>
> > > > >  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

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

[Jason Gunthorpe]

On Tue, Nov 12, 2019 at 01:31:09PM -0700, Jerry Snitselaar wrote:
> On Tue, Nov 12, 2019 at 1:28 PM Jerry Snitselaar <jsnitsel@redhat.com> wrote:
> >
> > On Tue, Nov 12, 2019 at 1:26 PM Jason Gunthorpe <jgg@ziepe.ca> 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
> > > > <jarkko.sakkinen@linux.intel.com> 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 <jarkko.sakkinen@linux.intel.com>
> > > > > > Cc: Peter Huewe <peterhuewe@gmx.de>
> > > > > > Cc: Jason Gunthorpe <jgg@ziepe.ca>
> > > > > > Cc: linux-kernel@vger.kernel.org
> > > > > > Cc: linux-stable@vger.kernel.org
> > > > > > Fixes: a3fbfae82b4c ("tpm: take TPM chip power gating out of tpm_transmit()")
> > > > > > Reported-by: Christian Bundy <christianbundy@fraction.io>
> > > > > > Signed-off-by: Jerry Snitselaar <jsnitsel@redhat.com>
> > > > > >  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 

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

[Jerry Snitselaar]

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 <jsnitsel@redhat.com> wrote:
>> >
>> > On Tue, Nov 12, 2019 at 1:26 PM Jason Gunthorpe <jgg@ziepe.ca> 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
>> > > > <jarkko.sakkinen@linux.intel.com> 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 <jarkko.sakkinen@linux.intel.com>
>> > > > > > Cc: Peter Huewe <peterhuewe@gmx.de>
>> > > > > > Cc: Jason Gunthorpe <jgg@ziepe.ca>
>> > > > > > Cc: linux-kernel@vger.kernel.org
>> > > > > > Cc: linux-stable@vger.kernel.org
>> > > > > > Fixes: a3fbfae82b4c ("tpm: take TPM chip power gating out of tpm_transmit()")
>> > > > > > Reported-by: Christian Bundy <christianbundy@fraction.io>
>> > > > > > Signed-off-by: Jerry Snitselaar <jsnitsel@redhat.com>
>> > > > > >  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?

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

[Jerry Snitselaar]

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 <jarkko.sakkinen@linux.intel.com>
Cc: Peter Huewe <peterhuewe@gmx.de>
Cc: Jason Gunthorpe <jgg@ziepe.ca>
Cc: linux-kernel@vger.kernel.org
Cc: stable@vger.kernel.org
Fixes: a3fbfae82b4c ("tpm: take TPM chip power gating out of tpm_transmit()")
Reported-by: Christian Bundy <christianbundy@fraction.io>
Signed-off-by: Jerry Snitselaar <jsnitsel@redhat.com>
---
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

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

[Jarkko Sakkinen]

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
> > <jarkko.sakkinen@linux.intel.com> 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 <jarkko.sakkinen@linux.intel.com>
> > > > Cc: Peter Huewe <peterhuewe@gmx.de>
> > > > Cc: Jason Gunthorpe <jgg@ziepe.ca>
> > > > Cc: linux-kernel@vger.kernel.org
> > > > Cc: linux-stable@vger.kernel.org
> > > > Fixes: a3fbfae82b4c ("tpm: take TPM chip power gating out of tpm_transmit()")
> > > > Reported-by: Christian Bundy <christianbundy@fraction.io>
> > > > Signed-off-by: Jerry Snitselaar <jsnitsel@redhat.com>
> > > >  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

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

[Jason Gunthorpe]

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
> > > <jarkko.sakkinen@linux.intel.com> 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 <jarkko.sakkinen@linux.intel.com>
> > > > > Cc: Peter Huewe <peterhuewe@gmx.de>
> > > > > Cc: Jason Gunthorpe <jgg@ziepe.ca>
> > > > > Cc: linux-kernel@vger.kernel.org
> > > > > Cc: linux-stable@vger.kernel.org
> > > > > Fixes: a3fbfae82b4c ("tpm: take TPM chip power gating out of tpm_transmit()")
> > > > > Reported-by: Christian Bundy <christianbundy@fraction.io>
> > > > > Signed-off-by: Jerry Snitselaar <jsnitsel@redhat.com>
> > > > >  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

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

[Jarkko Sakkinen]

On Tue, Nov 12, 2019 at 01:28:49PM -0700, Jerry Snitselaar wrote:
> On Tue, Nov 12, 2019 at 1:26 PM Jason Gunthorpe <jgg@ziepe.ca> 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
> > > <jarkko.sakkinen@linux.intel.com> 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 <jarkko.sakkinen@linux.intel.com>
> > > > > Cc: Peter Huewe <peterhuewe@gmx.de>
> > > > > Cc: Jason Gunthorpe <jgg@ziepe.ca>
> > > > > Cc: linux-kernel@vger.kernel.org
> > > > > Cc: linux-stable@vger.kernel.org
> > > > > Fixes: a3fbfae82b4c ("tpm: take TPM chip power gating out of tpm_transmit()")
> > > > > Reported-by: Christian Bundy <christianbundy@fraction.io>
> > > > > Signed-off-by: Jerry Snitselaar <jsnitsel@redhat.com>
> > > > >  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

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

[Jason Gunthorpe]

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

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

[Jarkko Sakkinen]

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 <jarkko.sakkinen@linux.intel.com>
> Cc: Peter Huewe <peterhuewe@gmx.de>
> Cc: Jason Gunthorpe <jgg@ziepe.ca>
> Cc: linux-kernel@vger.kernel.org
> Cc: stable@vger.kernel.org
> Fixes: a3fbfae82b4c ("tpm: take TPM chip power gating out of tpm_transmit()")
> Reported-by: Christian Bundy <christianbundy@fraction.io>
> Signed-off-by: Jerry Snitselaar <jsnitsel@redhat.com>
> ---
> 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

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

[Jarkko Sakkinen]

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

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

[Jason Gunthorpe]

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

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

[Jarkko Sakkinen]

On Fri, Nov 15, 2019 at 02:36:21PM -0400, Jason Gunthorpe wrote:
> 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()

I do not see any sense calling from get_timeouts() from call sites
in the same initialization flow.

/Jarkko