Date: Thu, 02 Mar 2017 14:43:45 +0100
User-Agent: K-9 Mail for Android
In-Reply-To: <20170302125543.bsn7z5zkerabzh4u@intel.com>
References: <20170301153617.10106-1-enric.balletbo@collabora.com> <20170302125543.bsn7z5zkerabzh4u@intel.com>
MIME-Version: 1.0
Content-Type: text/plain;
 charset=utf-8
Content-Transfer-Encoding: 8BIT
Subject: Re: [tpmdd-devel] [PATCH v2] tpm: Apply a sane minimum adapterlimit value for retransmission.
To: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>,
        Enric Balletbo i Serra <enric.balletbo@collabora.com>
CC: Bryan Freed <bfreed@chromium.org>, tpmdd-devel@lists.sourceforge.net,
        linux-kernel@vger.kernel.org, wsa@the-dreams.de
From: Peter Huewe <peterhuewe@gmx.de>
Message-ID: <BA3AA3EA-054E-4B65-82C2-C01EBD2849F2@gmx.de>
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4581
Lines: 140


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.
>> 
>> 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)

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) {
>>  		/* 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

-- 
Sent from my mobile