| Message ID | Z87ZcPZvoUXZ7M_f@earth.li (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] tpm, tpm_tis: Fix timeout handling when waiting for TPM status | expand |
On Mon, Mar 10, 2025 at 12:22:08PM +0000, Jonathan McDowell wrote: > The change to only use interrupts to handle supported status changes > introduced an issue when it is necessary to poll for the status. Rather > than checking for the status after sleeping the code now sleeps after > the check. This means a correct, but slower, status change on the part > of the TPM can be missed, resulting in a spurious timeout error, > especially on a more loaded system. Switch back to sleeping *then* > checking. An up front check of the status has been done at the start of > the function, so this does not 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> > Reviewed-by: Lino Sanfilippo <l.sanfilippo@kunbus.com> > --- > v2: Reword commit message > Don't needlessly wrap line > > drivers/char/tpm/tpm_tis_core.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c > index fdef214b9f6b..c969a1793184 100644 > --- a/drivers/char/tpm/tpm_tis_core.c > +++ b/drivers/char/tpm/tpm_tis_core.c > @@ -114,11 +114,10 @@ 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 > Probably apply late this and timeout fix late this week. Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org> BR, Jarkko
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c index fdef214b9f6b..c969a1793184 100644 --- a/drivers/char/tpm/tpm_tis_core.c +++ b/drivers/char/tpm/tpm_tis_core.c @@ -114,11 +114,10 @@ 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; }
The change to only use interrupts to handle supported status changes introduced an issue when it is necessary to poll for the status. Rather than checking for the status after sleeping the code now sleeps after the check. This means a correct, but slower, status change on the part of the TPM can be missed, resulting in a spurious timeout error, especially on a more loaded system. Switch back to sleeping *then* checking. An up front check of the status has been done at the start of the function, so this does not 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> Reviewed-by: Lino Sanfilippo <l.sanfilippo@kunbus.com> --- v2: Reword commit message Don't needlessly wrap line drivers/char/tpm/tpm_tis_core.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)