| Message ID | 1475927979-23484-5-git-send-email-tomas.winkler@intel.com |
|---|---|
| State | New |
| Headers | show |
On Sat, Oct 08, 2016 at 02:59:39PM +0300, Tomas Winkler wrote: > From: "Winkler, Tomas" <tomas.winkler@intel.com> > > 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@intel.com> 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
> > On Sat, Oct 08, 2016 at 02:59:39PM +0300, Tomas Winkler wrote: > > From: "Winkler, Tomas" <tomas.winkler@intel.com> > > > > 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@intel.com> > > 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
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" <tomas.winkler@intel.com> > > > > > > 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@intel.com> > > > > 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
> -----Original Message----- > From: Jarkko Sakkinen [mailto:jarkko.sakkinen@linux.intel.com] > Sent: Saturday, October 08, 2016 19:01 > To: Winkler, Tomas <tomas.winkler@intel.com> > Cc: tpmdd-devel@lists.sourceforge.net; Jason Gunthorpe > <jgunthorpe@obsidianresearch.com>; linux-kernel@vger.kernel.org > 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" <tomas.winkler@intel.com> > > > > > > > > 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@intel.com> > > > > > > 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
On Sat, Oct 08, 2016 at 06:18:36PM +0000, Winkler, Tomas wrote: > > > > -----Original Message----- > > From: Jarkko Sakkinen [mailto:jarkko.sakkinen@linux.intel.com] > > Sent: Saturday, October 08, 2016 19:01 > > To: Winkler, Tomas <tomas.winkler@intel.com> > > Cc: tpmdd-devel@lists.sourceforge.net; Jason Gunthorpe > > <jgunthorpe@obsidianresearch.com>; linux-kernel@vger.kernel.org > > 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" <tomas.winkler@intel.com> > > > > > > > > > > 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@intel.com> > > > > > > > > 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
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},