Messages in this thread | | | Date | Tue, 29 Jul 2008 18:16:58 +1000 | From | "Dave Airlie" <> | Subject | Re: [PATCH 1/1 repost #1] DRM: don't enable irqs in locking |
| |
On Tue, Jul 29, 2008 at 5:31 PM, Andrew Morton <akpm@linux-foundation.org> wrote: > On Mon, 28 Jul 2008 22:32:45 +0200 Thomas Hellstr__m <thomas@tungstengraphics.com> wrote: > >> Dave Airlie wrote: >> > On Fri, Jul 25, 2008 at 6:42 PM, Jiri Slaby <jirislaby@gmail.com> wrote: >> > >> >> drm_lock_take(); and drm_lock_free(); are called from >> >> drm_locked_tasklet_func(); which disables interrupts when grabbing its >> >> spinlock. >> >> >> >> Don't allow these locking functions to re-enable interrupts when >> >> the tasklet expects them disabled. I.e. use spin_lock_irqsave instead of >> >> spin_lock_bh (with their unlock opposites). >> >> >> > >> > Hmm this has bounced through 2-3 variations.. Thomas any ideas what >> > the final correct answer is? >> > >> > Dave. >> > >> Hmm, >> Yes, this bug could occur, but the remedy is not to use >> spin_lock_irqsave() for lock_data::spinlock but to avoid calling >> drm_lock_take with the drm_device::tasklet_lock held with irqs disabled. >> I'll see if I can come up with a patch. >> > > The code in drivers/gpu/drm/drm_lock.c needs some serious help in the > kerneldoc department. > > > /** > * Take the heavyweight lock. > * > * \param lock lock pointer. > * \param context locking context. > * \return one if the lock is held, or zero otherwise. > * > * Attempt to mark the lock as held by the given context, via the \p cmpxchg instruction. > */ > > The /** leadin specifically introduces a kerneldoc-formatted comment. > Yet that comment uses some strange home-made way of denoting function > arguments.
It not homemade, its a standard used by everyone else called doxygen :-), the Mesa people wrote the drm comments so they could have them all in one format, however it probably makes sense to move the kernel side ones into kernel format.
Dave.
| |