Linux MultiMedia Card development
 help / color / mirror / Atom feed
* [PATCH v1 1/1] mmc: core: Handle undervoltage events and register regulator notifiers
@ 2025-02-12 13:24 Oleksij Rempel
  2025-02-12 23:47 ` Christian Loehle
  2025-02-13 10:14 ` Avri Altman
  0 siblings, 2 replies; 6+ messages in thread
From: Oleksij Rempel @ 2025-02-12 13:24 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: Oleksij Rempel, kernel, linux-kernel, linux-mmc,
	Greg Kroah-Hartman, Mark Brown, Rafael J. Wysocki

Extend the MMC core to handle undervoltage events by implementing
infrastructure to notify the MMC bus about voltage drops.

Background & Decision at LPC24:

This solution was proposed and refined during LPC24 in the talk
"Graceful Under Pressure: Prioritizing Shutdown to Protect Your Data in
Embedded Systems" which aimed to address how Linux should handle power
fluctuations in embedded devices to prevent data corruption or storage
damage.

At the time, multiple possible solutions were considered:

1. Triggering a system-wide suspend or shutdown: when undervoltage is
   detected, with device-specific prioritization to ensure critical
   components shut down first.
   - This approach was disliked by Greg Kroah-Hartman, as it introduced
     complexity and was not suitable for all use cases.

2. Notifying relevant devices through the regulator framework: to allow
   graceful per-device handling.
   - This approach was agreed upon as the most acceptable: by participants
     in the discussion, including Greg Kroah-Hartman, Mark Brown,
     and Rafael J. Wysocki.
   - This patch implements that decision by integrating undervoltage
     handling into the MMC subsystem.

This patch was tested on iMX8MP based system with SDHCI controller.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/mmc/core/card.h      |  5 ++
 drivers/mmc/core/core.c      | 29 ++++++++++++
 drivers/mmc/core/core.h      |  2 +
 drivers/mmc/core/mmc.c       |  6 +++
 drivers/mmc/core/regulator.c | 90 ++++++++++++++++++++++++++++++++++++
 include/linux/mmc/host.h     |  4 ++
 6 files changed, 136 insertions(+)

diff --git a/drivers/mmc/core/card.h b/drivers/mmc/core/card.h
index 3205feb1e8ff..f8341e1657f0 100644
--- a/drivers/mmc/core/card.h
+++ b/drivers/mmc/core/card.h
@@ -24,6 +24,9 @@
 #define MMC_CARD_REMOVED	(1<<4)		/* card has been removed */
 #define MMC_STATE_SUSPENDED	(1<<5)		/* card is suspended */
 #define MMC_CARD_SDUC		(1<<6)		/* card is SDUC */
+#define MMC_CARD_UNDERVOLTAGE   (1<<7)		/* card is in undervoltage
+						 * condition
+						 */
 
 #define mmc_card_present(c)	((c)->state & MMC_STATE_PRESENT)
 #define mmc_card_readonly(c)	((c)->state & MMC_STATE_READONLY)
@@ -32,6 +35,7 @@
 #define mmc_card_removed(c)	((c) && ((c)->state & MMC_CARD_REMOVED))
 #define mmc_card_suspended(c)	((c)->state & MMC_STATE_SUSPENDED)
 #define mmc_card_ult_capacity(c) ((c)->state & MMC_CARD_SDUC)
+#define mmc_card_in_undervoltage(c) ((c)->state & MMC_CARD_UNDERVOLTAGE)
 
 #define mmc_card_set_present(c)	((c)->state |= MMC_STATE_PRESENT)
 #define mmc_card_set_readonly(c) ((c)->state |= MMC_STATE_READONLY)
@@ -41,6 +45,7 @@
 #define mmc_card_set_removed(c) ((c)->state |= MMC_CARD_REMOVED)
 #define mmc_card_set_suspended(c) ((c)->state |= MMC_STATE_SUSPENDED)
 #define mmc_card_clr_suspended(c) ((c)->state &= ~MMC_STATE_SUSPENDED)
+#define mmc_card_set_undervoltage(c) ((c)->state |= MMC_CARD_UNDERVOLTAGE)
 
 /*
  * The world is not perfect and supplies us with broken mmc/sdio devices.
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 5241528f8b90..4b94017d2600 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1399,6 +1399,35 @@ void mmc_power_cycle(struct mmc_host *host, u32 ocr)
 	mmc_power_up(host, ocr);
 }
 
+/**
+ * mmc_handle_undervoltage - Handle an undervoltage event on the MMC bus
+ * @host: The MMC host that detected the undervoltage condition
+ *
+ * This function is called when an undervoltage event is detected on one of
+ * the MMC regulators.
+ *
+ * Returns: 0 on success or a negative error code on failure.
+ */
+int mmc_handle_undervoltage(struct mmc_host *host)
+{
+	spin_lock(&host->lock);
+
+	if (!host->card || mmc_card_in_undervoltage(host->card)) {
+		spin_unlock(&host->lock);
+		return 0;
+	}
+
+	/* Mark the card as in undervoltage condition */
+	mmc_card_set_undervoltage(host->card);
+	spin_unlock(&host->lock);
+
+	/* Call bus-specific undervoltage handler if available */
+	if (host->bus_ops && host->bus_ops->handle_undervoltage)
+		return host->bus_ops->handle_undervoltage(host);
+
+	return 0;
+}
+
 /*
  * Assign a mmc bus handler to a host. Only one bus handler may control a
  * host at any given time.
diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h
index fc9c066e6468..578c98e2f04d 100644
--- a/drivers/mmc/core/core.h
+++ b/drivers/mmc/core/core.h
@@ -31,6 +31,7 @@ struct mmc_bus_ops {
 	int (*sw_reset)(struct mmc_host *);
 	bool (*cache_enabled)(struct mmc_host *);
 	int (*flush_cache)(struct mmc_host *);
+	int (*handle_undervoltage)(struct mmc_host *);
 };
 
 void mmc_attach_bus(struct mmc_host *host, const struct mmc_bus_ops *ops);
@@ -59,6 +60,7 @@ void mmc_power_off(struct mmc_host *host);
 void mmc_power_cycle(struct mmc_host *host, u32 ocr);
 void mmc_set_initial_state(struct mmc_host *host);
 u32 mmc_vddrange_to_ocrmask(int vdd_min, int vdd_max);
+int mmc_handle_undervoltage(struct mmc_host *host);
 
 static inline void mmc_delay(unsigned int ms)
 {
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 6a23be214543..c8b8e7a7b7d6 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -2273,6 +2273,11 @@ static int _mmc_hw_reset(struct mmc_host *host)
 	return mmc_init_card(host, card->ocr, card);
 }
 
+static int _mmc_handle_undervoltage(struct mmc_host *host)
+{
+	return mmc_shutdown(host);
+}
+
 static const struct mmc_bus_ops mmc_ops = {
 	.remove = mmc_remove,
 	.detect = mmc_detect,
@@ -2285,6 +2290,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,
 };
 
 /*
diff --git a/drivers/mmc/core/regulator.c b/drivers/mmc/core/regulator.c
index 3dae2e9b7978..0723afb2f9ed 100644
--- a/drivers/mmc/core/regulator.c
+++ b/drivers/mmc/core/regulator.c
@@ -262,6 +262,81 @@ static inline int mmc_regulator_get_ocrmask(struct regulator *supply)
 
 #endif /* CONFIG_REGULATOR */
 
+static int mmc_handle_regulator_event(struct mmc_host *mmc,
+				      const char *regulator_name,
+				      unsigned long event)
+{
+	struct device *dev = mmc_dev(mmc);
+	int ret;
+
+	switch (event) {
+	case REGULATOR_EVENT_UNDER_VOLTAGE:
+		ret = mmc_handle_undervoltage(mmc);
+		if (ret)
+			dev_err(dev, "Undervoltage handling failed: %pe\n",
+				ERR_PTR(ret));
+		break;
+	default:
+		return NOTIFY_DONE;
+	}
+
+	return NOTIFY_OK;
+}
+
+static int mmc_vmmc_notifier_callback(struct notifier_block *nb,
+				      unsigned long event, void *data)
+{
+	struct mmc_supply *supply;
+	struct mmc_host *mmc;
+
+	supply = container_of(nb, struct mmc_supply, vmmc_nb);
+	mmc = container_of(supply, struct mmc_host, supply);
+
+	return mmc_handle_regulator_event(mmc, "vmmc", event);
+}
+
+static int mmc_vqmmc_notifier_callback(struct notifier_block *nb,
+				       unsigned long event, void *data)
+{
+	struct mmc_supply *supply;
+	struct mmc_host *mmc;
+
+	supply = container_of(nb, struct mmc_supply, vqmmc_nb);
+	mmc = container_of(supply, struct mmc_host, supply);
+
+	return mmc_handle_regulator_event(mmc, "vqmmc", event);
+}
+
+static int mmc_vqmmc2_notifier_callback(struct notifier_block *nb,
+					unsigned long event, void *data)
+{
+	struct mmc_supply *supply;
+	struct mmc_host *mmc;
+
+	supply = container_of(nb, struct mmc_supply, vqmmc2_nb);
+	mmc = container_of(supply, struct mmc_host, supply);
+
+	return mmc_handle_regulator_event(mmc, "vqmmc2", event);
+}
+
+static void
+mmc_register_regulator_notifier(struct mmc_host *mmc,
+				struct regulator *regulator,
+				struct notifier_block *nb,
+				int (*callback)(struct notifier_block *,
+						unsigned long, void *),
+				const char *name)
+{
+	struct device *dev = mmc_dev(mmc);
+	int ret;
+
+	nb->notifier_call = callback;
+	ret = devm_regulator_register_notifier(regulator, nb);
+	if (ret)
+		dev_warn(dev, "Failed to register %s notifier: %pe\n", name,
+			 ERR_PTR(ret));
+}
+
 /**
  * mmc_regulator_get_supply - try to get VMMC and VQMMC regulators for a host
  * @mmc: the host to regulate
@@ -293,6 +368,11 @@ int mmc_regulator_get_supply(struct mmc_host *mmc)
 			mmc->ocr_avail = ret;
 		else
 			dev_warn(dev, "Failed getting OCR mask: %d\n", ret);
+
+		mmc_register_regulator_notifier(mmc, mmc->supply.vmmc,
+						&mmc->supply.vmmc_nb,
+						mmc_vmmc_notifier_callback,
+						"vmmc");
 	}
 
 	if (IS_ERR(mmc->supply.vqmmc)) {
@@ -301,12 +381,22 @@ int mmc_regulator_get_supply(struct mmc_host *mmc)
 					     "vqmmc regulator not available\n");
 
 		dev_dbg(dev, "No vqmmc regulator found\n");
+	} else {
+		mmc_register_regulator_notifier(mmc, mmc->supply.vqmmc,
+						&mmc->supply.vqmmc_nb,
+						mmc_vqmmc_notifier_callback,
+						"vqmmc");
 	}
 
 	if (IS_ERR(mmc->supply.vqmmc2)) {
 		if (PTR_ERR(mmc->supply.vqmmc2) == -EPROBE_DEFER)
 			return -EPROBE_DEFER;
 		dev_dbg(dev, "No vqmmc2 regulator found\n");
+	} else {
+		mmc_register_regulator_notifier(mmc, mmc->supply.vqmmc2,
+						&mmc->supply.vqmmc2_nb,
+						mmc_vqmmc2_notifier_callback,
+						"vqmmc2");
 	}
 
 	return 0;
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 68f09a955a90..7da053095c63 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -342,6 +342,10 @@ struct mmc_supply {
 	struct regulator *vmmc;		/* Card power supply */
 	struct regulator *vqmmc;	/* Optional Vccq supply */
 	struct regulator *vqmmc2;	/* Optional supply for phy */
+
+	struct notifier_block vmmc_nb;		/* Notifier for vmmc */
+	struct notifier_block vqmmc_nb;		/* Notifier for vqmmc */
+	struct notifier_block vqmmc2_nb;	/* Notifier for vqmmc2 */
 };
 
 struct mmc_ctx {
-- 
2.39.5


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v1 1/1] mmc: core: Handle undervoltage events and register regulator notifiers
  2025-02-12 13:24 [PATCH v1 1/1] mmc: core: Handle undervoltage events and register regulator notifiers Oleksij Rempel
@ 2025-02-12 23:47 ` Christian Loehle
  2025-02-13  8:57   ` Oleksij Rempel
  2025-02-13 10:14 ` Avri Altman
  1 sibling, 1 reply; 6+ messages in thread
From: Christian Loehle @ 2025-02-12 23:47 UTC (permalink / raw)
  To: Oleksij Rempel, Ulf Hansson
  Cc: kernel, linux-kernel, linux-mmc, Greg Kroah-Hartman, Mark Brown,
	Rafael J. Wysocki

On 2/12/25 13:24, Oleksij Rempel wrote:
> Extend the MMC core to handle undervoltage events by implementing
> infrastructure to notify the MMC bus about voltage drops.
> 
> Background & Decision at LPC24:
> 
> This solution was proposed and refined during LPC24 in the talk
> "Graceful Under Pressure: Prioritizing Shutdown to Protect Your Data in
> Embedded Systems" which aimed to address how Linux should handle power
> fluctuations in embedded devices to prevent data corruption or storage
> damage.
> 
> At the time, multiple possible solutions were considered:
> 
> 1. Triggering a system-wide suspend or shutdown: when undervoltage is
>    detected, with device-specific prioritization to ensure critical
>    components shut down first.
>    - This approach was disliked by Greg Kroah-Hartman, as it introduced
>      complexity and was not suitable for all use cases.
> 
> 2. Notifying relevant devices through the regulator framework: to allow
>    graceful per-device handling.
>    - This approach was agreed upon as the most acceptable: by participants
>      in the discussion, including Greg Kroah-Hartman, Mark Brown,
>      and Rafael J. Wysocki.
>    - This patch implements that decision by integrating undervoltage
>      handling into the MMC subsystem.
> 
> This patch was tested on iMX8MP based system with SDHCI controller.

Any details here? How long does it take from undervoltage to
poweroff notification.
Roughly how long of a heads up would that yield in realistic
undervoltage scenarios?

> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  drivers/mmc/core/card.h      |  5 ++
>  drivers/mmc/core/core.c      | 29 ++++++++++++
>  drivers/mmc/core/core.h      |  2 +
>  drivers/mmc/core/mmc.c       |  6 +++
>  drivers/mmc/core/regulator.c | 90 ++++++++++++++++++++++++++++++++++++
>  include/linux/mmc/host.h     |  4 ++
>  6 files changed, 136 insertions(+)
> 
> diff --git a/drivers/mmc/core/card.h b/drivers/mmc/core/card.h
> index 3205feb1e8ff..f8341e1657f0 100644
> --- a/drivers/mmc/core/card.h
> +++ b/drivers/mmc/core/card.h
> @@ -24,6 +24,9 @@
>  #define MMC_CARD_REMOVED	(1<<4)		/* card has been removed */
>  #define MMC_STATE_SUSPENDED	(1<<5)		/* card is suspended */
>  #define MMC_CARD_SDUC		(1<<6)		/* card is SDUC */
> +#define MMC_CARD_UNDERVOLTAGE   (1<<7)		/* card is in undervoltage
> +						 * condition
> +						 */
>  
>  #define mmc_card_present(c)	((c)->state & MMC_STATE_PRESENT)
>  #define mmc_card_readonly(c)	((c)->state & MMC_STATE_READONLY)
> @@ -32,6 +35,7 @@
>  #define mmc_card_removed(c)	((c) && ((c)->state & MMC_CARD_REMOVED))
>  #define mmc_card_suspended(c)	((c)->state & MMC_STATE_SUSPENDED)
>  #define mmc_card_ult_capacity(c) ((c)->state & MMC_CARD_SDUC)
> +#define mmc_card_in_undervoltage(c) ((c)->state & MMC_CARD_UNDERVOLTAGE)
>  
>  #define mmc_card_set_present(c)	((c)->state |= MMC_STATE_PRESENT)
>  #define mmc_card_set_readonly(c) ((c)->state |= MMC_STATE_READONLY)
> @@ -41,6 +45,7 @@
>  #define mmc_card_set_removed(c) ((c)->state |= MMC_CARD_REMOVED)
>  #define mmc_card_set_suspended(c) ((c)->state |= MMC_STATE_SUSPENDED)
>  #define mmc_card_clr_suspended(c) ((c)->state &= ~MMC_STATE_SUSPENDED)
> +#define mmc_card_set_undervoltage(c) ((c)->state |= MMC_CARD_UNDERVOLTAGE)
>  
>  /*
>   * The world is not perfect and supplies us with broken mmc/sdio devices.
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 5241528f8b90..4b94017d2600 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -1399,6 +1399,35 @@ void mmc_power_cycle(struct mmc_host *host, u32 ocr)
>  	mmc_power_up(host, ocr);
>  }
>  
> +/**
> + * mmc_handle_undervoltage - Handle an undervoltage event on the MMC bus
> + * @host: The MMC host that detected the undervoltage condition
> + *
> + * This function is called when an undervoltage event is detected on one of
> + * the MMC regulators.
> + *
> + * Returns: 0 on success or a negative error code on failure.
> + */
> +int mmc_handle_undervoltage(struct mmc_host *host)
> +{
> +	spin_lock(&host->lock);
> +
> +	if (!host->card || mmc_card_in_undervoltage(host->card)) {
> +		spin_unlock(&host->lock);
> +		return 0;
> +	}
> +
> +	/* Mark the card as in undervoltage condition */
> +	mmc_card_set_undervoltage(host->card);
> +	spin_unlock(&host->lock);
> +
> +	/* Call bus-specific undervoltage handler if available */
> +	if (host->bus_ops && host->bus_ops->handle_undervoltage)
> +		return host->bus_ops->handle_undervoltage(host);
> +
> +	return 0;
> +}
> +
>  /*
>   * Assign a mmc bus handler to a host. Only one bus handler may control a
>   * host at any given time.
> diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h
> index fc9c066e6468..578c98e2f04d 100644
> --- a/drivers/mmc/core/core.h
> +++ b/drivers/mmc/core/core.h
> @@ -31,6 +31,7 @@ struct mmc_bus_ops {
>  	int (*sw_reset)(struct mmc_host *);
>  	bool (*cache_enabled)(struct mmc_host *);
>  	int (*flush_cache)(struct mmc_host *);
> +	int (*handle_undervoltage)(struct mmc_host *);
>  };
>  
>  void mmc_attach_bus(struct mmc_host *host, const struct mmc_bus_ops *ops);
> @@ -59,6 +60,7 @@ void mmc_power_off(struct mmc_host *host);
>  void mmc_power_cycle(struct mmc_host *host, u32 ocr);
>  void mmc_set_initial_state(struct mmc_host *host);
>  u32 mmc_vddrange_to_ocrmask(int vdd_min, int vdd_max);
> +int mmc_handle_undervoltage(struct mmc_host *host);
>  
>  static inline void mmc_delay(unsigned int ms)
>  {
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index 6a23be214543..c8b8e7a7b7d6 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -2273,6 +2273,11 @@ static int _mmc_hw_reset(struct mmc_host *host)
>  	return mmc_init_card(host, card->ocr, card);
>  }
>  
> +static int _mmc_handle_undervoltage(struct mmc_host *host)
> +{
> +	return mmc_shutdown(host);
> +}
> +

The poweroff notification part I understand, because it polls for busy
(i.e. hopefully until the card thinks it's done committing to flash).
Poweroff isn't always available though, the other paths of
_mmc_suspend() are:

	else if (mmc_can_sleep(host->card))
		err = mmc_sleep(host);
	else if (!mmc_host_is_spi(host))
		err = mmc_deselect_cards(host);

	if (!err) {
		mmc_power_off(host);

So we may also just deselect, which AFAIR succeeds as a FSM (i.e.
doesn't mean anything was committed to flash) and then we just
poweroff.
Is that what we want in an undervoltage scenario?

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v1 1/1] mmc: core: Handle undervoltage events and register regulator notifiers
  2025-02-12 23:47 ` Christian Loehle
@ 2025-02-13  8:57   ` Oleksij Rempel
  2025-02-13 11:13     ` Christian Loehle
  0 siblings, 1 reply; 6+ messages in thread
From: Oleksij Rempel @ 2025-02-13  8:57 UTC (permalink / raw)
  To: Christian Loehle
  Cc: Ulf Hansson, kernel, linux-kernel, linux-mmc, Greg Kroah-Hartman,
	Mark Brown, Rafael J. Wysocki

On Wed, Feb 12, 2025 at 11:47:08PM +0000, Christian Loehle wrote:
> On 2/12/25 13:24, Oleksij Rempel wrote:
> > Extend the MMC core to handle undervoltage events by implementing
> > infrastructure to notify the MMC bus about voltage drops.
> > 
> > Background & Decision at LPC24:
> > 
> > This solution was proposed and refined during LPC24 in the talk
> > "Graceful Under Pressure: Prioritizing Shutdown to Protect Your Data in
> > Embedded Systems" which aimed to address how Linux should handle power
> > fluctuations in embedded devices to prevent data corruption or storage
> > damage.
> > 
> > At the time, multiple possible solutions were considered:
> > 
> > 1. Triggering a system-wide suspend or shutdown: when undervoltage is
> >    detected, with device-specific prioritization to ensure critical
> >    components shut down first.
> >    - This approach was disliked by Greg Kroah-Hartman, as it introduced
> >      complexity and was not suitable for all use cases.
> > 
> > 2. Notifying relevant devices through the regulator framework: to allow
> >    graceful per-device handling.
> >    - This approach was agreed upon as the most acceptable: by participants
> >      in the discussion, including Greg Kroah-Hartman, Mark Brown,
> >      and Rafael J. Wysocki.
> >    - This patch implements that decision by integrating undervoltage
> >      handling into the MMC subsystem.
> > 
> > This patch was tested on iMX8MP based system with SDHCI controller.
> 
> Any details here? How long does it take from undervoltage to
> poweroff notification.

On this system, with current implementation, it takes 4.5 millisecond
from voltage drop detection to mmc_poweroff_notify.

> Roughly how long of a heads up would that yield in realistic
> undervoltage scenarios?

It depends on the board implementation and attached power supply.
In my case, the testing system provides about 100ms capacity on board.
The power supply provides additional 1-2 seconds.

If the power is cut between power supply and board, we will have max
100ms.

> > +static int _mmc_handle_undervoltage(struct mmc_host *host)
> > +{
> > +	return mmc_shutdown(host);
> > +}
> > +
> 
> The poweroff notification part I understand, because it polls for busy
> (i.e. hopefully until the card thinks it's done committing to flash).
> Poweroff isn't always available though, the other paths of
> _mmc_suspend() are:
> 
> 	else if (mmc_can_sleep(host->card))
> 		err = mmc_sleep(host);
> 	else if (!mmc_host_is_spi(host))
> 		err = mmc_deselect_cards(host);
> 
> 	if (!err) {
> 		mmc_power_off(host);
> 
> So we may also just deselect, which AFAIR succeeds as a FSM (i.e.
> doesn't mean anything was committed to flash) and then we just
> poweroff.
> Is that what we want in an undervoltage scenario?

Yes. In an undervoltage scenario, our primary priority is to protect the
hardware from damage. Data integrity is secondary in this case. The most
critical action is to immediately stop writing to the card.  

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

^ permalink raw reply	[flat|nested] 6+ messages in thread

* RE: [PATCH v1 1/1] mmc: core: Handle undervoltage events and register regulator notifiers
  2025-02-12 13:24 [PATCH v1 1/1] mmc: core: Handle undervoltage events and register regulator notifiers Oleksij Rempel
  2025-02-12 23:47 ` Christian Loehle
@ 2025-02-13 10:14 ` Avri Altman
  2025-02-13 11:09   ` Oleksij Rempel
  1 sibling, 1 reply; 6+ messages in thread
From: Avri Altman @ 2025-02-13 10:14 UTC (permalink / raw)
  To: Oleksij Rempel, Ulf Hansson
  Cc: kernel@pengutronix.de, linux-kernel@vger.kernel.org,
	linux-mmc@vger.kernel.org, Greg Kroah-Hartman, Mark Brown,
	Rafael J. Wysocki

> Extend the MMC core to handle undervoltage events by implementing
> infrastructure to notify the MMC bus about voltage drops.
> 
> Background & Decision at LPC24:
> 
> This solution was proposed and refined during LPC24 in the talk "Graceful
> Under Pressure: Prioritizing Shutdown to Protect Your Data in Embedded
> Systems" which aimed to address how Linux should handle power fluctuations
> in embedded devices to prevent data corruption or storage damage.
> 
> At the time, multiple possible solutions were considered:
> 
> 1. Triggering a system-wide suspend or shutdown: when undervoltage is
>    detected, with device-specific prioritization to ensure critical
>    components shut down first.
>    - This approach was disliked by Greg Kroah-Hartman, as it introduced
>      complexity and was not suitable for all use cases.
> 
> 2. Notifying relevant devices through the regulator framework: to allow
>    graceful per-device handling.
>    - This approach was agreed upon as the most acceptable: by participants
>      in the discussion, including Greg Kroah-Hartman, Mark Brown,
>      and Rafael J. Wysocki.
>    - This patch implements that decision by integrating undervoltage
>      handling into the MMC subsystem.
> 
> This patch was tested on iMX8MP based system with SDHCI controller.
Has it been considered, to rely on user-space and not the kernel to handle those extreme conditions?
E.g. a pollable sysfs that would be monitored by select(), poll(), etc.  As Android might use?

Thanks,
Avri 

> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  drivers/mmc/core/card.h      |  5 ++
>  drivers/mmc/core/core.c      | 29 ++++++++++++
>  drivers/mmc/core/core.h      |  2 +
>  drivers/mmc/core/mmc.c       |  6 +++
>  drivers/mmc/core/regulator.c | 90
> ++++++++++++++++++++++++++++++++++++
>  include/linux/mmc/host.h     |  4 ++
>  6 files changed, 136 insertions(+)
> 
> diff --git a/drivers/mmc/core/card.h b/drivers/mmc/core/card.h index
> 3205feb1e8ff..f8341e1657f0 100644
> --- a/drivers/mmc/core/card.h
> +++ b/drivers/mmc/core/card.h
> @@ -24,6 +24,9 @@
>  #define MMC_CARD_REMOVED	(1<<4)		/* card has been
> removed */
>  #define MMC_STATE_SUSPENDED	(1<<5)		/* card is suspended
> */
>  #define MMC_CARD_SDUC		(1<<6)		/* card is SDUC */
> +#define MMC_CARD_UNDERVOLTAGE   (1<<7)		/* card is in
> undervoltage
> +						 * condition
> +						 */
> 
>  #define mmc_card_present(c)	((c)->state & MMC_STATE_PRESENT)
>  #define mmc_card_readonly(c)	((c)->state & MMC_STATE_READONLY)
> @@ -32,6 +35,7 @@
>  #define mmc_card_removed(c)	((c) && ((c)->state &
> MMC_CARD_REMOVED))
>  #define mmc_card_suspended(c)	((c)->state &
> MMC_STATE_SUSPENDED)
>  #define mmc_card_ult_capacity(c) ((c)->state & MMC_CARD_SDUC)
> +#define mmc_card_in_undervoltage(c) ((c)->state &
> +MMC_CARD_UNDERVOLTAGE)
> 
>  #define mmc_card_set_present(c)	((c)->state |= MMC_STATE_PRESENT)
>  #define mmc_card_set_readonly(c) ((c)->state |= MMC_STATE_READONLY)
> @@ -41,6 +45,7 @@  #define mmc_card_set_removed(c) ((c)->state |=
> MMC_CARD_REMOVED)  #define mmc_card_set_suspended(c) ((c)->state |=
> MMC_STATE_SUSPENDED)  #define mmc_card_clr_suspended(c) ((c)->state
> &= ~MMC_STATE_SUSPENDED)
> +#define mmc_card_set_undervoltage(c) ((c)->state |=
> +MMC_CARD_UNDERVOLTAGE)
> 
>  /*
>   * The world is not perfect and supplies us with broken mmc/sdio devices.
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c index
> 5241528f8b90..4b94017d2600 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -1399,6 +1399,35 @@ void mmc_power_cycle(struct mmc_host *host,
> u32 ocr)
>  	mmc_power_up(host, ocr);
>  }
> 
> +/**
> + * mmc_handle_undervoltage - Handle an undervoltage event on the MMC
> +bus
> + * @host: The MMC host that detected the undervoltage condition
> + *
> + * This function is called when an undervoltage event is detected on
> +one of
> + * the MMC regulators.
> + *
> + * Returns: 0 on success or a negative error code on failure.
> + */
> +int mmc_handle_undervoltage(struct mmc_host *host) {
> +	spin_lock(&host->lock);
> +
> +	if (!host->card || mmc_card_in_undervoltage(host->card)) {
> +		spin_unlock(&host->lock);
> +		return 0;
> +	}
> +
> +	/* Mark the card as in undervoltage condition */
> +	mmc_card_set_undervoltage(host->card);
> +	spin_unlock(&host->lock);
> +
> +	/* Call bus-specific undervoltage handler if available */
> +	if (host->bus_ops && host->bus_ops->handle_undervoltage)
> +		return host->bus_ops->handle_undervoltage(host);
> +
> +	return 0;
> +}
> +
>  /*
>   * Assign a mmc bus handler to a host. Only one bus handler may control a
>   * host at any given time.
> diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h index
> fc9c066e6468..578c98e2f04d 100644
> --- a/drivers/mmc/core/core.h
> +++ b/drivers/mmc/core/core.h
> @@ -31,6 +31,7 @@ struct mmc_bus_ops {
>  	int (*sw_reset)(struct mmc_host *);
>  	bool (*cache_enabled)(struct mmc_host *);
>  	int (*flush_cache)(struct mmc_host *);
> +	int (*handle_undervoltage)(struct mmc_host *);
>  };
> 
>  void mmc_attach_bus(struct mmc_host *host, const struct mmc_bus_ops
> *ops); @@ -59,6 +60,7 @@ void mmc_power_off(struct mmc_host *host);
> void mmc_power_cycle(struct mmc_host *host, u32 ocr);  void
> mmc_set_initial_state(struct mmc_host *host);
>  u32 mmc_vddrange_to_ocrmask(int vdd_min, int vdd_max);
> +int mmc_handle_undervoltage(struct mmc_host *host);
> 
>  static inline void mmc_delay(unsigned int ms)  { diff --git
> a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c index
> 6a23be214543..c8b8e7a7b7d6 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -2273,6 +2273,11 @@ static int _mmc_hw_reset(struct mmc_host
> *host)
>  	return mmc_init_card(host, card->ocr, card);  }
> 
> +static int _mmc_handle_undervoltage(struct mmc_host *host) {
> +	return mmc_shutdown(host);
> +}
> +
>  static const struct mmc_bus_ops mmc_ops = {
>  	.remove = mmc_remove,
>  	.detect = mmc_detect,
> @@ -2285,6 +2290,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,
>  };
> 
>  /*
> diff --git a/drivers/mmc/core/regulator.c b/drivers/mmc/core/regulator.c
> index 3dae2e9b7978..0723afb2f9ed 100644
> --- a/drivers/mmc/core/regulator.c
> +++ b/drivers/mmc/core/regulator.c
> @@ -262,6 +262,81 @@ static inline int mmc_regulator_get_ocrmask(struct
> regulator *supply)
> 
>  #endif /* CONFIG_REGULATOR */
> 
> +static int mmc_handle_regulator_event(struct mmc_host *mmc,
> +				      const char *regulator_name,
> +				      unsigned long event)
> +{
> +	struct device *dev = mmc_dev(mmc);
> +	int ret;
> +
> +	switch (event) {
> +	case REGULATOR_EVENT_UNDER_VOLTAGE:
> +		ret = mmc_handle_undervoltage(mmc);
> +		if (ret)
> +			dev_err(dev, "Undervoltage handling failed: %pe\n",
> +				ERR_PTR(ret));
> +		break;
> +	default:
> +		return NOTIFY_DONE;
> +	}
> +
> +	return NOTIFY_OK;
> +}
> +
> +static int mmc_vmmc_notifier_callback(struct notifier_block *nb,
> +				      unsigned long event, void *data) {
> +	struct mmc_supply *supply;
> +	struct mmc_host *mmc;
> +
> +	supply = container_of(nb, struct mmc_supply, vmmc_nb);
> +	mmc = container_of(supply, struct mmc_host, supply);
> +
> +	return mmc_handle_regulator_event(mmc, "vmmc", event); }
> +
> +static int mmc_vqmmc_notifier_callback(struct notifier_block *nb,
> +				       unsigned long event, void *data) {
> +	struct mmc_supply *supply;
> +	struct mmc_host *mmc;
> +
> +	supply = container_of(nb, struct mmc_supply, vqmmc_nb);
> +	mmc = container_of(supply, struct mmc_host, supply);
> +
> +	return mmc_handle_regulator_event(mmc, "vqmmc", event); }
> +
> +static int mmc_vqmmc2_notifier_callback(struct notifier_block *nb,
> +					unsigned long event, void *data)
> +{
> +	struct mmc_supply *supply;
> +	struct mmc_host *mmc;
> +
> +	supply = container_of(nb, struct mmc_supply, vqmmc2_nb);
> +	mmc = container_of(supply, struct mmc_host, supply);
> +
> +	return mmc_handle_regulator_event(mmc, "vqmmc2", event); }
> +
> +static void
> +mmc_register_regulator_notifier(struct mmc_host *mmc,
> +				struct regulator *regulator,
> +				struct notifier_block *nb,
> +				int (*callback)(struct notifier_block *,
> +						unsigned long, void *),
> +				const char *name)
> +{
> +	struct device *dev = mmc_dev(mmc);
> +	int ret;
> +
> +	nb->notifier_call = callback;
> +	ret = devm_regulator_register_notifier(regulator, nb);
> +	if (ret)
> +		dev_warn(dev, "Failed to register %s notifier: %pe\n", name,
> +			 ERR_PTR(ret));
> +}
> +
>  /**
>   * mmc_regulator_get_supply - try to get VMMC and VQMMC regulators for a
> host
>   * @mmc: the host to regulate
> @@ -293,6 +368,11 @@ int mmc_regulator_get_supply(struct mmc_host
> *mmc)
>  			mmc->ocr_avail = ret;
>  		else
>  			dev_warn(dev, "Failed getting OCR mask: %d\n", ret);
> +
> +		mmc_register_regulator_notifier(mmc, mmc->supply.vmmc,
> +						&mmc->supply.vmmc_nb,
> +
> 	mmc_vmmc_notifier_callback,
> +						"vmmc");
>  	}
> 
>  	if (IS_ERR(mmc->supply.vqmmc)) {
> @@ -301,12 +381,22 @@ int mmc_regulator_get_supply(struct mmc_host
> *mmc)
>  					     "vqmmc regulator not
> available\n");
> 
>  		dev_dbg(dev, "No vqmmc regulator found\n");
> +	} else {
> +		mmc_register_regulator_notifier(mmc, mmc->supply.vqmmc,
> +						&mmc->supply.vqmmc_nb,
> +
> 	mmc_vqmmc_notifier_callback,
> +						"vqmmc");
>  	}
> 
>  	if (IS_ERR(mmc->supply.vqmmc2)) {
>  		if (PTR_ERR(mmc->supply.vqmmc2) == -EPROBE_DEFER)
>  			return -EPROBE_DEFER;
>  		dev_dbg(dev, "No vqmmc2 regulator found\n");
> +	} else {
> +		mmc_register_regulator_notifier(mmc, mmc-
> >supply.vqmmc2,
> +						&mmc->supply.vqmmc2_nb,
> +
> 	mmc_vqmmc2_notifier_callback,
> +						"vqmmc2");
>  	}
> 
>  	return 0;
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h index
> 68f09a955a90..7da053095c63 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -342,6 +342,10 @@ struct mmc_supply {
>  	struct regulator *vmmc;		/* Card power supply */
>  	struct regulator *vqmmc;	/* Optional Vccq supply */
>  	struct regulator *vqmmc2;	/* Optional supply for phy */
> +
> +	struct notifier_block vmmc_nb;		/* Notifier for vmmc */
> +	struct notifier_block vqmmc_nb;		/* Notifier for vqmmc
> */
> +	struct notifier_block vqmmc2_nb;	/* Notifier for vqmmc2 */
>  };
> 
>  struct mmc_ctx {
> --
> 2.39.5
> 


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v1 1/1] mmc: core: Handle undervoltage events and register regulator notifiers
  2025-02-13 10:14 ` Avri Altman
@ 2025-02-13 11:09   ` Oleksij Rempel
  0 siblings, 0 replies; 6+ messages in thread
From: Oleksij Rempel @ 2025-02-13 11:09 UTC (permalink / raw)
  To: Avri Altman
  Cc: Ulf Hansson, kernel@pengutronix.de, linux-kernel@vger.kernel.org,
	linux-mmc@vger.kernel.org, Greg Kroah-Hartman, Mark Brown,
	Rafael J. Wysocki

On Thu, Feb 13, 2025 at 10:14:00AM +0000, Avri Altman wrote:
> > Extend the MMC core to handle undervoltage events by implementing
> > infrastructure to notify the MMC bus about voltage drops.
> > 
> > Background & Decision at LPC24:
> > 
> > This solution was proposed and refined during LPC24 in the talk "Graceful
> > Under Pressure: Prioritizing Shutdown to Protect Your Data in Embedded
> > Systems" which aimed to address how Linux should handle power fluctuations
> > in embedded devices to prevent data corruption or storage damage.
> > 
> > At the time, multiple possible solutions were considered:
> > 
> > 1. Triggering a system-wide suspend or shutdown: when undervoltage is
> >    detected, with device-specific prioritization to ensure critical
> >    components shut down first.
> >    - This approach was disliked by Greg Kroah-Hartman, as it introduced
> >      complexity and was not suitable for all use cases.
> > 
> > 2. Notifying relevant devices through the regulator framework: to allow
> >    graceful per-device handling.
> >    - This approach was agreed upon as the most acceptable: by participants
> >      in the discussion, including Greg Kroah-Hartman, Mark Brown,
> >      and Rafael J. Wysocki.
> >    - This patch implements that decision by integrating undervoltage
> >      handling into the MMC subsystem.
> > 
> > This patch was tested on iMX8MP based system with SDHCI controller.
> Has it been considered, to rely on user-space and not the kernel to handle
> those extreme conditions?
> E.g. a pollable sysfs that would be monitored by select(), poll(), etc.
> As Android might use?

Yes, the advantage of solving this in the kernel is:  

- The regulator framework already provides all the necessary components,
  including event support.  
- The MMC framework already supports regulators, making the implementation
  straightforward.  
- This approach works even if userspace is not ready at boot time.  

On the other hand, you are right that a userspace implementation would offer
more flexibility. However, I believe this can be built as an extension of the
current implementation. The regulator framework supports Netlink notifications
to userspace, eliminating the need for sysfs polling. The MMC framework would
also require a sysfs interface to force a quick shutdown - if such an interface
does not already exist - and a filter for regulator notifications to ignore
undervoltage events when they are handled in userspace.  

Best Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v1 1/1] mmc: core: Handle undervoltage events and register regulator notifiers
  2025-02-13  8:57   ` Oleksij Rempel
@ 2025-02-13 11:13     ` Christian Loehle
  0 siblings, 0 replies; 6+ messages in thread
From: Christian Loehle @ 2025-02-13 11:13 UTC (permalink / raw)
  To: Oleksij Rempel
  Cc: Ulf Hansson, kernel, linux-kernel, linux-mmc, Greg Kroah-Hartman,
	Mark Brown, Rafael J. Wysocki

On 2/13/25 08:57, Oleksij Rempel wrote:
> On Wed, Feb 12, 2025 at 11:47:08PM +0000, Christian Loehle wrote:
>> On 2/12/25 13:24, Oleksij Rempel wrote:
>>> Extend the MMC core to handle undervoltage events by implementing
>>> infrastructure to notify the MMC bus about voltage drops.
>>>
>>> Background & Decision at LPC24:
>>>
>>> This solution was proposed and refined during LPC24 in the talk
>>> "Graceful Under Pressure: Prioritizing Shutdown to Protect Your Data in
>>> Embedded Systems" which aimed to address how Linux should handle power
>>> fluctuations in embedded devices to prevent data corruption or storage
>>> damage.
>>>
>>> At the time, multiple possible solutions were considered:
>>>
>>> 1. Triggering a system-wide suspend or shutdown: when undervoltage is
>>>    detected, with device-specific prioritization to ensure critical
>>>    components shut down first.
>>>    - This approach was disliked by Greg Kroah-Hartman, as it introduced
>>>      complexity and was not suitable for all use cases.
>>>
>>> 2. Notifying relevant devices through the regulator framework: to allow
>>>    graceful per-device handling.
>>>    - This approach was agreed upon as the most acceptable: by participants
>>>      in the discussion, including Greg Kroah-Hartman, Mark Brown,
>>>      and Rafael J. Wysocki.
>>>    - This patch implements that decision by integrating undervoltage
>>>      handling into the MMC subsystem.
>>>
>>> This patch was tested on iMX8MP based system with SDHCI controller.
>>
>> Any details here? How long does it take from undervoltage to
>> poweroff notification.
> 
> On this system, with current implementation, it takes 4.5 millisecond
> from voltage drop detection to mmc_poweroff_notify.
> 
>> Roughly how long of a heads up would that yield in realistic
>> undervoltage scenarios?
> 
> It depends on the board implementation and attached power supply.
> In my case, the testing system provides about 100ms capacity on board.
> The power supply provides additional 1-2 seconds.
> 
> If the power is cut between power supply and board, we will have max
> 100ms.

Thanks, that's not too bad then.

> 
>>> +static int _mmc_handle_undervoltage(struct mmc_host *host)
>>> +{
>>> +	return mmc_shutdown(host);
>>> +}
>>> +
>>
>> The poweroff notification part I understand, because it polls for busy
>> (i.e. hopefully until the card thinks it's done committing to flash).
>> Poweroff isn't always available though, the other paths of
>> _mmc_suspend() are:
>>
>> 	else if (mmc_can_sleep(host->card))
>> 		err = mmc_sleep(host);
>> 	else if (!mmc_host_is_spi(host))
>> 		err = mmc_deselect_cards(host);
>>
>> 	if (!err) {
>> 		mmc_power_off(host);
>>
>> So we may also just deselect, which AFAIR succeeds as a FSM (i.e.
>> doesn't mean anything was committed to flash) and then we just
>> poweroff.
>> Is that what we want in an undervoltage scenario?
> 
> Yes. In an undervoltage scenario, our primary priority is to protect the
> hardware from damage. Data integrity is secondary in this case. The most
> critical action is to immediately stop writing to the card.  

Protect hardware from damage by not having the powerfail during a
host-write? An active host-write command doesn't sound like the
actual cause, any writing metadata to flash more likely, which in the
deselect->poweroff case isn't ensured at all to not be the case.

While I still think this should be handled at procurement instead of the
kernel (especially if you don't even care about data integrity?), I'd
still be interested how we would ensure we aren't doing more harm than
good here.
Any info from some vendors how they implement any of these? IME only
poweroff notify would do anything at all to help and if that isn't
available we should leave the voltage up for the card and idle.

FWIW poweroff notify should probably be
EXT_CSD_POWER_OFF_SHORT
instead of the current EXT_CSD_POWER_OFF_LONG for your intended use case.


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2025-02-13 11:13 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-02-12 13:24 [PATCH v1 1/1] mmc: core: Handle undervoltage events and register regulator notifiers Oleksij Rempel
2025-02-12 23:47 ` Christian Loehle
2025-02-13  8:57   ` Oleksij Rempel
2025-02-13 11:13     ` Christian Loehle
2025-02-13 10:14 ` Avri Altman
2025-02-13 11:09   ` Oleksij Rempel

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox