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

[PATCH v2 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.

v1->v2:
* Get back with tpm_cr50_check_locality to return 0 instead of locality
* Return from functions which get (locality < 0)

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 | 126 ++++++++++++++++++----------
 2 files changed, 85 insertions(+), 46 deletions(-)

-- 
2.38.1.273.g43a17bfeac-goog

[PATCH v2 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 v2 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 | 106 ++++++++++++++++++----------
 1 file changed, 70 insertions(+), 36 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis_i2c_cr50.c b/drivers/char/tpm/tpm_tis_i2c_cr50.c
index 77cea5b31c6e4..517d8410d7da0 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,20 +288,21 @@ 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.
  * - -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;
 
@@ -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))
+		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))
+			return loc;
 
 		msleep(TPM_CR50_TIMEOUT_SHORT_MS);
 	} while (time_before(jiffies, stop));
@@ -374,7 +386,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){
+		WARN_ONCE(1, "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];
@@ -390,7 +407,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) {
+		WARN_ONCE(1, "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);
 }
 
@@ -420,7 +442,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 +476,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) {
+		WARN_ONCE(1, "Incorrect tpm locality value\n");
+		return -EINVAL;
+	}
+
 	if (buf_len < TPM_HEADER_SIZE)
 		return -EINVAL;
 
@@ -516,7 +543,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 +550,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 +571,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) {
+		WARN_ONCE(1, "Incorrect tpm locality value\n");
+		return -EINVAL;
+	}
 
 	/* Wait until TPM is ready for a command */
 	stop = jiffies + chip->timeout_b;
@@ -578,7 +604,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 +626,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 +639,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 +677,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 +714,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 +757,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 +809,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 v2 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 517d8410d7da0..f8a67fc2382cc 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))
 		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 v2 1/3] char: tpm: Protect tpm_pm_suspend with locks

[Jarkko Sakkinen]

On Thu, Nov 03, 2022 at 03:54:48PM +0100, Jan Dabros wrote:
> 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>

AFAIK processes are freezed before suspend callbacks are called, and
the callbacks are called sequentially. I have no idea what is meant
by "TPM accessor" here.

Please describe the concurrency scenario in the commit message where the
race could happen, if it is hard to reproduce, and add an appropriate fixes
tag.

BR, Jarkko

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

[Jan Dąbroś]

niedz., 6 lis 2022 o 20:49 Jarkko Sakkinen <jarkko@kernel.org> napisał(a):
>
> On Thu, Nov 03, 2022 at 03:54:48PM +0100, Jan Dabros wrote:
> > 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>
>
> AFAIK processes are freezed before suspend callbacks are called, and
> the callbacks are called sequentially. I have no idea what is meant
> by "TPM accessor" here.

User space processes are freezed before suspend, but kernel threads
are not freezable by default. In my particular case it was a hwrng
thread started from drivers/char/hw_random/core.c - I was referring to
it as "TPM accessor". For sure I should be more precise in a commit
msg.

> Please describe the concurrency scenario in the commit message where the
> race could happen, if it is hard to reproduce, and add an appropriate fixes
> tag.

I will describe my scenario in more detail in the next version.
Regarding the "fixes" tag - I'm not too familiar with it, but looking
at the kernel submission guide, "fixes" should be used either when
there was a particular commit in the past which introduced the bug or
if a patch fixes an already logged bug entry (so that one can paste
URL). In my case both are not applicable, so please advise what
exactly I should put after this tag?

Best Regards,
Jan

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

[Jarkko Sakkinen]

On Thu, Nov 03, 2022 at 03:54:49PM +0100, Jan Dabros wrote:
> 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>

I get that architecturally using the standard callbacks is a good idea.
Still, you should explicitly document the gain because the existing code
is working and field tested.

> ---
>  drivers/char/tpm/tpm_tis_i2c_cr50.c | 106 ++++++++++++++++++----------
>  1 file changed, 70 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_tis_i2c_cr50.c b/drivers/char/tpm/tpm_tis_i2c_cr50.c
> index 77cea5b31c6e4..517d8410d7da0 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,20 +288,21 @@ 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.
>   * - -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;
>  
> @@ -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))
> +		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))
> +			return loc;
>  
>  		msleep(TPM_CR50_TIMEOUT_SHORT_MS);
>  	} while (time_before(jiffies, stop));
> @@ -374,7 +386,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){
> +		WARN_ONCE(1, "Incorrect tpm locality value\n");

Never ever add WARN() for a success case. It can ultimately crash the whole
system, if panic_on_warn is enabled.

Since this is a success case, judging from the return value, at most you
should use pr_info() here.

> +		return 0;
> +	}
> +
> +	if (tpm_cr50_i2c_read(chip, TPM_I2C_STS(chip->locality), buf, sizeof(buf)) < 0)
>  		return 0;
>  
>  	return buf[0];
> @@ -390,7 +407,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) {
> +		WARN_ONCE(1, "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);
>  }
>  
> @@ -420,7 +442,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 +476,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) {
> +		WARN_ONCE(1, "Incorrect tpm locality value\n");
> +		return -EINVAL;
> +	}
> +
>  	if (buf_len < TPM_HEADER_SIZE)
>  		return -EINVAL;
>  
> @@ -516,7 +543,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 +550,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 +571,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) {
> +		WARN_ONCE(1, "Incorrect tpm locality value\n");
> +		return -EINVAL;
> +	}
>  
>  	/* Wait until TPM is ready for a command */
>  	stop = jiffies + chip->timeout_b;
> @@ -578,7 +604,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 +626,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 +639,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 +677,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 +714,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 +757,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 +809,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
> 

BR, Jarkko

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

[Jarkko Sakkinen]

On Thu, Nov 03, 2022 at 03:54:50PM +0100, Jan Dabros wrote:
> 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).

Nit: s/CPUs/peripherals/ ? I'm not sure why you want to emphasize external
chips having CPU.

> 
> 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 517d8410d7da0..f8a67fc2382cc 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))
>  		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
> 

BR, Jarkko

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

[Jan Dąbroś]

> On Thu, Nov 03, 2022 at 03:54:49PM +0100, Jan Dabros wrote:
> > 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>
>
> I get that architecturally using the standard callbacks is a good idea.
> Still, you should explicitly document the gain because the existing code
> is working and field tested.

ACK. I will mention here about the overall idea I have.

>
> > ---
> >  drivers/char/tpm/tpm_tis_i2c_cr50.c | 106 ++++++++++++++++++----------
> >  1 file changed, 70 insertions(+), 36 deletions(-)
> >
> > diff --git a/drivers/char/tpm/tpm_tis_i2c_cr50.c b/drivers/char/tpm/tpm_tis_i2c_cr50.c
> > index 77cea5b31c6e4..517d8410d7da0 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,20 +288,21 @@ 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.
> >   * - -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;
> >
> > @@ -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))
> > +             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))
> > +                     return loc;
> >
> >               msleep(TPM_CR50_TIMEOUT_SHORT_MS);
> >       } while (time_before(jiffies, stop));
> > @@ -374,7 +386,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){
> > +             WARN_ONCE(1, "Incorrect tpm locality value\n");
>
> Never ever add WARN() for a success case. It can ultimately crash the whole
> system, if panic_on_warn is enabled.
>
> Since this is a success case, judging from the return value, at most you
> should use pr_info() here.

ACK.

Best Regards,
Jan

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

[Jan Dąbroś]

> On Thu, Nov 03, 2022 at 03:54:50PM +0100, Jan Dabros wrote:
> > 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).
>
> Nit: s/CPUs/peripherals/ ? I'm not sure why you want to emphasize external
> chips having CPU.

My point was that we can have a multi-controller setup and each
controller may work with the TPM chip, so let me s/CPUs/controllers/.

Best Regards,
Jan

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

[Jarkko Sakkinen]

On Mon, Nov 07, 2022 at 09:45:41AM +0100, Jan Dąbroś wrote:
> niedz., 6 lis 2022 o 20:49 Jarkko Sakkinen <jarkko@kernel.org> napisał(a):
> >
> > On Thu, Nov 03, 2022 at 03:54:48PM +0100, Jan Dabros wrote:
> > > 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>
> >
> > AFAIK processes are freezed before suspend callbacks are called, and
> > the callbacks are called sequentially. I have no idea what is meant
> > by "TPM accessor" here.
> 
> User space processes are freezed before suspend, but kernel threads
> are not freezable by default. In my particular case it was a hwrng
> thread started from drivers/char/hw_random/core.c - I was referring to
> it as "TPM accessor". For sure I should be more precise in a commit
> msg.

OK, great.

> 
> > Please describe the concurrency scenario in the commit message where the
> > race could happen, if it is hard to reproduce, and add an appropriate fixes
> > tag.
> 
> I will describe my scenario in more detail in the next version.
> Regarding the "fixes" tag - I'm not too familiar with it, but looking
> at the kernel submission guide, "fixes" should be used either when
> there was a particular commit in the past which introduced the bug or
> if a patch fixes an already logged bug entry (so that one can paste
> URL). In my case both are not applicable, so please advise what
> exactly I should put after this tag?

It is 

Fixed: <12 character prefix of the hash> ("short summary")

It should point out to the commit, which introduced the issue/bug.

> Best Regards,
> Jan

BR, Jarkko

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

[Jarkko Sakkinen]

On Mon, Nov 07, 2022 at 10:47:44AM +0100, Jan Dąbroś wrote:
> > On Thu, Nov 03, 2022 at 03:54:50PM +0100, Jan Dabros wrote:
> > > 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).
> >
> > Nit: s/CPUs/peripherals/ ? I'm not sure why you want to emphasize external
> > chips having CPU.
> 
> My point was that we can have a multi-controller setup and each
> controller may work with the TPM chip, so let me s/CPUs/controllers/.

ack

BR, Jarkko

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

[Guenter Roeck]

On Thu, Nov 03, 2022 at 03:54:49PM +0100, Jan Dabros wrote:
> 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>
> ---
[ ... ]

>  #ifdef CONFIG_ACPI
> @@ -686,6 +714,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 +757,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;

As reported by 0-day and Dan Carpenter:

		return loc;

Guenter

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

[Jason A. Donenfeld]

Hi Jarkko,

On Mon, Nov 07, 2022 at 06:35:25PM +0200, Jarkko Sakkinen wrote:
> On Mon, Nov 07, 2022 at 09:45:41AM +0100, Jan Dąbroś wrote:
> > niedz., 6 lis 2022 o 20:49 Jarkko Sakkinen <jarkko@kernel.org> napisał(a):
> > >
> > > On Thu, Nov 03, 2022 at 03:54:48PM +0100, Jan Dabros wrote:
> > > > 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>
> > >
> > > AFAIK processes are freezed before suspend callbacks are called, and
> > > the callbacks are called sequentially. I have no idea what is meant
> > > by "TPM accessor" here.
> > 
> > User space processes are freezed before suspend, but kernel threads
> > are not freezable by default. In my particular case it was a hwrng
> > thread started from drivers/char/hw_random/core.c - I was referring to
> > it as "TPM accessor". For sure I should be more precise in a commit
> > msg.
> 
> OK, great.
> 
> > 
> > > Please describe the concurrency scenario in the commit message where the
> > > race could happen, if it is hard to reproduce, and add an appropriate fixes
> > > tag.
> > 
> > I will describe my scenario in more detail in the next version.
> > Regarding the "fixes" tag - I'm not too familiar with it, but looking
> > at the kernel submission guide, "fixes" should be used either when
> > there was a particular commit in the past which introduced the bug or
> > if a patch fixes an already logged bug entry (so that one can paste
> > URL). In my case both are not applicable, so please advise what
> > exactly I should put after this tag?
> 
> It is 
> 
> Fixed: <12 character prefix of the hash> ("short summary")
> 
> It should point out to the commit, which introduced the issue/bug.

Somebody just reported a bug to me in hwrng stuff, which I traced down
to this lack of tpm_try_get_ops(), and then I coded up the exact same
patch as Jan's here, saw it fixed the problem, and then realized it was
already submitted in that exact form.

Because of some recent hwrng changes, this bug will be triggered more
often than it was before.

So, even though Jan hasn't submitted a v+1, do you think you could take
this 1/3 commit, fix up the commit message or whatever you need there,
and get this in for 6.1-rc8? It's an important fix.

Thanks,
Jason

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

[Jason A. Donenfeld]

On Mon, Nov 28, 2022 at 6:04 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> Hi Jarkko,
>
> On Mon, Nov 07, 2022 at 06:35:25PM +0200, Jarkko Sakkinen wrote:
> > On Mon, Nov 07, 2022 at 09:45:41AM +0100, Jan Dąbroś wrote:
> > > niedz., 6 lis 2022 o 20:49 Jarkko Sakkinen <jarkko@kernel.org> napisał(a):
> > > >
> > > > On Thu, Nov 03, 2022 at 03:54:48PM +0100, Jan Dabros wrote:
> > > > > 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>
> > > >
> > > > AFAIK processes are freezed before suspend callbacks are called, and
> > > > the callbacks are called sequentially. I have no idea what is meant
> > > > by "TPM accessor" here.
> > >
> > > User space processes are freezed before suspend, but kernel threads
> > > are not freezable by default. In my particular case it was a hwrng
> > > thread started from drivers/char/hw_random/core.c - I was referring to
> > > it as "TPM accessor". For sure I should be more precise in a commit
> > > msg.
> >
> > OK, great.
> >
> > >
> > > > Please describe the concurrency scenario in the commit message where the
> > > > race could happen, if it is hard to reproduce, and add an appropriate fixes
> > > > tag.
> > >
> > > I will describe my scenario in more detail in the next version.
> > > Regarding the "fixes" tag - I'm not too familiar with it, but looking
> > > at the kernel submission guide, "fixes" should be used either when
> > > there was a particular commit in the past which introduced the bug or
> > > if a patch fixes an already logged bug entry (so that one can paste
> > > URL). In my case both are not applicable, so please advise what
> > > exactly I should put after this tag?
> >
> > It is
> >
> > Fixed: <12 character prefix of the hash> ("short summary")
> >
> > It should point out to the commit, which introduced the issue/bug.
>
> Somebody just reported a bug to me in hwrng stuff, which I traced down
> to this lack of tpm_try_get_ops(), and then I coded up the exact same
> patch as Jan's here, saw it fixed the problem, and then realized it was
> already submitted in that exact form.
>
> Because of some recent hwrng changes, this bug will be triggered more
> often than it was before.
>
> So, even though Jan hasn't submitted a v+1, do you think you could take
> this 1/3 commit, fix up the commit message or whatever you need there,
> and get this in for 6.1-rc8? It's an important fix.
>
> Thanks,
> Jason

And if it helps, here's some more metadata:

Tested-by: Jason A. Donenfeld <Jason@zx2c4.com>
Reported-by: Vlastimil Babka <vbabka@suse.cz>
Link: https://lore.kernel.org/all/c5ba47ef-393f-1fba-30bd-1230d1b4b592@suse.cz/
Cc: stable@vger.kernel.org

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

[Vlastimil Babka]

On 11/28/22 18:04, Jason A. Donenfeld wrote:
> Hi Jarkko,
> 
> On Mon, Nov 07, 2022 at 06:35:25PM +0200, Jarkko Sakkinen wrote:
>> On Mon, Nov 07, 2022 at 09:45:41AM +0100, Jan Dąbroś wrote:
>> > niedz., 6 lis 2022 o 20:49 Jarkko Sakkinen <jarkko@kernel.org> napisał(a):
>> > >
>> > > On Thu, Nov 03, 2022 at 03:54:48PM +0100, Jan Dabros wrote:
>> > > > 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>
>> > >
>> > > AFAIK processes are freezed before suspend callbacks are called, and
>> > > the callbacks are called sequentially. I have no idea what is meant
>> > > by "TPM accessor" here.
>> > 
>> > User space processes are freezed before suspend, but kernel threads
>> > are not freezable by default. In my particular case it was a hwrng
>> > thread started from drivers/char/hw_random/core.c - I was referring to
>> > it as "TPM accessor". For sure I should be more precise in a commit
>> > msg.
>> 
>> OK, great.
>> 
>> > 
>> > > Please describe the concurrency scenario in the commit message where the
>> > > race could happen, if it is hard to reproduce, and add an appropriate fixes
>> > > tag.
>> > 
>> > I will describe my scenario in more detail in the next version.
>> > Regarding the "fixes" tag - I'm not too familiar with it, but looking
>> > at the kernel submission guide, "fixes" should be used either when
>> > there was a particular commit in the past which introduced the bug or
>> > if a patch fixes an already logged bug entry (so that one can paste
>> > URL). In my case both are not applicable, so please advise what
>> > exactly I should put after this tag?
>> 
>> It is 
>> 
>> Fixed: <12 character prefix of the hash> ("short summary")
>> 
>> It should point out to the commit, which introduced the issue/bug.
> 
> Somebody just reported a bug to me in hwrng stuff, which I traced down
> to this lack of tpm_try_get_ops(), and then I coded up the exact same
> patch as Jan's here, saw it fixed the problem, and then realized it was
> already submitted in that exact form.
> 
> Because of some recent hwrng changes, this bug will be triggered more
> often than it was before.
> 
> So, even though Jan hasn't submitted a v+1, do you think you could take
> this 1/3 commit, fix up the commit message or whatever you need there,
> and get this in for 6.1-rc8? It's an important fix.

For the record, the bug report is here:
https://lore.kernel.org/all/c5ba47ef-393f-1fba-30bd-1230d1b4b592@suse.cz/
(patch author Jan and TPM maintainers were Cc'd)

FWIW I tested the patch 1/3 on top of rc7 and didn't reproduce the issue.
But also had no luck of reproducing it on plain rc7 yet, so it doesn't say
much, unfortunately.

> Thanks,
> Jason

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

[Vlastimil Babka]

On 11/28/22 18:07, Jason A. Donenfeld wrote:
> On Mon, Nov 28, 2022 at 6:04 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>> > >
>> > > > Please describe the concurrency scenario in the commit message where the
>> > > > race could happen, if it is hard to reproduce, and add an appropriate fixes
>> > > > tag.
>> > >
>> > > I will describe my scenario in more detail in the next version.
>> > > Regarding the "fixes" tag - I'm not too familiar with it, but looking
>> > > at the kernel submission guide, "fixes" should be used either when
>> > > there was a particular commit in the past which introduced the bug or
>> > > if a patch fixes an already logged bug entry (so that one can paste
>> > > URL). In my case both are not applicable, so please advise what
>> > > exactly I should put after this tag?
>> >
>> > It is
>> >
>> > Fixed: <12 character prefix of the hash> ("short summary")
>> >
>> > It should point out to the commit, which introduced the issue/bug.
>>
>> Somebody just reported a bug to me in hwrng stuff, which I traced down
>> to this lack of tpm_try_get_ops(), and then I coded up the exact same
>> patch as Jan's here, saw it fixed the problem, and then realized it was
>> already submitted in that exact form.
>>
>> Because of some recent hwrng changes, this bug will be triggered more
>> often than it was before.
>>
>> So, even though Jan hasn't submitted a v+1, do you think you could take
>> this 1/3 commit, fix up the commit message or whatever you need there,
>> and get this in for 6.1-rc8? It's an important fix.
>>
>> Thanks,
>> Jason
> 
> And if it helps, here's some more metadata:
> 
> Tested-by: Jason A. Donenfeld <Jason@zx2c4.com>
> Reported-by: Vlastimil Babka <vbabka@suse.cz>

You can make that:
Reported-and-tested-by: Vlastimil Babka <vbabka@suse.cz>

> Link: https://lore.kernel.org/all/c5ba47ef-393f-1fba-30bd-1230d1b4b592@suse.cz/
> Cc: stable@vger.kernel.org

Not an expert here but looks like a valid Fixes: would be
Fixes: e891db1a18bf ("tpm: turn on TPM on suspend for TPM 1.x")

The 6.1-rc1 commit 3b006c439d58d ("hwrng: core - start hwrng kthread also
for untrusted sources") likely exposed the bug in my case, but it wouldn't
be the real root cause. But it makes it prudent to get the fix to 6.1.

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

[Jason A. Donenfeld]

On Mon, Nov 28, 2022 at 8:46 PM Vlastimil Babka <vbabka@suse.cz> wrote:
>
> On 11/28/22 18:07, Jason A. Donenfeld wrote:
> > On Mon, Nov 28, 2022 at 6:04 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> >> > >
> >> > > > Please describe the concurrency scenario in the commit message where the
> >> > > > race could happen, if it is hard to reproduce, and add an appropriate fixes
> >> > > > tag.
> >> > >
> >> > > I will describe my scenario in more detail in the next version.
> >> > > Regarding the "fixes" tag - I'm not too familiar with it, but looking
> >> > > at the kernel submission guide, "fixes" should be used either when
> >> > > there was a particular commit in the past which introduced the bug or
> >> > > if a patch fixes an already logged bug entry (so that one can paste
> >> > > URL). In my case both are not applicable, so please advise what
> >> > > exactly I should put after this tag?
> >> >
> >> > It is
> >> >
> >> > Fixed: <12 character prefix of the hash> ("short summary")
> >> >
> >> > It should point out to the commit, which introduced the issue/bug.
> >>
> >> Somebody just reported a bug to me in hwrng stuff, which I traced down
> >> to this lack of tpm_try_get_ops(), and then I coded up the exact same
> >> patch as Jan's here, saw it fixed the problem, and then realized it was
> >> already submitted in that exact form.
> >>
> >> Because of some recent hwrng changes, this bug will be triggered more
> >> often than it was before.
> >>
> >> So, even though Jan hasn't submitted a v+1, do you think you could take
> >> this 1/3 commit, fix up the commit message or whatever you need there,
> >> and get this in for 6.1-rc8? It's an important fix.
> >>
> >> Thanks,
> >> Jason
> >
> > And if it helps, here's some more metadata:
> >
> > Tested-by: Jason A. Donenfeld <Jason@zx2c4.com>
> > Reported-by: Vlastimil Babka <vbabka@suse.cz>
>
> You can make that:
> Reported-and-tested-by: Vlastimil Babka <vbabka@suse.cz>
>
> > Link: https://lore.kernel.org/all/c5ba47ef-393f-1fba-30bd-1230d1b4b592@suse.cz/
> > Cc: stable@vger.kernel.org
>
> Not an expert here but looks like a valid Fixes: would be
> Fixes: e891db1a18bf ("tpm: turn on TPM on suspend for TPM 1.x")
>
> The 6.1-rc1 commit 3b006c439d58d ("hwrng: core - start hwrng kthread also
> for untrusted sources") likely exposed the bug in my case, but it wouldn't
> be the real root cause. But it makes it prudent to get the fix to 6.1.

Since this thread went dormant, and we're already pretty close to the
end of the cycle, I'm going to resend this with the commit message
fixed, as Jarkko asked at the beginning of the month.

Jan can then send whatever needs fixing in 2/3 and 3/3 whenever he
gets around to it.

I'll keep the original authorship intact of course.

Jason