[PATCH] tpm: separate cmd_ready/go_idle from runtime_pm

[PATCH] tpm: separate cmd_ready/go_idle from runtime_pm

[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 a power saving, it's also a 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.
New wrappers are added tpm_cmd_ready() and tpm_go_idle() wrappers to
streamline tpm_try_transmit code. TPM_TRANSMIT_UNLOCKED flag is abused to
resolve tpm spaces recursive calls to tpm_transmit().

tpm_crb no longer needs own power saving functions and can drop using
tpm_pm_suspend/resume.

This patch cannot be really spearated from the locality fix.
Fixes: 888d867df441 (tpm: cmd_ready command can be issued only after granting locality)


Fixes: 888d867df441 (tpm: cmd_ready command can be issued only after granting locality)
Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
---
V2:resent
V3:resent
V4: 1. Use tpm_pm_suspend/resume in tpm_crb
    2. Drop cmd_ready/go_idle around tpm_chip_register, as there is no
       usage counter like in runtime_pm.
V5: 1. add tpm_cmd_ready and tpm_go_idle wrappers.
    2. Abuse TPM_TRANSMIT_UNLOCKED flag to resolve tpm space
       recursion.
 drivers/char/tpm/tpm-interface.c |  33 +++++++++++--
 drivers/char/tpm/tpm_crb.c       | 101 +++++++++++----------------------------
 include/linux/tpm.h              |   2 +
 3 files changed, 57 insertions(+), 79 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index e32f6e85dc6d..f802d4ee918c 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"
@@ -399,6 +398,28 @@ static void tpm_relinquish_locality(struct tpm_chip *chip)
 	chip->locality = -1;
 }
 
+static int tpm_cmd_ready(struct tpm_chip *chip, unsigned int flags)
+{
+	if (flags & TPM_TRANSMIT_UNLOCKED)
+		return 0;
+
+	if (!chip->ops->cmd_ready)
+		return 0;
+
+	return chip->ops->cmd_ready(chip);
+}
+
+static int tpm_go_idle(struct tpm_chip *chip, unsigned int flags)
+{
+	if (flags & TPM_TRANSMIT_UNLOCKED)
+		return 0;
+
+	if (!chip->ops->go_idle)
+		return 0;
+
+	return chip->ops->go_idle(chip);
+}
+
 static ssize_t tpm_try_transmit(struct tpm_chip *chip,
 				struct tpm_space *space,
 				u8 *buf, size_t bufsiz,
@@ -455,8 +476,9 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip,
 			goto out_no_locality;
 	}
 
-	if (chip->dev.parent)
-		pm_runtime_get_sync(chip->dev.parent);
+	rc = tpm_cmd_ready(chip, flags);
+	if (rc)
+		goto out;
 
 	rc = tpm2_prepare_space(chip, space, ordinal, buf);
 	if (rc)
@@ -518,8 +540,9 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip,
 	rc = tpm2_commit_space(chip, space, ordinal, buf, &len);
 
 out:
-	if (chip->dev.parent)
-		pm_runtime_put_sync(chip->dev.parent);
+	rc = tpm_go_idle(chip, flags);
+	if (rc)
+		goto out;
 
 	if (need_locality)
 		tpm_relinquish_locality(chip);
diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index 34fbc6cb097b..36952ef98f90 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) ||
@@ -163,11 +163,20 @@ static int crb_go_idle(struct device *dev, struct crb_priv *priv)
 		dev_warn(dev, "goIdle timed out\n");
 		return -ETIME;
 	}
+
 	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 +190,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 +209,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)
 {
@@ -401,6 +418,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,
@@ -520,7 +539,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)
 		goto out_relinquish_locality;
 
@@ -565,7 +584,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);
 
 out_relinquish_locality:
 
@@ -628,32 +647,7 @@ static int crb_acpi_add(struct acpi_device *device)
 	chip->acpi_dev_handle = device->handle;
 	chip->flags = TPM_CHIP_FLAG_TPM2;
 
-	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);
-
-	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);
-
-out:
-	__crb_relinquish_locality(dev, priv, 0);
-
-	return rc;
+	return tpm_chip_register(chip);
 }
 
 static int crb_acpi_remove(struct acpi_device *device)
@@ -663,52 +657,11 @@ 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)
-{
-	int ret;
-
-	ret = tpm_pm_suspend(dev);
-	if (ret)
-		return ret;
-
-	return crb_pm_runtime_suspend(dev);
-}
-
-static int __maybe_unused crb_pm_resume(struct device *dev)
-{
-	int ret;
-
-	ret = crb_pm_runtime_resume(dev);
-	if (ret)
-		return ret;
-
-	return tpm_pm_resume(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)
+	SET_SYSTEM_SLEEP_PM_OPS(tpm_pm_suspend, tpm_pm_resume)
 };
 
 static const struct acpi_device_id crb_device_ids[] = {
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index 06639fb6ab85..8eb5e5ebe136 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);
 	int (*relinquish_locality)(struct tpm_chip *chip, int loc);
 	void (*clk_enable)(struct tpm_chip *chip, bool value);
-- 
2.14.3

[PATCH] tpm: separate cmd_ready/go_idle from runtime_pm

[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 a power saving, it's also a 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.
New wrappers are added tpm_cmd_ready() and tpm_go_idle() wrappers to
streamline tpm_try_transmit code. TPM_TRANSMIT_UNLOCKED flag is abused to
resolve tpm spaces recursive calls to tpm_transmit().

tpm_crb no longer needs own power saving functions and can drop using
tpm_pm_suspend/resume.

This patch cannot be really spearated from the locality fix.
Fixes: 888d867df441 (tpm: cmd_ready command can be issued only after granting locality)


Fixes: 888d867df441 (tpm: cmd_ready command can be issued only after granting locality)
Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
---
V2:resent
V3:resent
V4: 1. Use tpm_pm_suspend/resume in tpm_crb
    2. Drop cmd_ready/go_idle around tpm_chip_register, as there is no
       usage counter like in runtime_pm.
V5: 1. add tpm_cmd_ready and tpm_go_idle wrappers.
    2. Abuse TPM_TRANSMIT_UNLOCKED flag to resolve tpm space
       recursion.
 drivers/char/tpm/tpm-interface.c |  33 +++++++++++--
 drivers/char/tpm/tpm_crb.c       | 101 +++++++++++----------------------------
 include/linux/tpm.h              |   2 +
 3 files changed, 57 insertions(+), 79 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index e32f6e85dc6d..f802d4ee918c 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"
@@ -399,6 +398,28 @@ static void tpm_relinquish_locality(struct tpm_chip *chip)
 	chip->locality = -1;
 }
 
+static int tpm_cmd_ready(struct tpm_chip *chip, unsigned int flags)
+{
+	if (flags & TPM_TRANSMIT_UNLOCKED)
+		return 0;
+
+	if (!chip->ops->cmd_ready)
+		return 0;
+
+	return chip->ops->cmd_ready(chip);
+}
+
+static int tpm_go_idle(struct tpm_chip *chip, unsigned int flags)
+{
+	if (flags & TPM_TRANSMIT_UNLOCKED)
+		return 0;
+
+	if (!chip->ops->go_idle)
+		return 0;
+
+	return chip->ops->go_idle(chip);
+}
+
 static ssize_t tpm_try_transmit(struct tpm_chip *chip,
 				struct tpm_space *space,
 				u8 *buf, size_t bufsiz,
@@ -455,8 +476,9 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip,
 			goto out_no_locality;
 	}
 
-	if (chip->dev.parent)
-		pm_runtime_get_sync(chip->dev.parent);
+	rc = tpm_cmd_ready(chip, flags);
+	if (rc)
+		goto out;
 
 	rc = tpm2_prepare_space(chip, space, ordinal, buf);
 	if (rc)
@@ -518,8 +540,9 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip,
 	rc = tpm2_commit_space(chip, space, ordinal, buf, &len);
 
 out:
-	if (chip->dev.parent)
-		pm_runtime_put_sync(chip->dev.parent);
+	rc = tpm_go_idle(chip, flags);
+	if (rc)
+		goto out;
 
 	if (need_locality)
 		tpm_relinquish_locality(chip);
diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index 34fbc6cb097b..36952ef98f90 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) ||
@@ -163,11 +163,20 @@ static int crb_go_idle(struct device *dev, struct crb_priv *priv)
 		dev_warn(dev, "goIdle timed out\n");
 		return -ETIME;
 	}
+
 	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 +190,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 +209,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)
 {
@@ -401,6 +418,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,
@@ -520,7 +539,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)
 		goto out_relinquish_locality;
 
@@ -565,7 +584,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);
 
 out_relinquish_locality:
 
@@ -628,32 +647,7 @@ static int crb_acpi_add(struct acpi_device *device)
 	chip->acpi_dev_handle = device->handle;
 	chip->flags = TPM_CHIP_FLAG_TPM2;
 
-	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);
-
-	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);
-
-out:
-	__crb_relinquish_locality(dev, priv, 0);
-
-	return rc;
+	return tpm_chip_register(chip);
 }
 
 static int crb_acpi_remove(struct acpi_device *device)
@@ -663,52 +657,11 @@ 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)
-{
-	int ret;
-
-	ret = tpm_pm_suspend(dev);
-	if (ret)
-		return ret;
-
-	return crb_pm_runtime_suspend(dev);
-}
-
-static int __maybe_unused crb_pm_resume(struct device *dev)
-{
-	int ret;
-
-	ret = crb_pm_runtime_resume(dev);
-	if (ret)
-		return ret;
-
-	return tpm_pm_resume(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)
+	SET_SYSTEM_SLEEP_PM_OPS(tpm_pm_suspend, tpm_pm_resume)
 };
 
 static const struct acpi_device_id crb_device_ids[] = {
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index 06639fb6ab85..8eb5e5ebe136 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);
 	int (*relinquish_locality)(struct tpm_chip *chip, int loc);
 	void (*clk_enable)(struct tpm_chip *chip, bool value);
-- 
2.14.3

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] tpm: separate cmd_ready/go_idle from runtime_pm

[Jarkko Sakkinen]

On Wed, May 16, 2018 at 10:46:00PM +0300, Tomas Winkler wrote:
> New wrappers are added tpm_cmd_ready() and tpm_go_idle() wrappers to
> streamline tpm_try_transmit code. TPM_TRANSMIT_UNLOCKED flag is abused to
> resolve tpm spaces recursive calls to tpm_transmit().

This looks good and all but I don't think we want to abuse anything in
the driver code, do we?

In other words, either

1. New flag is to be added.
2. Rename the existing flag to something else than UNLOCKED (perhaps
   SPACE).

 /Jarkko

[PATCH] tpm: separate cmd_ready/go_idle from runtime_pm

[Jarkko Sakkinen]

On Wed, May 16, 2018 at 10:46:00PM +0300, Tomas Winkler wrote:
> New wrappers are added tpm_cmd_ready() and tpm_go_idle() wrappers to
> streamline tpm_try_transmit code. TPM_TRANSMIT_UNLOCKED flag is abused to
> resolve tpm spaces recursive calls to tpm_transmit().

This looks good and all but I don't think we want to abuse anything in
the driver code, do we?

In other words, either

1. New flag is to be added.
2. Rename the existing flag to something else than UNLOCKED (perhaps
   SPACE).

 /Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] tpm: separate cmd_ready/go_idle from runtime_pm

[Winkler, Tomas]

> 
> On Wed, May 16, 2018 at 10:46:00PM +0300, Tomas Winkler wrote:
> > New wrappers are added tpm_cmd_ready() and tpm_go_idle() wrappers to
> > streamline tpm_try_transmit code. TPM_TRANSMIT_UNLOCKED flag is
> abused
> > to resolve tpm spaces recursive calls to tpm_transmit().
> 
> This looks good and all but I don't think we want to abuse anything in the
> driver code, do we?

It's not abuse just the flag UNLOCKED is not really named correctly 
I think this has to be backported so wanted to do less invasive change.
> 
> In other words, either
> 
> 1. New flag is to be added.
No
> 2. Rename the existing flag to something else than UNLOCKED (perhaps
>    SPACE).


Currently both TPM_TRANSMIT_UNLOCKED and TPM_TRANSMIT_RAW  servers the same purpose, to resolve
the recursion in tpm tranmit. 
What I believe should be done is to split the function into two to eliminate recursive call instead of using flags.
Please also there is clock_enable which is not protected from double call.
Thanks
Tomas

[PATCH] tpm: separate cmd_ready/go_idle from runtime_pm

[Winkler, Tomas]

> 
> On Wed, May 16, 2018 at 10:46:00PM +0300, Tomas Winkler wrote:
> > New wrappers are added tpm_cmd_ready() and tpm_go_idle() wrappers to
> > streamline tpm_try_transmit code. TPM_TRANSMIT_UNLOCKED flag is
> abused
> > to resolve tpm spaces recursive calls to tpm_transmit().
> 
> This looks good and all but I don't think we want to abuse anything in the
> driver code, do we?

It's not abuse just the flag UNLOCKED is not really named correctly 
I think this has to be backported so wanted to do less invasive change.
> 
> In other words, either
> 
> 1. New flag is to be added.
No
> 2. Rename the existing flag to something else than UNLOCKED (perhaps
>    SPACE).


Currently both TPM_TRANSMIT_UNLOCKED and TPM_TRANSMIT_RAW  servers the same purpose, to resolve
the recursion in tpm tranmit. 
What I believe should be done is to split the function into two to eliminate recursive call instead of using flags.
Please also there is clock_enable which is not protected from double call.
Thanks
Tomas

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] tpm: separate cmd_ready/go_idle from runtime_pm

[Jarkko Sakkinen]

On Tue, May 22, 2018 at 09:27:46AM +0000, Winkler, Tomas wrote:
> > 
> > On Wed, May 16, 2018 at 10:46:00PM +0300, Tomas Winkler wrote:
> > > New wrappers are added tpm_cmd_ready() and tpm_go_idle() wrappers to
> > > streamline tpm_try_transmit code. TPM_TRANSMIT_UNLOCKED flag is
> > abused
> > > to resolve tpm spaces recursive calls to tpm_transmit().
> > 
> > This looks good and all but I don't think we want to abuse anything in the
> > driver code, do we?
> 
> It's not abuse just the flag UNLOCKED is not really named correctly 
> I think this has to be backported so wanted to do less invasive change.

It should be renamed anyway and possible merge conflicts are not hard to
sort out in this change. Can you rename it as SPACE?

Right, and even without rename this will probably cause merge conflicts
at least in v4.4 an v4.9 since in-kernel RM landed in v4.12, so not much
gain not do the rename :-)

/Jarkko

[PATCH] tpm: separate cmd_ready/go_idle from runtime_pm

[Jarkko Sakkinen]

On Tue, May 22, 2018 at 09:27:46AM +0000, Winkler, Tomas wrote:
> > 
> > On Wed, May 16, 2018 at 10:46:00PM +0300, Tomas Winkler wrote:
> > > New wrappers are added tpm_cmd_ready() and tpm_go_idle() wrappers to
> > > streamline tpm_try_transmit code. TPM_TRANSMIT_UNLOCKED flag is
> > abused
> > > to resolve tpm spaces recursive calls to tpm_transmit().
> > 
> > This looks good and all but I don't think we want to abuse anything in the
> > driver code, do we?
> 
> It's not abuse just the flag UNLOCKED is not really named correctly 
> I think this has to be backported so wanted to do less invasive change.

It should be renamed anyway and possible merge conflicts are not hard to
sort out in this change. Can you rename it as SPACE?

Right, and even without rename this will probably cause merge conflicts
at least in v4.4 an v4.9 since in-kernel RM landed in v4.12, so not much
gain not do the rename :-)

/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] tpm: separate cmd_ready/go_idle from runtime_pm

[Winkler, Tomas]

> On Tue, May 22, 2018 at 09:27:46AM +0000, Winkler, Tomas wrote:
> > >
> > > On Wed, May 16, 2018 at 10:46:00PM +0300, Tomas Winkler wrote:
> > > > New wrappers are added tpm_cmd_ready() and tpm_go_idle()
> wrappers
> > > > to streamline tpm_try_transmit code. TPM_TRANSMIT_UNLOCKED flag
> is
> > > abused
> > > > to resolve tpm spaces recursive calls to tpm_transmit().
> > >
> > > This looks good and all but I don't think we want to abuse anything
> > > in the driver code, do we?
> >
> > It's not abuse just the flag UNLOCKED is not really named correctly I
> > think this has to be backported so wanted to do less invasive change.
> 
> It should be renamed anyway and possible merge conflicts are not hard to
> sort out in this change. Can you rename it as SPACE?

Not sure, I believe UNLOCKED is still better name than SPACE, I'm not sure this is 
Do you also want to remove TPM_TRANSMIT_RAW? 
clk_enable is handling its own anti recursion counter 'data->clkrun_enabled' 
but it should be all handled under one flag I guess.

> Right, and even without rename this will probably cause merge conflicts at
> least in v4.4 an v4.9 since in-kernel RM landed in v4.12, so not much gain not
> do the rename :-)

I belive we should do minimal change and the big cleanup after that.
Not sure, I believe UNLOCKED is still better name than SPACE even it wasn't the original intention.  
No the SPACE is the issue, but any recursion call into tpm_transmit. A bigger change is needed
and rename to SPACE would be just another intermediat change.

Please reconsider.

Thanks
Tomas 

[PATCH] tpm: separate cmd_ready/go_idle from runtime_pm

[Winkler, Tomas]

> On Tue, May 22, 2018 at 09:27:46AM +0000, Winkler, Tomas wrote:
> > >
> > > On Wed, May 16, 2018 at 10:46:00PM +0300, Tomas Winkler wrote:
> > > > New wrappers are added tpm_cmd_ready() and tpm_go_idle()
> wrappers
> > > > to streamline tpm_try_transmit code. TPM_TRANSMIT_UNLOCKED flag
> is
> > > abused
> > > > to resolve tpm spaces recursive calls to tpm_transmit().
> > >
> > > This looks good and all but I don't think we want to abuse anything
> > > in the driver code, do we?
> >
> > It's not abuse just the flag UNLOCKED is not really named correctly I
> > think this has to be backported so wanted to do less invasive change.
> 
> It should be renamed anyway and possible merge conflicts are not hard to
> sort out in this change. Can you rename it as SPACE?

Not sure, I believe UNLOCKED is still better name than SPACE, I'm not sure this is 
Do you also want to remove TPM_TRANSMIT_RAW? 
clk_enable is handling its own anti recursion counter 'data->clkrun_enabled' 
but it should be all handled under one flag I guess.

> Right, and even without rename this will probably cause merge conflicts at
> least in v4.4 an v4.9 since in-kernel RM landed in v4.12, so not much gain not
> do the rename :-)

I belive we should do minimal change and the big cleanup after that.
Not sure, I believe UNLOCKED is still better name than SPACE even it wasn't the original intention.  
No the SPACE is the issue, but any recursion call into tpm_transmit. A bigger change is needed
and rename to SPACE would be just another intermediat change.

Please reconsider.

Thanks
Tomas 
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] tpm: separate cmd_ready/go_idle from runtime_pm

[Jarkko Sakkinen]

On Wed, May 23, 2018 at 01:48:17PM +0000, Winkler, Tomas wrote:
> 
> > On Tue, May 22, 2018 at 09:27:46AM +0000, Winkler, Tomas wrote:
> > > >
> > > > On Wed, May 16, 2018 at 10:46:00PM +0300, Tomas Winkler wrote:
> > > > > New wrappers are added tpm_cmd_ready() and tpm_go_idle()
> > wrappers
> > > > > to streamline tpm_try_transmit code. TPM_TRANSMIT_UNLOCKED flag
> > is
> > > > abused
> > > > > to resolve tpm spaces recursive calls to tpm_transmit().
> > > >
> > > > This looks good and all but I don't think we want to abuse anything
> > > > in the driver code, do we?
> > >
> > > It's not abuse just the flag UNLOCKED is not really named correctly I
> > > think this has to be backported so wanted to do less invasive change.
> > 
> > It should be renamed anyway and possible merge conflicts are not hard to
> > sort out in this change. Can you rename it as SPACE?
> 
> Not sure, I believe UNLOCKED is still better name than SPACE, I'm not sure this is 
> Do you also want to remove TPM_TRANSMIT_RAW? 
> clk_enable is handling its own anti recursion counter 'data->clkrun_enabled' 
> but it should be all handled under one flag I guess.
> 
> > Right, and even without rename this will probably cause merge conflicts at
> > least in v4.4 an v4.9 since in-kernel RM landed in v4.12, so not much gain not
> > do the rename :-)
> 
> I belive we should do minimal change and the big cleanup after that.
> Not sure, I believe UNLOCKED is still better name than SPACE even it wasn't the original intention.  
> No the SPACE is the issue, but any recursion call into tpm_transmit. A bigger change is needed
> and rename to SPACE would be just another intermediat change.
> 
> Please reconsider.
> 
> Thanks
> Tomas 

Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

/Jarkko

[PATCH] tpm: separate cmd_ready/go_idle from runtime_pm

[Jarkko Sakkinen]

On Wed, May 23, 2018 at 01:48:17PM +0000, Winkler, Tomas wrote:
> 
> > On Tue, May 22, 2018 at 09:27:46AM +0000, Winkler, Tomas wrote:
> > > >
> > > > On Wed, May 16, 2018 at 10:46:00PM +0300, Tomas Winkler wrote:
> > > > > New wrappers are added tpm_cmd_ready() and tpm_go_idle()
> > wrappers
> > > > > to streamline tpm_try_transmit code. TPM_TRANSMIT_UNLOCKED flag
> > is
> > > > abused
> > > > > to resolve tpm spaces recursive calls to tpm_transmit().
> > > >
> > > > This looks good and all but I don't think we want to abuse anything
> > > > in the driver code, do we?
> > >
> > > It's not abuse just the flag UNLOCKED is not really named correctly I
> > > think this has to be backported so wanted to do less invasive change.
> > 
> > It should be renamed anyway and possible merge conflicts are not hard to
> > sort out in this change. Can you rename it as SPACE?
> 
> Not sure, I believe UNLOCKED is still better name than SPACE, I'm not sure this is 
> Do you also want to remove TPM_TRANSMIT_RAW? 
> clk_enable is handling its own anti recursion counter 'data->clkrun_enabled' 
> but it should be all handled under one flag I guess.
> 
> > Right, and even without rename this will probably cause merge conflicts at
> > least in v4.4 an v4.9 since in-kernel RM landed in v4.12, so not much gain not
> > do the rename :-)
> 
> I belive we should do minimal change and the big cleanup after that.
> Not sure, I believe UNLOCKED is still better name than SPACE even it wasn't the original intention.  
> No the SPACE is the issue, but any recursion call into tpm_transmit. A bigger change is needed
> and rename to SPACE would be just another intermediat change.
> 
> Please reconsider.
> 
> Thanks
> Tomas 

Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] tpm: separate cmd_ready/go_idle from runtime_pm

[Winkler, Tomas]

> 
> On Wed, May 23, 2018 at 01:48:17PM +0000, Winkler, Tomas wrote:
> >
> > > On Tue, May 22, 2018 at 09:27:46AM +0000, Winkler, Tomas wrote:
> > > > >
> > > > > On Wed, May 16, 2018 at 10:46:00PM +0300, Tomas Winkler wrote:
> > > > > > New wrappers are added tpm_cmd_ready() and tpm_go_idle()
> > > wrappers
> > > > > > to streamline tpm_try_transmit code. TPM_TRANSMIT_UNLOCKED
> > > > > > flag
> > > is
> > > > > abused
> > > > > > to resolve tpm spaces recursive calls to tpm_transmit().
> > > > >
> > > > > This looks good and all but I don't think we want to abuse
> > > > > anything in the driver code, do we?
> > > >
> > > > It's not abuse just the flag UNLOCKED is not really named
> > > > correctly I think this has to be backported so wanted to do less invasive
> change.
> > >
> > > It should be renamed anyway and possible merge conflicts are not
> > > hard to sort out in this change. Can you rename it as SPACE?
> >
> > Not sure, I believe UNLOCKED is still better name than SPACE, I'm not
> > sure this is Do you also want to remove TPM_TRANSMIT_RAW?
> > clk_enable is handling its own anti recursion counter 'data-
> >clkrun_enabled'
> > but it should be all handled under one flag I guess.
> >
> > > Right, and even without rename this will probably cause merge
> > > conflicts at least in v4.4 an v4.9 since in-kernel RM landed in
> > > v4.12, so not much gain not do the rename :-)
> >
> > I belive we should do minimal change and the big cleanup after that.
> > Not sure, I believe UNLOCKED is still better name than SPACE even it wasn't
> the original intention.
> > No the SPACE is the issue, but any recursion call into tpm_transmit. A
> > bigger change is needed and rename to SPACE would be just another
> intermediat change.
> >
> > Please reconsider.
> >
> > Thanks
> > Tomas
> 
> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>


Does it mean you're Okay with the patch now?
Thanks
Tomas

[PATCH] tpm: separate cmd_ready/go_idle from runtime_pm

[Winkler, Tomas]

> 
> On Wed, May 23, 2018 at 01:48:17PM +0000, Winkler, Tomas wrote:
> >
> > > On Tue, May 22, 2018 at 09:27:46AM +0000, Winkler, Tomas wrote:
> > > > >
> > > > > On Wed, May 16, 2018 at 10:46:00PM +0300, Tomas Winkler wrote:
> > > > > > New wrappers are added tpm_cmd_ready() and tpm_go_idle()
> > > wrappers
> > > > > > to streamline tpm_try_transmit code. TPM_TRANSMIT_UNLOCKED
> > > > > > flag
> > > is
> > > > > abused
> > > > > > to resolve tpm spaces recursive calls to tpm_transmit().
> > > > >
> > > > > This looks good and all but I don't think we want to abuse
> > > > > anything in the driver code, do we?
> > > >
> > > > It's not abuse just the flag UNLOCKED is not really named
> > > > correctly I think this has to be backported so wanted to do less invasive
> change.
> > >
> > > It should be renamed anyway and possible merge conflicts are not
> > > hard to sort out in this change. Can you rename it as SPACE?
> >
> > Not sure, I believe UNLOCKED is still better name than SPACE, I'm not
> > sure this is Do you also want to remove TPM_TRANSMIT_RAW?
> > clk_enable is handling its own anti recursion counter 'data-
> >clkrun_enabled'
> > but it should be all handled under one flag I guess.
> >
> > > Right, and even without rename this will probably cause merge
> > > conflicts at least in v4.4 an v4.9 since in-kernel RM landed in
> > > v4.12, so not much gain not do the rename :-)
> >
> > I belive we should do minimal change and the big cleanup after that.
> > Not sure, I believe UNLOCKED is still better name than SPACE even it wasn't
> the original intention.
> > No the SPACE is the issue, but any recursion call into tpm_transmit. A
> > bigger change is needed and rename to SPACE would be just another
> intermediat change.
> >
> > Please reconsider.
> >
> > Thanks
> > Tomas
> 
> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>


Does it mean you're Okay with the patch now?
Thanks
Tomas


--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] tpm: separate cmd_ready/go_idle from runtime_pm

[Jarkko Sakkinen]

On Wed, May 30, 2018 at 10:52:28AM +0000, Winkler, Tomas wrote:
> > 
> > On Wed, May 23, 2018 at 01:48:17PM +0000, Winkler, Tomas wrote:
> > >
> > > > On Tue, May 22, 2018 at 09:27:46AM +0000, Winkler, Tomas wrote:
> > > > > >
> > > > > > On Wed, May 16, 2018 at 10:46:00PM +0300, Tomas Winkler wrote:
> > > > > > > New wrappers are added tpm_cmd_ready() and tpm_go_idle()
> > > > wrappers
> > > > > > > to streamline tpm_try_transmit code. TPM_TRANSMIT_UNLOCKED
> > > > > > > flag
> > > > is
> > > > > > abused
> > > > > > > to resolve tpm spaces recursive calls to tpm_transmit().
> > > > > >
> > > > > > This looks good and all but I don't think we want to abuse
> > > > > > anything in the driver code, do we?
> > > > >
> > > > > It's not abuse just the flag UNLOCKED is not really named
> > > > > correctly I think this has to be backported so wanted to do less invasive
> > change.
> > > >
> > > > It should be renamed anyway and possible merge conflicts are not
> > > > hard to sort out in this change. Can you rename it as SPACE?
> > >
> > > Not sure, I believe UNLOCKED is still better name than SPACE, I'm not
> > > sure this is Do you also want to remove TPM_TRANSMIT_RAW?
> > > clk_enable is handling its own anti recursion counter 'data-
> > >clkrun_enabled'
> > > but it should be all handled under one flag I guess.
> > >
> > > > Right, and even without rename this will probably cause merge
> > > > conflicts at least in v4.4 an v4.9 since in-kernel RM landed in
> > > > v4.12, so not much gain not do the rename :-)
> > >
> > > I belive we should do minimal change and the big cleanup after that.
> > > Not sure, I believe UNLOCKED is still better name than SPACE even it wasn't
> > the original intention.
> > > No the SPACE is the issue, but any recursion call into tpm_transmit. A
> > > bigger change is needed and rename to SPACE would be just another
> > intermediat change.
> > >
> > > Please reconsider.
> > >
> > > Thanks
> > > Tomas
> > 
> > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> 
> 
> Does it mean you're Okay with the patch now?
> Thanks
> Tomas

The change looks good but I'll have to test it.

/Jarkko

[PATCH] tpm: separate cmd_ready/go_idle from runtime_pm

[Jarkko Sakkinen]

On Wed, May 30, 2018 at 10:52:28AM +0000, Winkler, Tomas wrote:
> > 
> > On Wed, May 23, 2018 at 01:48:17PM +0000, Winkler, Tomas wrote:
> > >
> > > > On Tue, May 22, 2018 at 09:27:46AM +0000, Winkler, Tomas wrote:
> > > > > >
> > > > > > On Wed, May 16, 2018 at 10:46:00PM +0300, Tomas Winkler wrote:
> > > > > > > New wrappers are added tpm_cmd_ready() and tpm_go_idle()
> > > > wrappers
> > > > > > > to streamline tpm_try_transmit code. TPM_TRANSMIT_UNLOCKED
> > > > > > > flag
> > > > is
> > > > > > abused
> > > > > > > to resolve tpm spaces recursive calls to tpm_transmit().
> > > > > >
> > > > > > This looks good and all but I don't think we want to abuse
> > > > > > anything in the driver code, do we?
> > > > >
> > > > > It's not abuse just the flag UNLOCKED is not really named
> > > > > correctly I think this has to be backported so wanted to do less invasive
> > change.
> > > >
> > > > It should be renamed anyway and possible merge conflicts are not
> > > > hard to sort out in this change. Can you rename it as SPACE?
> > >
> > > Not sure, I believe UNLOCKED is still better name than SPACE, I'm not
> > > sure this is Do you also want to remove TPM_TRANSMIT_RAW?
> > > clk_enable is handling its own anti recursion counter 'data-
> > >clkrun_enabled'
> > > but it should be all handled under one flag I guess.
> > >
> > > > Right, and even without rename this will probably cause merge
> > > > conflicts at least in v4.4 an v4.9 since in-kernel RM landed in
> > > > v4.12, so not much gain not do the rename :-)
> > >
> > > I belive we should do minimal change and the big cleanup after that.
> > > Not sure, I believe UNLOCKED is still better name than SPACE even it wasn't
> > the original intention.
> > > No the SPACE is the issue, but any recursion call into tpm_transmit. A
> > > bigger change is needed and rename to SPACE would be just another
> > intermediat change.
> > >
> > > Please reconsider.
> > >
> > > Thanks
> > > Tomas
> > 
> > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> 
> 
> Does it mean you're Okay with the patch now?
> Thanks
> Tomas

The change looks good but I'll have to test it.

/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] tpm: separate cmd_ready/go_idle from runtime_pm

[Winkler, Tomas]

> 
> On Wed, May 30, 2018 at 10:52:28AM +0000, Winkler, Tomas wrote:
> > >
> > > On Wed, May 23, 2018 at 01:48:17PM +0000, Winkler, Tomas wrote:
> > > >
> > > > > On Tue, May 22, 2018 at 09:27:46AM +0000, Winkler, Tomas wrote:
> > > > > > >
> > > > > > > On Wed, May 16, 2018 at 10:46:00PM +0300, Tomas Winkler
> wrote:
> > > > > > > > New wrappers are added tpm_cmd_ready() and tpm_go_idle()
> > > > > wrappers
> > > > > > > > to streamline tpm_try_transmit code.
> TPM_TRANSMIT_UNLOCKED
> > > > > > > > flag
> > > > > is
> > > > > > > abused
> > > > > > > > to resolve tpm spaces recursive calls to tpm_transmit().
> > > > > > >
> > > > > > > This looks good and all but I don't think we want to abuse
> > > > > > > anything in the driver code, do we?
> > > > > >
> > > > > > It's not abuse just the flag UNLOCKED is not really named
> > > > > > correctly I think this has to be backported so wanted to do
> > > > > > less invasive
> > > change.
> > > > >
> > > > > It should be renamed anyway and possible merge conflicts are not
> > > > > hard to sort out in this change. Can you rename it as SPACE?
> > > >
> > > > Not sure, I believe UNLOCKED is still better name than SPACE, I'm
> > > >not  sure this is Do you also want to remove TPM_TRANSMIT_RAW?
> > > > clk_enable is handling its own anti recursion counter 'data-
> > > >clkrun_enabled'
> > > > but it should be all handled under one flag I guess.
> > > >
> > > > > Right, and even without rename this will probably cause merge
> > > > > conflicts at least in v4.4 an v4.9 since in-kernel RM landed in
> > > > > v4.12, so not much gain not do the rename :-)
> > > >
> > > > I belive we should do minimal change and the big cleanup after that.
> > > > Not sure, I believe UNLOCKED is still better name than SPACE even
> > > > it wasn't
> > > the original intention.
> > > > No the SPACE is the issue, but any recursion call into
> > > > tpm_transmit. A bigger change is needed and rename to SPACE would
> > > > be just another
> > > intermediat change.
> > > >
> > > > Please reconsider.
> > > >
> > > > Thanks
> > > > Tomas
> > >
> > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> >
> >
> > Does it mean you're Okay with the patch now?
> > Thanks
> > Tomas
> 
> The change looks good but I'll have to test it.
Any updates?
Thanks

[PATCH] tpm: separate cmd_ready/go_idle from runtime_pm

[Winkler, Tomas]

> 
> On Wed, May 30, 2018 at 10:52:28AM +0000, Winkler, Tomas wrote:
> > >
> > > On Wed, May 23, 2018 at 01:48:17PM +0000, Winkler, Tomas wrote:
> > > >
> > > > > On Tue, May 22, 2018 at 09:27:46AM +0000, Winkler, Tomas wrote:
> > > > > > >
> > > > > > > On Wed, May 16, 2018 at 10:46:00PM +0300, Tomas Winkler
> wrote:
> > > > > > > > New wrappers are added tpm_cmd_ready() and tpm_go_idle()
> > > > > wrappers
> > > > > > > > to streamline tpm_try_transmit code.
> TPM_TRANSMIT_UNLOCKED
> > > > > > > > flag
> > > > > is
> > > > > > > abused
> > > > > > > > to resolve tpm spaces recursive calls to tpm_transmit().
> > > > > > >
> > > > > > > This looks good and all but I don't think we want to abuse
> > > > > > > anything in the driver code, do we?
> > > > > >
> > > > > > It's not abuse just the flag UNLOCKED is not really named
> > > > > > correctly I think this has to be backported so wanted to do
> > > > > > less invasive
> > > change.
> > > > >
> > > > > It should be renamed anyway and possible merge conflicts are not
> > > > > hard to sort out in this change. Can you rename it as SPACE?
> > > >
> > > > Not sure, I believe UNLOCKED is still better name than SPACE, I'm
> > > >not  sure this is Do you also want to remove TPM_TRANSMIT_RAW?
> > > > clk_enable is handling its own anti recursion counter 'data-
> > > >clkrun_enabled'
> > > > but it should be all handled under one flag I guess.
> > > >
> > > > > Right, and even without rename this will probably cause merge
> > > > > conflicts at least in v4.4 an v4.9 since in-kernel RM landed in
> > > > > v4.12, so not much gain not do the rename :-)
> > > >
> > > > I belive we should do minimal change and the big cleanup after that.
> > > > Not sure, I believe UNLOCKED is still better name than SPACE even
> > > > it wasn't
> > > the original intention.
> > > > No the SPACE is the issue, but any recursion call into
> > > > tpm_transmit. A bigger change is needed and rename to SPACE would
> > > > be just another
> > > intermediat change.
> > > >
> > > > Please reconsider.
> > > >
> > > > Thanks
> > > > Tomas
> > >
> > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> >
> >
> > Does it mean you're Okay with the patch now?
> > Thanks
> > Tomas
> 
> The change looks good but I'll have to test it.
Any updates?
Thanks

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] tpm: separate cmd_ready/go_idle from runtime_pm

[Jarkko Sakkinen]

On Wed, Jun 06, 2018 at 11:01:42AM +0000, Winkler, Tomas wrote:
> > 
> > On Wed, May 30, 2018 at 10:52:28AM +0000, Winkler, Tomas wrote:
> > > >
> > > > On Wed, May 23, 2018 at 01:48:17PM +0000, Winkler, Tomas wrote:
> > > > >
> > > > > > On Tue, May 22, 2018 at 09:27:46AM +0000, Winkler, Tomas wrote:
> > > > > > > >
> > > > > > > > On Wed, May 16, 2018 at 10:46:00PM +0300, Tomas Winkler
> > wrote:
> > > > > > > > > New wrappers are added tpm_cmd_ready() and tpm_go_idle()
> > > > > > wrappers
> > > > > > > > > to streamline tpm_try_transmit code.
> > TPM_TRANSMIT_UNLOCKED
> > > > > > > > > flag
> > > > > > is
> > > > > > > > abused
> > > > > > > > > to resolve tpm spaces recursive calls to tpm_transmit().
> > > > > > > >
> > > > > > > > This looks good and all but I don't think we want to abuse
> > > > > > > > anything in the driver code, do we?
> > > > > > >
> > > > > > > It's not abuse just the flag UNLOCKED is not really named
> > > > > > > correctly I think this has to be backported so wanted to do
> > > > > > > less invasive
> > > > change.
> > > > > >
> > > > > > It should be renamed anyway and possible merge conflicts are not
> > > > > > hard to sort out in this change. Can you rename it as SPACE?
> > > > >
> > > > > Not sure, I believe UNLOCKED is still better name than SPACE, I'm
> > > > >not  sure this is Do you also want to remove TPM_TRANSMIT_RAW?
> > > > > clk_enable is handling its own anti recursion counter 'data-
> > > > >clkrun_enabled'
> > > > > but it should be all handled under one flag I guess.
> > > > >
> > > > > > Right, and even without rename this will probably cause merge
> > > > > > conflicts at least in v4.4 an v4.9 since in-kernel RM landed in
> > > > > > v4.12, so not much gain not do the rename :-)
> > > > >
> > > > > I belive we should do minimal change and the big cleanup after that.
> > > > > Not sure, I believe UNLOCKED is still better name than SPACE even
> > > > > it wasn't
> > > > the original intention.
> > > > > No the SPACE is the issue, but any recursion call into
> > > > > tpm_transmit. A bigger change is needed and rename to SPACE would
> > > > > be just another
> > > > intermediat change.
> > > > >
> > > > > Please reconsider.
> > > > >
> > > > > Thanks
> > > > > Tomas
> > > >
> > > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > >
> > >
> > > Does it mean you're Okay with the patch now?
> > > Thanks
> > > Tomas
> > 
> > The change looks good but I'll have to test it.
> Any updates?
> Thanks

Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

/Jarkko

[PATCH] tpm: separate cmd_ready/go_idle from runtime_pm

[Jarkko Sakkinen]

On Wed, Jun 06, 2018 at 11:01:42AM +0000, Winkler, Tomas wrote:
> > 
> > On Wed, May 30, 2018 at 10:52:28AM +0000, Winkler, Tomas wrote:
> > > >
> > > > On Wed, May 23, 2018 at 01:48:17PM +0000, Winkler, Tomas wrote:
> > > > >
> > > > > > On Tue, May 22, 2018 at 09:27:46AM +0000, Winkler, Tomas wrote:
> > > > > > > >
> > > > > > > > On Wed, May 16, 2018 at 10:46:00PM +0300, Tomas Winkler
> > wrote:
> > > > > > > > > New wrappers are added tpm_cmd_ready() and tpm_go_idle()
> > > > > > wrappers
> > > > > > > > > to streamline tpm_try_transmit code.
> > TPM_TRANSMIT_UNLOCKED
> > > > > > > > > flag
> > > > > > is
> > > > > > > > abused
> > > > > > > > > to resolve tpm spaces recursive calls to tpm_transmit().
> > > > > > > >
> > > > > > > > This looks good and all but I don't think we want to abuse
> > > > > > > > anything in the driver code, do we?
> > > > > > >
> > > > > > > It's not abuse just the flag UNLOCKED is not really named
> > > > > > > correctly I think this has to be backported so wanted to do
> > > > > > > less invasive
> > > > change.
> > > > > >
> > > > > > It should be renamed anyway and possible merge conflicts are not
> > > > > > hard to sort out in this change. Can you rename it as SPACE?
> > > > >
> > > > > Not sure, I believe UNLOCKED is still better name than SPACE, I'm
> > > > >not  sure this is Do you also want to remove TPM_TRANSMIT_RAW?
> > > > > clk_enable is handling its own anti recursion counter 'data-
> > > > >clkrun_enabled'
> > > > > but it should be all handled under one flag I guess.
> > > > >
> > > > > > Right, and even without rename this will probably cause merge
> > > > > > conflicts at least in v4.4 an v4.9 since in-kernel RM landed in
> > > > > > v4.12, so not much gain not do the rename :-)
> > > > >
> > > > > I belive we should do minimal change and the big cleanup after that.
> > > > > Not sure, I believe UNLOCKED is still better name than SPACE even
> > > > > it wasn't
> > > > the original intention.
> > > > > No the SPACE is the issue, but any recursion call into
> > > > > tpm_transmit. A bigger change is needed and rename to SPACE would
> > > > > be just another
> > > > intermediat change.
> > > > >
> > > > > Please reconsider.
> > > > >
> > > > > Thanks
> > > > > Tomas
> > > >
> > > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > >
> > >
> > > Does it mean you're Okay with the patch now?
> > > Thanks
> > > Tomas
> > 
> > The change looks good but I'll have to test it.
> Any updates?
> Thanks

Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] tpm: separate cmd_ready/go_idle from runtime_pm

[Winkler, Tomas]

> -----Original Message-----
> From: Jarkko Sakkinen [mailto:jarkko.sakkinen@linux.intel.com]
> Sent: Thursday, June 07, 2018 13:25
> To: Winkler, Tomas <tomas.winkler@intel.com>
> Cc: Jason Gunthorpe <jgg@ziepe.ca>; Usyskin, Alexander
> <alexander.usyskin@intel.com>; linux-integrity@vger.kernel.org; linux-
> security-module@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] tpm: separate cmd_ready/go_idle from runtime_pm
> 
> On Wed, Jun 06, 2018 at 11:01:42AM +0000, Winkler, Tomas wrote:
> > >
> > > On Wed, May 30, 2018 at 10:52:28AM +0000, Winkler, Tomas wrote:
> > > > >
> > > > > On Wed, May 23, 2018 at 01:48:17PM +0000, Winkler, Tomas wrote:
> > > > > >
> > > > > > > On Tue, May 22, 2018 at 09:27:46AM +0000, Winkler, Tomas
> wrote:
> > > > > > > > >
> > > > > > > > > On Wed, May 16, 2018 at 10:46:00PM +0300, Tomas Winkler
> > > wrote:
> > > > > > > > > > New wrappers are added tpm_cmd_ready() and
> > > > > > > > > > tpm_go_idle()
> > > > > > > wrappers
> > > > > > > > > > to streamline tpm_try_transmit code.
> > > TPM_TRANSMIT_UNLOCKED
> > > > > > > > > > flag
> > > > > > > is
> > > > > > > > > abused
> > > > > > > > > > to resolve tpm spaces recursive calls to tpm_transmit().
> > > > > > > > >
> > > > > > > > > This looks good and all but I don't think we want to
> > > > > > > > > abuse anything in the driver code, do we?
> > > > > > > >
> > > > > > > > It's not abuse just the flag UNLOCKED is not really named
> > > > > > > > correctly I think this has to be backported so wanted to
> > > > > > > > do less invasive
> > > > > change.
> > > > > > >
> > > > > > > It should be renamed anyway and possible merge conflicts are
> > > > > > > not hard to sort out in this change. Can you rename it as SPACE?
> > > > > >
> > > > > > Not sure, I believe UNLOCKED is still better name than SPACE,
> > > > > >I'm not  sure this is Do you also want to remove
> TPM_TRANSMIT_RAW?
> > > > > > clk_enable is handling its own anti recursion counter 'data-
> > > > > >clkrun_enabled'
> > > > > > but it should be all handled under one flag I guess.
> > > > > >
> > > > > > > Right, and even without rename this will probably cause
> > > > > > > merge conflicts at least in v4.4 an v4.9 since in-kernel RM
> > > > > > > landed in v4.12, so not much gain not do the rename :-)
> > > > > >
> > > > > > I belive we should do minimal change and the big cleanup after
> that.
> > > > > > Not sure, I believe UNLOCKED is still better name than SPACE
> > > > > > even it wasn't
> > > > > the original intention.
> > > > > > No the SPACE is the issue, but any recursion call into
> > > > > > tpm_transmit. A bigger change is needed and rename to SPACE
> > > > > > would be just another
> > > > > intermediat change.
> > > > > >
> > > > > > Please reconsider.
> > > > > >
> > > > > > Thanks
> > > > > > Tomas
> > > > >
> > > > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > > >
> > > >
> > > > Does it mean you're Okay with the patch now?
> > > > Thanks
> > > > Tomas
> > >
> > > The change looks good but I'll have to test it.
> > Any updates?
> > Thanks
> 
> Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

I've just realized we have issue in tpm_unseal_trusted() 
As TPM_TRANSMIT_UNLOCKED is used really just in 'locking' sense of the flow, it's not nested.
Any of testing flows doesn't covers it. It's used only from by security/keys/trusted.c only

Then I don't have a short fix for this issue. Will use TPM_TRANSMIT_RAW,
 maybe calling it TPM_TRANSMIT_NESTED.  

Thanks
Tomas


> 
> /Jarkko

[PATCH] tpm: separate cmd_ready/go_idle from runtime_pm

[Winkler, Tomas]

> -----Original Message-----
> From: Jarkko Sakkinen [mailto:jarkko.sakkinen at linux.intel.com]
> Sent: Thursday, June 07, 2018 13:25
> To: Winkler, Tomas <tomas.winkler@intel.com>
> Cc: Jason Gunthorpe <jgg@ziepe.ca>; Usyskin, Alexander
> <alexander.usyskin@intel.com>; linux-integrity at vger.kernel.org; linux-
> security-module at vger.kernel.org; linux-kernel at vger.kernel.org
> Subject: Re: [PATCH] tpm: separate cmd_ready/go_idle from runtime_pm
> 
> On Wed, Jun 06, 2018 at 11:01:42AM +0000, Winkler, Tomas wrote:
> > >
> > > On Wed, May 30, 2018 at 10:52:28AM +0000, Winkler, Tomas wrote:
> > > > >
> > > > > On Wed, May 23, 2018 at 01:48:17PM +0000, Winkler, Tomas wrote:
> > > > > >
> > > > > > > On Tue, May 22, 2018 at 09:27:46AM +0000, Winkler, Tomas
> wrote:
> > > > > > > > >
> > > > > > > > > On Wed, May 16, 2018 at 10:46:00PM +0300, Tomas Winkler
> > > wrote:
> > > > > > > > > > New wrappers are added tpm_cmd_ready() and
> > > > > > > > > > tpm_go_idle()
> > > > > > > wrappers
> > > > > > > > > > to streamline tpm_try_transmit code.
> > > TPM_TRANSMIT_UNLOCKED
> > > > > > > > > > flag
> > > > > > > is
> > > > > > > > > abused
> > > > > > > > > > to resolve tpm spaces recursive calls to tpm_transmit().
> > > > > > > > >
> > > > > > > > > This looks good and all but I don't think we want to
> > > > > > > > > abuse anything in the driver code, do we?
> > > > > > > >
> > > > > > > > It's not abuse just the flag UNLOCKED is not really named
> > > > > > > > correctly I think this has to be backported so wanted to
> > > > > > > > do less invasive
> > > > > change.
> > > > > > >
> > > > > > > It should be renamed anyway and possible merge conflicts are
> > > > > > > not hard to sort out in this change. Can you rename it as SPACE?
> > > > > >
> > > > > > Not sure, I believe UNLOCKED is still better name than SPACE,
> > > > > >I'm not  sure this is Do you also want to remove
> TPM_TRANSMIT_RAW?
> > > > > > clk_enable is handling its own anti recursion counter 'data-
> > > > > >clkrun_enabled'
> > > > > > but it should be all handled under one flag I guess.
> > > > > >
> > > > > > > Right, and even without rename this will probably cause
> > > > > > > merge conflicts at least in v4.4 an v4.9 since in-kernel RM
> > > > > > > landed in v4.12, so not much gain not do the rename :-)
> > > > > >
> > > > > > I belive we should do minimal change and the big cleanup after
> that.
> > > > > > Not sure, I believe UNLOCKED is still better name than SPACE
> > > > > > even it wasn't
> > > > > the original intention.
> > > > > > No the SPACE is the issue, but any recursion call into
> > > > > > tpm_transmit. A bigger change is needed and rename to SPACE
> > > > > > would be just another
> > > > > intermediat change.
> > > > > >
> > > > > > Please reconsider.
> > > > > >
> > > > > > Thanks
> > > > > > Tomas
> > > > >
> > > > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > > >
> > > >
> > > > Does it mean you're Okay with the patch now?
> > > > Thanks
> > > > Tomas
> > >
> > > The change looks good but I'll have to test it.
> > Any updates?
> > Thanks
> 
> Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

I've just realized we have issue in tpm_unseal_trusted() 
As TPM_TRANSMIT_UNLOCKED is used really just in 'locking' sense of the flow, it's not nested.
Any of testing flows doesn't covers it. It's used only from by security/keys/trusted.c only

Then I don't have a short fix for this issue. Will use TPM_TRANSMIT_RAW,
 maybe calling it TPM_TRANSMIT_NESTED.  

Thanks
Tomas


> 
> /Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] tpm: separate cmd_ready/go_idle from runtime_pm

[Jarkko Sakkinen]

On Thu, Jun 07, 2018 at 11:03:50AM +0000, Winkler, Tomas wrote:
> 
> 
> > -----Original Message-----
> > From: Jarkko Sakkinen [mailto:jarkko.sakkinen@linux.intel.com]
> > Sent: Thursday, June 07, 2018 13:25
> > To: Winkler, Tomas <tomas.winkler@intel.com>
> > Cc: Jason Gunthorpe <jgg@ziepe.ca>; Usyskin, Alexander
> > <alexander.usyskin@intel.com>; linux-integrity@vger.kernel.org; linux-
> > security-module@vger.kernel.org; linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH] tpm: separate cmd_ready/go_idle from runtime_pm
> > 
> > On Wed, Jun 06, 2018 at 11:01:42AM +0000, Winkler, Tomas wrote:
> > > >
> > > > On Wed, May 30, 2018 at 10:52:28AM +0000, Winkler, Tomas wrote:
> > > > > >
> > > > > > On Wed, May 23, 2018 at 01:48:17PM +0000, Winkler, Tomas wrote:
> > > > > > >
> > > > > > > > On Tue, May 22, 2018 at 09:27:46AM +0000, Winkler, Tomas
> > wrote:
> > > > > > > > > >
> > > > > > > > > > On Wed, May 16, 2018 at 10:46:00PM +0300, Tomas Winkler
> > > > wrote:
> > > > > > > > > > > New wrappers are added tpm_cmd_ready() and
> > > > > > > > > > > tpm_go_idle()
> > > > > > > > wrappers
> > > > > > > > > > > to streamline tpm_try_transmit code.
> > > > TPM_TRANSMIT_UNLOCKED
> > > > > > > > > > > flag
> > > > > > > > is
> > > > > > > > > > abused
> > > > > > > > > > > to resolve tpm spaces recursive calls to tpm_transmit().
> > > > > > > > > >
> > > > > > > > > > This looks good and all but I don't think we want to
> > > > > > > > > > abuse anything in the driver code, do we?
> > > > > > > > >
> > > > > > > > > It's not abuse just the flag UNLOCKED is not really named
> > > > > > > > > correctly I think this has to be backported so wanted to
> > > > > > > > > do less invasive
> > > > > > change.
> > > > > > > >
> > > > > > > > It should be renamed anyway and possible merge conflicts are
> > > > > > > > not hard to sort out in this change. Can you rename it as SPACE?
> > > > > > >
> > > > > > > Not sure, I believe UNLOCKED is still better name than SPACE,
> > > > > > >I'm not  sure this is Do you also want to remove
> > TPM_TRANSMIT_RAW?
> > > > > > > clk_enable is handling its own anti recursion counter 'data-
> > > > > > >clkrun_enabled'
> > > > > > > but it should be all handled under one flag I guess.
> > > > > > >
> > > > > > > > Right, and even without rename this will probably cause
> > > > > > > > merge conflicts at least in v4.4 an v4.9 since in-kernel RM
> > > > > > > > landed in v4.12, so not much gain not do the rename :-)
> > > > > > >
> > > > > > > I belive we should do minimal change and the big cleanup after
> > that.
> > > > > > > Not sure, I believe UNLOCKED is still better name than SPACE
> > > > > > > even it wasn't
> > > > > > the original intention.
> > > > > > > No the SPACE is the issue, but any recursion call into
> > > > > > > tpm_transmit. A bigger change is needed and rename to SPACE
> > > > > > > would be just another
> > > > > > intermediat change.
> > > > > > >
> > > > > > > Please reconsider.
> > > > > > >
> > > > > > > Thanks
> > > > > > > Tomas
> > > > > >
> > > > > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > > > >
> > > > >
> > > > > Does it mean you're Okay with the patch now?
> > > > > Thanks
> > > > > Tomas
> > > >
> > > > The change looks good but I'll have to test it.
> > > Any updates?
> > > Thanks
> > 
> > Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> 
> I've just realized we have issue in tpm_unseal_trusted() 
> As TPM_TRANSMIT_UNLOCKED is used really just in 'locking' sense of the flow, it's not nested.
> Any of testing flows doesn't covers it. It's used only from by security/keys/trusted.c only
> 
> Then I don't have a short fix for this issue. Will use TPM_TRANSMIT_RAW,
>  maybe calling it TPM_TRANSMIT_NESTED.  

Ah, nested would a good name for that.

/Jarkko

[PATCH] tpm: separate cmd_ready/go_idle from runtime_pm

[Jarkko Sakkinen]

On Thu, Jun 07, 2018 at 11:03:50AM +0000, Winkler, Tomas wrote:
> 
> 
> > -----Original Message-----
> > From: Jarkko Sakkinen [mailto:jarkko.sakkinen at linux.intel.com]
> > Sent: Thursday, June 07, 2018 13:25
> > To: Winkler, Tomas <tomas.winkler@intel.com>
> > Cc: Jason Gunthorpe <jgg@ziepe.ca>; Usyskin, Alexander
> > <alexander.usyskin@intel.com>; linux-integrity at vger.kernel.org; linux-
> > security-module at vger.kernel.org; linux-kernel at vger.kernel.org
> > Subject: Re: [PATCH] tpm: separate cmd_ready/go_idle from runtime_pm
> > 
> > On Wed, Jun 06, 2018 at 11:01:42AM +0000, Winkler, Tomas wrote:
> > > >
> > > > On Wed, May 30, 2018 at 10:52:28AM +0000, Winkler, Tomas wrote:
> > > > > >
> > > > > > On Wed, May 23, 2018 at 01:48:17PM +0000, Winkler, Tomas wrote:
> > > > > > >
> > > > > > > > On Tue, May 22, 2018 at 09:27:46AM +0000, Winkler, Tomas
> > wrote:
> > > > > > > > > >
> > > > > > > > > > On Wed, May 16, 2018 at 10:46:00PM +0300, Tomas Winkler
> > > > wrote:
> > > > > > > > > > > New wrappers are added tpm_cmd_ready() and
> > > > > > > > > > > tpm_go_idle()
> > > > > > > > wrappers
> > > > > > > > > > > to streamline tpm_try_transmit code.
> > > > TPM_TRANSMIT_UNLOCKED
> > > > > > > > > > > flag
> > > > > > > > is
> > > > > > > > > > abused
> > > > > > > > > > > to resolve tpm spaces recursive calls to tpm_transmit().
> > > > > > > > > >
> > > > > > > > > > This looks good and all but I don't think we want to
> > > > > > > > > > abuse anything in the driver code, do we?
> > > > > > > > >
> > > > > > > > > It's not abuse just the flag UNLOCKED is not really named
> > > > > > > > > correctly I think this has to be backported so wanted to
> > > > > > > > > do less invasive
> > > > > > change.
> > > > > > > >
> > > > > > > > It should be renamed anyway and possible merge conflicts are
> > > > > > > > not hard to sort out in this change. Can you rename it as SPACE?
> > > > > > >
> > > > > > > Not sure, I believe UNLOCKED is still better name than SPACE,
> > > > > > >I'm not  sure this is Do you also want to remove
> > TPM_TRANSMIT_RAW?
> > > > > > > clk_enable is handling its own anti recursion counter 'data-
> > > > > > >clkrun_enabled'
> > > > > > > but it should be all handled under one flag I guess.
> > > > > > >
> > > > > > > > Right, and even without rename this will probably cause
> > > > > > > > merge conflicts at least in v4.4 an v4.9 since in-kernel RM
> > > > > > > > landed in v4.12, so not much gain not do the rename :-)
> > > > > > >
> > > > > > > I belive we should do minimal change and the big cleanup after
> > that.
> > > > > > > Not sure, I believe UNLOCKED is still better name than SPACE
> > > > > > > even it wasn't
> > > > > > the original intention.
> > > > > > > No the SPACE is the issue, but any recursion call into
> > > > > > > tpm_transmit. A bigger change is needed and rename to SPACE
> > > > > > > would be just another
> > > > > > intermediat change.
> > > > > > >
> > > > > > > Please reconsider.
> > > > > > >
> > > > > > > Thanks
> > > > > > > Tomas
> > > > > >
> > > > > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > > > >
> > > > >
> > > > > Does it mean you're Okay with the patch now?
> > > > > Thanks
> > > > > Tomas
> > > >
> > > > The change looks good but I'll have to test it.
> > > Any updates?
> > > Thanks
> > 
> > Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> 
> I've just realized we have issue in tpm_unseal_trusted() 
> As TPM_TRANSMIT_UNLOCKED is used really just in 'locking' sense of the flow, it's not nested.
> Any of testing flows doesn't covers it. It's used only from by security/keys/trusted.c only
> 
> Then I don't have a short fix for this issue. Will use TPM_TRANSMIT_RAW,
>  maybe calling it TPM_TRANSMIT_NESTED.  

Ah, nested would a good name for that.

/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] tpm: separate cmd_ready/go_idle from runtime_pm

[Jarkko Sakkinen]

On Mon, Sep 17, 2018 at 06:47:06AM +0200, Greg KH wrote:
> On Mon, Sep 17, 2018 at 12:55:24AM +0000, Sasha Levin wrote:
> > On Sun, Sep 16, 2018 at 02:46:42PM +0200, Greg KH wrote:
> > >On Sun, Sep 16, 2018 at 03:02:27PM +0300, Jarkko Sakkinen wrote:
> > >> From: Tomas Winkler <tomas.winkler@intel.com>
> > >>
> > >> commit 627448e85c766587f6fdde1ea3886d6615081c77 upstream
> > >
> > >Are you sure? That commit is not in Linus's tree :(
> > 
> > Are you sure? I see it went in during the 4.19 merge window.
> 
> Ugh, it's been a long week, sorry, you are right, my mistake, I was in
> the wrong git repo when I checked this.
> 
> greg k-h

Anyway remainded me that one should is always double check the mainline
tree (which I admit did not do, only James Morris' tree).

/Jarkko

Re: [PATCH] tpm: separate cmd_ready/go_idle from runtime_pm

[Greg KH]

On Sun, Sep 16, 2018 at 03:02:27PM +0300, Jarkko Sakkinen wrote:
> From: Tomas Winkler <tomas.winkler@intel.com>
> 
> commit 627448e85c766587f6fdde1ea3886d6615081c77 upstream

Thanks, now queued up.

greg k-h

RE: [PATCH] tpm: separate cmd_ready/go_idle from runtime_pm

[Winkler, Tomas]

> 
> On Sun, Sep 16, 2018 at 02:46:42PM +0200, Greg KH wrote:
> >On Sun, Sep 16, 2018 at 03:02:27PM +0300, Jarkko Sakkinen wrote:
> >> From: Tomas Winkler <tomas.winkler@intel.com>
> >>
> >> commit 627448e85c766587f6fdde1ea3886d6615081c77 upstream
> >
> >Are you sure? That commit is not in Linus's tree :(
> 
> Are you sure? I see it went in during the 4.19 merge window.

I see it as well in the  master branch under this exact SHA1.

Tomas

Re: [PATCH] tpm: separate cmd_ready/go_idle from runtime_pm

[Greg KH]

On Mon, Sep 17, 2018 at 12:55:24AM +0000, Sasha Levin wrote:
> On Sun, Sep 16, 2018 at 02:46:42PM +0200, Greg KH wrote:
> >On Sun, Sep 16, 2018 at 03:02:27PM +0300, Jarkko Sakkinen wrote:
> >> From: Tomas Winkler <tomas.winkler@intel.com>
> >>
> >> commit 627448e85c766587f6fdde1ea3886d6615081c77 upstream
> >
> >Are you sure? That commit is not in Linus's tree :(
> 
> Are you sure? I see it went in during the 4.19 merge window.

Ugh, it's been a long week, sorry, you are right, my mistake, I was in
the wrong git repo when I checked this.

greg k-h

Re: [PATCH] tpm: separate cmd_ready/go_idle from runtime_pm

[Sasha Levin]

On Sun, Sep 16, 2018 at 02:46:42PM +0200, Greg KH wrote:
>On Sun, Sep 16, 2018 at 03:02:27PM +0300, Jarkko Sakkinen wrote:
>> From: Tomas Winkler <tomas.winkler@intel.com>
>>
>> commit 627448e85c766587f6fdde1ea3886d6615081c77 upstream
>
>Are you sure? That commit is not in Linus's tree :(

Are you sure? I see it went in during the 4.19 merge window.

Re: [PATCH] tpm: separate cmd_ready/go_idle from runtime_pm

[Greg KH]

On Sun, Sep 16, 2018 at 03:02:27PM +0300, Jarkko Sakkinen wrote:
> From: Tomas Winkler <tomas.winkler@intel.com>
> 
> commit 627448e85c766587f6fdde1ea3886d6615081c77 upstream

Are you sure? That commit is not in Linus's tree :(

[PATCH] tpm: separate cmd_ready/go_idle from runtime_pm

[Jarkko Sakkinen]

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

commit 627448e85c766587f6fdde1ea3886d6615081c77 upstream

Fix tpm ptt initialization error:
tpm tpm0: A TPM error (378) occurred get tpm pcr allocation.

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 a power saving, it's also a 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.

When calling from nested context always use both flags:
TPM_TRANSMIT_UNLOCKED and TPM_TRANSMIT_RAW. Both are needed to resolve
tpm spaces and locality request recursive calls to tpm_transmit().
TPM_TRANSMIT_RAW should never be used standalone as it will fail
on double locking. While TPM_TRANSMIT_UNLOCKED standalone should be
called from non-recursive locked contexts.

New wrappers are added tpm_cmd_ready() and tpm_go_idle() to
streamline tpm_try_transmit code.

tpm_crb no longer needs own power saving functions and can drop using
tpm_pm_suspend/resume.

This patch cannot be really separated from the locality fix.
Fixes: 888d867df441 (tpm: cmd_ready command can be issued only after granting locality)

Cc: stable@vger.kernel.org
Fixes: 888d867df441 (tpm: cmd_ready command can be issued only after granting locality)
Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 drivers/char/tpm/tpm-interface.c |  50 ++++++++++++---
 drivers/char/tpm/tpm.h           |  12 +++-
 drivers/char/tpm/tpm2-space.c    |  16 +++--
 drivers/char/tpm/tpm_crb.c       | 101 +++++++++----------------------
 include/linux/tpm.h              |   2 +
 5 files changed, 90 insertions(+), 91 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 86b526b7d990..a2070ab86c82 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -369,10 +369,13 @@ static int tpm_validate_command(struct tpm_chip *chip,
 	return -EINVAL;
 }
 
-static int tpm_request_locality(struct tpm_chip *chip)
+static int tpm_request_locality(struct tpm_chip *chip, unsigned int flags)
 {
 	int rc;
 
+	if (flags & TPM_TRANSMIT_RAW)
+		return 0;
+
 	if (!chip->ops->request_locality)
 		return 0;
 
@@ -385,10 +388,13 @@ static int tpm_request_locality(struct tpm_chip *chip)
 	return 0;
 }
 
-static void tpm_relinquish_locality(struct tpm_chip *chip)
+static void tpm_relinquish_locality(struct tpm_chip *chip, unsigned int flags)
 {
 	int rc;
 
+	if (flags & TPM_TRANSMIT_RAW)
+		return;
+
 	if (!chip->ops->relinquish_locality)
 		return;
 
@@ -399,6 +405,28 @@ static void tpm_relinquish_locality(struct tpm_chip *chip)
 	chip->locality = -1;
 }
 
+static int tpm_cmd_ready(struct tpm_chip *chip, unsigned int flags)
+{
+	if (flags & TPM_TRANSMIT_RAW)
+		return 0;
+
+	if (!chip->ops->cmd_ready)
+		return 0;
+
+	return chip->ops->cmd_ready(chip);
+}
+
+static int tpm_go_idle(struct tpm_chip *chip, unsigned int flags)
+{
+	if (flags & TPM_TRANSMIT_RAW)
+		return 0;
+
+	if (!chip->ops->go_idle)
+		return 0;
+
+	return chip->ops->go_idle(chip);
+}
+
 static ssize_t tpm_try_transmit(struct tpm_chip *chip,
 				struct tpm_space *space,
 				u8 *buf, size_t bufsiz,
@@ -449,14 +477,15 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip,
 	/* Store the decision as chip->locality will be changed. */
 	need_locality = chip->locality == -1;
 
-	if (!(flags & TPM_TRANSMIT_RAW) && need_locality) {
-		rc = tpm_request_locality(chip);
+	if (need_locality) {
+		rc = tpm_request_locality(chip, flags);
 		if (rc < 0)
 			goto out_no_locality;
 	}
 
-	if (chip->dev.parent)
-		pm_runtime_get_sync(chip->dev.parent);
+	rc = tpm_cmd_ready(chip, flags);
+	if (rc)
+		goto out;
 
 	rc = tpm2_prepare_space(chip, space, ordinal, buf);
 	if (rc)
@@ -516,13 +545,16 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip,
 	}
 
 	rc = tpm2_commit_space(chip, space, ordinal, buf, &len);
+	if (rc)
+		dev_err(&chip->dev, "tpm2_commit_space: error %d\n", rc);
 
 out:
-	if (chip->dev.parent)
-		pm_runtime_put_sync(chip->dev.parent);
+	rc = tpm_go_idle(chip, flags);
+	if (rc)
+		goto out;
 
 	if (need_locality)
-		tpm_relinquish_locality(chip);
+		tpm_relinquish_locality(chip, flags);
 
 out_no_locality:
 	if (chip->ops->clk_enable != NULL)
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index b83b30a3eea5..4bb9b4aa9b49 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -511,9 +511,17 @@ extern const struct file_operations tpm_fops;
 extern const struct file_operations tpmrm_fops;
 extern struct idr dev_nums_idr;
 
+/**
+ * enum tpm_transmit_flags
+ *
+ * @TPM_TRANSMIT_UNLOCKED: used to lock sequence of tpm_transmit calls.
+ * @TPM_TRANSMIT_RAW: prevent recursive calls into setup steps
+ *                    (go idle, locality,..). Always use with UNLOCKED
+ *                    as it will fail on double locking.
+ */
 enum tpm_transmit_flags {
-	TPM_TRANSMIT_UNLOCKED	= BIT(0),
-	TPM_TRANSMIT_RAW	= BIT(1),
+	TPM_TRANSMIT_UNLOCKED = BIT(0),
+	TPM_TRANSMIT_RAW      = BIT(1),
 };
 
 ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2-space.c
index d26ea7513226..dabb2ae4e779 100644
--- a/drivers/char/tpm/tpm2-space.c
+++ b/drivers/char/tpm/tpm2-space.c
@@ -39,7 +39,8 @@ static void tpm2_flush_sessions(struct tpm_chip *chip, struct tpm_space *space)
 	for (i = 0; i < ARRAY_SIZE(space->session_tbl); i++) {
 		if (space->session_tbl[i])
 			tpm2_flush_context_cmd(chip, space->session_tbl[i],
-					       TPM_TRANSMIT_UNLOCKED);
+					       TPM_TRANSMIT_UNLOCKED |
+					       TPM_TRANSMIT_RAW);
 	}
 }
 
@@ -84,7 +85,7 @@ static int tpm2_load_context(struct tpm_chip *chip, u8 *buf,
 	tpm_buf_append(&tbuf, &buf[*offset], body_size);
 
 	rc = tpm_transmit_cmd(chip, NULL, tbuf.data, PAGE_SIZE, 4,
-			      TPM_TRANSMIT_UNLOCKED, NULL);
+			      TPM_TRANSMIT_UNLOCKED | TPM_TRANSMIT_RAW, NULL);
 	if (rc < 0) {
 		dev_warn(&chip->dev, "%s: failed with a system error %d\n",
 			 __func__, rc);
@@ -133,7 +134,7 @@ static int tpm2_save_context(struct tpm_chip *chip, u32 handle, u8 *buf,
 	tpm_buf_append_u32(&tbuf, handle);
 
 	rc = tpm_transmit_cmd(chip, NULL, tbuf.data, PAGE_SIZE, 0,
-			      TPM_TRANSMIT_UNLOCKED, NULL);
+			      TPM_TRANSMIT_UNLOCKED | TPM_TRANSMIT_RAW, NULL);
 	if (rc < 0) {
 		dev_warn(&chip->dev, "%s: failed with a system error %d\n",
 			 __func__, rc);
@@ -170,7 +171,8 @@ static void tpm2_flush_space(struct tpm_chip *chip)
 	for (i = 0; i < ARRAY_SIZE(space->context_tbl); i++)
 		if (space->context_tbl[i] && ~space->context_tbl[i])
 			tpm2_flush_context_cmd(chip, space->context_tbl[i],
-					       TPM_TRANSMIT_UNLOCKED);
+					       TPM_TRANSMIT_UNLOCKED |
+					       TPM_TRANSMIT_RAW);
 
 	tpm2_flush_sessions(chip, space);
 }
@@ -377,7 +379,8 @@ static int tpm2_map_response_header(struct tpm_chip *chip, u32 cc, u8 *rsp,
 
 	return 0;
 out_no_slots:
-	tpm2_flush_context_cmd(chip, phandle, TPM_TRANSMIT_UNLOCKED);
+	tpm2_flush_context_cmd(chip, phandle,
+			       TPM_TRANSMIT_UNLOCKED | TPM_TRANSMIT_RAW);
 	dev_warn(&chip->dev, "%s: out of slots for 0x%08X\n", __func__,
 		 phandle);
 	return -ENOMEM;
@@ -465,7 +468,8 @@ static int tpm2_save_space(struct tpm_chip *chip)
 			return rc;
 
 		tpm2_flush_context_cmd(chip, space->context_tbl[i],
-				       TPM_TRANSMIT_UNLOCKED);
+				       TPM_TRANSMIT_UNLOCKED |
+				       TPM_TRANSMIT_RAW);
 		space->context_tbl[i] = ~0;
 	}
 
diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index bb756ad7897e..5c7ce5aaaf6f 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -137,7 +137,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
@@ -151,7 +151,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->flags & CRB_FL_ACPI_START) ||
 	    (priv->flags & CRB_FL_CRB_SMC_START))
@@ -166,11 +166,20 @@ static int crb_go_idle(struct device *dev, struct crb_priv *priv)
 		dev_warn(dev, "goIdle timed out\n");
 		return -ETIME;
 	}
+
 	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
@@ -183,7 +192,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->flags & CRB_FL_ACPI_START) ||
 	    (priv->flags & CRB_FL_CRB_SMC_START))
@@ -201,6 +210,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)
 {
@@ -393,6 +410,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,
@@ -508,7 +527,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;
 
@@ -553,7 +572,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);
 
@@ -624,32 +643,7 @@ static int crb_acpi_add(struct acpi_device *device)
 	chip->acpi_dev_handle = device->handle;
 	chip->flags = TPM_CHIP_FLAG_TPM2;
 
-	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);
-
-	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);
-
-out:
-	__crb_relinquish_locality(dev, priv, 0);
-
-	return rc;
+	return tpm_chip_register(chip);
 }
 
 static int crb_acpi_remove(struct acpi_device *device)
@@ -659,52 +653,11 @@ 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)
-{
-	int ret;
-
-	ret = tpm_pm_suspend(dev);
-	if (ret)
-		return ret;
-
-	return crb_pm_runtime_suspend(dev);
-}
-
-static int __maybe_unused crb_pm_resume(struct device *dev)
-{
-	int ret;
-
-	ret = crb_pm_runtime_resume(dev);
-	if (ret)
-		return ret;
-
-	return tpm_pm_resume(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)
+	SET_SYSTEM_SLEEP_PM_OPS(tpm_pm_suspend, tpm_pm_resume)
 };
 
 static const struct acpi_device_id crb_device_ids[] = {
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index 2a6c3d96b31f..7f7b29f86c59 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -48,6 +48,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);
 	int (*relinquish_locality)(struct tpm_chip *chip, int loc);
 	void (*clk_enable)(struct tpm_chip *chip, bool value);
-- 
2.17.1