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.
> > 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(...);
/* ... */
BR, Jarkko
