diff mbox series

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

Message ID 20221103145450.1409273-4-jsd@semihalf.com (mailing list archive)
State New, archived
Headers show
Series char: tpm: Adjust cr50_i2c locking mechanism | expand

Commit Message

Jan Dąbroś Nov. 3, 2022, 2:54 p.m. UTC 
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(-)

Comments

Jarkko Sakkinen Nov. 7, 2022, 9:22 a.m. UTC | #1 
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
Jan Dąbroś Nov. 7, 2022, 9:47 a.m. UTC | #2 
> 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
Jarkko Sakkinen Nov. 7, 2022, 4:35 p.m. UTC | #3 
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
diff mbox series

Patch

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;
 }
 
 /**