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

[Philipp Zabel <p.zabel@pengutronix.de>]

On Di, 2026-10-20 at 15:17 +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?

Maybe add a bool can_sleep field to struct reset_controller_dev, set it
for the reset-gpio driver (which uses gpiod_set_value_cansleep), and
only take a mutex if can_sleep is set? That would have to be a new
lock, taken only inside the SRCU critical section.

regards
Philipp