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

[Loic Poulain <loic.poulain@oss.qualcomm.com>]

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