Re: [PATCH v3 12/13] drm/panfrost: Fix races between perfcnt and reset sequence

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [Critical] Adding a field to `struct drm_panfrost_perfcnt_dump` breaks UAPI ABI compatibility.
- [High] Ignored return value of `panfrost_perfcnt_hw_enable()` causes MMU Address Space refcount imbalance and hardware enablement failure.
- [High] Missing `reinit_completion()` inside the retry loop causes premature success returns with incomplete data.
- [High] The dump retry logic uses an unsigned integer for a `>= 0` check, creating an infinite loop.
- [Medium] Early return in `panfrost_perfcnt_reset()` prevents `panfrost_perfcnt_enable_locked()` from surviving a concurrent reset.
- [Medium] A TOCTOU race in `panfrost_perfcnt_dump_locked()` can conceal hardware reset events from userspace.
- [Medium] `synchronize_irq()` is called before the GPU interrupt source is masked.
--

--- Patch [12]: [PATCH v3 12/13] drm/panfrost: Fix races between perfcnt and reset sequence ---
Note: The format of this report has been altered due to recitation
restrictions. Instead of providing inline diff quotes, this report provides a
free-form summary of the findings.

commit 636e7ce6a7687c01b696bf2f1a79d4f8987d7340
Author: Adrián Larumbe <adrian.larumbe@collabora.com>
Subject: drm/panfrost: Fix races between perfcnt and reset sequence

This commit introduces a reset lock to govern hardware access during
performance counter operations. It also expands the DRM uAPI for the 
perfcnt DUMP operation to notify userspace of hardware resets, and ensures
performance counter configuration is restored after a reset completes.

[Severity: Critical]
Adding the hw_reset field to struct drm_panfrost_perfcnt_dump changes its
size, and the ioctl macro is updated from DRM_IOW to DRM_IOWR. Does this
alter the generated ioctl number for DRM_IOCTL_PANFROST_PERFCNT_DUMP and
break backwards compatibility for existing userspace binaries?

[Severity: High]
In panfrost_perfcnt_enable_locked(), the return value of
panfrost_perfcnt_hw_enable() is ignored. Since panfrost_perfcnt_hw_enable()
can return an error if panfrost_mmu_as_get() fails to find a free Address
Space slot, will ignoring this error cause the subsequent teardown in
panfrost_perfcnt_disable_locked() to unconditionally call
panfrost_mmu_as_put() and trigger a refcount underflow?

[Severity: High]
In panfrost_perfcnt_dump_locked(), reinit_completion() is called outside the
dump_retry loop. If a hardware reset occurs just after the wait times out,
the reset handler calls complete(). Won't the next wait attempt inside the
retry loop see a positive done count and return immediately without actually
waiting for the new sample?

[Severity: High]
Also in panfrost_perfcnt_dump_locked(), the loop counter retries is declared
as a u64. Since it is an unsigned integer, won't the retry check
(--retries >= 0) wrap around to U64_MAX and always evaluate to true,
creating an infinite loop if the GPU undergoes continuous resets?

[Severity: Medium]
In panfrost_perfcnt_reset(), there is an early return if !perfcnt->user.
However, panfrost_perfcnt_enable_locked() expects reset_happened to be
updated to handle timeouts gracefully. Since perfcnt->user is not assigned
until after the wait completes in the enable function, won't a concurrent
reset hit this early return and fail to update reset_happened, causing an
unintended -ETIMEDOUT error?

[Severity: Medium]
In panfrost_perfcnt_dump_locked(), the *reset_happened flag exported to
userspace is only updated on the very first iteration of the retry loop. If a
reset is detected by the atomic_cmpxchg check and the loop retries, will
userspace remain unaware of the reset if the second dump attempt succeeds?

[Severity: Medium]
In panfrost_device_reset(), panfrost_perfcnt_reset() is called, which then
calls synchronize_irq(pfdev->gpu_irq). However, the GPU interrupts are not
masked until panfrost_gpu_soft_reset() is executed on the following line.
Can the GPU raise a new interrupt immediately after synchronize_irq() returns
and before the mask is applied?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260724-claude-fixes-v3-0-542af61abd0f@collabora.com?part=12