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

[Philipp Zabel]

On Do, 2026-10-22 at 12:17 +0200, Alvin Šipraga wrote:
> 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.

Do you have any specific examples?

Most drivers that need locking do so to protect register read-modify-
writes. Usually a register is shared by multiple reset controls, so a
per-reset-control lock in the core doesn't help.

Many drivers that use regmap already get their locking at the regmap
level.

> On top of that it makes things consistent (all ops called
> under the same locks).

Consistency is nice, but so far it seems to boil down to aesthetics.

To me it still looks like the status lock would be unnecessary
overhead. That being said, the cases where reset_control_status() is
called in a loop are usually exclusive controls.

> Still think it's not worth serializing .status?

Yes.

> 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

Why would it? shared_lock is a mutex, too.

> then that's not OK. And converting rcdev->lock into a spinlock doesn't
> seem feasible.

Correct.

> 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.

release() doesn't have to be serialized. Only a single rstc per
rcdev+id should be possible to be acquired at the same time.

Racing acquire() on the same rstc is API misuse, but also shouldn't
hurt in itself. The concurrent reset ops that are expected to follow
would likely break things though.

> And it doesn't offer any synchronization with other exclusive reset
> controls for the same controller line.

Acquire between two different rstc for the same rcdev+id was serialized
via reset_list_mutex, before we apparently broke it with 8c91302a29bc
("reset: protect struct reset_control with its own mutex").

> 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.

I agree.

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

I think so too, yes.

regards
Philipp
>