Re: [PATCH V2 19/20] 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] 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 <yidong.zhang@amd.com>
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/20261006042230.547807-1-yidong.zhang@amd.com?part=19