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

[Mario Limonciello <mario.limonciello@amd.com>]

On 9/1/26 12:37, Jarkko Sakkinen wrote:
> On Tue, Sep 01, 2026 at 11:32:15AM -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>
>> ---
>> v2:
>>   * Drop redundant calls
>>   * Update commit message
>> ---
>>   drivers/char/tpm/tpm-chip.c      | 24 ------------------------
>>   drivers/char/tpm/tpm-interface.c | 26 ++++++++++++++++++++++----
>>   2 files changed, 22 insertions(+), 28 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..c5b5b5ba41821 100644
>> --- a/drivers/char/tpm/tpm-interface.c
>> +++ b/drivers/char/tpm/tpm-interface.c
>> @@ -113,12 +113,21 @@ 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;
>> +		}
>> +	}
> 
> I'd consider retaining tpm_go_idle and do this after arming the command:
> 
> 	chip = chip __free(tpm_go_idle);
> 

Good idea; that would be a lot less cleanup changes.  I'll modify and test.

>> +
>>   	rc = chip->ops->send(chip, buf, bufsiz, count);
>>   	if (rc < 0) {
>>   		if (rc != -EPIPE)
>>   			dev_err(&chip->dev,
>>   				"%s: send(): error %d\n", __func__, rc);
>> -		return rc;
>> +		goto out;
>>   	}
>>   
>>   	/*
>> @@ -152,7 +161,8 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip, void *buf, size_t bufsiz)
>>   
>>   		if (tpm_chip_req_canceled(chip, status)) {
>>   			dev_err(&chip->dev, "Operation Canceled\n");
>> -			return -ECANCELED;
>> +			rc = -ECANCELED;
>> +			goto out;
>>   		}
>>   
>>   		tpm_msleep(TPM_TIMEOUT_POLL);
>> @@ -168,19 +178,27 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip, void *buf, size_t bufsiz)
>>   
>>   	tpm_chip_cancel(chip);
>>   	dev_err(&chip->dev, "Operation Timed out\n");
>> -	return -ETIME;
>> +	rc = -ETIME;
>> +	goto out;
>>   
>>   out_recv:
>>   	len = chip->ops->recv(chip, buf, bufsiz);
>>   	if (len < 0) {
>>   		rc = len;
>>   		dev_err(&chip->dev, "tpm_transmit: tpm_recv: error %d\n", rc);
>> -		return rc;
>> +		goto out;
>>   	}
>>   out_sync:
>>   	if (len < TPM_HEADER_SIZE || len != be32_to_cpu(header->length))
>>   		rc = -EFAULT;
>>   
>> +out:
>> +	if (chip->ops->go_idle) {
>> +		int idle_rc = chip->ops->go_idle(chip);
>> +		if (idle_rc && !rc)
>> +			rc = idle_rc;
>> +	}
>> +
>>   	return rc ? rc : len;
>>   }
>>   
>> -- 
>> 2.43.0
>>
> 
> BR, Jarkko