Re: [PATCH 02/12] drm/panthor: Further delay reset work enablement

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

New issues:
- [High] Calling disable_work_sync() on a work item from within the work item itself causes a deadlock.

Pre-existing issues:
- [High] A race condition during device resume can cause a hardware fault to be permanently ignored, leaving the reset.pending flag stuck at 1 and blocking all future resets.
- [High] A race condition at the end of the reset work can cause a new hardware fault to be lost because `reset.pending` is cleared after interrupts are re-enabled.
--

--- Patch [2]: [PATCH 02/12] drm/panthor: Further delay reset work enablement ---
commit 08177a6c8d92319bf18b6e18c4bb7d3f3023ca20
Author: Boris Brezillon <[email protected]>

drm/panthor: Further delay reset work enablement

There's no point re-enabling the reset work before the DRM device
registration succeeds, so move the enable_work() after the point where
nothing can fail anymore, and in the unlikely event where a reset was
pending, reschedule it.

> 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

[ ... ]

> @@ -300,6 +297,14 @@ int panthor_device_init(struct panthor_device *ptdev)
>  	if (ret)
>  		goto err_disable_autosuspend;
>  
> +	/* Now that everything is initialized, we can enable the reset work.
> +	 * If there was a reset pending, clear and reschedule, otherwise the
> +	 * reset.pending bit is stuck.
> +	 */
> +	enable_work(&ptdev->reset.work);
> +	if (atomic_cmpxchg(&ptdev->reset.pending, 1, 0))
> +		panthor_device_schedule_reset(ptdev);

[Severity: High]
Can this lead to a deadlock if the firmware boot fails?

If panthor_fw_post_reset() fails during panthor_device_reset_work() in
drivers/gpu/drm/panthor/panthor_device.c, it directly calls
panthor_device_unplug().

panthor_device_unplug() then unconditionally calls:
disable_work_sync(&ptdev->reset.work);

Since disable_work_sync() synchronously waits for the currently executing
work item to complete, would this deadlock on itself if called from within
the reset work?

[Severity: High]
This is a pre-existing issue, but is the same fix needed in
panthor_device_resume()?

In panthor_device_resume(), pm.state is set to ACTIVE without checking if a
fault occurred. When resuming, pm.state starts as RESUMING and interrupts
are re-enabled by panthor_device_resume_hw_components().

If a fault fires before pm.state becomes ACTIVE, the interrupt handler calls
panthor_device_schedule_reset(), which sets reset.pending to 1 but skips
queuing the work because pm.state is not ACTIVE.

When panthor_device_resume() later sets pm.state to ACTIVE without checking
if pending became 1 (unlike the fix applied here in panthor_device_init),
does this leave reset.pending stuck at 1 and block all future resets?

[Severity: High]
This is a pre-existing issue, but can a hardware fault be lost due to a race
condition at the end of the reset work?

In panthor_device_reset_work(), panthor_mmu_post_reset() and
panthor_fw_post_reset() are called, which re-enable hardware interrupts.

If a fault occurs immediately after, the interrupt handler calls
panthor_device_schedule_reset(). Since reset.pending is still 1,
the atomic_cmpxchg fails and no work is queued.

Then, panthor_device_reset_work() executes:
atomic_set(&ptdev->reset.pending, 0);

Are faults occurring in this window silently ignored?

> +
>  	pm_runtime_put_autosuspend(ptdev->base.dev);
>  	return 0;
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2