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

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

[Jiri Slaby]

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).

We will get such a warnings otherwise:
------------[ cut here ]------------
WARNING: at kernel/softirq.c:136 local_bh_enable_ip+0x8b/0xb0()
Modules linked in: arc4 ecb crypto_blkcipher cryptomgr crypto_algapi ath5k usbhid mac80211 ohci1394 hid led_class floppy cfg80211 ff_memless ieee1394 rtc_cmos evdev [last unloaded: freq_table]
Pid: 0, comm: swapper Not tainted 2.6.26-rc8-mm1_64 #427

Call Trace:
 <IRQ>  [<ffffffff8023813f>] warn_on_slowpath+0x5f/0x90
[...]
 [<ffffffff8023e1fb>] local_bh_enable_ip+0x8b/0xb0
 [<ffffffff8055b2cf>] _spin_unlock_bh+0xf/0x20
 [<ffffffff803b0bd1>] drm_lock_take+0x81/0xe0
 [<ffffffff803b006b>] drm_locked_tasklet_func+0x4b/0xb0
 [<ffffffff8023daf9>] tasklet_hi_action+0x69/0xf0
 [<ffffffff8023e3e4>] __do_softirq+0x84/0xf0
[stack snipped]

Signed-off-by: Jiri Slaby <jirislaby@gmail.com>
Cc: David Airlie <airlied@linux.ie>
---
 drivers/gpu/drm/drm_lock.c |   12 +++++++-----
 1 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_lock.c b/drivers/gpu/drm/drm_lock.c
index 0998723..79943e4 100644
--- a/drivers/gpu/drm/drm_lock.c
+++ b/drivers/gpu/drm/drm_lock.c
@@ -196,10 +196,11 @@ int drm_unlock(struct drm_device *dev, void *data, struct drm_file *file_priv)
 int drm_lock_take(struct drm_lock_data *lock_data,
 		  unsigned int context)
 {
+	unsigned long flags;
 	unsigned int old, new, prev;
 	volatile unsigned int *lock = &lock_data->hw_lock->lock;
 
-	spin_lock_bh(&lock_data->spinlock);
+	spin_lock_irqsave(&lock_data->spinlock, flags);
 	do {
 		old = *lock;
 		if (old & _DRM_LOCK_HELD)
@@ -211,7 +212,7 @@ int drm_lock_take(struct drm_lock_data *lock_data,
 		}
 		prev = cmpxchg(lock, old, new);
 	} while (prev != old);
-	spin_unlock_bh(&lock_data->spinlock);
+	spin_unlock_irqrestore(&lock_data->spinlock, flags);
 
 	if (_DRM_LOCKING_CONTEXT(old) == context) {
 		if (old & _DRM_LOCK_HELD) {
@@ -270,17 +271,18 @@ static int drm_lock_transfer(struct drm_lock_data *lock_data,
  */
 int drm_lock_free(struct drm_lock_data *lock_data, unsigned int context)
 {
+	unsigned long flags;
 	unsigned int old, new, prev;
 	volatile unsigned int *lock = &lock_data->hw_lock->lock;
 
-	spin_lock_bh(&lock_data->spinlock);
+	spin_lock_irqsave(&lock_data->spinlock, flags);
 	if (lock_data->kernel_waiters != 0) {
 		drm_lock_transfer(lock_data, 0);
 		lock_data->idle_has_lock = 1;
-		spin_unlock_bh(&lock_data->spinlock);
+		spin_unlock_irqrestore(&lock_data->spinlock, flags);
 		return 1;
 	}
-	spin_unlock_bh(&lock_data->spinlock);
+	spin_unlock_irqrestore(&lock_data->spinlock, flags);
 
 	do {
 		old = *lock;
-- 
1.5.6.2

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

[Dave Airlie]

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.

>
> We will get such a warnings otherwise:
> ------------[ cut here ]------------
> WARNING: at kernel/softirq.c:136 local_bh_enable_ip+0x8b/0xb0()
> Modules linked in: arc4 ecb crypto_blkcipher cryptomgr crypto_algapi ath5k usbhid mac80211 ohci1394 hid led_class floppy cfg80211 ff_memless ieee1394 rtc_cmos evdev [last unloaded: freq_table]
> Pid: 0, comm: swapper Not tainted 2.6.26-rc8-mm1_64 #427
>
> Call Trace:
>  <IRQ>  [<ffffffff8023813f>] warn_on_slowpath+0x5f/0x90
> [...]
>  [<ffffffff8023e1fb>] local_bh_enable_ip+0x8b/0xb0
>  [<ffffffff8055b2cf>] _spin_unlock_bh+0xf/0x20
>  [<ffffffff803b0bd1>] drm_lock_take+0x81/0xe0
>  [<ffffffff803b006b>] drm_locked_tasklet_func+0x4b/0xb0
>  [<ffffffff8023daf9>] tasklet_hi_action+0x69/0xf0
>  [<ffffffff8023e3e4>] __do_softirq+0x84/0xf0
> [stack snipped]
>
> Signed-off-by: Jiri Slaby <jirislaby@gmail.com>
> Cc: David Airlie <airlied@linux.ie>
> ---
>  drivers/gpu/drm/drm_lock.c |   12 +++++++-----
>  1 files changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_lock.c b/drivers/gpu/drm/drm_lock.c
> index 0998723..79943e4 100644
> --- a/drivers/gpu/drm/drm_lock.c
> +++ b/drivers/gpu/drm/drm_lock.c
> @@ -196,10 +196,11 @@ int drm_unlock(struct drm_device *dev, void *data, struct drm_file *file_priv)
>  int drm_lock_take(struct drm_lock_data *lock_data,
>                  unsigned int context)
>  {
> +       unsigned long flags;
>        unsigned int old, new, prev;
>        volatile unsigned int *lock = &lock_data->hw_lock->lock;
>
> -       spin_lock_bh(&lock_data->spinlock);
> +       spin_lock_irqsave(&lock_data->spinlock, flags);
>        do {
>                old = *lock;
>                if (old & _DRM_LOCK_HELD)
> @@ -211,7 +212,7 @@ int drm_lock_take(struct drm_lock_data *lock_data,
>                }
>                prev = cmpxchg(lock, old, new);
>        } while (prev != old);
> -       spin_unlock_bh(&lock_data->spinlock);
> +       spin_unlock_irqrestore(&lock_data->spinlock, flags);
>
>        if (_DRM_LOCKING_CONTEXT(old) == context) {
>                if (old & _DRM_LOCK_HELD) {
> @@ -270,17 +271,18 @@ static int drm_lock_transfer(struct drm_lock_data *lock_data,
>  */
>  int drm_lock_free(struct drm_lock_data *lock_data, unsigned int context)
>  {
> +       unsigned long flags;
>        unsigned int old, new, prev;
>        volatile unsigned int *lock = &lock_data->hw_lock->lock;
>
> -       spin_lock_bh(&lock_data->spinlock);
> +       spin_lock_irqsave(&lock_data->spinlock, flags);
>        if (lock_data->kernel_waiters != 0) {
>                drm_lock_transfer(lock_data, 0);
>                lock_data->idle_has_lock = 1;
> -               spin_unlock_bh(&lock_data->spinlock);
> +               spin_unlock_irqrestore(&lock_data->spinlock, flags);
>                return 1;
>        }
> -       spin_unlock_bh(&lock_data->spinlock);
> +       spin_unlock_irqrestore(&lock_data->spinlock, flags);
>
>        do {
>                old = *lock;
> --
> 1.5.6.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

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

[Thomas Hellström]

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.

/Thomas


>> We will get such a warnings otherwise:
>> ------------[ cut here ]------------
>> WARNING: at kernel/softirq.c:136 local_bh_enable_ip+0x8b/0xb0()
>> Modules linked in: arc4 ecb crypto_blkcipher cryptomgr crypto_algapi ath5k usbhid mac80211 ohci1394 hid led_class floppy cfg80211 ff_memless ieee1394 rtc_cmos evdev [last unloaded: freq_table]
>> Pid: 0, comm: swapper Not tainted 2.6.26-rc8-mm1_64 #427
>>
>> Call Trace:
>>  <IRQ>  [<ffffffff8023813f>] warn_on_slowpath+0x5f/0x90
>> [...]
>>  [<ffffffff8023e1fb>] local_bh_enable_ip+0x8b/0xb0
>>  [<ffffffff8055b2cf>] _spin_unlock_bh+0xf/0x20
>>  [<ffffffff803b0bd1>] drm_lock_take+0x81/0xe0
>>  [<ffffffff803b006b>] drm_locked_tasklet_func+0x4b/0xb0
>>  [<ffffffff8023daf9>] tasklet_hi_action+0x69/0xf0
>>  [<ffffffff8023e3e4>] __do_softirq+0x84/0xf0
>> [stack snipped]
>>
>> Signed-off-by: Jiri Slaby <jirislaby@gmail.com>
>> Cc: David Airlie <airlied@linux.ie>
>> ---
>>  drivers/gpu/drm/drm_lock.c |   12 +++++++-----
>>  1 files changed, 7 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_lock.c b/drivers/gpu/drm/drm_lock.c
>> index 0998723..79943e4 100644
>> --- a/drivers/gpu/drm/drm_lock.c
>> +++ b/drivers/gpu/drm/drm_lock.c
>> @@ -196,10 +196,11 @@ int drm_unlock(struct drm_device *dev, void *data, struct drm_file *file_priv)
>>  int drm_lock_take(struct drm_lock_data *lock_data,
>>                  unsigned int context)
>>  {
>> +       unsigned long flags;
>>        unsigned int old, new, prev;
>>        volatile unsigned int *lock = &lock_data->hw_lock->lock;
>>
>> -       spin_lock_bh(&lock_data->spinlock);
>> +       spin_lock_irqsave(&lock_data->spinlock, flags);
>>        do {
>>                old = *lock;
>>                if (old & _DRM_LOCK_HELD)
>> @@ -211,7 +212,7 @@ int drm_lock_take(struct drm_lock_data *lock_data,
>>                }
>>                prev = cmpxchg(lock, old, new);
>>        } while (prev != old);
>> -       spin_unlock_bh(&lock_data->spinlock);
>> +       spin_unlock_irqrestore(&lock_data->spinlock, flags);
>>
>>        if (_DRM_LOCKING_CONTEXT(old) == context) {
>>                if (old & _DRM_LOCK_HELD) {
>> @@ -270,17 +271,18 @@ static int drm_lock_transfer(struct drm_lock_data *lock_data,
>>  */
>>  int drm_lock_free(struct drm_lock_data *lock_data, unsigned int context)
>>  {
>> +       unsigned long flags;
>>        unsigned int old, new, prev;
>>        volatile unsigned int *lock = &lock_data->hw_lock->lock;
>>
>> -       spin_lock_bh(&lock_data->spinlock);
>> +       spin_lock_irqsave(&lock_data->spinlock, flags);
>>        if (lock_data->kernel_waiters != 0) {
>>                drm_lock_transfer(lock_data, 0);
>>                lock_data->idle_has_lock = 1;
>> -               spin_unlock_bh(&lock_data->spinlock);
>> +               spin_unlock_irqrestore(&lock_data->spinlock, flags);
>>                return 1;
>>        }
>> -       spin_unlock_bh(&lock_data->spinlock);
>> +       spin_unlock_irqrestore(&lock_data->spinlock, flags);
>>
>>        do {
>>                old = *lock;
>> --
>> 1.5.6.2
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/
>>
>>     

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

[Andrew Morton]

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.

The comments could quite easily be converted to kerneldoc form, which
would be the best thing to do here.

While you're there, please reformat the comments (drm_idlelock_take(),
mainly) to fit in 80-cols.  The code carefully does this, but the block
comments then go and ruin it all.

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@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.

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

[Johannes Engel]

Thomas Hellström wrote:
> 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.
Hi Thomas,

any news on that so far?

Cheers, Johannes

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

[Thomas Hellström]

[-- Attachment #1: Type: text/plain, Size: 438 bytes --]

Johannes Engel wrote:
> Thomas Hellström wrote:
>> 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.
> Hi Thomas,
>
> any news on that so far?
>
> Cheers, Johannes
Hi!
Been on vacation.

Pls try the attached patch.

/Thomas


[-- Attachment #2: 0001-Don-t-call-the-vblank-tasklet-with-irqs-disabled.patch --]
[-- Type: text/plain, Size: 2775 bytes --]

>From af12ef4f6b4ca111d9a2ef45263ad89610498724 Mon Sep 17 00:00:00 2001
From: Thomas Hellstrom <thomas-at-tungstengraphics-dot-com>
Date: Wed, 13 Aug 2008 10:04:21 +0200
Subject: [PATCH] Don't call the vblank tasklet with irqs disabled.
 If a specific tasklet shares data with irq context,
 it needs to take a private irq-blocking spinlock within
 the tasklet itself.


Signed-off-by: Thomas Hellstrom <thomas-at-tungstengraphics-dot-com>
---
 linux-core/drm_irq.c  |   20 ++++++++++++--------
 linux-core/drm_lock.c |   12 +++++-------
 2 files changed, 17 insertions(+), 15 deletions(-)

diff --git a/linux-core/drm_irq.c b/linux-core/drm_irq.c
index 5b9f474..57419ca 100644
--- a/linux-core/drm_irq.c
+++ b/linux-core/drm_irq.c
@@ -705,27 +705,31 @@ static void drm_locked_tasklet_func(unsigned long data)
 {
 	struct drm_device *dev = (struct drm_device *)data;
 	unsigned long irqflags;
-
+	void (*tasklet_func)(struct drm_device *);
+	
 	spin_lock_irqsave(&dev->tasklet_lock, irqflags);
+	tasklet_func = dev->locked_tasklet_func;
+	spin_unlock_irqrestore(&dev->tasklet_lock, irqflags);
 
-	if (!dev->locked_tasklet_func ||
+	if (!tasklet_func ||
 	    !drm_lock_take(&dev->lock,
 			   DRM_KERNEL_CONTEXT)) {
-		spin_unlock_irqrestore(&dev->tasklet_lock, irqflags);
 		return;
 	}
 
 	dev->lock.lock_time = jiffies;
 	atomic_inc(&dev->counts[_DRM_STAT_LOCKS]);
 
-	dev->locked_tasklet_func(dev);
+	spin_lock_irqsave(&dev->tasklet_lock, irqflags);
+	tasklet_func = dev->locked_tasklet_func;
+	dev->locked_tasklet_func = NULL;
+	spin_unlock_irqrestore(&dev->tasklet_lock, irqflags);
+	
+	if (tasklet_func != NULL)
+		tasklet_func(dev);
 
 	drm_lock_free(&dev->lock,
 		      DRM_KERNEL_CONTEXT);
-
-	dev->locked_tasklet_func = NULL;
-
-	spin_unlock_irqrestore(&dev->tasklet_lock, irqflags);
 }
 
 /**
diff --git a/linux-core/drm_lock.c b/linux-core/drm_lock.c
index a2966ef..cad2e44 100644
--- a/linux-core/drm_lock.c
+++ b/linux-core/drm_lock.c
@@ -155,6 +155,7 @@ int drm_unlock(struct drm_device *dev, void *data, struct drm_file *file_priv)
 {
 	struct drm_lock *lock = data;
 	unsigned long irqflags;
+	void (*tasklet_func)(struct drm_device *);
 
 	if (lock->context == DRM_KERNEL_CONTEXT) {
 		DRM_ERROR("Process %d using kernel context %d\n",
@@ -163,14 +164,11 @@ int drm_unlock(struct drm_device *dev, void *data, struct drm_file *file_priv)
 	}
 
 	spin_lock_irqsave(&dev->tasklet_lock, irqflags);
-
-	if (dev->locked_tasklet_func) {
-		dev->locked_tasklet_func(dev);
-
-		dev->locked_tasklet_func = NULL;
-	}
-
+	tasklet_func = dev->locked_tasklet_func;
+	dev->locked_tasklet_func = NULL;
 	spin_unlock_irqrestore(&dev->tasklet_lock, irqflags);
+	if (tasklet_func != NULL)
+		tasklet_func(dev);
 
 	atomic_inc(&dev->counts[_DRM_STAT_UNLOCKS]);
 
-- 
1.5.4.3

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

[Johannes Engel]

Thomas Hellström wrote:
> Johannes Engel wrote:
>> Thomas Hellström wrote:
>>> 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.
>> Hi Thomas,
>>
>> any news on that so far?
>>
>> Cheers, Johannes
> Hi!
> Been on vacation.
>
> Pls try the attached patch. 
It works for me. Thanks a lot.
Hope you had pleasant holidays. :)

Cheers, Johannes