[4/4] tmp/tpm_crb: implement runtime pm for tpm_crb

Commit Message

Winkler, Tomas Oct. 8, 2016, 11:59 a.m. UTC

From: "Winkler, Tomas" <[e-mail]>

Utilize runtime_pm for driving tpm crb idle states.
The framework calls cmd_ready from the pm_runtime_resume handler
and go idle from the pm_runtime_suspend handler.
The TPM framework should wake the device before transmit and receive.
In case the runtime_pm framework is not compiled in or enabled, the device
will be in the permanent ready state.

Signed-off-by: Tomas Winkler <[e-mail]>
---
V2: new in the series
V3: resend
V4: fix unbalanced runtime pm put and get

 drivers/char/tpm/tpm-interface.c |  5 +++++
 drivers/char/tpm/tpm_crb.c       | 42 ++++++++++++++++++++++++++++++++++++----
 2 files changed, 43 insertions(+), 4 deletions(-)

Comments

Jarkko Sakkinen Oct. 8, 2016, 12:47 p.m. UTC | #1

On Sat, Oct 08, 2016 at 02:59:39PM +0300, Tomas Winkler wrote:
> From: "Winkler, Tomas" <[e-mail]>
> 
> Utilize runtime_pm for driving tpm crb idle states.
> The framework calls cmd_ready from the pm_runtime_resume handler
> and go idle from the pm_runtime_suspend handler.
> The TPM framework should wake the device before transmit and receive.
> In case the runtime_pm framework is not compiled in or enabled, the device
> will be in the permanent ready state.
> 
> Signed-off-by: Tomas Winkler <[e-mail]>

I would rather want the fix as a separate patch to make review + testing
easier (without and with). Thanks.

/Jarkko

> ---
> V2: new in the series
> V3: resend
> V4: fix unbalanced runtime pm put and get
> 
>  drivers/char/tpm/tpm-interface.c |  5 +++++
>  drivers/char/tpm/tpm_crb.c       | 42 ++++++++++++++++++++++++++++++++++++----
>  2 files changed, 43 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index 8de61876f633..7743e8a21b82 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -29,6 +29,7 @@
>  #include <linux/mutex.h>
>  #include <linux/spinlock.h>
>  #include <linux/freezer.h>
> +#include <linux/pm_runtime.h>
>  
>  #include "tpm.h"
>  #include "tpm_eventlog.h"
> @@ -356,6 +357,8 @@ ssize_t tpm_transmit(struct tpm_chip *chip, const u8 *buf, size_t bufsiz,
>  	if (!(flags & TPM_TRANSMIT_UNLOCKED))
>  		mutex_lock(&chip->tpm_mutex);
>  
> +	pm_runtime_get_sync(chip->dev.parent);
> +
>  	rc = chip->ops->send(chip, (u8 *) buf, count);
>  	if (rc < 0) {
>  		dev_err(&chip->dev,
> @@ -397,6 +400,8 @@ out_recv:
>  		dev_err(&chip->dev,
>  			"tpm_transmit: tpm_recv: error %zd\n", rc);
>  out:
> +	pm_runtime_put_sync(chip->dev.parent);
> +
>  	if (!(flags & TPM_TRANSMIT_UNLOCKED))
>  		mutex_unlock(&chip->tpm_mutex);
>  	return rc;
> diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> index def50b7d47b2..eccd82af6215 100644
> --- a/drivers/char/tpm/tpm_crb.c
> +++ b/drivers/char/tpm/tpm_crb.c
> @@ -19,6 +19,7 @@
>  #include <linux/highmem.h>
>  #include <linux/rculist.h>
>  #include <linux/module.h>
> +#include <linux/pm_runtime.h>
>  #include "tpm.h"
>  
>  #define ACPI_SIG_TPM2 "TPM2"
> @@ -153,8 +154,6 @@ static int __maybe_unused crb_cmd_ready(struct device *dev,
>  	return 0;
>  }
>  
> -static SIMPLE_DEV_PM_OPS(crb_pm, tpm_pm_suspend, tpm_pm_resume);
> -
>  static u8 crb_status(struct tpm_chip *chip)
>  {
>  	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
> @@ -437,11 +436,21 @@ static int crb_acpi_add(struct acpi_device *device)
>  	if (rc)
>  		return rc;
>  
> +	pm_runtime_get_noresume(dev);
> +	pm_runtime_set_active(dev);
> +	pm_runtime_enable(dev);
> +
>  	rc = tpm_chip_register(chip);
> -	if (rc)
> +	if (rc) {
>  		crb_go_idle(dev, priv);
> +		pm_runtime_put_noidle(dev);
> +		pm_runtime_disable(dev);
> +		return rc;
> +	}
>  
> -	return rc;
> +	pm_runtime_put(dev);
> +
> +	return 0;
>  }
>  
>  static int crb_acpi_remove(struct acpi_device *device)
> @@ -451,9 +460,34 @@ static int crb_acpi_remove(struct acpi_device *device)
>  
>  	tpm_chip_unregister(chip);
>  
> +	pm_runtime_disable(dev);
> +
>  	return 0;
>  }
>  
> +#ifdef CONFIG_PM
> +static int crb_pm_runtime_suspend(struct device *dev)
> +{
> +	struct tpm_chip *chip = dev_get_drvdata(dev);
> +	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
> +
> +	return crb_go_idle(dev, priv);
> +}
> +
> +static int crb_pm_runtime_resume(struct device *dev)
> +{
> +	struct tpm_chip *chip = dev_get_drvdata(dev);
> +	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
> +
> +	return crb_cmd_ready(dev, priv);
> +}
> +#endif /* CONFIG_PM */
> +
> +static const struct dev_pm_ops crb_pm = {
> +	SET_SYSTEM_SLEEP_PM_OPS(tpm_pm_suspend, tpm_pm_resume)
> +	SET_RUNTIME_PM_OPS(crb_pm_runtime_suspend, crb_pm_runtime_resume, NULL)
> +};
> +
>  static struct acpi_device_id crb_device_ids[] = {
>  	{"MSFT0101", 0},
>  	{"", 0},
> -- 
> 2.7.4
> 

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

Winkler, Tomas Oct. 8, 2016, 1:37 p.m. UTC | #2

> 
> On Sat, Oct 08, 2016 at 02:59:39PM +0300, Tomas Winkler wrote:
> > From: "Winkler, Tomas" <[e-mail]>
> >
> > Utilize runtime_pm for driving tpm crb idle states.
> > The framework calls cmd_ready from the pm_runtime_resume handler and
> > go idle from the pm_runtime_suspend handler.
> > The TPM framework should wake the device before transmit and receive.
> > In case the runtime_pm framework is not compiled in or enabled, the
> > device will be in the permanent ready state.
> >
> > Signed-off-by: Tomas Winkler <[e-mail]>
> 
> I would rather want the fix as a separate patch to make review + testing easier
> (without and with). Thanks.

But you've dropped the patches, so I've resent them. 
Can you do the diff yourself, just branch it off.  it's really just few lines 

Anyhow here is output of your smoke tests 
sudo python -m unittest -v tpm2_smoke

test_seal_with_auth (tpm2_smoke.SmokeTest) ... ok
test_seal_with_policy (tpm2_smoke.SmokeTest) ... ok
test_seal_with_too_long_auth (tpm2_smoke.SmokeTest) ... ok
test_unseal_with_wrong_auth (tpm2_smoke.SmokeTest) ... ok
test_unseal_with_wrong_policy (tpm2_smoke.SmokeTest) ... ok

----------------------------------------------------------------------
Ran 5 tests in 26.186s

OK
Tomas 

> 
> /Jarkko
> 
> > ---
> > V2: new in the series
> > V3: resend
> > V4: fix unbalanced runtime pm put and get
> >
> >  drivers/char/tpm/tpm-interface.c |  5 +++++
> >  drivers/char/tpm/tpm_crb.c       | 42
> ++++++++++++++++++++++++++++++++++++----
> >  2 files changed, 43 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/char/tpm/tpm-interface.c
> > b/drivers/char/tpm/tpm-interface.c
> > index 8de61876f633..7743e8a21b82 100644
> > --- a/drivers/char/tpm/tpm-interface.c
> > +++ b/drivers/char/tpm/tpm-interface.c
> > @@ -29,6 +29,7 @@
> >  #include <linux/mutex.h>
> >  #include <linux/spinlock.h>
> >  #include <linux/freezer.h>
> > +#include <linux/pm_runtime.h>
> >
> >  #include "tpm.h"
> >  #include "tpm_eventlog.h"
> > @@ -356,6 +357,8 @@ ssize_t tpm_transmit(struct tpm_chip *chip, const u8
> *buf, size_t bufsiz,
> >  	if (!(flags & TPM_TRANSMIT_UNLOCKED))
> >  		mutex_lock(&chip->tpm_mutex);
> >
> > +	pm_runtime_get_sync(chip->dev.parent);
> > +
> >  	rc = chip->ops->send(chip, (u8 *) buf, count);
> >  	if (rc < 0) {
> >  		dev_err(&chip->dev,
> > @@ -397,6 +400,8 @@ out_recv:
> >  		dev_err(&chip->dev,
> >  			"tpm_transmit: tpm_recv: error %zd\n", rc);
> >  out:
> > +	pm_runtime_put_sync(chip->dev.parent);
> > +
> >  	if (!(flags & TPM_TRANSMIT_UNLOCKED))
> >  		mutex_unlock(&chip->tpm_mutex);
> >  	return rc;
> > diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> > index def50b7d47b2..eccd82af6215 100644
> > --- a/drivers/char/tpm/tpm_crb.c
> > +++ b/drivers/char/tpm/tpm_crb.c
> > @@ -19,6 +19,7 @@
> >  #include <linux/highmem.h>
> >  #include <linux/rculist.h>
> >  #include <linux/module.h>
> > +#include <linux/pm_runtime.h>
> >  #include "tpm.h"
> >
> >  #define ACPI_SIG_TPM2 "TPM2"
> > @@ -153,8 +154,6 @@ static int __maybe_unused crb_cmd_ready(struct
> device *dev,
> >  	return 0;
> >  }
> >
> > -static SIMPLE_DEV_PM_OPS(crb_pm, tpm_pm_suspend, tpm_pm_resume);
> > -
> >  static u8 crb_status(struct tpm_chip *chip)  {
> >  	struct crb_priv *priv = dev_get_drvdata(&chip->dev); @@ -437,11
> > +436,21 @@ static int crb_acpi_add(struct acpi_device *device)
> >  	if (rc)
> >  		return rc;
> >
> > +	pm_runtime_get_noresume(dev);
> > +	pm_runtime_set_active(dev);
> > +	pm_runtime_enable(dev);
> > +
> >  	rc = tpm_chip_register(chip);
> > -	if (rc)
> > +	if (rc) {
> >  		crb_go_idle(dev, priv);
> > +		pm_runtime_put_noidle(dev);
> > +		pm_runtime_disable(dev);
> > +		return rc;
> > +	}
> >
> > -	return rc;
> > +	pm_runtime_put(dev);
> > +
> > +	return 0;
> >  }
> >
> >  static int crb_acpi_remove(struct acpi_device *device) @@ -451,9
> > +460,34 @@ static int crb_acpi_remove(struct acpi_device *device)
> >
> >  	tpm_chip_unregister(chip);
> >
> > +	pm_runtime_disable(dev);
> > +
> >  	return 0;
> >  }
> >
> > +#ifdef CONFIG_PM
> > +static int crb_pm_runtime_suspend(struct device *dev) {
> > +	struct tpm_chip *chip = dev_get_drvdata(dev);
> > +	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
> > +
> > +	return crb_go_idle(dev, priv);
> > +}
> > +
> > +static int crb_pm_runtime_resume(struct device *dev) {
> > +	struct tpm_chip *chip = dev_get_drvdata(dev);
> > +	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
> > +
> > +	return crb_cmd_ready(dev, priv);
> > +}
> > +#endif /* CONFIG_PM */
> > +
> > +static const struct dev_pm_ops crb_pm = {
> > +	SET_SYSTEM_SLEEP_PM_OPS(tpm_pm_suspend, tpm_pm_resume)
> > +	SET_RUNTIME_PM_OPS(crb_pm_runtime_suspend,
> crb_pm_runtime_resume,
> > +NULL) };
> > +
> >  static struct acpi_device_id crb_device_ids[] = {
> >  	{"MSFT0101", 0},
> >  	{"", 0},
> > --
> > 2.7.4
> >

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

Jarkko Sakkinen Oct. 8, 2016, 4:01 p.m. UTC | #3

On Sat, Oct 08, 2016 at 01:37:31PM +0000, Winkler, Tomas wrote:
> > 
> > On Sat, Oct 08, 2016 at 02:59:39PM +0300, Tomas Winkler wrote:
> > > From: "Winkler, Tomas" <[e-mail]>
> > >
> > > Utilize runtime_pm for driving tpm crb idle states.
> > > The framework calls cmd_ready from the pm_runtime_resume handler and
> > > go idle from the pm_runtime_suspend handler.
> > > The TPM framework should wake the device before transmit and receive.
> > > In case the runtime_pm framework is not compiled in or enabled, the
> > > device will be in the permanent ready state.
> > >
> > > Signed-off-by: Tomas Winkler <[e-mail]>
> > 
> > I would rather want the fix as a separate patch to make review + testing easier
> > (without and with). Thanks.
> 
> But you've dropped the patches, so I've resent them. 
> Can you do the diff yourself, just branch it off.  it's really just few lines 

Nope. I have only dropped the workaround.

/Jarkko

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

Winkler, Tomas Oct. 8, 2016, 6:18 p.m. UTC | #4

> -----Original Message-----
> From: Jarkko Sakkinen [mailto:[e-mail]]
> Sent: Saturday, October 08, 2016 19:01
> To: Winkler, Tomas <[e-mail]>
> Cc: [e-mail]; Jason Gunthorpe
> <[e-mail]>; [e-mail]
> Subject: Re: [PATCH 4/4] tmp/tpm_crb: implement runtime pm for tpm_crb
> 
> On Sat, Oct 08, 2016 at 01:37:31PM +0000, Winkler, Tomas wrote:
> > >
> > > On Sat, Oct 08, 2016 at 02:59:39PM +0300, Tomas Winkler wrote:
> > > > From: "Winkler, Tomas" <[e-mail]>
> > > >
> > > > Utilize runtime_pm for driving tpm crb idle states.
> > > > The framework calls cmd_ready from the pm_runtime_resume handler
> > > > and go idle from the pm_runtime_suspend handler.
> > > > The TPM framework should wake the device before transmit and receive.
> > > > In case the runtime_pm framework is not compiled in or enabled,
> > > > the device will be in the permanent ready state.
> > > >
> > > > Signed-off-by: Tomas Winkler <[e-mail]>
> > >
> > > I would rather want the fix as a separate patch to make review +
> > > testing easier (without and with). Thanks.
> >
> > But you've dropped the patches, so I've resent them.
> > Can you do the diff yourself, just branch it off.  it's really just
> > few lines
> 
> Nope. I have only dropped the workaround.

This is what is current in the linus tree:

git log --oneline linux/master -- drivers/char/tpm/

324152502b0e Revert "tpm/tpm_crb: implement tpm crb idle state"
cfa188220363 Revert "tmp/tpm_crb: fix Intel PTT hw bug during idle state"
2b7926ae1cd4 Revert "tpm/tpm_crb: open code the crb_init into acpi_add"
4886cd80cb8e Revert "tmp/tpm_crb: implement runtime pm for tpm_crb"
e350e24694e4 tmp/tpm_crb: implement runtime pm for tpm_crb
0c22db435bf7 tpm/tpm_crb: open code the crb_init into acpi_add
9514ff1961c6 tmp/tpm_crb: fix Intel PTT hw bug during idle state
e17acbbb69d3 tpm/tpm_crb: implement tpm crb idle state


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

Jarkko Sakkinen Oct. 8, 2016, 6:43 p.m. UTC | #5

On Sat, Oct 08, 2016 at 06:18:36PM +0000, Winkler, Tomas wrote:
> 
> 
> > -----Original Message-----
> > From: Jarkko Sakkinen [mailto:[e-mail]]
> > Sent: Saturday, October 08, 2016 19:01
> > To: Winkler, Tomas <[e-mail]>
> > Cc: [e-mail]; Jason Gunthorpe
> > <[e-mail]>; [e-mail]
> > Subject: Re: [PATCH 4/4] tmp/tpm_crb: implement runtime pm for tpm_crb
> > 
> > On Sat, Oct 08, 2016 at 01:37:31PM +0000, Winkler, Tomas wrote:
> > > >
> > > > On Sat, Oct 08, 2016 at 02:59:39PM +0300, Tomas Winkler wrote:
> > > > > From: "Winkler, Tomas" <[e-mail]>
> > > > >
> > > > > Utilize runtime_pm for driving tpm crb idle states.
> > > > > The framework calls cmd_ready from the pm_runtime_resume handler
> > > > > and go idle from the pm_runtime_suspend handler.
> > > > > The TPM framework should wake the device before transmit and receive.
> > > > > In case the runtime_pm framework is not compiled in or enabled,
> > > > > the device will be in the permanent ready state.
> > > > >
> > > > > Signed-off-by: Tomas Winkler <[e-mail]>
> > > >
> > > > I would rather want the fix as a separate patch to make review +
> > > > testing easier (without and with). Thanks.
> > >
> > > But you've dropped the patches, so I've resent them.
> > > Can you do the diff yourself, just branch it off.  it's really just
> > > few lines
> > 
> > Nope. I have only dropped the workaround.
> 
> This is what is current in the linus tree:
> 
> git log --oneline linux/master -- drivers/char/tpm/
> 
> 324152502b0e Revert "tpm/tpm_crb: implement tpm crb idle state"
> cfa188220363 Revert "tmp/tpm_crb: fix Intel PTT hw bug during idle state"
> 2b7926ae1cd4 Revert "tpm/tpm_crb: open code the crb_init into acpi_add"
> 4886cd80cb8e Revert "tmp/tpm_crb: implement runtime pm for tpm_crb"
> e350e24694e4 tmp/tpm_crb: implement runtime pm for tpm_crb
> 0c22db435bf7 tpm/tpm_crb: open code the crb_init into acpi_add
> 9514ff1961c6 tmp/tpm_crb: fix Intel PTT hw bug during idle state
> e17acbbb69d3 tpm/tpm_crb: implement tpm crb idle state

That's unrelated: https://lkml.org/lkml/2016/9/27/107

Anyway, I applied this because the change is fairly obvious but it would
be nicer to get the fix as a separate patch for a series that is already
applied. If you rely on "do the diff yourself", it is quite ineffective
way to crowdsource :)

/Jarkko

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

Patch

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 8de61876f633..7743e8a21b82 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -29,6 +29,7 @@ 
 #include <linux/mutex.h>
 #include <linux/spinlock.h>
 #include <linux/freezer.h>
+#include <linux/pm_runtime.h>
 
 #include "tpm.h"
 #include "tpm_eventlog.h"
@@ -356,6 +357,8 @@  ssize_t tpm_transmit(struct tpm_chip *chip, const u8 *buf, size_t bufsiz,
 	if (!(flags & TPM_TRANSMIT_UNLOCKED))
 		mutex_lock(&chip->tpm_mutex);
 
+	pm_runtime_get_sync(chip->dev.parent);
+
 	rc = chip->ops->send(chip, (u8 *) buf, count);
 	if (rc < 0) {
 		dev_err(&chip->dev,
@@ -397,6 +400,8 @@  out_recv:
 		dev_err(&chip->dev,
 			"tpm_transmit: tpm_recv: error %zd\n", rc);
 out:
+	pm_runtime_put_sync(chip->dev.parent);
+
 	if (!(flags & TPM_TRANSMIT_UNLOCKED))
 		mutex_unlock(&chip->tpm_mutex);
 	return rc;
diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index def50b7d47b2..eccd82af6215 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -19,6 +19,7 @@ 
 #include <linux/highmem.h>
 #include <linux/rculist.h>
 #include <linux/module.h>
+#include <linux/pm_runtime.h>
 #include "tpm.h"
 
 #define ACPI_SIG_TPM2 "TPM2"
@@ -153,8 +154,6 @@  static int __maybe_unused crb_cmd_ready(struct device *dev,
 	return 0;
 }
 
-static SIMPLE_DEV_PM_OPS(crb_pm, tpm_pm_suspend, tpm_pm_resume);
-
 static u8 crb_status(struct tpm_chip *chip)
 {
 	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
@@ -437,11 +436,21 @@  static int crb_acpi_add(struct acpi_device *device)
 	if (rc)
 		return rc;
 
+	pm_runtime_get_noresume(dev);
+	pm_runtime_set_active(dev);
+	pm_runtime_enable(dev);
+
 	rc = tpm_chip_register(chip);
-	if (rc)
+	if (rc) {
 		crb_go_idle(dev, priv);
+		pm_runtime_put_noidle(dev);
+		pm_runtime_disable(dev);
+		return rc;
+	}
 
-	return rc;
+	pm_runtime_put(dev);
+
+	return 0;
 }
 
 static int crb_acpi_remove(struct acpi_device *device)
@@ -451,9 +460,34 @@  static int crb_acpi_remove(struct acpi_device *device)
 
 	tpm_chip_unregister(chip);
 
+	pm_runtime_disable(dev);
+
 	return 0;
 }
 
+#ifdef CONFIG_PM
+static int crb_pm_runtime_suspend(struct device *dev)
+{
+	struct tpm_chip *chip = dev_get_drvdata(dev);
+	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
+
+	return crb_go_idle(dev, priv);
+}
+
+static int crb_pm_runtime_resume(struct device *dev)
+{
+	struct tpm_chip *chip = dev_get_drvdata(dev);
+	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
+
+	return crb_cmd_ready(dev, priv);
+}
+#endif /* CONFIG_PM */
+
+static const struct dev_pm_ops crb_pm = {
+	SET_SYSTEM_SLEEP_PM_OPS(tpm_pm_suspend, tpm_pm_resume)
+	SET_RUNTIME_PM_OPS(crb_pm_runtime_suspend, crb_pm_runtime_resume, NULL)
+};
+
 static struct acpi_device_id crb_device_ids[] = {
 	{"MSFT0101", 0},
 	{"", 0},