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

[Philipp Zabel]

On Di, 2026-10-20 at 10:29 +0200, Alvin Šipraga wrote:
> 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@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
> 
> 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.

Yes, that sounds like it could work.

> 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.)

Adding _atomic() variant at the very least would require correctly
setting can_sleep/fast_io for all reset controller implementations and
making sure that the sleeping controllers throw an error in the
_atomic() API.

> 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.

Seconded. I'd really like to move to an API that visibly separates
exclusive and shared resets, as well as sleeping / non-sleeping API,
but I'm afraid that would be a lot of churn.

> 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.

How about adding the can_sleep property, setting it for reset-gpio, and
adding the mutex first, and only then adding the other reset
controllers and the spinlock. That could allow to backport a relatively
constrained subset of the change.

regards
Philipp