[bug report] drm/amdkfd: Extend KFD device topology to surface peer-to-peer links

[bug report] drm/amdkfd: Extend KFD device topology to surface peer-to-peer links

[Dan Carpenter]

Hello Ramesh Errabolu,

The patch 0f28cca87e9a: "drm/amdkfd: Extend KFD device topology to
surface peer-to-peer links" from May 26, 2022, leads to the following
Smatch static checker warning:

drivers/gpu/drm/amd/amdgpu/../amdkfd/kfd_topology.c:1428 kfd_create_indirect_link_prop() warn: iterator used outside loop: 'cpu_link'
drivers/gpu/drm/amd/amdgpu/../amdkfd/kfd_topology.c:1462 kfd_create_indirect_link_prop() error: we previously assumed 'cpu_dev' could be null (see line 1420)
drivers/gpu/drm/amd/amdgpu/../amdkfd/kfd_topology.c:1516 kfd_add_peer_prop() warn: iterator used outside loop: 'iolink3'
drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:4997 add_modifier() warn: use safer allocation function (eg: kmalloc_array)
drivers/gpu/drm/amd/amdgpu/../display/dc/bios/bios_parser2.c:1601 bios_parser_is_device_id_supported() warn: inconsistent indenting
drivers/gpu/drm/amd/amdgpu/../display/dc/bios/bios_parser2.c:407 get_bios_object_from_path_v3() warn: inconsistent indenting
drivers/gpu/drm/amd/amdgpu/../display/dc/bios/bios_parser2.c:613 bios_parser_get_hpd_info() warn: inconsistent indenting
drivers/gpu/drm/amd/amdgpu/../display/dc/bios/bios_parser2.c:820 bios_parser_get_device_tag() warn: inconsistent indenting
drivers/gpu/drm/amd/amdgpu/../display/dc/core/dc.c:3151 commit_planes_for_stream() warn: inconsistent indenting
drivers/gpu/drm/amd/amdgpu/../display/dc/core/dc_stream.c:395 dc_stream_set_cursor_position() warn: variable dereferenced before check 'stream' (see line 392)
drivers/gpu/drm/amd/amdgpu/../pm/swsmu/amdgpu_smu.c:140 smu_set_gfx_power_up_by_imu() error: we previously assumed 'smu->ppt_funcs' could be null (see line 140)

drivers/gpu/drm/amd/amdgpu/../amdkfd/kfd_topology.c
    1393 static int kfd_create_indirect_link_prop(struct kfd_topology_device *kdev, int gpu_node)
    1394 {
    1395         struct kfd_iolink_properties *props = NULL, *props2 = NULL;
    1396         struct kfd_iolink_properties *gpu_link, *cpu_link;
    1397         struct kfd_topology_device *cpu_dev;
    1398         int ret = 0;
    1399         int i, num_cpu;
    1400 
    1401         num_cpu = 0;
    1402         list_for_each_entry(cpu_dev, &topology_device_list, list) {
    1403                 if (cpu_dev->gpu)
    1404                         break;
    1405                 num_cpu++;
    1406         }
    1407 
    1408         gpu_link = list_first_entry(&kdev->io_link_props,
    1409                                         struct kfd_iolink_properties, list);
    1410         if (!gpu_link)
    1411                 return -ENOMEM;
    1412 
    1413         for (i = 0; i < num_cpu; i++) {
    1414                 /* CPU <--> GPU */
    1415                 if (gpu_link->node_to == i)
    1416                         continue;
    1417 
    1418                 /* find CPU <-->  CPU links */
    1419                 cpu_dev = kfd_topology_device_by_proximity_domain(i);
    1420                 if (cpu_dev) {
    1421                         list_for_each_entry(cpu_link,
    1422                                         &cpu_dev->io_link_props, list) {
    1423                                 if (cpu_link->node_to == gpu_link->node_to)
    1424                                         break;
    1425                         }

If we exit the loop without hitting the break statement then "cpu_link"
is not a valid pointer.

    1426                 }
    1427 
--> 1428                 if (cpu_link->node_to != gpu_link->node_to)

It's weird that this doesn't trigger an uninitialized variable warning.

    1429                         return -ENOMEM;
    1430 
    1431                 /* CPU <--> CPU <--> GPU, GPU node*/
    1432                 props = kfd_alloc_struct(props);
    1433                 if (!props)
    1434                         return -ENOMEM;
    1435 
    1436                 memcpy(props, gpu_link, sizeof(struct kfd_iolink_properties));
    1437                 props->weight = gpu_link->weight + cpu_link->weight;
    1438                 props->min_latency = gpu_link->min_latency + cpu_link->min_latency;
    1439                 props->max_latency = gpu_link->max_latency + cpu_link->max_latency;
    1440                 props->min_bandwidth = min(gpu_link->min_bandwidth, cpu_link->min_bandwidth);
    1441                 props->max_bandwidth = min(gpu_link->max_bandwidth, cpu_link->max_bandwidth);
    1442 
    1443                 props->node_from = gpu_node;
    1444                 props->node_to = i;
    1445                 kdev->node_props.p2p_links_count++;
    1446                 list_add_tail(&props->list, &kdev->p2p_link_props);
    1447                 ret = kfd_build_p2p_node_entry(kdev, props);
    1448                 if (ret < 0)
    1449                         return ret;
    1450 
    1451                 /* for small Bar, no CPU --> GPU in-direct links */
    1452                 if (kfd_dev_is_large_bar(kdev->gpu)) {
    1453                         /* CPU <--> CPU <--> GPU, CPU node*/
    1454                         props2 = kfd_alloc_struct(props2);
    1455                         if (!props2)
    1456                                 return -ENOMEM;
    1457 
    1458                         memcpy(props2, props, sizeof(struct kfd_iolink_properties));
    1459                         props2->node_from = i;
    1460                         props2->node_to = gpu_node;
    1461                         props2->kobj = NULL;
    1462                         cpu_dev->node_props.p2p_links_count++;

If you fix the exit condition above then probably this warning will go
away too?

    1463                         list_add_tail(&props2->list, &cpu_dev->p2p_link_props);
    1464                         ret = kfd_build_p2p_node_entry(cpu_dev, props2);
    1465                         if (ret < 0)
    1466                                 return ret;
    1467                 }
    1468         }
    1469         return ret;
    1470 }

regards,
dan carpenter