Claude review: drm/ttm/tests: add a test to exercise ttm_bo_swapout

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

Patch Review

**Overall assessment: Good patch, minor observations only.**

**Positive aspects:**

- The test follows the established patterns in `ttm_bo_validate_test.c` well — using `ttm_mock_manager_init`, `ttm_place_kunit_init`, `ttm_placement_kunit_init`, `kunit_kzalloc`, `drm_gem_private_object_init`, `ttm_bo_init_reserved`, and `ttm_bo_fini` in the same manner as other tests.
- Using `EXPORT_SYMBOL_FOR_TESTS_ONLY` is the correct approach for making `ttm_bo_swapout` available to the test module, and it already has precedent (`ttm_tt_swapout` at `ttm_tt.c:368` uses the same macro).
- The commit message is well-written with good context about why the previous test was removed and why this new approach is better.
- The v2 changes (removing redundant asserts, requesting all pages) are good improvements.

**Observations:**

1. **`KUNIT_EXPECT_EQ` vs `KUNIT_ASSERT_EQ` for `ttm_bo_init_reserved`**: At line 135-136 of the test:
   ```c
   KUNIT_EXPECT_EQ(test, err, 0);
   dma_resv_unlock(bo->base.resv);
   ```
   If `ttm_bo_init_reserved` fails, `err != 0` and the BO may not be properly initialized, yet execution continues to `dma_resv_unlock` and `ttm_bo_swapout`. This should arguably be `KUNIT_ASSERT_EQ` to bail out on failure, since the subsequent code depends on successful initialization. That said, the v2 changelog says "Removed extra asserts that are already internally used," so this may have been a deliberate reviewer-requested change. The existing tests in this file use `KUNIT_EXPECT_EQ` in similar positions, so this is consistent with the file's convention.

2. **Documentation vs implementation mismatch (pre-existing)**: The `ttm_bo_swapout` doc says `@target: The desired number of bytes to swap out` and the return is described as "The number of bytes actually swapped out." However, `ttm_bo_swapout_cb` returns `ttm->num_pages` (pages, not bytes), and the test correctly uses `MANAGER_SIZE / PAGE_SIZE` (i.e., pages) for both target and expected return:
   ```c
   swapped = ttm_bo_swapout(priv->ttm_dev, &ctx_init, man, GFP_KERNEL,
                            MANAGER_SIZE / PAGE_SIZE);
   KUNIT_EXPECT_EQ(test, swapped, MANAGER_SIZE / PAGE_SIZE);
   ```
   This is correct behavior matching the actual implementation, but the doc comment on `ttm_bo_swapout` is misleading (pre-existing issue, not introduced by this patch).

3. **The `#include <drm/drm_util.h>` addition**: Adding this include to `ttm_bo.c` is correct since `EXPORT_SYMBOL_FOR_TESTS_ONLY` is defined in `drm/drm_util.h`. It's clean and minimal.

4. **Cleanup ordering**: The test calls `ttm_bo_fini(bo)` before `ttm_mock_manager_fini()`, which is the correct ordering — the BO's resource (now in SYSTEM) should be released before tearing down the mock manager. This matches the pattern used elsewhere in the file.

**Verdict**: The patch is well-crafted and addresses a real testing gap. The EXPECT vs ASSERT question in point 1 is the only potential concern, and it follows the file's existing convention. Reviewed-by from Tvrtko is already present. This looks good to merge.

---
Generated by Claude Code Patch Reviewer