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

[Shengjiu Wang]

On Wed, Oct 14, 2026 at 1:47 AM Philipp Zabel <p.zabel@xxxxxxxxxxxxxx> wrote:
>
> On Di, 2026-10-13 at 18:19 +0800, shengjiu.wang@xxxxxxxxxxx wrote:
> > From: Shengjiu Wang <shengjiu.wang@xxxxxxx>
> >
> > reset_control_deassert() is documented to guarantee that the reset is
> > deasserted after the call returns. For a shared reset control only the
> > first caller runs the hardware .deassert(); all later callers just
> > increment deassert_count and return immediately:
> >
> >       if (atomic_inc_return(&rstc->deassert_count) != 1)
> >               return 0;
> >
> > When two consumers of the same shared reset (for example two codecs that
> > share one reset GPIO) call reset_control_deassert() concurrently, the
> > second caller can return before the first caller's .deassert() - which
> > may sleep, e.g. for a GPIO-expander based controller - has actually
> > released the line. The second consumer then proceeds while the device is
> > still in reset, contradicting the documented guarantee. The assert and
> > reset paths have the same problem: only the first shared caller performs
> > the hardware operation while concurrent callers return early.
> >
> > Serialize the shared refcount update and the hardware .assert()/
> > .deassert()/.reset() under the existing per-control rstc->lock so that a
> > second concurrent caller waits for the first caller's hardware operation
> > to complete. The lock is taken before the SRCU read section, matching the
> > ordering already used by reset_control_acquire(). These paths do not take
> > rcdev->lock, so no new lock-ordering inversion is introduced.
> >
> > Fixes: 0b52297f2288 ("reset: Add support for shared reset controls")
> > Link: https://lore.kernel.org/all/aszDZlJ0KahNwgZO@xxxxxxxxxx/
> > Signed-off-by: Shengjiu Wang <shengjiu.wang@xxxxxxx>
> > ---
> >  drivers/reset/core.c | 21 ++++++++++++++++++++-
> >  1 file changed, 20 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/reset/core.c b/drivers/reset/core.c
> > index 38e189d04d09..7536b61cc5df 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
> > + *        assert/deassert/reset of shared reset controls
>
> The lock is also taken for exclusive reset controls.

Right - in v1 the guard(mutex) is taken before the rstc->shared check,
so it is taken for exclusive controls too, which both contradicts the
kerneldoc wording and is the root of the atomic-context concern below.
In v2 I will take it only inside the shared branch.

>
> >   */
> >  struct reset_control {
> >       struct reset_controller_dev __rcu *rcdev;
> > @@ -382,6 +383,12 @@ int reset_control_reset(struct reset_control *rstc)
> >       if (reset_control_is_array(rstc))
> >               return reset_control_array_reset(rstc_to_array(rstc));
> >
> > +     /*
> > +      * Hold the lock across the shared refcount update and the hardware
> > +      * reset so that a concurrent caller does not continue before this
> > +      * caller's not-yet-finished reset pulse has been issued.
> > +      */
> > +     guard(mutex)(&rstc->lock);
>
> The current wording of the API documentation does not require this, but
> it clearly makes no sense that the second of two racing
> reset_control_reset() calls on a shared reset control can return before
> the reset pulse has finished.
>
> Could you update the kerneldoc comment, for example add "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." to
> the shared reset paragraph?

Will do, using your suggested wording.

>
> That being said, this is not required for exclusive resets, and I think
> there may be users handling reset controls in atomic context.
> Unfortunately the API never explicitly declared that
> reset_control_assert/deassert_reset() calls may sleep, and this just
> happens to work if the specific reset controller implementation doesn't
> sleep.
>
> I'm not sure what to do about this. We could split the shared and
> exclusive paths and only lock in the shared path, or fix the atomic
> users, possibly by introducing
> reset_control_assert/deassert/reset_atomic() first.

Agreed, the mutex must not be added to the exclusive path. For v2 I will
only take rstc->lock inside the rstc->shared branch in
reset_control_deassert() and reset_control_reset(), leaving the exclusive
path (and any exclusive atomic callers) completely unchanged.

>
> >       guard(srcu)(&rstc->srcu);
> >
> >       rcdev = srcu_dereference(rstc->rcdev, &rstc->srcu);
> > @@ -503,6 +510,12 @@ int reset_control_assert(struct reset_control *rstc)
> >       if (reset_control_is_array(rstc))
> >               return reset_control_array_assert(rstc_to_array(rstc));
> >
> > +     /*
> > +      * Hold the lock across the shared refcount update and the hardware
> > +      * assert so that a concurrent caller does not observe the line as
> > +      * released while this caller is still driving it.
> > +      */
> > +     guard(mutex)(&rstc->lock);
>
> Nothing in the API requires shared reset_control_reset() to be
> serialized, why is this necessary? The comment doesn't make this clear
> to me.

Since the API explicitly does *not* guarantee that the line is asserted
after a shared reset_control_assert() call, there is no documented
guarantee to enforce, and no need to serialize that path. I will remove
the lock from reset_control_assert() in v2.

best regards
shengjiu Wang
>
> >       guard(srcu)(&rstc->srcu);
> >
> >       rcdev = srcu_dereference(rstc->rcdev, &rstc->srcu);
> > @@ -599,6 +612,12 @@ int reset_control_deassert(struct reset_control *rstc)
> >       if (reset_control_is_array(rstc))
> >               return reset_control_array_deassert(rstc_to_array(rstc));
> >
> > +     /*
> > +      * Hold the lock across the shared refcount update and the hardware
> > +      * deassert so that a concurrent caller does not continue while the
> > +      * line is still asserted by this caller's not-yet-finished deassert.
> > +      */
> > +     guard(mutex)(&rstc->lock);
>
> Yes, this aligns the function with the documented intention for shared
> resets. The above points about exclusive resets also apply.
>
> regards
> Philipp
>