From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm/amdgpu: Unify vm_fault_interrupt_state logic for gmc v10 and v12_0 Date: Mon, 25 May 2026 20:48:58 +1000 Message-ID: In-Reply-To: <20260520224217.17615-1-ryan.oliveiraf@usp.br> References: <20260520224217.17615-1-ryan.oliveiraf@usp.br> <20260520224217.17615-1-ryan.oliveiraf@usp.br> 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 extracted function is a faithful copy of the cod= e removed from both files. The `.set` callback pointer swap is correct. **Issue 1 =E2=80=94 Co-developed-by / Signed-off-by ordering (must fix)** The current tag ordering is: ``` Signed-off-by: Ryan Oliveira Co-developed-by: Kaiky Cintra Signed-off-by: Kaiky Cintra ``` Per `Documentation/process/submitting-patches.rst`, the submitter's (From: = author's) `Signed-off-by` must be last, and each `Co-developed-by` must be = immediately followed by the corresponding `Signed-off-by`. The correct orde= ring is: ``` Co-developed-by: Kaiky Cintra Signed-off-by: Kaiky Cintra Signed-off-by: Ryan Oliveira ``` **Issue 2 =E2=80=94 Extra blank line (nit)** There's a double blank line between the new function and `amdgpu_gmc_ras_sw= _init`: ```c return 0; } int amdgpu_gmc_ras_sw_init(struct amdgpu_device *adev) ``` Kernel style uses a single blank line between functions. **Issue 3 =E2=80=94 gmc_v11_0 has a near-identical copy that could also be = consolidated (suggestion)** `gmc_v11_0.c` has an almost identical implementation, differing only in the= `DISABLE` case where it adds an extra condition: ```c // gmc_v11_0 DISABLE case: if (!adev->in_s0ix && (adev->in_runpm || adev->in_suspend || amdgpu_in_reset(adev))) amdgpu_gmc_set_vm_fault_masks(adev, AMDGPU_GFXHUB(0), false); // gmc_v10_0 / gmc_v12_0 (and the new shared helper) DISABLE case: if (!adev->in_s0ix) amdgpu_gmc_set_vm_fault_masks(adev, AMDGPU_GFXHUB(0), false); ``` The ENABLE paths are identical across all three. This isn't a blocker for t= his patch, but the commit message could briefly note why gmc_v11_0 was inte= ntionally excluded (different DISABLE logic) so future readers don't wonder= . It's also worth considering whether gmc_v11_0 could be consolidated with = a flag or callback parameter, but that's a separate patch. Note: `gmc_v12_1`, `gmc_v9_0`, and the older versions (v6/v7/v8) all have s= tructurally different implementations (register-level iteration, different = hub handling), so they are correctly left untouched. **Summary:** Correct and useful deduplication. Fix the SoB ordering and the= extra blank line, and optionally note why gmc_v11_0 was left out. --- Generated by Claude Code Patch Reviewer