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/amdgpu/userq: clean up VA state on create failure Date: Fri, 05 Jun 2026 06:43:51 +1000 Message-ID: In-Reply-To: <20260604063943.1412955-1-lgs201920130244@gmail.com> References: <20260604063943.1412955-1-lgs201920130244@gmail.com> <20260604063943.1412955-1-lgs201920130244@gmail.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Commit message quality:** Good. The commit message clearly explains the b= ug: the short-circuit OR in `amdgpu_userq_input_va_validate()` calls means = earlier successful validations leak state when later ones fail. The Fixes t= ag references the right commit. **Correctness issues in the patch itself (against its target base):** 1. **Missing unwind for `xa_store_irq` into `userq_doorbell_xa`:** After th= e `xa_store_irq` at the original line ~865, if `xa_alloc` fails, the patch'= s `free_queue` label skips straight to `amdgpu_userq_buffer_vas_list_cleanu= p` + `kfree` without erasing the doorbell xarray entry. The old code also d= idn't do this, but since the patch is reorganizing error handling, it shoul= d have been caught. 2. **Missing unwind for `mqd_create`:** The patch adds `free_mqd` and `free= _fence_driver` labels but **never jumps to them**. Looking at the diff: - After `amdgpu_userq_fence_driver_alloc` fails (line 847), it goes to `= free_queue` =E2=80=94 correct, fence driver wasn't allocated. - After `mqd_create` fails (line 854), it calls `amdgpu_userq_fence_driv= er_free` inline then goes to `free_queue` =E2=80=94 this bypasses the new `= free_fence_driver` label but works. However it does NOT go to `free_mqd`, w= hich is correct since mqd_create failed. - After `xa_store_irq` fails (line 866), it goes to `free_queue` =E2=80= =94 but at this point both the fence driver AND the MQD have been successfu= lly created, so `free_mqd` and `free_fence_driver` should be invoked. **Thi= s is a resource leak bug introduced by the patch.** - After `xa_alloc` fails (line 876), same problem =E2=80=94 goes to `fre= e_queue` without cleaning up MQD or fence driver. - After `amdgpu_userq_map_helper` fails (line 891), goes to `free_queue`= =E2=80=94 same issue, plus the doorbell xarray entry isn't erased. The `free_mqd` and `free_fence_driver` labels are dead code =E2=80=94 no= thing jumps to them. 3. **Inconsistent fence driver cleanup:** In the `mqd_create` failure path = (line 856), the patch keeps the inline `amdgpu_userq_fence_driver_free(queu= e)` call before `goto free_queue`. But later failure paths that also need f= ence driver cleanup (post xa_store_irq, post xa_alloc, post map_helper) go = directly to `free_queue` and skip it. **Summary:** The patch correctly identifies a real bug and the VA cleanup a= ddition is the right idea, but the consolidated error handling has **regres= sion bugs** =E2=80=94 several failure paths that previously cleaned up the = MQD and fence driver now skip that cleanup by jumping to `free_queue` inste= ad of `free_mqd`. The `free_mqd` and `free_fence_driver` labels are dead co= de. This would need a v2 that properly threads the later error paths throug= h the full unwind chain. The point is moot for drm-next since the code has been completely refactore= d and the bug no longer exists in the current tree. --- Generated by Claude Code Patch Reviewer