On Mon, May 18, 2026 at 1:57 AM Boris Brezillon boris.brezillon@collabora.com wrote:
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.
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 -- linaro-mm-sig@lists.linaro.org To unsubscribe send an email to linaro-mm-sig-leave@lists.linaro.org