bttv: fix mutex use before init

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

Signed-off-by Dave Young <[email protected]>

Tested-by Chris Clayton <[email protected]>

---
 drivers/media/video/bt8xx/bttv-driver.c |    2 ++

 1 file changed, 2 insertions(+)

drivers/media/video/bt8xx/bttv-driver.c   2010-12-12 16:31:39.633333338 +0800 renamed from drivers/media/video/bt8xx/bttv-driver.c      2010-11-27 11:21:30.000000000 +0800 +2 -1

@@ -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 [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Quoted 2 lines · Dave Young

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:

(quoted diff) +2 -0

+	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()

Quoted 24 lines

Signed-off-by Dave Young <[email protected]>

Tested-by Chris Clayton <[email protected]>

---
 drivers/media/video/bt8xx/bttv-driver.c |    2 ++

 1 file changed, 2 insertions(+)

drivers/media/video/bt8xx/bttv-driver.c   2010-12-12 16:31:39.633333338 +0800 renamed from drivers/media/video/bt8xx/bttv-driver.c      2010-11-27 11:21:30.000000000 +0800 +2 -1

@@ -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 [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Quoted 19 lines · Torsten Kaiser

Quoted 2 lines · Dave Young

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!

Quoted 3 lines

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.

Quoted 5 lines

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.

Quoted 23 lines

Quoted 23 lines

Signed-off-by Dave Young <[email protected]>

Tested-by Chris Clayton <[email protected]>

---
 drivers/media/video/bt8xx/bttv-driver.c |    2 ++

 1 file changed, 2 insertions(+)

drivers/media/video/bt8xx/bttv-driver.c   2010-12-12 16:31:39.633333338 +0800 renamed from drivers/media/video/bt8xx/bttv-driver.c      2010-11-27 11:21:30.000000000 +0800 +2 -1

@@ -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 [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

* 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()

From 456dc0ce36db523c4c0c8a269f4eec43a72de1dc Mon Sep 17 00:00:00 2001

Quoted 3 lines · Torsten Kaiser

* 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 <[email protected]>
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 <[email protected]>

---
 drivers/media/video/bt8xx/bttv-driver.c |   24 +++++++++++++-----------
 1 files changed, 13 insertions(+), 11 deletions(-)

drivers/media/video/bt8xx/bttv-driver.c +13 -11

index a529619..e656424 100644
@@ -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;

Signature · 1 line

1.7.3.1

Quoted 3 lines · Torsten Kaiser

 * 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 <[email protected]>
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 <[email protected]>

---
 drivers/media/video/bt8xx/bttv-driver.c |   24 +++++++++++++-----------

 1 files changed, 13 insertions(+), 11 deletions(-)

drivers/media/video/bt8xx/bttv-driver.c +13 -12

index a529619..e656424 100644
@@ -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

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

Quoted 18 lines · Torsten Kaiser

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

Quoted 8 lines

[  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?

Quoted 8 lines · Brandon Philips

Quoted 3 lines · Torsten Kaiser

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

Quoted 1 line

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.

Quoted 13 lines · Torsten Kaiser

Quoted 9 lines · Brandon Philips

Quoted 3 lines · Torsten Kaiser

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

Quoted 4 lines

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

Quoted 13 lines · Torsten Kaiser

Quoted 9 lines · Brandon Philips

Quoted 3 lines · Torsten Kaiser

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

Quoted 4 lines

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?

Quoted 4 lines · Torsten Kaiser

* 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 <[email protected]>
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 <[email protected]>

---
 drivers/media/video/bt8xx/bttv-driver.c |   24 +++++++++++++-----------
 1 files changed, 13 insertions(+), 11 deletions(-)

(quoted diff) +0 -0

b/drivers/media/video/bt8xx/bttv-driver.c index a529619..e656424 100644

drivers/media/video/bt8xx/bttv-driver.c +12 -11

@@ -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);

Quoted 16 lines · Torsten Kaiser

Quoted 4 lines

* 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 <[email protected]>
Date: Mon, 13 Dec 2010 16:21:55 -0800
Subject: [PATCH] bttv: fix locking for btv->init

Quoted 4 lines

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 <[email protected]>

Em 15-12-2010 16:44, Chris Clayton escreveu:

Quoted 17 lines · Brandon Philips

Quoted 16 lines · Torsten Kaiser

Quoted 4 lines

* 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 <[email protected]>
Date: Mon, 13 Dec 2010 16:21:55 -0800
Subject: [PATCH] bttv: fix locking for btv->init

Quoted 4 lines

Quoted 4 lines

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 <[email protected]>

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 <[email protected]>

(quoted diff) +0 -0

b/drivers/media/video/bt8xx/bttv-driver.c index a529619..25e1ca0 100644

drivers/media/video/bt8xx/bttv-driver.c +0 -1

@@ -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 */

drivers/media/video/bt8xx/bttv-driver.c +0 -5

 	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,

drivers/media/video/bt8xx/bttv-driver.c +0 -15

 		/* 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))

drivers/media/video/bt8xx/bttv-driver.c +0 -11

 		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
 

drivers/media/video/bt8xx/bttv-driver.c +0 -4

 			: 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
 

drivers/media/video/bt8xx/bttv-driver.c +0 -14

 			: 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;

}

drivers/media/video/bt8xx/bttv-driver.c +0 -10

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

drivers/media/video/bt8xx/bttv-driver.c +0 -2

 	   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

drivers/media/video/bt8xx/bttv-driver.c +0 -8

 	   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;

drivers/media/video/bt8xx/bttv-driver.c +0 -3

 		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

drivers/media/video/bt8xx/bttv-driver.c +2 -20

 		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;
 }
 

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 <[email protected]>

drivers/media/video/bt8xx/bttv-driver.c +0 -2

index a529619..25e1ca0 100644
@@ -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;
 }

drivers/media/video/bt8xx/bttv-driver.c +0 -3

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

drivers/media/video/bt8xx/bttv-driver.c +0 -1

@@ -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;
 }

drivers/media/video/bt8xx/bttv-driver.c +0 -1

@@ -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;
 }

drivers/media/video/bt8xx/bttv-driver.c +0 -3

@@ -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;
 }

drivers/media/video/bt8xx/bttv-driver.c +0 -1

@@ -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;
 }

drivers/media/video/bt8xx/bttv-driver.c +0 -3

@@ -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;
 }

drivers/media/video/bt8xx/bttv-driver.c +0 -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;

drivers/media/video/bt8xx/bttv-driver.c +0 -0

@@ -2250,7 +2221,6 @@ limit_scaled_size_lock       (struct bttv_fh *               fh,

       rc = 0; /* success */

 fail:
-       mutex_unlock(&btv->lock);

       return rc;
 }

drivers/media/video/bt8xx/bttv-driver.c +0 -2

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

drivers/media/video/bt8xx/bttv-driver.c +0 -4

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

drivers/media/video/bt8xx/bttv-driver.c +0 -24

@@ -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;
 }

drivers/media/video/bt8xx/bttv-driver.c +0 -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;
       }

drivers/media/video/bt8xx/bttv-driver.c +0 -8

@@ -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;
 }

drivers/media/video/bt8xx/bttv-driver.c +3 -20

@@ -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,
  };
 

Em 15-12-2010 16:44, Chris Clayton escreveu:

Quoted 17 lines · Brandon Philips

Quoted 16 lines · Torsten Kaiser

Quoted 3 lines

* 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 <[email protected]> Date: Mon, 13 Dec 2010 16:21:55 -0800 Subject: [PATCH] bttv: fix locking for btv->init

Quoted 4 lines

Quoted 4 lines

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 <[email protected]>

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.

From 7643db7bf5e9e557a27e3783786a1abecbdf82a7 Mon Sep 17 00:00:00 2001

Quoted 23 lines

Em 15-12-2010 16:44, Chris Clayton escreveu:

Quoted 17 lines · Brandon Philips

Quoted 16 lines · Torsten Kaiser

Quoted 3 lines

* 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 <[email protected]> Date: Mon, 13 Dec 2010 16:21:55 -0800 Subject: [PATCH] bttv: fix locking for btv->init

Quoted 4 lines

Quoted 4 lines

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 <[email protected]>

Quoted 7 lines

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.

Quoted 2 lines

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 <[email protected]>
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 <[email protected]>

---
 drivers/media/video/bt8xx/bttv-driver.c |   20 --------------------
 1 files changed, 0 insertions(+), 20 deletions(-)

drivers/media/video/bt8xx/bttv-driver.c +0 -20

index 25e1ca0..0902ec0 100644
@@ -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);