[PATCH 0/3] char: tpm: Adjust cr50_i2c locking mechanism

[PATCH 0/3] char: tpm: Adjust cr50_i2c locking mechanism

[Jan Dabros]

This patchset aims to add support for a platforms with cr50(tpm) i2c
chip shared across two CPUs. We need to provide a mechanism, which will
allow to synchronize accesses on a TPM-transaction boundaries.

First commit in a patchset is a generic fix for an issue encountered
during suspend stress test.

Next two patches are modifying cr50 to use generic callbacks for
locality management and then leverage this solution to apply i2c bus
locking on a TPM-operation level.

Jan Dabros (3):
  char: tpm: Protect tpm_pm_suspend with locks
  char: tpm: cr50: Use generic request/relinquish locality ops
  char: tpm: cr50: Move i2c locking to request/relinquish locality ops

 drivers/char/tpm/tpm-interface.c    |   5 +-
 drivers/char/tpm/tpm_tis_i2c_cr50.c | 118 +++++++++++++++++-----------
 2 files changed, 75 insertions(+), 48 deletions(-)

-- 
2.38.1.273.g43a17bfeac-goog

[PATCH 1/3] char: tpm: Protect tpm_pm_suspend with locks

[Jan Dabros]

Currently tpm transactions are executed unconditionally in
tpm_pm_suspend() function, what may lead to races with other tpm
accessors in the system.

Add proper locking mechanisms by calling tpm_try_get_ops() which is a
wrapper on tpm_chip_start().

Signed-off-by: Jan Dabros <jsd@semihalf.com>
---
 drivers/char/tpm/tpm-interface.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 1621ce8187052..d69905233aff2 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -401,13 +401,14 @@ int tpm_pm_suspend(struct device *dev)
 	    !pm_suspend_via_firmware())
 		goto suspended;
 
-	if (!tpm_chip_start(chip)) {
+	rc = tpm_try_get_ops(chip);
+	if (!rc) {
 		if (chip->flags & TPM_CHIP_FLAG_TPM2)
 			tpm2_shutdown(chip, TPM2_SU_STATE);
 		else
 			rc = tpm1_pm_suspend(chip, tpm_suspend_pcr);
 
-		tpm_chip_stop(chip);
+		tpm_put_ops(chip);
 	}
 
 suspended:
-- 
2.38.1.273.g43a17bfeac-goog

[PATCH 2/3] char: tpm: cr50: Use generic request/relinquish locality ops

[Jan Dabros]

Instead of using static functions tpm_cr50_request_locality and
tpm_cr50_release_locality register callbacks from tpm class chip->ops
created for this purpose.

Signed-off-by: Jan Dabros <jsd@semihalf.com>
---
 drivers/char/tpm/tpm_tis_i2c_cr50.c | 98 ++++++++++++++++++-----------
 1 file changed, 60 insertions(+), 38 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis_i2c_cr50.c b/drivers/char/tpm/tpm_tis_i2c_cr50.c
index 77cea5b31c6e4..d3438a4ed1ef8 100644
--- a/drivers/char/tpm/tpm_tis_i2c_cr50.c
+++ b/drivers/char/tpm/tpm_tis_i2c_cr50.c
@@ -17,6 +17,7 @@
  */
 
 #include <linux/acpi.h>
+#include <linux/bug.h>
 #include <linux/completion.h>
 #include <linux/i2c.h>
 #include <linux/interrupt.h>
@@ -35,6 +36,7 @@
 #define TPM_CR50_I2C_MAX_RETRIES	3		/* Max retries due to I2C errors */
 #define TPM_CR50_I2C_RETRY_DELAY_LO	55		/* Min usecs between retries on I2C */
 #define TPM_CR50_I2C_RETRY_DELAY_HI	65		/* Max usecs between retries on I2C */
+#define TPM_CR50_I2C_DEFAULT_LOC	0
 
 #define TPM_I2C_ACCESS(l)	(0x0000 | ((l) << 4))
 #define TPM_I2C_STS(l)		(0x0001 | ((l) << 4))
@@ -286,25 +288,26 @@ static int tpm_cr50_i2c_write(struct tpm_chip *chip, u8 addr, u8 *buffer,
 }
 
 /**
- * tpm_cr50_check_locality() - Verify TPM locality 0 is active.
+ * tpm_cr50_check_locality() - Verify if required TPM locality is active.
  * @chip: A TPM chip.
+ * @loc: Locality to be verified
  *
  * Return:
- * - 0:		Success.
+ * - loc:	Success.
  * - -errno:	A POSIX error code.
  */
-static int tpm_cr50_check_locality(struct tpm_chip *chip)
+static int tpm_cr50_check_locality(struct tpm_chip *chip, int loc)
 {
 	u8 mask = TPM_ACCESS_VALID | TPM_ACCESS_ACTIVE_LOCALITY;
 	u8 buf;
 	int rc;
 
-	rc = tpm_cr50_i2c_read(chip, TPM_I2C_ACCESS(0), &buf, sizeof(buf));
+	rc = tpm_cr50_i2c_read(chip, TPM_I2C_ACCESS(loc), &buf, sizeof(buf));
 	if (rc < 0)
 		return rc;
 
 	if ((buf & mask) == mask)
-		return 0;
+		return loc;
 
 	return -EIO;
 }
@@ -312,48 +315,57 @@ static int tpm_cr50_check_locality(struct tpm_chip *chip)
 /**
  * tpm_cr50_release_locality() - Release TPM locality.
  * @chip:	A TPM chip.
- * @force:	Flag to force release if set.
+ * @loc:	Locality to be released
+ *
+ * Return:
+ * - 0:		Success.
+ * - -errno:	A POSIX error code.
  */
-static void tpm_cr50_release_locality(struct tpm_chip *chip, bool force)
+static int tpm_cr50_release_locality(struct tpm_chip *chip, int loc)
 {
 	u8 mask = TPM_ACCESS_VALID | TPM_ACCESS_REQUEST_PENDING;
-	u8 addr = TPM_I2C_ACCESS(0);
+	u8 addr = TPM_I2C_ACCESS(loc);
 	u8 buf;
+	int rc;
 
-	if (tpm_cr50_i2c_read(chip, addr, &buf, sizeof(buf)) < 0)
-		return;
+	rc = tpm_cr50_i2c_read(chip, addr, &buf, sizeof(buf));
+	if (rc < 0)
+		return rc;
 
-	if (force || (buf & mask) == mask) {
+	if ((buf & mask) == mask) {
 		buf = TPM_ACCESS_ACTIVE_LOCALITY;
-		tpm_cr50_i2c_write(chip, addr, &buf, sizeof(buf));
+		rc = tpm_cr50_i2c_write(chip, addr, &buf, sizeof(buf));
 	}
+
+	return rc;
 }
 
 /**
- * tpm_cr50_request_locality() - Request TPM locality 0.
+ * tpm_cr50_request_locality() - Request TPM locality.
  * @chip: A TPM chip.
+ * @loc: Locality to be requested.
  *
  * Return:
- * - 0:		Success.
+ * - loc:	Success.
  * - -errno:	A POSIX error code.
  */
-static int tpm_cr50_request_locality(struct tpm_chip *chip)
+static int tpm_cr50_request_locality(struct tpm_chip *chip, int loc)
 {
 	u8 buf = TPM_ACCESS_REQUEST_USE;
 	unsigned long stop;
 	int rc;
 
-	if (!tpm_cr50_check_locality(chip))
-		return 0;
+	if (tpm_cr50_check_locality(chip, loc) == loc)
+		return loc;
 
-	rc = tpm_cr50_i2c_write(chip, TPM_I2C_ACCESS(0), &buf, sizeof(buf));
+	rc = tpm_cr50_i2c_write(chip, TPM_I2C_ACCESS(loc), &buf, sizeof(buf));
 	if (rc < 0)
 		return rc;
 
 	stop = jiffies + chip->timeout_a;
 	do {
-		if (!tpm_cr50_check_locality(chip))
-			return 0;
+		if (tpm_cr50_check_locality(chip, loc) == loc)
+			return loc;
 
 		msleep(TPM_CR50_TIMEOUT_SHORT_MS);
 	} while (time_before(jiffies, stop));
@@ -374,7 +386,9 @@ static u8 tpm_cr50_i2c_tis_status(struct tpm_chip *chip)
 {
 	u8 buf[4];
 
-	if (tpm_cr50_i2c_read(chip, TPM_I2C_STS(0), buf, sizeof(buf)) < 0)
+	WARN_ONCE((chip->locality < 0), "Incorrect tpm locality value\n");
+
+	if (tpm_cr50_i2c_read(chip, TPM_I2C_STS(chip->locality), buf, sizeof(buf)) < 0)
 		return 0;
 
 	return buf[0];
@@ -390,7 +404,9 @@ static void tpm_cr50_i2c_tis_set_ready(struct tpm_chip *chip)
 {
 	u8 buf[4] = { TPM_STS_COMMAND_READY };
 
-	tpm_cr50_i2c_write(chip, TPM_I2C_STS(0), buf, sizeof(buf));
+	WARN_ONCE((chip->locality < 0), "Incorrect tpm locality value\n");
+
+	tpm_cr50_i2c_write(chip, TPM_I2C_STS(chip->locality), buf, sizeof(buf));
 	msleep(TPM_CR50_TIMEOUT_SHORT_MS);
 }
 
@@ -420,7 +436,7 @@ static int tpm_cr50_i2c_get_burst_and_status(struct tpm_chip *chip, u8 mask,
 	stop = jiffies + chip->timeout_b;
 
 	do {
-		if (tpm_cr50_i2c_read(chip, TPM_I2C_STS(0), buf, sizeof(buf)) < 0) {
+		if (tpm_cr50_i2c_read(chip, TPM_I2C_STS(chip->locality), buf, sizeof(buf)) < 0) {
 			msleep(TPM_CR50_TIMEOUT_SHORT_MS);
 			continue;
 		}
@@ -454,10 +470,12 @@ static int tpm_cr50_i2c_tis_recv(struct tpm_chip *chip, u8 *buf, size_t buf_len)
 
 	u8 mask = TPM_STS_VALID | TPM_STS_DATA_AVAIL;
 	size_t burstcnt, cur, len, expected;
-	u8 addr = TPM_I2C_DATA_FIFO(0);
+	u8 addr = TPM_I2C_DATA_FIFO(chip->locality);
 	u32 status;
 	int rc;
 
+	WARN_ONCE((chip->locality < 0), "Incorrect tpm locality value\n");
+
 	if (buf_len < TPM_HEADER_SIZE)
 		return -EINVAL;
 
@@ -516,7 +534,6 @@ static int tpm_cr50_i2c_tis_recv(struct tpm_chip *chip, u8 *buf, size_t buf_len)
 		goto out_err;
 	}
 
-	tpm_cr50_release_locality(chip, false);
 	return cur;
 
 out_err:
@@ -524,7 +541,6 @@ static int tpm_cr50_i2c_tis_recv(struct tpm_chip *chip, u8 *buf, size_t buf_len)
 	if (tpm_cr50_i2c_tis_status(chip) & TPM_STS_COMMAND_READY)
 		tpm_cr50_i2c_tis_set_ready(chip);
 
-	tpm_cr50_release_locality(chip, false);
 	return rc;
 }
 
@@ -546,9 +562,7 @@ static int tpm_cr50_i2c_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
 	u32 status;
 	int rc;
 
-	rc = tpm_cr50_request_locality(chip);
-	if (rc < 0)
-		return rc;
+	WARN_ONCE((chip->locality < 0), "Incorrect tpm locality value\n");
 
 	/* Wait until TPM is ready for a command */
 	stop = jiffies + chip->timeout_b;
@@ -578,7 +592,8 @@ static int tpm_cr50_i2c_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
 		 * that is inserted by tpm_cr50_i2c_write()
 		 */
 		limit = min_t(size_t, burstcnt - 1, len);
-		rc = tpm_cr50_i2c_write(chip, TPM_I2C_DATA_FIFO(0), &buf[sent], limit);
+		rc = tpm_cr50_i2c_write(chip, TPM_I2C_DATA_FIFO(chip->locality),
+					&buf[sent], limit);
 		if (rc < 0) {
 			dev_err(&chip->dev, "Write failed\n");
 			goto out_err;
@@ -599,7 +614,7 @@ static int tpm_cr50_i2c_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
 	}
 
 	/* Start the TPM command */
-	rc = tpm_cr50_i2c_write(chip, TPM_I2C_STS(0), tpm_go,
+	rc = tpm_cr50_i2c_write(chip, TPM_I2C_STS(chip->locality), tpm_go,
 				sizeof(tpm_go));
 	if (rc < 0) {
 		dev_err(&chip->dev, "Start command failed\n");
@@ -612,7 +627,6 @@ static int tpm_cr50_i2c_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
 	if (tpm_cr50_i2c_tis_status(chip) & TPM_STS_COMMAND_READY)
 		tpm_cr50_i2c_tis_set_ready(chip);
 
-	tpm_cr50_release_locality(chip, false);
 	return rc;
 }
 
@@ -651,6 +665,8 @@ static const struct tpm_class_ops cr50_i2c = {
 	.req_complete_mask = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
 	.req_complete_val = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
 	.req_canceled = &tpm_cr50_i2c_req_canceled,
+	.request_locality = &tpm_cr50_request_locality,
+	.relinquish_locality = &tpm_cr50_release_locality,
 };
 
 #ifdef CONFIG_ACPI
@@ -686,6 +702,7 @@ static int tpm_cr50_i2c_probe(struct i2c_client *client)
 	u32 vendor;
 	u8 buf[4];
 	int rc;
+	int loc;
 
 	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
 		return -ENODEV;
@@ -728,24 +745,30 @@ static int tpm_cr50_i2c_probe(struct i2c_client *client)
 			 TPM_CR50_TIMEOUT_NOIRQ_MS);
 	}
 
-	rc = tpm_cr50_request_locality(chip);
-	if (rc < 0) {
+	loc = tpm_cr50_request_locality(chip, TPM_CR50_I2C_DEFAULT_LOC);
+	if (loc < 0) {
 		dev_err(dev, "Could not request locality\n");
 		return rc;
 	}
 
 	/* Read four bytes from DID_VID register */
-	rc = tpm_cr50_i2c_read(chip, TPM_I2C_DID_VID(0), buf, sizeof(buf));
+	rc = tpm_cr50_i2c_read(chip, TPM_I2C_DID_VID(loc), buf, sizeof(buf));
 	if (rc < 0) {
 		dev_err(dev, "Could not read vendor id\n");
-		tpm_cr50_release_locality(chip, true);
+		if (tpm_cr50_release_locality(chip, loc))
+			dev_err(dev, "Could not release locality\n");
+		return rc;
+	}
+
+	rc = tpm_cr50_release_locality(chip, loc);
+	if (rc) {
+		dev_err(dev, "Could not release locality\n");
 		return rc;
 	}
 
 	vendor = le32_to_cpup((__le32 *)buf);
 	if (vendor != TPM_CR50_I2C_DID_VID && vendor != TPM_TI50_I2C_DID_VID) {
 		dev_err(dev, "Vendor ID did not match! ID was %08x\n", vendor);
-		tpm_cr50_release_locality(chip, true);
 		return -ENODEV;
 	}
 
@@ -774,7 +797,6 @@ static void tpm_cr50_i2c_remove(struct i2c_client *client)
 	}
 
 	tpm_chip_unregister(chip);
-	tpm_cr50_release_locality(chip, true);
 }
 
 static SIMPLE_DEV_PM_OPS(cr50_i2c_pm, tpm_pm_suspend, tpm_pm_resume);
-- 
2.38.1.273.g43a17bfeac-goog

[PATCH 3/3] char: tpm: cr50: Move i2c locking to request/relinquish locality ops

[Jan Dabros]

Move i2c locking primitives to request_locality and relinquish_locality
callbacks, what effectively blocks TPM bus for the whole duration of
logical TPM operation.

With this in place, cr50-equipped TPM may be shared with external CPUs -
assuming that underneath i2c controller driver is aware of this setup
(see i2c-designware-amdpsp as an example).

Signed-off-by: Jan Dabros <jsd@semihalf.com>
---
 drivers/char/tpm/tpm_tis_i2c_cr50.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis_i2c_cr50.c b/drivers/char/tpm/tpm_tis_i2c_cr50.c
index d3438a4ed1ef8..c6628351ec383 100644
--- a/drivers/char/tpm/tpm_tis_i2c_cr50.c
+++ b/drivers/char/tpm/tpm_tis_i2c_cr50.c
@@ -202,8 +202,6 @@ static int tpm_cr50_i2c_read(struct tpm_chip *chip, u8 addr, u8 *buffer, size_t
 	};
 	int rc;
 
-	i2c_lock_bus(client->adapter, I2C_LOCK_SEGMENT);
-
 	/* Prepare for completion interrupt */
 	tpm_cr50_i2c_enable_tpm_irq(chip);
 
@@ -222,7 +220,6 @@ static int tpm_cr50_i2c_read(struct tpm_chip *chip, u8 addr, u8 *buffer, size_t
 
 out:
 	tpm_cr50_i2c_disable_tpm_irq(chip);
-	i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT);
 
 	if (rc < 0)
 		return rc;
@@ -264,8 +261,6 @@ static int tpm_cr50_i2c_write(struct tpm_chip *chip, u8 addr, u8 *buffer,
 	priv->buf[0] = addr;
 	memcpy(priv->buf + 1, buffer, len);
 
-	i2c_lock_bus(client->adapter, I2C_LOCK_SEGMENT);
-
 	/* Prepare for completion interrupt */
 	tpm_cr50_i2c_enable_tpm_irq(chip);
 
@@ -279,7 +274,6 @@ static int tpm_cr50_i2c_write(struct tpm_chip *chip, u8 addr, u8 *buffer,
 
 out:
 	tpm_cr50_i2c_disable_tpm_irq(chip);
-	i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT);
 
 	if (rc < 0)
 		return rc;
@@ -323,6 +317,7 @@ static int tpm_cr50_check_locality(struct tpm_chip *chip, int loc)
  */
 static int tpm_cr50_release_locality(struct tpm_chip *chip, int loc)
 {
+	struct i2c_client *client = to_i2c_client(chip->dev.parent);
 	u8 mask = TPM_ACCESS_VALID | TPM_ACCESS_REQUEST_PENDING;
 	u8 addr = TPM_I2C_ACCESS(loc);
 	u8 buf;
@@ -330,13 +325,15 @@ static int tpm_cr50_release_locality(struct tpm_chip *chip, int loc)
 
 	rc = tpm_cr50_i2c_read(chip, addr, &buf, sizeof(buf));
 	if (rc < 0)
-		return rc;
+		goto unlock_out;
 
 	if ((buf & mask) == mask) {
 		buf = TPM_ACCESS_ACTIVE_LOCALITY;
 		rc = tpm_cr50_i2c_write(chip, addr, &buf, sizeof(buf));
 	}
 
+unlock_out:
+	i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT);
 	return rc;
 }
 
@@ -351,16 +348,19 @@ static int tpm_cr50_release_locality(struct tpm_chip *chip, int loc)
  */
 static int tpm_cr50_request_locality(struct tpm_chip *chip, int loc)
 {
+	struct i2c_client *client = to_i2c_client(chip->dev.parent);
 	u8 buf = TPM_ACCESS_REQUEST_USE;
 	unsigned long stop;
 	int rc;
 
+	i2c_lock_bus(client->adapter, I2C_LOCK_SEGMENT);
+
 	if (tpm_cr50_check_locality(chip, loc) == loc)
 		return loc;
 
 	rc = tpm_cr50_i2c_write(chip, TPM_I2C_ACCESS(loc), &buf, sizeof(buf));
 	if (rc < 0)
-		return rc;
+		goto unlock_out;
 
 	stop = jiffies + chip->timeout_a;
 	do {
@@ -370,7 +370,11 @@ static int tpm_cr50_request_locality(struct tpm_chip *chip, int loc)
 		msleep(TPM_CR50_TIMEOUT_SHORT_MS);
 	} while (time_before(jiffies, stop));
 
-	return -ETIMEDOUT;
+	rc = -ETIMEDOUT;
+
+unlock_out:
+	i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT);
+	return rc;
 }
 
 /**
-- 
2.38.1.273.g43a17bfeac-goog

Re: [PATCH 1/3] char: tpm: Protect tpm_pm_suspend with locks

[Tim Van Patten]

On Mon, Oct 31, 2022 at 8:04 PM Jan Dabros <jsd@semihalf.com> wrote:
>
> -       if (!tpm_chip_start(chip)) {
> +       rc = tpm_try_get_ops(chip);
> +       if (!rc) {
>                 if (chip->flags & TPM_CHIP_FLAG_TPM2)
>                         tpm2_shutdown(chip, TPM2_SU_STATE);
>                 else
>                         rc = tpm1_pm_suspend(chip, tpm_suspend_pcr);

This if-else block is still interacting with the TPM even though
you're not guaranteed to have the lock, which could lead to
racy/inchorent results. Would it be better to just bail out entirely
since we can't safely attempt any recovery at this point. If it's
still worth attempting the shutdown command, it would at least be good
to add a comment admitting that we have no choice but to communicate
with the TPM without a lock.

-- 

Tim Van Patten | ChromeOS | timvp@google.com | (720) 432-0997

Re: [PATCH 2/3] char: tpm: cr50: Use generic request/relinquish locality ops

[Tim Van Patten]

On Mon, Oct 31, 2022 at 8:04 PM Jan Dabros <jsd@semihalf.com> wrote:
>  /**
> - * tpm_cr50_check_locality() - Verify TPM locality 0 is active.
> + * tpm_cr50_check_locality() - Verify if required TPM locality is active.
>   * @chip: A TPM chip.
> + * @loc: Locality to be verified
>   *
>   * Return:
> - * - 0:                Success.
> + * - loc:      Success.
>   * - -errno:   A POSIX error code.
>   */
> -static int tpm_cr50_check_locality(struct tpm_chip *chip)
> +static int tpm_cr50_check_locality(struct tpm_chip *chip, int loc)
>  {
>         u8 mask = TPM_ACCESS_VALID | TPM_ACCESS_ACTIVE_LOCALITY;
>         u8 buf;
>         int rc;
>
> -       rc = tpm_cr50_i2c_read(chip, TPM_I2C_ACCESS(0), &buf, sizeof(buf));
> +       rc = tpm_cr50_i2c_read(chip, TPM_I2C_ACCESS(loc), &buf, sizeof(buf));
>         if (rc < 0)
>                 return rc;
>
>         if ((buf & mask) == mask)
> -               return 0;
> +               return loc;
>
>         return -EIO;
>  }

Why is it useful to return the same `loc` value that was passed in,
rather than just returning `0`? The caller already knows the value of
`loc`, so they aren't being told anything new.

I think this should continue to return `0` for success.


> - * tpm_cr50_request_locality() - Request TPM locality 0.
> + * tpm_cr50_request_locality() - Request TPM locality.
>   * @chip: A TPM chip.
> + * @loc: Locality to be requested.
>   *
>   * Return:
> - * - 0:                Success.
> + * - loc:      Success.

Same as above. Return `0`.

> @@ -374,7 +386,9 @@ static u8 tpm_cr50_i2c_tis_status(struct tpm_chip *chip)
>  {
>         u8 buf[4];
>
> -       if (tpm_cr50_i2c_read(chip, TPM_I2C_STS(0), buf, sizeof(buf)) < 0)
> +       WARN_ONCE((chip->locality < 0), "Incorrect tpm locality value\n");

For each of these ` WARN_ONCE((chip->locality < 0), ...).`, can it
return immediately rather than attempting to continue using an invalid
locality value? Do the following commands have a chance of succeeding
with the invalid value?

-- 

Tim Van Patten | ChromeOS | timvp@google.com | (720) 432-0997

Re: [PATCH 1/3] char: tpm: Protect tpm_pm_suspend with locks

[Jan Dąbroś]

> > -       if (!tpm_chip_start(chip)) {
> > +       rc = tpm_try_get_ops(chip);
> > +       if (!rc) {
> >                 if (chip->flags & TPM_CHIP_FLAG_TPM2)
> >                         tpm2_shutdown(chip, TPM2_SU_STATE);
> >                 else
> >                         rc = tpm1_pm_suspend(chip, tpm_suspend_pcr);
>
> This if-else block is still interacting with the TPM even though
> you're not guaranteed to have the lock, which could lead to
> racy/inchorent results. Would it be better to just bail out entirely
> since we can't safely attempt any recovery at this point. If it's
> still worth attempting the shutdown command, it would at least be good
> to add a comment admitting that we have no choice but to communicate
> with the TPM without a lock.

If tpm_try_get_ops() returns 0 it means that we have a lock. And if we
don't have a lock, then we are not executing any TPM commands. Are you
referring to tpm_mutex or something different?

Best Regards,
Jan

Re: [PATCH 2/3] char: tpm: cr50: Use generic request/relinquish locality ops

[Jan Dąbroś]

> >  /**
> > - * tpm_cr50_check_locality() - Verify TPM locality 0 is active.
> > + * tpm_cr50_check_locality() - Verify if required TPM locality is active.
> >   * @chip: A TPM chip.
> > + * @loc: Locality to be verified
> >   *
> >   * Return:
> > - * - 0:                Success.
> > + * - loc:      Success.
> >   * - -errno:   A POSIX error code.
> >   */
> > -static int tpm_cr50_check_locality(struct tpm_chip *chip)
> > +static int tpm_cr50_check_locality(struct tpm_chip *chip, int loc)
> >  {
> >         u8 mask = TPM_ACCESS_VALID | TPM_ACCESS_ACTIVE_LOCALITY;
> >         u8 buf;
> >         int rc;
> >
> > -       rc = tpm_cr50_i2c_read(chip, TPM_I2C_ACCESS(0), &buf, sizeof(buf));
> > +       rc = tpm_cr50_i2c_read(chip, TPM_I2C_ACCESS(loc), &buf, sizeof(buf));
> >         if (rc < 0)
> >                 return rc;
> >
> >         if ((buf & mask) == mask)
> > -               return 0;
> > +               return loc;
> >
> >         return -EIO;
> >  }
>
> Why is it useful to return the same `loc` value that was passed in,
> rather than just returning `0`? The caller already knows the value of
> `loc`, so they aren't being told anything new.
>
> I think this should continue to return `0` for success.

I agree, I should keep this as it was.

>
>
> > - * tpm_cr50_request_locality() - Request TPM locality 0.
> > + * tpm_cr50_request_locality() - Request TPM locality.
> >   * @chip: A TPM chip.
> > + * @loc: Locality to be requested.
> >   *
> >   * Return:
> > - * - 0:                Success.
> > + * - loc:      Success.
>
> Same as above. Return `0`.

The case here is that .request_locality callback should return active
locality. This value is assigned to chip->locality inside
tpm_request_locality() [drivers/char/tpm/tpm-chip.c].

>
> > @@ -374,7 +386,9 @@ static u8 tpm_cr50_i2c_tis_status(struct tpm_chip *chip)
> >  {
> >         u8 buf[4];
> >
> > -       if (tpm_cr50_i2c_read(chip, TPM_I2C_STS(0), buf, sizeof(buf)) < 0)
> > +       WARN_ONCE((chip->locality < 0), "Incorrect tpm locality value\n");
>
> For each of these ` WARN_ONCE((chip->locality < 0), ...).`, can it
> return immediately rather than attempting to continue using an invalid
> locality value? Do the following commands have a chance of succeeding
> with the invalid value?

I agree - it makes more sense to return immediately instead of trying
to send invalid configuration over i2c.

Best Regards,
Jan

Re: [PATCH 1/3] char: tpm: Protect tpm_pm_suspend with locks

[Tim Van Patten]

On Wed, Nov 2, 2022 at 3:28 PM Jan Dąbroś <jsd@semihalf.com> wrote:
>
> > > -       if (!tpm_chip_start(chip)) {
> > > +       rc = tpm_try_get_ops(chip);
> > > +       if (!rc) {
> > >                 if (chip->flags & TPM_CHIP_FLAG_TPM2)
> > >                         tpm2_shutdown(chip, TPM2_SU_STATE);
> > >                 else
> > >                         rc = tpm1_pm_suspend(chip, tpm_suspend_pcr);
> >
> > This if-else block is still interacting with the TPM even though
> > you're not guaranteed to have the lock, which could lead to
> > racy/inchorent results. Would it be better to just bail out entirely
> > since we can't safely attempt any recovery at this point. If it's
> > still worth attempting the shutdown command, it would at least be good
> > to add a comment admitting that we have no choice but to communicate
> > with the TPM without a lock.
>
> If tpm_try_get_ops() returns 0 it means that we have a lock. And if we
> don't have a lock, then we are not executing any TPM commands. Are you
> referring to tpm_mutex or something different?

Ah, yup, I was reading this backwards, thinking that something had
gone wrong when entering this block. Nevermind.

-- 

Tim Van Patten | ChromeOS | timvp@google.com | (720) 432-0997

Re: [PATCH 2/3] char: tpm: cr50: Use generic request/relinquish locality ops

[bernacki]

Tim,

[...]
> Why is it useful to return the same `loc` value that was passed in,
> rather than just returning `0`? The caller already knows the value of
> `loc`, so they aren't being told anything new.
> 
> I think this should continue to return `0` for success.

I think Jan just followed the conventions, when he returned 'loc' instead of
'0', some others request/release locality function do exactly the same.

[...]
> For each of these ` WARN_ONCE((chip->locality < 0), ...).`, can it
> return immediately rather than attempting to continue using an invalid
> locality value? Do the following commands have a chance of succeeding
> with the invalid value?

WARN_ONCE() macro does not remove checking of locality. If I understand
the code correctly layer above should not called this function if
request locality fails, so this code is an extra check. 
I can remove it in the next patchset if you want.

Jarkko,

Would it be possible to merge this changes. Patch 1. has already been merged,
only 2 and 3 are still waiting. Do you want me to create a new patchset for
these two patches?

thanks,
greg

Re: [PATCH 2/3] char: tpm: cr50: Use generic request/relinquish locality ops

[Jarkko Sakkinen]

On Mon, 2024-09-30 at 11:52 +0000, bernacki@chromium.org wrote:
> Tim,
> 
> [...]
> > Why is it useful to return the same `loc` value that was passed in,
> > rather than just returning `0`? The caller already knows the value
> > of
> > `loc`, so they aren't being told anything new.
> > 
> > I think this should continue to return `0` for success.
> 
> I think Jan just followed the conventions, when he returned 'loc'
> instead of
> '0', some others request/release locality function do exactly the
> same.
> 
> [...]
> > For each of these ` WARN_ONCE((chip->locality < 0), ...).`, can it
> > return immediately rather than attempting to continue using an
> > invalid
> > locality value? Do the following commands have a chance of
> > succeeding
> > with the invalid value?
> 
> WARN_ONCE() macro does not remove checking of locality. If I
> understand
> the code correctly layer above should not called this function if
> request locality fails, so this code is an extra check. 
> I can remove it in the next patchset if you want.
> 
> Jarkko,
> 
> Would it be possible to merge this changes. Patch 1. has already been
> merged,
> only 2 and 3 are still waiting. Do you want me to create a new
> patchset for
> these two patches?

Send a rebased version that applies. Let's check that through
then. Quick recap and some time gone, I don't see anything
extremely bad. Still acking patches that even do not apply
is not possible.

So yeah, send. If glitches are spotted in worst case this
means two rounds.

> 
> thanks,
> greg


BR, Jarkko

[PATCH V2 0/2] char: tpm: Adjust cr50_i2c locking mechanism

[Grzegorz Bernacki]

From: Grzegorz Bernacki <bernacki@chromium.org>

This patchset contains two patches from original patchset which have
not yet be merged. As per original description:

This patchset aims to add support for a platforms with cr50(tpm) i2c
chip shared across two CPUs. We need to provide a mechanism, which will
allow to synchronize accesses on a TPM-transaction boundaries.

These two patches are modifying cr50 to use generic callbacks for
locality management and then leverage this solution to apply i2c bus
locking on a TPM-operation level.

Changes from V1:
- add dev_err() and return instead of WARN_ONCE()
- correct returned value in tpm_cr50_i2c_probe() functions
- I did not address Tim's suggestion to return 0 instead of loc
in check and request locality, I would like to follow convention
from tpm_tis_core.c file 

Jan Dabros (2):
  char: tpm: cr50: Use generic request/relinquish locality ops
  char: tpm: cr50: Move i2c locking to request/relinquish locality ops

 drivers/char/tpm/tpm_tis_i2c_cr50.c | 132 ++++++++++++++++++----------
 1 file changed, 85 insertions(+), 47 deletions(-)

-- 
2.47.0.rc0.187.ge670bccf7e-goog

[PATCH V2 1/2] char: tpm: cr50: Use generic request/relinquish locality ops

[Grzegorz Bernacki]

From: Jan Dabros <jsd@semihalf.com>

Instead of using static functions tpm_cr50_request_locality and
tpm_cr50_release_locality register callbacks from tpm class chip->ops
created for this purpose.

Signed-off-by: Jan Dabros <jsd@semihalf.com>
Signed-off-by: Grzegorz Bernacki <bernacki@chromium.org>
---
 drivers/char/tpm/tpm_tis_i2c_cr50.c | 112 ++++++++++++++++++----------
 1 file changed, 73 insertions(+), 39 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis_i2c_cr50.c b/drivers/char/tpm/tpm_tis_i2c_cr50.c
index adf22992138e..1b1e403383fc 100644
--- a/drivers/char/tpm/tpm_tis_i2c_cr50.c
+++ b/drivers/char/tpm/tpm_tis_i2c_cr50.c
@@ -17,6 +17,7 @@
  */
 
 #include <linux/acpi.h>
+#include <linux/bug.h>
 #include <linux/completion.h>
 #include <linux/i2c.h>
 #include <linux/interrupt.h>
@@ -35,6 +36,7 @@
 #define TPM_CR50_I2C_MAX_RETRIES	3		/* Max retries due to I2C errors */
 #define TPM_CR50_I2C_RETRY_DELAY_LO	55		/* Min usecs between retries on I2C */
 #define TPM_CR50_I2C_RETRY_DELAY_HI	65		/* Max usecs between retries on I2C */
+#define TPM_CR50_I2C_DEFAULT_LOC	0
 
 #define TPM_I2C_ACCESS(l)	(0x0000 | ((l) << 4))
 #define TPM_I2C_STS(l)		(0x0001 | ((l) << 4))
@@ -285,25 +287,26 @@ static int tpm_cr50_i2c_write(struct tpm_chip *chip, u8 addr, u8 *buffer,
 }
 
 /**
- * tpm_cr50_check_locality() - Verify TPM locality 0 is active.
+ * tpm_cr50_check_locality() - Verify if required TPM locality is active.
  * @chip: A TPM chip.
+ * @loc: Locality to be verified
  *
  * Return:
- * - 0:		Success.
+ * - loc:	Success.
  * - -errno:	A POSIX error code.
  */
-static int tpm_cr50_check_locality(struct tpm_chip *chip)
+static int tpm_cr50_check_locality(struct tpm_chip *chip, int loc)
 {
 	u8 mask = TPM_ACCESS_VALID | TPM_ACCESS_ACTIVE_LOCALITY;
 	u8 buf;
 	int rc;
 
-	rc = tpm_cr50_i2c_read(chip, TPM_I2C_ACCESS(0), &buf, sizeof(buf));
+	rc = tpm_cr50_i2c_read(chip, TPM_I2C_ACCESS(loc), &buf, sizeof(buf));
 	if (rc < 0)
 		return rc;
 
 	if ((buf & mask) == mask)
-		return 0;
+		return loc;
 
 	return -EIO;
 }
@@ -311,48 +314,57 @@ static int tpm_cr50_check_locality(struct tpm_chip *chip)
 /**
  * tpm_cr50_release_locality() - Release TPM locality.
  * @chip:	A TPM chip.
- * @force:	Flag to force release if set.
+ * @loc:	Locality to be released
+ *
+ * Return:
+ * - 0:		Success.
+ * - -errno:	A POSIX error code.
  */
-static void tpm_cr50_release_locality(struct tpm_chip *chip, bool force)
+static int tpm_cr50_release_locality(struct tpm_chip *chip, int loc)
 {
 	u8 mask = TPM_ACCESS_VALID | TPM_ACCESS_REQUEST_PENDING;
-	u8 addr = TPM_I2C_ACCESS(0);
+	u8 addr = TPM_I2C_ACCESS(loc);
 	u8 buf;
+	int rc;
 
-	if (tpm_cr50_i2c_read(chip, addr, &buf, sizeof(buf)) < 0)
-		return;
+	rc = tpm_cr50_i2c_read(chip, addr, &buf, sizeof(buf));
+	if (rc < 0)
+		return rc;
 
-	if (force || (buf & mask) == mask) {
+	if ((buf & mask) == mask) {
 		buf = TPM_ACCESS_ACTIVE_LOCALITY;
-		tpm_cr50_i2c_write(chip, addr, &buf, sizeof(buf));
+		rc = tpm_cr50_i2c_write(chip, addr, &buf, sizeof(buf));
 	}
+
+	return rc;
 }
 
 /**
- * tpm_cr50_request_locality() - Request TPM locality 0.
+ * tpm_cr50_request_locality() - Request TPM locality.
  * @chip: A TPM chip.
+ * @loc: Locality to be requested.
  *
  * Return:
- * - 0:		Success.
+ * - loc:	Success.
  * - -errno:	A POSIX error code.
  */
-static int tpm_cr50_request_locality(struct tpm_chip *chip)
+static int tpm_cr50_request_locality(struct tpm_chip *chip, int loc)
 {
 	u8 buf = TPM_ACCESS_REQUEST_USE;
 	unsigned long stop;
 	int rc;
 
-	if (!tpm_cr50_check_locality(chip))
-		return 0;
+	if (tpm_cr50_check_locality(chip, loc) == loc)
+		return loc;
 
-	rc = tpm_cr50_i2c_write(chip, TPM_I2C_ACCESS(0), &buf, sizeof(buf));
+	rc = tpm_cr50_i2c_write(chip, TPM_I2C_ACCESS(loc), &buf, sizeof(buf));
 	if (rc < 0)
 		return rc;
 
 	stop = jiffies + chip->timeout_a;
 	do {
-		if (!tpm_cr50_check_locality(chip))
-			return 0;
+		if (tpm_cr50_check_locality(chip, loc) == loc)
+			return loc;
 
 		msleep(TPM_CR50_TIMEOUT_SHORT_MS);
 	} while (time_before(jiffies, stop));
@@ -373,7 +385,12 @@ static u8 tpm_cr50_i2c_tis_status(struct tpm_chip *chip)
 {
 	u8 buf[4];
 
-	if (tpm_cr50_i2c_read(chip, TPM_I2C_STS(0), buf, sizeof(buf)) < 0)
+	if (chip->locality < 0) {
+		dev_err(&chip->dev, "Incorrect tpm locality value\n");
+		return 0;
+	}
+
+	if (tpm_cr50_i2c_read(chip, TPM_I2C_STS(chip->locality), buf, sizeof(buf)) < 0)
 		return 0;
 
 	return buf[0];
@@ -389,7 +406,12 @@ static void tpm_cr50_i2c_tis_set_ready(struct tpm_chip *chip)
 {
 	u8 buf[4] = { TPM_STS_COMMAND_READY };
 
-	tpm_cr50_i2c_write(chip, TPM_I2C_STS(0), buf, sizeof(buf));
+	if (chip->locality < 0) {
+		dev_err(&chip->dev, "Incorrect tpm locality value\n");
+		return;
+	}
+
+	tpm_cr50_i2c_write(chip, TPM_I2C_STS(chip->locality), buf, sizeof(buf));
 	msleep(TPM_CR50_TIMEOUT_SHORT_MS);
 }
 
@@ -419,7 +441,7 @@ static int tpm_cr50_i2c_get_burst_and_status(struct tpm_chip *chip, u8 mask,
 	stop = jiffies + chip->timeout_b;
 
 	do {
-		if (tpm_cr50_i2c_read(chip, TPM_I2C_STS(0), buf, sizeof(buf)) < 0) {
+		if (tpm_cr50_i2c_read(chip, TPM_I2C_STS(chip->locality), buf, sizeof(buf)) < 0) {
 			msleep(TPM_CR50_TIMEOUT_SHORT_MS);
 			continue;
 		}
@@ -453,10 +475,15 @@ static int tpm_cr50_i2c_tis_recv(struct tpm_chip *chip, u8 *buf, size_t buf_len)
 
 	u8 mask = TPM_STS_VALID | TPM_STS_DATA_AVAIL;
 	size_t burstcnt, cur, len, expected;
-	u8 addr = TPM_I2C_DATA_FIFO(0);
+	u8 addr = TPM_I2C_DATA_FIFO(chip->locality);
 	u32 status;
 	int rc;
 
+	if (chip->locality < 0) {
+		dev_err(&chip->dev, "Incorrect tpm locality value\n");
+		return -EACCES;
+	}
+
 	if (buf_len < TPM_HEADER_SIZE)
 		return -EINVAL;
 
@@ -515,7 +542,6 @@ static int tpm_cr50_i2c_tis_recv(struct tpm_chip *chip, u8 *buf, size_t buf_len)
 		goto out_err;
 	}
 
-	tpm_cr50_release_locality(chip, false);
 	return cur;
 
 out_err:
@@ -523,7 +549,6 @@ static int tpm_cr50_i2c_tis_recv(struct tpm_chip *chip, u8 *buf, size_t buf_len)
 	if (tpm_cr50_i2c_tis_status(chip) & TPM_STS_COMMAND_READY)
 		tpm_cr50_i2c_tis_set_ready(chip);
 
-	tpm_cr50_release_locality(chip, false);
 	return rc;
 }
 
@@ -545,9 +570,10 @@ static int tpm_cr50_i2c_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
 	u32 status;
 	int rc;
 
-	rc = tpm_cr50_request_locality(chip);
-	if (rc < 0)
-		return rc;
+	if (chip->locality < 0) {
+		dev_err(&chip->dev, "Incorrect tpm locality value\n");
+		return -EACCES;
+	}
 
 	/* Wait until TPM is ready for a command */
 	stop = jiffies + chip->timeout_b;
@@ -577,7 +603,8 @@ static int tpm_cr50_i2c_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
 		 * that is inserted by tpm_cr50_i2c_write()
 		 */
 		limit = min_t(size_t, burstcnt - 1, len);
-		rc = tpm_cr50_i2c_write(chip, TPM_I2C_DATA_FIFO(0), &buf[sent], limit);
+		rc = tpm_cr50_i2c_write(chip, TPM_I2C_DATA_FIFO(chip->locality),
+					&buf[sent], limit);
 		if (rc < 0) {
 			dev_err(&chip->dev, "Write failed\n");
 			goto out_err;
@@ -598,7 +625,7 @@ static int tpm_cr50_i2c_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
 	}
 
 	/* Start the TPM command */
-	rc = tpm_cr50_i2c_write(chip, TPM_I2C_STS(0), tpm_go,
+	rc = tpm_cr50_i2c_write(chip, TPM_I2C_STS(chip->locality), tpm_go,
 				sizeof(tpm_go));
 	if (rc < 0) {
 		dev_err(&chip->dev, "Start command failed\n");
@@ -611,7 +638,6 @@ static int tpm_cr50_i2c_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
 	if (tpm_cr50_i2c_tis_status(chip) & TPM_STS_COMMAND_READY)
 		tpm_cr50_i2c_tis_set_ready(chip);
 
-	tpm_cr50_release_locality(chip, false);
 	return rc;
 }
 
@@ -650,6 +676,8 @@ static const struct tpm_class_ops cr50_i2c = {
 	.req_complete_mask = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
 	.req_complete_val = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
 	.req_canceled = &tpm_cr50_i2c_req_canceled,
+	.request_locality = &tpm_cr50_request_locality,
+	.relinquish_locality = &tpm_cr50_release_locality,
 };
 
 #ifdef CONFIG_ACPI
@@ -684,6 +712,7 @@ static int tpm_cr50_i2c_probe(struct i2c_client *client)
 	u32 vendor;
 	u8 buf[4];
 	int rc;
+	int loc;
 
 	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
 		return -ENODEV;
@@ -726,24 +755,30 @@ static int tpm_cr50_i2c_probe(struct i2c_client *client)
 			 TPM_CR50_TIMEOUT_NOIRQ_MS);
 	}
 
-	rc = tpm_cr50_request_locality(chip);
-	if (rc < 0) {
+	loc = tpm_cr50_request_locality(chip, TPM_CR50_I2C_DEFAULT_LOC);
+	if (loc < 0) {
 		dev_err(dev, "Could not request locality\n");
-		return rc;
+		return loc;
 	}
 
 	/* Read four bytes from DID_VID register */
-	rc = tpm_cr50_i2c_read(chip, TPM_I2C_DID_VID(0), buf, sizeof(buf));
+	rc = tpm_cr50_i2c_read(chip, TPM_I2C_DID_VID(loc), buf, sizeof(buf));
 	if (rc < 0) {
 		dev_err(dev, "Could not read vendor id\n");
-		tpm_cr50_release_locality(chip, true);
+		if (tpm_cr50_release_locality(chip, loc))
+			dev_err(dev, "Could not release locality\n");
+		return rc;
+	}
+
+	rc = tpm_cr50_release_locality(chip, loc);
+	if (rc) {
+		dev_err(dev, "Could not release locality\n");
 		return rc;
 	}
 
 	vendor = le32_to_cpup((__le32 *)buf);
 	if (vendor != TPM_CR50_I2C_DID_VID && vendor != TPM_TI50_I2C_DID_VID) {
 		dev_err(dev, "Vendor ID did not match! ID was %08x\n", vendor);
-		tpm_cr50_release_locality(chip, true);
 		return -ENODEV;
 	}
 
@@ -772,7 +807,6 @@ static void tpm_cr50_i2c_remove(struct i2c_client *client)
 	}
 
 	tpm_chip_unregister(chip);
-	tpm_cr50_release_locality(chip, true);
 }
 
 static SIMPLE_DEV_PM_OPS(cr50_i2c_pm, tpm_pm_suspend, tpm_pm_resume);
-- 
2.47.0.rc0.187.ge670bccf7e-goog

[PATCH V2 2/2] char: tpm: cr50: Move i2c locking to request/relinquish locality ops

[Grzegorz Bernacki]

From: Jan Dabros <jsd@semihalf.com>

Move i2c locking primitives to request_locality and relinquish_locality
callbacks, what effectively blocks TPM bus for the whole duration of
logical TPM operation.

With this in place, cr50-equipped TPM may be shared with external CPUs -
assuming that underneath i2c controller driver is aware of this setup
(see i2c-designware-amdpsp as an example).

Signed-off-by: Jan Dabros <jsd@semihalf.com>
---
 drivers/char/tpm/tpm_tis_i2c_cr50.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis_i2c_cr50.c b/drivers/char/tpm/tpm_tis_i2c_cr50.c
index 1b1e403383fc..104ef6c7f11c 100644
--- a/drivers/char/tpm/tpm_tis_i2c_cr50.c
+++ b/drivers/char/tpm/tpm_tis_i2c_cr50.c
@@ -201,8 +201,6 @@ static int tpm_cr50_i2c_read(struct tpm_chip *chip, u8 addr, u8 *buffer, size_t
 	};
 	int rc;
 
-	i2c_lock_bus(client->adapter, I2C_LOCK_SEGMENT);
-
 	/* Prepare for completion interrupt */
 	tpm_cr50_i2c_enable_tpm_irq(chip);
 
@@ -221,7 +219,6 @@ static int tpm_cr50_i2c_read(struct tpm_chip *chip, u8 addr, u8 *buffer, size_t
 
 out:
 	tpm_cr50_i2c_disable_tpm_irq(chip);
-	i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT);
 
 	if (rc < 0)
 		return rc;
@@ -263,8 +260,6 @@ static int tpm_cr50_i2c_write(struct tpm_chip *chip, u8 addr, u8 *buffer,
 	priv->buf[0] = addr;
 	memcpy(priv->buf + 1, buffer, len);
 
-	i2c_lock_bus(client->adapter, I2C_LOCK_SEGMENT);
-
 	/* Prepare for completion interrupt */
 	tpm_cr50_i2c_enable_tpm_irq(chip);
 
@@ -278,7 +273,6 @@ static int tpm_cr50_i2c_write(struct tpm_chip *chip, u8 addr, u8 *buffer,
 
 out:
 	tpm_cr50_i2c_disable_tpm_irq(chip);
-	i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT);
 
 	if (rc < 0)
 		return rc;
@@ -322,6 +316,7 @@ static int tpm_cr50_check_locality(struct tpm_chip *chip, int loc)
  */
 static int tpm_cr50_release_locality(struct tpm_chip *chip, int loc)
 {
+	struct i2c_client *client = to_i2c_client(chip->dev.parent);
 	u8 mask = TPM_ACCESS_VALID | TPM_ACCESS_REQUEST_PENDING;
 	u8 addr = TPM_I2C_ACCESS(loc);
 	u8 buf;
@@ -329,13 +324,15 @@ static int tpm_cr50_release_locality(struct tpm_chip *chip, int loc)
 
 	rc = tpm_cr50_i2c_read(chip, addr, &buf, sizeof(buf));
 	if (rc < 0)
-		return rc;
+		goto unlock_out;
 
 	if ((buf & mask) == mask) {
 		buf = TPM_ACCESS_ACTIVE_LOCALITY;
 		rc = tpm_cr50_i2c_write(chip, addr, &buf, sizeof(buf));
 	}
 
+unlock_out:
+	i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT);
 	return rc;
 }
 
@@ -350,16 +347,19 @@ static int tpm_cr50_release_locality(struct tpm_chip *chip, int loc)
  */
 static int tpm_cr50_request_locality(struct tpm_chip *chip, int loc)
 {
+	struct i2c_client *client = to_i2c_client(chip->dev.parent);
 	u8 buf = TPM_ACCESS_REQUEST_USE;
 	unsigned long stop;
 	int rc;
 
+	i2c_lock_bus(client->adapter, I2C_LOCK_SEGMENT);
+
 	if (tpm_cr50_check_locality(chip, loc) == loc)
 		return loc;
 
 	rc = tpm_cr50_i2c_write(chip, TPM_I2C_ACCESS(loc), &buf, sizeof(buf));
 	if (rc < 0)
-		return rc;
+		goto unlock_out;
 
 	stop = jiffies + chip->timeout_a;
 	do {
@@ -369,7 +369,11 @@ static int tpm_cr50_request_locality(struct tpm_chip *chip, int loc)
 		msleep(TPM_CR50_TIMEOUT_SHORT_MS);
 	} while (time_before(jiffies, stop));
 
-	return -ETIMEDOUT;
+	rc = -ETIMEDOUT;
+
+unlock_out:
+	i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT);
+	return rc;
 }
 
 /**
-- 
2.47.0.rc0.187.ge670bccf7e-goog

Re: [PATCH V2 1/2] char: tpm: cr50: Use generic request/relinquish locality ops

[Jarkko Sakkinen]

On Wed Oct 9, 2024 at 12:42 PM EEST, Grzegorz Bernacki wrote:
> From: Jan Dabros <jsd@semihalf.com>
>
> Instead of using static functions tpm_cr50_request_locality and
> tpm_cr50_release_locality register callbacks from tpm class chip->ops
> created for this purpose.
>
> Signed-off-by: Jan Dabros <jsd@semihalf.com>
> Signed-off-by: Grzegorz Bernacki <bernacki@chromium.org>
> ---
>  drivers/char/tpm/tpm_tis_i2c_cr50.c | 112 ++++++++++++++++++----------
>  1 file changed, 73 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm_tis_i2c_cr50.c b/drivers/char/tpm/tpm_tis_i2c_cr50.c
> index adf22992138e..1b1e403383fc 100644
> --- a/drivers/char/tpm/tpm_tis_i2c_cr50.c
> +++ b/drivers/char/tpm/tpm_tis_i2c_cr50.c
> @@ -17,6 +17,7 @@
>   */
>  
>  #include <linux/acpi.h>
> +#include <linux/bug.h>
>  #include <linux/completion.h>
>  #include <linux/i2c.h>
>  #include <linux/interrupt.h>
> @@ -35,6 +36,7 @@
>  #define TPM_CR50_I2C_MAX_RETRIES	3		/* Max retries due to I2C errors */
>  #define TPM_CR50_I2C_RETRY_DELAY_LO	55		/* Min usecs between retries on I2C */
>  #define TPM_CR50_I2C_RETRY_DELAY_HI	65		/* Max usecs between retries on I2C */
> +#define TPM_CR50_I2C_DEFAULT_LOC	0
>  
>  #define TPM_I2C_ACCESS(l)	(0x0000 | ((l) << 4))
>  #define TPM_I2C_STS(l)		(0x0001 | ((l) << 4))
> @@ -285,25 +287,26 @@ static int tpm_cr50_i2c_write(struct tpm_chip *chip, u8 addr, u8 *buffer,
>  }
>  
>  /**
> - * tpm_cr50_check_locality() - Verify TPM locality 0 is active.
> + * tpm_cr50_check_locality() - Verify if required TPM locality is active.
>   * @chip: A TPM chip.
> + * @loc: Locality to be verified
>   *
>   * Return:
> - * - 0:		Success.
> + * - loc:	Success.
>   * - -errno:	A POSIX error code.
>   */
> -static int tpm_cr50_check_locality(struct tpm_chip *chip)
> +static int tpm_cr50_check_locality(struct tpm_chip *chip, int loc)
>  {
>  	u8 mask = TPM_ACCESS_VALID | TPM_ACCESS_ACTIVE_LOCALITY;
>  	u8 buf;
>  	int rc;
>  
> -	rc = tpm_cr50_i2c_read(chip, TPM_I2C_ACCESS(0), &buf, sizeof(buf));
> +	rc = tpm_cr50_i2c_read(chip, TPM_I2C_ACCESS(loc), &buf, sizeof(buf));
>  	if (rc < 0)
>  		return rc;
>  
>  	if ((buf & mask) == mask)
> -		return 0;
> +		return loc;
>  
>  	return -EIO;
>  }
> @@ -311,48 +314,57 @@ static int tpm_cr50_check_locality(struct tpm_chip *chip)
>  /**
>   * tpm_cr50_release_locality() - Release TPM locality.
>   * @chip:	A TPM chip.
> - * @force:	Flag to force release if set.
> + * @loc:	Locality to be released
> + *
> + * Return:
> + * - 0:		Success.
> + * - -errno:	A POSIX error code.
>   */
> -static void tpm_cr50_release_locality(struct tpm_chip *chip, bool force)
> +static int tpm_cr50_release_locality(struct tpm_chip *chip, int loc)
>  {
>  	u8 mask = TPM_ACCESS_VALID | TPM_ACCESS_REQUEST_PENDING;
> -	u8 addr = TPM_I2C_ACCESS(0);
> +	u8 addr = TPM_I2C_ACCESS(loc);
>  	u8 buf;
> +	int rc;
>  
> -	if (tpm_cr50_i2c_read(chip, addr, &buf, sizeof(buf)) < 0)
> -		return;
> +	rc = tpm_cr50_i2c_read(chip, addr, &buf, sizeof(buf));
> +	if (rc < 0)
> +		return rc;
>  
> -	if (force || (buf & mask) == mask) {
> +	if ((buf & mask) == mask) {
>  		buf = TPM_ACCESS_ACTIVE_LOCALITY;
> -		tpm_cr50_i2c_write(chip, addr, &buf, sizeof(buf));
> +		rc = tpm_cr50_i2c_write(chip, addr, &buf, sizeof(buf));
>  	}
> +
> +	return rc;
>  }
>  
>  /**
> - * tpm_cr50_request_locality() - Request TPM locality 0.
> + * tpm_cr50_request_locality() - Request TPM locality.
>   * @chip: A TPM chip.
> + * @loc: Locality to be requested.
>   *
>   * Return:
> - * - 0:		Success.
> + * - loc:	Success.
>   * - -errno:	A POSIX error code.
>   */
> -static int tpm_cr50_request_locality(struct tpm_chip *chip)
> +static int tpm_cr50_request_locality(struct tpm_chip *chip, int loc)
>  {
>  	u8 buf = TPM_ACCESS_REQUEST_USE;
>  	unsigned long stop;
>  	int rc;
>  
> -	if (!tpm_cr50_check_locality(chip))
> -		return 0;
> +	if (tpm_cr50_check_locality(chip, loc) == loc)
> +		return loc;
>  
> -	rc = tpm_cr50_i2c_write(chip, TPM_I2C_ACCESS(0), &buf, sizeof(buf));
> +	rc = tpm_cr50_i2c_write(chip, TPM_I2C_ACCESS(loc), &buf, sizeof(buf));
>  	if (rc < 0)
>  		return rc;
>  
>  	stop = jiffies + chip->timeout_a;
>  	do {
> -		if (!tpm_cr50_check_locality(chip))
> -			return 0;
> +		if (tpm_cr50_check_locality(chip, loc) == loc)
> +			return loc;
>  
>  		msleep(TPM_CR50_TIMEOUT_SHORT_MS);
>  	} while (time_before(jiffies, stop));
> @@ -373,7 +385,12 @@ static u8 tpm_cr50_i2c_tis_status(struct tpm_chip *chip)
>  {
>  	u8 buf[4];
>  
> -	if (tpm_cr50_i2c_read(chip, TPM_I2C_STS(0), buf, sizeof(buf)) < 0)
> +	if (chip->locality < 0) {
> +		dev_err(&chip->dev, "Incorrect tpm locality value\n");
> +		return 0;
> +	}
> +
> +	if (tpm_cr50_i2c_read(chip, TPM_I2C_STS(chip->locality), buf, sizeof(buf)) < 0)
>  		return 0;
>  
>  	return buf[0];
> @@ -389,7 +406,12 @@ static void tpm_cr50_i2c_tis_set_ready(struct tpm_chip *chip)
>  {
>  	u8 buf[4] = { TPM_STS_COMMAND_READY };
>  
> -	tpm_cr50_i2c_write(chip, TPM_I2C_STS(0), buf, sizeof(buf));
> +	if (chip->locality < 0) {
> +		dev_err(&chip->dev, "Incorrect tpm locality value\n");
> +		return;
> +	}
> +
> +	tpm_cr50_i2c_write(chip, TPM_I2C_STS(chip->locality), buf, sizeof(buf));
>  	msleep(TPM_CR50_TIMEOUT_SHORT_MS);
>  }
>  
> @@ -419,7 +441,7 @@ static int tpm_cr50_i2c_get_burst_and_status(struct tpm_chip *chip, u8 mask,
>  	stop = jiffies + chip->timeout_b;
>  
>  	do {
> -		if (tpm_cr50_i2c_read(chip, TPM_I2C_STS(0), buf, sizeof(buf)) < 0) {
> +		if (tpm_cr50_i2c_read(chip, TPM_I2C_STS(chip->locality), buf, sizeof(buf)) < 0) {
>  			msleep(TPM_CR50_TIMEOUT_SHORT_MS);
>  			continue;
>  		}
> @@ -453,10 +475,15 @@ static int tpm_cr50_i2c_tis_recv(struct tpm_chip *chip, u8 *buf, size_t buf_len)
>  
>  	u8 mask = TPM_STS_VALID | TPM_STS_DATA_AVAIL;
>  	size_t burstcnt, cur, len, expected;
> -	u8 addr = TPM_I2C_DATA_FIFO(0);
> +	u8 addr = TPM_I2C_DATA_FIFO(chip->locality);
>  	u32 status;
>  	int rc;
>  
> +	if (chip->locality < 0) {
> +		dev_err(&chip->dev, "Incorrect tpm locality value\n");
> +		return -EACCES;
> +	}
> +
>  	if (buf_len < TPM_HEADER_SIZE)
>  		return -EINVAL;
>  
> @@ -515,7 +542,6 @@ static int tpm_cr50_i2c_tis_recv(struct tpm_chip *chip, u8 *buf, size_t buf_len)
>  		goto out_err;
>  	}
>  
> -	tpm_cr50_release_locality(chip, false);
>  	return cur;
>  
>  out_err:
> @@ -523,7 +549,6 @@ static int tpm_cr50_i2c_tis_recv(struct tpm_chip *chip, u8 *buf, size_t buf_len)
>  	if (tpm_cr50_i2c_tis_status(chip) & TPM_STS_COMMAND_READY)
>  		tpm_cr50_i2c_tis_set_ready(chip);
>  
> -	tpm_cr50_release_locality(chip, false);
>  	return rc;
>  }
>  
> @@ -545,9 +570,10 @@ static int tpm_cr50_i2c_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
>  	u32 status;
>  	int rc;
>  
> -	rc = tpm_cr50_request_locality(chip);
> -	if (rc < 0)
> -		return rc;
> +	if (chip->locality < 0) {
> +		dev_err(&chip->dev, "Incorrect tpm locality value\n");
> +		return -EACCES;
> +	}
>  
>  	/* Wait until TPM is ready for a command */
>  	stop = jiffies + chip->timeout_b;
> @@ -577,7 +603,8 @@ static int tpm_cr50_i2c_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
>  		 * that is inserted by tpm_cr50_i2c_write()
>  		 */
>  		limit = min_t(size_t, burstcnt - 1, len);
> -		rc = tpm_cr50_i2c_write(chip, TPM_I2C_DATA_FIFO(0), &buf[sent], limit);
> +		rc = tpm_cr50_i2c_write(chip, TPM_I2C_DATA_FIFO(chip->locality),
> +					&buf[sent], limit);
>  		if (rc < 0) {
>  			dev_err(&chip->dev, "Write failed\n");
>  			goto out_err;
> @@ -598,7 +625,7 @@ static int tpm_cr50_i2c_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
>  	}
>  
>  	/* Start the TPM command */
> -	rc = tpm_cr50_i2c_write(chip, TPM_I2C_STS(0), tpm_go,
> +	rc = tpm_cr50_i2c_write(chip, TPM_I2C_STS(chip->locality), tpm_go,
>  				sizeof(tpm_go));
>  	if (rc < 0) {
>  		dev_err(&chip->dev, "Start command failed\n");
> @@ -611,7 +638,6 @@ static int tpm_cr50_i2c_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
>  	if (tpm_cr50_i2c_tis_status(chip) & TPM_STS_COMMAND_READY)
>  		tpm_cr50_i2c_tis_set_ready(chip);
>  
> -	tpm_cr50_release_locality(chip, false);
>  	return rc;
>  }
>  
> @@ -650,6 +676,8 @@ static const struct tpm_class_ops cr50_i2c = {
>  	.req_complete_mask = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
>  	.req_complete_val = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
>  	.req_canceled = &tpm_cr50_i2c_req_canceled,
> +	.request_locality = &tpm_cr50_request_locality,
> +	.relinquish_locality = &tpm_cr50_release_locality,
>  };
>  
>  #ifdef CONFIG_ACPI
> @@ -684,6 +712,7 @@ static int tpm_cr50_i2c_probe(struct i2c_client *client)
>  	u32 vendor;
>  	u8 buf[4];
>  	int rc;
> +	int loc;
>  
>  	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
>  		return -ENODEV;
> @@ -726,24 +755,30 @@ static int tpm_cr50_i2c_probe(struct i2c_client *client)
>  			 TPM_CR50_TIMEOUT_NOIRQ_MS);
>  	}
>  
> -	rc = tpm_cr50_request_locality(chip);
> -	if (rc < 0) {
> +	loc = tpm_cr50_request_locality(chip, TPM_CR50_I2C_DEFAULT_LOC);
> +	if (loc < 0) {
>  		dev_err(dev, "Could not request locality\n");
> -		return rc;
> +		return loc;
>  	}
>  
>  	/* Read four bytes from DID_VID register */
> -	rc = tpm_cr50_i2c_read(chip, TPM_I2C_DID_VID(0), buf, sizeof(buf));
> +	rc = tpm_cr50_i2c_read(chip, TPM_I2C_DID_VID(loc), buf, sizeof(buf));
>  	if (rc < 0) {
>  		dev_err(dev, "Could not read vendor id\n");
> -		tpm_cr50_release_locality(chip, true);
> +		if (tpm_cr50_release_locality(chip, loc))
> +			dev_err(dev, "Could not release locality\n");
> +		return rc;
> +	}
> +
> +	rc = tpm_cr50_release_locality(chip, loc);
> +	if (rc) {
> +		dev_err(dev, "Could not release locality\n");
>  		return rc;
>  	}
>  
>  	vendor = le32_to_cpup((__le32 *)buf);
>  	if (vendor != TPM_CR50_I2C_DID_VID && vendor != TPM_TI50_I2C_DID_VID) {
>  		dev_err(dev, "Vendor ID did not match! ID was %08x\n", vendor);
> -		tpm_cr50_release_locality(chip, true);
>  		return -ENODEV;
>  	}
>  
> @@ -772,7 +807,6 @@ static void tpm_cr50_i2c_remove(struct i2c_client *client)
>  	}
>  
>  	tpm_chip_unregister(chip);
> -	tpm_cr50_release_locality(chip, true);
>  }
>  
>  static SIMPLE_DEV_PM_OPS(cr50_i2c_pm, tpm_pm_suspend, tpm_pm_resume);

I'd suggest to simply remove locality checks, which are sprinkled all
over the patch (with two different return values).

BR, Jarkko

Re: [PATCH V2 2/2] char: tpm: cr50: Move i2c locking to request/relinquish locality ops

[Jarkko Sakkinen]

On Wed Oct 9, 2024 at 12:42 PM EEST, Grzegorz Bernacki wrote:
> From: Jan Dabros <jsd@semihalf.com>
>
> Move i2c locking primitives to request_locality and relinquish_locality
> callbacks, what effectively blocks TPM bus for the whole duration of
> logical TPM operation.
>
> With this in place, cr50-equipped TPM may be shared with external CPUs -
> assuming that underneath i2c controller driver is aware of this setup
> (see i2c-designware-amdpsp as an example).
>
> Signed-off-by: Jan Dabros <jsd@semihalf.com>
> ---
>  drivers/char/tpm/tpm_tis_i2c_cr50.c | 22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm_tis_i2c_cr50.c b/drivers/char/tpm/tpm_tis_i2c_cr50.c
> index 1b1e403383fc..104ef6c7f11c 100644
> --- a/drivers/char/tpm/tpm_tis_i2c_cr50.c
> +++ b/drivers/char/tpm/tpm_tis_i2c_cr50.c
> @@ -201,8 +201,6 @@ static int tpm_cr50_i2c_read(struct tpm_chip *chip, u8 addr, u8 *buffer, size_t
>  	};
>  	int rc;
>  
> -	i2c_lock_bus(client->adapter, I2C_LOCK_SEGMENT);
> -
>  	/* Prepare for completion interrupt */
>  	tpm_cr50_i2c_enable_tpm_irq(chip);
>  
> @@ -221,7 +219,6 @@ static int tpm_cr50_i2c_read(struct tpm_chip *chip, u8 addr, u8 *buffer, size_t
>  
>  out:
>  	tpm_cr50_i2c_disable_tpm_irq(chip);
> -	i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT);
>  
>  	if (rc < 0)
>  		return rc;
> @@ -263,8 +260,6 @@ static int tpm_cr50_i2c_write(struct tpm_chip *chip, u8 addr, u8 *buffer,
>  	priv->buf[0] = addr;
>  	memcpy(priv->buf + 1, buffer, len);
>  
> -	i2c_lock_bus(client->adapter, I2C_LOCK_SEGMENT);
> -
>  	/* Prepare for completion interrupt */
>  	tpm_cr50_i2c_enable_tpm_irq(chip);
>  
> @@ -278,7 +273,6 @@ static int tpm_cr50_i2c_write(struct tpm_chip *chip, u8 addr, u8 *buffer,
>  
>  out:
>  	tpm_cr50_i2c_disable_tpm_irq(chip);
> -	i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT);
>  
>  	if (rc < 0)
>  		return rc;
> @@ -322,6 +316,7 @@ static int tpm_cr50_check_locality(struct tpm_chip *chip, int loc)
>   */
>  static int tpm_cr50_release_locality(struct tpm_chip *chip, int loc)
>  {
> +	struct i2c_client *client = to_i2c_client(chip->dev.parent);
>  	u8 mask = TPM_ACCESS_VALID | TPM_ACCESS_REQUEST_PENDING;
>  	u8 addr = TPM_I2C_ACCESS(loc);
>  	u8 buf;
> @@ -329,13 +324,15 @@ static int tpm_cr50_release_locality(struct tpm_chip *chip, int loc)
>  
>  	rc = tpm_cr50_i2c_read(chip, addr, &buf, sizeof(buf));
>  	if (rc < 0)
> -		return rc;
> +		goto unlock_out;
>  
>  	if ((buf & mask) == mask) {
>  		buf = TPM_ACCESS_ACTIVE_LOCALITY;
>  		rc = tpm_cr50_i2c_write(chip, addr, &buf, sizeof(buf));
>  	}
>  
> +unlock_out:
> +	i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT);
>  	return rc;
>  }
>  
> @@ -350,16 +347,19 @@ static int tpm_cr50_release_locality(struct tpm_chip *chip, int loc)
>   */
>  static int tpm_cr50_request_locality(struct tpm_chip *chip, int loc)
>  {
> +	struct i2c_client *client = to_i2c_client(chip->dev.parent);
>  	u8 buf = TPM_ACCESS_REQUEST_USE;
>  	unsigned long stop;
>  	int rc;
>  
> +	i2c_lock_bus(client->adapter, I2C_LOCK_SEGMENT);
> +
>  	if (tpm_cr50_check_locality(chip, loc) == loc)
>  		return loc;
>  
>  	rc = tpm_cr50_i2c_write(chip, TPM_I2C_ACCESS(loc), &buf, sizeof(buf));
>  	if (rc < 0)
> -		return rc;
> +		goto unlock_out;
>  
>  	stop = jiffies + chip->timeout_a;
>  	do {
> @@ -369,7 +369,11 @@ static int tpm_cr50_request_locality(struct tpm_chip *chip, int loc)
>  		msleep(TPM_CR50_TIMEOUT_SHORT_MS);
>  	} while (time_before(jiffies, stop));
>  
> -	return -ETIMEDOUT;
> +	rc = -ETIMEDOUT;
> +
> +unlock_out:
> +	i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT);
> +	return rc;
>  }
>  
>  /**

Nothing apparently wrong in this. Will re-check after 1/2 is good.

BR, Jarkko

[PATCH V3 0/2] char: tpm: Adjust cr50_i2c locking mechanism

[Grzegorz Bernacki]

From: Grzegorz Bernacki <bernacki@chromium.org>

This patchset contains two patches from original patchset which have
not yet be merged. As per original description:

This patchset aims to add support for a platforms with cr50(tpm) i2c
chip shared across two CPUs. We need to provide a mechanism, which will
allow to synchronize accesses on a TPM-transaction boundaries.

These two patches are modifying cr50 to use generic callbacks for
locality management and then leverage this solution to apply i2c bus
locking on a TPM-operation level.

Changes from V1:
- add dev_err() and return instead of WARN_ONCE()
- correct returned value in tpm_cr50_i2c_probe() functions
- I did not address Tim's suggestion to return 0 instead of loc
in check and request locality, I would like to follow convention
from tpm_tis_core.c file 

Changes from V2:
- remove unnecesary locality checking

Jan Dabros (2):
  char: tpm: cr50: Use generic request/relinquish locality ops
  char: tpm: cr50: Move i2c locking to request/relinquish locality ops

 drivers/char/tpm/tpm_tis_i2c_cr50.c | 114 ++++++++++++++++------------
 1 file changed, 66 insertions(+), 48 deletions(-)

-- 
2.47.0.rc0.187.ge670bccf7e-goog

[PATCH V3 1/2] char: tpm: cr50: Use generic request/relinquish locality ops

[Grzegorz Bernacki]

From: Jan Dabros <jsd@semihalf.com>

Instead of using static functions tpm_cr50_request_locality and
tpm_cr50_release_locality register callbacks from tpm class chip->ops
created for this purpose.

Change-Id: Iee45ab1cbb2f4d898fbac12e9632de27f615bfba
Signed-off-by: Jan Dabros <jsd@semihalf.com>
Signed-off-by: Grzegorz Bernacki <bernacki@chromium.org>
---
 drivers/char/tpm/tpm_tis_i2c_cr50.c | 94 +++++++++++++++++------------
 1 file changed, 54 insertions(+), 40 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis_i2c_cr50.c b/drivers/char/tpm/tpm_tis_i2c_cr50.c
index adf22992138e..eed1c296a00c 100644
--- a/drivers/char/tpm/tpm_tis_i2c_cr50.c
+++ b/drivers/char/tpm/tpm_tis_i2c_cr50.c
@@ -17,6 +17,7 @@
  */
 
 #include <linux/acpi.h>
+#include <linux/bug.h>
 #include <linux/completion.h>
 #include <linux/i2c.h>
 #include <linux/interrupt.h>
@@ -35,6 +36,7 @@
 #define TPM_CR50_I2C_MAX_RETRIES	3		/* Max retries due to I2C errors */
 #define TPM_CR50_I2C_RETRY_DELAY_LO	55		/* Min usecs between retries on I2C */
 #define TPM_CR50_I2C_RETRY_DELAY_HI	65		/* Max usecs between retries on I2C */
+#define TPM_CR50_I2C_DEFAULT_LOC	0
 
 #define TPM_I2C_ACCESS(l)	(0x0000 | ((l) << 4))
 #define TPM_I2C_STS(l)		(0x0001 | ((l) << 4))
@@ -285,25 +287,26 @@ static int tpm_cr50_i2c_write(struct tpm_chip *chip, u8 addr, u8 *buffer,
 }
 
 /**
- * tpm_cr50_check_locality() - Verify TPM locality 0 is active.
+ * tpm_cr50_check_locality() - Verify if required TPM locality is active.
  * @chip: A TPM chip.
+ * @loc: Locality to be verified
  *
  * Return:
- * - 0:		Success.
+ * - loc:	Success.
  * - -errno:	A POSIX error code.
  */
-static int tpm_cr50_check_locality(struct tpm_chip *chip)
+static int tpm_cr50_check_locality(struct tpm_chip *chip, int loc)
 {
 	u8 mask = TPM_ACCESS_VALID | TPM_ACCESS_ACTIVE_LOCALITY;
 	u8 buf;
 	int rc;
 
-	rc = tpm_cr50_i2c_read(chip, TPM_I2C_ACCESS(0), &buf, sizeof(buf));
+	rc = tpm_cr50_i2c_read(chip, TPM_I2C_ACCESS(loc), &buf, sizeof(buf));
 	if (rc < 0)
 		return rc;
 
 	if ((buf & mask) == mask)
-		return 0;
+		return loc;
 
 	return -EIO;
 }
@@ -311,48 +314,57 @@ static int tpm_cr50_check_locality(struct tpm_chip *chip)
 /**
  * tpm_cr50_release_locality() - Release TPM locality.
  * @chip:	A TPM chip.
- * @force:	Flag to force release if set.
+ * @loc:	Locality to be released
+ *
+ * Return:
+ * - 0:		Success.
+ * - -errno:	A POSIX error code.
  */
-static void tpm_cr50_release_locality(struct tpm_chip *chip, bool force)
+static int tpm_cr50_release_locality(struct tpm_chip *chip, int loc)
 {
 	u8 mask = TPM_ACCESS_VALID | TPM_ACCESS_REQUEST_PENDING;
-	u8 addr = TPM_I2C_ACCESS(0);
+	u8 addr = TPM_I2C_ACCESS(loc);
 	u8 buf;
+	int rc;
 
-	if (tpm_cr50_i2c_read(chip, addr, &buf, sizeof(buf)) < 0)
-		return;
+	rc = tpm_cr50_i2c_read(chip, addr, &buf, sizeof(buf));
+	if (rc < 0)
+		return rc;
 
-	if (force || (buf & mask) == mask) {
+	if ((buf & mask) == mask) {
 		buf = TPM_ACCESS_ACTIVE_LOCALITY;
-		tpm_cr50_i2c_write(chip, addr, &buf, sizeof(buf));
+		rc = tpm_cr50_i2c_write(chip, addr, &buf, sizeof(buf));
 	}
+
+	return rc;
 }
 
 /**
- * tpm_cr50_request_locality() - Request TPM locality 0.
+ * tpm_cr50_request_locality() - Request TPM locality.
  * @chip: A TPM chip.
+ * @loc: Locality to be requested.
  *
  * Return:
- * - 0:		Success.
+ * - loc:	Success.
  * - -errno:	A POSIX error code.
  */
-static int tpm_cr50_request_locality(struct tpm_chip *chip)
+static int tpm_cr50_request_locality(struct tpm_chip *chip, int loc)
 {
 	u8 buf = TPM_ACCESS_REQUEST_USE;
 	unsigned long stop;
 	int rc;
 
-	if (!tpm_cr50_check_locality(chip))
-		return 0;
+	if (tpm_cr50_check_locality(chip, loc) == loc)
+		return loc;
 
-	rc = tpm_cr50_i2c_write(chip, TPM_I2C_ACCESS(0), &buf, sizeof(buf));
+	rc = tpm_cr50_i2c_write(chip, TPM_I2C_ACCESS(loc), &buf, sizeof(buf));
 	if (rc < 0)
 		return rc;
 
 	stop = jiffies + chip->timeout_a;
 	do {
-		if (!tpm_cr50_check_locality(chip))
-			return 0;
+		if (tpm_cr50_check_locality(chip, loc) == loc)
+			return loc;
 
 		msleep(TPM_CR50_TIMEOUT_SHORT_MS);
 	} while (time_before(jiffies, stop));
@@ -373,7 +385,7 @@ static u8 tpm_cr50_i2c_tis_status(struct tpm_chip *chip)
 {
 	u8 buf[4];
 
-	if (tpm_cr50_i2c_read(chip, TPM_I2C_STS(0), buf, sizeof(buf)) < 0)
+	if (tpm_cr50_i2c_read(chip, TPM_I2C_STS(chip->locality), buf, sizeof(buf)) < 0)
 		return 0;
 
 	return buf[0];
@@ -389,7 +401,7 @@ static void tpm_cr50_i2c_tis_set_ready(struct tpm_chip *chip)
 {
 	u8 buf[4] = { TPM_STS_COMMAND_READY };
 
-	tpm_cr50_i2c_write(chip, TPM_I2C_STS(0), buf, sizeof(buf));
+	tpm_cr50_i2c_write(chip, TPM_I2C_STS(chip->locality), buf, sizeof(buf));
 	msleep(TPM_CR50_TIMEOUT_SHORT_MS);
 }
 
@@ -419,7 +431,7 @@ static int tpm_cr50_i2c_get_burst_and_status(struct tpm_chip *chip, u8 mask,
 	stop = jiffies + chip->timeout_b;
 
 	do {
-		if (tpm_cr50_i2c_read(chip, TPM_I2C_STS(0), buf, sizeof(buf)) < 0) {
+		if (tpm_cr50_i2c_read(chip, TPM_I2C_STS(chip->locality), buf, sizeof(buf)) < 0) {
 			msleep(TPM_CR50_TIMEOUT_SHORT_MS);
 			continue;
 		}
@@ -453,7 +465,7 @@ static int tpm_cr50_i2c_tis_recv(struct tpm_chip *chip, u8 *buf, size_t buf_len)
 
 	u8 mask = TPM_STS_VALID | TPM_STS_DATA_AVAIL;
 	size_t burstcnt, cur, len, expected;
-	u8 addr = TPM_I2C_DATA_FIFO(0);
+	u8 addr = TPM_I2C_DATA_FIFO(chip->locality);
 	u32 status;
 	int rc;
 
@@ -515,7 +527,6 @@ static int tpm_cr50_i2c_tis_recv(struct tpm_chip *chip, u8 *buf, size_t buf_len)
 		goto out_err;
 	}
 
-	tpm_cr50_release_locality(chip, false);
 	return cur;
 
 out_err:
@@ -523,7 +534,6 @@ static int tpm_cr50_i2c_tis_recv(struct tpm_chip *chip, u8 *buf, size_t buf_len)
 	if (tpm_cr50_i2c_tis_status(chip) & TPM_STS_COMMAND_READY)
 		tpm_cr50_i2c_tis_set_ready(chip);
 
-	tpm_cr50_release_locality(chip, false);
 	return rc;
 }
 
@@ -545,10 +555,6 @@ static int tpm_cr50_i2c_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
 	u32 status;
 	int rc;
 
-	rc = tpm_cr50_request_locality(chip);
-	if (rc < 0)
-		return rc;
-
 	/* Wait until TPM is ready for a command */
 	stop = jiffies + chip->timeout_b;
 	while (!(tpm_cr50_i2c_tis_status(chip) & TPM_STS_COMMAND_READY)) {
@@ -577,7 +583,8 @@ static int tpm_cr50_i2c_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
 		 * that is inserted by tpm_cr50_i2c_write()
 		 */
 		limit = min_t(size_t, burstcnt - 1, len);
-		rc = tpm_cr50_i2c_write(chip, TPM_I2C_DATA_FIFO(0), &buf[sent], limit);
+		rc = tpm_cr50_i2c_write(chip, TPM_I2C_DATA_FIFO(chip->locality),
+					&buf[sent], limit);
 		if (rc < 0) {
 			dev_err(&chip->dev, "Write failed\n");
 			goto out_err;
@@ -598,7 +605,7 @@ static int tpm_cr50_i2c_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
 	}
 
 	/* Start the TPM command */
-	rc = tpm_cr50_i2c_write(chip, TPM_I2C_STS(0), tpm_go,
+	rc = tpm_cr50_i2c_write(chip, TPM_I2C_STS(chip->locality), tpm_go,
 				sizeof(tpm_go));
 	if (rc < 0) {
 		dev_err(&chip->dev, "Start command failed\n");
@@ -611,7 +618,6 @@ static int tpm_cr50_i2c_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
 	if (tpm_cr50_i2c_tis_status(chip) & TPM_STS_COMMAND_READY)
 		tpm_cr50_i2c_tis_set_ready(chip);
 
-	tpm_cr50_release_locality(chip, false);
 	return rc;
 }
 
@@ -650,6 +656,8 @@ static const struct tpm_class_ops cr50_i2c = {
 	.req_complete_mask = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
 	.req_complete_val = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
 	.req_canceled = &tpm_cr50_i2c_req_canceled,
+	.request_locality = &tpm_cr50_request_locality,
+	.relinquish_locality = &tpm_cr50_release_locality,
 };
 
 #ifdef CONFIG_ACPI
@@ -684,6 +692,7 @@ static int tpm_cr50_i2c_probe(struct i2c_client *client)
 	u32 vendor;
 	u8 buf[4];
 	int rc;
+	int loc;
 
 	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
 		return -ENODEV;
@@ -726,24 +735,30 @@ static int tpm_cr50_i2c_probe(struct i2c_client *client)
 			 TPM_CR50_TIMEOUT_NOIRQ_MS);
 	}
 
-	rc = tpm_cr50_request_locality(chip);
-	if (rc < 0) {
+	loc = tpm_cr50_request_locality(chip, TPM_CR50_I2C_DEFAULT_LOC);
+	if (loc < 0) {
 		dev_err(dev, "Could not request locality\n");
-		return rc;
+		return loc;
 	}
 
 	/* Read four bytes from DID_VID register */
-	rc = tpm_cr50_i2c_read(chip, TPM_I2C_DID_VID(0), buf, sizeof(buf));
+	rc = tpm_cr50_i2c_read(chip, TPM_I2C_DID_VID(loc), buf, sizeof(buf));
 	if (rc < 0) {
 		dev_err(dev, "Could not read vendor id\n");
-		tpm_cr50_release_locality(chip, true);
+		if (tpm_cr50_release_locality(chip, loc))
+			dev_err(dev, "Could not release locality\n");
+		return rc;
+	}
+
+	rc = tpm_cr50_release_locality(chip, loc);
+	if (rc) {
+		dev_err(dev, "Could not release locality\n");
 		return rc;
 	}
 
 	vendor = le32_to_cpup((__le32 *)buf);
 	if (vendor != TPM_CR50_I2C_DID_VID && vendor != TPM_TI50_I2C_DID_VID) {
 		dev_err(dev, "Vendor ID did not match! ID was %08x\n", vendor);
-		tpm_cr50_release_locality(chip, true);
 		return -ENODEV;
 	}
 
@@ -772,7 +787,6 @@ static void tpm_cr50_i2c_remove(struct i2c_client *client)
 	}
 
 	tpm_chip_unregister(chip);
-	tpm_cr50_release_locality(chip, true);
 }
 
 static SIMPLE_DEV_PM_OPS(cr50_i2c_pm, tpm_pm_suspend, tpm_pm_resume);
-- 
2.47.0.rc0.187.ge670bccf7e-goog

[PATCH V3 2/2] char: tpm: cr50: Move i2c locking to request/relinquish locality ops

[Grzegorz Bernacki]

From: Jan Dabros <jsd@semihalf.com>

Move i2c locking primitives to request_locality and relinquish_locality
callbacks, what effectively blocks TPM bus for the whole duration of
logical TPM operation.

With this in place, cr50-equipped TPM may be shared with external CPUs -
assuming that underneath i2c controller driver is aware of this setup
(see i2c-designware-amdpsp as an example).

Signed-off-by: Jan Dabros <jsd@semihalf.com>
---
 drivers/char/tpm/tpm_tis_i2c_cr50.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis_i2c_cr50.c b/drivers/char/tpm/tpm_tis_i2c_cr50.c
index eed1c296a00c..80b0f41ffb5f 100644
--- a/drivers/char/tpm/tpm_tis_i2c_cr50.c
+++ b/drivers/char/tpm/tpm_tis_i2c_cr50.c
@@ -201,8 +201,6 @@ static int tpm_cr50_i2c_read(struct tpm_chip *chip, u8 addr, u8 *buffer, size_t
 	};
 	int rc;
 
-	i2c_lock_bus(client->adapter, I2C_LOCK_SEGMENT);
-
 	/* Prepare for completion interrupt */
 	tpm_cr50_i2c_enable_tpm_irq(chip);
 
@@ -221,7 +219,6 @@ static int tpm_cr50_i2c_read(struct tpm_chip *chip, u8 addr, u8 *buffer, size_t
 
 out:
 	tpm_cr50_i2c_disable_tpm_irq(chip);
-	i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT);
 
 	if (rc < 0)
 		return rc;
@@ -263,8 +260,6 @@ static int tpm_cr50_i2c_write(struct tpm_chip *chip, u8 addr, u8 *buffer,
 	priv->buf[0] = addr;
 	memcpy(priv->buf + 1, buffer, len);
 
-	i2c_lock_bus(client->adapter, I2C_LOCK_SEGMENT);
-
 	/* Prepare for completion interrupt */
 	tpm_cr50_i2c_enable_tpm_irq(chip);
 
@@ -278,7 +273,6 @@ static int tpm_cr50_i2c_write(struct tpm_chip *chip, u8 addr, u8 *buffer,
 
 out:
 	tpm_cr50_i2c_disable_tpm_irq(chip);
-	i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT);
 
 	if (rc < 0)
 		return rc;
@@ -322,6 +316,7 @@ static int tpm_cr50_check_locality(struct tpm_chip *chip, int loc)
  */
 static int tpm_cr50_release_locality(struct tpm_chip *chip, int loc)
 {
+	struct i2c_client *client = to_i2c_client(chip->dev.parent);
 	u8 mask = TPM_ACCESS_VALID | TPM_ACCESS_REQUEST_PENDING;
 	u8 addr = TPM_I2C_ACCESS(loc);
 	u8 buf;
@@ -329,13 +324,15 @@ static int tpm_cr50_release_locality(struct tpm_chip *chip, int loc)
 
 	rc = tpm_cr50_i2c_read(chip, addr, &buf, sizeof(buf));
 	if (rc < 0)
-		return rc;
+		goto unlock_out;
 
 	if ((buf & mask) == mask) {
 		buf = TPM_ACCESS_ACTIVE_LOCALITY;
 		rc = tpm_cr50_i2c_write(chip, addr, &buf, sizeof(buf));
 	}
 
+unlock_out:
+	i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT);
 	return rc;
 }
 
@@ -350,16 +347,19 @@ static int tpm_cr50_release_locality(struct tpm_chip *chip, int loc)
  */
 static int tpm_cr50_request_locality(struct tpm_chip *chip, int loc)
 {
+	struct i2c_client *client = to_i2c_client(chip->dev.parent);
 	u8 buf = TPM_ACCESS_REQUEST_USE;
 	unsigned long stop;
 	int rc;
 
+	i2c_lock_bus(client->adapter, I2C_LOCK_SEGMENT);
+
 	if (tpm_cr50_check_locality(chip, loc) == loc)
 		return loc;
 
 	rc = tpm_cr50_i2c_write(chip, TPM_I2C_ACCESS(loc), &buf, sizeof(buf));
 	if (rc < 0)
-		return rc;
+		goto unlock_out;
 
 	stop = jiffies + chip->timeout_a;
 	do {
@@ -369,7 +369,11 @@ static int tpm_cr50_request_locality(struct tpm_chip *chip, int loc)
 		msleep(TPM_CR50_TIMEOUT_SHORT_MS);
 	} while (time_before(jiffies, stop));
 
-	return -ETIMEDOUT;
+	rc = -ETIMEDOUT;
+
+unlock_out:
+	i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT);
+	return rc;
 }
 
 /**
-- 
2.47.0.rc0.187.ge670bccf7e-goog

Re: [PATCH V3 0/2] char: tpm: Adjust cr50_i2c locking mechanism

[Grzegorz Bernacki]

Jarkko,

Could you check if there is any additional work needed for these patches?
Please let me if you have any more comments, which can block it from being
merged.

thanks,
greg

Re: [PATCH V3 0/2] char: tpm: Adjust cr50_i2c locking mechanism

[Jarkko Sakkinen]

On Mon Oct 28, 2024 at 11:09 AM EET, Grzegorz Bernacki wrote:
> Jarkko,
>
> Could you check if there is any additional work needed for these patches?
> Please let me if you have any more comments, which can block it from being
> merged.
>
> thanks,
> greg

I got confused because they were in the same email thread, thus I just
saw my review comments in lore. I.e. I mixed v1 comments to v3.

ERROR: Remove Gerrit Change-Id's before submitting upstream
#11:
Change-Id: Iee45ab1cbb2f4d898fbac12e9632de27f615bfba

I'll remove it and apply.

BR, Jarkko