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 <[email protected]> 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/[email protected]?part=9
