[PATCH v2 0/3] Runtime PM for TPM2 CRB chips

[PATCH v2 0/3] Runtime PM for TPM2 CRB chips

[Jarkko Sakkinen]

These patches implement support for runtime PM with TPM2 CRB chips such
as PTT in Skylake.

This is very naive implementation since the TPM is resumed and supended
for every transaction thus causing total 40 ms latency for each
transaction but for the existing use cases this shouldn't be an issue.

Jarkko Sakkinen (3):
  tpm_crb: fix crb_req_canceled behavior
  tpm, tpm_crb: remove wmb()'s
  tpm, tpm_crb: runtime power management

 drivers/char/tpm/tpm-interface.c |  3 ++
 drivers/char/tpm/tpm_crb.c       | 60 +++++++++++++++++++++++++++++++++++-----
 2 files changed, 56 insertions(+), 7 deletions(-)

-- 
2.7.4

[PATCH v2 1/3] tpm_crb: fix crb_req_canceled behavior

[Jarkko Sakkinen]

The req_canceled() callback is used by tpm_transmit() periodically to
check whether the request has been canceled while it is receiving a
response from the TPM.

The TPM_CRB_CTRL_CANCEL register was cleared already in the crb_cancel
callback, which has two consequences:

* Cancel might not happen.
* req_canceled() always returns zero.

A better place to clear the register is when starting to send a new
command. The behavior of TPM_CRB_CTRL_CANCEL is described in the
section 5.5.3.6 of the PTP specification.

CC: stable@vger.kernel.org
Fixes: 30fc8d138e91 ("tpm: TPM 2.0 CRB Interface")
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 drivers/char/tpm/tpm_crb.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index 1b8e1b5..8819ef7 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -142,6 +142,11 @@ static int crb_send(struct tpm_chip *chip, u8 *buf, size_t len)
 	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
 	int rc = 0;
 
+	/* Zero the cancel register so that the next command will not get
+	 * canceled.
+	 */
+	iowrite32(0, &priv->cca->cancel);
+
 	if (len > ioread32(&priv->cca->cmd_size)) {
 		dev_err(&chip->dev,
 			"invalid command count value %x %zx\n",
@@ -175,8 +180,6 @@ static void crb_cancel(struct tpm_chip *chip)
 
 	if ((priv->flags & CRB_FL_ACPI_START) && crb_do_acpi_start(chip))
 		dev_err(&chip->dev, "ACPI Start failed\n");
-
-	iowrite32(0, &priv->cca->cancel);
 }
 
 static bool crb_req_canceled(struct tpm_chip *chip, u8 status)
-- 
2.7.4

[PATCH v2 2/3] tpm, tpm_crb: remove wmb()'s

[Jarkko Sakkinen]

wmb()'s are not needed as iowrite32() is used.

Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 drivers/char/tpm/tpm_crb.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index 8819ef7..0c8ed7f 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -175,9 +175,6 @@ static void crb_cancel(struct tpm_chip *chip)
 
 	iowrite32(cpu_to_le32(CRB_CANCEL_INVOKE), &priv->cca->cancel);
 
-	/* Make sure that cmd is populated before issuing cancel. */
-	wmb();
-
 	if ((priv->flags & CRB_FL_ACPI_START) && crb_do_acpi_start(chip))
 		dev_err(&chip->dev, "ACPI Start failed\n");
 }
-- 
2.7.4

[PATCH v2 3/3] tpm, tpm_crb: runtime power management

[Jarkko Sakkinen]

The register TPM_CRB_CTRL_REQ_0 contains bits goIdle and cmdReady for
invoking the chip to suspend and resume. This commit implements runtime
PM for tpm_crb by using these bits.

The legacy ACPI start (SMI + DMA) based devices do not support these
bits. Thus this functionality only is enabled only for CRB start (MMIO)
based devices.

Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 drivers/char/tpm/tpm-interface.c |  3 +++
 drivers/char/tpm/tpm_crb.c       | 50 ++++++++++++++++++++++++++++++++++++++--
 2 files changed, 51 insertions(+), 2 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 5e3c1b6..3b85648 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"
@@ -350,6 +351,7 @@ ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,
 		return -E2BIG;
 	}
 
+	pm_runtime_get_sync(chip->dev.parent);
 	mutex_lock(&chip->tpm_mutex);
 
 	rc = chip->ops->send(chip, (u8 *) buf, count);
@@ -394,6 +396,7 @@ out_recv:
 			"tpm_transmit: tpm_recv: error %zd\n", rc);
 out:
 	mutex_unlock(&chip->tpm_mutex);
+	pm_runtime_put_sync(chip->dev.parent);
 	return rc;
 }
 
diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index 0c8ed7f..ef545eb 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -20,6 +20,7 @@
 #include <linux/rculist.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
 #include "tpm.h"
 
 #define ACPI_SIG_TPM2 "TPM2"
@@ -41,7 +42,6 @@ enum crb_ca_request {
 
 enum crb_ca_status {
 	CRB_CA_STS_ERROR	= BIT(0),
-	CRB_CA_STS_TPM_IDLE	= BIT(1),
 };
 
 enum crb_start {
@@ -83,7 +83,50 @@ struct crb_priv {
 	u8 __iomem *rsp;
 };
 
-static SIMPLE_DEV_PM_OPS(crb_pm, tpm_pm_suspend, tpm_pm_resume);
+/* CONFIG_PM */
+static int __maybe_unused crb_runtime_suspend(struct device *dev)
+{
+	struct tpm_chip *chip = dev_get_drvdata(dev);
+	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
+	u32 req;
+
+	if (priv->flags & CRB_FL_ACPI_START)
+		return 0;
+
+	req = ioread32(&priv->cca->req);
+	iowrite32(cpu_to_le32(req | CRB_CA_REQ_GO_IDLE), &priv->cca->req);
+	msleep(chip->timeout_c);
+
+	if (ioread32(&priv->cca->req) & CRB_CA_REQ_GO_IDLE)
+		return -ETIME;
+
+	return 0;
+}
+
+/* CONFIG_PM */
+static int __maybe_unused crb_runtime_resume(struct device *dev)
+{
+	struct tpm_chip *chip = dev_get_drvdata(dev);
+	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
+	u32 req;
+
+	if (priv->flags & CRB_FL_ACPI_START)
+		return 0;
+
+	req = ioread32(&priv->cca->req);
+	iowrite32(cpu_to_le32(req | CRB_CA_REQ_CMD_READY), &priv->cca->req);
+	msleep(chip->timeout_c);
+
+	if (ioread32(&priv->cca->req) & CRB_CA_REQ_CMD_READY)
+		return -ETIME;
+
+	return 0;
+}
+
+static const struct dev_pm_ops crb_pm = {
+	SET_RUNTIME_PM_OPS(crb_runtime_suspend, crb_runtime_resume, NULL)
+	SET_SYSTEM_SLEEP_PM_OPS(tpm_pm_suspend, tpm_pm_resume)
+};
 
 static u8 crb_status(struct tpm_chip *chip)
 {
@@ -206,6 +249,8 @@ static int crb_init(struct acpi_device *device, struct crb_priv *priv)
 	if (IS_ERR(chip))
 		return PTR_ERR(chip);
 
+	pm_runtime_set_active(&device->dev);
+	pm_runtime_enable(&device->dev);
 	dev_set_drvdata(&chip->dev, priv);
 	chip->acpi_dev_handle = device->handle;
 	chip->flags = TPM_CHIP_FLAG_TPM2;
@@ -366,6 +411,7 @@ static int crb_acpi_remove(struct acpi_device *device)
 
 	tpm_chip_unregister(chip);
 
+	pm_runtime_disable(dev);
 	return 0;
 }
 
-- 
2.7.4

Re: [PATCH v2 0/3] Runtime PM for TPM2 CRB chips

[Jarkko Sakkinen]

On Sat, Jun 18, 2016 at 05:10:25PM +0200, Jarkko Sakkinen wrote:
> These patches implement support for runtime PM with TPM2 CRB chips such
> as PTT in Skylake.
> 
> This is very naive implementation since the TPM is resumed and supended
> for every transaction thus causing total 40 ms latency for each
> transaction but for the existing use cases this shouldn't be an issue.
> 
> Jarkko Sakkinen (3):
>   tpm_crb: fix crb_req_canceled behavior
>   tpm, tpm_crb: remove wmb()'s
>   tpm, tpm_crb: runtime power management
> 
>  drivers/char/tpm/tpm-interface.c |  3 ++
>  drivers/char/tpm/tpm_crb.c       | 60 +++++++++++++++++++++++++++++++++++-----
>  2 files changed, 56 insertions(+), 7 deletions(-)

Reviewing myself:

* I fsckd the description. The latency is whopping 400 ms, not 4 ms.
  Sorry about that. Thus it's better to use autosuspend.
* Callbacks should take tpm_mutex before doing anything.

I guess that tpm_tis could later on also take advantage of this work?

/Jarkko

Re: [PATCH v2 0/3] Runtime PM for TPM2 CRB chips

[Peter Huewe]

Faster suspend/resume is always appreciated.
-- 
Sent from my mobile