From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: drm/vmwgfx: Change ttm refs for dumb buffers.
Date: Mon, 25 May 2026 19:32:33 +1000
Message-ID: <review-patch4-20260521223756.627311-5-maaz.mombasawala@broadcom.com>
In-Reply-To: <20260521223756.627311-5-maaz.mombasawala@broadcom.com>
References: <20260521223756.627311-1-maaz.mombasawala@broadcom.com> <20260521223756.627311-5-maaz.mombasawala@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

**Purpose:** Fix dumb buffer lifetime by taking an extra ttm_base_object re=
ference during creation and releasing it properly on cleanup. Also fixes a =
dma_buf reference leak in `ttm_prime_fd_to_handle`.

**Issues:**

1. **Critical: Return value of `ttm_base_object_lookup_for_ref` is discarde=
d on success path:**

```c
ttm_base_object_lookup_for_ref(dev_priv->tdev, arg.rep.handle);

err:
```

`ttm_base_object_lookup_for_ref()` returns a `struct ttm_base_object *` whi=
ch is the refcount-incremented pointer, and this return value is completely=
 discarded. The intent is clearly to increment the base object's refcount, =
but ignoring the return value means:
- If the lookup fails (returns NULL), the code silently proceeds without er=
ror handling.
- The code is relying on the side-effect of `kref_get_unless_zero` inside t=
he function, but by convention the caller is expected to use the returned p=
ointer and eventually call `ttm_base_object_unref` =E2=80=94 ignoring the r=
eturn value makes the intent unclear.

This should at minimum check the return value and handle the error case. So=
mething like:
```c
if (!ttm_base_object_lookup_for_ref(dev_priv->tdev, arg.rep.handle)) {
    ret =3D -EINVAL;
    goto err;
}
```

2. **`ttm_prime_fd_to_handle` dma_buf leak fix =E2=80=94 correct and straig=
htforward:** The fix to add `dma_buf_put()` on the non-matching ops path is=
 clearly correct. The old code returned `-ENOSYS` without calling `dma_buf_=
put(dma_buf)`, leaking the reference acquired by `dma_buf_get(fd)`.

```c
if (dma_buf->ops !=3D &tdev->ops) {
    ret =3D -ENOSYS;
    goto out;
}
```

This is a clean and correct fix.

3. **`vmw_dumb_surface_unref` =E2=80=94 double-free risk?** The function do=
es:
```c
void vmw_dumb_surface_unref(struct vmw_surface **dumb_surface)
{
    struct ttm_base_object *base =3D &usurf->prime.base;
    ttm_base_object_unref(&base);
    vmw_surface_unreference(dumb_surface);
}
```

This relies on both the base object ref and the surface ref being in sync. =
If `refcount_release` is NULL (set earlier in `vmw_dumb_create`), then `ttm=
_base_object_unref` dropping to zero won't trigger any release callback. Th=
e `vmw_surface_unreference` then handles the surface cleanup. The ordering =
matters =E2=80=94 `ttm_base_object_unref` must go first since `vmw_surface_=
unreference` may free the containing structure. This looks correct as writt=
en, since `vmw_surface_unreference` will only free when its own refcount dr=
ops to zero.

4. **Commit message says "fixes all igt tests that use dumb buffers" =E2=80=
=94 that's a broad claim.** It would be better to list specific test names =
that were verified.

5. **The error-path `ttm_ref_object_base_unref` call ordering:** After the =
patch, on the success path the code does:
```c
ttm_base_object_lookup_for_ref(...)  // take base ref
err:
    vmw_resource_unreference(&res);   // drop resource ref
    ttm_ref_object_base_unref(tfile, arg.rep.handle);  // drop tfile ref
```

The `ttm_ref_object_base_unref` still runs on the success path. This means =
after `vmw_dumb_create` returns successfully, the only thing keeping the su=
rface alive is the extra base object reference taken by `ttm_base_object_lo=
okup_for_ref`. This is the design intent but should be documented with a co=
mment.

---
Generated by Claude Code Patch Reviewer