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

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 5389 bytes --]

On Mon, 15 Dec 2025, Mario Limonciello wrote:

> On 12/15/25 6:01 AM, Ilpo Järvinen wrote:
> > On Sun, 14 Dec 2025, 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/
> > > Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
> > > ---
> > > 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          |  2 ++
> > >   2 files changed, 14 insertions(+)
> > > 
> > > diff --git a/drivers/crypto/ccp/tee-dev.c b/drivers/crypto/ccp/tee-dev.c
> > > index ef1430f86ad62..9edb220abbc1a 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_STATUS_RING_BUSY) {
> > > +			dev_info(tee->dev, "tee: ring init command failed with
> > > busy status, retrying\n");
> > > +			if (tee_send_destroy_cmd(tee))
> > > +				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..a329148e3684b 100644
> > > --- a/include/linux/psp.h
> > > +++ b/include/linux/psp.h
> > > @@ -23,6 +23,8 @@
> > >   #define PSP_CMDRESP_RECOVERY	BIT(30)
> > >   #define PSP_CMDRESP_RESP	BIT(31)
> > >   +#define PSP_TEE_STATUS_RING_BUSY 0x0000000d  /* Ring already
> > > initialized */
> > 
> > It would be better to have this right underneath PSP_CMDRESP_STS (you
> > can use one extra space to indent different from the mask and bits).
> > 
> > Also, there's inconsistency in STS vs STATUS in the naming.
> 
> OK - to make sure I get it like you are suggesting for next spin, you mean
> like this right?
>
> diff --git a/include/linux/psp.h b/include/linux/psp.h
> index 92e60aeef21e..b337dcce1e99 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 */
>  #define PSP_CMDRESP_CMD                GENMASK(23, 16)
>  #define PSP_CMDRESP_RESERVED   GENMASK(29, 24)
>  #define PSP_CMDRESP_RECOVERY   BIT(30)

Yes.

-- 
 i.