Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] drm/panfrost: data race on perfcnt->state during dump error path - [Low] cleanup: mixing goto and scoped_guard in panfrost_perfcnt_enable_locked --
commit f2123ab59275f76a9a44c42db13e01ef9037cc87 Author: Adrián Larumbe <[email protected]> drm/panfrost: Fix races between perfcnt and reset sequence This commit updates panfrost's performance counter session logic to use the reset read/write semaphore when accessing hardware registers. It also expands the perfcnt dump ioctl to notify userspace if a reset occurred, rendering previous counter states invalid. > diff --git a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c > b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c > index b3f71d7fd82a9..a9def452d5ef9 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c > +++ b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c [ ... ] > @@ -58,27 +70,130 @@ void panfrost_perfcnt_sample_done(struct panfrost_device > *pfdev) > complete(&pfdev->perfcnt->dump_comp); > } > > -static int panfrost_perfcnt_dump_locked(struct panfrost_device *pfdev) > +static int panfrost_perfcnt_dump_locked(struct panfrost_device *pfdev, u32 > *state) > { [ ... ] > + scoped_guard(rwsem_read, &pfdev->reset.lock) { > + *state = perfcnt->state; > + if (perfcnt->state & PANFROST_PERFCNT_SESSION_DEAD) > + return -EIO; > + > + perfcnt->state = 0; > + > + reinit_completion(&pfdev->perfcnt->dump_comp); > + > + gpu_write(pfdev, GPU_PERFCNT_BASE_LO, lower_32_bits(gpuva)); > + gpu_write(pfdev, GPU_PERFCNT_BASE_HI, upper_32_bits(gpuva)); > + gpu_write(pfdev, GPU_INT_CLEAR, GPU_IRQ_CLEAN_CACHES_COMPLETED | > + > GPU_IRQ_PERFCNT_SAMPLE_COMPLETED); > + gpu_write(pfdev, GPU_CMD, GPU_CMD_PERFCNT_SAMPLE); > + } > + > + /* > + * Here we release the reset semaphore because perfcnt should not get > in the way > + * of a HW reset. Besides, a legitimate reset might be issued during > the wait. > + */ > ret = > wait_for_completion_interruptible_timeout(&pfdev->perfcnt->dump_comp, > msecs_to_jiffies(1000)); > + > if (!ret) > ret = -ETIMEDOUT; > > - return ret; > + /* We need to restore perfcnt state so the next successful dump can > relay it to UM */ > + if (ret < 0) { > + perfcnt->state |= *state; > + *state = 0; > + return ret; > + } [Severity: Medium] Is there a potential data race on perfcnt->state here? The dump wait function releases pfdev->reset.lock to allow concurrent GPU resets. If the wait is interrupted or times out (ret < 0), it performs a read-modify-write on perfcnt->state without re-acquiring the lock. Concurrently, if a GPU fault occurs, panfrost_device_reset() will run and invoke panfrost_perfcnt_reset(), which modifies the same state: panfrost_perfcnt_reset() perfcnt->state |= PANFROST_PERFCNT_SESSION_INTERRUPTED_BY_RESET; Since the error path in panfrost_perfcnt_dump_locked() modifies perfcnt->state without synchronization against this reset path, the PANFROST_PERFCNT_SESSION_INTERRUPTED_BY_RESET flag might be silently overwritten and lost. Would it be safer to wrap this restoration block inside a scoped_guard to ensure the flag from the reset thread is preserved? [ ... ] > @@ -122,54 +252,30 @@ static int panfrost_perfcnt_enable_locked(struct > panfrost_device *pfdev, > ret = drm_gem_vmap(&bo->base, &map); > if (ret) > goto err_put_mapping; > + > perfcnt->buf = map.vaddr; > + perfcnt->counterset = counterset; > > panfrost_gem_internal_set_label(&bo->base, "Perfcnt sample buffer"); > [ ... ] > + scoped_guard(rwsem_read, &pfdev->reset.lock) { > + /* > + * Clear the counters to start from a fresh state. > + */ > + gpu_write(pfdev, GPU_INT_CLEAR, > GPU_IRQ_PERFCNT_SAMPLE_COMPLETED); > + gpu_write(pfdev, GPU_CMD, GPU_CMD_PERFCNT_CLEAR); > > - gpu_write(pfdev, GPU_PERFCNT_CFG, cfg); > + ret = panfrost_perfcnt_hw_enable(pfdev); > + if (ret) > + goto err_vunmap; [Severity: Low] Does jumping out of a scoped_guard with a goto statement mix cleanup patterns? The kernel cleanup guidelines note that usage of "goto" and cleanup helpers (like scoped_guard) should not be mixed in the same function. Doing so can create confusing ownership semantics and increase the risk of leaks during future modifications. Would it be better to either convert all resources in this function to use scope-based cleanup, or stick to traditional locking (down_read / up_read) to match the existing goto-based error handling? [ ... ] -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=15
