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

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

[Ramesh Errabolu]

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'

Signed-off-by: Ramesh Errabolu <Ramesh.Errabolu@amd.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 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");
 		}
-- 
2.35.1

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

[Maíra Canal]

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@amd.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---

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://docs.kernel.org/process/submitting-patches.html#the-canonical-patch-format

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

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

[Errabolu, Ramesh]

[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@riseup.net> 
Sent: Wednesday, June 29, 2022 6:25 PM
To: Errabolu, Ramesh <Ramesh.Errabolu@amd.com>; amd-gfx@lists.freedesktop.org; dan.carpenter@oracle.com
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@amd.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---

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

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

[Maíra Canal]

On 7/5/22 15:43, Errabolu, Ramesh wrote:
> [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.
> 

Hi Ramesh,

The canonical patch format is basically, as described by [1]:

<commit message>
...
Signed-off-by: Author <author@mail>
---
V2 -> V3: Removed redundant helper function
V1 -> V2: Cleaned up coding style and addressed review comments

path/to/file | 5+++--
...

So, on your case, we have the commit message describing the warning
reported by Smatch and the error log, then we got the tags. The tags
should be in chronological order, so your tags should be:

Fixes: 40d6aa758b13 ("drm/amdkfd: Extend KFD device topology to surface
peer-to-peer links")
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Ramesh Errabolu <Ramesh.Errabolu@amd.com>

See that this canonical format reflects the chronological history of the
patch insofar as possible. After the ---, you describe the changes
between v1 and v2 in a small changelog.

The documentation linked in [1] explains this in details. So, some
examples are exposed in [2], [3] and [4].

[1] https://docs.kernel.org/process/submitting-patches.html
[2]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=1db88c5343712e411a2dd45375f27c477e33dc07
[3]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=34ad61514c4c3657df21a058f9961c3bb2f84ff2
[4]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d3f2a14b8906df913cb04a706367b012db94a6e8

Best Regards,
- Maíra Canal

> Could you provide an example.
> 
> Regards,
> Ramesh
> 
> -----Original Message-----
> From: Maíra Canal <mairacanal@riseup.net> 
> Sent: Wednesday, June 29, 2022 6:25 PM
> To: Errabolu, Ramesh <Ramesh.Errabolu@amd.com>; amd-gfx@lists.freedesktop.org; dan.carpenter@oracle.com
> 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@amd.com>
>> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
>> ---
> 
> 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");
>>               }

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

[Ramesh Errabolu]

The patch fixes warnings/error as reported by Smatch a static analyzer

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'

Fixes: 40d6aa758b13 ("drm/amdkfd: Extend KFD device topology to surface peer-to-peer links")
Signed-off-by: Ramesh Errabolu <Ramesh.Errabolu@amd.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>
---
 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");
 		}
-- 
2.35.1

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

[Felix Kuehling]

Am 2022-07-06 um 12:47 schrieb Ramesh Errabolu:
> The patch fixes warnings/error as reported by Smatch a static analyzer
>
> 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'
>
> Fixes: 40d6aa758b13 ("drm/amdkfd: Extend KFD device topology to surface peer-to-peer links")
> Signed-off-by: Ramesh Errabolu <Ramesh.Errabolu@amd.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>


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

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

[Ramesh Errabolu]

The patch fixes warnings/error as reported by Smatch a static analyzer

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'

Fixes: 40d6aa758b13 ("drm/amdkfd: Extend KFD device topology to surface peer-to-peer links")
Signed-off-by: Ramesh Errabolu <Ramesh.Errabolu@amd.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 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");
 		}
-- 
2.35.1

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

[Errabolu, Ramesh]

[AMD Official Use Only - General]

Thanks for running Smatch on the patch.

Will update the commit message with the list of warnings/errors pointed out by Smatch. Will post the updated patch shortly.

Regards,
Ramesh

-----Original Message-----
From: Dan Carpenter <dan.carpenter@oracle.com> 
Sent: Wednesday, June 29, 2022 11:13 AM
To: Errabolu, Ramesh <Ramesh.Errabolu@amd.com>
Cc: amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/amdkfd: Fix warnings from static analyzer Smatch

[CAUTION: External Email]

On Tue, Jun 28, 2022 at 06:25:38PM -0500, Ramesh Errabolu wrote:
> The patch fixes couple of warnings, as reported by Smatch a static 
> analyzer
>

I had forgotten what the warnings were.  Could you put that in the commit message or describe the bug somehow?


> Signed-off-by: Ramesh Errabolu <Ramesh.Errabolu@amd.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

This needs a Fixes tag as well.

regards,
dan carpenter

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

[Ramesh Errabolu]

The patch fixes couple of warnings, as reported by Smatch
a static analyzer

Signed-off-by: Ramesh Errabolu <Ramesh.Errabolu@amd.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 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");
 		}
-- 
2.35.1

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

[Dan Carpenter]

On Tue, Jun 28, 2022 at 06:25:38PM -0500, Ramesh Errabolu wrote:
> The patch fixes couple of warnings, as reported by Smatch
> a static analyzer
> 

I had forgotten what the warnings were.  Could you put that in the
commit message or describe the bug somehow?


> Signed-off-by: Ramesh Errabolu <Ramesh.Errabolu@amd.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

This needs a Fixes tag as well.

regards,
dan carpenter

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

[Dan Carpenter]

On Tue, Jun 28, 2022 at 07:41:09PM -0400, Felix Kuehling wrote:
> 
> 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@amd.com>
> > Reported-by: Dan Carpenter <dan.carpenter@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.
> 
> 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.

This patch will silence the Smatch warning.  Smatch is can parse the
code well enough to know that:

	list_entry_is_head(cpu_link, &cpu_dev->io_link_props, list)

and:

	cpu_link->node_to != gpu_link->node_to

are equivalent.  Or actually it's the reverses which are equivalent.  If
the cpu_link is at list_head then we can't know if they're equal or not
but if it's not a list_head then they must be equal.   Ugh...
Complicated to explain in English but easy to see in code.  If add some
Smatch debug code:

#include "/home/dcarpenter/smatch/check_debug.h"

...

		if (!list_entry_is_head(cpu_link, &cpu_dev->io_link_props, list))
			__smatch_compare(cpu_link->node_to, gpu_link->node_to);

Then it will display that:

drivers/gpu/drm/amd/amdgpu/../amdkfd/kfd_topology.c:1431 kfd_create_indirect_link_prop() cpu_link->node_to == gpu_link->node_to

*happy face*  Smatch knows that they are ==.

But your review comments are correct.  These days we prefer to just use
another pointer:

	found = NULL;

	list_for_each_entry() {
		if (entry == correct){
			found = entry;
			break;
		}
	}
	if (!found)
		return -ENODEV;

regards,
dan carpenter

RE: [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@amd.com> 
Sent: Wednesday, June 29, 2022 8:58 AM
To: Errabolu, Ramesh <Ramesh.Errabolu@amd.com>; amd-gfx@lists.freedesktop.org; dan.carpenter@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@amd.com>
> Sent: Tuesday, June 28, 2022 6:41 PM
> To: Errabolu, Ramesh <Ramesh.Errabolu@amd.com>; 
> amd-gfx@lists.freedesktop.org; dan.carpenter@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@amd.com>
>> Reported-by: Dan Carpenter <dan.carpenter@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");
>>    		}

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

[Felix Kuehling]

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@amd.com>
> Sent: Tuesday, June 28, 2022 6:41 PM
> To: Errabolu, Ramesh <Ramesh.Errabolu@amd.com>; amd-gfx@lists.freedesktop.org; dan.carpenter@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@amd.com>
>> Reported-by: Dan Carpenter <dan.carpenter@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.

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.

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

RE: [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@amd.com> 
Sent: Tuesday, June 28, 2022 6:41 PM
To: Errabolu, Ramesh <Ramesh.Errabolu@amd.com>; amd-gfx@lists.freedesktop.org; dan.carpenter@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@amd.com>
> Reported-by: Dan Carpenter <dan.carpenter@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");
>   		}

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

[Felix Kuehling]

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@amd.com>
> Reported-by: Dan Carpenter <dan.carpenter@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.

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.

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

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

[Ramesh Errabolu]

The patch fixes couple of warnings, as reported by Smatch
a static analyzer

Signed-off-by: Ramesh Errabolu <Ramesh.Errabolu@amd.com>
Reported-by: Dan Carpenter <dan.carpenter@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;
+		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))
 			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");
 		}
-- 
2.35.1