LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH 1/3] tpm: cmd_ready command can be issued only after granting locality
@ 2018-01-23 11:27 Tomas Winkler
  2018-01-23 11:27 ` [PATCH 2/3] tpm: separate cmd_ready/go_idle from runtime_pm Tomas Winkler
                   ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: Tomas Winkler @ 2018-01-23 11:27 UTC (permalink / raw)
  To: Jarkko Sakkinen, Jason Gunthorpe
  Cc: Alexander Usyskin, tpmdd-devel, linux-integrity,
	linux-security-module, linux-kernel, Tomas Winkler

The correct sequence is to first request locality and only after
that perform cmd_ready  handshake, otherwise the hardware will drop
the subsequent message as from the device point of view the cmd_ready
handshake wasn't performed. Symmetrically locality has to be relinquished
only after going idle handshake has completed, this requires that
go_idle has to poll for the completion.

The issue is only visible on devices that support multiple localities.

Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
---
 drivers/char/tpm/tpm-interface.c | 12 ++---
 drivers/char/tpm/tpm_crb.c       | 95 ++++++++++++++++++++++++++--------------
 2 files changed, 70 insertions(+), 37 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 76df4fbcf089..197fda39aab5 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -422,8 +422,6 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
 	if (!(flags & TPM_TRANSMIT_UNLOCKED))
 		mutex_lock(&chip->tpm_mutex);
 
-	if (chip->dev.parent)
-		pm_runtime_get_sync(chip->dev.parent);
 
 	if (chip->ops->clk_enable != NULL)
 		chip->ops->clk_enable(chip, true);
@@ -439,6 +437,9 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
 		chip->locality = rc;
 	}
 
+	if (chip->dev.parent)
+		pm_runtime_get_sync(chip->dev.parent);
+
 	rc = tpm2_prepare_space(chip, space, ordinal, buf);
 	if (rc)
 		goto out;
@@ -499,17 +500,18 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
 	rc = tpm2_commit_space(chip, space, ordinal, buf, &len);
 
 out:
+	if (chip->dev.parent)
+		pm_runtime_put_sync(chip->dev.parent);
+
 	if (need_locality && chip->ops->relinquish_locality) {
 		chip->ops->relinquish_locality(chip, chip->locality);
 		chip->locality = -1;
 	}
+
 out_no_locality:
 	if (chip->ops->clk_enable != NULL)
 		chip->ops->clk_enable(chip, false);
 
-	if (chip->dev.parent)
-		pm_runtime_put_sync(chip->dev.parent);
-
 	if (!(flags & TPM_TRANSMIT_UNLOCKED))
 		mutex_unlock(&chip->tpm_mutex);
 	return rc ? rc : len;
diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index 7b3c2a8aa9de..d174919c446d 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -112,6 +112,25 @@ struct tpm2_crb_smc {
 	u32 smc_func_id;
 };
 
+static bool crb_wait_for_reg_32(u32 __iomem *reg, u32 mask, u32 value,
+				unsigned long timeout)
+{
+	ktime_t start;
+	ktime_t stop;
+
+	start = ktime_get();
+	stop = ktime_add(start, ms_to_ktime(timeout));
+
+	do {
+		if ((ioread32(reg) & mask) == value)
+			return true;
+
+		usleep_range(50, 100);
+	} while (ktime_before(ktime_get(), stop));
+
+	return ((ioread32(reg) & mask) == value);
+}
+
 /**
  * crb_go_idle - request tpm crb device to go the idle state
  *
@@ -128,7 +147,7 @@ struct tpm2_crb_smc {
  *
  * Return: 0 always
  */
-static int __maybe_unused crb_go_idle(struct device *dev, struct crb_priv *priv)
+static int crb_go_idle(struct device *dev, struct crb_priv *priv)
 {
 	if ((priv->sm == ACPI_TPM2_START_METHOD) ||
 	    (priv->sm == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD) ||
@@ -136,30 +155,17 @@ static int __maybe_unused crb_go_idle(struct device *dev, struct crb_priv *priv)
 		return 0;
 
 	iowrite32(CRB_CTRL_REQ_GO_IDLE, &priv->regs_t->ctrl_req);
-	/* we don't really care when this settles */
 
+	if (!crb_wait_for_reg_32(&priv->regs_t->ctrl_req,
+				 CRB_CTRL_REQ_GO_IDLE/* mask */,
+				 0, /* value */
+				 TPM2_TIMEOUT_C)) {
+		dev_warn(dev, "goIdle timed out\n");
+		return -ETIME;
+	}
 	return 0;
 }
 
-static bool crb_wait_for_reg_32(u32 __iomem *reg, u32 mask, u32 value,
-				unsigned long timeout)
-{
-	ktime_t start;
-	ktime_t stop;
-
-	start = ktime_get();
-	stop = ktime_add(start, ms_to_ktime(timeout));
-
-	do {
-		if ((ioread32(reg) & mask) == value)
-			return true;
-
-		usleep_range(50, 100);
-	} while (ktime_before(ktime_get(), stop));
-
-	return false;
-}
-
 /**
  * crb_cmd_ready - request tpm crb device to enter ready state
  *
@@ -175,8 +181,7 @@ static bool crb_wait_for_reg_32(u32 __iomem *reg, u32 mask, u32 value,
  *
  * Return: 0 on success -ETIME on timeout;
  */
-static int __maybe_unused crb_cmd_ready(struct device *dev,
-					struct crb_priv *priv)
+static int crb_cmd_ready(struct device *dev, struct crb_priv *priv)
 {
 	if ((priv->sm == ACPI_TPM2_START_METHOD) ||
 	    (priv->sm == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD) ||
@@ -195,11 +200,11 @@ static int __maybe_unused crb_cmd_ready(struct device *dev,
 	return 0;
 }
 
-static int crb_request_locality(struct tpm_chip *chip, int loc)
+static int __crb_request_locality(struct device *dev,
+				  struct crb_priv *priv, int loc)
 {
-	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
 	u32 value = CRB_LOC_STATE_LOC_ASSIGNED |
-		CRB_LOC_STATE_TPM_REG_VALID_STS;
+		    CRB_LOC_STATE_TPM_REG_VALID_STS;
 
 	if (!priv->regs_h)
 		return 0;
@@ -207,23 +212,36 @@ static int crb_request_locality(struct tpm_chip *chip, int loc)
 	iowrite32(CRB_LOC_CTRL_REQUEST_ACCESS, &priv->regs_h->loc_ctrl);
 	if (!crb_wait_for_reg_32(&priv->regs_h->loc_state, value, value,
 				 TPM2_TIMEOUT_C)) {
-		dev_warn(&chip->dev, "TPM_LOC_STATE_x.requestAccess timed out\n");
+		dev_warn(dev, "TPM_LOC_STATE_x.requestAccess timed out\n");
 		return -ETIME;
 	}
 
 	return 0;
 }
 
-static void crb_relinquish_locality(struct tpm_chip *chip, int loc)
+static int crb_request_locality(struct tpm_chip *chip, int loc)
 {
 	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
 
+	return __crb_request_locality(&chip->dev, priv, loc);
+}
+
+static void __crb_relinquish_locality(struct device *dev,
+				      struct crb_priv *priv, int loc)
+{
 	if (!priv->regs_h)
 		return;
 
 	iowrite32(CRB_LOC_CTRL_RELINQUISH, &priv->regs_h->loc_ctrl);
 }
 
+static void crb_relinquish_locality(struct tpm_chip *chip, int loc)
+{
+	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
+
+	__crb_relinquish_locality(&chip->dev, priv, loc);
+}
+
 static u8 crb_status(struct tpm_chip *chip)
 {
 	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
@@ -475,6 +493,10 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
 			dev_warn(dev, FW_BUG "Bad ACPI memory layout");
 	}
 
+	ret = __crb_request_locality(dev, priv, 0);
+	if (ret)
+		return ret;
+
 	priv->regs_t = crb_map_res(dev, priv, &io_res, buf->control_address,
 				   sizeof(struct crb_regs_tail));
 	if (IS_ERR(priv->regs_t))
@@ -531,6 +553,8 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
 
 	crb_go_idle(dev, priv);
 
+	__crb_relinquish_locality(dev, priv, 0);
+
 	return ret;
 }
 
@@ -588,10 +612,14 @@ static int crb_acpi_add(struct acpi_device *device)
 	chip->acpi_dev_handle = device->handle;
 	chip->flags = TPM_CHIP_FLAG_TPM2;
 
-	rc  = crb_cmd_ready(dev, priv);
+	rc = __crb_request_locality(dev, priv, 0);
 	if (rc)
 		return rc;
 
+	rc  = crb_cmd_ready(dev, priv);
+	if (rc)
+		goto out;
+
 	pm_runtime_get_noresume(dev);
 	pm_runtime_set_active(dev);
 	pm_runtime_enable(dev);
@@ -601,12 +629,15 @@ static int crb_acpi_add(struct acpi_device *device)
 		crb_go_idle(dev, priv);
 		pm_runtime_put_noidle(dev);
 		pm_runtime_disable(dev);
-		return rc;
+		goto out;
 	}
 
-	pm_runtime_put(dev);
+	pm_runtime_put_sync(dev);
 
-	return 0;
+out:
+	__crb_relinquish_locality(dev, priv, 0);
+
+	return rc;
 }
 
 static int crb_acpi_remove(struct acpi_device *device)
-- 
2.14.3

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

* [PATCH 2/3] tpm: separate cmd_ready/go_idle from runtime_pm
  2018-01-23 11:27 [PATCH 1/3] tpm: cmd_ready command can be issued only after granting locality Tomas Winkler
@ 2018-01-23 11:27 ` Tomas Winkler
  2018-01-23 13:08   ` Jarkko Sakkinen
  2018-01-23 11:27 ` [PATCH 3/3] tpm: add longer timeouts for creation commands Tomas Winkler
  2018-01-23 12:54 ` [PATCH 1/3] tpm: cmd_ready command can be issued only after granting locality Jarkko Sakkinen
  2 siblings, 1 reply; 13+ messages in thread
From: Tomas Winkler @ 2018-01-23 11:27 UTC (permalink / raw)
  To: Jarkko Sakkinen, Jason Gunthorpe
  Cc: Alexander Usyskin, tpmdd-devel, linux-integrity,
	linux-security-module, linux-kernel, Tomas Winkler

We cannot use go_idle cmd_ready commands via runtime_pm handles
as with the introduction of localities this is no longer an optional
feature, while runtime pm can be not enabled.
Though cmd_ready/go_idle provides power saving feature, it's also part of
TPM2 protocol and should be called explicitly.
This patch exposes cmd_read/go_idle via tpm class ops and removes
runtime pm support as it is not used by any driver.

Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
---
 drivers/char/tpm/tpm-interface.c | 15 ++++++---
 drivers/char/tpm/tpm_crb.c       | 69 +++++++++++++++++-----------------------
 include/linux/tpm.h              |  2 ++
 3 files changed, 42 insertions(+), 44 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 197fda39aab5..e4d0291e22f6 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -29,7 +29,6 @@
 #include <linux/mutex.h>
 #include <linux/spinlock.h>
 #include <linux/freezer.h>
-#include <linux/pm_runtime.h>
 #include <linux/tpm_eventlog.h>
 
 #include "tpm.h"
@@ -437,8 +436,11 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
 		chip->locality = rc;
 	}
 
-	if (chip->dev.parent)
-		pm_runtime_get_sync(chip->dev.parent);
+	if (chip->ops->cmd_ready) {
+		rc = chip->ops->cmd_ready(chip);
+		if (rc)
+			goto out;
+	}
 
 	rc = tpm2_prepare_space(chip, space, ordinal, buf);
 	if (rc)
@@ -500,8 +502,11 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
 	rc = tpm2_commit_space(chip, space, ordinal, buf, &len);
 
 out:
-	if (chip->dev.parent)
-		pm_runtime_put_sync(chip->dev.parent);
+	if (chip->ops->go_idle) {
+		rc = chip->ops->go_idle(chip);
+		if (rc)
+			goto out;
+	}
 
 	if (need_locality && chip->ops->relinquish_locality) {
 		chip->ops->relinquish_locality(chip, chip->locality);
diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index d174919c446d..4b480f80cecc 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -132,7 +132,7 @@ static bool crb_wait_for_reg_32(u32 __iomem *reg, u32 mask, u32 value,
 }
 
 /**
- * crb_go_idle - request tpm crb device to go the idle state
+ * __crb_go_idle - request tpm crb device to go the idle state
  *
  * @dev:  crb device
  * @priv: crb private data
@@ -147,7 +147,7 @@ static bool crb_wait_for_reg_32(u32 __iomem *reg, u32 mask, u32 value,
  *
  * Return: 0 always
  */
-static int crb_go_idle(struct device *dev, struct crb_priv *priv)
+static int __crb_go_idle(struct device *dev, struct crb_priv *priv)
 {
 	if ((priv->sm == ACPI_TPM2_START_METHOD) ||
 	    (priv->sm == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD) ||
@@ -166,8 +166,16 @@ static int crb_go_idle(struct device *dev, struct crb_priv *priv)
 	return 0;
 }
 
+static int crb_go_idle(struct tpm_chip *chip)
+{
+	struct device *dev = &chip->dev;
+	struct crb_priv *priv = dev_get_drvdata(dev);
+
+	return __crb_go_idle(dev, priv);
+}
+
 /**
- * crb_cmd_ready - request tpm crb device to enter ready state
+ * __crb_cmd_ready - request tpm crb device to enter ready state
  *
  * @dev:  crb device
  * @priv: crb private data
@@ -181,7 +189,7 @@ static int crb_go_idle(struct device *dev, struct crb_priv *priv)
  *
  * Return: 0 on success -ETIME on timeout;
  */
-static int crb_cmd_ready(struct device *dev, struct crb_priv *priv)
+static int __crb_cmd_ready(struct device *dev, struct crb_priv *priv)
 {
 	if ((priv->sm == ACPI_TPM2_START_METHOD) ||
 	    (priv->sm == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD) ||
@@ -200,6 +208,14 @@ static int crb_cmd_ready(struct device *dev, struct crb_priv *priv)
 	return 0;
 }
 
+static int crb_cmd_ready(struct tpm_chip *chip)
+{
+	struct device *dev = &chip->dev;
+	struct crb_priv *priv = dev_get_drvdata(dev);
+
+	return __crb_cmd_ready(dev, priv);
+}
+
 static int __crb_request_locality(struct device *dev,
 				  struct crb_priv *priv, int loc)
 {
@@ -390,6 +406,8 @@ static const struct tpm_class_ops tpm_crb = {
 	.send = crb_send,
 	.cancel = crb_cancel,
 	.req_canceled = crb_req_canceled,
+	.go_idle  = crb_go_idle,
+	.cmd_ready = crb_cmd_ready,
 	.request_locality = crb_request_locality,
 	.relinquish_locality = crb_relinquish_locality,
 	.req_complete_mask = CRB_DRV_STS_COMPLETE,
@@ -506,7 +524,7 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
 	 * PTT HW bug w/a: wake up the device to access
 	 * possibly not retained registers.
 	 */
-	ret = crb_cmd_ready(dev, priv);
+	ret = __crb_cmd_ready(dev, priv);
 	if (ret)
 		return ret;
 
@@ -551,7 +569,7 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
 	if (!ret)
 		priv->cmd_size = cmd_size;
 
-	crb_go_idle(dev, priv);
+	__crb_go_idle(dev, priv);
 
 	__crb_relinquish_locality(dev, priv, 0);
 
@@ -616,23 +634,13 @@ static int crb_acpi_add(struct acpi_device *device)
 	if (rc)
 		return rc;
 
-	rc  = crb_cmd_ready(dev, priv);
+	rc  = __crb_cmd_ready(dev, priv);
 	if (rc)
 		goto out;
 
-	pm_runtime_get_noresume(dev);
-	pm_runtime_set_active(dev);
-	pm_runtime_enable(dev);
-
 	rc = tpm_chip_register(chip);
-	if (rc) {
-		crb_go_idle(dev, priv);
-		pm_runtime_put_noidle(dev);
-		pm_runtime_disable(dev);
-		goto out;
-	}
 
-	pm_runtime_put_sync(dev);
+	__crb_go_idle(dev, priv);
 
 out:
 	__crb_relinquish_locality(dev, priv, 0);
@@ -647,43 +655,27 @@ static int crb_acpi_remove(struct acpi_device *device)
 
 	tpm_chip_unregister(chip);
 
-	pm_runtime_disable(dev);
-
 	return 0;
 }
 
-static int __maybe_unused 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 __maybe_unused 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);
-}
-
 static int __maybe_unused crb_pm_suspend(struct device *dev)
 {
+	struct tpm_chip *chip = dev_get_drvdata(dev);
 	int ret;
 
 	ret = tpm_pm_suspend(dev);
 	if (ret)
 		return ret;
 
-	return crb_pm_runtime_suspend(dev);
+	return crb_go_idle(chip);
 }
 
 static int __maybe_unused crb_pm_resume(struct device *dev)
 {
+	struct tpm_chip *chip = dev_get_drvdata(dev);
 	int ret;
 
-	ret = crb_pm_runtime_resume(dev);
+	ret = crb_cmd_ready(chip);
 	if (ret)
 		return ret;
 
@@ -692,7 +684,6 @@ static int __maybe_unused crb_pm_resume(struct device *dev)
 
 static const struct dev_pm_ops crb_pm = {
 	SET_SYSTEM_SLEEP_PM_OPS(crb_pm_suspend, crb_pm_resume)
-	SET_RUNTIME_PM_OPS(crb_pm_runtime_suspend, crb_pm_runtime_resume, NULL)
 };
 
 static const struct acpi_device_id crb_device_ids[] = {
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index bcdd3790e94d..5c9c5a106350 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -43,6 +43,8 @@ struct tpm_class_ops {
 	u8 (*status) (struct tpm_chip *chip);
 	bool (*update_timeouts)(struct tpm_chip *chip,
 				unsigned long *timeout_cap);
+	int (*go_idle)(struct tpm_chip *chip);
+	int (*cmd_ready)(struct tpm_chip *chip);
 	int (*request_locality)(struct tpm_chip *chip, int loc);
 	void (*relinquish_locality)(struct tpm_chip *chip, int loc);
 	void (*clk_enable)(struct tpm_chip *chip, bool value);
-- 
2.14.3

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

* [PATCH 3/3] tpm: add longer timeouts for creation commands.
  2018-01-23 11:27 [PATCH 1/3] tpm: cmd_ready command can be issued only after granting locality Tomas Winkler
  2018-01-23 11:27 ` [PATCH 2/3] tpm: separate cmd_ready/go_idle from runtime_pm Tomas Winkler
@ 2018-01-23 11:27 ` Tomas Winkler
  2018-01-23 13:12   ` Jarkko Sakkinen
  2018-01-23 12:54 ` [PATCH 1/3] tpm: cmd_ready command can be issued only after granting locality Jarkko Sakkinen
  2 siblings, 1 reply; 13+ messages in thread
From: Tomas Winkler @ 2018-01-23 11:27 UTC (permalink / raw)
  To: Jarkko Sakkinen, Jason Gunthorpe
  Cc: Alexander Usyskin, tpmdd-devel, linux-integrity,
	linux-security-module, linux-kernel, Tomas Winkler

TPM2_CC_Create(0x153) and TPM2_CC_CreatePrimary (0x131) involve generation
of crypto keys which can be a computationally intensive task.
The timeout is set to 3min.

Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
---
 drivers/char/tpm/tpm-interface.c |  4 ++++
 drivers/char/tpm/tpm.h           | 27 ++++++++++++++++-----------
 drivers/char/tpm/tpm2-cmd.c      |  8 +++++---
 3 files changed, 25 insertions(+), 14 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index e4d0291e22f6..4c6334f0f9ad 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -673,6 +673,10 @@ int tpm_get_timeouts(struct tpm_chip *chip)
 		    msecs_to_jiffies(TPM2_DURATION_MEDIUM);
 		chip->duration[TPM_LONG] =
 		    msecs_to_jiffies(TPM2_DURATION_LONG);
+		chip->duration[TPM_LONG_LONG] =
+		    msecs_to_jiffies(TPM2_DURATION_LONG_LONG);
+		chip->duration[TPM_UNDEFINED] =
+		    msecs_to_jiffies(TPM2_DURATION_DEFAULT);
 
 		chip->flags |= TPM_CHIP_FLAG_HAVE_TIMEOUTS;
 		return 0;
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index f895fba4e20d..192ba68b39c2 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -67,7 +67,9 @@ enum tpm_duration {
 	TPM_SHORT = 0,
 	TPM_MEDIUM = 1,
 	TPM_LONG = 2,
-	TPM_UNDEFINED,
+	TPM_LONG_LONG = 3,
+	TPM_UNDEFINED = 4,
+	TPM_DURATION_MAX,
 };
 
 #define TPM_WARN_RETRY          0x800
@@ -79,15 +81,17 @@ enum tpm_duration {
 #define TPM_HEADER_SIZE		10
 
 enum tpm2_const {
-	TPM2_PLATFORM_PCR	= 24,
-	TPM2_PCR_SELECT_MIN	= ((TPM2_PLATFORM_PCR + 7) / 8),
-	TPM2_TIMEOUT_A		= 750,
-	TPM2_TIMEOUT_B		= 2000,
-	TPM2_TIMEOUT_C		= 200,
-	TPM2_TIMEOUT_D		= 30,
-	TPM2_DURATION_SHORT	= 20,
-	TPM2_DURATION_MEDIUM	= 750,
-	TPM2_DURATION_LONG	= 2000,
+	TPM2_PLATFORM_PCR       =     24,
+	TPM2_PCR_SELECT_MIN     = ((TPM2_PLATFORM_PCR + 7) / 8),
+	TPM2_TIMEOUT_A          =    750,
+	TPM2_TIMEOUT_B          =   2000,
+	TPM2_TIMEOUT_C          =    200,
+	TPM2_TIMEOUT_D          =     30,
+	TPM2_DURATION_SHORT     =     20,
+	TPM2_DURATION_MEDIUM    =    750,
+	TPM2_DURATION_LONG      =   2000,
+	TPM2_DURATION_LONG_LONG = 300000,
+	TPM2_DURATION_DEFAULT   = 120000,
 };
 
 enum tpm2_structures {
@@ -123,6 +127,7 @@ enum tpm2_algorithms {
 
 enum tpm2_command_codes {
 	TPM2_CC_FIRST		= 0x011F,
+	TPM2_CC_CREATE_PRIMARY  = 0x0131,
 	TPM2_CC_SELF_TEST	= 0x0143,
 	TPM2_CC_STARTUP		= 0x0144,
 	TPM2_CC_SHUTDOWN	= 0x0145,
@@ -227,7 +232,7 @@ struct tpm_chip {
 	unsigned long timeout_c; /* jiffies */
 	unsigned long timeout_d; /* jiffies */
 	bool timeout_adjusted;
-	unsigned long duration[3]; /* jiffies */
+	unsigned long duration[TPM_DURATION_MAX]; /* jiffies */
 	bool duration_adjusted;
 
 	struct dentry *bios_dir[TPM_NUM_EVENT_LOG_FILES];
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index c17e75348a99..aaa17e982b37 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -90,6 +90,8 @@ static struct tpm2_hash tpm2_hash_map[] = {
  * of time the chip could take to return the result. The values
  * of the SHORT, MEDIUM, and LONG durations are taken from the
  * PC Client Profile (PTP) specification.
+ * LONG_LONG is for commands that generates keys which empirically
+ * takes longer time on some systems.
  */
 static const u8 tpm2_ordinal_duration[TPM2_CC_LAST - TPM2_CC_FIRST + 1] = {
 	TPM_UNDEFINED,		/* 11F */
@@ -110,7 +112,7 @@ static const u8 tpm2_ordinal_duration[TPM2_CC_LAST - TPM2_CC_FIRST + 1] = {
 	TPM_UNDEFINED,		/* 12e */
 	TPM_UNDEFINED,		/* 12f */
 	TPM_UNDEFINED,		/* 130 */
-	TPM_UNDEFINED,		/* 131 */
+	TPM_LONG_LONG,		/* 131 */
 	TPM_UNDEFINED,		/* 132 */
 	TPM_UNDEFINED,		/* 133 */
 	TPM_UNDEFINED,		/* 134 */
@@ -144,7 +146,7 @@ static const u8 tpm2_ordinal_duration[TPM2_CC_LAST - TPM2_CC_FIRST + 1] = {
 	TPM_UNDEFINED,		/* 150 */
 	TPM_UNDEFINED,		/* 151 */
 	TPM_UNDEFINED,		/* 152 */
-	TPM_UNDEFINED,		/* 153 */
+	TPM_LONG_LONG,		/* 153 */
 	TPM_UNDEFINED,		/* 154 */
 	TPM_UNDEFINED,		/* 155 */
 	TPM_UNDEFINED,		/* 156 */
@@ -817,7 +819,7 @@ unsigned long tpm2_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal)
 		duration = chip->duration[index];
 
 	if (duration <= 0)
-		duration = 2 * 60 * HZ;
+		duration = msecs_to_jiffies(TPM2_DURATION_DEFAULT);
 
 	return duration;
 }
-- 
2.14.3

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

* Re: [PATCH 1/3] tpm: cmd_ready command can be issued only after granting locality
  2018-01-23 11:27 [PATCH 1/3] tpm: cmd_ready command can be issued only after granting locality Tomas Winkler
  2018-01-23 11:27 ` [PATCH 2/3] tpm: separate cmd_ready/go_idle from runtime_pm Tomas Winkler
  2018-01-23 11:27 ` [PATCH 3/3] tpm: add longer timeouts for creation commands Tomas Winkler
@ 2018-01-23 12:54 ` Jarkko Sakkinen
  2018-01-24 18:33   ` Winkler, Tomas
  2 siblings, 1 reply; 13+ messages in thread
From: Jarkko Sakkinen @ 2018-01-23 12:54 UTC (permalink / raw)
  To: Tomas Winkler, '
  Cc: Jason Gunthorpe, Alexander Usyskin, tpmdd-devel, linux-integrity,
	linux-security-module, linux-kernel

On Tue, Jan 23, 2018 at 01:27:29PM +0200, Tomas Winkler wrote:
> The correct sequence is to first request locality and only after
> that perform cmd_ready  handshake, otherwise the hardware will drop
> the subsequent message as from the device point of view the cmd_ready
> handshake wasn't performed. Symmetrically locality has to be relinquished
> only after going idle handshake has completed, this requires that
> go_idle has to poll for the completion.
> 
> The issue is only visible on devices that support multiple localities.
> 
> Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>

Thank you!

> ---
>  drivers/char/tpm/tpm-interface.c | 12 ++---
>  drivers/char/tpm/tpm_crb.c       | 95 ++++++++++++++++++++++++++--------------
>  2 files changed, 70 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index 76df4fbcf089..197fda39aab5 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -422,8 +422,6 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
>  	if (!(flags & TPM_TRANSMIT_UNLOCKED))
>  		mutex_lock(&chip->tpm_mutex);
>  
> -	if (chip->dev.parent)
> -		pm_runtime_get_sync(chip->dev.parent);
>  
>  	if (chip->ops->clk_enable != NULL)
>  		chip->ops->clk_enable(chip, true);
> @@ -439,6 +437,9 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
>  		chip->locality = rc;
>  	}
>  
> +	if (chip->dev.parent)
> +		pm_runtime_get_sync(chip->dev.parent);
> +
>  	rc = tpm2_prepare_space(chip, space, ordinal, buf);
>  	if (rc)
>  		goto out;
> @@ -499,17 +500,18 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
>  	rc = tpm2_commit_space(chip, space, ordinal, buf, &len);
>  
>  out:
> +	if (chip->dev.parent)
> +		pm_runtime_put_sync(chip->dev.parent);
> +
>  	if (need_locality && chip->ops->relinquish_locality) {
>  		chip->ops->relinquish_locality(chip, chip->locality);
>  		chip->locality = -1;
>  	}
> +
>  out_no_locality:
>  	if (chip->ops->clk_enable != NULL)
>  		chip->ops->clk_enable(chip, false);
>  
> -	if (chip->dev.parent)
> -		pm_runtime_put_sync(chip->dev.parent);
> -
>  	if (!(flags & TPM_TRANSMIT_UNLOCKED))
>  		mutex_unlock(&chip->tpm_mutex);
>  	return rc ? rc : len;
> diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> index 7b3c2a8aa9de..d174919c446d 100644
> --- a/drivers/char/tpm/tpm_crb.c
> +++ b/drivers/char/tpm/tpm_crb.c
> @@ -112,6 +112,25 @@ struct tpm2_crb_smc {
>  	u32 smc_func_id;
>  };
>  
> +static bool crb_wait_for_reg_32(u32 __iomem *reg, u32 mask, u32 value,
> +				unsigned long timeout)
> +{
> +	ktime_t start;
> +	ktime_t stop;
> +
> +	start = ktime_get();
> +	stop = ktime_add(start, ms_to_ktime(timeout));
> +
> +	do {
> +		if ((ioread32(reg) & mask) == value)
> +			return true;
> +
> +		usleep_range(50, 100);
> +	} while (ktime_before(ktime_get(), stop));
> +
> +	return ((ioread32(reg) & mask) == value);

Not sure about the change of return statement. Is this worth of doing?

> +}
> +
>  /**
>   * crb_go_idle - request tpm crb device to go the idle state
>   *
> @@ -128,7 +147,7 @@ struct tpm2_crb_smc {
>   *
>   * Return: 0 always
>   */
> -static int __maybe_unused crb_go_idle(struct device *dev, struct crb_priv *priv)
> +static int crb_go_idle(struct device *dev, struct crb_priv *priv)
>  {
>  	if ((priv->sm == ACPI_TPM2_START_METHOD) ||
>  	    (priv->sm == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD) ||
> @@ -136,30 +155,17 @@ static int __maybe_unused crb_go_idle(struct device *dev, struct crb_priv *priv)
>  		return 0;
>  
>  	iowrite32(CRB_CTRL_REQ_GO_IDLE, &priv->regs_t->ctrl_req);
> -	/* we don't really care when this settles */
>  
> +	if (!crb_wait_for_reg_32(&priv->regs_t->ctrl_req,
> +				 CRB_CTRL_REQ_GO_IDLE/* mask */,
> +				 0, /* value */
> +				 TPM2_TIMEOUT_C)) {
> +		dev_warn(dev, "goIdle timed out\n");
> +		return -ETIME;
> +	}
>  	return 0;
>  }
>  
> -static bool crb_wait_for_reg_32(u32 __iomem *reg, u32 mask, u32 value,
> -				unsigned long timeout)
> -{
> -	ktime_t start;
> -	ktime_t stop;
> -
> -	start = ktime_get();
> -	stop = ktime_add(start, ms_to_ktime(timeout));
> -
> -	do {
> -		if ((ioread32(reg) & mask) == value)
> -			return true;
> -
> -		usleep_range(50, 100);
> -	} while (ktime_before(ktime_get(), stop));
> -
> -	return false;
> -}
> -
>  /**
>   * crb_cmd_ready - request tpm crb device to enter ready state
>   *
> @@ -175,8 +181,7 @@ static bool crb_wait_for_reg_32(u32 __iomem *reg, u32 mask, u32 value,
>   *
>   * Return: 0 on success -ETIME on timeout;
>   */
> -static int __maybe_unused crb_cmd_ready(struct device *dev,
> -					struct crb_priv *priv)
> +static int crb_cmd_ready(struct device *dev, struct crb_priv *priv)
>  {
>  	if ((priv->sm == ACPI_TPM2_START_METHOD) ||
>  	    (priv->sm == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD) ||
> @@ -195,11 +200,11 @@ static int __maybe_unused crb_cmd_ready(struct device *dev,
>  	return 0;
>  }
>  
> -static int crb_request_locality(struct tpm_chip *chip, int loc)
> +static int __crb_request_locality(struct device *dev,
> +				  struct crb_priv *priv, int loc)
>  {
> -	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
>  	u32 value = CRB_LOC_STATE_LOC_ASSIGNED |
> -		CRB_LOC_STATE_TPM_REG_VALID_STS;
> +		    CRB_LOC_STATE_TPM_REG_VALID_STS;
>  
>  	if (!priv->regs_h)
>  		return 0;
> @@ -207,23 +212,36 @@ static int crb_request_locality(struct tpm_chip *chip, int loc)
>  	iowrite32(CRB_LOC_CTRL_REQUEST_ACCESS, &priv->regs_h->loc_ctrl);
>  	if (!crb_wait_for_reg_32(&priv->regs_h->loc_state, value, value,
>  				 TPM2_TIMEOUT_C)) {
> -		dev_warn(&chip->dev, "TPM_LOC_STATE_x.requestAccess timed out\n");
> +		dev_warn(dev, "TPM_LOC_STATE_x.requestAccess timed out\n");
>  		return -ETIME;
>  	}
>  
>  	return 0;
>  }
>  
> -static void crb_relinquish_locality(struct tpm_chip *chip, int loc)
> +static int crb_request_locality(struct tpm_chip *chip, int loc)
>  {
>  	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
>  
> +	return __crb_request_locality(&chip->dev, priv, loc);
> +}
> +
> +static void __crb_relinquish_locality(struct device *dev,
> +				      struct crb_priv *priv, int loc)
> +{
>  	if (!priv->regs_h)
>  		return;
>  
>  	iowrite32(CRB_LOC_CTRL_RELINQUISH, &priv->regs_h->loc_ctrl);
>  }
>  
> +static void crb_relinquish_locality(struct tpm_chip *chip, int loc)
> +{
> +	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
> +
> +	__crb_relinquish_locality(&chip->dev, priv, loc);
> +}
> +
>  static u8 crb_status(struct tpm_chip *chip)
>  {
>  	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
> @@ -475,6 +493,10 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
>  			dev_warn(dev, FW_BUG "Bad ACPI memory layout");
>  	}
>  
> +	ret = __crb_request_locality(dev, priv, 0);
> +	if (ret)
> +		return ret;
> +
>  	priv->regs_t = crb_map_res(dev, priv, &io_res, buf->control_address,
>  				   sizeof(struct crb_regs_tail));
>  	if (IS_ERR(priv->regs_t))
> @@ -531,6 +553,8 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
>  
>  	crb_go_idle(dev, priv);
>  
> +	__crb_relinquish_locality(dev, priv, 0);
> +
>  	return ret;
>  }
>  
> @@ -588,10 +612,14 @@ static int crb_acpi_add(struct acpi_device *device)
>  	chip->acpi_dev_handle = device->handle;
>  	chip->flags = TPM_CHIP_FLAG_TPM2;
>  
> -	rc  = crb_cmd_ready(dev, priv);
> +	rc = __crb_request_locality(dev, priv, 0);
>  	if (rc)
>  		return rc;
>  
> +	rc  = crb_cmd_ready(dev, priv);
> +	if (rc)
> +		goto out;
> +
>  	pm_runtime_get_noresume(dev);
>  	pm_runtime_set_active(dev);
>  	pm_runtime_enable(dev);
> @@ -601,12 +629,15 @@ static int crb_acpi_add(struct acpi_device *device)
>  		crb_go_idle(dev, priv);
>  		pm_runtime_put_noidle(dev);
>  		pm_runtime_disable(dev);
> -		return rc;
> +		goto out;
>  	}
>  
> -	pm_runtime_put(dev);
> +	pm_runtime_put_sync(dev);

Change to put_sync is needed so that the idle handshake gets completed?

>  
> -	return 0;
> +out:
> +	__crb_relinquish_locality(dev, priv, 0);
> +
> +	return rc;
>  }
>  
>  static int crb_acpi_remove(struct acpi_device *device)
> -- 
> 2.14.3
> 

/Jarkko

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

* Re: [PATCH 2/3] tpm: separate cmd_ready/go_idle from runtime_pm
  2018-01-23 11:27 ` [PATCH 2/3] tpm: separate cmd_ready/go_idle from runtime_pm Tomas Winkler
@ 2018-01-23 13:08   ` Jarkko Sakkinen
  2018-01-23 15:33     ` Jason Gunthorpe
  0 siblings, 1 reply; 13+ messages in thread
From: Jarkko Sakkinen @ 2018-01-23 13:08 UTC (permalink / raw)
  To: Tomas Winkler
  Cc: Jason Gunthorpe, Alexander Usyskin, tpmdd-devel, linux-integrity,
	linux-security-module, linux-kernel

On Tue, Jan 23, 2018 at 01:27:30PM +0200, Tomas Winkler wrote:
> We cannot use go_idle cmd_ready commands via runtime_pm handles
> as with the introduction of localities this is no longer an optional
> feature, while runtime pm can be not enabled.
> Though cmd_ready/go_idle provides power saving feature, it's also part of
> TPM2 protocol and should be called explicitly.
> This patch exposes cmd_read/go_idle via tpm class ops and removes
> runtime pm support as it is not used by any driver.
> 
> Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>

Thank you.

LGTM

Jason, what do you think?

/Jarkko

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

* Re: [PATCH 3/3] tpm: add longer timeouts for creation commands.
  2018-01-23 11:27 ` [PATCH 3/3] tpm: add longer timeouts for creation commands Tomas Winkler
@ 2018-01-23 13:12   ` Jarkko Sakkinen
  2018-01-24 21:29     ` Winkler, Tomas
  0 siblings, 1 reply; 13+ messages in thread
From: Jarkko Sakkinen @ 2018-01-23 13:12 UTC (permalink / raw)
  To: Tomas Winkler
  Cc: Jason Gunthorpe, Alexander Usyskin, tpmdd-devel, linux-integrity,
	linux-security-module, linux-kernel

On Tue, Jan 23, 2018 at 01:27:31PM +0200, Tomas Winkler wrote:
> TPM2_CC_Create(0x153) and TPM2_CC_CreatePrimary (0x131) involve generation
> of crypto keys which can be a computationally intensive task.
> The timeout is set to 3min.
> 
> Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>

>From where are these numbers are derived from?

Can you send 1/3 and 2/3 as a separate patch set with a cover
letter (with explanation) and this as a separate patch? This
is unrelated to first two changes albeit might make sense.

/Jarkko

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

* Re: [PATCH 2/3] tpm: separate cmd_ready/go_idle from runtime_pm
  2018-01-23 13:08   ` Jarkko Sakkinen
@ 2018-01-23 15:33     ` Jason Gunthorpe
  2018-01-29  8:42       ` Jarkko Sakkinen
  0 siblings, 1 reply; 13+ messages in thread
From: Jason Gunthorpe @ 2018-01-23 15:33 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: Tomas Winkler, Alexander Usyskin, tpmdd-devel, linux-integrity,
	linux-security-module, linux-kernel

On Tue, Jan 23, 2018 at 03:08:41PM +0200, Jarkko Sakkinen wrote:
> On Tue, Jan 23, 2018 at 01:27:30PM +0200, Tomas Winkler wrote:
> > We cannot use go_idle cmd_ready commands via runtime_pm handles
> > as with the introduction of localities this is no longer an optional
> > feature, while runtime pm can be not enabled.
> > Though cmd_ready/go_idle provides power saving feature, it's also part of
> > TPM2 protocol and should be called explicitly.
> > This patch exposes cmd_read/go_idle via tpm class ops and removes
> > runtime pm support as it is not used by any driver.
> > 
> > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> 
> Thank you.
> 
> LGTM
> 
> Jason, what do you think?

The PM stuff has been the source of confusion for a while, seems
reasonable to get rid of it.

Jason

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

* RE: [PATCH 1/3] tpm: cmd_ready command can be issued only after granting locality
  2018-01-23 12:54 ` [PATCH 1/3] tpm: cmd_ready command can be issued only after granting locality Jarkko Sakkinen
@ 2018-01-24 18:33   ` Winkler, Tomas
  2018-01-29  8:44     ` Jarkko Sakkinen
  0 siblings, 1 reply; 13+ messages in thread
From: Winkler, Tomas @ 2018-01-24 18:33 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: Jason Gunthorpe, Usyskin, Alexander, tpmdd-devel,
	linux-integrity, linux-security-module, linux-kernel


> > The correct sequence is to first request locality and only after that
> > perform cmd_ready  handshake, otherwise the hardware will drop the
> > subsequent message as from the device point of view the cmd_ready
> > handshake wasn't performed. Symmetrically locality has to be
> > relinquished only after going idle handshake has completed, this
> > requires that go_idle has to poll for the completion.
> >
> > The issue is only visible on devices that support multiple localities.
> >
> > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> 
> Thank you!
> 
> > ---
> >  drivers/char/tpm/tpm-interface.c | 12 ++---
> >  drivers/char/tpm/tpm_crb.c       | 95 ++++++++++++++++++++++++++---------
> -----
> >  2 files changed, 70 insertions(+), 37 deletions(-)
> >
> > diff --git a/drivers/char/tpm/tpm-interface.c
> > b/drivers/char/tpm/tpm-interface.c
> > index 76df4fbcf089..197fda39aab5 100644
> > --- a/drivers/char/tpm/tpm-interface.c
> > +++ b/drivers/char/tpm/tpm-interface.c
> > @@ -422,8 +422,6 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct
> tpm_space *space,
> >  	if (!(flags & TPM_TRANSMIT_UNLOCKED))
> >  		mutex_lock(&chip->tpm_mutex);
> >
> > -	if (chip->dev.parent)
> > -		pm_runtime_get_sync(chip->dev.parent);
> >
> >  	if (chip->ops->clk_enable != NULL)
> >  		chip->ops->clk_enable(chip, true);
> > @@ -439,6 +437,9 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct
> tpm_space *space,
> >  		chip->locality = rc;
> >  	}
> >
> > +	if (chip->dev.parent)
> > +		pm_runtime_get_sync(chip->dev.parent);
> > +
> >  	rc = tpm2_prepare_space(chip, space, ordinal, buf);
> >  	if (rc)
> >  		goto out;
> > @@ -499,17 +500,18 @@ ssize_t tpm_transmit(struct tpm_chip *chip,
> struct tpm_space *space,
> >  	rc = tpm2_commit_space(chip, space, ordinal, buf, &len);
> >
> >  out:
> > +	if (chip->dev.parent)
> > +		pm_runtime_put_sync(chip->dev.parent);
> > +
> >  	if (need_locality && chip->ops->relinquish_locality) {
> >  		chip->ops->relinquish_locality(chip, chip->locality);
> >  		chip->locality = -1;
> >  	}
> > +
> >  out_no_locality:
> >  	if (chip->ops->clk_enable != NULL)
> >  		chip->ops->clk_enable(chip, false);
> >
> > -	if (chip->dev.parent)
> > -		pm_runtime_put_sync(chip->dev.parent);
> > -
> >  	if (!(flags & TPM_TRANSMIT_UNLOCKED))
> >  		mutex_unlock(&chip->tpm_mutex);
> >  	return rc ? rc : len;
> > diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> > index 7b3c2a8aa9de..d174919c446d 100644
> > --- a/drivers/char/tpm/tpm_crb.c
> > +++ b/drivers/char/tpm/tpm_crb.c
> > @@ -112,6 +112,25 @@ struct tpm2_crb_smc {
> >  	u32 smc_func_id;
> >  };
> >
> > +static bool crb_wait_for_reg_32(u32 __iomem *reg, u32 mask, u32 value,
> > +				unsigned long timeout)
> > +{
> > +	ktime_t start;
> > +	ktime_t stop;
> > +
> > +	start = ktime_get();
> > +	stop = ktime_add(start, ms_to_ktime(timeout));
> > +
> > +	do {
> > +		if ((ioread32(reg) & mask) == value)
> > +			return true;
> > +
> > +		usleep_range(50, 100);
> > +	} while (ktime_before(ktime_get(), stop));
> > +
> > +	return ((ioread32(reg) & mask) == value);
> 
> Not sure about the change of return statement. Is this worth of doing?
The sleep statement is followed directly by loop time comparison. It should take care
for case when the change in the value happens in that time windows on the edge of the timeout.
When I look at the traces I think we should maybe make that sleep  little longer to reduce number of 
register access, but some more tests need to be done on multiple platforms. 


> 
> > +}
> > +
> >  /**
> >   * crb_go_idle - request tpm crb device to go the idle state
> >   *
> > @@ -128,7 +147,7 @@ struct tpm2_crb_smc {
> >   *
> >   * Return: 0 always
> >   */
> > -static int __maybe_unused crb_go_idle(struct device *dev, struct
> > crb_priv *priv)
> > +static int crb_go_idle(struct device *dev, struct crb_priv *priv)
> >  {
> >  	if ((priv->sm == ACPI_TPM2_START_METHOD) ||
> >  	    (priv->sm ==
> ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD) || @@
> > -136,30 +155,17 @@ static int __maybe_unused crb_go_idle(struct device
> *dev, struct crb_priv *priv)
> >  		return 0;
> >
> >  	iowrite32(CRB_CTRL_REQ_GO_IDLE, &priv->regs_t->ctrl_req);
> > -	/* we don't really care when this settles */
> >
> > +	if (!crb_wait_for_reg_32(&priv->regs_t->ctrl_req,
> > +				 CRB_CTRL_REQ_GO_IDLE/* mask */,
> > +				 0, /* value */
> > +				 TPM2_TIMEOUT_C)) {
> > +		dev_warn(dev, "goIdle timed out\n");
> > +		return -ETIME;
> > +	}
> >  	return 0;
> >  }
> >
> > -static bool crb_wait_for_reg_32(u32 __iomem *reg, u32 mask, u32 value,
> > -				unsigned long timeout)
> > -{
> > -	ktime_t start;
> > -	ktime_t stop;
> > -
> > -	start = ktime_get();
> > -	stop = ktime_add(start, ms_to_ktime(timeout));
> > -
> > -	do {
> > -		if ((ioread32(reg) & mask) == value)
> > -			return true;
> > -
> > -		usleep_range(50, 100);
> > -	} while (ktime_before(ktime_get(), stop));
> > -
> > -	return false;
> > -}
> > -
> >  /**
> >   * crb_cmd_ready - request tpm crb device to enter ready state
> >   *
> > @@ -175,8 +181,7 @@ static bool crb_wait_for_reg_32(u32 __iomem *reg,
> u32 mask, u32 value,
> >   *
> >   * Return: 0 on success -ETIME on timeout;
> >   */
> > -static int __maybe_unused crb_cmd_ready(struct device *dev,
> > -					struct crb_priv *priv)
> > +static int crb_cmd_ready(struct device *dev, struct crb_priv *priv)
> >  {
> >  	if ((priv->sm == ACPI_TPM2_START_METHOD) ||
> >  	    (priv->sm ==
> ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD) || @@
> > -195,11 +200,11 @@ static int __maybe_unused crb_cmd_ready(struct
> device *dev,
> >  	return 0;
> >  }
> >
> > -static int crb_request_locality(struct tpm_chip *chip, int loc)
> > +static int __crb_request_locality(struct device *dev,
> > +				  struct crb_priv *priv, int loc)
> >  {
> > -	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
> >  	u32 value = CRB_LOC_STATE_LOC_ASSIGNED |
> > -		CRB_LOC_STATE_TPM_REG_VALID_STS;
> > +		    CRB_LOC_STATE_TPM_REG_VALID_STS;
> >
> >  	if (!priv->regs_h)
> >  		return 0;
> > @@ -207,23 +212,36 @@ static int crb_request_locality(struct tpm_chip
> *chip, int loc)
> >  	iowrite32(CRB_LOC_CTRL_REQUEST_ACCESS, &priv->regs_h-
> >loc_ctrl);
> >  	if (!crb_wait_for_reg_32(&priv->regs_h->loc_state, value, value,
> >  				 TPM2_TIMEOUT_C)) {
> > -		dev_warn(&chip->dev, "TPM_LOC_STATE_x.requestAccess
> timed out\n");
> > +		dev_warn(dev, "TPM_LOC_STATE_x.requestAccess timed
> out\n");
> >  		return -ETIME;
> >  	}
> >
> >  	return 0;
> >  }
> >
> > -static void crb_relinquish_locality(struct tpm_chip *chip, int loc)
> > +static int crb_request_locality(struct tpm_chip *chip, int loc)
> >  {
> >  	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
> >
> > +	return __crb_request_locality(&chip->dev, priv, loc); }
> > +
> > +static void __crb_relinquish_locality(struct device *dev,
> > +				      struct crb_priv *priv, int loc) {
> >  	if (!priv->regs_h)
> >  		return;
> >
> >  	iowrite32(CRB_LOC_CTRL_RELINQUISH, &priv->regs_h->loc_ctrl);  }
> >
> > +static void crb_relinquish_locality(struct tpm_chip *chip, int loc) {
> > +	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
> > +
> > +	__crb_relinquish_locality(&chip->dev, priv, loc); }
> > +
> >  static u8 crb_status(struct tpm_chip *chip)  {
> >  	struct crb_priv *priv = dev_get_drvdata(&chip->dev); @@ -475,6
> > +493,10 @@ static int crb_map_io(struct acpi_device *device, struct
> crb_priv *priv,
> >  			dev_warn(dev, FW_BUG "Bad ACPI memory layout");
> >  	}
> >
> > +	ret = __crb_request_locality(dev, priv, 0);
> > +	if (ret)
> > +		return ret;
> > +
> >  	priv->regs_t = crb_map_res(dev, priv, &io_res, buf->control_address,
> >  				   sizeof(struct crb_regs_tail));
> >  	if (IS_ERR(priv->regs_t))
> > @@ -531,6 +553,8 @@ static int crb_map_io(struct acpi_device *device,
> > struct crb_priv *priv,
> >
> >  	crb_go_idle(dev, priv);
> >
> > +	__crb_relinquish_locality(dev, priv, 0);
> > +
> >  	return ret;
> >  }
> >
> > @@ -588,10 +612,14 @@ static int crb_acpi_add(struct acpi_device
> *device)
> >  	chip->acpi_dev_handle = device->handle;
> >  	chip->flags = TPM_CHIP_FLAG_TPM2;
> >
> > -	rc  = crb_cmd_ready(dev, priv);
> > +	rc = __crb_request_locality(dev, priv, 0);
> >  	if (rc)
> >  		return rc;
> >
> > +	rc  = crb_cmd_ready(dev, priv);
> > +	if (rc)
> > +		goto out;
> > +
> >  	pm_runtime_get_noresume(dev);
> >  	pm_runtime_set_active(dev);
> >  	pm_runtime_enable(dev);
> > @@ -601,12 +629,15 @@ static int crb_acpi_add(struct acpi_device
> *device)
> >  		crb_go_idle(dev, priv);
> >  		pm_runtime_put_noidle(dev);
> >  		pm_runtime_disable(dev);
> > -		return rc;
> > +		goto out;
> >  	}
> >
> > -	pm_runtime_put(dev);
> > +	pm_runtime_put_sync(dev);
> 
> Change to put_sync is needed so that the idle handshake gets completed?

Right.
 
> >
> > -	return 0;
> > +out:
> > +	__crb_relinquish_locality(dev, priv, 0);
> > +
> > +	return rc;
> >  }
> >
> >  static int crb_acpi_remove(struct acpi_device *device)
> > --
> > 2.14.3
> >
> 
> /Jarkko

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

* RE: [PATCH 3/3] tpm: add longer timeouts for creation commands.
  2018-01-23 13:12   ` Jarkko Sakkinen
@ 2018-01-24 21:29     ` Winkler, Tomas
  0 siblings, 0 replies; 13+ messages in thread
From: Winkler, Tomas @ 2018-01-24 21:29 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: Usyskin, Alexander, linux-integrity, linux-security-module, linux-kernel



> -----Original Message-----
> From: Jarkko Sakkinen [mailto:jarkko.sakkinen@linux.intel.com]
> Sent: Tuesday, January 23, 2018 15:12
> To: Winkler, Tomas <tomas.winkler@intel.com>
> Cc: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>; Usyskin,
> Alexander <alexander.usyskin@intel.com>; tpmdd-
> devel@lists.sourceforge.net; linux-integrity@vger.kernel.org; linux-security-
> module@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 3/3] tpm: add longer timeouts for creation commands.
> 
> On Tue, Jan 23, 2018 at 01:27:31PM +0200, Tomas Winkler wrote:
> > TPM2_CC_Create(0x153) and TPM2_CC_CreatePrimary (0x131) involve
> > generation of crypto keys which can be a computationally intensive task.
> > The timeout is set to 3min.
> >
> > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> 
> From where are these numbers are derived from?

An Intel SW architect.
 
> 
> Can you send 1/3 and 2/3 as a separate patch set with a cover letter (with
> explanation) and this as a separate patch? This is unrelated to first two
> changes albeit might make sense.

Will do, though the patches were born to  address the initialization issues we've encountered.
Thanks
Tomas

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

* Re: [PATCH 2/3] tpm: separate cmd_ready/go_idle from runtime_pm
  2018-01-23 15:33     ` Jason Gunthorpe
@ 2018-01-29  8:42       ` Jarkko Sakkinen
  0 siblings, 0 replies; 13+ messages in thread
From: Jarkko Sakkinen @ 2018-01-29  8:42 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: Tomas Winkler, Alexander Usyskin, tpmdd-devel, linux-integrity,
	linux-security-module, linux-kernel

On Tue, Jan 23, 2018 at 08:33:22AM -0700, Jason Gunthorpe wrote:
> On Tue, Jan 23, 2018 at 03:08:41PM +0200, Jarkko Sakkinen wrote:
> > On Tue, Jan 23, 2018 at 01:27:30PM +0200, Tomas Winkler wrote:
> > > We cannot use go_idle cmd_ready commands via runtime_pm handles
> > > as with the introduction of localities this is no longer an optional
> > > feature, while runtime pm can be not enabled.
> > > Though cmd_ready/go_idle provides power saving feature, it's also part of
> > > TPM2 protocol and should be called explicitly.
> > > This patch exposes cmd_read/go_idle via tpm class ops and removes
> > > runtime pm support as it is not used by any driver.
> > > 
> > > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> > 
> > Thank you.
> > 
> > LGTM
> > 
> > Jason, what do you think?
> 
> The PM stuff has been the source of confusion for a while, seems
> reasonable to get rid of it.

I'll test this ASAP.

/Jarkk

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

* Re: [PATCH 1/3] tpm: cmd_ready command can be issued only after granting locality
  2018-01-24 18:33   ` Winkler, Tomas
@ 2018-01-29  8:44     ` Jarkko Sakkinen
  2018-02-01 22:08       ` Winkler, Tomas
  0 siblings, 1 reply; 13+ messages in thread
From: Jarkko Sakkinen @ 2018-01-29  8:44 UTC (permalink / raw)
  To: Winkler, Tomas
  Cc: Jason Gunthorpe, Usyskin, Alexander, tpmdd-devel,
	linux-integrity, linux-security-module, linux-kernel

On Wed, Jan 24, 2018 at 06:33:53PM +0000, Winkler, Tomas wrote:
> > > -	pm_runtime_put(dev);
> > > +	pm_runtime_put_sync(dev);
> > 
> > Change to put_sync is needed so that the idle handshake gets completed?
> 
> Right.

Since we seem to agree that your change to get rid of the use of PM
runtime should that be actually the first change in the patch set?

Or are you looking forward to get these backported to stable kernels?

/Jarkko

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

* RE: [PATCH 1/3] tpm: cmd_ready command can be issued only after granting locality
  2018-01-29  8:44     ` Jarkko Sakkinen
@ 2018-02-01 22:08       ` Winkler, Tomas
  0 siblings, 0 replies; 13+ messages in thread
From: Winkler, Tomas @ 2018-02-01 22:08 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: Jason Gunthorpe, Usyskin, Alexander, tpmdd-devel,
	linux-integrity, linux-security-module, linux-kernel


> 
> On Wed, Jan 24, 2018 at 06:33:53PM +0000, Winkler, Tomas wrote:
> > > > -	pm_runtime_put(dev);
> > > > +	pm_runtime_put_sync(dev);
> > >
> > > Change to put_sync is needed so that the idle handshake gets
> completed?
> >
> > Right.
> 
> Since we seem to agree that your change to get rid of the use of PM runtime
> should that be actually the first change in the patch set?
> 
> Or are you looking forward to get these backported to stable kernels?

Yes, that was one of the reason behind for splitting this into two patches. 
So this first patch is the actual fix and second is the rather non user visible fix 
as runtime_pm should be enabled by default.  But .. maybe both need to be backported.

Thanks
Tomas

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

* [PATCH 1/3] tpm: cmd_ready command can be issued only after granting locality
  2018-04-25 10:44 [PATCH 0/3] v4.16 tpmdd backports Jarkko Sakkinen
@ 2018-04-25 10:44 ` Jarkko Sakkinen
  0 siblings, 0 replies; 13+ messages in thread
From: Jarkko Sakkinen @ 2018-04-25 10:44 UTC (permalink / raw)
  To: stable
  Cc: Tomas Winkler, Jarkko Sakkinen, Peter Huewe, Jarkko Sakkinen,
	Jason Gunthorpe, Arnd Bergmann, Greg Kroah-Hartman, Azhar Shaikh,
	open list:TPM DEVICE DRIVER, open list

From: Tomas Winkler <tomas.winkler@intel.com>

commit 888d867df4417deffc33927e6fc2c6925736fe92 upstream

The correct sequence is to first request locality and only after
that perform cmd_ready handshake, otherwise the hardware will drop
the subsequent message as from the device point of view the cmd_ready
handshake wasn't performed. Symmetrically locality has to be relinquished
only after going idle handshake has completed, this requires that
go_idle has to poll for the completion and as well locality
relinquish has to poll for completion so it is not overridden
in back to back commands flow.

Two wrapper functions are added (request_locality relinquish_locality)
to simplify the error handling.

The issue is only visible on devices that support multiple localities.

Fixes: 877c57d0d0ca ("tpm_crb: request and relinquish locality 0")
Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
Reviewed-by: Jarkko Sakkinen <jarkko.sakkine@linux.intel.com>
Tested-by: Jarkko Sakkinen <jarkko.sakkine@linux.intel.com>
Signed-off-by: Jarkko Sakkinen <jarkko.sakkine@linux.intel.com>
---
 drivers/char/tpm/tpm-interface.c |  54 ++++++++++++----
 drivers/char/tpm/tpm_crb.c       | 108 +++++++++++++++++++++----------
 drivers/char/tpm/tpm_tis_core.c  |   4 +-
 include/linux/tpm.h              |   2 +-
 4 files changed, 120 insertions(+), 48 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 248c04090dea..0091eccdaf8d 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -369,6 +369,36 @@ static int tpm_validate_command(struct tpm_chip *chip,
 	return -EINVAL;
 }
 
+static int tpm_request_locality(struct tpm_chip *chip)
+{
+	int rc;
+
+	if (!chip->ops->request_locality)
+		return 0;
+
+	rc = chip->ops->request_locality(chip, 0);
+	if (rc < 0)
+		return rc;
+
+	chip->locality = rc;
+
+	return 0;
+}
+
+static void tpm_relinquish_locality(struct tpm_chip *chip)
+{
+	int rc;
+
+	if (!chip->ops->relinquish_locality)
+		return;
+
+	rc = chip->ops->relinquish_locality(chip, chip->locality);
+	if (rc)
+		dev_err(&chip->dev, "%s: : error %d\n", __func__, rc);
+
+	chip->locality = -1;
+}
+
 /**
  * tmp_transmit - Internal kernel interface to transmit TPM commands.
  *
@@ -422,8 +452,6 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
 	if (!(flags & TPM_TRANSMIT_UNLOCKED))
 		mutex_lock(&chip->tpm_mutex);
 
-	if (chip->dev.parent)
-		pm_runtime_get_sync(chip->dev.parent);
 
 	if (chip->ops->clk_enable != NULL)
 		chip->ops->clk_enable(chip, true);
@@ -431,14 +459,15 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
 	/* Store the decision as chip->locality will be changed. */
 	need_locality = chip->locality == -1;
 
-	if (!(flags & TPM_TRANSMIT_RAW) &&
-	    need_locality && chip->ops->request_locality)  {
-		rc = chip->ops->request_locality(chip, 0);
+	if (!(flags & TPM_TRANSMIT_RAW) && need_locality) {
+		rc = tpm_request_locality(chip);
 		if (rc < 0)
 			goto out_no_locality;
-		chip->locality = rc;
 	}
 
+	if (chip->dev.parent)
+		pm_runtime_get_sync(chip->dev.parent);
+
 	rc = tpm2_prepare_space(chip, space, ordinal, buf);
 	if (rc)
 		goto out;
@@ -499,17 +528,16 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
 	rc = tpm2_commit_space(chip, space, ordinal, buf, &len);
 
 out:
-	if (need_locality && chip->ops->relinquish_locality) {
-		chip->ops->relinquish_locality(chip, chip->locality);
-		chip->locality = -1;
-	}
+	if (chip->dev.parent)
+		pm_runtime_put_sync(chip->dev.parent);
+
+	if (need_locality)
+		tpm_relinquish_locality(chip);
+
 out_no_locality:
 	if (chip->ops->clk_enable != NULL)
 		chip->ops->clk_enable(chip, false);
 
-	if (chip->dev.parent)
-		pm_runtime_put_sync(chip->dev.parent);
-
 	if (!(flags & TPM_TRANSMIT_UNLOCKED))
 		mutex_unlock(&chip->tpm_mutex);
 	return rc ? rc : len;
diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index 7b3c2a8aa9de..497edd9848cd 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -112,6 +112,25 @@ struct tpm2_crb_smc {
 	u32 smc_func_id;
 };
 
+static bool crb_wait_for_reg_32(u32 __iomem *reg, u32 mask, u32 value,
+				unsigned long timeout)
+{
+	ktime_t start;
+	ktime_t stop;
+
+	start = ktime_get();
+	stop = ktime_add(start, ms_to_ktime(timeout));
+
+	do {
+		if ((ioread32(reg) & mask) == value)
+			return true;
+
+		usleep_range(50, 100);
+	} while (ktime_before(ktime_get(), stop));
+
+	return ((ioread32(reg) & mask) == value);
+}
+
 /**
  * crb_go_idle - request tpm crb device to go the idle state
  *
@@ -128,7 +147,7 @@ struct tpm2_crb_smc {
  *
  * Return: 0 always
  */
-static int __maybe_unused crb_go_idle(struct device *dev, struct crb_priv *priv)
+static int crb_go_idle(struct device *dev, struct crb_priv *priv)
 {
 	if ((priv->sm == ACPI_TPM2_START_METHOD) ||
 	    (priv->sm == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD) ||
@@ -136,30 +155,17 @@ static int __maybe_unused crb_go_idle(struct device *dev, struct crb_priv *priv)
 		return 0;
 
 	iowrite32(CRB_CTRL_REQ_GO_IDLE, &priv->regs_t->ctrl_req);
-	/* we don't really care when this settles */
 
+	if (!crb_wait_for_reg_32(&priv->regs_t->ctrl_req,
+				 CRB_CTRL_REQ_GO_IDLE/* mask */,
+				 0, /* value */
+				 TPM2_TIMEOUT_C)) {
+		dev_warn(dev, "goIdle timed out\n");
+		return -ETIME;
+	}
 	return 0;
 }
 
-static bool crb_wait_for_reg_32(u32 __iomem *reg, u32 mask, u32 value,
-				unsigned long timeout)
-{
-	ktime_t start;
-	ktime_t stop;
-
-	start = ktime_get();
-	stop = ktime_add(start, ms_to_ktime(timeout));
-
-	do {
-		if ((ioread32(reg) & mask) == value)
-			return true;
-
-		usleep_range(50, 100);
-	} while (ktime_before(ktime_get(), stop));
-
-	return false;
-}
-
 /**
  * crb_cmd_ready - request tpm crb device to enter ready state
  *
@@ -175,8 +181,7 @@ static bool crb_wait_for_reg_32(u32 __iomem *reg, u32 mask, u32 value,
  *
  * Return: 0 on success -ETIME on timeout;
  */
-static int __maybe_unused crb_cmd_ready(struct device *dev,
-					struct crb_priv *priv)
+static int crb_cmd_ready(struct device *dev, struct crb_priv *priv)
 {
 	if ((priv->sm == ACPI_TPM2_START_METHOD) ||
 	    (priv->sm == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD) ||
@@ -195,11 +200,11 @@ static int __maybe_unused crb_cmd_ready(struct device *dev,
 	return 0;
 }
 
-static int crb_request_locality(struct tpm_chip *chip, int loc)
+static int __crb_request_locality(struct device *dev,
+				  struct crb_priv *priv, int loc)
 {
-	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
 	u32 value = CRB_LOC_STATE_LOC_ASSIGNED |
-		CRB_LOC_STATE_TPM_REG_VALID_STS;
+		    CRB_LOC_STATE_TPM_REG_VALID_STS;
 
 	if (!priv->regs_h)
 		return 0;
@@ -207,21 +212,45 @@ static int crb_request_locality(struct tpm_chip *chip, int loc)
 	iowrite32(CRB_LOC_CTRL_REQUEST_ACCESS, &priv->regs_h->loc_ctrl);
 	if (!crb_wait_for_reg_32(&priv->regs_h->loc_state, value, value,
 				 TPM2_TIMEOUT_C)) {
-		dev_warn(&chip->dev, "TPM_LOC_STATE_x.requestAccess timed out\n");
+		dev_warn(dev, "TPM_LOC_STATE_x.requestAccess timed out\n");
 		return -ETIME;
 	}
 
 	return 0;
 }
 
-static void crb_relinquish_locality(struct tpm_chip *chip, int loc)
+static int crb_request_locality(struct tpm_chip *chip, int loc)
 {
 	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
 
+	return __crb_request_locality(&chip->dev, priv, loc);
+}
+
+static int __crb_relinquish_locality(struct device *dev,
+				     struct crb_priv *priv, int loc)
+{
+	u32 mask = CRB_LOC_STATE_LOC_ASSIGNED |
+		   CRB_LOC_STATE_TPM_REG_VALID_STS;
+	u32 value = CRB_LOC_STATE_TPM_REG_VALID_STS;
+
 	if (!priv->regs_h)
-		return;
+		return 0;
 
 	iowrite32(CRB_LOC_CTRL_RELINQUISH, &priv->regs_h->loc_ctrl);
+	if (!crb_wait_for_reg_32(&priv->regs_h->loc_state, mask, value,
+				 TPM2_TIMEOUT_C)) {
+		dev_warn(dev, "TPM_LOC_STATE_x.requestAccess timed out\n");
+		return -ETIME;
+	}
+
+	return 0;
+}
+
+static int crb_relinquish_locality(struct tpm_chip *chip, int loc)
+{
+	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
+
+	return __crb_relinquish_locality(&chip->dev, priv, loc);
 }
 
 static u8 crb_status(struct tpm_chip *chip)
@@ -475,6 +504,10 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
 			dev_warn(dev, FW_BUG "Bad ACPI memory layout");
 	}
 
+	ret = __crb_request_locality(dev, priv, 0);
+	if (ret)
+		return ret;
+
 	priv->regs_t = crb_map_res(dev, priv, &io_res, buf->control_address,
 				   sizeof(struct crb_regs_tail));
 	if (IS_ERR(priv->regs_t))
@@ -531,6 +564,8 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
 
 	crb_go_idle(dev, priv);
 
+	__crb_relinquish_locality(dev, priv, 0);
+
 	return ret;
 }
 
@@ -588,10 +623,14 @@ static int crb_acpi_add(struct acpi_device *device)
 	chip->acpi_dev_handle = device->handle;
 	chip->flags = TPM_CHIP_FLAG_TPM2;
 
-	rc  = crb_cmd_ready(dev, priv);
+	rc = __crb_request_locality(dev, priv, 0);
 	if (rc)
 		return rc;
 
+	rc  = crb_cmd_ready(dev, priv);
+	if (rc)
+		goto out;
+
 	pm_runtime_get_noresume(dev);
 	pm_runtime_set_active(dev);
 	pm_runtime_enable(dev);
@@ -601,12 +640,15 @@ static int crb_acpi_add(struct acpi_device *device)
 		crb_go_idle(dev, priv);
 		pm_runtime_put_noidle(dev);
 		pm_runtime_disable(dev);
-		return rc;
+		goto out;
 	}
 
-	pm_runtime_put(dev);
+	pm_runtime_put_sync(dev);
 
-	return 0;
+out:
+	__crb_relinquish_locality(dev, priv, 0);
+
+	return rc;
 }
 
 static int crb_acpi_remove(struct acpi_device *device)
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index da074e3db19b..5a1f47b43947 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -143,11 +143,13 @@ static bool check_locality(struct tpm_chip *chip, int l)
 	return false;
 }
 
-static void release_locality(struct tpm_chip *chip, int l)
+static int release_locality(struct tpm_chip *chip, int l)
 {
 	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
 
 	tpm_tis_write8(priv, TPM_ACCESS(l), TPM_ACCESS_ACTIVE_LOCALITY);
+
+	return 0;
 }
 
 static int request_locality(struct tpm_chip *chip, int l)
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index bcdd3790e94d..06639fb6ab85 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -44,7 +44,7 @@ struct tpm_class_ops {
 	bool (*update_timeouts)(struct tpm_chip *chip,
 				unsigned long *timeout_cap);
 	int (*request_locality)(struct tpm_chip *chip, int loc);
-	void (*relinquish_locality)(struct tpm_chip *chip, int loc);
+	int (*relinquish_locality)(struct tpm_chip *chip, int loc);
 	void (*clk_enable)(struct tpm_chip *chip, bool value);
 };
 
-- 
2.17.0

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

end of thread, other threads:[~2018-04-25 10:48 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-01-23 11:27 [PATCH 1/3] tpm: cmd_ready command can be issued only after granting locality Tomas Winkler
2018-01-23 11:27 ` [PATCH 2/3] tpm: separate cmd_ready/go_idle from runtime_pm Tomas Winkler
2018-01-23 13:08   ` Jarkko Sakkinen
2018-01-23 15:33     ` Jason Gunthorpe
2018-01-29  8:42       ` Jarkko Sakkinen
2018-01-23 11:27 ` [PATCH 3/3] tpm: add longer timeouts for creation commands Tomas Winkler
2018-01-23 13:12   ` Jarkko Sakkinen
2018-01-24 21:29     ` Winkler, Tomas
2018-01-23 12:54 ` [PATCH 1/3] tpm: cmd_ready command can be issued only after granting locality Jarkko Sakkinen
2018-01-24 18:33   ` Winkler, Tomas
2018-01-29  8:44     ` Jarkko Sakkinen
2018-02-01 22:08       ` Winkler, Tomas
2018-04-25 10:44 [PATCH 0/3] v4.16 tpmdd backports Jarkko Sakkinen
2018-04-25 10:44 ` [PATCH 1/3] tpm: cmd_ready command can be issued only after granting locality 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).