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: Remove mmap and export support for ubuf Date: Sat, 16 May 2026 08:56:26 +1000 Message-ID: In-Reply-To: <20260515155332.743097-1-lizhi.hou@amd.com> References: <20260515155332.743097-1-lizhi.hou@amd.com> <20260515155332.743097-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 **Dead code: `amdxdna_ubuf_vm_ops` and `amdxdna_ubuf_vm_fault` are now orph= aned.** The patch removes `amdxdna_ubuf_mmap` which was the sole user of `amdxdna_u= buf_vm_ops`, which in turn was the sole user of `amdxdna_ubuf_vm_fault`. Af= ter this patch, both `amdxdna_ubuf_vm_fault` (line 72) and `amdxdna_ubuf_vm= _ops` (line 86) in `amdxdna_ubuf.c` are dead code. They should be removed i= n this patch to avoid a compiler warning (`-Wunused-function` for the stati= c `amdxdna_ubuf_vm_fault`). This will actually cause a build failure with `= -Werror`, which the kernel uses. **Flag naming: `pri` is cryptic and misleading.** The new field: ```c /* True, if BO is not exportable */ bool pri; ``` The name `pri` doesn't convey "not exportable." It could be misread as "pri= ority" or "primary" or "private." A name like `no_export` or `unexportable`= would be immediately clear. Even `private` (spelled out) would be better. = The comment helps, but a self-documenting name costs nothing. **The export-blocking logic is correct but could be more robust.** The check in `amdxdna_gem_prime_export`: ```c if (abo->pri) return ERR_PTR(-EOPNOTSUPP); ``` This correctly prevents re-export. The ubuf object is created through `amdx= dna_gem_prime_import` and then marked with `abo->pri =3D true`, which will = prevent it from being re-exported through the shmem funcs' `.export =3D amd= xdna_gem_prime_export` path. **No mmap blocking on the GEM object side.** The patch removes `amdxdna_ubuf_mmap` from the dma-buf ops, which prevents = mmap through the dma-buf fd. However, ubuf objects are imported via `drm_ge= m_shmem_prime_import_sg_table`, and the resulting GEM shmem object has `.mm= ap =3D amdxdna_gem_obj_mmap` set via `amdxdna_gem_shmem_funcs`. The commit = message says "ubuf pages should not be mmaped" =E2=80=94 should `amdxdna_ge= m_obj_mmap` also check `abo->pri` and reject the mmap? If the intent is onl= y to block the dma-buf mmap path (since user already has the pages mapped),= this should be clarified in the commit message. If GEM mmap should also be= blocked, it's missing. **Summary:** - **Must fix:** Remove the now-dead `amdxdna_ubuf_vm_fault` and `amdxdna_ub= uf_vm_ops` =E2=80=94 this will cause a build warning/error. - **Should fix:** Rename `pri` to something self-documenting (`no_export`, = `unexportable`, etc.). - **Should clarify:** Whether GEM-object-level mmap should also be blocked = for ubuf objects, or only dma-buf mmap. If only dma-buf mmap, update the co= mmit message to be precise. --- Generated by Claude Code Patch Reviewer