Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [High] Disabling the reset work during initialization causes asynchronous reset requests to be silently dropped, permanently wedging the GPU by leaving the pending flag stuck at 1. Pre-existing issues: - [High] The reset work remains enabled during the error teardown path if `drm_dev_register` fails, allowing concurrent reset work to access sub-components as they are destroyed. -- commit d88d89caab294e4f6ec4a893f2e535ca95fb0890 Author: Boris Brezillon <[email protected]> drm/panthor: Keep the reset work disabled until everything is initialized The reset work will sub-component reset helpers, which might not be ready if the reset happens during initialization, leading to NULL pointer dereferences or worse. Avoid that by keeping the reset work disabled while we're initializing those sub-components. Closes: https://sashiko.dev/#/patchset/[email protected]?part=4 > diff --git a/drivers/gpu/drm/panthor/panthor_device.c > b/drivers/gpu/drm/panthor/panthor_device.c > --- a/drivers/gpu/drm/panthor/panthor_device.c > +++ b/drivers/gpu/drm/panthor/panthor_device.c > @@ -207,6 +207,7 @@ int panthor_device_init(struct panthor_device *ptdev) > *dummy_page_virt = 1; > > INIT_WORK(&ptdev->reset.work, panthor_device_reset_work); > + disable_work(&ptdev->reset.work); > ptdev->reset.wq = alloc_ordered_workqueue("panthor-reset-wq", 0); > if (!ptdev->reset.wq) > return -ENOMEM; [ ... ] > @@ -285,6 +286,9 @@ int panthor_device_init(struct panthor_device *ptdev) > > panthor_gem_init(ptdev); > > + /* Now that everything is initialized, we can enable the reset work. */ > + enable_work(&ptdev->reset.work); > + > /* ~3 frames */ > pm_runtime_set_autosuspend_delay(ptdev->base.dev, 50); > pm_runtime_use_autosuspend(ptdev->base.dev); [Severity: High] If a reset is requested while the work is disabled (e.g., due to a firmware timeout or early MMU fault during driver probe), does the device become permanently wedged? Looking at panthor_device_schedule_reset(), it relies on queue_work() succeeding after atomic_cmpxchg() sets the pending flag: panthor_device_schedule_reset() { if (!atomic_cmpxchg(&ptdev->reset.pending, 0, 1) && atomic_read(&ptdev->pm.state) == PANTHOR_DEVICE_PM_STATE_ACTIVE) { queue_work(ptdev->reset.wq, &ptdev->reset.work); } } Because the work is disabled via disable_work() earlier, queue_work() fails and the work is never queued. When enable_work() is called here, it decrements the disable count but does not requeue the dropped work or clear the pending flag. Because ptdev->reset.pending remains 1, will all future reset requests fail the atomic_cmpxchg() check, leaving the GPU unable to recover? [Severity: High] This isn't a bug introduced by this patch, but if drm_dev_register() fails shortly after this, does the reset work remain enabled during the teardown sequence? If drm_dev_register() fails, the driver jumps to err_disable_autosuspend to destroy sub-components sequentially. Because drm_dev_unplug() is not called in this explicit error path, dev->unplugged remains false. If a concurrent FW timeout or fault triggers during this teardown sequence, any concurrently scheduled reset work will pass the drm_dev_enter() check in panthor_device_reset_work(): panthor_device_reset_work() { if (!drm_dev_enter(&ptdev->base, &cookie)) return; panthor_sched_pre_reset(ptdev); panthor_fw_pre_reset(ptdev, true); ... } Could this cause the reset work to interact with sub-components that are actively being destroyed, leading to use-after-free or NULL pointer dereferences? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
