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@xxxxxxx>
Sent: Tuesday, June 28, 2022 6:41 PM
To: Errabolu, Ramesh <Ramesh.Errabolu@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx; dan.carpenter@xxxxxxxxxx
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@xxxxxxx>
Reported-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
---
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");
}
