Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1585392 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH 1/2] tpm: Apply an adapterlimit for retransmission. Enric Balletbo i Serra <enric.balletbo@collabora.com> - 2017-02-21 15:50 +0100
    Re: [tpmdd-devel] [PATCH 1/2] tpm: Apply an adapterlimit for  retransmission. Andrew Lunn <andrew@lunn.ch> - 2017-02-21 17:40 +0100
      Re: [tpmdd-devel] [PATCH 1/2] tpm: Apply an adapterlimit for  retransmission. Enric Balletbo i Serra <enric.balletbo@collabora.com> - 2017-02-22 12:20 +0100
        Re: [tpmdd-devel] [PATCH 1/2] tpm: Apply an adapterlimit for  retransmission. Andrew Lunn <andrew@lunn.ch> - 2017-02-22 15:10 +0100
          Re: [tpmdd-devel] [PATCH 1/2] tpm: Apply an adapterlimit for retransmission. Enric Balletbo Serra <eballetbo@gmail.com> - 2017-02-27 19:50 +0100
            Re: [tpmdd-devel] [PATCH 1/2] tpm: Apply an adapterlimit for  retransmission. Wolfram Sang <wsa@the-dreams.de> - 2017-02-27 20:20 +0100
              Re: [tpmdd-devel] [PATCH 1/2] tpm: Apply an adapterlimit for retransmission. Enric Balletbo Serra <eballetbo@gmail.com> - 2017-02-27 23:40 +0100
      Re: [tpmdd-devel] [PATCH 1/2] tpm: Apply an adapterlimit for retransmission. Peter Huewe <peterhuewe@gmx.de> - 2017-02-22 13:50 +0100

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

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

[toc] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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
> 

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

-- 
Sent from my mobile

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web