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

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(-)

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

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

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

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

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

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