mplayer v4l hangs in 2.6.25.2/4

mplayer v4l hangs in 2.6.25.2/4

[koos vriezen]

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

Hi,

I'm using mplayer for tv watch quite some kernels but 2.6.25 makes
mplayer unkillable. Last working kernel is 2.24.5 that I have tried.

This gets printed on the console

INFO: task mplayer:3465 blocked for more than 120 seconds.
"echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
mplayer       D 0000000000000000     0  3465   3464
 ffff810074333c68 0000000000200086 0000000000000000 00000000ffffffff
 ffff81007a3889c0 ffff810074333c18 ffff810074333c98 ffffffff8064a800
 ffffffff8064a800 ffffffff80646eb0 ffffffff8064a800 ffff8100757c1900
Call Trace:
 [<ffffffff8049b26f>] __mutex_lock_slowpath+0x68/0xa0
 [<ffffffff802274aa>] enqueue_task_fair+0x72/0x9b
 [<ffffffff8049b0ee>] mutex_lock+0x1a/0x1e
 [<ffffffff802265aa>] enqueue_task+0x13/0x1e
 [<ffffffff880b51ab>] :videobuf_core:videobuf_mmap_setup+0x1c/0x45
 [<ffffffff8813dbbe>] :bttv:vidiocgmbuf+0x2c/0x99
 [<ffffffff8810b91d>] :videodev:__video_do_ioctl+0xa1/0x2ad9
 [<ffffffff802272d5>] set_next_entity+0x18/0x36
 [<ffffffff8049aa70>] thread_return+0x3a/0xa4
 [<ffffffff8810e622>] :videodev:video_ioctl2+0x17c/0x210
 [<ffffffff80238c7b>] ptrace_stop+0xfc/0x140
 [<ffffffff8023a705>] ptrace_notify+0x83/0xa6
 [<ffffffff80289221>] vfs_ioctl+0x55/0x6b
 [<ffffffff8028948a>] do_vfs_ioctl+0x253/0x264
 [<ffffffff802894ec>] sys_ioctl+0x51/0x71
 [<ffffffff8020bef8>] int_very_careful+0x35/0x3e
 [<ffffffff8020be79>] tracesys+0xdc/0xe1

I've attached what might be usable wrt to this.

Please fix :-)

Best regards,

Koos Vriezen

[-- Attachment #2: mplayer.strace.gz --]
[-- Type: application/x-gzip, Size: 7996 bytes --]

[-- Attachment #3: dmesg.out.gz --]
[-- Type: application/x-gzip, Size: 8487 bytes --]

[-- Attachment #4: lsmod.out.gz --]
[-- Type: application/x-gzip, Size: 1354 bytes --]

[-- Attachment #5: lspci.out.gz --]
[-- Type: application/x-gzip, Size: 1917 bytes --]

[-- Attachment #6: config-2.6.25.4.gz --]
[-- Type: application/x-gzip, Size: 17174 bytes --]

Re: mplayer v4l hangs in 2.6.25.2/4

[Arjan van de Ven]

On Sat, 17 May 2008 18:54:06 +0200
"koos vriezen" <koos.vriezen@gmail.com> wrote:

> Hi,
> 
> I'm using mplayer for tv watch quite some kernels but 2.6.25 makes
> mplayer unkillable. Last working kernel is 2.24.5 that I have tried.
> 
> This gets printed on the console
> 
> INFO: task mplayer:3465 blocked for more than 120 seconds.
> "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this
> message. mplayer       D 0000000000000000     0  3465   3464
>  ffff810074333c68 0000000000200086 0000000000000000 00000000ffffffff
>  ffff81007a3889c0 ffff810074333c18 ffff810074333c98 ffffffff8064a800
>  ffffffff8064a800 ffffffff80646eb0 ffffffff8064a800 ffff8100757c1900
> Call Trace:
>  [<ffffffff8049b26f>] __mutex_lock_slowpath+0x68/0xa0
>  [<ffffffff802274aa>] enqueue_task_fair+0x72/0x9b
>  [<ffffffff8049b0ee>] mutex_lock+0x1a/0x1e
>  [<ffffffff802265aa>] enqueue_task+0x13/0x1e
>  [<ffffffff880b51ab>] :videobuf_core:videobuf_mmap_setup+0x1c/0x45
>  [<ffffffff8813dbbe>] :bttv:vidiocgmbuf+0x2c/0x99
>  [<ffffffff8810b91d>] :videodev:__video_do_ioctl+0xa1/0x2ad9
>  [<ffffffff802272d5>] set_next_entity+0x18/0x36
>  [<ffffffff8049aa70>] thread_return+0x3a/0xa4
>  [<ffffffff8810e622>] :videodev:video_ioctl2+0x17c/0x210
>  [<ffffffff80238c7b>] ptrace_stop+0xfc/0x140
>  [<ffffffff8023a705>] ptrace_notify+0x83/0xa6
>  [<ffffffff80289221>] vfs_ioctl+0x55/0x6b
>  [<ffffffff8028948a>] do_vfs_ioctl+0x253/0x264
>  [<ffffffff802894ec>] sys_ioctl+0x51/0x71
>  [<ffffffff8020bef8>] int_very_careful+0x35/0x3e
>  [<ffffffff8020be79>] tracesys+0xdc/0xe1
> 
> I've attached what might be usable wrt to this.
> 

Hi,

this smells like a deadlock (probably in v4l). Thankfully the kernel
has a lot of diagnostics for detecting deadlocks (you just found
that the  least invasive one caught the deadlock, but unfortunately it's
also the method that yields the least information).

Is it possible for you to enable lockdep (CONFIG_PROVE_LOCKING)?
With that on, the kernel will print nicely which locks are being waited
on, and if there's a deadlock, it'll print that too. Speaking of that,
since this looks like a mutex related issue, it's worth enabling
CONFIG_DEBUG_MUTEXES as well.... more debug checks in this area.
If you also enable CONFIG_FRAME_POINTER then the backtrace will get
better too (but it's not totally required, just easier for diagnostics)

Re: mplayer v4l hangs in 2.6.25.2/4

[koos vriezen]

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

2008/5/17 Arjan van de Ven <arjan@infradead.org>:

> Is it possible for you to enable lockdep (CONFIG_PROVE_LOCKING)?
> With that on, the kernel will print nicely which locks are being waited
> on, and if there's a deadlock, it'll print that too. Speaking of that,
> since this looks like a mutex related issue, it's worth enabling
> CONFIG_DEBUG_MUTEXES as well.... more debug checks in this area.
> If you also enable CONFIG_FRAME_POINTER then the backtrace will get
> better too (but it's not totally required, just easier for diagnostics)

See attachments.
FWIW, while mplayer is hanging, I can use another Xvideo based tv player.

Regards,
Koos Vriezen

[-- Attachment #2: mplayer.out.gz --]
[-- Type: application/x-gzip, Size: 7980 bytes --]

[-- Attachment #3: config-2.6.25.4-2.gz --]
[-- Type: application/x-gzip, Size: 17193 bytes --]

[-- Attachment #4: dmesg.out-2.gz --]
[-- Type: application/x-gzip, Size: 9041 bytes --]

Re: mplayer v4l hangs in 2.6.25.2/4 (likely regression)

[Arjan van de Ven]

On Sat, 17 May 2008 21:11:35 +0200
"koos vriezen" <koos.vriezen@gmail.com> wrote:

> 2008/5/17 Arjan van de Ven <arjan@infradead.org>:
> 
> > Is it possible for you to enable lockdep (CONFIG_PROVE_LOCKING)?
> > With that on, the kernel will print nicely which locks are being
> > waited on, and if there's a deadlock, it'll print that too.
> > Speaking of that, since this looks like a mutex related issue, it's
> > worth enabling CONFIG_DEBUG_MUTEXES as well.... more debug checks
> > in this area. If you also enable CONFIG_FRAME_POINTER then the
> > backtrace will get better too (but it's not totally required, just
> > easier for diagnostics)
> 
> See attachments.
> FWIW, while mplayer is hanging, I can use another Xvideo based tv
> player.

excellent, you caught a real deadlock.

in drivers/media/bt8xx/bttv-driver.c the code looks like this:

static int vidiocgmbuf(struct file *file, void *priv, struct video_mbuf *mbuf)
{
        int retval;
        unsigned int i;
        struct bttv_fh *fh = priv;

        mutex_lock(&fh->cap.vb_lock);
        retval = videobuf_mmap_setup(&fh->cap, gbuffers, gbufsize,
                                     V4L2_MEMORY_MMAP);



and videobuf_mmap_setup is in drivers/media/videobuf-core.c:


int videobuf_mmap_setup(struct videobuf_queue *q,
                        unsigned int bcount, unsigned int bsize,
                        enum v4l2_memory memory)
{
        int ret;
        mutex_lock(&q->vb_lock);

so.. bttv first takes "fh->cap.vb_lock" in vidiocgmbuf, then calls videobuf_mmap_setup(), and the first thing that does
is to also take fh->cap.vb_lock!  This isn't even an ABBA deadlock, but a straight AA deadlock :)

According to git-blame, this code last got changed by Mauro (added to CC) with this commit:

commit 64f9477f95bf5d4ba49dc3988d47a15bc06bb5da
Author: Mauro Carvalho Chehab <mchehab@infradead.org>
Date:   Thu Jan 31 13:57:53 2008 -0300

    V4L/DVB (7121): Renames videobuf lock to vb_lock
    
    This helps to identify where vb_lock is being used, and find missusages of the
    locks.
    
    Signed-off-by: Mauro Carvalho Chehab <mchehab@infradead.org>




Just for history purposes, below is the full lockdep message:



[ INFO: possible recursive locking detected ]
2.6.25.4 #6
---------------------------------------------
mplayer/3454 is trying to acquire lock:
 (&q->vb_lock){--..}, at: [<ffffffff880d52f0>] videobuf_mmap_setup+0x1d/0x42 [videobuf_core]

but task is already holding lock:
 (&q->vb_lock){--..}, at: [<ffffffff88139dbc>] vidiocgmbuf+0x1e/0xac [bttv]

other info that might help us debug this:
1 lock held by mplayer/3454:
 #0:  (&q->vb_lock){--..}, at: [<ffffffff88139dbc>] vidiocgmbuf+0x1e/0xac [bttv]

stack backtrace:
Pid: 3454, comm: mplayer Not tainted 2.6.25.4 #6

Call Trace:
 [<ffffffff8024df94>] __lock_acquire+0x8b7/0xc60
 [<ffffffff880d52f0>] ? :videobuf_core:videobuf_mmap_setup+0x1d/0x42
 [<ffffffff8024e72d>] lock_acquire+0x55/0x6e
 [<ffffffff880d52f0>] ? :videobuf_core:videobuf_mmap_setup+0x1d/0x42
 [<ffffffff804c5790>] mutex_lock_nested+0xd9/0x255
 [<ffffffff880d52f0>] :videobuf_core:videobuf_mmap_setup+0x1d/0x42
 [<ffffffff88139dd5>] :bttv:vidiocgmbuf+0x37/0xac
 [<ffffffff88106997>] :videodev:__video_do_ioctl+0xb2/0x2e16
 [<ffffffff804c7048>] ? _spin_unlock_irq+0x2b/0x31
 [<ffffffff80286355>] ? __kmalloc+0xbd/0xe7
 [<ffffffff8024d25d>] ? trace_hardirqs_on+0xf1/0x115
 [<ffffffff88109934>] ? :videodev:video_ioctl2+0xe0/0x259
 [<ffffffff88109a0c>] :videodev:video_ioctl2+0x1b8/0x259
 [<ffffffff804c7048>] ? _spin_unlock_irq+0x2b/0x31
 [<ffffffff8024d25d>] ? trace_hardirqs_on+0xf1/0x115
 [<ffffffff804c7048>] ? _spin_unlock_irq+0x2b/0x31
 [<ffffffff80295b1a>] vfs_ioctl+0x5e/0x77

Re: mplayer v4l hangs in 2.6.25.2/4 (likely regression)

[Arjan van de Ven]

and here is an (untest) patch that should fix this problem:
Koos, can you apply this to your kernel tree and report back if this
fixes your deadlock?



From: Arjan van de Ven <arjan@linux.intel.com>
Subject: [PATCH] Fix a deadlock in the bttv driver

vidiocgmbuf() does this:
        mutex_lock(&fh->cap.vb_lock);
        retval = videobuf_mmap_setup(&fh->cap, gbuffers, gbufsize,
                                     V4L2_MEMORY_MMAP);

and videobuf_mmap_setup() then just does
        mutex_lock(&q->vb_lock);
        ret = __videobuf_mmap_setup(q, bcount, bsize, memory);
        mutex_unlock(&q->vb_lock);

which is an obvious double-take deadlock.

This patch fixes this by having vidiocgmbuf() just call the __videobuf_mmap_setup
function instead.

Reported-by: Koos Vriezen <koos.vriezen@gmail.com>
Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>
---
 drivers/media/video/bt8xx/bttv-driver.c |    2 +-
 drivers/media/video/videobuf-core.c     |    3 ++-
 include/media/videobuf-core.h           |    3 +++
 3 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/media/video/bt8xx/bttv-driver.c b/drivers/media/video/bt8xx/bttv-driver.c
index 2ca3e9c..0165aac 100644
--- a/drivers/media/video/bt8xx/bttv-driver.c
+++ b/drivers/media/video/bt8xx/bttv-driver.c
@@ -2613,7 +2613,7 @@ static int vidiocgmbuf(struct file *file, void *priv, struct video_mbuf *mbuf)
 	struct bttv_fh *fh = priv;
 
 	mutex_lock(&fh->cap.vb_lock);
-	retval = videobuf_mmap_setup(&fh->cap, gbuffers, gbufsize,
+	retval = __videobuf_mmap_setup(&fh->cap, gbuffers, gbufsize,
 				     V4L2_MEMORY_MMAP);
 	if (retval < 0) {
 		mutex_unlock(&fh->cap.vb_lock);
diff --git a/drivers/media/video/videobuf-core.c b/drivers/media/video/videobuf-core.c
index 982f446..0a88c44 100644
--- a/drivers/media/video/videobuf-core.c
+++ b/drivers/media/video/videobuf-core.c
@@ -331,7 +331,7 @@ int videobuf_mmap_free(struct videobuf_queue *q)
 }
 
 /* Locking: Caller holds q->vb_lock */
-static int __videobuf_mmap_setup(struct videobuf_queue *q,
+int __videobuf_mmap_setup(struct videobuf_queue *q,
 			unsigned int bcount, unsigned int bsize,
 			enum v4l2_memory memory)
 {
@@ -1129,6 +1129,7 @@ EXPORT_SYMBOL_GPL(videobuf_read_stream);
 EXPORT_SYMBOL_GPL(videobuf_read_one);
 EXPORT_SYMBOL_GPL(videobuf_poll_stream);
 
+EXPORT_SYMBOL_GPL(__videobuf_mmap_setup);
 EXPORT_SYMBOL_GPL(videobuf_mmap_setup);
 EXPORT_SYMBOL_GPL(videobuf_mmap_free);
 EXPORT_SYMBOL_GPL(videobuf_mmap_mapper);
diff --git a/include/media/videobuf-core.h b/include/media/videobuf-core.h
index 5b39a22..874f134 100644
--- a/include/media/videobuf-core.h
+++ b/include/media/videobuf-core.h
@@ -237,6 +237,9 @@ unsigned int videobuf_poll_stream(struct file *file,
 int videobuf_mmap_setup(struct videobuf_queue *q,
 			unsigned int bcount, unsigned int bsize,
 			enum v4l2_memory memory);
+int __videobuf_mmap_setup(struct videobuf_queue *q,
+			unsigned int bcount, unsigned int bsize,
+			enum v4l2_memory memory);
 int videobuf_mmap_free(struct videobuf_queue *q);
 int videobuf_mmap_mapper(struct videobuf_queue *q,
 			 struct vm_area_struct *vma);
-- 
1.5.4.5

Re: mplayer v4l hangs in 2.6.25.2/4 (likely regression)

[koos vriezen]

2008/5/17 Arjan van de Ven <arjan@infradead.org>:

> > so.. bttv first takes "fh->cap.vb_lock" in vidiocgmbuf, then calls videobuf_mmap_setup(), and the first thing that does
is to also take fh->cap.vb_lock!  This isn't even an ABBA deadlock,
but a straight AA deadlock :)

Looks like I'm the only one actually running this code ;-)

> and here is an (untest) patch that should fix this problem:
> Koos, can you apply this to your kernel tree and report back if this
> fixes your deadlock?

patching file drivers/media/video/bt8xx/bttv-driver.c
patching file drivers/media/video/videobuf-core.c
Hunk #1 succeeded at 335 (offset 4 lines).
Hunk #2 succeeded at 1093 (offset -36 lines).
patching file include/media/videobuf-core.h
Hunk #1 succeeded at 227 (offset -10 lines).

Deadlock is gone, only mplayer fails to unmute the audio. Again the
Xvideo player works okay (with audio).
Hmm, using v4l2 (mplayer .. -tv noaudio:driver=v4l2:devic..) works
with audio now (used to be the other way around). I can live with
that.

Thanks for your help,
Koos

Re: mplayer v4l hangs in 2.6.25.2/4 (likely regression)

[Arjan van de Ven]

On Sat, 17 May 2008 22:06:12 +0200
"koos vriezen" <koos.vriezen@gmail.com> wrote:

> 2008/5/17 Arjan van de Ven <arjan@infradead.org>:
> 
> > > so.. bttv first takes "fh->cap.vb_lock" in vidiocgmbuf, then
> > > calls videobuf_mmap_setup(), and the first thing that does
> is to also take fh->cap.vb_lock!  This isn't even an ABBA deadlock,
> but a straight AA deadlock :)
> 
> Looks like I'm the only one actually running this code ;-)
> 
> > and here is an (untest) patch that should fix this problem:
> > Koos, can you apply this to your kernel tree and report back if this
> > fixes your deadlock?
> 
> patching file drivers/media/video/bt8xx/bttv-driver.c
> patching file drivers/media/video/videobuf-core.c
> Hunk #1 succeeded at 335 (offset 4 lines).
> Hunk #2 succeeded at 1093 (offset -36 lines).
> patching file include/media/videobuf-core.h
> Hunk #1 succeeded at 227 (offset -10 lines).
> 
> Deadlock is gone, only mplayer fails to unmute the audio. 

that's something else entirely ;)

I'll call your deadlock bug fixed by the patch...

Mauro: will you send this on to Linus or should this go direct?

From: Arjan van de Ven <arjan@linux.intel.com>
Subject: [PATCH] Fix a deadlock in the bttv driver

vidiocgmbuf() does this:
        mutex_lock(&fh->cap.vb_lock);
        retval = videobuf_mmap_setup(&fh->cap, gbuffers, gbufsize,
                                     V4L2_MEMORY_MMAP);

and videobuf_mmap_setup() then just does
        mutex_lock(&q->vb_lock);
        ret = __videobuf_mmap_setup(q, bcount, bsize, memory);
        mutex_unlock(&q->vb_lock);

which is an obvious double-take deadlock.

This patch fixes this by having vidiocgmbuf() just call the __videobuf_mmap_setup
function instead.

Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>
---
 drivers/media/video/bt8xx/bttv-driver.c |    2 +-
 drivers/media/video/videobuf-core.c     |    3 ++-
 include/media/videobuf-core.h           |    3 +++
 3 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/media/video/bt8xx/bttv-driver.c b/drivers/media/video/bt8xx/bttv-driver.c
index 2ca3e9c..0165aac 100644
--- a/drivers/media/video/bt8xx/bttv-driver.c
+++ b/drivers/media/video/bt8xx/bttv-driver.c
@@ -2613,7 +2613,7 @@ static int vidiocgmbuf(struct file *file, void *priv, struct video_mbuf *mbuf)
 	struct bttv_fh *fh = priv;
 
 	mutex_lock(&fh->cap.vb_lock);
-	retval = videobuf_mmap_setup(&fh->cap, gbuffers, gbufsize,
+	retval = __videobuf_mmap_setup(&fh->cap, gbuffers, gbufsize,
 				     V4L2_MEMORY_MMAP);
 	if (retval < 0) {
 		mutex_unlock(&fh->cap.vb_lock);
diff --git a/drivers/media/video/videobuf-core.c b/drivers/media/video/videobuf-core.c
index 982f446..0a88c44 100644
--- a/drivers/media/video/videobuf-core.c
+++ b/drivers/media/video/videobuf-core.c
@@ -331,7 +331,7 @@ int videobuf_mmap_free(struct videobuf_queue *q)
 }
 
 /* Locking: Caller holds q->vb_lock */
-static int __videobuf_mmap_setup(struct videobuf_queue *q,
+int __videobuf_mmap_setup(struct videobuf_queue *q,
 			unsigned int bcount, unsigned int bsize,
 			enum v4l2_memory memory)
 {
@@ -1129,6 +1129,7 @@ EXPORT_SYMBOL_GPL(videobuf_read_stream);
 EXPORT_SYMBOL_GPL(videobuf_read_one);
 EXPORT_SYMBOL_GPL(videobuf_poll_stream);
 
+EXPORT_SYMBOL_GPL(__videobuf_mmap_setup);
 EXPORT_SYMBOL_GPL(videobuf_mmap_setup);
 EXPORT_SYMBOL_GPL(videobuf_mmap_free);
 EXPORT_SYMBOL_GPL(videobuf_mmap_mapper);
diff --git a/include/media/videobuf-core.h b/include/media/videobuf-core.h
index 5b39a22..874f134 100644
--- a/include/media/videobuf-core.h
+++ b/include/media/videobuf-core.h
@@ -237,6 +237,9 @@ unsigned int videobuf_poll_stream(struct file *file,
 int videobuf_mmap_setup(struct videobuf_queue *q,
 			unsigned int bcount, unsigned int bsize,
 			enum v4l2_memory memory);
+int __videobuf_mmap_setup(struct videobuf_queue *q,
+			unsigned int bcount, unsigned int bsize,
+			enum v4l2_memory memory);
 int videobuf_mmap_free(struct videobuf_queue *q);
 int videobuf_mmap_mapper(struct videobuf_queue *q,
 			 struct vm_area_struct *vma);
-- 
1.5.4.5

Re: mplayer v4l hangs in 2.6.25.2/4 (likely regression)

[Willy Tarreau]

On Sat, May 17, 2008 at 01:20:50PM -0700, Arjan van de Ven wrote:
> On Sat, 17 May 2008 22:06:12 +0200
> "koos vriezen" <koos.vriezen@gmail.com> wrote:
> 
> > 2008/5/17 Arjan van de Ven <arjan@infradead.org>:
> > 
> > > > so.. bttv first takes "fh->cap.vb_lock" in vidiocgmbuf, then
> > > > calls videobuf_mmap_setup(), and the first thing that does
> > is to also take fh->cap.vb_lock!  This isn't even an ABBA deadlock,
> > but a straight AA deadlock :)
> > 
> > Looks like I'm the only one actually running this code ;-)
> > 
> > > and here is an (untest) patch that should fix this problem:
> > > Koos, can you apply this to your kernel tree and report back if this
> > > fixes your deadlock?
> > 
> > patching file drivers/media/video/bt8xx/bttv-driver.c
> > patching file drivers/media/video/videobuf-core.c
> > Hunk #1 succeeded at 335 (offset 4 lines).
> > Hunk #2 succeeded at 1093 (offset -36 lines).
> > patching file include/media/videobuf-core.h
> > Hunk #1 succeeded at 227 (offset -10 lines).
> > 
> > Deadlock is gone, only mplayer fails to unmute the audio. 
> 
> that's something else entirely ;)
> 
> I'll call your deadlock bug fixed by the patch...
> 
> Mauro: will you send this on to Linus or should this go direct?

also please do not forget to CC stable.

Willy

Re: mplayer v4l hangs in 2.6.25.2/4 (likely regression)

[Mauro Carvalho Chehab]

Hi Arjan,

Sorry for a late answer. I'm currently travelling on a short vacations.

On Sat, 17 May 2008 13:20:50 -0700
Arjan van de Ven <arjan@infradead.org> wrote:

> On Sat, 17 May 2008 22:06:12 +0200
> "koos vriezen" <koos.vriezen@gmail.com> wrote:
> 
> > 2008/5/17 Arjan van de Ven <arjan@infradead.org>:
> > 
> > > > so.. bttv first takes "fh->cap.vb_lock" in vidiocgmbuf, then
> > > > calls videobuf_mmap_setup(), and the first thing that does
> > is to also take fh->cap.vb_lock!  This isn't even an ABBA deadlock,
> > but a straight AA deadlock :)
> > 
> > Looks like I'm the only one actually running this code ;-)
> > 
> > > and here is an (untest) patch that should fix this problem:
> > > Koos, can you apply this to your kernel tree and report back if this
> > > fixes your deadlock?
> > 
> > patching file drivers/media/video/bt8xx/bttv-driver.c
> > patching file drivers/media/video/videobuf-core.c
> > Hunk #1 succeeded at 335 (offset 4 lines).
> > Hunk #2 succeeded at 1093 (offset -36 lines).
> > patching file include/media/videobuf-core.h
> > Hunk #1 succeeded at 227 (offset -10 lines).
> > 
> > Deadlock is gone, only mplayer fails to unmute the audio.

I have a trivial patch to fix this already. I'll add it soon to my tree and
send Linus.
> 
> that's something else entirely ;)
> 
> I'll call your deadlock bug fixed by the patch...
> 
> Mauro: will you send this on to Linus or should this go direct?

The fix looks sane. I prefer to just remove the lock call from bttv, instead of
calling  __videobuf_mmap_setup() and make this symbol global. The better is to
avoid locking inside the drivers, except during the interrupts, and inside
videbuf code. This is what we're doing on the other drivers.

So, it would be better if you could change your patch to remove the lock/unlock
at bttv-driver handling for this ioctl.

To solve the bug, both ways work, so:

Acked-by: Mauro Carvalho Chehab <mchehab@infradead.org>

Could you please send it to Linus and to -stable for me? Otherwise, I'll do
this by Monday.

Cheers,
Mauro

Re: mplayer v4l hangs in 2.6.25.2/4 (likely regression)

[Arjan van de Ven]

On Tue, 20 May 2008 08:49:11 -0300
Mauro Carvalho Chehab <mchehab@infradead.org> wrote:

> > Mauro: will you send this on to Linus or should this go direct?
> 
> The fix looks sane. I prefer to just remove the lock call from bttv,
> instead of calling  __videobuf_mmap_setup() and make this symbol
> global. The better is to avoid locking inside the drivers, except
> during the interrupts, and inside videbuf code. This is what we're
> doing on the other drivers.
> 
> So, it would be better if you could change your patch to remove the
> lock/unlock at bttv-driver handling for this ioctl.

but it's more complex than I want to deal with right now.. lets changing
the locking in .27 instead rather than during an -rc
> 
> To solve the bug, both ways work, so:
> 
> Acked-by: Mauro Carvalho Chehab <mchehab@infradead.org>
> 
> Could you please send it to Linus and to -stable for me? Otherwise,
> I'll do this by Monday.
> 

Sure

Linus, please apply this bugfix:


From: Arjan van de Ven <arjan@linux.intel.com>
Subject: [PATCH] Fix a deadlock in the bttv driver

vidiocgmbuf() does this:
        mutex_lock(&fh->cap.vb_lock);
        retval = videobuf_mmap_setup(&fh->cap, gbuffers, gbufsize,
                                     V4L2_MEMORY_MMAP);

and videobuf_mmap_setup() then just does
        mutex_lock(&q->vb_lock);
        ret = __videobuf_mmap_setup(q, bcount, bsize, memory);
        mutex_unlock(&q->vb_lock);

which is an obvious double-take deadlock.

This patch fixes this by having vidiocgmbuf() just call the __videobuf_mmap_setup
function instead.

Acked-by: Mauro Carvalho Chehab <mchehab@infradead.org> 
Reported-by: Koos Vriezen <koos.vriezen@gmail.com>
Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>
---
 drivers/media/video/bt8xx/bttv-driver.c |    2 +-
 drivers/media/video/videobuf-core.c     |    3 ++-
 include/media/videobuf-core.h           |    3 +++
 3 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/media/video/bt8xx/bttv-driver.c b/drivers/media/video/bt8xx/bttv-driver.c
index 2ca3e9c..0165aac 100644
--- a/drivers/media/video/bt8xx/bttv-driver.c
+++ b/drivers/media/video/bt8xx/bttv-driver.c
@@ -2613,7 +2613,7 @@ static int vidiocgmbuf(struct file *file, void *priv, struct video_mbuf *mbuf)
 	struct bttv_fh *fh = priv;
 
 	mutex_lock(&fh->cap.vb_lock);
-	retval = videobuf_mmap_setup(&fh->cap, gbuffers, gbufsize,
+	retval = __videobuf_mmap_setup(&fh->cap, gbuffers, gbufsize,
 				     V4L2_MEMORY_MMAP);
 	if (retval < 0) {
 		mutex_unlock(&fh->cap.vb_lock);
diff --git a/drivers/media/video/videobuf-core.c b/drivers/media/video/videobuf-core.c
index 982f446..0a88c44 100644
--- a/drivers/media/video/videobuf-core.c
+++ b/drivers/media/video/videobuf-core.c
@@ -331,7 +331,7 @@ int videobuf_mmap_free(struct videobuf_queue *q)
 }
 
 /* Locking: Caller holds q->vb_lock */
-static int __videobuf_mmap_setup(struct videobuf_queue *q,
+int __videobuf_mmap_setup(struct videobuf_queue *q,
 			unsigned int bcount, unsigned int bsize,
 			enum v4l2_memory memory)
 {
@@ -1129,6 +1129,7 @@ EXPORT_SYMBOL_GPL(videobuf_read_stream);
 EXPORT_SYMBOL_GPL(videobuf_read_one);
 EXPORT_SYMBOL_GPL(videobuf_poll_stream);
 
+EXPORT_SYMBOL_GPL(__videobuf_mmap_setup);
 EXPORT_SYMBOL_GPL(videobuf_mmap_setup);
 EXPORT_SYMBOL_GPL(videobuf_mmap_free);
 EXPORT_SYMBOL_GPL(videobuf_mmap_mapper);
diff --git a/include/media/videobuf-core.h b/include/media/videobuf-core.h
index 5b39a22..874f134 100644
--- a/include/media/videobuf-core.h
+++ b/include/media/videobuf-core.h
@@ -237,6 +237,9 @@ unsigned int videobuf_poll_stream(struct file *file,
 int videobuf_mmap_setup(struct videobuf_queue *q,
 			unsigned int bcount, unsigned int bsize,
 			enum v4l2_memory memory);
+int __videobuf_mmap_setup(struct videobuf_queue *q,
+			unsigned int bcount, unsigned int bsize,
+			enum v4l2_memory memory);
 int videobuf_mmap_free(struct videobuf_queue *q);
 int videobuf_mmap_mapper(struct videobuf_queue *q,
 			 struct vm_area_struct *vma);
-- 
1.5.4.5

Re: mplayer v4l hangs in 2.6.25.2/4 (likely regression)

[koos vriezen]

2008/5/17 koos vriezen <koos.vriezen@gmail.com>:
> 2008/5/17 Arjan van de Ven <arjan@infradead.org>:
>
>> > so.. bttv first takes "fh->cap.vb_lock" in vidiocgmbuf, then calls videobuf_mmap_setup(), and the first thing that does
> is to also take fh->cap.vb_lock!  This isn't even an ABBA deadlock,
> but a straight AA deadlock :)
>
> Looks like I'm the only one actually running this code ;-)

Just to emphases the happy ever after, this TV card is from '97. So no
offense meant.

> Hmm, using v4l2 (mplayer .. -tv noaudio:driver=v4l2:devic..) works
> with audio now (used to be the other way around). I can live with
> that.

And I'm very glad that I can finally drop the v4l1 compat support.

Best regards,

Koos Vriezen