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

[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.

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");
>   		}