[PATCH] drm/panthor: Fix a race between the reset and suspend path

[Steven Price]

On 17/12/2024 09:24, Boris Brezillon wrote:
> If a reset is scheduled when the suspend happens, we drop the
> reset-pending info on the floor assuming the resume will fix things,
> but the resume logic might try a fast reset. If we're lucky, the
> fast reset fails and we fallback to a slow reset, but if the FW was
> corrupted in a way that makes it partially functional (it boots but
> doesn't quite do what it's expected to do), we won't notice immediately
> that things are not working correctly, leading to a new reset further
> down the road.
> 
> Fixes: 5fe909cae118 ("drm/panthor: Add the device logical block")
> Signed-off-by: Boris Brezillon <boris.brezillon at collabora.com>

Reviewed-by: Steven Price <steven.price at arm.com>

> ---
>  drivers/gpu/drm/panthor/panthor_device.c | 22 ++++++++++++----------
>  1 file changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_device.c b/drivers/gpu/drm/panthor/panthor_device.c
> index 2c817e65e6be..3285ac42d2cd 100644
> --- a/drivers/gpu/drm/panthor/panthor_device.c
> +++ b/drivers/gpu/drm/panthor/panthor_device.c
> @@ -128,14 +128,11 @@ static void panthor_device_reset_work(struct work_struct *work)
>  	struct panthor_device *ptdev = container_of(work, struct panthor_device, reset.work);
>  	int ret = 0, cookie;
>  
> -	if (atomic_read(&ptdev->pm.state) != PANTHOR_DEVICE_PM_STATE_ACTIVE) {
> -		/*
> -		 * No need for a reset as the device has been (or will be)
> -		 * powered down
> -		 */
> -		atomic_set(&ptdev->reset.pending, 0);
> +	/* If the device is entering suspend, we don't reset. A slow reset will
> +	 * be forced at resume time instead.
> +	 */
> +	if (atomic_read(&ptdev->pm.state) != PANTHOR_DEVICE_PM_STATE_ACTIVE)
>  		return;
> -	}
>  
>  	if (!drm_dev_enter(&ptdev->base, &cookie))
>  		return;
> @@ -473,6 +470,14 @@ int panthor_device_resume(struct device *dev)
>  
>  	if (panthor_device_is_initialized(ptdev) &&
>  	    drm_dev_enter(&ptdev->base, &cookie)) {
> +		/* If there was a reset pending at the time we suspended the
> +		 * device, we force a slow reset.
> +		 */
> +		if (atomic_read(&ptdev->reset.pending)) {
> +			ptdev->reset.fast = false;
> +			atomic_set(&ptdev->reset.pending, 0);
> +		}
> +
>  		ret = panthor_device_resume_hw_components(ptdev);
>  		if (ret && ptdev->reset.fast) {
>  			drm_err(&ptdev->base, "Fast reset failed, trying a slow reset");
> @@ -489,9 +494,6 @@ int panthor_device_resume(struct device *dev)
>  			goto err_suspend_devfreq;
>  	}
>  
> -	if (atomic_read(&ptdev->reset.pending))
> -		queue_work(ptdev->reset.wq, &ptdev->reset.work);
> -
>  	/* Clear all IOMEM mappings pointing to this device after we've
>  	 * resumed. This way the fake mappings pointing to the dummy pages
>  	 * are removed and the real iomem mapping will be restored on next