From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: Re: [PATCH v2 5/7] drm/panfrost: Make reset sequence deal with an active HWPerf session
Date: Fri, 05 Jun 2026 06:16:24 +1000
Message-ID: <review-patch5-20260604174918.2BC551F00893@smtp.kernel.org>
In-Reply-To: <20260604174918.2BC551F00893@smtp.kernel.org>
References: <20260604-claude-fixes-v2-0-57c6bd4c1655@collabora.com> <20260604174918.2BC551F00893@smtp.kernel.org>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0

Patch Review

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

**Bug =E2=80=94 missing negative sign on EAGAIN:**

```c
if (atomic_cmpxchg(&perfcnt->hw_reset_happened, 1, 0)) {
    ret =3D 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 =3D -EAGAIN;
    goto err_vunmap;
}
```

This check races with the reset path =E2=80=94 `reset.pending` could be set=
 immediately after this read. The comment acknowledges this and there's a s=
econd check later via `hw_reset_happened`, but the second check has the sam=
e TOCTOU issue. Between `panfrost_mmu_as_get()` and the `atomic_cmpxchg` of=
 `hw_reset_happened`, a reset could happen that sets `hw_reset_happened=3D1=
`, and the cmpxchg will catch that. So the design intent is sound, except f=
or 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, `pan=
frost_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_disa=
ble` 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 befor=
e `err_vunmap`, and `panfrost_mmu_as_get` was called *before* the block wit=
h the `atomic_cmpxchg`. So after `err_disable`, we skip `panfrost_mmu_as_pu=
t`. 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 en=
able/dump is happening concurrently, the GPU register writes could interlea=
ve. The comment explains the reasoning (deadlock avoidance due to GFP_KERNE=
L allocations under lock), which is fair, but it means the GPU register wri=
tes 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 `panfros=
t_gpu_soft_reset`), but it should be documented.

**`panfrost_perfcnt_disable_locked` =E2=80=94 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 cl=
eared 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-enab=
les? The flag stays set forever, which is fine since the struct outlives fi=
le close.

But what about `panfrost_perfcnt_close()`? It calls `panfrost_perfcnt_disab=
le_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