On Thu, Feb 06, 2025 at 10:35:32PM +0200, Jarkko Sakkinen wrote: > On Wed, Feb 05, 2025 at 02:29:36PM +0000, Jonathan McDowell wrote: > > Interesting. TPM2_CC_CONTEXT_LOAD (353) / TPM2_CC_FLUSH_CONTEXT (357) / > > TPM2_CC_CONTEXT_SAVE (354) I kinda expect to maybe take a bit longer, > > but TPM2_CC_GET_RANDOM (379) is a little surprising. > > The whole arithmetic with timeout_a/b/c is mostly gibberish and could > be replaced with a single "max" constant without issues (just set it > large enough). > > They could be all be replaced with let's say 3s timeout in a constant. This appears to have come up before: https://lore.kernel.org/linux-integrity/358e89ed2b766d51b5f57abf31ab7a925ac63379.1552348123.git.calvinowens@xxxxxx/ That patch was deemed overly complex and it was suggested to split it up; I can't find any indication that was ever done which I guess is why the discussion died off. > > > Failure is observed with another chip type as well: > > > > > > localhost kernel: tpm_tis MSFT0101:00: 2.0 TPM (device-id 0x1B, rev-id > > > 22) > > > > > > TPM Device > > > Vendor ID: IFX > > > Specification Version: 2.0 > > > Firmware Revision: 7.83 > > > Description: TPM 2.0, ManufacturerID: IFX , Firmware Version: 7.83.3358.0 > > > Characteristics: > > > Family configurable via firmware update > > > Family configurable via OEM proprietary mechanism > > > OEM-specific Information: 0x00000000 > > > > That looks like an SLB9670, not running the latest firmware (7.85). I > > think that might have the errata I've been trying to work around; my > > current patch in testing (with logging to see how effective it is): > > > > commit d8c680ec34e7f42f731e7d64605a670fb7b3b4d1 > > Author: Jonathan McDowell <noodles@xxxxxxxx> > > Date: Mon Aug 19 09:22:46 2024 -0700 > > > > tpm: Workaround failed command reception on Infineon devices > > > > Some Infineon devices have a issue where the status register will get > > stuck with a quick REQUEST_USE / COMMAND_READY sequence. The work around > > is to retry the command submission. Add appropriate logic to do this in > > the send path. > > > > diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c > > index fdef214b9f6b..561d6801e299 100644 > > --- a/drivers/char/tpm/tpm_tis_core.c > > +++ b/drivers/char/tpm/tpm_tis_core.c > > @@ -464,7 +464,12 @@ static int tpm_tis_send_data(struct tpm_chip *chip, const u8 *buf, size_t len) > > > > if (wait_for_tpm_stat(chip, TPM_STS_VALID, chip->timeout_c, > > &priv->int_queue, false) < 0) { > > - rc = -ETIME; > > + if (test_bit(TPM_TIS_STATUS_WORKAROUND, &priv->flags)) { > > + dev_err(&chip->dev, "Timed out waiting for status valid in send, retrying\n"); > > + rc = -EAGAIN; > > > I'm not sure why wait_for_tpm_stat() return value is ignored but it > should not be like that. E.g. it can return -ERESTARTSYS. Probably > would be better to check all the call sites for it that they do > same thing. > > I.e. rc = wait_for_tpm_stat(...); > > /* ... */ So just to clarify, this more recent patch is working around a situation where the status register gets stuck and needs a complete retry of the command send - it's an Infineon errata, not something that would be fixed with a longer timeout. We see what looks like Michal's issue with timeouts as well, I just haven't made the step of instrumenting it all the way he has. J. -- I have found the monster - the | .''`. Debian GNU/Linux Developer monster is us. | : :' : Happy to accept PGP signed | `. `' or encrypted mail - RSA | `- key on the keyservers.
