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

[Boris Brezillon <boris.brezillon@collabora.com>]

On Thu, 14 May 2026 14:16:37 +0100
Steven Price <steven.price@arm.com> 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.

I can try to split that up, but even after the split, it will still be
a pain to review.

> And it's a little dubious that the mechanical
> refactoring produces more readable code in some cases.

I agree, though the mix of guard()s and manual locks makes things even
harder to reason about, especially when they appear in the same
function/block. The very reason I ended up sending this series is
because, as part of the IRQ refactor, I decided to be a good citizen
and use guards when I could, and I realized how bad the partial
transition was in term of ergonomics: not only you have to think about
whether the function/block scope is what you want (that's basically
what guard provides, unless you used explicit scoped_guard()), but you
also have to think about the interactions with your other manual locks.

TLDR; I'd rather switch over to guards entirely, or go back to manual
locks, but the mix we have right now is far from ideal.

> 
> That said I asked my friendly AI bot...
> 
> [...]
> 
> > @@ -3142,48 +3126,44 @@ panthor_mmu_reclaim_priv_bos(struct panthor_device *ptdev,
> >  	LIST_HEAD(remaining_vms);
> >  	LIST_HEAD(vms);
> >  
> > -	mutex_lock(&ptdev->reclaim.lock);
> > -	list_splice_init(&ptdev->reclaim.vms, &vms);
> > +	scoped_guard(mutex, &ptdev->reclaim.lock)
> > +		list_splice_init(&ptdev->reclaim.vms, &vms);
> >  
> >  	while (freed < nr_to_scan) {
> >  		struct panthor_vm *vm;
> >  
> > -		vm = list_first_entry_or_null(&vms, typeof(*vm),
> > -					      reclaim.lru_node);
> > -		if (!vm)
> > -			break;
> > -
> > -		if (!kref_get_unless_zero(&vm->base.kref)) {
> > -			list_del_init(&vm->reclaim.lru_node);
> > -			continue;
> > +		scoped_guard(mutex, &ptdev->reclaim.lock) {
> > +			vm = list_first_entry_or_null(&vms, typeof(*vm),
> > +						      reclaim.lru_node);
> > +			if (vm && !kref_get_unless_zero(&vm->base.kref)) {
> > +				list_del_init(&vm->reclaim.lru_node);
> > +				vm = NULL;
> > +			}
> >  		}
> >  
> > -		mutex_unlock(&ptdev->reclaim.lock);
> > +		if (!vm)
> > +			break;  
> 
> ... and it said the above has changed behaviour.
> 
> In the !kref_get_unless_zero() case you now assign vm = NULL which then
> leads to the 'break' case above. Previously we 'continue'd.

Oops, that one wasn't intended, indeed.