Re: [PATCH v3 01/14] 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 2 potential
issues to consider:
- [Medium] drm/panfrost: unclocked MMIO access in
  panfrost_device_resume() on error
- [Low] drm/panfrost: race when clearing reset.pending in
  panfrost_reset_work()
--

commit 7ec017c5585451d1f5d87f4425f33e0c841194f8
Author: Arun Iyengar <aiyengar1990@gmail.com>

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

This commit takes a runtime PM reference in panfrost_reset_work() and
disables the reset work across system suspend and resume so GPU registers
are not accessed while the GPU is powered off.

> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
> index 40eda3612f849..2c11a67216e0f 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_device.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_device.c

[ ... ]

> @@ -604,14 +604,36 @@ static int panfrost_device_resume(struct device *dev)
>  	return ret;
>  }
>  
> +static void panfrost_device_enable_reset(struct panfrost_device *pfdev)
> +{
> +	enable_work(&pfdev->reset.work);
> +	if (atomic_read(&pfdev->reset.pending))
> +		queue_work(pfdev->reset.wq, &pfdev->reset.work);
> +}
> +
> +static int panfrost_device_resume(struct device *dev)
> +{
> +	int ret = panfrost_device_power_up(dev);
> +
> +	panfrost_device_enable_reset(dev_get_drvdata(dev));
> +	return ret;
> +}

[Severity: Medium]
Can panfrost_device_resume() queue pfdev->reset.work onto an unclocked GPU
when panfrost_device_power_up() fails?

If pm_runtime_force_resume() fails in panfrost_device_power_up(), it has
already called pm_runtime_set_suspended() and pm_runtime_enable(), and the
err_resume path disables the GPU_PM_CLK_DIS clocks and GPU_PM_VREG_OFF
regulator.  Calling panfrost_device_enable_reset() unconditionally then
queues panfrost_reset_work(), where pm_runtime_resume_and_get() runs
panfrost_device_runtime_resume() -> panfrost_device_reset() without
re-enabling those clocks or regulators, causing an unclocked MMIO access
and synchronous external abort on arm64.

> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
> index 3a46dd61cf87f..7319511db19f5 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_job.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_job.c
> @@ -810,7 +810,18 @@ static void panfrost_reset_work(struct work_struct *work)
>  	struct panfrost_device *pfdev;
>  
>  	pfdev = container_of(work, struct panfrost_device, reset.work);
> +
> +	/* The GPU may have been runtime suspended since the work was queued.
> +	 * If it cannot be resumed, no job is on it, and
> +	 * panfrost_device_runtime_resume() resets it later.
> +	 */
> +	if (pm_runtime_resume_and_get(pfdev->base.dev)) {
> +		atomic_set(&pfdev->reset.pending, 0);
> +		return;
> +	}

[Severity: Low]
Can clearing pfdev->reset.pending after pm_runtime_resume_and_get() fails
race with a concurrent job submission and leave the job unstarted?

kworker                                 CPU1
panfrost_reset_work()
  pm_runtime_resume_and_get() -> fails
                                        panfrost_job_run()
                                          pm_runtime_get_sync() -> succeeds
                                          panfrost_job_hw_submit()
                                            panfrost_enqueue_job()
                                            if (!atomic_read(&reset.pending))
                                              /* sees 1, skips JS_COMMAND_START */
  atomic_set(&pfdev->reset.pending, 0);
  return;                               <-- job left unstarted until timeout

rpm_resume() does not latch runtime_error on callback or supplier failure,
so a concurrent panfrost_job_run() can resume the GPU after
pm_runtime_resume_and_get() drops dev->power.lock.  Because
panfrost_device_runtime_resume() resets the GPU without clearing
pfdev->reset.pending, panfrost_job_hw_submit() sees reset.pending == 1 and
relies on panfrost_reset() to resubmit the job, which never happens once
panfrost_reset_work() clears reset.pending and returns.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-panfrost-long-jobs-v3-0-80ae78d38f6e@gmail.com?part=1