Re: [PATCH] drm/xe/hw_engine_group: Fix potential leak

[Matthew Brost <matthew.brost@intel.com>]

On Fri, Jun 27, 2025 at 08:41:43PM +0200, Michal Wajdeczko wrote:
> If we fail to allocate a workqueue we will leak kzalloc'ed group
> object since it was designed to be kfree'ed in the drmm cleanup
> action, but we didn't have a chance to register this action yet.
> 
> To avoid this leak allocate a group object using drmm_kzalloc()
> and start using predefined drmm action to release the workqueue.
> 
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Francois Dugast <francois.dugast@intel.com>
> Cc: Matthew Brost <matthew.brost@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_hw_engine_group.c | 19 +++++--------------
>  1 file changed, 5 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_hw_engine_group.c b/drivers/gpu/drm/xe/xe_hw_engine_group.c
> index 2d68c5b5262a..87a6dcb1b4b5 100644
> --- a/drivers/gpu/drm/xe/xe_hw_engine_group.c
> +++ b/drivers/gpu/drm/xe/xe_hw_engine_group.c
> @@ -12,15 +12,6 @@
>  #include "xe_hw_engine_group.h"
>  #include "xe_vm.h"
>  
> -static void
> -hw_engine_group_free(struct drm_device *drm, void *arg)
> -{
> -	struct xe_hw_engine_group *group = arg;
> -
> -	destroy_workqueue(group->resume_wq);
> -	kfree(group);
> -}
> -
>  static void
>  hw_engine_group_resume_lr_jobs_func(struct work_struct *w)
>  {
> @@ -53,7 +44,7 @@ hw_engine_group_alloc(struct xe_device *xe)
>  	struct xe_hw_engine_group *group;
>  	int err;
>  
> -	group = kzalloc(sizeof(*group), GFP_KERNEL);
> +	group = drmm_kzalloc(&xe->drm, sizeof(*group), GFP_KERNEL);
>  	if (!group)
>  		return ERR_PTR(-ENOMEM);
>  
> @@ -61,14 +52,14 @@ hw_engine_group_alloc(struct xe_device *xe)
>  	if (!group->resume_wq)
>  		return ERR_PTR(-ENOMEM);
>  
> +	err = drmm_add_action_or_reset(&xe->drm, __drmm_workqueue_release, group->resume_wq);

I was hoping something like this would get added. I suggested a similar
idea to the Xe maintainers a while back, but they didn’t like it. It
seems someone else has now added this at the DRM level. We can probably
update Xe in several places to use drmm_alloc_ordered_workqueue, but in
this case, resume_wq is not an ordered workqueue—although maybe it
should be.

Anyway, all of the above is follow-up work. This patch itself looks good
to me.

Reviewed-by: Matthew Brost <matthew.brost@intel.com>

> +	if (err)
> +		return ERR_PTR(err);
> +
>  	init_rwsem(&group->mode_sem);
>  	INIT_WORK(&group->resume_work, hw_engine_group_resume_lr_jobs_func);
>  	INIT_LIST_HEAD(&group->exec_queue_list);
>  
> -	err = drmm_add_action_or_reset(&xe->drm, hw_engine_group_free, group);
> -	if (err)
> -		return ERR_PTR(err);
> -
>  	return group;
>  }
>  
> -- 
> 2.47.1
>