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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] amdxdna: Missing runtime PM reference during hardware context destruction
- [Low] amdxdna: Dead code due to unconditional assignment of PM callbacks in aie2_ops
--

commit e84dc9f8e09a4669bbdb0e93c4368ea773cd8d73
Author: David Zhang <yidong.zhang@amd.com>

accel/amdxdna: Implement runtime suspend and resume support

Add runtime suspend/resume for the AIE4 classic device by using its
system suspend/resume handlers.

> diff --git a/drivers/accel/amdxdna/aie2_pci.c b/drivers/accel/amdxdna/aie2_pci.c
> index 690ad035463c5..a3866e8933e61 100644
> --- a/drivers/accel/amdxdna/aie2_pci.c
> +++ b/drivers/accel/amdxdna/aie2_pci.c
> @@ -1221,6 +1221,8 @@ const struct amdxdna_dev_ops aie2_ops = {
>  	.fini = aie2_fini,
>  	.resume = aie2_hw_resume,
>  	.suspend = aie2_hw_suspend,
> +	.runtime_resume = aie2_hw_resume,
> +	.runtime_suspend = aie2_hw_suspend,

[Severity: Low]
Are pm_ptr() wrappers missing here?

Unlike aie4_classic_ops, which correctly wraps these assignments in
pm_ptr(), aie2_ops omits the wrappers. When the power management
configuration is disabled, the compiler cannot automatically discard
these functions, resulting in dead code being linked into the kernel.

>  	.get_aie_info = aie2_get_info,
>  	.set_aie_state = aie2_set_state,
>  	.hwctx_init = aie2_hwctx_init,

[ ... ]

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

[Severity: High]
Could this introduce a bug when destroying hardware contexts while the device
is suspended?

The driver enables runtime autosuspend here, allowing the device to enter
a suspended state when idle. When a user destroys a hardware context,
the destruction path runs without ensuring the device is active:

amdxdna_drm_destroy_hwctx_ioctl()
  amdxdna_hwctx_destroy_rcu()
    aie4_hwctx_stop()
      aie4_hwctx_destroy()
        aie4_msg_destroy_context()

None of these destruction paths call amdxdna_pm_resume_get() to wake up the
device.

If the device has autosuspended, the firmware is stopped and the mailbox
is finalized (via aie4_mailbox_fini()). Accessing the mailbox registers
in this state will cause a bus error, system hang, or kernel crash.

>  	return 0;
>  
>  free_work_buf:

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