[PATCH V1] accel/amdxdna: Prevent PM resume deadlock in hwctx_sync_debug_bo()

[PATCH V1] accel/amdxdna: Prevent PM resume deadlock in hwctx_sync_debug_bo()

[Lizhi Hou]

amdxdna_hwctx_sync_debug_bo() invokes the hardware
hwctx_sync_debug_bo() callback while holding xdna->dev_lock.

The callback may call amdxdna_cmd_submit(), which in turn calls
amdxdna_pm_resume_get(). If the device is suspended,
amdxdna_pm_resume_get() may synchronously execute
amdxdna_pm_resume(), which also acquires xdna->dev_lock, resulting
in a deadlock.

Avoid the deadlock by calling amdxdna_pm_resume_get() before holding
xdna->dev_lock.

Fixes: 7ea046838021 ("accel/amdxdna: Support firmware debug buffer")
Signed-off-by: Lizhi Hou <lizhi.hou@amd.com>
---
 drivers/accel/amdxdna/amdxdna_ctx.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/accel/amdxdna/amdxdna_ctx.c b/drivers/accel/amdxdna/amdxdna_ctx.c
index ff6c3e8e5a15..539daca1b42c 100644
--- a/drivers/accel/amdxdna/amdxdna_ctx.c
+++ b/drivers/accel/amdxdna/amdxdna_ctx.c
@@ -386,16 +386,25 @@ int amdxdna_hwctx_sync_debug_bo(struct amdxdna_client *client, u32 debug_bo_hdl)
 	if (!gobj)
 		return -EINVAL;
 
+	ret = amdxdna_pm_resume_get(xdna);
+	if (ret) {
+		XDNA_ERR(xdna, "Resume failed, ret %d", ret);
+		goto put_obj;
+	}
+
 	abo = to_xdna_obj(gobj);
 	guard(mutex)(&xdna->dev_lock);
 	hwctx = xa_load(&client->hwctx_xa, abo->assigned_hwctx);
 	if (!hwctx) {
 		ret = -EINVAL;
-		goto put_obj;
+		goto suspend_put;
 	}
 
 	ret = xdna->dev_info->ops->hwctx_sync_debug_bo(hwctx, debug_bo_hdl);
 
+suspend_put:
+	amdxdna_pm_suspend_put(xdna);
+
 put_obj:
 	drm_gem_object_put(gobj);
 	return ret;
-- 
2.34.1

Re: [PATCH V1] accel/amdxdna: Prevent PM resume deadlock in hwctx_sync_debug_bo()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Mixing `goto` and `guard()` causes undefined behavior/build error
--

commit e82723d9f7a656583eeaed6e8880da6e23ecb6df
Author: Lizhi Hou <lizhi.hou@amd.com>

accel/amdxdna: Prevent PM resume deadlock in hwctx_sync_debug_bo()

This commit reorders amdxdna_pm_resume_get() to execute before acquiring
xdna->dev_lock. This prevents a potential deadlock that occurs if the
device is suspended during the callback.

> diff --git a/drivers/accel/amdxdna/amdxdna_ctx.c b/drivers/accel/amdxdna/amdxdna_ctx.c
> index 855da8c79a1cd..20e2fd096c913 100644
> --- a/drivers/accel/amdxdna/amdxdna_ctx.c
> +++ b/drivers/accel/amdxdna/amdxdna_ctx.c
> @@ -412,16 +412,25 @@ int amdxdna_hwctx_sync_debug_bo(struct amdxdna_client *client, u32 debug_bo_hdl)
>  	if (!gobj)
>  		return -EINVAL;
>  
> +	ret = amdxdna_pm_resume_get(xdna);
> +	if (ret) {
> +		XDNA_ERR(xdna, "Resume failed, ret %d", ret);
> +		goto put_obj;
> +	}
> +
>  	abo = to_xdna_obj(gobj);
>  	guard(mutex)(&xdna->dev_lock);

[Severity: High]
Does jumping forward over the guard(mutex) declaration with the goto put_obj
statement above trigger a compilation error?

Jumping over a variable initialized with __attribute__((cleanup)) is usually
rejected by modern compilers. If a compiler allowed it, the destructor would
execute at the end of the function using uninitialized stack memory for the
lock pointer, causing a kernel panic.

Additionally, include/linux/cleanup.h notes that mixing goto-based error
handling and scope-based cleanups in the same function should be avoided to
prevent issues like this.

>  	hwctx = xa_load(&client->hwctx_xa, abo->assigned_hwctx);
>  	if (!hwctx) {
>  		ret = -EINVAL;
> -		goto put_obj;
> +		goto suspend_put;
>  	}
>  
>  	ret = xdna->dev_info->ops->hwctx_sync_debug_bo(hwctx, debug_bo_hdl);
>  
> +suspend_put:
> +	amdxdna_pm_suspend_put(xdna);
> +
>  put_obj:
>  	drm_gem_object_put(gobj);
>  	return ret;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260616062008.3533989-1-lizhi.hou@amd.com?part=1