Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1582684 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH 5/5] tpm_tis_spi: Add small delay after last transfer Peter Huewe <peter.huewe@infineon.com> - 2017-02-16 17:20 +0100
    Re: [PATCH 5/5] tpm_tis_spi: Add small delay after last transfer Christophe Ricard <christophe.ricard@gmail.com> - 2017-02-17 06:20 +0100
      Re: [PATCH 5/5] tpm_tis_spi: Add small delay after last transfer Peter Huewe <peterhuewe@gmx.de> - 2017-02-17 08:30 +0100
    Re: [PATCH 5/5] tpm_tis_spi: Add small delay after last transfer Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2017-02-24 14:10 +0100

#1582684 — [PATCH 5/5] tpm_tis_spi: Add small delay after last transfer

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@vger.kernel.org>
Fixes: 0edbfea537d1 ("tpm/tpm_tis_spi: Add support for spi phy")
Signed-off-by: Alexander Steffen <Alexander.Steffen@infineon.com>
Signed-off-by: Peter Huewe <peter.huewe@infineon.com>
---
 drivers/char/tpm/tpm_tis_spi.c | 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 b50c5b072df3..685c51bf5d7e 100644
--- a/drivers/char/tpm/tpm_tis_spi.c
+++ b/drivers/char/tpm/tpm_tis_spi.c
@@ -110,6 +110,7 @@ static int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u8 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

[toc] | [next] | [standalone]

#1583094

Are you sure it is not better to introduce this delay directly in the 
rpi spi driver ?

Other than that i don't see any issue with it.


On 16/02/2017 08:09, Peter Huewe wrote:
> 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@vger.kernel.org>
> Fixes: 0edbfea537d1 ("tpm/tpm_tis_spi: Add support for spi phy")
> Signed-off-by: Alexander Steffen <Alexander.Steffen@infineon.com>
> Signed-off-by: Peter Huewe <peter.huewe@infineon.com>
> ---
>   drivers/char/tpm/tpm_tis_spi.c | 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 b50c5b072df3..685c51bf5d7e 100644
> --- a/drivers/char/tpm/tpm_tis_spi.c
> +++ b/drivers/char/tpm/tpm_tis_spi.c
> @@ -110,6 +110,7 @@ static int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u8 len,
>   
>   		spi_xfer.cs_change = 0;
>   		spi_xfer.len = transfer_len;
> +		spi_xfer.delay_usecs = 5;
>   
>   		if (direction) {
>   			spi_xfer.tx_buf = NULL;

[toc] | [prev] | [next] | [standalone]

#1583159

Am 17. Februar 2017 06:09:42 MEZ schrieb Christophe Ricard <christophe.ricard@gmail.com>:
>Are you sure it is not better to introduce this delay directly in the 
>rpi spi driver ?
>
>Other than that i don't see any issue with it.

Yes - it would be perfect to fix the issue in the upstream rpi spi master driver.
However this only happens sporadically in some strange cases (i.e. 300-500khz, single byte transfer after having more bytes in the same #cs frame) (unrelated to the tpm)

We are still looking into it, why /where it happens and how to reproduce it reliably and then file a bug/fix it.

For now however the priority is to make the tpm_tis_spi driver work reliably also on the rpi, and this is what this workaround does.

We can simply remove it once the rpi spi master is fixed.

Peter
>
>
>On 16/02/2017 08:09, Peter Huewe wrote:
>> 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@vger.kernel.org>
>> Fixes: 0edbfea537d1 ("tpm/tpm_tis_spi: Add support for spi phy")
>> Signed-off-by: Alexander Steffen <Alexander.Steffen@infineon.com>
>> Signed-off-by: Peter Huewe <peter.huewe@infineon.com>
>> ---
>>   drivers/char/tpm/tpm_tis_spi.c | 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 b50c5b072df3..685c51bf5d7e 100644
>> --- a/drivers/char/tpm/tpm_tis_spi.c
>> +++ b/drivers/char/tpm/tpm_tis_spi.c
>> @@ -110,6 +110,7 @@ static int tpm_tis_spi_transfer(struct
>tpm_tis_data *data, u32 addr, u8 len,
>>   
>>   		spi_xfer.cs_change = 0;
>>   		spi_xfer.len = transfer_len;
>> +		spi_xfer.delay_usecs = 5;
>>   
>>   		if (direction) {
>>   			spi_xfer.tx_buf = NULL;

-- 
Sent from my mobile

[toc] | [prev] | [next] | [standalone]

#1587601

On Thu, Feb 16, 2017 at 04:09:46PM +0000, Peter Huewe wrote:
> 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@vger.kernel.org>
> Fixes: 0edbfea537d1 ("tpm/tpm_tis_spi: Add support for spi phy")
> Signed-off-by: Alexander Steffen <Alexander.Steffen@infineon.com>
> Signed-off-by: Peter Huewe <peter.huewe@infineon.com>

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

/Jarkko

> ---
>  drivers/char/tpm/tpm_tis_spi.c | 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 b50c5b072df3..685c51bf5d7e 100644
> --- a/drivers/char/tpm/tpm_tis_spi.c
> +++ b/drivers/char/tpm/tpm_tis_spi.c
> @@ -110,6 +110,7 @@ static int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u8 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
> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web