Re: [PATCH V2] reset: core: Serialize concurrent deassert/reset of shared resets

[Shengjiu Wang]

Hi Philipp

On Mon, Oct 19, 2026 at 1:52 PM <shengjiu.wang@xxxxxxxxxxx> wrote:
>
> From: Shengjiu Wang <shengjiu.wang@xxxxxxx>
>
> reset_control_deassert() and reset_control_reset() guarantee that the
> reset line is deasserted / a reset pulse has been issued once they return.
> For a shared reset only the first caller runs the hardware op; others just
> bump the refcount and return early, so a concurrent caller can return
> before the first caller's (possibly sleeping) op has finished.
>
> Serialize the shared refcount update and the hardware op under the
> existing per-control rstc->lock, taken before the SRCU section to match
> reset_control_acquire()'s mutex->srcu order. The lock is taken only on the
> shared path and only for may-sleep callers (reset_control_reset() and
> reset_control_deassert()); the exclusive path stays lockless. Callers in
> atomic context must use the new lockless reset_control_reset_atomic() and
> reset_control_deassert_atomic() variants, which skip the mutex and so
> carry no completion guarantee.
>
> The op is now called with rstc->lock held on the shared path, so it must
> not re-enter reset_control_reset()/deassert()/acquire() on the same
> control; a lockdep_assert_not_held() at each lock site catches that.
>
> reset_control_assert() is unchanged: a shared assert makes no guarantee
> about the line state, so there is nothing to serialize.
>
> Fixes: 0b52297f2288 ("reset: Add support for shared reset controls")
> Link: https://lore.kernel.org/all/aszDZlJ0KahNwgZO@xxxxxxxxxx/
> Assisted-by: Anthropic:claude-opus-4-8
> Signed-off-by: Shengjiu Wang <shengjiu.wang@xxxxxxx>
> ---
> Changes in v2:
> - Serialize only the shared may-sleep path under rstc->lock (taken before
>   SRCU to match reset_control_acquire()'s order); exclusive path stays
>   lockless, and reset_control_assert() is left unchanged.
> - Add lockless reset_control_reset_atomic()/reset_control_deassert_atomic()
>   for atomic callers; gate the mutex on can_sleep so no shared consumer is
>   regressed and so it works on all configs.
> - Factor the body into __reset_control_*()/__reset_control_*_locked()
>   helpers, and lockdep_assert_not_held() at each lock site to catch a
>   controller op re-entering the API on the same control.
> - Declare the new variants and stubs in reset.h, and document the sleeping
>   vs atomic variants in the kerneldoc and reset.rst.
>
>  Documentation/driver-api/reset.rst |  10 ++
>  drivers/reset/core.c               | 204 ++++++++++++++++++++++-------
>  include/linux/reset.h              |  12 ++
>  3 files changed, 177 insertions(+), 49 deletions(-)
>
> diff --git a/Documentation/driver-api/reset.rst b/Documentation/driver-api/reset.rst
> index 932699c57d1e..0fbc1ba92dd8 100644
> --- a/Documentation/driver-api/reset.rst
> +++ b/Documentation/driver-api/reset.rst
> @@ -185,13 +185,23 @@ Given the reset control, consumers can call reset_control_assert() and
>  reset_control_deassert(), trigger a reset pulse using reset_control_reset(), or
>  query the reset line status using reset_control_status().
>
> +For shared reset controls, reset_control_reset() and reset_control_deassert()
> +may sleep and must be called from process context. Consumers that need to
> +operate a reset line from atomic context can use reset_control_reset_atomic()
> +or reset_control_deassert_atomic(); these never sleep, but do not serialize
> +concurrent shared callers and so do not provide the completion guarantee of
> +their sleeping counterparts. The underlying reset controller's ops must not
> +sleep when the atomic variants are used.

On second thought, this change lets reset_control_deassert() sleep, so
any driver
that currently calls it from atomic context would now block. The new
_atomic() variants
let those callers opt out, but I can't tell how many drivers would
need updating, and any
one we miss is a silent risk.

Do you have another idea to fix the concurrent deassert() problem on
shared resets
without this risk?  Or should we go back to the original proposal?
https://lore.kernel.org/all/aszDZlJ0KahNwgZO@xxxxxxxxxx/T/#m66615477929b38f788d6d22a2c0bb9ca62173e89

Best regards
Shengjiu Wang

> +
>  .. kernel-doc:: include/linux/reset.h
>     :internal:
>
>  .. kernel-doc:: drivers/reset/core.c
>     :functions: reset_control_reset
> +               reset_control_reset_atomic
>                 reset_control_assert
>                 reset_control_deassert
> +               reset_control_deassert_atomic
>                 reset_control_status
>                 reset_control_acquire
>                 reset_control_release
> diff --git a/drivers/reset/core.c b/drivers/reset/core.c
> index 38e189d04d09..d477935e37ea 100644
> --- a/drivers/reset/core.c
> +++ b/drivers/reset/core.c
> @@ -51,7 +51,8 @@ static DEFINE_IDA(reset_gpio_ida);
>   * @triggered_count: Number of times this reset line has been reset. Currently
>   *                   only used for shared resets, which means that the value
>   *                   will be either 0 or 1.
> - * @lock: serializes the internals of reset_control_acquire()
> + * @lock: serializes the internals of reset_control_acquire() and the
> + *        shared deassert/reset refcount update and hardware op
>   */
>  struct reset_control {
>         struct reset_controller_dev __rcu *rcdev;
> @@ -240,12 +241,16 @@ rstc_to_array(struct reset_control *rstc) {
>         return container_of(rstc, struct reset_control_array, base);
>  }
>
> -static int reset_control_array_reset(struct reset_control_array *resets)
> +static int __reset_control_reset(struct reset_control *rstc, bool can_sleep);
> +static int __reset_control_deassert(struct reset_control *rstc, bool can_sleep);
> +
> +static int reset_control_array_reset(struct reset_control_array *resets,
> +                                    bool can_sleep)
>  {
>         int ret, i;
>
>         for (i = 0; i < resets->num_rstcs; i++) {
> -               ret = reset_control_reset(resets->rstc[i]);
> +               ret = __reset_control_reset(resets->rstc[i], can_sleep);
>                 if (ret)
>                         return ret;
>         }
> @@ -304,12 +309,13 @@ static int reset_control_array_assert(struct reset_control_array *resets)
>         return ret;
>  }
>
> -static int reset_control_array_deassert(struct reset_control_array *resets)
> +static int reset_control_array_deassert(struct reset_control_array *resets,
> +                                       bool can_sleep)
>  {
>         int ret, i;
>
>         for (i = 0; i < resets->num_rstcs; i++) {
> -               ret = reset_control_deassert(resets->rstc[i]);
> +               ret = __reset_control_deassert(resets->rstc[i], can_sleep);
>                 if (ret)
>                         goto err;
>         }
> @@ -355,33 +361,11 @@ static inline bool reset_control_is_array(struct reset_control *rstc)
>         return rstc->array;
>  }
>
> -/**
> - * reset_control_reset - reset the controlled device
> - * @rstc: reset controller
> - *
> - * On a shared reset line the actual reset pulse is only triggered once for the
> - * lifetime of the reset_control instance: for all but the first caller this is
> - * a no-op.
> - * Consumers must not use reset_control_(de)assert on shared reset lines when
> - * reset_control_reset has been used.
> - *
> - * If rstc is NULL it is an optional reset and the function will just
> - * return 0.
> - */
> -int reset_control_reset(struct reset_control *rstc)
> +static int __reset_control_reset_locked(struct reset_control *rstc)
>  {
>         struct reset_controller_dev *rcdev;
>         int ret;
>
> -       if (!rstc)
> -               return 0;
> -
> -       if (WARN_ON(IS_ERR(rstc)))
> -               return -EINVAL;
> -
> -       if (reset_control_is_array(rstc))
> -               return reset_control_array_reset(rstc_to_array(rstc));
> -
>         guard(srcu)(&rstc->srcu);
>
>         rcdev = srcu_dereference(rstc->rcdev, &rstc->srcu);
> @@ -408,8 +392,83 @@ int reset_control_reset(struct reset_control *rstc)
>
>         return ret;
>  }
> +
> +static int __reset_control_reset(struct reset_control *rstc, bool can_sleep)
> +{
> +       if (!rstc)
> +               return 0;
> +
> +       if (WARN_ON(IS_ERR(rstc)))
> +               return -EINVAL;
> +
> +       if (reset_control_is_array(rstc))
> +               return reset_control_array_reset(rstc_to_array(rstc), can_sleep);
> +
> +       /*
> +        * Serialize only the shared may-sleep path: holding rstc->lock across
> +        * the refcount update and the controller .reset() op stops a
> +        * concurrent caller from returning before this caller's not-yet-
> +        * finished reset pulse has been issued. can_sleep is false only for
> +        * reset_control_reset_atomic() callers, which cannot take the mutex
> +        * and so get no completion guarantee. The lock is taken before the
> +        * SRCU section (in __reset_control_reset_locked()) to match the
> +        * mutex->srcu order used by reset_control_acquire(). rstc->shared is
> +        * set at creation and is safe to read without the lock.
> +        */
> +       if (!(can_sleep && rstc->shared))
> +               return __reset_control_reset_locked(rstc);
> +
> +       might_sleep();
> +       lockdep_assert_not_held(&rstc->lock);
> +       guard(mutex)(&rstc->lock);
> +       return __reset_control_reset_locked(rstc);
> +}
> +
> +/**
> + * reset_control_reset - reset the controlled device
> + * @rstc: reset controller
> + *
> + * On a shared reset line the actual reset pulse is only triggered once for the
> + * lifetime of the reset_control instance: for all but the first caller this is
> + * a no-op.
> + * After calling this function, the reset is guaranteed to have seen a reset
> + * pulse at any point in the past, not necessarily during the function call.
> + * Consumers must not use reset_control_(de)assert on shared reset lines when
> + * reset_control_reset has been used.
> + *
> + * This function may sleep when called on a shared reset control; use
> + * reset_control_reset_atomic() if the caller cannot sleep.
> + *
> + * If rstc is NULL it is an optional reset and the function will just
> + * return 0.
> + */
> +int reset_control_reset(struct reset_control *rstc)
> +{
> +       return __reset_control_reset(rstc, true);
> +}
>  EXPORT_SYMBOL_GPL(reset_control_reset);
>
> +/**
> + * reset_control_reset_atomic - reset the controlled device, usable in atomic
> + *                              context
> + * @rstc: reset controller
> + *
> + * Same as reset_control_reset() but does not sleep and does not serialize
> + * concurrent callers on a shared reset line. Because no lock is taken, a
> + * concurrent reset_control_reset()/_atomic() may still be in progress when
> + * this returns; the completion-ordering guarantee of reset_control_reset()
> + * does not apply. The underlying reset controller's .reset() op must not
> + * sleep.
> + *
> + * If rstc is NULL it is an optional reset and the function will just
> + * return 0.
> + */
> +int reset_control_reset_atomic(struct reset_control *rstc)
> +{
> +       return __reset_control_reset(rstc, false);
> +}
> +EXPORT_SYMBOL_GPL(reset_control_reset_atomic);
> +
>  /**
>   * reset_control_bulk_reset - reset the controlled devices in order
>   * @num_rstcs: number of entries in rstcs array
> @@ -575,30 +634,10 @@ int reset_control_bulk_assert(int num_rstcs,
>  }
>  EXPORT_SYMBOL_GPL(reset_control_bulk_assert);
>
> -/**
> - * reset_control_deassert - deasserts the reset line
> - * @rstc: reset controller
> - *
> - * After calling this function, the reset is guaranteed to be deasserted.
> - * Consumers must not use reset_control_reset on shared reset lines when
> - * reset_control_(de)assert has been used.
> - *
> - * If rstc is NULL it is an optional reset and the function will just
> - * return 0.
> - */
> -int reset_control_deassert(struct reset_control *rstc)
> +static int __reset_control_deassert_locked(struct reset_control *rstc)
>  {
>         struct reset_controller_dev *rcdev;
>
> -       if (!rstc)
> -               return 0;
> -
> -       if (WARN_ON(IS_ERR(rstc)))
> -               return -EINVAL;
> -
> -       if (reset_control_is_array(rstc))
> -               return reset_control_array_deassert(rstc_to_array(rstc));
> -
>         guard(srcu)(&rstc->srcu);
>
>         rcdev = srcu_dereference(rstc->rcdev, &rstc->srcu);
> @@ -631,8 +670,74 @@ int reset_control_deassert(struct reset_control *rstc)
>
>         return rcdev->ops->deassert(rcdev, rstc->id);
>  }
> +
> +static int __reset_control_deassert(struct reset_control *rstc, bool can_sleep)
> +{
> +       if (!rstc)
> +               return 0;
> +
> +       if (WARN_ON(IS_ERR(rstc)))
> +               return -EINVAL;
> +
> +       if (reset_control_is_array(rstc))
> +               return reset_control_array_deassert(rstc_to_array(rstc), can_sleep);
> +
> +       /*
> +        * See __reset_control_reset(): serialize only the shared may-sleep
> +        * path. can_sleep is false only for reset_control_deassert_atomic()
> +        * callers, which cannot take the mutex and get no completion
> +        * guarantee. The lock is taken before the SRCU section (in
> +        * __reset_control_deassert_locked()) to match the mutex->srcu order
> +        * used by reset_control_acquire().
> +        */
> +       if (!(can_sleep && rstc->shared))
> +               return __reset_control_deassert_locked(rstc);
> +
> +       might_sleep();
> +       lockdep_assert_not_held(&rstc->lock);
> +       guard(mutex)(&rstc->lock);
> +       return __reset_control_deassert_locked(rstc);
> +}
> +
> +/**
> + * reset_control_deassert - deasserts the reset line
> + * @rstc: reset controller
> + *
> + * After calling this function, the reset is guaranteed to be deasserted.
> + * Consumers must not use reset_control_reset on shared reset lines when
> + * reset_control_(de)assert has been used.
> + *
> + * This function may sleep when called on a shared reset control; use
> + * reset_control_deassert_atomic() if the caller cannot sleep.
> + *
> + * If rstc is NULL it is an optional reset and the function will just
> + * return 0.
> + */
> +int reset_control_deassert(struct reset_control *rstc)
> +{
> +       return __reset_control_deassert(rstc, true);
> +}
>  EXPORT_SYMBOL_GPL(reset_control_deassert);
>
> +/**
> + * reset_control_deassert_atomic - deasserts the reset line, usable in atomic
> + *                                 context
> + * @rstc: reset controller
> + *
> + * Same as reset_control_deassert() but does not sleep and does not serialize
> + * concurrent callers on a shared reset line, so the deassert-completion
> + * guarantee of reset_control_deassert() does not apply. The underlying reset
> + * controller's .deassert() op must not sleep.
> + *
> + * If rstc is NULL it is an optional reset and the function will just
> + * return 0.
> + */
> +int reset_control_deassert_atomic(struct reset_control *rstc)
> +{
> +       return __reset_control_deassert(rstc, false);
> +}
> +EXPORT_SYMBOL_GPL(reset_control_deassert_atomic);
> +
>  /**
>   * reset_control_bulk_deassert - deasserts the reset lines in reverse order
>   * @num_rstcs: number of entries in rstcs array
> @@ -726,6 +831,7 @@ int reset_control_acquire(struct reset_control *rstc)
>         if (reset_control_is_array(rstc))
>                 return reset_control_array_acquire(rstc_to_array(rstc));
>
> +       lockdep_assert_not_held(&rstc->lock);
>         guard(mutex)(&rstc->lock);
>
>         if (rstc->acquired)
> diff --git a/include/linux/reset.h b/include/linux/reset.h
> index 9c391cf0c822..ab697193dbe9 100644
> --- a/include/linux/reset.h
> +++ b/include/linux/reset.h
> @@ -73,9 +73,11 @@ enum reset_control_flags {
>  #ifdef CONFIG_RESET_CONTROLLER
>
>  int reset_control_reset(struct reset_control *rstc);
> +int reset_control_reset_atomic(struct reset_control *rstc);
>  int reset_control_rearm(struct reset_control *rstc);
>  int reset_control_assert(struct reset_control *rstc);
>  int reset_control_deassert(struct reset_control *rstc);
> +int reset_control_deassert_atomic(struct reset_control *rstc);
>  int reset_control_status(struct reset_control *rstc);
>  int reset_control_acquire(struct reset_control *rstc);
>  void reset_control_release(struct reset_control *rstc);
> @@ -117,6 +119,11 @@ static inline int reset_control_reset(struct reset_control *rstc)
>         return 0;
>  }
>
> +static inline int reset_control_reset_atomic(struct reset_control *rstc)
> +{
> +       return 0;
> +}
> +
>  static inline int reset_control_rearm(struct reset_control *rstc)
>  {
>         return 0;
> @@ -132,6 +139,11 @@ static inline int reset_control_deassert(struct reset_control *rstc)
>         return 0;
>  }
>
> +static inline int reset_control_deassert_atomic(struct reset_control *rstc)
> +{
> +       return 0;
> +}
> +
>  static inline int reset_control_status(struct reset_control *rstc)
>  {
>         return 0;
> --
> 2.34.1
>
>