[PATCH V1] accel/amdxdna: Handle DETACH_DEBUG_BO through config_debug_bo path

[PATCH V1] accel/amdxdna: Handle DETACH_DEBUG_BO through config_debug_bo path

[Lizhi Hou]

From: Max Zhen <max.zhen@amd.com>

Route DETACH_DEBUG_BO through aie2_config_debug_bo() the same way as
ATTACH_DEBUG_BO.

The scheduler switch in aie2_sched_job_run() already handles
ATTACH_DEBUG_BO with aie2_config_debug_bo(), but DETACH_DEBUG_BO was
not included in that path. Add an explicit fallthrough so both attach
and detach operations use the same handler.

This fixes debug BO detach handling by ensuring the detach command is
processed by the expected configuration path.

Fixes: 7ea046838021 ("accel/amdxdna: Support firmware debug buffer")
Signed-off-by: Max Zhen <max.zhen@amd.com>
Signed-off-by: Lizhi Hou <lizhi.hou@amd.com>
---
 drivers/accel/amdxdna/aie2_ctx.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/accel/amdxdna/aie2_ctx.c b/drivers/accel/amdxdna/aie2_ctx.c
index 8db32f8e2362..c464cf8024c3 100644
--- a/drivers/accel/amdxdna/aie2_ctx.c
+++ b/drivers/accel/amdxdna/aie2_ctx.c
@@ -360,6 +360,8 @@ aie2_sched_job_run(struct drm_sched_job *sched_job)
 			ret = aie2_sync_bo(hwctx, job, aie2_sched_drvcmd_resp_handler);
 			break;
 		case ATTACH_DEBUG_BO:
+			fallthrough;
+		case DETACH_DEBUG_BO:
 			ret = aie2_config_debug_bo(hwctx, job, aie2_sched_drvcmd_resp_handler);
 			break;
 		default:
-- 
2.34.1

Re: [PATCH V1] accel/amdxdna: Handle DETACH_DEBUG_BO through config_debug_bo path

[Mario Limonciello]

On 4/6/26 16:14, Lizhi Hou wrote:
> From: Max Zhen <max.zhen@amd.com>
> 
> Route DETACH_DEBUG_BO through aie2_config_debug_bo() the same way as
> ATTACH_DEBUG_BO.
> 
> The scheduler switch in aie2_sched_job_run() already handles
> ATTACH_DEBUG_BO with aie2_config_debug_bo(), but DETACH_DEBUG_BO was
> not included in that path. Add an explicit fallthrough so both attach
> and detach operations use the same handler.
> 
> This fixes debug BO detach handling by ensuring the detach command is
> processed by the expected configuration path.
> 
> Fixes: 7ea046838021 ("accel/amdxdna: Support firmware debug buffer")
> Signed-off-by: Max Zhen <max.zhen@amd.com>
> Signed-off-by: Lizhi Hou <lizhi.hou@amd.com>
> ---
>   drivers/accel/amdxdna/aie2_ctx.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/accel/amdxdna/aie2_ctx.c b/drivers/accel/amdxdna/aie2_ctx.c
> index 8db32f8e2362..c464cf8024c3 100644
> --- a/drivers/accel/amdxdna/aie2_ctx.c
> +++ b/drivers/accel/amdxdna/aie2_ctx.c
> @@ -360,6 +360,8 @@ aie2_sched_job_run(struct drm_sched_job *sched_job)
>   			ret = aie2_sync_bo(hwctx, job, aie2_sched_drvcmd_resp_handler);
>   			break;
>   		case ATTACH_DEBUG_BO:
> +			fallthrough;
> +		case DETACH_DEBUG_BO:

TBH - I don't think you actually need the fallthrough command here 
unless you plan to add new code in the ATTACH_DEBUG_BO case.

IE you can do this:

case ATTACH_DEBUG_BO:
case DETACH_DEBUG_BO:

Reviewed-by: Mario Limonciello (AMD) <superm1@kernel.org>

>   			ret = aie2_config_debug_bo(hwctx, job, aie2_sched_drvcmd_resp_handler);
>   			break;
>   		default:

Re: [PATCH V1] accel/amdxdna: Handle DETACH_DEBUG_BO through config_debug_bo path

[Lizhi Hou]

On 4/6/26 14:29, Mario Limonciello wrote:
>
>
> On 4/6/26 16:14, Lizhi Hou wrote:
>> From: Max Zhen <max.zhen@amd.com>
>>
>> Route DETACH_DEBUG_BO through aie2_config_debug_bo() the same way as
>> ATTACH_DEBUG_BO.
>>
>> The scheduler switch in aie2_sched_job_run() already handles
>> ATTACH_DEBUG_BO with aie2_config_debug_bo(), but DETACH_DEBUG_BO was
>> not included in that path. Add an explicit fallthrough so both attach
>> and detach operations use the same handler.
>>
>> This fixes debug BO detach handling by ensuring the detach command is
>> processed by the expected configuration path.
>>
>> Fixes: 7ea046838021 ("accel/amdxdna: Support firmware debug buffer")
>> Signed-off-by: Max Zhen <max.zhen@amd.com>
>> Signed-off-by: Lizhi Hou <lizhi.hou@amd.com>
>> ---
>>   drivers/accel/amdxdna/aie2_ctx.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/accel/amdxdna/aie2_ctx.c 
>> b/drivers/accel/amdxdna/aie2_ctx.c
>> index 8db32f8e2362..c464cf8024c3 100644
>> --- a/drivers/accel/amdxdna/aie2_ctx.c
>> +++ b/drivers/accel/amdxdna/aie2_ctx.c
>> @@ -360,6 +360,8 @@ aie2_sched_job_run(struct drm_sched_job *sched_job)
>>               ret = aie2_sync_bo(hwctx, job, 
>> aie2_sched_drvcmd_resp_handler);
>>               break;
>>           case ATTACH_DEBUG_BO:
>> +            fallthrough;
>> +        case DETACH_DEBUG_BO:
>
> TBH - I don't think you actually need the fallthrough command here 
> unless you plan to add new code in the ATTACH_DEBUG_BO case.
>
> IE you can do this:
>
> case ATTACH_DEBUG_BO:
> case DETACH_DEBUG_BO:
>
> Reviewed-by: Mario Limonciello (AMD) <superm1@kernel.org>

Thanks. Applied to drm-misc-next.

Lizhi

>
>>               ret = aie2_config_debug_bo(hwctx, job, 
>> aie2_sched_drvcmd_resp_handler);
>>               break;
>>           default:
>

Claude review: accel/amdxdna: Handle DETACH_DEBUG_BO through config_debug_bo path

[Claude Code Review Bot]

Overall Series Review

Subject: accel/amdxdna: Handle DETACH_DEBUG_BO through config_debug_bo path
Author: Lizhi Hou <lizhi.hou@amd.com>
Patches: 3
Reviewed: 2026-04-12T14:24:28.973286

---

This is a single-patch fix for the `accel/amdxdna` driver. The bug is clear and the fix is correct: `DETACH_DEBUG_BO` commands submitted via `amdxdna_cmd_submit()` (at `aie2_ctx.c:883`) would hit the `default` case in `aie2_sched_job_run()` and return `-EINVAL`, because only `ATTACH_DEBUG_BO` was handled in the switch statement. The downstream handler `aie2_config_debug_bo()` already distinguishes between attach and detach via the opcode (line 1179 of `aie2_message.c`), so routing both opcodes to that function is the correct approach.

The patch is minimal, well-targeted, and has an appropriate `Fixes:` tag.

One minor style nit below, but functionally this looks good to merge.

---
Generated by Claude Code Patch Reviewer

Claude review: accel/amdxdna: Handle DETACH_DEBUG_BO through config_debug_bo path

[Claude Code Review Bot]

Patch Review

**Correctness: Good.** The root cause is correctly identified. Looking at `aie2_ctx.c:858-884`, when `attach` is false, `cmd.opcode` is set to `DETACH_DEBUG_BO`, which then gets submitted to the scheduler. But in `aie2_sched_job_run()` at line 358, the switch only matched `ATTACH_DEBUG_BO`, so `DETACH_DEBUG_BO` fell through to `default: ret = -EINVAL`. The handler `aie2_config_debug_bo()` in `aie2_message.c:1179` already correctly checks the opcode to decide between `DEBUG_BO_REGISTER` and `DEBUG_BO_UNREGISTER`, so routing `DETACH_DEBUG_BO` there is the right fix.

**Style nit:** The explicit `fallthrough;` annotation is unnecessary here. In kernel style, `fallthrough` is used when there is code in the preceding case before falling into the next one. When two cases simply share the same body with no intervening code, stacking the `case` labels is the standard idiom:

```c
case ATTACH_DEBUG_BO:
case DETACH_DEBUG_BO:
    ret = aie2_config_debug_bo(hwctx, job, aie2_sched_drvcmd_resp_handler);
    break;
```

The `fallthrough` keyword is mandated by `-Wimplicit-fallthrough` only when there are statements between the case labels. With no statements, there is no implicit fallthrough warning, and the bare stacked-case form is both idiomatic and more concise. The existing `SYNC_DEBUG_BO` case just above demonstrates this — it doesn't use `fallthrough` before its own handler. This is a minor style point and shouldn't block the patch, but it would be cleaner without it.

**Overall:** The fix is correct and important — without it, debug BO detach would always fail silently (well, with `-EINVAL`). Recommend merging with the optional style cleanup.

---
Generated by Claude Code Patch Reviewer