Re: [PATCH v2 04/33] drm/ttm: Introduce ttm_pool_alloc_iter for __ttm_pool_alloc()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Calling `__fls()` with 0 causes undefined behavior when an allocation fulfills the exact remaining pages, potentially causing an infinite loop.
--

--- Patch [4]: [PATCH v2 04/33] drm/ttm: Introduce ttm_pool_alloc_iter for __ttm_pool_alloc() ---
commit 5c371f076caffdc9b759a99aaeaf1a30e3ac5dd7
Author: Matthew Brost <matthew.brost@intel.com>

drm/ttm: Introduce ttm_pool_alloc_iter for __ttm_pool_alloc()
    
This commit factors the working state of the __ttm_pool_alloc loop into a
struct ttm_pool_alloc_iter. It pulls the per-order page acquisition into 
smaller helpers to simplify the nested block and improve readability.

> diff --git a/drivers/gpu/drm/ttm/ttm_pool.c b/drivers/gpu/drm/ttm/ttm_pool.c
> index 370d991c9882b..6826c8ae6fc56 100644
> --- a/drivers/gpu/drm/ttm/ttm_pool.c
> +++ b/drivers/gpu/drm/ttm/ttm_pool.c

[ ... ]

> @@ -819,59 +903,28 @@ static int __ttm_pool_alloc(struct ttm_pool *pool, struct ttm_tt *tt,
>  	else
>  		gfp_flags |= GFP_HIGHUSER;
>  
> -	page_caching = tt->caching;
> -	allow_pools = true;
> -	for (order = ttm_pool_alloc_find_order(MAX_PAGE_ORDER, alloc);
> +	it.gfp_flags = gfp_flags;
> +
> +	for (it.order = ttm_pool_alloc_find_order(MAX_PAGE_ORDER, alloc);
>  	     alloc->remaining_pages;
> -	     order = ttm_pool_alloc_find_order(order, alloc)) {
> +	     it.order = ttm_pool_alloc_find_order(it.order, alloc)) {

[Severity: High]
This is a pre-existing issue, but does this loop increment expression
evaluate __fls(0) when the allocation exactly fulfills the request?

If the loop successfully allocates the final needed pages during an iteration,
alloc->remaining_pages drops to 0 inside the loop body. The C for-loop
evaluates the increment expression before checking the loop condition, which
calls ttm_pool_alloc_find_order().

Inside ttm_pool_alloc_find_order(), this directly executes:
min_t(unsigned int, highest, __fls(alloc->remaining_pages))

Calling __fls(0) translates to __builtin_clzl(0), which is explicitly
undefined behavior in GCC and Clang. 

Could this undefined behavior allow the compiler to assume
alloc->remaining_pages is always non-zero because it is passed to __fls, 
potentially optimizing away the loop termination condition and causing an 
infinite loop?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710215442.2444235-1-matthew.brost@intel.com?part=4