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

[Alvin Šipraga]

On Thu, Oct 22, 2026 at 10:43:35AM +0200, Philipp Zabel wrote:
> On Mi, 2026-10-21 at 16:16 +0200, Alvin Šipraga wrote:
> [...]
> > Looking forward a bit to the second phase where we have everything
> > serialized, maybe it will make sense to also lock around the call to the
> > .status driver op too? Not suggesting we do it here (as this is just a
> > bugfix), but in general I like when subsystems offer a guarantee that
> > driver ops can't be called concurrently. Also the status is kind of
> > undefined if you're currently in the process of manipulating the reset
> > line.
> 
> That doesn't change with locking if you can't act on the result of the
> status return value under the lock.
> 
> A concurrent assert/deassert may lose the race against status and
> toggle the reset line right after status unlocks, before it returns.

True, it doesn't make any material difference from a consumer point of
view.

I was mostly thinking about the provider side though. Such serialization
guarantees from the core allow reset controller driver authors to use
fewer locks in their own code if they know that the ops can't get called
concurrently. On top of that it makes things consistent (all ops called
under the same locks).

Still think it's not worth serializing .status? Just want to know if you
expressly don't want it in the patches I will send after Shengjiu is
done.

> 
> > What I had in mind later on was to rename what Shengjiu has called
> > shared_lock to ops_lock instead.
> 
> We could also combine this with the exclusive reset control's acquire
> "lock" if we move that inside the SRCU section.

I had the same thought. But I kept bumping into the fact that the
rcdev->lock mutex is taken in reset_control_acquire() while we are
holding rstc->lock. If rstc->lock becomes a spinlock then that's not OK.
And converting rcdev->lock into a spinlock doesn't seem feasible.

But on closer inspection I noticed a couple of things. Look at the function:

int reset_control_acquire(struct reset_control *rstc)
{
	struct reset_controller_dev *rcdev;
	struct reset_control *rc;

	if (!rstc)
		return 0;

	if (WARN_ON(IS_ERR(rstc)))
		return -EINVAL;

	if (reset_control_is_array(rstc))
		return reset_control_array_acquire(rstc_to_array(rstc));

(A)	guard(mutex)(&rstc->lock);

	if (rstc->acquired)
		return 0;

	guard(srcu)(&rstc->srcu);

	rcdev = srcu_dereference(rstc->rcdev, &rstc->srcu);
	if (!rcdev)
		return -ENODEV;

(B)	scoped_guard(mutex, &rcdev->lock) {
		list_for_each_entry(rc, &rcdev->reset_control_head, list) {
			if (rstc != rc && rstc->id == rc->id) {
				if (rc->acquired)
					return -EBUSY;
			}
		}
	}
(C)
(D)	rstc->acquired = true;

	return 0;
}

1. (A) is the only place where rstc->lock is actually taken. Ostensibly
it should be serializing acquire/release logic, but from an API point of
view it doesn't make much sense. Just like the remark [*] in my last
email (see below), you also shouldn't be trying to acquire the same
exclusive reset control from two places at once. And it doesn't offer
any synchronization with other exclusive reset controls for the same
controller line.

2. The synchronization with other reset controls is done with
rcdev->lock at (B). But actually there is a narrow race here. Two tasks
could try to acquire their own exclusive control of the same
line. Assuming the line is currently released, task 1 could reach (C),
get preempted by task 2, which reaches (D) before task 1 is run
again. Then you end up with two controls having acquired exclusive
control.

The obvious fix for (2) is to put (D) inside the scoped guard block.

But then you achieve everything that rstc->lock is supposed to
achieve. So I think we can get rid of that lock altogether?

> 
> > With the following primitives:
> > 
> >   can_sleep?    CONTROL TYPE       LOCKING PRIMITIVE
> >   --------------------------------------------------
> >   no            shared             spin_lock_irqsave
> >   yes           shared             mutex
> >   no            exclusive          none? see [*]
> >   yes           exclusive          none? see [*]
> 
> Yes.
> 
> > Where all reset control operation functions look like:
> > 
> >   reset_control_op() {
> >     ...
> >   
> >     lock(ops_lock);
> >   
> >     __reset_control_op(); /* inc/dec, ->op() call, etc. here */
> >   
> >     unlock(ops_lock);
> >   }
> 
> It might make sense to split these into _exclusive and _shared
> variants.
> 
> > [*] I'm not sure if it makes sense to add any locking here? I can only
> > see it as API misuse if somebody is manipulating an exclusive reset
> > control from two places at once.
> 
> Right.
> 

Kind regards,
Alvin