Message-ID: <20090908123151.1f2b18fe@jbarnes-g45>
Date:	Tue, 8 Sep 2009 12:31:51 -0700
From:	Jesse Barnes <jbarnes@...tuousgeek.org>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
Cc:	reinette chatre <reinette.chatre@...el.com>,
	"Rafael J. Wysocki" <rjw@...k.pl>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Kernel Testers List <kernel-testers@...r.kernel.org>,
	Eric Anholt <eric@...olt.net>, "Ma, Ling" <ling.ma@...el.com>,
	"bugzilla-daemon@...zilla.kernel.org" 
	<bugzilla-daemon@...zilla.kernel.org>
Subject: Re: [Bug #13819] system freeze when switching to console

On Tue, 8 Sep 2009 12:26:45 -0700 (PDT)
Linus Torvalds <torvalds@...ux-foundation.org> wrote:

> 
> 
> On Tue, 8 Sep 2009, Jesse Barnes wrote:
> > 
> > Theoretically i915_gem_idle should prevent any user interrupts from
> > coming in.
> 
> That is _entirely_ immaterial.
> 
> The thing is, interrupts can be shared. So it does not matter ONE
> WHIT that you are trying to idle the hardware - there may be _other_
> hardware in the machine that is not idle, and that raises the same
> shared interrupt. End result: the irq handler will be called, whether
> your particular hardware is idle or not.

Which is fine.  We can handle interrupts in the shared case.  It's
specific IRQ statuses we can't handle.  E.g. if we've explicitly turned
off vblank events we definitely won't expect to see them in the handler
(assuming we've taken care to barrier things like you mention below).

> So if you tear down data structures that the interrupt handler needs,
> you _ABSOLUTELY_ must first unregister the whole interrupt.
> 
> Also, even if there are no shared interrupts or any other devices,
> there can easily be old pending interrupts still queued up on
> IO-APIC's etc. So even though you quiesce the hardware, there is no
> guarantee that there aren't some pending interrupts that happened
> just before you turned off the interrupt from the hardware side, and
> are still "en route" to the CPU.

The way we barrier things should handle that case.

> Which gets us exactly the same rule as if there were shared
> interrupts: if your interrupt handler depends on some data structure,
> you must tear down the interrupt handler _before_ you tear down the
> data structures it depends on (and in the reverse order when setting
> things up, of course).
> 
> > If we uninstall the IRQ first we i915_gem_idle probably
> > won't work anymore, since it queues an interrupt and waits for it.
> 
> So then you'd better fix that. Because the code as is is very 
> fundamentally buggy.
> 
> > Eric, any thoughts on this?  We shouldn't be racing to queue new
> > work after the idle call since we suspend GEM at that point, so we
> > must be failing to manage our active lists properly somehow?
> 
> See my previous email. The bug is that you do
> 
>   i915_gem_cleanup_ringbuffer ->
>     i915_gem_cleanup_hws ->
>       dev_priv->hw_status_page = NULL;
> 
> while interrupts are still enabled and coming in. And the interrupt
> path wants to access that hw_status_page. Which you just destroyed.

Yeah, saw that.  I don't think that's the root cause though.  If we see
a user interrupt after gem_idle is called we may have serious issues in
our command handling code.

-- 
Jesse Barnes, Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists