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: Use a different name for latest firmware
Date: Fri, 27 Feb 2026 12:53:21 +1000
Message-ID: <review-patch1-20260225204752.2711734-1-lizhi.hou@amd.com>
In-Reply-To: <20260225204752.2711734-1-lizhi.hou@amd.com>
References: <20260225204752.2711734-1-lizhi.hou@amd.com> <20260225204752.2711734-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

**1. Missing `const` qualifiers on `npu_fw` array**

```c
static char *npu_fw[] =3D {
	"npu_7.sbin",
	"npu.sbin"
};
```

This should be `static const char * const npu_fw[]`. Both the pointers and =
the pointed-to strings are string literals and should never be modified. Th=
e kernel's `-Wwrite-strings` warning and general const-correctness conventi=
ons expect this.

**2. Misleading error message after `fw_path` semantic change**

The `fw_path` field has been changed from a full firmware path (e.g., `"amd=
npu/1502_00/npu.sbin"`) to a directory prefix (e.g., `"amdnpu/1502_00/"`). =
But the error message after the loop is unchanged:

```c
if (ret) {
    XDNA_ERR(xdna, "failed to request_firmware %s, ret %d",
             ndev->priv->fw_path, ret);
```

This will now print something like `"failed to request_firmware amdnpu/1502=
_00/, ret -2"` which is incomplete =E2=80=94 it doesn't tell the user which=
 firmware files were actually attempted. Consider logging all attempted nam=
es, e.g.:

```c
XDNA_ERR(xdna, "failed to load firmware from %s (tried %s, %s), ret %d",
         ndev->priv->fw_path, npu_fw[0], npu_fw[1], ret);
```

Or alternatively, log each individual failure at `dev_dbg` level inside the=
 loop.

**3. Missing `MODULE_FIRMWARE` for `17f0_20/npu_7.sbin`**

The existing code declares `MODULE_FIRMWARE("amdnpu/17f0_20/npu.sbin")` but=
 the patch does not add a corresponding `MODULE_FIRMWARE("amdnpu/17f0_20/np=
u_7.sbin")`. Since the firmware loading loop is global (applies to all NPU =
variants), any device using the `17f0_20` directory will also attempt to lo=
ad `npu_7.sbin`. The `MODULE_FIRMWARE` declarations should match what the d=
river may try to load so that packaging tools (like `dracut`, `mkinitcpio`)=
 can include the right files. If `17f0_20` genuinely does not have an `npu_=
7.sbin` firmware, a comment explaining why would be helpful.

**4. `XDNA_INFO` on every successful probe is noisy**

```c
if (!ret) {
    XDNA_INFO(xdna, "Load firmware %s%s", ndev->priv->fw_path, npu_fw[i]);
    break;
}
```

This prints an info-level message on every successful device probe. Typical=
ly firmware load success is logged at debug level (or not at all, since `re=
quest_firmware` already logs). Consider using `XDNA_DBG` instead, or only l=
ogging when the fallback firmware is used (i.e., `i > 0`), which is the act=
ually interesting case.

**5. `fw_path` field name no longer reflects its meaning**

The struct field `fw_path` in `struct amdxdna_dev_priv` (defined in `aie2_p=
ci.h:249`) previously held a full firmware path and now holds a directory p=
refix. The name `fw_path` is now misleading. Consider renaming it to `fw_di=
r` or adding a comment at the struct definition to clarify the new semantic=
s:

```c
const char  *fw_path;  /* firmware directory prefix, e.g. "amdnpu/1502_00/"=
 */
```

**6. Minor: `fw_full_path` string is constructed twice**

The firmware path string is constructed with `kasprintf` and freed, then th=
e same concatenation is repeated in the `XDNA_INFO` format string. This is =
fine functionally, but you could move the `XDNA_INFO` before the `kfree` an=
d just print `fw_full_path` for cleaner code:

```c
ret =3D firmware_request_nowarn(&fw, fw_full_path, &pdev->dev);
if (!ret) {
    XDNA_INFO(xdna, "Loaded firmware %s", fw_full_path);
    kfree(fw_full_path);
    break;
}
kfree(fw_full_path);
```

**Overall**: The fix approach is sound =E2=80=94 using a separate firmware =
filename with fallback is a clean way to handle the compatibility issue. Th=
e issues above are all addressable in a V3. The `const` fix and the error m=
essage fix are the most important; the rest are improvements.

---
Generated by Claude Code Patch Reviewer