| Message ID | Z8gdKxsw2unC5UID@earth.li (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | tpm, tpm_tis: Fix timeout handling when waiting for TPM status | expand |
On Wed, Mar 05, 2025 at 09:45:15AM +0000, Jonathan McDowell wrote: > From: Jonathan McDowell <noodles@meta.com> > > The change to only use interrupts to handle supported status changes, > then switch to polling for the rest, inverted the status test and sleep > such that we can end up sleeping beyond our timeout and not actually > checking the status. This can result in spurious TPM timeouts, I *really* have hard time understanding what I'm reading the first sentence *but* I do understand the code change. Maybe you could try to be a bit more punctual there... > especially on a more loaded system. Fix by switching the order back so > we sleep *then* check. We've done a up front check when we enter the > function so this won't cause an additional delay when the status is > already what we're looking for. Remove the use of we-pronoun: it is best for science papers. Also, e.g. why not just say "switch" instead of "fix by switching" ? :-) I'd rewrite this tail part like: "Switch the order back where it was i.e., sleep before check." Instead of "upfront check" it'd nice to be a bit more specific... > > Cc: stable@vger.kernel.org # v6.4+ > Fixes: e87fcf0dc2b4 ("tpm, tpm_tis: Only handle supported interrupts") > Signed-off-by: Jonathan McDowell <noodles@meta.com> > Reviewed-by: Michal Suchánek <msuchanek@suse.de> > --- > drivers/char/tpm/tpm_tis_core.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c > index fdef214b9f6b..167d71747666 100644 > --- a/drivers/char/tpm/tpm_tis_core.c > +++ b/drivers/char/tpm/tpm_tis_core.c > @@ -114,11 +114,11 @@ static int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask, > return 0; > /* process status changes without irq support */ > do { > + usleep_range(priv->timeout_min, > + priv->timeout_max); > status = chip->ops->status(chip); > if ((status & mask) == mask) > return 0; > - usleep_range(priv->timeout_min, > - priv->timeout_max); > } while (time_before(jiffies, stop)); > return -ETIME; > } > -- > 2.48.1 > > The fix itself looks legit although you could just as well put it into a single line. BR, Jarkko
Hi Jonathan, On 05.03.25 10:45, Jonathan McDowell wrote: > From: Jonathan McDowell <noodles@meta.com> > > The change to only use interrupts to handle supported status changes, > then switch to polling for the rest, inverted the status test and sleep > such that we can end up sleeping beyond our timeout and not actually > checking the status. This can result in spurious TPM timeouts, > especially on a more loaded system. Fix by switching the order back so > we sleep *then* check. We've done a up front check when we enter the > function so this won't cause an additional delay when the status is > already what we're looking for. > > Cc: stable@vger.kernel.org # v6.4+ > Fixes: e87fcf0dc2b4 ("tpm, tpm_tis: Only handle supported interrupts") > Signed-off-by: Jonathan McDowell <noodles@meta.com> > Reviewed-by: Michal Suchánek <msuchanek@suse.de> > --- > drivers/char/tpm/tpm_tis_core.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c > index fdef214b9f6b..167d71747666 100644 > --- a/drivers/char/tpm/tpm_tis_core.c > +++ b/drivers/char/tpm/tpm_tis_core.c > @@ -114,11 +114,11 @@ static int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask, > return 0; > /* process status changes without irq support */ > do { > + usleep_range(priv->timeout_min, > + priv->timeout_max); > status = chip->ops->status(chip); > if ((status & mask) == mask) > return 0; > - usleep_range(priv->timeout_min, > - priv->timeout_max); > } while (time_before(jiffies, stop)); > return -ETIME; > } > -- > 2.48.1 > FWIW: Reviewed-by: Lino Sanfilippo <l.sanfilippo@kunbus.com> I cannot remember any more but I think the change of the logic in the while loop must have been a leftover from some tests I did at this time. However it should not have been part of the patch, so good that you found it and thanks for the fix! BR, Lino
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c index fdef214b9f6b..167d71747666 100644 --- a/drivers/char/tpm/tpm_tis_core.c +++ b/drivers/char/tpm/tpm_tis_core.c @@ -114,11 +114,11 @@ static int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask, return 0; /* process status changes without irq support */ do { + usleep_range(priv->timeout_min, + priv->timeout_max); status = chip->ops->status(chip); if ((status & mask) == mask) return 0; - usleep_range(priv->timeout_min, - priv->timeout_max); } while (time_before(jiffies, stop)); return -ETIME; }