Re: [PATCH 12/12] drm/panthor: Fix unplug in the reset path

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

Pre-existing issues:
- [High] Concurrent execution of device removal and a failing reset work causes an unrecoverable AB-BA cross-thread deadlock.
--

--- Patch [12]: [PATCH 12/12] drm/panthor: Fix unplug in the reset path ---
commit cac1a79672cc7dc4544f4f69a02f107969842cb8
Author: Boris Brezillon <[email protected]>

drm/panthor: Fix unplug in the reset path

We can't use disable_work_sync() if panthor_device_unplug() is called
from the reset work or we'll deadlock. Pass a from_reset_work bool to
the panthor_device_unplug() function and lower the disable_work_sync()
to a disable_work() in that case.

> 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
> @@ -189,13 +192,16 @@ static void panthor_device_reset_work(struct work_struct *work)
>  	panthor_hw_soft_reset(ptdev);
>  	panthor_hw_l2_power_on(ptdev);
>  	panthor_mmu_post_reset(ptdev);
> -	ret = panthor_fw_post_reset(ptdev);
> +	if (ptdev->reset.fake_failure)
> +		ret = -EIO;
> +	else
> +		ret = panthor_fw_post_reset(ptdev);
>  	atomic_set(&ptdev->reset.pending, 0);
>  	panthor_sched_post_reset(ptdev, ret != 0);
>  	drm_dev_exit(cookie);
>  
>  	if (ret) {
> -		panthor_device_unplug(ptdev);
> +		panthor_device_unplug(ptdev, true);

[Severity: High]
This is a pre-existing issue, but does this sequence still leave an AB-BA
deadlock if driver removal runs concurrently with a failing reset work?

If CPU1 is running driver removal via panthor_remove(), it calls
panthor_device_unplug(..., false). This acquires ptdev->unplug.lock
earlier in the function.

If CPU2 is running panthor_device_reset_work(), it holds the SRCU
read lock via drm_dev_enter().

CPU1 then blocks in drm_dev_unplug() waiting for active SRCU readers.

CPU2 encounters a reset error, calls drm_dev_exit() (releasing SRCU),
and unblocks CPU1.

CPU1 continues in panthor_device_unplug() and blocks in
disable_work_sync() waiting for the reset work to finish (since it
holds ptdev->unplug.lock and from_reset_work is false):

CPU1:
panthor_remove()
  panthor_device_unplug(ptdev, false)
    mutex_lock(&ptdev->unplug.lock);
    ...
    drm_dev_unplug(&ptdev->base);
    ...
    disable_work_sync(&ptdev->reset.work);

Meanwhile, CPU2 proceeds to the error handling path and calls
panthor_device_unplug(ptdev, true). This attempts to acquire
ptdev->unplug.lock, but CPU1 already holds it:

CPU2:
panthor_device_reset_work()
  ...
  if (ret)
    panthor_device_unplug(ptdev, true)
      mutex_lock(&ptdev->unplug.lock);

Could this cause both threads to hang indefinitely?

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