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

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

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
>