Re: [PATCH v7 10/24] drm/xe: Update GuC submission backend to run PT jobs

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

On Fri, Sep 25, 2026 at 05:39:14AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] xe: memory leak of pt_job_ops when PT job is aborted before execution
> - [High] xe: PT jobs swallow cancellation error statuses and signal false success
> --
> 
> commit 784a2cd005e51d36d34825eebaf5abff98891eff
> Author: Matthew Brost <matthew.brost@intel.com>
> 
> drm/xe: Update GuC submission backend to run PT jobs
> 
> PT jobs bypass GPU execution for the final step of a bind job, using the
> CPU to program the required page tables. Teach the GuC submission backend
> how to execute these jobs.
> 
> > diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
> > index 4bd1ead57efa4..9faddb7c6407f 100644
> > --- a/drivers/gpu/drm/xe/xe_guc_submit.c
> > +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
> [ ... ]
> > @@ -1238,21 +1240,44 @@ static void submit_exec_queue(struct xe_exec_queue *q, struct xe_sched_job *job)
> >  static struct dma_fence *
> >  guc_exec_queue_run_job(struct drm_sched_job *drm_job)
> >  {
> >  	struct xe_sched_job *job = to_xe_sched_job(drm_job);
> >  	struct xe_exec_queue *q = job->q;
> >  	struct xe_guc *guc = exec_queue_to_guc(q);
> > -	bool killed_or_banned_or_wedged =
> > -		exec_queue_killed_or_banned_or_wedged(q);
> > +	bool killed_or_banned_or_wedged_or_error  =
> > +		exec_queue_killed_or_banned_or_wedged(q) ||
> > +		xe_sched_job_is_error(job);
> 
> [Severity: High]
> Can this condition fail to detect errors for PT jobs?
> 
> PT jobs are initialized with a global stub fence. When the queue is
> killed or banned, xe_sched_job_set_error() attempts to set an error on this
> fence but fails because the stub fence is already permanently signaled. As a
> result, xe_sched_job_is_error() will always be false for PT jobs.
> 

Non-issue - the TDR actually should never run for PT jobs, when split PT
jobs into its own backend (follow up) that will be clear, but sematic of
PT jobs is we always run them but if killed, bannded, or wedged we issue
a clear. This sematic holds.

> >  	xe_gt_assert(guc_to_gt(guc), !(exec_queue_destroyed(q) || exec_queue_pending_disable(q)) ||
> >  		     exec_queue_banned(q) || exec_queue_suspended(q));
> >  
> >  	trace_xe_sched_job_run(job);
> >  
> > -	if (!killed_or_banned_or_wedged && !xe_sched_job_is_error(job)) {
> > +	if (is_pt_job(job)) {
> > +		xe_gt_assert(guc_to_gt(guc), !exec_queue_registered(q));
> > +		run_pt_job(job, killed_or_banned_or_wedged_or_error);
> > +		xe_pt_job_ops_put(job->pt_update[0].pt_job_ops);
> 
> [Severity: High]
> Does this leak the pt_job_ops when a PT job is aborted before execution?
> 

A call _arm() guarantees a call of run_job(), thus xe_pt_job_ops_put is
always called. Non-issue.

> If the DRM scheduler is stopped, the entity killed, or the execution queue
> banned before run_job is invoked, the scheduler calls free_job directly
> without running it. The free_job callback invokes xe_sched_job_destroy(),
> which then calls xe_sched_job_free_fences(). That function explicitly skips
> PT jobs via an early return:
> 
> xe_sched_job_free_fences() {
> 	if (job->is_pt_job)
> 		return;
> 	...
> }
> 
> Since xe_pt_job_ops_put() is only called here in guc_exec_queue_run_job(),
> which is completely bypassed on this cancellation path, does the refcount
> leak?
> 
> > +		dma_fence_put(job->fence);	/* Drop ref from xe_sched_job_arm */
> > +
> > +		return NULL;
> 
> [Severity: High]
> Does this swallow cancellation error statuses and signal false success for
> PT jobs?
> 
> Even when killed_or_banned_or_wedged_or_error is true, this path
> unconditionally returns NULL. The DRM scheduler treats a NULL return from
> run_job as an immediate successful completion, signaling the software
> finished fence with 0 instead of an error (like -ECANCELED).
> 
> Userspace waiting on a failed or cancelled bind will observe false success
> and might proceed to access memory that hasn't been properly mapped.
> 

I'd say non-issue as AFIAK user space never latches onto as fence state.

Matt

> > +	} else if (!killed_or_banned_or_wedged_or_error) {
> >  		if (xe_exec_queue_is_multi_queue_secondary(q)) {
> >  			struct xe_exec_queue *primary = xe_exec_queue_multi_queue_primary(q);
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260925045320.1325860-1-matthew.brost@intel.com?part=10