mbox series

[v12,0/9] Add tpm i2c ptp driver

Message ID 20200625144650.269719-1-amirmizi6@gmail.com
Headers show
Series Add tpm i2c ptp driver | expand

Message

Amir Mizinski June 25, 2020, 2:46 p.m. UTC 
From: Amir Mizinski <amirmizi6@gmail.com>

This patch set adds support for TPM devices that implement the I2C.
Interface defined by TCG PTP specification:
https://trustedcomputinggroup.org/wp-content/uploads/TCG_PC_Client_Platform_TPM_Profile_PTP_2.0_r1.03_v22.pdf

The driver was tested on Raspberry-Pie 3, using Nuvoton NPCT75X TPM.

Interrupts are not implemented yet, preparing it for the next patch.
This patch is based on initial work by oshri Alkoby, Alexander Steffen and Christophe Ricard

Changes since version 1:
-"char:tpm:Add check_data handle to tpm_tis_phy_ops in order to check data integrity"
        - Fixed and extended commit description.
        - Fixed an issue regarding handling max retries.
-"dt-bindings: tpm: Add YAML schema for TPM TIS I2C options":
        -Converted "tpm_tis_i2c.txt" to "tpm-tis-i2c.yaml".
        - Renamed "tpm_tis-i2c" to "tpm-tis-i2c".
        - Removed interrupts properties.
-"char: tpm: add tpm_tis_i2c driver"
        - Replaced "tpm_tis-i2c" with "tpm-tis-i2c" in "tpm_tis_i2c.c".
Addressed comments from:
 - Jarkko Sakkinen: https://patchwork.kernel.org/patch/11236257/
 - Rob Herring: https://patchwork.kernel.org/patch/11236253/

Changes since version 2:
- Added 2 new commits with improvements suggested by Benoit Houyere.
        -"Fix expected bit handling and send all bytes in one shot without last byte in exception"
        -"Handle an exception for TPM Firmware Update mode."
- Updated patch to latest v5.5
-"dt-bindings: tpm: Add YAML schema for TPM TIS I2C options"
        - Added "interrupts" and "crc-checksum" to properties.
        - Updated binding description and commit info.
-"char: tpm: add tpm_tis_i2c driver" (suggested by Benoit Houyere)
        - Added repeat I2C frame after NACK.
        - Checksum I2C feature activation in DTS file configuration.
Addressed comments from:
 - Rob Herring: https://lore.kernel.org/patchwork/patch/1161287/

Changes since version 3:
- Updated patch to latest v5.6
- Updated commits headlines and development credit format by Jarkko Sakkinen suggestion
-"tpm: tpm_tis: Make implementation of read16 read32 write32 optional"
        - Updated commit description.
-"dt-bindings: tpm: Add YAML schema for TPM TIS I2C options"
        - Fixed 'make dt_binding_check' errors on YAML file.
        - Removed interrupts from required and examples since there is no use for them in current patch.
Addressed comments from:
 - Jarkko Sakkinen: https://lore.kernel.org/patchwork/patch/1192101/
 - Rob Herring: https://lore.kernel.org/patchwork/patch/1192099/

Changes since version 4:
-"tpm: tpm_tis: Make implementation of read16 read32 write32 optional"
        -Added a "Reviewed-by" tag:
-"tpm: tpm_tis: Add check_data handle to tpm_tis_phy_ops in order to check data integrity"
        -Fixed credit typos.
-"tpm: tpm_tis: rewrite "tpm_tis_req_canceled()""
        -Added fixes tag and removed changes for STM.
-"tpm: tpm_tis: Fix expected bit handling and send all bytes in one shot without last byte in exception"
        -Fixed typos, edited description to be clearer, and added a "Suggested-by" tag.
-"tpm: Handle an exception for TPM Firmware Update mode."
        -Added a "Suggested-by" tag.
-"dt-bindings: tpm: Add YAML schema for TPM TIS I2C options"
        -Fixed 'make dt_binding_check' errors.
-"tpm: tpm_tis: add tpm_tis_i2c driver"
        -Added tested-by tag by Eddie James.
        -Fixed indent in Kconfig file.
        -Fixed 'MODULE_DESCRIPTION'.
Addressed comments from:
 - Jarkko Sakkinen: https://patchwork.kernel.org/patch/11467645/
                https://patchwork.kernel.org/patch/11467655/
                https://patchwork.kernel.org/patch/11467643/
                https://patchwork.kernel.org/patch/11467659/
                https://patchwork.kernel.org/patch/11467651/
 - Rob Herring: https://patchwork.kernel.org/patch/11467653/
 - Randy Dunlap: https://patchwork.kernel.org/patch/11467651/
 - Eddie James: https://lore.kernel.org/patchwork/patch/1192104/

Changes since version 5:
-"tpm: tpm_tis: Add check_data handle to tpm_tis_phy_ops"
        -Updated short description and fixed long description to be more clear.
Addressed comments from:
 - Jarkko Sakkinen: https://lkml.org/lkml/2020/4/6/748

Changes since version 6:
-"tpm: tpm_tis: Make implementation of read16, read32 and write32 optional"
        -Fixed short description.
        -fixed long description proofreading issues.
-"tpm: tpm_tis: Add check_data handle to tpm_tis_phy_ops"
        -Fixed long description by Jarkko comments and proofreading issues.
        -Replaced "check_data" with verify_data_integrity".
        -New line before return statement.
-"tpm: tpm_tis: rewrite "tpm_tis_req_canceled()"
        -Fixed line over 80 characters.
        -fixed long description proofreading issues.
-" tpm: tpm_tis: Fix expected bit handling and send all bytes in one shot"
        -fixed long description proofreading issues.
-"dt-bindings: tpm: Add YAML schema for TPM TIS I2C option"
        -Replaced "tpm-tis-i2c@2e" with "tpm_tis@2e".
        -Fixed CRC_Checksum description.
-"tpm: tpm_tis: add tpm_tis_i2c driver"
        -Replaced "depends on CRC_CCIT" with "select CRC_CCIT".
        -Added tested-by tag by Joel Stanley.
        -Fixed checkpatch.pl warnings.
Addressed comments from:
Addressed comments from:
 - Jarkko Sakkinen:
        https://lore.kernel.org/patchwork/patch/1221336/
        https://lore.kernel.org/patchwork/patch/1221337/
        https://lore.kernel.org/patchwork/patch/1221339/
 - Joel Stanley:
        https://lore.kernel.org/patchwork/patch/1220543/
 - Rob Herring:
        https://lore.kernel.org/patchwork/patch/1221334/

Changes since version 7:
- Added a new commit with improvements suggested by Benoit Houyere.
        -"tpm: tpm_tis: verify TPM_STS register is valid after locality request"
-"tpm: tpm_tis: Rewrite "tpm_tis_req_canceled()""
        -Fixed Hash for Fixes tag.
-"tpm: Add YAML schema for TPM TIS I2C options"
        -Added a compatible string specific to the nuvoton npct75x chip.
-"tpm: tpm_tis: add tpm_tis_i2c driver"
        -added a compatible string according to yaml file.
Addressed comments from:
 - Jarkko Sakkinen:
        https://lore.kernel.org/patchwork/patch/1231524/
 - Rob Herring:
        https://lore.kernel.org/patchwork/patch/1231526/

Changes since version 8:
- "tpm: tpm_tis: Make implementation of read16, read32 and write32 optional"
        -Fixed a compile error conflicting CR50
- "tpm: tpm_tis: Fix expected bit handling and send all bytes in one shot without last byte in exception"
        -Moved commit backwards from 4/8 to 2/8 for a better flow with new data integrity check design
- "tpm: tpm_tis: Add retry in case of protocol failure or data integrity (on I2C only) failure."
        -Renamed from "tpm: tpm_tis: Add check_data handle to tpm_tis_phy_ops"
        -Redesign and added a retry for additional error cases.
- "tpm: Add YAML schema for TPM TIS I2C options"
        -Fixed Dual-license new binding
        -Removed "oneOf"
        -Fixed tpm_tis@2e to tpm@2e
Addressed comments from:
 - Jarkko Sakkinen:
        https://lore.kernel.org/patchwork/patch/1240728/
        https://lore.kernel.org/patchwork/patch/1240736/
 - Rob Herring:
        https://lore.kernel.org/patchwork/patch/1240733/

Changes since version 9:
- "tpm: Make read{16, 32}() and write32() in tpm_tis_phy_ops optional"
        -Fixed short description
- "tpm: tpm_tis: Fix expected bit handling and send all bytes in one shot without last byte in exception"
        -Canceled wait_for_tpm_stat() function renaming.
        -Fixed long description
- "tpm: Add YAML schema for TPM TIS I2C options"
        -Added a reviewed-by tag.
Addressed comments from:
 - Jarkko Sakkinen:
        https://lore.kernel.org/patchwork/patch/1247163/
        https://lore.kernel.org/patchwork/patch/1247164/
 - Rob Herring:
        https://lore.kernel.org/patchwork/patch/1247161/

Changes since version 10:
- "tpm: Make read{16, 32}() and write32() in tpm_tis_phy_ops optional"
        -Added a Reviewed-by and Tested-by tags
- "tpm: tpm_tis: Fix expected bit handling and send all bytes in one shot without last byte in exception"
        -Renamed "mask_result" parameter with "stat"
- "tpm: tpm_tis: Add retry in case of protocol failure or data integrity (on I2C only) failure."
        -Edited long description.
        -Modified tpm_tis_recv() to __tpm_tis_recv() and Introduced a new tpm_tis_recv() function
Addressed comments from:
 - Jarkko Sakkinen:
        https://lore.kernel.org/patchwork/patch/1252428/
        https://lore.kernel.org/patchwork/patch/1252422/
        https://lore.kernel.org/patchwork/patch/1252424/

Cahnges since version 11:
- "tpm: tpm_tis: Fix expected bit handling and send all bytes in one shot without last byte in exception"
        -Added a "Reviewed-by" tag
        -Renamed 'wait_for_tpm_stat()' function with 'tpm_tis_wait_for_stat()'
- "tpm: tpm_tis: Add retry in case of protocol failure."
        -Removed data integrity check and created a new commit for it.
        -Edited short and long description.
- "tpm: tpm_tis: Add verify_data_integrity handle to tpm_tis_phy_ops"
        -This is a new commit.
Addressed comments from:
 - Jarkko Sakkinen:
        https://lore.kernel.org/patchwork/patch/1258107/
        https://lore.kernel.org/patchwork/patch/1258110/

Amir Mizinski (9):
  tpm: Make read{16, 32}() and write32() in tpm_tis_phy_ops optional
  tpm: tpm_tis: Fix expected bit handling and send all bytes in one shot
    without last byte in exception
  tpm: tpm_tis: Add retry in case of protocol failure.
  tpm: tpm_tis: Add verify_data_integrity handle to tpm_tis_phy_ops
  tpm: tpm_tis: Rewrite "tpm_tis_req_canceled()"
  tpm: Handle an exception for TPM Firmware Update mode.
  tpm: tpm_tis: verify TPM_STS register is valid after locality request
  tpm: Add YAML schema for TPM TIS I2C options
  tpm: tpm_tis: add tpm_tis_i2c driver

 .../bindings/security/tpm/tpm-tis-i2c.yaml         |  50 ++++
 drivers/char/tpm/Kconfig                           |  12 +
 drivers/char/tpm/Makefile                          |   1 +
 drivers/char/tpm/tpm2-cmd.c                        |   4 +
 drivers/char/tpm/tpm_tis_core.c                    | 158 ++++++-----
 drivers/char/tpm/tpm_tis_core.h                    |  41 ++-
 drivers/char/tpm/tpm_tis_i2c.c                     | 292 +++++++++++++++++++++
 drivers/char/tpm/tpm_tis_spi.h                     |   4 -
 drivers/char/tpm/tpm_tis_spi_cr50.c                |   3 -
 drivers/char/tpm/tpm_tis_spi_main.c                |  41 ---
 include/linux/tpm.h                                |   1 +
 11 files changed, 486 insertions(+), 121 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/security/tpm/tpm-tis-i2c.yaml
 create mode 100644 drivers/char/tpm/tpm_tis_i2c.c

Comments

Jarkko Sakkinen June 25, 2020, 10 p.m. UTC | #1 
On Thu, Jun 25, 2020 at 05:46:43PM +0300, amirmizi6@gmail.com wrote:
> From: Amir Mizinski <amirmizi6@gmail.com>
> 
> Added a retry mechanism on any protocol error when sending a command or
> receiving a response. This was changed to comply with
> the TCG PC Client Device Driver Design Principles for TPM 2.0.

We don't have to comply with this.

> When sending a command, a loop is added in "tpm_tis_send_main()" that calls
> "tpm_tis_send_data()" and then issues a retry attempt if any error occurs.
> When receiving a response, the following new function derived from
> "tpm_tis_recv()" is added: "__tpm_tis_recv()". "tpm_tis_recv()" is modified
> to call "__tpm_tis_recv()" in a loop. If any error occurs, a retry is
> initiated by setting TPM_STS.responseRetry and "__tpm_tis_recv()" is
> called again.
> In both cases a retry is attempted up to "TPM_RETRY" times.
> 
> Co-developed-by: Christophe Ricard <christophe-h.ricard@st.com>
> Signed-off-by: Christophe Ricard <christophe-h.ricard@st.com>
> Signed-off-by: Amir Mizinski <amirmizi6@gmail.com>
> ---
>  drivers/char/tpm/tpm_tis_core.c | 62 ++++++++++++++++++++++++++---------------
>  drivers/char/tpm/tpm_tis_core.h |  1 +
>  2 files changed, 41 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index 6b33620..e136467 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -305,29 +305,24 @@ static int recv_data(struct tpm_chip *chip, u8 *buf, size_t count)
>  	return size;
>  }
>  
> -static int tpm_tis_recv(struct tpm_chip *chip, u8 *buf, size_t count)
> +static int __tpm_tis_recv(struct tpm_chip *chip, u8 *buf, size_t count)
>  {
>  	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
>  	int size = 0;
>  	int status;
>  	u32 expected;
>  
> -	if (count < TPM_HEADER_SIZE) {
> -		size = -EIO;
> -		goto out;
> -	}
> -
>  	size = recv_data(chip, buf, TPM_HEADER_SIZE);
>  	/* read first 10 bytes, including tag, paramsize, and result */
>  	if (size < TPM_HEADER_SIZE) {
>  		dev_err(&chip->dev, "Unable to read header\n");
> -		goto out;
> +		return size;
>  	}
>  
>  	expected = be32_to_cpu(*(__be32 *) (buf + 2));
>  	if (expected > count || expected < TPM_HEADER_SIZE) {
>  		size = -EIO;
> -		goto out;
> +		return size;
>  	}
>  
>  	size += recv_data(chip, &buf[TPM_HEADER_SIZE],
> @@ -335,22 +330,45 @@ static int tpm_tis_recv(struct tpm_chip *chip, u8 *buf, size_t count)
>  	if (size < expected) {
>  		dev_err(&chip->dev, "Unable to read remainder of result\n");
>  		size = -ETIME;
> -		goto out;
> +		return size;
>  	}
>  
>  	if (tpm_tis_wait_for_stat(chip, TPM_STS_VALID, TPM_STS_VALID,
>  				  chip->timeout_c, &priv->int_queue,
>  				  false) < 0) {
>  		size = -ETIME;
> -		goto out;
> +		return size;
>  	}
> +
>  	status = tpm_tis_status(chip);
>  	if (status & TPM_STS_DATA_AVAIL) {	/* retry? */
>  		dev_err(&chip->dev, "Error left over data\n");
>  		size = -EIO;
> +		return size;
> +	}
> +
> +	return size;
> +}
> +
> +static int tpm_tis_recv(struct tpm_chip *chip, u8 *buf, size_t count)
> +{
> +	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
> +	int size;
> +	int i;
> +
> +	if (count < TPM_HEADER_SIZE) {
> +		size = -EIO;
>  		goto out;
>  	}
>  
> +	for (i = 0; i < TPM_RETRY; i++) {
> +		size = __tpm_tis_recv(chip, buf, count);
> +		if (size <= 0)
> +			tpm_tis_write8(priv, TPM_STS(priv->locality),
> +				       TPM_STS_RESPONSE_RETRY);
> +		else
> +			goto out;
> +	}
>  out:
>  	tpm_tis_ready(chip);
>  	return size;
> @@ -375,7 +393,7 @@ static int tpm_tis_send_data(struct tpm_chip *chip, const u8 *buf, size_t len)
>  					  chip->timeout_b, &priv->int_queue,
>  					  false) < 0) {
>  			rc = -ETIME;
> -			goto out_err;
> +			return rc;
>  		}
>  	}
>  
> @@ -384,13 +402,13 @@ static int tpm_tis_send_data(struct tpm_chip *chip, const u8 *buf, size_t len)
>  		if (burstcnt < 0) {
>  			dev_err(&chip->dev, "Unable to read burstcount\n");
>  			rc = burstcnt;
> -			goto out_err;
> +			return rc;
>  		}
>  		burstcnt = min_t(int, burstcnt, len - count);
>  		rc = tpm_tis_write_bytes(priv, TPM_DATA_FIFO(priv->locality),
>  					 burstcnt, buf + count);
>  		if (rc < 0)
> -			goto out_err;
> +			return rc;
>  
>  		count += burstcnt;
>  	}
> @@ -398,14 +416,10 @@ static int tpm_tis_send_data(struct tpm_chip *chip, const u8 *buf, size_t len)
>  				  TPM_STS_VALID, chip->timeout_a,
>  				  &priv->int_queue, false) < 0) {
>  		rc = -ETIME;
> -		goto out_err;
> +		return rc;
>  	}
>  
>  	return 0;
> -
> -out_err:
> -	tpm_tis_ready(chip);
> -	return rc;
>  }
>  
>  static void disable_interrupts(struct tpm_chip *chip)
> @@ -434,13 +448,17 @@ static void disable_interrupts(struct tpm_chip *chip)
>  static int tpm_tis_send_main(struct tpm_chip *chip, const u8 *buf, size_t len)
>  {
>  	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
> -	int rc;
> +	int rc, i;
>  	u32 ordinal;
>  	unsigned long dur;
>  
> -	rc = tpm_tis_send_data(chip, buf, len);
> -	if (rc < 0)
> -		return rc;
> +	for (i = 0; i < TPM_RETRY; i++) {
> +		rc = tpm_tis_send_data(chip, buf, len);
> +		if (rc >= 0)
> +			break;
> +	}
> +	if (i == TPM_RETRY)
> +		goto out_err;
>  
>  	/* go and do it */
>  	rc = tpm_tis_write8(priv, TPM_STS(priv->locality), TPM_STS_GO);
> diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h
> index d06c65b..6cc6b76 100644
> --- a/drivers/char/tpm/tpm_tis_core.h
> +++ b/drivers/char/tpm/tpm_tis_core.h
> @@ -34,6 +34,7 @@ enum tis_status {
>  	TPM_STS_GO = 0x20,
>  	TPM_STS_DATA_AVAIL = 0x10,
>  	TPM_STS_DATA_EXPECT = 0x08,
> +	TPM_STS_RESPONSE_RETRY = 0x02,
>  };
>  
>  enum tis_int_flags {
> -- 
> 2.7.4
> 

Just based on the explanation this looks like a bunch of unconsidered
changes. You have explain yourself better.

/Jarkko
Jarkko Sakkinen June 25, 2020, 10:02 p.m. UTC | #2 
On Thu, Jun 25, 2020 at 05:46:44PM +0300, amirmizi6@gmail.com wrote:
> From: Amir Mizinski <amirmizi6@gmail.com>
> 
> When using I2C bus protocol, the TPM has the ability to report data
> integrity on incoming or outgoing command parameter bytes.
> According to the TCG specs, if this data validation functionality is
> enabled via the TPM_DATA_CSUM_ENABLE register, the TPM will update the
> TPM_DATA_CSUM register after reception of the last command byte and after
> the last response byte has been read.
> 
> Data integrity is checked if a "verify_data_integrity" handle is defined in
> "tpm_tis_phy_ops".
> 
> Co-developed-by: Christophe Ricard <christophe-h.ricard@st.com>
> Signed-off-by: Christophe Ricard <christophe-h.ricard@st.com>
> Signed-off-by: Amir Mizinski <amirmizi6@gmail.com>
> ---
>  drivers/char/tpm/tpm_tis_core.c | 14 ++++++++++++++
>  drivers/char/tpm/tpm_tis_core.h |  2 ++
>  2 files changed, 16 insertions(+)
> 
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index e136467..347c020 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -347,6 +347,13 @@ static int __tpm_tis_recv(struct tpm_chip *chip, u8 *buf, size_t count)
>  		return size;
>  	}
>  
> +	if (priv->phy_ops->verify_data_integrity)
> +		if (!priv->phy_ops->verify_data_integrity(priv, buf,
> +							  size)) {
> +			size = -EIO;
> +			return size;
> +		}
> +
>  	return size;
>  }
>  
> @@ -419,6 +426,13 @@ static int tpm_tis_send_data(struct tpm_chip *chip, const u8 *buf, size_t len)
>  		return rc;
>  	}
>  
> +	if (priv->phy_ops->verify_data_integrity) {
> +		if (!priv->phy_ops->verify_data_integrity(priv, buf, len)) {
> +			rc = -EIO;
> +			return rc;
> +		}
> +	}
> +
>  	return 0;
>  }
>  
> diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h
> index 6cc6b76..cd97c01 100644
> --- a/drivers/char/tpm/tpm_tis_core.h
> +++ b/drivers/char/tpm/tpm_tis_core.h
> @@ -107,6 +107,8 @@ struct tpm_tis_phy_ops {
>  	int (*read16)(struct tpm_tis_data *data, u32 addr, u16 *result);
>  	int (*read32)(struct tpm_tis_data *data, u32 addr, u32 *result);
>  	int (*write32)(struct tpm_tis_data *data, u32 addr, u32 src);
> +	bool (*verify_data_integrity)(struct tpm_tis_data *data, const u8 *buf,
> +				      size_t len);
>  };
>  
>  static inline int tpm_tis_read_bytes(struct tpm_tis_data *data, u32 addr,
> -- 
> 2.7.4
> 

As I've said before, I'm not too eager to add a new callback and nothing
in the commit message rationalizes adding one.

/Jarkko