Re: [PATCH V3 18/19] accel/amdxdna: Implement AIE4 suspend and resume

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] accel/amdxdna: missing job abort on pci_enable_device failure in resume
--

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

accel/amdxdna: Implement AIE4 suspend and resume

This commit implements suspend and resume callbacks for AIE4 Physical Function
(PF), Virtual Function (VF), and Classic device types. It introduces functions
to manage hardware contexts and pending jobs across suspend boundaries, and
establishes stateless links between VFs and PFs for SR-IOV coordination.

diff --git a/drivers/accel/amdxdna/aie4_pci.c b/drivers/accel/amdxdna/aie4_pci.c
--- a/drivers/accel/amdxdna/aie4_pci.c
+++ b/drivers/accel/amdxdna/aie4_pci.c
[ ... ]
@@ -1277,11 +1277,10 @@ static int aie4_vf_resume(struct amdxdna_dev *xdna)
>  static int aie4_vf_resume(struct amdxdna_dev *xdna)
>  {
>  	struct amdxdna_dev_hdl *ndev = xdna->dev_handle;
>  	struct pci_dev *pdev = to_pci_dev(xdna->ddev.dev);
>  	int ret;
>  
>  	drm_WARN_ON(&xdna->ddev, !mutex_is_locked(&xdna->dev_lock));
>  
>  	ret = pci_enable_device(pdev);
>  	if (ret) {
>  		XDNA_ERR(xdna, "enable pci device failed %d", ret);
>  		return ret;

[Severity: High]
If pci_enable_device fails here, does this return leave pending software jobs
hanging indefinitely?

During suspend, aie4_hwctx_suspend_all is called with clean_jobs set to false,
which destroys the hardware context but intentionally leaves software jobs
pending. If resume fails early, shouldn't those pending jobs be explicitly
aborted and their fences signaled with an error?

The later failure path for aie4_vf_hw_start calls aie4_hwctx_suspend_all with
clean_jobs set to true to properly handle this cleanup.

>  	}
>  	pci_set_master(pdev);
>  
>  	ret = aie4_vf_hw_start(ndev);
>  	if (ret) {
>  		XDNA_ERR(xdna, "hw_start failed %d", ret);
>  		/* Contexts cannot reconnect; fail waiters and abort running jobs. */
>  		aie4_hwctx_suspend_all(ndev, true);
>  		goto pci_disable;
>  	}
[ ... ]
@@ -1331,11 +1331,10 @@ static int aie4_classic_resume(struct amdxdna_dev *xdna)
>  static int aie4_classic_resume(struct amdxdna_dev *xdna)
>  {
>  	struct amdxdna_dev_hdl *ndev = xdna->dev_handle;
>  	struct pci_dev *pdev = to_pci_dev(xdna->ddev.dev);
>  	int ret;
>  
>  	drm_WARN_ON(&xdna->ddev, !mutex_is_locked(&xdna->dev_lock));
>  
>  	ret = pci_enable_device(pdev);
>  	if (ret) {
>  		XDNA_ERR(xdna, "enable pci device failed %d", ret);
>  		return ret;

[Severity: High]
Does this error path share the same issue as aie4_vf_resume where failing to
call aie4_hwctx_suspend_all with clean_jobs set to true might deadlock waiting
userspace tasks?

>  	}
>  	pci_set_master(pdev);
>  
>  	ret = aie4_classic_hw_start(ndev);
>  	if (ret) {
>  		XDNA_ERR(xdna, "hw_start failed %d", ret);
>  		/* Contexts cannot reconnect; fail waiters and abort running jobs. */
>  		aie4_hwctx_suspend_all(ndev, true);
>  		goto pci_disable;
>  	}

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