On Tue, Apr 17, 2012 at 10:53:24AM +0100, Chris Wilson wrote: > On Tue, 17 Apr 2012 10:29:38 +0100, Chris Wilson <chris at chris-wilson.co.uk> wrote: > > The unpin worker frees it work struct and so during intel_crtc_disable > > we should only also free the work struct if cancel_work_sync() reports > > that it successfully cancelled the work prior to it being executed and > > thus avoid the double free. > > > > The impact is only for people unloading modules during a fullscreen game > > or movie playback, so extremely small. > > Futher review (hunting for some sign of workqueue corruption, cf > https://bugs.freedesktop.org/show_bug.cgi?id=48798) says that if work is > non-NULL here it will not have been scheduled so cancel_work_sync() will > always return true. Well, I've failed to notice this while reviewing the unpin_work life-cycle ... > Which also means that we have no way of waiting upon the scheduled > unpin_work. :| ... but have noticed that we lose any reference from intel_crtc to the unpin work once it's scheduled, and checked that we properly flush the work queue: See the ordering of irq disable, flush workqueue, then crtc destroy (in mode_config_cleanup). We even have a flush_workqueu in i915_dma.c before tearing down gem, but that won't work too well now that unpin also frobs around with the fbc state (which is gone by now). But there's still the misleading comment that this syncs up with unpin work. Care to clean this up a bit by - ditching the unnecessary flush_workqueu in i915_dma.c - move the comment about syncing up with unpin_work to where we actually sync up - and kill the superflous cancel_work? Cheers, Daniel -- Daniel Vetter Mail: daniel at ffwll.ch Mobile: +41 (0)79 365 57 48
