[AMD Official Use Only - General]
Will post updated patch. My responses inline below
Regards,
Ramesh
-----Original Message-----
From: Kuehling, Felix <Felix.Kuehling@xxxxxxx>
Sent: Wednesday, June 29, 2022 8:58 AM
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 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.
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");
>> }
