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/vmwgfx: Don't overwrite KMS surface dirty tracker Date: Tue, 03 Mar 2026 12:53:48 +1000 Message-ID: In-Reply-To: <20260302200330.66763-1-ian.forbes@broadcom.com> References: <20260302200330.66763-1-ian.forbes@broadcom.com> <20260302200330.66763-1-ian.forbes@broadcom.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 **Correctness: Good.** The fix is sound. Looking at `vmw_surface_dirty_allo= c()` at `vmwgfx_surface.c:2040`: ```c res->dirty =3D (struct vmw_resource_dirty *) dirty; ``` This unconditionally overwrites `res->dirty`, so if a dirty tracker was alr= eady allocated, the previous allocation is leaked. The guard: ```c if (surface->res.dirty =3D=3D NULL) ret =3D surface->res.func->dirty_alloc(&surface->res); ``` correctly prevents the double allocation. This pattern is consistent with t= he other call site in `vmwgfx_resource.c:402` which also checks `!res->dirt= y` before calling `dirty_alloc`. **One minor observation:** The existing code at line 772 already checks `su= rface->res.func->dirty_alloc` (that the function pointer is non-NULL) befor= e entering the block. It might be slightly cleaner to also set `surface->re= s.coherent =3D true` only when the dirty tracker is actually being newly al= located (i.e., move it inside the `if (surface->res.dirty =3D=3D NULL)` blo= ck), since if `dirty` is already allocated, coherent should presumably alre= ady be true. However, setting `coherent =3D true` redundantly is harmless = =E2=80=94 it's just a boolean assignment =E2=80=94 so this is a style nit r= ather than a bug. **Commit message and tags: Good.** Has `Reported-by`, `Closes` with lore li= nk, `Fixes` tag referencing the commit that introduced the regression, and = proper `Signed-off-by`. All in order. **Verdict:** The patch is a clean, minimal fix for the reported memory leak= . It looks good to merge. --- Generated by Claude Code Patch Reviewer