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

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

v2: Applied Jarkko's Comments

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 | 160 ++++++++++++++++++-----------------------
 1 file changed, 68 insertions(+), 92 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 v2 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)

v2: u16 instead of u8 for the length.
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>
Tested-by: Benoit Houyere <benoit.houyere-qxv4g6HH51o@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..062799e04f04 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, u16 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 v2 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>
Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Tested-by: Benoit Houyere <benoit.houyere-qxv4g6HH51o@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 062799e04f04..639614f2d415 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, u16 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 v2 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>
Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Tested-by: Benoit Houyere <benoit.houyere-qxv4g6HH51o@public.gmane.org>
---
 drivers/char/tpm/tpm_tis_spi.c | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis_spi.c b/drivers/char/tpm/tpm_tis_spi.c
index 639614f2d415..62f50b6c9ef6 100644
--- a/drivers/char/tpm/tpm_tis_spi.c
+++ b/drivers/char/tpm/tpm_tis_spi.c
@@ -85,25 +85,25 @@ static int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u16 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
+		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 v2 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.

v2: Moved scope of spi_message to the top as requested by Jarkko
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>
Tested-by: Benoit Houyere <benoit.houyere-qxv4g6HH51o@public.gmane.org>
---
 drivers/char/tpm/tpm_tis_spi.c | 107 ++++++++++++++++++++++-------------------
 1 file changed, 58 insertions(+), 49 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis_spi.c b/drivers/char/tpm/tpm_tis_spi.c
index 62f50b6c9ef6..3015c8b65f18 100644
--- a/drivers/char/tpm/tpm_tis_spi.c
+++ b/drivers/char/tpm/tpm_tis_spi.c
@@ -60,67 +60,76 @@ static int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u16 len,
 				u8 *buffer, u8 direction)
 {
 	struct tpm_tis_spi_phy *phy = to_tpm_tis_spi_phy(data);
-	int ret, i;
+	int ret = 0;
+	int i;
 	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;
+	struct spi_transfer spi_xfer;
+	u8 transfer_len;
 
-	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) {
+		transfer_len = min_t(u16, len, MAX_SPI_FRAMESIZE);
 
-	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
-		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)
+		phy->tx_buf[0] = direction | (transfer_len - 1);
+		phy->tx_buf[1] = 0xd4;
+		phy->tx_buf[2] = addr >> 8;
+		phy->tx_buf[3] = addr;
+
+		memset(&spi_xfer, 0, sizeof(spi_xfer));
+		spi_xfer.tx_buf = phy->tx_buf;
+		spi_xfer.rx_buf = phy->rx_buf;
+		spi_xfer.len = 4;
+		spi_xfer.cs_change = 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[3] & 0x01) == 0) {
+			// handle SPI wait states
+			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

[PATCH v2 5/5] tpm_tis_spi: Add small delay after last transfer

[Peter Huewe]

Testing the implementation with a Raspberry Pi 2 showed that under some
circumstances its SPI master erroneously releases the CS line before the
transfer is complete, i.e. before the end of the last clock. In this case
the TPM ignores the transfer and misses for example the GO command. The
driver is unable to detect this communication problem and will wait for a
command response that is never going to arrive, timing out eventually.

As a workaround, the small delay ensures that the CS line is held long
enough, even with a faulty SPI master. Other SPI masters are not affected,
except for a negligible performance penalty.

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>
Tested-by: Benoit Houyere <benoit.houyere-qxv4g6HH51o@public.gmane.org>
---
 drivers/char/tpm/tpm_tis_spi.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/char/tpm/tpm_tis_spi.c b/drivers/char/tpm/tpm_tis_spi.c
index 3015c8b65f18..88fe72ae967f 100644
--- a/drivers/char/tpm/tpm_tis_spi.c
+++ b/drivers/char/tpm/tpm_tis_spi.c
@@ -111,6 +111,7 @@ static int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u16 len,
 
 		spi_xfer.cs_change = 0;
 		spi_xfer.len = transfer_len;
+		spi_xfer.delay_usecs = 5;
 
 		if (direction) {
 			spi_xfer.tx_buf = NULL;
-- 
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 v2 0/5] Fix whole native SPI TPM driver

[Jarkko Sakkinen]

On Thu, Mar 02, 2017 at 01:03:10PM +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)
> 
> v2: Applied Jarkko's Comments
> 
> 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 | 160 ++++++++++++++++++-----------------------
>  1 file changed, 68 insertions(+), 92 deletions(-)

I'll apply these.

/Jarkko

------------------------------------------------------------------------------
Announcing the Oxford Dictionaries API! The API offers world-renowned
dictionary content that is easy and intuitive to access. Sign up for an
account today to start using our lexical data to power your apps and
projects. Get started today and enter our developer competition.
http://sdm.link/oxford