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

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

[shengjiu.wang]

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.

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.

Fixes: 0b52297f2288 ("reset: Add support for shared reset controls")
Suggested-by: Philipp Zabel <p.zabel@pengutronix.de>
Assisted-by: Anthropic:claude-opus-4-8
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
+ * pulse at any point in the past, not necessarily during the function call.
  * Consumers must not use reset_control_(de)assert on shared reset lines when
  * reset_control_reset has been used.
  *
@@ -371,7 +405,6 @@ static inline bool reset_control_is_array(struct reset_control *rstc)
 int reset_control_reset(struct reset_control *rstc)
 {
 	struct reset_controller_dev *rcdev;
-	int ret;
 
 	if (!rstc)
 		return 0;
@@ -388,25 +421,18 @@ int reset_control_reset(struct reset_control *rstc)
 	if (!rcdev)
 		return -ENODEV;
 
-	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;
+	/*
+	 * For a shared reset only the first caller runs the hardware op while
+	 * concurrent callers bump the refcount and return. If the op may sleep
+	 * (rcdev->can_sleep), serialize the refcount update and the op so a
+	 * concurrent caller cannot return before the op has finished.
+	 */
+	if (rstc->shared && rcdev->can_sleep) {
+		guard(mutex)(&rstc->shared_lock);
+		return __reset_control_reset(rstc, rcdev);
 	}
 
-	ret = rcdev->ops->reset(rcdev, rstc->id);
-	if (rstc->shared && ret)
-		atomic_dec(&rstc->triggered_count);
-
-	return ret;
+	return __reset_control_reset(rstc, rcdev);
 }
 EXPORT_SYMBOL_GPL(reset_control_reset);
 
@@ -575,6 +601,36 @@ int reset_control_bulk_assert(int num_rstcs,
 }
 EXPORT_SYMBOL_GPL(reset_control_bulk_assert);
 
+static int __reset_control_deassert(struct reset_control *rstc,
+				    struct reset_controller_dev *rcdev)
+{
+	if (rstc->shared) {
+		if (WARN_ON(atomic_read(&rstc->triggered_count) != 0))
+			return -EINVAL;
+
+		if (atomic_inc_return(&rstc->deassert_count) != 1)
+			return 0;
+	} else {
+		if (!rstc->acquired) {
+			WARN(1, "reset %s (ID: %u) is not acquired\n",
+			     rcdev_name(rcdev), rstc->id);
+			return -EPERM;
+		}
+	}
+
+	/*
+	 * If the reset controller does not implement .deassert(), we assume
+	 * that it handles self-deasserting reset lines via .reset(). In that
+	 * case, the reset lines are deasserted by default. If that is not the
+	 * case, the reset controller driver should implement .deassert() and
+	 * return -ENOTSUPP.
+	 */
+	if (!rcdev->ops->deassert)
+		return 0;
+
+	return rcdev->ops->deassert(rcdev, rstc->id);
+}
+
 /**
  * reset_control_deassert - deasserts the reset line
  * @rstc: reset controller
@@ -605,31 +661,18 @@ int reset_control_deassert(struct reset_control *rstc)
 	if (!rcdev)
 		return -ENODEV;
 
-	if (rstc->shared) {
-		if (WARN_ON(atomic_read(&rstc->triggered_count) != 0))
-			return -EINVAL;
-
-		if (atomic_inc_return(&rstc->deassert_count) != 1)
-			return 0;
-	} else {
-		if (!rstc->acquired) {
-			WARN(1, "reset %s (ID: %u) is not acquired\n",
-			     rcdev_name(rcdev), rstc->id);
-			return -EPERM;
-		}
-	}
-
 	/*
-	 * If the reset controller does not implement .deassert(), we assume
-	 * that it handles self-deasserting reset lines via .reset(). In that
-	 * case, the reset lines are deasserted by default. If that is not the
-	 * case, the reset controller driver should implement .deassert() and
-	 * return -ENOTSUPP.
+	 * For a shared reset only the first caller runs the hardware op while
+	 * concurrent callers bump the refcount and return. If the op may sleep
+	 * (rcdev->can_sleep), serialize the refcount update and the op so a
+	 * concurrent caller cannot return before the op has finished.
 	 */
-	if (!rcdev->ops->deassert)
-		return 0;
+	if (rstc->shared && rcdev->can_sleep) {
+		guard(mutex)(&rstc->shared_lock);
+		return __reset_control_deassert(rstc, rcdev);
+	}
 
-	return rcdev->ops->deassert(rcdev, rstc->id);
+	return __reset_control_deassert(rstc, rcdev);
 }
 EXPORT_SYMBOL_GPL(reset_control_deassert);
 
@@ -879,6 +922,7 @@ __reset_control_get_internal(struct reset_controller_dev *rcdev,
 	rstc->id = index;
 	kref_init(&rstc->refcnt);
 	mutex_init(&rstc->lock);
+	mutex_init(&rstc->shared_lock);
 	rstc->acquired = acquired;
 	rstc->shared = shared;
 	get_device(rcdev->dev);
@@ -901,6 +945,7 @@ static void __reset_control_release(struct kref *kref)
 	}
 
 	mutex_destroy(&rstc->lock);
+	mutex_destroy(&rstc->shared_lock);
 }
 
 static void reset_control_put_internal(struct reset_control *rstc)
diff --git a/drivers/reset/reset-gpio.c b/drivers/reset/reset-gpio.c
index bf95296a512e..b4a198792107 100644
--- a/drivers/reset/reset-gpio.c
+++ b/drivers/reset/reset-gpio.c
@@ -67,6 +67,7 @@ static int reset_gpio_probe(struct auxiliary_device *adev,
 				     "Could not get reset gpios\n");
 
 	priv->rc.ops = &reset_gpio_ops;
+	priv->rc.can_sleep = true;
 	priv->rc.owner = THIS_MODULE;
 	priv->rc.dev = dev;
 
diff --git a/include/linux/reset-controller.h b/include/linux/reset-controller.h
index 52a5a4e81f18..7318c07b80e1 100644
--- a/include/linux/reset-controller.h
+++ b/include/linux/reset-controller.h
@@ -47,6 +47,8 @@ struct of_phandle_args;
  *                id as given to the reset control ops, defaults to
  *                :c:func:`fwnode_reset_simple_xlate`
  * @nr_resets: number of reset controls in this reset controller device
+ * @can_sleep: set by the provider if its reset ops may sleep; makes the core
+ *             serialize the shared reset and deassert paths under a mutex
  * @lock: protects the reset control list from concurrent access
  */
 struct reset_controller_dev {
@@ -64,6 +66,7 @@ struct reset_controller_dev {
 	int (*fwnode_xlate)(struct reset_controller_dev *rcdev,
 			    const struct fwnode_reference_args *reset_spec);
 	unsigned int nr_resets;
+	bool can_sleep;
 	struct mutex lock;
 };
 
-- 
2.34.1

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

[Philipp Zabel]

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

Disregard the other issue, deassert_count leaking on ops->deassert()
errors. It is pre-existing and can't be as easily fixed because
consumers adapted to it.


regards
Philipp

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

[Alvin Šipraga]

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

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

[Philipp Zabel]

On Mi, 2026-10-21 at 16:16 +0200, Alvin Šipraga wrote:
[...]
> 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.

That doesn't change with locking if you can't act on the result of the
status return value under the lock.

A concurrent assert/deassert may lose the race against status and
toggle the reset line right after status unlocks, before it returns.

> What I had in mind later on was to rename what Shengjiu has called
> shared_lock to ops_lock instead.

We could also combine this with the exclusive reset control's acquire
"lock" if we move that inside the SRCU section.

> 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 [*]

Yes.

> 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);
>   }

It might make sense to split these into _exclusive and _shared
variants.

> [*] 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.

Right.


regards
Philipp

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

[Alvin Šipraga]

On Thu, Oct 22, 2026 at 10:43:35AM +0200, Philipp Zabel wrote:
> On Mi, 2026-10-21 at 16:16 +0200, Alvin Šipraga wrote:
> [...]
> > 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.
> 
> That doesn't change with locking if you can't act on the result of the
> status return value under the lock.
> 
> A concurrent assert/deassert may lose the race against status and
> toggle the reset line right after status unlocks, before it returns.

True, it doesn't make any material difference from a consumer point of
view.

I was mostly thinking about the provider side though. Such serialization
guarantees from the core allow reset controller driver authors to use
fewer locks in their own code if they know that the ops can't get called
concurrently. On top of that it makes things consistent (all ops called
under the same locks).

Still think it's not worth serializing .status? Just want to know if you
expressly don't want it in the patches I will send after Shengjiu is
done.

> 
> > What I had in mind later on was to rename what Shengjiu has called
> > shared_lock to ops_lock instead.
> 
> We could also combine this with the exclusive reset control's acquire
> "lock" if we move that inside the SRCU section.

I had the same thought. But I kept bumping into the fact that the
rcdev->lock mutex is taken in reset_control_acquire() while we are
holding rstc->lock. If rstc->lock becomes a spinlock then that's not OK.
And converting rcdev->lock into a spinlock doesn't seem feasible.

But on closer inspection I noticed a couple of things. Look at the function:

int reset_control_acquire(struct reset_control *rstc)
{
	struct reset_controller_dev *rcdev;
	struct reset_control *rc;

	if (!rstc)
		return 0;

	if (WARN_ON(IS_ERR(rstc)))
		return -EINVAL;

	if (reset_control_is_array(rstc))
		return reset_control_array_acquire(rstc_to_array(rstc));

(A)	guard(mutex)(&rstc->lock);

	if (rstc->acquired)
		return 0;

	guard(srcu)(&rstc->srcu);

	rcdev = srcu_dereference(rstc->rcdev, &rstc->srcu);
	if (!rcdev)
		return -ENODEV;

(B)	scoped_guard(mutex, &rcdev->lock) {
		list_for_each_entry(rc, &rcdev->reset_control_head, list) {
			if (rstc != rc && rstc->id == rc->id) {
				if (rc->acquired)
					return -EBUSY;
			}
		}
	}
(C)
(D)	rstc->acquired = true;

	return 0;
}

1. (A) is the only place where rstc->lock is actually taken. Ostensibly
it should be serializing acquire/release logic, but from an API point of
view it doesn't make much sense. Just like the remark [*] in my last
email (see below), you also shouldn't be trying to acquire the same
exclusive reset control from two places at once. And it doesn't offer
any synchronization with other exclusive reset controls for the same
controller line.

2. The synchronization with other reset controls is done with
rcdev->lock at (B). But actually there is a narrow race here. Two tasks
could try to acquire their own exclusive control of the same
line. Assuming the line is currently released, task 1 could reach (C),
get preempted by task 2, which reaches (D) before task 1 is run
again. Then you end up with two controls having acquired exclusive
control.

The obvious fix for (2) is to put (D) inside the scoped guard block.

But then you achieve everything that rstc->lock is supposed to
achieve. So I think we can get rid of that lock altogether?

> 
> > 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 [*]
> 
> Yes.
> 
> > 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);
> >   }
> 
> It might make sense to split these into _exclusive and _shared
> variants.
> 
> > [*] 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.
> 
> Right.
> 

Kind regards,
Alvin

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

[Shengjiu Wang]

On Wed, Oct 21, 2026 at 9:31 PM Philipp Zabel <p.zabel@pengutronix.de> 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.

Ok, will move this to be 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.

Ok. will split them.

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

I could not find a specific change between v6.2 and now that newly exposed
this for ak4458; the window looks like it has always been there and just
became observable with async probe / SMP timing on the boards we hit it on.

>
> In short, do you have an opinion how (far) this should be backported,
> and does this warrant some Cc:stable tag?

You are right that this is only a partial fix for 0b52297f2288, and that
reset-gpio did not exist back then. Since the race is latent - it is only
reachable once a shared provider whose ops sleep exists (reset-gpio being
the first) - and since the patch would not apply before 8c91302a29bc1
("reset: protect struct reset_control with its own mutex", v7.1) anyway, I
would like to drop the Fixes: tag and Cc: stable for v4 and present this as
a correctness improvement to the shared-reset locking. Anyone hitting it on
an older kernel can backport by hand. But let me know if this is acceptable.


>
> > Suggested-by: Philipp Zabel <p.zabel@pengutronix.de>
> > Assisted-by: Anthropic:claude-opus-4-8
>
> Drop, see 1e035297ffda ("docs: remove Assisted-by references").

Ok.

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

Ok.

>
> 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
>
> Disregard the other issue, deassert_count leaking on ops->deassert()
> errors. It is pre-existing and can't be as easily fixed because
> consumers adapted to it.

Ok,  will update the _assert().

best regards
Shengjiu Wang