Re: [PATCH 1/6] drm/panthor: Driver-wide xxx_[un]lock -> [scoped_]guard replacement

[Boris Brezillon]

On Wed, 20 May 2026 16:26:42 +0100
Steven Price <steven.price@xxxxxxx> wrote:

> On 18/05/2026 09:43, Boris Brezillon wrote:
> > On Thu, 14 May 2026 10:09:10 -0700
> > Chia-I Wu <olvaffe@xxxxxxxxx> wrote:
> >   
> >> On Thu, May 14, 2026 at 6:24 AM Steven Price <steven.price@xxxxxxx> wrote:  
> >>>
> >>> On 13/05/2026 17:58, Boris Brezillon wrote:    
> >>>> Right now panthor is mixed bag of manual locks and guards. Let's
> >>>> make that more consitent and thus encourage new submissions to go
> >>>> for guards.    
> >>>
> >>> I'm fine with encouraging guards for future code - but I'm a little wary
> >>> of a big change like this - it's hard to review it and check that
> >>> everything works the same. And it's a little dubious that the mechanical
> >>> refactoring produces more readable code in some cases.    
> >> I agree with Steven in general, although I am in favor of landing now
> >> that you've gone through the trouble.  
> > 
> > Honestly, I agree with you. The only reason I went for it is
> > because the mix we have right now is pretty confusing. This has to do
> > with the fact the scopes are often loosely defined unless you used
> > scoped_guard(), so it's pretty easy to mess up the lock/unlock
> > ordering. For instance,
> > 
> > 	mutex_lock(locka);
> > 	guard(lockb);
> > 	mutex_unlock(locka);
> > 
> > 	...
> > 
> > once expanded, turns into inconsistent locked sections, where the inner
> > lock (lockb) is released after the outer one (locka).  
> 
> I think that's a good argument for getting all the guard forms available
> before tackling the conversion.

Yep, makes sense. The reason I didn't go for that in v1 is because I
wasn't sure how well the new guard definition would be received. Now
that we know there's a general consensus to define those, I'll re-order
the patches accordingly.

> Mostly I feel like it would be benefit
> from being split up into multiple patches (maybe one per file?) so that
> there are smaller units to review.

Sure, I can do that.

> 
> >>
> >> I also have mixed feelings about some of the non-scoped guards. Their
> >> scopes are extended slightly than before, supposedly to avoid adding
> >> another level of indentation. But other than slightly slower,  
> > 
> > I tried to used scoped_guard()s every where the extra non-guarded
> > section could be CPU heavy (the only bits left are some very simple
> > bit/arithmetic ops, and a couple queue_work() IIRC).
> >   
> >> it also
> >> becomes less clear what exactly do the guards protect.  
> > 
> > I know, and I have pretty much the same feeling, but we've crossed that
> > bridge when we started accepting non-scoped guard()s, unfortunately.  
> 
> The problem with scoped guards is the extra level of indentation.

Yep.

> Personally I find a mixture of all three is appropriate depending on the
> case.
> 
> E.g.
> 
> int small_simple_function() {
> 	if (simple_condition)
> 		return early;
> 
> 	guard(lock);
> 
> 	if (condition_that_needs_lock)
> 		return early;
> 	/* more work */
> 	return late;
> }
> 
> Here it's easy to reason because the lock is just held for the duration
> of the function after the initial early-out condition is checked.
> 
> int short_lock() {
> 	/* bunch of work */
> 
> 	scoped_guard(lock) {
> 		tmp = read_value();
> 		if (tmp == 42)
> 			return -ESOLONGANDTHANKSFORALLTHEFISH;
> 		tmp++;
> 		write_value(tmp);
> 	}
> 
> 	/* more work */
> }
> 
> Here there's a small section of code which is working on the lock, so it
> makes sense to indent it to show the boundaries of it. The other nice
> thing is that the error return handles the locks for us.
> 
> int old_fashioned() {
> 	if (lock_required)
> 		mutex_lock(lock);
> 
> 	/* some work */
> 
> 	if (lock_required)
> 		mutex_unlock(lock);
> }
> 
> Generally a pattern to be avoided if possible,

Yeah, honestly I try my best to never end up with that sort of
conditional locks.

> but IMHO this is much
> better than the equivalent of:
> 
> int dodgy_function() {
> 	/* some work */
> }
> 
> int outer_function() {
> 	if (lock_required) {
> 		scoped_guard(lock)
> 			dodgy_function();
> 	} else {
> 		dodgy_function();
> 	}
> }

If I were to choose, I'd probably go for this version, but luckily we
don't seem to have this conditional-locking pattern in panthor.