On 06/10/2023 19:50, John Harrison wrote:
Tvrtko, would you have any thoughts on this one?
I wasn't really involved in that work so without digging deep can only
say that smatch seems to be noticing a genuine inconsistency. Whether or
not it is possible at runtime Matt should know better.
3e28d37146db ("drm/i915: Move priolist to new i915_sched_engine object")
is what added the if (ve->base.sched_engine) guard - maybe that isn't
needed, I don't know.
Regards,
Tvrtko
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
