[RFC PATCH] v4l: saa7134: fix multiple clients access (and oops)

[RFC PATCH] v4l: saa7134: fix multiple clients access (and oops)

[Marcin Slusarz]

While looking for a reason of multiple oopses in empress_querycap as reported
by kerneloops.org I noticed that only first open of device initializes
struct_file->private_data properly. (Closing the device was broken too).

So initialize private_date and free all resources on last close.
I think this change will fix oops in empress_querycap.

http://kerneloops.org/guilty.php?guilty=empress_querycap&version=2.6.25-release&start=1671168&end=1703935&class=oops

Signed-off-by: Marcin Slusarz <marcin.slusarz@gmail.com>
Cc: Mauro Carvalho Chehab <mchehab@infradead.org>
Cc: video4linux-list@redhat.com
---

Compile tested only. Please test on real hardware.

---
 drivers/media/video/saa7134/saa7134-empress.c |   23 +++++++++++++----------
 1 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/media/video/saa7134/saa7134-empress.c b/drivers/media/video/saa7134/saa7134-empress.c
index 81431ee..e543074 100644
--- a/drivers/media/video/saa7134/saa7134-empress.c
+++ b/drivers/media/video/saa7134/saa7134-empress.c
@@ -96,11 +96,11 @@ static int ts_open(struct inode *inode, struct file *file)
 	saa_writeb(SAA7134_AUDIO_MUTE_CTRL,
 		saa_readb(SAA7134_AUDIO_MUTE_CTRL) & ~(1 << 6));
 
-	dev->empress_users++;
-	file->private_data = dev;
 	err = 0;
 
 done_up:
+	dev->empress_users++;
+	file->private_data = dev;
 	mutex_unlock(&dev->empress_tsq.vb_lock);
 done:
 	return err;
@@ -110,16 +110,19 @@ static int ts_release(struct inode *inode, struct file *file)
 {
 	struct saa7134_dev *dev = file->private_data;
 
-	videobuf_stop(&dev->empress_tsq);
-	videobuf_mmap_free(&dev->empress_tsq);
-	dev->empress_users--;
+	mutex_lock(&dev->empress_tsq.vb_lock);
+	if (--dev->empress_users == 0) {
+		videobuf_stop(&dev->empress_tsq);
+		videobuf_mmap_free(&dev->empress_tsq);
 
-	/* stop the encoder */
-	ts_reset_encoder(dev);
+		/* stop the encoder */
+		ts_reset_encoder(dev);
 
-	/* Mute audio */
-	saa_writeb(SAA7134_AUDIO_MUTE_CTRL,
-		saa_readb(SAA7134_AUDIO_MUTE_CTRL) | (1 << 6));
+		/* Mute audio */
+		saa_writeb(SAA7134_AUDIO_MUTE_CTRL,
+			saa_readb(SAA7134_AUDIO_MUTE_CTRL) | (1 << 6));
+	}
+	mutex_unlock(&dev->empress_tsq.vb_lock);
 
 	return 0;
 }
-- 
1.5.4.5

Re: [RFC PATCH] v4l: saa7134: fix multiple clients access (and oops)

[Marcin Slusarz]

On Sun, Jun 08, 2008 at 12:48:35AM +0200, Marcin Slusarz wrote:
> (...)

This patch is stupid. Please ignore.

Marcin

Re: [RFC PATCH] v4l: saa7134: fix multiple clients access (and oops)

[Mauro Carvalho Chehab]

On Sun, 8 Jun 2008 00:48:40 +0200
Marcin Slusarz <marcin.slusarz@gmail.com> wrote:

> While looking for a reason of multiple oopses in empress_querycap as reported
> by kerneloops.org I noticed that only first open of device initializes
> struct_file->private_data properly. (Closing the device was broken too).
> 
> So initialize private_date and free all resources on last close.
> I think this change will fix oops in empress_querycap.
> 
> http://kerneloops.org/guilty.php?guilty=empress_querycap&version=2.6.25-release&start=1671168&end=1703935&class=oops
> 
> Signed-off-by: Marcin Slusarz <marcin.slusarz@gmail.com>
> Cc: Mauro Carvalho Chehab <mchehab@infradead.org>
> Cc: video4linux-list@redhat.com
> ---
> 
> Compile tested only. Please test on real hardware.

Thanks.

The patch looks sane to my eyes. I'll apply at the tree, in order to allow more
people to test it.


Cheers,
Mauro

Re: [RFC PATCH] v4l: saa7134: fix multiple clients access (and oops)

[Mauro Carvalho Chehab]

On Sun, 8 Jun 2008 08:31:07 +0200
Marcin Slusarz <marcin.slusarz@gmail.com> wrote:

> On Sun, Jun 08, 2008 at 12:48:35AM +0200, Marcin Slusarz wrote:
> > (...)
> 
> This patch is stupid. Please ignore.

Patch ignored. Yet, it seemed ok to my eyes. We shouldn't stop/deallocate
resources if there is still someone using the module.

Cheers,
Mauro

Re: [RFC PATCH] v4l: saa7134: fix multiple clients access (and oops)

[Marcin Slusarz]

On Sun, Jun 08, 2008 at 07:32:55AM -0300, Mauro Carvalho Chehab wrote:
> On Sun, 8 Jun 2008 08:31:07 +0200
> Marcin Slusarz <marcin.slusarz@gmail.com> wrote:
> 
> > On Sun, Jun 08, 2008 at 12:48:35AM +0200, Marcin Slusarz wrote:
> > > (...)
> > 
> > This patch is stupid. Please ignore.
> 
> Patch ignored. Yet, it seemed ok to my eyes. We shouldn't stop/deallocate
> resources if there is still someone using the module.

But when there's at least one user, dev->empress_users will be bigger than 0,
so second call to ts_open will return -EBUSY.
And it lead to conclusion, that there's only one possible user...

Marcin

[PATCH] v4l: saa7134: fix race between opening and closing the device

[Marcin Slusarz]

On Sun, Jun 08, 2008 at 08:31:04AM +0200, Marcin Slusarz wrote:
> On Sun, Jun 08, 2008 at 12:48:35AM +0200, Marcin Slusarz wrote:
> > (...)
> 
> This patch is stupid. Please ignore.

Ok, I think this one should be considered for inclusion.
I don't think it has anything to do with oops in empress_querycap,
but who knows ;)
Compile tested only.
---
From: Marcin Slusarz <marcin.slusarz@gmail.com>
Subject: [PATCH] v4l: saa7134: fix race between opening and closing the device

decrementing dev->empress_users should be done as last action of ts_release,
because it sleeps and write access to dev->empress_started is not protected
in any way
(additionally closing thread could mute audio after opening thread unmuted it)

Signed-off-by: Marcin Slusarz <marcin.slusarz@gmail.com>
Cc: Mauro Carvalho Chehab <mchehab@infradead.org>
Cc: video4linux-list@redhat.com
---
 drivers/media/video/saa7134/saa7134-empress.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/media/video/saa7134/saa7134-empress.c b/drivers/media/video/saa7134/saa7134-empress.c
index 81431ee..0da683a 100644
--- a/drivers/media/video/saa7134/saa7134-empress.c
+++ b/drivers/media/video/saa7134/saa7134-empress.c
@@ -112,7 +112,6 @@ static int ts_release(struct inode *inode, struct file *file)
 
 	videobuf_stop(&dev->empress_tsq);
 	videobuf_mmap_free(&dev->empress_tsq);
-	dev->empress_users--;
 
 	/* stop the encoder */
 	ts_reset_encoder(dev);
@@ -121,6 +120,8 @@ static int ts_release(struct inode *inode, struct file *file)
 	saa_writeb(SAA7134_AUDIO_MUTE_CTRL,
 		saa_readb(SAA7134_AUDIO_MUTE_CTRL) | (1 << 6));
 
+	dev->empress_users--;
+
 	return 0;
 }
 
-- 
1.5.4.5