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: xe: selftests: fix fence refcount leak in run_sanity_job() Date: Thu, 28 May 2026 12:37:36 +1000 Message-ID: In-Reply-To: <20260527070244.858512-1-vulab@iscas.ac.cn> References: <20260527070244.858512-1-vulab@iscas.ac.cn> <20260527070244.858512-1-vulab@iscas.ac.cn> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Correctness: The code change is correct.** The flow is: 1. Line 55: `fence =3D dma_fence_get(&job->drm.s_fence->finished);` =E2=80= =94 takes a reference 2. Line 56: `xe_sched_job_push(job);` =E2=80=94 submits the job (job owners= hip transferred to scheduler) 3. Line 58: `sanity_fence_failed()` checks if the fence completed; if it fa= iled, the original code returned without releasing the fence reference The added `dma_fence_put(fence)` on the error path correctly balances the `= dma_fence_get()`. **Minor note on the commit message:** The commit message says "If the job s= ubmission fails, the function returns an error without calling dma_fence_pu= t()". This is slightly misleading =E2=80=94 the job submission (`xe_sched_j= ob_push`) itself doesn't fail here. What happens is the fence *times out* (= or is an error/NULL), and `sanity_fence_failed()` returns true. The leak oc= curs on the fence-wait failure path, not the job-submission failure path. T= he distinction matters because `xe_sched_job_push()` is a void function tha= t always succeeds =E2=80=94 the job is already pushed before the fence chec= k. **Fixes tag concern:** The `Fixes:` tag points to `dd08ebf6c352 ("drm/xe: I= ntroduce a new DRM driver for Intel GPUs")` which is the initial xe driver = commit. While technically the buggy code was introduced there, this is a ve= ry broad Fixes tag. A more precise tag would reference whatever commit intr= oduced or last modified `run_sanity_job()` into its current form. This matt= ers because the `Cc: stable@vger.kernel.org` tag means this will be conside= red for backporting =E2=80=94 using the initial driver commit as the Fixes = tag could cause confusion about which stable branches need the fix. **Stable backport concern:** This is a KUnit selftest fix =E2=80=94 not a p= roduction code path. Tagging it `Cc: stable@vger.kernel.org` is questionabl= e since the leak only occurs in test code during a fence timeout/failure sc= enario. Test-only fixes are generally not backported to stable kernels. **Verdict:** The code change itself is correct and trivially safe. The meta= data (commit message accuracy, Fixes tag precision, stable tag appropriaten= ess) could be improved. --- Generated by Claude Code Patch Reviewer