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

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

On Wed, Oct 21, 2026 at 03:24:23PM +0200, Philipp Zabel wrote:
> On Mi, 2026-10-21 at 11:09 +0800, shengjiu.wang@oss.nxp.com wrote:
> > From: Shengjiu Wang <shengjiu.wang@nxp.com>
> > 
> > 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.
> 
> > 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.
> 
> > 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?
> 
> In short, do you have an opinion how (far) this should be backported,
> and does this warrant some Cc:stable tag?
> 
> > Suggested-by: Philipp Zabel <p.zabel@pengutronix.de>
> > Assisted-by: Anthropic:claude-opus-4-8
> 
> Drop, see 1e035297ffda ("docs: remove Assisted-by references").
> 
> > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> > Link: https://lore.kernel.org/all/aszDZlJ0KahNwgZO@analog.com/
> > ---
> > 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/
> 
> 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

Looking forward a bit to the second phase where we have everything
serialized, maybe it will make sense to also lock around the call to the
.status driver op too? Not suggesting we do it here (as this is just a
bugfix), but in general I like when subsystems offer a guarantee that
driver ops can't be called concurrently. Also the status is kind of
undefined if you're currently in the process of manipulating the reset
line.

What I had in mind later on was to rename what Shengjiu has called
shared_lock to ops_lock instead. With the following primitives:

  can_sleep?    CONTROL TYPE       LOCKING PRIMITIVE
  --------------------------------------------------
  no            shared             spin_lock_irqsave
  yes           shared             mutex
  no            exclusive          none? see [*]
  yes           exclusive          none? see [*]

Where all reset control operation functions look like:

  reset_control_op() {
    ...
  
    lock(ops_lock);
  
    __reset_control_op(); /* inc/dec, ->op() call, etc. here */
  
    unlock(ops_lock);
  }

[*] I'm not sure if it makes sense to add any locking here? I can only
see it as API misuse if somebody is manipulating an exclusive reset
control from two places at once.

Just some thoughts, not particularly relevant to Shengjiu's change :)

Kind regards,
Alvin