From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: drm/amdgpu: fix sync handling in amdgpu_dma_buf_move_notify
Date: Wed, 11 Feb 2026 16:29:51 +1000
Message-ID: <review-patch1-20260210091417.1133-1-pierre-eric.pelloux-prayer@amd.com>
In-Reply-To: <20260210091417.1133-1-pierre-eric.pelloux-prayer@amd.com>
References: <20260210091417.1133-1-pierre-eric.pelloux-prayer@amd.com> <20260210091417.1133-1-pierre-eric.pelloux-prayer@amd.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit
X-Mailer: Claude Code Patch Reviewer

Patch Review

**Subject line**: Clear and descriptive. Follows subsystem conventions.

**Commit Message Analysis:**

The commit message effectively explains:
- **What**: Synchronization bug in dma-buf move notification
- **Why**: Passing ticket parameter caused premature page table updates
- **How**: Remove ticket parameter to force synchronous behavior
- **Example**: Detailed multi-GPU scenario with glxgears/Xorg

The scenario description is excellent - it provides concrete steps to reproduce and explains the race window clearly:

```
  drm_sched_job_run                       # GPU0, blit
  move linear buffer for GPU1 access      #
  amdgpu_dma_buf_move_notify -> update pt # GPU0
  
It this point the blit job on GPU0 is still running and would
likely produce a page fault.
```

**Minor nit**: "It this point" should be "At this point" (typo in line 412 of mbox).

**Code Review:**

```c
-		r = amdgpu_vm_handle_moved(adev, vm, ticket);
+		r = amdgpu_vm_handle_moved(adev, vm, NULL);
```

**Analysis:**

1. **Correctness**: The fix is correct. By passing NULL instead of `ticket`, the code forces `amdgpu_vm_handle_moved` to use the `clear=true` path, which ensures proper synchronization with in-flight command submissions from the same VM.

2. **Comment Quality**: The added comment is good but could be clearer:
```c
+		/* Don't pass 'ticket' to amdgpu_vm_handle_moved: we want the clear=true
+		 * path to be used otherwise we might update the PT of another process
+		 * while it's using the BO.
```

The phrase "update the PT of another process while it's using the BO" is slightly ambiguous. More precisely: we're updating the page table of the importing process (process B) while the exporting process (process A) still has GPU jobs using the old mapping.

3. **Fixes Tag**: Correct format and appropriate commit referenced:
```
Fixes: a448cb003edc ("drm/amdgpu: implement amdgpu_gem_prime_move_notify v2")
```

4. **Missing Stable Tag**: Christian König requested "CC: stable?" in the reply (line 417). This regression fix should include:
```
Cc: stable@vger.kernel.org
```

**Context Review (amdgpu_dma_buf.c:514)**:

Looking at the surrounding code:
```c
r = dma_resv_reserve_fences(resv, 2);
if (!r)
    r = amdgpu_vm_clear_freed(adev, vm, NULL);  // Already uses NULL

if (!r)
    r = amdgpu_vm_handle_moved(adev, vm, NULL); // Now also uses NULL
```

This creates symmetry - both calls now use NULL for the ticket parameter, which is logical since they're part of the same invalidation sequence.

**Potential Issues:**

1. **Performance Impact**: Forcing the synchronous path (clear=true) may have performance implications. However, correctness trumps performance, and cross-process shared buffer scenarios are typically not the fast path.

2. **Testing**: No information provided about:
   - How was this bug discovered?
   - Has the fix been tested with the glxgears/Xorg scenario described?
   - Are there any regressions?

**Review Feedback Items:**

1. **MUST FIX**: Add `Cc: stable@vger.kernel.org` tag as requested by maintainer
2. **SHOULD FIX**: Fix typo "It this point" → "At this point" in commit message
3. **NICE TO HAVE**: Add Tested-by tag if available
4. **NICE TO HAVE**: Clarify in commit message which kernel versions are affected

**Security Implications**: None. This is a robustness fix, not a security vulnerability.

**ABI/API Impact**: None. Internal driver change only.

**Verdict**: 
- **With stable tag**: Reviewed-by worthy
- **Without stable tag**: Needs revision as requested by Christian König

**Final Assessment**: The fix is technically correct and has maintainer approval (Reviewed-by: Christian König). The only blocking issue is the missing Cc: stable tag that was explicitly requested in review.

---
Generated by Claude Code Patch Reviewer