public inbox for drm-ai-reviews@public-inbox.freedesktop.org
 help / color / mirror / Atom feed
* [PATCH] drm/amdgpu: remove dead empty checks in ring_mux ib_mark_offset and end_ib
@ 2026-05-21 13:35 Maoyi Xie
  2026-05-25 10:07 ` Claude review: " Claude Code Review Bot
  2026-05-25 10:07 ` Claude Code Review Bot
  0 siblings, 2 replies; 3+ messages in thread
From: Maoyi Xie @ 2026-05-21 13:35 UTC (permalink / raw)
  To: Alex Deucher, Christian König
  Cc: David Airlie, Simona Vetter, amd-gfx, dri-devel, linux-kernel

amdgpu_ring_mux_ib_mark_offset() and amdgpu_ring_mux_end_ib() read
e->list with list_last_entry() and then test the returned pointer
against NULL. list_last_entry() never returns NULL. On an empty
list it returns container_of(&e->list, struct amdgpu_mux_chunk,
entry), which is an aliased pointer derived from the list head.
The "cannot find chunk!" error path is dead code.

These callsites run on the software ring submission path, after
amdgpu_ring_mux_start_ib() has linked a fresh chunk into e->list.
By the IB API contract, mark_offset and end_ib only execute between
start_ib and the chunk's removal in scan_and_remove_signaled_chunk(),
so e->list is non-empty at the read. The defensive check is
misleading and never fires.

Drop the dead block on both sites. The list_last_entry() read stays.
If a future change broke the precondition, callers would crash on
the aliased pointer, which is louder than the current silent error
log and easier to debug.

Suggested-by: Christian König <christian.koenig@amd.com>
Signed-off-by: Maoyi Xie <maoyixie.tju@gmail.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ring_mux.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring_mux.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring_mux.c
index 6e64a96fa285..83d62d9f76d4 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring_mux.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring_mux.c
@@ -495,10 +495,6 @@ void amdgpu_ring_mux_ib_mark_offset(struct amdgpu_ring_mux *mux,
 	}
 
 	chunk = list_last_entry(&e->list, struct amdgpu_mux_chunk, entry);
-	if (!chunk) {
-		DRM_ERROR("cannot find chunk!\n");
-		return;
-	}
 
 	switch (type) {
 	case AMDGPU_MUX_OFFSET_TYPE_CONTROL:
@@ -528,10 +524,6 @@ void amdgpu_ring_mux_end_ib(struct amdgpu_ring_mux *mux, struct amdgpu_ring *rin
 	}
 
 	chunk = list_last_entry(&e->list, struct amdgpu_mux_chunk, entry);
-	if (!chunk) {
-		DRM_ERROR("cannot find chunk!\n");
-		return;
-	}
 
 	chunk->end = ring->wptr;
 	chunk->sync_seq = READ_ONCE(ring->fence_drv.sync_seq);
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Claude review: drm/amdgpu: remove dead empty checks in ring_mux ib_mark_offset and end_ib
  2026-05-21 13:35 [PATCH] drm/amdgpu: remove dead empty checks in ring_mux ib_mark_offset and end_ib Maoyi Xie
@ 2026-05-25 10:07 ` Claude Code Review Bot
  2026-05-25 10:07 ` Claude Code Review Bot
  1 sibling, 0 replies; 3+ messages in thread
From: Claude Code Review Bot @ 2026-05-25 10:07 UTC (permalink / raw)
  To: dri-devel-reviews

Overall Series Review

Subject: drm/amdgpu: remove dead empty checks in ring_mux ib_mark_offset and end_ib
Author: Maoyi Xie <maoyixie.tju@gmail.com>
Patches: 1
Reviewed: 2026-05-25T20:07:45.690741

---

This is a single cleanup patch that removes dead NULL checks after `list_last_entry()` calls. The analysis in the commit message is correct: `list_last_entry()` is a macro wrapping `list_entry((ptr)->prev, type, member)` (see `include/linux/list.h:630-631`), which is just `container_of()` — it never returns NULL. The NULL checks were dead code.

The commit message is well-written, explains the API contract that guarantees the list is non-empty at these call sites (`start_ib` adds a chunk before `mark_offset`/`end_ib` run), and correctly notes that a crash on an aliased pointer would be more debuggable than a silent `DRM_ERROR` log. The `Suggested-by: Christian König` tag indicates this was requested by the subsystem maintainer.

**Verdict: The patch is correct and ready to merge.**

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Claude review: drm/amdgpu: remove dead empty checks in ring_mux ib_mark_offset and end_ib
  2026-05-21 13:35 [PATCH] drm/amdgpu: remove dead empty checks in ring_mux ib_mark_offset and end_ib Maoyi Xie
  2026-05-25 10:07 ` Claude review: " Claude Code Review Bot
@ 2026-05-25 10:07 ` Claude Code Review Bot
  1 sibling, 0 replies; 3+ messages in thread
From: Claude Code Review Bot @ 2026-05-25 10:07 UTC (permalink / raw)
  To: dri-devel-reviews

Patch Review

**Correctness: Good.** The two removed blocks are identical in structure:

```c
chunk = list_last_entry(&e->list, struct amdgpu_mux_chunk, entry);
-	if (!chunk) {
-		DRM_ERROR("cannot find chunk!\n");
-		return;
-	}
```

`list_last_entry` expands to `container_of((ptr)->prev, type, member)` (`include/linux/list.h:630`), which performs pointer arithmetic and can never yield NULL. The check was always false — removing it is correct.

**Precondition argument: Sound.** The commit message claims `e->list` is guaranteed non-empty at both call sites. Looking at the post-patch code at `amdgpu_ring_mux.c:497` and `:526`, both functions first look up the mux entry via `amdgpu_ring_mux_sw_entry()` and bail on failure. The `list_last_entry` call only executes after that guard, on a ring that has gone through `start_ib` which links a chunk. This is a valid invariant.

**No functional change:** The removal only deletes unreachable code paths. The `list_last_entry()` call and all subsequent logic are unchanged.

**No issues found.** Clean, minimal patch with an accurate commit message. No concerns.

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2026-05-25 10:07 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-05-21 13:35 [PATCH] drm/amdgpu: remove dead empty checks in ring_mux ib_mark_offset and end_ib Maoyi Xie
2026-05-25 10:07 ` Claude review: " Claude Code Review Bot
2026-05-25 10:07 ` Claude Code Review Bot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox