[PATCH 0/5] Fix whole native SPI TPM driver

[PATCH 0/5] Fix whole native SPI TPM driver

[Peter Huewe]

During our testing it showed that unfortunately the whole native spi tpm driver
was more or less non-functional since it was merged, e.g. the wrong byte for
waitstate handling was used and transfers larger than 64 bytes did not work at all.

This was probably caused by the merging of the different approaches back then,
as the initial RFC patch did not have these problems, and also my sudden lack
of time/commitment back then.
I'm sorry that the final driver code went untested for that long time.

This patch set fixes these issues one by one.
In order to avoid duplication the read/write function was consolidated to one
transfer function, so we do not have to apply the same fix at two locations.
Maybe consider squashing it - we splitted it for easier review.
 
Affected Kernels: 4.8, 4.9, 4.10
Patchset was tested on Raspberry Pi2 with SLB9670 (TPM1.2 and TPM2.0)

Peter Huewe (5):
  tpm_tis_spi: Use single function to transfer data
  tpm_tis_spi: Abort transfer when too many wait states are signaled
  tpm_tis_spi: Check correct byte for wait state indicator
  tpm_tis_spi: Remove limitation of transfers to MAX_SPI_FRAMESIZE bytes
  tpm_tis_spi: Add small delay after last transfer

 drivers/char/tpm/tpm_tis_spi.c | 163 +++++++++++++++++------------------------
 1 file changed, 69 insertions(+), 94 deletions(-)

-- 
2.7.4

[PATCH 0/5] Fix whole native SPI TPM driver

[Peter Huewe]

During our testing it showed that unfortunately the whole native spi tpm driver
was more or less non-functional since it was merged, e.g. the wrong byte for
waitstate handling was used and transfers larger than 64 bytes did not work at all.

This was probably caused by the merging of the different approaches back then,
as the initial RFC patch did not have these problems, and also my sudden lack
of time/commitment back then.
I'm sorry that the final driver code went untested for that long time.

This patch set fixes these issues one by one.
In order to avoid duplication the read/write function was consolidated to one
transfer function, so we do not have to apply the same fix at two locations.
Maybe consider squashing it - we splitted it for easier review.
 
Affected Kernels: 4.8, 4.9, 4.10
Patchset was tested on Raspberry Pi2 with SLB9670 (TPM1.2 and TPM2.0)

Peter Huewe (5):
  tpm_tis_spi: Use single function to transfer data
  tpm_tis_spi: Abort transfer when too many wait states are signaled
  tpm_tis_spi: Check correct byte for wait state indicator
  tpm_tis_spi: Remove limitation of transfers to MAX_SPI_FRAMESIZE bytes
  tpm_tis_spi: Add small delay after last transfer

 drivers/char/tpm/tpm_tis_spi.c | 163 +++++++++++++++++------------------------
 1 file changed, 69 insertions(+), 94 deletions(-)

-- 
2.7.4


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

[PATCH 1/5] tpm_tis_spi: Use single function to transfer data

[Peter Huewe]

The algorithm for sending data to the TPM is mostly identical to the
algorithm for receiving data from the TPM, so a single function is
sufficient to handle both cases.

This is a prequisite for all the other fixes, so we don't have to fix
everything twice (send/receive)

Cc: <stable@vger.kernel.org>
[prerequisite for other fixes in this series]
Fixes: 0edbfea537d1 ("tpm/tpm_tis_spi: Add support for spi phy")
Signed-off-by: Alexander Steffen <Alexander.Steffen@infineon.com>
Signed-off-by: Peter Huewe <peter.huewe@infineon.com>
---
 drivers/char/tpm/tpm_tis_spi.c | 87 ++++++++++++------------------------------
 1 file changed, 24 insertions(+), 63 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis_spi.c b/drivers/char/tpm/tpm_tis_spi.c
index 5292e5768a7e..6e1a3c43f621 100644
--- a/drivers/char/tpm/tpm_tis_spi.c
+++ b/drivers/char/tpm/tpm_tis_spi.c
@@ -47,8 +47,8 @@ struct tpm_tis_spi_phy {
 	struct tpm_tis_data priv;
 	struct spi_device *spi_device;
 
-	u8 tx_buf[MAX_SPI_FRAMESIZE + 4];
-	u8 rx_buf[MAX_SPI_FRAMESIZE + 4];
+	u8 tx_buf[4];
+	u8 rx_buf[4];
 };
 
 static inline struct tpm_tis_spi_phy *to_tpm_tis_spi_phy(struct tpm_tis_data *data)
@@ -56,8 +56,8 @@ static inline struct tpm_tis_spi_phy *to_tpm_tis_spi_phy(struct tpm_tis_data *da
 	return container_of(data, struct tpm_tis_spi_phy, priv);
 }
 
-static int tpm_tis_spi_read_bytes(struct tpm_tis_data *data, u32 addr,
-				  u16 len, u8 *result)
+static int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u8 len,
+				u8 *buffer, u8 direction)
 {
 	struct tpm_tis_spi_phy *phy = to_tpm_tis_spi_phy(data);
 	int ret, i;
@@ -66,17 +66,17 @@ static int tpm_tis_spi_read_bytes(struct tpm_tis_data *data, u32 addr,
 		.tx_buf = phy->tx_buf,
 		.rx_buf = phy->rx_buf,
 		.len = 4,
+		.cs_change = 1,
 	};
 
 	if (len > MAX_SPI_FRAMESIZE)
 		return -ENOMEM;
 
-	phy->tx_buf[0] = 0x80 | (len - 1);
+	phy->tx_buf[0] = direction | (len - 1);
 	phy->tx_buf[1] = 0xd4;
-	phy->tx_buf[2] = (addr >> 8)  & 0xFF;
-	phy->tx_buf[3] = addr	      & 0xFF;
+	phy->tx_buf[2] = addr >> 8;
+	phy->tx_buf[3] = addr;
 
-	spi_xfer.cs_change = 1;
 	spi_message_init(&m);
 	spi_message_add_tail(&spi_xfer, &m);
 
@@ -85,7 +85,7 @@ static int tpm_tis_spi_read_bytes(struct tpm_tis_data *data, u32 addr,
 	if (ret < 0)
 		goto exit;
 
-	memset(phy->tx_buf, 0, len);
+	phy->tx_buf[0] = 0;
 
 	/* According to TCG PTP specification, if there is no TPM present at
 	 * all, then the design has a weak pull-up on MISO. If a TPM is not
@@ -103,7 +103,14 @@ static int tpm_tis_spi_read_bytes(struct tpm_tis_data *data, u32 addr,
 
 	spi_xfer.cs_change = 0;
 	spi_xfer.len = len;
-	spi_xfer.rx_buf = result;
+
+	if (direction) {
+		spi_xfer.tx_buf = NULL;
+		spi_xfer.rx_buf = buffer;
+	} else {
+		spi_xfer.tx_buf = buffer;
+		spi_xfer.rx_buf = NULL;
+	}
 
 	spi_message_init(&m);
 	spi_message_add_tail(&spi_xfer, &m);
@@ -114,62 +121,16 @@ static int tpm_tis_spi_read_bytes(struct tpm_tis_data *data, u32 addr,
 	return ret;
 }
 
+static int tpm_tis_spi_read_bytes(struct tpm_tis_data *data, u32 addr,
+				  u16 len, u8 *result)
+{
+	return tpm_tis_spi_transfer(data, addr, len, result, 0x80);
+}
+
 static int tpm_tis_spi_write_bytes(struct tpm_tis_data *data, u32 addr,
 				   u16 len, u8 *value)
 {
-	struct tpm_tis_spi_phy *phy = to_tpm_tis_spi_phy(data);
-	int ret, i;
-	struct spi_message m;
-	struct spi_transfer spi_xfer = {
-		.tx_buf = phy->tx_buf,
-		.rx_buf = phy->rx_buf,
-		.len = 4,
-	};
-
-	if (len > MAX_SPI_FRAMESIZE)
-		return -ENOMEM;
-
-	phy->tx_buf[0] = len - 1;
-	phy->tx_buf[1] = 0xd4;
-	phy->tx_buf[2] = (addr >> 8)  & 0xFF;
-	phy->tx_buf[3] = addr         & 0xFF;
-
-	spi_xfer.cs_change = 1;
-	spi_message_init(&m);
-	spi_message_add_tail(&spi_xfer, &m);
-
-	spi_bus_lock(phy->spi_device->master);
-	ret = spi_sync_locked(phy->spi_device, &m);
-	if (ret < 0)
-		goto exit;
-
-	memset(phy->tx_buf, 0, len);
-
-	/* According to TCG PTP specification, if there is no TPM present at
-	 * all, then the design has a weak pull-up on MISO. If a TPM is not
-	 * present, a pull-up on MISO means that the SB controller sees a 1,
-	 * and will latch in 0xFF on the read.
-	 */
-	for (i = 0; (phy->rx_buf[0] & 0x01) == 0 && i < TPM_RETRY; i++) {
-		spi_xfer.len = 1;
-		spi_message_init(&m);
-		spi_message_add_tail(&spi_xfer, &m);
-		ret = spi_sync_locked(phy->spi_device, &m);
-		if (ret < 0)
-			goto exit;
-	}
-
-	spi_xfer.len = len;
-	spi_xfer.tx_buf = value;
-	spi_xfer.cs_change = 0;
-	spi_xfer.tx_buf = value;
-	spi_message_init(&m);
-	spi_message_add_tail(&spi_xfer, &m);
-	ret = spi_sync_locked(phy->spi_device, &m);
-
-exit:
-	spi_bus_unlock(phy->spi_device->master);
-	return ret;
+	return tpm_tis_spi_transfer(data, addr, len, value, 0);
 }
 
 static int tpm_tis_spi_read16(struct tpm_tis_data *data, u32 addr, u16 *result)
-- 
2.7.4

[PATCH 1/5] tpm_tis_spi: Use single function to transfer data

[Peter Huewe]

The algorithm for sending data to the TPM is mostly identical to the
algorithm for receiving data from the TPM, so a single function is
sufficient to handle both cases.

This is a prequisite for all the other fixes, so we don't have to fix
everything twice (send/receive)

Cc: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
[prerequisite for other fixes in this series]
Fixes: 0edbfea537d1 ("tpm/tpm_tis_spi: Add support for spi phy")
Signed-off-by: Alexander Steffen <Alexander.Steffen-d0qZbvYSIPpWk0Htik3J/w@public.gmane.org>
Signed-off-by: Peter Huewe <peter.huewe-d0qZbvYSIPpWk0Htik3J/w@public.gmane.org>
---
 drivers/char/tpm/tpm_tis_spi.c | 87 ++++++++++++------------------------------
 1 file changed, 24 insertions(+), 63 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis_spi.c b/drivers/char/tpm/tpm_tis_spi.c
index 5292e5768a7e..6e1a3c43f621 100644
--- a/drivers/char/tpm/tpm_tis_spi.c
+++ b/drivers/char/tpm/tpm_tis_spi.c
@@ -47,8 +47,8 @@ struct tpm_tis_spi_phy {
 	struct tpm_tis_data priv;
 	struct spi_device *spi_device;
 
-	u8 tx_buf[MAX_SPI_FRAMESIZE + 4];
-	u8 rx_buf[MAX_SPI_FRAMESIZE + 4];
+	u8 tx_buf[4];
+	u8 rx_buf[4];
 };
 
 static inline struct tpm_tis_spi_phy *to_tpm_tis_spi_phy(struct tpm_tis_data *data)
@@ -56,8 +56,8 @@ static inline struct tpm_tis_spi_phy *to_tpm_tis_spi_phy(struct tpm_tis_data *da
 	return container_of(data, struct tpm_tis_spi_phy, priv);
 }
 
-static int tpm_tis_spi_read_bytes(struct tpm_tis_data *data, u32 addr,
-				  u16 len, u8 *result)
+static int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u8 len,
+				u8 *buffer, u8 direction)
 {
 	struct tpm_tis_spi_phy *phy = to_tpm_tis_spi_phy(data);
 	int ret, i;
@@ -66,17 +66,17 @@ static int tpm_tis_spi_read_bytes(struct tpm_tis_data *data, u32 addr,
 		.tx_buf = phy->tx_buf,
 		.rx_buf = phy->rx_buf,
 		.len = 4,
+		.cs_change = 1,
 	};
 
 	if (len > MAX_SPI_FRAMESIZE)
 		return -ENOMEM;
 
-	phy->tx_buf[0] = 0x80 | (len - 1);
+	phy->tx_buf[0] = direction | (len - 1);
 	phy->tx_buf[1] = 0xd4;
-	phy->tx_buf[2] = (addr >> 8)  & 0xFF;
-	phy->tx_buf[3] = addr	      & 0xFF;
+	phy->tx_buf[2] = addr >> 8;
+	phy->tx_buf[3] = addr;
 
-	spi_xfer.cs_change = 1;
 	spi_message_init(&m);
 	spi_message_add_tail(&spi_xfer, &m);
 
@@ -85,7 +85,7 @@ static int tpm_tis_spi_read_bytes(struct tpm_tis_data *data, u32 addr,
 	if (ret < 0)
 		goto exit;
 
-	memset(phy->tx_buf, 0, len);
+	phy->tx_buf[0] = 0;
 
 	/* According to TCG PTP specification, if there is no TPM present at
 	 * all, then the design has a weak pull-up on MISO. If a TPM is not
@@ -103,7 +103,14 @@ static int tpm_tis_spi_read_bytes(struct tpm_tis_data *data, u32 addr,
 
 	spi_xfer.cs_change = 0;
 	spi_xfer.len = len;
-	spi_xfer.rx_buf = result;
+
+	if (direction) {
+		spi_xfer.tx_buf = NULL;
+		spi_xfer.rx_buf = buffer;
+	} else {
+		spi_xfer.tx_buf = buffer;
+		spi_xfer.rx_buf = NULL;
+	}
 
 	spi_message_init(&m);
 	spi_message_add_tail(&spi_xfer, &m);
@@ -114,62 +121,16 @@ static int tpm_tis_spi_read_bytes(struct tpm_tis_data *data, u32 addr,
 	return ret;
 }
 
+static int tpm_tis_spi_read_bytes(struct tpm_tis_data *data, u32 addr,
+				  u16 len, u8 *result)
+{
+	return tpm_tis_spi_transfer(data, addr, len, result, 0x80);
+}
+
 static int tpm_tis_spi_write_bytes(struct tpm_tis_data *data, u32 addr,
 				   u16 len, u8 *value)
 {
-	struct tpm_tis_spi_phy *phy = to_tpm_tis_spi_phy(data);
-	int ret, i;
-	struct spi_message m;
-	struct spi_transfer spi_xfer = {
-		.tx_buf = phy->tx_buf,
-		.rx_buf = phy->rx_buf,
-		.len = 4,
-	};
-
-	if (len > MAX_SPI_FRAMESIZE)
-		return -ENOMEM;
-
-	phy->tx_buf[0] = len - 1;
-	phy->tx_buf[1] = 0xd4;
-	phy->tx_buf[2] = (addr >> 8)  & 0xFF;
-	phy->tx_buf[3] = addr         & 0xFF;
-
-	spi_xfer.cs_change = 1;
-	spi_message_init(&m);
-	spi_message_add_tail(&spi_xfer, &m);
-
-	spi_bus_lock(phy->spi_device->master);
-	ret = spi_sync_locked(phy->spi_device, &m);
-	if (ret < 0)
-		goto exit;
-
-	memset(phy->tx_buf, 0, len);
-
-	/* According to TCG PTP specification, if there is no TPM present at
-	 * all, then the design has a weak pull-up on MISO. If a TPM is not
-	 * present, a pull-up on MISO means that the SB controller sees a 1,
-	 * and will latch in 0xFF on the read.
-	 */
-	for (i = 0; (phy->rx_buf[0] & 0x01) == 0 && i < TPM_RETRY; i++) {
-		spi_xfer.len = 1;
-		spi_message_init(&m);
-		spi_message_add_tail(&spi_xfer, &m);
-		ret = spi_sync_locked(phy->spi_device, &m);
-		if (ret < 0)
-			goto exit;
-	}
-
-	spi_xfer.len = len;
-	spi_xfer.tx_buf = value;
-	spi_xfer.cs_change = 0;
-	spi_xfer.tx_buf = value;
-	spi_message_init(&m);
-	spi_message_add_tail(&spi_xfer, &m);
-	ret = spi_sync_locked(phy->spi_device, &m);
-
-exit:
-	spi_bus_unlock(phy->spi_device->master);
-	return ret;
+	return tpm_tis_spi_transfer(data, addr, len, value, 0);
 }
 
 static int tpm_tis_spi_read16(struct tpm_tis_data *data, u32 addr, u16 *result)
-- 
2.7.4


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

[PATCH 2/5] tpm_tis_spi: Abort transfer when too many wait states are signaled

[Peter Huewe]

Abort the transfer with ETIMEDOUT when the TPM signals more than
TPM_RETRY wait states. Continuing with the transfer in this state
will only lead to arbitrary failures in other parts of the code.

Cc: <stable@vger.kernel.org>
Fixes: 0edbfea537d1 ("tpm/tpm_tis_spi: Add support for spi phy")
Signed-off-by: Alexander Steffen <Alexander.Steffen@infineon.com>
Signed-off-by: Peter Huewe <peter.huewe@infineon.com>
---
 drivers/char/tpm/tpm_tis_spi.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/char/tpm/tpm_tis_spi.c b/drivers/char/tpm/tpm_tis_spi.c
index 6e1a3c43f621..d782b9974c14 100644
--- a/drivers/char/tpm/tpm_tis_spi.c
+++ b/drivers/char/tpm/tpm_tis_spi.c
@@ -101,6 +101,11 @@ static int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u8 len,
 			goto exit;
 	}
 
+	if (i == TPM_RETRY) {
+		ret = -ETIMEDOUT;
+		goto exit;
+	}
+
 	spi_xfer.cs_change = 0;
 	spi_xfer.len = len;
 
-- 
2.7.4

[PATCH 2/5] tpm_tis_spi: Abort transfer when too many wait states are signaled

[Peter Huewe]

Abort the transfer with ETIMEDOUT when the TPM signals more than
TPM_RETRY wait states. Continuing with the transfer in this state
will only lead to arbitrary failures in other parts of the code.

Cc: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
Fixes: 0edbfea537d1 ("tpm/tpm_tis_spi: Add support for spi phy")
Signed-off-by: Alexander Steffen <Alexander.Steffen-d0qZbvYSIPpWk0Htik3J/w@public.gmane.org>
Signed-off-by: Peter Huewe <peter.huewe-d0qZbvYSIPpWk0Htik3J/w@public.gmane.org>
---
 drivers/char/tpm/tpm_tis_spi.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/char/tpm/tpm_tis_spi.c b/drivers/char/tpm/tpm_tis_spi.c
index 6e1a3c43f621..d782b9974c14 100644
--- a/drivers/char/tpm/tpm_tis_spi.c
+++ b/drivers/char/tpm/tpm_tis_spi.c
@@ -101,6 +101,11 @@ static int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u8 len,
 			goto exit;
 	}
 
+	if (i == TPM_RETRY) {
+		ret = -ETIMEDOUT;
+		goto exit;
+	}
+
 	spi_xfer.cs_change = 0;
 	spi_xfer.len = len;
 
-- 
2.7.4


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

[PATCH 3/5] tpm_tis_spi: Check correct byte for wait state indicator

[Peter Huewe]

Wait states are signaled in the last byte received from the TPM in
response to the header, not the first byte. Check rx_buf[3] instead of
rx_buf[0].

Cc: <stable@vger.kernel.org>
Fixes: 0edbfea537d1 ("tpm/tpm_tis_spi: Add support for spi phy")
Signed-off-by: Alexander Steffen <Alexander.Steffen@infineon.com>
Signed-off-by: Peter Huewe <peter.huewe@infineon.com>
---
 drivers/char/tpm/tpm_tis_spi.c | 40 +++++++++++++++++++++-------------------
 1 file changed, 21 insertions(+), 19 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis_spi.c b/drivers/char/tpm/tpm_tis_spi.c
index d782b9974c14..16938e2253d2 100644
--- a/drivers/char/tpm/tpm_tis_spi.c
+++ b/drivers/char/tpm/tpm_tis_spi.c
@@ -60,7 +60,7 @@ static int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u8 len,
 				u8 *buffer, u8 direction)
 {
 	struct tpm_tis_spi_phy *phy = to_tpm_tis_spi_phy(data);
-	int ret, i;
+	int ret;
 	struct spi_message m;
 	struct spi_transfer spi_xfer = {
 		.tx_buf = phy->tx_buf,
@@ -85,25 +85,27 @@ static int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u8 len,
 	if (ret < 0)
 		goto exit;
 
-	phy->tx_buf[0] = 0;
-
-	/* According to TCG PTP specification, if there is no TPM present at
-	 * all, then the design has a weak pull-up on MISO. If a TPM is not
-	 * present, a pull-up on MISO means that the SB controller sees a 1,
-	 * and will latch in 0xFF on the read.
-	 */
-	for (i = 0; (phy->rx_buf[0] & 0x01) == 0 && i < TPM_RETRY; i++) {
-		spi_xfer.len = 1;
-		spi_message_init(&m);
-		spi_message_add_tail(&spi_xfer, &m);
-		ret = spi_sync_locked(phy->spi_device, &m);
-		if (ret < 0)
+	if ((phy->rx_buf[3] & 0x01) == 0) {
+		// handle SPI wait states
+		int i;
+
+		phy->tx_buf[0] = 0;
+
+		for (i = 0; i < TPM_RETRY; i++) {
+			spi_xfer.len = 1;
+			spi_message_init(&m);
+			spi_message_add_tail(&spi_xfer, &m);
+			ret = spi_sync_locked(phy->spi_device, &m);
+			if (ret < 0)
+				goto exit;
+			if (phy->rx_buf[0] & 0x01)
+				break;
+		}
+
+		if (i == TPM_RETRY) {
+			ret = -ETIMEDOUT;
 			goto exit;
-	}
-
-	if (i == TPM_RETRY) {
-		ret = -ETIMEDOUT;
-		goto exit;
+		}
 	}
 
 	spi_xfer.cs_change = 0;
-- 
2.7.4

[PATCH 3/5] tpm_tis_spi: Check correct byte for wait state indicator

[Peter Huewe]

Wait states are signaled in the last byte received from the TPM in
response to the header, not the first byte. Check rx_buf[3] instead of
rx_buf[0].

Cc: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
Fixes: 0edbfea537d1 ("tpm/tpm_tis_spi: Add support for spi phy")
Signed-off-by: Alexander Steffen <Alexander.Steffen-d0qZbvYSIPpWk0Htik3J/w@public.gmane.org>
Signed-off-by: Peter Huewe <peter.huewe-d0qZbvYSIPpWk0Htik3J/w@public.gmane.org>
---
 drivers/char/tpm/tpm_tis_spi.c | 40 +++++++++++++++++++++-------------------
 1 file changed, 21 insertions(+), 19 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis_spi.c b/drivers/char/tpm/tpm_tis_spi.c
index d782b9974c14..16938e2253d2 100644
--- a/drivers/char/tpm/tpm_tis_spi.c
+++ b/drivers/char/tpm/tpm_tis_spi.c
@@ -60,7 +60,7 @@ static int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u8 len,
 				u8 *buffer, u8 direction)
 {
 	struct tpm_tis_spi_phy *phy = to_tpm_tis_spi_phy(data);
-	int ret, i;
+	int ret;
 	struct spi_message m;
 	struct spi_transfer spi_xfer = {
 		.tx_buf = phy->tx_buf,
@@ -85,25 +85,27 @@ static int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u8 len,
 	if (ret < 0)
 		goto exit;
 
-	phy->tx_buf[0] = 0;
-
-	/* According to TCG PTP specification, if there is no TPM present at
-	 * all, then the design has a weak pull-up on MISO. If a TPM is not
-	 * present, a pull-up on MISO means that the SB controller sees a 1,
-	 * and will latch in 0xFF on the read.
-	 */
-	for (i = 0; (phy->rx_buf[0] & 0x01) == 0 && i < TPM_RETRY; i++) {
-		spi_xfer.len = 1;
-		spi_message_init(&m);
-		spi_message_add_tail(&spi_xfer, &m);
-		ret = spi_sync_locked(phy->spi_device, &m);
-		if (ret < 0)
+	if ((phy->rx_buf[3] & 0x01) == 0) {
+		// handle SPI wait states
+		int i;
+
+		phy->tx_buf[0] = 0;
+
+		for (i = 0; i < TPM_RETRY; i++) {
+			spi_xfer.len = 1;
+			spi_message_init(&m);
+			spi_message_add_tail(&spi_xfer, &m);
+			ret = spi_sync_locked(phy->spi_device, &m);
+			if (ret < 0)
+				goto exit;
+			if (phy->rx_buf[0] & 0x01)
+				break;
+		}
+
+		if (i == TPM_RETRY) {
+			ret = -ETIMEDOUT;
 			goto exit;
-	}
-
-	if (i == TPM_RETRY) {
-		ret = -ETIMEDOUT;
-		goto exit;
+		}
 	}
 
 	spi_xfer.cs_change = 0;
-- 
2.7.4


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

[PATCH 4/5] tpm_tis_spi: Remove limitation of transfers to MAX_SPI_FRAMESIZE bytes

[Peter Huewe]

Limiting transfers to MAX_SPI_FRAMESIZE was not expected by the upper
layers, as tpm_tis has no such limitation. Add a loop to hide that
limitation.

Cc: <stable@vger.kernel.org>
Fixes: 0edbfea537d1 ("tpm/tpm_tis_spi: Add support for spi phy")
Signed-off-by: Alexander Steffen <Alexander.Steffen@infineon.com>
Signed-off-by: Peter Huewe <peter.huewe@infineon.com>
---
 drivers/char/tpm/tpm_tis_spi.c | 108 ++++++++++++++++++++++-------------------
 1 file changed, 57 insertions(+), 51 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis_spi.c b/drivers/char/tpm/tpm_tis_spi.c
index 16938e2253d2..b50c5b072df3 100644
--- a/drivers/char/tpm/tpm_tis_spi.c
+++ b/drivers/char/tpm/tpm_tis_spi.c
@@ -61,68 +61,74 @@ static int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u8 len,
 {
 	struct tpm_tis_spi_phy *phy = to_tpm_tis_spi_phy(data);
 	int ret;
-	struct spi_message m;
-	struct spi_transfer spi_xfer = {
-		.tx_buf = phy->tx_buf,
-		.rx_buf = phy->rx_buf,
-		.len = 4,
-		.cs_change = 1,
-	};
-
-	if (len > MAX_SPI_FRAMESIZE)
-		return -ENOMEM;
 
-	phy->tx_buf[0] = direction | (len - 1);
-	phy->tx_buf[1] = 0xd4;
-	phy->tx_buf[2] = addr >> 8;
-	phy->tx_buf[3] = addr;
+	spi_bus_lock(phy->spi_device->master);
 
-	spi_message_init(&m);
-	spi_message_add_tail(&spi_xfer, &m);
+	while (len) {
+		struct spi_message m;
+		struct spi_transfer spi_xfer = {
+			.tx_buf = phy->tx_buf,
+			.rx_buf = phy->rx_buf,
+			.len = 4,
+			.cs_change = 1,
+		};
+		u8 transfer_len = min_t(u16, len, MAX_SPI_FRAMESIZE);
+
+		phy->tx_buf[0] = direction | (transfer_len - 1);
+		phy->tx_buf[1] = 0xd4;
+		phy->tx_buf[2] = addr >> 8;
+		phy->tx_buf[3] = addr;
+
+		spi_message_init(&m);
+		spi_message_add_tail(&spi_xfer, &m);
+		ret = spi_sync_locked(phy->spi_device, &m);
+		if (ret < 0)
+			goto exit;
 
-	spi_bus_lock(phy->spi_device->master);
-	ret = spi_sync_locked(phy->spi_device, &m);
-	if (ret < 0)
-		goto exit;
-
-	if ((phy->rx_buf[3] & 0x01) == 0) {
-		// handle SPI wait states
-		int i;
-
-		phy->tx_buf[0] = 0;
-
-		for (i = 0; i < TPM_RETRY; i++) {
-			spi_xfer.len = 1;
-			spi_message_init(&m);
-			spi_message_add_tail(&spi_xfer, &m);
-			ret = spi_sync_locked(phy->spi_device, &m);
-			if (ret < 0)
+		if ((phy->rx_buf[3] & 0x01) == 0) {
+			// handle SPI wait states
+			int i;
+
+			phy->tx_buf[0] = 0;
+
+			for (i = 0; i < TPM_RETRY; i++) {
+				spi_xfer.len = 1;
+				spi_message_init(&m);
+				spi_message_add_tail(&spi_xfer, &m);
+				ret = spi_sync_locked(phy->spi_device, &m);
+				if (ret < 0)
+					goto exit;
+				if (phy->rx_buf[0] & 0x01)
+					break;
+			}
+
+			if (i == TPM_RETRY) {
+				ret = -ETIMEDOUT;
 				goto exit;
-			if (phy->rx_buf[0] & 0x01)
-				break;
+			}
 		}
 
-		if (i == TPM_RETRY) {
-			ret = -ETIMEDOUT;
-			goto exit;
+		spi_xfer.cs_change = 0;
+		spi_xfer.len = transfer_len;
+
+		if (direction) {
+			spi_xfer.tx_buf = NULL;
+			spi_xfer.rx_buf = buffer;
+		} else {
+			spi_xfer.tx_buf = buffer;
+			spi_xfer.rx_buf = NULL;
 		}
-	}
 
-	spi_xfer.cs_change = 0;
-	spi_xfer.len = len;
+		spi_message_init(&m);
+		spi_message_add_tail(&spi_xfer, &m);
+		ret = spi_sync_locked(phy->spi_device, &m);
+		if (ret < 0)
+			goto exit;
 
-	if (direction) {
-		spi_xfer.tx_buf = NULL;
-		spi_xfer.rx_buf = buffer;
-	} else {
-		spi_xfer.tx_buf = buffer;
-		spi_xfer.rx_buf = NULL;
+		len -= transfer_len;
+		buffer += transfer_len;
 	}
 
-	spi_message_init(&m);
-	spi_message_add_tail(&spi_xfer, &m);
-	ret = spi_sync_locked(phy->spi_device, &m);
-
 exit:
 	spi_bus_unlock(phy->spi_device->master);
 	return ret;
-- 
2.7.4

[PATCH 4/5] tpm_tis_spi: Remove limitation of transfers to MAX_SPI_FRAMESIZE bytes

[Peter Huewe]

Limiting transfers to MAX_SPI_FRAMESIZE was not expected by the upper
layers, as tpm_tis has no such limitation. Add a loop to hide that
limitation.

Cc: <stable@vger.kernel.org>
Fixes: 0edbfea537d1 ("tpm/tpm_tis_spi: Add support for spi phy")
Signed-off-by: Alexander Steffen <Alexander.Steffen@infineon.com>
Signed-off-by: Peter Huewe <peter.huewe@infineon.com>
---
 drivers/char/tpm/tpm_tis_spi.c | 108 ++++++++++++++++++++++-------------------
 1 file changed, 57 insertions(+), 51 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis_spi.c b/drivers/char/tpm/tpm_tis_spi.c
index 16938e2253d2..b50c5b072df3 100644
--- a/drivers/char/tpm/tpm_tis_spi.c
+++ b/drivers/char/tpm/tpm_tis_spi.c
@@ -61,68 +61,74 @@ static int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u8 len,
 {
 	struct tpm_tis_spi_phy *phy = to_tpm_tis_spi_phy(data);
 	int ret;
-	struct spi_message m;
-	struct spi_transfer spi_xfer = {
-		.tx_buf = phy->tx_buf,
-		.rx_buf = phy->rx_buf,
-		.len = 4,
-		.cs_change = 1,
-	};
-
-	if (len > MAX_SPI_FRAMESIZE)
-		return -ENOMEM;
 
-	phy->tx_buf[0] = direction | (len - 1);
-	phy->tx_buf[1] = 0xd4;
-	phy->tx_buf[2] = addr >> 8;
-	phy->tx_buf[3] = addr;
+	spi_bus_lock(phy->spi_device->master);
 
-	spi_message_init(&m);
-	spi_message_add_tail(&spi_xfer, &m);
+	while (len) {
+		struct spi_message m;
+		struct spi_transfer spi_xfer = {
+			.tx_buf = phy->tx_buf,
+			.rx_buf = phy->rx_buf,
+			.len = 4,
+			.cs_change = 1,
+		};
+		u8 transfer_len = min_t(u16, len, MAX_SPI_FRAMESIZE);
+
+		phy->tx_buf[0] = direction | (transfer_len - 1);
+		phy->tx_buf[1] = 0xd4;
+		phy->tx_buf[2] = addr >> 8;
+		phy->tx_buf[3] = addr;
+
+		spi_message_init(&m);
+		spi_message_add_tail(&spi_xfer, &m);
+		ret = spi_sync_locked(phy->spi_device, &m);
+		if (ret < 0)
+			goto exit;
 
-	spi_bus_lock(phy->spi_device->master);
-	ret = spi_sync_locked(phy->spi_device, &m);
-	if (ret < 0)
-		goto exit;
-
-	if ((phy->rx_buf[3] & 0x01) == 0) {
-		// handle SPI wait states
-		int i;
-
-		phy->tx_buf[0] = 0;
-
-		for (i = 0; i < TPM_RETRY; i++) {
-			spi_xfer.len = 1;
-			spi_message_init(&m);
-			spi_message_add_tail(&spi_xfer, &m);
-			ret = spi_sync_locked(phy->spi_device, &m);
-			if (ret < 0)
+		if ((phy->rx_buf[3] & 0x01) == 0) {
+			// handle SPI wait states
+			int i;
+
+			phy->tx_buf[0] = 0;
+
+			for (i = 0; i < TPM_RETRY; i++) {
+				spi_xfer.len = 1;
+				spi_message_init(&m);
+				spi_message_add_tail(&spi_xfer, &m);
+				ret = spi_sync_locked(phy->spi_device, &m);
+				if (ret < 0)
+					goto exit;
+				if (phy->rx_buf[0] & 0x01)
+					break;
+			}
+
+			if (i == TPM_RETRY) {
+				ret = -ETIMEDOUT;
 				goto exit;
-			if (phy->rx_buf[0] & 0x01)
-				break;
+			}
 		}
 
-		if (i == TPM_RETRY) {
-			ret = -ETIMEDOUT;
-			goto exit;
+		spi_xfer.cs_change = 0;
+		spi_xfer.len = transfer_len;
+
+		if (direction) {
+			spi_xfer.tx_buf = NULL;
+			spi_xfer.rx_buf = buffer;
+		} else {
+			spi_xfer.tx_buf = buffer;
+			spi_xfer.rx_buf = NULL;
 		}
-	}
 
-	spi_xfer.cs_change = 0;
-	spi_xfer.len = len;
+		spi_message_init(&m);
+		spi_message_add_tail(&spi_xfer, &m);
+		ret = spi_sync_locked(phy->spi_device, &m);
+		if (ret < 0)
+			goto exit;
 
-	if (direction) {
-		spi_xfer.tx_buf = NULL;
-		spi_xfer.rx_buf = buffer;
-	} else {
-		spi_xfer.tx_buf = buffer;
-		spi_xfer.rx_buf = NULL;
+		len -= transfer_len;
+		buffer += transfer_len;
 	}
 
-	spi_message_init(&m);
-	spi_message_add_tail(&spi_xfer, &m);
-	ret = spi_sync_locked(phy->spi_device, &m);
-
 exit:
 	spi_bus_unlock(phy->spi_device->master);
 	return ret;
-- 
2.7.4

Re: [PATCH 4/5] tpm_tis_spi: Remove limitation of transfers to MAX_SPI_FRAMESIZE bytes

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2992 bytes --]

Hi Peter,

[auto build test WARNING on char-misc/char-misc-testing]
[also build test WARNING on v4.10-rc8 next-20170216]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Peter-Huewe/Fix-whole-native-SPI-TPM-driver/20170217-010419
config: x86_64-randconfig-x001-201707 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

Note: it may well be a FALSE warning. FWIW you are at least aware of it now.
http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings

All warnings (new ones prefixed by >>):

   drivers/char/tpm/tpm_tis_spi.c: In function 'tpm_tis_spi_transfer':
>> drivers/char/tpm/tpm_tis_spi.c:135:9: warning: 'ret' may be used uninitialized in this function [-Wmaybe-uninitialized]
     return ret;
            ^~~

vim +/ret +135 drivers/char/tpm/tpm_tis_spi.c

dae9219c Peter Huewe       2017-02-16  119  			spi_xfer.tx_buf = buffer;
dae9219c Peter Huewe       2017-02-16  120  			spi_xfer.rx_buf = NULL;
dae9219c Peter Huewe       2017-02-16  121  		}
0edbfea5 Christophe Ricard 2016-05-19  122  
0edbfea5 Christophe Ricard 2016-05-19  123  		spi_message_init(&m);
0edbfea5 Christophe Ricard 2016-05-19  124  		spi_message_add_tail(&spi_xfer, &m);
0edbfea5 Christophe Ricard 2016-05-19  125  		ret = spi_sync_locked(phy->spi_device, &m);
4dea582e Peter Huewe       2017-02-16  126  		if (ret < 0)
4dea582e Peter Huewe       2017-02-16  127  			goto exit;
4dea582e Peter Huewe       2017-02-16  128  
4dea582e Peter Huewe       2017-02-16  129  		len -= transfer_len;
4dea582e Peter Huewe       2017-02-16  130  		buffer += transfer_len;
4dea582e Peter Huewe       2017-02-16  131  	}
0edbfea5 Christophe Ricard 2016-05-19  132  
0edbfea5 Christophe Ricard 2016-05-19  133  exit:
0edbfea5 Christophe Ricard 2016-05-19  134  	spi_bus_unlock(phy->spi_device->master);
0edbfea5 Christophe Ricard 2016-05-19 @135  	return ret;
0edbfea5 Christophe Ricard 2016-05-19  136  }
0edbfea5 Christophe Ricard 2016-05-19  137  
dae9219c Peter Huewe       2017-02-16  138  static int tpm_tis_spi_read_bytes(struct tpm_tis_data *data, u32 addr,
dae9219c Peter Huewe       2017-02-16  139  				  u16 len, u8 *result)
0edbfea5 Christophe Ricard 2016-05-19  140  {
dae9219c Peter Huewe       2017-02-16  141  	return tpm_tis_spi_transfer(data, addr, len, result, 0x80);
0edbfea5 Christophe Ricard 2016-05-19  142  }
0edbfea5 Christophe Ricard 2016-05-19  143  

:::::: The code at line 135 was first introduced by commit
:::::: 0edbfea537d10c0de5505d0413368aad71027663 tpm/tpm_tis_spi: Add support for spi phy

:::::: TO: Christophe Ricard <christophe.ricard@gmail.com>
:::::: CC: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 26040 bytes --]

Re: [PATCH 4/5] tpm_tis_spi: Remove limitation of transfers to MAX_SPI_FRAMESIZE bytes

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3046 bytes --]

Hi Peter,

[auto build test WARNING on char-misc/char-misc-testing]
[also build test WARNING on v4.10-rc8 next-20170216]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Peter-Huewe/Fix-whole-native-SPI-TPM-driver/20170217-010419
config: x86_64-randconfig-x001-201707 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

Note: it may well be a FALSE warning. FWIW you are at least aware of it now.
http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings

All warnings (new ones prefixed by >>):

   drivers/char/tpm/tpm_tis_spi.c: In function 'tpm_tis_spi_transfer':
>> drivers/char/tpm/tpm_tis_spi.c:135:9: warning: 'ret' may be used uninitialized in this function [-Wmaybe-uninitialized]
     return ret;
            ^~~

vim +/ret +135 drivers/char/tpm/tpm_tis_spi.c

dae9219c Peter Huewe       2017-02-16  119  			spi_xfer.tx_buf = buffer;
dae9219c Peter Huewe       2017-02-16  120  			spi_xfer.rx_buf = NULL;
dae9219c Peter Huewe       2017-02-16  121  		}
0edbfea5 Christophe Ricard 2016-05-19  122  
0edbfea5 Christophe Ricard 2016-05-19  123  		spi_message_init(&m);
0edbfea5 Christophe Ricard 2016-05-19  124  		spi_message_add_tail(&spi_xfer, &m);
0edbfea5 Christophe Ricard 2016-05-19  125  		ret = spi_sync_locked(phy->spi_device, &m);
4dea582e Peter Huewe       2017-02-16  126  		if (ret < 0)
4dea582e Peter Huewe       2017-02-16  127  			goto exit;
4dea582e Peter Huewe       2017-02-16  128  
4dea582e Peter Huewe       2017-02-16  129  		len -= transfer_len;
4dea582e Peter Huewe       2017-02-16  130  		buffer += transfer_len;
4dea582e Peter Huewe       2017-02-16  131  	}
0edbfea5 Christophe Ricard 2016-05-19  132  
0edbfea5 Christophe Ricard 2016-05-19  133  exit:
0edbfea5 Christophe Ricard 2016-05-19  134  	spi_bus_unlock(phy->spi_device->master);
0edbfea5 Christophe Ricard 2016-05-19 @135  	return ret;
0edbfea5 Christophe Ricard 2016-05-19  136  }
0edbfea5 Christophe Ricard 2016-05-19  137  
dae9219c Peter Huewe       2017-02-16  138  static int tpm_tis_spi_read_bytes(struct tpm_tis_data *data, u32 addr,
dae9219c Peter Huewe       2017-02-16  139  				  u16 len, u8 *result)
0edbfea5 Christophe Ricard 2016-05-19  140  {
dae9219c Peter Huewe       2017-02-16  141  	return tpm_tis_spi_transfer(data, addr, len, result, 0x80);
0edbfea5 Christophe Ricard 2016-05-19  142  }
0edbfea5 Christophe Ricard 2016-05-19  143  

:::::: The code at line 135 was first introduced by commit
:::::: 0edbfea537d10c0de5505d0413368aad71027663 tpm/tpm_tis_spi: Add support for spi phy

:::::: TO: Christophe Ricard <christophe.ricard-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
:::::: CC: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 26040 bytes --]

[-- Attachment #3: Type: text/plain, Size: 202 bytes --]

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

[-- Attachment #4: Type: text/plain, Size: 192 bytes --]

_______________________________________________
tpmdd-devel mailing list
tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [PATCH 2/5] tpm_tis_spi: Abort transfer when too many wait states are signaled

[Christophe Ricard]

Nitpick: Are you sure this patch is necessary having #3 on top of it ?

On 16/02/2017 08:08, Peter Huewe wrote:
> Abort the transfer with ETIMEDOUT when the TPM signals more than
> TPM_RETRY wait states. Continuing with the transfer in this state
> will only lead to arbitrary failures in other parts of the code.
>
> Cc: <stable@vger.kernel.org>
> Fixes: 0edbfea537d1 ("tpm/tpm_tis_spi: Add support for spi phy")
> Signed-off-by: Alexander Steffen <Alexander.Steffen@infineon.com>
> Signed-off-by: Peter Huewe <peter.huewe@infineon.com>
> ---
>   drivers/char/tpm/tpm_tis_spi.c | 5 +++++
>   1 file changed, 5 insertions(+)
>
> diff --git a/drivers/char/tpm/tpm_tis_spi.c b/drivers/char/tpm/tpm_tis_spi.c
> index 6e1a3c43f621..d782b9974c14 100644
> --- a/drivers/char/tpm/tpm_tis_spi.c
> +++ b/drivers/char/tpm/tpm_tis_spi.c
> @@ -101,6 +101,11 @@ static int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u8 len,
>   			goto exit;
>   	}
>   
> +	if (i == TPM_RETRY) {
> +		ret = -ETIMEDOUT;
> +		goto exit;
> +	}
> +
>   	spi_xfer.cs_change = 0;
>   	spi_xfer.len = len;
>   

Re: [PATCH 3/5] tpm_tis_spi: Check correct byte for wait state indicator

[Christophe Ricard]

That's is correct, this is a mistake on my side and never saw it :-(.

I guess it was possibly leading to "waste" at least 1 wait state on some 
TPMs.

Wouldn't it be better to merge that with #1 and update the comment 
consequently?


On 16/02/2017 08:08, Peter Huewe wrote:
> Wait states are signaled in the last byte received from the TPM in
> response to the header, not the first byte. Check rx_buf[3] instead of
> rx_buf[0].
>
> Cc: <stable@vger.kernel.org>
> Fixes: 0edbfea537d1 ("tpm/tpm_tis_spi: Add support for spi phy")
> Signed-off-by: Alexander Steffen <Alexander.Steffen@infineon.com>
> Signed-off-by: Peter Huewe <peter.huewe@infineon.com>
> ---
>   drivers/char/tpm/tpm_tis_spi.c | 40 +++++++++++++++++++++-------------------
>   1 file changed, 21 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm_tis_spi.c b/drivers/char/tpm/tpm_tis_spi.c
> index d782b9974c14..16938e2253d2 100644
> --- a/drivers/char/tpm/tpm_tis_spi.c
> +++ b/drivers/char/tpm/tpm_tis_spi.c
> @@ -60,7 +60,7 @@ static int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u8 len,
>   				u8 *buffer, u8 direction)
>   {
>   	struct tpm_tis_spi_phy *phy = to_tpm_tis_spi_phy(data);
> -	int ret, i;
> +	int ret;
>   	struct spi_message m;
>   	struct spi_transfer spi_xfer = {
>   		.tx_buf = phy->tx_buf,
> @@ -85,25 +85,27 @@ static int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u8 len,
>   	if (ret < 0)
>   		goto exit;
>   
> -	phy->tx_buf[0] = 0;
> -
> -	/* According to TCG PTP specification, if there is no TPM present at
> -	 * all, then the design has a weak pull-up on MISO. If a TPM is not
> -	 * present, a pull-up on MISO means that the SB controller sees a 1,
> -	 * and will latch in 0xFF on the read.
> -	 */
> -	for (i = 0; (phy->rx_buf[0] & 0x01) == 0 && i < TPM_RETRY; i++) {
> -		spi_xfer.len = 1;
> -		spi_message_init(&m);
> -		spi_message_add_tail(&spi_xfer, &m);
> -		ret = spi_sync_locked(phy->spi_device, &m);
> -		if (ret < 0)
> +	if ((phy->rx_buf[3] & 0x01) == 0) {
> +		// handle SPI wait states
> +		int i;
> +
> +		phy->tx_buf[0] = 0;
> +
> +		for (i = 0; i < TPM_RETRY; i++) {
> +			spi_xfer.len = 1;
> +			spi_message_init(&m);
> +			spi_message_add_tail(&spi_xfer, &m);
> +			ret = spi_sync_locked(phy->spi_device, &m);
> +			if (ret < 0)
> +				goto exit;
> +			if (phy->rx_buf[0] & 0x01)
> +				break;
> +		}
> +
> +		if (i == TPM_RETRY) {
> +			ret = -ETIMEDOUT;
>   			goto exit;
> -	}
> -
> -	if (i == TPM_RETRY) {
> -		ret = -ETIMEDOUT;
> -		goto exit;
> +		}
>   	}
>   
>   	spi_xfer.cs_change = 0;

Re: [PATCH 3/5] tpm_tis_spi: Check correct byte for wait state indicator

[Christophe Ricard]

That's is correct, this is a mistake on my side and never saw it :-(.

I guess it was possibly leading to "waste" at least 1 wait state on some 
TPMs.

Wouldn't it be better to merge that with #1 and update the comment 
consequently?


On 16/02/2017 08:08, Peter Huewe wrote:
> Wait states are signaled in the last byte received from the TPM in
> response to the header, not the first byte. Check rx_buf[3] instead of
> rx_buf[0].
>
> Cc: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
> Fixes: 0edbfea537d1 ("tpm/tpm_tis_spi: Add support for spi phy")
> Signed-off-by: Alexander Steffen <Alexander.Steffen-d0qZbvYSIPpWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Peter Huewe <peter.huewe-d0qZbvYSIPpWk0Htik3J/w@public.gmane.org>
> ---
>   drivers/char/tpm/tpm_tis_spi.c | 40 +++++++++++++++++++++-------------------
>   1 file changed, 21 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm_tis_spi.c b/drivers/char/tpm/tpm_tis_spi.c
> index d782b9974c14..16938e2253d2 100644
> --- a/drivers/char/tpm/tpm_tis_spi.c
> +++ b/drivers/char/tpm/tpm_tis_spi.c
> @@ -60,7 +60,7 @@ static int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u8 len,
>   				u8 *buffer, u8 direction)
>   {
>   	struct tpm_tis_spi_phy *phy = to_tpm_tis_spi_phy(data);
> -	int ret, i;
> +	int ret;
>   	struct spi_message m;
>   	struct spi_transfer spi_xfer = {
>   		.tx_buf = phy->tx_buf,
> @@ -85,25 +85,27 @@ static int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u8 len,
>   	if (ret < 0)
>   		goto exit;
>   
> -	phy->tx_buf[0] = 0;
> -
> -	/* According to TCG PTP specification, if there is no TPM present at
> -	 * all, then the design has a weak pull-up on MISO. If a TPM is not
> -	 * present, a pull-up on MISO means that the SB controller sees a 1,
> -	 * and will latch in 0xFF on the read.
> -	 */
> -	for (i = 0; (phy->rx_buf[0] & 0x01) == 0 && i < TPM_RETRY; i++) {
> -		spi_xfer.len = 1;
> -		spi_message_init(&m);
> -		spi_message_add_tail(&spi_xfer, &m);
> -		ret = spi_sync_locked(phy->spi_device, &m);
> -		if (ret < 0)
> +	if ((phy->rx_buf[3] & 0x01) == 0) {
> +		// handle SPI wait states
> +		int i;
> +
> +		phy->tx_buf[0] = 0;
> +
> +		for (i = 0; i < TPM_RETRY; i++) {
> +			spi_xfer.len = 1;
> +			spi_message_init(&m);
> +			spi_message_add_tail(&spi_xfer, &m);
> +			ret = spi_sync_locked(phy->spi_device, &m);
> +			if (ret < 0)
> +				goto exit;
> +			if (phy->rx_buf[0] & 0x01)
> +				break;
> +		}
> +
> +		if (i == TPM_RETRY) {
> +			ret = -ETIMEDOUT;
>   			goto exit;
> -	}
> -
> -	if (i == TPM_RETRY) {
> -		ret = -ETIMEDOUT;
> -		goto exit;
> +		}
>   	}
>   
>   	spi_xfer.cs_change = 0;


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

Re: [PATCH 4/5] tpm_tis_spi: Remove limitation of transfers to MAX_SPI_FRAMESIZE bytes

[Christophe Ricard]

I am not sure i understand here, are you considering there could be 
burstcount > 64 with "TCG" TPM ?

Or is this because of TIS vs PTP differences ?

To be honest, this is a little behind me now :-)


On 16/02/2017 08:08, Peter Huewe wrote:
> Limiting transfers to MAX_SPI_FRAMESIZE was not expected by the upper
> layers, as tpm_tis has no such limitation. Add a loop to hide that
> limitation.
>
> Cc: <stable@vger.kernel.org>
> Fixes: 0edbfea537d1 ("tpm/tpm_tis_spi: Add support for spi phy")
> Signed-off-by: Alexander Steffen <Alexander.Steffen@infineon.com>
> Signed-off-by: Peter Huewe <peter.huewe@infineon.com>
> ---
>   drivers/char/tpm/tpm_tis_spi.c | 108 ++++++++++++++++++++++-------------------
>   1 file changed, 57 insertions(+), 51 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm_tis_spi.c b/drivers/char/tpm/tpm_tis_spi.c
> index 16938e2253d2..b50c5b072df3 100644
> --- a/drivers/char/tpm/tpm_tis_spi.c
> +++ b/drivers/char/tpm/tpm_tis_spi.c
> @@ -61,68 +61,74 @@ static int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u8 len,
>   {
>   	struct tpm_tis_spi_phy *phy = to_tpm_tis_spi_phy(data);
>   	int ret;
> -	struct spi_message m;
> -	struct spi_transfer spi_xfer = {
> -		.tx_buf = phy->tx_buf,
> -		.rx_buf = phy->rx_buf,
> -		.len = 4,
> -		.cs_change = 1,
> -	};
> -
> -	if (len > MAX_SPI_FRAMESIZE)
> -		return -ENOMEM;
>   
> -	phy->tx_buf[0] = direction | (len - 1);
> -	phy->tx_buf[1] = 0xd4;
> -	phy->tx_buf[2] = addr >> 8;
> -	phy->tx_buf[3] = addr;
> +	spi_bus_lock(phy->spi_device->master);
>   
> -	spi_message_init(&m);
> -	spi_message_add_tail(&spi_xfer, &m);
> +	while (len) {
> +		struct spi_message m;
> +		struct spi_transfer spi_xfer = {
> +			.tx_buf = phy->tx_buf,
> +			.rx_buf = phy->rx_buf,
> +			.len = 4,
> +			.cs_change = 1,
> +		};
> +		u8 transfer_len = min_t(u16, len, MAX_SPI_FRAMESIZE);
> +
> +		phy->tx_buf[0] = direction | (transfer_len - 1);
> +		phy->tx_buf[1] = 0xd4;
> +		phy->tx_buf[2] = addr >> 8;
> +		phy->tx_buf[3] = addr;
> +
> +		spi_message_init(&m);
> +		spi_message_add_tail(&spi_xfer, &m);
> +		ret = spi_sync_locked(phy->spi_device, &m);
> +		if (ret < 0)
> +			goto exit;
>   
> -	spi_bus_lock(phy->spi_device->master);
> -	ret = spi_sync_locked(phy->spi_device, &m);
> -	if (ret < 0)
> -		goto exit;
> -
> -	if ((phy->rx_buf[3] & 0x01) == 0) {
> -		// handle SPI wait states
> -		int i;
> -
> -		phy->tx_buf[0] = 0;
> -
> -		for (i = 0; i < TPM_RETRY; i++) {
> -			spi_xfer.len = 1;
> -			spi_message_init(&m);
> -			spi_message_add_tail(&spi_xfer, &m);
> -			ret = spi_sync_locked(phy->spi_device, &m);
> -			if (ret < 0)
> +		if ((phy->rx_buf[3] & 0x01) == 0) {
> +			// handle SPI wait states
> +			int i;
> +
> +			phy->tx_buf[0] = 0;
> +
> +			for (i = 0; i < TPM_RETRY; i++) {
> +				spi_xfer.len = 1;
> +				spi_message_init(&m);
> +				spi_message_add_tail(&spi_xfer, &m);
> +				ret = spi_sync_locked(phy->spi_device, &m);
> +				if (ret < 0)
> +					goto exit;
> +				if (phy->rx_buf[0] & 0x01)
> +					break;
> +			}
> +
> +			if (i == TPM_RETRY) {
> +				ret = -ETIMEDOUT;
>   				goto exit;
> -			if (phy->rx_buf[0] & 0x01)
> -				break;
> +			}
>   		}
>   
> -		if (i == TPM_RETRY) {
> -			ret = -ETIMEDOUT;
> -			goto exit;
> +		spi_xfer.cs_change = 0;
> +		spi_xfer.len = transfer_len;
> +
> +		if (direction) {
> +			spi_xfer.tx_buf = NULL;
> +			spi_xfer.rx_buf = buffer;
> +		} else {
> +			spi_xfer.tx_buf = buffer;
> +			spi_xfer.rx_buf = NULL;
>   		}
> -	}
>   
> -	spi_xfer.cs_change = 0;
> -	spi_xfer.len = len;
> +		spi_message_init(&m);
> +		spi_message_add_tail(&spi_xfer, &m);
> +		ret = spi_sync_locked(phy->spi_device, &m);
> +		if (ret < 0)
> +			goto exit;
>   
> -	if (direction) {
> -		spi_xfer.tx_buf = NULL;
> -		spi_xfer.rx_buf = buffer;
> -	} else {
> -		spi_xfer.tx_buf = buffer;
> -		spi_xfer.rx_buf = NULL;
> +		len -= transfer_len;
> +		buffer += transfer_len;
>   	}
>   
> -	spi_message_init(&m);
> -	spi_message_add_tail(&spi_xfer, &m);
> -	ret = spi_sync_locked(phy->spi_device, &m);
> -
>   exit:
>   	spi_bus_unlock(phy->spi_device->master);
>   	return ret;

Re: [PATCH 3/5] tpm_tis_spi: Check correct byte for wait state indicator

[Peter Huewe]

Am 17. Februar 2017 06:09:30 MEZ schrieb Christophe Ricard <christophe.ricard@gmail.com>:
>That's is correct, this is a mistake on my side and never saw it :-(.
>
>I guess it was possibly leading to "waste" at least 1 wait state on
>some 
>TPMs.

Unfortunately the 1 for indicating end of waitstates does only appear once so it actually rendered the driver non-functional - atleast with our tpms.


>
>Wouldn't it be better to merge that with #1 and update the comment 
>consequently?

Yes, that's what I wanted to express in the cover letter, logically it makes sense to squash #1 and #3 - but reviewing it merged with #1 is quite hard since it "obfuscates" the problem - since too much stuff moves around.
That's why I decided to split it - for easier review.

Peter


>
>
>On 16/02/2017 08:08, Peter Huewe wrote:
>> Wait states are signaled in the last byte received from the TPM in
>> response to the header, not the first byte. Check rx_buf[3] instead
>of
>> rx_buf[0].
>>
>> Cc: <stable@vger.kernel.org>
>> Fixes: 0edbfea537d1 ("tpm/tpm_tis_spi: Add support for spi phy")
>> Signed-off-by: Alexander Steffen <Alexander.Steffen@infineon.com>
>> Signed-off-by: Peter Huewe <peter.huewe@infineon.com>
>> ---
>>   drivers/char/tpm/tpm_tis_spi.c | 40
>+++++++++++++++++++++-------------------
>>   1 file changed, 21 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/char/tpm/tpm_tis_spi.c
>b/drivers/char/tpm/tpm_tis_spi.c
>> index d782b9974c14..16938e2253d2 100644
>> --- a/drivers/char/tpm/tpm_tis_spi.c
>> +++ b/drivers/char/tpm/tpm_tis_spi.c
>> @@ -60,7 +60,7 @@ static int tpm_tis_spi_transfer(struct tpm_tis_data
>*data, u32 addr, u8 len,
>>   				u8 *buffer, u8 direction)
>>   {
>>   	struct tpm_tis_spi_phy *phy = to_tpm_tis_spi_phy(data);
>> -	int ret, i;
>> +	int ret;
>>   	struct spi_message m;
>>   	struct spi_transfer spi_xfer = {
>>   		.tx_buf = phy->tx_buf,
>> @@ -85,25 +85,27 @@ static int tpm_tis_spi_transfer(struct
>tpm_tis_data *data, u32 addr, u8 len,
>>   	if (ret < 0)
>>   		goto exit;
>>   
>> -	phy->tx_buf[0] = 0;
>> -
>> -	/* According to TCG PTP specification, if there is no TPM present
>at
>> -	 * all, then the design has a weak pull-up on MISO. If a TPM is not
>> -	 * present, a pull-up on MISO means that the SB controller sees a
>1,
>> -	 * and will latch in 0xFF on the read.
>> -	 */
>> -	for (i = 0; (phy->rx_buf[0] & 0x01) == 0 && i < TPM_RETRY; i++) {
>> -		spi_xfer.len = 1;
>> -		spi_message_init(&m);
>> -		spi_message_add_tail(&spi_xfer, &m);
>> -		ret = spi_sync_locked(phy->spi_device, &m);
>> -		if (ret < 0)
>> +	if ((phy->rx_buf[3] & 0x01) == 0) {
>> +		// handle SPI wait states
>> +		int i;
>> +
>> +		phy->tx_buf[0] = 0;
>> +
>> +		for (i = 0; i < TPM_RETRY; i++) {
>> +			spi_xfer.len = 1;
>> +			spi_message_init(&m);
>> +			spi_message_add_tail(&spi_xfer, &m);
>> +			ret = spi_sync_locked(phy->spi_device, &m);
>> +			if (ret < 0)
>> +				goto exit;
>> +			if (phy->rx_buf[0] & 0x01)
>> +				break;
>> +		}
>> +
>> +		if (i == TPM_RETRY) {
>> +			ret = -ETIMEDOUT;
>>   			goto exit;
>> -	}
>> -
>> -	if (i == TPM_RETRY) {
>> -		ret = -ETIMEDOUT;
>> -		goto exit;
>> +		}
>>   	}
>>   
>>   	spi_xfer.cs_change = 0;

-- 
Sent from my mobile

Re: [PATCH 3/5] tpm_tis_spi: Check correct byte for wait state indicator

[Peter Huewe]

Am 17. Februar 2017 06:09:30 MEZ schrieb Christophe Ricard <christophe.ricard-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
>That's is correct, this is a mistake on my side and never saw it :-(.
>
>I guess it was possibly leading to "waste" at least 1 wait state on
>some 
>TPMs.

Unfortunately the 1 for indicating end of waitstates does only appear once so it actually rendered the driver non-functional - atleast with our tpms.


>
>Wouldn't it be better to merge that with #1 and update the comment 
>consequently?

Yes, that's what I wanted to express in the cover letter, logically it makes sense to squash #1 and #3 - but reviewing it merged with #1 is quite hard since it "obfuscates" the problem - since too much stuff moves around.
That's why I decided to split it - for easier review.

Peter


>
>
>On 16/02/2017 08:08, Peter Huewe wrote:
>> Wait states are signaled in the last byte received from the TPM in
>> response to the header, not the first byte. Check rx_buf[3] instead
>of
>> rx_buf[0].
>>
>> Cc: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
>> Fixes: 0edbfea537d1 ("tpm/tpm_tis_spi: Add support for spi phy")
>> Signed-off-by: Alexander Steffen <Alexander.Steffen-d0qZbvYSIPpWk0Htik3J/w@public.gmane.org>
>> Signed-off-by: Peter Huewe <peter.huewe-d0qZbvYSIPpWk0Htik3J/w@public.gmane.org>
>> ---
>>   drivers/char/tpm/tpm_tis_spi.c | 40
>+++++++++++++++++++++-------------------
>>   1 file changed, 21 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/char/tpm/tpm_tis_spi.c
>b/drivers/char/tpm/tpm_tis_spi.c
>> index d782b9974c14..16938e2253d2 100644
>> --- a/drivers/char/tpm/tpm_tis_spi.c
>> +++ b/drivers/char/tpm/tpm_tis_spi.c
>> @@ -60,7 +60,7 @@ static int tpm_tis_spi_transfer(struct tpm_tis_data
>*data, u32 addr, u8 len,
>>   				u8 *buffer, u8 direction)
>>   {
>>   	struct tpm_tis_spi_phy *phy = to_tpm_tis_spi_phy(data);
>> -	int ret, i;
>> +	int ret;
>>   	struct spi_message m;
>>   	struct spi_transfer spi_xfer = {
>>   		.tx_buf = phy->tx_buf,
>> @@ -85,25 +85,27 @@ static int tpm_tis_spi_transfer(struct
>tpm_tis_data *data, u32 addr, u8 len,
>>   	if (ret < 0)
>>   		goto exit;
>>   
>> -	phy->tx_buf[0] = 0;
>> -
>> -	/* According to TCG PTP specification, if there is no TPM present
>at
>> -	 * all, then the design has a weak pull-up on MISO. If a TPM is not
>> -	 * present, a pull-up on MISO means that the SB controller sees a
>1,
>> -	 * and will latch in 0xFF on the read.
>> -	 */
>> -	for (i = 0; (phy->rx_buf[0] & 0x01) == 0 && i < TPM_RETRY; i++) {
>> -		spi_xfer.len = 1;
>> -		spi_message_init(&m);
>> -		spi_message_add_tail(&spi_xfer, &m);
>> -		ret = spi_sync_locked(phy->spi_device, &m);
>> -		if (ret < 0)
>> +	if ((phy->rx_buf[3] & 0x01) == 0) {
>> +		// handle SPI wait states
>> +		int i;
>> +
>> +		phy->tx_buf[0] = 0;
>> +
>> +		for (i = 0; i < TPM_RETRY; i++) {
>> +			spi_xfer.len = 1;
>> +			spi_message_init(&m);
>> +			spi_message_add_tail(&spi_xfer, &m);
>> +			ret = spi_sync_locked(phy->spi_device, &m);
>> +			if (ret < 0)
>> +				goto exit;
>> +			if (phy->rx_buf[0] & 0x01)
>> +				break;
>> +		}
>> +
>> +		if (i == TPM_RETRY) {
>> +			ret = -ETIMEDOUT;
>>   			goto exit;
>> -	}
>> -
>> -	if (i == TPM_RETRY) {
>> -		ret = -ETIMEDOUT;
>> -		goto exit;
>> +		}
>>   	}
>>   
>>   	spi_xfer.cs_change = 0;

-- 
Sent from my mobile

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

Re: [PATCH 4/5] tpm_tis_spi: Remove limitation of transfers to MAX_SPI_FRAMESIZE bytes

[Peter Huewe]

Am 17. Februar 2017 06:11:53 MEZ schrieb Christophe Ricard <christophe.ricard@gmail.com>:
>I am not sure i understand here, are you considering there could be 
>burstcount > 64 with "TCG" TPM ?
>
With the current upstream version, any command larger with a response of more than 64 byte is broken, since the splitting loop went missing during the merge.
At least with our tpms that's the case.
(I think we are signalling a higher burstcount value than 64, which is allowed)

>Or is this because of TIS vs PTP differences ?
>
>To be honest, this is a little behind me now :-)
>
>
>On 16/02/2017 08:08, Peter Huewe wrote:
>> Limiting transfers to MAX_SPI_FRAMESIZE was not expected by the upper
>> layers, as tpm_tis has no such limitation. Add a loop to hide that
>> limitation.
>>
>> Cc: <stable@vger.kernel.org>
>> Fixes: 0edbfea537d1 ("tpm/tpm_tis_spi: Add support for spi phy")
>> Signed-off-by: Alexander Steffen <Alexander.Steffen@infineon.com>
>> Signed-off-by: Peter Huewe <peter.huewe@infineon.com>
>> ---
>>   drivers/char/tpm/tpm_tis_spi.c | 108
>++++++++++++++++++++++-------------------
>>   1 file changed, 57 insertions(+), 51 deletions(-)
>>
>> diff --git a/drivers/char/tpm/tpm_tis_spi.c
>b/drivers/char/tpm/tpm_tis_spi.c
>> index 16938e2253d2..b50c5b072df3 100644
>> --- a/drivers/char/tpm/tpm_tis_spi.c
>> +++ b/drivers/char/tpm/tpm_tis_spi.c
>> @@ -61,68 +61,74 @@ static int tpm_tis_spi_transfer(struct
>tpm_tis_data *data, u32 addr, u8 len,
>>   {
>>   	struct tpm_tis_spi_phy *phy = to_tpm_tis_spi_phy(data);
>>   	int ret;
>> -	struct spi_message m;
>> -	struct spi_transfer spi_xfer = {
>> -		.tx_buf = phy->tx_buf,
>> -		.rx_buf = phy->rx_buf,
>> -		.len = 4,
>> -		.cs_change = 1,
>> -	};
>> -
>> -	if (len > MAX_SPI_FRAMESIZE)
>> -		return -ENOMEM;
>>   
>> -	phy->tx_buf[0] = direction | (len - 1);
>> -	phy->tx_buf[1] = 0xd4;
>> -	phy->tx_buf[2] = addr >> 8;
>> -	phy->tx_buf[3] = addr;
>> +	spi_bus_lock(phy->spi_device->master);
>>   
>> -	spi_message_init(&m);
>> -	spi_message_add_tail(&spi_xfer, &m);
>> +	while (len) {
>> +		struct spi_message m;
>> +		struct spi_transfer spi_xfer = {
>> +			.tx_buf = phy->tx_buf,
>> +			.rx_buf = phy->rx_buf,
>> +			.len = 4,
>> +			.cs_change = 1,
>> +		};
>> +		u8 transfer_len = min_t(u16, len, MAX_SPI_FRAMESIZE);
>> +
>> +		phy->tx_buf[0] = direction | (transfer_len - 1);
>> +		phy->tx_buf[1] = 0xd4;
>> +		phy->tx_buf[2] = addr >> 8;
>> +		phy->tx_buf[3] = addr;
>> +
>> +		spi_message_init(&m);
>> +		spi_message_add_tail(&spi_xfer, &m);
>> +		ret = spi_sync_locked(phy->spi_device, &m);
>> +		if (ret < 0)
>> +			goto exit;
>>   
>> -	spi_bus_lock(phy->spi_device->master);
>> -	ret = spi_sync_locked(phy->spi_device, &m);
>> -	if (ret < 0)
>> -		goto exit;
>> -
>> -	if ((phy->rx_buf[3] & 0x01) == 0) {
>> -		// handle SPI wait states
>> -		int i;
>> -
>> -		phy->tx_buf[0] = 0;
>> -
>> -		for (i = 0; i < TPM_RETRY; i++) {
>> -			spi_xfer.len = 1;
>> -			spi_message_init(&m);
>> -			spi_message_add_tail(&spi_xfer, &m);
>> -			ret = spi_sync_locked(phy->spi_device, &m);
>> -			if (ret < 0)
>> +		if ((phy->rx_buf[3] & 0x01) == 0) {
>> +			// handle SPI wait states
>> +			int i;
>> +
>> +			phy->tx_buf[0] = 0;
>> +
>> +			for (i = 0; i < TPM_RETRY; i++) {
>> +				spi_xfer.len = 1;
>> +				spi_message_init(&m);
>> +				spi_message_add_tail(&spi_xfer, &m);
>> +				ret = spi_sync_locked(phy->spi_device, &m);
>> +				if (ret < 0)
>> +					goto exit;
>> +				if (phy->rx_buf[0] & 0x01)
>> +					break;
>> +			}
>> +
>> +			if (i == TPM_RETRY) {
>> +				ret = -ETIMEDOUT;
>>   				goto exit;
>> -			if (phy->rx_buf[0] & 0x01)
>> -				break;
>> +			}
>>   		}
>>   
>> -		if (i == TPM_RETRY) {
>> -			ret = -ETIMEDOUT;
>> -			goto exit;
>> +		spi_xfer.cs_change = 0;
>> +		spi_xfer.len = transfer_len;
>> +
>> +		if (direction) {
>> +			spi_xfer.tx_buf = NULL;
>> +			spi_xfer.rx_buf = buffer;
>> +		} else {
>> +			spi_xfer.tx_buf = buffer;
>> +			spi_xfer.rx_buf = NULL;
>>   		}
>> -	}
>>   
>> -	spi_xfer.cs_change = 0;
>> -	spi_xfer.len = len;
>> +		spi_message_init(&m);
>> +		spi_message_add_tail(&spi_xfer, &m);
>> +		ret = spi_sync_locked(phy->spi_device, &m);
>> +		if (ret < 0)
>> +			goto exit;
>>   
>> -	if (direction) {
>> -		spi_xfer.tx_buf = NULL;
>> -		spi_xfer.rx_buf = buffer;
>> -	} else {
>> -		spi_xfer.tx_buf = buffer;
>> -		spi_xfer.rx_buf = NULL;
>> +		len -= transfer_len;
>> +		buffer += transfer_len;
>>   	}
>>   
>> -	spi_message_init(&m);
>> -	spi_message_add_tail(&spi_xfer, &m);
>> -	ret = spi_sync_locked(phy->spi_device, &m);
>> -
>>   exit:
>>   	spi_bus_unlock(phy->spi_device->master);
>>   	return ret;

-- 
Sent from my mobile

Re: [PATCH 4/5] tpm_tis_spi: Remove limitation of transfers to MAX_SPI_FRAMESIZE bytes

[Peter Huewe]

Am 17. Februar 2017 06:11:53 MEZ schrieb Christophe Ricard <christophe.ricard-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
>I am not sure i understand here, are you considering there could be 
>burstcount > 64 with "TCG" TPM ?
>
With the current upstream version, any command larger with a response of more than 64 byte is broken, since the splitting loop went missing during the merge.
At least with our tpms that's the case.
(I think we are signalling a higher burstcount value than 64, which is allowed)

>Or is this because of TIS vs PTP differences ?
>
>To be honest, this is a little behind me now :-)
>
>
>On 16/02/2017 08:08, Peter Huewe wrote:
>> Limiting transfers to MAX_SPI_FRAMESIZE was not expected by the upper
>> layers, as tpm_tis has no such limitation. Add a loop to hide that
>> limitation.
>>
>> Cc: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
>> Fixes: 0edbfea537d1 ("tpm/tpm_tis_spi: Add support for spi phy")
>> Signed-off-by: Alexander Steffen <Alexander.Steffen-d0qZbvYSIPpWk0Htik3J/w@public.gmane.org>
>> Signed-off-by: Peter Huewe <peter.huewe-d0qZbvYSIPpWk0Htik3J/w@public.gmane.org>
>> ---
>>   drivers/char/tpm/tpm_tis_spi.c | 108
>++++++++++++++++++++++-------------------
>>   1 file changed, 57 insertions(+), 51 deletions(-)
>>
>> diff --git a/drivers/char/tpm/tpm_tis_spi.c
>b/drivers/char/tpm/tpm_tis_spi.c
>> index 16938e2253d2..b50c5b072df3 100644
>> --- a/drivers/char/tpm/tpm_tis_spi.c
>> +++ b/drivers/char/tpm/tpm_tis_spi.c
>> @@ -61,68 +61,74 @@ static int tpm_tis_spi_transfer(struct
>tpm_tis_data *data, u32 addr, u8 len,
>>   {
>>   	struct tpm_tis_spi_phy *phy = to_tpm_tis_spi_phy(data);
>>   	int ret;
>> -	struct spi_message m;
>> -	struct spi_transfer spi_xfer = {
>> -		.tx_buf = phy->tx_buf,
>> -		.rx_buf = phy->rx_buf,
>> -		.len = 4,
>> -		.cs_change = 1,
>> -	};
>> -
>> -	if (len > MAX_SPI_FRAMESIZE)
>> -		return -ENOMEM;
>>   
>> -	phy->tx_buf[0] = direction | (len - 1);
>> -	phy->tx_buf[1] = 0xd4;
>> -	phy->tx_buf[2] = addr >> 8;
>> -	phy->tx_buf[3] = addr;
>> +	spi_bus_lock(phy->spi_device->master);
>>   
>> -	spi_message_init(&m);
>> -	spi_message_add_tail(&spi_xfer, &m);
>> +	while (len) {
>> +		struct spi_message m;
>> +		struct spi_transfer spi_xfer = {
>> +			.tx_buf = phy->tx_buf,
>> +			.rx_buf = phy->rx_buf,
>> +			.len = 4,
>> +			.cs_change = 1,
>> +		};
>> +		u8 transfer_len = min_t(u16, len, MAX_SPI_FRAMESIZE);
>> +
>> +		phy->tx_buf[0] = direction | (transfer_len - 1);
>> +		phy->tx_buf[1] = 0xd4;
>> +		phy->tx_buf[2] = addr >> 8;
>> +		phy->tx_buf[3] = addr;
>> +
>> +		spi_message_init(&m);
>> +		spi_message_add_tail(&spi_xfer, &m);
>> +		ret = spi_sync_locked(phy->spi_device, &m);
>> +		if (ret < 0)
>> +			goto exit;
>>   
>> -	spi_bus_lock(phy->spi_device->master);
>> -	ret = spi_sync_locked(phy->spi_device, &m);
>> -	if (ret < 0)
>> -		goto exit;
>> -
>> -	if ((phy->rx_buf[3] & 0x01) == 0) {
>> -		// handle SPI wait states
>> -		int i;
>> -
>> -		phy->tx_buf[0] = 0;
>> -
>> -		for (i = 0; i < TPM_RETRY; i++) {
>> -			spi_xfer.len = 1;
>> -			spi_message_init(&m);
>> -			spi_message_add_tail(&spi_xfer, &m);
>> -			ret = spi_sync_locked(phy->spi_device, &m);
>> -			if (ret < 0)
>> +		if ((phy->rx_buf[3] & 0x01) == 0) {
>> +			// handle SPI wait states
>> +			int i;
>> +
>> +			phy->tx_buf[0] = 0;
>> +
>> +			for (i = 0; i < TPM_RETRY; i++) {
>> +				spi_xfer.len = 1;
>> +				spi_message_init(&m);
>> +				spi_message_add_tail(&spi_xfer, &m);
>> +				ret = spi_sync_locked(phy->spi_device, &m);
>> +				if (ret < 0)
>> +					goto exit;
>> +				if (phy->rx_buf[0] & 0x01)
>> +					break;
>> +			}
>> +
>> +			if (i == TPM_RETRY) {
>> +				ret = -ETIMEDOUT;
>>   				goto exit;
>> -			if (phy->rx_buf[0] & 0x01)
>> -				break;
>> +			}
>>   		}
>>   
>> -		if (i == TPM_RETRY) {
>> -			ret = -ETIMEDOUT;
>> -			goto exit;
>> +		spi_xfer.cs_change = 0;
>> +		spi_xfer.len = transfer_len;
>> +
>> +		if (direction) {
>> +			spi_xfer.tx_buf = NULL;
>> +			spi_xfer.rx_buf = buffer;
>> +		} else {
>> +			spi_xfer.tx_buf = buffer;
>> +			spi_xfer.rx_buf = NULL;
>>   		}
>> -	}
>>   
>> -	spi_xfer.cs_change = 0;
>> -	spi_xfer.len = len;
>> +		spi_message_init(&m);
>> +		spi_message_add_tail(&spi_xfer, &m);
>> +		ret = spi_sync_locked(phy->spi_device, &m);
>> +		if (ret < 0)
>> +			goto exit;
>>   
>> -	if (direction) {
>> -		spi_xfer.tx_buf = NULL;
>> -		spi_xfer.rx_buf = buffer;
>> -	} else {
>> -		spi_xfer.tx_buf = buffer;
>> -		spi_xfer.rx_buf = NULL;
>> +		len -= transfer_len;
>> +		buffer += transfer_len;
>>   	}
>>   
>> -	spi_message_init(&m);
>> -	spi_message_add_tail(&spi_xfer, &m);
>> -	ret = spi_sync_locked(phy->spi_device, &m);
>> -
>>   exit:
>>   	spi_bus_unlock(phy->spi_device->master);
>>   	return ret;

-- 
Sent from my mobile

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

Re: [PATCH 0/5] Fix whole native SPI TPM driver

[Jarkko Sakkinen]

On Thu, Feb 16, 2017 at 04:08:21PM +0000, Peter Huewe wrote:
> During our testing it showed that unfortunately the whole native spi tpm driver
> was more or less non-functional since it was merged, e.g. the wrong byte for
> waitstate handling was used and transfers larger than 64 bytes did not work at all.
> 
> This was probably caused by the merging of the different approaches back then,
> as the initial RFC patch did not have these problems, and also my sudden lack
> of time/commitment back then.
> I'm sorry that the final driver code went untested for that long time.
> 
> This patch set fixes these issues one by one.
> In order to avoid duplication the read/write function was consolidated to one
> transfer function, so we do not have to apply the same fix at two locations.
> Maybe consider squashing it - we splitted it for easier review.
>  
> Affected Kernels: 4.8, 4.9, 4.10
> Patchset was tested on Raspberry Pi2 with SLB9670 (TPM1.2 and TPM2.0)
> 
> Peter Huewe (5):
>   tpm_tis_spi: Use single function to transfer data
>   tpm_tis_spi: Abort transfer when too many wait states are signaled
>   tpm_tis_spi: Check correct byte for wait state indicator
>   tpm_tis_spi: Remove limitation of transfers to MAX_SPI_FRAMESIZE bytes
>   tpm_tis_spi: Add small delay after last transfer
> 
>  drivers/char/tpm/tpm_tis_spi.c | 163 +++++++++++++++++------------------------
>  1 file changed, 69 insertions(+), 94 deletions(-)
> 
> -- 
> 2.7.4
> 

I also found type errors a whilea ago:

https://patchwork.kernel.org/patch/9547993/

Do this take care of these issues? 

/JArkko

Re: [PATCH 0/5] Fix whole native SPI TPM driver

[Jarkko Sakkinen]

On Thu, Feb 16, 2017 at 04:08:21PM +0000, Peter Huewe wrote:
> During our testing it showed that unfortunately the whole native spi tpm driver
> was more or less non-functional since it was merged, e.g. the wrong byte for
> waitstate handling was used and transfers larger than 64 bytes did not work at all.
> 
> This was probably caused by the merging of the different approaches back then,
> as the initial RFC patch did not have these problems, and also my sudden lack
> of time/commitment back then.
> I'm sorry that the final driver code went untested for that long time.
> 
> This patch set fixes these issues one by one.
> In order to avoid duplication the read/write function was consolidated to one
> transfer function, so we do not have to apply the same fix at two locations.
> Maybe consider squashing it - we splitted it for easier review.
>  
> Affected Kernels: 4.8, 4.9, 4.10
> Patchset was tested on Raspberry Pi2 with SLB9670 (TPM1.2 and TPM2.0)
> 
> Peter Huewe (5):
>   tpm_tis_spi: Use single function to transfer data
>   tpm_tis_spi: Abort transfer when too many wait states are signaled
>   tpm_tis_spi: Check correct byte for wait state indicator
>   tpm_tis_spi: Remove limitation of transfers to MAX_SPI_FRAMESIZE bytes
>   tpm_tis_spi: Add small delay after last transfer
> 
>  drivers/char/tpm/tpm_tis_spi.c | 163 +++++++++++++++++------------------------
>  1 file changed, 69 insertions(+), 94 deletions(-)
> 
> -- 
> 2.7.4
> 

I also found type errors a whilea ago:

https://patchwork.kernel.org/patch/9547993/

Do this take care of these issues? 

/JArkko

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

Re: [PATCH 0/5] Fix whole native SPI TPM driver

[Peter Huewe]

Am 22. Februar 2017 22:19:24 MEZ schrieb Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>:
>On Thu, Feb 16, 2017 at 04:08:21PM +0000, Peter Huewe wrote:
>> During our testing it showed that unfortunately the whole native spi
>tpm driver
>> was more or less non-functional since it was merged, e.g. the wrong
>byte for
>> waitstate handling was used and transfers larger than 64 bytes did
>not work at all.
>> 
>> This was probably caused by the merging of the different approaches
>back then,
>> as the initial RFC patch did not have these problems, and also my
>sudden lack
>> of time/commitment back then.
>> I'm sorry that the final driver code went untested for that long
>time.
>> 
>> This patch set fixes these issues one by one.
>> In order to avoid duplication the read/write function was
>consolidated to one
>> transfer function, so we do not have to apply the same fix at two
>locations.
>> Maybe consider squashing it - we splitted it for easier review.
>>  
>> Affected Kernels: 4.8, 4.9, 4.10
>> Patchset was tested on Raspberry Pi2 with SLB9670 (TPM1.2 and TPM2.0)
>> 
>> Peter Huewe (5):
>>   tpm_tis_spi: Use single function to transfer data
>>   tpm_tis_spi: Abort transfer when too many wait states are signaled
>>   tpm_tis_spi: Check correct byte for wait state indicator
>>   tpm_tis_spi: Remove limitation of transfers to MAX_SPI_FRAMESIZE
>bytes
>>   tpm_tis_spi: Add small delay after last transfer
>> 
>>  drivers/char/tpm/tpm_tis_spi.c | 163
>+++++++++++++++++------------------------
>>  1 file changed, 69 insertions(+), 94 deletions(-)
>> 
>> -- 
>> 2.7.4
>> 
>
>I also found type errors a whilea ago:
>
>https://patchwork.kernel.org/patch/9547993/
>
>Do this take care of these issues? 
Not that I'm aware of.
We did not test it on any big endian system yet so the problem never became visible.

In any case it would be really nice to see this patchset to be applied so people have a working,but maybe not perfect, spi tpm on the common little endian platforms.
When do you think you can merge and pull-request them?

Peter
>
>/JArkko

-- 
Sent from my mobile

Re: [PATCH 0/5] Fix whole native SPI TPM driver

[Peter Huewe]

Am 22. Februar 2017 22:19:24 MEZ schrieb Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>:
>On Thu, Feb 16, 2017 at 04:08:21PM +0000, Peter Huewe wrote:
>> During our testing it showed that unfortunately the whole native spi
>tpm driver
>> was more or less non-functional since it was merged, e.g. the wrong
>byte for
>> waitstate handling was used and transfers larger than 64 bytes did
>not work at all.
>> 
>> This was probably caused by the merging of the different approaches
>back then,
>> as the initial RFC patch did not have these problems, and also my
>sudden lack
>> of time/commitment back then.
>> I'm sorry that the final driver code went untested for that long
>time.
>> 
>> This patch set fixes these issues one by one.
>> In order to avoid duplication the read/write function was
>consolidated to one
>> transfer function, so we do not have to apply the same fix at two
>locations.
>> Maybe consider squashing it - we splitted it for easier review.
>>  
>> Affected Kernels: 4.8, 4.9, 4.10
>> Patchset was tested on Raspberry Pi2 with SLB9670 (TPM1.2 and TPM2.0)
>> 
>> Peter Huewe (5):
>>   tpm_tis_spi: Use single function to transfer data
>>   tpm_tis_spi: Abort transfer when too many wait states are signaled
>>   tpm_tis_spi: Check correct byte for wait state indicator
>>   tpm_tis_spi: Remove limitation of transfers to MAX_SPI_FRAMESIZE
>bytes
>>   tpm_tis_spi: Add small delay after last transfer
>> 
>>  drivers/char/tpm/tpm_tis_spi.c | 163
>+++++++++++++++++------------------------
>>  1 file changed, 69 insertions(+), 94 deletions(-)
>> 
>> -- 
>> 2.7.4
>> 
>
>I also found type errors a whilea ago:
>
>https://patchwork.kernel.org/patch/9547993/
>
>Do this take care of these issues? 
Not that I'm aware of.
We did not test it on any big endian system yet so the problem never became visible.

In any case it would be really nice to see this patchset to be applied so people have a working,but maybe not perfect, spi tpm on the common little endian platforms.
When do you think you can merge and pull-request them?

Peter
>
>/JArkko

-- 
Sent from my mobile

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

Re: [PATCH 4/5] tpm_tis_spi: Remove limitation of transfers to MAX_SPI_FRAMESIZE bytes

[Jarkko Sakkinen]

On Thu, Feb 16, 2017 at 04:08:25PM +0000, Peter Huewe wrote:
> Limiting transfers to MAX_SPI_FRAMESIZE was not expected by the upper
> layers, as tpm_tis has no such limitation. Add a loop to hide that
> limitation.
> 
> Cc: <stable@vger.kernel.org>
> Fixes: 0edbfea537d1 ("tpm/tpm_tis_spi: Add support for spi phy")
> Signed-off-by: Alexander Steffen <Alexander.Steffen@infineon.com>
> Signed-off-by: Peter Huewe <peter.huewe@infineon.com>
> ---
>  drivers/char/tpm/tpm_tis_spi.c | 108 ++++++++++++++++++++++-------------------
>  1 file changed, 57 insertions(+), 51 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_tis_spi.c b/drivers/char/tpm/tpm_tis_spi.c
> index 16938e2253d2..b50c5b072df3 100644
> --- a/drivers/char/tpm/tpm_tis_spi.c
> +++ b/drivers/char/tpm/tpm_tis_spi.c
> @@ -61,68 +61,74 @@ static int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u8 len,
>  {
>  	struct tpm_tis_spi_phy *phy = to_tpm_tis_spi_phy(data);
>  	int ret;
> -	struct spi_message m;
> -	struct spi_transfer spi_xfer = {
> -		.tx_buf = phy->tx_buf,
> -		.rx_buf = phy->rx_buf,
> -		.len = 4,
> -		.cs_change = 1,
> -	};
> -
> -	if (len > MAX_SPI_FRAMESIZE)
> -		return -ENOMEM;
>  
> -	phy->tx_buf[0] = direction | (len - 1);
> -	phy->tx_buf[1] = 0xd4;
> -	phy->tx_buf[2] = addr >> 8;
> -	phy->tx_buf[3] = addr;
> +	spi_bus_lock(phy->spi_device->master);
>  
> -	spi_message_init(&m);
> -	spi_message_add_tail(&spi_xfer, &m);
> +	while (len) {
> +		struct spi_message m;
> +		struct spi_transfer spi_xfer = {
> +			.tx_buf = phy->tx_buf,
> +			.rx_buf = phy->rx_buf,
> +			.len = 4,
> +			.cs_change = 1,
> +		};

I like the habbit of keeping all the declarations in the beginning of
the function as we mostly have in the subsystem. It gives a fast view
what the function eats and how much stack it uses.

If you really think that declaring spi_xfer here is a good idea please
create a separate "helper" function for it.

/Jarkko

Re: [PATCH 4/5] tpm_tis_spi: Remove limitation of transfers to MAX_SPI_FRAMESIZE bytes

[Jarkko Sakkinen]

On Thu, Feb 16, 2017 at 04:08:25PM +0000, Peter Huewe wrote:
> Limiting transfers to MAX_SPI_FRAMESIZE was not expected by the upper
> layers, as tpm_tis has no such limitation. Add a loop to hide that
> limitation.
> 
> Cc: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
> Fixes: 0edbfea537d1 ("tpm/tpm_tis_spi: Add support for spi phy")
> Signed-off-by: Alexander Steffen <Alexander.Steffen-d0qZbvYSIPpWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Peter Huewe <peter.huewe-d0qZbvYSIPpWk0Htik3J/w@public.gmane.org>
> ---
>  drivers/char/tpm/tpm_tis_spi.c | 108 ++++++++++++++++++++++-------------------
>  1 file changed, 57 insertions(+), 51 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_tis_spi.c b/drivers/char/tpm/tpm_tis_spi.c
> index 16938e2253d2..b50c5b072df3 100644
> --- a/drivers/char/tpm/tpm_tis_spi.c
> +++ b/drivers/char/tpm/tpm_tis_spi.c
> @@ -61,68 +61,74 @@ static int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u8 len,
>  {
>  	struct tpm_tis_spi_phy *phy = to_tpm_tis_spi_phy(data);
>  	int ret;
> -	struct spi_message m;
> -	struct spi_transfer spi_xfer = {
> -		.tx_buf = phy->tx_buf,
> -		.rx_buf = phy->rx_buf,
> -		.len = 4,
> -		.cs_change = 1,
> -	};
> -
> -	if (len > MAX_SPI_FRAMESIZE)
> -		return -ENOMEM;
>  
> -	phy->tx_buf[0] = direction | (len - 1);
> -	phy->tx_buf[1] = 0xd4;
> -	phy->tx_buf[2] = addr >> 8;
> -	phy->tx_buf[3] = addr;
> +	spi_bus_lock(phy->spi_device->master);
>  
> -	spi_message_init(&m);
> -	spi_message_add_tail(&spi_xfer, &m);
> +	while (len) {
> +		struct spi_message m;
> +		struct spi_transfer spi_xfer = {
> +			.tx_buf = phy->tx_buf,
> +			.rx_buf = phy->rx_buf,
> +			.len = 4,
> +			.cs_change = 1,
> +		};

I like the habbit of keeping all the declarations in the beginning of
the function as we mostly have in the subsystem. It gives a fast view
what the function eats and how much stack it uses.

If you really think that declaring spi_xfer here is a good idea please
create a separate "helper" function for it.

/Jarkko

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

Re: [PATCH 1/5] tpm_tis_spi: Use single function to transfer data

[Jarkko Sakkinen]

On Thu, Feb 16, 2017 at 04:08:22PM +0000, Peter Huewe wrote:
> The algorithm for sending data to the TPM is mostly identical to the
> algorithm for receiving data from the TPM, so a single function is
> sufficient to handle both cases.
> 
> This is a prequisite for all the other fixes, so we don't have to fix
> everything twice (send/receive)
> 
> Cc: <stable@vger.kernel.org>
> [prerequisite for other fixes in this series]
> Fixes: 0edbfea537d1 ("tpm/tpm_tis_spi: Add support for spi phy")
> Signed-off-by: Alexander Steffen <Alexander.Steffen@infineon.com>
> Signed-off-by: Peter Huewe <peter.huewe@infineon.com>

Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

/Jarkko

> ---
>  drivers/char/tpm/tpm_tis_spi.c | 87 ++++++++++++------------------------------
>  1 file changed, 24 insertions(+), 63 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_tis_spi.c b/drivers/char/tpm/tpm_tis_spi.c
> index 5292e5768a7e..6e1a3c43f621 100644
> --- a/drivers/char/tpm/tpm_tis_spi.c
> +++ b/drivers/char/tpm/tpm_tis_spi.c
> @@ -47,8 +47,8 @@ struct tpm_tis_spi_phy {
>  	struct tpm_tis_data priv;
>  	struct spi_device *spi_device;
>  
> -	u8 tx_buf[MAX_SPI_FRAMESIZE + 4];
> -	u8 rx_buf[MAX_SPI_FRAMESIZE + 4];
> +	u8 tx_buf[4];
> +	u8 rx_buf[4];
>  };
>  
>  static inline struct tpm_tis_spi_phy *to_tpm_tis_spi_phy(struct tpm_tis_data *data)
> @@ -56,8 +56,8 @@ static inline struct tpm_tis_spi_phy *to_tpm_tis_spi_phy(struct tpm_tis_data *da
>  	return container_of(data, struct tpm_tis_spi_phy, priv);
>  }
>  
> -static int tpm_tis_spi_read_bytes(struct tpm_tis_data *data, u32 addr,
> -				  u16 len, u8 *result)
> +static int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u8 len,
> +				u8 *buffer, u8 direction)
>  {
>  	struct tpm_tis_spi_phy *phy = to_tpm_tis_spi_phy(data);
>  	int ret, i;
> @@ -66,17 +66,17 @@ static int tpm_tis_spi_read_bytes(struct tpm_tis_data *data, u32 addr,
>  		.tx_buf = phy->tx_buf,
>  		.rx_buf = phy->rx_buf,
>  		.len = 4,
> +		.cs_change = 1,
>  	};
>  
>  	if (len > MAX_SPI_FRAMESIZE)
>  		return -ENOMEM;
>  
> -	phy->tx_buf[0] = 0x80 | (len - 1);
> +	phy->tx_buf[0] = direction | (len - 1);
>  	phy->tx_buf[1] = 0xd4;
> -	phy->tx_buf[2] = (addr >> 8)  & 0xFF;
> -	phy->tx_buf[3] = addr	      & 0xFF;
> +	phy->tx_buf[2] = addr >> 8;
> +	phy->tx_buf[3] = addr;
>  
> -	spi_xfer.cs_change = 1;
>  	spi_message_init(&m);
>  	spi_message_add_tail(&spi_xfer, &m);
>  
> @@ -85,7 +85,7 @@ static int tpm_tis_spi_read_bytes(struct tpm_tis_data *data, u32 addr,
>  	if (ret < 0)
>  		goto exit;
>  
> -	memset(phy->tx_buf, 0, len);
> +	phy->tx_buf[0] = 0;
>  
>  	/* According to TCG PTP specification, if there is no TPM present at
>  	 * all, then the design has a weak pull-up on MISO. If a TPM is not
> @@ -103,7 +103,14 @@ static int tpm_tis_spi_read_bytes(struct tpm_tis_data *data, u32 addr,
>  
>  	spi_xfer.cs_change = 0;
>  	spi_xfer.len = len;
> -	spi_xfer.rx_buf = result;
> +
> +	if (direction) {
> +		spi_xfer.tx_buf = NULL;
> +		spi_xfer.rx_buf = buffer;
> +	} else {
> +		spi_xfer.tx_buf = buffer;
> +		spi_xfer.rx_buf = NULL;
> +	}
>  
>  	spi_message_init(&m);
>  	spi_message_add_tail(&spi_xfer, &m);
> @@ -114,62 +121,16 @@ static int tpm_tis_spi_read_bytes(struct tpm_tis_data *data, u32 addr,
>  	return ret;
>  }
>  
> +static int tpm_tis_spi_read_bytes(struct tpm_tis_data *data, u32 addr,
> +				  u16 len, u8 *result)
> +{
> +	return tpm_tis_spi_transfer(data, addr, len, result, 0x80);
> +}
> +
>  static int tpm_tis_spi_write_bytes(struct tpm_tis_data *data, u32 addr,
>  				   u16 len, u8 *value)
>  {
> -	struct tpm_tis_spi_phy *phy = to_tpm_tis_spi_phy(data);
> -	int ret, i;
> -	struct spi_message m;
> -	struct spi_transfer spi_xfer = {
> -		.tx_buf = phy->tx_buf,
> -		.rx_buf = phy->rx_buf,
> -		.len = 4,
> -	};
> -
> -	if (len > MAX_SPI_FRAMESIZE)
> -		return -ENOMEM;
> -
> -	phy->tx_buf[0] = len - 1;
> -	phy->tx_buf[1] = 0xd4;
> -	phy->tx_buf[2] = (addr >> 8)  & 0xFF;
> -	phy->tx_buf[3] = addr         & 0xFF;
> -
> -	spi_xfer.cs_change = 1;
> -	spi_message_init(&m);
> -	spi_message_add_tail(&spi_xfer, &m);
> -
> -	spi_bus_lock(phy->spi_device->master);
> -	ret = spi_sync_locked(phy->spi_device, &m);
> -	if (ret < 0)
> -		goto exit;
> -
> -	memset(phy->tx_buf, 0, len);
> -
> -	/* According to TCG PTP specification, if there is no TPM present at
> -	 * all, then the design has a weak pull-up on MISO. If a TPM is not
> -	 * present, a pull-up on MISO means that the SB controller sees a 1,
> -	 * and will latch in 0xFF on the read.
> -	 */
> -	for (i = 0; (phy->rx_buf[0] & 0x01) == 0 && i < TPM_RETRY; i++) {
> -		spi_xfer.len = 1;
> -		spi_message_init(&m);
> -		spi_message_add_tail(&spi_xfer, &m);
> -		ret = spi_sync_locked(phy->spi_device, &m);
> -		if (ret < 0)
> -			goto exit;
> -	}
> -
> -	spi_xfer.len = len;
> -	spi_xfer.tx_buf = value;
> -	spi_xfer.cs_change = 0;
> -	spi_xfer.tx_buf = value;
> -	spi_message_init(&m);
> -	spi_message_add_tail(&spi_xfer, &m);
> -	ret = spi_sync_locked(phy->spi_device, &m);
> -
> -exit:
> -	spi_bus_unlock(phy->spi_device->master);
> -	return ret;
> +	return tpm_tis_spi_transfer(data, addr, len, value, 0);
>  }
>  
>  static int tpm_tis_spi_read16(struct tpm_tis_data *data, u32 addr, u16 *result)
> -- 
> 2.7.4
> 

Re: [PATCH 2/5] tpm_tis_spi: Abort transfer when too many wait states are signaled

[Jarkko Sakkinen]

On Thu, Feb 16, 2017 at 04:08:23PM +0000, Peter Huewe wrote:
> Abort the transfer with ETIMEDOUT when the TPM signals more than
> TPM_RETRY wait states. Continuing with the transfer in this state
> will only lead to arbitrary failures in other parts of the code.
> 
> Cc: <stable@vger.kernel.org>
> Fixes: 0edbfea537d1 ("tpm/tpm_tis_spi: Add support for spi phy")
> Signed-off-by: Alexander Steffen <Alexander.Steffen@infineon.com>
> Signed-off-by: Peter Huewe <peter.huewe@infineon.com>

Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

/Jarkko

> ---
>  drivers/char/tpm/tpm_tis_spi.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/char/tpm/tpm_tis_spi.c b/drivers/char/tpm/tpm_tis_spi.c
> index 6e1a3c43f621..d782b9974c14 100644
> --- a/drivers/char/tpm/tpm_tis_spi.c
> +++ b/drivers/char/tpm/tpm_tis_spi.c
> @@ -101,6 +101,11 @@ static int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u8 len,
>  			goto exit;
>  	}
>  
> +	if (i == TPM_RETRY) {
> +		ret = -ETIMEDOUT;
> +		goto exit;
> +	}
> +
>  	spi_xfer.cs_change = 0;
>  	spi_xfer.len = len;
>  
> -- 
> 2.7.4
> 

Re: [PATCH 2/5] tpm_tis_spi: Abort transfer when too many wait states are signaled

[Jarkko Sakkinen]

On Thu, Feb 16, 2017 at 04:08:23PM +0000, Peter Huewe wrote:
> Abort the transfer with ETIMEDOUT when the TPM signals more than
> TPM_RETRY wait states. Continuing with the transfer in this state
> will only lead to arbitrary failures in other parts of the code.
> 
> Cc: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
> Fixes: 0edbfea537d1 ("tpm/tpm_tis_spi: Add support for spi phy")
> Signed-off-by: Alexander Steffen <Alexander.Steffen-d0qZbvYSIPpWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Peter Huewe <peter.huewe-d0qZbvYSIPpWk0Htik3J/w@public.gmane.org>

Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>

/Jarkko

> ---
>  drivers/char/tpm/tpm_tis_spi.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/char/tpm/tpm_tis_spi.c b/drivers/char/tpm/tpm_tis_spi.c
> index 6e1a3c43f621..d782b9974c14 100644
> --- a/drivers/char/tpm/tpm_tis_spi.c
> +++ b/drivers/char/tpm/tpm_tis_spi.c
> @@ -101,6 +101,11 @@ static int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u8 len,
>  			goto exit;
>  	}
>  
> +	if (i == TPM_RETRY) {
> +		ret = -ETIMEDOUT;
> +		goto exit;
> +	}
> +
>  	spi_xfer.cs_change = 0;
>  	spi_xfer.len = len;
>  
> -- 
> 2.7.4
> 

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

Re: [PATCH 3/5] tpm_tis_spi: Check correct byte for wait state indicator

[Jarkko Sakkinen]

On Thu, Feb 16, 2017 at 04:08:24PM +0000, Peter Huewe wrote:
> Wait states are signaled in the last byte received from the TPM in
> response to the header, not the first byte. Check rx_buf[3] instead of
> rx_buf[0].
> 
> Cc: <stable@vger.kernel.org>
> Fixes: 0edbfea537d1 ("tpm/tpm_tis_spi: Add support for spi phy")
> Signed-off-by: Alexander Steffen <Alexander.Steffen@infineon.com>
> Signed-off-by: Peter Huewe <peter.huewe@infineon.com>

Reviewed-by: Jarkko Sakkien <jarkko.sakkinen@linux.intel.com>

/Jarkko

> ---
>  drivers/char/tpm/tpm_tis_spi.c | 40 +++++++++++++++++++++-------------------
>  1 file changed, 21 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_tis_spi.c b/drivers/char/tpm/tpm_tis_spi.c
> index d782b9974c14..16938e2253d2 100644
> --- a/drivers/char/tpm/tpm_tis_spi.c
> +++ b/drivers/char/tpm/tpm_tis_spi.c
> @@ -60,7 +60,7 @@ static int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u8 len,
>  				u8 *buffer, u8 direction)
>  {
>  	struct tpm_tis_spi_phy *phy = to_tpm_tis_spi_phy(data);
> -	int ret, i;
> +	int ret;
>  	struct spi_message m;
>  	struct spi_transfer spi_xfer = {
>  		.tx_buf = phy->tx_buf,
> @@ -85,25 +85,27 @@ static int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u8 len,
>  	if (ret < 0)
>  		goto exit;
>  
> -	phy->tx_buf[0] = 0;
> -
> -	/* According to TCG PTP specification, if there is no TPM present at
> -	 * all, then the design has a weak pull-up on MISO. If a TPM is not
> -	 * present, a pull-up on MISO means that the SB controller sees a 1,
> -	 * and will latch in 0xFF on the read.
> -	 */
> -	for (i = 0; (phy->rx_buf[0] & 0x01) == 0 && i < TPM_RETRY; i++) {
> -		spi_xfer.len = 1;
> -		spi_message_init(&m);
> -		spi_message_add_tail(&spi_xfer, &m);
> -		ret = spi_sync_locked(phy->spi_device, &m);
> -		if (ret < 0)
> +	if ((phy->rx_buf[3] & 0x01) == 0) {
> +		// handle SPI wait states
> +		int i;
> +
> +		phy->tx_buf[0] = 0;
> +
> +		for (i = 0; i < TPM_RETRY; i++) {
> +			spi_xfer.len = 1;
> +			spi_message_init(&m);
> +			spi_message_add_tail(&spi_xfer, &m);
> +			ret = spi_sync_locked(phy->spi_device, &m);
> +			if (ret < 0)
> +				goto exit;
> +			if (phy->rx_buf[0] & 0x01)
> +				break;
> +		}
> +
> +		if (i == TPM_RETRY) {
> +			ret = -ETIMEDOUT;
>  			goto exit;
> -	}
> -
> -	if (i == TPM_RETRY) {
> -		ret = -ETIMEDOUT;
> -		goto exit;
> +		}
>  	}
>  
>  	spi_xfer.cs_change = 0;
> -- 
> 2.7.4
> 

Re: [PATCH 3/5] tpm_tis_spi: Check correct byte for wait state indicator

[Jarkko Sakkinen]

On Thu, Feb 16, 2017 at 04:08:24PM +0000, Peter Huewe wrote:
> Wait states are signaled in the last byte received from the TPM in
> response to the header, not the first byte. Check rx_buf[3] instead of
> rx_buf[0].
> 
> Cc: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
> Fixes: 0edbfea537d1 ("tpm/tpm_tis_spi: Add support for spi phy")
> Signed-off-by: Alexander Steffen <Alexander.Steffen-d0qZbvYSIPpWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Peter Huewe <peter.huewe-d0qZbvYSIPpWk0Htik3J/w@public.gmane.org>

Reviewed-by: Jarkko Sakkien <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>

/Jarkko

> ---
>  drivers/char/tpm/tpm_tis_spi.c | 40 +++++++++++++++++++++-------------------
>  1 file changed, 21 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_tis_spi.c b/drivers/char/tpm/tpm_tis_spi.c
> index d782b9974c14..16938e2253d2 100644
> --- a/drivers/char/tpm/tpm_tis_spi.c
> +++ b/drivers/char/tpm/tpm_tis_spi.c
> @@ -60,7 +60,7 @@ static int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u8 len,
>  				u8 *buffer, u8 direction)
>  {
>  	struct tpm_tis_spi_phy *phy = to_tpm_tis_spi_phy(data);
> -	int ret, i;
> +	int ret;
>  	struct spi_message m;
>  	struct spi_transfer spi_xfer = {
>  		.tx_buf = phy->tx_buf,
> @@ -85,25 +85,27 @@ static int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u8 len,
>  	if (ret < 0)
>  		goto exit;
>  
> -	phy->tx_buf[0] = 0;
> -
> -	/* According to TCG PTP specification, if there is no TPM present at
> -	 * all, then the design has a weak pull-up on MISO. If a TPM is not
> -	 * present, a pull-up on MISO means that the SB controller sees a 1,
> -	 * and will latch in 0xFF on the read.
> -	 */
> -	for (i = 0; (phy->rx_buf[0] & 0x01) == 0 && i < TPM_RETRY; i++) {
> -		spi_xfer.len = 1;
> -		spi_message_init(&m);
> -		spi_message_add_tail(&spi_xfer, &m);
> -		ret = spi_sync_locked(phy->spi_device, &m);
> -		if (ret < 0)
> +	if ((phy->rx_buf[3] & 0x01) == 0) {
> +		// handle SPI wait states
> +		int i;
> +
> +		phy->tx_buf[0] = 0;
> +
> +		for (i = 0; i < TPM_RETRY; i++) {
> +			spi_xfer.len = 1;
> +			spi_message_init(&m);
> +			spi_message_add_tail(&spi_xfer, &m);
> +			ret = spi_sync_locked(phy->spi_device, &m);
> +			if (ret < 0)
> +				goto exit;
> +			if (phy->rx_buf[0] & 0x01)
> +				break;
> +		}
> +
> +		if (i == TPM_RETRY) {
> +			ret = -ETIMEDOUT;
>  			goto exit;
> -	}
> -
> -	if (i == TPM_RETRY) {
> -		ret = -ETIMEDOUT;
> -		goto exit;
> +		}
>  	}
>  
>  	spi_xfer.cs_change = 0;
> -- 
> 2.7.4
> 

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

Re: [PATCH 4/5] tpm_tis_spi: Remove limitation of transfers to MAX_SPI_FRAMESIZE bytes

[Jarkko Sakkinen]

On Thu, Feb 16, 2017 at 04:08:25PM +0000, Peter Huewe wrote:
> Limiting transfers to MAX_SPI_FRAMESIZE was not expected by the upper
> layers, as tpm_tis has no such limitation. Add a loop to hide that
> limitation.
> 
> Cc: <stable@vger.kernel.org>
> Fixes: 0edbfea537d1 ("tpm/tpm_tis_spi: Add support for spi phy")
> Signed-off-by: Alexander Steffen <Alexander.Steffen@infineon.com>
> Signed-off-by: Peter Huewe <peter.huewe@infineon.com>

This commit also had kbuild warning.

PS. When you send the next patch set version, please add this to
every commit:

  Tested-by: Benoit Houyere <benoit.houyere@st.com>

/Jarkko

> ---
>  drivers/char/tpm/tpm_tis_spi.c | 108 ++++++++++++++++++++++-------------------
>  1 file changed, 57 insertions(+), 51 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_tis_spi.c b/drivers/char/tpm/tpm_tis_spi.c
> index 16938e2253d2..b50c5b072df3 100644
> --- a/drivers/char/tpm/tpm_tis_spi.c
> +++ b/drivers/char/tpm/tpm_tis_spi.c
> @@ -61,68 +61,74 @@ static int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u8 len,
>  {
>  	struct tpm_tis_spi_phy *phy = to_tpm_tis_spi_phy(data);
>  	int ret;
> -	struct spi_message m;
> -	struct spi_transfer spi_xfer = {
> -		.tx_buf = phy->tx_buf,
> -		.rx_buf = phy->rx_buf,
> -		.len = 4,
> -		.cs_change = 1,
> -	};
> -
> -	if (len > MAX_SPI_FRAMESIZE)
> -		return -ENOMEM;
>  
> -	phy->tx_buf[0] = direction | (len - 1);
> -	phy->tx_buf[1] = 0xd4;
> -	phy->tx_buf[2] = addr >> 8;
> -	phy->tx_buf[3] = addr;
> +	spi_bus_lock(phy->spi_device->master);
>  
> -	spi_message_init(&m);
> -	spi_message_add_tail(&spi_xfer, &m);
> +	while (len) {
> +		struct spi_message m;
> +		struct spi_transfer spi_xfer = {
> +			.tx_buf = phy->tx_buf,
> +			.rx_buf = phy->rx_buf,
> +			.len = 4,
> +			.cs_change = 1,
> +		};
> +		u8 transfer_len = min_t(u16, len, MAX_SPI_FRAMESIZE);
> +
> +		phy->tx_buf[0] = direction | (transfer_len - 1);
> +		phy->tx_buf[1] = 0xd4;
> +		phy->tx_buf[2] = addr >> 8;
> +		phy->tx_buf[3] = addr;
> +
> +		spi_message_init(&m);
> +		spi_message_add_tail(&spi_xfer, &m);
> +		ret = spi_sync_locked(phy->spi_device, &m);
> +		if (ret < 0)
> +			goto exit;
>  
> -	spi_bus_lock(phy->spi_device->master);
> -	ret = spi_sync_locked(phy->spi_device, &m);
> -	if (ret < 0)
> -		goto exit;
> -
> -	if ((phy->rx_buf[3] & 0x01) == 0) {
> -		// handle SPI wait states
> -		int i;
> -
> -		phy->tx_buf[0] = 0;
> -
> -		for (i = 0; i < TPM_RETRY; i++) {
> -			spi_xfer.len = 1;
> -			spi_message_init(&m);
> -			spi_message_add_tail(&spi_xfer, &m);
> -			ret = spi_sync_locked(phy->spi_device, &m);
> -			if (ret < 0)
> +		if ((phy->rx_buf[3] & 0x01) == 0) {
> +			// handle SPI wait states
> +			int i;
> +
> +			phy->tx_buf[0] = 0;
> +
> +			for (i = 0; i < TPM_RETRY; i++) {
> +				spi_xfer.len = 1;
> +				spi_message_init(&m);
> +				spi_message_add_tail(&spi_xfer, &m);
> +				ret = spi_sync_locked(phy->spi_device, &m);
> +				if (ret < 0)
> +					goto exit;
> +				if (phy->rx_buf[0] & 0x01)
> +					break;
> +			}
> +
> +			if (i == TPM_RETRY) {
> +				ret = -ETIMEDOUT;
>  				goto exit;
> -			if (phy->rx_buf[0] & 0x01)
> -				break;
> +			}
>  		}
>  
> -		if (i == TPM_RETRY) {
> -			ret = -ETIMEDOUT;
> -			goto exit;
> +		spi_xfer.cs_change = 0;
> +		spi_xfer.len = transfer_len;
> +
> +		if (direction) {
> +			spi_xfer.tx_buf = NULL;
> +			spi_xfer.rx_buf = buffer;
> +		} else {
> +			spi_xfer.tx_buf = buffer;
> +			spi_xfer.rx_buf = NULL;
>  		}
> -	}
>  
> -	spi_xfer.cs_change = 0;
> -	spi_xfer.len = len;
> +		spi_message_init(&m);
> +		spi_message_add_tail(&spi_xfer, &m);
> +		ret = spi_sync_locked(phy->spi_device, &m);
> +		if (ret < 0)
> +			goto exit;
>  
> -	if (direction) {
> -		spi_xfer.tx_buf = NULL;
> -		spi_xfer.rx_buf = buffer;
> -	} else {
> -		spi_xfer.tx_buf = buffer;
> -		spi_xfer.rx_buf = NULL;
> +		len -= transfer_len;
> +		buffer += transfer_len;
>  	}
>  
> -	spi_message_init(&m);
> -	spi_message_add_tail(&spi_xfer, &m);
> -	ret = spi_sync_locked(phy->spi_device, &m);
> -
>  exit:
>  	spi_bus_unlock(phy->spi_device->master);
>  	return ret;
> -- 
> 2.7.4
> 

Re: [PATCH 4/5] tpm_tis_spi: Remove limitation of transfers to MAX_SPI_FRAMESIZE bytes

[Jarkko Sakkinen]

On Thu, Feb 16, 2017 at 04:08:25PM +0000, Peter Huewe wrote:
> Limiting transfers to MAX_SPI_FRAMESIZE was not expected by the upper
> layers, as tpm_tis has no such limitation. Add a loop to hide that
> limitation.
> 
> Cc: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
> Fixes: 0edbfea537d1 ("tpm/tpm_tis_spi: Add support for spi phy")
> Signed-off-by: Alexander Steffen <Alexander.Steffen-d0qZbvYSIPpWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Peter Huewe <peter.huewe-d0qZbvYSIPpWk0Htik3J/w@public.gmane.org>

This commit also had kbuild warning.

PS. When you send the next patch set version, please add this to
every commit:

  Tested-by: Benoit Houyere <benoit.houyere-qxv4g6HH51o@public.gmane.org>

/Jarkko

> ---
>  drivers/char/tpm/tpm_tis_spi.c | 108 ++++++++++++++++++++++-------------------
>  1 file changed, 57 insertions(+), 51 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_tis_spi.c b/drivers/char/tpm/tpm_tis_spi.c
> index 16938e2253d2..b50c5b072df3 100644
> --- a/drivers/char/tpm/tpm_tis_spi.c
> +++ b/drivers/char/tpm/tpm_tis_spi.c
> @@ -61,68 +61,74 @@ static int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u8 len,
>  {
>  	struct tpm_tis_spi_phy *phy = to_tpm_tis_spi_phy(data);
>  	int ret;
> -	struct spi_message m;
> -	struct spi_transfer spi_xfer = {
> -		.tx_buf = phy->tx_buf,
> -		.rx_buf = phy->rx_buf,
> -		.len = 4,
> -		.cs_change = 1,
> -	};
> -
> -	if (len > MAX_SPI_FRAMESIZE)
> -		return -ENOMEM;
>  
> -	phy->tx_buf[0] = direction | (len - 1);
> -	phy->tx_buf[1] = 0xd4;
> -	phy->tx_buf[2] = addr >> 8;
> -	phy->tx_buf[3] = addr;
> +	spi_bus_lock(phy->spi_device->master);
>  
> -	spi_message_init(&m);
> -	spi_message_add_tail(&spi_xfer, &m);
> +	while (len) {
> +		struct spi_message m;
> +		struct spi_transfer spi_xfer = {
> +			.tx_buf = phy->tx_buf,
> +			.rx_buf = phy->rx_buf,
> +			.len = 4,
> +			.cs_change = 1,
> +		};
> +		u8 transfer_len = min_t(u16, len, MAX_SPI_FRAMESIZE);
> +
> +		phy->tx_buf[0] = direction | (transfer_len - 1);
> +		phy->tx_buf[1] = 0xd4;
> +		phy->tx_buf[2] = addr >> 8;
> +		phy->tx_buf[3] = addr;
> +
> +		spi_message_init(&m);
> +		spi_message_add_tail(&spi_xfer, &m);
> +		ret = spi_sync_locked(phy->spi_device, &m);
> +		if (ret < 0)
> +			goto exit;
>  
> -	spi_bus_lock(phy->spi_device->master);
> -	ret = spi_sync_locked(phy->spi_device, &m);
> -	if (ret < 0)
> -		goto exit;
> -
> -	if ((phy->rx_buf[3] & 0x01) == 0) {
> -		// handle SPI wait states
> -		int i;
> -
> -		phy->tx_buf[0] = 0;
> -
> -		for (i = 0; i < TPM_RETRY; i++) {
> -			spi_xfer.len = 1;
> -			spi_message_init(&m);
> -			spi_message_add_tail(&spi_xfer, &m);
> -			ret = spi_sync_locked(phy->spi_device, &m);
> -			if (ret < 0)
> +		if ((phy->rx_buf[3] & 0x01) == 0) {
> +			// handle SPI wait states
> +			int i;
> +
> +			phy->tx_buf[0] = 0;
> +
> +			for (i = 0; i < TPM_RETRY; i++) {
> +				spi_xfer.len = 1;
> +				spi_message_init(&m);
> +				spi_message_add_tail(&spi_xfer, &m);
> +				ret = spi_sync_locked(phy->spi_device, &m);
> +				if (ret < 0)
> +					goto exit;
> +				if (phy->rx_buf[0] & 0x01)
> +					break;
> +			}
> +
> +			if (i == TPM_RETRY) {
> +				ret = -ETIMEDOUT;
>  				goto exit;
> -			if (phy->rx_buf[0] & 0x01)
> -				break;
> +			}
>  		}
>  
> -		if (i == TPM_RETRY) {
> -			ret = -ETIMEDOUT;
> -			goto exit;
> +		spi_xfer.cs_change = 0;
> +		spi_xfer.len = transfer_len;
> +
> +		if (direction) {
> +			spi_xfer.tx_buf = NULL;
> +			spi_xfer.rx_buf = buffer;
> +		} else {
> +			spi_xfer.tx_buf = buffer;
> +			spi_xfer.rx_buf = NULL;
>  		}
> -	}
>  
> -	spi_xfer.cs_change = 0;
> -	spi_xfer.len = len;
> +		spi_message_init(&m);
> +		spi_message_add_tail(&spi_xfer, &m);
> +		ret = spi_sync_locked(phy->spi_device, &m);
> +		if (ret < 0)
> +			goto exit;
>  
> -	if (direction) {
> -		spi_xfer.tx_buf = NULL;
> -		spi_xfer.rx_buf = buffer;
> -	} else {
> -		spi_xfer.tx_buf = buffer;
> -		spi_xfer.rx_buf = NULL;
> +		len -= transfer_len;
> +		buffer += transfer_len;
>  	}
>  
> -	spi_message_init(&m);
> -	spi_message_add_tail(&spi_xfer, &m);
> -	ret = spi_sync_locked(phy->spi_device, &m);
> -
>  exit:
>  	spi_bus_unlock(phy->spi_device->master);
>  	return ret;
> -- 
> 2.7.4
> 

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

Re: [PATCH 0/5] Fix whole native SPI TPM driver

[Jarkko Sakkinen]

On Wed, Feb 22, 2017 at 11:32:17PM +0100, Peter Huewe wrote:
> 
> 
> Am 22. Februar 2017 22:19:24 MEZ schrieb Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>:
> >On Thu, Feb 16, 2017 at 04:08:21PM +0000, Peter Huewe wrote:
> >> During our testing it showed that unfortunately the whole native spi
> >tpm driver
> >> was more or less non-functional since it was merged, e.g. the wrong
> >byte for
> >> waitstate handling was used and transfers larger than 64 bytes did
> >not work at all.
> >> 
> >> This was probably caused by the merging of the different approaches
> >back then,
> >> as the initial RFC patch did not have these problems, and also my
> >sudden lack
> >> of time/commitment back then.
> >> I'm sorry that the final driver code went untested for that long
> >time.
> >> 
> >> This patch set fixes these issues one by one.
> >> In order to avoid duplication the read/write function was
> >consolidated to one
> >> transfer function, so we do not have to apply the same fix at two
> >locations.
> >> Maybe consider squashing it - we splitted it for easier review.
> >>  
> >> Affected Kernels: 4.8, 4.9, 4.10
> >> Patchset was tested on Raspberry Pi2 with SLB9670 (TPM1.2 and TPM2.0)
> >> 
> >> Peter Huewe (5):
> >>   tpm_tis_spi: Use single function to transfer data
> >>   tpm_tis_spi: Abort transfer when too many wait states are signaled
> >>   tpm_tis_spi: Check correct byte for wait state indicator
> >>   tpm_tis_spi: Remove limitation of transfers to MAX_SPI_FRAMESIZE
> >bytes
> >>   tpm_tis_spi: Add small delay after last transfer
> >> 
> >>  drivers/char/tpm/tpm_tis_spi.c | 163
> >+++++++++++++++++------------------------
> >>  1 file changed, 69 insertions(+), 94 deletions(-)
> >> 
> >> -- 
> >> 2.7.4
> >> 
> >
> >I also found type errors a whilea ago:
> >
> >https://patchwork.kernel.org/patch/9547993/
> >
> >Do this take care of these issues? 
> Not that I'm aware of.
> We did not test it on any big endian system yet so the problem never became visible.
> 
> In any case it would be really nice to see this patchset to be applied so people have a working,but maybe not perfect, spi tpm on the common little endian platforms.
> When do you think you can merge and pull-request them?
> 
> Peter

Please fix 4/5 and send v2. Other patches were fine.

/Jarkko

Re: [PATCH 0/5] Fix whole native SPI TPM driver

[Jarkko Sakkinen]

On Wed, Feb 22, 2017 at 11:32:17PM +0100, Peter Huewe wrote:
> 
> 
> Am 22. Februar 2017 22:19:24 MEZ schrieb Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>:
> >On Thu, Feb 16, 2017 at 04:08:21PM +0000, Peter Huewe wrote:
> >> During our testing it showed that unfortunately the whole native spi
> >tpm driver
> >> was more or less non-functional since it was merged, e.g. the wrong
> >byte for
> >> waitstate handling was used and transfers larger than 64 bytes did
> >not work at all.
> >> 
> >> This was probably caused by the merging of the different approaches
> >back then,
> >> as the initial RFC patch did not have these problems, and also my
> >sudden lack
> >> of time/commitment back then.
> >> I'm sorry that the final driver code went untested for that long
> >time.
> >> 
> >> This patch set fixes these issues one by one.
> >> In order to avoid duplication the read/write function was
> >consolidated to one
> >> transfer function, so we do not have to apply the same fix at two
> >locations.
> >> Maybe consider squashing it - we splitted it for easier review.
> >>  
> >> Affected Kernels: 4.8, 4.9, 4.10
> >> Patchset was tested on Raspberry Pi2 with SLB9670 (TPM1.2 and TPM2.0)
> >> 
> >> Peter Huewe (5):
> >>   tpm_tis_spi: Use single function to transfer data
> >>   tpm_tis_spi: Abort transfer when too many wait states are signaled
> >>   tpm_tis_spi: Check correct byte for wait state indicator
> >>   tpm_tis_spi: Remove limitation of transfers to MAX_SPI_FRAMESIZE
> >bytes
> >>   tpm_tis_spi: Add small delay after last transfer
> >> 
> >>  drivers/char/tpm/tpm_tis_spi.c | 163
> >+++++++++++++++++------------------------
> >>  1 file changed, 69 insertions(+), 94 deletions(-)
> >> 
> >> -- 
> >> 2.7.4
> >> 
> >
> >I also found type errors a whilea ago:
> >
> >https://patchwork.kernel.org/patch/9547993/
> >
> >Do this take care of these issues? 
> Not that I'm aware of.
> We did not test it on any big endian system yet so the problem never became visible.
> 
> In any case it would be really nice to see this patchset to be applied so people have a working,but maybe not perfect, spi tpm on the common little endian platforms.
> When do you think you can merge and pull-request them?
> 
> Peter

Please fix 4/5 and send v2. Other patches were fine.

/Jarkko

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot