On 02/03/17 14:43, Peter Huewe wrote:
>
>
> Am 2. März 2017 13:55:43 MEZ schrieb Jarkko Sakkinen
> <[email protected]>:
>> On Wed, Mar 01, 2017 at 04:36:17PM +0100, Enric Balletbo i Serra wrote:
>>> From: Bryan Freed <[email protected]>
>>>
>>> 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 <[email protected]>
>>> [rework the patch to adapt to the feedback received]
>>> Signed-off-by: Enric Balletbo i Serra <[email protected]>
>>> ---
>>> 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
>>> [email protected]
>>> https://lists.sourceforge.net/lists/listinfo/tpmdd-devel
>
Cheers,
Enric
------------------------------------------------------------------------------
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
[email protected]
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel
