[PATCH] drm/amdkfd: Fix warnings from static analyzer Smatch

[Errabolu, Ramesh]

[AMD Official Use Only - General]

Will post updated patch. My responses inline below

Regards,
Ramesh

-----Original Message-----
From: Kuehling, Felix <Felix.Kuehling at amd.com> 
Sent: Wednesday, June 29, 2022 8:58 AM
To: Errabolu, Ramesh <Ramesh.Errabolu at amd.com>; amd-gfx at lists.freedesktop.org; dan.carpenter at oracle.com
Subject: Re: [PATCH] drm/amdkfd: Fix warnings from static analyzer Smatch

Am 2022-06-28 um 20:03 schrieb Errabolu, Ramesh:
> [AMD Official Use Only - General]
>
> My responses are inline
>
> -----Original Message-----
> From: Kuehling, Felix <Felix.Kuehling at amd.com>
> Sent: Tuesday, June 28, 2022 6:41 PM
> To: Errabolu, Ramesh <Ramesh.Errabolu at amd.com>; 
> amd-gfx at lists.freedesktop.org; dan.carpenter at oracle.com
> Subject: Re: [PATCH] drm/amdkfd: Fix warnings from static analyzer 
> Smatch
>
>
> Am 2022-06-28 um 19:25 schrieb Ramesh Errabolu:
>> The patch fixes couple of warnings, as reported by Smatch a static 
>> analyzer
>>
>> Signed-off-by: Ramesh Errabolu <Ramesh.Errabolu at amd.com>
>> Reported-by: Dan Carpenter <dan.carpenter at oracle.com>
>> ---
>>    drivers/gpu/drm/amd/amdkfd/kfd_topology.c | 36 ++++++++++++-----------
>>    1 file changed, 19 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
>> b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
>> index 25990bec600d..9d7b9ad70bc8 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
>> @@ -1417,15 +1417,17 @@ static int
>> kfd_create_indirect_link_prop(struct kfd_topology_device *kdev, int g
>>    
>>    		/* find CPU <-->  CPU links */
>>    		cpu_dev = kfd_topology_device_by_proximity_domain(i);
>> -		if (cpu_dev) {
>> -			list_for_each_entry(cpu_link,
>> -					&cpu_dev->io_link_props, list) {
>> -				if (cpu_link->node_to == gpu_link->node_to)
>> -					break;
>> -			}
>> -		}
>> +		if (!cpu_dev)
>> +			continue;
>> +
>> +		cpu_link = NULL;
> This initialization is unnecessary. list_for_each_entry will always initialize it.
>
>
>> +		list_for_each_entry(cpu_link, &cpu_dev->io_link_props, list)
>> +			if (cpu_link->node_to == gpu_link->node_to)
>> +				break;
>>    
>> -		if (cpu_link->node_to != gpu_link->node_to)
>> +		/* Ensures we didn't exit from list search with no hits */
>> +		if (list_entry_is_head(cpu_link, &cpu_dev->io_link_props, list) ||
>> +			(cpu_link->node_to != gpu_link->node_to))
> The second condition is redundant. If the list entry is not the head, 
> the node_to must have already matched in the loop.
>
> Ramesh: Syntactically, it is possible to walk down the list without having the hit. The check list_entry_is_head() is for that scenario.

If you traverse the whole list without a hit, list_entry_is_head will be true. That is also the only case where cpu_link->node_to != gpu_link->node_to is possible. Therefore that condition is redundant. 
Just checking list_entry_is_head is sufficient.

Ramesh: You are right, check for list head is sufficient as it implies all cpu_link objects do not pass the test of (cpu_link->node_to == gpu_link->node_to).

That said, as I pointed out below, you're still using cpu_link outside the loop. Therefore it's likely the static checker is still going to complain.

Ramesh: I suspect the reason static analyzer was complaining about cpu_link is because it was being used in the loop conditionally. The new code uses cpu_link in the loop unconditionally. Per this logic it must be valid once we cross the list_entry_is head statement.

Regards,
   Felix


>
> But I'm no sure this solution is going to satisfy the static checker. 
> It objects to using the iterator (cpu_link) outside the loop. I think 
> a proper solution, that doesn't make any assumptions about how 
> list_for_each_entry is implemented, would be to declare a separate 
> variable as the iterator, and assign cpu_link in the loop only if 
> there is a match.
>
> Ramesh: Will wait for a response from Dan.
>
> Regards,
>     Felix
>
>
>>    			return -ENOMEM;
>>    
>>    		/* CPU <--> CPU <--> GPU, GPU node*/ @@ -1510,16 +1512,16 @@ 
>> static int kfd_add_peer_prop(struct kfd_topology_device *kdev,
>>    		cpu_dev = kfd_topology_device_by_proximity_domain(iolink1->node_to);
>>    		if (cpu_dev) {
>>    			list_for_each_entry(iolink3, &cpu_dev->io_link_props, list)
>> -				if (iolink3->node_to == iolink2->node_to)
>> +				if (iolink3->node_to == iolink2->node_to) {
>> +					props->weight += iolink3->weight;
>> +					props->min_latency += iolink3->min_latency;
>> +					props->max_latency += iolink3->max_latency;
>> +					props->min_bandwidth = min(props->min_bandwidth,
>> +									iolink3->min_bandwidth);
>> +					props->max_bandwidth = min(props->max_bandwidth,
>> +									iolink3->max_bandwidth);
>>    					break;
>> -
>> -			props->weight += iolink3->weight;
>> -			props->min_latency += iolink3->min_latency;
>> -			props->max_latency += iolink3->max_latency;
>> -			props->min_bandwidth = min(props->min_bandwidth,
>> -							iolink3->min_bandwidth);
>> -			props->max_bandwidth = min(props->max_bandwidth,
>> -							iolink3->max_bandwidth);
>> +				}
>>    		} else {
>>    			WARN(1, "CPU node not found");
>>    		}