[PATCH] bttv: fix mutex use before init

[PATCH] bttv: fix mutex use before init

[Dave Young]

oops happen in bttv_open while locking uninitialized mutex fh->cap.vb_lock
add mutex_init before usage

Signed-off-by: Dave Young <hidave.darkstar@gmail.com>
Tested-by: Chris Clayton <chris2553@googlemail.com>
---
 drivers/media/video/bt8xx/bttv-driver.c |    2 ++
 1 file changed, 2 insertions(+)

--- linux-2.6.orig/drivers/media/video/bt8xx/bttv-driver.c	2010-11-27 11:21:30.000000000 +0800
+++ linux-2.6/drivers/media/video/bt8xx/bttv-driver.c	2010-12-12 16:31:39.633333338 +0800
@@ -3291,6 +3291,8 @@ static int bttv_open(struct file *file)
 	fh = kmalloc(sizeof(*fh), GFP_KERNEL);
 	if (unlikely(!fh))
 		return -ENOMEM;
+
+	mutex_init(&fh->cap.vb_lock);
 	file->private_data = fh;
 
 	/*

Re: [PATCH] bttv: fix mutex use before init

[Torsten Kaiser]

On Sun, Dec 12, 2010 at 2:15 PM, Dave Young <hidave.darkstar@gmail.com> wrote:
> oops happen in bttv_open while locking uninitialized mutex fh->cap.vb_lock
> add mutex_init before usage

I have seen the same problem twice since I switched of the BKL in
2.6.37-rc2, but only had the time today to search for the cause.
(It only happend on two boots out of probably more then 50, so I only
investigated after it happened a second time with -rc5)

The cause was making this change to bttv_open() and radio_open() to
remove the BKL from them:
+	mutex_lock(&fh->cap.vb_lock);
 	*fh = btv->init;
+	mutex_unlock(&fh->cap.vb_lock);

This is wrong because of two things:
First: The fh->cap.vb_lock has not been initialised, as you noted.
Second: The middle line will overwrite the mutex!

Just adding mutex_init() will not really help the second problem and
seems to be wrong from the point that cap.vb_lock already has a
mutex_init() via videobuf_queue_sg_init().

I'm not sure what the correct fix is, but I would rather suggest this:

 * change &fh->cap.vb_lock in bttv_open() AND radio_open() to
&btv->init.cap.vb_lock
 * add a mutex_init(&btv->init.cap.vb_lock) to the setup of init in bttv_probe()

> Signed-off-by: Dave Young <hidave.darkstar@gmail.com>
> Tested-by: Chris Clayton <chris2553@googlemail.com>
> ---
>  drivers/media/video/bt8xx/bttv-driver.c |    2 ++
>  1 file changed, 2 insertions(+)
>
> --- linux-2.6.orig/drivers/media/video/bt8xx/bttv-driver.c      2010-11-27 11:21:30.000000000 +0800
> +++ linux-2.6/drivers/media/video/bt8xx/bttv-driver.c   2010-12-12 16:31:39.633333338 +0800
> @@ -3291,6 +3291,8 @@ static int bttv_open(struct file *file)
>        fh = kmalloc(sizeof(*fh), GFP_KERNEL);
>        if (unlikely(!fh))
>                return -ENOMEM;
> +
> +       mutex_init(&fh->cap.vb_lock);
>        file->private_data = fh;
>
>        /*
> --
> 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] bttv: fix mutex use before init

[Dave Young]

On Sun, Dec 12, 2010 at 05:13:47PM +0100, Torsten Kaiser wrote:
> On Sun, Dec 12, 2010 at 2:15 PM, Dave Young <hidave.darkstar@gmail.com> wrote:
> > oops happen in bttv_open while locking uninitialized mutex fh->cap.vb_lock
> > add mutex_init before usage
> 
> I have seen the same problem twice since I switched of the BKL in
> 2.6.37-rc2, but only had the time today to search for the cause.
> (It only happend on two boots out of probably more then 50, so I only
> investigated after it happened a second time with -rc5)
> 
> The cause was making this change to bttv_open() and radio_open() to
> remove the BKL from them:
> +	mutex_lock(&fh->cap.vb_lock);
>  	*fh = btv->init;
> +	mutex_unlock(&fh->cap.vb_lock);
> 
> This is wrong because of two things:
> First: The fh->cap.vb_lock has not been initialised, as you noted.
> Second: The middle line will overwrite the mutex!
> 
> Just adding mutex_init() will not really help the second problem and
> seems to be wrong from the point that cap.vb_lock already has a
> mutex_init() via videobuf_queue_sg_init().

Yes, you are right. I wonder if the above lock code in bttv_open and radio_open can be removed or just use btv->lock.

> 
> I'm not sure what the correct fix is, but I would rather suggest this:
> 
>  * change &fh->cap.vb_lock in bttv_open() AND radio_open() to
> &btv->init.cap.vb_lock
>  * add a mutex_init(&btv->init.cap.vb_lock) to the setup of init in bttv_probe()

I will test this when the bttv card is available for me tommorrow.

> 
> > Signed-off-by: Dave Young <hidave.darkstar@gmail.com>
> > Tested-by: Chris Clayton <chris2553@googlemail.com>
> > ---
> >  drivers/media/video/bt8xx/bttv-driver.c |    2 ++
> >  1 file changed, 2 insertions(+)
> >
> > --- linux-2.6.orig/drivers/media/video/bt8xx/bttv-driver.c      2010-11-27 11:21:30.000000000 +0800
> > +++ linux-2.6/drivers/media/video/bt8xx/bttv-driver.c   2010-12-12 16:31:39.633333338 +0800
> > @@ -3291,6 +3291,8 @@ static int bttv_open(struct file *file)
> >        fh = kmalloc(sizeof(*fh), GFP_KERNEL);
> >        if (unlikely(!fh))
> >                return -ENOMEM;
> > +
> > +       mutex_init(&fh->cap.vb_lock);
> >        file->private_data = fh;
> >
> >        /*
> > --
> > 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] bttv: fix mutex use before init

[Torsten Kaiser]

On Mon, Dec 13, 2010 at 3:04 PM, Dave Young <hidave.darkstar@gmail.com> wrote:
> On Sun, Dec 12, 2010 at 05:13:47PM +0100, Torsten Kaiser wrote:
>> On Sun, Dec 12, 2010 at 2:15 PM, Dave Young <hidave.darkstar@gmail.com> wrote:
>> > oops happen in bttv_open while locking uninitialized mutex fh->cap.vb_lock
>> > add mutex_init before usage
>>
>> I have seen the same problem twice since I switched of the BKL in
>> 2.6.37-rc2, but only had the time today to search for the cause.
>> (It only happend on two boots out of probably more then 50, so I only
>> investigated after it happened a second time with -rc5)
>>
>> The cause was making this change to bttv_open() and radio_open() to
>> remove the BKL from them:
>> +     mutex_lock(&fh->cap.vb_lock);
>>       *fh = btv->init;
>> +     mutex_unlock(&fh->cap.vb_lock);
>>
>> This is wrong because of two things:
>> First: The fh->cap.vb_lock has not been initialised, as you noted.
>> Second: The middle line will overwrite the mutex!
>>
>> Just adding mutex_init() will not really help the second problem and
>> seems to be wrong from the point that cap.vb_lock already has a
>> mutex_init() via videobuf_queue_sg_init().
>
> Yes, you are right. I wonder if the above lock code in bttv_open and radio_open can be removed or just use btv->lock.

In bttv_open there is a comment directly above this lock/unlock
sequence that explains why cap.vb_lock was used. As that comment
sounded correct, I did not look further into changing it.
Hmmh: I'm not sure, if I understand how this locking should work. It
looks like each bttv_open() call will create a bttv_fh structure with
its own cap.vb_lock. So if there ever is more then one bttv_fh for one
btv, there is nothing that prevents concurrent access to btv->init,
because there are two different cap.vb_lock. But if there always is
only one bttv_fh, then why allocate it at all and not exclusively use
btv->init, as that one will always be there already.
Disclaimer: I only started looking into bttv-driver.c yesterday when I
was trying to find out, why I got these OOPSes. I probably missed
something there...

>> I'm not sure what the correct fix is, but I would rather suggest this:
>>
>>  * change &fh->cap.vb_lock in bttv_open() AND radio_open() to
>> &btv->init.cap.vb_lock
>>  * add a mutex_init(&btv->init.cap.vb_lock) to the setup of init in bttv_probe()
>
> I will test this when the bttv card is available for me tommorrow.

I do have a bttv card, so if you want me to test something, I could do this.
Although: the current version of the code works for me. Even after the
OOPS during bootup, I was later able to watch TV with that card.

>> > Signed-off-by: Dave Young <hidave.darkstar@gmail.com>
>> > Tested-by: Chris Clayton <chris2553@googlemail.com>
>> > ---
>> >  drivers/media/video/bt8xx/bttv-driver.c |    2 ++
>> >  1 file changed, 2 insertions(+)
>> >
>> > --- linux-2.6.orig/drivers/media/video/bt8xx/bttv-driver.c      2010-11-27 11:21:30.000000000 +0800
>> > +++ linux-2.6/drivers/media/video/bt8xx/bttv-driver.c   2010-12-12 16:31:39.633333338 +0800
>> > @@ -3291,6 +3291,8 @@ static int bttv_open(struct file *file)
>> >        fh = kmalloc(sizeof(*fh), GFP_KERNEL);
>> >        if (unlikely(!fh))
>> >                return -ENOMEM;
>> > +
>> > +       mutex_init(&fh->cap.vb_lock);
>> >        file->private_data = fh;
>> >
>> >        /*
>> > --
>> > 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] bttv: fix mutex use before init

[Brandon Philips]

On 17:13 Sun 12 Dec 2010, Torsten Kaiser wrote:
>  * change &fh->cap.vb_lock in bttv_open() AND radio_open() to
> &btv->init.cap.vb_lock
>  * add a mutex_init(&btv->init.cap.vb_lock) to the setup of init in bttv_probe()

That seems like a reasonable suggestion. An openSUSE user submitted this
bug to our tracker too. Here is the patch I am having him test.

Would you mind testing it?

>From 456dc0ce36db523c4c0c8a269f4eec43a72de1dc Mon Sep 17 00:00:00 2001
From: Brandon Philips <bphilips@suse.de>
Date: Mon, 13 Dec 2010 16:21:55 -0800
Subject: [PATCH] bttv: fix locking for btv->init

Fix locking for the btv->init by using btv->init.cap.vb_lock and in the
process fix uninitialized deref introduced in c37db91fd0d.

Signed-off-by: Brandon Philips <bphilips@suse.de>
---
 drivers/media/video/bt8xx/bttv-driver.c |   24 +++++++++++++-----------
 1 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/drivers/media/video/bt8xx/bttv-driver.c b/drivers/media/video/bt8xx/bttv-driver.c
index a529619..e656424 100644
--- a/drivers/media/video/bt8xx/bttv-driver.c
+++ b/drivers/media/video/bt8xx/bttv-driver.c
@@ -2391,16 +2391,11 @@ static int setup_window_lock(struct bttv_fh *fh, struct bttv *btv,
 	fh->ov.field    = win->field;
 	fh->ov.setup_ok = 1;
 
-	/*
-	 * FIXME: btv is protected by btv->lock mutex, while btv->init
-	 *	  is protected by fh->cap.vb_lock. This seems to open the
-	 *	  possibility for some race situations. Maybe the better would
-	 *	  be to unify those locks or to use another way to store the
-	 *	  init values that will be consumed by videobuf callbacks
-	 */
+	mutex_lock(&btv->init.cap.vb_lock);
 	btv->init.ov.w.width   = win->w.width;
 	btv->init.ov.w.height  = win->w.height;
 	btv->init.ov.field     = win->field;
+	mutex_unlock(&btv->init.cap.vb_lock);
 
 	/* update overlay if needed */
 	retval = 0;
@@ -2620,9 +2615,11 @@ static int bttv_s_fmt_vid_cap(struct file *file, void *priv,
 	fh->cap.last         = V4L2_FIELD_NONE;
 	fh->width            = f->fmt.pix.width;
 	fh->height           = f->fmt.pix.height;
+	mutex_lock(&btv->init.cap.vb_lock);
 	btv->init.fmt        = fmt;
 	btv->init.width      = f->fmt.pix.width;
 	btv->init.height     = f->fmt.pix.height;
+	mutex_unlock(&btv->init.cap.vb_lock);
 	mutex_unlock(&fh->cap.vb_lock);
 
 	return 0;
@@ -2855,6 +2852,7 @@ static int bttv_s_fbuf(struct file *file, void *f,
 
 	retval = 0;
 	fh->ovfmt = fmt;
+	mutex_lock(&btv->init.cap.vb_lock);
 	btv->init.ovfmt = fmt;
 	if (fb->flags & V4L2_FBUF_FLAG_OVERLAY) {
 		fh->ov.w.left   = 0;
@@ -2876,6 +2874,7 @@ static int bttv_s_fbuf(struct file *file, void *f,
 			retval = bttv_switch_overlay(btv, fh, new);
 		}
 	}
+	mutex_unlock(&btv->init.cap.vb_lock);
 	mutex_unlock(&fh->cap.vb_lock);
 	return retval;
 }
@@ -3141,6 +3140,7 @@ static int bttv_s_crop(struct file *file, void *f, struct v4l2_crop *crop)
 	fh->do_crop = 1;
 
 	mutex_lock(&fh->cap.vb_lock);
+	mutex_lock(&btv->init.cap.vb_lock);
 
 	if (fh->width < c.min_scaled_width) {
 		fh->width = c.min_scaled_width;
@@ -3158,6 +3158,7 @@ static int bttv_s_crop(struct file *file, void *f, struct v4l2_crop *crop)
 		btv->init.height = c.max_scaled_height;
 	}
 
+	mutex_unlock(&btv->init.cap.vb_lock);
 	mutex_unlock(&fh->cap.vb_lock);
 
 	return 0;
@@ -3302,9 +3303,9 @@ static int bttv_open(struct file *file)
 	 * Let's first copy btv->init at fh, holding cap.vb_lock, and then work
 	 * with the rest of init, holding btv->lock.
 	 */
-	mutex_lock(&fh->cap.vb_lock);
+	mutex_lock(&btv->init.cap.vb_lock);
 	*fh = btv->init;
-	mutex_unlock(&fh->cap.vb_lock);
+	mutex_unlock(&btv->init.cap.vb_lock);
 
 	fh->type = type;
 	fh->ov.setup_ok = 0;
@@ -3502,9 +3503,9 @@ static int radio_open(struct file *file)
 	if (unlikely(!fh))
 		return -ENOMEM;
 	file->private_data = fh;
-	mutex_lock(&fh->cap.vb_lock);
+	mutex_lock(&btv->init.cap.vb_lock);
 	*fh = btv->init;
-	mutex_unlock(&fh->cap.vb_lock);
+	mutex_unlock(&btv->init.cap.vb_lock);
 
 	mutex_lock(&btv->lock);
 	v4l2_prio_open(&btv->prio, &fh->prio);
@@ -4489,6 +4490,7 @@ static int __devinit bttv_probe(struct pci_dev *dev,
 	btv->opt_coring     = coring;
 
 	/* fill struct bttv with some useful defaults */
+	mutex_init(&btv->init.cap.vb_lock);
 	btv->init.btv         = btv;
 	btv->init.ov.w.width  = 320;
 	btv->init.ov.w.height = 240;
-- 
1.7.3.1

Re: [PATCH] bttv: fix mutex use before init

[Dave Young]

On Mon, Dec 13, 2010 at 04:30:24PM -0800, Brandon Philips wrote:
> On 17:13 Sun 12 Dec 2010, Torsten Kaiser wrote:
> >  * change &fh->cap.vb_lock in bttv_open() AND radio_open() to
> > &btv->init.cap.vb_lock
> >  * add a mutex_init(&btv->init.cap.vb_lock) to the setup of init in bttv_probe()
> 
> That seems like a reasonable suggestion. An openSUSE user submitted this
> bug to our tracker too. Here is the patch I am having him test.
> 
> Would you mind testing it?
> 
> From 456dc0ce36db523c4c0c8a269f4eec43a72de1dc Mon Sep 17 00:00:00 2001
> From: Brandon Philips <bphilips@suse.de>
> Date: Mon, 13 Dec 2010 16:21:55 -0800
> Subject: [PATCH] bttv: fix locking for btv->init
> 
> Fix locking for the btv->init by using btv->init.cap.vb_lock and in the
> process fix uninitialized deref introduced in c37db91fd0d.

Tested this patch, seems fine. But there's still possible deadlock, kernel hangs with lockdep warning, I think maybe it is another issue.

Could some v4l2 guys who have deep understand about bttv driver have a look at this issue?

[  322.172772] bttv: driver version 0.9.18 loaded
[  322.172781] bttv: using 8 buffers with 2080k (520 pages) each for capture
[  322.173300] bttv: Bt8xx card found (0).
[  322.174510] bttv 0000:03:01.0: PCI INT A -> GSI 17 (level, low) -> IRQ 17
[  322.174560] bttv0: Bt878 (rev 17) at 0000:03:01.0, irq: 17, latency: 64, mmio: 0xd0001000
[  322.174679] bttv0: detected: Leadtek WinFast TV 2000 [card=34], PCI subsystem ID is 107d:6606
[  322.174687] bttv0: using: Leadtek WinFast 2000/ WinFast 2000 XP [card=34,autodetected]
[  322.175062] bttv0: gpio: en=00000000, out=00000000 in=003fbfff [init]
[  322.178792] bttv0: tuner type=5
[  322.254454] bttv0: audio absent, no audio device found!
[  322.289946] tuner 1-0061: chip found @ 0xc2 (bt878 #0 [sw])
[  322.290944] tuner-simple 1-0061: creating new instance
[  322.290953] tuner-simple 1-0061: type set to 5 (Philips PAL_BG (FI1216 and compatibles))
[  322.295830] bttv0: registered device video0
[  322.300020] bttv0: registered device vbi0
[  322.303216] bttv0: registered device radio0
[  322.303636] bttv0: PLL: 28636363 => 35468950 .. ok
[  322.334635] Registered IR keymap rc-winfast
[  322.341201] input: bttv IR (card=34) as /devices/pci0000:00/0000:00:1e.0/0000:03:01.0/rc/rc0/input4
[  322.357025] rc0: bttv IR (card=34) as /devices/pci0000:00/0000:00:1e.0/0000:03:01.0/rc/rc0
[  382.342433] bttv0: PLL can sleep, using XTAL (28636363).
[  382.816353] irq event stamp: 1078776
[  382.816363] hardirqs last  enabled at (1078775): [<c105604d>] debug_check_no_locks_freed+0x102/0x113
[  382.816381] 
[  382.816384] =======================================================
[  382.816389] [ INFO: possible circular locking dependency detected ]
[  382.816394] 2.6.37-rc5-00062-g6313e3c-dirty #114
[  382.816398] -------------------------------------------------------
[  382.816403] kdetv/2109 is trying to acquire lock:
[  382.816408]  (&mm->mmap_sem){++++++}, at: [<c10b1bf5>] might_fault+0x47/0x81
[  382.816422] 
[  382.816424] but task is already holding lock:
[  382.816427]  (&q->vb_lock){+.+.+.}, at: [<e00af2be>] videobuf_queue_lock+0x10/0x12 [videobuf_core]
[  382.816442] 
[  382.816444] which lock already depends on the new lock.
[  382.816446] 
[  382.816449] 
[  382.816450] the existing dependency chain (in reverse order) is:
[  382.816455] 
[  382.816456] -> #1 (&q->vb_lock){+.+.+.}:
[  382.816463]        [<c10574dc>] lock_acquire+0xa1/0xc4
[  382.816472]        [<c14ef90d>] __mutex_lock_common+0x35/0x2dc
[  382.816482]        [<c14efc52>] mutex_lock_nested+0x30/0x38
[  382.816489]        [<e00af2be>] videobuf_queue_lock+0x10/0x12 [videobuf_core]
[  382.816499]        [<e00af339>] videobuf_mmap_mapper+0x69/0xc0 [videobuf_core]
[  382.816509]        [<e18aae49>] bttv_mmap+0x66/0x6d [bttv]
[  382.816522]        [<c140009a>] v4l2_mmap+0x5a/0x73
[  382.816532]        [<c10b7f0c>] mmap_region+0x24c/0x400
[  382.816540]        [<c10b82ef>] do_mmap_pgoff+0x22f/0x27f
[  382.816547]        [<c10b841c>] sys_mmap_pgoff+0xdd/0x119
[  382.816560]        [<c14f0d55>] syscall_call+0x7/0xb
[  382.816569] 
[  382.816571] -> #0 (&mm->mmap_sem){++++++}:
[  382.816577]        [<c105718c>] __lock_acquire+0x9d7/0xc86
[  382.816585]        [<c10574dc>] lock_acquire+0xa1/0xc4
[  382.816592]        [<c10b1c12>] might_fault+0x64/0x81
[  382.816599]        [<c1297552>] copy_to_user+0x2c/0xfe
[  382.816608]        [<e00b0851>] videobuf_read_stream+0x17f/0x24c [videobuf_core]
[  382.816619]        [<e18ab05b>] bttv_read+0xcf/0xe9 [bttv]
[  382.816633]        [<c14002bb>] v4l2_read+0x63/0x7f
[  382.816640]        [<c10ce67e>] vfs_read+0x81/0xdb
[  382.816649]        [<c10ce771>] sys_read+0x3b/0x60
[  382.816655]        [<c14f0d55>] syscall_call+0x7/0xb
[  382.816663] 
[  382.816665] other info that might help us debug this:
[  382.816667] 
[  382.816671] 1 lock held by kdetv/2109:
[  382.816675]  #0:  (&q->vb_lock){+.+.+.}, at: [<e00af2be>] videobuf_queue_lock+0x10/0x12 [videobuf_core]
[  382.816688] 
[  382.816690] stack backtrace:
[  382.816695] Pid: 2109, comm: kdetv Not tainted 2.6.37-rc5-00062-g6313e3c-dirty #114
[  382.816700] Call Trace:
[  382.816639] hardirqs last disabled at (1078776): [<c1002ea7>] common_interrupt+0x27/0x34
[  382.816639] softirqs last  enabled at (1078294): [<c1034a8b>] __do_softirq+0x17b/0x183
[  382.816639] softirqs last disabled at (1078271): [<c1003ffb>] do_softirq+0x67/0xc0
[  382.816738]  [<c14ee80b>] ? printk+0x20/0x24
[  382.816748]  [<c1056480>] print_circular_bug+0x9c/0xa8
[  382.816758]  [<c105718c>] __lock_acquire+0x9d7/0xc86
[  382.816769]  [<c103a908>] ? __mod_timer+0x10c/0x117
[  382.816779]  [<c10574dc>] lock_acquire+0xa1/0xc4
[  382.816788]  [<c10b1bf5>] ? might_fault+0x47/0x81
[  382.816798]  [<c10b1c12>] might_fault+0x64/0x81
[  382.816807]  [<c10b1bf5>] ? might_fault+0x47/0x81
[  382.816815]  [<c1297552>] copy_to_user+0x2c/0xfe
[  382.816828]  [<e00b0851>] videobuf_read_stream+0x17f/0x24c [videobuf_core]
[  382.816844]  [<e18ab05b>] bttv_read+0xcf/0xe9 [bttv]
[  382.816854]  [<c14002bb>] v4l2_read+0x63/0x7f
[  382.816865]  [<c1400258>] ? v4l2_read+0x0/0x7f
[  382.816874]  [<c10ce67e>] vfs_read+0x81/0xdb
[  382.816883]  [<c10ce771>] sys_read+0x3b/0x60
[  382.816893]  [<c14f0d55>] syscall_call+0x7/0xb
[  382.816902]  [<c14f0000>] ? rt_mutex_trylock+0x2a/0x2d

> 
> Signed-off-by: Brandon Philips <bphilips@suse.de>
> ---
>  drivers/media/video/bt8xx/bttv-driver.c |   24 +++++++++++++-----------
>  1 files changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/media/video/bt8xx/bttv-driver.c b/drivers/media/video/bt8xx/bttv-driver.c
> index a529619..e656424 100644
> --- a/drivers/media/video/bt8xx/bttv-driver.c
> +++ b/drivers/media/video/bt8xx/bttv-driver.c
> @@ -2391,16 +2391,11 @@ static int setup_window_lock(struct bttv_fh *fh, struct bttv *btv,
>  	fh->ov.field    = win->field;
>  	fh->ov.setup_ok = 1;
>  
> -	/*
> -	 * FIXME: btv is protected by btv->lock mutex, while btv->init
> -	 *	  is protected by fh->cap.vb_lock. This seems to open the
> -	 *	  possibility for some race situations. Maybe the better would
> -	 *	  be to unify those locks or to use another way to store the
> -	 *	  init values that will be consumed by videobuf callbacks
> -	 */
> +	mutex_lock(&btv->init.cap.vb_lock);
>  	btv->init.ov.w.width   = win->w.width;
>  	btv->init.ov.w.height  = win->w.height;
>  	btv->init.ov.field     = win->field;
> +	mutex_unlock(&btv->init.cap.vb_lock);
>  
>  	/* update overlay if needed */
>  	retval = 0;
> @@ -2620,9 +2615,11 @@ static int bttv_s_fmt_vid_cap(struct file *file, void *priv,
>  	fh->cap.last         = V4L2_FIELD_NONE;
>  	fh->width            = f->fmt.pix.width;
>  	fh->height           = f->fmt.pix.height;
> +	mutex_lock(&btv->init.cap.vb_lock);
>  	btv->init.fmt        = fmt;
>  	btv->init.width      = f->fmt.pix.width;
>  	btv->init.height     = f->fmt.pix.height;
> +	mutex_unlock(&btv->init.cap.vb_lock);
>  	mutex_unlock(&fh->cap.vb_lock);
>  
>  	return 0;
> @@ -2855,6 +2852,7 @@ static int bttv_s_fbuf(struct file *file, void *f,
>  
>  	retval = 0;
>  	fh->ovfmt = fmt;
> +	mutex_lock(&btv->init.cap.vb_lock);
>  	btv->init.ovfmt = fmt;
>  	if (fb->flags & V4L2_FBUF_FLAG_OVERLAY) {
>  		fh->ov.w.left   = 0;
> @@ -2876,6 +2874,7 @@ static int bttv_s_fbuf(struct file *file, void *f,
>  			retval = bttv_switch_overlay(btv, fh, new);
>  		}
>  	}
> +	mutex_unlock(&btv->init.cap.vb_lock);
>  	mutex_unlock(&fh->cap.vb_lock);
>  	return retval;
>  }
> @@ -3141,6 +3140,7 @@ static int bttv_s_crop(struct file *file, void *f, struct v4l2_crop *crop)
>  	fh->do_crop = 1;
>  
>  	mutex_lock(&fh->cap.vb_lock);
> +	mutex_lock(&btv->init.cap.vb_lock);
>  
>  	if (fh->width < c.min_scaled_width) {
>  		fh->width = c.min_scaled_width;
> @@ -3158,6 +3158,7 @@ static int bttv_s_crop(struct file *file, void *f, struct v4l2_crop *crop)
>  		btv->init.height = c.max_scaled_height;
>  	}
>  
> +	mutex_unlock(&btv->init.cap.vb_lock);
>  	mutex_unlock(&fh->cap.vb_lock);
>  
>  	return 0;
> @@ -3302,9 +3303,9 @@ static int bttv_open(struct file *file)
>  	 * Let's first copy btv->init at fh, holding cap.vb_lock, and then work
>  	 * with the rest of init, holding btv->lock.
>  	 */
> -	mutex_lock(&fh->cap.vb_lock);
> +	mutex_lock(&btv->init.cap.vb_lock);
>  	*fh = btv->init;
> -	mutex_unlock(&fh->cap.vb_lock);
> +	mutex_unlock(&btv->init.cap.vb_lock);
>  
>  	fh->type = type;
>  	fh->ov.setup_ok = 0;
> @@ -3502,9 +3503,9 @@ static int radio_open(struct file *file)
>  	if (unlikely(!fh))
>  		return -ENOMEM;
>  	file->private_data = fh;
> -	mutex_lock(&fh->cap.vb_lock);
> +	mutex_lock(&btv->init.cap.vb_lock);
>  	*fh = btv->init;
> -	mutex_unlock(&fh->cap.vb_lock);
> +	mutex_unlock(&btv->init.cap.vb_lock);
>  
>  	mutex_lock(&btv->lock);
>  	v4l2_prio_open(&btv->prio, &fh->prio);
> @@ -4489,6 +4490,7 @@ static int __devinit bttv_probe(struct pci_dev *dev,
>  	btv->opt_coring     = coring;
>  
>  	/* fill struct bttv with some useful defaults */
> +	mutex_init(&btv->init.cap.vb_lock);
>  	btv->init.btv         = btv;
>  	btv->init.ov.w.width  = 320;
>  	btv->init.ov.w.height = 240;
> -- 
> 1.7.3.1
> 

Re: [PATCH] bttv: fix mutex use before init

[Torsten Kaiser]

On Tue, Dec 14, 2010 at 1:30 AM, Brandon Philips <brandon@ifup.org> wrote:
> On 17:13 Sun 12 Dec 2010, Torsten Kaiser wrote:
>>  * change &fh->cap.vb_lock in bttv_open() AND radio_open() to
>> &btv->init.cap.vb_lock
>>  * add a mutex_init(&btv->init.cap.vb_lock) to the setup of init in bttv_probe()
>
> That seems like a reasonable suggestion. An openSUSE user submitted this
> bug to our tracker too. Here is the patch I am having him test.
>
> Would you mind testing it?

No. :-)

Without this patch (==vanilla 2.6.37-rc5) I got 2 more OOPSe by
restarting hal around 20 times.
After applying this patch, I did not see a single OOPS after 100 restarts.
So it looks like the fix is correct.

Using the card also still works, but I think I found out what was
causing sporadic shutdown problems with 37-rc kernels: When I try to
exit tvtime it gets stuck in an uninterruptible D state and can't be
killed. And that seems to mess up the shutdown.

But this happens independent with or without your patch and looks like
different problem.

SysRQ+W provided this stack trace, maybe someone seens an obvious bug...:
[  274.772528]  ffff8800dec69680 0000000000000086 ffffffff81089d73
ffff8800729d05a0
[  274.778599]  0000000000011480 ffff8800df923fd8 0000000000011480
ffff8800df922000
[  274.778599]  ffff8800df923fd8 0000000000011480 ffff8800dec69680
0000000000011480
[  274.778599] Call Trace:
[  274.778599]  [<ffffffff81089d73>] ? free_pcppages_bulk+0x343/0x3b0
[  274.778599]  [<ffffffff8156d0e1>] ? __mutex_lock_slowpath+0xe1/0x160
[  274.778599]  [<ffffffff8156cd8a>] ? mutex_lock+0x1a/0x40
[  274.778599]  [<ffffffff8141ab7f>] ? free_btres_lock.clone.19+0x3f/0x100
[  274.778599]  [<ffffffff8141d311>] ? bttv_release+0x1c1/0x1e0
[  274.778599]  [<ffffffff813fe4ba>] ? v4l2_release+0x4a/0x70
[  274.778599]  [<ffffffff810c5291>] ? fput+0xe1/0x250
[  274.778599]  [<ffffffff810c1d59>] ? filp_close+0x59/0x80
[  274.778599]  [<ffffffff810c1e0b>] ? sys_close+0x8b/0xe0
[  274.778599]  [<ffffffff8100253b>] ? system_call_fastpath+0x16/0x1b


> From 456dc0ce36db523c4c0c8a269f4eec43a72de1dc Mon Sep 17 00:00:00 2001
> From: Brandon Philips <bphilips@suse.de>
> Date: Mon, 13 Dec 2010 16:21:55 -0800
> Subject: [PATCH] bttv: fix locking for btv->init
>
> Fix locking for the btv->init by using btv->init.cap.vb_lock and in the
> process fix uninitialized deref introduced in c37db91fd0d.
>
> Signed-off-by: Brandon Philips <bphilips@suse.de>
> ---
>  drivers/media/video/bt8xx/bttv-driver.c |   24 +++++++++++++-----------
>  1 files changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/media/video/bt8xx/bttv-driver.c b/drivers/media/video/bt8xx/bttv-driver.c
> index a529619..e656424 100644
> --- a/drivers/media/video/bt8xx/bttv-driver.c
> +++ b/drivers/media/video/bt8xx/bttv-driver.c
> @@ -2391,16 +2391,11 @@ static int setup_window_lock(struct bttv_fh *fh, struct bttv *btv,
>        fh->ov.field    = win->field;
>        fh->ov.setup_ok = 1;
>
> -       /*
> -        * FIXME: btv is protected by btv->lock mutex, while btv->init
> -        *        is protected by fh->cap.vb_lock. This seems to open the
> -        *        possibility for some race situations. Maybe the better would
> -        *        be to unify those locks or to use another way to store the
> -        *        init values that will be consumed by videobuf callbacks
> -        */
> +       mutex_lock(&btv->init.cap.vb_lock);
>        btv->init.ov.w.width   = win->w.width;
>        btv->init.ov.w.height  = win->w.height;
>        btv->init.ov.field     = win->field;
> +       mutex_unlock(&btv->init.cap.vb_lock);
>
>        /* update overlay if needed */
>        retval = 0;
> @@ -2620,9 +2615,11 @@ static int bttv_s_fmt_vid_cap(struct file *file, void *priv,
>        fh->cap.last         = V4L2_FIELD_NONE;
>        fh->width            = f->fmt.pix.width;
>        fh->height           = f->fmt.pix.height;
> +       mutex_lock(&btv->init.cap.vb_lock);
>        btv->init.fmt        = fmt;
>        btv->init.width      = f->fmt.pix.width;
>        btv->init.height     = f->fmt.pix.height;
> +       mutex_unlock(&btv->init.cap.vb_lock);
>        mutex_unlock(&fh->cap.vb_lock);
>
>        return 0;
> @@ -2855,6 +2852,7 @@ static int bttv_s_fbuf(struct file *file, void *f,
>
>        retval = 0;
>        fh->ovfmt = fmt;
> +       mutex_lock(&btv->init.cap.vb_lock);
>        btv->init.ovfmt = fmt;
>        if (fb->flags & V4L2_FBUF_FLAG_OVERLAY) {
>                fh->ov.w.left   = 0;
> @@ -2876,6 +2874,7 @@ static int bttv_s_fbuf(struct file *file, void *f,
>                        retval = bttv_switch_overlay(btv, fh, new);
>                }
>        }
> +       mutex_unlock(&btv->init.cap.vb_lock);
>        mutex_unlock(&fh->cap.vb_lock);
>        return retval;
>  }
> @@ -3141,6 +3140,7 @@ static int bttv_s_crop(struct file *file, void *f, struct v4l2_crop *crop)
>        fh->do_crop = 1;
>
>        mutex_lock(&fh->cap.vb_lock);
> +       mutex_lock(&btv->init.cap.vb_lock);
>
>        if (fh->width < c.min_scaled_width) {
>                fh->width = c.min_scaled_width;
> @@ -3158,6 +3158,7 @@ static int bttv_s_crop(struct file *file, void *f, struct v4l2_crop *crop)
>                btv->init.height = c.max_scaled_height;
>        }
>
> +       mutex_unlock(&btv->init.cap.vb_lock);
>        mutex_unlock(&fh->cap.vb_lock);
>
>        return 0;
> @@ -3302,9 +3303,9 @@ static int bttv_open(struct file *file)
>         * Let's first copy btv->init at fh, holding cap.vb_lock, and then work
>         * with the rest of init, holding btv->lock.
>         */
> -       mutex_lock(&fh->cap.vb_lock);
> +       mutex_lock(&btv->init.cap.vb_lock);
>        *fh = btv->init;
> -       mutex_unlock(&fh->cap.vb_lock);
> +       mutex_unlock(&btv->init.cap.vb_lock);
>
>        fh->type = type;
>        fh->ov.setup_ok = 0;
> @@ -3502,9 +3503,9 @@ static int radio_open(struct file *file)
>        if (unlikely(!fh))
>                return -ENOMEM;
>        file->private_data = fh;
> -       mutex_lock(&fh->cap.vb_lock);
> +       mutex_lock(&btv->init.cap.vb_lock);
>        *fh = btv->init;
> -       mutex_unlock(&fh->cap.vb_lock);
> +       mutex_unlock(&btv->init.cap.vb_lock);
>
>        mutex_lock(&btv->lock);
>        v4l2_prio_open(&btv->prio, &fh->prio);
> @@ -4489,6 +4490,7 @@ static int __devinit bttv_probe(struct pci_dev *dev,
>        btv->opt_coring     = coring;
>
>        /* fill struct bttv with some useful defaults */
> +       mutex_init(&btv->init.cap.vb_lock);
>        btv->init.btv         = btv;
>        btv->init.ov.w.width  = 320;
>        btv->init.ov.w.height = 240;
> --
> 1.7.3.1
>
>

Re: [PATCH] bttv: fix mutex use before init

[Torsten Kaiser]

On Tue, Dec 14, 2010 at 9:56 PM, Torsten Kaiser
<just.for.lkml@googlemail.com> wrote:
> Using the card also still works, but I think I found out what was
> causing sporadic shutdown problems with 37-rc kernels: When I try to
> exit tvtime it gets stuck in an uninterruptible D state and can't be
> killed. And that seems to mess up the shutdown.
>
> But this happens independent with or without your patch and looks like
> different problem.
>
> SysRQ+W provided this stack trace, maybe someone seens an obvious bug...:
> [  274.772528]  ffff8800dec69680 0000000000000086 ffffffff81089d73
> ffff8800729d05a0
> [  274.778599]  0000000000011480 ffff8800df923fd8 0000000000011480
> ffff8800df922000
> [  274.778599]  ffff8800df923fd8 0000000000011480 ffff8800dec69680
> 0000000000011480
> [  274.778599] Call Trace:
> [  274.778599]  [<ffffffff81089d73>] ? free_pcppages_bulk+0x343/0x3b0
> [  274.778599]  [<ffffffff8156d0e1>] ? __mutex_lock_slowpath+0xe1/0x160
> [  274.778599]  [<ffffffff8156cd8a>] ? mutex_lock+0x1a/0x40
> [  274.778599]  [<ffffffff8141ab7f>] ? free_btres_lock.clone.19+0x3f/0x100
> [  274.778599]  [<ffffffff8141d311>] ? bttv_release+0x1c1/0x1e0
> [  274.778599]  [<ffffffff813fe4ba>] ? v4l2_release+0x4a/0x70
> [  274.778599]  [<ffffffff810c5291>] ? fput+0xe1/0x250
> [  274.778599]  [<ffffffff810c1d59>] ? filp_close+0x59/0x80
> [  274.778599]  [<ffffffff810c1e0b>] ? sys_close+0x8b/0xe0
> [  274.778599]  [<ffffffff8100253b>] ? system_call_fastpath+0x16/0x1b

Hmm:bttv_release() does mutex_lock(&btv->lock), then calls into
free_btres_lock(...) that also first does mutex_lock(&btv->lock);

The calls to lock btv->lock in bttv_release() where added as part of
the BKL removal, but I do not understand enough to fix this.
Can this be dropped from bttv_release() completely, or would an
unlocked version of free_btres_lock() be better?

Torsten

Re: [PATCH] bttv: fix mutex use before init

[Brandon Philips]

On 22:13 Tue 14 Dec 2010, Torsten Kaiser wrote:
> On Tue, Dec 14, 2010 at 9:56 PM, Torsten Kaiser
> <just.for.lkml@googlemail.com> wrote:
> > Using the card also still works, but I think I found out what was
> > causing sporadic shutdown problems with 37-rc kernels: When I try to
> > exit tvtime it gets stuck in an uninterruptible D state and can't be
> > killed. And that seems to mess up the shutdown.
> >
> > But this happens independent with or without your patch and looks like
> > different problem.
> >
> > SysRQ+W provided this stack trace, maybe someone seens an obvious bug...:
> > [  274.772528]  ffff8800dec69680 0000000000000086 ffffffff81089d73
> > ffff8800729d05a0
> > [  274.778599]  0000000000011480 ffff8800df923fd8 0000000000011480
> > ffff8800df922000
> > [  274.778599]  ffff8800df923fd8 0000000000011480 ffff8800dec69680
> > 0000000000011480
> > [  274.778599] Call Trace:
> > [  274.778599]  [<ffffffff81089d73>] ? free_pcppages_bulk+0x343/0x3b0
> > [  274.778599]  [<ffffffff8156d0e1>] ? __mutex_lock_slowpath+0xe1/0x160
> > [  274.778599]  [<ffffffff8156cd8a>] ? mutex_lock+0x1a/0x40
> > [  274.778599]  [<ffffffff8141ab7f>] ? free_btres_lock.clone.19+0x3f/0x100
> > [  274.778599]  [<ffffffff8141d311>] ? bttv_release+0x1c1/0x1e0
> > [  274.778599]  [<ffffffff813fe4ba>] ? v4l2_release+0x4a/0x70
> > [  274.778599]  [<ffffffff810c5291>] ? fput+0xe1/0x250
> > [  274.778599]  [<ffffffff810c1d59>] ? filp_close+0x59/0x80
> > [  274.778599]  [<ffffffff810c1e0b>] ? sys_close+0x8b/0xe0
> > [  274.778599]  [<ffffffff8100253b>] ? system_call_fastpath+0x16/0x1b
> 
> The calls to lock btv->lock in bttv_release() where added as part of
> the BKL removal, but I do not understand enough to fix this.
> Can this be dropped from bttv_release() completely, or would an
> unlocked version of free_btres_lock() be better?

I would create an unlocked version of free_btres_lock. Does that fix the
issue?

Cheers,

	Brandon

Re: [PATCH] bttv: fix mutex use before init

[Brandon Philips]

On 21:56 Tue 14 Dec 2010, Torsten Kaiser wrote:
> On Tue, Dec 14, 2010 at 1:30 AM, Brandon Philips <brandon@ifup.org> wrote:
> > On 17:13 Sun 12 Dec 2010, Torsten Kaiser wrote:
> >>  * change &fh->cap.vb_lock in bttv_open() AND radio_open() to
> >> &btv->init.cap.vb_lock
> >>  * add a mutex_init(&btv->init.cap.vb_lock) to the setup of init in bttv_probe()
> >
> > That seems like a reasonable suggestion. An openSUSE user submitted this
> > bug to our tracker too. Here is the patch I am having him test.
> >
> > Would you mind testing it?
> 
> No. :-)
> 
> Without this patch (==vanilla 2.6.37-rc5) I got 2 more OOPSe by
> restarting hal around 20 times.
> After applying this patch, I did not see a single OOPS after 100 restarts.
> So it looks like the fix is correct.

Dave, Torsten- Great thanks for testing, can I get both you and Dave's
Tested-by then?

Mauro- can you please pick up this patch?

Cheers,

	Brandon

Re: [PATCH] bttv: fix mutex use before init

[Dave Young]

On Wed, Dec 15, 2010 at 5:43 AM, Brandon Philips <brandon@ifup.org> wrote:
> On 21:56 Tue 14 Dec 2010, Torsten Kaiser wrote:
>> On Tue, Dec 14, 2010 at 1:30 AM, Brandon Philips <brandon@ifup.org> wrote:
>> > On 17:13 Sun 12 Dec 2010, Torsten Kaiser wrote:
>> >>  * change &fh->cap.vb_lock in bttv_open() AND radio_open() to
>> >> &btv->init.cap.vb_lock
>> >>  * add a mutex_init(&btv->init.cap.vb_lock) to the setup of init in bttv_probe()
>> >
>> > That seems like a reasonable suggestion. An openSUSE user submitted this
>> > bug to our tracker too. Here is the patch I am having him test.
>> >
>> > Would you mind testing it?
>>
>> No. :-)
>>
>> Without this patch (==vanilla 2.6.37-rc5) I got 2 more OOPSe by
>> restarting hal around 20 times.
>> After applying this patch, I did not see a single OOPS after 100 restarts.
>> So it looks like the fix is correct.
>
> Dave, Torsten- Great thanks for testing, can I get both you and Dave's
> Tested-by then?

Feel free to add my tested-by line for this, Thanks

>
> Mauro- can you please pick up this patch?
>
> Cheers,
>
>        Brandon
>



-- 
Regards
dave

Re: [PATCH] bttv: fix mutex use before init

[Torsten Kaiser]

On Tue, Dec 14, 2010 at 10:43 PM, Brandon Philips <brandon@ifup.org> wrote:
> On 21:56 Tue 14 Dec 2010, Torsten Kaiser wrote:
>> On Tue, Dec 14, 2010 at 1:30 AM, Brandon Philips <brandon@ifup.org> wrote:
>> > On 17:13 Sun 12 Dec 2010, Torsten Kaiser wrote:
>> >>  * change &fh->cap.vb_lock in bttv_open() AND radio_open() to
>> >> &btv->init.cap.vb_lock
>> >>  * add a mutex_init(&btv->init.cap.vb_lock) to the setup of init in bttv_probe()
>> >
>> > That seems like a reasonable suggestion. An openSUSE user submitted this
>> > bug to our tracker too. Here is the patch I am having him test.
>> >
>> > Would you mind testing it?
>>
>> No. :-)
>>
>> Without this patch (==vanilla 2.6.37-rc5) I got 2 more OOPSe by
>> restarting hal around 20 times.
>> After applying this patch, I did not see a single OOPS after 100 restarts.
>> So it looks like the fix is correct.
>
> Dave, Torsten- Great thanks for testing, can I get both you and Dave's
> Tested-by then?

Tested-by: Torsten Kaiser <just.for.lkml@googlemail.com>

Thanks for providing this patch.

Torsten

Re: [PATCH] bttv: fix mutex use before init

[Chris Clayton]

On Tuesday 14 December 2010, Brandon Philips wrote:
> On 17:13 Sun 12 Dec 2010, Torsten Kaiser wrote:
> >  * change &fh->cap.vb_lock in bttv_open() AND radio_open() to
> > &btv->init.cap.vb_lock
> >  * add a mutex_init(&btv->init.cap.vb_lock) to the setup of init in
> > bttv_probe()
>
> That seems like a reasonable suggestion. An openSUSE user submitted this
> bug to our tracker too. Here is the patch I am having him test.
>
> Would you mind testing it?
>
> From 456dc0ce36db523c4c0c8a269f4eec43a72de1dc Mon Sep 17 00:00:00 2001
> From: Brandon Philips <bphilips@suse.de>
> Date: Mon, 13 Dec 2010 16:21:55 -0800
> Subject: [PATCH] bttv: fix locking for btv->init
>
> Fix locking for the btv->init by using btv->init.cap.vb_lock and in the
> process fix uninitialized deref introduced in c37db91fd0d.
>
> Signed-off-by: Brandon Philips <bphilips@suse.de>
> ---
>  drivers/media/video/bt8xx/bttv-driver.c |   24 +++++++++++++-----------
>  1 files changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/media/video/bt8xx/bttv-driver.c
> b/drivers/media/video/bt8xx/bttv-driver.c index a529619..e656424 100644
> --- a/drivers/media/video/bt8xx/bttv-driver.c
> +++ b/drivers/media/video/bt8xx/bttv-driver.c
> @@ -2391,16 +2391,11 @@ static int setup_window_lock(struct bttv_fh *fh,
> struct bttv *btv, fh->ov.field    = win->field;
>  	fh->ov.setup_ok = 1;
>
> -	/*
> -	 * FIXME: btv is protected by btv->lock mutex, while btv->init
> -	 *	  is protected by fh->cap.vb_lock. This seems to open the
> -	 *	  possibility for some race situations. Maybe the better would
> -	 *	  be to unify those locks or to use another way to store the
> -	 *	  init values that will be consumed by videobuf callbacks
> -	 */
> +	mutex_lock(&btv->init.cap.vb_lock);
>  	btv->init.ov.w.width   = win->w.width;
>  	btv->init.ov.w.height  = win->w.height;
>  	btv->init.ov.field     = win->field;
> +	mutex_unlock(&btv->init.cap.vb_lock);
>
>  	/* update overlay if needed */
>  	retval = 0;
> @@ -2620,9 +2615,11 @@ static int bttv_s_fmt_vid_cap(struct file *file,
> void *priv, fh->cap.last         = V4L2_FIELD_NONE;
>  	fh->width            = f->fmt.pix.width;
>  	fh->height           = f->fmt.pix.height;
> +	mutex_lock(&btv->init.cap.vb_lock);
>  	btv->init.fmt        = fmt;
>  	btv->init.width      = f->fmt.pix.width;
>  	btv->init.height     = f->fmt.pix.height;
> +	mutex_unlock(&btv->init.cap.vb_lock);
>  	mutex_unlock(&fh->cap.vb_lock);
>
>  	return 0;
> @@ -2855,6 +2852,7 @@ static int bttv_s_fbuf(struct file *file, void *f,
>
>  	retval = 0;
>  	fh->ovfmt = fmt;
> +	mutex_lock(&btv->init.cap.vb_lock);
>  	btv->init.ovfmt = fmt;
>  	if (fb->flags & V4L2_FBUF_FLAG_OVERLAY) {
>  		fh->ov.w.left   = 0;
> @@ -2876,6 +2874,7 @@ static int bttv_s_fbuf(struct file *file, void *f,
>  			retval = bttv_switch_overlay(btv, fh, new);
>  		}
>  	}
> +	mutex_unlock(&btv->init.cap.vb_lock);
>  	mutex_unlock(&fh->cap.vb_lock);
>  	return retval;
>  }
> @@ -3141,6 +3140,7 @@ static int bttv_s_crop(struct file *file, void *f,
> struct v4l2_crop *crop) fh->do_crop = 1;
>
>  	mutex_lock(&fh->cap.vb_lock);
> +	mutex_lock(&btv->init.cap.vb_lock);
>
>  	if (fh->width < c.min_scaled_width) {
>  		fh->width = c.min_scaled_width;
> @@ -3158,6 +3158,7 @@ static int bttv_s_crop(struct file *file, void *f,
> struct v4l2_crop *crop) btv->init.height = c.max_scaled_height;
>  	}
>
> +	mutex_unlock(&btv->init.cap.vb_lock);
>  	mutex_unlock(&fh->cap.vb_lock);
>
>  	return 0;
> @@ -3302,9 +3303,9 @@ static int bttv_open(struct file *file)
>  	 * Let's first copy btv->init at fh, holding cap.vb_lock, and then work
>  	 * with the rest of init, holding btv->lock.
>  	 */
> -	mutex_lock(&fh->cap.vb_lock);
> +	mutex_lock(&btv->init.cap.vb_lock);
>  	*fh = btv->init;
> -	mutex_unlock(&fh->cap.vb_lock);
> +	mutex_unlock(&btv->init.cap.vb_lock);
>
>  	fh->type = type;
>  	fh->ov.setup_ok = 0;
> @@ -3502,9 +3503,9 @@ static int radio_open(struct file *file)
>  	if (unlikely(!fh))
>  		return -ENOMEM;
>  	file->private_data = fh;
> -	mutex_lock(&fh->cap.vb_lock);
> +	mutex_lock(&btv->init.cap.vb_lock);
>  	*fh = btv->init;
> -	mutex_unlock(&fh->cap.vb_lock);
> +	mutex_unlock(&btv->init.cap.vb_lock);
>
>  	mutex_lock(&btv->lock);
>  	v4l2_prio_open(&btv->prio, &fh->prio);
> @@ -4489,6 +4490,7 @@ static int __devinit bttv_probe(struct pci_dev *dev,
>  	btv->opt_coring     = coring;
>
>  	/* fill struct bttv with some useful defaults */
> +	mutex_init(&btv->init.cap.vb_lock);
>  	btv->init.btv         = btv;
>  	btv->init.ov.w.width  = 320;
>  	btv->init.ov.w.height = 240;

The patch is good here too. Thanks.

Tested-by: Chris Clayton <chris2553@googlemail.com>



-- 
The more I see, the more I know. The more I know, the less I understand. 
Changing Man - Paul Weller

Re: [PATCH] bttv: fix mutex use before init

[Mauro Carvalho Chehab]

Em 15-12-2010 16:44, Chris Clayton escreveu:
> On Tuesday 14 December 2010, Brandon Philips wrote:
>> On 17:13 Sun 12 Dec 2010, Torsten Kaiser wrote:
>>>  * change &fh->cap.vb_lock in bttv_open() AND radio_open() to
>>> &btv->init.cap.vb_lock
>>>  * add a mutex_init(&btv->init.cap.vb_lock) to the setup of init in
>>> bttv_probe()
>>
>> That seems like a reasonable suggestion. An openSUSE user submitted this
>> bug to our tracker too. Here is the patch I am having him test.
>>
>> Would you mind testing it?
>>
>> From 456dc0ce36db523c4c0c8a269f4eec43a72de1dc Mon Sep 17 00:00:00 2001
>> From: Brandon Philips <bphilips@suse.de>
>> Date: Mon, 13 Dec 2010 16:21:55 -0800
>> Subject: [PATCH] bttv: fix locking for btv->init
>>
>> Fix locking for the btv->init by using btv->init.cap.vb_lock and in the
>> process fix uninitialized deref introduced in c37db91fd0d.
>>
>> Signed-off-by: Brandon Philips <bphilips@suse.de>

Hi Brandon & all,

While your patch fixes the issue, it has some other troubles, like to the presence of
lock code at free_btres_lock(). It is possible to fix, but the better is to just
use the core-assisted locking schema. This way, V4L2 core will serialize access to all
ioctl's/open/close/mmap/read/poll operations, avoiding to have two processes accessing
the hardware at the same time. Also, as there's just one lock, instead of 3, there's no
risk of dead locks.

The net result is a cleaner code, with just one lock.

I tested the patch here with an bttv-based STB board (card=3, tuner=6), and it worked fine for me. 
Could you please test if this fixes the issue? 

PS.: The patch is based against the bkl_removal patches, at my linux-next tree:

http://git.kernel.org/?p=linux/kernel/git/mchehab/linux-next.git;a=commit;h=673eb9ff33e26ee6f4278cdab06749aef1bbef5b

(I just created the bkl_removal branch, so, it may take some time for you to see at the kernel.org
mirrors, but it is basically changeset 673eb9ff. You may also just apply it on the top of the master
branch of my linux-next tree).

---

[media] bttv: Fix locking issues due to BKL removal code

The BKL removal patch added a condition where the code would try to use a non-initialized 
lock. While a patch just addressing the issue is possible, there are some other troubles, 
like to the presence of lock code at free_btres_lock(), called on some places with the lock
already taken. It is possible to fix, but the better is to just use the core-assisted 
locking schema. 

This way, V4L2 core will serialize access to all ioctl's/open/close/mmap/read/poll 
operations, avoiding to have two processes accessing the hardware at the same time. 
Also, as there's just one lock, instead of 3, there's no risk of dead locks.

Tested with bttv STB, Gateway P/N 6000699 (card 3, tuner 6).

Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>

diff --git a/drivers/media/video/bt8xx/bttv-driver.c b/drivers/media/video/bt8xx/bttv-driver.c
index a529619..25e1ca0 100644
--- a/drivers/media/video/bt8xx/bttv-driver.c
+++ b/drivers/media/video/bt8xx/bttv-driver.c
@@ -854,7 +854,6 @@ int check_alloc_btres_lock(struct bttv *btv, struct bttv_fh *fh, int bit)
 		xbits |= RESOURCE_VIDEO_READ | RESOURCE_VIDEO_STREAM;
 
 	/* is it free? */
-	mutex_lock(&btv->lock);
 	if (btv->resources & xbits) {
 		/* no, someone else uses it */
 		goto fail;
@@ -884,11 +883,9 @@ int check_alloc_btres_lock(struct bttv *btv, struct bttv_fh *fh, int bit)
 	/* it's free, grab it */
 	fh->resources  |= bit;
 	btv->resources |= bit;
-	mutex_unlock(&btv->lock);
 	return 1;
 
  fail:
-	mutex_unlock(&btv->lock);
 	return 0;
 }
 
@@ -940,7 +937,6 @@ void free_btres_lock(struct bttv *btv, struct bttv_fh *fh, int bits)
 		/* trying to free ressources not allocated by us ... */
 		printk("bttv: BUG! (btres)\n");
 	}
-	mutex_lock(&btv->lock);
 	fh->resources  &= ~bits;
 	btv->resources &= ~bits;
 
@@ -951,8 +947,6 @@ void free_btres_lock(struct bttv *btv, struct bttv_fh *fh, int bits)
 
 	if (0 == (bits & VBI_RESOURCES))
 		disclaim_vbi_lines(btv);
-
-	mutex_unlock(&btv->lock);
 }
 
 /* ----------------------------------------------------------------------- */
@@ -1713,28 +1707,20 @@ static int bttv_prepare_buffer(struct videobuf_queue *q,struct bttv *btv,
 
 		/* Make sure tvnorm and vbi_end remain consistent
 		   until we're done. */
-		mutex_lock(&btv->lock);
 
 		norm = btv->tvnorm;
 
 		/* In this mode capturing always starts at defrect.top
 		   (default VDELAY), ignoring cropping parameters. */
 		if (btv->vbi_end > bttv_tvnorms[norm].cropcap.defrect.top) {
-			mutex_unlock(&btv->lock);
 			return -EINVAL;
 		}
 
-		mutex_unlock(&btv->lock);
-
 		c.rect = bttv_tvnorms[norm].cropcap.defrect;
 	} else {
-		mutex_lock(&btv->lock);
-
 		norm = btv->tvnorm;
 		c = btv->crop[!!fh->do_crop];
 
-		mutex_unlock(&btv->lock);
-
 		if (width < c.min_scaled_width ||
 		    width > c.max_scaled_width ||
 		    height < c.min_scaled_height)
@@ -1858,7 +1844,6 @@ static int bttv_s_std(struct file *file, void *priv, v4l2_std_id *id)
 	unsigned int i;
 	int err;
 
-	mutex_lock(&btv->lock);
 	err = v4l2_prio_check(&btv->prio, fh->prio);
 	if (err)
 		goto err;
@@ -1874,7 +1859,6 @@ static int bttv_s_std(struct file *file, void *priv, v4l2_std_id *id)
 	set_tvnorm(btv, i);
 
 err:
-	mutex_unlock(&btv->lock);
 
 	return err;
 }
@@ -1898,7 +1882,6 @@ static int bttv_enum_input(struct file *file, void *priv,
 	struct bttv *btv = fh->btv;
 	int rc = 0;
 
-	mutex_lock(&btv->lock);
 	if (i->index >= bttv_tvcards[btv->c.type].video_inputs) {
 		rc = -EINVAL;
 		goto err;
@@ -1928,7 +1911,6 @@ static int bttv_enum_input(struct file *file, void *priv,
 	i->std = BTTV_NORMS;
 
 err:
-	mutex_unlock(&btv->lock);
 
 	return rc;
 }
@@ -1938,9 +1920,7 @@ static int bttv_g_input(struct file *file, void *priv, unsigned int *i)
 	struct bttv_fh *fh = priv;
 	struct bttv *btv = fh->btv;
 
-	mutex_lock(&btv->lock);
 	*i = btv->input;
-	mutex_unlock(&btv->lock);
 
 	return 0;
 }
@@ -1952,7 +1932,6 @@ static int bttv_s_input(struct file *file, void *priv, unsigned int i)
 
 	int err;
 
-	mutex_lock(&btv->lock);
 	err = v4l2_prio_check(&btv->prio, fh->prio);
 	if (unlikely(err))
 		goto err;
@@ -1965,7 +1944,6 @@ static int bttv_s_input(struct file *file, void *priv, unsigned int i)
 	set_input(btv, i, btv->tvnorm);
 
 err:
-	mutex_unlock(&btv->lock);
 	return 0;
 }
 
@@ -1979,7 +1957,6 @@ static int bttv_s_tuner(struct file *file, void *priv,
 	if (unlikely(0 != t->index))
 		return -EINVAL;
 
-	mutex_lock(&btv->lock);
 	if (unlikely(btv->tuner_type == TUNER_ABSENT)) {
 		err = -EINVAL;
 		goto err;
@@ -1995,7 +1972,6 @@ static int bttv_s_tuner(struct file *file, void *priv,
 		btv->audio_mode_gpio(btv, t, 1);
 
 err:
-	mutex_unlock(&btv->lock);
 
 	return 0;
 }
@@ -2006,10 +1982,8 @@ static int bttv_g_frequency(struct file *file, void *priv,
 	struct bttv_fh *fh  = priv;
 	struct bttv *btv = fh->btv;
 
-	mutex_lock(&btv->lock);
 	f->type = btv->radio_user ? V4L2_TUNER_RADIO : V4L2_TUNER_ANALOG_TV;
 	f->frequency = btv->freq;
-	mutex_unlock(&btv->lock);
 
 	return 0;
 }
@@ -2024,7 +1998,6 @@ static int bttv_s_frequency(struct file *file, void *priv,
 	if (unlikely(f->tuner != 0))
 		return -EINVAL;
 
-	mutex_lock(&btv->lock);
 	err = v4l2_prio_check(&btv->prio, fh->prio);
 	if (unlikely(err))
 		goto err;
@@ -2039,7 +2012,6 @@ static int bttv_s_frequency(struct file *file, void *priv,
 	if (btv->has_matchbox && btv->radio_user)
 		tea5757_set_freq(btv, btv->freq);
 err:
-	mutex_unlock(&btv->lock);
 
 	return 0;
 }
@@ -2172,7 +2144,6 @@ limit_scaled_size_lock       (struct bttv_fh *               fh,
 
 	/* Make sure tvnorm, vbi_end and the current cropping parameters
 	   remain consistent until we're done. */
-	mutex_lock(&btv->lock);
 
 	b = &bttv_tvnorms[btv->tvnorm].cropcap.bounds;
 
@@ -2250,7 +2221,6 @@ limit_scaled_size_lock       (struct bttv_fh *               fh,
 	rc = 0; /* success */
 
  fail:
-	mutex_unlock(&btv->lock);
 
 	return rc;
 }
@@ -2282,9 +2252,7 @@ verify_window_lock		(struct bttv_fh *               fh,
 	if (V4L2_FIELD_ANY == field) {
 		__s32 height2;
 
-		mutex_lock(&fh->btv->lock);
 		height2 = fh->btv->crop[!!fh->do_crop].rect.height >> 1;
-		mutex_unlock(&fh->btv->lock);
 		field = (win->w.height > height2)
 			? V4L2_FIELD_INTERLACED
 			: V4L2_FIELD_TOP;
@@ -2360,7 +2328,6 @@ static int setup_window_lock(struct bttv_fh *fh, struct bttv *btv,
 		}
 	}
 
-	mutex_lock(&fh->cap.vb_lock);
 	/* clip against screen */
 	if (NULL != btv->fbuf.base)
 		n = btcx_screen_clips(btv->fbuf.fmt.width, btv->fbuf.fmt.height,
@@ -2412,7 +2379,6 @@ static int setup_window_lock(struct bttv_fh *fh, struct bttv *btv,
 		bttv_overlay_risc(btv, &fh->ov, fh->ovfmt, new);
 		retval = bttv_switch_overlay(btv,fh,new);
 	}
-	mutex_unlock(&fh->cap.vb_lock);
 	return retval;
 }
 
@@ -2526,9 +2492,7 @@ static int bttv_try_fmt_vid_cap(struct file *file, void *priv,
 	if (V4L2_FIELD_ANY == field) {
 		__s32 height2;
 
-		mutex_lock(&btv->lock);
 		height2 = btv->crop[!!fh->do_crop].rect.height >> 1;
-		mutex_unlock(&btv->lock);
 		field = (f->fmt.pix.height > height2)
 			? V4L2_FIELD_INTERLACED
 			: V4L2_FIELD_BOTTOM;
@@ -2614,7 +2578,6 @@ static int bttv_s_fmt_vid_cap(struct file *file, void *priv,
 	fmt = format_by_fourcc(f->fmt.pix.pixelformat);
 
 	/* update our state informations */
-	mutex_lock(&fh->cap.vb_lock);
 	fh->fmt              = fmt;
 	fh->cap.field        = f->fmt.pix.field;
 	fh->cap.last         = V4L2_FIELD_NONE;
@@ -2623,7 +2586,6 @@ static int bttv_s_fmt_vid_cap(struct file *file, void *priv,
 	btv->init.fmt        = fmt;
 	btv->init.width      = f->fmt.pix.width;
 	btv->init.height     = f->fmt.pix.height;
-	mutex_unlock(&fh->cap.vb_lock);
 
 	return 0;
 }
@@ -2649,11 +2611,9 @@ static int vidiocgmbuf(struct file *file, void *priv, struct video_mbuf *mbuf)
 	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);
 	if (retval < 0) {
-		mutex_unlock(&fh->cap.vb_lock);
 		return retval;
 	}
 
@@ -2665,7 +2625,6 @@ static int vidiocgmbuf(struct file *file, void *priv, struct video_mbuf *mbuf)
 	for (i = 0; i < gbuffers; i++)
 		mbuf->offsets[i] = i * gbufsize;
 
-	mutex_unlock(&fh->cap.vb_lock);
 	return 0;
 }
 #endif
@@ -2775,10 +2734,8 @@ static int bttv_overlay(struct file *file, void *f, unsigned int on)
 	int retval = 0;
 
 	if (on) {
-		mutex_lock(&fh->cap.vb_lock);
 		/* verify args */
 		if (unlikely(!btv->fbuf.base)) {
-			mutex_unlock(&fh->cap.vb_lock);
 			return -EINVAL;
 		}
 		if (unlikely(!fh->ov.setup_ok)) {
@@ -2787,13 +2744,11 @@ static int bttv_overlay(struct file *file, void *f, unsigned int on)
 		}
 		if (retval)
 			return retval;
-		mutex_unlock(&fh->cap.vb_lock);
 	}
 
 	if (!check_alloc_btres_lock(btv, fh, RESOURCE_OVERLAY))
 		return -EBUSY;
 
-	mutex_lock(&fh->cap.vb_lock);
 	if (on) {
 		fh->ov.tvnorm = btv->tvnorm;
 		new = videobuf_sg_alloc(sizeof(*new));
@@ -2805,7 +2760,6 @@ static int bttv_overlay(struct file *file, void *f, unsigned int on)
 
 	/* switch over */
 	retval = bttv_switch_overlay(btv, fh, new);
-	mutex_unlock(&fh->cap.vb_lock);
 	return retval;
 }
 
@@ -2844,7 +2798,6 @@ static int bttv_s_fbuf(struct file *file, void *f,
 	}
 
 	/* ok, accept it */
-	mutex_lock(&fh->cap.vb_lock);
 	btv->fbuf.base       = fb->base;
 	btv->fbuf.fmt.width  = fb->fmt.width;
 	btv->fbuf.fmt.height = fb->fmt.height;
@@ -2876,7 +2829,6 @@ static int bttv_s_fbuf(struct file *file, void *f,
 			retval = bttv_switch_overlay(btv, fh, new);
 		}
 	}
-	mutex_unlock(&fh->cap.vb_lock);
 	return retval;
 }
 
@@ -2955,7 +2907,6 @@ static int bttv_queryctrl(struct file *file, void *priv,
 	     c->id >= V4L2_CID_PRIVATE_LASTP1))
 		return -EINVAL;
 
-	mutex_lock(&btv->lock);
 	if (!btv->volume_gpio && (c->id == V4L2_CID_AUDIO_VOLUME))
 		*c = no_ctl;
 	else {
@@ -2963,7 +2914,6 @@ static int bttv_queryctrl(struct file *file, void *priv,
 
 		*c = (NULL != ctrl) ? *ctrl : no_ctl;
 	}
-	mutex_unlock(&btv->lock);
 
 	return 0;
 }
@@ -2974,10 +2924,8 @@ static int bttv_g_parm(struct file *file, void *f,
 	struct bttv_fh *fh = f;
 	struct bttv *btv = fh->btv;
 
-	mutex_lock(&btv->lock);
 	v4l2_video_std_frame_period(bttv_tvnorms[btv->tvnorm].v4l2_id,
 				    &parm->parm.capture.timeperframe);
-	mutex_unlock(&btv->lock);
 
 	return 0;
 }
@@ -2993,7 +2941,6 @@ static int bttv_g_tuner(struct file *file, void *priv,
 	if (0 != t->index)
 		return -EINVAL;
 
-	mutex_lock(&btv->lock);
 	t->rxsubchans = V4L2_TUNER_SUB_MONO;
 	bttv_call_all(btv, tuner, g_tuner, t);
 	strcpy(t->name, "Television");
@@ -3005,7 +2952,6 @@ static int bttv_g_tuner(struct file *file, void *priv,
 	if (btv->audio_mode_gpio)
 		btv->audio_mode_gpio(btv, t, 0);
 
-	mutex_unlock(&btv->lock);
 	return 0;
 }
 
@@ -3014,9 +2960,7 @@ static int bttv_g_priority(struct file *file, void *f, enum v4l2_priority *p)
 	struct bttv_fh *fh = f;
 	struct bttv *btv = fh->btv;
 
-	mutex_lock(&btv->lock);
 	*p = v4l2_prio_max(&btv->prio);
-	mutex_unlock(&btv->lock);
 
 	return 0;
 }
@@ -3028,9 +2972,7 @@ static int bttv_s_priority(struct file *file, void *f,
 	struct bttv *btv = fh->btv;
 	int	rc;
 
-	mutex_lock(&btv->lock);
 	rc = v4l2_prio_change(&btv->prio, &fh->prio, prio);
-	mutex_unlock(&btv->lock);
 
 	return rc;
 }
@@ -3045,9 +2987,7 @@ static int bttv_cropcap(struct file *file, void *priv,
 	    cap->type != V4L2_BUF_TYPE_VIDEO_OVERLAY)
 		return -EINVAL;
 
-	mutex_lock(&btv->lock);
 	*cap = bttv_tvnorms[btv->tvnorm].cropcap;
-	mutex_unlock(&btv->lock);
 
 	return 0;
 }
@@ -3065,9 +3005,7 @@ static int bttv_g_crop(struct file *file, void *f, struct v4l2_crop *crop)
 	   inconsistent with fh->width or fh->height and apps
 	   do not expect a change here. */
 
-	mutex_lock(&btv->lock);
 	crop->c = btv->crop[!!fh->do_crop].rect;
-	mutex_unlock(&btv->lock);
 
 	return 0;
 }
@@ -3091,17 +3029,14 @@ static int bttv_s_crop(struct file *file, void *f, struct v4l2_crop *crop)
 	/* Make sure tvnorm, vbi_end and the current cropping
 	   parameters remain consistent until we're done. Note
 	   read() may change vbi_end in check_alloc_btres_lock(). */
-	mutex_lock(&btv->lock);
 	retval = v4l2_prio_check(&btv->prio, fh->prio);
 	if (0 != retval) {
-		mutex_unlock(&btv->lock);
 		return retval;
 	}
 
 	retval = -EBUSY;
 
 	if (locked_btres(fh->btv, VIDEO_RESOURCES)) {
-		mutex_unlock(&btv->lock);
 		return retval;
 	}
 
@@ -3113,7 +3048,6 @@ static int bttv_s_crop(struct file *file, void *f, struct v4l2_crop *crop)
 
 	b_top = max(b->top, btv->vbi_end);
 	if (b_top + 32 >= b_bottom) {
-		mutex_unlock(&btv->lock);
 		return retval;
 	}
 
@@ -3136,12 +3070,8 @@ static int bttv_s_crop(struct file *file, void *f, struct v4l2_crop *crop)
 
 	btv->crop[1] = c;
 
-	mutex_unlock(&btv->lock);
-
 	fh->do_crop = 1;
 
-	mutex_lock(&fh->cap.vb_lock);
-
 	if (fh->width < c.min_scaled_width) {
 		fh->width = c.min_scaled_width;
 		btv->init.width = c.min_scaled_width;
@@ -3158,8 +3088,6 @@ static int bttv_s_crop(struct file *file, void *f, struct v4l2_crop *crop)
 		btv->init.height = c.max_scaled_height;
 	}
 
-	mutex_unlock(&fh->cap.vb_lock);
-
 	return 0;
 }
 
@@ -3227,7 +3155,6 @@ static unsigned int bttv_poll(struct file *file, poll_table *wait)
 		return videobuf_poll_stream(file, &fh->vbi, wait);
 	}
 
-	mutex_lock(&fh->cap.vb_lock);
 	if (check_btres(fh,RESOURCE_VIDEO_STREAM)) {
 		/* streaming capture */
 		if (list_empty(&fh->cap.stream))
@@ -3262,7 +3189,6 @@ static unsigned int bttv_poll(struct file *file, poll_table *wait)
 	else
 		rc = 0;
 err:
-	mutex_unlock(&fh->cap.vb_lock);
 	return rc;
 }
 
@@ -3302,14 +3228,11 @@ static int bttv_open(struct file *file)
 	 * Let's first copy btv->init at fh, holding cap.vb_lock, and then work
 	 * with the rest of init, holding btv->lock.
 	 */
-	mutex_lock(&fh->cap.vb_lock);
 	*fh = btv->init;
-	mutex_unlock(&fh->cap.vb_lock);
 
 	fh->type = type;
 	fh->ov.setup_ok = 0;
 
-	mutex_lock(&btv->lock);
 	v4l2_prio_open(&btv->prio, &fh->prio);
 
 	videobuf_queue_sg_init(&fh->cap, &bttv_video_qops,
@@ -3317,13 +3240,13 @@ static int bttv_open(struct file *file)
 			    V4L2_BUF_TYPE_VIDEO_CAPTURE,
 			    V4L2_FIELD_INTERLACED,
 			    sizeof(struct bttv_buffer),
-			    fh, NULL);
+			    fh, &btv->lock);
 	videobuf_queue_sg_init(&fh->vbi, &bttv_vbi_qops,
 			    &btv->c.pci->dev, &btv->s_lock,
 			    V4L2_BUF_TYPE_VBI_CAPTURE,
 			    V4L2_FIELD_SEQ_TB,
 			    sizeof(struct bttv_buffer),
-			    fh, NULL);
+			    fh, &btv->lock);
 	set_tvnorm(btv,btv->tvnorm);
 	set_input(btv, btv->input, btv->tvnorm);
 
@@ -3346,7 +3269,6 @@ static int bttv_open(struct file *file)
 	bttv_vbi_fmt_reset(&fh->vbi_fmt, btv->tvnorm);
 
 	bttv_field_count(btv);
-	mutex_unlock(&btv->lock);
 	return 0;
 }
 
@@ -3355,7 +3277,6 @@ static int bttv_release(struct file *file)
 	struct bttv_fh *fh = file->private_data;
 	struct bttv *btv = fh->btv;
 
-	mutex_lock(&btv->lock);
 	/* turn off overlay */
 	if (check_btres(fh, RESOURCE_OVERLAY))
 		bttv_switch_overlay(btv,fh,NULL);
@@ -3385,10 +3306,8 @@ static int bttv_release(struct file *file)
 	 * videobuf uses cap.vb_lock - we should avoid holding btv->lock,
 	 * otherwise we may have dead lock conditions
 	 */
-	mutex_unlock(&btv->lock);
 	videobuf_mmap_free(&fh->cap);
 	videobuf_mmap_free(&fh->vbi);
-	mutex_lock(&btv->lock);
 	v4l2_prio_close(&btv->prio, fh->prio);
 	file->private_data = NULL;
 	kfree(fh);
@@ -3398,7 +3317,6 @@ static int bttv_release(struct file *file)
 
 	if (!btv->users)
 		audio_mute(btv, 1);
-	mutex_unlock(&btv->lock);
 
 	return 0;
 }
@@ -3502,11 +3420,8 @@ static int radio_open(struct file *file)
 	if (unlikely(!fh))
 		return -ENOMEM;
 	file->private_data = fh;
-	mutex_lock(&fh->cap.vb_lock);
 	*fh = btv->init;
-	mutex_unlock(&fh->cap.vb_lock);
 
-	mutex_lock(&btv->lock);
 	v4l2_prio_open(&btv->prio, &fh->prio);
 
 	btv->radio_user++;
@@ -3514,7 +3429,6 @@ static int radio_open(struct file *file)
 	bttv_call_all(btv, tuner, s_radio);
 	audio_input(btv,TVAUDIO_INPUT_RADIO);
 
-	mutex_unlock(&btv->lock);
 	return 0;
 }
 
@@ -3524,7 +3438,6 @@ static int radio_release(struct file *file)
 	struct bttv *btv = fh->btv;
 	struct rds_command cmd;
 
-	mutex_lock(&btv->lock);
 	v4l2_prio_close(&btv->prio, fh->prio);
 	file->private_data = NULL;
 	kfree(fh);
@@ -3532,7 +3445,6 @@ static int radio_release(struct file *file)
 	btv->radio_user--;
 
 	bttv_call_all(btv, core, ioctl, RDS_CMD_CLOSE, &cmd);
-	mutex_unlock(&btv->lock);
 
 	return 0;
 }
@@ -3561,7 +3473,6 @@ static int radio_g_tuner(struct file *file, void *priv, struct v4l2_tuner *t)
 		return -EINVAL;
 	if (0 != t->index)
 		return -EINVAL;
-	mutex_lock(&btv->lock);
 	strcpy(t->name, "Radio");
 	t->type = V4L2_TUNER_RADIO;
 
@@ -3570,8 +3481,6 @@ static int radio_g_tuner(struct file *file, void *priv, struct v4l2_tuner *t)
 	if (btv->audio_mode_gpio)
 		btv->audio_mode_gpio(btv, t, 0);
 
-	mutex_unlock(&btv->lock);
-
 	return 0;
 }
 
@@ -3692,7 +3601,7 @@ static const struct v4l2_file_operations radio_fops =
 	.open	  = radio_open,
 	.read     = radio_read,
 	.release  = radio_release,
-	.ioctl	  = video_ioctl2,
+	.unlocked_ioctl = video_ioctl2,
 	.poll     = radio_poll,
 };
 

Re: [PATCH] bttv: fix mutex use before init

[Chris Clayton]

On Wednesday 15 December 2010, Mauro Carvalho Chehab wrote:
> Em 15-12-2010 16:44, Chris Clayton escreveu:
> > On Tuesday 14 December 2010, Brandon Philips wrote:
> >> On 17:13 Sun 12 Dec 2010, Torsten Kaiser wrote:
> >>>  * change &fh->cap.vb_lock in bttv_open() AND radio_open() to
> >>> &btv->init.cap.vb_lock
> >>>  * add a mutex_init(&btv->init.cap.vb_lock) to the setup of init in
> >>> bttv_probe()
> >>
> >> That seems like a reasonable suggestion. An openSUSE user submitted this
> >> bug to our tracker too. Here is the patch I am having him test.
> >>
> >> Would you mind testing it?
> >>
> >> From 456dc0ce36db523c4c0c8a269f4eec43a72de1dc Mon Sep 17 00:00:00 2001
> >> From: Brandon Philips <bphilips@suse.de>
> >> Date: Mon, 13 Dec 2010 16:21:55 -0800
> >> Subject: [PATCH] bttv: fix locking for btv->init
> >>
> >> Fix locking for the btv->init by using btv->init.cap.vb_lock and in the
> >> process fix uninitialized deref introduced in c37db91fd0d.
> >>
> >> Signed-off-by: Brandon Philips <bphilips@suse.de>
>
> Hi Brandon & all,
>
> While your patch fixes the issue, it has some other troubles, like to the
> presence of lock code at free_btres_lock(). It is possible to fix, but the
> better is to just use the core-assisted locking schema. This way, V4L2 core
> will serialize access to all ioctl's/open/close/mmap/read/poll operations,
> avoiding to have two processes accessing the hardware at the same time.
> Also, as there's just one lock, instead of 3, there's no risk of dead
> locks.
>
> The net result is a cleaner code, with just one lock.
>
> I tested the patch here with an bttv-based STB board (card=3, tuner=6), and
> it worked fine for me. Could you please test if this fixes the issue?
>
> PS.: The patch is based against the bkl_removal patches, at my linux-next
> tree:
>
> http://git.kernel.org/?p=linux/kernel/git/mchehab/linux-next.git;a=commit;h
>=673eb9ff33e26ee6f4278cdab06749aef1bbef5b
>
> (I just created the bkl_removal branch, so, it may take some time for you
> to see at the kernel.org mirrors, but it is basically changeset 673eb9ff.
> You may also just apply it on the top of the master branch of my linux-next
> tree).
>
> ---
>
> [media] bttv: Fix locking issues due to BKL removal code
>
> The BKL removal patch added a condition where the code would try to use a
> non-initialized lock. While a patch just addressing the issue is possible,
> there are some other troubles, like to the presence of lock code at
> free_btres_lock(), called on some places with the lock already taken. It is
> possible to fix, but the better is to just use the core-assisted locking
> schema.
>
> This way, V4L2 core will serialize access to all
> ioctl's/open/close/mmap/read/poll operations, avoiding to have two
> processes accessing the hardware at the same time. Also, as there's just
> one lock, instead of 3, there's no risk of dead locks.
>
> Tested with bttv STB, Gateway P/N 6000699 (card 3, tuner 6).
>
> Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
>
> diff --git a/drivers/media/video/bt8xx/bttv-driver.c
> b/drivers/media/video/bt8xx/bttv-driver.c index a529619..25e1ca0 100644
> --- a/drivers/media/video/bt8xx/bttv-driver.c
> +++ b/drivers/media/video/bt8xx/bttv-driver.c
> @@ -854,7 +854,6 @@ int check_alloc_btres_lock(struct bttv *btv, struct
> bttv_fh *fh, int bit) xbits |= RESOURCE_VIDEO_READ | RESOURCE_VIDEO_STREAM;
>
>  	/* is it free? */
> -	mutex_lock(&btv->lock);
>  	if (btv->resources & xbits) {
>  		/* no, someone else uses it */
>  		goto fail;
> @@ -884,11 +883,9 @@ int check_alloc_btres_lock(struct bttv *btv, struct
> bttv_fh *fh, int bit) /* it's free, grab it */
>  	fh->resources  |= bit;
>  	btv->resources |= bit;
> -	mutex_unlock(&btv->lock);
>  	return 1;
>
>   fail:
> -	mutex_unlock(&btv->lock);
>  	return 0;
>  }
>
> @@ -940,7 +937,6 @@ void free_btres_lock(struct bttv *btv, struct bttv_fh
> *fh, int bits) /* trying to free ressources not allocated by us ... */
>  		printk("bttv: BUG! (btres)\n");
>  	}
> -	mutex_lock(&btv->lock);
>  	fh->resources  &= ~bits;
>  	btv->resources &= ~bits;
>
> @@ -951,8 +947,6 @@ void free_btres_lock(struct bttv *btv, struct bttv_fh
> *fh, int bits)
>
>  	if (0 == (bits & VBI_RESOURCES))
>  		disclaim_vbi_lines(btv);
> -
> -	mutex_unlock(&btv->lock);
>  }
>
>  /* -----------------------------------------------------------------------
> */ @@ -1713,28 +1707,20 @@ static int bttv_prepare_buffer(struct
> videobuf_queue *q,struct bttv *btv,
>
>  		/* Make sure tvnorm and vbi_end remain consistent
>  		   until we're done. */
> -		mutex_lock(&btv->lock);
>
>  		norm = btv->tvnorm;
>
>  		/* In this mode capturing always starts at defrect.top
>  		   (default VDELAY), ignoring cropping parameters. */
>  		if (btv->vbi_end > bttv_tvnorms[norm].cropcap.defrect.top) {
> -			mutex_unlock(&btv->lock);
>  			return -EINVAL;
>  		}
>
> -		mutex_unlock(&btv->lock);
> -
>  		c.rect = bttv_tvnorms[norm].cropcap.defrect;
>  	} else {
> -		mutex_lock(&btv->lock);
> -
>  		norm = btv->tvnorm;
>  		c = btv->crop[!!fh->do_crop];
>
> -		mutex_unlock(&btv->lock);
> -
>  		if (width < c.min_scaled_width ||
>  		    width > c.max_scaled_width ||
>  		    height < c.min_scaled_height)
> @@ -1858,7 +1844,6 @@ static int bttv_s_std(struct file *file, void *priv,
> v4l2_std_id *id) unsigned int i;
>  	int err;
>
> -	mutex_lock(&btv->lock);
>  	err = v4l2_prio_check(&btv->prio, fh->prio);
>  	if (err)
>  		goto err;
> @@ -1874,7 +1859,6 @@ static int bttv_s_std(struct file *file, void *priv,
> v4l2_std_id *id) set_tvnorm(btv, i);
>
>  err:
> -	mutex_unlock(&btv->lock);
>
>  	return err;
>  }
> @@ -1898,7 +1882,6 @@ static int bttv_enum_input(struct file *file, void
> *priv, struct bttv *btv = fh->btv;
>  	int rc = 0;
>
> -	mutex_lock(&btv->lock);
>  	if (i->index >= bttv_tvcards[btv->c.type].video_inputs) {
>  		rc = -EINVAL;
>  		goto err;
> @@ -1928,7 +1911,6 @@ static int bttv_enum_input(struct file *file, void
> *priv, i->std = BTTV_NORMS;
>
>  err:
> -	mutex_unlock(&btv->lock);
>
>  	return rc;
>  }
> @@ -1938,9 +1920,7 @@ static int bttv_g_input(struct file *file, void
> *priv, unsigned int *i) struct bttv_fh *fh = priv;
>  	struct bttv *btv = fh->btv;
>
> -	mutex_lock(&btv->lock);
>  	*i = btv->input;
> -	mutex_unlock(&btv->lock);
>
>  	return 0;
>  }
> @@ -1952,7 +1932,6 @@ static int bttv_s_input(struct file *file, void
> *priv, unsigned int i)
>
>  	int err;
>
> -	mutex_lock(&btv->lock);
>  	err = v4l2_prio_check(&btv->prio, fh->prio);
>  	if (unlikely(err))
>  		goto err;
> @@ -1965,7 +1944,6 @@ static int bttv_s_input(struct file *file, void
> *priv, unsigned int i) set_input(btv, i, btv->tvnorm);
>
>  err:
> -	mutex_unlock(&btv->lock);
>  	return 0;
>  }
>
> @@ -1979,7 +1957,6 @@ static int bttv_s_tuner(struct file *file, void
> *priv, if (unlikely(0 != t->index))
>  		return -EINVAL;
>
> -	mutex_lock(&btv->lock);
>  	if (unlikely(btv->tuner_type == TUNER_ABSENT)) {
>  		err = -EINVAL;
>  		goto err;
> @@ -1995,7 +1972,6 @@ static int bttv_s_tuner(struct file *file, void
> *priv, btv->audio_mode_gpio(btv, t, 1);
>
>  err:
> -	mutex_unlock(&btv->lock);
>
>  	return 0;
>  }
> @@ -2006,10 +1982,8 @@ static int bttv_g_frequency(struct file *file, void
> *priv, struct bttv_fh *fh  = priv;
>  	struct bttv *btv = fh->btv;
>
> -	mutex_lock(&btv->lock);
>  	f->type = btv->radio_user ? V4L2_TUNER_RADIO : V4L2_TUNER_ANALOG_TV;
>  	f->frequency = btv->freq;
> -	mutex_unlock(&btv->lock);
>
>  	return 0;
>  }
> @@ -2024,7 +1998,6 @@ static int bttv_s_frequency(struct file *file, void
> *priv, if (unlikely(f->tuner != 0))
>  		return -EINVAL;
>
> -	mutex_lock(&btv->lock);
>  	err = v4l2_prio_check(&btv->prio, fh->prio);
>  	if (unlikely(err))
>  		goto err;
> @@ -2039,7 +2012,6 @@ static int bttv_s_frequency(struct file *file, void
> *priv, if (btv->has_matchbox && btv->radio_user)
>  		tea5757_set_freq(btv, btv->freq);
>  err:
> -	mutex_unlock(&btv->lock);
>
>  	return 0;
>  }
> @@ -2172,7 +2144,6 @@ limit_scaled_size_lock       (struct bttv_fh *       
>        fh,
>
>  	/* Make sure tvnorm, vbi_end and the current cropping parameters
>  	   remain consistent until we're done. */
> -	mutex_lock(&btv->lock);
>
>  	b = &bttv_tvnorms[btv->tvnorm].cropcap.bounds;
>
> @@ -2250,7 +2221,6 @@ limit_scaled_size_lock       (struct bttv_fh *       
>        fh, rc = 0; /* success */
>
>   fail:
> -	mutex_unlock(&btv->lock);
>
>  	return rc;
>  }
> @@ -2282,9 +2252,7 @@ verify_window_lock		(struct bttv_fh *              
> fh, if (V4L2_FIELD_ANY == field) {
>  		__s32 height2;
>
> -		mutex_lock(&fh->btv->lock);
>  		height2 = fh->btv->crop[!!fh->do_crop].rect.height >> 1;
> -		mutex_unlock(&fh->btv->lock);
>  		field = (win->w.height > height2)
>  			? V4L2_FIELD_INTERLACED
>
>  			: V4L2_FIELD_TOP;
>
> @@ -2360,7 +2328,6 @@ static int setup_window_lock(struct bttv_fh *fh,
> struct bttv *btv, }
>  	}
>
> -	mutex_lock(&fh->cap.vb_lock);
>  	/* clip against screen */
>  	if (NULL != btv->fbuf.base)
>  		n = btcx_screen_clips(btv->fbuf.fmt.width, btv->fbuf.fmt.height,
> @@ -2412,7 +2379,6 @@ static int setup_window_lock(struct bttv_fh *fh,
> struct bttv *btv, bttv_overlay_risc(btv, &fh->ov, fh->ovfmt, new);
>  		retval = bttv_switch_overlay(btv,fh,new);
>  	}
> -	mutex_unlock(&fh->cap.vb_lock);
>  	return retval;
>  }
>
> @@ -2526,9 +2492,7 @@ static int bttv_try_fmt_vid_cap(struct file *file,
> void *priv, if (V4L2_FIELD_ANY == field) {
>  		__s32 height2;
>
> -		mutex_lock(&btv->lock);
>  		height2 = btv->crop[!!fh->do_crop].rect.height >> 1;
> -		mutex_unlock(&btv->lock);
>  		field = (f->fmt.pix.height > height2)
>  			? V4L2_FIELD_INTERLACED
>
>  			: V4L2_FIELD_BOTTOM;
>
> @@ -2614,7 +2578,6 @@ static int bttv_s_fmt_vid_cap(struct file *file, void
> *priv, fmt = format_by_fourcc(f->fmt.pix.pixelformat);
>
>  	/* update our state informations */
> -	mutex_lock(&fh->cap.vb_lock);
>  	fh->fmt              = fmt;
>  	fh->cap.field        = f->fmt.pix.field;
>  	fh->cap.last         = V4L2_FIELD_NONE;
> @@ -2623,7 +2586,6 @@ static int bttv_s_fmt_vid_cap(struct file *file, void
> *priv, btv->init.fmt        = fmt;
>  	btv->init.width      = f->fmt.pix.width;
>  	btv->init.height     = f->fmt.pix.height;
> -	mutex_unlock(&fh->cap.vb_lock);
>
>  	return 0;
>  }
> @@ -2649,11 +2611,9 @@ static int vidiocgmbuf(struct file *file, void
> *priv, struct video_mbuf *mbuf) 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);
>  	if (retval < 0) {
> -		mutex_unlock(&fh->cap.vb_lock);
>  		return retval;
>  	}
>
> @@ -2665,7 +2625,6 @@ static int vidiocgmbuf(struct file *file, void *priv,
> struct video_mbuf *mbuf) for (i = 0; i < gbuffers; i++)
>  		mbuf->offsets[i] = i * gbufsize;
>
> -	mutex_unlock(&fh->cap.vb_lock);
>  	return 0;
>  }
>  #endif
> @@ -2775,10 +2734,8 @@ static int bttv_overlay(struct file *file, void *f,
> unsigned int on) int retval = 0;
>
>  	if (on) {
> -		mutex_lock(&fh->cap.vb_lock);
>  		/* verify args */
>  		if (unlikely(!btv->fbuf.base)) {
> -			mutex_unlock(&fh->cap.vb_lock);
>  			return -EINVAL;
>  		}
>  		if (unlikely(!fh->ov.setup_ok)) {
> @@ -2787,13 +2744,11 @@ static int bttv_overlay(struct file *file, void *f,
> unsigned int on) }
>  		if (retval)
>  			return retval;
> -		mutex_unlock(&fh->cap.vb_lock);
>  	}
>
>  	if (!check_alloc_btres_lock(btv, fh, RESOURCE_OVERLAY))
>  		return -EBUSY;
>
> -	mutex_lock(&fh->cap.vb_lock);
>  	if (on) {
>  		fh->ov.tvnorm = btv->tvnorm;
>  		new = videobuf_sg_alloc(sizeof(*new));
> @@ -2805,7 +2760,6 @@ static int bttv_overlay(struct file *file, void *f,
> unsigned int on)
>
>  	/* switch over */
>  	retval = bttv_switch_overlay(btv, fh, new);
> -	mutex_unlock(&fh->cap.vb_lock);
>  	return retval;
>  }
>
> @@ -2844,7 +2798,6 @@ static int bttv_s_fbuf(struct file *file, void *f,
>  	}
>
>  	/* ok, accept it */
> -	mutex_lock(&fh->cap.vb_lock);
>  	btv->fbuf.base       = fb->base;
>  	btv->fbuf.fmt.width  = fb->fmt.width;
>  	btv->fbuf.fmt.height = fb->fmt.height;
> @@ -2876,7 +2829,6 @@ static int bttv_s_fbuf(struct file *file, void *f,
>  			retval = bttv_switch_overlay(btv, fh, new);
>  		}
>  	}
> -	mutex_unlock(&fh->cap.vb_lock);
>  	return retval;
>  }
>
> @@ -2955,7 +2907,6 @@ static int bttv_queryctrl(struct file *file, void
> *priv, c->id >= V4L2_CID_PRIVATE_LASTP1))
>  		return -EINVAL;
>
> -	mutex_lock(&btv->lock);
>  	if (!btv->volume_gpio && (c->id == V4L2_CID_AUDIO_VOLUME))
>  		*c = no_ctl;
>  	else {
> @@ -2963,7 +2914,6 @@ static int bttv_queryctrl(struct file *file, void
> *priv,
>
>  		*c = (NULL != ctrl) ? *ctrl : no_ctl;
>  	}
> -	mutex_unlock(&btv->lock);
>
>  	return 0;
>  }
> @@ -2974,10 +2924,8 @@ static int bttv_g_parm(struct file *file, void *f,
>  	struct bttv_fh *fh = f;
>  	struct bttv *btv = fh->btv;
>
> -	mutex_lock(&btv->lock);
>  	v4l2_video_std_frame_period(bttv_tvnorms[btv->tvnorm].v4l2_id,
>  				    &parm->parm.capture.timeperframe);
> -	mutex_unlock(&btv->lock);
>
>  	return 0;
>  }
> @@ -2993,7 +2941,6 @@ static int bttv_g_tuner(struct file *file, void
> *priv, if (0 != t->index)
>  		return -EINVAL;
>
> -	mutex_lock(&btv->lock);
>  	t->rxsubchans = V4L2_TUNER_SUB_MONO;
>  	bttv_call_all(btv, tuner, g_tuner, t);
>  	strcpy(t->name, "Television");
> @@ -3005,7 +2952,6 @@ static int bttv_g_tuner(struct file *file, void
> *priv, if (btv->audio_mode_gpio)
>  		btv->audio_mode_gpio(btv, t, 0);
>
> -	mutex_unlock(&btv->lock);
>  	return 0;
>  }
>
> @@ -3014,9 +2960,7 @@ static int bttv_g_priority(struct file *file, void
> *f, enum v4l2_priority *p) struct bttv_fh *fh = f;
>  	struct bttv *btv = fh->btv;
>
> -	mutex_lock(&btv->lock);
>  	*p = v4l2_prio_max(&btv->prio);
> -	mutex_unlock(&btv->lock);
>
>  	return 0;
>  }
> @@ -3028,9 +2972,7 @@ static int bttv_s_priority(struct file *file, void
> *f, struct bttv *btv = fh->btv;
>  	int	rc;
>
> -	mutex_lock(&btv->lock);
>  	rc = v4l2_prio_change(&btv->prio, &fh->prio, prio);
> -	mutex_unlock(&btv->lock);
>
>  	return rc;
>  }
> @@ -3045,9 +2987,7 @@ static int bttv_cropcap(struct file *file, void
> *priv, cap->type != V4L2_BUF_TYPE_VIDEO_OVERLAY)
>  		return -EINVAL;
>
> -	mutex_lock(&btv->lock);
>  	*cap = bttv_tvnorms[btv->tvnorm].cropcap;
> -	mutex_unlock(&btv->lock);
>
>  	return 0;
>  }
> @@ -3065,9 +3005,7 @@ static int bttv_g_crop(struct file *file, void *f,
> struct v4l2_crop *crop) inconsistent with fh->width or fh->height and apps
>  	   do not expect a change here. */
>
> -	mutex_lock(&btv->lock);
>  	crop->c = btv->crop[!!fh->do_crop].rect;
> -	mutex_unlock(&btv->lock);
>
>  	return 0;
>  }
> @@ -3091,17 +3029,14 @@ static int bttv_s_crop(struct file *file, void *f,
> struct v4l2_crop *crop) /* Make sure tvnorm, vbi_end and the current
> cropping
>  	   parameters remain consistent until we're done. Note
>  	   read() may change vbi_end in check_alloc_btres_lock(). */
> -	mutex_lock(&btv->lock);
>  	retval = v4l2_prio_check(&btv->prio, fh->prio);
>  	if (0 != retval) {
> -		mutex_unlock(&btv->lock);
>  		return retval;
>  	}
>
>  	retval = -EBUSY;
>
>  	if (locked_btres(fh->btv, VIDEO_RESOURCES)) {
> -		mutex_unlock(&btv->lock);
>  		return retval;
>  	}
>
> @@ -3113,7 +3048,6 @@ static int bttv_s_crop(struct file *file, void *f,
> struct v4l2_crop *crop)
>
>  	b_top = max(b->top, btv->vbi_end);
>  	if (b_top + 32 >= b_bottom) {
> -		mutex_unlock(&btv->lock);
>  		return retval;
>  	}
>
> @@ -3136,12 +3070,8 @@ static int bttv_s_crop(struct file *file, void *f,
> struct v4l2_crop *crop)
>
>  	btv->crop[1] = c;
>
> -	mutex_unlock(&btv->lock);
> -
>  	fh->do_crop = 1;
>
> -	mutex_lock(&fh->cap.vb_lock);
> -
>  	if (fh->width < c.min_scaled_width) {
>  		fh->width = c.min_scaled_width;
>  		btv->init.width = c.min_scaled_width;
> @@ -3158,8 +3088,6 @@ static int bttv_s_crop(struct file *file, void *f,
> struct v4l2_crop *crop) btv->init.height = c.max_scaled_height;
>  	}
>
> -	mutex_unlock(&fh->cap.vb_lock);
> -
>  	return 0;
>  }
>
> @@ -3227,7 +3155,6 @@ static unsigned int bttv_poll(struct file *file,
> poll_table *wait) return videobuf_poll_stream(file, &fh->vbi, wait);
>  	}
>
> -	mutex_lock(&fh->cap.vb_lock);
>  	if (check_btres(fh,RESOURCE_VIDEO_STREAM)) {
>  		/* streaming capture */
>  		if (list_empty(&fh->cap.stream))
> @@ -3262,7 +3189,6 @@ static unsigned int bttv_poll(struct file *file,
> poll_table *wait) else
>  		rc = 0;
>  err:
> -	mutex_unlock(&fh->cap.vb_lock);
>  	return rc;
>  }
>
> @@ -3302,14 +3228,11 @@ static int bttv_open(struct file *file)
>  	 * Let's first copy btv->init at fh, holding cap.vb_lock, and then work
>  	 * with the rest of init, holding btv->lock.
>  	 */
> -	mutex_lock(&fh->cap.vb_lock);
>  	*fh = btv->init;
> -	mutex_unlock(&fh->cap.vb_lock);
>
>  	fh->type = type;
>  	fh->ov.setup_ok = 0;
>
> -	mutex_lock(&btv->lock);
>  	v4l2_prio_open(&btv->prio, &fh->prio);
>
>  	videobuf_queue_sg_init(&fh->cap, &bttv_video_qops,
> @@ -3317,13 +3240,13 @@ static int bttv_open(struct file *file)
>  			    V4L2_BUF_TYPE_VIDEO_CAPTURE,
>  			    V4L2_FIELD_INTERLACED,
>  			    sizeof(struct bttv_buffer),
> -			    fh, NULL);
> +			    fh, &btv->lock);
>  	videobuf_queue_sg_init(&fh->vbi, &bttv_vbi_qops,
>  			    &btv->c.pci->dev, &btv->s_lock,
>  			    V4L2_BUF_TYPE_VBI_CAPTURE,
>  			    V4L2_FIELD_SEQ_TB,
>  			    sizeof(struct bttv_buffer),
> -			    fh, NULL);
> +			    fh, &btv->lock);
>  	set_tvnorm(btv,btv->tvnorm);
>  	set_input(btv, btv->input, btv->tvnorm);
>
> @@ -3346,7 +3269,6 @@ static int bttv_open(struct file *file)
>  	bttv_vbi_fmt_reset(&fh->vbi_fmt, btv->tvnorm);
>
>  	bttv_field_count(btv);
> -	mutex_unlock(&btv->lock);
>  	return 0;
>  }
>
> @@ -3355,7 +3277,6 @@ static int bttv_release(struct file *file)
>  	struct bttv_fh *fh = file->private_data;
>  	struct bttv *btv = fh->btv;
>
> -	mutex_lock(&btv->lock);
>  	/* turn off overlay */
>  	if (check_btres(fh, RESOURCE_OVERLAY))
>  		bttv_switch_overlay(btv,fh,NULL);
> @@ -3385,10 +3306,8 @@ static int bttv_release(struct file *file)
>  	 * videobuf uses cap.vb_lock - we should avoid holding btv->lock,
>  	 * otherwise we may have dead lock conditions
>  	 */
> -	mutex_unlock(&btv->lock);
>  	videobuf_mmap_free(&fh->cap);
>  	videobuf_mmap_free(&fh->vbi);
> -	mutex_lock(&btv->lock);
>  	v4l2_prio_close(&btv->prio, fh->prio);
>  	file->private_data = NULL;
>  	kfree(fh);
> @@ -3398,7 +3317,6 @@ static int bttv_release(struct file *file)
>
>  	if (!btv->users)
>  		audio_mute(btv, 1);
> -	mutex_unlock(&btv->lock);
>
>  	return 0;
>  }
> @@ -3502,11 +3420,8 @@ static int radio_open(struct file *file)
>  	if (unlikely(!fh))
>  		return -ENOMEM;
>  	file->private_data = fh;
> -	mutex_lock(&fh->cap.vb_lock);
>  	*fh = btv->init;
> -	mutex_unlock(&fh->cap.vb_lock);
>
> -	mutex_lock(&btv->lock);
>  	v4l2_prio_open(&btv->prio, &fh->prio);
>
>  	btv->radio_user++;
> @@ -3514,7 +3429,6 @@ static int radio_open(struct file *file)
>  	bttv_call_all(btv, tuner, s_radio);
>  	audio_input(btv,TVAUDIO_INPUT_RADIO);
>
> -	mutex_unlock(&btv->lock);
>  	return 0;
>  }
>
> @@ -3524,7 +3438,6 @@ static int radio_release(struct file *file)
>  	struct bttv *btv = fh->btv;
>  	struct rds_command cmd;
>
> -	mutex_lock(&btv->lock);
>  	v4l2_prio_close(&btv->prio, fh->prio);
>  	file->private_data = NULL;
>  	kfree(fh);
> @@ -3532,7 +3445,6 @@ static int radio_release(struct file *file)
>  	btv->radio_user--;
>
>  	bttv_call_all(btv, core, ioctl, RDS_CMD_CLOSE, &cmd);
> -	mutex_unlock(&btv->lock);
>
>  	return 0;
>  }
> @@ -3561,7 +3473,6 @@ static int radio_g_tuner(struct file *file, void
> *priv, struct v4l2_tuner *t) return -EINVAL;
>  	if (0 != t->index)
>  		return -EINVAL;
> -	mutex_lock(&btv->lock);
>  	strcpy(t->name, "Radio");
>  	t->type = V4L2_TUNER_RADIO;
>
> @@ -3570,8 +3481,6 @@ static int radio_g_tuner(struct file *file, void
> *priv, struct v4l2_tuner *t) if (btv->audio_mode_gpio)
>  		btv->audio_mode_gpio(btv, t, 0);
>
> -	mutex_unlock(&btv->lock);
> -
>  	return 0;
>  }
>
> @@ -3692,7 +3601,7 @@ static const struct v4l2_file_operations radio_fops =
>  	.open	  = radio_open,
>  	.read     = radio_read,
>  	.release  = radio_release,
> -	.ioctl	  = video_ioctl2,
> +	.unlocked_ioctl = video_ioctl2,
>  	.poll     = radio_poll,
>  };

This patch also prevents the oops and the Pinnacle PCTV Pro card TV is fine. 
Thanks Mauro.

Tested-by: Chris Clayton <chris2553@googlemail.com>

-- 
The more I see, the more I know. The more I know, the less I understand. 
Changing Man - Paul Weller

Re: [PATCH] bttv: fix mutex use before init

[Torsten Kaiser]

On Wed, Dec 15, 2010 at 10:45 PM, Mauro Carvalho Chehab
<mchehab@infradead.org> wrote:
> While your patch fixes the issue, it has some other troubles, like to the presence of
> lock code at free_btres_lock(). It is possible to fix, but the better is to just
> use the core-assisted locking schema. This way, V4L2 core will serialize access to all
> ioctl's/open/close/mmap/read/poll operations, avoiding to have two processes accessing
> the hardware at the same time. Also, as there's just one lock, instead of 3, there's no
> risk of dead locks.
>
> The net result is a cleaner code, with just one lock.
>
> I tested the patch here with an bttv-based STB board (card=3, tuner=6), and it worked fine for me.
> Could you please test if this fixes the issue?

I tested your patch against 2.6.37-rc6 and it fixed both problems I was seeing.
Restarting hald 100 times did not oops and tvtime now again quits cleanly.

So as it "WorksForMe", you can add my Tested-By, if you want.

Thanks,

Torsten

> PS.: The patch is based against the bkl_removal patches, at my linux-next tree:
>
> http://git.kernel.org/?p=linux/kernel/git/mchehab/linux-next.git;a=commit;h=673eb9ff33e26ee6f4278cdab06749aef1bbef5b
>
> (I just created the bkl_removal branch, so, it may take some time for you to see at the kernel.org
> mirrors, but it is basically changeset 673eb9ff. You may also just apply it on the top of the master
> branch of my linux-next tree).
>
> ---
>
> [media] bttv: Fix locking issues due to BKL removal code
>
> The BKL removal patch added a condition where the code would try to use a non-initialized
> lock. While a patch just addressing the issue is possible, there are some other troubles,
> like to the presence of lock code at free_btres_lock(), called on some places with the lock
> already taken. It is possible to fix, but the better is to just use the core-assisted
> locking schema.
>
> This way, V4L2 core will serialize access to all ioctl's/open/close/mmap/read/poll
> operations, avoiding to have two processes accessing the hardware at the same time.
> Also, as there's just one lock, instead of 3, there's no risk of dead locks.
>
> Tested with bttv STB, Gateway P/N 6000699 (card 3, tuner 6).
>
> Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
>
> diff --git a/drivers/media/video/bt8xx/bttv-driver.c b/drivers/media/video/bt8xx/bttv-driver.c
> index a529619..25e1ca0 100644
> --- a/drivers/media/video/bt8xx/bttv-driver.c
> +++ b/drivers/media/video/bt8xx/bttv-driver.c
> @@ -854,7 +854,6 @@ int check_alloc_btres_lock(struct bttv *btv, struct bttv_fh *fh, int bit)
>                xbits |= RESOURCE_VIDEO_READ | RESOURCE_VIDEO_STREAM;
>
>        /* is it free? */
> -       mutex_lock(&btv->lock);
>        if (btv->resources & xbits) {
>                /* no, someone else uses it */
>                goto fail;
> @@ -884,11 +883,9 @@ int check_alloc_btres_lock(struct bttv *btv, struct bttv_fh *fh, int bit)
>        /* it's free, grab it */
>        fh->resources  |= bit;
>        btv->resources |= bit;
> -       mutex_unlock(&btv->lock);
>        return 1;
>
>  fail:
> -       mutex_unlock(&btv->lock);
>        return 0;
>  }
>
> @@ -940,7 +937,6 @@ void free_btres_lock(struct bttv *btv, struct bttv_fh *fh, int bits)
>                /* trying to free ressources not allocated by us ... */
>                printk("bttv: BUG! (btres)\n");
>        }
> -       mutex_lock(&btv->lock);
>        fh->resources  &= ~bits;
>        btv->resources &= ~bits;
>
> @@ -951,8 +947,6 @@ void free_btres_lock(struct bttv *btv, struct bttv_fh *fh, int bits)
>
>        if (0 == (bits & VBI_RESOURCES))
>                disclaim_vbi_lines(btv);
> -
> -       mutex_unlock(&btv->lock);
>  }
>
>  /* ----------------------------------------------------------------------- */
> @@ -1713,28 +1707,20 @@ static int bttv_prepare_buffer(struct videobuf_queue *q,struct bttv *btv,
>
>                /* Make sure tvnorm and vbi_end remain consistent
>                   until we're done. */
> -               mutex_lock(&btv->lock);
>
>                norm = btv->tvnorm;
>
>                /* In this mode capturing always starts at defrect.top
>                   (default VDELAY), ignoring cropping parameters. */
>                if (btv->vbi_end > bttv_tvnorms[norm].cropcap.defrect.top) {
> -                       mutex_unlock(&btv->lock);
>                        return -EINVAL;
>                }
>
> -               mutex_unlock(&btv->lock);
> -
>                c.rect = bttv_tvnorms[norm].cropcap.defrect;
>        } else {
> -               mutex_lock(&btv->lock);
> -
>                norm = btv->tvnorm;
>                c = btv->crop[!!fh->do_crop];
>
> -               mutex_unlock(&btv->lock);
> -
>                if (width < c.min_scaled_width ||
>                    width > c.max_scaled_width ||
>                    height < c.min_scaled_height)
> @@ -1858,7 +1844,6 @@ static int bttv_s_std(struct file *file, void *priv, v4l2_std_id *id)
>        unsigned int i;
>        int err;
>
> -       mutex_lock(&btv->lock);
>        err = v4l2_prio_check(&btv->prio, fh->prio);
>        if (err)
>                goto err;
> @@ -1874,7 +1859,6 @@ static int bttv_s_std(struct file *file, void *priv, v4l2_std_id *id)
>        set_tvnorm(btv, i);
>
>  err:
> -       mutex_unlock(&btv->lock);
>
>        return err;
>  }
> @@ -1898,7 +1882,6 @@ static int bttv_enum_input(struct file *file, void *priv,
>        struct bttv *btv = fh->btv;
>        int rc = 0;
>
> -       mutex_lock(&btv->lock);
>        if (i->index >= bttv_tvcards[btv->c.type].video_inputs) {
>                rc = -EINVAL;
>                goto err;
> @@ -1928,7 +1911,6 @@ static int bttv_enum_input(struct file *file, void *priv,
>        i->std = BTTV_NORMS;
>
>  err:
> -       mutex_unlock(&btv->lock);
>
>        return rc;
>  }
> @@ -1938,9 +1920,7 @@ static int bttv_g_input(struct file *file, void *priv, unsigned int *i)
>        struct bttv_fh *fh = priv;
>        struct bttv *btv = fh->btv;
>
> -       mutex_lock(&btv->lock);
>        *i = btv->input;
> -       mutex_unlock(&btv->lock);
>
>        return 0;
>  }
> @@ -1952,7 +1932,6 @@ static int bttv_s_input(struct file *file, void *priv, unsigned int i)
>
>        int err;
>
> -       mutex_lock(&btv->lock);
>        err = v4l2_prio_check(&btv->prio, fh->prio);
>        if (unlikely(err))
>                goto err;
> @@ -1965,7 +1944,6 @@ static int bttv_s_input(struct file *file, void *priv, unsigned int i)
>        set_input(btv, i, btv->tvnorm);
>
>  err:
> -       mutex_unlock(&btv->lock);
>        return 0;
>  }
>
> @@ -1979,7 +1957,6 @@ static int bttv_s_tuner(struct file *file, void *priv,
>        if (unlikely(0 != t->index))
>                return -EINVAL;
>
> -       mutex_lock(&btv->lock);
>        if (unlikely(btv->tuner_type == TUNER_ABSENT)) {
>                err = -EINVAL;
>                goto err;
> @@ -1995,7 +1972,6 @@ static int bttv_s_tuner(struct file *file, void *priv,
>                btv->audio_mode_gpio(btv, t, 1);
>
>  err:
> -       mutex_unlock(&btv->lock);
>
>        return 0;
>  }
> @@ -2006,10 +1982,8 @@ static int bttv_g_frequency(struct file *file, void *priv,
>        struct bttv_fh *fh  = priv;
>        struct bttv *btv = fh->btv;
>
> -       mutex_lock(&btv->lock);
>        f->type = btv->radio_user ? V4L2_TUNER_RADIO : V4L2_TUNER_ANALOG_TV;
>        f->frequency = btv->freq;
> -       mutex_unlock(&btv->lock);
>
>        return 0;
>  }
> @@ -2024,7 +1998,6 @@ static int bttv_s_frequency(struct file *file, void *priv,
>        if (unlikely(f->tuner != 0))
>                return -EINVAL;
>
> -       mutex_lock(&btv->lock);
>        err = v4l2_prio_check(&btv->prio, fh->prio);
>        if (unlikely(err))
>                goto err;
> @@ -2039,7 +2012,6 @@ static int bttv_s_frequency(struct file *file, void *priv,
>        if (btv->has_matchbox && btv->radio_user)
>                tea5757_set_freq(btv, btv->freq);
>  err:
> -       mutex_unlock(&btv->lock);
>
>        return 0;
>  }
> @@ -2172,7 +2144,6 @@ limit_scaled_size_lock       (struct bttv_fh *               fh,
>
>        /* Make sure tvnorm, vbi_end and the current cropping parameters
>           remain consistent until we're done. */
> -       mutex_lock(&btv->lock);
>
>        b = &bttv_tvnorms[btv->tvnorm].cropcap.bounds;
>
> @@ -2250,7 +2221,6 @@ limit_scaled_size_lock       (struct bttv_fh *               fh,
>        rc = 0; /* success */
>
>  fail:
> -       mutex_unlock(&btv->lock);
>
>        return rc;
>  }
> @@ -2282,9 +2252,7 @@ verify_window_lock                (struct bttv_fh *               fh,
>        if (V4L2_FIELD_ANY == field) {
>                __s32 height2;
>
> -               mutex_lock(&fh->btv->lock);
>                height2 = fh->btv->crop[!!fh->do_crop].rect.height >> 1;
> -               mutex_unlock(&fh->btv->lock);
>                field = (win->w.height > height2)
>                        ? V4L2_FIELD_INTERLACED
>                        : V4L2_FIELD_TOP;
> @@ -2360,7 +2328,6 @@ static int setup_window_lock(struct bttv_fh *fh, struct bttv *btv,
>                }
>        }
>
> -       mutex_lock(&fh->cap.vb_lock);
>        /* clip against screen */
>        if (NULL != btv->fbuf.base)
>                n = btcx_screen_clips(btv->fbuf.fmt.width, btv->fbuf.fmt.height,
> @@ -2412,7 +2379,6 @@ static int setup_window_lock(struct bttv_fh *fh, struct bttv *btv,
>                bttv_overlay_risc(btv, &fh->ov, fh->ovfmt, new);
>                retval = bttv_switch_overlay(btv,fh,new);
>        }
> -       mutex_unlock(&fh->cap.vb_lock);
>        return retval;
>  }
>
> @@ -2526,9 +2492,7 @@ static int bttv_try_fmt_vid_cap(struct file *file, void *priv,
>        if (V4L2_FIELD_ANY == field) {
>                __s32 height2;
>
> -               mutex_lock(&btv->lock);
>                height2 = btv->crop[!!fh->do_crop].rect.height >> 1;
> -               mutex_unlock(&btv->lock);
>                field = (f->fmt.pix.height > height2)
>                        ? V4L2_FIELD_INTERLACED
>                        : V4L2_FIELD_BOTTOM;
> @@ -2614,7 +2578,6 @@ static int bttv_s_fmt_vid_cap(struct file *file, void *priv,
>        fmt = format_by_fourcc(f->fmt.pix.pixelformat);
>
>        /* update our state informations */
> -       mutex_lock(&fh->cap.vb_lock);
>        fh->fmt              = fmt;
>        fh->cap.field        = f->fmt.pix.field;
>        fh->cap.last         = V4L2_FIELD_NONE;
> @@ -2623,7 +2586,6 @@ static int bttv_s_fmt_vid_cap(struct file *file, void *priv,
>        btv->init.fmt        = fmt;
>        btv->init.width      = f->fmt.pix.width;
>        btv->init.height     = f->fmt.pix.height;
> -       mutex_unlock(&fh->cap.vb_lock);
>
>        return 0;
>  }
> @@ -2649,11 +2611,9 @@ static int vidiocgmbuf(struct file *file, void *priv, struct video_mbuf *mbuf)
>        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);
>        if (retval < 0) {
> -               mutex_unlock(&fh->cap.vb_lock);
>                return retval;
>        }
>
> @@ -2665,7 +2625,6 @@ static int vidiocgmbuf(struct file *file, void *priv, struct video_mbuf *mbuf)
>        for (i = 0; i < gbuffers; i++)
>                mbuf->offsets[i] = i * gbufsize;
>
> -       mutex_unlock(&fh->cap.vb_lock);
>        return 0;
>  }
>  #endif
> @@ -2775,10 +2734,8 @@ static int bttv_overlay(struct file *file, void *f, unsigned int on)
>        int retval = 0;
>
>        if (on) {
> -               mutex_lock(&fh->cap.vb_lock);
>                /* verify args */
>                if (unlikely(!btv->fbuf.base)) {
> -                       mutex_unlock(&fh->cap.vb_lock);
>                        return -EINVAL;
>                }
>                if (unlikely(!fh->ov.setup_ok)) {
> @@ -2787,13 +2744,11 @@ static int bttv_overlay(struct file *file, void *f, unsigned int on)
>                }
>                if (retval)
>                        return retval;
> -               mutex_unlock(&fh->cap.vb_lock);
>        }
>
>        if (!check_alloc_btres_lock(btv, fh, RESOURCE_OVERLAY))
>                return -EBUSY;
>
> -       mutex_lock(&fh->cap.vb_lock);
>        if (on) {
>                fh->ov.tvnorm = btv->tvnorm;
>                new = videobuf_sg_alloc(sizeof(*new));
> @@ -2805,7 +2760,6 @@ static int bttv_overlay(struct file *file, void *f, unsigned int on)
>
>        /* switch over */
>        retval = bttv_switch_overlay(btv, fh, new);
> -       mutex_unlock(&fh->cap.vb_lock);
>        return retval;
>  }
>
> @@ -2844,7 +2798,6 @@ static int bttv_s_fbuf(struct file *file, void *f,
>        }
>
>        /* ok, accept it */
> -       mutex_lock(&fh->cap.vb_lock);
>        btv->fbuf.base       = fb->base;
>        btv->fbuf.fmt.width  = fb->fmt.width;
>        btv->fbuf.fmt.height = fb->fmt.height;
> @@ -2876,7 +2829,6 @@ static int bttv_s_fbuf(struct file *file, void *f,
>                        retval = bttv_switch_overlay(btv, fh, new);
>                }
>        }
> -       mutex_unlock(&fh->cap.vb_lock);
>        return retval;
>  }
>
> @@ -2955,7 +2907,6 @@ static int bttv_queryctrl(struct file *file, void *priv,
>             c->id >= V4L2_CID_PRIVATE_LASTP1))
>                return -EINVAL;
>
> -       mutex_lock(&btv->lock);
>        if (!btv->volume_gpio && (c->id == V4L2_CID_AUDIO_VOLUME))
>                *c = no_ctl;
>        else {
> @@ -2963,7 +2914,6 @@ static int bttv_queryctrl(struct file *file, void *priv,
>
>                *c = (NULL != ctrl) ? *ctrl : no_ctl;
>        }
> -       mutex_unlock(&btv->lock);
>
>        return 0;
>  }
> @@ -2974,10 +2924,8 @@ static int bttv_g_parm(struct file *file, void *f,
>        struct bttv_fh *fh = f;
>        struct bttv *btv = fh->btv;
>
> -       mutex_lock(&btv->lock);
>        v4l2_video_std_frame_period(bttv_tvnorms[btv->tvnorm].v4l2_id,
>                                    &parm->parm.capture.timeperframe);
> -       mutex_unlock(&btv->lock);
>
>        return 0;
>  }
> @@ -2993,7 +2941,6 @@ static int bttv_g_tuner(struct file *file, void *priv,
>        if (0 != t->index)
>                return -EINVAL;
>
> -       mutex_lock(&btv->lock);
>        t->rxsubchans = V4L2_TUNER_SUB_MONO;
>        bttv_call_all(btv, tuner, g_tuner, t);
>        strcpy(t->name, "Television");
> @@ -3005,7 +2952,6 @@ static int bttv_g_tuner(struct file *file, void *priv,
>        if (btv->audio_mode_gpio)
>                btv->audio_mode_gpio(btv, t, 0);
>
> -       mutex_unlock(&btv->lock);
>        return 0;
>  }
>
> @@ -3014,9 +2960,7 @@ static int bttv_g_priority(struct file *file, void *f, enum v4l2_priority *p)
>        struct bttv_fh *fh = f;
>        struct bttv *btv = fh->btv;
>
> -       mutex_lock(&btv->lock);
>        *p = v4l2_prio_max(&btv->prio);
> -       mutex_unlock(&btv->lock);
>
>        return 0;
>  }
> @@ -3028,9 +2972,7 @@ static int bttv_s_priority(struct file *file, void *f,
>        struct bttv *btv = fh->btv;
>        int     rc;
>
> -       mutex_lock(&btv->lock);
>        rc = v4l2_prio_change(&btv->prio, &fh->prio, prio);
> -       mutex_unlock(&btv->lock);
>
>        return rc;
>  }
> @@ -3045,9 +2987,7 @@ static int bttv_cropcap(struct file *file, void *priv,
>            cap->type != V4L2_BUF_TYPE_VIDEO_OVERLAY)
>                return -EINVAL;
>
> -       mutex_lock(&btv->lock);
>        *cap = bttv_tvnorms[btv->tvnorm].cropcap;
> -       mutex_unlock(&btv->lock);
>
>        return 0;
>  }
> @@ -3065,9 +3005,7 @@ static int bttv_g_crop(struct file *file, void *f, struct v4l2_crop *crop)
>           inconsistent with fh->width or fh->height and apps
>           do not expect a change here. */
>
> -       mutex_lock(&btv->lock);
>        crop->c = btv->crop[!!fh->do_crop].rect;
> -       mutex_unlock(&btv->lock);
>
>        return 0;
>  }
> @@ -3091,17 +3029,14 @@ static int bttv_s_crop(struct file *file, void *f, struct v4l2_crop *crop)
>        /* Make sure tvnorm, vbi_end and the current cropping
>           parameters remain consistent until we're done. Note
>           read() may change vbi_end in check_alloc_btres_lock(). */
> -       mutex_lock(&btv->lock);
>        retval = v4l2_prio_check(&btv->prio, fh->prio);
>        if (0 != retval) {
> -               mutex_unlock(&btv->lock);
>                return retval;
>        }
>
>        retval = -EBUSY;
>
>        if (locked_btres(fh->btv, VIDEO_RESOURCES)) {
> -               mutex_unlock(&btv->lock);
>                return retval;
>        }
>
> @@ -3113,7 +3048,6 @@ static int bttv_s_crop(struct file *file, void *f, struct v4l2_crop *crop)
>
>        b_top = max(b->top, btv->vbi_end);
>        if (b_top + 32 >= b_bottom) {
> -               mutex_unlock(&btv->lock);
>                return retval;
>        }
>
> @@ -3136,12 +3070,8 @@ static int bttv_s_crop(struct file *file, void *f, struct v4l2_crop *crop)
>
>        btv->crop[1] = c;
>
> -       mutex_unlock(&btv->lock);
> -
>        fh->do_crop = 1;
>
> -       mutex_lock(&fh->cap.vb_lock);
> -
>        if (fh->width < c.min_scaled_width) {
>                fh->width = c.min_scaled_width;
>                btv->init.width = c.min_scaled_width;
> @@ -3158,8 +3088,6 @@ static int bttv_s_crop(struct file *file, void *f, struct v4l2_crop *crop)
>                btv->init.height = c.max_scaled_height;
>        }
>
> -       mutex_unlock(&fh->cap.vb_lock);
> -
>        return 0;
>  }
>
> @@ -3227,7 +3155,6 @@ static unsigned int bttv_poll(struct file *file, poll_table *wait)
>                return videobuf_poll_stream(file, &fh->vbi, wait);
>        }
>
> -       mutex_lock(&fh->cap.vb_lock);
>        if (check_btres(fh,RESOURCE_VIDEO_STREAM)) {
>                /* streaming capture */
>                if (list_empty(&fh->cap.stream))
> @@ -3262,7 +3189,6 @@ static unsigned int bttv_poll(struct file *file, poll_table *wait)
>        else
>                rc = 0;
>  err:
> -       mutex_unlock(&fh->cap.vb_lock);
>        return rc;
>  }
>
> @@ -3302,14 +3228,11 @@ static int bttv_open(struct file *file)
>         * Let's first copy btv->init at fh, holding cap.vb_lock, and then work
>         * with the rest of init, holding btv->lock.
>         */
> -       mutex_lock(&fh->cap.vb_lock);
>        *fh = btv->init;
> -       mutex_unlock(&fh->cap.vb_lock);
>
>        fh->type = type;
>        fh->ov.setup_ok = 0;
>
> -       mutex_lock(&btv->lock);
>        v4l2_prio_open(&btv->prio, &fh->prio);
>
>        videobuf_queue_sg_init(&fh->cap, &bttv_video_qops,
> @@ -3317,13 +3240,13 @@ static int bttv_open(struct file *file)
>                            V4L2_BUF_TYPE_VIDEO_CAPTURE,
>                            V4L2_FIELD_INTERLACED,
>                            sizeof(struct bttv_buffer),
> -                           fh, NULL);
> +                           fh, &btv->lock);
>        videobuf_queue_sg_init(&fh->vbi, &bttv_vbi_qops,
>                            &btv->c.pci->dev, &btv->s_lock,
>                            V4L2_BUF_TYPE_VBI_CAPTURE,
>                            V4L2_FIELD_SEQ_TB,
>                            sizeof(struct bttv_buffer),
> -                           fh, NULL);
> +                           fh, &btv->lock);
>        set_tvnorm(btv,btv->tvnorm);
>        set_input(btv, btv->input, btv->tvnorm);
>
> @@ -3346,7 +3269,6 @@ static int bttv_open(struct file *file)
>        bttv_vbi_fmt_reset(&fh->vbi_fmt, btv->tvnorm);
>
>        bttv_field_count(btv);
> -       mutex_unlock(&btv->lock);
>        return 0;
>  }
>
> @@ -3355,7 +3277,6 @@ static int bttv_release(struct file *file)
>        struct bttv_fh *fh = file->private_data;
>        struct bttv *btv = fh->btv;
>
> -       mutex_lock(&btv->lock);
>        /* turn off overlay */
>        if (check_btres(fh, RESOURCE_OVERLAY))
>                bttv_switch_overlay(btv,fh,NULL);
> @@ -3385,10 +3306,8 @@ static int bttv_release(struct file *file)
>         * videobuf uses cap.vb_lock - we should avoid holding btv->lock,
>         * otherwise we may have dead lock conditions
>         */
> -       mutex_unlock(&btv->lock);
>        videobuf_mmap_free(&fh->cap);
>        videobuf_mmap_free(&fh->vbi);
> -       mutex_lock(&btv->lock);
>        v4l2_prio_close(&btv->prio, fh->prio);
>        file->private_data = NULL;
>        kfree(fh);
> @@ -3398,7 +3317,6 @@ static int bttv_release(struct file *file)
>
>        if (!btv->users)
>                audio_mute(btv, 1);
> -       mutex_unlock(&btv->lock);
>
>        return 0;
>  }
> @@ -3502,11 +3420,8 @@ static int radio_open(struct file *file)
>        if (unlikely(!fh))
>                return -ENOMEM;
>        file->private_data = fh;
> -       mutex_lock(&fh->cap.vb_lock);
>        *fh = btv->init;
> -       mutex_unlock(&fh->cap.vb_lock);
>
> -       mutex_lock(&btv->lock);
>        v4l2_prio_open(&btv->prio, &fh->prio);
>
>        btv->radio_user++;
> @@ -3514,7 +3429,6 @@ static int radio_open(struct file *file)
>        bttv_call_all(btv, tuner, s_radio);
>        audio_input(btv,TVAUDIO_INPUT_RADIO);
>
> -       mutex_unlock(&btv->lock);
>        return 0;
>  }
>
> @@ -3524,7 +3438,6 @@ static int radio_release(struct file *file)
>        struct bttv *btv = fh->btv;
>        struct rds_command cmd;
>
> -       mutex_lock(&btv->lock);
>        v4l2_prio_close(&btv->prio, fh->prio);
>        file->private_data = NULL;
>        kfree(fh);
> @@ -3532,7 +3445,6 @@ static int radio_release(struct file *file)
>        btv->radio_user--;
>
>        bttv_call_all(btv, core, ioctl, RDS_CMD_CLOSE, &cmd);
> -       mutex_unlock(&btv->lock);
>
>        return 0;
>  }
> @@ -3561,7 +3473,6 @@ static int radio_g_tuner(struct file *file, void *priv, struct v4l2_tuner *t)
>                return -EINVAL;
>        if (0 != t->index)
>                return -EINVAL;
> -       mutex_lock(&btv->lock);
>        strcpy(t->name, "Radio");
>        t->type = V4L2_TUNER_RADIO;
>
> @@ -3570,8 +3481,6 @@ static int radio_g_tuner(struct file *file, void *priv, struct v4l2_tuner *t)
>        if (btv->audio_mode_gpio)
>                btv->audio_mode_gpio(btv, t, 0);
>
> -       mutex_unlock(&btv->lock);
> -
>        return 0;
>  }
>
> @@ -3692,7 +3601,7 @@ static const struct v4l2_file_operations radio_fops =
>        .open     = radio_open,
>        .read     = radio_read,
>        .release  = radio_release,
> -       .ioctl    = video_ioctl2,
> +       .unlocked_ioctl = video_ioctl2,
>        .poll     = radio_poll,
>  };
>
>

Re: [PATCH] bttv: fix mutex use before init

[Brandon Philips]

On 19:45 Wed 15 Dec 2010, Mauro Carvalho Chehab wrote:
> Em 15-12-2010 16:44, Chris Clayton escreveu:
> > On Tuesday 14 December 2010, Brandon Philips wrote:
> >> On 17:13 Sun 12 Dec 2010, Torsten Kaiser wrote:
> >>>  * change &fh->cap.vb_lock in bttv_open() AND radio_open() to
> >>>  &btv->init.cap.vb_lock * add a mutex_init(&btv->init.cap.vb_lock)
> >>>  to the setup of init in bttv_probe()
> >>
> >> That seems like a reasonable suggestion. An openSUSE user submitted
> >> this bug to our tracker too. Here is the patch I am having him
> >> test.
> >>
> >> Would you mind testing it?
> >>
> >> From 456dc0ce36db523c4c0c8a269f4eec43a72de1dc Mon Sep 17 00:00:00
> >> 2001 From: Brandon Philips <bphilips@suse.de> Date: Mon, 13 Dec
> >> 2010 16:21:55 -0800 Subject: [PATCH] bttv: fix locking for
> >> btv->init
> >>
> >> Fix locking for the btv->init by using btv->init.cap.vb_lock and in
> >> the process fix uninitialized deref introduced in c37db91fd0d.
> >>
> >> Signed-off-by: Brandon Philips <bphilips@suse.de>
> 
> While your patch fixes the issue, it has some other troubles, like to
> the presence of lock code at free_btres_lock(). It is possible to fix,
> but the better is to just use the core-assisted locking schema. This
> way, V4L2 core will serialize access to all
> ioctl's/open/close/mmap/read/poll operations, avoiding to have two
> processes accessing the hardware at the same time. Also, as there's
> just one lock, instead of 3, there's no risk of dead locks.

Thanks, but, why wasn't this done instead of c37db91f?

Will this make it in before 2.6.37 is released? Otherwise 2.6.37 will
need to be fixed in -stable immediatly after release.

> The net result is a cleaner code, with just one lock.

Could you take this patch to remove all of the comments about locking
order with btv->lock since it doesn't seem to matter any longer.

Cheers,

	Brandon

P.S. Your mail client creates really long lines- somewhere around 90
characters. Could you fix that?

>From 7643db7bf5e9e557a27e3783786a1abecbdf82a7 Mon Sep 17 00:00:00 2001
From: Brandon Philips <brandon@ifup.org>
Date: Fri, 17 Dec 2010 07:58:22 -0800
Subject: [PATCH] bttv: remove unneeded locking comments

After Mauro's "bttv: Fix locking issues due to BKL removal code" there
are a number of comments that are no longer needed about lock ordering.
Remove them.

Signed-off-by: Brandon Philips <bphilips@suse.de>
---
 drivers/media/video/bt8xx/bttv-driver.c |   20 --------------------
 1 files changed, 0 insertions(+), 20 deletions(-)

diff --git a/drivers/media/video/bt8xx/bttv-driver.c b/drivers/media/video/bt8xx/bttv-driver.c
index 25e1ca0..0902ec0 100644
--- a/drivers/media/video/bt8xx/bttv-driver.c
+++ b/drivers/media/video/bt8xx/bttv-driver.c
@@ -2358,13 +2358,6 @@ static int setup_window_lock(struct bttv_fh *fh, struct bttv *btv,
 	fh->ov.field    = win->field;
 	fh->ov.setup_ok = 1;
 
-	/*
-	 * FIXME: btv is protected by btv->lock mutex, while btv->init
-	 *	  is protected by fh->cap.vb_lock. This seems to open the
-	 *	  possibility for some race situations. Maybe the better would
-	 *	  be to unify those locks or to use another way to store the
-	 *	  init values that will be consumed by videobuf callbacks
-	 */
 	btv->init.ov.w.width   = win->w.width;
 	btv->init.ov.w.height  = win->w.height;
 	btv->init.ov.field     = win->field;
@@ -3219,15 +3212,6 @@ static int bttv_open(struct file *file)
 		return -ENOMEM;
 	file->private_data = fh;
 
-	/*
-	 * btv is protected by btv->lock mutex, while btv->init and other
-	 * streaming vars are protected by fh->cap.vb_lock. We need to take
-	 * care of both locks to avoid troubles. However, vb_lock is used also
-	 * inside videobuf, without calling buf->lock. So, it is a very bad
-	 * idea to hold both locks at the same time.
-	 * Let's first copy btv->init at fh, holding cap.vb_lock, and then work
-	 * with the rest of init, holding btv->lock.
-	 */
 	*fh = btv->init;
 
 	fh->type = type;
@@ -3302,10 +3286,6 @@ static int bttv_release(struct file *file)
 
 	/* free stuff */
 
-	/*
-	 * videobuf uses cap.vb_lock - we should avoid holding btv->lock,
-	 * otherwise we may have dead lock conditions
-	 */
 	videobuf_mmap_free(&fh->cap);
 	videobuf_mmap_free(&fh->vbi);
 	v4l2_prio_close(&btv->prio, fh->prio);
-- 
1.7.3.1

Re: [PATCH] bttv: fix mutex use before init

[Mauro Carvalho Chehab]

Em 17-12-2010 14:07, Brandon Philips escreveu:
> On 19:45 Wed 15 Dec 2010, Mauro Carvalho Chehab wrote:
>> Em 15-12-2010 16:44, Chris Clayton escreveu:
>>> On Tuesday 14 December 2010, Brandon Philips wrote:
>>>> On 17:13 Sun 12 Dec 2010, Torsten Kaiser wrote:
>>>>>  * change &fh->cap.vb_lock in bttv_open() AND radio_open() to
>>>>>  &btv->init.cap.vb_lock * add a mutex_init(&btv->init.cap.vb_lock)
>>>>>  to the setup of init in bttv_probe()
>>>>
>>>> That seems like a reasonable suggestion. An openSUSE user submitted
>>>> this bug to our tracker too. Here is the patch I am having him
>>>> test.
>>>>
>>>> Would you mind testing it?
>>>>
>>>> From 456dc0ce36db523c4c0c8a269f4eec43a72de1dc Mon Sep 17 00:00:00
>>>> 2001 From: Brandon Philips <bphilips@suse.de> Date: Mon, 13 Dec
>>>> 2010 16:21:55 -0800 Subject: [PATCH] bttv: fix locking for
>>>> btv->init
>>>>
>>>> Fix locking for the btv->init by using btv->init.cap.vb_lock and in
>>>> the process fix uninitialized deref introduced in c37db91fd0d.
>>>>
>>>> Signed-off-by: Brandon Philips <bphilips@suse.de>
>>
>> While your patch fixes the issue, it has some other troubles, like to
>> the presence of lock code at free_btres_lock(). It is possible to fix,
>> but the better is to just use the core-assisted locking schema. This
>> way, V4L2 core will serialize access to all
>> ioctl's/open/close/mmap/read/poll operations, avoiding to have two
>> processes accessing the hardware at the same time. Also, as there's
>> just one lock, instead of 3, there's no risk of dead locks.
> 
> Thanks, but, why wasn't this done instead of c37db91f?

Because c37db91f were a first attempt. I was expecting that it would be enough,
but, after some discussions at the ML, it seemed to be better to use a different
approach. As you probably noticed, the locking schema at bttv driver were very
complex, and it were likely to ask for problems. The new schema is simpler. The
principle is to serialize the access to the hardware or to the hardware-mirrored
data. It does that by serializing the access to all file operations for a given
bttv device.

While this is a little overkill (as there are a very few set of operations that
won't require locking, like retrieving some static read-only data), there's no
performance impact at the critical path, as such ioctl's typically happen only
during hardware discovery phase at the userspace apps.

> 
> Will this make it in before 2.6.37 is released? Otherwise 2.6.37 will
> need to be fixed in -stable immediatly after release.

I'll intend to add it today for -next, and send upstream by Sunday, hopefully
in time for .37.

> 
>> The net result is a cleaner code, with just one lock.
> 
> Could you take this patch to remove all of the comments about locking
> order with btv->lock since it doesn't seem to matter any longer.

Yes, sure. Thanks for the patch.
> 
> Cheers,
> 
> 	Brandon
> 
> P.S. Your mail client creates really long lines- somewhere around 90
> characters. Could you fix that?
> 
> From 7643db7bf5e9e557a27e3783786a1abecbdf82a7 Mon Sep 17 00:00:00 2001
> From: Brandon Philips <brandon@ifup.org>
> Date: Fri, 17 Dec 2010 07:58:22 -0800
> Subject: [PATCH] bttv: remove unneeded locking comments
> 
> After Mauro's "bttv: Fix locking issues due to BKL removal code" there
> are a number of comments that are no longer needed about lock ordering.
> Remove them.
> 
> Signed-off-by: Brandon Philips <bphilips@suse.de>
> ---
>  drivers/media/video/bt8xx/bttv-driver.c |   20 --------------------
>  1 files changed, 0 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/media/video/bt8xx/bttv-driver.c b/drivers/media/video/bt8xx/bttv-driver.c
> index 25e1ca0..0902ec0 100644
> --- a/drivers/media/video/bt8xx/bttv-driver.c
> +++ b/drivers/media/video/bt8xx/bttv-driver.c
> @@ -2358,13 +2358,6 @@ static int setup_window_lock(struct bttv_fh *fh, struct bttv *btv,
>  	fh->ov.field    = win->field;
>  	fh->ov.setup_ok = 1;
>  
> -	/*
> -	 * FIXME: btv is protected by btv->lock mutex, while btv->init
> -	 *	  is protected by fh->cap.vb_lock. This seems to open the
> -	 *	  possibility for some race situations. Maybe the better would
> -	 *	  be to unify those locks or to use another way to store the
> -	 *	  init values that will be consumed by videobuf callbacks
> -	 */
>  	btv->init.ov.w.width   = win->w.width;
>  	btv->init.ov.w.height  = win->w.height;
>  	btv->init.ov.field     = win->field;
> @@ -3219,15 +3212,6 @@ static int bttv_open(struct file *file)
>  		return -ENOMEM;
>  	file->private_data = fh;
>  
> -	/*
> -	 * btv is protected by btv->lock mutex, while btv->init and other
> -	 * streaming vars are protected by fh->cap.vb_lock. We need to take
> -	 * care of both locks to avoid troubles. However, vb_lock is used also
> -	 * inside videobuf, without calling buf->lock. So, it is a very bad
> -	 * idea to hold both locks at the same time.
> -	 * Let's first copy btv->init at fh, holding cap.vb_lock, and then work
> -	 * with the rest of init, holding btv->lock.
> -	 */
>  	*fh = btv->init;
>  
>  	fh->type = type;
> @@ -3302,10 +3286,6 @@ static int bttv_release(struct file *file)
>  
>  	/* free stuff */
>  
> -	/*
> -	 * videobuf uses cap.vb_lock - we should avoid holding btv->lock,
> -	 * otherwise we may have dead lock conditions
> -	 */
>  	videobuf_mmap_free(&fh->cap);
>  	videobuf_mmap_free(&fh->vbi);
>  	v4l2_prio_close(&btv->prio, fh->prio);