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/ttm/tests: Remove checks from ttm_pool_free_no_dma_alloc Date: Sun, 12 Apr 2026 11:03:53 +1000 Message-ID: In-Reply-To: <20260409142658.1511941-2-dev@lankhorst.se> References: <20260409142658.1511941-2-dev@lankhorst.se> <20260409142658.1511941-2-dev@lankhorst.se> 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 removed assertions were fundamentally wrong for = this test case. Looking at `ttm_pool_select_type()` (`ttm_pool.c:401`): ```c static struct ttm_pool_type *ttm_pool_select_type(struct ttm_pool *pool, enum ttm_caching caching, unsigned int order) { if (ttm_pool_uses_dma_alloc(pool)) return &pool->caching[caching].orders[order]; #ifdef CONFIG_X86 switch (caching) { case ttm_write_combined: ... return &global_write_combined[order]; case ttm_uncached: ... return &global_uncached[order]; ``` When `alloc_flags=3D0` (no DMA alloc), `ttm_pool_init()` skips `ttm_pool_ty= pe_init()` for `pool->caching[caching].orders[order]` because `ttm_pool_sel= ect_type()` returns either a global pool (x86) or NULL (!x86) =E2=80=94 nev= er the pool's own entry. The test was accessing an uninitialized `list_lru`= , which is undefined behavior on any architecture. Note that the sibling test `ttm_pool_free_dma_alloc` (which passes `TTM_ALL= OCATION_POOL_USE_DMA_ALLOC`) correctly accesses `pool->caching[caching].ord= ers[order]` because `ttm_pool_select_type()` *does* return the per-pool ent= ry in that case, and `ttm_pool_type_init()` initializes it. **Minor nits:** 1. **Commit message grammar:** "I'm not sure how that it was supposed to wo= rk previously" =E2=80=94 the word "that" appears extraneous. Should be "I'm= not sure how it was supposed to work previously." 2. **Duplicate Cc:** Christian K=C3=B6nig is Cc'd twice (once with the =C3= =B6, once as "Koenig"). This is harmless but unnecessary. 3. **Test value:** After this patch, the test performs `ttm_pool_alloc` + `= ttm_pool_free` + `ttm_pool_fini` with no assertions beyond the initial setu= p checks. It still has value as a crash/leak regression test, but it might = be worth adding a TODO comment or a follow-up to add meaningful assertions = =E2=80=94 e.g., verifying pages were returned to the global pool (on x86) o= r freed to the system (on !x86). The commit message says "reverting for now= " which implies this is intended as temporary, but in practice such things = tend to persist. 4. **Pre-existing issue in the original test:** Even before the `list_lru` = rewrite, the original test's assertions were conceptually wrong =E2=80=94 t= hey were checking the pool's own `ttm_pool_type` entry, but without DMA all= oc, pages never go through that entry. On x86 they go to `global_uncached[o= rder]`, and on !x86 they're freed directly. This suggests the original test= (from the `list_lru` port commit `444e2a19d7fd`) had a latent bug that was= masked by the old `list_head`-based pool implementation. This is consisten= t with the commit message's observation. Overall this is a straightforward and correct CI fix. No functional concern= s. --- Generated by Claude Code Patch Reviewer