Re: [PATCH 1/1 repost #1] DRM: don't enable irqs in locking

[Dave Airlie]

On Tue, Jul 29, 2008 at 5:31 PM, Andrew Morton
<akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
> On Mon, 28 Jul 2008 22:32:45 +0200 Thomas Hellstr__m <thomas@xxxxxxxxxxxxxxxxxxxx> wrote:
>
>> Dave Airlie wrote:
>> > On Fri, Jul 25, 2008 at 6:42 PM, Jiri Slaby <jirislaby@xxxxxxxxx> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/