Re: [PATCH v2 01/11] drm/panfrost: Power the GPU up for a reset queued from the MMU code

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] drm/panfrost: race with system suspend in
  panfrost_reset_work()
--

commit 4c2188106dfefcf238af31be2bfb9e60d51e0307
Author: Arun Iyengar <aiyengar1990@gmail.com>

drm/panfrost: Power the GPU up for a reset queued from the MMU code

This commit acquires a runtime PM reference in panfrost_reset_work() before
calling panfrost_reset(), and clears reset.pending if
pm_runtime_resume_and_get() fails.

> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
> index 3a46dd61cf87f..0c2fda80c51ef 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_job.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_job.c
> @@ -810,7 +810,21 @@ static void panfrost_reset_work(struct work_struct *work)
>  	struct panfrost_device *pfdev;
>  
>  	pfdev = container_of(work, struct panfrost_device, reset.work);
> +
> +	/* The work can run long after it was queued, when the GPU has gone
> +	 * idle and been runtime suspended. Power it up for the reset: its
> +	 * registers are not accessible while it is suspended. If it cannot
> +	 * be resumed (runtime PM is disabled during system sleep), there is
> +	 * no job on it, and panfrost_device_runtime_resume() resets it when
> +	 * it comes back up.
> +	 */
> +	if (pm_runtime_resume_and_get(pfdev->base.dev)) {
> +		atomic_set(&pfdev->reset.pending, 0);
> +		return;
> +	}
> +
>  	panfrost_reset(pfdev, NULL);
> +	pm_runtime_put_autosuspend(pfdev->base.dev);
>  }

[Severity: Medium]
Can panfrost_reset_work() still race with panfrost_device_suspend() and
access GPU registers while the GPU is powered off?

pfdev->reset.wq is allocated without WQ_FREEZABLE and
panfrost_device_suspend() does not flush or cancel pfdev->reset.work.
Holding a runtime PM usage reference does not block
pm_runtime_force_suspend(), which ignores usage_count and calls
panfrost_device_runtime_suspend() directly:

kworker                               CPU0
panfrost_reset_work()
  pm_runtime_resume_and_get()
                                      panfrost_device_suspend()
                                        pm_runtime_force_suspend()
                                          panfrost_device_runtime_suspend()
                                            panfrost_jm_is_idle() -> true
                                            panfrost_gpu_power_off()
                                        panfrost_clks_disable()
  panfrost_reset()
    job_write(pfdev, JOB_INT_MASK, 0) <-- external abort (GPU powered off)

In addition, if the GPU is RPM_ACTIVE when pm_runtime_force_suspend()
disables runtime PM and usage_count > 1 (for example if
__pm_runtime_resume() incremented usage_count while
panfrost_device_runtime_suspend() was running, or perfcnt is enabled),
pm_runtime_force_suspend() sets needs_force_resume and leaves both
runtime_status and last_status set to RPM_ACTIVE.  rpm_resume() then
returns 1 even with disable_depth > 0, so pm_runtime_resume_and_get()
returns 0 while the GPU is suspended.

Finally, when pm_runtime_resume_and_get() fails, clearing
pfdev->reset.pending is not serialized against panfrost_device_resume().
If the device resumes concurrently and panfrost_job_hw_submit() observes
reset.pending == 1, it places the job in pfdev->jobs without writing
JS_COMMAND_START, and panfrost_reset_work() then clears reset.pending and
returns without resetting or starting that job.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112-panfrost-long-jobs-v2-0-a06830da3d99@gmail.com?part=1