Re: [PATCH v9 3/3] mmc: core: Honor keep-power-in-suspend and reset-card-at-resume for (e)MMC

[Ulf Hansson]

On Thu, Oct 22, 2026 at 11:14 PM Kamal Dasu <kamal.dasu@xxxxxxxxxxxx> wrote:
>
> On Mon, Sep 7, 2026 at 9:59 AM Ulf Hansson <ulf.hansson@xxxxxxxxxxxxxxxx> wrote:
> >
> > On Tue, Aug 11, 2026 at 6:40 PM Kamal Dasu <kamal.dasu@xxxxxxxxxxxx> wrote:
> > >
> > > On some platforms, firmware accesses the (e)MMC card directly during
> > > resume from Suspend-to-DRAM, before the kernel's own resume path has
> > > run, in order to load boot code. This requires the card to remain
> > > powered and responsive throughout suspend: putting it to sleep,
> > > sending it a power-off notification, or removing its supply is not
> > > safe, since firmware needs to talk to a live card. Since the card is
> > > never power-cycled, nothing else resets it back to a known state
> > > before the kernel reuses it after resume.
> > >
> > > keep-power-in-suspend / MMC_PM_KEEP_POWER already exist for the first
> > > part, but are only consumed in the SDIO suspend/resume path
> > > (mmc_sdio_suspend()/mmc_sdio_resume()), gated on a per-function
> > > runtime request via sdio_set_host_pm_flags(). (e)MMC has no
> > > equivalent function-driver layer to make that request, and the
> > > requirement here is a fixed platform characteristic rather than a
> > > per-cycle one, so _mmc_suspend() checks host->pm_caps directly
> > > instead of pm_flags.
> > >
> > > reset-card-at-resume covers the second part: when set, _mmc_resume()
> > > resets the host to its initial bus state the same way _mmc_hw_reset()
> > > does for a non-power-cycle reset, before mmc_power_up() and
> > > mmc_init_card() re-identify the card.
> > >
> > > _mmc_suspend()'s fast path requires both MMC_PM_KEEP_POWER and
> > > MMC_CAP2_RESET_AT_RESUME to be set. Keeping the card powered without
> > > also resetting it at resume is not safe for this driver: skipping
> > > mmc_power_off() leaves power_mode at MMC_POWER_ON, so mmc_power_up()
> > > no-ops in _mmc_resume(), and without an explicit reset first,
> > > mmc_init_card() runs against whatever bus speed/width was active
> > > before suspend instead of the initial state it expects. Conversely,
> > > _mmc_resume()'s reset checks pm_flags rather than the
> > > MMC_CAP2_RESET_AT_RESUME capability directly, since pm_flags is only
> > > set when the fast path actually ran -- the only time power_mode is
> > > guaranteed to still be MMC_POWER_ON, and so the only time resetting
> > > the bus before mmc_power_up() is both necessary and safe. Without
> > > that check, MMC_CAP2_RESET_AT_RESUME set on its own would drive the
> > > clock and bus lines while the card's supply is still off following a
> > > normal mmc_power_off().
> > >
> > > The fast path also leaves the card marked suspended without powering
> > > it off, so _mmc_suspend()'s pre-existing early exit for an
> > > already-suspended card can no longer assume there is nothing left to
> > > do: a shutdown, unbind or undervoltage event can still arrive before
> > > the card's next real access lazily triggers _mmc_resume() via runtime
> > > PM, and those events need the normal power-off sequence regardless.
> > > Reselect the card and continue into that sequence in that case,
> > > instead of returning early and silently skipping mmc_poweroff_notify()
> > > and mmc_power_off(). Reset the host to its initial bus state first,
> > > the same way _mmc_resume() does, since firmware may have left the
> > > card in a state that doesn't decode reliably at whatever clock/timing
> > > was negotiated before suspend -- otherwise mmc_select_card() itself
> > > can fail and abort into that same early exit, before ever reaching
> > > the sequence it was supposed to fall through to. If mmc_select_card()
> > > still fails after that reset -- the card may genuinely be gone -- fall
> > > through to mmc_power_off() directly rather than aborting again:
> > > unlike the other steps here, powering off doesn't need a responsive
> > > card, and shutdown/undervoltage need the supply actually removed
> > > regardless.
> > >
> > > host->pm_flags is set alongside marking the card suspended, and
> > > cleared in _mmc_resume(), so host controller resume handlers can tell
> > > power was preserved if they need to.
> >
> > While I certainly appreciate a descriptive commit message, this is a
> > bit overwhelming. Please consider making commit messages a bit less
> > wordy and more to the point.
> >
> > By reading the above, it becomes clear to me that we should really
> > split this patch. Both for reviewing purposes, but also to use the
> > step-by-step approach.
> >
> > I believe the first the patch should take care of the updated
> > interpretation of MMC_PM_KEEP_POWER for eMMC and a second patch on top
> > should deal with the "reset-card-at-resume" thing.
> >
> > Anyway, I understand the code in the mmc core for power management is
> > somewhat tricky to follow. Rather than continue to provide you with
> > in-depth guidance and review comments, allow me to spend a couple of
> > days trying to make the implementation for the above and send out a
> > few patches that you can test.
> >
>
> Please let me know if you want me to fix the above issues and send a v10.

Apologize for the delay, I am on it - expect a few patches to be
posted beginning of the next week.

Kind regards
Uffe

>
> Kamal
>
> > Kind regards
> > Uffe
> >
> > >
> > > Reported-by: Florian Fainelli <florian.fainelli@xxxxxxxxxxxx>
> > > Closes: https://lore.kernel.org/r/20260413180551.3683969-1-florian.fainelli@xxxxxxxxxxxx/
> > > Signed-off-by: Kamal Dasu <kamal.dasu@xxxxxxxxxxxx>
> > > ---
> > > Changes in v9:
> > >   - Sashiko's AI review found two more High severity issues in the
> > >     same shutdown/undervoltage fallback added in v8:
> > >       * Calling mmc_select_card() straight after finding the card
> > >         still marked suspended assumed the card would respond to
> > >         CMD7 as-is, but that's exactly the assumption
> > >         reset-card-at-resume exists to distrust -- firmware may have
> > >         left the card unable to decode commands at whatever
> > >         clock/timing was negotiated before suspend. A CMD7 timeout
> > >         there hit "goto out" and skipped the power-off sequence
> > >         entirely, the same failure mode this fallback was added to
> > >         close in v8. Fixed by resetting the host to its initial bus
> > >         state before mmc_select_card(), the same way _mmc_resume()
> > >         already does before touching the card.
> > >       * Even with that reset, mmc_select_card() can still legitimately
> > >         fail (card genuinely gone). The code still unconditionally
> > >         "goto out" in that case, skipping mmc_power_off() entirely --
> > >         but mmc_power_off() is host/regulator-side only and doesn't
> > >         need a responsive card, so there's no reason a select failure
> > >         should leave the supply on indefinitely for a shutdown or
> > >         undervoltage event, or leave pm_flags/suspended state stuck
> > >         retrying the same failing select on every later call. Fixed
> > >         by falling through to mmc_power_off() directly on select
> > >         failure instead of aborting.
> > >   - Patches 1/3 and 2/3 are unchanged from v8.
> > >
> > > Changes in v8:
> > >   - Sashiko's AI review of v7 found a High severity issue: the
> > >     keep-power fast path marks the card suspended without powering
> > >     it off, but the pre-existing early exit at the top of
> > >     _mmc_suspend() (for an already-suspended card) doesn't know that
> > >     -- it just returns immediately regardless of pm_type. Since the
> > >     system-resume path only re-enables runtime PM rather than calling
> > >     _mmc_resume() directly (that happens lazily, via runtime PM, on
> > >     the card's next real access), a shutdown, unbind or undervoltage
> > >     event landing before that next access would hit the early exit
> > >     and silently skip mmc_poweroff_notify()/mmc_power_off() entirely.
> > >     Fixed by reselecting the card and continuing into the normal
> > >     power-off sequence whenever the card is suspended via our fast
> > >     path but pm_type isn't a plain suspend, instead of a bare early
> > >     exit. Calling _mmc_resume() itself from here isn't an option, since
> > >     _mmc_suspend() already holds the host claim.
> > >
> > > Changes in v7:
> > >   - Sashiko's AI review of v6 found two real, complementary bugs in
> > >     treating MMC_PM_KEEP_POWER and MMC_CAP2_RESET_AT_RESUME as fully
> > >     independent in this driver:
> > >       * keep-power-in-suspend without reset-card-at-resume: confirmed
> > >         on hardware to hang -- _mmc_resume()'s mmc_power_up() no-ops
> > >         since power_mode never left MMC_POWER_ON, so mmc_init_card()
> > >         runs at the pre-suspend bus speed and CMD1 times out.
> > >       * reset-card-at-resume without keep-power-in-suspend: the reset
> > >         block ran mmc_set_clock()/mmc_set_initial_state() ahead of
> > >         mmc_power_up(), while power_mode was still MMC_POWER_OFF from
> > >         a normal suspend-time mmc_power_off() -- driving the clock
> > >         and bus lines before the card's supply is enabled.
> > >     Fixed by requiring both capabilities together for the suspend
> > >     fast path, and checking pm_flags (not the raw capability) for the
> > >     resume-side reset, so it only ever runs when power was actually
> > >     kept this cycle. Verified on hardware: the keep-power-in-suspend-
> > >     without-reset-card-at-resume case now correctly falls through to
> > >     a normal power-off/power-on cycle instead of hanging, and the
> > >     paired-capability case is unaffected (still hardware-verified,
> > >     now with an extra confirmation run after this fix).
> > >
> > > Changes in v6:
> > >   - Reworked around Ulf's two-property split: dropped the
> > >     unconditional mmc_set_clock()/mmc_set_initial_state() reset from
> > >     the suspend-side fast path, and instead perform it in
> > >     _mmc_resume(), gated on the new MMC_CAP2_RESET_AT_RESUME (from
> > >     reset-card-at-resume), matching the property's name and
> > >     description ("before the card can be used, it must be reset").
> > >   - No longer touches MMC_CAP2_NO_POWEROFF_SUSPEND/no-mmc-poweroff-
> > >     suspend at all -- that capability and property are gone, per the
> > >     v4 rework; this patch only adds MMC_CAP2_RESET_AT_RESUME.
> > >
> > > Changes in v5:
> > >   - Only set host->pm_flags |= MMC_PM_KEEP_POWER after
> > >     mmc_deselect_cards() succeeds, instead of unconditionally before
> > >     it. Otherwise, if the deselect fails, the card is never marked
> > >     suspended, _mmc_resume() takes its early exit, and the flag never
> > >     gets cleared -- leaking it for the rest of uptime.
> > >
> > > Changes in v4:
> > >   - Gated the fast path on pm_type == MMC_POWEROFF_SUSPEND; it was
> > >     previously unconditional, so it wrongly skipped the required
> > >     power-off/notify handling during shutdown, unbind and
> > >     undervoltage as well.
> > >   - Set/clear host->pm_flags |= MMC_PM_KEEP_POWER around the suspend/
> > >     resume, mirroring the SDIO convention, so host controller resume
> > >     handlers can tell power was preserved and perform a soft resume
> > >     sequence instead of assuming power was lost.
> > >   - Dropped MMC_CAP2_NO_POWEROFF_SUSPEND and the no-mmc-poweroff-
> > >     suspend DT property entirely. Reuse keep-power-in-suspend /
> > >     MMC_PM_KEEP_POWER instead, per Krzysztof's point that the new
> > >     property described the same contract as the existing one.
> > >     _mmc_suspend() now checks host->pm_caps directly rather than
> > >     pm_flags, since (e)MMC has no per-function driver to make the
> > >     dynamic sdio_set_host_pm_flags()-style request SDIO uses.
> > >
> > > Changes in v3:
> > >   - Reworked _mmc_suspend() to skip poweroff-notify/sleep/power-off
> > >     entirely, not just SLEEP, per Ulf.
> > >   - Renamed to MMC_CAP2_NO_POWEROFF_SUSPEND/no-mmc-poweroff-suspend.
> > >
> > > Changes in v2:
> > >   - Replaced the card-level MMC_QUIRK_BROKEN_SLEEP quirk with a host
> > >     capability, per Ulf.
> > >   - Added Reported-by/Closes crediting Florian.
> > >
> > >  drivers/mmc/core/host.c  |  2 +
> > >  drivers/mmc/core/mmc.c   | 98 +++++++++++++++++++++++++++++++++++++++-
> > >  include/linux/mmc/host.h |  1 +
> > >  3 files changed, 99 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
> > > index b7ce3137d452..1622f7846441 100644
> > > --- a/drivers/mmc/core/host.c
> > > +++ b/drivers/mmc/core/host.c
> > > @@ -400,6 +400,8 @@ int mmc_of_parse(struct mmc_host *host)
> > >         if (device_property_read_bool(dev, "no-mmc-hs400"))
> > >                 host->caps2 &= ~(MMC_CAP2_HS400_1_8V | MMC_CAP2_HS400_1_2V |
> > >                                  MMC_CAP2_HS400_ES);
> > > +       if (device_property_read_bool(dev, "reset-card-at-resume"))
> > > +               host->caps2 |= MMC_CAP2_RESET_AT_RESUME;
> > >
> > >         /* Must be after "non-removable" check */
> > >         if (device_property_read_u32(dev, "fixed-emmc-driver-type", &drv_type) == 0) {
> > > diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> > > index 05444ecf3909..3a0855a4ca44 100644
> > > --- a/drivers/mmc/core/mmc.c
> > > +++ b/drivers/mmc/core/mmc.c
> > > @@ -2144,8 +2144,55 @@ static int _mmc_suspend(struct mmc_host *host, enum mmc_poweroff_type pm_type)
> > >
> > >         mmc_claim_host(host);
> > >
> > > -       if (mmc_card_suspended(host->card))
> > > -               goto out;
> > > +       if (mmc_card_suspended(host->card)) {
> > > +               /*
> > > +                * Nothing to do for a redundant suspend call. Otherwise, the
> > > +                * card can only still be marked suspended here because the
> > > +                * keep-power fast path below left it powered and merely
> > > +                * deselected -- reselect it and continue into the normal
> > > +                * power-off sequence below, since shutdown, unbind and
> > > +                * undervoltage need mmc_power_off() regardless of how the
> > > +                * card got here.
> > > +                */
> > > +               if (pm_type == MMC_POWEROFF_SUSPEND ||
> > > +                   !(host->pm_flags & MMC_PM_KEEP_POWER))
> > > +                       goto out;
> > > +
> > > +               /*
> > > +                * Firmware may have accessed the card while it stayed
> > > +                * powered through suspend, leaving it in a state the kernel
> > > +                * can no longer assume it knows. Reset the host to its
> > > +                * initial bus state first, same as _mmc_resume() does before
> > > +                * touching the card, so mmc_select_card() below is talking
> > > +                * at a clock/timing every card is guaranteed to decode
> > > +                * rather than whatever mode was negotiated before suspend.
> > > +                */
> > > +               mmc_set_clock(host, host->f_init);
> > > +               mmc_set_initial_state(host);
> > > +
> > > +               if (!mmc_host_is_spi(host))
> > > +                       err = mmc_select_card(host->card);
> > > +
> > > +               if (err) {
> > > +                       /*
> > > +                        * The card still isn't responding even after the
> > > +                        * reset above -- it may genuinely be gone. Nothing
> > > +                        * else below needs a responsive card except
> > > +                        * mmc_power_off() itself, which is host/regulator
> > > +                        * side only, so cut power directly instead of
> > > +                        * leaving the card's supply on indefinitely (and
> > > +                        * pm_flags/suspended state stuck retrying this same
> > > +                        * failing select on every later call).
> > > +                        */
> > > +                       mmc_power_off(host);
> > > +                       host->pm_flags &= ~MMC_PM_KEEP_POWER;
> > > +                       mmc_card_set_suspended(host->card);
> > > +                       goto out;
> > > +               }
> > > +
> > > +               mmc_card_clr_suspended(host->card);
> > > +               host->pm_flags &= ~MMC_PM_KEEP_POWER;
> > > +       }
> > >
> > >         /*
> > >          * For the undervoltage case, we care more about device integrity.
> > > @@ -2157,6 +2204,33 @@ static int _mmc_suspend(struct mmc_host *host, enum mmc_poweroff_type pm_type)
> > >                         goto out;
> > >         }
> > >
> > > +       /*
> > > +        * Keep the card powered across an actual suspend; shutdown, unbind
> > > +        * and undervoltage still need the normal power-off path below,
> > > +        * since they aren't guaranteed a subsequent _mmc_resume().
> > > +        *
> > > +        * Check pm_caps, not pm_flags: unlike SDIO, (e)MMC has no
> > > +        * per-function driver to request this via
> > > +        * sdio_set_host_pm_flags(), so it's a fixed platform trait here.
> > > +        *
> > > +        * Require MMC_CAP2_RESET_AT_RESUME too: without it, _mmc_resume()
> > > +        * has no way to bring the host back to a state mmc_init_card() can
> > > +        * use, since mmc_power_up() no-ops when power_mode is already
> > > +        * MMC_POWER_ON. Keeping power without also resetting at resume is
> > > +        * not a safe combination for this driver.
> > > +        */
> > > +       if (pm_type == MMC_POWEROFF_SUSPEND &&
> > > +           (host->pm_caps & MMC_PM_KEEP_POWER) &&
> > > +           (host->caps2 & MMC_CAP2_RESET_AT_RESUME)) {
> > > +               if (!mmc_host_is_spi(host))
> > > +                       err = mmc_deselect_cards(host);
> > > +               if (!err) {
> > > +                       host->pm_flags |= MMC_PM_KEEP_POWER;
> > > +                       mmc_card_set_suspended(host->card);
> > > +               }
> > > +               goto out;
> > > +       }
> > > +
> > >         if (mmc_card_can_poweroff_notify(host->card) &&
> > >             mmc_host_can_poweroff_notify(host, pm_type))
> > >                 err = mmc_poweroff_notify(host->card, notify_type);
> > > @@ -2217,9 +2291,29 @@ static int _mmc_resume(struct mmc_host *host)
> > >         if (!mmc_card_suspended(host->card))
> > >                 goto out;
> > >
> > > +       /*
> > > +        * Firmware or other hardware may have accessed the card while it
> > > +        * stayed powered through suspend, leaving it in a state the kernel
> > > +        * can no longer assume it knows. Reset the host to its initial bus
> > > +        * state like _mmc_hw_reset() does for a non-power-cycle reset,
> > > +        * before mmc_init_card() re-identifies the card.
> > > +        *
> > > +        * Check pm_flags, not the MMC_CAP2_RESET_AT_RESUME capability
> > > +        * directly: pm_flags only ends up set here when _mmc_suspend()
> > > +        * actually took the keep-power fast path this cycle, which is the
> > > +        * only time power_mode is guaranteed to still be MMC_POWER_ON (and
> > > +        * so the only time this reset is both necessary and safe to do
> > > +        * before mmc_power_up() touches the bus).
> > > +        */
> > > +       if (host->pm_flags & MMC_PM_KEEP_POWER) {
> > > +               mmc_set_clock(host, host->f_init);
> > > +               mmc_set_initial_state(host);
> > > +       }
> > > +
> > >         mmc_power_up(host, host->card->ocr);
> > >         err = mmc_init_card(host, host->card->ocr, host->card);
> > >         mmc_card_clr_suspended(host->card);
> > > +       host->pm_flags &= ~MMC_PM_KEEP_POWER;
> > >
> > >  out:
> > >         mmc_release_host(host);
> > > diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> > > index ba84f02c2a10..14a407a9f9b7 100644
> > > --- a/include/linux/mmc/host.h
> > > +++ b/include/linux/mmc/host.h
> > > @@ -463,6 +463,7 @@ struct mmc_host {
> > >  #define MMC_CAP2_CRYPTO                0
> > >  #endif
> > >  #define MMC_CAP2_ALT_GPT_TEGRA (1 << 28)       /* Host with eMMC that has GPT entry at a non-standard location */
> > > +#define MMC_CAP2_RESET_AT_RESUME (1 << 29)     /* Card must be reset before use at resume */
> > >
> > >         bool                    uhs2_sd_tran;   /* UHS-II flag for SD_TRAN state */
> > >         bool                    uhs2_app_cmd;   /* UHS-II flag for APP command */
> > > --
> > > 2.34.1
> > >