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

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

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