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