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

[Winkler, Tomas]

> 
> On Sat, Oct 08, 2016 at 02:59:39PM +0300, Tomas Winkler wrote:
> > From: "Winkler, Tomas" <tomas.winkler@xxxxxxxxx>
> >
> > 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 <tomas.winkler@xxxxxxxxx>
> 
> 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
> >