Claude review: drm/vmwgfx: Change ttm refs for dumb buffers.

[Claude Code Review Bot <claude-review@example.com>]

Patch Review

**Purpose:** Fix dumb buffer lifetime by taking an extra ttm_base_object reference 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 discarded 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 *` which 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 error handling.
- The code is relying on the side-effect of `kref_get_unless_zero` inside the function, but by convention the caller is expected to use the returned pointer and eventually call `ttm_base_object_unref` — ignoring the return value makes the intent unclear.

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

2. **`ttm_prime_fd_to_handle` dma_buf leak fix — correct and straightforward:** 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 != &tdev->ops) {
    ret = -ENOSYS;
    goto out;
}
```

This is a clean and correct fix.

3. **`vmw_dumb_surface_unref` — double-free risk?** The function does:
```c
void vmw_dumb_surface_unref(struct vmw_surface **dumb_surface)
{
    struct ttm_base_object *base = &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. The `vmw_surface_unreference` then handles the surface cleanup. The ordering matters — `ttm_base_object_unref` must go first since `vmw_surface_unreference` may free the containing structure. This looks correct as written, since `vmw_surface_unreference` will only free when its own refcount drops to zero.

4. **Commit message says "fixes all igt tests that use dumb buffers" — 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 surface alive is the extra base object reference taken by `ttm_base_object_lookup_for_ref`. This is the design intent but should be documented with a comment.

---
Generated by Claude Code Patch Reviewer