From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: accel/amdxdna: Block running when IOMMU is off Date: Mon, 25 May 2026 21:10:20 +1000 Message-ID: In-Reply-To: <20260520223531.1403302-1-lizhi.hou@amd.com> References: <20260520223531.1403302-1-lizhi.hou@amd.com> <20260520223531.1403302-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 **Correctness of the check:** The patch checks `!xdna->group` in `aie2_init= ()`. Looking at `amdxdna_iommu_init()` in `amdxdna_iommu.c:161`, `xdna->gro= up` is set via `iommu_group_get()` =E2=80=94 it will be NULL when no IOMMU = is present. The `amdxdna_iommu_init()` call happens at `amdxdna_pci_drv.c:3= 74` **before** `xdna->dev_info->ops->init(xdna)` at line 385, which dispatc= hes to `aie2_init()`. So by the time `aie2_init()` runs, `xdna->group` is a= lready populated (or NULL). The check is valid. **Placement concern:** The check is placed inside the AIE2-specific `aie2_i= nit()`, which means it only protects AIE2 devices. That seems intentional p= er 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 e= ither, so this is AIE2-specific policy. That's fine. **Return code:** The patch returns `-EINVAL`, but `-ENODEV` or `-EOPNOTSUPP= ` might be more semantically appropriate =E2=80=94 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 existi= ng hypervisor check on the line just above also returns `-EINVAL`, so at le= ast it's consistent within the function. **Context mismatch:** The patch's diff context shows the new check being ad= ded *after* the hypervisor check and *before* the `ndev =3D 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 =E2=80=94 the base the patch was developed aga= inst differs from drm-next. **Error message:** The message `"Running without IOMMU not supported"` is a= dequate but could be slightly more informative, e.g., `"AIE2 firmware requi= res IOMMU to be enabled"` to help users understand what to fix. **Minor style observation:** Checking `xdna->group` as a proxy for "IOMMU i= s available" works because `iommu_group_get()` returns NULL when there's no= IOMMU. However, this is an indirect test =E2=80=94 it relies on the fact t= hat `amdxdna_iommu_init()` always runs first and sets `xdna->group`. If tha= t initialization order ever changes, this check would silently break. A bri= ef 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`, thou= gh consistency with the adjacent hypervisor check is a reasonable argument = for keeping it. No bugs found. --- Generated by Claude Code Patch Reviewer