Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [High] Clocks and reset controllers are leaked on driver unbind because `pfdev->js` is never set to NULL during teardown. Pre-existing issues: - [High] The perfcnt ioctls (`panfrost_ioctl_perfcnt_enable` and `panfrost_ioctl_perfcnt_dump`) access hardware registers directly without holding the newly introduced `pfdev->reset.lock`, which can race with a concurrent GPU reset. -- --- Patch [11]: [PATCH v3 11/13] drm/panfrost: Introduce a reset lock --- commit 17d0aa882609f1a77cf16c55ac27f808cf38cb0a Author: Adrián Larumbe <[email protected]> drm/panfrost: Introduce a reset lock This commit introduces a read/write semaphore to envelop the GPU reset sequence. This lock is used to prevent driver entry points from accessing hardware registers while a reset is ongoing. > So as to avoid accessing HW register while a reset is ongoing, a > read/write > semaphore that envelopes the reset sequence will help driver entry points > avoid racing with it. For now, the only such racy entry point is the ioctl > that returns the current GPU timestmap. [Severity: High] This is a pre-existing issue, but are the perfcnt ioctls (panfrost_ioctl_perfcnt_enable and panfrost_ioctl_perfcnt_dump in drivers/gpu/drm/panfrost/panfrost_perfcnt.c) also racy? Both ioctls call functions that write directly to GPU_CMD and other HW registers. If a userspace thread calls DRM_IOCTL_PANFROST_PERFCNT_DUMP while the GPU is undergoing a soft reset (e.g., triggered by a job timeout handled asynchronously by panfrost_reset_work), could this corrupt the reset state machine and lead to undefined hardware behavior? Should these ioctls also be wrapped with pfdev->reset.lock? > diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c > b/drivers/gpu/drm/panfrost/panfrost_job.c > index c2a1670a74e86..c1d0b111dde1a 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_job.c > +++ b/drivers/gpu/drm/panfrost/panfrost_job.c > @@ -895,7 +895,9 @@ int panfrost_jm_init(struct panfrost_device *pfdev) > if (!pfdev->reset.wq) > return -ENOMEM; > args.timeout_wq = pfdev->reset.wq; > + > init_waitqueue_head(&pfdev->reset.wait); > + init_rwsem(&pfdev->reset.lock); > > for (j = 0; j < NUM_JOB_SLOTS; j++) { > js->queue[j].fence_context = dma_fence_context_alloc(1); [Severity: High] Does the job manager teardown leak clocks and reset controllers? This regression was introduced earlier in this patch series by commit "drm/panfrost: Move all device power up and down into RPM callbacks". When unbinding the device, panfrost_remove() calls panfrost_device_fini(). panfrost_jm_fini() in drivers/gpu/drm/panfrost/panfrost_job.c destroys the job manager but does not set pfdev->js to NULL. Later, panfrost_try_suspend_device() invokes panfrost_device_runtime_suspend() in drivers/gpu/drm/panfrost/panfrost_device.c. Because pfdev->js is still non-NULL, panfrost_device_is_initialized() incorrectly evaluates to true. This causes the cleanup block in panfrost_device_runtime_suspend() to be skipped, and panfrost_clk_fini() and reset_control_assert() are never executed. Should panfrost_jm_fini() set pfdev->js to NULL after cleaning up the job slots? -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=11
