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 <[email protected]>
Cc: Mauro Carvalho Chehab <[email protected]>
Cc: [email protected]
---
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
On Sun, Jun 08, 2008 at 12:48:35AM +0200, Marcin Slusarz wrote:
> (...)
This patch is stupid. Please ignore.
Marcin
On Sun, 8 Jun 2008 00:48:40 +0200
Marcin Slusarz <[email protected]> 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 <[email protected]>
> Cc: Mauro Carvalho Chehab <[email protected]>
> Cc: [email protected]
> ---
>
> 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
On Sun, 8 Jun 2008 08:31:07 +0200
Marcin Slusarz <[email protected]> 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
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 <[email protected]> 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
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 <[email protected]>
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 <[email protected]>
Cc: Mauro Carvalho Chehab <[email protected]>
Cc: [email protected]
---
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