Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] Perfmon hardware counters are permanently lost if a capture (via ioctl or stop) races with runtime PM autosuspend. - [Medium] A global perfmon is incorrectly stopped and silently disabled if userspace sets it to the same perfmon object that is already attached to a pending job. --
commit 8ce781eec267bf896e263ac16cfc79142b7af5ee Author: Maíra Canal <[email protected]> Subject: drm/v3d: Refactor perfmon locking This commit refactors the performance monitor locking in the v3d driver to serialize access to the device-wide active performance monitor. It introduces a new device-wide spinlock and centralizes state transitions to protect the hardware counters and software state from concurrent access. > diff --git a/drivers/gpu/drm/v3d/v3d_perfmon.c > b/drivers/gpu/drm/v3d/v3d_perfmon.c > index 48ae748247be8..3ad0f022753c8 100644 > --- a/drivers/gpu/drm/v3d/v3d_perfmon.c > +++ b/drivers/gpu/drm/v3d/v3d_perfmon.c [ ... ] > @@ -288,6 +300,21 @@ static void v3d_perfmon_capture_locked(struct v3d_dev > *v3d, > v3d_pm_runtime_put(v3d); > } > > +void v3d_perfmon_stop_locked(struct v3d_dev *v3d, struct v3d_perfmon > *perfmon, > + bool capture) > +{ > + lockdep_assert_held(&v3d->perfmon_state.lock); > + > + if (!perfmon || perfmon != v3d->perfmon_state.active) > + return; > + [Severity: Medium] Could a global perfmon be incorrectly stopped and silently disabled if userspace sets it to the same perfmon object already attached to a pending job? If userspace submits a job with perfmon X attached, and before the job starts, calls v3d_perfmon_set_global_ioctl() to set X as the global perfmon, the ioctl succeeds, setting v3d->global_perfmon = X and v3d->perfmon_state.active = X. When the job completes, its IRQ handler calls v3d_perfmon_stop() with X. Because the job's perfmon matches the active perfmon here, v3d_perfmon_stop_locked() clears active to NULL and stops the hardware, breaking the global perfmon while leaving v3d->global_perfmon set. > + v3d->perfmon_state.active = NULL; > + > + /* If the device is suspended, the HW has already stopped counting. */ > + if (!pm_runtime_get_if_active(v3d->drm.dev)) > + return; > + [Severity: Medium] Can the hardware counters be permanently lost if a capture (via ioctl or stop) races with runtime PM autosuspend? If v3d_perfmon_set_global_ioctl() (with the CLEAR flag) executes while the device state is RPM_SUSPENDING, pm_runtime_get_if_active() returns 0. Because v3d->perfmon_state.active is set to NULL just before the check, this function returns early without reading the hardware registers. When the driver's suspend callback subsequently calls v3d_perfmon_suspend(), it sees active == NULL and also skips capturing the counters, resulting in lost counter data. > + v3d_perfmon_hw_stop(v3d, perfmon, capture); > + v3d_pm_runtime_put(v3d); > +} -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
