From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm/gem/shmem: Introduce __drm_gem_shmem_free_sgt_locked() Date: Thu, 23 Apr 2026 08:09:05 +1000 Message-ID: In-Reply-To: <20260421234234.638503-3-lyude@redhat.com> References: <20260421234234.638503-1-lyude@redhat.com> <20260421234234.638503-3-lyude@redhat.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit MIME-Version: 1.0 Patch Review **Concern: Missing NULL check on `shmem->sgt`.** The extracted function unconditionally dereferences `shmem->sgt`: ```c void __drm_gem_shmem_free_sgt_locked(struct drm_gem_shmem_object *shmem) { dma_resv_assert_held(shmem->base.resv); dma_unmap_sgtable(shmem->base.dev->dev, shmem->sgt, DMA_BIDIRECTIONAL, 0); sg_free_table(shmem->sgt); kfree(shmem->sgt); shmem->sgt = NULL; } ``` The original code in `drm_gem_shmem_release()` wrapped this with `if (shmem->sgt)`. The call site in `drm_gem_shmem_release` still checks `if (shmem->sgt)` before calling the new function, and the Rust callers (patch 4) also only call it when the sgt is known-valid. So this is safe in practice, but since the function is exported and documented, it would be more defensive to either: - Add a NULL check inside the function, or - Document in the kdoc that `shmem->sgt` must be non-NULL. Currently the kdoc says "If the passed shmem object has an active scatter/gather table..." which implies it handles the NULL case, but it doesn't. **Minor: Doc/function name mismatch in commit message.** The commit message references `__drm_gem_shmem_release_sgt_locked()` in the V10 changelog, but the actual function is `__drm_gem_shmem_free_sgt_locked()`. This is just the changelog, not the code, so it's a cosmetic issue only. --- --- Generated by Claude Code Patch Reviewer