linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v6 0/7] Add tpm i2c ptp driver
@ 2020-04-07 16:20 amirmizi6
  2020-04-07 16:20 ` [PATCH v6 1/7] tpm: tpm_tis: Make implementation of read16 read32 write32 optional amirmizi6
                   ` (6 more replies)
  0 siblings, 7 replies; 16+ messages in thread
From: amirmizi6 @ 2020-04-07 16:20 UTC (permalink / raw)
  To: Eyal.Cohen, jarkko.sakkinen, oshrialkoby85, alexander.steffen,
	robh+dt, mark.rutland, peterhuewe, jgg, arnd, gregkh
  Cc: devicetree, linux-kernel, linux-integrity, oshri.alkoby,
	tmaimon77, gcwilson, kgoldman, Dan.Morav, oren.tanami,
	shmulik.hager, amir.mizinski, Amir Mizinski

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 exeption 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 more clear, 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

Amir Mizinski (7):
  tpm: tpm_tis: Make implementation of read16 read32 write32 optional
  tpm: tpm_tis: Add check_data handle to tpm_tis_phy_ops
  tpm: tpm_tis: rewrite "tpm_tis_req_canceled()"
  tpm: tpm_tis: Fix expected bit handling and send all      bytes in one
    shot without last byte in exception
  tpm: Handle an exception for TPM Firmware Update mode.
  dt-bindings: tpm: Add YAML schema for TPM TIS I2C options
  tpm: tpm_tis: add tpm_tis_i2c driver

 .../bindings/security/tpm/tpm-tis-i2c.yaml         |  47 ++++
 drivers/char/tpm/Kconfig                           |  12 +
 drivers/char/tpm/Makefile                          |   1 +
 drivers/char/tpm/tpm2-cmd.c                        |   4 +
 drivers/char/tpm/tpm_tis_core.c                    | 175 ++++++------
 drivers/char/tpm/tpm_tis_core.h                    |  41 ++-
 drivers/char/tpm/tpm_tis_i2c.c                     | 292 +++++++++++++++++++++
 drivers/char/tpm/tpm_tis_spi_main.c                |  41 ---
 include/linux/tpm.h                                |   1 +
 9 files changed, 486 insertions(+), 128 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/security/tpm/tpm-tis-i2c.yaml
 create mode 100644 drivers/char/tpm/tpm_tis_i2c.c

-- 
2.7.4


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

* [PATCH v6 1/7] tpm: tpm_tis: Make implementation of read16 read32 write32 optional
  2020-04-07 16:20 [PATCH v6 0/7] Add tpm i2c ptp driver amirmizi6
@ 2020-04-07 16:20 ` amirmizi6
  2020-04-08 16:46   ` Jarkko Sakkinen
  2020-04-07 16:20 ` [PATCH v6 2/7] tpm: tpm_tis: Add check_data handle to tpm_tis_phy_ops amirmizi6
                   ` (5 subsequent siblings)
  6 siblings, 1 reply; 16+ messages in thread
From: amirmizi6 @ 2020-04-07 16:20 UTC (permalink / raw)
  To: Eyal.Cohen, jarkko.sakkinen, oshrialkoby85, alexander.steffen,
	robh+dt, mark.rutland, peterhuewe, jgg, arnd, gregkh
  Cc: devicetree, linux-kernel, linux-integrity, oshri.alkoby,
	tmaimon77, gcwilson, kgoldman, Dan.Morav, oren.tanami,
	shmulik.hager, amir.mizinski, Amir Mizinski, Alexander Steffen

From: Amir Mizinski <amirmizi6@gmail.com>

Only tpm_tis can use memory mapped I/O, which is truly mapped into
the kernel's memory space. So using ioread16/ioread32/iowrite32 turn into a
straightforward pointer dereference.
Every other driver require more complicated operations to read more than 1
byte at a time and will just fall back to read_bytes/write_bytes.
Therefore, move this common code out of tpm_tis_spi into tpm_tis_core, so
that it is automatically used when low-level drivers do not implement the
specialized methods.

Co-developed-by: Alexander Steffen <Alexander.Steffen@infineon.com>
Signed-off-by: Alexander Steffen <Alexander.Steffen@infineon.com>
Signed-off-by: Amir Mizinski <amirmizi6@gmail.com>
Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 drivers/char/tpm/tpm_tis_core.h     | 38 +++++++++++++++++++++++++++++++---
 drivers/char/tpm/tpm_tis_spi_main.c | 41 -------------------------------------
 2 files changed, 35 insertions(+), 44 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h
index 7337819..d06c65b 100644
--- a/drivers/char/tpm/tpm_tis_core.h
+++ b/drivers/char/tpm/tpm_tis_core.h
@@ -122,13 +122,35 @@ static inline int tpm_tis_read8(struct tpm_tis_data *data, u32 addr, u8 *result)
 static inline int tpm_tis_read16(struct tpm_tis_data *data, u32 addr,
 				 u16 *result)
 {
-	return data->phy_ops->read16(data, addr, result);
+	__le16 result_le;
+	int rc;
+
+	if (data->phy_ops->read16)
+		return data->phy_ops->read16(data, addr, result);
+
+	rc = data->phy_ops->read_bytes(data, addr, sizeof(u16),
+				       (u8 *)&result_le);
+	if (!rc)
+		*result = le16_to_cpu(result_le);
+
+	return rc;
 }
 
 static inline int tpm_tis_read32(struct tpm_tis_data *data, u32 addr,
 				 u32 *result)
 {
-	return data->phy_ops->read32(data, addr, result);
+	__le32 result_le;
+	int rc;
+
+	if (data->phy_ops->read32)
+		return data->phy_ops->read32(data, addr, result);
+
+	rc = data->phy_ops->read_bytes(data, addr, sizeof(u32),
+				       (u8 *)&result_le);
+	if (!rc)
+		*result = le32_to_cpu(result_le);
+
+	return rc;
 }
 
 static inline int tpm_tis_write_bytes(struct tpm_tis_data *data, u32 addr,
@@ -145,7 +167,17 @@ static inline int tpm_tis_write8(struct tpm_tis_data *data, u32 addr, u8 value)
 static inline int tpm_tis_write32(struct tpm_tis_data *data, u32 addr,
 				  u32 value)
 {
-	return data->phy_ops->write32(data, addr, value);
+	__le32 value_le;
+	int rc;
+
+	if (data->phy_ops->write32)
+		return data->phy_ops->write32(data, addr, value);
+
+	value_le = cpu_to_le32(value);
+	rc = data->phy_ops->write_bytes(data, addr, sizeof(u32),
+					(u8 *)&value_le);
+
+	return rc;
 }
 
 static inline bool is_bsw(void)
diff --git a/drivers/char/tpm/tpm_tis_spi_main.c b/drivers/char/tpm/tpm_tis_spi_main.c
index d1754fd..95fef9d 100644
--- a/drivers/char/tpm/tpm_tis_spi_main.c
+++ b/drivers/char/tpm/tpm_tis_spi_main.c
@@ -152,44 +152,6 @@ static int tpm_tis_spi_write_bytes(struct tpm_tis_data *data, u32 addr,
 	return tpm_tis_spi_transfer(data, addr, len, NULL, value);
 }
 
-int tpm_tis_spi_read16(struct tpm_tis_data *data, u32 addr, u16 *result)
-{
-	__le16 result_le;
-	int rc;
-
-	rc = data->phy_ops->read_bytes(data, addr, sizeof(u16),
-				       (u8 *)&result_le);
-	if (!rc)
-		*result = le16_to_cpu(result_le);
-
-	return rc;
-}
-
-int tpm_tis_spi_read32(struct tpm_tis_data *data, u32 addr, u32 *result)
-{
-	__le32 result_le;
-	int rc;
-
-	rc = data->phy_ops->read_bytes(data, addr, sizeof(u32),
-				       (u8 *)&result_le);
-	if (!rc)
-		*result = le32_to_cpu(result_le);
-
-	return rc;
-}
-
-int tpm_tis_spi_write32(struct tpm_tis_data *data, u32 addr, u32 value)
-{
-	__le32 value_le;
-	int rc;
-
-	value_le = cpu_to_le32(value);
-	rc = data->phy_ops->write_bytes(data, addr, sizeof(u32),
-					(u8 *)&value_le);
-
-	return rc;
-}
-
 int tpm_tis_spi_init(struct spi_device *spi, struct tpm_tis_spi_phy *phy,
 		     int irq, const struct tpm_tis_phy_ops *phy_ops)
 {
@@ -205,9 +167,6 @@ int tpm_tis_spi_init(struct spi_device *spi, struct tpm_tis_spi_phy *phy,
 static const struct tpm_tis_phy_ops tpm_spi_phy_ops = {
 	.read_bytes = tpm_tis_spi_read_bytes,
 	.write_bytes = tpm_tis_spi_write_bytes,
-	.read16 = tpm_tis_spi_read16,
-	.read32 = tpm_tis_spi_read32,
-	.write32 = tpm_tis_spi_write32,
 };
 
 static int tpm_tis_spi_probe(struct spi_device *dev)
-- 
2.7.4


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

* [PATCH v6 2/7] tpm: tpm_tis: Add check_data handle to tpm_tis_phy_ops
  2020-04-07 16:20 [PATCH v6 0/7] Add tpm i2c ptp driver amirmizi6
  2020-04-07 16:20 ` [PATCH v6 1/7] tpm: tpm_tis: Make implementation of read16 read32 write32 optional amirmizi6
@ 2020-04-07 16:20 ` amirmizi6
  2020-04-08 18:33   ` Jarkko Sakkinen
  2020-04-07 16:20 ` [PATCH v6 3/7] tpm: tpm_tis: rewrite "tpm_tis_req_canceled()" amirmizi6
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 16+ messages in thread
From: amirmizi6 @ 2020-04-07 16:20 UTC (permalink / raw)
  To: Eyal.Cohen, jarkko.sakkinen, oshrialkoby85, alexander.steffen,
	robh+dt, mark.rutland, peterhuewe, jgg, arnd, gregkh
  Cc: devicetree, linux-kernel, linux-integrity, oshri.alkoby,
	tmaimon77, gcwilson, kgoldman, Dan.Morav, oren.tanami,
	shmulik.hager, amir.mizinski, Amir Mizinski, Christophe Ricard

From: Amir Mizinski <amirmizi6@gmail.com>

In order to validate data integrity we need to compute the crc over the data
sent in lower layer (I2C for instance).
To do that tpm_tis_check_data() calls a "check_data" operation (if available).
If data integrity check fails, a retry to save the sent/received
data is implemented in tpm_tis_send_main()/tpm_tis_recv() functions.

Considering this commit, the following steps are done when sending a command:
   1. Host writes to TPM_STS.commandReady.
   2. Host writes command.
   3. Host checks that TPM received data is valid.
   4. If data is currupted go to step 1.

When receiving data:
   1. Host checks that TPM_STS.dataAvail is set.
   2. Host saves received data.
   3. Host checks that received data is correct.
   4. If data is currupted Host writes to TPM_STS.responseRetry and go to
      step 1.

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 | 102 +++++++++++++++++++++++++---------------
 drivers/char/tpm/tpm_tis_core.h |   3 ++
 2 files changed, 67 insertions(+), 38 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 27c6ca0..6c4f232 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -242,6 +242,15 @@ static u8 tpm_tis_status(struct tpm_chip *chip)
 	return status;
 }
 
+static bool tpm_tis_check_data(struct tpm_chip *chip, const u8 *buf, size_t len)
+{
+	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
+
+	if (priv->phy_ops->check_data)
+		return priv->phy_ops->check_data(priv, buf, len);
+	return true;
+}
+
 static void tpm_tis_ready(struct tpm_chip *chip)
 {
 	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
@@ -308,47 +317,59 @@ 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;
+	int status, i;
 	u32 expected;
+	bool check_data = false;
 
-	if (count < TPM_HEADER_SIZE) {
-		size = -EIO;
-		goto out;
-	}
+	for (i = 0; i < TPM_RETRY; i++) {
+		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;
-	}
+		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;
+		}
 
-	expected = be32_to_cpu(*(__be32 *) (buf + 2));
-	if (expected > count || expected < TPM_HEADER_SIZE) {
-		size = -EIO;
-		goto out;
-	}
+		expected = be32_to_cpu(*(__be32 *) (buf + 2));
+		if (expected > count || expected < TPM_HEADER_SIZE) {
+			size = -EIO;
+			goto out;
+		}
 
-	size += recv_data(chip, &buf[TPM_HEADER_SIZE],
-			  expected - TPM_HEADER_SIZE);
-	if (size < expected) {
-		dev_err(&chip->dev, "Unable to read remainder of result\n");
-		size = -ETIME;
-		goto out;
-	}
+		size += recv_data(chip, &buf[TPM_HEADER_SIZE],
+				  expected - TPM_HEADER_SIZE);
+		if (size < expected) {
+			dev_err(&chip->dev, "Unable to read remainder of result\n");
+			size = -ETIME;
+			goto out;
+		}
 
-	if (wait_for_tpm_stat(chip, TPM_STS_VALID, chip->timeout_c,
-				&priv->int_queue, false) < 0) {
-		size = -ETIME;
-		goto out;
+		if (wait_for_tpm_stat(chip, TPM_STS_VALID, chip->timeout_c,
+				      &priv->int_queue, false) < 0) {
+			size = -ETIME;
+			goto out;
+		}
+
+		status = tpm_tis_status(chip);
+		if (status & TPM_STS_DATA_AVAIL) {	/* retry? */
+			dev_err(&chip->dev, "Error left over data\n");
+			size = -EIO;
+			goto out;
+		}
+
+		check_data = tpm_tis_check_data(chip, buf, size);
+		if (!check_data)
+			tpm_tis_write8(priv, TPM_STS(priv->locality),
+				       TPM_STS_RESPONSE_RETRY);
+		else
+			break;
 	}
-	status = tpm_tis_status(chip);
-	if (status & TPM_STS_DATA_AVAIL) {	/* retry? */
-		dev_err(&chip->dev, "Error left over data\n");
+	if (!check_data)
 		size = -EIO;
-		goto out;
-	}
-
 out:
 	tpm_tis_ready(chip);
 	return size;
@@ -453,14 +474,19 @@ 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;
+	bool data_valid = false;
 
-	rc = tpm_tis_send_data(chip, buf, len);
-	if (rc < 0)
-		return rc;
-
+	for (i = 0; i < TPM_RETRY && !data_valid; i++) {
+		rc = tpm_tis_send_data(chip, buf, len);
+		if (rc < 0)
+			return rc;
+		data_valid = tpm_tis_check_data(chip, buf, len);
+	}
+	if (!data_valid)
+		return -EIO;
 	/* go and do it */
 	rc = tpm_tis_write8(priv, TPM_STS(priv->locality), TPM_STS_GO);
 	if (rc < 0)
diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h
index d06c65b..486c2e9 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 {
@@ -106,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 (*check_data)(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


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

* [PATCH v6 3/7] tpm: tpm_tis: rewrite "tpm_tis_req_canceled()"
  2020-04-07 16:20 [PATCH v6 0/7] Add tpm i2c ptp driver amirmizi6
  2020-04-07 16:20 ` [PATCH v6 1/7] tpm: tpm_tis: Make implementation of read16 read32 write32 optional amirmizi6
  2020-04-07 16:20 ` [PATCH v6 2/7] tpm: tpm_tis: Add check_data handle to tpm_tis_phy_ops amirmizi6
@ 2020-04-07 16:20 ` amirmizi6
  2020-04-07 16:20 ` [PATCH v6 4/7] tpm: tpm_tis: Fix expected bit handling and send all bytes in one shot without last byte in exception amirmizi6
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 16+ messages in thread
From: amirmizi6 @ 2020-04-07 16:20 UTC (permalink / raw)
  To: Eyal.Cohen, jarkko.sakkinen, oshrialkoby85, alexander.steffen,
	robh+dt, mark.rutland, peterhuewe, jgg, arnd, gregkh
  Cc: devicetree, linux-kernel, linux-integrity, oshri.alkoby,
	tmaimon77, gcwilson, kgoldman, Dan.Morav, oren.tanami,
	shmulik.hager, amir.mizinski, Amir Mizinski

From: Amir Mizinski <amirmizi6@gmail.com>

Using this function while read/write data resulted in aborted operation.
After investigating according to TCG TPM Profile (PTP) Specifications,
i found cancel should happen only if TPM_STS.commandReady bit is lit and
couldn't find a case when the current condition is valid.
So i'm removing the case for "TPM_VID_WINBOND" since we have no need for it

Also default comparison is wrong. only cmdReady bit need to be compared
instead of the full lower status register byte.

Fixes: 1f86605 (tpm: Fix cancellation of TPM commands (polling mode))
Signed-off-by: Amir Mizinski <amirmizi6@gmail.com>
---
 drivers/char/tpm/tpm_tis_core.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 6c4f232..453d7ef 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -713,13 +713,10 @@ static bool tpm_tis_req_canceled(struct tpm_chip *chip, u8 status)
 	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
 
 	switch (priv->manufacturer_id) {
-	case TPM_VID_WINBOND:
-		return ((status == TPM_STS_VALID) ||
-			(status == (TPM_STS_VALID | TPM_STS_COMMAND_READY)));
 	case TPM_VID_STM:
 		return (status == (TPM_STS_VALID | TPM_STS_COMMAND_READY));
 	default:
-		return (status == TPM_STS_COMMAND_READY);
+		return ((status & TPM_STS_COMMAND_READY) == TPM_STS_COMMAND_READY);
 	}
 }
 
-- 
2.7.4


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

* [PATCH v6 4/7] tpm: tpm_tis: Fix expected bit handling and send all  bytes in one shot without last byte in exception
  2020-04-07 16:20 [PATCH v6 0/7] Add tpm i2c ptp driver amirmizi6
                   ` (2 preceding siblings ...)
  2020-04-07 16:20 ` [PATCH v6 3/7] tpm: tpm_tis: rewrite "tpm_tis_req_canceled()" amirmizi6
@ 2020-04-07 16:20 ` amirmizi6
  2020-04-07 16:20 ` [PATCH v6 5/7] tpm: Handle an exception for TPM Firmware Update mode amirmizi6
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 16+ messages in thread
From: amirmizi6 @ 2020-04-07 16:20 UTC (permalink / raw)
  To: Eyal.Cohen, jarkko.sakkinen, oshrialkoby85, alexander.steffen,
	robh+dt, mark.rutland, peterhuewe, jgg, arnd, gregkh
  Cc: devicetree, linux-kernel, linux-integrity, oshri.alkoby,
	tmaimon77, gcwilson, kgoldman, Dan.Morav, oren.tanami,
	shmulik.hager, amir.mizinski, Amir Mizinski, Benoit Houyere

From: Amir Mizinski <amirmizi6@gmail.com>

Incorrect implementation to send message has been detected. We check
and loop only on TPM_STS.stsValid bit and next we single check
TPM_STS.expect bit value.
TPM_STS.expected bit should be checked in the same time of
TPM_STS.stsValid, and shall be repeated until timeout_A.

To detect TPM_STS.expected bit reset "wait_for_tpm_stat" function is
modified to "wait_for_tpm_stat_result". This function read regularly
status register and check bit defined by "mask" to reach value defined in
"mask_result".

This correct implementation is mandatory to use new CRC calculation on I2C
TPM command bytes or I2C TPM answer bytes. TPM_STS.expected bit is reset
after all bytes acquired and CRC result inserted in dedicated register. It
introduces a normal latency to have TPM_STS.expected bit reset.

Respectively, to send message as defined in
TCG_DesignPrinciples_TPM2p0Driver_vp24_pubrev.pdf, all bytes should be
sent in one shot instead of sending last byte in exception.

Suggested-by: Benoit Houyere <benoit.houyere@st.com>
Signed-off-by: Amir Mizinski <amirmizi6@gmail.com>
---
 drivers/char/tpm/tpm_tis_core.c | 72 ++++++++++++++++-------------------------
 1 file changed, 28 insertions(+), 44 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 453d7ef..0a340c0 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -44,9 +44,10 @@ static bool wait_for_tpm_stat_cond(struct tpm_chip *chip, u8 mask,
 	return false;
 }
 
-static int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask,
-		unsigned long timeout, wait_queue_head_t *queue,
-		bool check_cancel)
+static int wait_for_tpm_stat_result(struct tpm_chip *chip, u8 mask,
+				    u8 mask_result, unsigned long timeout,
+				    wait_queue_head_t *queue,
+				    bool check_cancel)
 {
 	unsigned long stop;
 	long rc;
@@ -55,7 +56,7 @@ static int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask,
 
 	/* check current status */
 	status = chip->ops->status(chip);
-	if ((status & mask) == mask)
+	if ((status & mask) == mask_result)
 		return 0;
 
 	stop = jiffies + timeout;
@@ -83,7 +84,7 @@ static int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask,
 			usleep_range(TPM_TIMEOUT_USECS_MIN,
 				     TPM_TIMEOUT_USECS_MAX);
 			status = chip->ops->status(chip);
-			if ((status & mask) == mask)
+			if ((status & mask) == mask_result)
 				return 0;
 		} while (time_before(jiffies, stop));
 	}
@@ -290,10 +291,11 @@ static int recv_data(struct tpm_chip *chip, u8 *buf, size_t count)
 	int size = 0, burstcnt, rc;
 
 	while (size < count) {
-		rc = wait_for_tpm_stat(chip,
-				 TPM_STS_DATA_AVAIL | TPM_STS_VALID,
-				 chip->timeout_c,
-				 &priv->read_queue, true);
+		rc = wait_for_tpm_stat_result(chip,
+					TPM_STS_DATA_AVAIL | TPM_STS_VALID,
+					TPM_STS_DATA_AVAIL | TPM_STS_VALID,
+					chip->timeout_c,
+					&priv->read_queue, true);
 		if (rc < 0)
 			return rc;
 		burstcnt = get_burstcount(chip);
@@ -348,8 +350,9 @@ static int tpm_tis_recv(struct tpm_chip *chip, u8 *buf, size_t count)
 			goto out;
 		}
 
-		if (wait_for_tpm_stat(chip, TPM_STS_VALID, chip->timeout_c,
-				      &priv->int_queue, false) < 0) {
+		if (wait_for_tpm_stat_result(chip, TPM_STS_VALID,
+					     TPM_STS_VALID, chip->timeout_c,
+					     &priv->int_queue, false) < 0) {
 			size = -ETIME;
 			goto out;
 		}
@@ -385,61 +388,40 @@ static int tpm_tis_send_data(struct tpm_chip *chip, const u8 *buf, size_t len)
 	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
 	int rc, status, burstcnt;
 	size_t count = 0;
-	bool itpm = priv->flags & TPM_TIS_ITPM_WORKAROUND;
 
 	status = tpm_tis_status(chip);
 	if ((status & TPM_STS_COMMAND_READY) == 0) {
 		tpm_tis_ready(chip);
-		if (wait_for_tpm_stat
-		    (chip, TPM_STS_COMMAND_READY, chip->timeout_b,
-		     &priv->int_queue, false) < 0) {
+		if (wait_for_tpm_stat_result(chip, TPM_STS_COMMAND_READY,
+					     TPM_STS_COMMAND_READY,
+					     chip->timeout_b,
+					     &priv->int_queue, false) < 0) {
 			rc = -ETIME;
 			goto out_err;
 		}
 	}
 
-	while (count < len - 1) {
+	while (count < len) {
 		burstcnt = get_burstcount(chip);
 		if (burstcnt < 0) {
 			dev_err(&chip->dev, "Unable to read burstcount\n");
 			rc = burstcnt;
 			goto out_err;
 		}
-		burstcnt = min_t(int, burstcnt, len - count - 1);
+		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;
 
 		count += burstcnt;
-
-		if (wait_for_tpm_stat(chip, TPM_STS_VALID, chip->timeout_c,
-					&priv->int_queue, false) < 0) {
-			rc = -ETIME;
-			goto out_err;
-		}
-		status = tpm_tis_status(chip);
-		if (!itpm && (status & TPM_STS_DATA_EXPECT) == 0) {
-			rc = -EIO;
-			goto out_err;
-		}
 	}
-
-	/* write last byte */
-	rc = tpm_tis_write8(priv, TPM_DATA_FIFO(priv->locality), buf[count]);
-	if (rc < 0)
-		goto out_err;
-
-	if (wait_for_tpm_stat(chip, TPM_STS_VALID, chip->timeout_c,
-				&priv->int_queue, false) < 0) {
+	if (wait_for_tpm_stat_result(chip, TPM_STS_VALID | TPM_STS_DATA_EXPECT,
+				     TPM_STS_VALID, chip->timeout_a,
+				     &priv->int_queue, false) < 0) {
 		rc = -ETIME;
 		goto out_err;
 	}
-	status = tpm_tis_status(chip);
-	if (!itpm && (status & TPM_STS_DATA_EXPECT) != 0) {
-		rc = -EIO;
-		goto out_err;
-	}
 
 	return 0;
 
@@ -496,9 +478,11 @@ static int tpm_tis_send_main(struct tpm_chip *chip, const u8 *buf, size_t len)
 		ordinal = be32_to_cpu(*((__be32 *) (buf + 6)));
 
 		dur = tpm_calc_ordinal_duration(chip, ordinal);
-		if (wait_for_tpm_stat
-		    (chip, TPM_STS_DATA_AVAIL | TPM_STS_VALID, dur,
-		     &priv->read_queue, false) < 0) {
+		if (wait_for_tpm_stat_result(chip,
+					     TPM_STS_DATA_AVAIL | TPM_STS_VALID,
+					     TPM_STS_DATA_AVAIL | TPM_STS_VALID,
+					     dur,
+					     &priv->read_queue, false) < 0) {
 			rc = -ETIME;
 			goto out_err;
 		}
-- 
2.7.4


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

* [PATCH v6 5/7] tpm: Handle an exception for TPM Firmware Update mode.
  2020-04-07 16:20 [PATCH v6 0/7] Add tpm i2c ptp driver amirmizi6
                   ` (3 preceding siblings ...)
  2020-04-07 16:20 ` [PATCH v6 4/7] tpm: tpm_tis: Fix expected bit handling and send all bytes in one shot without last byte in exception amirmizi6
@ 2020-04-07 16:20 ` amirmizi6
  2020-04-07 16:20 ` [PATCH v6 6/7] dt-bindings: tpm: Add YAML schema for TPM TIS I2C options amirmizi6
  2020-04-07 16:20 ` [PATCH v6 7/7] tpm: tpm_tis: add tpm_tis_i2c driver amirmizi6
  6 siblings, 0 replies; 16+ messages in thread
From: amirmizi6 @ 2020-04-07 16:20 UTC (permalink / raw)
  To: Eyal.Cohen, jarkko.sakkinen, oshrialkoby85, alexander.steffen,
	robh+dt, mark.rutland, peterhuewe, jgg, arnd, gregkh
  Cc: devicetree, linux-kernel, linux-integrity, oshri.alkoby,
	tmaimon77, gcwilson, kgoldman, Dan.Morav, oren.tanami,
	shmulik.hager, amir.mizinski, Amir Mizinski, Benoit Houyere

From: Amir Mizinski <amirmizi6@gmail.com>

An extra precaution for TPM Firmware Update Mode.
For example if TPM power was cut while in Firmware update, platform
should ignore selftest failure and skip TPM initialization sequence.

Suggested-by: Benoit Houyere <benoit.houyere@st.com>
Signed-off-by: Amir Mizinski <amirmizi6@gmail.com>
---
 drivers/char/tpm/tpm2-cmd.c | 4 ++++
 include/linux/tpm.h         | 1 +
 2 files changed, 5 insertions(+)

diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index 7603295..b77e394 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -727,6 +727,10 @@ int tpm2_auto_startup(struct tpm_chip *chip)
 		goto out;
 
 	rc = tpm2_do_selftest(chip);
+
+	if ((rc == TPM2_RC_UPGRADE) || (rc == TPM2_RC_COMMAND_CODE))
+		return 0;
+
 	if (rc && rc != TPM2_RC_INITIALIZE)
 		goto out;
 
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index 03e9b18..5a2e031 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -199,6 +199,7 @@ enum tpm2_return_codes {
 	TPM2_RC_INITIALIZE	= 0x0100, /* RC_VER1 */
 	TPM2_RC_FAILURE		= 0x0101,
 	TPM2_RC_DISABLED	= 0x0120,
+	TPM2_RC_UPGRADE         = 0x012D,
 	TPM2_RC_COMMAND_CODE    = 0x0143,
 	TPM2_RC_TESTING		= 0x090A, /* RC_WARN */
 	TPM2_RC_REFERENCE_H0	= 0x0910,
-- 
2.7.4


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

* [PATCH v6 6/7] dt-bindings: tpm: Add YAML schema for TPM TIS I2C options
  2020-04-07 16:20 [PATCH v6 0/7] Add tpm i2c ptp driver amirmizi6
                   ` (4 preceding siblings ...)
  2020-04-07 16:20 ` [PATCH v6 5/7] tpm: Handle an exception for TPM Firmware Update mode amirmizi6
@ 2020-04-07 16:20 ` amirmizi6
  2020-04-15 15:20   ` Rob Herring
  2020-04-07 16:20 ` [PATCH v6 7/7] tpm: tpm_tis: add tpm_tis_i2c driver amirmizi6
  6 siblings, 1 reply; 16+ messages in thread
From: amirmizi6 @ 2020-04-07 16:20 UTC (permalink / raw)
  To: Eyal.Cohen, jarkko.sakkinen, oshrialkoby85, alexander.steffen,
	robh+dt, mark.rutland, peterhuewe, jgg, arnd, gregkh
  Cc: devicetree, linux-kernel, linux-integrity, oshri.alkoby,
	tmaimon77, gcwilson, kgoldman, Dan.Morav, oren.tanami,
	shmulik.hager, amir.mizinski, Amir Mizinski

From: Amir Mizinski <amirmizi6@gmail.com>

Added a YAML schema to support tpm tis i2c realted dt-bindings for the I2c
PTP based physical layer.

This patch adds the documentation for corresponding device tree bindings of
I2C based Physical TPM.
Refer to the 'I2C Interface Definition' section in
'TCG PC Client PlatformTPMProfile(PTP) Specification' publication
for specification.

Signed-off-by: Amir Mizinski <amirmizi6@gmail.com>
---
 .../bindings/security/tpm/tpm-tis-i2c.yaml         | 47 ++++++++++++++++++++++
 1 file changed, 47 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/security/tpm/tpm-tis-i2c.yaml

diff --git a/Documentation/devicetree/bindings/security/tpm/tpm-tis-i2c.yaml b/Documentation/devicetree/bindings/security/tpm/tpm-tis-i2c.yaml
new file mode 100644
index 0000000..13d7c2c
--- /dev/null
+++ b/Documentation/devicetree/bindings/security/tpm/tpm-tis-i2c.yaml
@@ -0,0 +1,47 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/security/tpm/tpm-tis-i2c.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: I2C PTP based TPM Device Tree Bindings
+
+maintainers:
+  - Amir Mizinski <amirmizi6@gmail.com>
+
+description:
+  Device Tree Bindings for I2C based Trusted Platform Module(TPM).
+
+properties:
+  compatible:
+    contains:
+      const: tcg,tpm-tis-i2c
+
+  reg:
+    maxItems: 1
+
+  interrupt:
+    maxItems: 1
+
+  crc-checksum:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description:
+      CRC checksum enable.
+
+required:
+  - compatible
+  - reg
+
+examples:
+  - |
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      tpm-tis-i2c@2e {
+        compatible = "tcg,tpm-tis-i2c";
+        reg = <0x2e>;
+        crc-checksum;
+      };
+    };
+...
-- 
2.7.4


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

* [PATCH v6 7/7] tpm: tpm_tis: add tpm_tis_i2c driver
  2020-04-07 16:20 [PATCH v6 0/7] Add tpm i2c ptp driver amirmizi6
                   ` (5 preceding siblings ...)
  2020-04-07 16:20 ` [PATCH v6 6/7] dt-bindings: tpm: Add YAML schema for TPM TIS I2C options amirmizi6
@ 2020-04-07 16:20 ` amirmizi6
  2020-04-08 18:36   ` Jarkko Sakkinen
  6 siblings, 1 reply; 16+ messages in thread
From: amirmizi6 @ 2020-04-07 16:20 UTC (permalink / raw)
  To: Eyal.Cohen, jarkko.sakkinen, oshrialkoby85, alexander.steffen,
	robh+dt, mark.rutland, peterhuewe, jgg, arnd, gregkh
  Cc: devicetree, linux-kernel, linux-integrity, oshri.alkoby,
	tmaimon77, gcwilson, kgoldman, Dan.Morav, oren.tanami,
	shmulik.hager, amir.mizinski, Amir Mizinski, Eddie James

From: Amir Mizinski <amirmizi6@gmail.com>

Implements the functionality needed to communicate with an I2C TPM
according to the TCG TPM I2C Interface Specification.

Signed-off-by: Amir Mizinski <amirmizi6@gmail.com>
Tested-by: Eddie James <eajames@linux.ibm.com>
---
 drivers/char/tpm/Kconfig       |  12 ++
 drivers/char/tpm/Makefile      |   1 +
 drivers/char/tpm/tpm_tis_i2c.c | 292 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 305 insertions(+)
 create mode 100644 drivers/char/tpm/tpm_tis_i2c.c

diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
index aacdeed..b166ad3 100644
--- a/drivers/char/tpm/Kconfig
+++ b/drivers/char/tpm/Kconfig
@@ -74,6 +74,18 @@ config TCG_TIS_SPI_CR50
 	  If you have a H1 secure module running Cr50 firmware on SPI bus,
 	  say Yes and it will be accessible from within Linux.
 
+config TCG_TIS_I2C
+	tristate "TPM I2C Interface Specification"
+	depends on I2C
+	depends on CRC_CCITT
+	select TCG_TIS_CORE
+	---help---
+	  If you have a TPM security chip which is connected to a regular
+	  I2C master (i.e. most embedded platforms) that is compliant with the
+	  TCG TPM I2C Interface Specification say Yes and it will be accessible from
+	  within Linux. To compile this driver as a module, choose  M here;
+	  the module will be called tpm_tis_i2c.
+
 config TCG_TIS_I2C_ATMEL
 	tristate "TPM Interface Specification 1.2 Interface (I2C - Atmel)"
 	depends on I2C
diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
index 9567e51..97999cf 100644
--- a/drivers/char/tpm/Makefile
+++ b/drivers/char/tpm/Makefile
@@ -26,6 +26,7 @@ obj-$(CONFIG_TCG_TIS_SPI) += tpm_tis_spi.o
 tpm_tis_spi-y := tpm_tis_spi_main.o
 tpm_tis_spi-$(CONFIG_TCG_TIS_SPI_CR50) += tpm_tis_spi_cr50.o
 
+obj-$(CONFIG_TCG_TIS_I2C) += tpm_tis_i2c.o
 obj-$(CONFIG_TCG_TIS_I2C_ATMEL) += tpm_i2c_atmel.o
 obj-$(CONFIG_TCG_TIS_I2C_INFINEON) += tpm_i2c_infineon.o
 obj-$(CONFIG_TCG_TIS_I2C_NUVOTON) += tpm_i2c_nuvoton.o
diff --git a/drivers/char/tpm/tpm_tis_i2c.c b/drivers/char/tpm/tpm_tis_i2c.c
new file mode 100644
index 0000000..83c0b3a
--- /dev/null
+++ b/drivers/char/tpm/tpm_tis_i2c.c
@@ -0,0 +1,292 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2014-2019 Nuvoton Technology corporation
+ *
+ * TPM TIS I2C
+ *
+ * TPM TIS I2C Device Driver Interface for devices that implement the TPM I2C
+ * Interface defined by TCG PC Client Platform TPM Profile (PTP) Specification
+ * Revision 01.03 v22 at www.trustedcomputinggroup.org
+ */
+
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/slab.h>
+#include <linux/interrupt.h>
+#include <linux/wait.h>
+#include <linux/acpi.h>
+#include <linux/freezer.h>
+#include <linux/crc-ccitt.h>
+
+#include <linux/module.h>
+#include <linux/i2c.h>
+#include <linux/gpio.h>
+#include <linux/of_irq.h>
+#include <linux/of_gpio.h>
+#include <linux/tpm.h>
+#include "tpm.h"
+#include "tpm_tis_core.h"
+
+#define TPM_LOC_SEL                    0x04
+#define TPM_I2C_INTERFACE_CAPABILITY   0x30
+#define TPM_I2C_DEVICE_ADDRESS         0x38
+#define TPM_DATA_CSUM_ENABLE           0x40
+#define TPM_DATA_CSUM                  0x44
+#define TPM_I2C_DID_VID                        0x48
+#define TPM_I2C_RID                    0x4C
+
+//#define I2C_IS_TPM2 1
+
+struct tpm_tis_i2c_phy {
+	struct tpm_tis_data priv;
+	struct i2c_client *i2c_client;
+	bool data_csum;
+	u8 *iobuf;
+};
+
+static inline struct tpm_tis_i2c_phy *to_tpm_tis_i2c_phy(struct tpm_tis_data *data)
+{
+	return container_of(data, struct tpm_tis_i2c_phy, priv);
+}
+
+static u8 address_to_register(u32 addr)
+{
+	addr &= 0xFFF;
+
+	switch (addr) {
+		// adapt register addresses that have changed compared to
+		// older TIS versions
+	case TPM_ACCESS(0):
+		return 0x04;
+	case TPM_LOC_SEL:
+		return 0x00;
+	case TPM_DID_VID(0):
+		return 0x48;
+	case TPM_RID(0):
+		return 0x4C;
+	default:
+		return addr;
+	}
+}
+
+static int tpm_tis_i2c_read_bytes(struct tpm_tis_data *data, u32 addr,
+				  u16 len, u8 *result)
+{
+	struct tpm_tis_i2c_phy *phy = to_tpm_tis_i2c_phy(data);
+	int ret = 0;
+	int i = 0;
+	u8 reg = address_to_register(addr);
+	struct i2c_msg msgs[] = {
+		{
+			.addr = phy->i2c_client->addr,
+			.len = sizeof(reg),
+			.buf = &reg,
+		},
+		{
+			.addr = phy->i2c_client->addr,
+			.len = len,
+			.buf = result,
+			.flags = I2C_M_RD,
+		},
+	};
+
+	do {
+		ret = i2c_transfer(phy->i2c_client->adapter, msgs,
+				   ARRAY_SIZE(msgs));
+		usleep_range(250, 300); // wait default GUARD_TIME of 250µs
+
+	} while (ret < 0 && i++ < TPM_RETRY);
+
+	if (ret < 0)
+		return ret;
+
+
+	return 0;
+}
+
+static int tpm_tis_i2c_write_bytes(struct tpm_tis_data *data, u32 addr,
+				   u16 len, const u8 *value)
+{
+	struct tpm_tis_i2c_phy *phy = to_tpm_tis_i2c_phy(data);
+	int ret = 0;
+	int i = 0;
+
+	if (phy->iobuf) {
+		if (len > TPM_BUFSIZE - 1)
+			return -EIO;
+
+		phy->iobuf[0] = address_to_register(addr);
+		memcpy(phy->iobuf + 1, value, len);
+
+		{
+			struct i2c_msg msgs[] = {
+				{
+					.addr = phy->i2c_client->addr,
+					.len = len + 1,
+					.buf = phy->iobuf,
+				},
+			};
+
+			do {
+				ret = i2c_transfer(phy->i2c_client->adapter,
+						   msgs, ARRAY_SIZE(msgs));
+				// wait default GUARD_TIME of 250µs
+				usleep_range(250, 300);
+			} while (ret < 0 && i++ < TPM_RETRY);
+		}
+	} else {
+		u8 reg = address_to_register(addr);
+
+		struct i2c_msg msgs[] = {
+			{
+				.addr = phy->i2c_client->addr,
+				.len = sizeof(reg),
+				.buf = &reg,
+			},
+			{
+				.addr = phy->i2c_client->addr,
+				.len = len,
+				.buf = (u8 *)value,
+				.flags = I2C_M_NOSTART,
+			},
+		};
+		do {
+			ret = i2c_transfer(phy->i2c_client->adapter, msgs,
+					   ARRAY_SIZE(msgs));
+			// wait default GUARD_TIME of 250µs
+			usleep_range(250, 300);
+		} while (ret < 0 && i++ < TPM_RETRY);
+	}
+
+	if (ret < 0)
+		return ret;
+
+
+	return 0;
+}
+
+static bool tpm_tis_i2c_check_data(struct tpm_tis_data *data,
+				   const u8 *buf, size_t len)
+{
+	struct tpm_tis_i2c_phy *phy = to_tpm_tis_i2c_phy(data);
+	u16 crc, crc_tpm;
+	int rc;
+
+	if (phy->data_csum) {
+		crc = crc_ccitt(0x0000, buf, len);
+		rc = tpm_tis_read16(data, TPM_DATA_CSUM, &crc_tpm);
+		if (rc < 0)
+			return false;
+
+		crc_tpm = be16_to_cpu(crc_tpm);
+		return crc == crc_tpm;
+	}
+
+	return true;
+}
+
+static SIMPLE_DEV_PM_OPS(tpm_tis_pm, tpm_pm_suspend, tpm_tis_resume);
+
+static int csum_state_store(struct tpm_tis_data *data, u8 new_state)
+{
+	struct tpm_tis_i2c_phy *phy = to_tpm_tis_i2c_phy(data);
+	u8 cur_state;
+	int rc;
+
+	rc = tpm_tis_i2c_write_bytes(&phy->priv, TPM_DATA_CSUM_ENABLE,
+				     1, &new_state);
+	if (rc < 0)
+		return rc;
+
+	rc = tpm_tis_i2c_read_bytes(&phy->priv, TPM_DATA_CSUM_ENABLE,
+				    1, &cur_state);
+	if (rc < 0)
+		return rc;
+
+	if (new_state == cur_state)
+		phy->data_csum = (bool)new_state;
+
+	return rc;
+}
+
+static const struct tpm_tis_phy_ops tpm_i2c_phy_ops = {
+	.read_bytes = tpm_tis_i2c_read_bytes,
+	.write_bytes = tpm_tis_i2c_write_bytes,
+	.check_data = tpm_tis_i2c_check_data,
+};
+
+static int tpm_tis_i2c_probe(struct i2c_client *dev,
+			     const struct i2c_device_id *id)
+{
+	struct tpm_tis_i2c_phy *phy;
+	int rc;
+	int CRC_Checksum = 0;
+	const u8 loc_init = 0;
+	struct device_node *np;
+
+	phy = devm_kzalloc(&dev->dev, sizeof(struct tpm_tis_i2c_phy),
+			   GFP_KERNEL);
+	if (!phy)
+		return -ENOMEM;
+
+	phy->i2c_client = dev;
+
+	if (!i2c_check_functionality(dev->adapter, I2C_FUNC_NOSTART)) {
+		phy->iobuf = devm_kmalloc(&dev->dev, TPM_BUFSIZE, GFP_KERNEL);
+		if (!phy->iobuf)
+			return -ENOMEM;
+	}
+
+	// select locality 0 (the driver will access only via locality 0)
+	rc = tpm_tis_i2c_write_bytes(&phy->priv, TPM_LOC_SEL, 1, &loc_init);
+	if (rc < 0)
+		return rc;
+
+	// set CRC checksum calculation enable
+	np = dev->dev.of_node;
+	if (of_property_read_bool(np, "crc-checksum"))
+		CRC_Checksum = 1;
+
+	rc = csum_state_store(&phy->priv, CRC_Checksum);
+	if (rc < 0)
+		return rc;
+
+	return tpm_tis_core_init(&dev->dev, &phy->priv, -1, &tpm_i2c_phy_ops,
+					NULL);
+}
+
+static const struct i2c_device_id tpm_tis_i2c_id[] = {
+	{"tpm_tis_i2c", 0},
+	{}
+};
+MODULE_DEVICE_TABLE(i2c, tpm_tis_i2c_id);
+
+static const struct of_device_id of_tis_i2c_match[] = {
+	{ .compatible = "tcg,tpm-tis-i2c", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, of_tis_i2c_match);
+
+static const struct acpi_device_id acpi_tis_i2c_match[] = {
+	{"SMO0768", 0},
+	{}
+};
+MODULE_DEVICE_TABLE(acpi, acpi_tis_i2c_match);
+
+static struct i2c_driver tpm_tis_i2c_driver = {
+	.driver = {
+		.owner = THIS_MODULE,
+		.name = "tpm_tis_i2c",
+		.pm = &tpm_tis_pm,
+		.of_match_table = of_match_ptr(of_tis_i2c_match),
+		.acpi_match_table = ACPI_PTR(acpi_tis_i2c_match),
+	},
+	.probe = tpm_tis_i2c_probe,
+	.id_table = tpm_tis_i2c_id,
+};
+
+module_i2c_driver(tpm_tis_i2c_driver);
+
+MODULE_DESCRIPTION("TPM Driver");
+MODULE_LICENSE("GPL");
-- 
2.7.4


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

* Re: [PATCH v6 1/7] tpm: tpm_tis: Make implementation of read16 read32 write32 optional
  2020-04-07 16:20 ` [PATCH v6 1/7] tpm: tpm_tis: Make implementation of read16 read32 write32 optional amirmizi6
@ 2020-04-08 16:46   ` Jarkko Sakkinen
  0 siblings, 0 replies; 16+ messages in thread
From: Jarkko Sakkinen @ 2020-04-08 16:46 UTC (permalink / raw)
  To: amirmizi6
  Cc: Eyal.Cohen, oshrialkoby85, alexander.steffen, robh+dt,
	mark.rutland, peterhuewe, jgg, arnd, gregkh, devicetree,
	linux-kernel, linux-integrity, oshri.alkoby, tmaimon77, gcwilson,
	kgoldman, Dan.Morav, oren.tanami, shmulik.hager, amir.mizinski

On Tue, Apr 07, 2020 at 07:20:38PM +0300, amirmizi6@gmail.com wrote:
> From: Amir Mizinski <amirmizi6@gmail.com>
> 
> Only tpm_tis can use memory mapped I/O, which is truly mapped into
> the kernel's memory space. So using ioread16/ioread32/iowrite32 turn into a
> straightforward pointer dereference.
> Every other driver require more complicated operations to read more than 1
> byte at a time and will just fall back to read_bytes/write_bytes.
> Therefore, move this common code out of tpm_tis_spi into tpm_tis_core, so
> that it is automatically used when low-level drivers do not implement the
> specialized methods.
> 
> Co-developed-by: Alexander Steffen <Alexander.Steffen@infineon.com>
> Signed-off-by: Alexander Steffen <Alexander.Steffen@infineon.com>
> Signed-off-by: Amir Mizinski <amirmizi6@gmail.com>
> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

Just noticed that the short summary is still wrong. As far as I remember
I asked to fix it.

tpm: tpm_tis: Make implementation of read16 read32 write32 optional

In English you put comma between enumerated things except for the last
thing where you use and-conjuction. How hard is it seriously to write
legit sentences?

/Jarkko

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

* Re: [PATCH v6 2/7] tpm: tpm_tis: Add check_data handle to tpm_tis_phy_ops
  2020-04-07 16:20 ` [PATCH v6 2/7] tpm: tpm_tis: Add check_data handle to tpm_tis_phy_ops amirmizi6
@ 2020-04-08 18:33   ` Jarkko Sakkinen
       [not found]     ` <CAMHTsUUzQ9rpPfsnEtJruAC3THr+XNA=5Wk7OCteqZXOQ0L=UA@mail.gmail.com>
  2020-04-21 13:34     ` Amir Mizinski
  0 siblings, 2 replies; 16+ messages in thread
From: Jarkko Sakkinen @ 2020-04-08 18:33 UTC (permalink / raw)
  To: amirmizi6
  Cc: Eyal.Cohen, oshrialkoby85, alexander.steffen, robh+dt,
	mark.rutland, peterhuewe, jgg, arnd, gregkh, devicetree,
	linux-kernel, linux-integrity, oshri.alkoby, tmaimon77, gcwilson,
	kgoldman, Dan.Morav, oren.tanami, shmulik.hager, amir.mizinski,
	Christophe Ricard

On Tue, Apr 07, 2020 at 07:20:39PM +0300, amirmizi6@gmail.com wrote:
> From: Amir Mizinski <amirmizi6@gmail.com>
> 
> In order to validate data integrity we need to compute the crc over the data
> sent in lower layer (I2C for instance).

s/crc/CRC/

> To do that tpm_tis_check_data() calls a "check_data" operation (if available).

"check_data" does not exist.

> If data integrity check fails, a retry to save the sent/received
> data is implemented in tpm_tis_send_main()/tpm_tis_recv() functions.
> 
> Considering this commit, the following steps are done when sending a command:
>    1. Host writes to TPM_STS.commandReady.
>    2. Host writes command.
>    3. Host checks that TPM received data is valid.
>    4. If data is currupted go to step 1.
> 
> When receiving data:
>    1. Host checks that TPM_STS.dataAvail is set.
>    2. Host saves received data.
>    3. Host checks that received data is correct.
>    4. If data is currupted Host writes to TPM_STS.responseRetry and go to
>       step 1.

These sequences in the commit message look somewhat uselss. Maybe
just remove them.

> 
> 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 | 102 +++++++++++++++++++++++++---------------
>  drivers/char/tpm/tpm_tis_core.h |   3 ++
>  2 files changed, 67 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index 27c6ca0..6c4f232 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -242,6 +242,15 @@ static u8 tpm_tis_status(struct tpm_chip *chip)
>  	return status;
>  }
>  
> +static bool tpm_tis_check_data(struct tpm_chip *chip, const u8 *buf, size_t len)

Not sure if this is the best possible function name, "check" can
mean almost anything.

> +{
> +	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
> +
> +	if (priv->phy_ops->check_data)
> +		return priv->phy_ops->check_data(priv, buf, len);

New line here before the return statement.

> +	return true;
> +}
> +
>  static void tpm_tis_ready(struct tpm_chip *chip)
>  {
>  	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
> @@ -308,47 +317,59 @@ 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;
> +	int status, i;
>  	u32 expected;
> +	bool check_data = false;
>  
> -	if (count < TPM_HEADER_SIZE) {
> -		size = -EIO;
> -		goto out;
> -	}
> +	for (i = 0; i < TPM_RETRY; i++) {
> +		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;
> -	}
> +		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;
> +		}
>  
> -	expected = be32_to_cpu(*(__be32 *) (buf + 2));
> -	if (expected > count || expected < TPM_HEADER_SIZE) {
> -		size = -EIO;
> -		goto out;
> -	}
> +		expected = be32_to_cpu(*(__be32 *) (buf + 2));
> +		if (expected > count || expected < TPM_HEADER_SIZE) {
> +			size = -EIO;
> +			goto out;
> +		}
>  
> -	size += recv_data(chip, &buf[TPM_HEADER_SIZE],
> -			  expected - TPM_HEADER_SIZE);
> -	if (size < expected) {
> -		dev_err(&chip->dev, "Unable to read remainder of result\n");
> -		size = -ETIME;
> -		goto out;
> -	}
> +		size += recv_data(chip, &buf[TPM_HEADER_SIZE],
> +				  expected - TPM_HEADER_SIZE);
> +		if (size < expected) {
> +			dev_err(&chip->dev, "Unable to read remainder of result\n");
> +			size = -ETIME;
> +			goto out;
> +		}
>  
> -	if (wait_for_tpm_stat(chip, TPM_STS_VALID, chip->timeout_c,
> -				&priv->int_queue, false) < 0) {
> -		size = -ETIME;
> -		goto out;
> +		if (wait_for_tpm_stat(chip, TPM_STS_VALID, chip->timeout_c,
> +				      &priv->int_queue, false) < 0) {
> +			size = -ETIME;
> +			goto out;
> +		}
> +
> +		status = tpm_tis_status(chip);
> +		if (status & TPM_STS_DATA_AVAIL) {	/* retry? */
> +			dev_err(&chip->dev, "Error left over data\n");
> +			size = -EIO;
> +			goto out;
> +		}
> +
> +		check_data = tpm_tis_check_data(chip, buf, size);
> +		if (!check_data)
> +			tpm_tis_write8(priv, TPM_STS(priv->locality),
> +				       TPM_STS_RESPONSE_RETRY);
> +		else
> +			break;
>  	}
> -	status = tpm_tis_status(chip);
> -	if (status & TPM_STS_DATA_AVAIL) {	/* retry? */
> -		dev_err(&chip->dev, "Error left over data\n");
> +	if (!check_data)
>  		size = -EIO;
> -		goto out;
> -	}
> -
>  out:
>  	tpm_tis_ready(chip);
>  	return size;
> @@ -453,14 +474,19 @@ 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;
> +	bool data_valid = false;
>  
> -	rc = tpm_tis_send_data(chip, buf, len);
> -	if (rc < 0)
> -		return rc;
> -
> +	for (i = 0; i < TPM_RETRY && !data_valid; i++) {
> +		rc = tpm_tis_send_data(chip, buf, len);
> +		if (rc < 0)
> +			return rc;
> +		data_valid = tpm_tis_check_data(chip, buf, len);
> +	}
> +	if (!data_valid)
> +		return -EIO;
>  	/* go and do it */
>  	rc = tpm_tis_write8(priv, TPM_STS(priv->locality), TPM_STS_GO);
>  	if (rc < 0)
> diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h
> index d06c65b..486c2e9 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 {
> @@ -106,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 (*check_data)(struct tpm_tis_data *data, const u8 *buf,
> +			   size_t len);

Aren't you validating the contents of the buf?

/Jarkko

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

* Re: [PATCH v6 7/7] tpm: tpm_tis: add tpm_tis_i2c driver
  2020-04-07 16:20 ` [PATCH v6 7/7] tpm: tpm_tis: add tpm_tis_i2c driver amirmizi6
@ 2020-04-08 18:36   ` Jarkko Sakkinen
  0 siblings, 0 replies; 16+ messages in thread
From: Jarkko Sakkinen @ 2020-04-08 18:36 UTC (permalink / raw)
  To: amirmizi6
  Cc: Eyal.Cohen, oshrialkoby85, alexander.steffen, robh+dt,
	mark.rutland, peterhuewe, jgg, arnd, gregkh, devicetree,
	linux-kernel, linux-integrity, oshri.alkoby, tmaimon77, gcwilson,
	kgoldman, Dan.Morav, oren.tanami, shmulik.hager, amir.mizinski,
	Eddie James

On Tue, Apr 07, 2020 at 07:20:44PM +0300, amirmizi6@gmail.com wrote:
> From: Amir Mizinski <amirmizi6@gmail.com>
> 
> Implements the functionality needed to communicate with an I2C TPM
> according to the TCG TPM I2C Interface Specification.
> 
> Signed-off-by: Amir Mizinski <amirmizi6@gmail.com>
> Tested-by: Eddie James <eajames@linux.ibm.com>
> ---
>  drivers/char/tpm/Kconfig       |  12 ++
>  drivers/char/tpm/Makefile      |   1 +
>  drivers/char/tpm/tpm_tis_i2c.c | 292 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 305 insertions(+)
>  create mode 100644 drivers/char/tpm/tpm_tis_i2c.c
> 
> diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
> index aacdeed..b166ad3 100644
> --- a/drivers/char/tpm/Kconfig
> +++ b/drivers/char/tpm/Kconfig
> @@ -74,6 +74,18 @@ config TCG_TIS_SPI_CR50
>  	  If you have a H1 secure module running Cr50 firmware on SPI bus,
>  	  say Yes and it will be accessible from within Linux.
>  
> +config TCG_TIS_I2C
> +	tristate "TPM I2C Interface Specification"
> +	depends on I2C
> +	depends on CRC_CCITT
> +	select TCG_TIS_CORE
> +	---help---
> +	  If you have a TPM security chip which is connected to a regular
> +	  I2C master (i.e. most embedded platforms) that is compliant with the
> +	  TCG TPM I2C Interface Specification say Yes and it will be accessible from
> +	  within Linux. To compile this driver as a module, choose  M here;
> +	  the module will be called tpm_tis_i2c.
> +
>  config TCG_TIS_I2C_ATMEL
>  	tristate "TPM Interface Specification 1.2 Interface (I2C - Atmel)"
>  	depends on I2C
> diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
> index 9567e51..97999cf 100644
> --- a/drivers/char/tpm/Makefile
> +++ b/drivers/char/tpm/Makefile
> @@ -26,6 +26,7 @@ obj-$(CONFIG_TCG_TIS_SPI) += tpm_tis_spi.o
>  tpm_tis_spi-y := tpm_tis_spi_main.o
>  tpm_tis_spi-$(CONFIG_TCG_TIS_SPI_CR50) += tpm_tis_spi_cr50.o
>  
> +obj-$(CONFIG_TCG_TIS_I2C) += tpm_tis_i2c.o
>  obj-$(CONFIG_TCG_TIS_I2C_ATMEL) += tpm_i2c_atmel.o
>  obj-$(CONFIG_TCG_TIS_I2C_INFINEON) += tpm_i2c_infineon.o
>  obj-$(CONFIG_TCG_TIS_I2C_NUVOTON) += tpm_i2c_nuvoton.o
> diff --git a/drivers/char/tpm/tpm_tis_i2c.c b/drivers/char/tpm/tpm_tis_i2c.c
> new file mode 100644
> index 0000000..83c0b3a
> --- /dev/null
> +++ b/drivers/char/tpm/tpm_tis_i2c.c
> @@ -0,0 +1,292 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2014-2019 Nuvoton Technology corporation
> + *
> + * TPM TIS I2C
> + *
> + * TPM TIS I2C Device Driver Interface for devices that implement the TPM I2C
> + * Interface defined by TCG PC Client Platform TPM Profile (PTP) Specification
> + * Revision 01.03 v22 at www.trustedcomputinggroup.org
> + */
> +
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/moduleparam.h>
> +#include <linux/slab.h>
> +#include <linux/interrupt.h>
> +#include <linux/wait.h>
> +#include <linux/acpi.h>
> +#include <linux/freezer.h>
> +#include <linux/crc-ccitt.h>
> +
> +#include <linux/module.h>
> +#include <linux/i2c.h>
> +#include <linux/gpio.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_gpio.h>
> +#include <linux/tpm.h>
> +#include "tpm.h"
> +#include "tpm_tis_core.h"
> +
> +#define TPM_LOC_SEL                    0x04
> +#define TPM_I2C_INTERFACE_CAPABILITY   0x30
> +#define TPM_I2C_DEVICE_ADDRESS         0x38
> +#define TPM_DATA_CSUM_ENABLE           0x40
> +#define TPM_DATA_CSUM                  0x44
> +#define TPM_I2C_DID_VID                        0x48
> +#define TPM_I2C_RID                    0x4C
> +
> +//#define I2C_IS_TPM2 1
> +
> +struct tpm_tis_i2c_phy {
> +	struct tpm_tis_data priv;
> +	struct i2c_client *i2c_client;
> +	bool data_csum;
> +	u8 *iobuf;
> +};
> +
> +static inline struct tpm_tis_i2c_phy *to_tpm_tis_i2c_phy(struct tpm_tis_data *data)
> +{
> +	return container_of(data, struct tpm_tis_i2c_phy, priv);
> +}
> +
> +static u8 address_to_register(u32 addr)
> +{
> +	addr &= 0xFFF;
> +
> +	switch (addr) {
> +		// adapt register addresses that have changed compared to
> +		// older TIS versions
> +	case TPM_ACCESS(0):
> +		return 0x04;
> +	case TPM_LOC_SEL:
> +		return 0x00;
> +	case TPM_DID_VID(0):
> +		return 0x48;
> +	case TPM_RID(0):
> +		return 0x4C;
> +	default:
> +		return addr;
> +	}
> +}
> +
> +static int tpm_tis_i2c_read_bytes(struct tpm_tis_data *data, u32 addr,
> +				  u16 len, u8 *result)
> +{
> +	struct tpm_tis_i2c_phy *phy = to_tpm_tis_i2c_phy(data);
> +	int ret = 0;
> +	int i = 0;
> +	u8 reg = address_to_register(addr);
> +	struct i2c_msg msgs[] = {
> +		{
> +			.addr = phy->i2c_client->addr,
> +			.len = sizeof(reg),
> +			.buf = &reg,
> +		},
> +		{
> +			.addr = phy->i2c_client->addr,
> +			.len = len,
> +			.buf = result,
> +			.flags = I2C_M_RD,
> +		},
> +	};
> +
> +	do {
> +		ret = i2c_transfer(phy->i2c_client->adapter, msgs,
> +				   ARRAY_SIZE(msgs));
> +		usleep_range(250, 300); // wait default GUARD_TIME of 250µs
> +
> +	} while (ret < 0 && i++ < TPM_RETRY);
> +
> +	if (ret < 0)
> +		return ret;
> +
> +
> +	return 0;
> +}
> +
> +static int tpm_tis_i2c_write_bytes(struct tpm_tis_data *data, u32 addr,
> +				   u16 len, const u8 *value)
> +{
> +	struct tpm_tis_i2c_phy *phy = to_tpm_tis_i2c_phy(data);
> +	int ret = 0;
> +	int i = 0;
> +
> +	if (phy->iobuf) {
> +		if (len > TPM_BUFSIZE - 1)
> +			return -EIO;
> +
> +		phy->iobuf[0] = address_to_register(addr);
> +		memcpy(phy->iobuf + 1, value, len);
> +
> +		{
> +			struct i2c_msg msgs[] = {
> +				{
> +					.addr = phy->i2c_client->addr,
> +					.len = len + 1,
> +					.buf = phy->iobuf,
> +				},
> +			};
> +
> +			do {
> +				ret = i2c_transfer(phy->i2c_client->adapter,
> +						   msgs, ARRAY_SIZE(msgs));
> +				// wait default GUARD_TIME of 250µs
> +				usleep_range(250, 300);
> +			} while (ret < 0 && i++ < TPM_RETRY);
> +		}
> +	} else {
> +		u8 reg = address_to_register(addr);
> +
> +		struct i2c_msg msgs[] = {
> +			{
> +				.addr = phy->i2c_client->addr,
> +				.len = sizeof(reg),
> +				.buf = &reg,
> +			},
> +			{
> +				.addr = phy->i2c_client->addr,
> +				.len = len,
> +				.buf = (u8 *)value,
> +				.flags = I2C_M_NOSTART,
> +			},
> +		};
> +		do {
> +			ret = i2c_transfer(phy->i2c_client->adapter, msgs,
> +					   ARRAY_SIZE(msgs));
> +			// wait default GUARD_TIME of 250µs
> +			usleep_range(250, 300);
> +		} while (ret < 0 && i++ < TPM_RETRY);
> +	}
> +
> +	if (ret < 0)
> +		return ret;
> +
> +
> +	return 0;
> +}
> +
> +static bool tpm_tis_i2c_check_data(struct tpm_tis_data *data,
> +				   const u8 *buf, size_t len)
> +{
> +	struct tpm_tis_i2c_phy *phy = to_tpm_tis_i2c_phy(data);
> +	u16 crc, crc_tpm;
> +	int rc;
> +
> +	if (phy->data_csum) {
> +		crc = crc_ccitt(0x0000, buf, len);
> +		rc = tpm_tis_read16(data, TPM_DATA_CSUM, &crc_tpm);
> +		if (rc < 0)
> +			return false;
> +
> +		crc_tpm = be16_to_cpu(crc_tpm);
> +		return crc == crc_tpm;
> +	}
> +
> +	return true;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(tpm_tis_pm, tpm_pm_suspend, tpm_tis_resume);
> +
> +static int csum_state_store(struct tpm_tis_data *data, u8 new_state)
> +{
> +	struct tpm_tis_i2c_phy *phy = to_tpm_tis_i2c_phy(data);
> +	u8 cur_state;
> +	int rc;
> +
> +	rc = tpm_tis_i2c_write_bytes(&phy->priv, TPM_DATA_CSUM_ENABLE,
> +				     1, &new_state);
> +	if (rc < 0)
> +		return rc;
> +
> +	rc = tpm_tis_i2c_read_bytes(&phy->priv, TPM_DATA_CSUM_ENABLE,
> +				    1, &cur_state);
> +	if (rc < 0)
> +		return rc;
> +
> +	if (new_state == cur_state)
> +		phy->data_csum = (bool)new_state;
> +
> +	return rc;
> +}
> +
> +static const struct tpm_tis_phy_ops tpm_i2c_phy_ops = {
> +	.read_bytes = tpm_tis_i2c_read_bytes,
> +	.write_bytes = tpm_tis_i2c_write_bytes,
> +	.check_data = tpm_tis_i2c_check_data,
> +};
> +
> +static int tpm_tis_i2c_probe(struct i2c_client *dev,
> +			     const struct i2c_device_id *id)
> +{
> +	struct tpm_tis_i2c_phy *phy;
> +	int rc;
> +	int CRC_Checksum = 0;
> +	const u8 loc_init = 0;
> +	struct device_node *np;
> +
> +	phy = devm_kzalloc(&dev->dev, sizeof(struct tpm_tis_i2c_phy),
> +			   GFP_KERNEL);
> +	if (!phy)
> +		return -ENOMEM;
> +
> +	phy->i2c_client = dev;
> +
> +	if (!i2c_check_functionality(dev->adapter, I2C_FUNC_NOSTART)) {
> +		phy->iobuf = devm_kmalloc(&dev->dev, TPM_BUFSIZE, GFP_KERNEL);
> +		if (!phy->iobuf)
> +			return -ENOMEM;
> +	}
> +
> +	// select locality 0 (the driver will access only via locality 0)
> +	rc = tpm_tis_i2c_write_bytes(&phy->priv, TPM_LOC_SEL, 1, &loc_init);
> +	if (rc < 0)
> +		return rc;
> +
> +	// set CRC checksum calculation enable
> +	np = dev->dev.of_node;
> +	if (of_property_read_bool(np, "crc-checksum"))
> +		CRC_Checksum = 1;
> +
> +	rc = csum_state_store(&phy->priv, CRC_Checksum);
> +	if (rc < 0)
> +		return rc;
> +
> +	return tpm_tis_core_init(&dev->dev, &phy->priv, -1, &tpm_i2c_phy_ops,
> +					NULL);
> +}
> +
> +static const struct i2c_device_id tpm_tis_i2c_id[] = {
> +	{"tpm_tis_i2c", 0},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(i2c, tpm_tis_i2c_id);
> +
> +static const struct of_device_id of_tis_i2c_match[] = {
> +	{ .compatible = "tcg,tpm-tis-i2c", },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, of_tis_i2c_match);
> +
> +static const struct acpi_device_id acpi_tis_i2c_match[] = {
> +	{"SMO0768", 0},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(acpi, acpi_tis_i2c_match);
> +
> +static struct i2c_driver tpm_tis_i2c_driver = {
> +	.driver = {
> +		.owner = THIS_MODULE,
> +		.name = "tpm_tis_i2c",
> +		.pm = &tpm_tis_pm,
> +		.of_match_table = of_match_ptr(of_tis_i2c_match),
> +		.acpi_match_table = ACPI_PTR(acpi_tis_i2c_match),
> +	},
> +	.probe = tpm_tis_i2c_probe,
> +	.id_table = tpm_tis_i2c_id,
> +};
> +
> +module_i2c_driver(tpm_tis_i2c_driver);
> +
> +MODULE_DESCRIPTION("TPM Driver");
> +MODULE_LICENSE("GPL");
> -- 
> 2.7.4
> 

This patch is in corrupted state essentially. No reason to review it.
Not even checkpatch errors have been fixed.

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

* Re: [PATCH v6 2/7] tpm: tpm_tis: Add check_data handle to tpm_tis_phy_ops
       [not found]     ` <CAMHTsUUzQ9rpPfsnEtJruAC3THr+XNA=5Wk7OCteqZXOQ0L=UA@mail.gmail.com>
@ 2020-04-12 13:53       ` Jarkko Sakkinen
  0 siblings, 0 replies; 16+ messages in thread
From: Jarkko Sakkinen @ 2020-04-12 13:53 UTC (permalink / raw)
  To: Amir Mizinski
  Cc: Eyal.Cohen, Oshri Alkobi, Alexander Steffen, Rob Herring,
	Mark Rutland, peterhuewe, jgg, Arnd Bergmann, Greg KH,
	devicetree, Linux Kernel Mailing List, linux-integrity,
	IS20 Oshri Alkoby, Tomer Maimon, gcwilson, kgoldman,
	IS30 Dan Morav, oren.tanami, shmulik.hager, amir.mizinski,
	Christophe Ricard

There is something seriously wrong in your email client.
Please reconfigure your email client according to:

https://www.kernel.org/doc/html/v5.5/process/email-clients.html

Then re-response.

This is.. I've lost to the count how many times I've requsted this.

Thanks.

/Jarkko

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

* Re: [PATCH v6 6/7] dt-bindings: tpm: Add YAML schema for TPM TIS I2C options
  2020-04-07 16:20 ` [PATCH v6 6/7] dt-bindings: tpm: Add YAML schema for TPM TIS I2C options amirmizi6
@ 2020-04-15 15:20   ` Rob Herring
  2020-04-22  8:15     ` Amir Mizinski
  0 siblings, 1 reply; 16+ messages in thread
From: Rob Herring @ 2020-04-15 15:20 UTC (permalink / raw)
  To: amirmizi6
  Cc: Eyal.Cohen, jarkko.sakkinen, oshrialkoby85, alexander.steffen,
	mark.rutland, peterhuewe, jgg, arnd, gregkh, devicetree,
	linux-kernel, linux-integrity, oshri.alkoby, tmaimon77, gcwilson,
	kgoldman, Dan.Morav, oren.tanami, shmulik.hager, amir.mizinski

On Tue, Apr 07, 2020 at 07:20:43PM +0300, amirmizi6@gmail.com wrote:
> From: Amir Mizinski <amirmizi6@gmail.com>
> 
> Added a YAML schema to support tpm tis i2c realted dt-bindings for the I2c
> PTP based physical layer.
> 
> This patch adds the documentation for corresponding device tree bindings of
> I2C based Physical TPM.
> Refer to the 'I2C Interface Definition' section in
> 'TCG PC Client PlatformTPMProfile(PTP) Specification' publication
> for specification.
> 
> Signed-off-by: Amir Mizinski <amirmizi6@gmail.com>
> ---
>  .../bindings/security/tpm/tpm-tis-i2c.yaml         | 47 ++++++++++++++++++++++
>  1 file changed, 47 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/security/tpm/tpm-tis-i2c.yaml
> 
> diff --git a/Documentation/devicetree/bindings/security/tpm/tpm-tis-i2c.yaml b/Documentation/devicetree/bindings/security/tpm/tpm-tis-i2c.yaml
> new file mode 100644
> index 0000000..13d7c2c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/security/tpm/tpm-tis-i2c.yaml
> @@ -0,0 +1,47 @@
> +# SPDX-License-Identifier: GPL-2.0
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/security/tpm/tpm-tis-i2c.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: I2C PTP based TPM Device Tree Bindings
> +
> +maintainers:
> +  - Amir Mizinski <amirmizi6@gmail.com>
> +
> +description:
> +  Device Tree Bindings for I2C based Trusted Platform Module(TPM).
> +
> +properties:
> +  compatible:
> +    contains:
> +      const: tcg,tpm-tis-i2c

This is not sufficient. I assume you are testing on some specific TPM 
chip.

> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupt:
> +    maxItems: 1
> +
> +  crc-checksum:
> +    $ref: /schemas/types.yaml#/definitions/flag
> +    description:
> +      CRC checksum enable.

Why would you not want CRC? Some chips support and some don't? If so, 
the compatible for the chip should imply that.

> +
> +required:
> +  - compatible
> +  - reg
> +
> +examples:
> +  - |
> +    i2c {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      tpm-tis-i2c@2e {

tpm@2e

> +        compatible = "tcg,tpm-tis-i2c";
> +        reg = <0x2e>;
> +        crc-checksum;
> +      };
> +    };
> +...
> -- 
> 2.7.4
> 

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

* Re: [PATCH v6 2/7] tpm: tpm_tis: Add check_data handle to tpm_tis_phy_ops
  2020-04-08 18:33   ` Jarkko Sakkinen
       [not found]     ` <CAMHTsUUzQ9rpPfsnEtJruAC3THr+XNA=5Wk7OCteqZXOQ0L=UA@mail.gmail.com>
@ 2020-04-21 13:34     ` Amir Mizinski
  2020-04-21 20:19       ` Jarkko Sakkinen
  1 sibling, 1 reply; 16+ messages in thread
From: Amir Mizinski @ 2020-04-21 13:34 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: Eyal.Cohen, oshrialkoby85, alexander.steffen, robh+dt,
	mark.rutland, peterhuewe, jgg, arnd, gregkh, devicetree,
	linux-kernel, linux-integrity, oshri.alkoby, tmaimon77, gcwilson,
	kgoldman, Dan.Morav, oren.tanami, shmulik.hager, amir.mizinski,
	Christophe Ricard


Hello jarkko,
I reconfigure my email client by the instructions you've sent, and
re-responsing as you requested.
please tell me if there are still any issues. thank you.

On 2020-04-08 18:33, Jarkko Sakkinen wrote:
> On Tue, Apr 07, 2020 at 07:20:39PM +0300, amirmizi6@gmail.com wrote:
>> From: Amir Mizinski <amirmizi6@gmail.com>
>>
>> In order to validate data integrity we need to compute the crc over the data
>> sent in lower layer (I2C for instance).
>
> s/crc/CRC/
>
>> To do that tpm_tis_check_data() calls a "check_data" operation (if available).
>
> "check_data" does not exist.
>

it is added in this commit to "tpm_tis_phy_ops" struct in
"tpm_tis_core.h", which is inherited in "tpm_tis_i2c.c" on later patch
(7/7).

>> If data integrity check fails, a retry to save the sent/received
>> data is implemented in tpm_tis_send_main()/tpm_tis_recv() functions.
>>
>> Considering this commit, the following steps are done when sending a command:
>>    1. Host writes to TPM_STS.commandReady.
>>    2. Host writes command.
>>    3. Host checks that TPM received data is valid.
>>    4. If data is currupted go to step 1.
>>
>> When receiving data:
>>    1. Host checks that TPM_STS.dataAvail is set.
>>    2. Host saves received data.
>>    3. Host checks that received data is correct.
>>    4. If data is currupted Host writes to TPM_STS.responseRetry and go to
>>       step 1.
>
> These sequences in the commit message look somewhat uselss. Maybe
> just remove them.
>

Their main porpose is to describe how the retry attempt is implemented
in case of currupted data.
should i just describe that with a few words or that's unnecessary?

>>
>> 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 | 102 +++++++++++++++++++++++++---------------
>>  drivers/char/tpm/tpm_tis_core.h |   3 ++
>>  2 files changed, 67 insertions(+), 38 deletions(-)
>>
>> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
>> index 27c6ca0..6c4f232 100644
>> --- a/drivers/char/tpm/tpm_tis_core.c
>> +++ b/drivers/char/tpm/tpm_tis_core.c
>> @@ -242,6 +242,15 @@ static u8 tpm_tis_status(struct tpm_chip *chip)
>>      return status;
>>  }
>>  
>> +static bool tpm_tis_check_data(struct tpm_chip *chip, const u8 *buf, size_t len)
>
> Not sure if this is the best possible function name, "check" can
> mean almost anything.
>

Ok, i'm changing it to "verify_data_integrity". is that ok?

>> +{
>> +    struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
>> +
>> +    if (priv->phy_ops->check_data)
>> +        return priv->phy_ops->check_data(priv, buf, len);
>
> New line here before the return statement.
>
>> +    return true;
>> +}
>> +
>>  static void tpm_tis_ready(struct tpm_chip *chip)
>>  {
>>      struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
>> @@ -308,47 +317,59 @@ 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;
>> +    int status, i;
>>      u32 expected;
>> +    bool check_data = false;
>>  
>> -    if (count < TPM_HEADER_SIZE) {
>> -        size = -EIO;
>> -        goto out;
>> -    }
>> +    for (i = 0; i < TPM_RETRY; i++) {
>> +        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;
>> -    }
>> +        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;
>> +        }
>>  
>> -    expected = be32_to_cpu(*(__be32 *) (buf + 2));
>> -    if (expected > count || expected < TPM_HEADER_SIZE) {
>> -        size = -EIO;
>> -        goto out;
>> -    }
>> +        expected = be32_to_cpu(*(__be32 *) (buf + 2));
>> +        if (expected > count || expected < TPM_HEADER_SIZE) {
>> +            size = -EIO;
>> +            goto out;
>> +        }
>>  
>> -    size += recv_data(chip, &buf[TPM_HEADER_SIZE],
>> -              expected - TPM_HEADER_SIZE);
>> -    if (size < expected) {
>> -        dev_err(&chip->dev, "Unable to read remainder of result\n");
>> -        size = -ETIME;
>> -        goto out;
>> -    }
>> +        size += recv_data(chip, &buf[TPM_HEADER_SIZE],
>> +                  expected - TPM_HEADER_SIZE);
>> +        if (size < expected) {
>> +            dev_err(&chip->dev, "Unable to read remainder of result\n");
>> +            size = -ETIME;
>> +            goto out;
>> +        }
>>  
>> -    if (wait_for_tpm_stat(chip, TPM_STS_VALID, chip->timeout_c,
>> -                &priv->int_queue, false) < 0) {
>> -        size = -ETIME;
>> -        goto out;
>> +        if (wait_for_tpm_stat(chip, TPM_STS_VALID, chip->timeout_c,
>> +                      &priv->int_queue, false) < 0) {
>> +            size = -ETIME;
>> +            goto out;
>> +        }
>> +
>> +        status = tpm_tis_status(chip);
>> +        if (status & TPM_STS_DATA_AVAIL) {    /* retry? */
>> +            dev_err(&chip->dev, "Error left over data\n");
>> +            size = -EIO;
>> +            goto out;
>> +        }
>> +
>> +        check_data = tpm_tis_check_data(chip, buf, size);
>> +        if (!check_data)
>> +            tpm_tis_write8(priv, TPM_STS(priv->locality),
>> +                       TPM_STS_RESPONSE_RETRY);
>> +        else
>> +            break;
>>      }
>> -    status = tpm_tis_status(chip);
>> -    if (status & TPM_STS_DATA_AVAIL) {    /* retry? */
>> -        dev_err(&chip->dev, "Error left over data\n");
>> +    if (!check_data)
>>          size = -EIO;
>> -        goto out;
>> -    }
>> -
>>  out:
>>      tpm_tis_ready(chip);
>>      return size;
>> @@ -453,14 +474,19 @@ 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;
>> +    bool data_valid = false;
>>  
>> -    rc = tpm_tis_send_data(chip, buf, len);
>> -    if (rc < 0)
>> -        return rc;
>> -
>> +    for (i = 0; i < TPM_RETRY && !data_valid; i++) {
>> +        rc = tpm_tis_send_data(chip, buf, len);
>> +        if (rc < 0)
>> +            return rc;
>> +        data_valid = tpm_tis_check_data(chip, buf, len);
>> +    }
>> +    if (!data_valid)
>> +        return -EIO;
>>      /* go and do it */
>>      rc = tpm_tis_write8(priv, TPM_STS(priv->locality), TPM_STS_GO);
>>      if (rc < 0)
>> diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h
>> index d06c65b..486c2e9 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 {
>> @@ -106,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 (*check_data)(struct tpm_tis_data *data, const u8 *buf,
>> +               size_t len);
>
> Aren't you validating the contents of the buf?
>
> /Jarkko

i do.
when sending, the data is written to the buff in "tpm_tis_send_data(chip,buf, len)".
and validated in "data_valid = tpm_tis_check_data(chip, buf, len)".
data is not sent until TPM_STS_GO is set.

when receiving, the data in the buffer is verified after recv_data, and
writing to TPM_STS_RESPONSE_RETRY in case it fails to recive it again.


Thank you
Amir Mizinski


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

* Re: [PATCH v6 2/7] tpm: tpm_tis: Add check_data handle to tpm_tis_phy_ops
  2020-04-21 13:34     ` Amir Mizinski
@ 2020-04-21 20:19       ` Jarkko Sakkinen
  0 siblings, 0 replies; 16+ messages in thread
From: Jarkko Sakkinen @ 2020-04-21 20:19 UTC (permalink / raw)
  To: Amir Mizinski
  Cc: Eyal.Cohen, oshrialkoby85, alexander.steffen, robh+dt,
	mark.rutland, peterhuewe, jgg, arnd, gregkh, devicetree,
	linux-kernel, linux-integrity, oshri.alkoby, tmaimon77, gcwilson,
	kgoldman, Dan.Morav, oren.tanami, shmulik.hager, amir.mizinski,
	Christophe Ricard

On Tue, Apr 21, 2020 at 01:34:44PM +0000, Amir Mizinski wrote:
> 
> Hello jarkko,
> I reconfigure my email client by the instructions you've sent, and
> re-responsing as you requested.
> please tell me if there are still any issues. thank you.

This is great, thanks a lot!

Easy enough to check too:

https://lore.kernel.org/linux-integrity/0cfa0486-8ccb-d7d1-acf2-ca103f723b3a@gmail.com/

It wouldn't end up to the ML if your config wasn't correct.

/Jarkko

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

* Re: [PATCH v6 6/7] dt-bindings: tpm: Add YAML schema for TPM TIS I2C options
  2020-04-15 15:20   ` Rob Herring
@ 2020-04-22  8:15     ` Amir Mizinski
  0 siblings, 0 replies; 16+ messages in thread
From: Amir Mizinski @ 2020-04-22  8:15 UTC (permalink / raw)
  To: Rob Herring
  Cc: Eyal.Cohen, jarkko.sakkinen, oshrialkoby85, alexander.steffen,
	mark.rutland, peterhuewe, jgg, arnd, gregkh, devicetree,
	linux-kernel, linux-integrity, oshri.alkoby, tmaimon77, gcwilson,
	kgoldman, Dan.Morav, oren.tanami, shmulik.hager, amir.mizinski


On 2020-04-15 15:20, Rob Herring wrote:
> On Tue, Apr 07, 2020 at 07:20:43PM +0300, amirmizi6@gmail.com wrote:
>> From: Amir Mizinski <amirmizi6@gmail.com>
>>
>> Added a YAML schema to support tpm tis i2c realted dt-bindings for the I2c
>> PTP based physical layer.
>>
>> This patch adds the documentation for corresponding device tree bindings of
>> I2C based Physical TPM.
>> Refer to the 'I2C Interface Definition' section in
>> 'TCG PC Client PlatformTPMProfile(PTP) Specification' publication
>> for specification.
>>
>> Signed-off-by: Amir Mizinski <amirmizi6@gmail.com>
>> ---
>>  .../bindings/security/tpm/tpm-tis-i2c.yaml         | 47 ++++++++++++++++++++++
>>  1 file changed, 47 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/security/tpm/tpm-tis-i2c.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/security/tpm/tpm-tis-i2c.yaml b/Documentation/devicetree/bindings/security/tpm/tpm-tis-i2c.yaml
>> new file mode 100644
>> index 0000000..13d7c2c
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/security/tpm/tpm-tis-i2c.yaml
>> @@ -0,0 +1,47 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/security/tpm/tpm-tis-i2c.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: I2C PTP based TPM Device Tree Bindings
>> +
>> +maintainers:
>> +  - Amir Mizinski <amirmizi6@gmail.com>
>> +
>> +description:
>> +  Device Tree Bindings for I2C based Trusted Platform Module(TPM).
>> +
>> +properties:
>> +  compatible:
>> +    contains:
>> +      const: tcg,tpm-tis-i2c
>
> This is not sufficient. I assume you are testing on some specific TPM
> chip.
>

I am, but this implementation follows the "TCG PC client Device Driver Design Principles for TPM 2.0"
It's not meant solely for out chip.

>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  interrupt:
>> +    maxItems: 1
>> +
>> +  crc-checksum:
>> +    $ref: /schemas/types.yaml#/definitions/flag
>> +    description:
>> +      CRC checksum enable.
>
> Why would you not want CRC? Some chips support and some don't? If so,
> the compatible for the chip should imply that.
>

There's an Enable/Disable CRC option in the TPM chip, not all vendors
use this by default.

>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +
>> +examples:
>> +  - |
>> +    i2c {
>> +      #address-cells = <1>;
>> +      #size-cells = <0>;
>> +
>> +      tpm-tis-i2c@2e {
>
> tpm@2e
>

I understand why i should remove "i2c", but i think it should be "tpm_tis@2e".
Respectively with "tpm_tis_spi.txt" and "tpm_tis_mmio.txt".

>> +        compatible = "tcg,tpm-tis-i2c";
>> +        reg = <0x2e>;
>> +        crc-checksum;
>> +      };
>> +    };
>> +...
>> --
>> 2.7.4
>>

Thank you for your feedback.
Best regards,
Amir Mizinski


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

end of thread, other threads:[~2020-04-22  8:16 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-07 16:20 [PATCH v6 0/7] Add tpm i2c ptp driver amirmizi6
2020-04-07 16:20 ` [PATCH v6 1/7] tpm: tpm_tis: Make implementation of read16 read32 write32 optional amirmizi6
2020-04-08 16:46   ` Jarkko Sakkinen
2020-04-07 16:20 ` [PATCH v6 2/7] tpm: tpm_tis: Add check_data handle to tpm_tis_phy_ops amirmizi6
2020-04-08 18:33   ` Jarkko Sakkinen
     [not found]     ` <CAMHTsUUzQ9rpPfsnEtJruAC3THr+XNA=5Wk7OCteqZXOQ0L=UA@mail.gmail.com>
2020-04-12 13:53       ` Jarkko Sakkinen
2020-04-21 13:34     ` Amir Mizinski
2020-04-21 20:19       ` Jarkko Sakkinen
2020-04-07 16:20 ` [PATCH v6 3/7] tpm: tpm_tis: rewrite "tpm_tis_req_canceled()" amirmizi6
2020-04-07 16:20 ` [PATCH v6 4/7] tpm: tpm_tis: Fix expected bit handling and send all bytes in one shot without last byte in exception amirmizi6
2020-04-07 16:20 ` [PATCH v6 5/7] tpm: Handle an exception for TPM Firmware Update mode amirmizi6
2020-04-07 16:20 ` [PATCH v6 6/7] dt-bindings: tpm: Add YAML schema for TPM TIS I2C options amirmizi6
2020-04-15 15:20   ` Rob Herring
2020-04-22  8:15     ` Amir Mizinski
2020-04-07 16:20 ` [PATCH v6 7/7] tpm: tpm_tis: add tpm_tis_i2c driver amirmizi6
2020-04-08 18:36   ` Jarkko Sakkinen

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).