[AMD Official Use Only - AMD Internal Distribution Only] Greetings Philip,
Thanks. A few non-essential comment inline. > -----Original Message----- > From: Yang, Philip <[email protected]> > Sent: Tuesday, December 2, 2025 2:23 PM > To: Martin, Andrew <[email protected]>; amd- > [email protected] > Subject: Re: [PATCH v3] drm/amdkfd: FORWARD NULL > > > > On 2025-11-18 15:34, Andrew Martin wrote: > > This patch fixes issues when the code moves forward with a potential > > NULL pointer, without checking. > > > > Signed-off-by: Andrew Martin <[email protected]> > > --- > > drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c | 2 +- > > drivers/gpu/drm/amd/amdkfd/kfd_crat.c | 2 ++ > > drivers/gpu/drm/amd/amdkfd/kfd_debug.c | 7 ++++++- > > drivers/gpu/drm/amd/amdkfd/kfd_process.c | 3 --- > > 4 files changed, 9 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c > > index 1ef758ac5076..73c5749d4243 100644 > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c > > @@ -105,7 +105,7 @@ static const char > *amdkfd_fence_get_timeline_name(struct dma_fence *f) > > { > > struct amdgpu_amdkfd_fence *fence = to_amdgpu_amdkfd_fence(f); > > > > - return fence->timeline_name; > > + return fence ? fence->timeline_name : NULL; > > } > > > > /** > > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_crat.c > > b/drivers/gpu/drm/amd/amdkfd/kfd_crat.c > > index 4a7180b46b71..c086a9ed8c89 100644 > > --- a/drivers/gpu/drm/amd/amdkfd/kfd_crat.c > > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_crat.c > > @@ -2357,6 +2357,8 @@ static int kfd_create_vcrat_image_gpu(void > *pcrat_image, > > if (kdev->kfd->hive_id) { > > for (nid = 0; nid < proximity_domain; ++nid) { > > peer_dev = > kfd_topology_device_by_proximity_domain_no_lock(nid); > > + if (!peer_dev) > > + continue; > > if (!peer_dev->gpu) > > continue; > if (!peer_dev || !peer_dev->gpu) > continue; > > if (peer_dev->gpu->kfd->hive_id != kdev->kfd->hive_id) > diff --git > > a/drivers/gpu/drm/amd/amdkfd/kfd_debug.c > > b/drivers/gpu/drm/amd/amdkfd/kfd_debug.c > > index ba99e0f258ae..11af0c1cddcd 100644 > > --- a/drivers/gpu/drm/amd/amdkfd/kfd_debug.c > > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_debug.c > > @@ -517,7 +517,10 @@ int kfd_dbg_trap_set_flags(struct kfd_process > > *target, uint32_t *flags) > > > > for (i = 0; i < target->n_pdds; i++) { > > struct kfd_topology_device *topo_dev = > > - kfd_topology_device_by_id(target->pdds[i]- > >dev->id); > > + kfd_topology_device_by_id(target->pdds[i]->dev->id); > > + if (!topo_dev) > > + return -EINVAL; > > + > move this after variable caps definition > > uint32_t caps = topo_dev->node_props.capability; > > > uin32_t caps; > > if (!topo_dev) > return -EINVAL; > > caps = topo_dev->node_props.capability; Thanks again, I thought this flow is slightly cleaner, already in PATCH 4. ... uint32_t caps; struct kfd_topology_device *topo_dev = kfd_topology_device_by_id(target->pdds[i]->dev->id); if (!topo_dev) return -EINVAL; caps = topo_dev->node_props.capability; ... > > if (!(caps & > > HSA_CAP_TRAP_DEBUG_PRECISE_MEMORY_OPERATIONS_SUPPORTED) && > @@ -1071,6 +1074,8 @@ int kfd_dbg_trap_device_snapshot(struct > kfd_process *target, > > for (i = 0; i < tmp_num_devices; i++) { > > struct kfd_process_device *pdd = target->pdds[i]; > > struct kfd_topology_device *topo_dev = > > kfd_topology_device_by_id(pdd->dev->id); > add empty line after variable. > > + if (!topo_dev) > > + return -EINVAL; > > > break out of the loop to mutex_unlock then return Grateful for this lesson, had you not mentioned it, I would have not notice the unlock. > > if (!topo_dev) { > r = -EINVAL; > break; > } > > Regards, > Philip > > device_info.gpu_id = pdd->dev->id; > > device_info.exception_status = pdd->exception_status; diff --git > > a/drivers/gpu/drm/amd/amdkfd/kfd_process.c > > b/drivers/gpu/drm/amd/amdkfd/kfd_process.c > > index f5d173f1ca3b..888b1c24c2a6 100644 > > --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c > > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c > > @@ -1685,9 +1685,6 @@ int kfd_process_device_init_vm(struct > kfd_process_device *pdd, > > struct kfd_node *dev; > > int ret; > > > > - if (!drm_file) > > - return -EINVAL; > > - > > if (pdd->drm_priv) > > return -EBUSY; > >
