On Mon, May 18, 2026 at 1:57 AM Boris Brezillon
<[email protected]> wrote:
>
> 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.
Splitting up a bit can be helpful. If we did introduce errors
unintentionally, at least we could bisect and there would be less code
to look through.
That said, for changes like these, AI should be very effective at
catching errors.
>
> > 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.
> _______________________________________________
> Linaro-mm-sig mailing list -- [email protected]
> To unsubscribe send an email to [email protected]
