Claude review: Re: [PATCH v2 5/7] drm/panfrost: Make reset sequence deal with an active HWPerf session

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

Patch Review

**Verdict: Has a bug. Needs revision.**

**Bug — missing negative sign on EAGAIN:**

```c
if (atomic_cmpxchg(&perfcnt->hw_reset_happened, 1, 0)) {
    ret = EAGAIN;  // <-- BUG: should be -EAGAIN
    goto err_disable;
}
```

This returns a positive `EAGAIN` (11) which will be interpreted as success by the ioctl layer. It must be `-EAGAIN`.

**Race condition concern with `reset.pending` check:**

```c
if (atomic_read(&pfdev->reset.pending)) {
    ret = -EAGAIN;
    goto err_vunmap;
}
```

This check races with the reset path — `reset.pending` could be set immediately after this read. The comment acknowledges this and there's a second check later via `hw_reset_happened`, but the second check has the same TOCTOU issue. Between `panfrost_mmu_as_get()` and the `atomic_cmpxchg` of `hw_reset_happened`, a reset could happen that sets `hw_reset_happened=1`, and the cmpxchg will catch that. So the design intent is sound, except for the missing negation bug above.

**`err_disable` error path is incomplete:**

When hitting the `err_disable` label, the code calls `panfrost_perfcnt_gpu_disable(pfdev)` then falls through to `err_vunmap`. But at this point, `panfrost_mmu_as_get` has already succeeded (line before the `err_disable` goto target was added). The error path doesn't call `panfrost_mmu_as_put()`, so the AS reference will be leaked. The code should either:
1. Add `panfrost_mmu_as_put(pfdev, perfcnt->mapping->mmu)` before `err_disable` falls through to `err_vunmap`, or
2. Have `err_disable` jump to a label after `panfrost_mmu_as_put`.

Wait, looking more carefully: the `err_disable` label is placed right before `err_vunmap`, and `panfrost_mmu_as_get` was called *before* the block with the `atomic_cmpxchg`. So after `err_disable`, we skip `panfrost_mmu_as_put`. This is an AS reference leak.

**`panfrost_perfcnt_reset` called without lock:**

```c
void panfrost_perfcnt_reset(struct panfrost_device *pfdev)
{
    ...
    panfrost_perfcnt_gpu_disable(pfdev);
    atomic_set(&perfcnt->hw_reset_happened, 1);
}
```

This writes GPU registers without holding the perfcnt lock. If a perfcnt enable/dump is happening concurrently, the GPU register writes could interleave. The comment explains the reasoning (deadlock avoidance due to GFP_KERNEL allocations under lock), which is fair, but it means the GPU register writes in `panfrost_perfcnt_gpu_disable` are racing with the enable path's GPU register writes. This is probably tolerable since we're about to do a GPU soft reset anyway (the next call after `panfrost_perfcnt_reset` is `panfrost_gpu_soft_reset`), but it should be documented.

**`panfrost_perfcnt_disable_locked` — skipping `as_put` on reset:**

```c
if (!atomic_read(&perfcnt->hw_reset_happened))
    panfrost_mmu_as_put(pfdev, perfcnt->mapping->mmu);
```

This correctly avoids the WARN when the reset already zeroed the as_count. However, after this, `atomic_read(&perfcnt->hw_reset_happened)` is never cleared back to 0 in the disable path. It's cleared in `perfcnt_enable_locked` via `atomic_cmpxchg`. What if the user disables perfcnt and never re-enables? The flag stays set forever, which is fine since the struct outlives file close.

But what about `panfrost_perfcnt_close()`? It calls `panfrost_perfcnt_disable_locked()` which will skip `as_put` if `hw_reset_happened` is set. Then `panfrost_perfcnt_close` does `pm_runtime_put_autosuspend` at the end. This seems consistent.

---

---
Generated by Claude Code Patch Reviewer