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 <[email protected]> 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/[email protected]?part=12
