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 <[email protected]> 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/[email protected]?part=18
