Re: [PATCH v8 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] Scheduler teardown races with pending reset work in panfrost_jm_fini().
- [High] panfrost_reset() drops PM references prematurely, allowing concurrent autosuspend to power off the GPU before reset.
--

commit 41ac0c5362c3dc2a1e03fb9d2659c68f74cd414d
Author: Adrián Larumbe <adrian.larumbe@collabora.com>

drm/panfrost: Introduce a reset lock

This commit adds a read/write semaphore to the panfrost device reset structure.
It protects hardware register access during an ongoing reset sequence,
specifically preventing the timestamp query ioctl from racing with the reset
process.

> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
> index f171abe26f80d..e90efcff5ce7e 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_device.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_device.c
> @@ -466,6 +466,8 @@ bool panfrost_exception_needs_reset(const struct panfrost_device *pfdev,
>  
>  void panfrost_device_reset(struct panfrost_device *pfdev, bool enable_job_int)
>  {
> +	guard(rwsem_write)(&pfdev->reset.lock);
> +
>  	panfrost_gpu_soft_reset(pfdev);
>  
>  	panfrost_gpu_power_on(pfdev);

[Severity: High]
This isn't a bug introduced by this patch, but looking at how
panfrost_device_reset() is called from panfrost_reset() in
drivers/gpu/drm/panfrost/panfrost_job.c, is there a race with GPU
autosuspend?

In panfrost_reset(), the PM references for stuck jobs are dropped right
before calling panfrost_device_reset():

	pm_runtime_put_noidle(pfdev->base.dev);
	panfrost_devfreq_record_idle(&pfdev->pfdevfreq);
	...
	/* Proceed with reset now. */
	panfrost_device_reset(pfdev, false);

If an autosuspend timer expires in this small window, could
panfrost_device_runtime_suspend() execute and power off the GPU? If so,
the subsequent register accesses inside panfrost_device_reset() (like writing
to GPU_INT_MASK in panfrost_gpu_soft_reset()) might hit powered-off hardware,
potentially causing a bus fault.

[ ... ]
> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
> index 00fb36c577df7..c2b1f1087e21e 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_job.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_job.c
> @@ -877,6 +877,7 @@ int panfrost_jm_init(struct panfrost_device *pfdev)
>  	INIT_WORK(&pfdev->reset.work, panfrost_reset_work);
>  	spin_lock_init(&js->job_lock);
>  	mutex_init(&pfdev->sched_lock);
> +	init_rwsem(&pfdev->reset.lock);
>  
>  	js->irq = platform_get_irq_byname(to_platform_device(pfdev->base.dev), "job");
>  	if (js->irq < 0)

[Severity: High]
This is a pre-existing issue, but while we are setting up the reset worker
and its locks here, does the teardown order in panfrost_jm_fini() race with
the reset work?

Looking at panfrost_jm_fini() in drivers/gpu/drm/panfrost/panfrost_job.c, it
destroys the schedulers before cancelling the pending reset work:

	for (j = 0; j < NUM_JOB_SLOTS; j++) {
		drm_sched_fini(&js->queue[j].sched);
	}

	cancel_work_sync(&pfdev->reset.work);

If a user triggers a GPU reset via debugfs or a delayed reset work is queued
just as the module is unloaded, could panfrost_reset_work() execute
concurrently? Since panfrost_reset() calls drm_sched_stop() and
drm_sched_start() on the torn-down schedulers, this sequence could lead to
accessing freed scheduler memory.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260907-claude-fixes-v8-0-c2bcb5e82184@collabora.com?part=14