[v2,3/6] tpm_tis_spi: add max xfer size

Commit Message

Stephen Boyd July 16, 2019, 10:45 p.m. UTC

From: Andrey Pronin <[e-mail]>

Reject burstcounts larger than 64 bytes reported by tpm.
SPI Hardware Protocol defined in section 6.4 of TCG PTP
Spec supports up to 64 bytes of data in a transaction.

Signed-off-by: Andrey Pronin <[e-mail]>
Reviewed-by: Dmitry Torokhov <[e-mail]>
Reviewed-by: Jarkko Sakkinen <[e-mail]>
Signed-off-by: Stephen Boyd <[e-mail]>
---
 drivers/char/tpm/tpm_tis_spi.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Alexander Steffen July 17, 2019, 8:07 a.m. UTC | #1

On 17.07.2019 00:45, Stephen Boyd wrote:
> From: Andrey Pronin <[e-mail]>
> 
> Reject burstcounts larger than 64 bytes reported by tpm.

This is not the correct thing to do here. To quote the specification:

"burstCount is defined as the number of bytes that can be written to or 
read from the data FIFO by the software without incurring a wait state."
(https://trustedcomputinggroup.org/wp-content/uploads/TCG_PC_Client_Platform_TPM_Profile_PTP_2.0_r1.03_v22.pdf 
Page 84)

If the FIFO contains 1k of data, it is completely valid for the TPM to 
report that as its burstCount, there is no need to arbitrarily limit it.

Also, burstCount is a property of the high-level TIS protocol, that 
should not really care whether the low-level transfers are done via LPC 
or SPI (or I2C). Since tpm_tis_spi can only transfer 64 bytes at a time, 
it is its job to split larger transfers (which it does perfectly fine). 
This also has the advantage that burstCount needs only to be read once, 
and then we can do 16 SPI transfers in a row to read that 1k of data. 
With your change, it will read 64 bytes, then read burstCount again, 
before reading the next 64 bytes and so on. This unnecessarily limits 
performance.

Maybe you can describe the problem you're trying to solve in more 
detail, so that a better solution can be found, since this is clearly 
something not intended by the spec.

Alexander

Stephen Boyd July 17, 2019, 6:19 p.m. UTC | #2

Quoting Alexander Steffen (2019-07-17 01:07:11)
> On 17.07.2019 00:45, Stephen Boyd wrote:
> > From: Andrey Pronin <[e-mail]>
> > 
> > Reject burstcounts larger than 64 bytes reported by tpm.
> 
> This is not the correct thing to do here. To quote the specification:
> 
> "burstCount is defined as the number of bytes that can be written to or 
> read from the data FIFO by the software without incurring a wait state."
> (https://trustedcomputinggroup.org/wp-content/uploads/TCG_PC_Client_Platform_TPM_Profile_PTP_2.0_r1.03_v22.pdf 
> Page 84)

Thanks for pointing this out. I think we were using this SPI driver for
cr50 but then we wrote our own version of this driver with the
differences required to make cr50 work properly. I suspect we can drop
this patch, but we've been carrying it forward for a while now, so I'll
have to check with Andrey and others to make sure it's safe to remove.

> 
> If the FIFO contains 1k of data, it is completely valid for the TPM to 
> report that as its burstCount, there is no need to arbitrarily limit it.
> 
> Also, burstCount is a property of the high-level TIS protocol, that 
> should not really care whether the low-level transfers are done via LPC 
> or SPI (or I2C). Since tpm_tis_spi can only transfer 64 bytes at a time, 
> it is its job to split larger transfers (which it does perfectly fine). 
> This also has the advantage that burstCount needs only to be read once, 
> and then we can do 16 SPI transfers in a row to read that 1k of data. 
> With your change, it will read 64 bytes, then read burstCount again, 
> before reading the next 64 bytes and so on. This unnecessarily limits 
> performance.
> 
> Maybe you can describe the problem you're trying to solve in more 
> detail, so that a better solution can be found, since this is clearly 
> something not intended by the spec.

Right. The burst count we read from cr50 is never going to be larger
than max_xfer_size that we specify in the cr50 driver here, so this is
probably all useless and we can even drop the patch before this one that
adds support for this burst count capping feature.

Patch

diff --git a/drivers/char/tpm/tpm_tis_spi.c b/drivers/char/tpm/tpm_tis_spi.c
index 19513e622053..27393275a3f0 100644
--- a/drivers/char/tpm/tpm_tis_spi.c
+++ b/drivers/char/tpm/tpm_tis_spi.c
@@ -190,6 +190,7 @@  static const struct tpm_tis_phy_ops tpm_spi_phy_ops = {
 	.read16 = tpm_tis_spi_read16,
 	.read32 = tpm_tis_spi_read32,
 	.write32 = tpm_tis_spi_write32,
+	.max_xfer_size = MAX_SPI_FRAMESIZE,
 };
 
 static int tpm_tis_spi_probe(struct spi_device *dev)