[PATCH RFC 0/2] Avoid reprogram all keys to Inline Crypto Engine for MMC runtime suspend resume

[PATCH RFC 0/2] Avoid reprogram all keys to Inline Crypto Engine for MMC runtime suspend resume

[Seshu Madhavi Puppala]

Crypto reprogram all keys is called for each MMC runtime
suspend/resume in current upstream design. If this is implemented
as a non-interruptible call to TEE for security, the cpu core is
blocked for execution while this call executes although the crypto
engine already has the keys. For example, glitches in audio/video
streaming applications have been observed due to this. Add mmc_host_ops
hook to control reprogramming keys to crypto engine for socs which dont
require this feature.

This patch addresses the following:
- Adds vendor hook to control reprogram all keys.
- Avoids reprogram of keys for Qualcomm SOCs only.

Seshu Madhavi Puppala (2):
  mmc: core: Add vendor hook to control reprogram keys to Crypto Engine
  mmc: host: sdhci-msm: Avoid reprogram keys for QCOM socs

 drivers/mmc/core/crypto.c    | 8 +++++---
 drivers/mmc/host/sdhci-msm.c | 6 ++++++
 drivers/mmc/host/sdhci.c     | 6 ++++++
 include/linux/mmc/host.h     | 7 +++++++
 4 files changed, 24 insertions(+), 3 deletions(-)

-- 
2.17.1

[PATCH RFC v3 1/2] mmc: core: Add vendor hook to control reprogram keys to Crypto Engine

[Seshu Madhavi Puppala]

Add mmc_host_ops hook avoid_reprogram_allkeys to control
reprogramming keys to Inline Crypto Engine by vendor as some
vendors might not require this feature.

Signed-off-by: Seshu Madhavi Puppala <quic_spuppala@quicinc.com>
Co-developed-by: Ram Prakash Gupta <quic_rampraka@quicinc.com>
Signed-off-by: Ram Prakash Gupta <quic_rampraka@quicinc.com>
---
 drivers/mmc/core/crypto.c | 8 +++++---
 drivers/mmc/host/sdhci.c  | 6 ++++++
 include/linux/mmc/host.h  | 7 +++++++
 3 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/core/crypto.c b/drivers/mmc/core/crypto.c
index fec4fbf16a5b..4168f7d135ff 100644
--- a/drivers/mmc/core/crypto.c
+++ b/drivers/mmc/core/crypto.c
@@ -14,9 +14,11 @@
 
 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)
-		blk_crypto_reprogram_all_keys(&host->crypto_profile);
+	if (host->ops->avoid_reprogram_allkeys && !host->ops->avoid_reprogram_allkeys()) {
+		/* Reset might clear all keys, so reprogram all the keys. */
+		if (host->caps2 & MMC_CAP2_CRYPTO)
+			blk_crypto_reprogram_all_keys(&host->crypto_profile);
+	}
 }
 
 void mmc_crypto_setup_queue(struct request_queue *q, struct mmc_host *host)
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index fbf7a91bed35..cd663899c025 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2704,6 +2704,11 @@ int sdhci_start_signal_voltage_switch(struct mmc_host *mmc,
 }
 EXPORT_SYMBOL_GPL(sdhci_start_signal_voltage_switch);
 
+static bool sdhci_avoid_reprogram_allkeys(void)
+{
+	return false;
+}
+
 static int sdhci_card_busy(struct mmc_host *mmc)
 {
 	struct sdhci_host *host = mmc_priv(mmc);
@@ -3066,6 +3071,7 @@ static const struct mmc_host_ops sdhci_ops = {
 	.execute_tuning			= sdhci_execute_tuning,
 	.card_event			= sdhci_card_event,
 	.card_busy	= sdhci_card_busy,
+	.avoid_reprogram_allkeys	= sdhci_avoid_reprogram_allkeys,
 };
 
 /*****************************************************************************\
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 88c6a76042ee..c4109d17f177 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -218,6 +218,13 @@ struct mmc_host_ops {
 
 	/* Initialize an SD express card, mandatory for MMC_CAP2_SD_EXP. */
 	int	(*init_sd_express)(struct mmc_host *host, struct mmc_ios *ios);
+
+	/*
+	 * Optional callback to support controllers that dont require to
+	 * reprogram all crypto keys on card suspend/resume.
+	 */
+	bool	(*avoid_reprogram_allkeys)(void);
+
 };
 
 struct mmc_cqe_ops {
-- 
2.17.1

[PATCH RFC v3 2/2] mmc: host: sdhci-msm: Avoid reprogram keys for QCOM socs

[Seshu Madhavi Puppala]

Implement Qualcomm hook to avoid reprogram of all
keys to Inline Crypto Engine on runtime suspend resume
of MMC since keys are not lost for QCOM socs.

Signed-off-by: Seshu Madhavi Puppala <quic_spuppala@quicinc.com>
Co-developed-by: Ram Prakash Gupta <quic_rampraka@quicinc.com>
Signed-off-by: Ram Prakash Gupta <quic_rampraka@quicinc.com>
---
 drivers/mmc/host/sdhci-msm.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index e113b99a3eab..427e0126459a 100644
--- a/drivers/mmc/host/sdhci-msm.c
+++ b/drivers/mmc/host/sdhci-msm.c
@@ -1183,6 +1183,11 @@ static void sdhci_msm_set_cdr(struct sdhci_host *host, bool enable)
 	}
 }
 
+static bool sdhci_msm_avoid_reprogram_allkeys(void)
+{
+	return true;
+}
+
 static int sdhci_msm_execute_tuning(struct mmc_host *mmc, u32 opcode)
 {
 	struct sdhci_host *host = mmc_priv(mmc);
@@ -2641,6 +2646,7 @@ static int sdhci_msm_probe(struct platform_device *pdev)
 	host->mmc_host_ops.start_signal_voltage_switch =
 		sdhci_msm_start_signal_voltage_switch;
 	host->mmc_host_ops.execute_tuning = sdhci_msm_execute_tuning;
+	host->mmc_host_ops.avoid_reprogram_allkeys = sdhci_msm_avoid_reprogram_allkeys;
 	if (of_property_read_bool(node, "supports-cqe"))
 		ret = sdhci_msm_cqe_add_host(host, pdev);
 	else
-- 
2.17.1

Re: [PATCH RFC v3 1/2] mmc: core: Add vendor hook to control reprogram keys to Crypto Engine

[Ulf Hansson]

On Sun, 6 Oct 2024 at 15:55, Seshu Madhavi Puppala
<quic_spuppala@quicinc.com> wrote:
>
> Add mmc_host_ops hook avoid_reprogram_allkeys to control
> reprogramming keys to Inline Crypto Engine by vendor as some
> vendors might not require this feature.
>
> Signed-off-by: Seshu Madhavi Puppala <quic_spuppala@quicinc.com>
> Co-developed-by: Ram Prakash Gupta <quic_rampraka@quicinc.com>
> Signed-off-by: Ram Prakash Gupta <quic_rampraka@quicinc.com>
> ---
>  drivers/mmc/core/crypto.c | 8 +++++---
>  drivers/mmc/host/sdhci.c  | 6 ++++++
>  include/linux/mmc/host.h  | 7 +++++++
>  3 files changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mmc/core/crypto.c b/drivers/mmc/core/crypto.c
> index fec4fbf16a5b..4168f7d135ff 100644
> --- a/drivers/mmc/core/crypto.c
> +++ b/drivers/mmc/core/crypto.c
> @@ -14,9 +14,11 @@
>
>  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)
> -               blk_crypto_reprogram_all_keys(&host->crypto_profile);
> +       if (host->ops->avoid_reprogram_allkeys && !host->ops->avoid_reprogram_allkeys()) {
> +               /* Reset might clear all keys, so reprogram all the keys. */
> +               if (host->caps2 & MMC_CAP2_CRYPTO)
> +                       blk_crypto_reprogram_all_keys(&host->crypto_profile);

Don't you even need to call this once, during the first initialization
of the card?

> +       }
>  }
>

[...]

Kind regards
Uffe

Re: [PATCH RFC v3 1/2] mmc: core: Add vendor hook to control reprogram keys to Crypto Engine

[Seshu Madhavi Puppala]

On 10/8/2024 7:30 PM, Ulf Hansson wrote:
> On Sun, 6 Oct 2024 at 15:55, Seshu Madhavi Puppala
> <quic_spuppala@quicinc.com> wrote:
>>
>> Add mmc_host_ops hook avoid_reprogram_allkeys to control
>> reprogramming keys to Inline Crypto Engine by vendor as some
>> vendors might not require this feature.
>>
>> Signed-off-by: Seshu Madhavi Puppala <quic_spuppala@quicinc.com>
>> Co-developed-by: Ram Prakash Gupta <quic_rampraka@quicinc.com>
>> Signed-off-by: Ram Prakash Gupta <quic_rampraka@quicinc.com>
>> ---
>>   drivers/mmc/core/crypto.c | 8 +++++---
>>   drivers/mmc/host/sdhci.c  | 6 ++++++
>>   include/linux/mmc/host.h  | 7 +++++++
>>   3 files changed, 18 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/mmc/core/crypto.c b/drivers/mmc/core/crypto.c
>> index fec4fbf16a5b..4168f7d135ff 100644
>> --- a/drivers/mmc/core/crypto.c
>> +++ b/drivers/mmc/core/crypto.c
>> @@ -14,9 +14,11 @@
>>
>>   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)
>> -               blk_crypto_reprogram_all_keys(&host->crypto_profile);
>> +       if (host->ops->avoid_reprogram_allkeys && !host->ops->avoid_reprogram_allkeys()) {
>> +               /* Reset might clear all keys, so reprogram all the keys. */
>> +               if (host->caps2 & MMC_CAP2_CRYPTO)
>> +                       blk_crypto_reprogram_all_keys(&host->crypto_profile);
> 
> Don't you even need to call this once, during the first initialization
> of the card?

The first card initialization is done during the boot up for qcom socs 
and the kernel keyring contains no keys immediately after bootup.After 
the initialization of the card, the block i/o operations to encrypted 
folders will automatically trigger the corresponding program key calls 
to the crypto engine since the kernel keyring does not contain the 
required encryption key. So, it is not necessary to explicitly reprogram 
all keys for qcom socs.
> 
>> +       }
>>   }
>>
> 
> [...]
> 
> Kind regards
> Uffe

Re: [PATCH RFC v3 1/2] mmc: core: Add vendor hook to control reprogram keys to Crypto Engine

[Ulf Hansson]

+ Eric, Abel

On Sat, 19 Oct 2024 at 06:55, Seshu Madhavi Puppala
<quic_spuppala@quicinc.com> wrote:
>
>
>
> On 10/8/2024 7:30 PM, Ulf Hansson wrote:
> > On Sun, 6 Oct 2024 at 15:55, Seshu Madhavi Puppala
> > <quic_spuppala@quicinc.com> wrote:
> >>
> >> Add mmc_host_ops hook avoid_reprogram_allkeys to control
> >> reprogramming keys to Inline Crypto Engine by vendor as some
> >> vendors might not require this feature.
> >>
> >> Signed-off-by: Seshu Madhavi Puppala <quic_spuppala@quicinc.com>
> >> Co-developed-by: Ram Prakash Gupta <quic_rampraka@quicinc.com>
> >> Signed-off-by: Ram Prakash Gupta <quic_rampraka@quicinc.com>
> >> ---
> >>   drivers/mmc/core/crypto.c | 8 +++++---
> >>   drivers/mmc/host/sdhci.c  | 6 ++++++
> >>   include/linux/mmc/host.h  | 7 +++++++
> >>   3 files changed, 18 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/mmc/core/crypto.c b/drivers/mmc/core/crypto.c
> >> index fec4fbf16a5b..4168f7d135ff 100644
> >> --- a/drivers/mmc/core/crypto.c
> >> +++ b/drivers/mmc/core/crypto.c
> >> @@ -14,9 +14,11 @@
> >>
> >>   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)
> >> -               blk_crypto_reprogram_all_keys(&host->crypto_profile);
> >> +       if (host->ops->avoid_reprogram_allkeys && !host->ops->avoid_reprogram_allkeys()) {
> >> +               /* Reset might clear all keys, so reprogram all the keys. */
> >> +               if (host->caps2 & MMC_CAP2_CRYPTO)
> >> +                       blk_crypto_reprogram_all_keys(&host->crypto_profile);
> >
> > Don't you even need to call this once, during the first initialization
> > of the card?
>
> The first card initialization is done during the boot up for qcom socs
> and the kernel keyring contains no keys immediately after bootup.After
> the initialization of the card, the block i/o operations to encrypted
> folders will automatically trigger the corresponding program key calls
> to the crypto engine since the kernel keyring does not contain the
> required encryption key. So, it is not necessary to explicitly reprogram
> all keys for qcom socs.

Okay, I see. I have looped in Abel and Eric who worked on this
feature, just to confirm that this makes sense for them too.

I assume the reason why you want to avoid the re-programming is that
it adds latency when re-initializing the card, right? In that case, do
you have some numbers of what we save by doing this?

Kind regards
Uffe

Re: [PATCH RFC v3 1/2] mmc: core: Add vendor hook to control reprogram keys to Crypto Engine

[Adrian Hunter]

On 6/10/24 16:55, Seshu Madhavi Puppala wrote:
> Add mmc_host_ops hook avoid_reprogram_allkeys to control
> reprogramming keys to Inline Crypto Engine by vendor as some
> vendors might not require this feature.
> 
> Signed-off-by: Seshu Madhavi Puppala <quic_spuppala@quicinc.com>
> Co-developed-by: Ram Prakash Gupta <quic_rampraka@quicinc.com>
> Signed-off-by: Ram Prakash Gupta <quic_rampraka@quicinc.com>
> ---
>  drivers/mmc/core/crypto.c | 8 +++++---
>  drivers/mmc/host/sdhci.c  | 6 ++++++
>  include/linux/mmc/host.h  | 7 +++++++
>  3 files changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mmc/core/crypto.c b/drivers/mmc/core/crypto.c
> index fec4fbf16a5b..4168f7d135ff 100644
> --- a/drivers/mmc/core/crypto.c
> +++ b/drivers/mmc/core/crypto.c
> @@ -14,9 +14,11 @@
>  
>  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)
> -		blk_crypto_reprogram_all_keys(&host->crypto_profile);
> +	if (host->ops->avoid_reprogram_allkeys && !host->ops->avoid_reprogram_allkeys()) {
> +		/* Reset might clear all keys, so reprogram all the keys. */
> +		if (host->caps2 & MMC_CAP2_CRYPTO)
> +			blk_crypto_reprogram_all_keys(&host->crypto_profile);
> +	}

Probably nicer to put MMC_CAP2_CRYPTO check first, but also the logic
needs a tweak:

	/* Reset might clear all keys, so reprogram all the keys. */
	if (host->caps2 & MMC_CAP2_CRYPTO &&
	    (!host->ops->avoid_reprogram_allkeys ||
	     !host->ops->avoid_reprogram_allkeys()))
		blk_crypto_reprogram_all_keys(&host->crypto_profile);

>  }
>  
>  void mmc_crypto_setup_queue(struct request_queue *q, struct mmc_host *host)
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index fbf7a91bed35..cd663899c025 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -2704,6 +2704,11 @@ int sdhci_start_signal_voltage_switch(struct mmc_host *mmc,
>  }
>  EXPORT_SYMBOL_GPL(sdhci_start_signal_voltage_switch);
>  
> +static bool sdhci_avoid_reprogram_allkeys(void)
> +{
> +	return false;
> +}
> +
>  static int sdhci_card_busy(struct mmc_host *mmc)
>  {
>  	struct sdhci_host *host = mmc_priv(mmc);
> @@ -3066,6 +3071,7 @@ static const struct mmc_host_ops sdhci_ops = {
>  	.execute_tuning			= sdhci_execute_tuning,
>  	.card_event			= sdhci_card_event,
>  	.card_busy	= sdhci_card_busy,
> +	.avoid_reprogram_allkeys	= sdhci_avoid_reprogram_allkeys,

There isn't any need for this

>  };
>  
>  /*****************************************************************************\
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index 88c6a76042ee..c4109d17f177 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -218,6 +218,13 @@ struct mmc_host_ops {
>  
>  	/* Initialize an SD express card, mandatory for MMC_CAP2_SD_EXP. */
>  	int	(*init_sd_express)(struct mmc_host *host, struct mmc_ios *ios);
> +
> +	/*
> +	 * Optional callback to support controllers that dont require to
> +	 * reprogram all crypto keys on card suspend/resume.
> +	 */
> +	bool	(*avoid_reprogram_allkeys)(void);
> +
>  };
>  
>  struct mmc_cqe_ops {

Re: [PATCH RFC v3 1/2] mmc: core: Add vendor hook to control reprogram keys to Crypto Engine

[Eric Biggers]

On Sun, Oct 06, 2024 at 07:25:29PM +0530, Seshu Madhavi Puppala wrote:
> Add mmc_host_ops hook avoid_reprogram_allkeys to control
> reprogramming keys to Inline Crypto Engine by vendor as some
> vendors might not require this feature.
> 
> Signed-off-by: Seshu Madhavi Puppala <quic_spuppala@quicinc.com>
> Co-developed-by: Ram Prakash Gupta <quic_rampraka@quicinc.com>
> Signed-off-by: Ram Prakash Gupta <quic_rampraka@quicinc.com>
> ---
>  drivers/mmc/core/crypto.c | 8 +++++---
>  drivers/mmc/host/sdhci.c  | 6 ++++++
>  include/linux/mmc/host.h  | 7 +++++++
>  3 files changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mmc/core/crypto.c b/drivers/mmc/core/crypto.c
> index fec4fbf16a5b..4168f7d135ff 100644
> --- a/drivers/mmc/core/crypto.c
> +++ b/drivers/mmc/core/crypto.c
> @@ -14,9 +14,11 @@
>  
>  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)
> -		blk_crypto_reprogram_all_keys(&host->crypto_profile);
> +	if (host->ops->avoid_reprogram_allkeys && !host->ops->avoid_reprogram_allkeys()) {
> +		/* Reset might clear all keys, so reprogram all the keys. */
> +		if (host->caps2 & MMC_CAP2_CRYPTO)
> +			blk_crypto_reprogram_all_keys(&host->crypto_profile);
> +	}

This should be a simple flag, not an indirect function call which is
inefficient.

It could be a bit in mmc_host_ops, though based on the existing code maybe a new
bit in MMC_CAP2_* would be more appropriate.

- Eric

Re: [PATCH RFC 0/2] Avoid reprogram all keys to Inline Crypto Engine for MMC runtime suspend resume

[Eric Biggers]

On Sun, Oct 06, 2024 at 07:25:28PM +0530, Seshu Madhavi Puppala wrote:
> Crypto reprogram all keys is called for each MMC runtime
> suspend/resume in current upstream design.

Is that correct?  I thought that similar to what is done for UFS, the key
reprogramming happens only after the MMC controller is reset.  I thought that is
different from a runtime suspend.

If it's in fact triggering more often, maybe that is what needs to be fixed?

- Eric

Re: [PATCH RFC v3 1/2] mmc: core: Add vendor hook to control reprogram keys to Crypto Engine

[Ulf Hansson]

On Wed, 23 Oct 2024 at 23:28, Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Sun, Oct 06, 2024 at 07:25:29PM +0530, Seshu Madhavi Puppala wrote:
> > Add mmc_host_ops hook avoid_reprogram_allkeys to control
> > reprogramming keys to Inline Crypto Engine by vendor as some
> > vendors might not require this feature.
> >
> > Signed-off-by: Seshu Madhavi Puppala <quic_spuppala@quicinc.com>
> > Co-developed-by: Ram Prakash Gupta <quic_rampraka@quicinc.com>
> > Signed-off-by: Ram Prakash Gupta <quic_rampraka@quicinc.com>
> > ---
> >  drivers/mmc/core/crypto.c | 8 +++++---
> >  drivers/mmc/host/sdhci.c  | 6 ++++++
> >  include/linux/mmc/host.h  | 7 +++++++
> >  3 files changed, 18 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/mmc/core/crypto.c b/drivers/mmc/core/crypto.c
> > index fec4fbf16a5b..4168f7d135ff 100644
> > --- a/drivers/mmc/core/crypto.c
> > +++ b/drivers/mmc/core/crypto.c
> > @@ -14,9 +14,11 @@
> >
> >  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)
> > -             blk_crypto_reprogram_all_keys(&host->crypto_profile);
> > +     if (host->ops->avoid_reprogram_allkeys && !host->ops->avoid_reprogram_allkeys()) {
> > +             /* Reset might clear all keys, so reprogram all the keys. */
> > +             if (host->caps2 & MMC_CAP2_CRYPTO)
> > +                     blk_crypto_reprogram_all_keys(&host->crypto_profile);
> > +     }
>
> This should be a simple flag, not an indirect function call which is
> inefficient.
>
> It could be a bit in mmc_host_ops, though based on the existing code maybe a new
> bit in MMC_CAP2_* would be more appropriate.

Thanks for commenting Eric. From a principle point of view, it sounds
like this makes sense to you too.

When it comes to the implementation details, I agree with the above.
Perhaps MMC_CAP2_CRYPTO_NO_PROG, or something along those lines would
be better. Unless the callback is intended to dynamically allow the
decision to be changed, but it doesn't look like that in subsequent
patches.

Kind regards
Uffe

Re: [PATCH RFC 0/2] Avoid reprogram all keys to Inline Crypto Engine for MMC runtime suspend resume

[Ulf Hansson]

On Wed, 23 Oct 2024 at 23:31, Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Sun, Oct 06, 2024 at 07:25:28PM +0530, Seshu Madhavi Puppala wrote:
> > Crypto reprogram all keys is called for each MMC runtime
> > suspend/resume in current upstream design.
>
> Is that correct?  I thought that similar to what is done for UFS, the key
> reprogramming happens only after the MMC controller is reset.  I thought that is
> different from a runtime suspend.

Looks like Seshu is not really worried about the host's runtime
suspend, but the card's runtime suspend.

Perhaps there are some out of tree code involved here that makes use
of MMC_CAP_AGGRESSIVE_PM, which is what allows the card to be runtime
suspended?

>
> If it's in fact triggering more often, maybe that is what needs to be fixed?

We could extend the runtime PM autosusend timeout for the card, if
that makes sense.

Kind regards
Uffe

Re: [PATCH RFC 0/2] Avoid reprogram all keys to Inline Crypto Engine for MMC runtime suspend resume

[Eric Biggers]

On Fri, Oct 25, 2024 at 01:07:18AM +0200, Ulf Hansson wrote:
> On Wed, 23 Oct 2024 at 23:31, Eric Biggers <ebiggers@kernel.org> wrote:
> >
> > On Sun, Oct 06, 2024 at 07:25:28PM +0530, Seshu Madhavi Puppala wrote:
> > > Crypto reprogram all keys is called for each MMC runtime
> > > suspend/resume in current upstream design.
> >
> > Is that correct?  I thought that similar to what is done for UFS, the key
> > reprogramming happens only after the MMC controller is reset.  I thought that is
> > different from a runtime suspend.
> 
> Looks like Seshu is not really worried about the host's runtime
> suspend, but the card's runtime suspend.
> 
> Perhaps there are some out of tree code involved here that makes use
> of MMC_CAP_AGGRESSIVE_PM, which is what allows the card to be runtime
> suspended?
> 
> >
> > If it's in fact triggering more often, maybe that is what needs to be fixed?
> 
> We could extend the runtime PM autosusend timeout for the card, if
> that makes sense.
> 
> Kind regards
> Uffe

The keyslots are being reprogrammed from mmc_set_initial_state(), which is
documented as:

    /*
     * Set initial state after a power cycle or a hw_reset.
     */
    void mmc_set_initial_state(struct mmc_host *host)

It's called by: mmc_power_up(), mmc_power_off(), _mmc_hw_reset(), and
mmc_sdio_sw_reset().

Can that mean a power cycle of the card, not a power cycle of the host
controller?  The keyslots are part of the host controller, so that may explain
the problem.  The keyslots should be reprogrammed only when the host controller
is reset, as that is when they are lost.  (And it should not be skipped entirely
as this patchset does, as a host controller reset is possible.)

I am not an expert in MMC or in the details of how Qualcomm ICE is wired up to
the system, so I might have this wrong.  But let me know if it sounds right.

- Eric

Re: [PATCH RFC 0/2] Avoid reprogram all keys to Inline Crypto Engine for MMC runtime suspend resume

[Ulf Hansson]

On Fri, 25 Oct 2024 at 04:56, Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Fri, Oct 25, 2024 at 01:07:18AM +0200, Ulf Hansson wrote:
> > On Wed, 23 Oct 2024 at 23:31, Eric Biggers <ebiggers@kernel.org> wrote:
> > >
> > > On Sun, Oct 06, 2024 at 07:25:28PM +0530, Seshu Madhavi Puppala wrote:
> > > > Crypto reprogram all keys is called for each MMC runtime
> > > > suspend/resume in current upstream design.
> > >
> > > Is that correct?  I thought that similar to what is done for UFS, the key
> > > reprogramming happens only after the MMC controller is reset.  I thought that is
> > > different from a runtime suspend.
> >
> > Looks like Seshu is not really worried about the host's runtime
> > suspend, but the card's runtime suspend.
> >
> > Perhaps there are some out of tree code involved here that makes use
> > of MMC_CAP_AGGRESSIVE_PM, which is what allows the card to be runtime
> > suspended?
> >
> > >
> > > If it's in fact triggering more often, maybe that is what needs to be fixed?
> >
> > We could extend the runtime PM autosusend timeout for the card, if
> > that makes sense.
> >
> > Kind regards
> > Uffe
>
> The keyslots are being reprogrammed from mmc_set_initial_state(), which is
> documented as:
>
>     /*
>      * Set initial state after a power cycle or a hw_reset.
>      */
>     void mmc_set_initial_state(struct mmc_host *host)
>
> It's called by: mmc_power_up(), mmc_power_off(), _mmc_hw_reset(), and
> mmc_sdio_sw_reset().
>
> Can that mean a power cycle of the card, not a power cycle of the host
> controller?

Yes, that's correct.

Well, indirectly the host is likely to be power cycled too, but not necessarily.

> The keyslots are part of the host controller, so that may explain
> the problem.  The keyslots should be reprogrammed only when the host controller
> is reset, as that is when they are lost.  (And it should not be skipped entirely
> as this patchset does, as a host controller reset is possible.)
>
> I am not an expert in MMC or in the details of how Qualcomm ICE is wired up to
> the system, so I might have this wrong.  But let me know if it sounds right.

It sounds reasonable to me, but I also don't know the HW well enough
to be able to tell.

Looks like we need some more input from Seshu and the QC folkz to
understand better.

Kind regards
Uffe

Re: [PATCH RFC v3 1/2] mmc: core: Add vendor hook to control reprogram keys to Crypto Engine

[Seshu Madhavi Puppala]

On 10/24/2024 2:58 AM, Eric Biggers wrote:
> On Sun, Oct 06, 2024 at 07:25:29PM +0530, Seshu Madhavi Puppala wrote:
>> Add mmc_host_ops hook avoid_reprogram_allkeys to control
>> reprogramming keys to Inline Crypto Engine by vendor as some
>> vendors might not require this feature.
>>
>> Signed-off-by: Seshu Madhavi Puppala <quic_spuppala@quicinc.com>
>> Co-developed-by: Ram Prakash Gupta <quic_rampraka@quicinc.com>
>> Signed-off-by: Ram Prakash Gupta <quic_rampraka@quicinc.com>
>> ---
>>   drivers/mmc/core/crypto.c | 8 +++++---
>>   drivers/mmc/host/sdhci.c  | 6 ++++++
>>   include/linux/mmc/host.h  | 7 +++++++
>>   3 files changed, 18 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/mmc/core/crypto.c b/drivers/mmc/core/crypto.c
>> index fec4fbf16a5b..4168f7d135ff 100644
>> --- a/drivers/mmc/core/crypto.c
>> +++ b/drivers/mmc/core/crypto.c
>> @@ -14,9 +14,11 @@
>>   
>>   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)
>> -		blk_crypto_reprogram_all_keys(&host->crypto_profile);
>> +	if (host->ops->avoid_reprogram_allkeys && !host->ops->avoid_reprogram_allkeys()) {
>> +		/* Reset might clear all keys, so reprogram all the keys. */
>> +		if (host->caps2 & MMC_CAP2_CRYPTO)
>> +			blk_crypto_reprogram_all_keys(&host->crypto_profile);
>> +	}
> 
> This should be a simple flag, not an indirect function call which is
> inefficient.
> 
> It could be a bit in mmc_host_ops, though based on the existing code maybe a new
> bit in MMC_CAP2_* would be more appropriate.

I will update this in v2 patchset by adding the flag as part of host->caps2

Thanks,
Seshu
> 
> - Eric

Re: [PATCH RFC 0/2] Avoid reprogram all keys to Inline Crypto Engine for MMC runtime suspend resume

[Seshu Madhavi Puppala]

On 10/25/2024 2:12 PM, Ulf Hansson wrote:
> On Fri, 25 Oct 2024 at 04:56, Eric Biggers <ebiggers@kernel.org> wrote:
>>
>> On Fri, Oct 25, 2024 at 01:07:18AM +0200, Ulf Hansson wrote:
>>> On Wed, 23 Oct 2024 at 23:31, Eric Biggers <ebiggers@kernel.org> wrote:
>>>>
>>>> On Sun, Oct 06, 2024 at 07:25:28PM +0530, Seshu Madhavi Puppala wrote:
>>>>> Crypto reprogram all keys is called for each MMC runtime
>>>>> suspend/resume in current upstream design.
>>>>
>>>> Is that correct?  I thought that similar to what is done for UFS, the key
>>>> reprogramming happens only after the MMC controller is reset.  I thought that is
>>>> different from a runtime suspend.
>>>
>>> Looks like Seshu is not really worried about the host's runtime
>>> suspend, but the card's runtime suspend.
>>>
>>> Perhaps there are some out of tree code involved here that makes use
>>> of MMC_CAP_AGGRESSIVE_PM, which is what allows the card to be runtime
>>> suspended?
>>>
>>>>
>>>> If it's in fact triggering more often, maybe that is what needs to be fixed?
>>>
>>> We could extend the runtime PM autosusend timeout for the card, if
>>> that makes sense.
>>>
This change aims to address host side feature by not tying it up to card 
side flag/feature.
>>> Kind regards
>>> Uffe
>>
>> The keyslots are being reprogrammed from mmc_set_initial_state(), which is
>> documented as:
>>
>>      /*
>>       * Set initial state after a power cycle or a hw_reset.
>>       */
>>      void mmc_set_initial_state(struct mmc_host *host)
>>
>> It's called by: mmc_power_up(), mmc_power_off(), _mmc_hw_reset(), and
>> mmc_sdio_sw_reset().
>>
>> Can that mean a power cycle of the card, not a power cycle of the host
>> controller?
> 
> Yes, that's correct.
> 
> Well, indirectly the host is likely to be power cycled too, but not necessarily.
> 
>> The keyslots are part of the host controller, so that may explain
>> the problem.  The keyslots should be reprogrammed only when the host controller
>> is reset, as that is when they are lost.  (And it should not be skipped entirely
>> as this patchset does, as a host controller reset is possible.)
>>

This will be update via a separate patch by invoking reprogram_all_keys 
API from sdhci_msm_gcc_reset() API 
https://github.com/torvalds/linux/blob/master/drivers/mmc/host/sdhci-msm.c#L2363

Thanks,
Seshu

>> I am not an expert in MMC or in the details of how Qualcomm ICE is wired up to
>> the system, so I might have this wrong.  But let me know if it sounds right.
> 
> It sounds reasonable to me, but I also don't know the HW well enough
> to be able to tell.
> 
> Looks like we need some more input from Seshu and the QC folkz to
> understand better.
> 
> Kind regards
> Uffe

Re: [PATCH RFC 0/2] Avoid reprogram all keys to Inline Crypto Engine for MMC runtime suspend resume

[Ulf Hansson]

On Thu, 21 Nov 2024 at 06:16, Seshu Madhavi Puppala
<quic_spuppala@quicinc.com> wrote:
>
>
>
> On 10/25/2024 2:12 PM, Ulf Hansson wrote:
> > On Fri, 25 Oct 2024 at 04:56, Eric Biggers <ebiggers@kernel.org> wrote:
> >>
> >> On Fri, Oct 25, 2024 at 01:07:18AM +0200, Ulf Hansson wrote:
> >>> On Wed, 23 Oct 2024 at 23:31, Eric Biggers <ebiggers@kernel.org> wrote:
> >>>>
> >>>> On Sun, Oct 06, 2024 at 07:25:28PM +0530, Seshu Madhavi Puppala wrote:
> >>>>> Crypto reprogram all keys is called for each MMC runtime
> >>>>> suspend/resume in current upstream design.
> >>>>
> >>>> Is that correct?  I thought that similar to what is done for UFS, the key
> >>>> reprogramming happens only after the MMC controller is reset.  I thought that is
> >>>> different from a runtime suspend.
> >>>
> >>> Looks like Seshu is not really worried about the host's runtime
> >>> suspend, but the card's runtime suspend.
> >>>
> >>> Perhaps there are some out of tree code involved here that makes use
> >>> of MMC_CAP_AGGRESSIVE_PM, which is what allows the card to be runtime
> >>> suspended?
> >>>
> >>>>
> >>>> If it's in fact triggering more often, maybe that is what needs to be fixed?
> >>>
> >>> We could extend the runtime PM autosusend timeout for the card, if
> >>> that makes sense.
> >>>
> This change aims to address host side feature by not tying it up to card
> side flag/feature.
> >>> Kind regards
> >>> Uffe
> >>
> >> The keyslots are being reprogrammed from mmc_set_initial_state(), which is
> >> documented as:
> >>
> >>      /*
> >>       * Set initial state after a power cycle or a hw_reset.
> >>       */
> >>      void mmc_set_initial_state(struct mmc_host *host)
> >>
> >> It's called by: mmc_power_up(), mmc_power_off(), _mmc_hw_reset(), and
> >> mmc_sdio_sw_reset().
> >>
> >> Can that mean a power cycle of the card, not a power cycle of the host
> >> controller?
> >
> > Yes, that's correct.
> >
> > Well, indirectly the host is likely to be power cycled too, but not necessarily.
> >
> >> The keyslots are part of the host controller, so that may explain
> >> the problem.  The keyslots should be reprogrammed only when the host controller
> >> is reset, as that is when they are lost.  (And it should not be skipped entirely
> >> as this patchset does, as a host controller reset is possible.)
> >>
>
> This will be update via a separate patch by invoking reprogram_all_keys
> API from sdhci_msm_gcc_reset() API
> https://github.com/torvalds/linux/blob/master/drivers/mmc/host/sdhci-msm.c#L2363

Okay, in that case, please post the complete solution in the next
version. It seems like $subject series on its own doesn't make sense
to us.

[...]

Kind regards
Uffe