From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: misc: fastrpc: Remove buffer from list prior to unmap operation Date: Thu, 04 Jun 2026 13:11:15 +1000 Message-ID: In-Reply-To: <20260602071750.526-3-jianping.li@oss.qualcomm.com> References: <20260602071750.526-1-jianping.li@oss.qualcomm.com> <20260602071750.526-3-jianping.li@oss.qualcomm.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit MIME-Version: 1.0 Patch Review **Purpose:** Fix a race condition where two concurrent unmap calls on the same buffer could both find it in the list and proceed concurrently. **Analysis:** The current code finds the buffer in the list under `fl->lock`, drops the lock, calls `fastrpc_req_munmap_impl` (which does the DSP unmap), and only then removes from the list inside `fastrpc_req_munmap_impl`. Two threads could both find the same buffer and both attempt to unmap it. The fix moves `list_del` into `fastrpc_req_munmap` (under the existing `fl->lock`) *before* calling `fastrpc_req_munmap_impl`. If the unmap fails, the buffer is re-added: ```c + err = fastrpc_req_munmap_impl(fl, buf); + if (err) { + spin_lock(&fl->lock); + list_add_tail(&buf->node, &fl->mmaps); + spin_unlock(&fl->lock); + } ``` The corresponding `list_del` + `spin_unlock` in `fastrpc_req_munmap_impl` is removed, and on success it just calls `fastrpc_buf_free(buf)`. **Minor note:** After the `list_del` in `fastrpc_req_munmap`, `buf->node` is in an undefined state. The `list_add_tail` in the error path works here because `list_del` doesn't corrupt the node, but using `list_del_init` would be slightly safer and more idiomatic for the "remove, try, re-add on failure" pattern. Not a blocker. **Verdict:** Correct fix for a real race. The re-add on failure approach is reasonable. --- --- Generated by Claude Code Patch Reviewer