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

["Alvin Šipraga" <alvin.sipraga@analog.com>]

On Tue, Oct 20, 2026 at 03:17:40PM +0800, Shengjiu Wang wrote:
> Hi Philipp
> 
> On Mon, Oct 19, 2026 at 1:52 PM <shengjiu.wang@oss.nxp.com> wrote:
> >
> > From: Shengjiu Wang <shengjiu.wang@nxp.com>
> >
> > 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@analog.com/
> > Assisted-by: Anthropic:claude-opus-4-8
> > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> > ---
> > 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@analog.com/T/#m66615477929b38f788d6d22a2c0bb9ca62173e89

The reset core could borrow a trick from the regmap library, where it
uses either a mutex or spinlock depending on whether the underlying
reset controller's ops can sleep or not. Check the struct
regmap_bus::fast_io member and how it's used in __regmap_init().

It would also allow the problem to be fixed for atomic users. Right now
you still have the race even if the underlying reset controller's ops
don't sleep and the _atomic() API is being used. But if you know that
the reset controller ops don't sleep, you can serialize shared reset
control operations using a spinlock.

With the above design you can leave the consumers alone and drop the
_atomic() variant of the API functions. (I'll also point out that adding
sleepy/atomic-awareness to the core would open the door for consumers to
report their requirements, e.g. that the reset control musn't sleep, and
for the core to validate this with the properties of the consumer.)

But... it requires an audit of all reset controller providers in the
tree, to see if they are atomic-safe or not, and report some kind of
.can_sleep property to the core (like regmap's .fast_io). I have a
feeling this is still more straightforward and less error-prone than
auditing all the consumers.

The other downside is that it's not a simple patch which can be
backported, which might be what you are looking for.

Any thoughts? I can also help out if the idea seems good.

Kind regards,
Alvin