[PATCH 1/2] tpm: Apply an adapterlimit for retransmission.

[PATCH 1/2] tpm: Apply an adapterlimit for retransmission.

[Enric Balletbo i Serra]

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(-)

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:
-- 
2.9.3

[PATCH 2/2] tpm: Do not assume an i2c adapter preserves the msg len

[Enric Balletbo i Serra]

From: Bryan Freed <bfreed@chromium.org>

Prevent a possible infinite loop by using a local variable to
advance len down to 0.  This is safer than trusting the I2C and
adapter drivers to preserve msg2.len.

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 | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/char/tpm/tpm_i2c_infineon.c b/drivers/char/tpm/tpm_i2c_infineon.c
index f04c6b7..fbee05b 100644
--- a/drivers/char/tpm/tpm_i2c_infineon.c
+++ b/drivers/char/tpm/tpm_i2c_infineon.c
@@ -107,11 +107,10 @@ 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;
-	int adapterlimit = len;
+	unsigned int adapterlimit = len;
 
 	/* Lock the adapter for the duration of the whole sequence. */
 	if (!tpm_dev.client->adapter->algo->master_xfer)
@@ -137,18 +136,19 @@ 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);
-			msg2.len = min(adapterlimit, len);
 			rc = __i2c_transfer(tpm_dev.client->adapter, &msg2, 1);
 			if (rc > 0) {
 				/* Since len is unsigned, make doubly sure we
 				 * do not underflow it.
 				 */
-				if (msg2.len > len)
+				if (msglen > len)
 					len = 0;
 				else
-					len -= msg2.len;
-				msg2.buf += msg2.len;
+					len -= msglen;
+				msg2.buf += msglen;
 				break;
 			}
 			/* If the I2C adapter rejected the request,
@@ -156,7 +156,7 @@ static int iic_tpm_read(u8 addr, u8 *buffer, size_t len)
 			 */
 			if (rc == -EINVAL) {
 				adapterlimit = (adapterlimit + 1) / 2;
-				adapterlimit = max(adapterlimit, 32);
+				adapterlimit = max(adapterlimit, 32U);
 			}
 		}
 		if (rc <= 0)
-- 
2.9.3

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

[Andrew Lunn]

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

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

[Enric Balletbo i Serra]

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
> 

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

[Andrew Lunn]

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

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

[Enric Balletbo Serra]

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

Re: [PATCH 1/2] tpm: Apply an adapterlimit for retransmission.

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 1328 bytes --]

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-z923LK4zBo2bacvFa/9K2g@public.gmane.org>, 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


[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 202 bytes --]

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

[-- Attachment #3: Type: text/plain, Size: 192 bytes --]

_______________________________________________
tpmdd-devel mailing list
tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [PATCH 1/2] tpm: Apply an adapterlimit for retransmission.

[Enric Balletbo Serra]

2017-02-27 20:12 GMT+01:00 Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>:
> 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-z923LK4zBo2bacvFa/9K2g@public.gmane.org>, 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

Re: [PATCH 1/2] tpm: Apply an adapterlimit for retransmission.

[Peter Huewe]

Am 27. Februar 2017 20:12:45 MEZ schrieb Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>:
>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-z923LK4zBo2bacvFa/9K2g@public.gmane.org>, 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

-- 
Sent from my mobile

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 341 bytes --]


> 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.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 115 bytes --]

> TPMs are a not the best devices when it comes to i2c :/

BTW I blame the quirky I2C HW of the adapters, so far.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/2] tpm: Apply an adapterlimit for retransmission.

[Peter Huewe]

Am 21. Februar 2017 17:29:48 MEZ schrieb Andrew Lunn <andrew-g2DYL2Zd6BY@public.gmane.org>:
>On Tue, Feb 21, 2017 at 03:44:59PM +0100, Enric Balletbo i Serra wrote:
>> From: Bryan Freed <bfreed-F7+t8E8rja9g9hUCZPvPmw@public.gmane.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

-- 
Sent from my mobile

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot