[AMD Official Use Only - General] My responses are inline
-----Original Message----- From: Kuehling, Felix <felix.kuehl...@amd.com> Sent: Tuesday, June 28, 2022 6:41 PM To: Errabolu, Ramesh <ramesh.errab...@amd.com>; amd-gfx@lists.freedesktop.org; dan.carpen...@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.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. 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"); > }