Message-ID: <CAPDyKFqczOb2i6XHCmTyLr-AOm89zdnP8Ti-xNMEJxCrR6zOjw@mail.gmail.com>
Date: Fri, 4 Jul 2025 15:06:33 +0200
From: Ulf Hansson <ulf.hansson@...aro.org>
To: Oleksij Rempel <o.rempel@...gutronix.de>
Cc: kernel@...gutronix.de, linux-kernel@...r.kernel.org, 
	linux-mmc@...r.kernel.org, Greg Kroah-Hartman <gregkh@...uxfoundation.org>, 
	Mark Brown <broonie@...nel.org>, "Rafael J. Wysocki" <rafael@...nel.org>, Søren Andersen <san@...v.dk>, 
	Christian Loehle <christian.loehle@....com>, Adrian Hunter <adrian.hunter@...el.com>, 
	Avri Altman <Avri.Altman@...disk.com>
Subject: Re: [PATCH v6 3/4] mmc: core: add undervoltage handler for MMC/eMMC devices

On Mon, 16 Jun 2025 at 12:29, Oleksij Rempel <o.rempel@...gutronix.de> wrote:
>
> Introduce `_mmc_handle_undervoltage()` to handle undervoltage events for
> MMC/eMMC devices. The handler performs a controlled emergency suspend and
> then marks the card as removed to prevent further I/O.
>
> This is achieved by calling a new internal helper, `__mmc_suspend()`,
> with `MMC_POWEROFF_UNDERVOLTAGE`. This ensures a fast power-down sequence
> by using the short power-off notification and skipping the cache flush.
> If power-off notify is not supported, it falls back to sleep or deselect.
>
> Signed-off-by: Oleksij Rempel <o.rempel@...gutronix.de>
> ---
> changes v6:
> - Refactor suspend logic: move cache flush skipping during undervoltage
>   to a separate, preceding commit.
> - update commit message
> changes v5:
> - Rebased on top of patch introducing enum mmc_poweroff_type
> - Updated call to __mmc_suspend() to use MMC_POWEROFF_UNDERVOLTAGE
> - Dropped __mmc_resume() helper, as it is no longer needed
> - Updated commit message to reflect API change and code removal
> changes v4:
> - Drop HPI step.
> changes v3:
> - reword commit message.
> - add comments in the code
> - do not try to resume sleeping device
> ---
>  drivers/mmc/core/mmc.c | 76 ++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 73 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index 6812df679ba9..fe4fc2ad261e 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -2120,7 +2120,7 @@ static int _mmc_flush_cache(struct mmc_host *host)
>         return err;
>  }
>
> -static int _mmc_suspend(struct mmc_host *host, enum mmc_poweroff_type pm_type)
> +static int __mmc_suspend(struct mmc_host *host, enum mmc_poweroff_type pm_type)
>  {
>         unsigned int notify_type = EXT_CSD_POWER_OFF_SHORT;
>         int err = 0;
> @@ -2128,8 +2128,6 @@ static int _mmc_suspend(struct mmc_host *host, enum mmc_poweroff_type pm_type)
>         if (pm_type == MMC_POWEROFF_SHUTDOWN)
>                 notify_type = EXT_CSD_POWER_OFF_LONG;
>
> -       mmc_claim_host(host);
> -
>         if (mmc_card_suspended(host->card))
>                 goto out;
>
> @@ -2156,7 +2154,17 @@ static int _mmc_suspend(struct mmc_host *host, enum mmc_poweroff_type pm_type)
>                 mmc_card_set_suspended(host->card);
>         }
>  out:
> +       return err;
> +}
> +
> +static int _mmc_suspend(struct mmc_host *host, enum mmc_poweroff_type pm_type)
> +{
> +       int err;
> +
> +       mmc_claim_host(host);
> +       err = __mmc_suspend(host, pm_type);
>         mmc_release_host(host);
> +
>         return err;
>  }
>
> @@ -2219,6 +2227,13 @@ static int mmc_shutdown(struct mmc_host *host)
>  {
>         int err = 0;
>
> +       /*
> +        * In case of undervoltage, the card will be powered off by
> +        * _mmc_handle_undervoltage()
> +        */
> +       if (host->undervoltage)
> +               return 0;

Maybe use mmc_card_removed() instead?

BTW, I wonder if there are any other places where we need to add more
bailout points... Let me think a bit more about this...

> +
>         /*
>          * If the card remains suspended at this point and it was done by using
>          * the sleep-cmd (CMD5), we may need to re-initialize it first, to allow
> @@ -2309,6 +2324,60 @@ static int _mmc_hw_reset(struct mmc_host *host)
>         return mmc_init_card(host, card->ocr, card);
>  }
>
> +/**
> + * _mmc_handle_undervoltage - Handle an undervoltage event for MMC/eMMC devices
> + * @host: MMC host structure
> + *
> + * This function is triggered when an undervoltage condition is detected.
> + * It attempts to transition the device into a low-power or safe state to
> + * prevent data corruption.
> + *
> + * Steps performed:
> + * 1. If no card is present, return immediately.
> + * 2. Perform an emergency suspend using EXT_CSD_POWER_OFF_SHORT if possible.
> + *    - If power-off notify is not supported, fallback mechanisms like sleep or
> + *      deselecting the card are attempted.
> + *    - Cache flushing is skipped to reduce execution time.
> + * 3. Mark the card as removed to prevent further interactions after
> + *    undervoltage.
> + *
> + * Note: This function does not handle host claiming or releasing. The caller
> + *      must ensure that the host is properly claimed before calling this
> + *      function and released afterward.
> + *
> + * Returns: 0 on success, or a negative error code if any step fails.
> + */
> +static int _mmc_handle_undervoltage(struct mmc_host *host)
> +{
> +       struct mmc_card *card = host->card;
> +       int err;
> +
> +       /* If there is no card attached, nothing to do */
> +       if (!card)
> +               return 0;

This check should not be needed. Refer to my comments on patch1 for
more information.

> +
> +       /*
> +        * Perform an emergency suspend to power off the eMMC quickly.
> +        * This ensures the device enters a safe state before power is lost.
> +        * We first attempt EXT_CSD_POWER_OFF_SHORT, but if power-off notify
> +        * is not supported, we fall back to sleep mode or deselecting the card.
> +        * Cache flushing is skipped to minimize delay.
> +        */
> +       err = __mmc_suspend(host, MMC_POWEROFF_UNDERVOLTAGE);
> +       if (err)
> +               pr_err("%s: undervoltage suspend failed: %pe\n",
> +                      mmc_hostname(host), ERR_PTR(err));
> +
> +       /*
> +        * Mark the card as removed to prevent further operations.
> +        * This ensures the system does not attempt to access the device
> +        * after an undervoltage event, avoiding potential corruption.
> +        */
> +       mmc_card_set_removed(card);
> +
> +       return err;
> +}
> +
>  static const struct mmc_bus_ops mmc_ops = {
>         .remove = mmc_remove,
>         .detect = mmc_detect,
> @@ -2321,6 +2390,7 @@ static const struct mmc_bus_ops mmc_ops = {
>         .hw_reset = _mmc_hw_reset,
>         .cache_enabled = _mmc_cache_enabled,
>         .flush_cache = _mmc_flush_cache,
> +       .handle_undervoltage = _mmc_handle_undervoltage,
>  };
>
>  /*
> --
> 2.39.5
>

Kind regards
Uffe

Powered by blists - more mailing lists