Re: [PATCH v9 14/16] 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:

Pre-existing issues:
- [High] Unprotected hardware register access in panfrost_gpu_get_latest_flush_id() risks data corruption and bus errors during concurrent GPU resets.
- [High] Power leak in panfrost_ioctl_query_timestamp() due to incorrect PM runtime put API.
--

commit 8d0ec71108e67c1469d4f0da23df91f3320d12ae
Author: Adrián Larumbe <adrian.larumbe@collabora.com>

drm/panfrost: Introduce a reset lock

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.

> 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 does panfrost_ioctl_submit() in
drivers/gpu/drm/panfrost/panfrost_drv.c also need to hold pfdev->reset.lock?

It calls panfrost_gpu_get_latest_flush_id() in
drivers/gpu/drm/panfrost/panfrost_gpu.c, which reads the
GPU_LATEST_FLUSH_ID hardware register directly:

panfrost_ioctl_submit()
  job->flush_id = panfrost_gpu_get_latest_flush_id(pfdev);
    flush_id = gpu_read(pfdev, GPU_LATEST_FLUSH_ID);

If a user calls panfrost_ioctl_submit() concurrently with a job timeout
triggering panfrost_reset(), reading the hardware register during a GPU soft
reset could return garbage data or trigger a bus error. A corrupted flush ID
could cause the GPU to skip necessary cache flushes.

[ ... ]

> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> index f77780c72a1ae..571a26b841269 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> @@ -41,9 +41,12 @@ static int panfrost_ioctl_query_timestamp(struct panfrost_device *pfdev,
>  	if (ret)
>  		return ret;
>  
> -	panfrost_cycle_counter_get(pfdev);
> -	*arg = panfrost_timestamp_read(pfdev);
> -	panfrost_cycle_counter_put(pfdev);
> +	/* We should not read timestamp register while the GPU  is being reset */
> +	scoped_guard(rwsem_read, &pfdev->reset.lock) {
> +		panfrost_cycle_counter_get(pfdev);
> +		*arg = panfrost_timestamp_read(pfdev);
> +		panfrost_cycle_counter_put(pfdev);
> +	}
>  
>  	pm_runtime_put(pfdev->base.dev);

[Severity: High]
This isn't a bug introduced by this patch, but does calling pm_runtime_put()
in panfrost_ioctl_query_timestamp() cause the GPU power to remain on
indefinitely?

Since the driver uses autosuspend (dev->power.use_autosuspend is true), the PM
core's rpm_idle returns -EAGAIN and fails to schedule the suspend timer when
pm_runtime_put() is called instead of pm_runtime_put_autosuspend(). Any
unprivileged user calling the query IOCTL could trigger this.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260912-claude-fixes-v9-0-e588feaa61ef@collabora.com?part=14