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

[Neeraj Soni]

On 8/7/2026 10:30 PM, Eric Biggers wrote:
> On Fri, Aug 07, 2026 at 07:49:24PM +0530, Neeraj Soni wrote:
>>
>>
>> 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 <ram.gupta@xxxxxxxxxxxxxxxx>
>>>>
>>>> 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 <ram.gupta@xxxxxxxxxxxxxxxx>
>>>> Signed-off-by: Seshu Madhavi Puppala <quic_spuppala@xxxxxxxxxxx>
>>>> Co-developed-by: Ram Prakash Gupta <quic_rampraka@xxxxxxxxxxx>
>>>> Signed-off-by: Ram Prakash Gupta <quic_rampraka@xxxxxxxxxxx>
>>>> Co-developed-by: Sarthak Garg <quic_sartgarg@xxxxxxxxxxx>
>>>> Signed-off-by: Sarthak Garg <quic_sartgarg@xxxxxxxxxxx>
>>>> Signed-off-by: Debraj Mukhopadhyay <quic_dmukhopa@xxxxxxxxxxx>
>>>> Signed-off-by: Neeraj Soni <neeraj.soni@xxxxxxxxxxxxxxxx>
>>>> ---
>>>>  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?
> 
> Yes, because 'if (a & 0)' just makes the code under it be optimized out
> and able to "use" undefined symbols.  It still otherwise has to be valid
> code and refer to declared symbols and fields.  See my suggestion above.
> 
Ack. Will fix in v7.

> - Eric
> 
Regards
Neeraj