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/v3d: Fix vaddr leak when indirect CSD has zeroed workgroups Date: Thu, 04 Jun 2026 12:49:58 +1000 Message-ID: In-Reply-To: <20260602-v3d-fix-indirect-csd-v3-1-cc79e06e543c@igalia.com> References: <20260602-v3d-fix-indirect-csd-v3-0-cc79e06e543c@igalia.com> <20260602-v3d-fix-indirect-csd-v3-1-cc79e06e543c@igalia.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 **Status: Good** This patch fixes a resource leak in `v3d_rewrite_csd_job_wg_counts_from_ind= irect()`. Looking at the existing code in the kernel tree (`v3d_sched.c:400= -406`): ```c v3d_get_bo_vaddr(bo); v3d_get_bo_vaddr(indirect); wg_counts =3D (uint32_t *)(bo->vaddr + indirect_csd->offset); if (wg_counts[0] =3D=3D 0 || wg_counts[1] =3D=3D 0 || wg_counts[2] =3D=3D 0) return; ``` The bare `return` at line 406 skips the cleanup at lines 431-432 (`v3d_put_= bo_vaddr(indirect)` / `v3d_put_bo_vaddr(bo)`), leaking both mappings. The f= ix correctly replaces `return` with `goto unmap_bo`, which jumps to the cle= anup path: ```c if (wg_counts[0] =3D=3D 0 || wg_counts[1] =3D=3D 0 || wg_counts[2] =3D=3D 0) goto unmap_bo; ``` ```c unmap_bo: v3d_put_bo_vaddr(indirect); v3d_put_bo_vaddr(bo); ``` The label placement is correct =E2=80=94 it sits right before both `v3d_put= _bo_vaddr` calls, matching the acquisition order (bo, indirect acquired; in= direct, bo released). The label name `unmap_bo` is slightly ambiguous since= it unmaps both BOs, but this is a minor style nit that doesn't affect corr= ectness. One observation: when the zero-workgroup path is taken, `args->cfg[0..4]` a= re left untouched =E2=80=94 they retain whatever userspace submitted. This = is fine because patch 2 ensures the CSD job is never submitted to hardware = when counts are zero. --- Generated by Claude Code Patch Reviewer