[PATCH v2 0/2] PM: clk: Release clocks across runtime suspend

[PATCH v2 0/2] PM: clk: Release clocks across runtime suspend

[Loic Poulain]

pm_clk prepares each clock when it is acquired and keeps that reference
until the entry is removed, runtime suspend only dropping the enable
reference. A prepare reference propagates to the parents though, so a
leaf clock can pin an ancestor that only implements prepare/unprepare
for as long as the device exists. On Qualcomm platforms this is how the
RPM XO clock stays active across runtime and system suspend even though
all of its consumers are suspended.

Drivers that manage their own clock usually unprepare in runtime suspend
245 clk_disable_unprepare() call sites under drivers/ against 27 that
only disable. pm_clk is the outlier, and only because it cannot
sleep for a device that suspends with interrupts disabled. So rather
than asking each clock whether it must sleep, ask the device whether it
may.

Patch 1 derives the locking from pm_runtime_is_irq_safe() and drops
the bookkeeping, patch 2 then unprepares on suspend. IRQ-safe devices
keep the current behaviour.

A non-IRQ-safe device now prepares and unprepares on every runtime PM
cycle rather than once per lifetime.

Signed-off-by: Loic Poulain <loic.poulain@oss.qualcomm.com>
---
Changes in v2:
- Split into two patches: the locking rework, then the unprepare on
  suspend.
- Keep clk_is_enabled_when_prepared() in pm_clk_acquire(). v1 used
  dev->power.irq_safe there, which runs before probe() can call
  pm_runtime_irq_safe(), so the flag always read false.
- Drop the clock_op_might_sleep count, the pm_clk_op_lock() retry loop
  and the caller name argument.
- Reject an IRQ-safe device holding a clock that is enabled when
  prepared in __pm_clk_enable() rather than from an in_atomic() test in
  the locking helper.
- Link to v1: https://lore.kernel.org/r/20261030-clk-leak-v1-1-db8cb68a5a8d@oss.qualcomm.com

---
Loic Poulain (2):
      PM: clk: Derive PM clock locking from the device's IRQ safety
      PM: clk: Release the clock prepare reference across runtime suspend

 drivers/base/power/clock_ops.c | 145 +++++++++++++++++++----------------------
 include/linux/pm.h             |   1 -
 2 files changed, 68 insertions(+), 78 deletions(-)
---
base-commit: e726cf08ee6ba7e8b852c77760f12c59f90f1338
change-id: 20261030-clk-leak-3ea6f7c7da25

Best regards,
-- 
Loic Poulain <loic.poulain@oss.qualcomm.com>

[PATCH v2 1/2] PM: clk: Derive PM clock locking from the device's IRQ safety

[Loic Poulain]

pm_clk_suspend() and pm_clk_resume() take the pm_subsys_data spinlock
when every clock in the list can be gated atomically and the mutex
otherwise. That is tracked by a clock_op_might_sleep count.

Whether the operations may sleep is a property of the device rather
than of the clocks though, runtime PM only invokes the callbacks with
interrupts disabled for an IRQ-safe device.

Use pm_runtime_is_irq_safe() and drop the count, the retry loop and
the caller name argument. Devices that are not IRQ-safe now always
take the mutex, which is legal as they only suspend in process context.

The count also rejected an IRQ-safe device holding a clock that is
enabled when prepared, with -EPERM. __pm_clk_enable() now does that
instead.

Signed-off-by: Loic Poulain <loic.poulain@oss.qualcomm.com>
---
 drivers/base/power/clock_ops.c | 135 +++++++++++++++++++----------------------
 include/linux/pm.h             |   1 -
 2 files changed, 62 insertions(+), 74 deletions(-)

diff --git a/drivers/base/power/clock_ops.c b/drivers/base/power/clock_ops.c
index 3b8ba27802a800bac29abd8e21756ccdb0f2a716..6f66e11a12428fb718abca197f5519b1ee21ff90 100644
--- a/drivers/base/power/clock_ops.c
+++ b/drivers/base/power/clock_ops.c
@@ -40,14 +40,13 @@ struct pm_clock_entry {
  * pm_clk_list_lock - ensure exclusive access for modifying the PM clock
  *		      entry list.
  * @psd: pm_subsys_data instance corresponding to the PM clock entry list
- *	 and clk_op_might_sleep count to be modified.
+ *	 to be modified.
  *
- * Get exclusive access before modifying the PM clock entry list and the
- * clock_op_might_sleep count to guard against concurrent modifications.
- * This also protects against a concurrent clock_op_might_sleep and PM clock
- * entry list usage in pm_clk_suspend()/pm_clk_resume() that may or may not
- * happen in atomic context, hence both the mutex and the spinlock must be
- * taken here.
+ * Get exclusive access before modifying the PM clock entry list to guard
+ * against concurrent modifications. This also protects against a concurrent
+ * PM clock entry list usage in pm_clk_suspend()/pm_clk_resume() that may or
+ * may not happen in atomic context, hence both the mutex and the spinlock
+ * must be taken here.
  */
 static void pm_clk_list_lock(struct pm_subsys_data *psd)
 	__acquires(&psd->lock)
@@ -70,72 +69,51 @@ static void pm_clk_list_unlock(struct pm_subsys_data *psd)
 
 /**
  * pm_clk_op_lock - ensure exclusive access for performing clock operations.
+ * @dev: device the clock operations are performed for.
  * @psd: pm_subsys_data instance corresponding to the PM clock entry list
- *	 and clk_op_might_sleep count being used.
+ *	 being used.
  * @flags: stored irq flags.
- * @fn: string for the caller function's name.
  *
- * This is used by pm_clk_suspend() and pm_clk_resume() to guard
- * against concurrent modifications to the clock entry list and the
- * clock_op_might_sleep count. If clock_op_might_sleep is != 0 then
- * only the mutex can be locked and those functions can only be used in
- * non atomic context. If clock_op_might_sleep == 0 then these functions
- * may be used in any context and only the spinlock can be locked.
- * Returns -EINVAL if called in atomic context when clock ops might sleep.
+ * This is used by pm_clk_suspend() and pm_clk_resume() to guard against
+ * concurrent modifications to the clock entry list.
+ *
+ * An IRQ-safe device may runtime suspend and resume with interrupts
+ * disabled, so for such a device only the spinlock can be taken and the
+ * clock operations are restricted to the atomic clk_enable()/clk_disable().
+ * Any other device is suspended and resumed in process context only, so
+ * the mutex is taken instead and the clock operations are allowed to
+ * sleep.
  */
-static int pm_clk_op_lock(struct pm_subsys_data *psd, unsigned long *flags,
-			  const char *fn)
+static void pm_clk_op_lock(struct device *dev, struct pm_subsys_data *psd,
+			   unsigned long *flags)
 	/* sparse annotations don't work here as exit state isn't static */
 {
-	bool atomic_context = in_atomic() || irqs_disabled();
-
-try_again:
-	spin_lock_irqsave(&psd->lock, *flags);
-	if (!psd->clock_op_might_sleep) {
+	if (pm_runtime_is_irq_safe(dev)) {
+		spin_lock_irqsave(&psd->lock, *flags);
 		/* the __release is there to work around sparse limitations */
 		__release(&psd->lock);
-		return 0;
-	}
-
-	/* bail out if in atomic context */
-	if (atomic_context) {
-		pr_err("%s: atomic context with clock_ops_might_sleep = %d",
-		       fn, psd->clock_op_might_sleep);
-		spin_unlock_irqrestore(&psd->lock, *flags);
-		might_sleep();
-		return -EPERM;
+	} else {
+		mutex_lock(&psd->clock_mutex);
 	}
-
-	/* we must switch to the mutex */
-	spin_unlock_irqrestore(&psd->lock, *flags);
-	mutex_lock(&psd->clock_mutex);
-
-	/*
-	 * There was a possibility for psd->clock_op_might_sleep
-	 * to become 0 above. Keep the mutex only if not the case.
-	 */
-	if (likely(psd->clock_op_might_sleep))
-		return 0;
-
-	mutex_unlock(&psd->clock_mutex);
-	goto try_again;
 }
 
 /**
  * pm_clk_op_unlock - counterpart to pm_clk_op_lock().
+ * @dev: the same device previously passed to pm_clk_op_lock().
  * @psd: the same pm_subsys_data instance previously passed to
  *	 pm_clk_op_lock().
  * @flags: irq flags provided by pm_clk_op_lock().
  */
-static void pm_clk_op_unlock(struct pm_subsys_data *psd, unsigned long *flags)
+static void pm_clk_op_unlock(struct device *dev, struct pm_subsys_data *psd,
+			     unsigned long *flags)
 	/* sparse annotations don't work here as entry state isn't static */
 {
-	if (psd->clock_op_might_sleep) {
-		mutex_unlock(&psd->clock_mutex);
-	} else {
+	if (pm_runtime_is_irq_safe(dev)) {
 		/* the __acquire is there to work around sparse limitations */
 		__acquire(&psd->lock);
 		spin_unlock_irqrestore(&psd->lock, *flags);
+	} else {
+		mutex_unlock(&psd->clock_mutex);
 	}
 }
 
@@ -143,26 +121,44 @@ static void pm_clk_op_unlock(struct pm_subsys_data *psd, unsigned long *flags)
  * __pm_clk_enable - Enable a clock, reporting any errors
  * @dev: The device for the given clock
  * @ce: PM clock entry corresponding to the clock.
+ * @sleepable: Whether the clock operations are allowed to sleep.
+ *
+ * Returns -EPERM if the clock can only be started by preparing it while that
+ * is not allowed here.
  */
-static inline void __pm_clk_enable(struct device *dev, struct pm_clock_entry *ce)
+static inline int __pm_clk_enable(struct device *dev, struct pm_clock_entry *ce,
+				  bool sleepable)
 {
 	int ret;
 
 	switch (ce->status) {
 	case PCE_STATUS_ACQUIRED:
+		if (!sleepable) {
+			/*
+			 * An entry left unprepared is an entry whose clock is
+			 * enabled when prepared, so it cannot be started
+			 * without sleeping and the device must not be
+			 * IRQ-safe.
+			 */
+			dev_err(dev, "clk %pC cannot be enabled atomically\n",
+				ce->clk);
+			return -EPERM;
+		}
 		ret = clk_prepare_enable(ce->clk);
 		break;
 	case PCE_STATUS_PREPARED:
 		ret = clk_enable(ce->clk);
 		break;
 	default:
-		return;
+		return 0;
 	}
 	if (!ret)
 		ce->status = PCE_STATUS_ENABLED;
 	else
 		dev_err(dev, "%s: failed to enable clk %p, error %d\n",
 			__func__, ce->clk, ret);
+
+	return 0;
 }
 
 /**
@@ -223,8 +219,6 @@ static int __pm_clk_add(struct device *dev, const char *con_id,
 
 	pm_clk_list_lock(psd);
 	list_add_tail(&ce->node, &psd->clock_list);
-	if (ce->enabled_when_prepared)
-		psd->clock_op_might_sleep++;
 	pm_clk_list_unlock(psd);
 	return 0;
 }
@@ -371,8 +365,6 @@ void pm_clk_remove_clk(struct device *dev, struct clk *clk)
 
  remove:
 	list_del(&ce->node);
-	if (ce->enabled_when_prepared)
-		psd->clock_op_might_sleep--;
 	pm_clk_list_unlock(psd);
 
 	__pm_clk_remove(ce);
@@ -384,7 +376,7 @@ EXPORT_SYMBOL_GPL(pm_clk_remove_clk);
  * @dev: Device to initialize the list of PM clocks for.
  *
  * Initialize the lock and clock_list members of the device's pm_subsys_data
- * object, set the count of clocks that might sleep to 0.
+ * object.
  */
 void pm_clk_init(struct device *dev)
 {
@@ -392,7 +384,6 @@ void pm_clk_init(struct device *dev)
 	if (psd) {
 		INIT_LIST_HEAD(&psd->clock_list);
 		mutex_init(&psd->clock_mutex);
-		psd->clock_op_might_sleep = 0;
 	}
 }
 EXPORT_SYMBOL_GPL(pm_clk_init);
@@ -433,7 +424,6 @@ void pm_clk_destroy(struct device *dev)
 
 	list_for_each_entry_safe_reverse(ce, c, &psd->clock_list, node)
 		list_move(&ce->node, &list);
-	psd->clock_op_might_sleep = 0;
 
 	pm_clk_list_unlock(psd);
 
@@ -472,16 +462,13 @@ int pm_clk_suspend(struct device *dev)
 	struct pm_subsys_data *psd = dev_to_psd(dev);
 	struct pm_clock_entry *ce;
 	unsigned long flags;
-	int ret;
 
 	dev_dbg(dev, "%s()\n", __func__);
 
 	if (!psd)
 		return 0;
 
-	ret = pm_clk_op_lock(psd, &flags, __func__);
-	if (ret)
-		return ret;
+	pm_clk_op_lock(dev, psd, &flags);
 
 	list_for_each_entry_reverse(ce, &psd->clock_list, node) {
 		if (ce->status == PCE_STATUS_ENABLED) {
@@ -495,7 +482,7 @@ int pm_clk_suspend(struct device *dev)
 		}
 	}
 
-	pm_clk_op_unlock(psd, &flags);
+	pm_clk_op_unlock(dev, psd, &flags);
 
 	return 0;
 }
@@ -508,25 +495,27 @@ EXPORT_SYMBOL_GPL(pm_clk_suspend);
 int pm_clk_resume(struct device *dev)
 {
 	struct pm_subsys_data *psd = dev_to_psd(dev);
+	bool sleepable = !pm_runtime_is_irq_safe(dev);
 	struct pm_clock_entry *ce;
 	unsigned long flags;
-	int ret;
+	int ret = 0;
 
 	dev_dbg(dev, "%s()\n", __func__);
 
 	if (!psd)
 		return 0;
 
-	ret = pm_clk_op_lock(psd, &flags, __func__);
-	if (ret)
-		return ret;
+	pm_clk_op_lock(dev, psd, &flags);
 
-	list_for_each_entry(ce, &psd->clock_list, node)
-		__pm_clk_enable(dev, ce);
+	list_for_each_entry(ce, &psd->clock_list, node) {
+		ret = __pm_clk_enable(dev, ce, sleepable);
+		if (ret)
+			break;
+	}
 
-	pm_clk_op_unlock(psd, &flags);
+	pm_clk_op_unlock(dev, psd, &flags);
 
-	return 0;
+	return ret;
 }
 EXPORT_SYMBOL_GPL(pm_clk_resume);
 
diff --git a/include/linux/pm.h b/include/linux/pm.h
index ef3f1310e749c34d4aff75a2fd7e95b9f2ef3a10..c38c59b8a52831ea44ba3ce89217c99a616adda9 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -636,7 +636,6 @@ struct pm_subsys_data {
 	spinlock_t lock;
 	unsigned int refcount;
 #ifdef CONFIG_PM_CLK
-	unsigned int clock_op_might_sleep;
 	struct mutex clock_mutex;
 	struct list_head clock_list;
 #endif

-- 
2.34.1

[PATCH v2 2/2] PM: clk: Release the clock prepare reference across runtime suspend

[Loic Poulain]

The PM clock layer prepares a clock at acquire time and keeps that
reference until the entry is removed, pm_clk_suspend() only dropping
the enable reference. A prepare reference propagates to the parents
though, so a clock with enable/disable ops can keep an ancestor that
only implements prepare/unprepare running for as long as the device
exists. On Qualcomm platforms this keeps the RPM XO clock active
across runtime and system suspend even though all of its consumers
are suspended.

Unprepare on suspend for devices that are not IRQ-safe, as those
suspend in process context, and let the next pm_clk_resume() prepare
the clock again. This generalises the clk_is_enabled_when_prepared()
check in pm_clk_acquire(), which only considers the clock itself and
not its parents. Most drivers managing their own clocks already
unprepare in runtime suspend: 245 call sites under drivers/ against
27 that only disable.

Reported-by: Nihal Kumar Gupta <nihal.gupta@oss.qualcomm.com>
Signed-off-by: Loic Poulain <loic.poulain@oss.qualcomm.com>
---
 drivers/base/power/clock_ops.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/base/power/clock_ops.c b/drivers/base/power/clock_ops.c
index 6f66e11a12428fb718abca197f5519b1ee21ff90..43150ecb58add7783d8602d525219c676fd808fe 100644
--- a/drivers/base/power/clock_ops.c
+++ b/drivers/base/power/clock_ops.c
@@ -33,7 +33,6 @@ struct pm_clock_entry {
 	char *con_id;
 	struct clk *clk;
 	enum pce_status status;
-	bool enabled_when_prepared;
 };
 
 /**
@@ -174,9 +173,11 @@ static void pm_clk_acquire(struct device *dev, struct pm_clock_entry *ce)
 		ce->status = PCE_STATUS_ERROR;
 		return;
 	} else if (clk_is_enabled_when_prepared(ce->clk)) {
-		/* we defer preparing the clock in that case */
+		/*
+		 * Preparing the clock would also start it, so defer that to
+		 * the first pm_clk_resume().
+		 */
 		ce->status = PCE_STATUS_ACQUIRED;
-		ce->enabled_when_prepared = true;
 	} else if (clk_prepare(ce->clk)) {
 		ce->status = PCE_STATUS_ERROR;
 		dev_err(dev, "clk_prepare() failed\n");
@@ -460,6 +461,7 @@ EXPORT_SYMBOL_GPL(devm_pm_clk_create);
 int pm_clk_suspend(struct device *dev)
 {
 	struct pm_subsys_data *psd = dev_to_psd(dev);
+	bool sleepable = !pm_runtime_is_irq_safe(dev);
 	struct pm_clock_entry *ce;
 	unsigned long flags;
 
@@ -472,7 +474,7 @@ int pm_clk_suspend(struct device *dev)
 
 	list_for_each_entry_reverse(ce, &psd->clock_list, node) {
 		if (ce->status == PCE_STATUS_ENABLED) {
-			if (ce->enabled_when_prepared) {
+			if (sleepable) {
 				clk_disable_unprepare(ce->clk);
 				ce->status = PCE_STATUS_ACQUIRED;
 			} else {

-- 
2.34.1