Claude review: accel/amdxdna: Support retrieving hardware context debug information

[Claude Code Review Bot <claude-review@example.com>]

Patch Review

**1. Large stack allocation of `app_health_report` in `aie2_hwctx_status_cb`**

In `aie2_pci.c`, the patched `aie2_hwctx_status_cb` declares:

```c
struct app_health_report report;
```

The `app_health_report` struct embeds `struct fatal_error_info` which contains `__u32 reserved[128]` — 512 bytes of reserved space. The total struct is ~560 bytes on the kernel stack. This is called from an IOCTL path that already has other stack frames. While not immediately fatal, this is uncomfortably large for kernel stack usage and could trigger `checkstack` / `CONFIG_FRAME_WARN` warnings.

**Recommendation:** Dynamically allocate `report` with `kzalloc_obj` (as is already done in `aie2_sched_job_timedout`), or reduce the `reserved[128]` to something more reasonable if the firmware protocol allows it.

**2. Missing cache invalidation after DMA in `aie2_query_app_health`**

```c
drm_clflush_virt_range(buf, sizeof(*report));
ret = aie2_send_mgmt_msg_wait(ndev, &msg);
if (ret) {
    ...
    goto free_buf;
}

/* Copy the report to caller's buffer */
memcpy(report, buf, sizeof(*report));
```

The `drm_clflush_virt_range` call before `aie2_send_mgmt_msg_wait` flushes stale cache lines so the device gets a clean buffer. However, after the device writes to the buffer via DMA (`DMA_FROM_DEVICE`), there is no `drm_clflush_virt_range` or `dma_sync_single_for_cpu` call before `memcpy` reads the buffer. On architectures with non-coherent DMA (which this uses via `dma_alloc_noncoherent`), the CPU could read stale cached data instead of what the firmware wrote.

Looking at other similar patterns in this driver (e.g., `aie2_query_aie_status` at `aie2_message.c`), I see the same pattern is used — flush before send, read after wait. So this may be a pre-existing issue in the driver, or the platform may guarantee coherency through other means. Nonetheless, for correctness, a cache invalidation before the `memcpy` would be prudent:

```c
drm_clflush_virt_range(buf, sizeof(*report));
memcpy(report, buf, sizeof(*report));
```

**3. Firmware query under `dev_lock` in `aie2_hwctx_status_cb`**

The `aie2_hwctx_status_cb` is called from `aie2_get_hwctx_status`, which holds `xdna->dev_lock` and iterates over all clients' hardware contexts:

```c
drm_WARN_ON(&xdna->ddev, !mutex_is_locked(&xdna->dev_lock));
...
list_for_each_entry(tmp_client, &xdna->client_list, node) {
    ret = amdxdna_hwctx_walk(tmp_client, &array_args,
                             aie2_hwctx_status_cb);
```

Each invocation of the callback now issues a synchronous firmware mailbox command (`aie2_query_app_health` → `aie2_send_mgmt_msg_wait`). With many contexts, this could hold `dev_lock` for an extended period, blocking other operations. Consider whether this query can be done outside the lock, or batched.

**4. `aie2_set_cmd_timeout` passes size with potentially NULL pointer**

```c
aie2_health = kzalloc_obj(*aie2_health);
if (!aie2_health)
    goto set_timeout;
...
set_timeout:
amdxdna_cmd_set_error(cmd_abo, job, fail_cmd_idx, ERT_CMD_STATE_TIMEOUT,
                      aie2_health, sizeof(*aie2_health));
```

When `aie2_health` is NULL (allocation failure or no report), `sizeof(*aie2_health)` is still passed. The callee checks `if (err_data)` so this is functionally safe, but it's misleading. The same applies when `report` (i.e., `job->priv`) is NULL — we jump to `set_timeout` with `aie2_health` still NULL but pass a non-zero size. Consider passing `0` for size when `aie2_health` is NULL for clarity, or restructure to only reach the `amdxdna_cmd_set_error` call once with proper arguments.

**5. `aie2_ctx_health` struct not packed**

The `struct aie2_ctx_health` in `aie2_ctx.c` is written into `cmd->data` which is shared with userspace:

```c
struct aie2_ctx_health {
    struct amdxdna_ctx_health header;
    u32 txn_op_idx;
    u32 ctx_pc;
    ...
};
```

Neither `aie2_ctx_health` nor `amdxdna_ctx_health` are marked `__packed`. While natural alignment of `u32` fields should make this consistent, for a structure that crosses the kernel/userspace boundary (written into a shared command buffer), explicit `__packed` or a static_assert on the layout would be safer to avoid any future surprises.

**6. Feature table update looks correct**

In `npu4_regs.c`:

```c
{ .features = BIT_U64(AIE2_APP_HEALTH), .major = 6, .min_minor = 18 },
{ .features = GENMASK_ULL(AIE2_APP_HEALTH, AIE2_NPU_COMMAND), .major = 7 },
```

The `GENMASK_ULL` is updated from `AIE2_TEMPORAL_ONLY` to `AIE2_APP_HEALTH` to include the new feature bit in the major=7 catch-all entry. This correctly assumes `AIE2_APP_HEALTH` is the highest enum value before `AIE2_FEATURE_MAX`, which it is.

**7. Memory lifecycle for `job->priv` looks correct**

The `report` allocated in `aie2_sched_job_timedout` is stored in `job->priv`, and `kfree(job->priv)` is added to `aie2_job_release`. The data is consumed synchronously in `aie2_set_cmd_timeout` (called from the response handler). This looks correct — allocation and free paths are balanced.

**8. Minor: `__u32` vs `u32` in firmware struct types**

The new structs in `aie2_msg_priv.h` use `__u32` / `__u16`:

```c
struct fatal_error_info {
    __u32 fatal_type;
    ...
};
```

These are kernel-internal firmware message structures, not UAPI. Other structs in the same file use plain types (e.g., `u32`, `u16`). The `__u32` types are conventionally reserved for UAPI headers. Consider using `u32` for consistency with the rest of the file.

---
Generated by Claude Code Patch Reviewer