Claude review: drm/panthor: Driver-wide xxx_[un]lock -> [scoped_]guard replacement

[Claude Code Review Bot <claude-review@example.com>]

Patch Review

This is the workhorse patch — 484 insertions and 559 deletions across 8 files. The mechanical conversions are generally correct. A few observations:

**panthor_device.c — `panthor_device_unplug()` restructuring:**
The original code released `unplug.lock` before the slow teardown path, with a comment "We do the rest of the unplug with the unplug lock released." The new version uses `scoped_guard` which correctly releases after the block, achieving the same semantics. The `was_already_unplugged` variable is a clean way to handle this. Good.

**panthor_device.c — `panthor_mmio_vm_fault()` restructuring:**
In the original, the `DRM_PANTHOR_USER_FLUSH_ID_MMIO_OFFSET` case set `pfn` and `pgprot` inside the lock, then called `vmf_insert_pfn_prot()` after the `switch` but still inside the lock. The new version moves `vmf_insert_pfn_prot()` inside the `case` block with a `break`, which is functionally correct but adds indentation. This is fine.

**panthor_gpu.c — `panthor_gpu_flush_caches()` early return inside `scoped_guard`:**
```c
scoped_guard(spinlock_irqsave, &ptdev->gpu->reqs_lock) {
    if (ptdev->gpu->pending_reqs & GPU_IRQ_CLEAN_CACHES_COMPLETED)
        return -EIO;
```
Returning from inside a `scoped_guard` block is valid — the cleanup runs. This is correct.

**panthor_mmu.c — `panthor_vm_idle()` refactoring:**
The original used `refcount_dec_and_mutex_lock()` which is an atomic "decrement and if it reaches zero, return with lock held" operation. The replacement:
```c
if (refcount_dec_not_one(&vm->as.active_cnt))
    return;

guard(mutex)(&ptdev->mmu->as.slots_lock);
if (!refcount_dec_and_test(&vm->as.active_cnt))
    return;
```
This is a correct decomposition. `refcount_dec_not_one()` returns true if the count was >1 after decrementing (i.e., not last ref), so the fast path avoids the lock. Then under the lock, `refcount_dec_and_test()` does the final atomic check. The refcount is decremented exactly once in all paths. Correct, though slightly less efficient than the original `refcount_dec_and_mutex_lock()` in the contended-last-ref case (two atomic ops instead of one). Acceptable tradeoff.

**panthor_mmu.c — `panthor_mmu_reclaim_priv_bos()` restructuring:**
This is the most delicate conversion. The original held `ptdev->reclaim.lock` across the loop iteration, dropping it only around `drm_gem_lru_scan()`. The new version uses multiple `scoped_guard` blocks:
```c
scoped_guard(mutex, &ptdev->reclaim.lock)
    list_splice_init(&ptdev->reclaim.vms, &vms);

while (freed < nr_to_scan) {
    scoped_guard(mutex, &ptdev->reclaim.lock) {
        vm = list_first_entry_or_null(&vms, ...);
        if (vm && !kref_get_unless_zero(&vm->base.kref)) {
            list_del_init(&vm->reclaim.lru_node);
            vm = NULL;
        }
    }
    if (!vm)
        break;
```
There's a behavioral difference here: the original loop would `continue` when `kref_get_unless_zero` failed (trying the next VM in the list), but the new version sets `vm = NULL` and then `break`s out of the while loop. This means if the first VM in the list has a zero refcount, the reclaim loop stops entirely rather than trying subsequent VMs. **This looks like a bug** — it should keep looping. The `list_del_init` removes the dead VM from `vms`, but then `vm = NULL` causes the `if (!vm) break;` to exit the entire while loop instead of continuing.

**panthor_pwr.c — `panthor_pwr_irq_handler()` spinlock change:**
The original used `spin_lock()`/`spin_unlock()` (without irqsave), but the replacement uses `guard(spinlock_irqsave)`. Since this is an IRQ handler (hardirq context), interrupts are already disabled, so `spinlock_irqsave` is unnecessary overhead (saves/restores flags that won't change). The original `spin_lock()` was correct here. While functionally harmless, using `spinlock_irqsave` in a hardirq handler is a style regression — `spinlock` would be sufficient and more efficient.

**panthor_sched.c — `tick()` extraction:**
Clean refactoring to separate the guard-heavy wrapper from the core logic. The `tick()` function now uses `guard(mutex)(&sched->lock)` with early returns, which is clean.

---
Generated by Claude Code Patch Reviewer