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

[Liviu Dudau]

On Tue, Dec 17, 2024 at 12:06:59PM +0100, Boris Brezillon wrote:
> On Tue, 17 Dec 2024 10:56:14 +0000
> Liviu Dudau <liviu.dudau at arm.com> wrote:
> 
> > On Tue, Dec 17, 2024 at 10:24:57AM +0100, 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>
> > > ---
> > >  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);
> > > -  
> > 
> > If you remove this, why do we need a reset workqueue at all? Nothing gets scheduled in it.
> 
> git grep reset\.wq drivers/gpu/drm/panthor/ reports a few users ;-).

Sorry, I've only looked at panthor_device.c and brain was not fully engaged.

I think a more relevant comment is that panthor_device_schedule_reset() is still called in a few
places and that in turn queues work in reset.wq. Also, the firmware watchdog uses that workqueue.

Patch looks good to me,

Reviewed-by: Liviu Dudau <liviu.dudau at arm.com>

Best regards,
Liviu

> 
> > 
> > Best regards,
> > Liviu
> > 
> > >  	/* 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
> > > -- 
> > > 2.47.0
> > >   
> > 
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯