Re: [PATCH v4 09/13] drm/panfrost: Add debugfs knob for manually triggering a GPU reset

[sashiko-bot@kernel.org]

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

New issues:
- [High] Debugfs `reset_set` drops the PM runtime reference on wait timeout or signal before the reset work completes, leading to hardware access while the device is suspended.
- [Medium] Incorrect return value logic in `reset_set` masks signal interruptions and suspended PM states by unconditionally returning `-ETIMEDOUT`.

Pre-existing issues:
- [High] The GPU reset sequence drops PM runtime references from stuck jobs before accessing hardware registers, risking a crash if the device suspends concurrently.
--

--- Patch [9]: [PATCH v4 09/13] drm/panfrost: Add debugfs knob for manually triggering a GPU reset ---
commit f19b1f8bf36d92ef2c4bcc1bdcf953c6bd32d4fd
Author: Adrián Larumbe <adrian.larumbe@collabora.com>

drm/panfrost: Add debugfs knob for manually triggering a GPU reset

This commit adds a debugfs file to manually trigger a GPU reset. This is
intended to help test potential race conditions between the GPU reset
sequence and other parts of the code accessing hardware registers.

> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
> index a51f1ef8563ce..c0334b72c70b3 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_device.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_device.c

[ ... ]

> @@ -629,3 +630,43 @@ void panfrost_device_reset(struct panfrost_device *pfdev, bool enable_job_int)
>  	if (!panfrost_device_started(pfdev) && !ret)
>  		pfdev->gpu_started = true;
>  }
> +
> +#ifdef CONFIG_DEBUG_FS
> +static int reset_get(void *data, u64 *val)
> +{

[ ... ]

> +static int reset_set(void *data, u64 val)
> +{
> +	struct panfrost_device *pfdev =
> +		container_of(data, struct panfrost_device, base);
> +	int ret;
> +
> +	ret = pm_runtime_get_if_in_use(pfdev->base.dev);
> +
> +	if (ret > 0) {
> +		panfrost_device_schedule_reset(pfdev);
> +		ret = wait_event_interruptible_timeout(pfdev->reset.wait,
> +						       !atomic_read(&pfdev->reset.pending),
> +						       msecs_to_jiffies(60));
> +		pm_runtime_put(pfdev->base.dev);

[Severity: High]
If the wait is interrupted by a signal (like Ctrl-C) or times out, does
this immediately drop the PM usage count to 0 and queue a suspend request?

Since the scheduled reset work continues executing asynchronously without a
PM reference, could it access the GPU registers via panfrost_device_reset()
while the device is already powered down, leading to a system crash?

> +	}
> +
> +	return (ret > 0) ? 0 : -ETIMEDOUT;

[Severity: Medium]
Does this ternary return statement mask signal interruptions and PM runtime
errors?

If wait_event_interruptible_timeout() is interrupted, it returns -ERESTARTSYS.
Converting this negative error code to -ETIMEDOUT prevents the kernel from
correctly restarting the syscall or returning -EINTR to user space. It also
masks legitimate errors if pm_runtime_get_if_in_use() returned 0 initially.

> +}
> +
> +DEFINE_DEBUGFS_ATTRIBUTE(panfrost_reset_debugfs_fops,
> +			 reset_get, reset_set,
> +			 "0x%08llx\n");

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing the reset
sequences, I noticed a potential race condition in the existing
panfrost_reset() handler in drivers/gpu/drm/panfrost/panfrost_job.c:

panfrost_job.c:panfrost_reset() {
    ...
    for (i = 0; i < NUM_JOB_SLOTS; i++) {
        if (pfdev->jobs[i][0])
            pm_runtime_put_noidle(pfdev->base.dev);
        ...
    }
    memset(pfdev->jobs, 0, sizeof(pfdev->jobs));
    ...
    panfrost_device_reset(pfdev, true);
}

Does dropping the PM runtime references for all jobs before executing the
hardware reset risk a crash?

By dropping the usage count and clearing the jobs array, panfrost_jm_is_idle()
will return true. If a concurrent thread had triggered a PM autosuspend timer,
could the device be suspended before panfrost_device_reset() accesses the GPU
registers? Should panfrost_reset() acquire its own PM reference before
dropping the job references?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260729-claude-fixes-v4-0-01968f2ec77a@collabora.com?part=9