| Message ID | 20191111233418.17676-1-jsnitsel@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tpm_tis: turn on TPM before calling tpm_get_timeouts | expand |
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
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.
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
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)?
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
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
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?
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
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
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
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
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
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
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
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,
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(-)