Re: [PATCH V2 16/20] accel/amdxdna: Finalize runtime PM before acquiring dev_lock on removal

[Lizhi Hou]

On 10/6/26 00:13, Eva Crystal wrote:
> On Mon, Oct 05, 2026 at 09:22:26PM -0700, David Zhang wrote:
>
> > When the device is runtime-suspended, pm_runtime_forbid() synchronously
> > resumes the device via rpm_resume(), which invokes
> > amdxdna_pm_runtime_resume(). Because amdxdna_pm_runtime_resume()
> > acquires dev_lock, calling amdxdna_pm_fini() inside ops->fini() while
> > holding dev_lock in amdxdna_remove() causes a deadlock.
> > -	amdxdna_pm_fini(xdna);
> >   	aie2_hw_stop(xdna);
> >   	aie2_hwctx_sched_fini(xdna->dev_handle);
> This is worth more than its position in the series suggests: the deadlock is already live on shipping AIE2 parts, not only on the new AIE4 path.
>
> On current drm-misc-next, amdxdna_remove() holds dev_lock across ops->fini(xdna) (drivers/accel/amdxdna/amdxdna_pci_drv.c:457 and drivers/accel/amdxdna/amdxdna_pci_drv.c:463 at 34e9ab018249), and aie2_fini() opens with amdxdna_pm_fini() (drivers/accel/amdxdna/aie2_pci.c:640 at the same commit). pm_runtime_forbid() then calls rpm_resume(dev, 0) synchronously (drivers/base/power/runtime.c:1672), which lands in amdxdna_pm_resume() and its guard(mutex)(&xdna->dev_lock) on the same task. The base wires RUNTIME_PM_OPS(amdxdna_pm_suspend, amdxdna_pm_resume, NULL) and aie2_ops supplies .suspend and .resume, so runtime PM is active on aie2 before this series adds .runtime_suspend. With amdxdna_pm_init() setting a 5000 ms autosuspend delay then pm_runtime_allow(), an unbind or rmmod more than five seconds after the last NPU access hangs holding dev_lock.
On the remove path, pm_runtime_forbid() does not call 
amdxdna_pm_resume(). The PCI core has already resumed the device.

pci_device_remove() calls pm_runtime_get_sync() and pm_runtime_barrier() 
before amdxdna_remove(). That get resumes a runtime-suspended device, 
and the usage count stays elevated through aie2_fini(). 
pm_runtime_forbid() then enters rpm_resume() with status RPM_ACTIVE, 
which returns immediately and does not run ->runtime_resume()


Lizhi

> Three things that would help it travel:
>
> * Fixes: 1aa82181a3c2 ("accel/amdxdna: Fix dead lock for suspend and resume") looks right. amdxdna_pm.c had no dev_lock when 063db451832b created it, and 1aa82181a3c2 adds exactly the two guards the base still carries.
> * Cc: stable@xxxxxxxxxxxxxxx is warranted, since 1aa82181a3c2 is in v7.0 and later.
> * Could this be split out to drm-misc-fixes on its own? At position 16 of a 20 patch AIE4 series it is unlikely to be picked up as a fix, and splitting it stops the fixes cadence holding up the feature work.
>
> One question: amdxdna_pm_fini() now runs after drm_dev_unplug(), so pm_runtime_forbid() resumes hardware on a device already unregistered with its user mappings torn down. Intended?
>
> Eva Crystal (0xiviel)
> XSource Security
> https://xsourcesec.com