Re: [Intel-gfx] [bug report] drm/i915: Move submission tasklet to i915_sched_engine

[John Harrison <john.c.harrison@intel.com>]

Tvrtko, would you have any thoughts on this one?

John.


On 10/4/2023 02:57, Dan Carpenter wrote:
> Hello Matthew Brost,
>
> This is a semi-automatic email about new static checker warnings.
>
> The patch 22916bad07a5: "drm/i915: Move submission tasklet to
> i915_sched_engine" from Jun 17, 2021, leads to the following Smatch
> complaint:
>
>      drivers/gpu/drm/i915/gt/intel_execlists_submission.c:3659 rcu_virtual_context_destroy()
>      warn: variable dereferenced before check 've->base.sched_engine' (see line 3633)
>
> drivers/gpu/drm/i915/gt/intel_execlists_submission.c
>    3632		 */
>    3633		tasklet_kill(&ve->base.sched_engine->tasklet);
>                               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> The patch introduced a new dereference here
>
>    3634	
>    3635		/* Decouple ourselves from the siblings, no more access allowed. */
>    3636		for (n = 0; n < ve->num_siblings; n++) {
>    3637			struct intel_engine_cs *sibling = ve->siblings[n];
>    3638			struct rb_node *node = &ve->nodes[sibling->id].rb;
>    3639	
>    3640			if (RB_EMPTY_NODE(node))
>    3641				continue;
>    3642	
>    3643			spin_lock_irq(&sibling->sched_engine->lock);
>    3644	
>    3645			/* Detachment is lazily performed in the sched_engine->tasklet */
>    3646			if (!RB_EMPTY_NODE(node))
>    3647				rb_erase_cached(node, &sibling->execlists.virtual);
>    3648	
>    3649			spin_unlock_irq(&sibling->sched_engine->lock);
>    3650		}
>    3651		GEM_BUG_ON(__tasklet_is_scheduled(&ve->base.sched_engine->tasklet));
>    3652		GEM_BUG_ON(!list_empty(virtual_queue(ve)));
>    3653	
>    3654		lrc_fini(&ve->context);
>    3655		intel_context_fini(&ve->context);
>    3656	
>    3657		if (ve->base.breadcrumbs)
>    3658			intel_breadcrumbs_put(ve->base.breadcrumbs);
>    3659		if (ve->base.sched_engine)
>                      ^^^^^^^^^^^^^^^^^^^^^
> But previous code had assumed the sched_engine could be NULL.
>
>    3660			i915_sched_engine_put(ve->base.sched_engine);
>    3661		intel_engine_free_request_pool(&ve->base);
>
> regards,
> dan carpenter