Re: [PATCH v6 2/2] mmc: sdhci-msm: Use pm ops instead of macro to restore crypto keys

On 8/4/2026 1:48 AM, Eric Biggers wrote:
> On Mon, Aug 03, 2026 at 10:01:23PM +0530, Neeraj Soni wrote:
>> From: Ram Prakash Gupta <[email protected]>
>>
>> Inline Crypto Engine (ICE) keys are lost after hibernation entry and this
>> needs to be restored when hibernation exits. ICE keys are re-programmed
>> during sdhci_msm_ice_init() but it may not cover cases where the
>> hibernation image is already restored.
>>
>> Unwrap the pm ops and use directly in driver to add the call to restore
>> Inline Crypto Engine (ICE) keys. This ensures that ICE is brought into
>> same state as before hibernation.
>>
>> Also set MMC_CAP2_CRYPTO_NO_REPROG to indicate that re-programming of ICE
>> keys is not needed during MMC runtime suspend/resume.
>>
>> Signed-off-by: Ram Prakash Gupta <[email protected]>
>> Signed-off-by: Seshu Madhavi Puppala <[email protected]>
>> Co-developed-by: Ram Prakash Gupta <[email protected]>
>> Signed-off-by: Ram Prakash Gupta <[email protected]>
>> Co-developed-by: Sarthak Garg <[email protected]>
>> Signed-off-by: Sarthak Garg <[email protected]>
>> Signed-off-by: Debraj Mukhopadhyay <[email protected]>
>> Signed-off-by: Neeraj Soni <[email protected]>
>> ---
>>  drivers/mmc/host/sdhci-msm.c | 23 ++++++++++++++++++++++-
>>  1 file changed, 22 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
>> index 4aff965f0e2e..f64788a70309 100644
>> --- a/drivers/mmc/host/sdhci-msm.c
>> +++ b/drivers/mmc/host/sdhci-msm.c
>> @@ -1959,6 +1959,7 @@ static int sdhci_msm_ice_init(struct sdhci_msm_host *msm_host,
>>  	}
>>  
>>  	mmc->caps2 |= MMC_CAP2_CRYPTO;
>> +	mmc->caps2 |= MMC_CAP2_CRYPTO_NO_REPROG;
>>  	return 0;
>>  }
>>  
>> @@ -2983,9 +2984,29 @@ static int sdhci_msm_runtime_resume(struct device *dev)
>>  	return ret;
>>  }
>>  
>> +static int sdhci_msm_restore(struct device *dev)
>> +{
>> +	struct sdhci_host *host = dev_get_drvdata(dev);
>> +	int ret;
>> +
>> +	ret = pm_runtime_force_resume(dev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (host->mmc->caps2 & MMC_CAP2_CRYPTO)
>> +		blk_crypto_reprogram_all_keys(&host->mmc->crypto_profile);
>> +
>> +	return ret;
>> +}
> 
> Sorry, it looks like this actually does need an #ifdef because the
> crypto_profile field is conditional.  It doesn't need to be a new #ifdef
> though, as you could add a function sdhci_msm_ice_restore() within the
> existing CONFIG_MMC_CRYPTO section of this file (along with a no-op stub
> in the !CONFIG_MMC_CRYPTO section), then call it from here.  Similar to
> how the existing sdhci_msm_ice_resume() works, for example.
>
But MMC_CPA2_CRYPTO=0 if !CONFIG_MMC_CRYPTO so crypto_profile field is
not accessed if !CONFIG_MMC_CRYPTO. Does it need encapsulation?
 
>>  static const struct dev_pm_ops sdhci_msm_pm_ops = {
>> -	SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume)
>>  	RUNTIME_PM_OPS(sdhci_msm_runtime_suspend, sdhci_msm_runtime_resume, NULL)
>> +	.suspend	= pm_runtime_force_suspend,
>> +	.resume		= pm_runtime_force_resume,
>> +	.freeze		= pm_runtime_force_suspend,
>> +	.restore	= sdhci_msm_restore,
>> +	.thaw		= pm_runtime_force_resume,
>> +	.poweroff	= pm_runtime_force_suspend,
> 
> Should these use pm_sleep_ptr() to allow dead code elimination when
> !CONFIG_PM_SLEEP?
>
Ram can you please check this. Looks valid to me. 
 
> - Eric
> 
Regards
Neeraj