From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: accel/amdxdna: Handle DETACH_DEBUG_BO through config_debug_bo path Date: Sun, 12 Apr 2026 14:24:29 +1000 Message-ID: In-Reply-To: <20260406211403.4011988-1-lizhi.hou@amd.com> References: <20260406211403.4011988-1-lizhi.hou@amd.com> <20260406211403.4011988-1-lizhi.hou@amd.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 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 `DETA= CH_DEBUG_BO` fell through to `default: ret =3D -EINVAL`. The handler `aie2_= config_debug_bo()` in `aie2_message.c:1179` already correctly checks the op= code to decide between `DEBUG_BO_REGISTER` and `DEBUG_BO_UNREGISTER`, so ro= uting `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 =3D 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 id= iomatic and more concise. The existing `SYNC_DEBUG_BO` case just above demo= nstrates this =E2=80=94 it doesn't use `fallthrough` before its own handler= . This is a minor style point and shouldn't block the patch, but it would b= e cleaner without it. **Overall:** The fix is correct and important =E2=80=94 without it, debug B= O detach would always fail silently (well, with `-EINVAL`). Recommend mergi= ng with the optional style cleanup. --- Generated by Claude Code Patch Reviewer