Re: [PATCH v3] tpm: Call cmd_ready/go_idle for each command transmission

[Jarkko Sakkinen <jarkko@kernel.org>]

On Wed, Sep 09, 2026 at 11:08:59AM -0500, Mario Limonciello wrote:
> 
> 
> On 9/1/26 17:55, Jarkko Sakkinen wrote:
> > On Tue, Sep 01, 2026 at 04:37:22PM -0500, Mario Limonciello wrote:
> > > Some TPM implementations, particularly fTPM using the CRB interface,
> > > require the TPM to transition through idle and ready states for each
> > > command rather than once per session.
> > > 
> > > The current implementation calls cmd_ready once during tpm_chip_start()
> > > and go_idle once during tpm_chip_stop(). For fTPM, when multiple commands
> > > are sent without per-command idle transitions, subsequent commands timeout
> > > as the TPM is waiting for the transition.
> > > 
> > > Fix this by calling cmd_ready before each command and go_idle on all exit
> > > paths in tpm_try_transmit(). For TPM implementations that don't require
> > > per-command transitions, the callbacks return immediately based on the
> > > start method.
> > > 
> > > Remove the now-redundant per-session calls from tpm_chip_start() and
> > > tpm_chip_stop() along with their helpers.
> > > 
> > > This resolves timeout errors during TPM initialization on systems where
> > > BIOS has already performed TPM startup.
> > > 
> > > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> > > ---
> > > v3:
> > >   * Use __free() to clean up
> > > v2:
> > >   * Drop redundant calls
> > >   * Update commit message
> > > ---
> > >   drivers/char/tpm/tpm-chip.c      | 24 ------------------------
> > >   drivers/char/tpm/tpm-interface.c | 21 +++++++++++++++++++++
> > >   2 files changed, 21 insertions(+), 24 deletions(-)
> > > 
> > > diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> > > index 12b7394b34bdc..0be5dbfaa72eb 100644
> > > --- a/drivers/char/tpm/tpm-chip.c
> > > +++ b/drivers/char/tpm/tpm-chip.c
> > > @@ -66,22 +66,6 @@ static void tpm_relinquish_locality(struct tpm_chip *chip)
> > >   	chip->locality = -1;
> > >   }
> > > -static int tpm_cmd_ready(struct tpm_chip *chip)
> > > -{
> > > -	if (!chip->ops->cmd_ready)
> > > -		return 0;
> > > -
> > > -	return chip->ops->cmd_ready(chip);
> > > -}
> > > -
> > > -static int tpm_go_idle(struct tpm_chip *chip)
> > > -{
> > > -	if (!chip->ops->go_idle)
> > > -		return 0;
> > > -
> > > -	return chip->ops->go_idle(chip);
> > > -}
> > > -
> > >   static void tpm_clk_enable(struct tpm_chip *chip)
> > >   {
> > >   	if (chip->ops->clk_enable)
> > > @@ -116,13 +100,6 @@ int tpm_chip_start(struct tpm_chip *chip)
> > >   		}
> > >   	}
> > > -	ret = tpm_cmd_ready(chip);
> > > -	if (ret) {
> > > -		tpm_relinquish_locality(chip);
> > > -		tpm_clk_disable(chip);
> > > -		return ret;
> > > -	}
> > > -
> > >   	return 0;
> > >   }
> > >   EXPORT_SYMBOL_GPL(tpm_chip_start);
> > > @@ -137,7 +114,6 @@ EXPORT_SYMBOL_GPL(tpm_chip_start);
> > >    */
> > >   void tpm_chip_stop(struct tpm_chip *chip)
> > >   {
> > > -	tpm_go_idle(chip);
> > >   	tpm_relinquish_locality(chip);
> > >   	tpm_clk_disable(chip);
> > >   }
> > > diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> > > index f745a098908b3..8d434d7fa5bab 100644
> > > --- a/drivers/char/tpm/tpm-interface.c
> > > +++ b/drivers/char/tpm/tpm-interface.c
> > > @@ -19,6 +19,7 @@
> > >    * calls to msleep.
> > >    */
> > > +#include <linux/cleanup.h>
> > >   #include <linux/poll.h>
> > >   #include <linux/slab.h>
> > >   #include <linux/mutex.h>
> > > @@ -89,8 +90,16 @@ static bool tpm_transmit_completed(u8 status, struct tpm_chip *chip)
> > >   	return status_masked == chip->ops->req_complete_val;
> > >   }
> > > +static void tpm_go_idle(struct tpm_chip *chip)
> > > +{
> > > +	if (chip->ops->go_idle)
> > > +		chip->ops->go_idle(chip);
> > > +}
> > > +DEFINE_FREE(tpm_go_idle, struct tpm_chip *, if (_T) tpm_go_idle(_T))
> > > +
> > >   static ssize_t tpm_try_transmit(struct tpm_chip *chip, void *buf, size_t bufsiz)
> > >   {
> > > +	struct tpm_chip *chip_idle __free(tpm_go_idle) = NULL;
> > >   	struct tpm_header *header = buf;
> > >   	int rc;
> > >   	ssize_t len = 0;
> > > @@ -113,6 +122,18 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip, void *buf, size_t bufsiz)
> > >   		return -E2BIG;
> > >   	}
> > > +	if (chip->ops->cmd_ready) {
> > > +		rc = chip->ops->cmd_ready(chip);
> > > +		if (rc) {
> > > +			dev_err(&chip->dev,
> > > +				"%s: cmd_ready(): error %d\n", __func__, rc);
> > > +			return rc;
> > > +		}
> > > +	}
> > > +
> > > +	/* Ensure go_idle() is called on every exit path from here on. */
> > > +	chip_idle = chip;
> > > +
> > >   	rc = chip->ops->send(chip, buf, bufsiz, count);
> > >   	if (rc < 0) {
> > >   		if (rc != -EPIPE)
> > > -- 
> > > 2.43.0
> > > 
> > 
> > This is now much better as it addition adds better guarantees that
> > resources always get released after use no matter how this function
> > is modified later on.
> > 
> > Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
> > 
> > BR, Jarkko
> 
> Thanks.  If no other concerns, can this be picked up now?
> 

Sorry for the delay, done

BR, Jarkko