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

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

On Tue, Oct 20, 2026 at 06:23:29PM +0800, Shengjiu Wang wrote:
> On Tue, Oct 20, 2026 at 6:03 PM Alvin Šipraga <alvin.sipraga@analog.com> wrote:
> >
> > On Tue, Oct 20, 2026 at 10:56:14AM +0200, Philipp Zabel wrote:
> > > 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@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://urldefense.com/v3/__https://lore.kernel.org/all/aszDZlJ0KahNwgZO@analog.com/__;!!A3Ni8CS0y2Y!-DPstHB0ai7dP0DW897uht_CO6I0TiFaRb7LtPgEydRyEl3Hliigq6BxOLE3llSaFx-aTBDOtenncE_4XlyaU7qCNQ$ 
> > > > > > 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://urldefense.com/v3/__https://lore.kernel.org/all/aszDZlJ0KahNwgZO@analog.com/T/*m66615477929b38f788d6d22a2c0bb9ca62173e89__;Iw!!A3Ni8CS0y2Y!-DPstHB0ai7dP0DW897uht_CO6I0TiFaRb7LtPgEydRyEl3Hliigq6BxOLE3llSaFx-aTBDOtenncE_4XlyfNK_xgw$ 
> > > >
> > > > 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.
> >
> > Yeah that makes sense. Shengjiu, if you also agree, I can pick up the
> > second phase (tree-wide audit + spinlock variant) if you don't have the
> > time. Or Philipp, do you think it both phases should be part of the same
> > overall series?
> >
> 
> Agreed on the phased approach. I will send the patch for phase 1.
> 
> Alvin, please go ahead and pick up phase 2 (the tree-wide audit +
> spinlock variant) - I'm happy for you to own it.

Sure thing! I'll wait for your patch(es) and build on top from there.

Cheers,
Alvin