pvr_power_device_suspend() and pvr_power_device_resume() take
drm_dev_enter() for their whole body and return -EIO once the device is
unplugged. The section protects nothing: the callbacks run on the
platform device, which stays bound until pvr_remove() returns. It is
being used as a "device lost" test, and pvr_dev->lost already is one.
Used that way it causes two problems. The suspend callback waits for
the watchdog worker in pvr_power_fw_disable() while holding the section,
and a worker losing the device calls drm_dev_unplug(), which waits in
synchronize_srcu() for that section: neither finishes. Runtime PM never
lets the two overlap because the worker holds a usage-count reference,
but pm_runtime_force_suspend() ignores the count, so a system-sleep
implementation on top of these callbacks deadlocks. Reproduced on
mt8173 with the worker delayed between taking its reference and losing
the device: system suspend froze until the hardware watchdog fired. And
once the device is lost the callbacks fail forever, which leaves runtime
PM in runtime_error and would abort every system suspend.
Test pvr_dev->lost instead and hold no section; a lost device is a
no-op for both callbacks. Re-test the flag after the cancel in
pvr_power_fw_disable(), since the worker just waited for may be the one
that lost the device.
Fixes: 727538a4bbff ("drm/imagination: Implement power management")
Cc: [email protected]
Assisted-by: LLM
Signed-off-by: Ryan Brue <[email protected]>
---
drivers/gpu/drm/imagination/pvr_power.c | 44 +++++++++++++--------------------
1 file changed, 17 insertions(+), 27 deletions(-)
diff --git a/drivers/gpu/drm/imagination/pvr_power.c
b/drivers/gpu/drm/imagination/pvr_power.c
index eb4b6ecdf4f4..8d82b9a79daf 100644
--- a/drivers/gpu/drm/imagination/pvr_power.c
+++ b/drivers/gpu/drm/imagination/pvr_power.c
@@ -97,6 +97,10 @@ pvr_power_fw_disable(struct pvr_device *pvr_dev, bool
hard_reset, bool rpm_suspe
if (!hard_reset) {
cancel_delayed_work_sync(&pvr_dev->watchdog.work);
+ /* The worker just cancelled may have lost the device. */
+ if (pvr_dev->lost)
+ return -EIO;
+
err = pvr_power_request_idle(pvr_dev);
if (err)
return err;
@@ -372,24 +376,19 @@ pvr_power_device_suspend(struct device *dev)
struct platform_device *plat_dev = to_platform_device(dev);
struct drm_device *drm_dev = platform_get_drvdata(plat_dev);
struct pvr_device *pvr_dev = to_pvr_device(drm_dev);
- int err = 0;
- int idx;
+ int err;
- if (!drm_dev_enter(drm_dev, &idx))
- return -EIO;
+ /* A lost device is left as the failed reset left it. */
+ if (pvr_dev->lost)
+ return 0;
if (READ_ONCE(pvr_dev->fw_dev.initialised)) {
err = pvr_power_fw_disable(pvr_dev, false, true);
if (err)
- goto err_drm_dev_exit;
+ return pvr_dev->lost ? 0 : err;
}
- err = pvr_dev->device_data->pwr_ops->power_off(pvr_dev);
-
-err_drm_dev_exit:
- drm_dev_exit(idx);
-
- return err;
+ return pvr_dev->device_data->pwr_ops->power_off(pvr_dev);
}
int
@@ -398,33 +397,24 @@ pvr_power_device_resume(struct device *dev)
struct platform_device *plat_dev = to_platform_device(dev);
struct drm_device *drm_dev = platform_get_drvdata(plat_dev);
struct pvr_device *pvr_dev = to_pvr_device(drm_dev);
- int idx;
int err;
- if (!drm_dev_enter(drm_dev, &idx))
- return -EIO;
+ if (pvr_dev->lost)
+ return 0;
err = pvr_dev->device_data->pwr_ops->power_on(pvr_dev);
if (err)
- goto err_drm_dev_exit;
+ return err;
if (READ_ONCE(pvr_dev->fw_dev.initialised)) {
err = pvr_power_fw_enable(pvr_dev, true);
- if (err)
- goto err_power_off;
+ if (err) {
+ pvr_dev->device_data->pwr_ops->power_off(pvr_dev);
+ return err;
+ }
}
- drm_dev_exit(idx);
-
return 0;
-
-err_power_off:
- pvr_dev->device_data->pwr_ops->power_off(pvr_dev);
-
-err_drm_dev_exit:
- drm_dev_exit(idx);
-
- return err;
}
int
--
2.55.0
