Re: [PATCH v4 5/5] crypto: ccp - Send PSP_CMD_TEE_RING_DESTROY when PSP_CMD_TEE_RING_INIT fails

[Mario Limonciello <superm1@kernel.org>]

On 1/7/26 3:04 AM, Shyam Sundar S K wrote:
> 
> 
> On 1/6/2026 10:24, Mario Limonciello (AMD) wrote:
>> The hibernate resume sequence involves loading a resume kernel that is just
>> used for loading the hibernate image before shifting back to the existing
>> kernel.
>>
>> During that hibernate resume sequence the resume kernel may have loaded
>> the ccp driver.  If this happens the resume kernel will also have called
>> PSP_CMD_TEE_RING_INIT but it will never have called
>> PSP_CMD_TEE_RING_DESTROY.
>>
>> This is problematic because the existing kernel needs to re-initialize the
>> ring.  One could argue that the existing kernel should call destroy
>> as part of restore() but there is no guarantee that the resume kernel did
>> or didn't load the ccp driver.  There is also no callback opportunity for
>> the resume kernel to destroy before handing back control to the existing
>> kernel.
>>
>> Similar problems could potentially exist with the use of kdump and
>> crash handling. I actually reproduced this issue like this:
>>
>> 1) rmmod ccp
>> 2) hibernate the system
>> 3) resume the system
>> 4) modprobe ccp
>>
>> The resume kernel will have loaded ccp but never destroyed and then when
>> I try to modprobe it fails.
>>
>> Because of these possible cases add a flow that checks the error code from
>> the PSP_CMD_TEE_RING_INIT call and tries to call PSP_CMD_TEE_RING_DESTROY
>> if it failed.  If this succeeds then call PSP_CMD_TEE_RING_INIT again.
>>
>> Fixes: f892a21f51162 ("crypto: ccp - use generic power management")
>> Reported-by: Lars Francke <lars.francke@gmail.com>
>> Closes: https://lore.kernel.org/platform-driver-x86/CAD-Ua_gfJnQSo8ucS_7ZwzuhoBRJ14zXP7s8b-zX3ZcxcyWePw@mail.gmail.com/
>> Tested-by: Yijun Shen <Yijun.Shen@Dell.com>
>> Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
>> ---
>> v4:
>>   * Add tag (Yijun)
>>   * Move and rename PSP_TEE_STS_RING_BUSY (Ilpo)
>> v3:
>>   * Add a comment (Tom)
>>   * Add a define for busy condition (Shyam)
>>   * Rename label (Shyam)
>>   * Upgrade message to info (Shyam)
>>   * Use a helper that validates result for destroy command (Shyam)
>> ---
>>   drivers/crypto/ccp/tee-dev.c | 12 ++++++++++++
>>   include/linux/psp.h          |  1 +
>>   2 files changed, 13 insertions(+)
>>
>> diff --git a/drivers/crypto/ccp/tee-dev.c b/drivers/crypto/ccp/tee-dev.c
>> index ef1430f86ad62..ea9b94d5b10ba 100644
>> --- a/drivers/crypto/ccp/tee-dev.c
>> +++ b/drivers/crypto/ccp/tee-dev.c
>> @@ -113,6 +113,7 @@ static int tee_init_ring(struct psp_tee_device *tee)
>>   {
>>   	int ring_size = MAX_RING_BUFFER_ENTRIES * sizeof(struct tee_ring_cmd);
>>   	struct tee_init_ring_cmd *cmd;
>> +	bool retry = false;
>>   	unsigned int reg;
>>   	int ret;
>>   
>> @@ -135,6 +136,7 @@ static int tee_init_ring(struct psp_tee_device *tee)
>>   	/* Send command buffer details to Trusted OS by writing to
>>   	 * CPU-PSP message registers
>>   	 */
>> +retry_init:
>>   	ret = psp_mailbox_command(tee->psp, PSP_CMD_TEE_RING_INIT, cmd,
>>   				  TEE_DEFAULT_CMD_TIMEOUT, &reg);
>>   	if (ret) {
>> @@ -145,6 +147,16 @@ static int tee_init_ring(struct psp_tee_device *tee)
>>   	}
>>   
>>   	if (FIELD_GET(PSP_CMDRESP_STS, reg)) {
>> +		/*
>> +		 * During the hibernate resume sequence driver may have gotten loaded
>> +		 * but the ring not properly destroyed. If the ring doesn't work, try
>> +		 * to destroy and re-init once.
>> +		 */
>> +		if (!retry && FIELD_GET(PSP_CMDRESP_STS, reg) == PSP_TEE_STS_RING_BUSY) {
>> +			dev_info(tee->dev, "tee: ring init command failed with busy status, retrying\n");
>> +			if (tee_send_destroy_cmd(tee))
> 
> so it becomes infinite retry? I think we need to set the retry flag to
> true to indicate that ring busy.

Great catch, thanks.  I'll add the retry=true for this case.

> 
>> +				goto retry_init;
>> +		}
>>   		dev_err(tee->dev, "tee: ring init command failed (%#010lx)\n",
>>   			FIELD_GET(PSP_CMDRESP_STS, reg));
>>   		tee_free_ring(tee);
>> diff --git a/include/linux/psp.h b/include/linux/psp.h
>> index 92e60aeef21e1..b337dcce1e991 100644
>> --- a/include/linux/psp.h
>> +++ b/include/linux/psp.h
>> @@ -18,6 +18,7 @@
>>    * and should include an appropriate local definition in their source file.
>>    */
>>   #define PSP_CMDRESP_STS		GENMASK(15, 0)
>> +#define  PSP_TEE_STS_RING_BUSY 0x0000000d  /* Ring already initialized */
> 
> additional spaces between the macro names.

This was actually intended.  I wanted to make it obvious that 
PSP_TEE_STS_RING_BUSY reflects a value used for PSP_CMDRESP_STS.

> 
> Thanks,
> Shyam
> 
>>   #define PSP_CMDRESP_CMD		GENMASK(23, 16)
>>   #define PSP_CMDRESP_RESERVED	GENMASK(29, 24)
>>   #define PSP_CMDRESP_RECOVERY	BIT(30)
> 
>