Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753314AbdCBQem (ORCPT ); Thu, 2 Mar 2017 11:34:42 -0500 Received: from bhuna.collabora.co.uk ([46.235.227.227]:48418 "EHLO bhuna.collabora.co.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751957AbdCBQej (ORCPT ); Thu, 2 Mar 2017 11:34:39 -0500 Subject: Re: [tpmdd-devel] [PATCH v2] tpm: Apply a sane minimum adapterlimit value for retransmission. To: Peter Huewe , Jarkko Sakkinen References: <20170301153617.10106-1-enric.balletbo@collabora.com> <20170302125543.bsn7z5zkerabzh4u@intel.com> Cc: Bryan Freed , tpmdd-devel@lists.sourceforge.net, linux-kernel@vger.kernel.org, wsa@the-dreams.de From: Enric Balletbo i Serra Message-ID: <2c648dea-35df-cdf7-3882-40bb855d3573@collabora.com> Date: Thu, 2 Mar 2017 17:34:32 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5965 Lines: 179 On 02/03/17 14:43, Peter Huewe wrote: > > > Am 2. März 2017 13:55:43 MEZ schrieb Jarkko Sakkinen : >> On Wed, Mar 01, 2017 at 04:36:17PM +0100, Enric Balletbo i Serra wrote: >>> From: Bryan Freed >>> >>> 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 >>> [rework the patch to adapt to the feedback received] >>> Signed-off-by: Enric Balletbo i Serra >>> --- >>> 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