Re: [PATCH v2 23/28] drm/panthor: Synchronize HW component PM transitions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] drm/panthor: missing slow-reset fallback in
  panthor_device_resume_hw_components()
--

commit b392fdf733ca625b6c9ab7dbb3e11caa5a1d1b14
Author: Karunika Choo <karunika.choo@arm.com>

drm/panthor: Synchronize HW component PM transitions

This commit splits hardware component suspend and resume into
panthor_device_suspend_hw_components() and
panthor_device_resume_hw_components(), serializing transitions with
ptdev->pm.hw_component_lock and ptdev->pm.hw_component_state.

> diff --git a/drivers/gpu/drm/panthor/panthor_device.c b/drivers/gpu/drm/panthor/panthor_device.c
> index 98c0dd3ce7bcb..2ccd8ad6318d1 100644
> --- a/drivers/gpu/drm/panthor/panthor_device.c
> +++ b/drivers/gpu/drm/panthor/panthor_device.c
[ ... ]
> @@ -484,21 +491,55 @@ int panthor_device_mmap_io(struct panthor_device *ptdev, struct vm_area_struct *
>  	return 0;
>  }
>  
> -static int panthor_device_resume_hw_components(struct panthor_device *ptdev)
> +int panthor_device_suspend_hw_components(struct panthor_device *ptdev)
> +{
> +	guard(mutex)(&ptdev->pm.hw_component_lock);
> +
> +	if (atomic_read(&ptdev->pm.hw_component_state) ==
> +	    PANTHOR_DEVICE_PM_STATE_SUSPENDED)
> +		return 0;
> +
> +	if (!pm_runtime_suspended(ptdev->base.dev)) {
> +		panthor_fw_suspend(ptdev);
> +		panthor_mmu_suspend(ptdev);
> +		panthor_gpu_suspend(ptdev);
> +		panthor_pwr_suspend(ptdev);
> +	}
> +
> +	atomic_set(&ptdev->pm.hw_component_state,
> +		   PANTHOR_DEVICE_PM_STATE_SUSPENDED);
> +
> +	return 0;
> +}
> +
> +int panthor_device_resume_hw_components(struct panthor_device *ptdev)
>  {
>  	int ret;
>  
> +	guard(mutex)(&ptdev->pm.hw_component_lock);
> +
> +	if (atomic_read(&ptdev->pm.hw_component_state) ==
> +	    PANTHOR_DEVICE_PM_STATE_ACTIVE)
> +		return 0;
> +
>  	panthor_pwr_resume(ptdev);
>  	panthor_gpu_resume(ptdev);
>  	panthor_mmu_resume(ptdev);
>  
>  	ret = panthor_fw_resume(ptdev);
> -	if (!ret)
> +	if (!ret) {
> +		atomic_set(&ptdev->pm.hw_component_state,
> +			   PANTHOR_DEVICE_PM_STATE_ACTIVE);
>  		return 0;
> +	}
>  
>  	panthor_mmu_suspend(ptdev);
>  	panthor_gpu_suspend(ptdev);
>  	panthor_pwr_suspend(ptdev);
> +
> +	atomic_set(&ptdev->pm.hw_component_state,
> +		   PANTHOR_DEVICE_PM_STATE_SUSPENDED);
> +
>  	return ret;
>  }

[Severity: Medium]
Should the ptdev->reset.pending check and the fast-to-slow reset retry live
inside panthor_device_resume_hw_components() under
ptdev->pm.hw_component_lock rather than only in panthor_device_resume()?

drivers/gpu/drm/panthor/panthor_device.c:panthor_device_resume() {
    ...
		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");
			ptdev->reset.fast = false;
			ret = panthor_device_resume_hw_components(ptdev);
		}
    ...
}

When access-window arbitration later calls
panthor_device_resume_hw_components() while runtime PM remains active,
panthor_device_resume() is not run.  A pending reset in ptdev->reset.pending
is then ignored, and a failed fast reset does not fall back to a slow reset
by reloading firmware sections, causing component resume to fail.  In
panthor_device_resume(), ptdev->reset.fast is also modified outside
ptdev->pm.hw_component_lock and the lock is dropped between the fast-reset
attempt and the slow-reset retry.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110153907.3241978-1-karunika.choo@arm.com?part=23