All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] tpm_crb: request and relinquish locality 0
@ 2017-03-11 13:02 ` Jarkko Sakkinen
  0 siblings, 0 replies; 17+ messages in thread
From: Jarkko Sakkinen @ 2017-03-11 13:02 UTC (permalink / raw)
  To: tpmdd-devel
  Cc: linux-security-module, Jerry Snitselaar, gang.wei,
	Jarkko Sakkinen, Peter Huewe, Marcel Selhorst, Jason Gunthorpe,
	open list

Added two new callbacks to struct tpm_class_ops:

- request_locality
- relinquish_locality

These are called before sending and receiving data from the TPM.  We
update also tpm_tis_core to use these callbacks. Small modification to
request_locality() is done so that it returns -EBUSY instead of locality
number when check_locality() fails.

Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 drivers/char/tpm/tpm-interface.c |  9 +++++++++
 drivers/char/tpm/tpm_crb.c       | 41 +++++++++++++++++++++++++++++++++++++++-
 drivers/char/tpm/tpm_tis_core.c  | 12 ++++--------
 include/linux/tpm.h              |  3 ++-
 4 files changed, 55 insertions(+), 10 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index e38c792..9c56581 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -407,6 +407,12 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
 	if (chip->dev.parent)
 		pm_runtime_get_sync(chip->dev.parent);
 
+	if (chip->ops->request_locality)  {
+		rc = chip->ops->request_locality(chip, 0);
+		if (rc)
+			goto out;
+	}
+
 	rc = tpm2_prepare_space(chip, space, ordinal, buf);
 	if (rc)
 		goto out;
@@ -466,6 +472,9 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
 	rc = tpm2_commit_space(chip, space, ordinal, buf, &len);
 
 out:
+	if (chip->ops->relinquish_locality)
+		chip->ops->relinquish_locality(chip, 0, false);
+
 	if (chip->dev.parent)
 		pm_runtime_put_sync(chip->dev.parent);
 
diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index 3245618..15b22a0 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -34,6 +34,15 @@ enum crb_defaults {
 	CRB_ACPI_START_INDEX = 1,
 };
 
+enum crb_loc_ctrl {
+	CRB_LOC_CTRL_REQUEST_ACCESS	= BIT(0),
+	CRB_LOC_CTRL_RELINQUISH		= BIT(1),
+};
+
+enum crb_loc_state {
+	CRB_LOC_STATE_LOC_ASSIGNED	= BIT(1),
+};
+
 enum crb_ctrl_req {
 	CRB_CTRL_REQ_CMD_READY	= BIT(0),
 	CRB_CTRL_REQ_GO_IDLE	= BIT(1),
@@ -172,6 +181,35 @@ static int __maybe_unused crb_cmd_ready(struct device *dev,
 	return 0;
 }
 
+static int crb_request_locality(struct tpm_chip *chip, int loc)
+{
+	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
+
+	if (!priv->regs_h)
+		return 0;
+
+	iowrite32(CRB_LOC_CTRL_REQUEST_ACCESS, &priv->regs_h->loc_ctrl);
+	if (!crb_wait_for_reg_32(&priv->regs_h->loc_state,
+				 CRB_LOC_STATE_LOC_ASSIGNED, /* mask */
+				 CRB_LOC_STATE_LOC_ASSIGNED, /* value */
+				 TPM2_TIMEOUT_C)) {
+		dev_warn(&chip->dev, "TPM_LOC_STATE_x.requestAccess timed out\n");
+		return -ETIME;
+	}
+
+	return 0;
+}
+
+static void crb_relinquish_locality(struct tpm_chip *chip, int loc, bool force)
+{
+	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
+
+	if (!priv->regs_h)
+		return;
+
+	iowrite32(CRB_LOC_CTRL_RELINQUISH, &priv->regs_h->loc_ctrl);
+}
+
 static u8 crb_status(struct tpm_chip *chip)
 {
 	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
@@ -198,7 +236,6 @@ static int crb_recv(struct tpm_chip *chip, u8 *buf, size_t count)
 
 	memcpy_fromio(buf, priv->rsp, 6);
 	expected = be32_to_cpup((__be32 *) &buf[2]);
-
 	if (expected > count)
 		return -EIO;
 
@@ -279,6 +316,8 @@ static const struct tpm_class_ops tpm_crb = {
 	.send = crb_send,
 	.cancel = crb_cancel,
 	.req_canceled = crb_req_canceled,
+	.request_locality = crb_request_locality,
+	.relinquish_locality = crb_relinquish_locality,
 	.req_complete_mask = CRB_DRV_STS_COMPLETE,
 	.req_complete_val = CRB_DRV_STS_COMPLETE,
 };
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index fc0e9a2..c3cbe24 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -73,7 +73,7 @@ static int check_locality(struct tpm_chip *chip, int l)
 	return -1;
 }
 
-static void release_locality(struct tpm_chip *chip, int l, int force)
+static void release_locality(struct tpm_chip *chip, int l, bool force)
 {
 	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
 	int rc;
@@ -97,7 +97,7 @@ static int request_locality(struct tpm_chip *chip, int l)
 	long rc;
 
 	if (check_locality(chip, l) >= 0)
-		return l;
+		return -EBUSY;
 
 	rc = tpm_tis_write8(priv, TPM_ACCESS(l), TPM_ACCESS_REQUEST_USE);
 	if (rc < 0)
@@ -252,7 +252,6 @@ static int tpm_tis_recv(struct tpm_chip *chip, u8 *buf, size_t count)
 
 out:
 	tpm_tis_ready(chip);
-	release_locality(chip, priv->locality, 0);
 	return size;
 }
 
@@ -268,9 +267,6 @@ static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t len)
 	size_t count = 0;
 	bool itpm = priv->flags & TPM_TIS_ITPM_WORKAROUND;
 
-	if (request_locality(chip, 0) < 0)
-		return -EBUSY;
-
 	status = tpm_tis_status(chip);
 	if ((status & TPM_STS_COMMAND_READY) == 0) {
 		tpm_tis_ready(chip);
@@ -329,7 +325,6 @@ static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t len)
 
 out_err:
 	tpm_tis_ready(chip);
-	release_locality(chip, priv->locality, 0);
 	return rc;
 }
 
@@ -390,7 +385,6 @@ static int tpm_tis_send_main(struct tpm_chip *chip, u8 *buf, size_t len)
 	return len;
 out_err:
 	tpm_tis_ready(chip);
-	release_locality(chip, priv->locality, 0);
 	return rc;
 }
 
@@ -681,6 +675,8 @@ static const struct tpm_class_ops tpm_tis = {
 	.send = tpm_tis_send,
 	.cancel = tpm_tis_ready,
 	.update_timeouts = tpm_tis_update_timeouts,
+	.request_locality = request_locality,
+	.relinquish_locality = release_locality,
 	.req_complete_mask = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
 	.req_complete_val = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
 	.req_canceled = tpm_tis_req_canceled,
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index da158f0..65e05f9 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -48,7 +48,8 @@ struct tpm_class_ops {
 	u8 (*status) (struct tpm_chip *chip);
 	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, bool force);
 };
 
 #if defined(CONFIG_TCG_TPM) || defined(CONFIG_TCG_TPM_MODULE)
-- 
2.9.3

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

* [PATCH v2] tpm_crb: request and relinquish locality 0
@ 2017-03-11 13:02 ` Jarkko Sakkinen
  0 siblings, 0 replies; 17+ messages in thread
From: Jarkko Sakkinen @ 2017-03-11 13:02 UTC (permalink / raw)
  To: linux-security-module

Added two new callbacks to struct tpm_class_ops:

- request_locality
- relinquish_locality

These are called before sending and receiving data from the TPM.  We
update also tpm_tis_core to use these callbacks. Small modification to
request_locality() is done so that it returns -EBUSY instead of locality
number when check_locality() fails.

Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 drivers/char/tpm/tpm-interface.c |  9 +++++++++
 drivers/char/tpm/tpm_crb.c       | 41 +++++++++++++++++++++++++++++++++++++++-
 drivers/char/tpm/tpm_tis_core.c  | 12 ++++--------
 include/linux/tpm.h              |  3 ++-
 4 files changed, 55 insertions(+), 10 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index e38c792..9c56581 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -407,6 +407,12 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
 	if (chip->dev.parent)
 		pm_runtime_get_sync(chip->dev.parent);
 
+	if (chip->ops->request_locality)  {
+		rc = chip->ops->request_locality(chip, 0);
+		if (rc)
+			goto out;
+	}
+
 	rc = tpm2_prepare_space(chip, space, ordinal, buf);
 	if (rc)
 		goto out;
@@ -466,6 +472,9 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
 	rc = tpm2_commit_space(chip, space, ordinal, buf, &len);
 
 out:
+	if (chip->ops->relinquish_locality)
+		chip->ops->relinquish_locality(chip, 0, false);
+
 	if (chip->dev.parent)
 		pm_runtime_put_sync(chip->dev.parent);
 
diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index 3245618..15b22a0 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -34,6 +34,15 @@ enum crb_defaults {
 	CRB_ACPI_START_INDEX = 1,
 };
 
+enum crb_loc_ctrl {
+	CRB_LOC_CTRL_REQUEST_ACCESS	= BIT(0),
+	CRB_LOC_CTRL_RELINQUISH		= BIT(1),
+};
+
+enum crb_loc_state {
+	CRB_LOC_STATE_LOC_ASSIGNED	= BIT(1),
+};
+
 enum crb_ctrl_req {
 	CRB_CTRL_REQ_CMD_READY	= BIT(0),
 	CRB_CTRL_REQ_GO_IDLE	= BIT(1),
@@ -172,6 +181,35 @@ static int __maybe_unused crb_cmd_ready(struct device *dev,
 	return 0;
 }
 
+static int crb_request_locality(struct tpm_chip *chip, int loc)
+{
+	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
+
+	if (!priv->regs_h)
+		return 0;
+
+	iowrite32(CRB_LOC_CTRL_REQUEST_ACCESS, &priv->regs_h->loc_ctrl);
+	if (!crb_wait_for_reg_32(&priv->regs_h->loc_state,
+				 CRB_LOC_STATE_LOC_ASSIGNED, /* mask */
+				 CRB_LOC_STATE_LOC_ASSIGNED, /* value */
+				 TPM2_TIMEOUT_C)) {
+		dev_warn(&chip->dev, "TPM_LOC_STATE_x.requestAccess timed out\n");
+		return -ETIME;
+	}
+
+	return 0;
+}
+
+static void crb_relinquish_locality(struct tpm_chip *chip, int loc, bool force)
+{
+	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
+
+	if (!priv->regs_h)
+		return;
+
+	iowrite32(CRB_LOC_CTRL_RELINQUISH, &priv->regs_h->loc_ctrl);
+}
+
 static u8 crb_status(struct tpm_chip *chip)
 {
 	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
@@ -198,7 +236,6 @@ static int crb_recv(struct tpm_chip *chip, u8 *buf, size_t count)
 
 	memcpy_fromio(buf, priv->rsp, 6);
 	expected = be32_to_cpup((__be32 *) &buf[2]);
-
 	if (expected > count)
 		return -EIO;
 
@@ -279,6 +316,8 @@ static const struct tpm_class_ops tpm_crb = {
 	.send = crb_send,
 	.cancel = crb_cancel,
 	.req_canceled = crb_req_canceled,
+	.request_locality = crb_request_locality,
+	.relinquish_locality = crb_relinquish_locality,
 	.req_complete_mask = CRB_DRV_STS_COMPLETE,
 	.req_complete_val = CRB_DRV_STS_COMPLETE,
 };
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index fc0e9a2..c3cbe24 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -73,7 +73,7 @@ static int check_locality(struct tpm_chip *chip, int l)
 	return -1;
 }
 
-static void release_locality(struct tpm_chip *chip, int l, int force)
+static void release_locality(struct tpm_chip *chip, int l, bool force)
 {
 	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
 	int rc;
@@ -97,7 +97,7 @@ static int request_locality(struct tpm_chip *chip, int l)
 	long rc;
 
 	if (check_locality(chip, l) >= 0)
-		return l;
+		return -EBUSY;
 
 	rc = tpm_tis_write8(priv, TPM_ACCESS(l), TPM_ACCESS_REQUEST_USE);
 	if (rc < 0)
@@ -252,7 +252,6 @@ static int tpm_tis_recv(struct tpm_chip *chip, u8 *buf, size_t count)
 
 out:
 	tpm_tis_ready(chip);
-	release_locality(chip, priv->locality, 0);
 	return size;
 }
 
@@ -268,9 +267,6 @@ static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t len)
 	size_t count = 0;
 	bool itpm = priv->flags & TPM_TIS_ITPM_WORKAROUND;
 
-	if (request_locality(chip, 0) < 0)
-		return -EBUSY;
-
 	status = tpm_tis_status(chip);
 	if ((status & TPM_STS_COMMAND_READY) == 0) {
 		tpm_tis_ready(chip);
@@ -329,7 +325,6 @@ static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t len)
 
 out_err:
 	tpm_tis_ready(chip);
-	release_locality(chip, priv->locality, 0);
 	return rc;
 }
 
@@ -390,7 +385,6 @@ static int tpm_tis_send_main(struct tpm_chip *chip, u8 *buf, size_t len)
 	return len;
 out_err:
 	tpm_tis_ready(chip);
-	release_locality(chip, priv->locality, 0);
 	return rc;
 }
 
@@ -681,6 +675,8 @@ static const struct tpm_class_ops tpm_tis = {
 	.send = tpm_tis_send,
 	.cancel = tpm_tis_ready,
 	.update_timeouts = tpm_tis_update_timeouts,
+	.request_locality = request_locality,
+	.relinquish_locality = release_locality,
 	.req_complete_mask = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
 	.req_complete_val = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
 	.req_canceled = tpm_tis_req_canceled,
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index da158f0..65e05f9 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -48,7 +48,8 @@ struct tpm_class_ops {
 	u8 (*status) (struct tpm_chip *chip);
 	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, bool force);
 };
 
 #if defined(CONFIG_TCG_TPM) || defined(CONFIG_TCG_TPM_MODULE)
-- 
2.9.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

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

* [PATCH v2] tpm_crb: request and relinquish locality 0
@ 2017-03-11 13:02 ` Jarkko Sakkinen
  0 siblings, 0 replies; 17+ messages in thread
From: Jarkko Sakkinen @ 2017-03-11 13:02 UTC (permalink / raw)
  To: tpmdd-devel
  Cc: linux-security-module, Jerry Snitselaar, gang.wei,
	Jarkko Sakkinen, Peter Huewe, Marcel Selhorst, Jason Gunthorpe,
	open list

Added two new callbacks to struct tpm_class_ops:

- request_locality
- relinquish_locality

These are called before sending and receiving data from the TPM.  We
update also tpm_tis_core to use these callbacks. Small modification to
request_locality() is done so that it returns -EBUSY instead of locality
number when check_locality() fails.

Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 drivers/char/tpm/tpm-interface.c |  9 +++++++++
 drivers/char/tpm/tpm_crb.c       | 41 +++++++++++++++++++++++++++++++++++++++-
 drivers/char/tpm/tpm_tis_core.c  | 12 ++++--------
 include/linux/tpm.h              |  3 ++-
 4 files changed, 55 insertions(+), 10 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index e38c792..9c56581 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -407,6 +407,12 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
 	if (chip->dev.parent)
 		pm_runtime_get_sync(chip->dev.parent);
 
+	if (chip->ops->request_locality)  {
+		rc = chip->ops->request_locality(chip, 0);
+		if (rc)
+			goto out;
+	}
+
 	rc = tpm2_prepare_space(chip, space, ordinal, buf);
 	if (rc)
 		goto out;
@@ -466,6 +472,9 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
 	rc = tpm2_commit_space(chip, space, ordinal, buf, &len);
 
 out:
+	if (chip->ops->relinquish_locality)
+		chip->ops->relinquish_locality(chip, 0, false);
+
 	if (chip->dev.parent)
 		pm_runtime_put_sync(chip->dev.parent);
 
diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index 3245618..15b22a0 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -34,6 +34,15 @@ enum crb_defaults {
 	CRB_ACPI_START_INDEX = 1,
 };
 
+enum crb_loc_ctrl {
+	CRB_LOC_CTRL_REQUEST_ACCESS	= BIT(0),
+	CRB_LOC_CTRL_RELINQUISH		= BIT(1),
+};
+
+enum crb_loc_state {
+	CRB_LOC_STATE_LOC_ASSIGNED	= BIT(1),
+};
+
 enum crb_ctrl_req {
 	CRB_CTRL_REQ_CMD_READY	= BIT(0),
 	CRB_CTRL_REQ_GO_IDLE	= BIT(1),
@@ -172,6 +181,35 @@ static int __maybe_unused crb_cmd_ready(struct device *dev,
 	return 0;
 }
 
+static int crb_request_locality(struct tpm_chip *chip, int loc)
+{
+	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
+
+	if (!priv->regs_h)
+		return 0;
+
+	iowrite32(CRB_LOC_CTRL_REQUEST_ACCESS, &priv->regs_h->loc_ctrl);
+	if (!crb_wait_for_reg_32(&priv->regs_h->loc_state,
+				 CRB_LOC_STATE_LOC_ASSIGNED, /* mask */
+				 CRB_LOC_STATE_LOC_ASSIGNED, /* value */
+				 TPM2_TIMEOUT_C)) {
+		dev_warn(&chip->dev, "TPM_LOC_STATE_x.requestAccess timed out\n");
+		return -ETIME;
+	}
+
+	return 0;
+}
+
+static void crb_relinquish_locality(struct tpm_chip *chip, int loc, bool force)
+{
+	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
+
+	if (!priv->regs_h)
+		return;
+
+	iowrite32(CRB_LOC_CTRL_RELINQUISH, &priv->regs_h->loc_ctrl);
+}
+
 static u8 crb_status(struct tpm_chip *chip)
 {
 	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
@@ -198,7 +236,6 @@ static int crb_recv(struct tpm_chip *chip, u8 *buf, size_t count)
 
 	memcpy_fromio(buf, priv->rsp, 6);
 	expected = be32_to_cpup((__be32 *) &buf[2]);
-
 	if (expected > count)
 		return -EIO;
 
@@ -279,6 +316,8 @@ static const struct tpm_class_ops tpm_crb = {
 	.send = crb_send,
 	.cancel = crb_cancel,
 	.req_canceled = crb_req_canceled,
+	.request_locality = crb_request_locality,
+	.relinquish_locality = crb_relinquish_locality,
 	.req_complete_mask = CRB_DRV_STS_COMPLETE,
 	.req_complete_val = CRB_DRV_STS_COMPLETE,
 };
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index fc0e9a2..c3cbe24 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -73,7 +73,7 @@ static int check_locality(struct tpm_chip *chip, int l)
 	return -1;
 }
 
-static void release_locality(struct tpm_chip *chip, int l, int force)
+static void release_locality(struct tpm_chip *chip, int l, bool force)
 {
 	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
 	int rc;
@@ -97,7 +97,7 @@ static int request_locality(struct tpm_chip *chip, int l)
 	long rc;
 
 	if (check_locality(chip, l) >= 0)
-		return l;
+		return -EBUSY;
 
 	rc = tpm_tis_write8(priv, TPM_ACCESS(l), TPM_ACCESS_REQUEST_USE);
 	if (rc < 0)
@@ -252,7 +252,6 @@ static int tpm_tis_recv(struct tpm_chip *chip, u8 *buf, size_t count)
 
 out:
 	tpm_tis_ready(chip);
-	release_locality(chip, priv->locality, 0);
 	return size;
 }
 
@@ -268,9 +267,6 @@ static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t len)
 	size_t count = 0;
 	bool itpm = priv->flags & TPM_TIS_ITPM_WORKAROUND;
 
-	if (request_locality(chip, 0) < 0)
-		return -EBUSY;
-
 	status = tpm_tis_status(chip);
 	if ((status & TPM_STS_COMMAND_READY) == 0) {
 		tpm_tis_ready(chip);
@@ -329,7 +325,6 @@ static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t len)
 
 out_err:
 	tpm_tis_ready(chip);
-	release_locality(chip, priv->locality, 0);
 	return rc;
 }
 
@@ -390,7 +385,6 @@ static int tpm_tis_send_main(struct tpm_chip *chip, u8 *buf, size_t len)
 	return len;
 out_err:
 	tpm_tis_ready(chip);
-	release_locality(chip, priv->locality, 0);
 	return rc;
 }
 
@@ -681,6 +675,8 @@ static const struct tpm_class_ops tpm_tis = {
 	.send = tpm_tis_send,
 	.cancel = tpm_tis_ready,
 	.update_timeouts = tpm_tis_update_timeouts,
+	.request_locality = request_locality,
+	.relinquish_locality = release_locality,
 	.req_complete_mask = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
 	.req_complete_val = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
 	.req_canceled = tpm_tis_req_canceled,
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index da158f0..65e05f9 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -48,7 +48,8 @@ struct tpm_class_ops {
 	u8 (*status) (struct tpm_chip *chip);
 	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, bool force);
 };
 
 #if defined(CONFIG_TCG_TPM) || defined(CONFIG_TCG_TPM_MODULE)
-- 
2.9.3


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

* Re: [PATCH v2] tpm_crb: request and relinquish locality 0
  2017-03-11 13:02 ` Jarkko Sakkinen
  (?)
@ 2017-03-12 19:47   ` Jerry Snitselaar
  -1 siblings, 0 replies; 17+ messages in thread
From: Jerry Snitselaar @ 2017-03-12 19:47 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: tpmdd-devel, linux-security-module, gang.wei, Peter Huewe,
	Marcel Selhorst, Jason Gunthorpe, open list


Jarkko Sakkinen @ 2017-03-11 13:02 GMT:

> Added two new callbacks to struct tpm_class_ops:
>
> - request_locality
> - relinquish_locality
>
> These are called before sending and receiving data from the TPM.  We
> update also tpm_tis_core to use these callbacks. Small modification to
> request_locality() is done so that it returns -EBUSY instead of locality
> number when check_locality() fails.
>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> ---
>  drivers/char/tpm/tpm-interface.c |  9 +++++++++
>  drivers/char/tpm/tpm_crb.c       | 41 +++++++++++++++++++++++++++++++++++++++-
>  drivers/char/tpm/tpm_tis_core.c  | 12 ++++--------
>  include/linux/tpm.h              |  3 ++-
>  4 files changed, 55 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index e38c792..9c56581 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -407,6 +407,12 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
>  	if (chip->dev.parent)
>  		pm_runtime_get_sync(chip->dev.parent);
>  
> +	if (chip->ops->request_locality)  {
> +		rc = chip->ops->request_locality(chip, 0);
> +		if (rc)
> +			goto out;
> +	}
> +
>  	rc = tpm2_prepare_space(chip, space, ordinal, buf);
>  	if (rc)
>  		goto out;
> @@ -466,6 +472,9 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
>  	rc = tpm2_commit_space(chip, space, ordinal, buf, &len);
>  
>  out:
> +	if (chip->ops->relinquish_locality)
> +		chip->ops->relinquish_locality(chip, 0, false);
> +
>  	if (chip->dev.parent)
>  		pm_runtime_put_sync(chip->dev.parent);
>  
> diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> index 3245618..15b22a0 100644
> --- a/drivers/char/tpm/tpm_crb.c
> +++ b/drivers/char/tpm/tpm_crb.c
> @@ -34,6 +34,15 @@ enum crb_defaults {
>  	CRB_ACPI_START_INDEX = 1,
>  };
>  
> +enum crb_loc_ctrl {
> +	CRB_LOC_CTRL_REQUEST_ACCESS	= BIT(0),
> +	CRB_LOC_CTRL_RELINQUISH		= BIT(1),
> +};
> +
> +enum crb_loc_state {
> +	CRB_LOC_STATE_LOC_ASSIGNED	= BIT(1),
> +};
> +
>  enum crb_ctrl_req {
>  	CRB_CTRL_REQ_CMD_READY	= BIT(0),
>  	CRB_CTRL_REQ_GO_IDLE	= BIT(1),
> @@ -172,6 +181,35 @@ static int __maybe_unused crb_cmd_ready(struct device *dev,
>  	return 0;
>  }
>  
> +static int crb_request_locality(struct tpm_chip *chip, int loc)
> +{
> +	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
> +
> +	if (!priv->regs_h)
> +		return 0;
> +
> +	iowrite32(CRB_LOC_CTRL_REQUEST_ACCESS, &priv->regs_h->loc_ctrl);
> +	if (!crb_wait_for_reg_32(&priv->regs_h->loc_state,
> +				 CRB_LOC_STATE_LOC_ASSIGNED, /* mask */
> +				 CRB_LOC_STATE_LOC_ASSIGNED, /* value */

Should this mask and check bit 7 as well (tpmRegValidSts)? The
table with the definition in the PTP spec says it indicates whether
all other bits contain valid values, but the text above it doesn't
discuss the locAssigned and activeLocality bits with respect to
tpmRegValidSts, so not completely clear.

> +				 TPM2_TIMEOUT_C)) {
> +		dev_warn(&chip->dev, "TPM_LOC_STATE_x.requestAccess timed out\n");
> +		return -ETIME;
> +	}
> +
> +	return 0;
> +}
> +
> +static void crb_relinquish_locality(struct tpm_chip *chip, int loc, bool force)
> +{
> +	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
> +
> +	if (!priv->regs_h)
> +		return;
> +
> +	iowrite32(CRB_LOC_CTRL_RELINQUISH, &priv->regs_h->loc_ctrl);
> +}
> +
>  static u8 crb_status(struct tpm_chip *chip)
>  {
>  	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
> @@ -198,7 +236,6 @@ static int crb_recv(struct tpm_chip *chip, u8 *buf, size_t count)
>  
>  	memcpy_fromio(buf, priv->rsp, 6);
>  	expected = be32_to_cpup((__be32 *) &buf[2]);
> -
>  	if (expected > count)
>  		return -EIO;
>  
> @@ -279,6 +316,8 @@ static const struct tpm_class_ops tpm_crb = {
>  	.send = crb_send,
>  	.cancel = crb_cancel,
>  	.req_canceled = crb_req_canceled,
> +	.request_locality = crb_request_locality,
> +	.relinquish_locality = crb_relinquish_locality,
>  	.req_complete_mask = CRB_DRV_STS_COMPLETE,
>  	.req_complete_val = CRB_DRV_STS_COMPLETE,
>  };
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index fc0e9a2..c3cbe24 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -73,7 +73,7 @@ static int check_locality(struct tpm_chip *chip, int l)
>  	return -1;
>  }
>  
> -static void release_locality(struct tpm_chip *chip, int l, int force)
> +static void release_locality(struct tpm_chip *chip, int l, bool force)
>  {
>  	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
>  	int rc;
> @@ -97,7 +97,7 @@ static int request_locality(struct tpm_chip *chip, int l)
>  	long rc;
>  
>  	if (check_locality(chip, l) >= 0)
> -		return l;
> +		return -EBUSY;
>  
>  	rc = tpm_tis_write8(priv, TPM_ACCESS(l), TPM_ACCESS_REQUEST_USE);
>  	if (rc < 0)
> @@ -252,7 +252,6 @@ static int tpm_tis_recv(struct tpm_chip *chip, u8 *buf, size_t count)
>  
>  out:
>  	tpm_tis_ready(chip);
> -	release_locality(chip, priv->locality, 0);
>  	return size;
>  }
>  
> @@ -268,9 +267,6 @@ static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t len)
>  	size_t count = 0;
>  	bool itpm = priv->flags & TPM_TIS_ITPM_WORKAROUND;
>  
> -	if (request_locality(chip, 0) < 0)
> -		return -EBUSY;
> -
>  	status = tpm_tis_status(chip);
>  	if ((status & TPM_STS_COMMAND_READY) == 0) {
>  		tpm_tis_ready(chip);
> @@ -329,7 +325,6 @@ static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t len)
>  
>  out_err:
>  	tpm_tis_ready(chip);
> -	release_locality(chip, priv->locality, 0);
>  	return rc;
>  }
>  
> @@ -390,7 +385,6 @@ static int tpm_tis_send_main(struct tpm_chip *chip, u8 *buf, size_t len)
>  	return len;
>  out_err:
>  	tpm_tis_ready(chip);
> -	release_locality(chip, priv->locality, 0);
>  	return rc;
>  }
>  
> @@ -681,6 +675,8 @@ static const struct tpm_class_ops tpm_tis = {
>  	.send = tpm_tis_send,
>  	.cancel = tpm_tis_ready,
>  	.update_timeouts = tpm_tis_update_timeouts,
> +	.request_locality = request_locality,
> +	.relinquish_locality = release_locality,
>  	.req_complete_mask = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
>  	.req_complete_val = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
>  	.req_canceled = tpm_tis_req_canceled,
> diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> index da158f0..65e05f9 100644
> --- a/include/linux/tpm.h
> +++ b/include/linux/tpm.h
> @@ -48,7 +48,8 @@ struct tpm_class_ops {
>  	u8 (*status) (struct tpm_chip *chip);
>  	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, bool force);
>  };
>  
>  #if defined(CONFIG_TCG_TPM) || defined(CONFIG_TCG_TPM_MODULE)

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

* [PATCH v2] tpm_crb: request and relinquish locality 0
@ 2017-03-12 19:47   ` Jerry Snitselaar
  0 siblings, 0 replies; 17+ messages in thread
From: Jerry Snitselaar @ 2017-03-12 19:47 UTC (permalink / raw)
  To: linux-security-module


Jarkko Sakkinen @ 2017-03-11 13:02 GMT:

> Added two new callbacks to struct tpm_class_ops:
>
> - request_locality
> - relinquish_locality
>
> These are called before sending and receiving data from the TPM.  We
> update also tpm_tis_core to use these callbacks. Small modification to
> request_locality() is done so that it returns -EBUSY instead of locality
> number when check_locality() fails.
>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> ---
>  drivers/char/tpm/tpm-interface.c |  9 +++++++++
>  drivers/char/tpm/tpm_crb.c       | 41 +++++++++++++++++++++++++++++++++++++++-
>  drivers/char/tpm/tpm_tis_core.c  | 12 ++++--------
>  include/linux/tpm.h              |  3 ++-
>  4 files changed, 55 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index e38c792..9c56581 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -407,6 +407,12 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
>  	if (chip->dev.parent)
>  		pm_runtime_get_sync(chip->dev.parent);
>  
> +	if (chip->ops->request_locality)  {
> +		rc = chip->ops->request_locality(chip, 0);
> +		if (rc)
> +			goto out;
> +	}
> +
>  	rc = tpm2_prepare_space(chip, space, ordinal, buf);
>  	if (rc)
>  		goto out;
> @@ -466,6 +472,9 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
>  	rc = tpm2_commit_space(chip, space, ordinal, buf, &len);
>  
>  out:
> +	if (chip->ops->relinquish_locality)
> +		chip->ops->relinquish_locality(chip, 0, false);
> +
>  	if (chip->dev.parent)
>  		pm_runtime_put_sync(chip->dev.parent);
>  
> diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> index 3245618..15b22a0 100644
> --- a/drivers/char/tpm/tpm_crb.c
> +++ b/drivers/char/tpm/tpm_crb.c
> @@ -34,6 +34,15 @@ enum crb_defaults {
>  	CRB_ACPI_START_INDEX = 1,
>  };
>  
> +enum crb_loc_ctrl {
> +	CRB_LOC_CTRL_REQUEST_ACCESS	= BIT(0),
> +	CRB_LOC_CTRL_RELINQUISH		= BIT(1),
> +};
> +
> +enum crb_loc_state {
> +	CRB_LOC_STATE_LOC_ASSIGNED	= BIT(1),
> +};
> +
>  enum crb_ctrl_req {
>  	CRB_CTRL_REQ_CMD_READY	= BIT(0),
>  	CRB_CTRL_REQ_GO_IDLE	= BIT(1),
> @@ -172,6 +181,35 @@ static int __maybe_unused crb_cmd_ready(struct device *dev,
>  	return 0;
>  }
>  
> +static int crb_request_locality(struct tpm_chip *chip, int loc)
> +{
> +	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
> +
> +	if (!priv->regs_h)
> +		return 0;
> +
> +	iowrite32(CRB_LOC_CTRL_REQUEST_ACCESS, &priv->regs_h->loc_ctrl);
> +	if (!crb_wait_for_reg_32(&priv->regs_h->loc_state,
> +				 CRB_LOC_STATE_LOC_ASSIGNED, /* mask */
> +				 CRB_LOC_STATE_LOC_ASSIGNED, /* value */

Should this mask and check bit 7 as well (tpmRegValidSts)? The
table with the definition in the PTP spec says it indicates whether
all other bits contain valid values, but the text above it doesn't
discuss the locAssigned and activeLocality bits with respect to
tpmRegValidSts, so not completely clear.

> +				 TPM2_TIMEOUT_C)) {
> +		dev_warn(&chip->dev, "TPM_LOC_STATE_x.requestAccess timed out\n");
> +		return -ETIME;
> +	}
> +
> +	return 0;
> +}
> +
> +static void crb_relinquish_locality(struct tpm_chip *chip, int loc, bool force)
> +{
> +	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
> +
> +	if (!priv->regs_h)
> +		return;
> +
> +	iowrite32(CRB_LOC_CTRL_RELINQUISH, &priv->regs_h->loc_ctrl);
> +}
> +
>  static u8 crb_status(struct tpm_chip *chip)
>  {
>  	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
> @@ -198,7 +236,6 @@ static int crb_recv(struct tpm_chip *chip, u8 *buf, size_t count)
>  
>  	memcpy_fromio(buf, priv->rsp, 6);
>  	expected = be32_to_cpup((__be32 *) &buf[2]);
> -
>  	if (expected > count)
>  		return -EIO;
>  
> @@ -279,6 +316,8 @@ static const struct tpm_class_ops tpm_crb = {
>  	.send = crb_send,
>  	.cancel = crb_cancel,
>  	.req_canceled = crb_req_canceled,
> +	.request_locality = crb_request_locality,
> +	.relinquish_locality = crb_relinquish_locality,
>  	.req_complete_mask = CRB_DRV_STS_COMPLETE,
>  	.req_complete_val = CRB_DRV_STS_COMPLETE,
>  };
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index fc0e9a2..c3cbe24 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -73,7 +73,7 @@ static int check_locality(struct tpm_chip *chip, int l)
>  	return -1;
>  }
>  
> -static void release_locality(struct tpm_chip *chip, int l, int force)
> +static void release_locality(struct tpm_chip *chip, int l, bool force)
>  {
>  	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
>  	int rc;
> @@ -97,7 +97,7 @@ static int request_locality(struct tpm_chip *chip, int l)
>  	long rc;
>  
>  	if (check_locality(chip, l) >= 0)
> -		return l;
> +		return -EBUSY;
>  
>  	rc = tpm_tis_write8(priv, TPM_ACCESS(l), TPM_ACCESS_REQUEST_USE);
>  	if (rc < 0)
> @@ -252,7 +252,6 @@ static int tpm_tis_recv(struct tpm_chip *chip, u8 *buf, size_t count)
>  
>  out:
>  	tpm_tis_ready(chip);
> -	release_locality(chip, priv->locality, 0);
>  	return size;
>  }
>  
> @@ -268,9 +267,6 @@ static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t len)
>  	size_t count = 0;
>  	bool itpm = priv->flags & TPM_TIS_ITPM_WORKAROUND;
>  
> -	if (request_locality(chip, 0) < 0)
> -		return -EBUSY;
> -
>  	status = tpm_tis_status(chip);
>  	if ((status & TPM_STS_COMMAND_READY) == 0) {
>  		tpm_tis_ready(chip);
> @@ -329,7 +325,6 @@ static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t len)
>  
>  out_err:
>  	tpm_tis_ready(chip);
> -	release_locality(chip, priv->locality, 0);
>  	return rc;
>  }
>  
> @@ -390,7 +385,6 @@ static int tpm_tis_send_main(struct tpm_chip *chip, u8 *buf, size_t len)
>  	return len;
>  out_err:
>  	tpm_tis_ready(chip);
> -	release_locality(chip, priv->locality, 0);
>  	return rc;
>  }
>  
> @@ -681,6 +675,8 @@ static const struct tpm_class_ops tpm_tis = {
>  	.send = tpm_tis_send,
>  	.cancel = tpm_tis_ready,
>  	.update_timeouts = tpm_tis_update_timeouts,
> +	.request_locality = request_locality,
> +	.relinquish_locality = release_locality,
>  	.req_complete_mask = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
>  	.req_complete_val = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
>  	.req_canceled = tpm_tis_req_canceled,
> diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> index da158f0..65e05f9 100644
> --- a/include/linux/tpm.h
> +++ b/include/linux/tpm.h
> @@ -48,7 +48,8 @@ struct tpm_class_ops {
>  	u8 (*status) (struct tpm_chip *chip);
>  	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, bool force);
>  };
>  
>  #if defined(CONFIG_TCG_TPM) || defined(CONFIG_TCG_TPM_MODULE)

--
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

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

* Re: [PATCH v2] tpm_crb: request and relinquish locality 0
@ 2017-03-12 19:47   ` Jerry Snitselaar
  0 siblings, 0 replies; 17+ messages in thread
From: Jerry Snitselaar @ 2017-03-12 19:47 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: open list, linux-security-module-u79uwXL29TY76Z2rM5mHXA,
	tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
	gang.wei-ral2JQCrhuEAvxtiuMwx3w


Jarkko Sakkinen @ 2017-03-11 13:02 GMT:

> Added two new callbacks to struct tpm_class_ops:
>
> - request_locality
> - relinquish_locality
>
> These are called before sending and receiving data from the TPM.  We
> update also tpm_tis_core to use these callbacks. Small modification to
> request_locality() is done so that it returns -EBUSY instead of locality
> number when check_locality() fails.
>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> ---
>  drivers/char/tpm/tpm-interface.c |  9 +++++++++
>  drivers/char/tpm/tpm_crb.c       | 41 +++++++++++++++++++++++++++++++++++++++-
>  drivers/char/tpm/tpm_tis_core.c  | 12 ++++--------
>  include/linux/tpm.h              |  3 ++-
>  4 files changed, 55 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index e38c792..9c56581 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -407,6 +407,12 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
>  	if (chip->dev.parent)
>  		pm_runtime_get_sync(chip->dev.parent);
>  
> +	if (chip->ops->request_locality)  {
> +		rc = chip->ops->request_locality(chip, 0);
> +		if (rc)
> +			goto out;
> +	}
> +
>  	rc = tpm2_prepare_space(chip, space, ordinal, buf);
>  	if (rc)
>  		goto out;
> @@ -466,6 +472,9 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
>  	rc = tpm2_commit_space(chip, space, ordinal, buf, &len);
>  
>  out:
> +	if (chip->ops->relinquish_locality)
> +		chip->ops->relinquish_locality(chip, 0, false);
> +
>  	if (chip->dev.parent)
>  		pm_runtime_put_sync(chip->dev.parent);
>  
> diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> index 3245618..15b22a0 100644
> --- a/drivers/char/tpm/tpm_crb.c
> +++ b/drivers/char/tpm/tpm_crb.c
> @@ -34,6 +34,15 @@ enum crb_defaults {
>  	CRB_ACPI_START_INDEX = 1,
>  };
>  
> +enum crb_loc_ctrl {
> +	CRB_LOC_CTRL_REQUEST_ACCESS	= BIT(0),
> +	CRB_LOC_CTRL_RELINQUISH		= BIT(1),
> +};
> +
> +enum crb_loc_state {
> +	CRB_LOC_STATE_LOC_ASSIGNED	= BIT(1),
> +};
> +
>  enum crb_ctrl_req {
>  	CRB_CTRL_REQ_CMD_READY	= BIT(0),
>  	CRB_CTRL_REQ_GO_IDLE	= BIT(1),
> @@ -172,6 +181,35 @@ static int __maybe_unused crb_cmd_ready(struct device *dev,
>  	return 0;
>  }
>  
> +static int crb_request_locality(struct tpm_chip *chip, int loc)
> +{
> +	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
> +
> +	if (!priv->regs_h)
> +		return 0;
> +
> +	iowrite32(CRB_LOC_CTRL_REQUEST_ACCESS, &priv->regs_h->loc_ctrl);
> +	if (!crb_wait_for_reg_32(&priv->regs_h->loc_state,
> +				 CRB_LOC_STATE_LOC_ASSIGNED, /* mask */
> +				 CRB_LOC_STATE_LOC_ASSIGNED, /* value */

Should this mask and check bit 7 as well (tpmRegValidSts)? The
table with the definition in the PTP spec says it indicates whether
all other bits contain valid values, but the text above it doesn't
discuss the locAssigned and activeLocality bits with respect to
tpmRegValidSts, so not completely clear.

> +				 TPM2_TIMEOUT_C)) {
> +		dev_warn(&chip->dev, "TPM_LOC_STATE_x.requestAccess timed out\n");
> +		return -ETIME;
> +	}
> +
> +	return 0;
> +}
> +
> +static void crb_relinquish_locality(struct tpm_chip *chip, int loc, bool force)
> +{
> +	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
> +
> +	if (!priv->regs_h)
> +		return;
> +
> +	iowrite32(CRB_LOC_CTRL_RELINQUISH, &priv->regs_h->loc_ctrl);
> +}
> +
>  static u8 crb_status(struct tpm_chip *chip)
>  {
>  	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
> @@ -198,7 +236,6 @@ static int crb_recv(struct tpm_chip *chip, u8 *buf, size_t count)
>  
>  	memcpy_fromio(buf, priv->rsp, 6);
>  	expected = be32_to_cpup((__be32 *) &buf[2]);
> -
>  	if (expected > count)
>  		return -EIO;
>  
> @@ -279,6 +316,8 @@ static const struct tpm_class_ops tpm_crb = {
>  	.send = crb_send,
>  	.cancel = crb_cancel,
>  	.req_canceled = crb_req_canceled,
> +	.request_locality = crb_request_locality,
> +	.relinquish_locality = crb_relinquish_locality,
>  	.req_complete_mask = CRB_DRV_STS_COMPLETE,
>  	.req_complete_val = CRB_DRV_STS_COMPLETE,
>  };
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index fc0e9a2..c3cbe24 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -73,7 +73,7 @@ static int check_locality(struct tpm_chip *chip, int l)
>  	return -1;
>  }
>  
> -static void release_locality(struct tpm_chip *chip, int l, int force)
> +static void release_locality(struct tpm_chip *chip, int l, bool force)
>  {
>  	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
>  	int rc;
> @@ -97,7 +97,7 @@ static int request_locality(struct tpm_chip *chip, int l)
>  	long rc;
>  
>  	if (check_locality(chip, l) >= 0)
> -		return l;
> +		return -EBUSY;
>  
>  	rc = tpm_tis_write8(priv, TPM_ACCESS(l), TPM_ACCESS_REQUEST_USE);
>  	if (rc < 0)
> @@ -252,7 +252,6 @@ static int tpm_tis_recv(struct tpm_chip *chip, u8 *buf, size_t count)
>  
>  out:
>  	tpm_tis_ready(chip);
> -	release_locality(chip, priv->locality, 0);
>  	return size;
>  }
>  
> @@ -268,9 +267,6 @@ static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t len)
>  	size_t count = 0;
>  	bool itpm = priv->flags & TPM_TIS_ITPM_WORKAROUND;
>  
> -	if (request_locality(chip, 0) < 0)
> -		return -EBUSY;
> -
>  	status = tpm_tis_status(chip);
>  	if ((status & TPM_STS_COMMAND_READY) == 0) {
>  		tpm_tis_ready(chip);
> @@ -329,7 +325,6 @@ static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t len)
>  
>  out_err:
>  	tpm_tis_ready(chip);
> -	release_locality(chip, priv->locality, 0);
>  	return rc;
>  }
>  
> @@ -390,7 +385,6 @@ static int tpm_tis_send_main(struct tpm_chip *chip, u8 *buf, size_t len)
>  	return len;
>  out_err:
>  	tpm_tis_ready(chip);
> -	release_locality(chip, priv->locality, 0);
>  	return rc;
>  }
>  
> @@ -681,6 +675,8 @@ static const struct tpm_class_ops tpm_tis = {
>  	.send = tpm_tis_send,
>  	.cancel = tpm_tis_ready,
>  	.update_timeouts = tpm_tis_update_timeouts,
> +	.request_locality = request_locality,
> +	.relinquish_locality = release_locality,
>  	.req_complete_mask = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
>  	.req_complete_val = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
>  	.req_canceled = tpm_tis_req_canceled,
> diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> index da158f0..65e05f9 100644
> --- a/include/linux/tpm.h
> +++ b/include/linux/tpm.h
> @@ -48,7 +48,8 @@ struct tpm_class_ops {
>  	u8 (*status) (struct tpm_chip *chip);
>  	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, bool force);
>  };
>  
>  #if defined(CONFIG_TCG_TPM) || defined(CONFIG_TCG_TPM_MODULE)


------------------------------------------------------------------------------
Announcing the Oxford Dictionaries API! The API offers world-renowned
dictionary content that is easy and intuitive to access. Sign up for an
account today to start using our lexical data to power your apps and
projects. Get started today and enter our developer competition.
http://sdm.link/oxford

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

* Re: [PATCH v2] tpm_crb: request and relinquish locality 0
  2017-03-12 19:47   ` Jerry Snitselaar
@ 2017-03-13 11:58     ` Jarkko Sakkinen
  -1 siblings, 0 replies; 17+ messages in thread
From: Jarkko Sakkinen @ 2017-03-13 11:58 UTC (permalink / raw)
  To: Jerry Snitselaar
  Cc: tpmdd-devel, linux-security-module, gang.wei, Peter Huewe,
	Marcel Selhorst, Jason Gunthorpe, open list

On Sun, Mar 12, 2017 at 12:47:58PM -0700, Jerry Snitselaar wrote:
> 
> Jarkko Sakkinen @ 2017-03-11 13:02 GMT:
> 
> > Added two new callbacks to struct tpm_class_ops:
> >
> > - request_locality
> > - relinquish_locality
> >
> > These are called before sending and receiving data from the TPM.  We
> > update also tpm_tis_core to use these callbacks. Small modification to
> > request_locality() is done so that it returns -EBUSY instead of locality
> > number when check_locality() fails.
> >
> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > ---
> >  drivers/char/tpm/tpm-interface.c |  9 +++++++++
> >  drivers/char/tpm/tpm_crb.c       | 41 +++++++++++++++++++++++++++++++++++++++-
> >  drivers/char/tpm/tpm_tis_core.c  | 12 ++++--------
> >  include/linux/tpm.h              |  3 ++-
> >  4 files changed, 55 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> > index e38c792..9c56581 100644
> > --- a/drivers/char/tpm/tpm-interface.c
> > +++ b/drivers/char/tpm/tpm-interface.c
> > @@ -407,6 +407,12 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
> >  	if (chip->dev.parent)
> >  		pm_runtime_get_sync(chip->dev.parent);
> >  
> > +	if (chip->ops->request_locality)  {
> > +		rc = chip->ops->request_locality(chip, 0);
> > +		if (rc)
> > +			goto out;
> > +	}
> > +
> >  	rc = tpm2_prepare_space(chip, space, ordinal, buf);
> >  	if (rc)
> >  		goto out;
> > @@ -466,6 +472,9 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
> >  	rc = tpm2_commit_space(chip, space, ordinal, buf, &len);
> >  
> >  out:
> > +	if (chip->ops->relinquish_locality)
> > +		chip->ops->relinquish_locality(chip, 0, false);
> > +
> >  	if (chip->dev.parent)
> >  		pm_runtime_put_sync(chip->dev.parent);
> >  
> > diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> > index 3245618..15b22a0 100644
> > --- a/drivers/char/tpm/tpm_crb.c
> > +++ b/drivers/char/tpm/tpm_crb.c
> > @@ -34,6 +34,15 @@ enum crb_defaults {
> >  	CRB_ACPI_START_INDEX = 1,
> >  };
> >  
> > +enum crb_loc_ctrl {
> > +	CRB_LOC_CTRL_REQUEST_ACCESS	= BIT(0),
> > +	CRB_LOC_CTRL_RELINQUISH		= BIT(1),
> > +};
> > +
> > +enum crb_loc_state {
> > +	CRB_LOC_STATE_LOC_ASSIGNED	= BIT(1),
> > +};
> > +
> >  enum crb_ctrl_req {
> >  	CRB_CTRL_REQ_CMD_READY	= BIT(0),
> >  	CRB_CTRL_REQ_GO_IDLE	= BIT(1),
> > @@ -172,6 +181,35 @@ static int __maybe_unused crb_cmd_ready(struct device *dev,
> >  	return 0;
> >  }
> >  
> > +static int crb_request_locality(struct tpm_chip *chip, int loc)
> > +{
> > +	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
> > +
> > +	if (!priv->regs_h)
> > +		return 0;
> > +
> > +	iowrite32(CRB_LOC_CTRL_REQUEST_ACCESS, &priv->regs_h->loc_ctrl);
> > +	if (!crb_wait_for_reg_32(&priv->regs_h->loc_state,
> > +				 CRB_LOC_STATE_LOC_ASSIGNED, /* mask */
> > +				 CRB_LOC_STATE_LOC_ASSIGNED, /* value */
> 
> Should this mask and check bit 7 as well (tpmRegValidSts)? The
> table with the definition in the PTP spec says it indicates whether
> all other bits contain valid values, but the text above it doesn't
> discuss the locAssigned and activeLocality bits with respect to
> tpmRegValidSts, so not completely clear.

You are probably right. There's also regression with the resource
manager (in this patch not in RM) that I'll fix. Thaks for reporting
this.

/Jarkko

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

* [PATCH v2] tpm_crb: request and relinquish locality 0
@ 2017-03-13 11:58     ` Jarkko Sakkinen
  0 siblings, 0 replies; 17+ messages in thread
From: Jarkko Sakkinen @ 2017-03-13 11:58 UTC (permalink / raw)
  To: linux-security-module

On Sun, Mar 12, 2017 at 12:47:58PM -0700, Jerry Snitselaar wrote:
> 
> Jarkko Sakkinen @ 2017-03-11 13:02 GMT:
> 
> > Added two new callbacks to struct tpm_class_ops:
> >
> > - request_locality
> > - relinquish_locality
> >
> > These are called before sending and receiving data from the TPM.  We
> > update also tpm_tis_core to use these callbacks. Small modification to
> > request_locality() is done so that it returns -EBUSY instead of locality
> > number when check_locality() fails.
> >
> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > ---
> >  drivers/char/tpm/tpm-interface.c |  9 +++++++++
> >  drivers/char/tpm/tpm_crb.c       | 41 +++++++++++++++++++++++++++++++++++++++-
> >  drivers/char/tpm/tpm_tis_core.c  | 12 ++++--------
> >  include/linux/tpm.h              |  3 ++-
> >  4 files changed, 55 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> > index e38c792..9c56581 100644
> > --- a/drivers/char/tpm/tpm-interface.c
> > +++ b/drivers/char/tpm/tpm-interface.c
> > @@ -407,6 +407,12 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
> >  	if (chip->dev.parent)
> >  		pm_runtime_get_sync(chip->dev.parent);
> >  
> > +	if (chip->ops->request_locality)  {
> > +		rc = chip->ops->request_locality(chip, 0);
> > +		if (rc)
> > +			goto out;
> > +	}
> > +
> >  	rc = tpm2_prepare_space(chip, space, ordinal, buf);
> >  	if (rc)
> >  		goto out;
> > @@ -466,6 +472,9 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
> >  	rc = tpm2_commit_space(chip, space, ordinal, buf, &len);
> >  
> >  out:
> > +	if (chip->ops->relinquish_locality)
> > +		chip->ops->relinquish_locality(chip, 0, false);
> > +
> >  	if (chip->dev.parent)
> >  		pm_runtime_put_sync(chip->dev.parent);
> >  
> > diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> > index 3245618..15b22a0 100644
> > --- a/drivers/char/tpm/tpm_crb.c
> > +++ b/drivers/char/tpm/tpm_crb.c
> > @@ -34,6 +34,15 @@ enum crb_defaults {
> >  	CRB_ACPI_START_INDEX = 1,
> >  };
> >  
> > +enum crb_loc_ctrl {
> > +	CRB_LOC_CTRL_REQUEST_ACCESS	= BIT(0),
> > +	CRB_LOC_CTRL_RELINQUISH		= BIT(1),
> > +};
> > +
> > +enum crb_loc_state {
> > +	CRB_LOC_STATE_LOC_ASSIGNED	= BIT(1),
> > +};
> > +
> >  enum crb_ctrl_req {
> >  	CRB_CTRL_REQ_CMD_READY	= BIT(0),
> >  	CRB_CTRL_REQ_GO_IDLE	= BIT(1),
> > @@ -172,6 +181,35 @@ static int __maybe_unused crb_cmd_ready(struct device *dev,
> >  	return 0;
> >  }
> >  
> > +static int crb_request_locality(struct tpm_chip *chip, int loc)
> > +{
> > +	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
> > +
> > +	if (!priv->regs_h)
> > +		return 0;
> > +
> > +	iowrite32(CRB_LOC_CTRL_REQUEST_ACCESS, &priv->regs_h->loc_ctrl);
> > +	if (!crb_wait_for_reg_32(&priv->regs_h->loc_state,
> > +				 CRB_LOC_STATE_LOC_ASSIGNED, /* mask */
> > +				 CRB_LOC_STATE_LOC_ASSIGNED, /* value */
> 
> Should this mask and check bit 7 as well (tpmRegValidSts)? The
> table with the definition in the PTP spec says it indicates whether
> all other bits contain valid values, but the text above it doesn't
> discuss the locAssigned and activeLocality bits with respect to
> tpmRegValidSts, so not completely clear.

You are probably right. There's also regression with the resource
manager (in this patch not in RM) that I'll fix. Thaks for reporting
this.

/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

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

* Re: [PATCH v2] tpm_crb: request and relinquish locality 0
  2017-03-11 13:02 ` Jarkko Sakkinen
  (?)
@ 2017-03-13 16:34   ` Jason Gunthorpe
  -1 siblings, 0 replies; 17+ messages in thread
From: Jason Gunthorpe @ 2017-03-13 16:34 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: tpmdd-devel, linux-security-module, Jerry Snitselaar, gang.wei,
	Peter Huewe, Marcel Selhorst, open list

On Sat, Mar 11, 2017 at 03:02:14PM +0200, Jarkko Sakkinen wrote:
> Added two new callbacks to struct tpm_class_ops:
> 
> - request_locality
> - relinquish_locality
> 
> These are called before sending and receiving data from the TPM.  We
> update also tpm_tis_core to use these callbacks. Small modification to
> request_locality() is done so that it returns -EBUSY instead of locality
> number when check_locality() fails.

Make sense

I think you may as well do the other two drivers, even though you
can't run them the transformation looks safe enough to me.

> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
>  drivers/char/tpm/tpm-interface.c |  9 +++++++++
>  drivers/char/tpm/tpm_crb.c       | 41 +++++++++++++++++++++++++++++++++++++++-
>  drivers/char/tpm/tpm_tis_core.c  | 12 ++++--------
>  include/linux/tpm.h              |  3 ++-
>  4 files changed, 55 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index e38c792..9c56581 100644
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -407,6 +407,12 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
>  	if (chip->dev.parent)
>  		pm_runtime_get_sync(chip->dev.parent);
>  
> +	if (chip->ops->request_locality)  {
> +		rc = chip->ops->request_locality(chip, 0);
> +		if (rc)
> +			goto out;

If request_locality fails we probably shouldn't call
relinquish_locality on the unwind path..

I think you should also put a relinquish_locality inside tpm_remove ?

> +	int (*request_locality)(struct tpm_chip *chip, int loc);
> +	void (*relinquish_locality)(struct tpm_chip *chip, int loc,
> bool force);

Let us document what force is supposed to do...

I'm not sure why we have it?

Jason

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

* [PATCH v2] tpm_crb: request and relinquish locality 0
@ 2017-03-13 16:34   ` Jason Gunthorpe
  0 siblings, 0 replies; 17+ messages in thread
From: Jason Gunthorpe @ 2017-03-13 16:34 UTC (permalink / raw)
  To: linux-security-module

On Sat, Mar 11, 2017 at 03:02:14PM +0200, Jarkko Sakkinen wrote:
> Added two new callbacks to struct tpm_class_ops:
> 
> - request_locality
> - relinquish_locality
> 
> These are called before sending and receiving data from the TPM.  We
> update also tpm_tis_core to use these callbacks. Small modification to
> request_locality() is done so that it returns -EBUSY instead of locality
> number when check_locality() fails.

Make sense

I think you may as well do the other two drivers, even though you
can't run them the transformation looks safe enough to me.

> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
>  drivers/char/tpm/tpm-interface.c |  9 +++++++++
>  drivers/char/tpm/tpm_crb.c       | 41 +++++++++++++++++++++++++++++++++++++++-
>  drivers/char/tpm/tpm_tis_core.c  | 12 ++++--------
>  include/linux/tpm.h              |  3 ++-
>  4 files changed, 55 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index e38c792..9c56581 100644
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -407,6 +407,12 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
>  	if (chip->dev.parent)
>  		pm_runtime_get_sync(chip->dev.parent);
>  
> +	if (chip->ops->request_locality)  {
> +		rc = chip->ops->request_locality(chip, 0);
> +		if (rc)
> +			goto out;

If request_locality fails we probably shouldn't call
relinquish_locality on the unwind path..

I think you should also put a relinquish_locality inside tpm_remove ?

> +	int (*request_locality)(struct tpm_chip *chip, int loc);
> +	void (*relinquish_locality)(struct tpm_chip *chip, int loc,
> bool force);

Let us document what force is supposed to do...

I'm not sure why we have it?

Jason
--
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

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

* Re: [PATCH v2] tpm_crb: request and relinquish locality 0
@ 2017-03-13 16:34   ` Jason Gunthorpe
  0 siblings, 0 replies; 17+ messages in thread
From: Jason Gunthorpe @ 2017-03-13 16:34 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: Jerry Snitselaar, open list,
	linux-security-module-u79uwXL29TY76Z2rM5mHXA,
	tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
	gang.wei-ral2JQCrhuEAvxtiuMwx3w

On Sat, Mar 11, 2017 at 03:02:14PM +0200, Jarkko Sakkinen wrote:
> Added two new callbacks to struct tpm_class_ops:
> 
> - request_locality
> - relinquish_locality
> 
> These are called before sending and receiving data from the TPM.  We
> update also tpm_tis_core to use these callbacks. Small modification to
> request_locality() is done so that it returns -EBUSY instead of locality
> number when check_locality() fails.

Make sense

I think you may as well do the other two drivers, even though you
can't run them the transformation looks safe enough to me.

> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
>  drivers/char/tpm/tpm-interface.c |  9 +++++++++
>  drivers/char/tpm/tpm_crb.c       | 41 +++++++++++++++++++++++++++++++++++++++-
>  drivers/char/tpm/tpm_tis_core.c  | 12 ++++--------
>  include/linux/tpm.h              |  3 ++-
>  4 files changed, 55 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index e38c792..9c56581 100644
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -407,6 +407,12 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
>  	if (chip->dev.parent)
>  		pm_runtime_get_sync(chip->dev.parent);
>  
> +	if (chip->ops->request_locality)  {
> +		rc = chip->ops->request_locality(chip, 0);
> +		if (rc)
> +			goto out;

If request_locality fails we probably shouldn't call
relinquish_locality on the unwind path..

I think you should also put a relinquish_locality inside tpm_remove ?

> +	int (*request_locality)(struct tpm_chip *chip, int loc);
> +	void (*relinquish_locality)(struct tpm_chip *chip, int loc,
> bool force);

Let us document what force is supposed to do...

I'm not sure why we have it?

Jason

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

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

* Re: [PATCH v2] tpm_crb: request and relinquish locality 0
  2017-03-13 16:34   ` Jason Gunthorpe
  (?)
@ 2017-03-13 20:12     ` Jarkko Sakkinen
  -1 siblings, 0 replies; 17+ messages in thread
From: Jarkko Sakkinen @ 2017-03-13 20:12 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: tpmdd-devel, linux-security-module, Jerry Snitselaar, gang.wei,
	Peter Huewe, Marcel Selhorst, open list

On Mon, Mar 13, 2017 at 10:34:52AM -0600, Jason Gunthorpe wrote:
> On Sat, Mar 11, 2017 at 03:02:14PM +0200, Jarkko Sakkinen wrote:
> > Added two new callbacks to struct tpm_class_ops:
> > 
> > - request_locality
> > - relinquish_locality
> > 
> > These are called before sending and receiving data from the TPM.  We
> > update also tpm_tis_core to use these callbacks. Small modification to
> > request_locality() is done so that it returns -EBUSY instead of locality
> > number when check_locality() fails.
> 
> Make sense
> 
> I think you may as well do the other two drivers, even though you
> can't run them the transformation looks safe enough to me.
> 
> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> >  drivers/char/tpm/tpm-interface.c |  9 +++++++++
> >  drivers/char/tpm/tpm_crb.c       | 41 +++++++++++++++++++++++++++++++++++++++-
> >  drivers/char/tpm/tpm_tis_core.c  | 12 ++++--------
> >  include/linux/tpm.h              |  3 ++-
> >  4 files changed, 55 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> > index e38c792..9c56581 100644
> > +++ b/drivers/char/tpm/tpm-interface.c
> > @@ -407,6 +407,12 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
> >  	if (chip->dev.parent)
> >  		pm_runtime_get_sync(chip->dev.parent);
> >  
> > +	if (chip->ops->request_locality)  {
> > +		rc = chip->ops->request_locality(chip, 0);
> > +		if (rc)
> > +			goto out;
> 
> If request_locality fails we probably shouldn't call
> relinquish_locality on the unwind path..
> 
> I think you should also put a relinquish_locality inside tpm_remove ?

Right. I was wondering why release_locality is called inside
tpm_tis_remove().

So is the idea of checking pendingRequest such that the release
part is "lazy" and not like what I'm doing in tpm_crb (always
relinquish).

Is that done for performance reasons? Should I do the same (pr
similar in tpm_crb?

> > +	int (*request_locality)(struct tpm_chip *chip, int loc);
> > +	void (*relinquish_locality)(struct tpm_chip *chip, int loc,
> > bool force);
> 
> Let us document what force is supposed to do...
> 
> I'm not sure why we have it?
> 
> Jason

I guess since it is lazy in tpm_tis_core the force is done in
tpm_tis_remove so that you always relinquish the locality even
if someone is not requesting it, right?

Where should this be documented, to the header?

/Jarkko

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

* [PATCH v2] tpm_crb: request and relinquish locality 0
@ 2017-03-13 20:12     ` Jarkko Sakkinen
  0 siblings, 0 replies; 17+ messages in thread
From: Jarkko Sakkinen @ 2017-03-13 20:12 UTC (permalink / raw)
  To: linux-security-module

On Mon, Mar 13, 2017 at 10:34:52AM -0600, Jason Gunthorpe wrote:
> On Sat, Mar 11, 2017 at 03:02:14PM +0200, Jarkko Sakkinen wrote:
> > Added two new callbacks to struct tpm_class_ops:
> > 
> > - request_locality
> > - relinquish_locality
> > 
> > These are called before sending and receiving data from the TPM.  We
> > update also tpm_tis_core to use these callbacks. Small modification to
> > request_locality() is done so that it returns -EBUSY instead of locality
> > number when check_locality() fails.
> 
> Make sense
> 
> I think you may as well do the other two drivers, even though you
> can't run them the transformation looks safe enough to me.
> 
> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> >  drivers/char/tpm/tpm-interface.c |  9 +++++++++
> >  drivers/char/tpm/tpm_crb.c       | 41 +++++++++++++++++++++++++++++++++++++++-
> >  drivers/char/tpm/tpm_tis_core.c  | 12 ++++--------
> >  include/linux/tpm.h              |  3 ++-
> >  4 files changed, 55 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> > index e38c792..9c56581 100644
> > +++ b/drivers/char/tpm/tpm-interface.c
> > @@ -407,6 +407,12 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
> >  	if (chip->dev.parent)
> >  		pm_runtime_get_sync(chip->dev.parent);
> >  
> > +	if (chip->ops->request_locality)  {
> > +		rc = chip->ops->request_locality(chip, 0);
> > +		if (rc)
> > +			goto out;
> 
> If request_locality fails we probably shouldn't call
> relinquish_locality on the unwind path..
> 
> I think you should also put a relinquish_locality inside tpm_remove ?

Right. I was wondering why release_locality is called inside
tpm_tis_remove().

So is the idea of checking pendingRequest such that the release
part is "lazy" and not like what I'm doing in tpm_crb (always
relinquish).

Is that done for performance reasons? Should I do the same (pr
similar in tpm_crb?

> > +	int (*request_locality)(struct tpm_chip *chip, int loc);
> > +	void (*relinquish_locality)(struct tpm_chip *chip, int loc,
> > bool force);
> 
> Let us document what force is supposed to do...
> 
> I'm not sure why we have it?
> 
> Jason

I guess since it is lazy in tpm_tis_core the force is done in
tpm_tis_remove so that you always relinquish the locality even
if someone is not requesting it, right?

Where should this be documented, to the header?

/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

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

* Re: [PATCH v2] tpm_crb: request and relinquish locality 0
@ 2017-03-13 20:12     ` Jarkko Sakkinen
  0 siblings, 0 replies; 17+ messages in thread
From: Jarkko Sakkinen @ 2017-03-13 20:12 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: Jerry Snitselaar, open list,
	linux-security-module-u79uwXL29TY76Z2rM5mHXA,
	tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
	gang.wei-ral2JQCrhuEAvxtiuMwx3w

On Mon, Mar 13, 2017 at 10:34:52AM -0600, Jason Gunthorpe wrote:
> On Sat, Mar 11, 2017 at 03:02:14PM +0200, Jarkko Sakkinen wrote:
> > Added two new callbacks to struct tpm_class_ops:
> > 
> > - request_locality
> > - relinquish_locality
> > 
> > These are called before sending and receiving data from the TPM.  We
> > update also tpm_tis_core to use these callbacks. Small modification to
> > request_locality() is done so that it returns -EBUSY instead of locality
> > number when check_locality() fails.
> 
> Make sense
> 
> I think you may as well do the other two drivers, even though you
> can't run them the transformation looks safe enough to me.
> 
> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> >  drivers/char/tpm/tpm-interface.c |  9 +++++++++
> >  drivers/char/tpm/tpm_crb.c       | 41 +++++++++++++++++++++++++++++++++++++++-
> >  drivers/char/tpm/tpm_tis_core.c  | 12 ++++--------
> >  include/linux/tpm.h              |  3 ++-
> >  4 files changed, 55 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> > index e38c792..9c56581 100644
> > +++ b/drivers/char/tpm/tpm-interface.c
> > @@ -407,6 +407,12 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
> >  	if (chip->dev.parent)
> >  		pm_runtime_get_sync(chip->dev.parent);
> >  
> > +	if (chip->ops->request_locality)  {
> > +		rc = chip->ops->request_locality(chip, 0);
> > +		if (rc)
> > +			goto out;
> 
> If request_locality fails we probably shouldn't call
> relinquish_locality on the unwind path..
> 
> I think you should also put a relinquish_locality inside tpm_remove ?

Right. I was wondering why release_locality is called inside
tpm_tis_remove().

So is the idea of checking pendingRequest such that the release
part is "lazy" and not like what I'm doing in tpm_crb (always
relinquish).

Is that done for performance reasons? Should I do the same (pr
similar in tpm_crb?

> > +	int (*request_locality)(struct tpm_chip *chip, int loc);
> > +	void (*relinquish_locality)(struct tpm_chip *chip, int loc,
> > bool force);
> 
> Let us document what force is supposed to do...
> 
> I'm not sure why we have it?
> 
> Jason

I guess since it is lazy in tpm_tis_core the force is done in
tpm_tis_remove so that you always relinquish the locality even
if someone is not requesting it, right?

Where should this be documented, to the header?

/Jarkko

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

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

* Re: [PATCH v2] tpm_crb: request and relinquish locality 0
  2017-03-13 20:12     ` Jarkko Sakkinen
  (?)
@ 2017-03-13 20:43       ` Jason Gunthorpe
  -1 siblings, 0 replies; 17+ messages in thread
From: Jason Gunthorpe @ 2017-03-13 20:43 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: tpmdd-devel, linux-security-module, Jerry Snitselaar, gang.wei,
	Peter Huewe, Marcel Selhorst, open list

On Mon, Mar 13, 2017 at 10:12:35PM +0200, Jarkko Sakkinen wrote:
> > I think you should also put a relinquish_locality inside tpm_remove ?
> 
> Right. I was wondering why release_locality is called inside
> tpm_tis_remove().

I also wonder that.. It sort of makes send to idle the TPM, but it is
kinda goofy to have paired function calls that are not ultimately
paried.

> So is the idea of checking pendingRequest such that the release
> part is "lazy" and not like what I'm doing in tpm_crb (always
> relinquish).
>
> Is that done for performance reasons? Should I do the same (pr
> similar in tpm_crb?

No idea, sorry. This stuff is so old and locality has never been an
exposed API by the kernel, AFAIK. Wouldn't surprise me at all if it
doesn't work right.

But.. If you don't need 'force' for CRB, I suggest that we drop the
'force' from the public API. TIS can do its special !force behavior in
its own remove as it does today.

However.. if we want to do some kind of locality caching for
performance then I think the core code should do it, not the
individual drivers.

Why do we need to reliquish the locality on every command anyhow? Does
the firmware interact with this?

Why are you adding locality to crb without a user anyhow?

Jason

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

* [PATCH v2] tpm_crb: request and relinquish locality 0
@ 2017-03-13 20:43       ` Jason Gunthorpe
  0 siblings, 0 replies; 17+ messages in thread
From: Jason Gunthorpe @ 2017-03-13 20:43 UTC (permalink / raw)
  To: linux-security-module

On Mon, Mar 13, 2017 at 10:12:35PM +0200, Jarkko Sakkinen wrote:
> > I think you should also put a relinquish_locality inside tpm_remove ?
> 
> Right. I was wondering why release_locality is called inside
> tpm_tis_remove().

I also wonder that.. It sort of makes send to idle the TPM, but it is
kinda goofy to have paired function calls that are not ultimately
paried.

> So is the idea of checking pendingRequest such that the release
> part is "lazy" and not like what I'm doing in tpm_crb (always
> relinquish).
>
> Is that done for performance reasons? Should I do the same (pr
> similar in tpm_crb?

No idea, sorry. This stuff is so old and locality has never been an
exposed API by the kernel, AFAIK. Wouldn't surprise me at all if it
doesn't work right.

But.. If you don't need 'force' for CRB, I suggest that we drop the
'force' from the public API. TIS can do its special !force behavior in
its own remove as it does today.

However.. if we want to do some kind of locality caching for
performance then I think the core code should do it, not the
individual drivers.

Why do we need to reliquish the locality on every command anyhow? Does
the firmware interact with this?

Why are you adding locality to crb without a user anyhow?

Jason
--
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

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

* Re: [PATCH v2] tpm_crb: request and relinquish locality 0
@ 2017-03-13 20:43       ` Jason Gunthorpe
  0 siblings, 0 replies; 17+ messages in thread
From: Jason Gunthorpe @ 2017-03-13 20:43 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: Jerry Snitselaar, open list,
	linux-security-module-u79uwXL29TY76Z2rM5mHXA,
	tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
	gang.wei-ral2JQCrhuEAvxtiuMwx3w

On Mon, Mar 13, 2017 at 10:12:35PM +0200, Jarkko Sakkinen wrote:
> > I think you should also put a relinquish_locality inside tpm_remove ?
> 
> Right. I was wondering why release_locality is called inside
> tpm_tis_remove().

I also wonder that.. It sort of makes send to idle the TPM, but it is
kinda goofy to have paired function calls that are not ultimately
paried.

> So is the idea of checking pendingRequest such that the release
> part is "lazy" and not like what I'm doing in tpm_crb (always
> relinquish).
>
> Is that done for performance reasons? Should I do the same (pr
> similar in tpm_crb?

No idea, sorry. This stuff is so old and locality has never been an
exposed API by the kernel, AFAIK. Wouldn't surprise me at all if it
doesn't work right.

But.. If you don't need 'force' for CRB, I suggest that we drop the
'force' from the public API. TIS can do its special !force behavior in
its own remove as it does today.

However.. if we want to do some kind of locality caching for
performance then I think the core code should do it, not the
individual drivers.

Why do we need to reliquish the locality on every command anyhow? Does
the firmware interact with this?

Why are you adding locality to crb without a user anyhow?

Jason

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

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

end of thread, other threads:[~2017-03-13 20:43 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-11 13:02 [PATCH v2] tpm_crb: request and relinquish locality 0 Jarkko Sakkinen
2017-03-11 13:02 ` Jarkko Sakkinen
2017-03-11 13:02 ` Jarkko Sakkinen
2017-03-12 19:47 ` Jerry Snitselaar
2017-03-12 19:47   ` Jerry Snitselaar
2017-03-12 19:47   ` Jerry Snitselaar
2017-03-13 11:58   ` Jarkko Sakkinen
2017-03-13 11:58     ` Jarkko Sakkinen
2017-03-13 16:34 ` Jason Gunthorpe
2017-03-13 16:34   ` Jason Gunthorpe
2017-03-13 16:34   ` Jason Gunthorpe
2017-03-13 20:12   ` Jarkko Sakkinen
2017-03-13 20:12     ` Jarkko Sakkinen
2017-03-13 20:12     ` Jarkko Sakkinen
2017-03-13 20:43     ` Jason Gunthorpe
2017-03-13 20:43       ` Jason Gunthorpe
2017-03-13 20:43       ` Jason Gunthorpe

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.