Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] accel/amdxdna: VF device link removed on unbind causes PF to suspend - [Medium] pm: returning -EOPNOTSUPP from runtime_suspend bricks PM state machine - [Low] accel/amdxdna: runtime suspend deferral spams kernel log for passthrough VFs --
commit 9c2d41f547122e82068d6c644e6875d8381ac627 Author: David Zhang <[email protected]> Subject: accel/amdxdna: Implement runtime suspend and resume support This commit adds runtime PM support to the AIE4 driver. It wraps the device PM operations and implements runtime suspend/resume callbacks for the PF and VF. It also detects passthrough VFs to prevent the PF from suspending when VFs are in use. > diff --git a/drivers/accel/amdxdna/aie4_sriov.c > b/drivers/accel/amdxdna/aie4_sriov.c > index 3cad0526087db..c89bb033bb167 100644 > --- a/drivers/accel/amdxdna/aie4_sriov.c > +++ b/drivers/accel/amdxdna/aie4_sriov.c [ ... ] > +/* Check if a VF is actively running workloads or in passthrough. */ > +static int aie4_check_vf_alive(struct amdxdna_dev *xdna, struct pci_dev > *pdev_vf) > +{ > + struct device *pf_dev = xdna->ddev.dev; > + struct device_driver *drv; > + bool busy = false; > + > + if (!device_trylock(&pdev_vf->dev)) { > + XDNA_WARN(xdna, "VF:%s is busy (locked)", pci_name(pdev_vf)); > + return -EBUSY; > + } > + > + drv = pdev_vf->dev.driver; > + if (!drv) { > + device_unlock(&pdev_vf->dev); > + return 0; > + } [Severity: High] Does returning 0 here allow the PF to suspend prematurely when a VF is unbound? In aie4_link_vf(), the device link between the PF and VF is created with DL_FLAG_AUTOREMOVE_CONSUMER. When the user unbinds the host driver to prepare for passthrough (e.g., to vfio-pci), this device link drops. Since the VF temporarily has no driver during this transition, returning 0 here allows the PM core to successfully runtime-suspend the PF. When vfio-pci later binds to the VF, it wakes the VF, but because the PM link to the PF no longer exists, the PF remains suspended. The guest VM will then attempt to use the VF with the underlying hardware powered off. Could this lead to PCIe AER, host freezes, or MMIO faults? > + > + /* VF bound to a driver other than the PF's (amdxdna) is passthrough. */ > + if (drv != pf_dev->driver) { > + XDNA_WARN(xdna, "VF:%s is in passthrough", pci_name(pdev_vf)); > + busy = true; > + } else if (!pm_runtime_suspended(&pdev_vf->dev)) { > + XDNA_WARN(xdna, "VF:%s is busy", pci_name(pdev_vf)); > + busy = true; > + } > + > + device_unlock(&pdev_vf->dev); > + return busy ? -EBUSY : 0; > +} > + > +int aie4_vfs_alive(struct amdxdna_dev *xdna) > +{ > + if (pci_vfs_assigned(to_pci_dev(xdna->ddev.dev))) { > + XDNA_WARN(xdna, "VF devices are being used in VMs, cannot > suspend"); > + return -EBUSY; > + } [Severity: Low] Will using XDNA_WARN here flood the kernel log during normal passthrough operation? Returning -EBUSY to defer suspend is correct, but the PM core re-evaluates auto-suspend upon every pm_runtime_get/put cycle (e.g., from sysfs reads or background tasks). Because passthrough or VM assignment is a normal, expected state, returning -EBUSY repeatedly will spam the log with these warnings. Should these expected state deferrals use XDNA_DBG instead? > diff --git a/drivers/accel/amdxdna/amdxdna_pm.c > b/drivers/accel/amdxdna/amdxdna_pm.c > index 9c030b7836fbb..2b4ce301bb96f 100644 > --- a/drivers/accel/amdxdna/amdxdna_pm.c > +++ b/drivers/accel/amdxdna/amdxdna_pm.c [ ... ] > +int amdxdna_pm_runtime_suspend(struct device *dev) > +{ > + struct amdxdna_dev *xdna = to_xdna_dev(dev_get_drvdata(dev)); > + int ret = -EOPNOTSUPP; > + > + guard(mutex)(&xdna->dev_lock); > + if (xdna->dev_info->ops->runtime_suspend) > + ret = xdna->dev_info->ops->runtime_suspend(xdna); [Severity: Medium] Can returning -EOPNOTSUPP here permanently break the PM state machine for devices without a runtime_suspend callback? The Linux PM core (specifically rpm_suspend() in drivers/base/power/runtime.c) treats any return value from the runtime_suspend callback other than 0, -EAGAIN, or -EBUSY as a fatal error. This triggers the PM core to set dev->power.runtime_error = 1, which blocks all future runtime PM operations (get, put, suspend, resume) for that device until manually cleared. If a device variant does not implement the callback, shouldn't this wrapper return 0 so the PM core can safely transition? > + > + XDNA_DBG(xdna, "Runtime suspend done ret %d", ret); > + return ret; > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=19
