Claude review: accel/amdxdna: Block running when IOMMU is off

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

Patch Review

**Correctness of the check:** The patch checks `!xdna->group` in `aie2_init()`. Looking at `amdxdna_iommu_init()` in `amdxdna_iommu.c:161`, `xdna->group` is set via `iommu_group_get()` — it will be NULL when no IOMMU is present. The `amdxdna_iommu_init()` call happens at `amdxdna_pci_drv.c:374` **before** `xdna->dev_info->ops->init(xdna)` at line 385, which dispatches to `aie2_init()`. So by the time `aie2_init()` runs, `xdna->group` is already populated (or NULL). The check is valid.

**Placement concern:** The check is placed inside the AIE2-specific `aie2_init()`, which means it only protects AIE2 devices. That seems intentional per the commit message ("The AIE2 device firmware requires IOMMU on"), and I confirmed that `aie4_pci.c` has no similar check and no hypervisor check either, so this is AIE2-specific policy. That's fine.

**Return code:** The patch returns `-EINVAL`, but `-ENODEV` or `-EOPNOTSUPP` might be more semantically appropriate — the hardware is present but lacks a required system feature (IOMMU). `-EINVAL` typically means "bad argument" which doesn't describe the situation well. That said, the existing hypervisor check on the line just above also returns `-EINVAL`, so at least it's consistent within the function.

**Context mismatch:** The patch's diff context shows the new check being added *after* the hypervisor check and *before* the `ndev = drmm_kzalloc(...)` allocation. But in the drm-next tree, the `ndev` allocation is already at line 493, immediately after the hypervisor check with no blank line gap for the patch's `@@ -490,6 +490,11 @@` hunk to land cleanly. This explains why the patches didn't apply — the base the patch was developed against differs from drm-next.

**Error message:** The message `"Running without IOMMU not supported"` is adequate but could be slightly more informative, e.g., `"AIE2 firmware requires IOMMU to be enabled"` to help users understand what to fix.

**Minor style observation:** Checking `xdna->group` as a proxy for "IOMMU is available" works because `iommu_group_get()` returns NULL when there's no IOMMU. However, this is an indirect test — it relies on the fact that `amdxdna_iommu_init()` always runs first and sets `xdna->group`. If that initialization order ever changes, this check would silently break. A brief comment could help, but the ordering is well-established in `amdxdna_pci_drv.c` so this is a very low risk.

**Overall:** This is a straightforward, low-risk patch. The main suggestion would be to consider `-ENODEV` or `-EOPNOTSUPP` instead of `-EINVAL`, though consistency with the adjacent hypervisor check is a reasonable argument for keeping it. No bugs found.

---
Generated by Claude Code Patch Reviewer