Re: [PATCH 02/11] drm/panthor: Keep the reset work disabled until everything is initialized

[sashiko-bot@kernel.org]

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 <boris.brezillon@collabora.com>

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/20260625-panthor-signal-from-irq-v5-0-8836a74e0ef9@collabora.com?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/20260625-panthor-misc-fixes-v1-0-b67ed973fea6@collabora.com?part=2