Re: [PATCH v3 11/13] drm/panfrost: Introduce a reset lock

[sashiko-bot@kernel.org]

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 <adrian.larumbe@collabora.com>

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/20260724-claude-fixes-v3-0-542af61abd0f@collabora.com?part=11