Re: [PATCH v2] mmc: Avoid reprogram all keys to Inline Crypto Engine for MMC runtime suspend resume

[Debraj Mukhopadhyay <quic_dmukhopa@quicinc.com>]

Hi Eric,

Thanks for the review.

Clarifications are given inline below.

On 6/6/2025 11:29 PM, Eric Biggers wrote:
> On Fri, Jun 06, 2025 at 04:17:14PM +0530, Debraj Mukhopadhyay wrote:
>> Crypto reprogram all keys is called for each MMC runtime
>> suspend/resume in current upstream design.
> It's called from mmc_set_initial_state(), which is documented as:
>
> /*
>   * Set initial state after a power cycle or a hw_reset.
>   */
>
> Please clarify how that corresponds to "MMC runtime suspend/resume".
As part of suspend/resume, mmc regulators are turned off/on - 
effectively performing a power cycle, and hence mmc_set_initial_state is 
called after runtime suspend/resume.
Call flow:
Suspend: mmc_runtime_suspend() → _mmc_suspend() → mmc_power_off() → 
mmc_set_initial_state()
Resume: mmc_runtime_resume() → _mmc_resume() → mmc_power_up() → 
mmc_set_initial_state()
>> streaming applications have been observed due to this. Add the flag
>> MMC_CAP2_DONT_REPROGRAM as part of host->caps2 to control reprogramming
>> keys to crypto engine for socs which dont require this feature.
> The flag has a different name in the code.
Ack, will be addressed in the v3 patch.
>> diff --git a/drivers/mmc/core/crypto.c b/drivers/mmc/core/crypto.c
>> index fec4fbf16a5b..d41672e2856e 100644
>> --- a/drivers/mmc/core/crypto.c
>> +++ b/drivers/mmc/core/crypto.c
>> @@ -15,7 +15,7 @@
>>   void mmc_crypto_set_initial_state(struct mmc_host *host)
>>   {
>>   	/* Reset might clear all keys, so reprogram all the keys. */
>> -	if (host->caps2 & MMC_CAP2_CRYPTO)
>> +	if (host->caps2 & MMC_CAP2_CRYPTO && !(host->caps2 & MMC_CAP2_CRYPTO_NO_REPROG))
>>   		blk_crypto_reprogram_all_keys(&host->crypto_profile);
> Add parentheses around 'host->caps2 & MMC_CAP2_CRYPTO'
Ack, will be addressed in the v3 patch.
>> +#ifdef CONFIG_MMC_CRYPTO
>> +#define MMC_CAP2_CRYPTO_NO_REPROG	(1 << 29)	/* Host does not support inline crypto key reprogramming */
>> +#else
>> +#define MMC_CAP2_CRYPTO_NO_REPROG	0
>> +#endif
> Well, it does support inline crypto key reprogramming.  It just doesn't want the
> MMC core driver to handle it.  Please update the comment to something like:
> /* Host driver handles crypto key reprogramming */
>
> - Eric

Ack, will be addressed in the v3 patch.

Thanks,

Debraj