Re: [PATCH 2/2] reset: Fix circular locking dependency in reset_controller_unregister()

[Tzung-Bi Shih]

On Mon, Nov 02, 2026 at 06:54:04PM +0100, Philipp Zabel wrote:
> On Mo, 2026-11-02 at 23:58 +0800, Tzung-Bi Shih wrote:
> > @@ -179,6 +179,7 @@ static void reset_controller_remove(struct reset_controller_dev *rcdev,
> >  void reset_controller_unregister(struct reset_controller_dev *rcdev)
> >  {
> >  	struct reset_control *rstc, *pos;
> > +	LIST_HEAD(reset_control_head);
> >  
> >  	scoped_guard(mutex, &reset_list_mutex)
> >  		list_del(&rcdev->list);
> > @@ -195,12 +196,22 @@ void reset_controller_unregister(struct reset_controller_dev *rcdev)
> >  		list_for_each_entry_safe(rstc, pos, &rcdev->reset_control_head, list) {
> >  			kref_get(&rstc->refcnt);
> >  			rcu_assign_pointer(rstc->rcdev, NULL);
> > -			synchronize_srcu(&rstc->srcu);
> 
> Now reset_controller_remove() can be called while a concurrent reset
> operation that got an rcdev reference just before the
> rcu_assign_pointer() is about to enter a reset_control_ops callback.

Ack, fixed in v2[2].

[2] https://lore.kernel.org/all/20261104162038.48622-3-tzungbi@xxxxxxxxxx

> 
> >  			reset_controller_remove(rcdev, rstc);
> 
> I think this should only do the list_del(&rstc->list); ...
> 
> > -			reset_control_put_internal(rstc);
> > +			list_add(&rstc->list, &reset_control_head);
> 
> ... which could be combined with this into list_move(). That would also
> make clearer what happens here (move into a temporary list that doesn't
> need to be rcdev->lock'ed).

Ack, fixed in v2.

> 
> The module_put() and put_device() calls should be deferred into the
> loop below, after synchronize_srcu(). We don't make it possible to
> unload the reset controller driver module until then.

Ack, drop the lockdep_assert_held() and simply call
reset_controller_remove() in v2.

> > +	/*
> > +	 * Run synchronize_srcu() outside rcdev->lock to avoid a circular
> > +	 * locking dependency with paths that acquire rcdev->lock inside
> > +	 * the rstc->srcu read-side critical section.
> > +	 */
> > +	list_for_each_entry_safe(rstc, pos, &reset_control_head, list) {
> > +		list_del(&rstc->list);
> > +		synchronize_srcu(&rstc->srcu);
> > +		reset_control_put_internal(rstc);
> 
> This shouldn't call reset_control_put_internal(), which is supposed to
> be the common internal implementation of the consumer
> reset_control_put() API variants.
> 
> Rather, I'd prefer this to call ref = kref_put() directly and then put
> the "if (ret) { ... }" part of reset_control_put_internal() into a
> separate helper function that can be called from here as well.

Ack, the call of reset_control_put_internal() is actually from [PATCH v1],
fixed in [PATCH v2 1/2][3].

[3] https://lore.kernel.org/all/20261104162038.48622-2-tzungbi@xxxxxxxxxx