Re: [PATCH v5 3/5] crypto: ccp - Add an S4 restore flow

[Tom Lendacky <thomas.lendacky@amd.com>]

On 1/12/26 15:25, Mario Limonciello wrote:
> On 1/12/26 10:33 AM, Tom Lendacky wrote:
>> On 1/11/26 23:36, Mario Limonciello (AMD) wrote:
>>> The system will have lost power during S4.  The ring used for TEE
>>> communications needs to be initialized before use.
>>>
>>> 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>
>>> ---
>>> v5:
>>>   * Fix return for psp_restore() (LKP robot, Shyam)
>>> v4:
>>>   * Add tag (Yijun)
>>>   * Remove blank line (Ilpo)
>>>   * Adjust whitespace in psp_restore() (Ilpo)
>>> ---
>>>   drivers/crypto/ccp/psp-dev.c | 11 +++++++++++
>>>   drivers/crypto/ccp/sp-dev.h  |  2 ++
>>>   drivers/crypto/ccp/sp-pci.c  | 24 +++++++++++++++++++++++-
>>>   drivers/crypto/ccp/tee-dev.c |  5 +++++
>>>   drivers/crypto/ccp/tee-dev.h |  1 +
>>>   5 files changed, 42 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
>>> index 9e21da0e298ad..5c7f7e02a7d8a 100644
>>> --- a/drivers/crypto/ccp/psp-dev.c
>>> +++ b/drivers/crypto/ccp/psp-dev.c
>>> @@ -351,6 +351,17 @@ struct psp_device *psp_get_master_device(void)
>>>       return sp ? sp->psp_data : NULL;
>>>   }
>>>   +int psp_restore(struct sp_device *sp)
>>> +{
>>> +    struct psp_device *psp = sp->psp_data;
>>> +    int ret = 0;
>>> +
>>> +    if (psp->tee_data)
>>> +        ret = tee_restore(psp);
>>> +
>>> +    return ret;
>>> +}
>>> +
>>>   void psp_pci_init(void)
>>>   {
>>>       psp_master = psp_get_master_device();
>>> diff --git a/drivers/crypto/ccp/sp-dev.h b/drivers/crypto/ccp/sp-dev.h
>>> index 1335a83fe052e..0deea1a399e47 100644
>>> --- a/drivers/crypto/ccp/sp-dev.h
>>> +++ b/drivers/crypto/ccp/sp-dev.h
>>> @@ -174,6 +174,7 @@ int psp_dev_init(struct sp_device *sp);
>>>   void psp_pci_init(void);
>>>   void psp_dev_destroy(struct sp_device *sp);
>>>   void psp_pci_exit(void);
>>> +int psp_restore(struct sp_device *sp);
>>>     #else /* !CONFIG_CRYPTO_DEV_SP_PSP */
>>>   @@ -181,6 +182,7 @@ static inline int psp_dev_init(struct sp_device
>>> *sp) { return 0; }
>>>   static inline void psp_pci_init(void) { }
>>>   static inline void psp_dev_destroy(struct sp_device *sp) { }
>>>   static inline void psp_pci_exit(void) { }
>>> +static inline int psp_restore(struct sp_device *sp) { return 0; }
>>>     #endif /* CONFIG_CRYPTO_DEV_SP_PSP */
>>>   diff --git a/drivers/crypto/ccp/sp-pci.c b/drivers/crypto/ccp/sp-pci.c
>>> index 8891ceee1d7d0..931ec6bf2cec6 100644
>>> --- a/drivers/crypto/ccp/sp-pci.c
>>> +++ b/drivers/crypto/ccp/sp-pci.c
>>> @@ -353,6 +353,21 @@ static int __maybe_unused sp_pci_resume(struct
>>> device *dev)
>>>       return sp_resume(sp);
>>>   }
>>>   +static int __maybe_unused sp_pci_restore(struct device *dev)
>>> +{
>>> +    struct sp_device *sp = dev_get_drvdata(dev);
>>> +
>>> +#ifdef CONFIG_CRYPTO_DEV_SP_PSP
>>
>> This isn't needed since you have the "#else" definition in sp-dev.h.
> 
> Ack.
> 
>>
>>> +    if (sp->psp_data) {
>>> +        int ret = psp_restore(sp);
>>> +
>>
>> Remove blank line.
> 
> Then checkpatch complains:
> 
> Missing a blank line after declarations

Ah, yeah, bummer...
> 
>>
>>> +        if (ret)
>>> +            return ret;
>>> +    }
>>> +#endif
>>
>> This isn't the way the other functions work. This function should just
>> be calling to sp_resume() and then sp_resume() should call psp_restore()
>> (or psp_dev_restore() ?) if the psp_data is present.
>>
>> And if you need to distinguish between the resume and restore case in
>> sp_resume(), then it sounds like you need an sp_restore().
> 
> We do need a different path for resume and restore.
> 
> I guess the call path you're looking for is:
> 
> sp_pci_restore()
> -> sp_restore()
> ->-> psp_restore()
> ->-> sp_resume()
> 
> Basically move the contents of sp_pci_restore() into a new sp_restore().

Right, without the #ifdef's in the .c file but in the .h file instead.

Thanks,
Tom

> 
>>
>> Thanks,
>> Tom
>>
>>> +    return sp_resume(sp);
>>> +}
>>> +
>>>   #ifdef CONFIG_CRYPTO_DEV_SP_PSP
>>>   static const struct sev_vdata sevv1 = {
>>>       .cmdresp_reg        = 0x10580,    /* C2PMSG_32 */
>>> @@ -563,7 +578,14 @@ static const struct pci_device_id sp_pci_table[]
>>> = {
>>>   };
>>>   MODULE_DEVICE_TABLE(pci, sp_pci_table);
>>>   -static SIMPLE_DEV_PM_OPS(sp_pci_pm_ops, sp_pci_suspend,
>>> sp_pci_resume);
>>> +static const struct dev_pm_ops sp_pci_pm_ops = {
>>> +    .suspend = pm_sleep_ptr(sp_pci_suspend),
>>> +    .resume = pm_sleep_ptr(sp_pci_resume),
>>> +    .freeze = pm_sleep_ptr(sp_pci_suspend),
>>> +    .thaw = pm_sleep_ptr(sp_pci_resume),
>>> +    .poweroff = pm_sleep_ptr(sp_pci_suspend),
>>> +    .restore_early = pm_sleep_ptr(sp_pci_restore),
>>> +};
>>>     static struct pci_driver sp_pci_driver = {
>>>       .name = "ccp",
>>> diff --git a/drivers/crypto/ccp/tee-dev.c b/drivers/crypto/ccp/tee-dev.c
>>> index af881daa5855b..11c4b05e2f3a2 100644
>>> --- a/drivers/crypto/ccp/tee-dev.c
>>> +++ b/drivers/crypto/ccp/tee-dev.c
>>> @@ -366,3 +366,8 @@ int psp_check_tee_status(void)
>>>       return 0;
>>>   }
>>>   EXPORT_SYMBOL(psp_check_tee_status);
>>> +
>>> +int tee_restore(struct psp_device *psp)
>>> +{
>>> +    return tee_init_ring(psp->tee_data);
>>> +}
>>> diff --git a/drivers/crypto/ccp/tee-dev.h b/drivers/crypto/ccp/tee-dev.h
>>> index ea9a2b7c05f57..c23416cb7bb37 100644
>>> --- a/drivers/crypto/ccp/tee-dev.h
>>> +++ b/drivers/crypto/ccp/tee-dev.h
>>> @@ -111,5 +111,6 @@ struct tee_ring_cmd {
>>>     int tee_dev_init(struct psp_device *psp);
>>>   void tee_dev_destroy(struct psp_device *psp);
>>> +int tee_restore(struct psp_device *psp);
>>>     #endif /* __TEE_DEV_H__ */
>>
>>
>