Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] Incorrect checking of pm_runtime_get_if_active() return values leads to unbalanced pm_runtime_put() calls and hardware access when runtime PM is disabled. --
commit cf45a6afd7e990783e893c6013389c01aac4b43c Author: Maíra Canal <[email protected]> Subject: drm/v3d: Refactor perfmon locking This commit refactors performance monitor locking in the v3d driver to serialize access to the active perfmon globally. It introduces a device-wide spinlock and routes state transitions through a small set of locked entry points. > 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 [ ... ] > void v3d_perfmon_start(struct v3d_dev *v3d, struct v3d_perfmon *perfmon) > { > + guard(spinlock_irqsave)(&v3d->perfmon_state.lock); > + > + if (!perfmon || v3d->global_perfmon) > + return; > > - if (WARN_ON_ONCE(!perfmon || v3d->active_perfmon)) > - return; > - > - if (!pm_runtime_get_if_active(v3d->drm.dev)) > - return; > + if (!pm_runtime_get_if_active(v3d->drm.dev)) > + return; [Severity: High] Does this code correctly handle the case where runtime PM is disabled? When runtime PM is disabled, pm_runtime_get_if_active() returns a negative error code (like -EINVAL). Since a negative number evaluates to true in C, !pm_runtime_get_if_active() evaluates to false. This causes the early return to be bypassed, allowing the code to proceed with hardware MMIO access when the device might be powered off. It also results in an unbalanced v3d_pm_runtime_put() call at the end of the function. Should this be checking if the return value is <= 0? > + > + v3d_perfmon_hw_start(v3d, perfmon); > + v3d->perfmon_state.active = perfmon; > + > + v3d_pm_runtime_put(v3d); > +} [ ... ] > +static void v3d_perfmon_capture_locked(struct v3d_dev *v3d, > + struct v3d_perfmon *perfmon) > +{ > + lockdep_assert_held(&v3d->perfmon_state.lock); > + > + if (!perfmon || perfmon != v3d->perfmon_state.active) > + return; > > if (!pm_runtime_get_if_active(v3d->drm.dev)) > - goto out_clear; > + return; [Severity: High] Does this face the same issue with negative return values from pm_runtime_get_if_active() as v3d_perfmon_start()? > > - if (capture) > - for (i = 0; i < perfmon->ncounters; i++) > - perfmon->values[i] += V3D_CORE_READ(0, > V3D_PCTR_0_PCTRX(i)); > + v3d_perfmon_hw_capture(v3d, perfmon); > + 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; > + > + 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: High] Will a negative return value here prevent the function from returning early, leading to unexpected hardware access? > > - V3D_CORE_WRITE(0, V3D_V4_PCTR_0_EN, 0); > + v3d_perfmon_hw_stop(v3d, perfmon, capture); > v3d_pm_runtime_put(v3d); > +} [ ... ] > @@ -482,18 +545,36 @@ int v3d_perfmon_set_global_ioctl(struct drm_device > *dev, void *data, [ ... ] > + /* If the device is suspended, v3d_perfmon_resume() will > + * program the HW on the next resume. > + */ > + if (pm_runtime_get_if_active(v3d->drm.dev)) { > + v3d_perfmon_hw_start(v3d, perfmon); > + v3d_pm_runtime_put(v3d); > + } [Severity: High] Is it possible for pm_runtime_get_if_active() to return -EINVAL here? If runtime PM is disabled, the negative return value evaluates to true. This would incorrectly grant hardware access and execute an unbalanced v3d_pm_runtime_put() call. Could this be updated to check if the return value is > 0? > } > > return 0; > } -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
