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

[Shengjiu Wang]

On Wed, Oct 21, 2026 at 9:31 PM Philipp Zabel <p.zabel@xxxxxxxxxxxxxx> wrote:
>
> On Mi, 2026-10-21 at 11:09 +0800, shengjiu.wang@xxxxxxxxxxx wrote:
> > From: Shengjiu Wang <shengjiu.wang@xxxxxxx>
> >
> > reset_control_deassert() and reset_control_reset() are documented to
> > guarantee that the reset line has been deasserted, respectively that a
> > reset pulse has been issued, by the time they return.
> >
> > For a shared reset only the first caller runs the hardware op; the other
> > callers just bump the refcount and return. When the op can sleep this
> > breaks the guarantee: a second caller can observe the bumped refcount and
> > return before the first caller's op has finished, and then touch hardware
> > that is still in reset.
> >
> > Add a reset_controller_dev::can_sleep flag that a provider sets when its
> > reset ops may sleep. When it is set, serialize the shared refcount update
> > and the hardware op in reset_control_reset() and reset_control_deassert()
> > under a new per-control mutex, taken inside the SRCU read-side critical
> > section. Providers that leave the flag clear keep the existing lockless
> > path, so callers in atomic context are unaffected.
>
> I think this paragraph should go first.

Ok, will move this to be first.
>
> > Set can_sleep in the reset-gpio driver, which drives the line with
> > gpiod_set_value_cansleep() and may therefore sleep. Other providers whose
> > reset ops may sleep still need to opt in.
>
> Please split this out into a separate patch.

Ok. will split them.

>
> > Fixes: 0b52297f2288 ("reset: Add support for shared reset controls")
>
> This is a partial fix for an issue introduced by 0b52297f2288, but only
> for reset-gpio, which didn't exist back then.
>
> On the other hand, the patch doesn't apply before commit 8c91302a29bc1
> ("reset: protect struct reset_control with its own mutex"), which only
> allows this to be trivially backported to v7.1, with adjustments needed
> for older kernels.
>
> The commit this is supposed to fix, 8a0de73cf9dc ("ASoC: ak4458: add
> optional reset control to instead of gpio"), is in v6.2. I'm not sure
> if something changed between v6.2 and now that exposed this issue for
> ak4458?

I could not find a specific change between v6.2 and now that newly exposed
this for ak4458; the window looks like it has always been there and just
became observable with async probe / SMP timing on the boards we hit it on.

>
> In short, do you have an opinion how (far) this should be backported,
> and does this warrant some Cc:stable tag?

You are right that this is only a partial fix for 0b52297f2288, and that
reset-gpio did not exist back then. Since the race is latent - it is only
reachable once a shared provider whose ops sleep exists (reset-gpio being
the first) - and since the patch would not apply before 8c91302a29bc1
("reset: protect struct reset_control with its own mutex", v7.1) anyway, I
would like to drop the Fixes: tag and Cc: stable for v4 and present this as
a correctness improvement to the shared-reset locking. Anyone hitting it on
an older kernel can backport by hand. But let me know if this is acceptable.


>
> > Suggested-by: Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>
> > Assisted-by: Anthropic:claude-opus-4-8
>
> Drop, see 1e035297ffda ("docs: remove Assisted-by references").

Ok.

>
> > Signed-off-by: Shengjiu Wang <shengjiu.wang@xxxxxxx>
> > Link: https://lore.kernel.org/all/aszDZlJ0KahNwgZO@xxxxxxxxxx/
> > ---
> > Changes in v3:
> > - Drop the reset_control_reset_atomic()/reset_control_deassert_atomic()
> >   variants and their reset.h declarations/stubs; gate the mutex on
> >   rcdev->can_sleep instead, so non-sleeping controllers keep the lockless
> >   path and need no new API.
> > - Use a dedicated rstc->shared_lock taken inside the SRCU read-side section
> >   instead of reusing rstc->lock before SRCU.
> > - Set can_sleep in reset-gpio; no other provider is converted.
> > - Drop the lockdep_assert_not_held() additions and the reset.rst / kerneldoc
> >   text about sleeping-vs-atomic variants.
> >
> > 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.
> >
> >  drivers/reset/core.c             | 125 +++++++++++++++++++++----------
> >  drivers/reset/reset-gpio.c       |   1 +
> >  include/linux/reset-controller.h |   3 +
> >  3 files changed, 89 insertions(+), 40 deletions(-)
> >
> > diff --git a/drivers/reset/core.c b/drivers/reset/core.c
> > index 38e189d04d09..dd6e47163f6c 100644
> > --- a/drivers/reset/core.c
> > +++ b/drivers/reset/core.c
> > @@ -52,6 +52,9 @@ static DEFINE_IDA(reset_gpio_ida);
> >   *                   only used for shared resets, which means that the value
> >   *                   will be either 0 or 1.
> >   * @lock: serializes the internals of reset_control_acquire()
> > + * @shared_lock: serializes the shared refcount update and the hardware op for
> > + *               controllers whose ops may sleep (rcdev->can_sleep); taken only
> > + *               inside the SRCU read-side critical section
> >   */
> >  struct reset_control {
> >       struct reset_controller_dev __rcu *rcdev;
> > @@ -65,6 +68,8 @@ struct reset_control {
> >       atomic_t deassert_count;
> >       atomic_t triggered_count;
> >       struct mutex lock;
> > +     /* serializes the shared refcount update with its hardware op */
> > +     struct mutex shared_lock;
> >  };
> >
> >  /**
> > @@ -355,6 +360,32 @@ static inline bool reset_control_is_array(struct reset_control *rstc)
> >       return rstc->array;
> >  }
> >
> > +static int __reset_control_reset(struct reset_control *rstc,
> > +                              struct reset_controller_dev *rcdev)
> > +{
> > +     int ret;
> > +
> > +     if (!rcdev->ops->reset)
> > +             return -ENOTSUPP;
> > +
> > +     if (rstc->shared) {
> > +             if (WARN_ON(atomic_read(&rstc->deassert_count) != 0))
> > +                     return -EINVAL;
> > +
> > +             if (atomic_inc_return(&rstc->triggered_count) != 1)
> > +                     return 0;
> > +     } else {
> > +             if (!rstc->acquired)
> > +                     return -EPERM;
> > +     }
> > +
> > +     ret = rcdev->ops->reset(rcdev, rstc->id);
> > +     if (rstc->shared && ret)
> > +             atomic_dec(&rstc->triggered_count);
> > +
> > +     return ret;
> > +}
> > +
> >  /**
> >   * reset_control_reset - reset the controlled device
> >   * @rstc: reset controller
> > @@ -362,6 +393,9 @@ static inline bool reset_control_is_array(struct reset_control *rstc)
> >   * 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
>
> s/reset/reset line/

Ok.

>
> The sashiko bot constructed an unlikely but theoretically possible
> scenario that requires to lock _assert() after all:
>
> https://sashiko.dev/#/patchset/20261021030925.3543841-1-shengjiu.wang%40oss.nxp.com
>
> Disregard the other issue, deassert_count leaking on ops->deassert()
> errors. It is pre-existing and can't be as easily fixed because
> consumers adapted to it.

Ok,  will update the _assert().

best regards
Shengjiu Wang