| Message ID | 20241217092457.1582053-1-boris.brezillon@collabora.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm/panthor: Fix a race between the reset and suspend path | expand |
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@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. 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 >
On Tue, 17 Dec 2024 10:56:14 +0000 Liviu Dudau <liviu.dudau@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@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 ;-). > > 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 > > >
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@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@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@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 > > > > > >
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
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@collabora.com> --- drivers/gpu/drm/panthor/panthor_device.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-)