[RFC][PATCH 0/9] tpm: fix driver so that burstcount can be safely ignored

[RFC][PATCH 0/9] tpm: fix driver so that burstcount can be safely ignored

[Alexander Steffen]

Disclaimer: This is RFC because unfortunately I do not have the time to bring it up to the usual standards. But from lying around on my hard disk it definitely won't improve, so I've decided to publish it now, to get at least some feedback or maybe for somebody else to pick it up. Since the work began some time ago, it is probably not rebased against the current state (but hopefully still applies).

This is a collection of all the fixes I made during the investigation of the problems with the "ignore burstcount" change. These are mainly fixes for the wait state handling in tpm_tis_spi, that probably was not really tested so far, since the previous implementation avoided wait states as far as possible. It also includes changes to tpm_tis_core, so that it follows the specification more closely. Finally, I've included a rebased version of the original "ignore burstcount" patch. Together, those patches pass all my tests and also each intermediate step is fine (so that bisecting still works).

Alexander Steffen (8):
  tpm_tis_core: clean up whitespace
  tpm_tis_core: access single TIS registers before doing complex
    transfers
  tpm_tis_core: correctly wait for flags to become zero
  tpm_tis_core: send all data in single operation
  tpm_tis_core: use XDATA_FIFO for transfers if available
  tpm_tis_spi: fix sending wrong data during wait state handling
  tpm_tis_spi: release CS line when wait state handling fails
  tpm_tis_spi: add delay between wait state retries

Nayna Jain (1):
  tpm: ignore burstcount to improve tpm_tis send() performance

 drivers/char/tpm/tpm_tis_core.c | 131 ++++++++++++++++++++--------------------
 drivers/char/tpm/tpm_tis_core.h |  26 ++++----
 drivers/char/tpm/tpm_tis_spi.c  |  21 +++++--
 3 files changed, 95 insertions(+), 83 deletions(-)

-- 
2.7.4

[RFC][PATCH 1/9] tpm_tis_core: clean up whitespace

[Alexander Steffen]

Replace tabs with spaces where those are commonly used.

Signed-off-by: Alexander Steffen <Alexander.Steffen@infineon.com>
---
 drivers/char/tpm/tpm_tis_core.h | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h
index 6bbac31..357df13 100644
--- a/drivers/char/tpm/tpm_tis_core.h
+++ b/drivers/char/tpm/tpm_tis_core.h
@@ -67,17 +67,17 @@ enum tis_defaults {
 #define TIS_TIMEOUT_C_MAX	max(TIS_SHORT_TIMEOUT, TPM2_TIMEOUT_C)
 #define TIS_TIMEOUT_D_MAX	max(TIS_SHORT_TIMEOUT, TPM2_TIMEOUT_D)
 
-#define	TPM_ACCESS(l)			(0x0000 | ((l) << 12))
-#define	TPM_INT_ENABLE(l)		(0x0008 | ((l) << 12))
-#define	TPM_INT_VECTOR(l)		(0x000C | ((l) << 12))
-#define	TPM_INT_STATUS(l)		(0x0010 | ((l) << 12))
-#define	TPM_INTF_CAPS(l)		(0x0014 | ((l) << 12))
-#define	TPM_STS(l)			(0x0018 | ((l) << 12))
-#define	TPM_STS3(l)			(0x001b | ((l) << 12))
-#define	TPM_DATA_FIFO(l)		(0x0024 | ((l) << 12))
-
-#define	TPM_DID_VID(l)			(0x0F00 | ((l) << 12))
-#define	TPM_RID(l)			(0x0F04 | ((l) << 12))
+#define TPM_ACCESS(l)			(0x0000 | ((l) << 12))
+#define TPM_INT_ENABLE(l)		(0x0008 | ((l) << 12))
+#define TPM_INT_VECTOR(l)		(0x000C | ((l) << 12))
+#define TPM_INT_STATUS(l)		(0x0010 | ((l) << 12))
+#define TPM_INTF_CAPS(l)		(0x0014 | ((l) << 12))
+#define TPM_STS(l)			(0x0018 | ((l) << 12))
+#define TPM_STS3(l)			(0x001b | ((l) << 12))
+#define TPM_DATA_FIFO(l)		(0x0024 | ((l) << 12))
+
+#define TPM_DID_VID(l)			(0x0F00 | ((l) << 12))
+#define TPM_RID(l)			(0x0F04 | ((l) << 12))
 
 enum tpm_tis_flags {
 	TPM_TIS_ITPM_WORKAROUND		= BIT(0),
-- 
2.7.4

Re: [RFC][PATCH 1/9] tpm_tis_core: clean up whitespace

[Jarkko Sakkinen]

On Fri, Dec 08, 2017 at 07:46:50PM +0100, Alexander Steffen wrote:
> Replace tabs with spaces where those are commonly used.

You are replacing tab with eight spaces or? 

/Jarkko

[RFC][PATCH 2/9] tpm_tis_core: access single TIS registers before doing complex transfers

[Alexander Steffen]

There are several TIS registers that contain information on how exactly the
TPM implements the TIS protocol. Complex commands that are used by the
probe functions should only be sent to the TPM after all other registers
have been read and the driver is properly configured to handle those
transactions.

Signed-off-by: Alexander Steffen <Alexander.Steffen@infineon.com>
---
 drivers/char/tpm/tpm_tis_core.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index ca6b2b5..d367016 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -781,10 +781,6 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
 	intmask &= ~TPM_GLOBAL_INT_ENABLE;
 	tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), intmask);
 
-	rc = tpm2_probe(chip);
-	if (rc)
-		goto out_err;
-
 	rc = tpm_tis_read32(priv, TPM_DID_VID(0), &vendor);
 	if (rc < 0)
 		goto out_err;
@@ -795,16 +791,6 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
 	if (rc < 0)
 		goto out_err;
 
-	dev_info(dev, "%s TPM (device-id 0x%X, rev-id %d)\n",
-		 (chip->flags & TPM_CHIP_FLAG_TPM2) ? "2.0" : "1.2",
-		 vendor >> 16, rid);
-
-	probe = probe_itpm(chip);
-	if (probe < 0) {
-		rc = -ENODEV;
-		goto out_err;
-	}
-
 	/* Figure out the capabilities */
 	rc = tpm_tis_read32(priv, TPM_INTF_CAPS(priv->locality), &intfcaps);
 	if (rc < 0)
@@ -831,6 +817,20 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
 	if (intfcaps & TPM_INTF_DATA_AVAIL_INT)
 		dev_dbg(dev, "\tData Avail Int Support\n");
 
+	rc = tpm2_probe(chip);
+	if (rc)
+		goto out_err;
+
+	dev_info(dev, "%s TPM (device-id 0x%X, rev-id %d)\n",
+		 (chip->flags & TPM_CHIP_FLAG_TPM2) ? "2.0" : "1.2",
+		 vendor >> 16, rid);
+
+	probe = probe_itpm(chip);
+	if (probe < 0) {
+		rc = -ENODEV;
+		goto out_err;
+	}
+
 	/* INTERRUPT Setup */
 	init_waitqueue_head(&priv->read_queue);
 	init_waitqueue_head(&priv->int_queue);
-- 
2.7.4

Re: [RFC][PATCH 2/9] tpm_tis_core: access single TIS registers before doing complex transfers

[Jarkko Sakkinen]

On Fri, Dec 08, 2017 at 07:46:51PM +0100, Alexander Steffen wrote:
> There are several TIS registers that contain information on how exactly the
> TPM implements the TIS protocol. Complex commands that are used by the
> probe functions should only be sent to the TPM after all other registers
> have been read and the driver is properly configured to handle those
> transactions.
> 
> Signed-off-by: Alexander Steffen <Alexander.Steffen@infineon.com>

The code change looks ok but the commit message does not describe what
the commit does. It describes a guideline and uses an abstract term
"complex command".

/Jarkko

[RFC][PATCH 3/9] tpm_tis_core: correctly wait for flags to become zero

[Alexander Steffen]

According to TIS/PTP the dataAvail flag and the Expect flag in the STS
register contain valid values if and only if the stsValid flag in the same
register is set. Currently, the code first waits for the stsValid flag to
be set and then looks at the other flags. This causes the STS register to
be read twice, so that the stsValid flag might not be set anymore when the
other flags are evaluated.

Other parts of the code already check both flags in a single operation
within wait_for_tpm_stat. But the current implementation can only check for
flags being set to 1, not 0. Therefore, add a parameter to
wait_for_tpm_stat that allows to specify the expected value in addition to
the selected flags and adapt all callers accordingly.

In addition, this now checks the dataAvail and Expect flags multiple times
within the specified timeout, so those flags no longer need to have the
expected value right away. This is important for example when sending large
amounts of data to the TPM, when the TPM might not process its I/O buffer
fast enough for the flags to be set correctly when they are checked for the
first time.

Signed-off-by: Alexander Steffen <Alexander.Steffen@infineon.com>
---
 drivers/char/tpm/tpm_tis_core.c | 51 +++++++++++++++++------------------------
 1 file changed, 21 insertions(+), 30 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index d367016..0df05b4 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -37,13 +37,13 @@
  */
 #define TPM_POLL_SLEEP	1  /* msec */
 
-static bool wait_for_tpm_stat_cond(struct tpm_chip *chip, u8 mask,
+static bool wait_for_tpm_stat_cond(struct tpm_chip *chip, u8 mask, u8 value,
 					bool check_cancel, bool *canceled)
 {
 	u8 status = chip->ops->status(chip);
 
 	*canceled = false;
-	if ((status & mask) == mask)
+	if ((status & mask) == value)
 		return true;
 	if (check_cancel && chip->ops->req_canceled(chip, status)) {
 		*canceled = true;
@@ -52,7 +52,7 @@ static bool wait_for_tpm_stat_cond(struct tpm_chip *chip, u8 mask,
 	return false;
 }
 
-static int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask,
+static int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask, u8 value,
 		unsigned long timeout, wait_queue_head_t *queue,
 		bool check_cancel)
 {
@@ -63,7 +63,7 @@ static int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask,
 
 	/* check current status */
 	status = chip->ops->status(chip);
-	if ((status & mask) == mask)
+	if ((status & mask) == value)
 		return 0;
 
 	stop = jiffies + timeout;
@@ -74,7 +74,7 @@ static int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask,
 		if ((long)timeout <= 0)
 			return -ETIME;
 		rc = wait_event_interruptible_timeout(*queue,
-			wait_for_tpm_stat_cond(chip, mask, check_cancel,
+			wait_for_tpm_stat_cond(chip, mask, value, check_cancel,
 					       &canceled),
 			timeout);
 		if (rc > 0) {
@@ -90,7 +90,7 @@ static int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask,
 		do {
 			tpm_msleep(TPM_POLL_SLEEP);
 			status = chip->ops->status(chip);
-			if ((status & mask) == mask)
+			if ((status & mask) == value)
 				return 0;
 		} while (time_before(jiffies, stop));
 	}
@@ -243,6 +243,7 @@ static int recv_data(struct tpm_chip *chip, u8 *buf, size_t count)
 	while (size < count) {
 		rc = wait_for_tpm_stat(chip,
 				 TPM_STS_DATA_AVAIL | TPM_STS_VALID,
+				 TPM_STS_DATA_AVAIL | TPM_STS_VALID,
 				 chip->timeout_c,
 				 &priv->read_queue, true);
 		if (rc < 0)
@@ -268,7 +269,7 @@ static int tpm_tis_recv(struct tpm_chip *chip, u8 *buf, size_t count)
 {
 	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
 	int size = 0;
-	int expected, status;
+	int expected;
 
 	if (count < TPM_HEADER_SIZE) {
 		size = -EIO;
@@ -296,13 +297,9 @@ static int tpm_tis_recv(struct tpm_chip *chip, u8 *buf, size_t count)
 		goto out;
 	}
 
-	if (wait_for_tpm_stat(chip, TPM_STS_VALID, chip->timeout_c,
+	if (wait_for_tpm_stat(chip, TPM_STS_DATA_AVAIL | TPM_STS_VALID,
+			      TPM_STS_VALID, chip->timeout_c,
 				&priv->int_queue, false) < 0) {
-		size = -ETIME;
-		goto out;
-	}
-	status = tpm_tis_status(chip);
-	if (status & TPM_STS_DATA_AVAIL) {	/* retry? */
 		dev_err(&chip->dev, "Error left over data\n");
 		size = -EIO;
 		goto out;
@@ -329,8 +326,8 @@ static int tpm_tis_send_data(struct tpm_chip *chip, const u8 *buf, size_t len)
 	if ((status & TPM_STS_COMMAND_READY) == 0) {
 		tpm_tis_ready(chip);
 		if (wait_for_tpm_stat
-		    (chip, TPM_STS_COMMAND_READY, chip->timeout_b,
-		     &priv->int_queue, false) < 0) {
+		    (chip, TPM_STS_COMMAND_READY, TPM_STS_COMMAND_READY,
+		     chip->timeout_b, &priv->int_queue, false) < 0) {
 			rc = -ETIME;
 			goto out_err;
 		}
@@ -351,13 +348,10 @@ static int tpm_tis_send_data(struct tpm_chip *chip, const u8 *buf, size_t len)
 
 		count += burstcnt;
 
-		if (wait_for_tpm_stat(chip, TPM_STS_VALID, chip->timeout_c,
-					&priv->int_queue, false) < 0) {
-			rc = -ETIME;
-			goto out_err;
-		}
-		status = tpm_tis_status(chip);
-		if (!itpm && (status & TPM_STS_DATA_EXPECT) == 0) {
+		if (!itpm && wait_for_tpm_stat
+			     (chip, TPM_STS_DATA_EXPECT | TPM_STS_VALID,
+			      TPM_STS_DATA_EXPECT | TPM_STS_VALID,
+			      chip->timeout_c, &priv->int_queue, false) < 0) {
 			rc = -EIO;
 			goto out_err;
 		}
@@ -368,13 +362,9 @@ static int tpm_tis_send_data(struct tpm_chip *chip, const u8 *buf, size_t len)
 	if (rc < 0)
 		goto out_err;
 
-	if (wait_for_tpm_stat(chip, TPM_STS_VALID, chip->timeout_c,
-				&priv->int_queue, false) < 0) {
-		rc = -ETIME;
-		goto out_err;
-	}
-	status = tpm_tis_status(chip);
-	if (!itpm && (status & TPM_STS_DATA_EXPECT) != 0) {
+	if (!itpm && wait_for_tpm_stat
+		     (chip, TPM_STS_DATA_EXPECT | TPM_STS_VALID, TPM_STS_VALID,
+		      chip->timeout_c, &priv->int_queue, false) < 0) {
 		rc = -EIO;
 		goto out_err;
 	}
@@ -434,7 +424,8 @@ static int tpm_tis_send_main(struct tpm_chip *chip, const u8 *buf, size_t len)
 			dur = tpm_calc_ordinal_duration(chip, ordinal);
 
 		if (wait_for_tpm_stat
-		    (chip, TPM_STS_DATA_AVAIL | TPM_STS_VALID, dur,
+		    (chip, TPM_STS_DATA_AVAIL | TPM_STS_VALID,
+		     TPM_STS_DATA_AVAIL | TPM_STS_VALID, dur,
 		     &priv->read_queue, false) < 0) {
 			rc = -ETIME;
 			goto out_err;
-- 
2.7.4

Re: [RFC][PATCH 3/9] tpm_tis_core: correctly wait for flags to become zero

[Jarkko Sakkinen]

On Fri, Dec 08, 2017 at 07:46:52PM +0100, Alexander Steffen wrote:
> According to TIS/PTP the dataAvail flag and the Expect flag in the STS
> register contain valid values if and only if the stsValid flag in the same
> register is set. Currently, the code first waits for the stsValid flag to

What is "the code"?

> be set and then looks at the other flags. This causes the STS register to
> be read twice, so that the stsValid flag might not be set anymore when the
> other flags are evaluated.

I can understand from this paragraph is that flags should be set atomically,
which makes sense.

> Other parts of the code already check both flags in a single operation
> within wait_for_tpm_stat. But the current implementation can only check for
> flags being set to 1, not 0. Therefore, add a parameter to
> wait_for_tpm_stat that allows to specify the expected value in addition to
> the selected flags and adapt all callers accordingly.

What are the "other parts of the code"?

> In addition, this now checks the dataAvail and Expect flags multiple times

What is "this"?

> within the specified timeout, so those flags no longer need to have the
> expected value right away. This is important for example when sending large
> amounts of data to the TPM, when the TPM might not process its I/O buffer
> fast enough for the flags to be set correctly when they are checked for the
> first time.

About the code change itself. Is there any play where you would not
bitwise or TPM_STS_VALID in mask and value parameters?

I guess I understand what you are doing but the commit message is
complete nonsense.

/Jarkko

Re: [RFC][PATCH 3/9] tpm_tis_core: correctly wait for flags to become zero

[Jarkko Sakkinen]

On Fri, Dec 08, 2017 at 07:46:52PM +0100, Alexander Steffen wrote:
> According to TIS/PTP the dataAvail flag and the Expect flag in the STS
> register contain valid values if and only if the stsValid flag in the same
> register is set. Currently, the code first waits for the stsValid flag to
> be set and then looks at the other flags. This causes the STS register to
> be read twice, so that the stsValid flag might not be set anymore when the
> other flags are evaluated.
> 
> Other parts of the code already check both flags in a single operation
> within wait_for_tpm_stat. But the current implementation can only check for
> flags being set to 1, not 0. Therefore, add a parameter to
> wait_for_tpm_stat that allows to specify the expected value in addition to
> the selected flags and adapt all callers accordingly.
> 
> In addition, this now checks the dataAvail and Expect flags multiple times
> within the specified timeout, so those flags no longer need to have the
> expected value right away. This is important for example when sending large
> amounts of data to the TPM, when the TPM might not process its I/O buffer
> fast enough for the flags to be set correctly when they are checked for the
> first time.
> 
> Signed-off-by: Alexander Steffen <Alexander.Steffen@infineon.com>

LGTM

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

/Jarkko

Re: [RFC][PATCH 3/9] tpm_tis_core: correctly wait for flags to become zero

[Jarkko Sakkinen]

On Thu, Jan 18, 2018 at 07:58:32PM +0200, Jarkko Sakkinen wrote:
> On Fri, Dec 08, 2017 at 07:46:52PM +0100, Alexander Steffen wrote:
> > According to TIS/PTP the dataAvail flag and the Expect flag in the STS
> > register contain valid values if and only if the stsValid flag in the same
> > register is set. Currently, the code first waits for the stsValid flag to
> > be set and then looks at the other flags. This causes the STS register to
> > be read twice, so that the stsValid flag might not be set anymore when the
> > other flags are evaluated.
> > 
> > Other parts of the code already check both flags in a single operation
> > within wait_for_tpm_stat. But the current implementation can only check for
> > flags being set to 1, not 0. Therefore, add a parameter to
> > wait_for_tpm_stat that allows to specify the expected value in addition to
> > the selected flags and adapt all callers accordingly.
> > 
> > In addition, this now checks the dataAvail and Expect flags multiple times
> > within the specified timeout, so those flags no longer need to have the
> > expected value right away. This is important for example when sending large
> > amounts of data to the TPM, when the TPM might not process its I/O buffer
> > fast enough for the flags to be set correctly when they are checked for the
> > first time.
> > 
> > Signed-off-by: Alexander Steffen <Alexander.Steffen@infineon.com>
> 
> LGTM
> 
> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> 
> /Jarkko

Ugh, was meant for 4/9 :-) Ignore this.

/Jarkko

[RFC][PATCH 4/9] tpm_tis_core: send all data in single operation

[Alexander Steffen]

tpm_tis_send_data splits all commands sent to the TPM into at least two
transfers, even if the commands are small enough to fit into a single
transfer. The intention seems to be to make extra sure that the TPM has
received all data correctly by observing the Expect flag flipping from 1 to
0 when writing the last byte.

Unfortunately, this does not work as intended, because the Expect flag will
change not when the last byte is written to the FIFO, but when the TPM
processes the last byte from the FIFO. With a large FIFO and long commands
it might well be that there are still many bytes left in the FIFO when the
Expect flag is checked. Obviously, the flag will be 1 then, even if the
FIFO contains more bytes than expected. Since there is no indication
whether the FIFO is already empty or not, there is no way to implement this
check reliably.

But then again, this check is not necessary at all. The driver already
ensures that not more data is sent to the TPM than is announced in the
header. In addition, the TPM is required to throw away all extraneous bytes
anyway. So if the Expect flag is 0 after all bytes have been sent to the
TPM, the TPM has received the command correctly and is ready to execute it.

Therefore, remove this intermediary check and send all data in a single
operation. This simplifies the code and improves the performance,
especially for short commands.

Signed-off-by: Alexander Steffen <Alexander.Steffen@infineon.com>
---
 drivers/char/tpm/tpm_tis_core.c | 25 ++++++-------------------
 1 file changed, 6 insertions(+), 19 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 0df05b4..1359c52 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -320,7 +320,6 @@ static int tpm_tis_send_data(struct tpm_chip *chip, const u8 *buf, size_t len)
 	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
 	int rc, status, burstcnt;
 	size_t count = 0;
-	bool itpm = priv->flags & TPM_TIS_ITPM_WORKAROUND;
 
 	status = tpm_tis_status(chip);
 	if ((status & TPM_STS_COMMAND_READY) == 0) {
@@ -333,38 +332,26 @@ static int tpm_tis_send_data(struct tpm_chip *chip, const u8 *buf, size_t len)
 		}
 	}
 
-	while (count < len - 1) {
+	while (count < len) {
 		burstcnt = get_burstcount(chip);
 		if (burstcnt < 0) {
 			dev_err(&chip->dev, "Unable to read burstcount\n");
 			rc = burstcnt;
 			goto out_err;
 		}
-		burstcnt = min_t(int, burstcnt, len - count - 1);
+		burstcnt = min_t(int, burstcnt, len - count);
 		rc = tpm_tis_write_bytes(priv, TPM_DATA_FIFO(priv->locality),
 					 burstcnt, buf + count);
 		if (rc < 0)
 			goto out_err;
 
 		count += burstcnt;
-
-		if (!itpm && wait_for_tpm_stat
-			     (chip, TPM_STS_DATA_EXPECT | TPM_STS_VALID,
-			      TPM_STS_DATA_EXPECT | TPM_STS_VALID,
-			      chip->timeout_c, &priv->int_queue, false) < 0) {
-			rc = -EIO;
-			goto out_err;
-		}
 	}
 
-	/* write last byte */
-	rc = tpm_tis_write8(priv, TPM_DATA_FIFO(priv->locality), buf[count]);
-	if (rc < 0)
-		goto out_err;
-
-	if (!itpm && wait_for_tpm_stat
-		     (chip, TPM_STS_DATA_EXPECT | TPM_STS_VALID, TPM_STS_VALID,
-		      chip->timeout_c, &priv->int_queue, false) < 0) {
+	if (!(priv->flags & TPM_TIS_ITPM_WORKAROUND) &&
+	    wait_for_tpm_stat(chip, TPM_STS_DATA_EXPECT | TPM_STS_VALID,
+			      TPM_STS_VALID, chip->timeout_c, &priv->int_queue,
+			      false) < 0) {
 		rc = -EIO;
 		goto out_err;
 	}
-- 
2.7.4

Re: [RFC][PATCH 4/9] tpm_tis_core: send all data in single operation

[Nayna Jain]

On 12/09/2017 12:16 AM, Alexander Steffen wrote:
> tpm_tis_send_data splits all commands sent to the TPM into at least two
> transfers, even if the commands are small enough to fit into a single
> transfer. The intention seems to be to make extra sure that the TPM has
> received all data correctly by observing the Expect flag flipping from 1 to
> 0 when writing the last byte.
>
> Unfortunately, this does not work as intended, because the Expect flag will
> change not when the last byte is written to the FIFO, but when the TPM
> processes the last byte from the FIFO. With a large FIFO and long commands
> it might well be that there are still many bytes left in the FIFO when the
> Expect flag is checked. Obviously, the flag will be 1 then, even if the
> FIFO contains more bytes than expected. Since there is no indication
> whether the FIFO is already empty or not, there is no way to implement this
> check reliably.
>
> But then again, this check is not necessary at all. The driver already
> ensures that not more data is sent to the TPM than is announced in the
> header. In addition, the TPM is required to throw away all extraneous bytes
> anyway. So if the Expect flag is 0 after all bytes have been sent to the
> TPM, the TPM has received the command correctly and is ready to execute it.
>
> Therefore, remove this intermediary check and send all data in a single
> operation. This simplifies the code and improves the performance,
> especially for short commands.
I was thinking if this can be taken care as part of burstcount patch itself.

Thanks & Regards,
     - Nayna

> Signed-off-by: Alexander Steffen <Alexander.Steffen@infineon.com>
> ---
>   drivers/char/tpm/tpm_tis_core.c | 25 ++++++-------------------
>   1 file changed, 6 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index 0df05b4..1359c52 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -320,7 +320,6 @@ static int tpm_tis_send_data(struct tpm_chip *chip, const u8 *buf, size_t len)
>   	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
>   	int rc, status, burstcnt;
>   	size_t count = 0;
> -	bool itpm = priv->flags & TPM_TIS_ITPM_WORKAROUND;
>
>   	status = tpm_tis_status(chip);
>   	if ((status & TPM_STS_COMMAND_READY) == 0) {
> @@ -333,38 +332,26 @@ static int tpm_tis_send_data(struct tpm_chip *chip, const u8 *buf, size_t len)
>   		}
>   	}
>
> -	while (count < len - 1) {
> +	while (count < len) {
>   		burstcnt = get_burstcount(chip);
>   		if (burstcnt < 0) {
>   			dev_err(&chip->dev, "Unable to read burstcount\n");
>   			rc = burstcnt;
>   			goto out_err;
>   		}
> -		burstcnt = min_t(int, burstcnt, len - count - 1);
> +		burstcnt = min_t(int, burstcnt, len - count);
>   		rc = tpm_tis_write_bytes(priv, TPM_DATA_FIFO(priv->locality),
>   					 burstcnt, buf + count);
>   		if (rc < 0)
>   			goto out_err;
>
>   		count += burstcnt;
> -
> -		if (!itpm && wait_for_tpm_stat
> -			     (chip, TPM_STS_DATA_EXPECT | TPM_STS_VALID,
> -			      TPM_STS_DATA_EXPECT | TPM_STS_VALID,
> -			      chip->timeout_c, &priv->int_queue, false) < 0) {
> -			rc = -EIO;
> -			goto out_err;
> -		}
>   	}
>
> -	/* write last byte */
> -	rc = tpm_tis_write8(priv, TPM_DATA_FIFO(priv->locality), buf[count]);
> -	if (rc < 0)
> -		goto out_err;
> -
> -	if (!itpm && wait_for_tpm_stat
> -		     (chip, TPM_STS_DATA_EXPECT | TPM_STS_VALID, TPM_STS_VALID,
> -		      chip->timeout_c, &priv->int_queue, false) < 0) {
> +	if (!(priv->flags & TPM_TIS_ITPM_WORKAROUND) &&
> +	    wait_for_tpm_stat(chip, TPM_STS_DATA_EXPECT | TPM_STS_VALID,
> +			      TPM_STS_VALID, chip->timeout_c, &priv->int_queue,
> +			      false) < 0) {
>   		rc = -EIO;
>   		goto out_err;
>   	}

Re: [RFC][PATCH 4/9] tpm_tis_core: send all data in single operation

[Jarkko Sakkinen]

On Fri, Dec 08, 2017 at 07:46:53PM +0100, Alexander Steffen wrote:
> tpm_tis_send_data splits all commands sent to the TPM into at least two
> transfers, even if the commands are small enough to fit into a single
> transfer. The intention seems to be to make extra sure that the TPM has
> received all data correctly by observing the Expect flag flipping from 1 to
> 0 when writing the last byte.
> 
> Unfortunately, this does not work as intended, because the Expect flag will
> change not when the last byte is written to the FIFO, but when the TPM
> processes the last byte from the FIFO. With a large FIFO and long commands
> it might well be that there are still many bytes left in the FIFO when the
> Expect flag is checked. Obviously, the flag will be 1 then, even if the
> FIFO contains more bytes than expected. Since there is no indication
> whether the FIFO is already empty or not, there is no way to implement this
> check reliably.
> 
> But then again, this check is not necessary at all. The driver already
> ensures that not more data is sent to the TPM than is announced in the
> header. In addition, the TPM is required to throw away all extraneous bytes
> anyway. So if the Expect flag is 0 after all bytes have been sent to the
> TPM, the TPM has received the command correctly and is ready to execute it.
> 
> Therefore, remove this intermediary check and send all data in a single
> operation. This simplifies the code and improves the performance,
> especially for short commands.
> 
> Signed-off-by: Alexander Steffen <Alexander.Steffen@infineon.com>

LGTM

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

/Jarkko

[RFC][PATCH 5/9] tpm_tis_core: use XDATA_FIFO for transfers if available

[Alexander Steffen]

The TPM indicates in DataTransferSizeSupport in TPM_INTF_CAPABILITY what
the maximum supported transfer size is. Transfers larger than four bytes
are only supported for the XDATA_FIFO, not the legacy DATA_FIFO. Therefore,
always use the XDATA_FIFO when the TPM indicates support for non-legacy
transfer sizes for maximum throughput.

Signed-off-by: Alexander Steffen <Alexander.Steffen@infineon.com>
---
 drivers/char/tpm/tpm_tis_core.c | 24 ++++++++++++++++++++++--
 drivers/char/tpm/tpm_tis_core.h |  4 ++++
 drivers/char/tpm/tpm_tis_spi.c  |  2 +-
 3 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 1359c52..7240ee4 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -255,7 +255,8 @@ static int recv_data(struct tpm_chip *chip, u8 *buf, size_t count)
 		}
 		burstcnt = min_t(int, burstcnt, count - size);
 
-		rc = tpm_tis_read_bytes(priv, TPM_DATA_FIFO(priv->locality),
+		rc = tpm_tis_read_bytes(priv, ADD_LOCALITY(priv->fifo_address,
+							   priv->locality),
 					burstcnt, buf + size);
 		if (rc < 0)
 			return rc;
@@ -340,7 +341,8 @@ static int tpm_tis_send_data(struct tpm_chip *chip, const u8 *buf, size_t len)
 			goto out_err;
 		}
 		burstcnt = min_t(int, burstcnt, len - count);
-		rc = tpm_tis_write_bytes(priv, TPM_DATA_FIFO(priv->locality),
+		rc = tpm_tis_write_bytes(priv, ADD_LOCALITY(priv->fifo_address,
+							    priv->locality),
 					 burstcnt, buf + count);
 		if (rc < 0)
 			goto out_err;
@@ -742,6 +744,7 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
 	chip->timeout_c = msecs_to_jiffies(TIS_TIMEOUT_C_MAX);
 	chip->timeout_d = msecs_to_jiffies(TIS_TIMEOUT_D_MAX);
 	priv->phy_ops = phy_ops;
+	priv->max_transfer_size = 4;
 	dev_set_drvdata(&chip->dev, priv);
 
 	if (wait_startup(chip, 0) != 0) {
@@ -794,6 +797,23 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
 		dev_dbg(dev, "\tSts Valid Int Support\n");
 	if (intfcaps & TPM_INTF_DATA_AVAIL_INT)
 		dev_dbg(dev, "\tData Avail Int Support\n");
+	switch ((intfcaps >> 9) & 3) {
+		case 0:
+			priv->fifo_address = TPM_DATA_FIFO(0);
+			break;
+		case 1:
+			priv->fifo_address = TPM_XDATA_FIFO(0);
+			priv->max_transfer_size = 8;
+			break;
+		case 2:
+			priv->fifo_address = TPM_XDATA_FIFO(0);
+			priv->max_transfer_size = 32;
+			break;
+		case 3:
+			priv->fifo_address = TPM_XDATA_FIFO(0);
+			priv->max_transfer_size = 64;
+			break;
+	}
 
 	rc = tpm2_probe(chip);
 	if (rc)
diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h
index 357df13..b7df6ee 100644
--- a/drivers/char/tpm/tpm_tis_core.h
+++ b/drivers/char/tpm/tpm_tis_core.h
@@ -67,6 +67,7 @@ enum tis_defaults {
 #define TIS_TIMEOUT_C_MAX	max(TIS_SHORT_TIMEOUT, TPM2_TIMEOUT_C)
 #define TIS_TIMEOUT_D_MAX	max(TIS_SHORT_TIMEOUT, TPM2_TIMEOUT_D)
 
+#define ADD_LOCALITY(r, l)		((r) | ((l) << 12))
 #define TPM_ACCESS(l)			(0x0000 | ((l) << 12))
 #define TPM_INT_ENABLE(l)		(0x0008 | ((l) << 12))
 #define TPM_INT_VECTOR(l)		(0x000C | ((l) << 12))
@@ -75,6 +76,7 @@ enum tis_defaults {
 #define TPM_STS(l)			(0x0018 | ((l) << 12))
 #define TPM_STS3(l)			(0x001b | ((l) << 12))
 #define TPM_DATA_FIFO(l)		(0x0024 | ((l) << 12))
+#define TPM_XDATA_FIFO(l)		(0x0080 | ((l) << 12))
 
 #define TPM_DID_VID(l)			(0x0F00 | ((l) << 12))
 #define TPM_RID(l)			(0x0F04 | ((l) << 12))
@@ -86,6 +88,8 @@ enum tpm_tis_flags {
 struct tpm_tis_data {
 	u16 manufacturer_id;
 	int locality;
+	u8 fifo_address;
+	u8 max_transfer_size;
 	int irq;
 	bool irq_tested;
 	unsigned int flags;
diff --git a/drivers/char/tpm/tpm_tis_spi.c b/drivers/char/tpm/tpm_tis_spi.c
index 424ff2f..c4f511b 100644
--- a/drivers/char/tpm/tpm_tis_spi.c
+++ b/drivers/char/tpm/tpm_tis_spi.c
@@ -67,7 +67,7 @@ static int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u16 len,
 	spi_bus_lock(phy->spi_device->master);
 
 	while (len) {
-		transfer_len = min_t(u16, len, MAX_SPI_FRAMESIZE);
+		transfer_len = min_t(u16, len, data->max_transfer_size);
 
 		phy->iobuf[0] = (in ? 0x80 : 0) | (transfer_len - 1);
 		phy->iobuf[1] = 0xd4;
-- 
2.7.4

Re: [RFC][PATCH 5/9] tpm_tis_core: use XDATA_FIFO for transfers if available

[Jarkko Sakkinen]

On Fri, Dec 08, 2017 at 07:46:54PM +0100, Alexander Steffen wrote:
> The TPM indicates in DataTransferSizeSupport in TPM_INTF_CAPABILITY what
> the maximum supported transfer size is. Transfers larger than four bytes
> are only supported for the XDATA_FIFO, not the legacy DATA_FIFO. Therefore,
> always use the XDATA_FIFO when the TPM indicates support for non-legacy
> transfer sizes for maximum throughput.
> 
> Signed-off-by: Alexander Steffen <Alexander.Steffen@infineon.com>
> ---

The commit message should say something about the introduced new macro.

/Jarkko

[RFC][PATCH 6/9] tpm_tis_spi: fix sending wrong data during wait state handling

[Alexander Steffen]

When the TPM signals wait states, the driver has to repeat the last byte
until the TPM stops signaling wait states and accepts that byte. It should
not send a dummy byte, even though that might work with some TPM
implementations.

Signed-off-by: Alexander Steffen <Alexander.Steffen@infineon.com>
---
 drivers/char/tpm/tpm_tis_spi.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis_spi.c b/drivers/char/tpm/tpm_tis_spi.c
index c4f511b..2758b41 100644
--- a/drivers/char/tpm/tpm_tis_spi.c
+++ b/drivers/char/tpm/tpm_tis_spi.c
@@ -88,9 +88,8 @@ static int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u16 len,
 
 		if ((phy->iobuf[3] & 0x01) == 0) {
 			// handle SPI wait states
-			phy->iobuf[0] = 0;
-
 			for (i = 0; i < TPM_RETRY; i++) {
+				phy->iobuf[0] = addr;
 				spi_xfer.len = 1;
 				spi_message_init(&m);
 				spi_message_add_tail(&spi_xfer, &m);
-- 
2.7.4

Re: [RFC][PATCH 6/9] tpm_tis_spi: fix sending wrong data during wait state handling

[Jarkko Sakkinen]

On Fri, Dec 08, 2017 at 07:46:55PM +0100, Alexander Steffen wrote:
> When the TPM signals wait states, the driver has to repeat the last byte
> until the TPM stops signaling wait states and accepts that byte. It should
> not send a dummy byte, even though that might work with some TPM
> implementations.
> 
> Signed-off-by: Alexander Steffen <Alexander.Steffen@infineon.com>
> ---
>  drivers/char/tpm/tpm_tis_spi.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_tis_spi.c b/drivers/char/tpm/tpm_tis_spi.c
> index c4f511b..2758b41 100644
> --- a/drivers/char/tpm/tpm_tis_spi.c
> +++ b/drivers/char/tpm/tpm_tis_spi.c
> @@ -88,9 +88,8 @@ static int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u16 len,
>  
>  		if ((phy->iobuf[3] & 0x01) == 0) {
>  			// handle SPI wait states
> -			phy->iobuf[0] = 0;
> -
>  			for (i = 0; i < TPM_RETRY; i++) {
> +				phy->iobuf[0] = addr;

Why it has to be moved inside the loop?

/Jarkko

[RFC][PATCH 7/9] tpm_tis_spi: release CS line when wait state handling fails

[Alexander Steffen]

By setting cs_change=1 multiple messages are kept within the same
transaction, i.e. the CS line is not released after the first message. This
is fine during normal transactions, when the last message sets cs_change=0,
so that the CS line is released at the end.

But when transactions cannot be completed, e.g. when the wait state
handling times out, the CS line is not released before leaving the
function, because no message is sent with cs_change=0. This breaks future
SPI transactions, so ensure that the CS line is correcly released in this
error case by sending an empty message.

Signed-off-by: Alexander Steffen <Alexander.Steffen@infineon.com>
---
Note: I expect it to work that way, but have not tested it yet.

 drivers/char/tpm/tpm_tis_spi.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/char/tpm/tpm_tis_spi.c b/drivers/char/tpm/tpm_tis_spi.c
index 2758b41..2cc6aa9 100644
--- a/drivers/char/tpm/tpm_tis_spi.c
+++ b/drivers/char/tpm/tpm_tis_spi.c
@@ -101,6 +101,17 @@ static int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u16 len,
 			}
 
 			if (i == TPM_RETRY) {
+				spi_xfer.tx_buf = NULL;
+				spi_xfer.rx_buf = NULL;
+				spi_xfer.len = 0;
+				spi_xfer.cs_change = 0;
+
+				spi_message_init(&m);
+				spi_message_add_tail(&spi_xfer, &m);
+				ret = spi_sync_locked(phy->spi_device, &m);
+				if (ret < 0)
+					goto exit;
+
 				ret = -ETIMEDOUT;
 				goto exit;
 			}
-- 
2.7.4

Re: [RFC][PATCH 7/9] tpm_tis_spi: release CS line when wait state handling fails

[Jarkko Sakkinen]

On Fri, Dec 08, 2017 at 07:46:56PM +0100, Alexander Steffen wrote:
> By setting cs_change=1 multiple messages are kept within the same
> transaction, i.e. the CS line is not released after the first message. This
> is fine during normal transactions, when the last message sets cs_change=0,
> so that the CS line is released at the end.
> 
> But when transactions cannot be completed, e.g. when the wait state
> handling times out, the CS line is not released before leaving the
> function, because no message is sent with cs_change=0. This breaks future
> SPI transactions, so ensure that the CS line is correcly released in this
> error case by sending an empty message.
> 
> Signed-off-by: Alexander Steffen <Alexander.Steffen@infineon.com>

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

/Jarkko

[RFC][PATCH 8/9] tpm_tis_spi: add delay between wait state retries

[Alexander Steffen]

There do not seem to be any real limits one the amount/duration of wait
states that the TPM is allowed to generate. So at least give the TPM some
time to clean up the situation that caused the wait states instead of
retrying the transfers as fast as possible.

Signed-off-by: Alexander Steffen <Alexander.Steffen@infineon.com>
---
 drivers/char/tpm/tpm_tis_spi.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis_spi.c b/drivers/char/tpm/tpm_tis_spi.c
index 2cc6aa9..e53c9c3 100644
--- a/drivers/char/tpm/tpm_tis_spi.c
+++ b/drivers/char/tpm/tpm_tis_spi.c
@@ -88,7 +88,7 @@ static int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u16 len,
 
 		if ((phy->iobuf[3] & 0x01) == 0) {
 			// handle SPI wait states
-			for (i = 0; i < TPM_RETRY; i++) {
+			for (i = 1; i <= TPM_RETRY; i++) {
 				phy->iobuf[0] = addr;
 				spi_xfer.len = 1;
 				spi_message_init(&m);
@@ -98,9 +98,10 @@ static int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u16 len,
 					goto exit;
 				if (phy->iobuf[0] & 0x01)
 					break;
+				tpm_msleep(i * TPM_TIMEOUT);
 			}
 
-			if (i == TPM_RETRY) {
+			if (i > TPM_RETRY) {
 				spi_xfer.tx_buf = NULL;
 				spi_xfer.rx_buf = NULL;
 				spi_xfer.len = 0;
-- 
2.7.4

Re: [RFC][PATCH 8/9] tpm_tis_spi: add delay between wait state retries

[Mimi Zohar]

Hi Alexander,

On Fri, 2017-12-08 at 19:46 +0100, Alexander Steffen wrote:
> There do not seem to be any real limits one the amount/duration of wait
> states that the TPM is allowed to generate. So at least give the TPM some
> time to clean up the situation that caused the wait states instead of
> retrying the transfers as fast as possible.

Without this patch, the TPM performance on the pi is amazing!  A
thousand extends takes ~6.5 seconds.  Unfortunately, the same thousand
extends with this patch takes > 2+ minutes.  TPM_TIMEOUT (5 msecs) is
a really long time.  Why so long?

Mimi
  

> Signed-off-by: Alexander Steffen <Alexander.Steffen@infineon.com>
> ---
>  drivers/char/tpm/tpm_tis_spi.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_tis_spi.c b/drivers/char/tpm/tpm_tis_spi.c
> index 2cc6aa9..e53c9c3 100644
> --- a/drivers/char/tpm/tpm_tis_spi.c
> +++ b/drivers/char/tpm/tpm_tis_spi.c
> @@ -88,7 +88,7 @@ static int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u16 len,
> 
>  		if ((phy->iobuf[3] & 0x01) == 0) {
>  			// handle SPI wait states
> -			for (i = 0; i < TPM_RETRY; i++) {
> +			for (i = 1; i <= TPM_RETRY; i++) {
>  				phy->iobuf[0] = addr;
>  				spi_xfer.len = 1;
>  				spi_message_init(&m);
> @@ -98,9 +98,10 @@ static int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u16 len,
>  					goto exit;
>  				if (phy->iobuf[0] & 0x01)
>  					break;
> +				tpm_msleep(i * TPM_TIMEOUT);
>  			}
> 
> -			if (i == TPM_RETRY) {
> +			if (i > TPM_RETRY) {
>  				spi_xfer.tx_buf = NULL;
>  				spi_xfer.rx_buf = NULL;
>  				spi_xfer.len = 0;

Re: [RFC][PATCH 8/9] tpm_tis_spi: add delay between wait state retries

[Alexander Steffen]

On 11.01.2018 20:46, Mimi Zohar wrote:
> Hi Alexander,
> 
> On Fri, 2017-12-08 at 19:46 +0100, Alexander Steffen wrote:
>> There do not seem to be any real limits one the amount/duration of wait
>> states that the TPM is allowed to generate. So at least give the TPM some
>> time to clean up the situation that caused the wait states instead of
>> retrying the transfers as fast as possible.
> 
> Without this patch, the TPM performance on the pi is amazing!  A
> thousand extends takes ~6.5 seconds.  Unfortunately, the same thousand
> extends with this patch takes > 2+ minutes.  TPM_TIMEOUT (5 msecs) is
> a really long time.  Why so long?

My problem is the lack of specification for the wait state 
functionality. How many wait states may be signaled by the TPM? For how 
long may the TPM signal wait states? Do you know any more details?

The current implementation is based on the assumption that wait states 
are the exception rather than the rule, so that longer delays are 
acceptable. And without knowing for how long wait states can happen, I 
chose rather longer delays (with this patch the TPM has several seconds 
to clear wait states) to avoid failing on some unknown TPM 
implementation in some special cases.

What would be a better implementation? No delay and simply retry for 
five seconds?

What TPMs are you using for your tests? At least for the TPMs that I 
have available for my tests (with a FIFO size of ~256 bytes) I would not 
expect any wait states for extend commands.

Alexander

> 
> Mimi
>    
> 
>> Signed-off-by: Alexander Steffen <Alexander.Steffen@infineon.com>
>> ---
>>   drivers/char/tpm/tpm_tis_spi.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/char/tpm/tpm_tis_spi.c b/drivers/char/tpm/tpm_tis_spi.c
>> index 2cc6aa9..e53c9c3 100644
>> --- a/drivers/char/tpm/tpm_tis_spi.c
>> +++ b/drivers/char/tpm/tpm_tis_spi.c
>> @@ -88,7 +88,7 @@ static int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u16 len,
>>
>>   		if ((phy->iobuf[3] & 0x01) == 0) {
>>   			// handle SPI wait states
>> -			for (i = 0; i < TPM_RETRY; i++) {
>> +			for (i = 1; i <= TPM_RETRY; i++) {
>>   				phy->iobuf[0] = addr;
>>   				spi_xfer.len = 1;
>>   				spi_message_init(&m);
>> @@ -98,9 +98,10 @@ static int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u16 len,
>>   					goto exit;
>>   				if (phy->iobuf[0] & 0x01)
>>   					break;
>> +				tpm_msleep(i * TPM_TIMEOUT);
>>   			}
>>
>> -			if (i == TPM_RETRY) {
>> +			if (i > TPM_RETRY) {
>>   				spi_xfer.tx_buf = NULL;
>>   				spi_xfer.rx_buf = NULL;
>>   				spi_xfer.len = 0;
> 
>

Re: [RFC][PATCH 8/9] tpm_tis_spi: add delay between wait state retries

[Mimi Zohar]

On Fri, 2018-01-12 at 09:28 +0100, Alexander Steffen wrote:
> On 11.01.2018 20:46, Mimi Zohar wrote:
> > Hi Alexander,
> > 
> > On Fri, 2017-12-08 at 19:46 +0100, Alexander Steffen wrote:
> >> There do not seem to be any real limits one the amount/duration of wait
> >> states that the TPM is allowed to generate. So at least give the TPM some
> >> time to clean up the situation that caused the wait states instead of
> >> retrying the transfers as fast as possible.
> > 
> > Without this patch, the TPM performance on the pi is amazing!  A
> > thousand extends takes ~6.5 seconds.  Unfortunately, the same thousand
> > extends with this patch takes > 2+ minutes.  TPM_TIMEOUT (5 msecs) is
> > a really long time.  Why so long?
> 
> My problem is the lack of specification for the wait state 
> functionality. How many wait states may be signaled by the TPM? For how 
> long may the TPM signal wait states? Do you know any more details?

First, let me apologize for not thanking you for making these changes
in the first place.  With just the burstcount patch, but without your
wait state patches, the pi doesn't boot.  I now have a test
environment and can reproduce the problem.  :)  

With your wait state patches, but without this patch, the time is ~14
seconds per thousand extends, as opposed to the ~6.5 I reported above.
 [The ~6.5 seconds is for already measured files.  Files that have
already been measured are not re-measured, so the TPM is not extended
again.  On other systems, "re-measuring" a 1000 files takes less than
1 sec.  I'm not sure why it is taking so long on the pi.]

Without any sleeps, I'm seeing that when it goes into a wait state, it
mostly loops once, every so often 3 times, but printing the counter
introduces delays and probably skews the results.  To get more
accurate results, would require aggregating the results and only
printing them after N number of extends.  I'll try to get that
information for you next week.

> The current implementation is based on the assumption that wait states 
> are the exception rather than the rule, so that longer delays are 
> acceptable. And without knowing for how long wait states can happen, I 
> chose rather longer delays (with this patch the TPM has several seconds 
> to clear wait states) to avoid failing on some unknown TPM 
> implementation in some special cases.
> 
> What would be a better implementation? No delay and simply retry for 
> five seconds?
  
Perhaps use TPM_POLL_SLEEP (1 msec), which Nayna introduced to sleep
within a max time loop.  Instead of using the number of retries, which
is currently defined as 50, define a max end time.  You could still
increment the timeout, but instead of "i * TPM_TIMEOUT) use "i *
TPM_POLL_SLEEP".  Although usleep_range is a lot better than msleep(),
there is still some overhead.  With tpm_msleep(), we don't have a way
of sleeping for less than 1 msec.  Perhaps the first time, when "i ==
0", try again without sleeping.

> What TPMs are you using for your tests? At least for the TPMs that I 
> have available for my tests (with a FIFO size of ~256 bytes) I would not 
> expect any wait states for extend commands.

I'm also surprised. The TPM on the pi isn't on a real SPI bus.  The
test is a tight loop, which forces N number of extends.  The pi is at
work, but I'm working from home today. :(  I'll have to get this info
next week.

Have you tried booting the pi with the "ima_tcb" boot command line
option?  Do you have any performance results that you could share? 

thanks,

Mimi

Re: [RFC][PATCH 8/9] tpm_tis_spi: add delay between wait state retries

[Mimi Zohar]

> What would be a better implementation? No delay and simply retry for 
> five seconds?
> 
> What TPMs are you using for your tests? At least for the TPMs that I 
> have available for my tests (with a FIFO size of ~256 bytes) I would not 
> expect any wait states for extend commands.

The TPM burstcount is 32.  Unfortunately, with this patch set the
performance on the pi is worse than without it.  Without this patch
set we're seeing ~14 secs for a thousand measurements vs. either ~38s
or ~23s, depending on the wait sleep length introduced in patch 8/9.
 This testing was done on a pi with an emulated SPI bus.

On systems with a large burstcount, there is no difference, but on
systems with a small burstcount, we're seeing an improvement of ~39%
(~14s base time, ~8.5 with patches).  

Does anyone have a system with a TPM on a real SPI bus and is willing
to test this patch set?

thanks,

Mimi

Re: [RFC][PATCH 8/9] tpm_tis_spi: add delay between wait state retries

[Mimi Zohar]

On Mon, 2018-01-15 at 17:30 -0500, Mimi Zohar wrote:
> > What would be a better implementation? No delay and simply retry for 
> > five seconds?
> > 
> > What TPMs are you using for your tests? At least for the TPMs that I 
> > have available for my tests (with a FIFO size of ~256 bytes) I would not 
> > expect any wait states for extend commands.
> 
> The TPM burstcount is 32.  Unfortunately, with this patch set the
> performance on the pi is worse than without it.  Without this patch
> set we're seeing ~14 secs for a thousand measurements vs. either ~38s
> or ~23s, depending on the wait sleep length introduced in patch 8/9.

>From my response, it might not have been clear that with all of the
patches, except this one 8/9 calling tpm_msleep(), the performance is
equivalent to without any of the patches.  With this patch set, but
with or without the call to tpm_msleep(), it loops 2 or 3 times.

Mimi

Re: [RFC][PATCH 8/9] tpm_tis_spi: add delay between wait state retries

[Alexander Steffen]

On 17.01.2018 18:15, Mimi Zohar wrote:
> On Mon, 2018-01-15 at 17:30 -0500, Mimi Zohar wrote:
>>> What would be a better implementation? No delay and simply retry for
>>> five seconds?
>>>
>>> What TPMs are you using for your tests? At least for the TPMs that I
>>> have available for my tests (with a FIFO size of ~256 bytes) I would not
>>> expect any wait states for extend commands.
>>
>> The TPM burstcount is 32.  Unfortunately, with this patch set the
>> performance on the pi is worse than without it.  Without this patch
>> set we're seeing ~14 secs for a thousand measurements vs. either ~38s
>> or ~23s, depending on the wait sleep length introduced in patch 8/9.
> 
>  From my response, it might not have been clear that with all of the
> patches, except this one 8/9 calling tpm_msleep(), the performance is
> equivalent to without any of the patches.  With this patch set, but
> with or without the call to tpm_msleep(), it loops 2 or 3 times.

Thanks for your tests. I haven't yet done such extensive performance 
tests for those changes. But I've got some performance tests that I 
still want to run to see what performance impact I can measure with my TPMs.

It seems clear to me that for optimum performance there shouldn't be any 
sleeps, at least not for the first few retries. If I use a time limit 
instead of counting the number of retries, are sleeps necessary at all? 
The SPI bus is locked anyway, so nothing else can use it while we are 
sleeping. The SPI transfers should slow us down sufficiently, so that we 
are not consuming 100% of CPU time. And in the common cases (at least 
those that we know) the wait states seem to clear up pretty quickly.

If there are no objections, I'll rewrite the patch in such a way (time 
limit of several seconds, no sleeps). Maybe it is also worth trying to 
see what performance can be gained by removing more sleeps, in 
wait_for_tpm_stat and elsewhere. Perhaps the first millisecond(s) or so 
should always be busy waiting, and sleeps should only come afterwards.

Alexander

Re: [RFC][PATCH 8/9] tpm_tis_spi: add delay between wait state retries

[Jarkko Sakkinen]

On Fri, Dec 08, 2017 at 07:46:57PM +0100, Alexander Steffen wrote:
> There do not seem to be any real limits one the amount/duration of wait
> states that the TPM is allowed to generate. So at least give the TPM some
> time to clean up the situation that caused the wait states instead of
> retrying the transfers as fast as possible.
> 
> Signed-off-by: Alexander Steffen <Alexander.Steffen@infineon.com>

I agreee with Mimis comment that this change should be based on
time rather than retries.

/Jarkko

[RFC][PATCH 9/9] tpm: ignore burstcount to improve tpm_tis send() performance

[Alexander Steffen]

From: Nayna Jain <nayna@linux.vnet.ibm.com>

The TPM burstcount status indicates the number of bytes that can
be sent to the TPM without causing bus wait states.  Effectively,
it is the number of empty bytes in the command FIFO.

This patch optimizes the tpm_tis_send_data() function by checking
the burstcount only once. And if the burstcount is valid, it writes
all the bytes at once, permitting wait state.

After this change, performance on a TPM 1.2 with an 8 byte
burstcount for 1000 extends improved from ~41sec to ~14sec.

Suggested-by: Ken Goldman <kgold@linux.vnet.ibm.com> in
conjunction with the TPM Device Driver work group.
Signed-off-by: Nayna Jain <nayna@linux.vnet.ibm.com>
Acked-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 drivers/char/tpm/tpm_tis_core.c | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 7240ee4..7979b06 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -320,7 +320,6 @@ static int tpm_tis_send_data(struct tpm_chip *chip, const u8 *buf, size_t len)
 {
 	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
 	int rc, status, burstcnt;
-	size_t count = 0;
 
 	status = tpm_tis_status(chip);
 	if ((status & TPM_STS_COMMAND_READY) == 0) {
@@ -333,23 +332,23 @@ static int tpm_tis_send_data(struct tpm_chip *chip, const u8 *buf, size_t len)
 		}
 	}
 
-	while (count < len) {
-		burstcnt = get_burstcount(chip);
-		if (burstcnt < 0) {
-			dev_err(&chip->dev, "Unable to read burstcount\n");
-			rc = burstcnt;
-			goto out_err;
-		}
-		burstcnt = min_t(int, burstcnt, len - count);
-		rc = tpm_tis_write_bytes(priv, ADD_LOCALITY(priv->fifo_address,
-							    priv->locality),
-					 burstcnt, buf + count);
-		if (rc < 0)
-			goto out_err;
-
-		count += burstcnt;
+	/*
+	 * Get the initial burstcount to ensure TPM is ready to
+	 * accept data.
+	 */
+	burstcnt = get_burstcount(chip);
+	if (burstcnt < 0) {
+		dev_err(&chip->dev, "Unable to read burstcount\n");
+		rc = burstcnt;
+		goto out_err;
 	}
 
+	rc = tpm_tis_write_bytes(priv, ADD_LOCALITY(priv->fifo_address,
+						    priv->locality),
+				 len, buf);
+	if (rc < 0)
+		goto out_err;
+
 	if (!(priv->flags & TPM_TIS_ITPM_WORKAROUND) &&
 	    wait_for_tpm_stat(chip, TPM_STS_DATA_EXPECT | TPM_STS_VALID,
 			      TPM_STS_VALID, chip->timeout_c, &priv->int_queue,
-- 
2.7.4

Re: [RFC][PATCH 0/9] tpm: fix driver so that burstcount can be safely ignored

[Jarkko Sakkinen]

On Fri, Dec 08, 2017 at 07:46:49PM +0100, Alexander Steffen wrote:
> Disclaimer: This is RFC because unfortunately I do not have the time to bring it up to the usual standards. But from lying around on my hard disk it definitely won't improve, so I've decided to publish it now, to get at least some feedback or maybe for somebody else to pick it up. Since the work began some time ago, it is probably not rebased against the current state (but hopefully still applies).
> 
> This is a collection of all the fixes I made during the investigation of the problems with the "ignore burstcount" change. These are mainly fixes for the wait state handling in tpm_tis_spi, that probably was not really tested so far, since the previous implementation avoided wait states as far as possible. It also includes changes to tpm_tis_core, so that it follows the specification more closely. Finally, I've included a rebased version of the original "ignore burstcount" patch. Together, those patches pass all my tests and also each intermediate step is fine (so that bisecting still works).
> 
> Alexander Steffen (8):
>   tpm_tis_core: clean up whitespace
>   tpm_tis_core: access single TIS registers before doing complex
>     transfers
>   tpm_tis_core: correctly wait for flags to become zero
>   tpm_tis_core: send all data in single operation
>   tpm_tis_core: use XDATA_FIFO for transfers if available
>   tpm_tis_spi: fix sending wrong data during wait state handling
>   tpm_tis_spi: release CS line when wait state handling fails
>   tpm_tis_spi: add delay between wait state retries
> 
> Nayna Jain (1):
>   tpm: ignore burstcount to improve tpm_tis send() performance
> 
>  drivers/char/tpm/tpm_tis_core.c | 131 ++++++++++++++++++++--------------------
>  drivers/char/tpm/tpm_tis_core.h |  26 ++++----
>  drivers/char/tpm/tpm_tis_spi.c  |  21 +++++--
>  3 files changed, 95 insertions(+), 83 deletions(-)
> 
> -- 
> 2.7.4
> 

Might not have time to review this before holidays but will do if
I can.

/Jarkko

Re: [RFC][PATCH 0/9] tpm: fix driver so that burstcount can be safely ignored

[Jarkko Sakkinen]

On Fri, Dec 15, 2017 at 02:04:39PM +0200, Jarkko Sakkinen wrote:
> On Fri, Dec 08, 2017 at 07:46:49PM +0100, Alexander Steffen wrote:
> > Disclaimer: This is RFC because unfortunately I do not have the time to bring it up to the usual standards. But from lying around on my hard disk it definitely won't improve, so I've decided to publish it now, to get at least some feedback or maybe for somebody else to pick it up. Since the work began some time ago, it is probably not rebased against the current state (but hopefully still applies).
> > 
> > This is a collection of all the fixes I made during the investigation of the problems with the "ignore burstcount" change. These are mainly fixes for the wait state handling in tpm_tis_spi, that probably was not really tested so far, since the previous implementation avoided wait states as far as possible. It also includes changes to tpm_tis_core, so that it follows the specification more closely. Finally, I've included a rebased version of the original "ignore burstcount" patch. Together, those patches pass all my tests and also each intermediate step is fine (so that bisecting still works).
> > 
> > Alexander Steffen (8):
> >   tpm_tis_core: clean up whitespace
> >   tpm_tis_core: access single TIS registers before doing complex
> >     transfers
> >   tpm_tis_core: correctly wait for flags to become zero
> >   tpm_tis_core: send all data in single operation
> >   tpm_tis_core: use XDATA_FIFO for transfers if available
> >   tpm_tis_spi: fix sending wrong data during wait state handling
> >   tpm_tis_spi: release CS line when wait state handling fails
> >   tpm_tis_spi: add delay between wait state retries
> > 
> > Nayna Jain (1):
> >   tpm: ignore burstcount to improve tpm_tis send() performance
> > 
> >  drivers/char/tpm/tpm_tis_core.c | 131 ++++++++++++++++++++--------------------
> >  drivers/char/tpm/tpm_tis_core.h |  26 ++++----
> >  drivers/char/tpm/tpm_tis_spi.c  |  21 +++++--
> >  3 files changed, 95 insertions(+), 83 deletions(-)
> > 
> > -- 
> > 2.7.4
> > 
> 
> Might not have time to review this before holidays but will do if
> I can.

Unfortunately this and your patches for broken TPMs came so late that
they have no chance to make into 4.16. Thus, I will postpone their
review after the pull request and take care of the high priority stuff
for that release.

/Jarkko

RE: [RFC][PATCH 0/9] tpm: fix driver so that burstcount can be safely ignored

[Alexander.Steffen]

> On Fri, Dec 15, 2017 at 02:04:39PM +0200, Jarkko Sakkinen wrote:
> > On Fri, Dec 08, 2017 at 07:46:49PM +0100, Alexander Steffen wrote:
> > > Disclaimer: This is RFC because unfortunately I do not have the time to
> bring it up to the usual standards. But from lying around on my hard disk it
> definitely won't improve, so I've decided to publish it now, to get at least
> some feedback or maybe for somebody else to pick it up. Since the work
> began some time ago, it is probably not rebased against the current state
> (but hopefully still applies).
> > >
> > > This is a collection of all the fixes I made during the investigation of the
> problems with the "ignore burstcount" change. These are mainly fixes for the
> wait state handling in tpm_tis_spi, that probably was not really tested so far,
> since the previous implementation avoided wait states as far as possible. It
> also includes changes to tpm_tis_core, so that it follows the specification
> more closely. Finally, I've included a rebased version of the original "ignore
> burstcount" patch. Together, those patches pass all my tests and also each
> intermediate step is fine (so that bisecting still works).
> > >
> > > Alexander Steffen (8):
> > >   tpm_tis_core: clean up whitespace
> > >   tpm_tis_core: access single TIS registers before doing complex
> > >     transfers
> > >   tpm_tis_core: correctly wait for flags to become zero
> > >   tpm_tis_core: send all data in single operation
> > >   tpm_tis_core: use XDATA_FIFO for transfers if available
> > >   tpm_tis_spi: fix sending wrong data during wait state handling
> > >   tpm_tis_spi: release CS line when wait state handling fails
> > >   tpm_tis_spi: add delay between wait state retries
> > >
> > > Nayna Jain (1):
> > >   tpm: ignore burstcount to improve tpm_tis send() performance
> > >
> > >  drivers/char/tpm/tpm_tis_core.c | 131 ++++++++++++++++++++---------
> -----------
> > >  drivers/char/tpm/tpm_tis_core.h |  26 ++++----
> > >  drivers/char/tpm/tpm_tis_spi.c  |  21 +++++--
> > >  3 files changed, 95 insertions(+), 83 deletions(-)
> > >
> > > --
> > > 2.7.4
> > >
> >
> > Might not have time to review this before holidays but will do if
> > I can.
> 
> Unfortunately this and your patches for broken TPMs came so late that
> they have no chance to make into 4.16. Thus, I will postpone their
> review after the pull request and take care of the high priority stuff
> for that release.

As long the "ignore burstcount" patch does not make it into 4.16 either, that's fine. Then nothing new gets broken, it just does not get any faster.

Alexander

Re: [RFC][PATCH 0/9] tpm: fix driver so that burstcount can be safely ignored

[Nayna Jain]

On 01/04/2018 06:41 PM, Alexander.Steffen@infineon.com wrote:
>> On Fri, Dec 15, 2017 at 02:04:39PM +0200, Jarkko Sakkinen wrote:
>>> On Fri, Dec 08, 2017 at 07:46:49PM +0100, Alexander Steffen wrote:
>>>> Disclaimer: This is RFC because unfortunately I do not have the time to
>> bring it up to the usual standards. But from lying around on my hard disk it
>> definitely won't improve, so I've decided to publish it now, to get at least
>> some feedback or maybe for somebody else to pick it up. Since the work
>> began some time ago, it is probably not rebased against the current state
>> (but hopefully still applies).
>>>> This is a collection of all the fixes I made during the investigation of the
>> problems with the "ignore burstcount" change. These are mainly fixes for the
>> wait state handling in tpm_tis_spi, that probably was not really tested so far,
>> since the previous implementation avoided wait states as far as possible. It
>> also includes changes to tpm_tis_core, so that it follows the specification
>> more closely. Finally, I've included a rebased version of the original "ignore
>> burstcount" patch. Together, those patches pass all my tests and also each
>> intermediate step is fine (so that bisecting still works).
>>>> Alexander Steffen (8):
>>>>    tpm_tis_core: clean up whitespace
>>>>    tpm_tis_core: access single TIS registers before doing complex
>>>>      transfers
>>>>    tpm_tis_core: correctly wait for flags to become zero
>>>>    tpm_tis_core: send all data in single operation
>>>>    tpm_tis_core: use XDATA_FIFO for transfers if available
>>>>    tpm_tis_spi: fix sending wrong data during wait state handling
>>>>    tpm_tis_spi: release CS line when wait state handling fails
>>>>    tpm_tis_spi: add delay between wait state retries
>>>>
>>>> Nayna Jain (1):
>>>>    tpm: ignore burstcount to improve tpm_tis send() performance
>>>>
>>>>   drivers/char/tpm/tpm_tis_core.c | 131 ++++++++++++++++++++---------
>> -----------
>>>>   drivers/char/tpm/tpm_tis_core.h |  26 ++++----
>>>>   drivers/char/tpm/tpm_tis_spi.c  |  21 +++++--
>>>>   3 files changed, 95 insertions(+), 83 deletions(-)
>>>>
>>>> --
>>>> 2.7.4
>>>>
>>> Might not have time to review this before holidays but will do if
>>> I can.
>> Unfortunately this and your patches for broken TPMs came so late that
>> they have no chance to make into 4.16. Thus, I will postpone their
>> review after the pull request and take care of the high priority stuff
>> for that release.
> As long the "ignore burstcount" patch does not make it into 4.16 either, that's fine. Then nothing new gets broken, it just does not get any faster.

Alex, if I am right, among following 4 patches,
  
   Patch 1 - tpm: move wait_for_tpm_stat() to respective driver files
   Patch 2 - tpm: ignore burstcount to improve tpm_tis send() performance
   Patch 3 - tpm: reduce tpm polling delay in tpm_tis_core
   Patch 4 - tpm: use tpm_msleep() value as max delay

it is only the burstcount patch (Patch 2) which caused the issue.
With that, Jarkko, Alex, can we just exclude this and have other three patches pulled for 4.16 ?
I tested with these other three patches, and performance improvement is from ~41 to ~14.

Thanks & Regards,
    - Nayna

> Alexander
>

Re: [RFC][PATCH 0/9] tpm: fix driver so that burstcount can be safely ignored

[Alexander Steffen]

On 05.01.2018 07:46, Nayna Jain wrote:
> On 01/04/2018 06:41 PM, Alexander.Steffen@infineon.com wrote:
>>> On Fri, Dec 15, 2017 at 02:04:39PM +0200, Jarkko Sakkinen wrote:
>>>> On Fri, Dec 08, 2017 at 07:46:49PM +0100, Alexander Steffen wrote:
>>>>> Disclaimer: This is RFC because unfortunately I do not have the 
>>>>> time to
>>> bring it up to the usual standards. But from lying around on my hard 
>>> disk it
>>> definitely won't improve, so I've decided to publish it now, to get 
>>> at least
>>> some feedback or maybe for somebody else to pick it up. Since the work
>>> began some time ago, it is probably not rebased against the current 
>>> state
>>> (but hopefully still applies).
>>>>> This is a collection of all the fixes I made during the 
>>>>> investigation of the
>>> problems with the "ignore burstcount" change. These are mainly fixes 
>>> for the
>>> wait state handling in tpm_tis_spi, that probably was not really 
>>> tested so far,
>>> since the previous implementation avoided wait states as far as 
>>> possible. It
>>> also includes changes to tpm_tis_core, so that it follows the 
>>> specification
>>> more closely. Finally, I've included a rebased version of the 
>>> original "ignore
>>> burstcount" patch. Together, those patches pass all my tests and also 
>>> each
>>> intermediate step is fine (so that bisecting still works).
>>>>> Alexander Steffen (8):
>>>>>    tpm_tis_core: clean up whitespace
>>>>>    tpm_tis_core: access single TIS registers before doing complex
>>>>>      transfers
>>>>>    tpm_tis_core: correctly wait for flags to become zero
>>>>>    tpm_tis_core: send all data in single operation
>>>>>    tpm_tis_core: use XDATA_FIFO for transfers if available
>>>>>    tpm_tis_spi: fix sending wrong data during wait state handling
>>>>>    tpm_tis_spi: release CS line when wait state handling fails
>>>>>    tpm_tis_spi: add delay between wait state retries
>>>>>
>>>>> Nayna Jain (1):
>>>>>    tpm: ignore burstcount to improve tpm_tis send() performance
>>>>>
>>>>>   drivers/char/tpm/tpm_tis_core.c | 131 ++++++++++++++++++++---------
>>> -----------
>>>>>   drivers/char/tpm/tpm_tis_core.h |  26 ++++----
>>>>>   drivers/char/tpm/tpm_tis_spi.c  |  21 +++++--
>>>>>   3 files changed, 95 insertions(+), 83 deletions(-)
>>>>>
>>>>> -- 
>>>>> 2.7.4
>>>>>
>>>> Might not have time to review this before holidays but will do if
>>>> I can.
>>> Unfortunately this and your patches for broken TPMs came so late that
>>> they have no chance to make into 4.16. Thus, I will postpone their
>>> review after the pull request and take care of the high priority stuff
>>> for that release.
>> As long the "ignore burstcount" patch does not make it into 4.16 
>> either, that's fine. Then nothing new gets broken, it just does not 
>> get any faster.
> 
> Alex, if I am right, among following 4 patches,
> 
>    Patch 1 - tpm: move wait_for_tpm_stat() to respective driver files
>    Patch 2 - tpm: ignore burstcount to improve tpm_tis send() performance
>    Patch 3 - tpm: reduce tpm polling delay in tpm_tis_core
>    Patch 4 - tpm: use tpm_msleep() value as max delay
> 
> it is only the burstcount patch (Patch 2) which caused the issue.
> With that, Jarkko, Alex, can we just exclude this and have other three 
> patches pulled for 4.16 ?

Those other three patches do not cause any issues in my tests, so I'm 
fine with that.

Alexander

> I tested with these other three patches, and performance improvement is 
> from ~41 to ~14.
> 
> Thanks & Regards,
>     - Nayna
> 
>> Alexander
>>

Re: [RFC][PATCH 0/9] tpm: fix driver so that burstcount can be safely ignored

[Jarkko Sakkinen]

On Fri, Jan 05, 2018 at 08:38:11AM +0100, Alexander Steffen wrote:
> On 05.01.2018 07:46, Nayna Jain wrote:
> > On 01/04/2018 06:41 PM, Alexander.Steffen@infineon.com wrote:
> > > > On Fri, Dec 15, 2017 at 02:04:39PM +0200, Jarkko Sakkinen wrote:
> > > > > On Fri, Dec 08, 2017 at 07:46:49PM +0100, Alexander Steffen wrote:
> > > > > > Disclaimer: This is RFC because unfortunately I do not
> > > > > > have the time to
> > > > bring it up to the usual standards. But from lying around on my
> > > > hard disk it
> > > > definitely won't improve, so I've decided to publish it now, to
> > > > get at least
> > > > some feedback or maybe for somebody else to pick it up. Since the work
> > > > began some time ago, it is probably not rebased against the
> > > > current state
> > > > (but hopefully still applies).
> > > > > > This is a collection of all the fixes I made during the
> > > > > > investigation of the
> > > > problems with the "ignore burstcount" change. These are mainly
> > > > fixes for the
> > > > wait state handling in tpm_tis_spi, that probably was not really
> > > > tested so far,
> > > > since the previous implementation avoided wait states as far as
> > > > possible. It
> > > > also includes changes to tpm_tis_core, so that it follows the
> > > > specification
> > > > more closely. Finally, I've included a rebased version of the
> > > > original "ignore
> > > > burstcount" patch. Together, those patches pass all my tests and
> > > > also each
> > > > intermediate step is fine (so that bisecting still works).
> > > > > > Alexander Steffen (8):
> > > > > >    tpm_tis_core: clean up whitespace
> > > > > >    tpm_tis_core: access single TIS registers before doing complex
> > > > > >      transfers
> > > > > >    tpm_tis_core: correctly wait for flags to become zero
> > > > > >    tpm_tis_core: send all data in single operation
> > > > > >    tpm_tis_core: use XDATA_FIFO for transfers if available
> > > > > >    tpm_tis_spi: fix sending wrong data during wait state handling
> > > > > >    tpm_tis_spi: release CS line when wait state handling fails
> > > > > >    tpm_tis_spi: add delay between wait state retries
> > > > > > 
> > > > > > Nayna Jain (1):
> > > > > >    tpm: ignore burstcount to improve tpm_tis send() performance
> > > > > > 
> > > > > >   drivers/char/tpm/tpm_tis_core.c | 131 ++++++++++++++++++++---------
> > > > -----------
> > > > > >   drivers/char/tpm/tpm_tis_core.h |  26 ++++----
> > > > > >   drivers/char/tpm/tpm_tis_spi.c  |  21 +++++--
> > > > > >   3 files changed, 95 insertions(+), 83 deletions(-)
> > > > > > 
> > > > > > -- 
> > > > > > 2.7.4
> > > > > > 
> > > > > Might not have time to review this before holidays but will do if
> > > > > I can.
> > > > Unfortunately this and your patches for broken TPMs came so late that
> > > > they have no chance to make into 4.16. Thus, I will postpone their
> > > > review after the pull request and take care of the high priority stuff
> > > > for that release.
> > > As long the "ignore burstcount" patch does not make it into 4.16
> > > either, that's fine. Then nothing new gets broken, it just does not
> > > get any faster.
> > 
> > Alex, if I am right, among following 4 patches,
> > 
> >    Patch 1 - tpm: move wait_for_tpm_stat() to respective driver files
> >    Patch 2 - tpm: ignore burstcount to improve tpm_tis send() performance
> >    Patch 3 - tpm: reduce tpm polling delay in tpm_tis_core
> >    Patch 4 - tpm: use tpm_msleep() value as max delay
> > 
> > it is only the burstcount patch (Patch 2) which caused the issue.
> > With that, Jarkko, Alex, can we just exclude this and have other three
> > patches pulled for 4.16 ?
> 
> Those other three patches do not cause any issues in my tests, so I'm fine
> with that.

Yes.

/Jarkko

Re: [RFC][PATCH 0/9] tpm: fix driver so that burstcount can be safely ignored

[Jarkko Sakkinen]

On Thu, Jan 04, 2018 at 01:11:34PM +0000, Alexander.Steffen@infineon.com wrote:
> > On Fri, Dec 15, 2017 at 02:04:39PM +0200, Jarkko Sakkinen wrote:
> > > On Fri, Dec 08, 2017 at 07:46:49PM +0100, Alexander Steffen wrote:
> > > > Disclaimer: This is RFC because unfortunately I do not have the time to
> > bring it up to the usual standards. But from lying around on my hard disk it
> > definitely won't improve, so I've decided to publish it now, to get at least
> > some feedback or maybe for somebody else to pick it up. Since the work
> > began some time ago, it is probably not rebased against the current state
> > (but hopefully still applies).
> > > >
> > > > This is a collection of all the fixes I made during the investigation of the
> > problems with the "ignore burstcount" change. These are mainly fixes for the
> > wait state handling in tpm_tis_spi, that probably was not really tested so far,
> > since the previous implementation avoided wait states as far as possible. It
> > also includes changes to tpm_tis_core, so that it follows the specification
> > more closely. Finally, I've included a rebased version of the original "ignore
> > burstcount" patch. Together, those patches pass all my tests and also each
> > intermediate step is fine (so that bisecting still works).
> > > >
> > > > Alexander Steffen (8):
> > > >   tpm_tis_core: clean up whitespace
> > > >   tpm_tis_core: access single TIS registers before doing complex
> > > >     transfers
> > > >   tpm_tis_core: correctly wait for flags to become zero
> > > >   tpm_tis_core: send all data in single operation
> > > >   tpm_tis_core: use XDATA_FIFO for transfers if available
> > > >   tpm_tis_spi: fix sending wrong data during wait state handling
> > > >   tpm_tis_spi: release CS line when wait state handling fails
> > > >   tpm_tis_spi: add delay between wait state retries
> > > >
> > > > Nayna Jain (1):
> > > >   tpm: ignore burstcount to improve tpm_tis send() performance
> > > >
> > > >  drivers/char/tpm/tpm_tis_core.c | 131 ++++++++++++++++++++---------
> > -----------
> > > >  drivers/char/tpm/tpm_tis_core.h |  26 ++++----
> > > >  drivers/char/tpm/tpm_tis_spi.c  |  21 +++++--
> > > >  3 files changed, 95 insertions(+), 83 deletions(-)
> > > >
> > > > --
> > > > 2.7.4
> > > >
> > >
> > > Might not have time to review this before holidays but will do if
> > > I can.
> > 
> > Unfortunately this and your patches for broken TPMs came so late that
> > they have no chance to make into 4.16. Thus, I will postpone their
> > review after the pull request and take care of the high priority stuff
> > for that release.
> 
> As long the "ignore burstcount" patch does not make it into 4.16 either, that's fine. Then nothing new gets broken, it just does not get any faster.

OK, I guess it has to be dropped from 4.16 and postponed to 4.17.

Thanks for pointing that out.

/Jarkko

Re: [RFC][PATCH 0/9] tpm: fix driver so that burstcount can be safely ignored

[Nayna Jain]

On 12/09/2017 12:16 AM, Alexander Steffen wrote:
> Disclaimer: This is RFC because unfortunately I do not have the time to bring it up to the usual standards. But from lying around on my hard disk it definitely won't improve, so I've decided to publish it now, to get at least some feedback or maybe for somebody else to pick it up. Since the work began some time ago, it is probably not rebased against the current state (but hopefully still applies).
>
> This is a collection of all the fixes I made during the investigation of the problems with the "ignore burstcount" change. These are mainly fixes for the wait state handling in tpm_tis_spi, that probably was not really tested so far, since the previous implementation avoided wait states as far as possible. It also includes changes to tpm_tis_core, so that it follows the specification more closely. Finally, I've included a rebased version of the original "ignore burstcount" patch. Together, those patches pass all my tests and also each intermediate step is fine (so that bisecting still works).

Thanks Alex for debugging and fixing. Sorry, I haven't been able to look 
yet because was mostly on vacation most of the time in last few weeks 
and would be for few more days in next two weeks.

Thanks & Regards,
    - Nayna

>
> Alexander Steffen (8):
>    tpm_tis_core: clean up whitespace
>    tpm_tis_core: access single TIS registers before doing complex
>      transfers
>    tpm_tis_core: correctly wait for flags to become zero
>    tpm_tis_core: send all data in single operation
>    tpm_tis_core: use XDATA_FIFO for transfers if available
>    tpm_tis_spi: fix sending wrong data during wait state handling
>    tpm_tis_spi: release CS line when wait state handling fails
>    tpm_tis_spi: add delay between wait state retries
>
> Nayna Jain (1):
>    tpm: ignore burstcount to improve tpm_tis send() performance
>
>   drivers/char/tpm/tpm_tis_core.c | 131 ++++++++++++++++++++--------------------
>   drivers/char/tpm/tpm_tis_core.h |  26 ++++----
>   drivers/char/tpm/tpm_tis_spi.c  |  21 +++++--
>   3 files changed, 95 insertions(+), 83 deletions(-)
>