From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: rust: drm: gem: Add vmap functions to shmem bindings Date: Thu, 04 Jun 2026 16:06:44 +1000 Message-ID: In-Reply-To: <20260529183702.677677-4-lyude@redhat.com> References: <20260529183702.677677-1-lyude@redhat.com> <20260529183702.677677-4-lyude@redhat.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 **Status: Looks good overall, minor notes** This is the largest patch in the series. Key design decisions are sound: **VMap lifecycle**: The `make_vmap` method acquires the DMA reservation loc= k, calls `drm_gem_shmem_vmap_locked`, then explicitly drops the guard befor= e potentially calling `raw_vunmap` (which re-acquires the lock). This avoid= s deadlock: ```rust let guard =3D DmaResvGuard::new(self); to_result(unsafe { bindings::drm_gem_shmem_vmap_locked(self.as_raw_shmem(), map.as_mut_ptr= ()) })?; drop(guard); ``` **iomem rejection**: The iomem path correctly unmaps before returning an er= ror: ```rust if map.is_iomem { unsafe { self.raw_vunmap(map) }; Err(ENOTSUPP) } ``` **SGTableMap and Devres interaction**: The `SGTableMap` is wrapped in `Devr= es` and stored in a `LazyInit`. On device unbind, Devres drops the SGTableM= ap which calls `__drm_gem_shmem_free_sgt_locked()` and sets `shmem->sgt =3D= NULL`. In `free_callback`, `sgt_res.reset()` is called before `drm_gem_shm= em_release()`, ensuring no double-free since `drm_gem_shmem_release()` chec= ks `if (shmem->sgt)`. The reordering in `free_callback` is critical and correct =E2=80=94 `sgt_re= s` must be reset while the GEM object's dma_resv is still valid: ```rust unsafe { Pin::new_unchecked(&mut (*this).sgt_res) }.reset(); unsafe { bindings::drm_gem_shmem_release(base) }; let _ =3D unsafe { KBox::from_raw(this) }; ``` **Device validation in `sg_table()`**: The check that the caller's device m= atches the GEM object's device is a good safety measure: ```rust if dev.as_raw() !=3D self.dev().as_ref().as_raw() { return Err(EINVAL); } ``` **Test coverage**: The kunit tests are well-structured, covering size valid= ation, IO read/write, and wrong-device rejection. Minor observation on the vmap_io test: the test uses `0xFFFFFFFF` for write= 32 and checks individual bytes are all `0xFF` =E2=80=94 this works regardle= ss of endianness, which is nice. --- Generated by Claude Code Patch Reviewer