Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Message-ID: <cea6758b-6e7c-46d2-7d44-4372742ed449@amd.com>
Date:   Mon, 21 Mar 2022 10:55:55 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.0
Subject: Re: [PATCH] ttm: remove check of list iterator against head outside
 the loop
Content-Language: en-US
To:     Xiaomeng Tong <xiam0nd.tong@gmail.com>, ray.huang@amd.com,
        airlied@linux.ie, daniel@ffwll.ch
Cc:     dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org
References: <20220319073143.30184-1-xiam0nd.tong@gmail.com>
From:   =?UTF-8?Q?Christian_K=c3=b6nig?= <christian.koenig@amd.com>
In-Reply-To: <20220319073143.30184-1-xiam0nd.tong@gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?cGd0WG1vTlZ5MkZVQ0cvZFFYYVFYMWdEZHk4cXBERlFKUm90U1UyWkZheXhO?=
 =?utf-8?B?cWN0YVR6TjhJOUtkZVVLRWVEaGJPRVFBZE9Od1U0N1ZHbUw3anNacjR3bS8y?=
 =?utf-8?B?WllTS1RBVFNEUkUyLzA2QVRtOG5rS2JDdkg3bVIrZjRGNWwvYjRVWTJ5dzBW?=
 =?utf-8?B?TnNreEw3T0NjREhCcWo2NkptU1QyQXdtVEtOQ1JURjNJdVF5Q1RpMW9sWUV5?=
 =?utf-8?B?MTZ2Z3ZHZUx2WExmck9XNnBlK0VWZFJIZTBpTDV6OGVzN2NtSGgrOU94eHZK?=
 =?utf-8?B?UFEwc2g3c3FVUVU0eEZROW5vbHJrL29GYnRYYmZ0b2RNVSsvOWlaMmduVUJZ?=
 =?utf-8?B?QWVCM0Fpb1R6Lzc1NTUyY3JhMlBMeHdQR3lPREVacWFHRFF0NUo0alRaZXQx?=
 =?utf-8?B?TmdmNWIyWEE0Y1JpT3c3d0ZHL3ZoYnJKVEphbC9mbmU5TGdJWVV3VEVLSnlI?=
 =?utf-8?B?TlV1QlRHOGh1SFpYdGVKOTNMTU5JRTR0QnRTSWoyeDliSWs3eFR6cjZKUzJu?=
 =?utf-8?B?emdFOTNtbmtsbSsrdUluOWFndkdldFZ4bVZnYTdRUnYwc0FLdU93ZVdTSmtK?=
 =?utf-8?B?dFYrN1JCNVlOQnQwQnplOFQxb3lvUHduSEFJVWxUOGdGRWpGY0xEempxTHFE?=
 =?utf-8?B?U1dRaDk1ZktmektmbjZRQlFhM1dQeXJSbGhCSHJXbXZkTkdTVVZZYUZ5NldT?=
 =?utf-8?B?ekRQWnQvVVlHODRwdzY1NlE1YlZ5Mlc4VzBjMmZCd2czS2VRWmw0MSt6R3pO?=
 =?utf-8?B?aWhuMzQ1bzZyNVlRaUtaU0kvMG90VlBhOVJLUVl3Q0JVU2s0UWh3M1JBOWJK?=
 =?utf-8?B?TTc5M2xzSHcrQ2crWlVTVjhkOVJyd0R1RE9OMU9jZWhBRzVxSFBEemxIZTJU?=
 =?utf-8?B?ZEljMmVpNVc2blJjR2RndEZ2T1hQelhRNjVrZDJCTjE4dVYySXRvcmM0MUN4?=
 =?utf-8?B?ZFY0aGJuTlAyb0JpMFdiZGw1MXhxM25WdnhKQmMxTlZ2cjlDby85UmxhMkdj?=
 =?utf-8?B?YTd6Yy9LTTcyU2dwSHFOUU5BNmkwdWFsQW1FS0ZXMHFkTFdBaEJ2S0ZCOTBh?=
 =?utf-8?B?YldEbE5IRnF6ZWt5WThnTXFnbzFlMzRzd2VoQWhYNnlZVFRKV1AzS3ZhdExL?=
 =?utf-8?B?eUpVTjJGUkF5SUZDV2hkSmtXa0lKRW9iQkI2UXRKUDdiYXRwQWcydytZckR4?=
 =?utf-8?B?V0JqWXVoMjJnMGt1T1RKdFRma1lMWCtYbFNkblhlVDZ6dk42Wmt3bHdra0ph?=
 =?utf-8?B?Tjk4VmIwS1dBNlhKNGR0R1E3WTR6N3Y2NXNidXFqY2trUmRqb3k4TGJpMmtN?=
 =?utf-8?B?Ty9CeExwVXIyZTBmbnNjd1dsYlFhclYvRDZFTDFtREVTOUVtTXpLVjZOYjQw?=
 =?utf-8?B?OFNaenNXeEZ1TjVDYlVFZ0pnZ2dBN2U4clR3M0lVcU5yeGYxOHRtV0VOZ2l0?=
 =?utf-8?B?ZVB4MXJQL0tWbVNLMTcrTytsOVNicUxSSHpaRi9Mc1J6SkNWV3pCR3NHMkRj?=
 =?utf-8?B?SXljR1BnUitMdHpJSGpyM3JGdExMcnIxS1dPVHZOaHZyT0ZOOE1HcUppOVh6?=
 =?utf-8?B?SzZnalhKMTNvNTZhUTNpSDRtN1FPMEJ5ZW1jTXV0ZFVwMVNMMVd2bWFRamY3?=
 =?utf-8?B?aWkvdG9kWjVXYWJFWjg4WUZ1SldHN29ScWYvcUF3bXlnL1F4aVZneHIwaUg2?=
 =?utf-8?B?a1NGdjROMnBBRWh4N2xiTkpRQ2VoTWZvMWt3SG9VSW42dUd4NjN3d1ZQU3VI?=
 =?utf-8?B?dDdHcVJlS2t0UW9wQzI1RXdQWXlKb29sYVd4WWhuMkNWbUh6aFVqMmNta2hH?=
 =?utf-8?B?SEJ0REpIa2FTdG1WWDd6QTRGMkx0OXRMOXd3a0ZYOXlSYUZRSmg0WjArRjhm?=
 =?utf-8?B?SFRwQklscUQvYktpSmMrYVZhNmFOWVova2dwTGJYR1hjbmpGeEtnNDBSSDFX?=
 =?utf-8?B?ZksvYzdXbVNuRWpHNDg2NUpheFZ5cjRFanRzOGQzZmx2Zld5NGhvbVNKQUhS?=
 =?utf-8?B?OXQ1S1JxR2V2Wm40SHJZMFhXNHVxSVN5VTNHVzFYSTE0NUtvd1h6Q2JCcHkx?=
 =?utf-8?Q?wcZ4R/?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 25e3207a-49cf-4373-8377-08da0b210077
X-MS-Exchange-CrossTenant-AuthSource: BN8PR12MB3587.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 21 Mar 2022 09:55:59.7880
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 1dR9VPnxqK7yg9gyCnF4By6gyetlqoA3s84LS91XYhToUWSoH1JV1P3Lm8oXMbMV
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MWHPR12MB1502
X-Spam-Status: No, score=-2.0 required=5.0 tests=BAYES_00,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,NICE_REPLY_A,RDNS_NONE,SPF_HELO_NONE,
	T_SCC_BODY_TEXT_LINE autolearn=no autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
	lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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) {