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: Initial support for AIE4 platform Date: Tue, 31 Mar 2026 17:05:08 +1000 Message-ID: In-Reply-To: <20260330163705.3153647-1-lizhi.hou@amd.com> References: <20260330163705.3153647-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 Overall Series Review Subject: accel/amdxdna: Initial support for AIE4 platform Author: Lizhi Hou Patches: 10 Reviewed: 2026-03-31T17:05:08.529572 --- This 6-patch series adds initial AIE4 (NPU3) Physical Function support with= SR-IOV to the amdxdna accel driver (PCI IDs 0x17F2 and 0x1B0B). The approa= ch is to refactor existing AIE2-specific mailbox, PSP, and SMU code into sh= ared "aie_" common code, then build AIE4 support on top. **Overall assessment: The refactoring approach is sound**, and the series i= s structured logically (common code first, then AIE4 specifics). However, t= here are several issues: 1. **Two `struct amdxdna_dev_hdl` definitions** =E2=80=94 AIE2 (`aie2_pci.h= `) and AIE4 (`aie4_pci.h`) define completely different structs with the sam= e name. This works because they're compiled in separate translation units, = but it's fragile and makes the codebase confusing. A more scalable approach= would use different names or a proper inheritance pattern. 2. **Two `struct amdxdna_dev_priv` definitions** =E2=80=94 same problem. Bo= th `aie2_pci.h` and `aie4_pci.h` define this struct differently. 3. **Behavioral change in status checking** =E2=80=94 The old `DECLARE_AIE2= _MSG` used `MAX_AIE2_STATUS_CODE` for the status sentinel, the new `DECLARE= _AIE_MSG` uses `-1`. This changes how success/failure is determined in `aie= _send_mgmt_msg_wait()`. 4. **UAPI gap** =E2=80=94 `AMDXDNA_DEV_TYPE_PF =3D 2` skips value 1, which = should be explicitly documented. 5. **Missing `sriov_configure` in existing ops** =E2=80=94 NULL dereference= is avoided by a check, but the ops struct lacks the field declaration in t= he same patch that adds it. --- --- Generated by Claude Code Patch Reviewer