Hi!
On Tue, 2025-07-01 at 12:49 -0500, Dan Carpenter wrote:
> Hello Thomas Hellström,
>
> Commit bb8aa27eff6f ("drm/ttm, drm_xe, Implement
> ttm_lru_walk_for_evict() using the guarded LRU iteration") from Jun
> 23, 2025 (linux-next), leads to the following Smatch static checker
> warning:
>
> drivers/gpu/drm/ttm/ttm_bo_util.c:899
> ttm_lru_walk_for_evict()
> warn: 'bo' isn't an ERR_PTR
>
> drivers/gpu/drm/ttm/ttm_bo_util.c
> 883 s64 ttm_lru_walk_for_evict(struct ttm_lru_walk *walk, struct
> ttm_device *bdev,
> 884 struct ttm_resource_manager *man,
> s64 target)
> 885 {
> 886 struct ttm_bo_lru_cursor cursor;
> 887 struct ttm_buffer_object *bo;
> 888 s64 progress = 0;
> 889 s64 lret;
> 890
> 891 ttm_bo_lru_for_each_reserved_guarded(&cursor, man,
> &walk->arg, bo) {
> 892 lret = walk->ops->process_bo(walk, bo);
> 893 if (lret == -EBUSY || lret == -EALREADY)
> 894 lret = 0;
> 895 progress = (lret < 0) ? lret : progress +
> lret;
> 896 if (progress < 0 || progress >= target)
> 897 break;
> 898 }
> 899 if (IS_ERR(bo))
> 900 return PTR_ERR(bo);
>
> The ttm_bo_lru_for_each_reserved_guarded() macro checks for
> IS_ERR_OR_NULL()
> but it can only be NULL.
That's not correct.
> These things are a bit frustrating to me because
> when a function returns both error pointers and NULL then ideally
> there is a
> reason for that and it should be documented. "This function returns
> error
> pointers if there is an error (kmalloc failed etc) or NULL if the
> object is
> not found".
The error pointer is documented under the
ttm_bo_lru_for_each_reserved_guarded() macro. But it is true that I've
forgotten to update the doc for ttm_bo_lru_cursor_first() and
ttm_bo_lru_cursor_next() to reflect that they may return an error
pointer or NULL. I will put together a patch for that.
>
> https://staticthinking.wordpress.com/2022/08/01/mixing-error-pointers-and-null/
>
> 901
> 902 return progress;
>
> It's strange to me that we returns progress values which are greater
> than the
> target value.
This is also documented in the ttm_lru_walk_for_evict() kerneldoc. In
short a typical intended use-case is that we're requested to evict
@target pages, but since we evict a buffer object at a time (multiple
pages) the total may exceed @progress.
In any case it looks like the ttm_lru_walk_for_evict() function may go
away with upcoming patches from Christian.
Thanks,
Thomas
>
> 903 }
>
> regards,
> dan carpenter
