Re: [PATCH V3 19/19] accel/amdxdna: Implement runtime suspend and resume support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] amdxdna: PM deadlock in probe error path via pm_runtime_forbid()
- [High] amdxdna: missing PM reference in AIE4 hwctx_init causes hardware access while suspended
- [Low] pm: Unconditional external PM callbacks defeat dead code elimination
--

commit cc7f853ee4ac4c49242812a5366af4f77504c2a6
Author: David Zhang <yidong.zhang@amd.com>
Subject: accel/amdxdna: Implement runtime suspend and resume support

This commit adds runtime suspend and resume handlers for the AIE4 classic
device by wrapping its system PM handlers. It also attempts to discard
dev_pm_ops and associated callbacks by using the pm_ptr() wrapper when
CONFIG_PM is disabled.

> diff --git a/drivers/accel/amdxdna/aie4_pci.c b/drivers/accel/amdxdna/aie4_pci.c
> index a368bd3c257f5..ee64a5f3405dd 100644
> --- a/drivers/accel/amdxdna/aie4_pci.c
> +++ b/drivers/accel/amdxdna/aie4_pci.c
> @@ -1040,6 +1040,7 @@ static int aie4_classic_init(struct amdxdna_dev *xdna)
>  	if (ret)
>  		goto free_work_buf;
>  
> +	amdxdna_pm_init(xdna);

[Severity: High]
Does enabling runtime PM autosuspend here expose a missing wake-up in the
context creation path?

If the device autosuspends, and a user application calls the context creation
ioctl, aie4_hwctx_init() invokes aie4_hwctx_create(), which calls
aie_send_mgmt_msg_wait() to send a message via the hardware doorbell ring.

Unlike aie2_hwctx_init() which takes a PM reference, aie4_hwctx_init() does
not, which could lead to a synchronous external abort or system hang when
accessing the powered-down PCI device.

>  	return 0;
>  
>  free_work_buf:

[ ... ]

> @@ -1403,4 +1405,6 @@ const struct amdxdna_dev_ops aie4_classic_ops = {
>  	.set_aie_state		= aie4_set_state,
>  	.resume			= aie4_classic_resume,
>  	.suspend		= aie4_classic_suspend,
> +	.runtime_resume		= aie4_classic_resume,
> +	.runtime_suspend	= aie4_classic_suspend,

[Severity: Low]
Will these callbacks actually be discarded as dead code when CONFIG_PM is
disabled?

Since aie4_classic_suspend and aie4_classic_resume are unconditionally
assigned to the globally visible aie4_classic_ops structure, they will remain
referenced and cannot be eliminated by the compiler.

>  };

> diff --git a/drivers/accel/amdxdna/amdxdna_pci_drv.c b/drivers/accel/amdxdna/amdxdna_pci_drv.c
> index f5f7831c4e802..cb78f145ffa1f 100644
> --- a/drivers/accel/amdxdna/amdxdna_pci_drv.c
> +++ b/drivers/accel/amdxdna/amdxdna_pci_drv.c
> @@ -484,7 +484,7 @@ static struct pci_driver amdxdna_pci_driver = {
>  	.id_table = pci_ids,
>  	.probe = amdxdna_probe,
>  	.remove = amdxdna_remove,
> -	.driver.pm = &amdxdna_pm_ops,
> +	.driver.pm = pm_ptr(&amdxdna_pm_ops),

[Severity: Low]
Does this pm_ptr() actually prevent the PM callbacks from being compiled into
the kernel when CONFIG_PM is disabled?

Because amdxdna_pm_runtime_suspend and amdxdna_pm_runtime_resume are defined
unconditionally with external linkage in amdxdna_pm.c, the compiler cannot
eliminate them across translation units even when amdxdna_pm_ops is dropped.

>  	.sriov_configure = amdxdna_sriov_configure,
>  };

> diff --git a/drivers/accel/amdxdna/amdxdna_pm.c b/drivers/accel/amdxdna/amdxdna_pm.c
> index b1fafddd7ad59..7d7bc965342dc 100644
> --- a/drivers/accel/amdxdna/amdxdna_pm.c
> +++ b/drivers/accel/amdxdna/amdxdna_pm.c
> @@ -37,11 +37,40 @@ int amdxdna_pm_resume(struct device *dev)
>  	return ret;
>  }
>  

[ ... ]

> +int amdxdna_pm_runtime_resume(struct device *dev)
> +{
> +	struct amdxdna_dev *xdna = to_xdna_dev(dev_get_drvdata(dev));
> +	int ret = -EOPNOTSUPP;
> +
> +	guard(mutex)(&xdna->dev_lock);

[Severity: High]
Can this dev_lock acquisition cause a deadlock during the probe error path?

If amdxdna_probe() encounters an error after initialization (like a sysfs
or DRM registration failure), the error path calls xdna->dev_info->ops->fini()
with dev_lock held:

amdxdna_pci_drv.c:amdxdna_probe() {
    ...
failed_dev_fini:
	mutex_lock(&xdna->dev_lock);
	xdna->dev_info->ops->fini(xdna);
	mutex_unlock(&xdna->dev_lock);
    ...
}

This fini handler maps to aie4_classic_fini(), which now calls
amdxdna_pm_fini().

amdxdna_pm_fini() calls pm_runtime_forbid(). If the device has autosuspended
in the background, pm_runtime_forbid() synchronously resumes it, calling
amdxdna_pm_runtime_resume().

This leads to a second attempt to acquire dev_lock, deadlocking the probe
thread.

> +	if (xdna->dev_info->ops->runtime_resume)
> +		ret = xdna->dev_info->ops->runtime_resume(xdna);
> +
> +	XDNA_DBG(xdna, "Runtime resume done ret %d", ret);
> +	return ret;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008032348.2044667-1-yidong.zhang@amd.com?part=19