Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider: - [High] Infinite loop in `panfrost_perfcnt_dump_locked` due to unsigned integer underflow. - [High] Race condition bypassing wait for completion in `panfrost_perfcnt_dump_locked`. - [High] Dead code in reset recovery during performance counter enablement leads to spurious timeouts. - [High] Ignored return value of `panfrost_perfcnt_hw_enable()` leads to address space reference count underflow. - [Medium] Stale `reset_happened` flag masks timeout errors in subsequent performance counter sessions. - [Medium] Race condition in `panfrost_perfcnt_dump_locked()` fails to notify userspace of hardware resets. --
--- Patch [13]: [PATCH v3 13/13] drm/panfrost: Bump driver minor to reflect new DUMP IOCTL req field --- commit a7210878c461c0a2b1d591ac0e3e2de27fb4740f Author: Adrián Larumbe <[email protected]> drm/panfrost: Bump driver minor to reflect new DUMP IOCTL req field Expose expanded PERFCNT_DUMP IOCTL interface to user mode. > diff --git a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c > b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c > index 7020c0192e189..a6f1b0b961fcd 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c > +++ b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c [ ... ] > -static int panfrost_perfcnt_dump_locked(struct panfrost_device *pfdev) > +static int panfrost_perfcnt_dump_locked(struct panfrost_device *pfdev, > + u64 *reset_happened) > { > + struct panfrost_perfcnt *perfcnt = pfdev->perfcnt; > + u64 retries = PERFCNT_DUMP_MAX_RETRIES; > + bool previous_reset; > u64 gpuva; > int ret; > > reinit_completion(&pfdev->perfcnt->dump_comp); > gpuva = pfdev->perfcnt->mapping->mmnode.start << PAGE_SHIFT; > + previous_reset = atomic_read(&perfcnt->reset_happened); > + > +dump_retry: [Severity: High] Does this sequence have a race condition if a GPU reset occurs during the dump? The reinit_completion() is called outside the dump_retry loop. If a hardware sample completion IRQ fires (waking the wait and consuming the completion so done=0), and a GPU reset occurs immediately after but before atomic_cmpxchg(), the reset handler calls complete(&dump_comp) (done=1). The code then jumps to dump_retry because of the reset. Will the second wait_for_completion immediately consume the pending completion (done=1) and return without actually waiting for the new sample commands to finish? > + scoped_guard(rwsem_read, &pfdev->reset.lock) { > + gpu_write(pfdev, GPU_PERFCNT_BASE_LO, lower_32_bits(gpuva)); [ ... ] > + } > + > ret = > wait_for_completion_interruptible_timeout(&pfdev->perfcnt->dump_comp, > msecs_to_jiffies(1000)); > + if (retries == PERFCNT_DUMP_MAX_RETRIES) > + *reset_happened = previous_reset || > atomic_read(&perfcnt->reset_happened); > + > + if (atomic_cmpxchg(&perfcnt->reset_happened, 1, 0)) { [Severity: Medium] Can this miss notifying userspace of a hardware reset? If a GPU reset occurs precisely between the atomic_read() and atomic_cmpxchg(&perfcnt->reset_happened, 1, 0), *reset_happened is assigned 0. The atomic_cmpxchg() then succeeds, decrements retries, and loops to dump_retry. On the subsequent successful iteration, the condition (retries == PERFCNT_DUMP_MAX_RETRIES) is false, so *reset_happened is never updated to true. Will this break userspace counter accumulation logic by causing it to aggregate post-reset small counter values as if they were continuous? > + if (--retries >= 0) > + goto dump_retry; [Severity: High] Is it possible for this to enter an infinite loop? The retries variable is declared as a u64. When it reaches 0, --retries will underflow to UINT64_MAX, making the condition >= 0 always evaluate to true. If resets continuously occur, could this bypass the PERFCNT_DUMP_MAX_RETRIES limit and cause a soft lockup? > + ret = -EBUSY; > + } else { [ ... ] > @@ -117,57 +191,37 @@ static int panfrost_perfcnt_enable_locked(struct > panfrost_device *pfdev, [ ... ] > + ret = wait_for_completion_timeout(&pfdev->perfcnt->dump_comp, > msecs_to_jiffies(1000)); > - if (!ret) { > + if (!ret && !atomic_read(&perfcnt->reset_happened)) { > ret = -ETIMEDOUT; > goto err_vunmap; > } [Severity: High] Is this reset recovery path dead code? In panfrost_perfcnt_enable_locked(), the code waits for cache cleaning and expects perfcnt->reset_happened to be set if a reset occurs during the wait. However, panfrost_perfcnt_reset() checks if (!perfcnt->user) and returns early. Because perfcnt->user is only assigned after the wait completes, any reset during the wait causes the reset handler to return without setting reset_happened. Does this mean the wait will just time out and return -ETIMEDOUT instead of recovering? [ ... ] > - gpu_write(pfdev, GPU_PERFCNT_CFG, cfg); > - > - if (panfrost_has_hw_issue(pfdev, HW_ISSUE_8186)) > - gpu_write(pfdev, GPU_PRFCNT_TILER_EN, 0xffffffff); > + scoped_guard(rwsem_read, &pfdev->reset.lock) { > + perfcnt->counterset = counterset; > + perfcnt->user = user; > + atomic_set(&perfcnt->reset_happened, 0); > + panfrost_perfcnt_hw_enable(pfdev); > + } [Severity: High] Is it safe to ignore the return value of panfrost_perfcnt_hw_enable()? If panfrost_mmu_as_get() fails inside the helper, the hardware is not enabled and the AS refcount is not incremented. However, perfcnt->user is still set and the ioctl returns success. Upon closing the session, panfrost_perfcnt_disable_locked() unconditionally calls panfrost_mmu_as_put(). Will this cause an MMU address space reference count underflow? [ ... ] > @@ -193,18 +247,15 @@ static int panfrost_perfcnt_disable_locked(struct > panfrost_device *pfdev, > if (user != perfcnt->user) > return -EINVAL; > > - gpu_write(pfdev, GPU_PRFCNT_JM_EN, 0x0); [ ... ] > + scoped_guard(rwsem_read, &pfdev->reset.lock) { > + panfrost_mmu_as_put(pfdev, perfcnt->mapping->mmu); > + panfrost_perfcnt_gpu_disable(pfdev); > + perfcnt->user = NULL; > + } [Severity: Medium] Does tearing down the session here without clearing the reset_happened atomic flag cause issues for future sessions? If a GPU reset occurs during a performance counter session, the flag is set to 1. If the session is then closed without calling PERFCNT_DUMP (which normally clears it), the flag remains 1. When a new session is started via panfrost_perfcnt_enable_locked(), will the stale flag cause the wait timeout condition !ret && !atomic_read(&perfcnt->reset_happened) to evaluate to false, masking any legitimate timeouts? -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=13
