| Message ID | 20170221144500.502-1-enric.balletbo@collabora.com |
|---|---|
| State | New |
| Headers | show |
On Tue, Feb 21, 2017 at 03:44:59PM +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 -EINVAL. > Retry them with size backoff without re-issuing the 0x05 command > as this appears to occasionally put the TPM in a bad state. Hi Enric Rather than trying small and smaller transfers, would it not be better to get the i2c core to expose the quirk info about transfer limits? 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, Removing Bryan Freed from the loop as seems his email is not valid anymore. I already CC'ied Andrey which is doing the TPM bit in chromeos kernel. On 21/02/17 17:29, Andrew Lunn wrote: > On Tue, Feb 21, 2017 at 03:44:59PM +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 -EINVAL. >> Retry them with size backoff without re-issuing the 0x05 command >> as this appears to occasionally put the TPM in a bad state. > > Hi Enric > > Rather than trying small and smaller transfers, would it not be better > to get the i2c core to expose the quirk info about transfer limits? > Sounds a good idea to me, I guess the quirk info can be accessed with tpm_dev.client->adapter->quirks->max_read_len so I think we don't need to touch the i2c core. I'll propose a second version of the patch. Thanks, Enric > Andrew > ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
Am 21. Februar 2017 17:29:48 MEZ schrieb Andrew Lunn <andrew@lunn.ch>: >On Tue, Feb 21, 2017 at 03:44:59PM +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 -EINVAL. >> Retry them with size backoff without re-issuing the 0x05 command >> as this appears to occasionally put the TPM in a bad state. > >Hi Enric > >Rather than trying small and smaller transfers, would it not be better >to get the i2c core to expose the quirk info about transfer limits? > +1 I think that would be the better idea. Peter > Andrew
On Wed, Feb 22, 2017 at 12:16:08PM +0100, Enric Balletbo i Serra wrote: > Hi Andrew, > > Removing Bryan Freed from the loop as seems his email is not valid anymore. I already CC'ied Andrey which is doing the TPM bit in chromeos kernel. > > On 21/02/17 17:29, Andrew Lunn wrote: > > On Tue, Feb 21, 2017 at 03:44:59PM +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 -EINVAL. > >> Retry them with size backoff without re-issuing the 0x05 command > >> as this appears to occasionally put the TPM in a bad state. > > > > Hi Enric > > > > Rather than trying small and smaller transfers, would it not be better > > to get the i2c core to expose the quirk info about transfer limits? > > > > Sounds a good idea to me, I guess the quirk info can be accessed with > > tpm_dev.client->adapter->quirks->max_read_len > > so I think we don't need to touch the i2c core. I'll propose a second version of the patch. Hi Enric You should probably ask Wolfram Sang <wsa@the-dreams.de>, the i2c subsystem maintainer. He may prefer adding an API call. Andrew ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
Bounce to Wolfram Sang 2017-02-22 15:01 GMT+01:00 Andrew Lunn <andrew@lunn.ch>: > On Wed, Feb 22, 2017 at 12:16:08PM +0100, Enric Balletbo i Serra wrote: >> Hi Andrew, >> >> Removing Bryan Freed from the loop as seems his email is not valid anymore. I already CC'ied Andrey which is doing the TPM bit in chromeos kernel. >> >> On 21/02/17 17:29, Andrew Lunn wrote: >> > On Tue, Feb 21, 2017 at 03:44:59PM +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 -EINVAL. >> >> Retry them with size backoff without re-issuing the 0x05 command >> >> as this appears to occasionally put the TPM in a bad state. >> > >> > Hi Enric >> > >> > Rather than trying small and smaller transfers, would it not be better >> > to get the i2c core to expose the quirk info about transfer limits? >> > >> >> Sounds a good idea to me, I guess the quirk info can be accessed with >> >> tpm_dev.client->adapter->quirks->max_read_len >> >> so I think we don't need to touch the i2c core. I'll propose a second version of the patch. > > Hi Enric > > You should probably ask Wolfram Sang <wsa@the-dreams.de>, the i2c > subsystem maintainer. He may prefer adding an API call. > > Andrew ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
Hi, > >> > Rather than trying small and smaller transfers, would it not be better > >> > to get the i2c core to expose the quirk info about transfer limits? > >> > > >> > >> Sounds a good idea to me, I guess the quirk info can be accessed with > >> > >> tpm_dev.client->adapter->quirks->max_read_len > >> > >> so I think we don't need to touch the i2c core. I'll propose a second version of the patch. > > > > Hi Enric > > > > You should probably ask Wolfram Sang <wsa@the-dreams.de>, the i2c > > subsystem maintainer. He may prefer adding an API call. Thanks for pointing me to this thread. I understand it looks tempting to use the quirks struct directly, but I don't think this is the proper solution. Quirks are complex and and to determine which one finally applies, you need all the logic encoded in i2c_check_for_quirks(). Which already gets called on every transfer. So, my suggestion would be to simply fall back to a sane minimum when the maximum failed. 32 (I2C_SMBUS_BLOCK_MAX) should be a good choice. BTW I noted that the original patch checks for -EINVAL. The core returns -EOPNOTSUPP, though. So, a) the patch needs to be adapted and b) it looks the i2c host driver returning -EINVAL could be converted to use the quirk infrastructure? Which driver is it? Regards, Wolfram ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
2017-02-27 20:12 GMT+01:00 Wolfram Sang <wsa@the-dreams.de>: > Hi, > >> >> > Rather than trying small and smaller transfers, would it not be better >> >> > to get the i2c core to expose the quirk info about transfer limits? >> >> > >> >> >> >> Sounds a good idea to me, I guess the quirk info can be accessed with >> >> >> >> tpm_dev.client->adapter->quirks->max_read_len >> >> >> >> so I think we don't need to touch the i2c core. I'll propose a second version of the patch. >> > >> > Hi Enric >> > >> > You should probably ask Wolfram Sang <wsa@the-dreams.de>, the i2c >> > subsystem maintainer. He may prefer adding an API call. > > Thanks for pointing me to this thread. > > I understand it looks tempting to use the quirks struct directly, but I > don't think this is the proper solution. Quirks are complex and and to > determine which one finally applies, you need all the logic encoded in > i2c_check_for_quirks(). Which already gets called on every transfer. > > So, my suggestion would be to simply fall back to a sane minimum when > the maximum failed. 32 (I2C_SMBUS_BLOCK_MAX) should be a good choice. > Sounds a good solution for me, I'll test and send a new version of the patches. > BTW I noted that the original patch checks for -EINVAL. The core returns > -EOPNOTSUPP, though. So, a) the patch needs to be adapted Yes I already detected this, In this series I forget to fixup the patch that fixed this when I did the git rebase. It's is fixed in the second version. > and b) it > looks the i2c host driver returning -EINVAL could be converted to use > the quirk infrastructure? Which driver is it? > > Regards, > > Wolfram > Regards, Enric ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
Am 27. Februar 2017 20:12:45 MEZ schrieb Wolfram Sang <wsa@the-dreams.de>: >Hi, > >> >> > Rather than trying small and smaller transfers, would it not be >better >> >> > to get the i2c core to expose the quirk info about transfer >limits? >> >> > >> >> >> >> Sounds a good idea to me, I guess the quirk info can be accessed >with >> >> >> >> tpm_dev.client->adapter->quirks->max_read_len >> >> >> >> so I think we don't need to touch the i2c core. I'll propose a >second version of the patch. >> > >> > Hi Enric >> > >> > You should probably ask Wolfram Sang <wsa@the-dreams.de>, the i2c >> > subsystem maintainer. He may prefer adding an API call. > >Thanks for pointing me to this thread. > >I understand it looks tempting to use the quirks struct directly, but I >don't think this is the proper solution. Quirks are complex and and to >determine which one finally applies, you need all the logic encoded in >i2c_check_for_quirks(). Which already gets called on every transfer. > >So, my suggestion would be to simply fall back to a sane minimum when >the maximum failed. 32 (I2C_SMBUS_BLOCK_MAX) should be a good choice. Hi, One problem is however that e.g. in the case of the atmel tpms, there is no sane minimum, since you mustn't split the tpm apdu. If the command is larger than the supported adapter limit the command the only viable option is to return an error. For this the tpm layer would need the adapter limit. If somehow possible I would seriously vote for a adapter limit field (maybe not in the quirks). TPMs are a not the best devices when it comes to i2c :/ Peter > >BTW I noted that the original patch checks for -EINVAL. The core >returns >-EOPNOTSUPP, though. So, a) the patch needs to be adapted and b) it >looks the i2c host driver returning -EINVAL could be converted to use >the quirk infrastructure? Which driver is it? > >Regards, > > Wolfram
> If the command is larger than the supported adapter limit the command the only viable option is to return an error. > For this the tpm layer would need the adapter limit. There is no single value for a limit. This is why the quirks struct is complex and we need a function to determine the limit each time for a given transfer from it. ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
> TPMs are a not the best devices when it comes to i2c :/
BTW I blame the quirky I2C HW of the adapters, so far.
------------------------------------------------------------------------------
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..f04c6b7 100644 --- a/drivers/char/tpm/tpm_i2c_infineon.c +++ b/drivers/char/tpm/tpm_i2c_infineon.c @@ -111,35 +111,24 @@ static int iic_tpm_read(u8 addr, u8 *buffer, size_t len) int rc = 0; int count; + 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; @@ -149,10 +138,29 @@ static int iic_tpm_read(u8 addr, u8 *buffer, size_t len) */ for (count = 0; count < MAX_COUNT; count++) { usleep_range(SLEEP_DURATION_LOW, SLEEP_DURATION_HI); + msg2.len = min(adapterlimit, len); 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 (msg2.len > len) + len = 0; + else + len -= msg2.len; + msg2.buf += msg2.len; break; + } + /* If the I2C adapter rejected the request, + * try a smaller chunk. + */ + if (rc == -EINVAL) { + adapterlimit = (adapterlimit + 1) / 2; + adapterlimit = max(adapterlimit, 32); + } } + if (rc <= 0) + goto out; } out: