| Message ID | 20220319073143.30184-1-xiam0nd.tong@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ttm: remove check of list iterator against head outside the loop | expand |
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/YhdfEIwI4EdtHdym@kroah.com/ 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 <xiam0nd.tong@gmail.com> > --- > 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) {
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) {
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/YhdfEIwI4EdtHdym@kroah.com/ Signed-off-by: Xiaomeng Tong <xiam0nd.tong@gmail.com> --- drivers/gpu/drm/ttm/ttm_bo.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)