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

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

[shengjiu.wang]

From: Shengjiu Wang <shengjiu.wang@nxp.com>

reset_control_deassert() is documented to guarantee that the reset is
deasserted after the call returns. For a shared reset control only the
first caller runs the hardware .deassert(); all later callers just
increment deassert_count and return immediately:

	if (atomic_inc_return(&rstc->deassert_count) != 1)
		return 0;

When two consumers of the same shared reset (for example two codecs that
share one reset GPIO) call reset_control_deassert() concurrently, the
second caller can return before the first caller's .deassert() - which
may sleep, e.g. for a GPIO-expander based controller - has actually
released the line. The second consumer then proceeds while the device is
still in reset, contradicting the documented guarantee. The assert and
reset paths have the same problem: only the first shared caller performs
the hardware operation while concurrent callers return early.

Serialize the shared refcount update and the hardware .assert()/
.deassert()/.reset() under the existing per-control rstc->lock so that a
second concurrent caller waits for the first caller's hardware operation
to complete. The lock is taken before the SRCU read section, matching the
ordering already used by reset_control_acquire(). These paths do not take
rcdev->lock, so no new lock-ordering inversion is introduced.

Fixes: 0b52297f2288 ("reset: Add support for shared reset controls")
Link: https://lore.kernel.org/all/aszDZlJ0KahNwgZO@analog.com/
Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
---
 drivers/reset/core.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/reset/core.c b/drivers/reset/core.c
index 38e189d04d09..7536b61cc5df 100644
--- a/drivers/reset/core.c
+++ b/drivers/reset/core.c
@@ -51,7 +51,8 @@ static DEFINE_IDA(reset_gpio_ida);
  * @triggered_count: Number of times this reset line has been reset. Currently
  *                   only used for shared resets, which means that the value
  *                   will be either 0 or 1.
- * @lock: serializes the internals of reset_control_acquire()
+ * @lock: serializes the internals of reset_control_acquire() and the
+ *        assert/deassert/reset of shared reset controls
  */
 struct reset_control {
 	struct reset_controller_dev __rcu *rcdev;
@@ -382,6 +383,12 @@ int reset_control_reset(struct reset_control *rstc)
 	if (reset_control_is_array(rstc))
 		return reset_control_array_reset(rstc_to_array(rstc));
 
+	/*
+	 * Hold the lock across the shared refcount update and the hardware
+	 * reset so that a concurrent caller does not continue before this
+	 * caller's not-yet-finished reset pulse has been issued.
+	 */
+	guard(mutex)(&rstc->lock);
 	guard(srcu)(&rstc->srcu);
 
 	rcdev = srcu_dereference(rstc->rcdev, &rstc->srcu);
@@ -503,6 +510,12 @@ int reset_control_assert(struct reset_control *rstc)
 	if (reset_control_is_array(rstc))
 		return reset_control_array_assert(rstc_to_array(rstc));
 
+	/*
+	 * Hold the lock across the shared refcount update and the hardware
+	 * assert so that a concurrent caller does not observe the line as
+	 * released while this caller is still driving it.
+	 */
+	guard(mutex)(&rstc->lock);
 	guard(srcu)(&rstc->srcu);
 
 	rcdev = srcu_dereference(rstc->rcdev, &rstc->srcu);
@@ -599,6 +612,12 @@ int reset_control_deassert(struct reset_control *rstc)
 	if (reset_control_is_array(rstc))
 		return reset_control_array_deassert(rstc_to_array(rstc));
 
+	/*
+	 * Hold the lock across the shared refcount update and the hardware
+	 * deassert so that a concurrent caller does not continue while the
+	 * line is still asserted by this caller's not-yet-finished deassert.
+	 */
+	guard(mutex)(&rstc->lock);
 	guard(srcu)(&rstc->srcu);
 
 	rcdev = srcu_dereference(rstc->rcdev, &rstc->srcu);
-- 
2.34.1

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

[Alvin Šipraga]

On Tue, Oct 13, 2026 at 06:19:47PM +0800, shengjiu.wang@oss.nxp.com wrote:
> From: Shengjiu Wang <shengjiu.wang@nxp.com>
> 
> reset_control_deassert() is documented to guarantee that the reset is
> deasserted after the call returns. For a shared reset control only the
> first caller runs the hardware .deassert(); all later callers just
> increment deassert_count and return immediately:
> 
> 	if (atomic_inc_return(&rstc->deassert_count) != 1)
> 		return 0;
> 
> When two consumers of the same shared reset (for example two codecs that
> share one reset GPIO) call reset_control_deassert() concurrently, the
> second caller can return before the first caller's .deassert() - which
> may sleep, e.g. for a GPIO-expander based controller - has actually
> released the line. The second consumer then proceeds while the device is
> still in reset, contradicting the documented guarantee. The assert and
> reset paths have the same problem: only the first shared caller performs
> the hardware operation while concurrent callers return early.
> 
> Serialize the shared refcount update and the hardware .assert()/
> .deassert()/.reset() under the existing per-control rstc->lock so that a
> second concurrent caller waits for the first caller's hardware operation
> to complete. The lock is taken before the SRCU read section, matching the
> ordering already used by reset_control_acquire(). These paths do not take
> rcdev->lock, so no new lock-ordering inversion is introduced.
> 
> Fixes: 0b52297f2288 ("reset: Add support for shared reset controls")
> Link: https://lore.kernel.org/all/aszDZlJ0KahNwgZO@analog.com/
> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>

Reviewed-by: Alvin Šipraga <alvin.sipraga@analog.com>

Minor remark about the comments added below.

> ---
>  drivers/reset/core.c | 21 ++++++++++++++++++++-
>  1 file changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/reset/core.c b/drivers/reset/core.c
> index 38e189d04d09..7536b61cc5df 100644
> --- a/drivers/reset/core.c
> +++ b/drivers/reset/core.c
> @@ -51,7 +51,8 @@ static DEFINE_IDA(reset_gpio_ida);
>   * @triggered_count: Number of times this reset line has been reset. Currently
>   *                   only used for shared resets, which means that the value
>   *                   will be either 0 or 1.
> - * @lock: serializes the internals of reset_control_acquire()
> + * @lock: serializes the internals of reset_control_acquire() and the
> + *        assert/deassert/reset of shared reset controls
>   */
>  struct reset_control {
>  	struct reset_controller_dev __rcu *rcdev;
> @@ -382,6 +383,12 @@ int reset_control_reset(struct reset_control *rstc)
>  	if (reset_control_is_array(rstc))
>  		return reset_control_array_reset(rstc_to_array(rstc));
>  
> +	/*
> +	 * Hold the lock across the shared refcount update and the hardware
> +	 * reset so that a concurrent caller does not continue before this
> +	 * caller's not-yet-finished reset pulse has been issued.
> +	 */
> +	guard(mutex)(&rstc->lock);
>  	guard(srcu)(&rstc->srcu);
>  
>  	rcdev = srcu_dereference(rstc->rcdev, &rstc->srcu);
> @@ -503,6 +510,12 @@ int reset_control_assert(struct reset_control *rstc)
>  	if (reset_control_is_array(rstc))
>  		return reset_control_array_assert(rstc_to_array(rstc));
>  
> +	/*
> +	 * Hold the lock across the shared refcount update and the hardware
> +	 * assert so that a concurrent caller does not observe the line as
> +	 * released while this caller is still driving it.
> +	 */

This comment is a little hard to interpret (what is meant by
"driving"/"released" here?).

But given the kdoc string update for reset_control::lock, IMO you could
drop all the inline comments in this patch.

> +	guard(mutex)(&rstc->lock);
>  	guard(srcu)(&rstc->srcu);
>  
>  	rcdev = srcu_dereference(rstc->rcdev, &rstc->srcu);
> @@ -599,6 +612,12 @@ int reset_control_deassert(struct reset_control *rstc)
>  	if (reset_control_is_array(rstc))
>  		return reset_control_array_deassert(rstc_to_array(rstc));
>  
> +	/*
> +	 * Hold the lock across the shared refcount update and the hardware
> +	 * deassert so that a concurrent caller does not continue while the
> +	 * line is still asserted by this caller's not-yet-finished deassert.
> +	 */
> +	guard(mutex)(&rstc->lock);
>  	guard(srcu)(&rstc->srcu);
>  
>  	rcdev = srcu_dereference(rstc->rcdev, &rstc->srcu);
> -- 
> 2.34.1
> 

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

[Philipp Zabel]

On Di, 2026-10-13 at 18:19 +0800, shengjiu.wang@oss.nxp.com wrote:
> From: Shengjiu Wang <shengjiu.wang@nxp.com>
> 
> reset_control_deassert() is documented to guarantee that the reset is
> deasserted after the call returns. For a shared reset control only the
> first caller runs the hardware .deassert(); all later callers just
> increment deassert_count and return immediately:
> 
> 	if (atomic_inc_return(&rstc->deassert_count) != 1)
> 		return 0;
> 
> When two consumers of the same shared reset (for example two codecs that
> share one reset GPIO) call reset_control_deassert() concurrently, the
> second caller can return before the first caller's .deassert() - which
> may sleep, e.g. for a GPIO-expander based controller - has actually
> released the line. The second consumer then proceeds while the device is
> still in reset, contradicting the documented guarantee. The assert and
> reset paths have the same problem: only the first shared caller performs
> the hardware operation while concurrent callers return early.
> 
> Serialize the shared refcount update and the hardware .assert()/
> .deassert()/.reset() under the existing per-control rstc->lock so that a
> second concurrent caller waits for the first caller's hardware operation
> to complete. The lock is taken before the SRCU read section, matching the
> ordering already used by reset_control_acquire(). These paths do not take
> rcdev->lock, so no new lock-ordering inversion is introduced.
> 
> Fixes: 0b52297f2288 ("reset: Add support for shared reset controls")
> Link: https://lore.kernel.org/all/aszDZlJ0KahNwgZO@analog.com/
> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> ---
>  drivers/reset/core.c | 21 ++++++++++++++++++++-
>  1 file changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/reset/core.c b/drivers/reset/core.c
> index 38e189d04d09..7536b61cc5df 100644
> --- a/drivers/reset/core.c
> +++ b/drivers/reset/core.c
> @@ -51,7 +51,8 @@ static DEFINE_IDA(reset_gpio_ida);
>   * @triggered_count: Number of times this reset line has been reset. Currently
>   *                   only used for shared resets, which means that the value
>   *                   will be either 0 or 1.
> - * @lock: serializes the internals of reset_control_acquire()
> + * @lock: serializes the internals of reset_control_acquire() and the
> + *        assert/deassert/reset of shared reset controls

The lock is also taken for exclusive reset controls.

>   */
>  struct reset_control {
>  	struct reset_controller_dev __rcu *rcdev;
> @@ -382,6 +383,12 @@ int reset_control_reset(struct reset_control *rstc)
>  	if (reset_control_is_array(rstc))
>  		return reset_control_array_reset(rstc_to_array(rstc));
>  
> +	/*
> +	 * Hold the lock across the shared refcount update and the hardware
> +	 * reset so that a concurrent caller does not continue before this
> +	 * caller's not-yet-finished reset pulse has been issued.
> +	 */
> +	guard(mutex)(&rstc->lock);

The current wording of the API documentation does not require this, but
it clearly makes no sense that the second of two racing
reset_control_reset() calls on a shared reset control can return before
the reset pulse has finished.

Could you update the kerneldoc comment, for example add "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." to
the shared reset paragraph?

That being said, this is not required for exclusive resets, and I think
there may be users handling reset controls in atomic context.
Unfortunately the API never explicitly declared that
reset_control_assert/deassert_reset() calls may sleep, and this just
happens to work if the specific reset controller implementation doesn't
sleep.

I'm not sure what to do about this. We could split the shared and
exclusive paths and only lock in the shared path, or fix the atomic
users, possibly by introducing
reset_control_assert/deassert/reset_atomic() first.

>  	guard(srcu)(&rstc->srcu);
>  
>  	rcdev = srcu_dereference(rstc->rcdev, &rstc->srcu);
> @@ -503,6 +510,12 @@ int reset_control_assert(struct reset_control *rstc)
>  	if (reset_control_is_array(rstc))
>  		return reset_control_array_assert(rstc_to_array(rstc));
>  
> +	/*
> +	 * Hold the lock across the shared refcount update and the hardware
> +	 * assert so that a concurrent caller does not observe the line as
> +	 * released while this caller is still driving it.
> +	 */
> +	guard(mutex)(&rstc->lock);

Nothing in the API requires shared reset_control_reset() to be
serialized, why is this necessary? The comment doesn't make this clear
to me.

>  	guard(srcu)(&rstc->srcu);
>  
>  	rcdev = srcu_dereference(rstc->rcdev, &rstc->srcu);
> @@ -599,6 +612,12 @@ int reset_control_deassert(struct reset_control *rstc)
>  	if (reset_control_is_array(rstc))
>  		return reset_control_array_deassert(rstc_to_array(rstc));
>  
> +	/*
> +	 * Hold the lock across the shared refcount update and the hardware
> +	 * deassert so that a concurrent caller does not continue while the
> +	 * line is still asserted by this caller's not-yet-finished deassert.
> +	 */
> +	guard(mutex)(&rstc->lock);

Yes, this aligns the function with the documented intention for shared
resets. The above points about exclusive resets also apply.

regards
Philipp

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

[Shengjiu Wang]

On Wed, Oct 14, 2026 at 1:47 AM Philipp Zabel <p.zabel@pengutronix.de> wrote:
>
> On Di, 2026-10-13 at 18:19 +0800, shengjiu.wang@oss.nxp.com wrote:
> > From: Shengjiu Wang <shengjiu.wang@nxp.com>
> >
> > reset_control_deassert() is documented to guarantee that the reset is
> > deasserted after the call returns. For a shared reset control only the
> > first caller runs the hardware .deassert(); all later callers just
> > increment deassert_count and return immediately:
> >
> >       if (atomic_inc_return(&rstc->deassert_count) != 1)
> >               return 0;
> >
> > When two consumers of the same shared reset (for example two codecs that
> > share one reset GPIO) call reset_control_deassert() concurrently, the
> > second caller can return before the first caller's .deassert() - which
> > may sleep, e.g. for a GPIO-expander based controller - has actually
> > released the line. The second consumer then proceeds while the device is
> > still in reset, contradicting the documented guarantee. The assert and
> > reset paths have the same problem: only the first shared caller performs
> > the hardware operation while concurrent callers return early.
> >
> > Serialize the shared refcount update and the hardware .assert()/
> > .deassert()/.reset() under the existing per-control rstc->lock so that a
> > second concurrent caller waits for the first caller's hardware operation
> > to complete. The lock is taken before the SRCU read section, matching the
> > ordering already used by reset_control_acquire(). These paths do not take
> > rcdev->lock, so no new lock-ordering inversion is introduced.
> >
> > Fixes: 0b52297f2288 ("reset: Add support for shared reset controls")
> > Link: https://lore.kernel.org/all/aszDZlJ0KahNwgZO@analog.com/
> > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> > ---
> >  drivers/reset/core.c | 21 ++++++++++++++++++++-
> >  1 file changed, 20 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/reset/core.c b/drivers/reset/core.c
> > index 38e189d04d09..7536b61cc5df 100644
> > --- a/drivers/reset/core.c
> > +++ b/drivers/reset/core.c
> > @@ -51,7 +51,8 @@ static DEFINE_IDA(reset_gpio_ida);
> >   * @triggered_count: Number of times this reset line has been reset. Currently
> >   *                   only used for shared resets, which means that the value
> >   *                   will be either 0 or 1.
> > - * @lock: serializes the internals of reset_control_acquire()
> > + * @lock: serializes the internals of reset_control_acquire() and the
> > + *        assert/deassert/reset of shared reset controls
>
> The lock is also taken for exclusive reset controls.

Right - in v1 the guard(mutex) is taken before the rstc->shared check,
so it is taken for exclusive controls too, which both contradicts the
kerneldoc wording and is the root of the atomic-context concern below.
In v2 I will take it only inside the shared branch.

>
> >   */
> >  struct reset_control {
> >       struct reset_controller_dev __rcu *rcdev;
> > @@ -382,6 +383,12 @@ int reset_control_reset(struct reset_control *rstc)
> >       if (reset_control_is_array(rstc))
> >               return reset_control_array_reset(rstc_to_array(rstc));
> >
> > +     /*
> > +      * Hold the lock across the shared refcount update and the hardware
> > +      * reset so that a concurrent caller does not continue before this
> > +      * caller's not-yet-finished reset pulse has been issued.
> > +      */
> > +     guard(mutex)(&rstc->lock);
>
> The current wording of the API documentation does not require this, but
> it clearly makes no sense that the second of two racing
> reset_control_reset() calls on a shared reset control can return before
> the reset pulse has finished.
>
> Could you update the kerneldoc comment, for example add "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." to
> the shared reset paragraph?

Will do, using your suggested wording.

>
> That being said, this is not required for exclusive resets, and I think
> there may be users handling reset controls in atomic context.
> Unfortunately the API never explicitly declared that
> reset_control_assert/deassert_reset() calls may sleep, and this just
> happens to work if the specific reset controller implementation doesn't
> sleep.
>
> I'm not sure what to do about this. We could split the shared and
> exclusive paths and only lock in the shared path, or fix the atomic
> users, possibly by introducing
> reset_control_assert/deassert/reset_atomic() first.

Agreed, the mutex must not be added to the exclusive path. For v2 I will
only take rstc->lock inside the rstc->shared branch in
reset_control_deassert() and reset_control_reset(), leaving the exclusive
path (and any exclusive atomic callers) completely unchanged.

>
> >       guard(srcu)(&rstc->srcu);
> >
> >       rcdev = srcu_dereference(rstc->rcdev, &rstc->srcu);
> > @@ -503,6 +510,12 @@ int reset_control_assert(struct reset_control *rstc)
> >       if (reset_control_is_array(rstc))
> >               return reset_control_array_assert(rstc_to_array(rstc));
> >
> > +     /*
> > +      * Hold the lock across the shared refcount update and the hardware
> > +      * assert so that a concurrent caller does not observe the line as
> > +      * released while this caller is still driving it.
> > +      */
> > +     guard(mutex)(&rstc->lock);
>
> Nothing in the API requires shared reset_control_reset() to be
> serialized, why is this necessary? The comment doesn't make this clear
> to me.

Since the API explicitly does *not* guarantee that the line is asserted
after a shared reset_control_assert() call, there is no documented
guarantee to enforce, and no need to serialize that path. I will remove
the lock from reset_control_assert() in v2.

best regards
shengjiu Wang
>
> >       guard(srcu)(&rstc->srcu);
> >
> >       rcdev = srcu_dereference(rstc->rcdev, &rstc->srcu);
> > @@ -599,6 +612,12 @@ int reset_control_deassert(struct reset_control *rstc)
> >       if (reset_control_is_array(rstc))
> >               return reset_control_array_deassert(rstc_to_array(rstc));
> >
> > +     /*
> > +      * Hold the lock across the shared refcount update and the hardware
> > +      * deassert so that a concurrent caller does not continue while the
> > +      * line is still asserted by this caller's not-yet-finished deassert.
> > +      */
> > +     guard(mutex)(&rstc->lock);
>
> Yes, this aligns the function with the documented intention for shared
> resets. The above points about exclusive resets also apply.
>
> regards
> Philipp
>