Re: [PATCH v2 10/14] reset: protect struct reset_control with its own mutex

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

On Mo, 2026-02-23 at 11:06 +0100, Bartosz Golaszewski wrote:
> Currently we use a single, global mutex - misleadingly names
> reset_list_mutex - to protect the global list of reset devices,
> per-controller list of reset control handles and also internal fields of
> struct reset_control. Locking can be made a lot more fine-grained if we
> use a separate mutex for serializing operations on the list AND
> accessing the reset control handle.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
> ---
>  drivers/reset/core.c | 38 ++++++++++++++------------------------
>  1 file changed, 14 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/reset/core.c b/drivers/reset/core.c
> index d4813c712abf3df7993b0c2be1fe292b89241d11..647e7112779517d2425f55728e1e7fb6e76a8045 100644
> --- a/drivers/reset/core.c
> +++ b/drivers/reset/core.c
> @@ -49,6 +49,7 @@ 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 access to other fields of this structure

This is not correct, the lock only serializes reset_control_acquire().

>   */
>  struct reset_control {
>  	struct reset_controller_dev __rcu *rcdev;
> @@ -61,6 +62,7 @@ struct reset_control {
>  	bool array;
>  	atomic_t deassert_count;
>  	atomic_t triggered_count;
> +	struct mutex lock;
>  };
>  
>  /**
> @@ -707,7 +709,7 @@ int reset_control_acquire(struct reset_control *rstc)
>  	if (reset_control_is_array(rstc))
>  		return reset_control_array_acquire(rstc_to_array(rstc));
>  
> -	guard(mutex)(&reset_list_mutex);
> +	guard(mutex)(&rstc->lock);
>  
>  	if (rstc->acquired)
>  		return 0;
> @@ -859,6 +861,7 @@ __reset_control_get_internal(struct reset_controller_dev *rcdev,
>  	list_add(&rstc->list, &rcdev->reset_control_head);
>  	rstc->id = index;
>  	kref_init(&rstc->refcnt);
> +	mutex_init(&rstc->lock);
>  	rstc->acquired = acquired;
>  	rstc->shared = shared;
>  	get_device(rcdev->dev);
> @@ -872,8 +875,6 @@ static void __reset_control_release(struct kref *kref)
>  						  refcnt);
>  	struct reset_controller_dev *rcdev;
>  
> -	lockdep_assert_held(&reset_list_mutex);
> -
>  	scoped_guard(srcu, &rstc->srcu) {
>  		rcdev = rcu_replace_pointer(rstc->rcdev, NULL, true);
>  		if (rcdev) {
> @@ -882,15 +883,14 @@ static void __reset_control_release(struct kref *kref)
>  		}
>  	}
>  
> +	mutex_destroy(&rstc->lock);
>  	synchronize_srcu(&rstc->srcu);
>  	cleanup_srcu_struct(&rstc->srcu);
>  	kfree(rstc);
>  }
>  
> -static void __reset_control_put_internal(struct reset_control *rstc)
> +static void reset_control_put_internal(struct reset_control *rstc)
>  {
> -	lockdep_assert_held(&reset_list_mutex);
> -

With this, reset_control_put() is not serialized against
reset_control_get() anymore.
A __reset_control_get_internal(), running concurrently under rcdev-
>lock, might find this rstc right before __reset_control_release()
starts tearing it down.


regards
Philipp