LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] tpm: fix suspend/resume paths for TPM 2.0
@ 2015-01-27 11:02 Jarkko Sakkinen
  2015-01-27 16:52 ` [tpmdd-devel] " Scot Doyle
  0 siblings, 1 reply; 6+ messages in thread
From: Jarkko Sakkinen @ 2015-01-27 11:02 UTC (permalink / raw)
  To: Peter Huewe, Ashley Lai, Marcel Selhorst
  Cc: tpmdd-devel, linux-kernel, josh, christophe.ricard,
	jason.gunthorpe, stefanb, linux-api, trousers-tech, jmorris,
	Jarkko Sakkinen

Fixed suspend/resume paths for TPM 2.0 and consolidated all the
associated code to the tpm_pm_suspend() and tpm_pm_resume()
functions. Resume path should be handled by the firmware, i.e.
Startup(CLEAR) for hibernate and Startup(STATE) for suspend.

There might be some non-PC embedded devices in the future where
Startup() is not the handled by the FW but fixing the code for
those IMHO should be postponed until there is hardware available
to test the fixes although extra Startup in the driver code is
essentially a NOP.

Reported-by: Peter Hüwe <PeterHuewe@gmx.de>
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 drivers/char/tpm/tpm-interface.c | 10 ++++++++--
 drivers/char/tpm/tpm_crb.c       | 16 +---------------
 drivers/char/tpm/tpm_tis.c       | 22 ++++++++++------------
 3 files changed, 19 insertions(+), 29 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index bf53a37..93c8b90fd 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -901,8 +901,13 @@ int tpm_pm_suspend(struct device *dev)
 	if (chip == NULL)
 		return -ENODEV;
 
-	if (chip->flags & TPM_CHIP_FLAG_TPM2)
-		return tpm2_shutdown(chip, TPM2_SU_CLEAR);
+	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
+		rc = tpm2_shutdown(chip, TPM2_SU_STATE);
+		if (rc < 0)
+			return rc;
+
+		return 0;
+	}
 
 	/* for buggy tpm, flush pcrs with extend to selected dummy */
 	if (tpm_suspend_pcr) {
@@ -952,6 +957,7 @@ EXPORT_SYMBOL_GPL(tpm_pm_suspend);
 int tpm_pm_resume(struct device *dev)
 {
 	struct tpm_chip *chip = dev_get_drvdata(dev);
+	int ret;
 
 	if (chip == NULL)
 		return -ENODEV;
diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index 3dd23cf..3e080f5 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -95,21 +95,7 @@ struct crb_priv {
 	u8 __iomem *rsp;
 };
 
-#ifdef CONFIG_PM_SLEEP
-static int crb_resume(struct device *dev)
-{
-	int rc;
-	struct tpm_chip *chip = dev_get_drvdata(dev);
-
-	rc = tpm2_shutdown(chip, TPM2_SU_STATE);
-	if (!rc)
-		rc = tpm2_do_selftest(chip);
-
-	return rc;
-}
-#endif
-
-static SIMPLE_DEV_PM_OPS(crb_pm, tpm_pm_suspend, crb_resume);
+static SIMPLE_DEV_PM_OPS(crb_pm, tpm_pm_suspend, tpm_pm_resume);
 
 static u8 crb_status(struct tpm_chip *chip)
 {
diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index 6725bef..c105eb7 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -865,25 +865,23 @@ static void tpm_tis_reenable_interrupts(struct tpm_chip *chip)
 static int tpm_tis_resume(struct device *dev)
 {
 	struct tpm_chip *chip = dev_get_drvdata(dev);
-	int ret = 0;
+	int ret;
 
 	if (chip->vendor.irq)
 		tpm_tis_reenable_interrupts(chip);
 
-	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
-		/* NOP if firmware properly does this. */
-		tpm2_startup(chip, TPM2_SU_STATE);
+	ret = tpm_pm_resume(dev);
+	if (ret)
+		return ret;
 
-		ret = tpm2_shutdown(chip, TPM2_SU_STATE);
-		if (!ret)
-			ret = tpm2_do_selftest(chip);
-	} else {
-		ret = tpm_pm_resume(dev);
-		if (!ret)
-			tpm_do_selftest(chip);
+	/* TPM 1.2 requires self-test on resume. */
+	if (!(chip->flags & TPM_CHIP_FLAG_TPM2)) {
+		ret = tpm_do_selftest(chip);
+		if (ret < 0)
+			return ret;
 	}
 
-	return ret;
+	return 0;
 }
 #endif
 
-- 
2.1.4


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [tpmdd-devel] [PATCH] tpm: fix suspend/resume paths for TPM 2.0
  2015-01-27 11:02 [PATCH] tpm: fix suspend/resume paths for TPM 2.0 Jarkko Sakkinen
@ 2015-01-27 16:52 ` Scot Doyle
  2015-01-27 16:57   ` Jarkko Sakkinen
  0 siblings, 1 reply; 6+ messages in thread
From: Scot Doyle @ 2015-01-27 16:52 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: peterhuewe, ashley, christophe.ricard, jason.gunthorpe,
	linux-api, linux-kernel, tpmdd-devel, trousers-tech

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1214 bytes --]

On Tue, 27 Jan 2015, Jarkko Sakkinen wrote:
> Fixed suspend/resume paths for TPM 2.0 and consolidated all the
> associated code to the tpm_pm_suspend() and tpm_pm_resume()
> functions. Resume path should be handled by the firmware, i.e.
> Startup(CLEAR) for hibernate and Startup(STATE) for suspend.
> 
> There might be some non-PC embedded devices in the future where
> Startup() is not the handled by the FW but fixing the code for
> those IMHO should be postponed until there is hardware available
> to test the fixes although extra Startup in the driver code is
> essentially a NOP.
> 
> Reported-by: Peter Hüwe <PeterHuewe@gmx.de>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> ---

...

> --- a/drivers/char/tpm/tpm_tis.c
> +++ b/drivers/char/tpm/tpm_tis.c
> @@ -865,25 +865,23 @@ static void tpm_tis_reenable_interrupts(struct tpm_chip *chip)
>  static int tpm_tis_resume(struct device *dev)
>  {

...

> +	/* TPM 1.2 requires self-test on resume. */
> +	if (!(chip->flags & TPM_CHIP_FLAG_TPM2)) {
> +		ret = tpm_do_selftest(chip);
> +		if (ret < 0)
> +			return ret;

Just to note, the return value from tpm_do_selftest() on TPM 1.2 chips was 
previously ignored. Mine does return 0.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [tpmdd-devel] [PATCH] tpm: fix suspend/resume paths for TPM 2.0
  2015-01-27 16:52 ` [tpmdd-devel] " Scot Doyle
@ 2015-01-27 16:57   ` Jarkko Sakkinen
  2015-01-27 17:03     ` Jason Gunthorpe
  0 siblings, 1 reply; 6+ messages in thread
From: Jarkko Sakkinen @ 2015-01-27 16:57 UTC (permalink / raw)
  To: Scot Doyle
  Cc: peterhuewe, ashley, christophe.ricard, jason.gunthorpe,
	linux-api, linux-kernel, tpmdd-devel, trousers-tech

On Tue, 2015-01-27 at 16:52 +0000, Scot Doyle wrote:
> On Tue, 27 Jan 2015, Jarkko Sakkinen wrote:
> > Fixed suspend/resume paths for TPM 2.0 and consolidated all the
> > associated code to the tpm_pm_suspend() and tpm_pm_resume()
> > functions. Resume path should be handled by the firmware, i.e.
> > Startup(CLEAR) for hibernate and Startup(STATE) for suspend.
> > 
> > There might be some non-PC embedded devices in the future where
> > Startup() is not the handled by the FW but fixing the code for
> > those IMHO should be postponed until there is hardware available
> > to test the fixes although extra Startup in the driver code is
> > essentially a NOP.
> > 
> > Reported-by: Peter Hüwe <PeterHuewe@gmx.de>
> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > ---
> 
> ...
> 
> > --- a/drivers/char/tpm/tpm_tis.c
> > +++ b/drivers/char/tpm/tpm_tis.c
> > @@ -865,25 +865,23 @@ static void tpm_tis_reenable_interrupts(struct tpm_chip *chip)
> >  static int tpm_tis_resume(struct device *dev)
> >  {
> 
> ...
> 
> > +	/* TPM 1.2 requires self-test on resume. */
> > +	if (!(chip->flags & TPM_CHIP_FLAG_TPM2)) {
> > +		ret = tpm_do_selftest(chip);
> > +		if (ret < 0)
> > +			return ret;
> 
> Just to note, the return value from tpm_do_selftest() on TPM 1.2 chips was 
> previously ignored. Mine does return 0.

Right. I can update the patch to ignore return value if the majority
wants that.

/Jarkko


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [tpmdd-devel] [PATCH] tpm: fix suspend/resume paths for TPM 2.0
  2015-01-27 16:57   ` Jarkko Sakkinen
@ 2015-01-27 17:03     ` Jason Gunthorpe
  2015-01-27 17:23       ` Jarkko Sakkinen
  0 siblings, 1 reply; 6+ messages in thread
From: Jason Gunthorpe @ 2015-01-27 17:03 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: Scot Doyle, peterhuewe, ashley, christophe.ricard,
	jason.gunthorpe, linux-api, linux-kernel, tpmdd-devel,
	trousers-tech

On Tue, Jan 27, 2015 at 06:57:22PM +0200, Jarkko Sakkinen wrote:
> > > +	/* TPM 1.2 requires self-test on resume. */
> > > +	if (!(chip->flags & TPM_CHIP_FLAG_TPM2)) {
> > > +		ret = tpm_do_selftest(chip);
> > > +		if (ret < 0)
> > > +			return ret;
> > 
> > Just to note, the return value from tpm_do_selftest() on TPM 1.2 chips was 
> > previously ignored. Mine does return 0.
> 
> Right. I can update the patch to ignore return value if the majority
> wants that.

What happens to the system when pnp_driver.resume() returns failure?

Should tpm ever report failure on resume to the rest of the kernel?

Shouldn't this stuff be in tpm_pm_resume common code anyhow?

Jason

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [tpmdd-devel] [PATCH] tpm: fix suspend/resume paths for TPM 2.0
  2015-01-27 17:03     ` Jason Gunthorpe
@ 2015-01-27 17:23       ` Jarkko Sakkinen
  2015-01-27 17:33         ` Jarkko Sakkinen
  0 siblings, 1 reply; 6+ messages in thread
From: Jarkko Sakkinen @ 2015-01-27 17:23 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: Scot Doyle, peterhuewe, ashley, christophe.ricard,
	jason.gunthorpe, linux-api, linux-kernel, tpmdd-devel,
	trousers-tech

On Tue, 2015-01-27 at 10:03 -0700, Jason Gunthorpe wrote:
> On Tue, Jan 27, 2015 at 06:57:22PM +0200, Jarkko Sakkinen wrote:
> > > > +	/* TPM 1.2 requires self-test on resume. */
> > > > +	if (!(chip->flags & TPM_CHIP_FLAG_TPM2)) {
> > > > +		ret = tpm_do_selftest(chip);
> > > > +		if (ret < 0)
> > > > +			return ret;
> > > 
> > > Just to note, the return value from tpm_do_selftest() on TPM 1.2 chips was 
> > > previously ignored. Mine does return 0.
> > 
> > Right. I can update the patch to ignore return value if the majority
> > wants that.
> 
> What happens to the system when pnp_driver.resume() returns failure?
> 
> Should tpm ever report failure on resume to the rest of the kernel?
> 
> Shouldn't this stuff be in tpm_pm_resume common code anyhow?

I think it should but not in the scope of this bug fix IMHO.

> Jason

/Jarkko


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [tpmdd-devel] [PATCH] tpm: fix suspend/resume paths for TPM 2.0
  2015-01-27 17:23       ` Jarkko Sakkinen
@ 2015-01-27 17:33         ` Jarkko Sakkinen
  0 siblings, 0 replies; 6+ messages in thread
From: Jarkko Sakkinen @ 2015-01-27 17:33 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: Scot Doyle, peterhuewe, ashley, christophe.ricard,
	jason.gunthorpe, linux-api, linux-kernel, tpmdd-devel,
	trousers-tech

On Tue, 2015-01-27 at 19:23 +0200, Jarkko Sakkinen wrote:
> On Tue, 2015-01-27 at 10:03 -0700, Jason Gunthorpe wrote:
> > On Tue, Jan 27, 2015 at 06:57:22PM +0200, Jarkko Sakkinen wrote:
> > > > > +	/* TPM 1.2 requires self-test on resume. */
> > > > > +	if (!(chip->flags & TPM_CHIP_FLAG_TPM2)) {
> > > > > +		ret = tpm_do_selftest(chip);
> > > > > +		if (ret < 0)
> > > > > +			return ret;
> > > > 
> > > > Just to note, the return value from tpm_do_selftest() on TPM 1.2 chips was 
> > > > previously ignored. Mine does return 0.
> > > 
> > > Right. I can update the patch to ignore return value if the majority
> > > wants that.
> > 
> > What happens to the system when pnp_driver.resume() returns failure?
> > 
> > Should tpm ever report failure on resume to the rest of the kernel?
> > 
> > Shouldn't this stuff be in tpm_pm_resume common code anyhow?
> 
> I think it should but not in the scope of this bug fix IMHO.

This may sound stupid but maybe I should not handle the return value of
tpm_do_selftest() with the same reasoning (not in the scope of this fix)
because it modifies semantics and my fix only fixes TPM 2.0 stuff.

I could leave a comment there that this return value is not handle as a
remainder.

> > Jason

/Jarkko


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2015-01-27 17:33 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-01-27 11:02 [PATCH] tpm: fix suspend/resume paths for TPM 2.0 Jarkko Sakkinen
2015-01-27 16:52 ` [tpmdd-devel] " Scot Doyle
2015-01-27 16:57   ` Jarkko Sakkinen
2015-01-27 17:03     ` Jason Gunthorpe
2015-01-27 17:23       ` Jarkko Sakkinen
2015-01-27 17:33         ` Jarkko Sakkinen

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).