Re: [PATCH v3 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:
- [High] The return value of `pm_runtime_get_if_in_use()` is evaluated as a boolean, which incorrectly treats negative error codes (e.g., `-EINVAL`) as success, leading to a PM usage count underflow and unsafe hardware access.
- [High] Dropping the runtime PM reference prematurely when `wait_event_interruptible_timeout()` times out or is interrupted allows the device to suspend while the asynchronous hardware reset is still executing.
- [Medium] The return value of `wait_event_interruptible_timeout()` is ignored, masking failures from userspace.
--

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

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

This commit introduces a new debugfs file "reset" to the panfrost driver.
It allows userspace to manually trigger a GPU reset by writing to it, and
to read whether a reset is currently pending.

> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
> index cf115db9cebfd..80746e3754100 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_device.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_device.c
[ ... ]
> @@ -631,3 +632,40 @@ void panfrost_device_reset(struct panfrost_device *pfdev, bool enable_job_int)
[ ... ]
> +static int reset_set(void *data, u64 val)
> +{
> +	struct panfrost_device *pfdev =
> +		container_of(data, struct panfrost_device, base);
> +
> +	if (pm_runtime_get_if_in_use(pfdev->base.dev)) {

[Severity: High]
Does this incorrectly treat negative error codes as success?

If runtime PM is disabled (e.g., CONFIG_PM is disabled or during unbind),
pm_runtime_get_if_in_use() returns -EINVAL. This condition will evaluate to
true, causing the code to schedule a reset and call pm_runtime_put() without
an actual reference being taken. 

This can lead to a PM usage counter underflow and potential hardware access
on an unpowered device. Should this explicitly check for a return value > 0?

> +		panfrost_device_schedule_reset(pfdev);
> +		wait_event_interruptible_timeout(pfdev->reset.wait,
> +						 !atomic_read(&pfdev->reset.pending),
> +						 msecs_to_jiffies(60));

[Severity: Medium]
Is it intentional to ignore the return value of the wait?

If the wait is interrupted by a signal (-ERESTARTSYS) or times out (0), the
function unconditionally returns 0, falsely indicating success to userspace.

[Severity: High]
Can dropping the PM reference on timeout or signal interruption cause the
device to suspend while the reset is still executing?

If the wait is interrupted or times out, the code proceeds to pm_runtime_put()
even though the reset work might still be pending or running. 

Because panfrost_reset() drops PM references for active jobs before the
hardware reset, dropping this reference can drop the usage counter to zero.
If the device suspends while the async reset work is concurrently accessing
MMIO registers, could this trigger a synchronous external abort?

> +		pm_runtime_put(pfdev->base.dev);
> +	}
> +
> +	return 0;
> +}
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260724-claude-fixes-v3-0-542af61abd0f@collabora.com?part=9