On Thu, 14 May 2026 14:16:37 +0100
Steven Price <[email protected]> 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.
