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

["Errabolu, Ramesh" <Ramesh.Errabolu@xxxxxxx>]

[AMD Official Use Only - General]

Maira,

Thanks for taking time to review. I understand the request to tag the patch as version 2. However I don't follow your comments on "Fixes" block. Looking at the git log of the branch I see many "Fixes" block that precede the "Signed-off-by" statement.

Could you provide an example.

Regards,
Ramesh

-----Original Message-----
From: Maíra Canal <mairacanal@xxxxxxxxxx> 
Sent: Wednesday, June 29, 2022 6:25 PM
To: Errabolu, Ramesh <Ramesh.Errabolu@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx; dan.carpenter@xxxxxxxxxx
Subject: Re: [PATCH] drm/amdkfd: Fix warnings from static analyzer Smatch

[CAUTION: External Email]

Hi Ramesh,

On 6/29/22 14:04, Ramesh Errabolu wrote:
> The patch fixes couple of warnings, as reported by Smatch a static 
> analyzer.
>
> Fixes: 40d6aa758b13 ("drm/amdkfd: Extend KFD device topology to 
> surface peer-to-peer links")>
> drivers/gpu/drm/amd/amdgpu/../amdkfd/kfd_topology.c:1428 kfd_create_indirect_link_prop() warn: iterator used outside loop: 'cpu_link'
> drivers/gpu/drm/amd/amdgpu/../amdkfd/kfd_topology.c:1462 
> kfd_create_indirect_link_prop() error: we previously assumed 'cpu_dev' 
> could be null (see line 1420)
> drivers/gpu/drm/amd/amdgpu/../amdkfd/kfd_topology.c:1516 kfd_add_peer_prop() warn: iterator used outside loop: 'iolink3'
>

Usually, the Fixes tag would go here, after the commit message.

> Signed-off-by: Ramesh Errabolu <Ramesh.Errabolu@xxxxxxx>
> Reported-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> ---

As this is a v2 PATCH, it would be nice to have a small changelog here, describing what has changed between the v1 and v2 versions of the patch.
Also, you can mark the patch as v2 with git send-email by adding the flag -v2. More on the canonical patch format can be seen in [1].

[1]
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdocs.kernel.org%2Fprocess%2Fsubmitting-patches.html%23the-canonical-patch-format&amp;data=05%7C01%7CRamesh.Errabolu%40amd.com%7Cc54753a9471843cc9d1f08da5a26898d%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637921418813227961%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=PAc5A8z2EvJAOUiY378K9XyVBCKewQNsSNCr9pB3Ias%3D&amp;reserved=0

Best Regards,
- Maíra Canal

>  drivers/gpu/drm/amd/amdkfd/kfd_topology.c | 34 
> +++++++++++------------
>  1 file changed, 17 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..ca4825e555b7 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
> @@ -1417,15 +1417,15 @@ 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;
> +
> +             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))
>                       return -ENOMEM;
>
>               /* CPU <--> CPU <--> GPU, GPU node*/ @@ -1510,16 
> +1510,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");
>               }