Re: [tpmdd-devel] [PATCH v2] tpm: Apply a sane minimum adapterlimit value for retransmission.

[Enric Balletbo i Serra <enric.balletbo@collabora.com>]

On 02/03/17 14:43, Peter Huewe wrote:
> 
> 
> Am 2. März 2017 13:55:43 MEZ schrieb Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>:
>> On Wed, Mar 01, 2017 at 04:36:17PM +0100, 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.
>>
>> What is 0x05 command?
> 0x05 is the address of the fifo.
> I honestly think that it needs toll​ be repeated after a stop condition.
> I'll look that up.
> 
>>
>> /Jarkko
>>
>>> 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.
>>>

>> Hi Enric
>>
>> Maybe you should remember that you need to use smaller transfers? If
>> you don't remember, but use the full size message every time and only
>> drop back on error, the i2c core is going to log rate limited
>> messages. By remembering, there will only be one such message in the
>> log.
>>

Maybe I did not explain well but this is what the code does, when i2c-core fails with -EOPNOTSUPP because the msg is too long for this adapter it loop with a smaller chunk of fixed size, so you only see the i2c-core message once.

>>	  Andrew
>>> 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 | 45
>> +++++++++++++++++++++----------------
>>>  1 file changed, 26 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/drivers/char/tpm/tpm_i2c_infineon.c
>> b/drivers/char/tpm/tpm_i2c_infineon.c
>>> index 62ee44e..88bf947 100644
>>> --- a/drivers/char/tpm/tpm_i2c_infineon.c
>>> +++ b/drivers/char/tpm/tpm_i2c_infineon.c
>>> @@ -107,39 +107,27 @@ static int iic_tpm_read(u8 addr, u8 *buffer,
>> size_t len)
>>>  		.len = len,
>>>  		.buf = buffer
>>>  	};
>>> -	struct i2c_msg msgs[] = {msg1, msg2};
>>>  
>>>  	int rc = 0;
>>>  	int count;
>>> +	unsigned int adapterlimit = len;
>>>  
>>>  	/* Lock the adapter for the duration of the whole sequence. */
>>>  	if (!tpm_dev.client->adapter->algo->master_xfer)
>>>  		return -EOPNOTSUPP;
>>>  	i2c_lock_adapter(tpm_dev.client->adapter);
>>>  
>>> -	if (tpm_dev.chip_type == SLB9645) {
> Why are you / bryan removing this code path here?
> I put it there for a good reason (i.e. faster transfers)
> 

Good question, I might be wrong as the original code is not mine but I think that is for safety.

I'm wondering what will happen if you connect a SLB9645 with a bus that has the max_read_len quirk defined and you try to read a message greater than the max_read_len. In such case __i2c_transfer will fail with -EOPNOTSUPP and do not work.

Thinking about this maybe we can do this here:

if (tpm_dev.chip_type == SLB9645) {
   /* fast transfer */
   rc = __i2c...
   ...
} else {
   /* normal transfer */
   rc = __i2c...
   ...
}

/* If the transfer failed try with chunks of 32 */
if (rc == -EOPNOTSUPP)
  rc = __i2c...



> Thanks,
> Peter
>>> -		/* use a combined read for newer chips
>>> -		 * unfortunately the smbus functions are not suitable due to
>>> -		 * the 32 byte limit of the smbus.
>>> -		 * retries should usually not be needed, but are kept just to
>>> -		 * be on the safe side.
>>> -		 */
>>> -		for (count = 0; count < MAX_COUNT; count++) {
>>> -			rc = __i2c_transfer(tpm_dev.client->adapter, msgs, 2);
>>> -			if (rc > 0)
>>> -				break;	/* break here to skip sleep */
>>> -			usleep_range(SLEEP_DURATION_LOW, SLEEP_DURATION_HI);
>>> -		}
>>> -	} else {
>>> +	/* Expect to send one command message and one data message, but
>>> +	 * support looping over each or both if necessary.
>>> +	 */
>>> +	while (len > 0) {if (rc == -EOPNOTSUPP)
>>>  		/* 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 */
>>> -
>>> +				break;
>>>  			usleep_range(SLEEP_DURATION_LOW, SLEEP_DURATION_HI);
>>>  		}
>>> -
>>>  		if (rc <= 0)
>>>  			goto out;
>>>  
>>> @@ -148,11 +136,30 @@ static int iic_tpm_read(u8 addr, u8 *buffer,
>> size_t len)
>>>  		 * retrieving the data
>>>  		 */
>>>  		for (count = 0; count < MAX_COUNT; count++) {
>>> +			unsigned int msglen = msg2.len =
>>> +					min_t(unsigned int, adapterlimit, len);
>>>  			usleep_range(SLEEP_DURATION_LOW, SLEEP_DURATION_HI);
>>>  			rc = __i2c_transfer(tpm_dev.client->adapter, &msg2, 1);
>>> -			if (rc > 0)
>>> +			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)
>>> +				adapterlimit = I2C_SMBUS_BLOCK_MAX;
>>>  		}
>>> +		if (rc <= 0)
>>> +			goto out;
>>>  	}
>>>  
>>>  out:
>>> -- 
>>> 2.9.3
>>>
>>>
>>>
>> ------------------------------------------------------------------------------
>>> Check out the vibrant tech community on one of the world's most
>>> engaging tech sites, SlashDot.org! http://sdm.link/slashdot
>>> _______________________________________________
>>> tpmdd-devel mailing list
>>> tpmdd-devel@lists.sourceforge.net
>>> https://lists.sourceforge.net/lists/listinfo/tpmdd-devel
> 
Cheers,
 Enric