retry handling and intermittent self test failure fix
Subsystems (2)
James Bottomley
maintainerv2 cover retry handling and intermittent self test failure fix
The original fix for the intermittent self test failure contained a bug in the retry loop that resent the command with a mangled header. This was discovered when coding up the TPM2_RC_RETRY handling, so this two patch series bases the intermittent self test failure fix on the working TPM2_RC_RETRY handling code. This time I actually created an emulator that sends TPM2_RC_TESTING and verified it does work.
James
---
James Bottomley (2):
tpm: add retry logic
tpm: fix intermittent failure with self tests
drivers/char/tpm/tpm-interface.c | 87 +++++++++++++++++++++++++++++++++------- drivers/char/tpm/tpm.h | 2 + drivers/char/tpm/tpm2-cmd.c | 54 ++++++++----------------- 3 files changed, 90 insertions(+), 53 deletions(-)
Signature · 1 line
2.12.3
James Bottomley
maintainerPATCH v2 1/2 tpm: add retry logic
TPM2 can return TPM2_RC_RETRY to any command and when it does we get unexpected failures inside the kernel that surprise users (this is mostly observed in the trusted key handling code). The UEFI 2.6 spec has advice on how to handle this:
The firmware SHALL not return TPM2_RC_RETRY prior to the completion of the call to ExitBootServices().
Implementer's Note: the implementation of this function should check the return value in the TPM response and, if it is TPM2_RC_RETRY, resend the command. The implementation may abort if a sufficient number of retries has been done.
So we follow that advice in our tpm_transmit() code using TPM2_DURATION_SHORT as the initial wait duration and TPM2_DURATION_LONG as the maximum wait time. This should fix all the in-kernel use cases and also means that user space TSS implementations don't have to have their own retry handling.
---
v2: renamed tpm_transmit_internal() to tpm_try_transmit()
--- drivers/char/tpm/tpm-interface.c | 75 ++++++++++++++++++++++++++++++++-------- drivers/char/tpm/tpm.h | 1 + 2 files changed, 61 insertions(+), 15 deletions(-)
drivers/char/tpm/tpm-interface.c +60 -15
@@ -398,21 +398,10 @@ static void tpm_relinquish_locality(struct tpm_chip *chip) chip->locality =-1 ; } -/** -* tpm_transmit - Internal kernel interface to transmit TPM commands. -* -* @chip: TPM chip to use -* @space: tpm space -* @buf: TPM command buffer -* @bufsiz: length of the TPM command buffer -* @flags: tpm transmit flags - bitmap -* -* Return: -* 0 when the operation is successful. -* A negative number for system errors (errno). -*/ -ssize_t tpm_transmit (struct tpm_chip *chip,struct tpm_space *space, -u8 *buf,size_t bufsiz,unsigned int flags) +static ssize_t tpm_try_transmit (struct tpm_chip *chip, +struct tpm_space *space, +u8 *buf,size_t bufsiz, +unsigned int flags) {struct tpm_output_header *header = (void *)buf;int rc;
@@ -550,6 +539,62 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space, }/** +* tpm_transmit - Internal kernel interface to transmit TPM commands. +* +* @chip: TPM chip to use +* @space: tpm space +* @buf: TPM command buffer +* @bufsiz: length of the TPM command buffer +* @flags: tpm transmit flags - bitmap +* +* A wrapper around tpm_try_transmit that handles TPM2_RC_RETRY +* returns from the TPM and retransmits the command after a delay up +* to a maximum wait of TPM2_DURATION_LONG. +* +* Note: TPM1 never returns TPM2_RC_RETRY so the retry logic is TPM2 +* only +* +* Return: +* the length of the return when the operation is successful. +* A negative number for system errors (errno). +*/ +ssize_t tpm_transmit (struct tpm_chip *chip,struct tpm_space *space, +u8 *buf,size_t bufsiz,unsigned int flags) +{ +struct tpm_output_header *header = (struct tpm_output_header *)buf; +/* space for header and handles */ +u8 save[TPM_HEADER_SIZE +3 *sizeof (u32)]; +unsigned int delay_msec =TPM2_DURATION_SHORT ; +u32 rc =0 ; +ssize_t ret; +const size_t save_size =min (space ?sizeof (save) :TPM_HEADER_SIZE , + bufsiz); + +/* +* Subtlety here: if we have a space, the handles will be +* transformed, so when we restore the header we also have to +* restore the handles. +*/ +memcpy (save, buf, save_size); + +for (;;) { + ret =tpm_try_transmit (chip, space, buf, bufsiz, flags); +if (ret <0 ) +break ; + rc =be32_to_cpu (header->return_code); +if (rc !=TPM2_RC_RETRY ) +break ; + delay_msec *=2 ; +if (delay_msec >TPM2_DURATION_LONG ) { +dev_err (&chip->dev,"TPM is in retry loop\n" ); +break ; + } +tpm_msleep (delay_msec); +memcpy (buf, save, save_size); + } +return ret; +} +/** * tpm_transmit_cmd - send a tpm command to the device * The function extracts tpm out header return code *
drivers/char/tpm/tpm.h +1 -0
@@ -112,6 +112,7 @@ enum tpm2_return_codes {TPM2_RC_COMMAND_CODE =0x0143 ,TPM2_RC_TESTING =0x090A ,/* RC_WARN */ TPM2_RC_REFERENCE_H0 =0x0910 , + TPM2_RC_RETRY = 0x0922, }; enum tpm2_algorithms {
Signature · 1 line
2.12.3
James Bottomley
maintainerPATCH v2 2/2 tpm: fix intermittent failure with self tests
In reply to James BottomleyMy Nuvoton 6xx in a Dell XPS-13 has been intermittently failing to work (necessitating a reboot). The problem seems to be that the TPM gets into a state where the partial self-test doesn't return TPM_RC_SUCCESS (meaning all tests have run to completion), but instead returns TPM_RC_TESTING (meaning some tests are still running in the background). There are various theories that resending the self-test command actually causes the tests to restart and thus triggers more TPM_RC_TESTING returns until the timeout is exceeded.
There are several issues here: firstly being we shouldn't slow down the boot sequence waiting for the self test to complete once the TPM backgrounds them. It will actually make available all functions that have passed and if it gets a failure return TPM_RC_FAILURE to every subsequent command. So the fix is to kick off self tests once and if they return TPM_RC_TESTING log that as a backgrounded self test and continue on. In order to prevent other tpm users from seeing any TPM_RC_TESTING returns (which it might if they send a command that needs a TPM subsystem which is still under test), we loop in tpm_transmit_cmd until either a timeout or we don't get a TPM_RC_TESTING return.
Finally, there have been observations of strange returns from a partial test. One Nuvoton is occasionally returning TPM_RC_COMMAND_CODE, so treat any unexpected return from a partial self test as an indication we need to run a full self test.
[[email protected]: cleaned up some klog messages and
dropped tpm_transmit_check() helper function from James' original commit.]
---
v2:
Take the original commit from the tpmdd tree (which didn't work because of a failure to restore the header and handles) and integrate the TPM2_RC_TESTING retry logic with TPM2_RC_RETRY handling. Have tested this version in the face of TPM2_RC_TESTING returns using an emulator
--- drivers/char/tpm/tpm-interface.c | 16 ++++++++++-- drivers/char/tpm/tpm.h | 1 + drivers/char/tpm/tpm2-cmd.c | 54 ++++++++++++---------------------------- 3 files changed, 31 insertions(+), 40 deletions(-)
drivers/char/tpm/tpm-interface.c +14 -2
@@ -569,6 +569,8 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,ssize_t ret;const size_t save_size =min (space ?sizeof (save) :TPM_HEADER_SIZE , bufsiz); +/* the command code is where the return code will be */ +u32 cc =be32_to_cpu (header->return_code);/* * Subtlety here: if we have a space, the handles will be
@@ -582,11 +584,21 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,if (ret <0 )break ; rc =be32_to_cpu (header->return_code); -if (rc !=TPM2_RC_RETRY ) +if (rc !=TPM2_RC_RETRY && rc !=TPM2_RC_TESTING ) +break ; +/* +* return immediately if self test returns test +* still running to shorten boot time. +*/ +if (rc ==TPM2_RC_TESTING && cc ==TPM2_CC_SELF_TEST )break ; delay_msec *=2 ;if (delay_msec >TPM2_DURATION_LONG ) { -dev_err (&chip->dev,"TPM is in retry loop\n" ); +if (rc ==TPM2_RC_RETRY ) +dev_err (&chip->dev,"in retry loop\n" ); +else +dev_err (&chip->dev, +"self test is still running\n" );break ; }tpm_msleep (delay_msec);
drivers/char/tpm/tpm.h +1 -0
@@ -108,6 +108,7 @@ enum tpm2_return_codes {TPM2_RC_HASH =0x0083 ,/* RC_FMT1 */ TPM2_RC_HANDLE =0x008B ,TPM2_RC_INITIALIZE =0x0100 ,/* RC_VER1 */ +TPM2_RC_FAILURE =0x0101 ,TPM2_RC_DISABLED =0x0120 ,TPM2_RC_COMMAND_CODE =0x0143 , TPM2_RC_TESTING = 0x090A, /* RC_WARN */
drivers/char/tpm/tpm2-cmd.c +16 -38
96 lines not on this page · raw message
Signature · 1 line
2.12.3
Re: [PATCH v2 1/2] tpm: add retry logic
In reply to James BottomleyQuoted 13 lines · James Bottomley
TPM2 can return
TPM2_RC_RETRYto any command and when it does we get unexpected failures inside the kernel that surprise users (this is mostly observed in the trusted key handling code). The UEFI 2.6 spec has advice on how to handle this:The firmware SHALL not return
TPM2_RC_RETRYprior to the completion of the call toExitBootServices().Implementer's Note: the implementation of this function should check the return value in the TPM response and, if it is
TPM2_RC_RETRY, resend the command. The implementation may abort if a sufficient number of retries has been done.
Quoted 8 lines
So we follow that advice in our
tpm_transmit()code usingTPM2_DURATION_SHORTas the initial wait duration andTPM2_DURATION_LONGas the maximum wait time. This should fix all the in-kernel use cases and also means that user space TSS implementations don't have to have their own retry handling.Signed-off-by James Bottomley <[email protected]>
/Jarkko
Re: [PATCH v2 1/2] tpm: add retry logic
In reply to James BottomleyQuoted 39 lines
/**
+ *tpm_transmit- Internal kernel interface to transmit TPM commands.
+ *
+ * @chip: TPM chip to use
+ * @space: tpm space
+ * @buf: TPM command buffer
+ * @bufsiz: length of the TPM command buffer
+ * @flags: tpm transmit flags - bitmap
+ *
+ * A wrapper aroundtpm_try_transmitthat handlesTPM2_RC_RETRY
+ * returns from the TPM and retransmits the command after a delay up
+ * to a maximum wait ofTPM2_DURATION_LONG.
+ *
+ * Note: TPM1 never returnsTPM2_RC_RETRYso the retry logic is TPM2
+ * only
+ *
+ * Return:
+ * the length of the return when the operation is successful.
+ * A negative number for system errors (errno).
+ */drivers/char/tpm/tpm-interface.c +17
-0+ssize_t tpm_transmit (struct tpm_chip *chip,struct tpm_space *space, +u8 *buf,size_t bufsiz,unsigned int flags) +{ +struct tpm_output_header *header = (struct tpm_output_header *)buf; +/* space for header and handles */ +u8 save[TPM_HEADER_SIZE +3 *sizeof (u32)]; +unsigned int delay_msec =TPM2_DURATION_SHORT ; +u32 rc =0 ; +ssize_t ret; +const size_t save_size =min (space ?sizeof (save) :TPM_HEADER_SIZE , + bufsiz); + +/* +* Subtlety here: if we have a space, the handles will be +* transformed, so when we restore the header we also have to +* restore the handles. +*/
drivers/char/tpm/tpm-interface.c ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space, · line 587
Quoted 10 lines
drivers/char/tpm/tpm-interface.c +10
-0+memcpy (save, buf, save_size); + +for (;;) { + ret =tpm_try_transmit (chip, space, buf, bufsiz, flags); +if (ret <0 ) +break ; + rc =be32_to_cpu (header->return_code); +if (rc !=TPM2_RC_RETRY ) +break ; + delay_msec *=2 ;
Was wondering if this increment could be moved after tpm_msleep(delay_msec) ?
Thanks & Regards,
- Nayna
Quoted 25 lines
drivers/char/tpm/tpm-interface.c +10
-0+if (delay_msec >TPM2_DURATION_LONG ) { +dev_err (&chip->dev,"TPM is in retry loop\n" ); +break ; + } +tpm_msleep (delay_msec); +memcpy (buf, save, save_size); + } +return ret; +} +/** * tpm_transmit_cmd - send a tpm command to the device * The function extracts tpm out header return code * drivers/char/tpm/tpm.h +1
-0@@ -112,6 +112,7 @@ enum tpm2_return_codes {TPM2_RC_COMMAND_CODE =0x0143 ,TPM2_RC_TESTING =0x090A ,/* RC_WARN */ TPM2_RC_REFERENCE_H0 =0x0910 , + TPM2_RC_RETRY = 0x0922, }; enum tpm2_algorithms {
James Bottomley
maintainerRe: [PATCH v2 1/2] tpm: add retry logic
Quoted 52 lines · Nayna Jain
Quoted 52 lines
/**
+ *tpm_transmit- Internal kernel interface to transmit TPM commands.
+ *
+ * @chip: TPM chip to use
+ * @space: tpm space
+ * @buf: TPM command buffer
+ * @bufsiz: length of the TPM command buffer
+ * @flags: tpm transmit flags - bitmap
+ *
+ * A wrapper aroundtpm_try_transmitthat handlesTPM2_RC_RETRY
+ * returns from the TPM and retransmits the command after a delay up
+ * to a maximum wait ofTPM2_DURATION_LONG.
+ *
+ * Note: TPM1 never returnsTPM2_RC_RETRYso the retry logic is TPM2
+ * only
+ *
+ * Return:
+ * the length of the return when the operation is successful.
+ * A negative number for system errors (errno).
+ */
+ssize_t tpm_transmit(structtpm_chip*chip, structtpm_space*space,drivers/char/tpm/tpm-interface.c +3
-0+ u8 *buf, size_t bufsiz, unsigned int flags) +{ + struct tpm_output_header *header = (struct
tpm_output_header*)buf;drivers/char/tpm/tpm-interface.c +6
-0+/* space for header and handles */ +u8 save[TPM_HEADER_SIZE +3 *sizeof (u32)]; +unsigned int delay_msec =TPM2_DURATION_SHORT ; +u32 rc =0 ; +ssize_t ret; + const size_t save_size = min(space ? sizeof(save) :
TPM_HEADER_SIZE,drivers/char/tpm/tpm-interface.c +5
-0+ bufsiz); + +/* +* Subtlety here: if we have a space, the handles will be +* transformed, so when we restore the header we also have to
drivers/char/tpm/tpm-interface.c +6
-0+* restore the handles. +*/ +memcpy (save, buf, save_size); + +for (;;) { + ret = tpm_try_transmit(chip, space, buf, bufsiz,flags);
drivers/char/tpm/tpm-interface.c +1
-0+if (ret <0 )
Quoted 8 lines
drivers/char/tpm/tpm-interface.c ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space, · line 587
Quoted 5 lines
drivers/char/tpm/tpm-interface.c +5
-0+break ; + rc =be32_to_cpu (header->return_code); +if (rc !=TPM2_RC_RETRY ) +break ; + delay_msec *=2 ;Was wondering if this increment could be moved after
tpm_msleep(delay_msec)?
I thought about that when I saw the logic in the original but what it would do is double the maximum delay (which is already nearly double TPM2_DURATION_LONG). I figured doubling the initial timeout was fine rather than trying to do complex logic to get the delay exact (and then having to litter the file with comments explaining why).
James
Re: [PATCH v2 1/2] tpm: add retry logic
Quoted 58 lines · James Bottomley
Quoted 49 lines · Nayna Jain
Quoted 49 lines
/**+ *
tpm_transmit- Internal kernel interface to transmit TPM commands.
+ *
+ * @chip: TPM chip to use
+ * @space: tpm space
+ * @buf: TPM command buffer
+ * @bufsiz: length of the TPM command buffer
+ * @flags: tpm transmit flags - bitmap
+ *
+ * A wrapper aroundtpm_try_transmitthat handlesTPM2_RC_RETRY
+ * returns from the TPM and retransmits the command after a delay up
+ * to a maximum wait ofTPM2_DURATION_LONG.
+ *
+ * Note: TPM1 never returnsTPM2_RC_RETRYso the retry logic is TPM2
+ * only
+ *
+ * Return:
+ * the length of the return when the operation is successful.
+ * A negative number for system errors (errno).
+ */
+ssize_t tpm_transmit(structtpm_chip*chip, structtpm_space*space,drivers/char/tpm/tpm-interface.c +3
-0+ u8 *buf, size_t bufsiz, unsigned int flags) +{ + struct tpm_output_header *header = (struct
tpm_output_header*)buf;drivers/char/tpm/tpm-interface.c +6
-0+/* space for header and handles */ +u8 save[TPM_HEADER_SIZE +3 *sizeof (u32)]; +unsigned int delay_msec =TPM2_DURATION_SHORT ; +u32 rc =0 ; +ssize_t ret; + const size_t save_size = min(space ? sizeof(save) :
TPM_HEADER_SIZE,drivers/char/tpm/tpm-interface.c +5
-0+ bufsiz); + +/* +* Subtlety here: if we have a space, the handles will be +* transformed, so when we restore the header we also have to
drivers/char/tpm/tpm-interface.c +6
-0+* restore the handles. +*/ +memcpy (save, buf, save_size); + +for (;;) { + ret = tpm_try_transmit(chip, space, buf, bufsiz,flags);
Quoted 8 lines
drivers/char/tpm/tpm-interface.c ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space, · line 587
Quoted 6 lines
drivers/char/tpm/tpm-interface.c +6
-0+if (ret <0 ) +break ; + rc =be32_to_cpu (header->return_code); +if (rc !=TPM2_RC_RETRY ) +break ; + delay_msec *=2 ;Was wondering if this increment could be moved after
tpm_msleep(delay_msec)?
Quoted 5 lines
I thought about that when I saw the logic in the original but what it would do is double the maximum delay (which is already nearly double
TPM2_DURATION_LONG). I figured doubling the initial timeout was fine rather than trying to do complex logic to get the delay exact (and then having to litter the file with comments explaining why).
Sorry James, I didn't understand exactly about what complex logic is involved. Can you please explain it more ? My point was to just move "delay_msec *= 2" after tpm_msleep() as shown below:
if (delay_msec > TPM2_DURATION_LONG) {
dev_err(&chip->dev, "TPM is in retry loop\n");
break;
}tpm_msleep(delay_msec)delay_msec *= 2;
And the first time sleep will use the already initialized value of delay_msec as:
unsigned int delay_msec = TPM2_DURATION_SHORT;
Thanks & Regards,
- Nayna
Quoted 3 lines
James
James Bottomley
maintainerRe: [PATCH v2 1/2] tpm: add retry logic
Quoted 4 lines · Nayna Jain
Quoted 2 lines · James Bottomley
On Thu, 2018-03-22 at 21:43 +0530, Nayna Jain wrote:
[...]
Quoted 50 lines
Quoted 38 lines
Quoted 30 lines
Quoted 30 lines
+ssize_t tpm_transmit(struct
tpm_chip*chip, structtpm_space*space,(quoted diff) +1
-0+ u8 *buf, size_t bufsiz, unsigned intflags)
(quoted diff) +2
-0+{ + struct tpm_output_header *header = (struct
tpm_output_header*)buf;drivers/char/tpm/tpm-interface.c +6
-0+/* space for header and handles */ +u8 save[TPM_HEADER_SIZE +3 *sizeof (u32)]; +unsigned int delay_msec =TPM2_DURATION_SHORT ; +u32 rc =0 ; +ssize_t ret; + const size_t save_size = min(space ? sizeof(save) :
TPM_HEADER_SIZE,drivers/char/tpm/tpm-interface.c +4
-0+ bufsiz); + +/* +* Subtlety here: if we have a space, the handles will be
(quoted diff) +1
-0+ * transformed, so when we restore the header we alsohave
todrivers/char/tpm/tpm-interface.c +6
-0+* restore the handles. +*/ +memcpy (save, buf, save_size); + +for (;;) { + ret = tpm_try_transmit(chip, space, buf,bufsiz,
flags);Quoted 8 lines
drivers/char/tpm/tpm-interface.c ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space, · line 587
Quoted 6 lines
drivers/char/tpm/tpm-interface.c +6
-0+if (ret <0 ) +break ; + rc =be32_to_cpu (header->return_code); +if (rc !=TPM2_RC_RETRY ) +break ; + delay_msec *=2 ;Was wondering if this increment could be moved after
tpm_msleep(delay_msec)?Quoted 6 lines
I thought about that when I saw the logic in the original but what it would do is double the maximum delay (which is already nearly double
TPM2_DURATION_LONG). I figured doubling the initial timeout was fine rather than trying to do complex logic to get the delay exact (and then having to litter the file with comments explaining why).Sorry James, I didn't understand exactly about what complex logic is involved. Can you please explain it more ?
My point was to just move "delay_msec*= 2" aftertpm_msleep()as shown below:
Quoted 6 lines
if (delay_msec > TPM2_DURATION_LONG) { dev_err(&chip->dev, "TPM is in retry loop\n"); break; }
tpm_msleep(delay_msec)delay_msec*= 2;
Yes, I understand the suggestion; however, if you simply do that, you'll end up with a useless sleep at the end of the wait period before you check to see if you've waited too long, which is even more suboptimal than the current situation. The cardinal rule is we should only sleep if we know we're going to retry.
James
Re: [PATCH v2 1/2] tpm: add retry logic
Quoted 63 lines · James Bottomley
Quoted 4 lines · Nayna Jain
Quoted 2 lines · James Bottomley
On Thu, 2018-03-22 at 21:43 +0530, Nayna Jain wrote:
[...]
Quoted 50 lines
Quoted 38 lines
Quoted 30 lines
Quoted 30 lines
+ssize_t tpm_transmit(struct
tpm_chip*chip, structtpm_space*space,(quoted diff) +1
-0+ u8 *buf, size_t bufsiz, unsigned intflags)
(quoted diff) +2
-0+{ + struct tpm_output_header *header = (struct
tpm_output_header*)buf;drivers/char/tpm/tpm-interface.c +6
-0+/* space for header and handles */ +u8 save[TPM_HEADER_SIZE +3 *sizeof (u32)]; +unsigned int delay_msec =TPM2_DURATION_SHORT ; +u32 rc =0 ; +ssize_t ret; + const size_t save_size = min(space ? sizeof(save) :
TPM_HEADER_SIZE,drivers/char/tpm/tpm-interface.c +4
-0+ bufsiz); + +/* +* Subtlety here: if we have a space, the handles will be
(quoted diff) +1
-0+ * transformed, so when we restore the header we alsohave
todrivers/char/tpm/tpm-interface.c +6
-0+* restore the handles. +*/ +memcpy (save, buf, save_size); + +for (;;) { + ret = tpm_try_transmit(chip, space, buf,bufsiz,
flags);Quoted 8 lines
drivers/char/tpm/tpm-interface.c ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space, · line 587
Quoted 6 lines
drivers/char/tpm/tpm-interface.c +6
-0+if (ret <0 ) +break ; + rc =be32_to_cpu (header->return_code); +if (rc !=TPM2_RC_RETRY ) +break ; + delay_msec *=2 ;Was wondering if this increment could be moved after
tpm_msleep(delay_msec)?Quoted 6 lines
I thought about that when I saw the logic in the original but what it would do is double the maximum delay (which is already nearly double
TPM2_DURATION_LONG). I figured doubling the initial timeout was fine rather than trying to do complex logic to get the delay exact (and then having to litter the file with comments explaining why).Sorry James, I didn't understand exactly about what complex logic is involved. Can you please explain it more ?
My point was to just move "delay_msec*= 2" aftertpm_msleep()as shown below:Quoted 6 lines
if (delay_msec > TPM2_DURATION_LONG) { dev_err(&chip->dev, "TPM is in retry loop\n"); break; }
tpm_msleep(delay_msec)delay_msec*= 2;
Quoted 5 lines
Yes, I understand the suggestion; however, if you simply do that, you'll end up with a useless sleep at the end of the wait period before you check to see if you've waited too long, which is even more suboptimal than the current situation. The cardinal rule is we should only sleep if we know we're going to retry.
By the time delay_msec is incremented here, you've already finished sleeping and will resend the command. This is the reason I assume you didn't use a normal for loop:
for (delay_msec = TPM2_DURATION_SHORT; delay_msec <TPM2_DURATION_LONG;
delay_msec *= 2)Incrementing the sleep here will only affect the next sleep.
Mimi
James Bottomley
maintainerRe: [PATCH v2 1/2] tpm: add retry logic
Quoted 95 lines · Mimi Zohar
Quoted 81 lines · James Bottomley
Quoted 6 lines · Nayna Jain
Quoted 3 lines · James Bottomley
On Thu, 2018-03-22 at 21:43 +0530, Nayna Jain wrote:
[...]
Quoted 65 lines
Quoted 46 lines
Quoted 38 lines
Quoted 38 lines
+ssize_t tpm_transmit(struct
tpm_chip*chip, structtpm_space
*space,(quoted diff) +1
-0+ u8 *buf, size_t bufsiz, unsigned intflags)
(quoted diff) +2
-0+{ + struct tpm_output_header *header = (struct
tpm_output_header*)buf;drivers/char/tpm/tpm-interface.c +6
-0+/* space for header and handles */ +u8 save[TPM_HEADER_SIZE +3 *sizeof (u32)]; +unsigned int delay_msec =TPM2_DURATION_SHORT ; +u32 rc =0 ; +ssize_t ret; + const size_t save_size = min(space ? sizeof(save):
TPM_HEADER_SIZE,drivers/char/tpm/tpm-interface.c +4
-0+ bufsiz); + +/* +* Subtlety here: if we have a space, the handles will
be(quoted diff) +1
-0+ * transformed, so when we restore the header wealso
have
todrivers/char/tpm/tpm-interface.c +6
-0+* restore the handles. +*/ +memcpy (save, buf, save_size); + +for (;;) { + ret = tpm_try_transmit(chip, space, buf,bufsiz,
flags);Quoted 8 lines
drivers/char/tpm/tpm-interface.c ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space, · line 587
Quoted 6 lines
drivers/char/tpm/tpm-interface.c +6
-0+if (ret <0 ) +break ; + rc =be32_to_cpu (header->return_code); +if (rc !=TPM2_RC_RETRY ) +break ; + delay_msec *=2 ;Was wondering if this increment could be moved after
tpm_msleep(delay_msec)?Quoted 11 lines
I thought about that when I saw the logic in the original but what
it would do is double the maximum delay (which is already nearly
doubleTPM2_DURATION_LONG). I figured doubling the initial timeout
was fine rather than trying to do complex logic to get the delay
exact (and then having to litter the file with comments explaining
why).Sorry James, I didn't understand exactly about what complex logic is
involved. Can you please explain it more ?
My point was to just move "delay_msec*= 2" aftertpm_msleep()as
shown below:Quoted 6 lines
if (delay_msec > TPM2_DURATION_LONG) { dev_err(&chip->dev, "TPM is in retry loop\n"); break; }
tpm_msleep(delay_msec)delay_msec*= 2;Quoted 7 lines
Yes, I understand the suggestion; however, if you simply do that, you'll end up with a useless sleep at the end of the wait period before
you check to see if you've waited too long, which is even more suboptimal than the current situation. The cardinal rule is we should
only sleep if we know we're going to retry.By the time
delay_msecis incremented here, you've already finished sleeping and will resend the command. This is the reason I assume you
didn't use a normal for loop:
Quoted 5 lines
for (
delay_msec=TPM2_DURATION_SHORT;delay_msec<TPM2_DURATION_LONG; delay_msec *= 2)Incrementing the sleep here will only affect the next sleep.
Yes, looking at it again, that seems to be true. I remember thinking the same of the original code when I looked at it, but saw there was some problem. However, it's so long ago, I can't remember what it actually was and I can't see it now.
James
Re: [PATCH v2 1/2] tpm: add retry logic
Quoted 100 lines · James Bottomley
Quoted 99 lines · Mimi Zohar
Quoted 81 lines · James Bottomley
Quoted 6 lines · Nayna Jain
Quoted 3 lines · James Bottomley
On Thu, 2018-03-22 at 21:43 +0530, Nayna Jain wrote:
[...]
Quoted 65 lines
Quoted 46 lines
Quoted 38 lines
Quoted 38 lines
+ssize_t tpm_transmit(struct
tpm_chip*chip, structtpm_space
*space,(quoted diff) +1
-0+ u8 *buf, size_t bufsiz, unsigned intflags)
(quoted diff) +2
-0+{ + struct tpm_output_header *header = (struct
tpm_output_header*)buf;(quoted diff) +6
-0+ /* space for header and handles */ + u8 save[TPM_HEADER_SIZE + 3*sizeof(u32)]; + unsigned int delay_msec = TPM2_DURATION_SHORT; + u32 rc = 0; + ssize_t ret; + const size_t save_size = min(space ? sizeof(save):
TPM_HEADER_SIZE,(quoted diff) +4
-0+ bufsiz); + + /* + * Subtlety here: if we have a space, the handleswill
be(quoted diff) +1
-0+ * transformed, so when we restore the header wealso
have
to(quoted diff) +6
-0+ * restore the handles. + */ + memcpy(save, buf, save_size); + + for (;;) { + ret = tpm_try_transmit(chip, space, buf,bufsiz,
flags);Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+ if (ret < 0) + break; + rc = be32_to_cpu(header->return_code); + if (rc != TPM2_RC_RETRY) + break; + delay_msec *= 2;Was wondering if this increment could be moved after
tpm_msleep(delay_msec)?Quoted 11 lines
I thought about that when I saw the logic in the original but what
it would do is double the maximum delay (which is already nearly
doubleTPM2_DURATION_LONG). I figured doubling the initial timeout
was fine rather than trying to do complex logic to get the delay
exact (and then having to litter the file with comments explaining
why).Sorry James, I didn't understand exactly about what complex logic is
involved. Can you please explain it more ?
My point was to just move "delay_msec*= 2" aftertpm_msleep()as
shown below:Quoted 6 lines
if (delay_msec > TPM2_DURATION_LONG) { dev_err(&chip->dev, "TPM is in retry loop\n"); break; }
tpm_msleep(delay_msec)delay_msec*= 2;Quoted 7 lines
Yes, I understand the suggestion; however, if you simply do that, you'll end up with a useless sleep at the end of the wait period before
you check to see if you've waited too long, which is even more suboptimal than the current situation. The cardinal rule is we should
only sleep if we know we're going to retry.By the time
delay_msecis incremented here, you've already finished sleeping and will resend the command. This is the reason I assume you
didn't use a normal for loop:for (
delay_msec=TPM2_DURATION_SHORT;delay_msec<TPM2_DURATION_LONG; delay_msec *= 2)
Quoted 6 lines
Quoted 1 line
Incrementing the sleep here will only affect the next sleep.
Yes, looking at it again, that seems to be true. I remember thinking the same of the original code when I looked at it, but saw there was some problem. However, it's so long ago, I can't remember what it actually was and I can't see it now.
As this patch is already in the security's next-testing branch, it's too late to change it. I assume Nayna should post a patch that moves it.
Mimi