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: Preserve user address when PASID is disabled Date: Thu, 04 Jun 2026 13:24:31 +1000 Message-ID: In-Reply-To: <20260602040624.2206774-1-lizhi.hou@amd.com> References: <20260602040624.2206774-1-lizhi.hou@amd.com> <20260602040624.2206774-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: The fix is correct.** Setting `abo->mem.uva =3D addr` before= the early return ensures that `amdxdna_gem_uva()` returns a valid address = for heap validation in `amdxdna_init_dev_bo()`, regardless of PASID state. **Minor observations (non-blocking):** 1. **Cleanup asymmetry on munmap.** In the PASID-on path, `uva` is reset to= `AMDXDNA_INVALID_ADDR` when the last umap entry is removed (`amdxdna_umap_= release()` at `amdxdna_gem.c:322`). In the no-PASID path, no `mapp` is crea= ted and no entry is added to `umap_list`, so `uva` is never reset after mun= map =E2=80=94 it persists until the BO is freed. This means `amdxdna_gem_uv= a()` could return a stale address for an unmapped BO in the no-PASID case. = In practice this is likely harmless because `amdxdna_obj_dma_addr()` uses `= abo->mem.dma_addr` (not `uva`) when PASID is off, and heap validation would= only occur during subsequent `amdxdna_init_dev_bo()` calls. But it's worth= calling out. 2. **Repeated mmap behavior.** In the PASID-on path, `uva` is only set on t= he first mmap (guarded by `list_empty(&abo->mem.umap_list)` at line 389). W= ith this patch, the no-PASID path sets `uva` on every mmap call uncondition= ally. For heap BOs this is fine since they're typically mmapped once, but i= t's a subtle behavioral difference. 3. **Locking.** The PASID-on path writes `uva` under `notifier_lock` (line = 388=E2=80=93392). This patch writes `uva` without the lock. Since the no-PA= SID path doesn't manipulate `umap_list` and mmap is serialized by the mm's = `mmap_lock`, this is safe in practice. The header comment (amdxdna_gem.h:33= =E2=80=9336) already states `uva` can be accessed without `notifier_lock`. **Verdict:** The patch is a clear bug fix with a correct Fixes tag. The asy= mmetries noted above are pre-existing design characteristics of the PASID-o= n vs PASID-off paths rather than issues introduced by this patch. Reviewed-by tag warranted. --- Generated by Claude Code Patch Reviewer