From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: accel/amdxdna: Add AIE4 firmware loading
Date: Tue, 31 Mar 2026 17:05:09 +1000 [thread overview]
Message-ID: <review-patch4-20260330163705.3153647-5-lizhi.hou@amd.com> (raw)
In-Reply-To: <20260330163705.3153647-5-lizhi.hou@amd.com>
Patch Review
Adds CERT firmware support to PSP, AIE4 firmware loading paths, and NPU3 PSP register definitions.
**Issue 1 — Bisection fix for patch 3:**
```c
+ psp_conf.arg2_mask = GENMASK(23, 0);
+ psp_conf.notify_val = 1;
```
This is added in `aie2_pci.c`, fixing the bisection issue from patch 3. These two lines should have been in patch 3.
**Issue 2 — `PSP_SET_CMD` macro readability:**
```c
#define PSP_SET_CMD(psp, reg_vals, cmd, arg0, arg1, arg2) \
({ \
...
_regs[3] = ((arg2) | ((_cmd) << 24)) & (psp)->conf.arg2_mask; \
})
```
The `arg2` register now encodes both `arg2` data AND the command shifted left by 24 bits, masked by a per-device mask. For AIE2, `arg2_mask = GENMASK(23, 0)` strips the command bits (so `arg2` register = just the size, backwards compatible). For AIE4, `arg2_mask = ~0` keeps command bits. This is clever but should be documented in a comment explaining the protocol difference.
**Issue 3 — PSP register aliasing for NPU3:**
```c
DEFINE_BAR_OFFSET(PSP_CMD_REG, NPU3_PSP, MPASP_C2PMSG_123_ALT_1),
...
DEFINE_BAR_OFFSET(PSP_ARG2_REG, NPU3_PSP, MPASP_C2PMSG_123_ALT_1),
...
DEFINE_BAR_OFFSET(PSP_STATUS_REG, NPU3_PSP, MPASP_C2PMSG_123_ALT_1),
```
Three registers (`PSP_CMD_REG`, `PSP_ARG2_REG`, `PSP_STATUS_REG`) all map to the same physical address `MPASP_C2PMSG_123_ALT_1`. Similarly `PSP_ARG0_REG` and `PSP_RESP_REG` share `MPASP_C2PMSG_156_ALT_1`. This means writes to CMD, ARG2, and reads from STATUS all hit the same MMIO register. This is a fundamental protocol difference from AIE2 and should be documented, as it means the register serves different purposes depending on direction (write for CMD/ARG2, read for STATUS).
**Issue 4 — `virt_to_phys` usage:**
```c
*paddr = virt_to_phys(buffer);
```
`virt_to_phys` on `kmalloc`/`drmm_kmalloc` memory is technically valid but fragile. The existing AIE2 code already does this, so this isn't new, but it's worth noting that this won't work on systems with IOMMU remapping if the PSP can't bypass it. This appears to be an existing design choice.
**Issue 5 — psp_conf stored by value:**
```c
memcpy(&psp->conf, conf, sizeof(psp->conf));
```
`psp->conf` stores `fw_buf` and `certfw_buf` pointers. After `aie4_release_firmware()` is called, these pointers become dangling. This is fine if the firmware data was already copied into the aligned buffers, but the pointers in `conf` should not be accessed after this point. The code doesn't seem to use them after `aiem_psp_create`, but storing dangling pointers is a latent bug.
---
Generated by Claude Code Patch Reviewer
next prev parent reply other threads:[~2026-03-31 7:05 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-03-30 16:36 [PATCH V1 0/6] accel/amdxdna: Initial support for AIE4 platform Lizhi Hou
2026-03-30 16:37 ` [PATCH V1 1/6] accel/amdxdna: Create shared functions for AIE2 and AIE4 Lizhi Hou
2026-03-31 7:05 ` Claude review: " Claude Code Review Bot
2026-03-30 16:37 ` [PATCH V1 2/6] accel/amdxdna: Add basic support for AIE4 devices Lizhi Hou
2026-03-31 7:05 ` Claude review: " Claude Code Review Bot
2026-03-30 16:37 ` [PATCH V1 3/6] accel/amdxdna: Create common PSP interfaces for AIE2 and AIE4 Lizhi Hou
2026-03-31 7:05 ` Claude review: " Claude Code Review Bot
2026-03-30 16:37 ` [PATCH V1 4/6] accel/amdxdna: Add AIE4 firmware loading Lizhi Hou
2026-03-30 20:17 ` Mario Limonciello
2026-03-30 20:30 ` yidong Zhang
2026-03-31 2:45 ` Mario Limonciello
2026-03-31 7:05 ` Claude Code Review Bot [this message]
2026-03-30 16:37 ` [PATCH V1 5/6] accel/amdxdna: Create common SMU interfaces for AIE2 and AIE4 Lizhi Hou
2026-03-31 7:05 ` Claude review: " Claude Code Review Bot
2026-03-30 16:37 ` [PATCH V1 6/6] accel/amdxdna: Add AIE4 power on and off support Lizhi Hou
2026-03-31 7:05 ` Claude review: " Claude Code Review Bot
2026-03-31 7:05 ` Claude review: accel/amdxdna: Initial support for AIE4 platform Claude Code Review Bot
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=review-patch4-20260330163705.3153647-5-lizhi.hou@amd.com \
--to=claude-review@example.com \
--cc=dri-devel-reviews@example.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox