Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> writes:
> Quoting Mika Kuoppala (2018-04-23 14:03:02)
>> Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> writes:
>> > @@ -148,6 +149,12 @@ static void intel_breadcrumbs_fake_irq(struct timer_list *t)
>> > if (!b->irq_armed)
>> > return;
>> >
>> > + /* If the user has disabled the fake-irq, restore the hangchecking */
>> > + if (!test_bit(engine->id, &engine->i915->gpu_error.missed_irq_rings)) {
>> > + mod_timer(&b->hangcheck, wait_timeout());
>> > + return;
>> > + }
>> > +
>>
>> Looking at the cancel_fake_irq() now, which we still need to keep as
>> sync, I think there is race introduce now as this can queue itself.
>>
>> I think we want to also change the cancel_fake_irq() to do
>> the bit clearing first, not last after the del_timer_syncs().
>
> I see what you mean, but I think we want
>
> del_timer_sync(&b->fake_irq); // may queue b->hangcheck
> del_timer_sync(&b->hangcheck);
> clear_bit(engine->id. missed_irq_rings);
>
Ok got it. Swapping the order makes the newly
queued hangcheck from fake_irq to be cancelled and
the clearing should be last.
But now as the cancel is done only in fini path,
consider adding assertion for !irq_armed in start of
cancel_fake_irq().
Reviewed-by: Mika Kuoppala <mika.kuoppala@xxxxxxxxxxxxxxx>
-Mika
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
