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

[Chia-I Wu]

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.

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, it also
becomes less clear what exactly do the guards protect.

>
> 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.
>
> Thanks,
> Steve