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