Claude review: drm/amdgpu: fix root reservation in amdgpu_vm_handle_fault

[Claude Code Review Bot <claude-review@example.com>]

Patch Review

**The deadlock path (confirmed by code reading):**
1. `amdgpu_vm_handle_fault` → `amdgpu_vm_lock_by_pasid` → `amdgpu_bo_reserve(root, true)` — acquires `dma_resv` lock with NULL `ww_acquire_ctx`
2. → `svm_range_restore_pages` → `svm_range_validate_and_map` → `svm_range_reserve_bos` → `amdgpu_vm_lock_pd(vm, &ctx->exec, 2)` — tries to lock the same root BO via `drm_exec` with a non-NULL `ww_acquire_ctx`
3. Same thread, same mutex, two different acquire contexts → deadlock

**Correctness of the fix:**

The new code structure:
```c
if (is_compute_context) {
    amdgpu_bo_unreserve(root);
    if (!svm_range_restore_pages(...)) {
        amdgpu_bo_unref(&root);
        return true;
    }
    amdgpu_bo_unref(&root);
    vm = amdgpu_vm_lock_by_pasid(adev, &root, pasid);
    if (!vm)
        return false;
}
```

- **Success path** (`svm_range_restore_pages` returns 0): unreserve, unref, return true — correctly matches original behavior.
- **Failure path** (non-zero return): unreserve + unref the old root, then re-acquire a fresh lock+ref via `amdgpu_vm_lock_by_pasid`. Falls through to the NORETRY PTE installation path at line 3037 (`flags = AMDGPU_VM_NORETRY_FLAGS`), which needs root reserved — satisfied by the fresh lock.
- **VM destruction race**: Between unreserve/unref and the re-lock call, the VM could be torn down. `amdgpu_vm_lock_by_pasid` handles this correctly — it re-looks up the pasid, re-refs the root BO, re-reserves it, and double-checks that `vm->root.bo` still matches (lines 2979–2983 in the existing code).
- **`amdgpu_bo_unref(&root)`** nulls out `root`, so the subsequent `amdgpu_vm_lock_by_pasid` writes to a clean pointer. No use-after-free risk.

**Minor observations:**

1. **Missing `Cc: stable@vger.kernel.org`**: The patch has a `Fixes:` tag pointing to commit `32b486e8541c` but no `Cc: stable`. If this deadlock is reproducible in practice (e.g., SVM/XNACK workloads on compute contexts), it should probably carry a stable tag.

2. **Two-line comment block**: The `/* Unreserve root ... */` and `/* TODO: ... */` are two separate single-line comments. A single multi-line comment would be more conventional kernel style, but this is purely cosmetic.

3. **The TODO is well-placed**: The unlock/relock pattern is inherently fragile — if future code adds state between the initial lock and the `is_compute_context` check, it could be invalidated by the relock. The TODO acknowledging that `svm_range_restore_pages` should be reworked to not need the root reservation (or to accept it already held) is appropriate guidance for a future cleanup.

4. **Behavioral change is intentional and correct**: In the original code, for compute contexts where `svm_range_restore_pages` fails, the function falls through with the original lock still held. In the new code, it drops and re-acquires the lock. This is equivalent — the re-acquired lock protects the same critical section, and the `vm` pointer is refreshed.

Overall this is a clean, minimal fix for a real deadlock. Reviewed-by worthy.

---
Generated by Claude Code Patch Reviewer