Re: [PATCH v3] tpm: Apply a sane minimum adapterlimit value for retransmission.

[Peter Huewe <peterhuewe@gmx.de>]

Am 5. April 2017 11:03:27 MESZ schrieb Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>:
>On Tue, Mar 28, 2017 at 05:29:38PM +0200, Enric Balletbo i Serra wrote:
>> From: Bryan Freed <bfreed@chromium.org>
>> 
>> When the I2C Infineon part is attached to an I2C adapter that imposes
>> a size limitation, large requests will fail with -EOPNOTSUPP. Retry
>> them with a sane minimum size without re-issuing the 0x05 command
>> as this appears to occasionally put the TPM in a bad state.
>> 
>> Signed-off-by: Bryan Freed <bfreed@chromium.org>
>> [rework the patch to adapt to the feedback received]
>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
>> ---
>> This is a reworked version of the original patch based on the
>> suggestion made by Wolfram Sang to simply fall back to a sane minimum
>> when the maximum fails.
>> 
>> Changes since v2:
>>  - Do not remove faster transfers when chip is SLB9645 (Peter Huewe)
>>  - Remember the adapterlimit length once it fails to not generate
>extra
>>    i2c core messages (suggested by Andrew Lunn)
>> Changes since v1:
>>  - Check the correct return value (-EOPNOTSUPP instead of -EINVAL)
>>  - Fall back len to I2C_SMBUS_BLOCK_MAX if fails.
>> 
>>  drivers/char/tpm/tpm_i2c_infineon.c | 76
>+++++++++++++++++++++++++++----------
>>  1 file changed, 56 insertions(+), 20 deletions(-)
>> 
>> diff --git a/drivers/char/tpm/tpm_i2c_infineon.c
>b/drivers/char/tpm/tpm_i2c_infineon.c
>> index 62ee44e..fdefcdb 100644
>> --- a/drivers/char/tpm/tpm_i2c_infineon.c
>> +++ b/drivers/char/tpm/tpm_i2c_infineon.c
>> @@ -70,6 +70,7 @@ struct tpm_inf_dev {
>>  	u8 buf[TPM_BUFSIZE + sizeof(u8)]; /* max. buffer size + addr */
>>  	struct tpm_chip *chip;
>>  	enum i2c_chip_type chip_type;
>> +	unsigned int adapterlimit;
>>  };
>>  
>>  static struct tpm_inf_dev tpm_dev;
>> @@ -111,6 +112,7 @@ static int iic_tpm_read(u8 addr, u8 *buffer,
>size_t len)
>>  
>>  	int rc = 0;
>>  	int count;
>> +	unsigned int msglen = len;
>>  
>>  	/* Lock the adapter for the duration of the whole sequence. */
>>  	if (!tpm_dev.client->adapter->algo->master_xfer)
>> @@ -131,27 +133,61 @@ static int iic_tpm_read(u8 addr, u8 *buffer,
>size_t len)
>>  			usleep_range(SLEEP_DURATION_LOW, SLEEP_DURATION_HI);
>>  		}
>>  	} else {
>> -		/* slb9635 protocol should work in all cases */
>> -		for (count = 0; count < MAX_COUNT; count++) {
>> -			rc = __i2c_transfer(tpm_dev.client->adapter, &msg1, 1);
>> -			if (rc > 0)
>> -				break;	/* break here to skip sleep */
>> -
>> -			usleep_range(SLEEP_DURATION_LOW, SLEEP_DURATION_HI);
>> -		}
>> -
>> -		if (rc <= 0)
>> -			goto out;
>> -
>> -		/* After the TPM has successfully received the register address
>> -		 * it needs some time, thus we're sleeping here again, before
>> -		 * retrieving the data
>> +		/* Expect to send one command message and one data message, but
>> +		 * support looping over each or both if necessary.
>>  		 */
>> -		for (count = 0; count < MAX_COUNT; count++) {
>> -			usleep_range(SLEEP_DURATION_LOW, SLEEP_DURATION_HI);
>> -			rc = __i2c_transfer(tpm_dev.client->adapter, &msg2, 1);
>> -			if (rc > 0)
>> -				break;
>> +		while (len > 0) {
>> +			/* slb9635 protocol should work in all cases */
>> +			for (count = 0; count < MAX_COUNT; count++) {
>> +				rc = __i2c_transfer(tpm_dev.client->adapter,
>> +						    &msg1, 1);
>> +				if (rc > 0)
>> +					break;	/* break here to skip sleep */
>> +
>> +				usleep_range(SLEEP_DURATION_LOW,
>> +					     SLEEP_DURATION_HI);
>> +			}
>> +
>> +			if (rc <= 0)
>> +				goto out;
>> +
>> +			/* After the TPM has successfully received the register
>> +			 * address it needs some time, thus we're sleeping here
>> +			 * again, before retrieving the data
>> +			 */
>> +			for (count = 0; count < MAX_COUNT; count++) {
>> +				if (tpm_dev.adapterlimit) {
>> +					msglen = min_t(unsigned int,
>> +						       tpm_dev.adapterlimit,
>> +						       len);
>> +					msg2.len = msglen;
>> +				}
>> +				usleep_range(SLEEP_DURATION_LOW,
>> +					     SLEEP_DURATION_HI);
>> +				rc = __i2c_transfer(tpm_dev.client->adapter,
>> +						    &msg2, 1);
>> +				if (rc > 0) {
>> +					/* Since len is unsigned, make doubly
>> +					 * sure we do not underflow it.
>> +					 */
>> +					if (msglen > len)
>> +						len = 0;
>> +					else
>> +						len -= msglen;
>> +					msg2.buf += msglen;
>> +					break;
>> +				}
>> +				/* If the I2C adapter rejected the request (e.g
>> +				 * when the quirk read_max_len < len) fall back
>> +				 * to a sane minimum value and try again.
>> +				 */
>> +				if (rc == -EOPNOTSUPP)
>> +					tpm_dev.adapterlimit =
>> +							I2C_SMBUS_BLOCK_MAX;
>> +			}
>> +
>> +			if (rc <= 0)
>> +				goto out;
>>  		}
>>  	}
>>  
>> -- 
>> 2.9.3
>> 
>
>Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
>
>Peter, Andrew, anyone: Tested-by?
>

Not yet, I'll put it on my list to test.
Hopefully by next tuesday.
Peter

>/Jarkko

-- 
Sent from my mobile