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

[Rafael J. Wysocki (Intel)]

So can people sending the assorted clock_ops.c fixes look at the
others' changes and provide feedback?

On Tue, Nov 10, 2026 at 1:01 PM Loic Poulain
<loic.poulain@xxxxxxxxxxxxxxxx> wrote:
>
> 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@xxxxxxxxxxxxxxxx>
> ---
>  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
>
>