[tpmdd-devel,1/2] tpm: Apply an adapterlimit for retransmission.

Submitted by Enric Balletbo i Serra on Feb. 21, 2017, 2:44 p.m.

Details

Message ID 20170221144500.502-1-enric.balletbo@collabora.com
State New
Headers show

Commit Message

Enric Balletbo i Serra Feb. 21, 2017, 2:44 p.m.
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.

Signed-off-by: Bryan Freed <bfreed@chromium.org>
Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---
 drivers/char/tpm/tpm_i2c_infineon.c | 44 ++++++++++++++++++++++---------------
 1 file changed, 26 insertions(+), 18 deletions(-)

Comments

Andrew Lunn Feb. 21, 2017, 4:29 p.m.
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
Enric Balletbo i Serra Feb. 22, 2017, 11:16 a.m.
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
Peter Hüwe Feb. 22, 2017, 12:39 p.m.
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
Andrew Lunn Feb. 22, 2017, 2:01 p.m.
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
Enric Balletbò i Serra Feb. 27, 2017, 6:48 p.m.
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
Wolfram Sang Feb. 27, 2017, 7:12 p.m.
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
Enric Balletbò i Serra Feb. 27, 2017, 9:30 p.m.
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
Peter Hüwe March 2, 2017, 1:15 p.m.
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
Wolfram Sang March 2, 2017, 2:05 p.m.
> 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
Wolfram Sang March 2, 2017, 2:10 p.m.
> 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

Patch hide | download patch | download mbox

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: