* [Intel-gfx] [bug report] drm/i915: Move submission tasklet to i915_sched_engine
@ 2023-10-04 9:57 Dan Carpenter
2023-10-06 18:50 ` John Harrison
0 siblings, 1 reply; 3+ messages in thread
From: Dan Carpenter @ 2023-10-04 9:57 UTC (permalink / raw)
To: matthew.brost; +Cc: intel-gfx
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [Intel-gfx] [bug report] drm/i915: Move submission tasklet to i915_sched_engine
2023-10-04 9:57 [Intel-gfx] [bug report] drm/i915: Move submission tasklet to i915_sched_engine Dan Carpenter
@ 2023-10-06 18:50 ` John Harrison
2023-10-09 8:30 ` Tvrtko Ursulin
0 siblings, 1 reply; 3+ messages in thread
From: John Harrison @ 2023-10-06 18:50 UTC (permalink / raw)
To: Dan Carpenter, matthew.brost, Tvrtko Ursulin; +Cc: intel-gfx
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [Intel-gfx] [bug report] drm/i915: Move submission tasklet to i915_sched_engine
2023-10-06 18:50 ` John Harrison
@ 2023-10-09 8:30 ` Tvrtko Ursulin
0 siblings, 0 replies; 3+ messages in thread
From: Tvrtko Ursulin @ 2023-10-09 8:30 UTC (permalink / raw)
To: John Harrison, Dan Carpenter, matthew.brost; +Cc: intel-gfx
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
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-10-09 8:31 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-10-04 9:57 [Intel-gfx] [bug report] drm/i915: Move submission tasklet to i915_sched_engine Dan Carpenter
2023-10-06 18:50 ` John Harrison
2023-10-09 8:30 ` Tvrtko Ursulin
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.