| Message ID | 20170301153617.10106-1-enric.balletbo@collabora.com |
|---|---|
| State | New |
| Headers | show |
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. > > 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. Andrew ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
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? /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) { > - /* 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 ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
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
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 ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
> >> 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. Hi Enric Would it not be more accurate to say, that you only see the i2c-core message once, for this transfer request. Is the next transfer request again going to use the longer length? then fail, maybe generate another i2c core message, depending on rate limiting, and then use the lower message size? I think it does. Which is why i suggested remembering the length. Andrew ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
Hi Andrew, On 02/03/17 18:24, Andrew Lunn wrote: >>>> 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. > > Hi Enric > > Would it not be more accurate to say, that you only see the i2c-core > message once, for this transfer request. Is the next transfer request > again going to use the longer length? then fail, maybe generate > another i2c core message, depending on rate limiting, and then use the > lower message size? I think it does. > > Which is why i suggested remembering the length. > Ah ok, sorry I did not understand you, got it now. Yes, so with this every time you do, i.e. a tpm_getpubek, you will see the i2c-core message, right. Thanks. Sounds a good suggestion for me. Cheers, Enric > Andrew > ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
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) { - /* 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: