When list_for_each_entry() completes the iteration over the whole list
without breaking the loop, the iterator value will be a bogus pointer
computed based on the head element.
While it is safe to use the pointer to determine if it was computed
based on the head element with &pos->member == head, using the iterator
variable after the loop should be avoided.
In preparation to limiting the scope of a list iterator to the list
traversal loop, use a dedicated pointer to point to the found element [1].
Link: https://lore.kernel.org/all/[email protected]/
Signed-off-by: Xiaomeng Tong <[email protected]>
---
drivers/gpu/drm/ttm/ttm_bo.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index db3dc7ef5382..413b5bbf2414 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -673,36 +673,36 @@ int ttm_mem_evict_first(struct ttm_device *bdev,
struct ww_acquire_ctx *ticket)
{
struct ttm_buffer_object *bo = NULL, *busy_bo = NULL;
+ struct ttm_buffer_object *iter;
bool locked = false;
unsigned i;
int ret;
spin_lock(&bdev->lru_lock);
for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
- list_for_each_entry(bo, &man->lru[i], lru) {
+ list_for_each_entry(iter, &man->lru[i], lru) {
bool busy;
- if (!ttm_bo_evict_swapout_allowable(bo, ctx, place,
+ if (!ttm_bo_evict_swapout_allowable(iter, ctx, place,
&locked, &busy)) {
if (busy && !busy_bo && ticket !=
- dma_resv_locking_ctx(bo->base.resv))
- busy_bo = bo;
+ dma_resv_locking_ctx(iter->base.resv))
+ busy_bo = iter;
continue;
}
- if (!ttm_bo_get_unless_zero(bo)) {
+ if (!ttm_bo_get_unless_zero(iter)) {
if (locked)
- dma_resv_unlock(bo->base.resv);
+ dma_resv_unlock(iter->base.resv);
continue;
}
+
+ bo = iter;
break;
}
- /* If the inner loop terminated early, we have our candidate */
- if (&bo->lru != &man->lru[i])
+ if (bo)
break;
-
- bo = NULL;
}
if (!bo) {
--
2.17.1
Am 19.03.22 um 08:31 schrieb Xiaomeng Tong:
> When list_for_each_entry() completes the iteration over the whole list
> without breaking the loop, the iterator value will be a bogus pointer
> computed based on the head element.
>
> While it is safe to use the pointer to determine if it was computed
> based on the head element with &pos->member == head, using the iterator
> variable after the loop should be avoided.
>
> In preparation to limiting the scope of a list iterator to the list
> traversal loop, use a dedicated pointer to point to the found element [1].
>
> Link: https://lore.kernel.org/all/[email protected]/
Well exactly that's why I'm pushing back to those changes.
We have tons of cases like this and I certainly won't accept patches to
make the code more complex than necessary. Especially not adding extra
local variables.
Regards,
Christian.
>
> Signed-off-by: Xiaomeng Tong <[email protected]>
> ---
> drivers/gpu/drm/ttm/ttm_bo.c | 20 ++++++++++----------
> 1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> index db3dc7ef5382..413b5bbf2414 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -673,36 +673,36 @@ int ttm_mem_evict_first(struct ttm_device *bdev,
> struct ww_acquire_ctx *ticket)
> {
> struct ttm_buffer_object *bo = NULL, *busy_bo = NULL;
> + struct ttm_buffer_object *iter;
> bool locked = false;
> unsigned i;
> int ret;
>
> spin_lock(&bdev->lru_lock);
> for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
> - list_for_each_entry(bo, &man->lru[i], lru) {
> + list_for_each_entry(iter, &man->lru[i], lru) {
> bool busy;
>
> - if (!ttm_bo_evict_swapout_allowable(bo, ctx, place,
> + if (!ttm_bo_evict_swapout_allowable(iter, ctx, place,
> &locked, &busy)) {
> if (busy && !busy_bo && ticket !=
> - dma_resv_locking_ctx(bo->base.resv))
> - busy_bo = bo;
> + dma_resv_locking_ctx(iter->base.resv))
> + busy_bo = iter;
> continue;
> }
>
> - if (!ttm_bo_get_unless_zero(bo)) {
> + if (!ttm_bo_get_unless_zero(iter)) {
> if (locked)
> - dma_resv_unlock(bo->base.resv);
> + dma_resv_unlock(iter->base.resv);
> continue;
> }
> +
> + bo = iter;
> break;
> }
>
> - /* If the inner loop terminated early, we have our candidate */
> - if (&bo->lru != &man->lru[i])
> + if (bo)
> break;
> -
> - bo = NULL;
> }
>
> if (!bo) {