On 9/9/26 2:26 PM, [email protected] wrote:
[Severity: High]
Does delegating system suspend to pm_runtime_force_suspend() cause the system
sleep to abort if the device is lost or unplugged?
If the GPU crashes and is marked lost via drm_dev_unplug(), the PM core calls
the .suspend callback, which delegates to pvr_power_device_suspend():
drivers/gpu/drm/imagination/pvr_power.c:pvr_power_device_suspend()
if (!drm_dev_enter(drm_dev, &idx))
return -EIO;
Since the device is unplugged, drm_dev_enter() fails, and the function returns
-EIO. Will this permanently break system suspend for the entire system until
reboot, since the PM core aborts system suspend if any device's .suspend
callback returns an error?
[ ... ]
Yes, this is a real regression in v1. Thanks for catching it. The chain is:
pvr_device_lost() sets pvr_dev->lost and calls drm_dev_unplug(), which
sets dev->unplugged (drm_drv.c);
drm_dev_enter() then returns false;
pvr_power_device_suspend() returns -EIO (pvr_power.c);
pm_runtime_force_suspend() propagates it -- "if (ret) goto err;", and
the error path is pm_runtime_enable(dev); return ret; (runtime.c);
the PM core aborts the system suspend.
pm_runtime_force_suspend() only reaches the callback if the device is
runtime-active, and a lost device cannot runtime-suspend either, for the
same -EIO. As a result, it stays active and every subsequent system
suspend fails the same way. All that can fix it at that point is a
reboot or a rebind.
v2 wraps the force helpers and returns 0 early when the device is
already unplugged, because at that point there's nothing left to power
down. I test drm_dev_is_unplugged() rather than pvr_dev->lost so the
guard matches the exact condition the -EIO comes from, therefore also
covering an unplug arriving by any other path.
I verified v2 on my Amazon Fire HD 10 (2017) tablet, with no patch as
the control. The same kernel, same config, same tree, with the only
different being this patch applied and powervr.ko rebuilt and swapped (I
have the module as =m, so no reflash was needed; vermagic identical).
The control failed, and the v2 patch worked.
The test pins the GPU runtime-active -- the condition that fails -- then
system-suspends for 35 s and touches the GPU.
With the patch:
runtime_status before suspend: active
state write rc=0, uptime 606.04 -> 642.39
runtime_status after resume: active
vulkaninfo --summary: rc=0, deviceName = PowerVR Rogue GX6250
Without it, the same run, GPU working beforehand (rc=0, same deviceName)
and the suspend itself succeeding (uptime 52.62 -> 87.82):
VERDICT: FAIL -- GPU op still running after 100s
pid 2862 state: D
and the kernel's hung-task detector produced two stacks, from two
different ioctls, both wedged in the same place:
pvr_power_reset+0x64/0x534 [powervr]
pvr_mmu_flush_exec+0xec/0x18c [powervr]
pvr_mmu_op_context_destroy+0x58/0x1ec [powervr]
pvr_vm_bind_op_fini+0xa4/0xd0 [powervr]
pvr_vm_unmap_obj_locked+0x1f4/0x260 [powervr]
pvr_vm_unmap+0x5c/0x90 [powervr]
pvr_ioctl_vm_unmap+0x50/0x80 [powervr]
drm_ioctl_kernel+0xec/0x13c [drm]
drm_ioctl+0x254/0x3c4 [drm]
pvr_power_reset+0x64/0x534 [powervr]
pvr_mmu_flush_exec+0xec/0x18c [powervr]
pvr_submit_jobs+0x838/0xa60 [powervr]
pvr_ioctl_submit_jobs+0x6c/0x19c [powervr]
drm_ioctl_kernel+0xec/0x13c [drm]
drm_ioctl+0x254/0x3c4 [drm]
That's the chain from the commit message, and it also answers your
question about the deadlock: both stacks are under drm_ioctl, so
pvr_power_reset() is calling drm_dev_unplug() from inside the very SRCU
read section the ioctl holds. The box needed a reboot afterwards, and
the reboot itself was slow because the D-state task cannot be killed.
One note on this: this is a check-then-act, and kernel workqueues are
not frozen during dpm_suspend, so the watchdog worker could in principle
lose the device between the test and the callback. The window is small
and the consequence is one failed suspend rather than a permanent one,
but it is not zero. The alternative is to stop
pvr_power_device_suspend() reporting failure for hardware that is gone,
which removes the race and is arguably more correct for the runtime path
too -- but it changes existing behaviour, so I didn't fold it into this
fix. I can do it that way instead if it's preferred, just let me know in
v2.
[Severity: High]
This isn't a bug introduced by this patch, but does failing to suspend leave
the GPU watchdog permanently disabled?
Confirmed, pvr_power_fw_disable() does
cancel_delayed_work_sync(&watchdog.work) and then returns early if
pvr_power_request_idle() fails, without requeueing. The only requeue is
the queue_delayed_work() in pvr_power_fw_enable(), on the resume path,
and the PM core doesn't call resume for a device whose suspend failed.
So the watchdog stays cancelled and a firmware hang later on would go
undetected.
It's pre-existing, but my patch adds a new way to reach it through
system sleep, so I mentioned it in the v2 commit message. I can also
send a separate patch restarting the watchdog on that error path, if
that's wanted.
---
Everything else seems to be pre-existing and probably not worth
addressing in my v2.
For the v2, I'm not including [email protected] and
[email protected] -- the sends to them bounced. b4 --auto-to-cc
included them, but I'll just remove them for v2.
I'll also Cc Chen-Yu Tsai, Icenowy Zheng, and YoungJoon Lee, who are
also working on the same GPU and are much more experienced with this
hardware than I am :) I have an Amazon Fire HD 10 with the same GPU, but
I believe they are working on Chromebooks.
Thanks again,
Ryan
