From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: accel/amdxdna: Create shared functions for AIE2 and AIE4
Date: Tue, 31 Mar 2026 17:05:08 +1000
Message-ID: <review-patch1-20260330163705.3153647-2-lizhi.hou@amd.com>
In-Reply-To: <20260330163705.3153647-2-lizhi.hou@amd.com>
References: <20260330163705.3153647-1-lizhi.hou@amd.com> <20260330163705.3153647-2-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

This is the refactoring foundation. Moves mailbox management channel, proto=
col checking, and feature mask into a new `struct aie_device` embedded in `=
amdxdna_dev_hdl`.

**Issue 1 =E2=80=94 Status check semantic change:**
```c
// Old (aie2_message.c):
#define DECLARE_AIE2_MSG(name, op) \
    DECLARE_XDNA_MSG_COMMON(name, op, MAX_AIE2_STATUS_CODE)
...
if (!ret && *hdl->status !=3D AIE2_STATUS_SUCCESS) {

// New (aie.h / aie.c):
#define DECLARE_AIE_MSG(name, op) \
    DECLARE_XDNA_MSG_COMMON(name, op, -1)
...
if (!ret && *hdl->status) {
```
The old code checked against `AIE2_STATUS_SUCCESS` (0) explicitly with a `M=
AX_AIE2_STATUS_CODE` sentinel for the notify callback. The new code uses `-=
1` as sentinel and checks `*hdl->status` (truthy =3D nonzero =3D error). Th=
is happens to be functionally equivalent if `AIE2_STATUS_SUCCESS =3D=3D 0`,=
 but merging the AIE2 path into a generic check that uses `-1` is a subtle =
behavioral change that should be called out in the commit message.

**Issue 2 =E2=80=94 `fw_feature_tbl` moved to `amdxdna_dev_info`:**
Moving `fw_feature_tbl` from `amdxdna_dev_priv` to `amdxdna_dev_info` is a =
layering change. The code in `aie_check_protocol()` accesses it via `aie->x=
dna->dev_info->fw_feature_tbl`. This is fine but the commit message doesn't=
 mention this relocation.

**Issue 3 =E2=80=94 Extra blank line removal:**
```c
-
  	op =3D amdxdna_cmd_get_op(cmd_abo);
```
Unrelated whitespace cleanup snuck into this patch (line ~891 area). Should=
 be in a separate patch or at least noted.

**Minor: `drm_WARN_ON` in `aie_destroy_chann`** takes a `**chann` double po=
inter, which is a reasonable pattern for NULL-out-on-destroy but differs fr=
om the original which operated directly on `ndev->mgmt_chann`.

---
Generated by Claude Code Patch Reviewer