On 01/03/2012 01:13 PM, Keith Packard wrote: > On Tue, 3 Jan 2012 20:12:35 +0100, Daniel Vetter <daniel.vetter at ffwll.ch> wrote: > >> I'm a bit confused by this. With the current code forcewake is >> protected by dev->struct_mutex. Which doesn't work out because we need >> to access registers that require the forcewake dance from non-process >> context. > > Right, I like adding a spinlock around this to allow it to be called > without needing to be able to lock the struct_mutex. (I remember > suggesting that a spinlock would be necessary when the force wake code > first showed up...) > > However, the commit message talks about the error capture and > hang check code, but doesn't appear to change them at all. > > I think all this patch does is replace the locking for forcewake_count > From struct_mutex to a new irq-safe spinlock, the commit message makes > it sound like it's actually fixing stuff, which it isn't; it just > enables fixing stuff in future patches, right? As Daniel mentioned in the commit message, it fixes existing bugs simply by using a spinlock. In the timer, we do not grab struct_mutex and there is currently a race there (which we've known about since day 1). >> Afaik the atomic ops stuff is just ducttape for paranoia reasons. > > The atomic ops stuff would allow reading of the value without holding > struct_mutex, if that were actually useful. The atomic ops stuff was simply there to help reduce the races (even if we don't have the lock, we can still safely increment the variable). It should be safe to get rid of with the spinlock in place. My only gripe here is Chris shot down my earlier version of this patch many moons ago :( Ben
