On 12/18/25 16:04, Tvrtko Ursulin wrote: > Removing the output parameter from a few functions should result in more > readable code and also enables us to save some lines.
Oh, yes please. That was just because somebody had a personal preference for this coding style and it resulted in at least one CVE entry for amdgpu a couple of years ago. I should have pushed back on this harder when I've seen it initially. > > Signed-off-by: Tvrtko Ursulin <[email protected]> > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c | 86 ++++++++++----------- > drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.h | 17 ++-- > drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 58 +++++++------- > 3 files changed, 75 insertions(+), 86 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c > index 628d32fd2fae..0ab307317145 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c > @@ -67,9 +67,9 @@ static int amdgpu_bo_list_entry_cmp(const void *_a, const > void *_b) > return 0; > } > > -int amdgpu_bo_list_create(struct amdgpu_device *adev, struct drm_file *filp, > - struct drm_amdgpu_bo_list_entry *info, > - size_t num_entries, struct amdgpu_bo_list **result) > +struct amdgpu_bo_list * > +amdgpu_bo_list_create(struct amdgpu_device *adev, struct drm_file *filp, > + struct drm_amdgpu_bo_list_entry *info, size_t num_entries) > { > unsigned last_entry = 0, first_userptr = num_entries; > struct amdgpu_bo_list_entry *array; > @@ -80,7 +80,7 @@ int amdgpu_bo_list_create(struct amdgpu_device *adev, > struct drm_file *filp, > > list = kvzalloc(struct_size(list, entries, num_entries), GFP_KERNEL); > if (!list) > - return -ENOMEM; > + return ERR_PTR(-ENOMEM); > > kref_init(&list->refcount); > > @@ -136,8 +136,7 @@ int amdgpu_bo_list_create(struct amdgpu_device *adev, > struct drm_file *filp, > trace_amdgpu_cs_bo_status(list->num_entries, total_size); > > mutex_init(&list->bo_list_mutex); > - *result = list; > - return 0; > + return list; > > error_free: > for (i = 0; i < last_entry; ++i) > @@ -145,24 +144,21 @@ int amdgpu_bo_list_create(struct amdgpu_device *adev, > struct drm_file *filp, > for (i = first_userptr; i < num_entries; ++i) > amdgpu_bo_unref(&array[i].bo); > kvfree(list); > - return r; > + return ERR_PTR(r); > > } > > -int amdgpu_bo_list_get(struct amdgpu_fpriv *fpriv, u32 id, > - struct amdgpu_bo_list **result) > +struct amdgpu_bo_list *amdgpu_bo_list_get(struct amdgpu_fpriv *fpriv, u32 id) > { > struct amdgpu_bo_list *list; > > xa_lock(&fpriv->bo_list_handles); > list = xa_load(&fpriv->bo_list_handles, id); > - if (list && !kref_get_unless_zero(&list->refcount)) > - list = NULL; > + if (!list || !kref_get_unless_zero(&list->refcount)) > + list = ERR_PTR(-ENOENT); > xa_unlock(&fpriv->bo_list_handles); > > - *result = list; > - > - return list ? 0 : -ENOENT; > + return list; > } > > void amdgpu_bo_list_put(struct amdgpu_bo_list *list) > @@ -170,8 +166,8 @@ void amdgpu_bo_list_put(struct amdgpu_bo_list *list) > kref_put(&list->refcount, amdgpu_bo_list_free); > } > > -int amdgpu_bo_create_list_entry_array(struct drm_amdgpu_bo_list_in *in, > - struct drm_amdgpu_bo_list_entry > **info_param) > +struct drm_amdgpu_bo_list_entry * > +amdgpu_bo_create_list_entry_array(struct drm_amdgpu_bo_list_in *in) > { > const uint32_t info_size = sizeof(struct drm_amdgpu_bo_list_entry); > const void __user *uptr = u64_to_user_ptr(in->bo_info_ptr); > @@ -182,27 +178,24 @@ int amdgpu_bo_create_list_entry_array(struct > drm_amdgpu_bo_list_in *in, > /* copy the handle array from userspace to a kernel buffer */ > if (likely(info_size == bo_info_size)) { > info = vmemdup_array_user(uptr, bo_number, info_size); > - if (IS_ERR(info)) > - return PTR_ERR(info); > } else { > const uint32_t bytes = min(bo_info_size, info_size); > unsigned i; > > info = kvmalloc_array(bo_number, info_size, GFP_KERNEL); > if (!info) > - return -ENOMEM; > + return ERR_PTR(-ENOMEM); > > memset(info, 0, bo_number * info_size); > for (i = 0; i < bo_number; ++i, uptr += bo_info_size) { > if (copy_from_user(&info[i], uptr, bytes)) { > kvfree(info); > - return -EFAULT; > + return ERR_PTR(-EFAULT); > } > } > } > > - *info_param = info; > - return 0; > + return info; > } > > int amdgpu_bo_list_ioctl(struct drm_device *dev, void *data, > @@ -210,27 +203,24 @@ int amdgpu_bo_list_ioctl(struct drm_device *dev, void > *data, > { > struct amdgpu_fpriv *fpriv = filp->driver_priv; > struct amdgpu_device *adev = drm_to_adev(dev); > - struct drm_amdgpu_bo_list_entry *info = NULL; > struct amdgpu_bo_list *list, *prev, *curr; > - uint32_t handle = args->in.list_handle; > union drm_amdgpu_bo_list *args = data; > + uint32_t handle = args->in.list_handle; > + struct drm_amdgpu_bo_list_entry *info; > int r; > > - r = amdgpu_bo_create_list_entry_array(&args->in, &info); > - if (r) > - return r; > - > switch (args->in.operation) { > case AMDGPU_BO_LIST_OP_CREATE: > - r = amdgpu_bo_list_create(adev, filp, info, args->in.bo_number, > - &list); > - if (r) > - goto error_free; > + case AMDGPU_BO_LIST_OP_UPDATE: > + info = amdgpu_bo_create_list_entry_array(&args->in); > + if (IS_ERR(info)) > + return PTR_ERR(info); > > - r = xa_alloc(&fpriv->bo_list_handles, &handle, list, > - xa_limit_32b, GFP_KERNEL); > - if (r) > - goto error_put_list; > + list = amdgpu_bo_list_create(adev, filp, info, > + args->in.bo_number); > + kvfree(info); > + if (IS_ERR(list)) > + return PTR_ERR(list); > > break; > > @@ -242,12 +232,20 @@ int amdgpu_bo_list_ioctl(struct drm_device *dev, void > *data, > > break; > > - case AMDGPU_BO_LIST_OP_UPDATE: > - r = amdgpu_bo_list_create(adev, filp, info, args->in.bo_number, > - &list); > + default: > + return -EINVAL; > + }; > + > + switch (args->in.operation) { > + case AMDGPU_BO_LIST_OP_CREATE: > + r = xa_alloc(&fpriv->bo_list_handles, &handle, list, > + xa_limit_32b, GFP_KERNEL); > if (r) > - goto error_free; > + goto error_put_list; > > + break; > + > + case AMDGPU_BO_LIST_OP_UPDATE: > curr = xa_load(&fpriv->bo_list_handles, handle); > if (!curr) { > r = -ENOENT; > @@ -267,21 +265,17 @@ int amdgpu_bo_list_ioctl(struct drm_device *dev, void > *data, > amdgpu_bo_list_put(curr); > break; > > + case AMDGPU_BO_LIST_OP_DESTROY: > default: > - r = -EINVAL; > - goto error_free; > + /* Handled above. */ I think I prefer to keep that a single switch statement. This looks a bit mixed up to me. > } > > memset(args, 0, sizeof(*args)); > args->out.list_handle = handle; > - kvfree(info); > > return 0; > > error_put_list: > amdgpu_bo_list_put(list); > - > -error_free: > - kvfree(info); > return r; > } > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.h > b/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.h > index 0989f1090c63..085ca94f97a2 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.h > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.h > @@ -58,17 +58,16 @@ struct amdgpu_bo_list { > struct amdgpu_bo_list_entry entries[] __counted_by(num_entries); > }; > > -int amdgpu_bo_list_get(struct amdgpu_fpriv *fpriv, u32 id, > - struct amdgpu_bo_list **result); > +struct amdgpu_bo_list *amdgpu_bo_list_get(struct amdgpu_fpriv *fpriv, u32 > id); > void amdgpu_bo_list_put(struct amdgpu_bo_list *list); > -int amdgpu_bo_create_list_entry_array(struct drm_amdgpu_bo_list_in *in, > - struct drm_amdgpu_bo_list_entry > **info_param); > +struct drm_amdgpu_bo_list_entry * > +amdgpu_bo_create_list_entry_array(struct drm_amdgpu_bo_list_in *in); > > -int amdgpu_bo_list_create(struct amdgpu_device *adev, > - struct drm_file *filp, > - struct drm_amdgpu_bo_list_entry *info, > - size_t num_entries, > - struct amdgpu_bo_list **list); > +struct amdgpu_bo_list * > +amdgpu_bo_list_create(struct amdgpu_device *adev, > + struct drm_file *filp, > + struct drm_amdgpu_bo_list_entry *info, > + size_t num_entries); > > #define amdgpu_bo_list_for_each_entry(e, list) \ > for (e = list->entries; \ > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c > index 203223fd0b54..a4cdaebaefe5 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c > @@ -152,24 +152,19 @@ static int amdgpu_cs_p1_bo_handles(struct > amdgpu_cs_parser *p, > struct drm_amdgpu_bo_list_in *data) > { > struct drm_amdgpu_bo_list_entry *info; > - int r; > + struct amdgpu_bo_list *list; > > - r = amdgpu_bo_create_list_entry_array(data, &info); > - if (r) > - return r; > - > - r = amdgpu_bo_list_create(p->adev, p->filp, info, data->bo_number, > - &p->bo_list); > - if (r) > - goto error_free; > + info = amdgpu_bo_create_list_entry_array(data); > + if (IS_ERR(info)) > + return PTR_ERR(info); > > + list = amdgpu_bo_list_create(p->adev, p->filp, info, data->bo_number); > kvfree(info); > + if (IS_ERR(list)) > + return PTR_ERR(list); > + > + p->bo_list = list; > return 0; > - > -error_free: > - kvfree(info); > - > - return r; > } > > /* Copy the data from userspace and go over it the first time */ > @@ -857,6 +852,7 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser > *p, > { > struct amdgpu_fpriv *fpriv = p->filp->driver_priv; > struct ttm_operation_ctx ctx = { true, false }; > + struct amdgpu_bo_list *list = NULL; > struct amdgpu_vm *vm = &fpriv->vm; > struct amdgpu_bo_list_entry *e; > struct drm_gem_object *obj; > @@ -869,25 +865,26 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser > *p, > if (p->bo_list) > return -EINVAL; > > - r = amdgpu_bo_list_get(fpriv, cs->in.bo_list_handle, > - &p->bo_list); > - if (r) > - return r; > + list = amdgpu_bo_list_get(fpriv, cs->in.bo_list_handle); > } else if (!p->bo_list) { > /* Create a empty bo_list when no handle is provided */ I think we can drop this handling. This was just a broken fallback for the closed source driver which the OpenGL stack never used, need to double check with the RADV guys but I don't think they ever used this as well. Looks good to me otherwise, Christian. > - r = amdgpu_bo_list_create(p->adev, p->filp, NULL, 0, > - &p->bo_list); > - if (r) > - return r; > + list = amdgpu_bo_list_create(p->adev, p->filp, NULL, 0); > } > > - mutex_lock(&p->bo_list->bo_list_mutex); > + if (IS_ERR(list)) > + return PTR_ERR(list); > + else if (list) > + p->bo_list = list; > + else > + list = p->bo_list; > + > + mutex_lock(&list->bo_list_mutex); > > /* Get userptr backing pages. If pages are updated after registered > * in amdgpu_gem_userptr_ioctl(), amdgpu_cs_list_validate() will do > * amdgpu_ttm_backend_bind() to flush and invalidate new pages > */ > - amdgpu_bo_list_for_each_userptr_entry(e, p->bo_list) { > + amdgpu_bo_list_for_each_userptr_entry(e, list) { > bool userpage_invalidated = false; > struct amdgpu_bo *bo = e->bo; > > @@ -915,7 +912,7 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser > *p, > if (unlikely(r)) > goto out_free_user_pages; > > - amdgpu_bo_list_for_each_entry(e, p->bo_list) { > + amdgpu_bo_list_for_each_entry(e, list) { > /* One fence for TTM and one for each CS job */ > r = drm_exec_prepare_obj(&p->exec, &e->bo->tbo.base, > 1 + p->gang_size); > @@ -935,7 +932,7 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser > *p, > } > } > > - amdgpu_bo_list_for_each_userptr_entry(e, p->bo_list) { > + amdgpu_bo_list_for_each_userptr_entry(e, list) { > struct mm_struct *usermm; > > usermm = amdgpu_ttm_tt_get_usermm(e->bo->tbo.ttm); > @@ -988,17 +985,16 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser > *p, > p->bytes_moved_vis); > > for (i = 0; i < p->gang_size; ++i) > - amdgpu_job_set_resources(p->jobs[i], p->bo_list->gds_obj, > - p->bo_list->gws_obj, > - p->bo_list->oa_obj); > + amdgpu_job_set_resources(p->jobs[i], list->gds_obj, > + list->gws_obj, list->oa_obj); > return 0; > > out_free_user_pages: > - amdgpu_bo_list_for_each_userptr_entry(e, p->bo_list) { > + amdgpu_bo_list_for_each_userptr_entry(e, list) { > amdgpu_hmm_range_free(e->range); > e->range = NULL; > } > - mutex_unlock(&p->bo_list->bo_list_mutex); > + mutex_unlock(&list->bo_list_mutex); > return r; > } >
