On 2025-02-21 4:23, Zhu Lingshan wrote:
> This commit initialized svm lists at where they are
> defined. This is defensive programing for security
> and consistency.
>
> Initalizing variables ensures that their states are
> always valid, avoiding issues caused by
> uninitialized variables that could lead to
> unpredictable behaviros.
The lists are clearly documented as output parameters in the svm_range_add function definition. I think it's more readable to do the list initialization in svm_range_add to keep the logic of the caller more readable. One suggestion inline that would move the initialization to the caller without cluttering the calling function's code.
>
> And we should not assume the callee would always
> initialize them
>
> Signed-off-by: Zhu Lingshan <lingshan.zhu@xxxxxxx>
> ---
> drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> index bd3e20d981e0..cbc997449379 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> @@ -2130,11 +2130,7 @@ svm_range_add(struct kfd_process *p, uint64_t start, uint64_t size,
>
> pr_debug("svms 0x%p [0x%llx 0x%lx]\n", &p->svms, start, last);
>
> - INIT_LIST_HEAD(update_list);
> - INIT_LIST_HEAD(insert_list);
> - INIT_LIST_HEAD(remove_list);
> INIT_LIST_HEAD(&new_list);
> - INIT_LIST_HEAD(remap_list);
>
> node = interval_tree_iter_first(&svms->objects, start, last);
> while (node) {
> @@ -3635,6 +3631,11 @@ svm_range_set_attr(struct kfd_process *p, struct mm_struct *mm,
> if (r)
> return r;
>
> + INIT_LIST_HEAD(&update_list);
> + INIT_LIST_HEAD(&insert_list);
> + INIT_LIST_HEAD(&remove_list);
> + INIT_LIST_HEAD(&remap_list);
> +
You could initialize these where they are defined by replacing the struct list_head ... definitions with
LIST_HEAD(update_list);
LIST_HEAD(insert_list);
...
Regards,
Felix
> svms = &p->svms;
>
> mutex_lock(&process_info->lock);
