Re: [PATCH v7 07/17] drm/panfrost: Stop all jobs before commencing device teardown

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [Critical] panfrost_stop_jobs() unintentionally commands the hardware to restart pending jobs during device teardown, leading to GPU execution with freed memory and unmapped page tables.
- [High] Jobs aborted during device teardown are never signaled or properly freed, causing a memory leak and permanently hanging userspace waiters.
- [High] Scheduler timeout handler (work_tdr) is not canceled when schedulers are stopped during teardown, allowing a concurrent TDR to resuscitate the device mid-teardown.

Pre-existing issues:
- [High] Concurrent executions of panfrost_reset() from multiple scheduler timeouts corrupt the scheduler pending list.
- [High] Sleeping functions called from DMA fence signalling path during panfrost_reset.
--

commit bbe9e4a95112d42e26f093db1d29071e85c60996
Author: Adrián Larumbe <adrian.larumbe@collabora.com>
Subject: drm/panfrost: Stop all jobs before commencing device teardown

This patch ensures that no jobs are inflight at the time the driver finishes
its structures and synchronizes IRQs. To achieve this, it halts submissions
from existing scheduler entities and forces the hardware to soft-stop all
running jobs during device teardown.

> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
> index 9549ddb7854a3..9e02fb5f73c82 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_device.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_device.c
> @@ -341,6 +341,8 @@ void panfrost_device_fini(struct panfrost_device *pfdev)
>  	pm_runtime_get_sync(pfdev->base.dev);
>  	pm_runtime_disable(pfdev->base.dev);
>  
> +	panfrost_jm_stop_sched_jobs(pfdev);
> +

[Severity: High]
Does this code leak the job structures and GEM references while permanently
hanging userspace waiters?

This adds a call to soft-stop inflight jobs during device teardown without
providing a mechanism to signal their fences. The interrupt handler skips
signaling the fence for soft-stopped jobs (signal_fence = false), expecting
a reset. Stuck jobs are erased from driver state via memset, but panfrost
does not implement the cancel_job callback in drm_sched_backend_ops.

Since they are abandoned on the scheduler's pending_list without being freed
or having their fences signaled, any teardown of the device while jobs are
in flight will trigger this leak and hang.

>  	panfrost_gem_fini(pfdev);
>  	panfrost_perfcnt_fini(pfdev);

[ ... ]

> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
> index 5016d2d53da26..630298b7ea8a5 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_job.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_job.c

[ ... ]

> +void panfrost_jm_stop_sched_jobs(struct panfrost_device *pfdev)
> +{
> +	for (u32 i = 0; i < NUM_JOB_SLOTS; i++)
> +		drm_sched_wqueue_stop(&pfdev->js->queue[i].sched);
> +

[Severity: High]
Does this create a race window where the timeout handler can execute
concurrently mid-teardown?

The stop logic uses drm_sched_wqueue_stop() which pauses submit queues, but
fails to cancel the delayed timeout work (work_tdr). If the timeout expires
before teardown reaches panfrost_jm_fini(), panfrost_job_timedout() executes
concurrently and triggers a GPU reset. The reset sequence unconditionally
invokes drm_sched_start() and re-enables hardware interrupts, forcing the
GPU and schedulers to resume execution while panfrost_device_fini() destroys
underlying memory and structures.

> +	panfrost_stop_jobs(pfdev);
> +}
> +

[Severity: Critical]
Does panfrost_stop_jobs() unintentionally command the hardware to restart
pending jobs during device teardown?

This new teardown function reuses the reset path's soft-stop mechanism
without setting reset.pending. During teardown, the resulting IRQ is handled
by panfrost_jm_handle_irq(). Because this is a teardown and not a GPU reset,
pfdev->reset.pending is 0. 

The handler logic unconditionally sees !atomic_read(&pfdev->reset.pending)
and writes JS_COMMAND_START to launch the next queued job. The GPU hardware
resumes executing jobs while panfrost_device_fini() concurrently frees all
backing structures, leading to GPU execution with freed memory and unmapped
page tables.

> +static void
> +panfrost_reset(struct panfrost_device *pfdev,
> +	       struct drm_sched_job *bad)
> +{
> +	unsigned int i;
> +	bool cookie;
> +
> +	if (!atomic_read(&pfdev->reset.pending))
> +		return;
> +
> +	/* Stop the schedulers.
> +	 *
> +	 * FIXME: We temporarily get out of the dma_fence_signalling section
> +	 * because the cleanup path generate lockdep splats when taking locks
> +	 * to release job resources. We should rework the code to follow this
> +	 * pattern:
> +	 *
> +	 *	try_lock
> +	 *	if (locked)
> +	 *		release
> +	 *	else
> +	 *		schedule_work_to_release_later
> +	 */
> +	for (i = 0; i < NUM_JOB_SLOTS; i++)
> +		drm_sched_stop(&pfdev->js->queue[i].sched, bad);
> +

[Severity: High]
This is a pre-existing issue, but can concurrent executions of
panfrost_reset() from multiple scheduler timeouts corrupt the scheduler
pending list?

Panfrost uses multiple independent DRM schedulers (one per job slot). If
multiple slots timeout simultaneously (e.g. due to a global GPU hang),
multiple work_tdr works fire concurrently. They enter panfrost_job_timedout()
which calls atomic_set(&pfdev->reset.pending, 1) rather than an atomic
exchange.

Both threads bypass the !atomic_read() exclusion check here and concurrently
invoke drm_sched_stop(). Since drm_sched_stop() manipulates
sched->pending_list without internal locks for list_add, the concurrent
modifications corrupt the list.

> +	cookie = dma_fence_begin_signalling();
> +
> +	if (bad)
> +		drm_sched_increase_karma(bad);
> +
> +	panfrost_stop_jobs(pfdev);

[Severity: High]
This isn't a bug introduced by this patch (the code was simply refactored
into a helper), but does this invoke sleeping functions from the DMA fence
signalling path?

dma_fence_begin_signalling() enters a critical section where sleeping is
strictly forbidden. Immediately afterward, panfrost_stop_jobs() is called,
which invokes synchronize_irq() (which can block waiting for IRQ handlers)
and readl_poll_timeout() (which sleeps via usleep_range()).

>  
>  	/* Proceed with reset now. */
>  	panfrost_device_reset(pfdev, false);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260828-claude-fixes-v7-0-72a13b2c125d@collabora.com?part=7