Re: [PATCH v6 09/24] drm/xe: Add struct xe_pt_job_ops

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Missing BO refcount re-initialization causes refcount underflow and memory leak during async free.
--

commit 6e95dc31189c14b0582d03e7feb8e249921d75d3
Author: Matthew Brost <matthew.brost@intel.com>
Subject: drm/xe: Add struct xe_pt_job_ops

This commit introduces struct xe_pt_job_ops, a dynamically refcounted object
containing the information required to issue a CPU bind via a job after the
initial bind IOCTL returns.

Link: https://patch.msgid.link/20260228013501.106680-10-matthew.brost@intel.com

> diff --git a/drivers/gpu/drm/xe/xe_pt.c b/drivers/gpu/drm/xe/xe_pt.c
> index e560f167fdf1b..30127ebf1b605 100644
> --- a/drivers/gpu/drm/xe/xe_pt.c
> +++ b/drivers/gpu/drm/xe/xe_pt.c

[ ... ]

> @@ -213,7 +214,19 @@ void xe_pt_destroy(struct xe_pt *pt, u32 flags, struct llist_head *deferred)
>  
>  	XE_WARN_ON(!list_empty(&pt->bo->ttm.base.gpuva.list));
>  	xe_bo_unpin(pt->bo);
> -	xe_bo_put_deferred(pt->bo, deferred, NULL);
> +	xe_bo_put_deferred(pt->bo, deferred, &added);
> +	if (added) {
> +		xe_assert(pt->bo->vm->xe, !kref_read(&pt->bo->ttm.base.refcount));
> +
> +		/*
> +		 * We need the VM present until the BO is destroyed as it shares
> +		 * a dma-resv and BO destroy is async. Reinit BO refcount so
> +		 * xe_bo_put_async can be used when the PT job ops refcount goes
> +		 * to zero.
> +		 */
> +		xe_vm_get(pt->bo->vm);
> +		pt->bo->flags |= XE_BO_FLAG_PUT_VM_ASYNC;
> +	}

[Severity: High]
Does this code forget to actually re-initialize the BO refcount? The comment
explicitly mentions reinitializing the BO refcount so xe_bo_put_async can be
used, but it looks like only the VM is retrieved and the flag is set. Without a
call to reinitialize the kref (like kref_init for pt->bo->ttm.base.refcount), 
the refcount remains at zero.

[ ... ]

> +static void xe_pt_job_ops_destroy(struct kref *ref)
> +{
> +	struct xe_pt_job_ops *pt_job_ops =
> +		container_of(ref, struct xe_pt_job_ops, refcount);
> +	struct llist_node *freed;
> +	struct xe_bo *bo, *next;
> +
> +	xe_pt_update_ops_free(pt_job_ops->ops,
> +			      pt_job_ops->current_op);
> +
> +	freed = llist_del_all(&pt_job_ops->deferred);
> +	if (freed) {
> +		llist_for_each_entry_safe(bo, next, freed, freed) {
> +			struct xe_bo_dev *bo_device =
> +				&xe_bo_device(bo)->bo_device;
> +			/*
> +			 * If called from run_job, we are in the dma-fencing
> +			 * path and cannot take dma-resv locks so use an async
> +			 * put.
> +			 */
> +			if (llist_add(&bo->freed, &bo_device->async_list))
> +				schedule_work(&bo_device->async_free);
> +		}
> +	}

[Severity: High]
Because the BO refcount was left at zero during xe_pt_destroy, will the later
xe_bo_put in the async_free worker trigger a refcount underflow here? A kref_put
on a zero refcount returns false and skips the release function (such as
drm_gem_object_free). Can this cause the page table BO memory to be permanently
leaked?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260904211613.3934307-1-matthew.brost@intel.com?part=9