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

["Alvin Šipraga" <alvin.sipraga@analog.com>]

On Thu, Oct 22, 2026 at 03:00:06PM +0200, Philipp Zabel wrote:
> 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.

Good points. Yeah, you're right, it's mostly 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.

Alright, will refrain from doing that then.

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

I'm talking about the next step, after auditing all drivers and setting
can_sleep = true where necessary.

As I pointed out in my reply to v2 [1], the shared reset race exists
regardless of whether the underlying reset controller can sleep or
not. Shengjiu's fix only applies to controllers which can sleep. The fix
can be extended to controllers that can't sleep too, but only if
shared_lock is a spinlock - since, in general, the API permits calls in
atomic context. (Those atomic callers obviously rely implicitly on the
non-blocking nature of the backing controller, but that's a bit besides
the point).

So the idea I outlined in [1] is for this shared_lock to be either a
mutex or a spinlock depending on the controller's can_sleep. The logic
of this patch staying the same.

Maybe I misunderstood you here:

> > > We could also combine this with the exclusive reset control's acquire
> > > "lock" if we move that inside the SRCU section.

I understood this to mean "collapse lock and shared_lock into a single
lock". And I was just saying that - with the above in mind - if
shared_lock might be either a mutex or a spinlock, then it doesn't work
to collapse them, because...

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

of this.

Anyway, I will prepare some patches and you can take a look. I might
have overlooked something but I don't want to waste any more of your
time until we have some code to talk about. :)

[1] https://lore.kernel.org/lkml/atcf1PFVcn5pdsVO@analog.com/

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

Agree.

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

Yes, I noticed the same.

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

I'll send a patch.

Kind regards,
Alvin