From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: Re: [PATCH] drm/amd/amdgpu: remove duplicated code in gfx_v11 and gfx_v12 Date: Tue, 05 May 2026 10:53:47 +1000 Message-ID: In-Reply-To: References: X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Tag ordering (must fix):** The current trailer block is: ``` Signed-off-by: Ulisses Paixao Co-developed-by: Felipe Sousa Signed-off-by: Felipe Sousa ``` Per [Documentation/process/submitting-patches.rst](https://www.kernel.org/d= oc/html/latest/process/submitting-patches.html#when-to-use-acked-by-cc-and-= co-developed-by), every `Co-developed-by:` must be immediately followed by = the corresponding `Signed-off-by:`, and the submitter's `Signed-off-by:` mu= st be last. The correct ordering is: ``` Co-developed-by: Felipe Sousa Signed-off-by: Felipe Sousa Signed-off-by: Ulisses Paixao ``` **`BUG()` in default case (should fix):** ```c default: BUG(); break; ``` This is an interrupt handler context. `BUG()` will panic the kernel, which = is disproportionate for receiving an unexpected `me_id` value. The newer `g= fx_v12_1_handle_priv_fault` already handles this more gracefully: ```c default: dev_dbg(adev->dev, "Unexpected me %d in priv_fault\n", me_id); break; ``` Since this function is being promoted to shared infrastructure, this is the= right time to replace `BUG()` with `WARN_ONCE()` or `dev_err()` =E2=80=94 = something that logs the problem without crashing the machine. **Incomplete consolidation (minor, worth mentioning in commit message):** The tree also has `handle_priv_fault` in: - `gfx_v10_0.c` =E2=80=94 nearly identical, only missing the `!adev->gfx.di= sable_kq` guard - `gfx_v12_1.c` =E2=80=94 different (has `xcc_id` / multi-XCC logic, no gfx= ring case) The commit message should mention why these aren't included. For `gfx_v10_0= `, the `disable_kq` guard is likely just a missing backport =E2=80=94 wrapp= ing it in the same shared function (which has the guard) might even be a bu= gfix. Worth investigating. **Kernel-doc comment trailing whitespace:** ```c + * amdgpu_gfx_handle_priv_fault - Handle privileged instruction fault + *=20 + * @adev: amdgpu_device pointer + * @entry: interrupt vector entry from the hardware + *=20 ``` Lines 3 and 5 of the doc comment (the blank lines marked with ` * `) have t= railing whitespace after the `*`. These should be just ` *` with no trailin= g space. Some CI checks flag this. **Code correctness:** The consolidated function itself is a faithful copy of the originals. The e= xtraction into `amdgpu_gfx.c` is placed logically after `amdgpu_gfx_enable_= kgq()` near other queue-management code. The declaration in `amdgpu_gfx.h` = is placed alphabetically near related functions. The callers in both `gfx_v= 11_0.c` and `gfx_v12_0.c` are updated correctly across all three IRQ handle= rs (`priv_reg_irq`, `bad_op_irq`, `priv_inst_irq`). No functional change for v11 and v12 =E2=80=94 this is a clean refactor. --- Generated by Claude Code Patch Reviewer