Re: [linux-dvb] cx88-blackbird broken (since 2.6.37)

Re: [linux-dvb] cx88-blackbird broken (since 2.6.37)

[Jonathan Nieder]

Hi Andi,

Huber Andreas wrote[1]:

> [Symptom]
> Processes that try to open a cx88-blackbird driven MPEG device will hang up.

Thanks for reporting.  Just cc-ing some relevant people.  Could you file a
bug to track this at <http://bugzilla.kernel.org/>, product v4l-dvb,
component cx88, and then send the bug number to 619827@bugs.debian.org ?

Report follows.

Jonathan

[1] http://bugs.debian.org/619827

> [Cause]
> Nestet mutex_locks (which are not allowed) result in a deadlock.
> 
> [Details]
> Source-File: drivers/media/video/cx88/cx88-blackbird.c
> Function: int mpeg_open(struct file *file)
> Problem: the calls to  drv->request_acquire(drv); and
> drv->request_release(drv); will hang because they try to lock a
> mutex that has already been locked by a previouse call to
> mutex_lock(&dev->core->lock) ...
> 
> 1050 static int mpeg_open(struct file *file)
> 1051 {
> [...]
> 1060         mutex_lock(&dev->core->lock);         // MUTEX LOCKED !!!!!!!!!!!!!!!!
> 1061
> 1062         /* Make sure we can acquire the hardware */
> 1063         drv = cx8802_get_driver(dev, CX88_MPEG_BLACKBIRD);
> 1064         if (drv) {
> 1065                 err = drv->request_acquire(drv);  // HANGS !!!!!!!!!!!!!!!!!!!
> 1066                 if(err != 0) {
> 1067                         dprintk(1,"%s: Unable to acquire hardware, %d\n", __func__, err);
> 1068                         mutex_unlock(&dev->core->lock);;
> 1069                         return err;
> 1070                 }
> 1071         }
> [...]
> 
> Here's the relevant kernel log extract (Linux version 2.6.38-1-amd64 (Debian 2.6.38-1)) ...
> 
> Mar 24 21:25:10 xen kernel: [  241.472067] INFO: task v4l_id:1000 blocked for more than 120 seconds.
> Mar 24 21:25:10 xen kernel: [  241.478845] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
> Mar 24 21:25:10 xen kernel: [  241.482412] v4l_id          D ffff88006bcb6540     0  1000      1 0x00000000
> Mar 24 21:25:10 xen kernel: [  241.486031]  ffff88006bcb6540 0000000000000086 ffff880000000001 ffff88006981c380
> Mar 24 21:25:10 xen kernel: [  241.489694]  0000000000013700 ffff88006be5bfd8 ffff88006be5bfd8 0000000000013700
> Mar 24 21:25:10 xen kernel: [  241.493301]  ffff88006bcb6540 ffff88006be5a010 ffff88006bcb6540 000000016be5a000
> Mar 24 21:25:10 xen kernel: [  241.496766] Call Trace:
> Mar 24 21:25:10 xen kernel: [  241.500145]  [<ffffffff81321c4a>] ? __mutex_lock_common+0x127/0x193
> Mar 24 21:25:10 xen kernel: [  241.503630]  [<ffffffff81321d82>] ? mutex_lock+0x1a/0x33
> Mar 24 21:25:10 xen kernel: [  241.507145]  [<ffffffffa09dd155>] ? cx8802_request_acquire+0x66/0xc6 [cx8802]
> Mar 24 21:25:10 xen kernel: [  241.510699]  [<ffffffffa0aab7f2>] ? mpeg_open+0x7a/0x1fc [cx88_blackbird]
> Mar 24 21:25:10 xen kernel: [  241.514279]  [<ffffffff8123bfb6>] ? kobj_lookup+0x139/0x173
> Mar 24 21:25:10 xen kernel: [  241.517856]  [<ffffffffa062d5fd>] ? v4l2_open+0xb3/0xdf [videodev]

[RFC/PATCH 0/3] locking fixes for cx88

[Jonathan Nieder]

Hi,

Huber Andreas wrote[1]:

> Processes that try to open a cx88-blackbird driven MPEG device will hang up.

Here's a possible fix based on a patch by Ben Hutchings and
corrections from Andi Huber.  Warning: probably full of mistakes (my
fault) since I'm not familiar with any of this stuff.  Untested.
Review and testing would be welcome.

Ben Hutchings (2):
  [media] cx88: fix locking of sub-driver operations
  [media] cx88: use a mutex to protect cx8802_devlist

Jonathan Nieder (1):
  [media] cx88: protect per-device driver list with device lock

 drivers/media/video/cx88/cx88-blackbird.c |    3 +-
 drivers/media/video/cx88/cx88-dvb.c       |    2 +
 drivers/media/video/cx88/cx88-mpeg.c      |   35 +++++++++++++++++++---------
 drivers/media/video/cx88/cx88.h           |   10 +++++++-
 4 files changed, 37 insertions(+), 13 deletions(-)

[PATCH 1/3] [media] cx88: protect per-device driver list with device lock

[Jonathan Nieder]

The BKL conversion of this family of drivers seems to have gone wrong.
Opening cx88-blackbird will deadlock.  Various other uses of the
sub-device and driver lists appear to be subject to race conditions.

For example: various functions access drvlist without a relevant
lock held, which will race against removal of drivers.  Let's
start with that --- clean up by consistently protecting dev->drvlist
with dev->core->lock, noting driver functions that require the device
lock to be held or not held.

The only goal is to make the semantics clearer in preparation for
other changes.  There are still some relevant races (noted in
comments) and the deadlock noticed by Andi remains; later patches
will address that.

Based-on-patch-by: Ben Hutchings <ben@decadent.org.uk>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Cc: Andi Huber <hobrom@gmx.at>
Cc: stable@kernel.org
---
 drivers/media/video/cx88/cx88-blackbird.c |    8 +++++++-
 drivers/media/video/cx88/cx88-dvb.c       |    8 ++++++++
 drivers/media/video/cx88/cx88-mpeg.c      |   11 +++++++----
 drivers/media/video/cx88/cx88.h           |    9 ++++++++-
 4 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/drivers/media/video/cx88/cx88-blackbird.c b/drivers/media/video/cx88/cx88-blackbird.c
index bca307e..85910c6 100644
--- a/drivers/media/video/cx88/cx88-blackbird.c
+++ b/drivers/media/video/cx88/cx88-blackbird.c
@@ -1122,10 +1122,16 @@ static int mpeg_release(struct file *file)
 	mutex_lock(&dev->core->lock);
 	file->private_data = NULL;
 	kfree(fh);
-	mutex_unlock(&dev->core->lock);
 
 	/* Make sure we release the hardware */
 	drv = cx8802_get_driver(dev, CX88_MPEG_BLACKBIRD);
+	mutex_unlock(&dev->core->lock);
+
+	/*
+	 * NEEDSWORK: the driver can be yanked from under our feet.
+	 * The following really ought to be protected with core->lock.
+	 */
+
 	if (drv)
 		drv->request_release(drv);
 
diff --git a/drivers/media/video/cx88/cx88-dvb.c b/drivers/media/video/cx88/cx88-dvb.c
index 7b8c9d3..5d0f947 100644
--- a/drivers/media/video/cx88/cx88-dvb.c
+++ b/drivers/media/video/cx88/cx88-dvb.c
@@ -133,7 +133,15 @@ static int cx88_dvb_bus_ctrl(struct dvb_frontend* fe, int acquire)
 		return -EINVAL;
 	}
 
+	mutex_lock(&dev->core->lock);
 	drv = cx8802_get_driver(dev, CX88_MPEG_DVB);
+	mutex_unlock(&dev->core->lock);
+
+	/*
+	 * NEEDSWORK: The driver can be yanked from under our feet now.
+	 * We ought to keep holding core->lock during the below.
+	 */
+
 	if (drv) {
 		if (acquire){
 			dev->frontends.active_fe_id = fe_id;
diff --git a/drivers/media/video/cx88/cx88-mpeg.c b/drivers/media/video/cx88/cx88-mpeg.c
index addf954..918172b 100644
--- a/drivers/media/video/cx88/cx88-mpeg.c
+++ b/drivers/media/video/cx88/cx88-mpeg.c
@@ -748,6 +748,8 @@ int cx8802_unregister_driver(struct cx8802_driver *drv)
 		       dev->pci->subsystem_device, dev->core->board.name,
 		       dev->core->boardnr);
 
+		mutex_lock(&dev->core->lock);
+
 		list_for_each_entry_safe(d, dtmp, &dev->drvlist, drvlist) {
 			/* only unregister the correct driver type */
 			if (d->type_id != drv->type_id)
@@ -755,15 +757,14 @@ int cx8802_unregister_driver(struct cx8802_driver *drv)
 
 			err = d->remove(d);
 			if (err == 0) {
-				mutex_lock(&drv->core->lock);
 				list_del(&d->drvlist);
-				mutex_unlock(&drv->core->lock);
 				kfree(d);
 			} else
 				printk(KERN_ERR "%s/2: cx8802 driver remove "
 				       "failed (%d)\n", dev->core->name, err);
 		}
 
+		mutex_unlock(&dev->core->lock);
 	}
 
 	return err;
@@ -827,6 +828,8 @@ static void __devexit cx8802_remove(struct pci_dev *pci_dev)
 
 	flush_request_modules(dev);
 
+	mutex_lock(&dev->core->lock);
+
 	if (!list_empty(&dev->drvlist)) {
 		struct cx8802_driver *drv, *tmp;
 		int err;
@@ -838,9 +841,7 @@ static void __devexit cx8802_remove(struct pci_dev *pci_dev)
 		list_for_each_entry_safe(drv, tmp, &dev->drvlist, drvlist) {
 			err = drv->remove(drv);
 			if (err == 0) {
-				mutex_lock(&drv->core->lock);
 				list_del(&drv->drvlist);
-				mutex_unlock(&drv->core->lock);
 			} else
 				printk(KERN_ERR "%s/2: cx8802 driver remove "
 				       "failed (%d)\n", dev->core->name, err);
@@ -848,6 +849,8 @@ static void __devexit cx8802_remove(struct pci_dev *pci_dev)
 		}
 	}
 
+	mutex_unlock(&dev->core->lock);
+
 	/* Destroy any 8802 reference. */
 	dev->core->dvbdev = NULL;
 
diff --git a/drivers/media/video/cx88/cx88.h b/drivers/media/video/cx88/cx88.h
index 9b3742a..e3d56c2 100644
--- a/drivers/media/video/cx88/cx88.h
+++ b/drivers/media/video/cx88/cx88.h
@@ -506,7 +506,11 @@ struct cx8802_driver {
 	int (*resume)(struct pci_dev *pci_dev);
 
 	/* MPEG 8802 -> mini driver - Driver probe and configuration */
+
+	/* Caller must _not_ hold core->lock */
 	int (*probe)(struct cx8802_driver *drv);
+
+	/* Caller must hold core->lock */
 	int (*remove)(struct cx8802_driver *drv);
 
 	/* MPEG 8802 -> mini driver - Access for hardware control */
@@ -561,8 +565,9 @@ struct cx8802_dev {
 	/* for switching modulation types */
 	unsigned char              ts_gen_cntrl;
 
-	/* List of attached drivers */
+	/* List of attached drivers; must hold core->lock to access */
 	struct list_head	   drvlist;
+
 	struct work_struct	   request_module_wk;
 };
 
@@ -685,6 +690,8 @@ int cx88_audio_thread(void *data);
 
 int cx8802_register_driver(struct cx8802_driver *drv);
 int cx8802_unregister_driver(struct cx8802_driver *drv);
+
+/* Caller must hold core->lock */
 struct cx8802_driver * cx8802_get_driver(struct cx8802_dev *dev, enum cx88_board_type btype);
 
 /* ----------------------------------------------------------- */
-- 
1.7.5.rc0

[PATCH 2/3] [media] cx88: fix locking of sub-driver operations

[Jonathan Nieder]

From: Ben Hutchings <ben@decadent.org.uk>
Date: Tue, 29 Mar 2011 03:25:15 +0100

The BKL conversion of this family of drivers seems to have gone wrong.
Opening cx88-blackbird will deadlock.  Various other uses of the
sub-device and driver lists appear to be subject to race conditions.

In particular, mpeg_ops::open in the cx2388x blackbird driver acquires
the device lock and then calls the drivers' request_acquire, which
tries to acquire the lock again --- deadlock.  Fix it by clarifying
the semantics of request_acquire, request_release, advise_acquire, and
advise_release: all require the caller to hold the device lock now.

[jn: split from a larger patch, with new commit message]

Reported-by: Andi Huber <hobrom@gmx.at>
Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=31962
Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Cc: stable@kernel.org
---
 drivers/media/video/cx88/cx88-blackbird.c |    9 ++-------
 drivers/media/video/cx88/cx88-dvb.c       |    8 +-------
 drivers/media/video/cx88/cx88-mpeg.c      |    4 ----
 drivers/media/video/cx88/cx88.h           |    3 ++-
 4 files changed, 5 insertions(+), 19 deletions(-)

diff --git a/drivers/media/video/cx88/cx88-blackbird.c b/drivers/media/video/cx88/cx88-blackbird.c
index 85910c6..a6f7d53 100644
--- a/drivers/media/video/cx88/cx88-blackbird.c
+++ b/drivers/media/video/cx88/cx88-blackbird.c
@@ -1125,18 +1125,13 @@ static int mpeg_release(struct file *file)
 
 	/* Make sure we release the hardware */
 	drv = cx8802_get_driver(dev, CX88_MPEG_BLACKBIRD);
-	mutex_unlock(&dev->core->lock);
-
-	/*
-	 * NEEDSWORK: the driver can be yanked from under our feet.
-	 * The following really ought to be protected with core->lock.
-	 */
-
 	if (drv)
 		drv->request_release(drv);
 
 	atomic_dec(&dev->core->mpeg_users);
 
+	mutex_unlock(&dev->core->lock);
+
 	return 0;
 }
 
diff --git a/drivers/media/video/cx88/cx88-dvb.c b/drivers/media/video/cx88/cx88-dvb.c
index 5d0f947..c69df7e 100644
--- a/drivers/media/video/cx88/cx88-dvb.c
+++ b/drivers/media/video/cx88/cx88-dvb.c
@@ -135,13 +135,6 @@ static int cx88_dvb_bus_ctrl(struct dvb_frontend* fe, int acquire)
 
 	mutex_lock(&dev->core->lock);
 	drv = cx8802_get_driver(dev, CX88_MPEG_DVB);
-	mutex_unlock(&dev->core->lock);
-
-	/*
-	 * NEEDSWORK: The driver can be yanked from under our feet now.
-	 * We ought to keep holding core->lock during the below.
-	 */
-
 	if (drv) {
 		if (acquire){
 			dev->frontends.active_fe_id = fe_id;
@@ -151,6 +144,7 @@ static int cx88_dvb_bus_ctrl(struct dvb_frontend* fe, int acquire)
 			dev->frontends.active_fe_id = 0;
 		}
 	}
+	mutex_unlock(&dev->core->lock);
 
 	return ret;
 }
diff --git a/drivers/media/video/cx88/cx88-mpeg.c b/drivers/media/video/cx88/cx88-mpeg.c
index 918172b..9147c16 100644
--- a/drivers/media/video/cx88/cx88-mpeg.c
+++ b/drivers/media/video/cx88/cx88-mpeg.c
@@ -624,13 +624,11 @@ static int cx8802_request_acquire(struct cx8802_driver *drv)
 
 	if (drv->advise_acquire)
 	{
-		mutex_lock(&drv->core->lock);
 		core->active_ref++;
 		if (core->active_type_id == CX88_BOARD_NONE) {
 			core->active_type_id = drv->type_id;
 			drv->advise_acquire(drv);
 		}
-		mutex_unlock(&drv->core->lock);
 
 		mpeg_dbg(1,"%s() Post acquire GPIO=%x\n", __func__, cx_read(MO_GP0_IO));
 	}
@@ -643,14 +641,12 @@ static int cx8802_request_release(struct cx8802_driver *drv)
 {
 	struct cx88_core *core = drv->core;
 
-	mutex_lock(&drv->core->lock);
 	if (drv->advise_release && --core->active_ref == 0)
 	{
 		drv->advise_release(drv);
 		core->active_type_id = CX88_BOARD_NONE;
 		mpeg_dbg(1,"%s() Post release GPIO=%x\n", __func__, cx_read(MO_GP0_IO));
 	}
-	mutex_unlock(&drv->core->lock);
 
 	return 0;
 }
diff --git a/drivers/media/video/cx88/cx88.h b/drivers/media/video/cx88/cx88.h
index e3d56c2..9731daa 100644
--- a/drivers/media/video/cx88/cx88.h
+++ b/drivers/media/video/cx88/cx88.h
@@ -510,7 +510,8 @@ struct cx8802_driver {
 	/* Caller must _not_ hold core->lock */
 	int (*probe)(struct cx8802_driver *drv);
 
-	/* Caller must hold core->lock */
+	/* Callers to the following functions must hold core->lock */
+
 	int (*remove)(struct cx8802_driver *drv);
 
 	/* MPEG 8802 -> mini driver - Access for hardware control */
-- 
1.7.5.rc0

[PATCH 3/3] [media] cx88: use a mutex to protect cx8802_devlist

[Jonathan Nieder]

From: Ben Hutchings <ben@decadent.org.uk>
Date: Tue, 29 Mar 2011 03:25:15 +0100

Add and use a mutex to protect the cx88-mpeg device list.  Previously
the BKL prevented races.

[jn: split from a larger patch, with new commit message; also protect
 use in cx8802_probe]

Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
That's the end of the series.  Hopefully I haven't mangled the patches
too much --- if we're lucky they might even still work, though I
wouldn't bet on it.  Bug reports and improvements welcome.

Good night,
Jonathan

 drivers/media/video/cx88/cx88-mpeg.c |   20 +++++++++++++++++---
 1 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/drivers/media/video/cx88/cx88-mpeg.c b/drivers/media/video/cx88/cx88-mpeg.c
index 9147c16..6b58647 100644
--- a/drivers/media/video/cx88/cx88-mpeg.c
+++ b/drivers/media/video/cx88/cx88-mpeg.c
@@ -78,6 +78,7 @@ static void flush_request_modules(struct cx8802_dev *dev)
 
 
 static LIST_HEAD(cx8802_devlist);
+static DEFINE_MUTEX(cx8802_mutex);
 /* ------------------------------------------------------------------ */
 
 static int cx8802_start_dma(struct cx8802_dev    *dev,
@@ -689,6 +690,8 @@ int cx8802_register_driver(struct cx8802_driver *drv)
 		return err;
 	}
 
+	mutex_lock(&cx8802_mutex);
+
 	list_for_each_entry(dev, &cx8802_devlist, devlist) {
 		printk(KERN_INFO
 		       "%s/2: subsystem: %04x:%04x, board: %s [card=%d]\n",
@@ -698,8 +701,10 @@ int cx8802_register_driver(struct cx8802_driver *drv)
 
 		/* Bring up a new struct for each driver instance */
 		driver = kzalloc(sizeof(*drv),GFP_KERNEL);
-		if (driver == NULL)
-			return -ENOMEM;
+		if (driver == NULL) {
+			err = -ENOMEM;
+			goto out;
+		}
 
 		/* Snapshot of the driver registration data */
 		drv->core = dev->core;
@@ -723,7 +728,10 @@ int cx8802_register_driver(struct cx8802_driver *drv)
 
 	}
 
-	return i ? 0 : -ENODEV;
+	err = i ? 0 : -ENODEV;
+out:
+	mutex_unlock(&cx8802_mutex);
+	return err;
 }
 
 int cx8802_unregister_driver(struct cx8802_driver *drv)
@@ -737,6 +745,8 @@ int cx8802_unregister_driver(struct cx8802_driver *drv)
 	       drv->type_id == CX88_MPEG_DVB ? "dvb" : "blackbird",
 	       drv->hw_access == CX8802_DRVCTL_SHARED ? "shared" : "exclusive");
 
+	mutex_lock(&cx8802_mutex);
+
 	list_for_each_entry(dev, &cx8802_devlist, devlist) {
 		printk(KERN_INFO
 		       "%s/2: subsystem: %04x:%04x, board: %s [card=%d]\n",
@@ -763,6 +773,8 @@ int cx8802_unregister_driver(struct cx8802_driver *drv)
 		mutex_unlock(&dev->core->lock);
 	}
 
+	mutex_unlock(&cx8802_mutex);
+
 	return err;
 }
 
@@ -800,7 +812,9 @@ static int __devinit cx8802_probe(struct pci_dev *pci_dev,
 		goto fail_free;
 
 	INIT_LIST_HEAD(&dev->drvlist);
+	mutex_lock(&cx8802_mutex);
 	list_add_tail(&dev->devlist,&cx8802_devlist);
+	mutex_unlock(&cx8802_mutex);
 
 	/* now autoload cx88-dvb or cx88-blackbird */
 	request_modules(dev);
-- 
1.7.5.rc0

Re: [PATCH 3/3] [media] cx88: use a mutex to protect cx8802_devlist

[Jonathan Nieder]

Hi Andreas,

(please turn off HTML mail.)
Andreas Huber wrote:

> There is a reference count bug in the driver code. The driver's
> active_ref count may become negative which leads to unpredictable
> behavior. (mpeg video device inaccessible, etc ...)

Hmm, the patchset didn't touch active_ref handling.

active_ref was added by v2.6.25-rc3~132^2~7 (V4L/DVB (7194):
cx88-mpeg: Allow concurrent access to cx88-mpeg devices, 2008-02-11)
and relies on three assumptions:

 * (successful) calls to cx8802_driver::request_acquire are balanced
   with calls to cx8802_driver::request_release;

 * cx8802_driver::advise_acquire is non-null if and only if
   cx8802_driver::advise_release is (since both are NULL for
   blackbird, non-NULL for dvb);

 * no data races.

I suppose it would be more idiomatic to use an atomic_t, but access to
active_ref was previously protected by the BKL and now it is protected
by core->lock.  So it's not clear to me why this doesn't work.

Any hints?  (e.g., a detailed reproduction recipe, or a log after
adding a printk to find out when exactly active_ref becomes negative)

Thanks for reporting.
Jonathan

Re: [PATCH 3/3] [media] cx88: use a mutex to protect cx8802_devlist

[Andreas Huber]

Hi Jonathan, thanks for locking into it.
I'll try to debug more deeply what's going wrong and keep you up to date.
Andi.

On 02.04.2011 21:29, Jonathan Nieder wrote:
> Hi Andreas,
>
> (please turn off HTML mail.)
> Andreas Huber wrote:
>
>> There is a reference count bug in the driver code. The driver's
>> active_ref count may become negative which leads to unpredictable
>> behavior. (mpeg video device inaccessible, etc ...)
> Hmm, the patchset didn't touch active_ref handling.
>
> active_ref was added by v2.6.25-rc3~132^2~7 (V4L/DVB (7194):
> cx88-mpeg: Allow concurrent access to cx88-mpeg devices, 2008-02-11)
> and relies on three assumptions:
>
>   * (successful) calls to cx8802_driver::request_acquire are balanced
>     with calls to cx8802_driver::request_release;
>
>   * cx8802_driver::advise_acquire is non-null if and only if
>     cx8802_driver::advise_release is (since both are NULL for
>     blackbird, non-NULL for dvb);
>
>   * no data races.
>
> I suppose it would be more idiomatic to use an atomic_t, but access to
> active_ref was previously protected by the BKL and now it is protected
> by core->lock.  So it's not clear to me why this doesn't work.
>
> Any hints?  (e.g., a detailed reproduction recipe, or a log after
> adding a printk to find out when exactly active_ref becomes negative)
>
> Thanks for reporting.
> Jonathan

Re: [PATCH 3/3] [media] cx88: use a mutex to protect cx8802_devlist

[Andreas Huber]

I added some printk entries and finally got suspicious output after

rmmod cx88_blackbird ; modprobe cx88_blackbird debug=true

[...]
cx88[0]/2: registered device video3 [mpeg]
cx88[0]/2-bb: mpeg_open
[cx88-blackbird.c,mpeg_open(),line 1059] mutex_lock(&dev->core->lock);
cx88[0]/2-bb: Initialize codec
[...]
cx88[0]/2-bb: open dev=video3
[cx88-blackbird.c,mpeg_open(),line 1103] mutex_unlock(&dev->core->lock); 
// normal exit
[...]
cx88[1]/2: subsystem: 0070:9601, board: Hauppauge WinTV-HVR1300 
DVB-T/Hybrid MPEG Encoder [card=56]
[cx88-blackbird.c,mpeg_release(),line 1122] mutex_lock(&dev->core->lock);
cx88[0] core->active_ref=0
[cx88-mpeg.c,cx8802_request_release(),line 655]                         
   // BANG !!!!!!!!!!!!!!! core->active_ref=-1
[cx88-blackbird.c,mpeg_release(),line 1129] drv->request_release(drv); 
// drv->core->active_ref=(0->0)
[cx88-blackbird.c,mpeg_release(),line 1133] 
mutex_unlock(&dev->core->lock); // normal exit
cx88[1]/2-bb: cx8802_blackbird_probe
[...]

Analyzing this lead me to the conclusion, that a call to mpeg_open() in 
cx88-blackbird.c
returns with 0 (= success)
while not actually having increased the active_ref count.

Here's the relevant code fragment ...

static int mpeg_open(struct file *file)
{
[...]
     /* Make sure we can acquire the hardware */
     drv = cx8802_get_driver(dev, CX88_MPEG_BLACKBIRD);      // HOW TO 
DEAL WITH NULL  ??????
     if (drv) {
         err = drv->request_acquire(drv);
         if(err != 0) {
             dprintk(1,"%s: Unable to acquire hardware, %d\n", __func__, 
err);
             mutex_unlock(&dev->core->lock);
             return err;
         }
     }
[...]
     return 0;
}

I suspect, that

drv = cx8802_get_driver(dev, CX88_MPEG_BLACKBIRD);

in my case might evaluates to NULL (not normally but during driver 
initialization!?)
which then leads to

1) mpeg_open() returns with success

and

2) active_ref count has not been increased

which results in a negative active_ref count later on.

But I might as well be totally wrong.

Andi

On 02.04.2011 22:13, Andreas Huber wrote:
> Hi Jonathan, thanks for locking into it.
> I'll try to debug more deeply what's going wrong and keep you up to date.
> Andi.
>
> On 02.04.2011 21:29, Jonathan Nieder wrote:
>> Hi Andreas,
>>
>> (please turn off HTML mail.)
>> Andreas Huber wrote:
>>
>>> There is a reference count bug in the driver code. The driver's
>>> active_ref count may become negative which leads to unpredictable
>>> behavior. (mpeg video device inaccessible, etc ...)
>> Hmm, the patchset didn't touch active_ref handling.
>>
>> active_ref was added by v2.6.25-rc3~132^2~7 (V4L/DVB (7194):
>> cx88-mpeg: Allow concurrent access to cx88-mpeg devices, 2008-02-11)
>> and relies on three assumptions:
>>
>>   * (successful) calls to cx8802_driver::request_acquire are balanced
>>     with calls to cx8802_driver::request_release;
>>
>>   * cx8802_driver::advise_acquire is non-null if and only if
>>     cx8802_driver::advise_release is (since both are NULL for
>>     blackbird, non-NULL for dvb);
>>
>>   * no data races.
>>
>> I suppose it would be more idiomatic to use an atomic_t, but access to
>> active_ref was previously protected by the BKL and now it is protected
>> by core->lock.  So it's not clear to me why this doesn't work.
>>
>> Any hints?  (e.g., a detailed reproduction recipe, or a log after
>> adding a printk to find out when exactly active_ref becomes negative)
>>
>> Thanks for reporting.
>> Jonathan
>

Re: [PATCH 3/3] [media] cx88: use a mutex to protect cx8802_devlist

[Jonathan Nieder]

Hi again,

Andreas Huber wrote:

> [...]
>     /* Make sure we can acquire the hardware */
>     drv = cx8802_get_driver(dev, CX88_MPEG_BLACKBIRD);      // HOW TO DEAL WITH NULL  ??????
>     if (drv) {

Reasonable question.  Could you try this patch?

-- 8< -- 
Subject: [media] cx88: lock device during driver initialization

Reported-by: Andreas Huber <hobrom@gmx.at>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 drivers/media/video/cx88/cx88-blackbird.c |    2 --
 drivers/media/video/cx88/cx88-mpeg.c      |    5 ++---
 drivers/media/video/cx88/cx88.h           |    7 ++-----
 3 files changed, 4 insertions(+), 10 deletions(-)

diff --git a/drivers/media/video/cx88/cx88-blackbird.c b/drivers/media/video/cx88/cx88-blackbird.c
index a6f7d53..f637d34 100644
--- a/drivers/media/video/cx88/cx88-blackbird.c
+++ b/drivers/media/video/cx88/cx88-blackbird.c
@@ -1335,11 +1335,9 @@ static int cx8802_blackbird_probe(struct cx8802_driver *drv)
 	blackbird_register_video(dev);
 
 	/* initial device configuration: needed ? */
-	mutex_lock(&dev->core->lock);
 //	init_controls(core);
 	cx88_set_tvnorm(core,core->tvnorm);
 	cx88_video_mux(core,0);
-	mutex_unlock(&dev->core->lock);
 
 	return 0;
 
diff --git a/drivers/media/video/cx88/cx88-mpeg.c b/drivers/media/video/cx88/cx88-mpeg.c
index 6b58647..b18f9fe 100644
--- a/drivers/media/video/cx88/cx88-mpeg.c
+++ b/drivers/media/video/cx88/cx88-mpeg.c
@@ -714,18 +714,17 @@ int cx8802_register_driver(struct cx8802_driver *drv)
 		drv->request_release = cx8802_request_release;
 		memcpy(driver, drv, sizeof(*driver));
 
+		mutex_lock(&drv->core->lock);
 		err = drv->probe(driver);
 		if (err == 0) {
 			i++;
-			mutex_lock(&drv->core->lock);
 			list_add_tail(&driver->drvlist, &dev->drvlist);
-			mutex_unlock(&drv->core->lock);
 		} else {
 			printk(KERN_ERR
 			       "%s/2: cx8802 probe failed, err = %d\n",
 			       dev->core->name, err);
 		}
-
+		mutex_unlock(&drv->core->lock);
 	}
 
 	err = i ? 0 : -ENODEV;
diff --git a/drivers/media/video/cx88/cx88.h b/drivers/media/video/cx88/cx88.h
index 9731daa..3d32f4a 100644
--- a/drivers/media/video/cx88/cx88.h
+++ b/drivers/media/video/cx88/cx88.h
@@ -505,13 +505,10 @@ struct cx8802_driver {
 	int (*suspend)(struct pci_dev *pci_dev, pm_message_t state);
 	int (*resume)(struct pci_dev *pci_dev);
 
+	/* Callers to the following functions must hold core->lock */
+
 	/* MPEG 8802 -> mini driver - Driver probe and configuration */
-
-	/* Caller must _not_ hold core->lock */
 	int (*probe)(struct cx8802_driver *drv);
-
-	/* Callers to the following functions must hold core->lock */
-
 	int (*remove)(struct cx8802_driver *drv);
 
 	/* MPEG 8802 -> mini driver - Access for hardware control */
-- 
1.7.5.rc0

Re: [RFC/PATCH 0/3] locking fixes for cx88

[Andreas Huber]

Jonathan Nieder <jrnieder <at> gmail.com> writes:

> 
> Hi,
> 
> Huber Andreas wrote[1]:
> 
> > Processes that try to open a cx88-blackbird driven MPEG device will hang up.
> 
> Here's a possible fix based on a patch by Ben Hutchings and
> corrections from Andi Huber.  Warning: probably full of mistakes (my
> fault) since I'm not familiar with any of this stuff.  Untested.
> Review and testing would be welcome.
> 
> Ben Hutchings (2):
>   [media] cx88: fix locking of sub-driver operations
>   [media] cx88: use a mutex to protect cx8802_devlist
> 
> Jonathan Nieder (1):
>   [media] cx88: protect per-device driver list with device lock
> 
>  drivers/media/video/cx88/cx88-blackbird.c |    3 +-
>  drivers/media/video/cx88/cx88-dvb.c       |    2 +
>  drivers/media/video/cx88/cx88-mpeg.c      |   35 +++++++++++++++++++---------
>  drivers/media/video/cx88/cx88.h           |   10 +++++++-
>  4 files changed, 37 insertions(+), 13 deletions(-)
> 

There is an unrelated issue!!!

The driver's active_ref count may become negative which leads to unpredictable
behavior. (MPEG video device inaccessible, etc ...)

Here's a possible fix ...


diff -Nur a/drivers/media/video/cx88/cx88-mpeg.c
b/drivers/media/video/cx88/cx88-mpeg.c
--- a/drivers/media/video/cx88/cx88-mpeg.c	2011-04-02 14:34:21.456569849 +0200
+++ b/drivers/media/video/cx88/cx88-mpeg.c	2011-04-02 14:32:55.467038000 +0200
@@ -642,12 +642,16 @@
 {
 	struct cx88_core *core = drv->core;
 
+	mpeg_dbg(1,"%s active driver references before release attempt:
%d\n",core->name,core->active_ref);
+	
 	if (drv->advise_release && --core->active_ref == 0)
 	{
 		drv->advise_release(drv);
 		core->active_type_id = CX88_BOARD_NONE;
 		mpeg_dbg(1,"%s() Post release GPIO=%x\n", __func__, cx_read(MO_GP0_IO));
 	}
+	
+	if(core->active_ref<0) core->active_ref=0; // prevent us from getting negative
ref counts!
 
 	return 0;
 }


I can confirm that Johnathan's patchset plus applying this fix works!
Tests done on amd64 hardware with 2 Hauppauge HVR1300 TV cards: 
Both were independently able to tune channels and stream encoded video through
their MPEG encoder devices.

Andi.

Re: [RFC/PATCH 0/3] locking fixes for cx88

[Ben Hutchings]

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

On Sat, 2011-04-02 at 04:38 -0500, Jonathan Nieder wrote:
> Hi,
> 
> Huber Andreas wrote[1]:
> 
> > Processes that try to open a cx88-blackbird driven MPEG device will hang up.
> 
> Here's a possible fix based on a patch by Ben Hutchings and
> corrections from Andi Huber.  Warning: probably full of mistakes (my
> fault) since I'm not familiar with any of this stuff.  Untested.
> Review and testing would be welcome.

Since you have split up and otherwise modified the patch I sent, please
remove the 'From' and 'Signed-off-by' lines with my name and address.
Just state that the patches are based on my work.

Ben.

> Ben Hutchings (2):
>   [media] cx88: fix locking of sub-driver operations
>   [media] cx88: use a mutex to protect cx8802_devlist
> 
> Jonathan Nieder (1):
>   [media] cx88: protect per-device driver list with device lock
> 
>  drivers/media/video/cx88/cx88-blackbird.c |    3 +-
>  drivers/media/video/cx88/cx88-dvb.c       |    2 +
>  drivers/media/video/cx88/cx88-mpeg.c      |   35 +++++++++++++++++++---------
>  drivers/media/video/cx88/cx88.h           |   10 +++++++-
>  4 files changed, 37 insertions(+), 13 deletions(-)
> 

-- 
Ben Hutchings
Once a job is fouled up, anything done to improve it makes it worse.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [RFC/PATCH 0/3] locking fixes for cx88

[Jonathan Nieder]

Ben Hutchings wrote:

> Since you have split up and otherwise modified the patch I sent, please
> remove the 'From' and 'Signed-off-by' lines with my name and address.
> Just state that the patches are based on my work.

Okay, will do.

Thanks again for writing it.

[RFC/PATCH v2 0/7] locking fixes for cx88

[Jonathan Nieder]

Hi again,

Jonathan Nieder wrote:
> Huber Andreas wrote[1]:

>> Processes that try to open a cx88-blackbird driven MPEG device will hang up.
>
> Here's a possible fix based on a patch by Ben Hutchings and
> corrections from Andi Huber.  Warning: probably full of mistakes (my
> fault) since I'm not familiar with any of this stuff.  Untested.
> Review and testing would be welcome.

A reroll.  As before, the goals are: (1) eliminate deadlock, (2)
eliminate races, (3) introduce some clarity.  The same caveats as
last time apply --- this is only compile-tested.  Thanks again to Andi
for testing the previous series and for other useful feedback.

Patch 1 is meant to protect dev->drvlist against data races.
Since v1, I removed some clutter in the patch itself and clarified the
change description to match.

Patch 2 addresses the original deadlock.  The only changes are the
description and declared authorship of the patch (at Ben's request).

Patch 3 is new.  It fixes the reference count breakage Andi noticed
(another race previously protected against by the BKL).

Patch 4 fixes a data race noticed by Ben (also from his patch).  It's
unchanged.

Patches 5, 6, and 7 are cleanups.

Bugs?  Thoughts?
Jonathan Nieder (7):
  [media] cx88: protect per-device driver list with device lock
  [media] cx88: fix locking of sub-driver operations
  [media] cx88: hold device lock during sub-driver initialization
  [media] cx88: use a mutex to protect cx8802_devlist
  [media] cx88: handle attempts to use unregistered cx88-blackbird
    driver
  [media] cx88: don't use atomic_t for core->mpeg_users
  [media] cx88: don't use atomic_t for core->users

 drivers/media/video/cx88/cx88-blackbird.c |   41 +++++++++++++++-------------
 drivers/media/video/cx88/cx88-dvb.c       |    2 +
 drivers/media/video/cx88/cx88-mpeg.c      |   40 ++++++++++++++++++----------
 drivers/media/video/cx88/cx88-video.c     |    5 ++-
 drivers/media/video/cx88/cx88.h           |   11 +++++--
 5 files changed, 61 insertions(+), 38 deletions(-)

[PATCH 1/7] [media] cx88: protect per-device driver list with device lock

[Jonathan Nieder]

The BKL conversion of this driver seems to have gone wrong.  Opening
cx88-blackbird will deadlock.  Various other uses of the sub-device
and driver lists appear to be subject to race conditions.

For example: various functions access drvlist without a relevant lock
held, which will race against removal of drivers.  Let's start with
that --- clean up by consistently protecting dev->drvlist with
dev->core->lock, noting driver functions that require the device lock
to be held or not to be held.

There are still some races --- e.g., cx8802_blackbird_remove can run
between the time the blackbird driver is acquired and the time it is
used in mpeg_release, and there's a similar race in cx88_dvb_bus_ctrl.

The only goal is to make the semantics clearer in preparation for more
changes.  Later patches will address the remaining known races and the
deadlock noticed by Andi.

Based on a patch by Ben Hutchings <ben@decadent.org.uk>.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Cc: Andi Huber <hobrom@gmx.at>
Cc: stable@kernel.org
---
 drivers/media/video/cx88/cx88-blackbird.c |    3 ++-
 drivers/media/video/cx88/cx88-dvb.c       |    3 +++
 drivers/media/video/cx88/cx88-mpeg.c      |   11 +++++++----
 drivers/media/video/cx88/cx88.h           |    9 ++++++++-
 4 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/drivers/media/video/cx88/cx88-blackbird.c b/drivers/media/video/cx88/cx88-blackbird.c
index bca307e..b93fbd3 100644
--- a/drivers/media/video/cx88/cx88-blackbird.c
+++ b/drivers/media/video/cx88/cx88-blackbird.c
@@ -1122,10 +1122,11 @@ static int mpeg_release(struct file *file)
 	mutex_lock(&dev->core->lock);
 	file->private_data = NULL;
 	kfree(fh);
-	mutex_unlock(&dev->core->lock);
 
 	/* Make sure we release the hardware */
 	drv = cx8802_get_driver(dev, CX88_MPEG_BLACKBIRD);
+	mutex_unlock(&dev->core->lock);
+
 	if (drv)
 		drv->request_release(drv);
 
diff --git a/drivers/media/video/cx88/cx88-dvb.c b/drivers/media/video/cx88/cx88-dvb.c
index 7b8c9d3..84002bc 100644
--- a/drivers/media/video/cx88/cx88-dvb.c
+++ b/drivers/media/video/cx88/cx88-dvb.c
@@ -133,7 +133,10 @@ static int cx88_dvb_bus_ctrl(struct dvb_frontend* fe, int acquire)
 		return -EINVAL;
 	}
 
+	mutex_lock(&dev->core->lock);
 	drv = cx8802_get_driver(dev, CX88_MPEG_DVB);
+	mutex_unlock(&dev->core->lock);
+
 	if (drv) {
 		if (acquire){
 			dev->frontends.active_fe_id = fe_id;
diff --git a/drivers/media/video/cx88/cx88-mpeg.c b/drivers/media/video/cx88/cx88-mpeg.c
index addf954..918172b 100644
--- a/drivers/media/video/cx88/cx88-mpeg.c
+++ b/drivers/media/video/cx88/cx88-mpeg.c
@@ -748,6 +748,8 @@ int cx8802_unregister_driver(struct cx8802_driver *drv)
 		       dev->pci->subsystem_device, dev->core->board.name,
 		       dev->core->boardnr);
 
+		mutex_lock(&dev->core->lock);
+
 		list_for_each_entry_safe(d, dtmp, &dev->drvlist, drvlist) {
 			/* only unregister the correct driver type */
 			if (d->type_id != drv->type_id)
@@ -755,15 +757,14 @@ int cx8802_unregister_driver(struct cx8802_driver *drv)
 
 			err = d->remove(d);
 			if (err == 0) {
-				mutex_lock(&drv->core->lock);
 				list_del(&d->drvlist);
-				mutex_unlock(&drv->core->lock);
 				kfree(d);
 			} else
 				printk(KERN_ERR "%s/2: cx8802 driver remove "
 				       "failed (%d)\n", dev->core->name, err);
 		}
 
+		mutex_unlock(&dev->core->lock);
 	}
 
 	return err;
@@ -827,6 +828,8 @@ static void __devexit cx8802_remove(struct pci_dev *pci_dev)
 
 	flush_request_modules(dev);
 
+	mutex_lock(&dev->core->lock);
+
 	if (!list_empty(&dev->drvlist)) {
 		struct cx8802_driver *drv, *tmp;
 		int err;
@@ -838,9 +841,7 @@ static void __devexit cx8802_remove(struct pci_dev *pci_dev)
 		list_for_each_entry_safe(drv, tmp, &dev->drvlist, drvlist) {
 			err = drv->remove(drv);
 			if (err == 0) {
-				mutex_lock(&drv->core->lock);
 				list_del(&drv->drvlist);
-				mutex_unlock(&drv->core->lock);
 			} else
 				printk(KERN_ERR "%s/2: cx8802 driver remove "
 				       "failed (%d)\n", dev->core->name, err);
@@ -848,6 +849,8 @@ static void __devexit cx8802_remove(struct pci_dev *pci_dev)
 		}
 	}
 
+	mutex_unlock(&dev->core->lock);
+
 	/* Destroy any 8802 reference. */
 	dev->core->dvbdev = NULL;
 
diff --git a/drivers/media/video/cx88/cx88.h b/drivers/media/video/cx88/cx88.h
index 9b3742a..e3d56c2 100644
--- a/drivers/media/video/cx88/cx88.h
+++ b/drivers/media/video/cx88/cx88.h
@@ -506,7 +506,11 @@ struct cx8802_driver {
 	int (*resume)(struct pci_dev *pci_dev);
 
 	/* MPEG 8802 -> mini driver - Driver probe and configuration */
+
+	/* Caller must _not_ hold core->lock */
 	int (*probe)(struct cx8802_driver *drv);
+
+	/* Caller must hold core->lock */
 	int (*remove)(struct cx8802_driver *drv);
 
 	/* MPEG 8802 -> mini driver - Access for hardware control */
@@ -561,8 +565,9 @@ struct cx8802_dev {
 	/* for switching modulation types */
 	unsigned char              ts_gen_cntrl;
 
-	/* List of attached drivers */
+	/* List of attached drivers; must hold core->lock to access */
 	struct list_head	   drvlist;
+
 	struct work_struct	   request_module_wk;
 };
 
@@ -685,6 +690,8 @@ int cx88_audio_thread(void *data);
 
 int cx8802_register_driver(struct cx8802_driver *drv);
 int cx8802_unregister_driver(struct cx8802_driver *drv);
+
+/* Caller must hold core->lock */
 struct cx8802_driver * cx8802_get_driver(struct cx8802_dev *dev, enum cx88_board_type btype);
 
 /* ----------------------------------------------------------- */
-- 
1.7.5.rc0

[PATCH 2/7] [media] cx88: fix locking of sub-driver operations

[Jonathan Nieder]

The BKL conversion of this driver seems to have gone wrong.
Loading the cx88-blackbird driver deadlocks.

The cause: mpeg_ops::open in the cx2388x blackbird driver acquires the
device lock and calls the sub-driver's request_acquire, which tries to
acquire the lock again.  Fix it by clarifying the semantics of
request_acquire, request_release, advise_acquire, and advise_release:
all will require the caller to hold the device lock.

Based on a patch by Ben Hutchings <ben@decadent.org.uk>.

Reported-by: Andi Huber <hobrom@gmx.at>
Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=31962
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Cc: stable@kernel.org
---
 drivers/media/video/cx88/cx88-blackbird.c |    4 ++--
 drivers/media/video/cx88/cx88-dvb.c       |    3 +--
 drivers/media/video/cx88/cx88-mpeg.c      |    4 ----
 drivers/media/video/cx88/cx88.h           |    3 ++-
 4 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/media/video/cx88/cx88-blackbird.c b/drivers/media/video/cx88/cx88-blackbird.c
index b93fbd3..a6f7d53 100644
--- a/drivers/media/video/cx88/cx88-blackbird.c
+++ b/drivers/media/video/cx88/cx88-blackbird.c
@@ -1125,13 +1125,13 @@ static int mpeg_release(struct file *file)
 
 	/* Make sure we release the hardware */
 	drv = cx8802_get_driver(dev, CX88_MPEG_BLACKBIRD);
-	mutex_unlock(&dev->core->lock);
-
 	if (drv)
 		drv->request_release(drv);
 
 	atomic_dec(&dev->core->mpeg_users);
 
+	mutex_unlock(&dev->core->lock);
+
 	return 0;
 }
 
diff --git a/drivers/media/video/cx88/cx88-dvb.c b/drivers/media/video/cx88/cx88-dvb.c
index 84002bc..c69df7e 100644
--- a/drivers/media/video/cx88/cx88-dvb.c
+++ b/drivers/media/video/cx88/cx88-dvb.c
@@ -135,8 +135,6 @@ static int cx88_dvb_bus_ctrl(struct dvb_frontend* fe, int acquire)
 
 	mutex_lock(&dev->core->lock);
 	drv = cx8802_get_driver(dev, CX88_MPEG_DVB);
-	mutex_unlock(&dev->core->lock);
-
 	if (drv) {
 		if (acquire){
 			dev->frontends.active_fe_id = fe_id;
@@ -146,6 +144,7 @@ static int cx88_dvb_bus_ctrl(struct dvb_frontend* fe, int acquire)
 			dev->frontends.active_fe_id = 0;
 		}
 	}
+	mutex_unlock(&dev->core->lock);
 
 	return ret;
 }
diff --git a/drivers/media/video/cx88/cx88-mpeg.c b/drivers/media/video/cx88/cx88-mpeg.c
index 918172b..9147c16 100644
--- a/drivers/media/video/cx88/cx88-mpeg.c
+++ b/drivers/media/video/cx88/cx88-mpeg.c
@@ -624,13 +624,11 @@ static int cx8802_request_acquire(struct cx8802_driver *drv)
 
 	if (drv->advise_acquire)
 	{
-		mutex_lock(&drv->core->lock);
 		core->active_ref++;
 		if (core->active_type_id == CX88_BOARD_NONE) {
 			core->active_type_id = drv->type_id;
 			drv->advise_acquire(drv);
 		}
-		mutex_unlock(&drv->core->lock);
 
 		mpeg_dbg(1,"%s() Post acquire GPIO=%x\n", __func__, cx_read(MO_GP0_IO));
 	}
@@ -643,14 +641,12 @@ static int cx8802_request_release(struct cx8802_driver *drv)
 {
 	struct cx88_core *core = drv->core;
 
-	mutex_lock(&drv->core->lock);
 	if (drv->advise_release && --core->active_ref == 0)
 	{
 		drv->advise_release(drv);
 		core->active_type_id = CX88_BOARD_NONE;
 		mpeg_dbg(1,"%s() Post release GPIO=%x\n", __func__, cx_read(MO_GP0_IO));
 	}
-	mutex_unlock(&drv->core->lock);
 
 	return 0;
 }
diff --git a/drivers/media/video/cx88/cx88.h b/drivers/media/video/cx88/cx88.h
index e3d56c2..9731daa 100644
--- a/drivers/media/video/cx88/cx88.h
+++ b/drivers/media/video/cx88/cx88.h
@@ -510,7 +510,8 @@ struct cx8802_driver {
 	/* Caller must _not_ hold core->lock */
 	int (*probe)(struct cx8802_driver *drv);
 
-	/* Caller must hold core->lock */
+	/* Callers to the following functions must hold core->lock */
+
 	int (*remove)(struct cx8802_driver *drv);
 
 	/* MPEG 8802 -> mini driver - Access for hardware control */
-- 
1.7.5.rc0

[PATCH 3/7] [media] cx88: hold device lock during sub-driver initialization

[Jonathan Nieder]

cx8802_blackbird_probe makes a device node for the mpeg sub-device
before it has been added to dev->drvlist.  If the device is opened
during that time, the open succeeds but request_acquire cannot be
called, so the reference count remains zero.  Later, when the device
is closed, the reference count becomes negative --- uh oh.

Close the race by holding core->lock during probe and not releasing
until the device is in drvlist and initialization finished.
Previously the BKL prevented this race.

Reported-by: Andreas Huber <hobrom@gmx.at>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 drivers/media/video/cx88/cx88-blackbird.c |    2 --
 drivers/media/video/cx88/cx88-mpeg.c      |    5 ++---
 drivers/media/video/cx88/cx88.h           |    7 ++-----
 3 files changed, 4 insertions(+), 10 deletions(-)

diff --git a/drivers/media/video/cx88/cx88-blackbird.c b/drivers/media/video/cx88/cx88-blackbird.c
index a6f7d53..f637d34 100644
--- a/drivers/media/video/cx88/cx88-blackbird.c
+++ b/drivers/media/video/cx88/cx88-blackbird.c
@@ -1335,11 +1335,9 @@ static int cx8802_blackbird_probe(struct cx8802_driver *drv)
 	blackbird_register_video(dev);
 
 	/* initial device configuration: needed ? */
-	mutex_lock(&dev->core->lock);
 //	init_controls(core);
 	cx88_set_tvnorm(core,core->tvnorm);
 	cx88_video_mux(core,0);
-	mutex_unlock(&dev->core->lock);
 
 	return 0;
 
diff --git a/drivers/media/video/cx88/cx88-mpeg.c b/drivers/media/video/cx88/cx88-mpeg.c
index 9147c16..497f26f 100644
--- a/drivers/media/video/cx88/cx88-mpeg.c
+++ b/drivers/media/video/cx88/cx88-mpeg.c
@@ -709,18 +709,17 @@ int cx8802_register_driver(struct cx8802_driver *drv)
 		drv->request_release = cx8802_request_release;
 		memcpy(driver, drv, sizeof(*driver));
 
+		mutex_lock(&drv->core->lock);
 		err = drv->probe(driver);
 		if (err == 0) {
 			i++;
-			mutex_lock(&drv->core->lock);
 			list_add_tail(&driver->drvlist, &dev->drvlist);
-			mutex_unlock(&drv->core->lock);
 		} else {
 			printk(KERN_ERR
 			       "%s/2: cx8802 probe failed, err = %d\n",
 			       dev->core->name, err);
 		}
-
+		mutex_unlock(&drv->core->lock);
 	}
 
 	return i ? 0 : -ENODEV;
diff --git a/drivers/media/video/cx88/cx88.h b/drivers/media/video/cx88/cx88.h
index 9731daa..3d32f4a 100644
--- a/drivers/media/video/cx88/cx88.h
+++ b/drivers/media/video/cx88/cx88.h
@@ -505,13 +505,10 @@ struct cx8802_driver {
 	int (*suspend)(struct pci_dev *pci_dev, pm_message_t state);
 	int (*resume)(struct pci_dev *pci_dev);
 
+	/* Callers to the following functions must hold core->lock */
+
 	/* MPEG 8802 -> mini driver - Driver probe and configuration */
-
-	/* Caller must _not_ hold core->lock */
 	int (*probe)(struct cx8802_driver *drv);
-
-	/* Callers to the following functions must hold core->lock */
-
 	int (*remove)(struct cx8802_driver *drv);
 
 	/* MPEG 8802 -> mini driver - Access for hardware control */
-- 
1.7.5.rc0

[PATCH 4/7] [media] cx88: use a mutex to protect cx8802_devlist

[Jonathan Nieder]

Add and use a mutex to protect the cx88-mpeg device list.  Previously
the BKL prevented races.

Based on a patch by Ben Hutchings <ben@decadent.org.uk>.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 drivers/media/video/cx88/cx88-mpeg.c |   20 +++++++++++++++++---
 1 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/drivers/media/video/cx88/cx88-mpeg.c b/drivers/media/video/cx88/cx88-mpeg.c
index 497f26f..b18f9fe 100644
--- a/drivers/media/video/cx88/cx88-mpeg.c
+++ b/drivers/media/video/cx88/cx88-mpeg.c
@@ -78,6 +78,7 @@ static void flush_request_modules(struct cx8802_dev *dev)
 
 
 static LIST_HEAD(cx8802_devlist);
+static DEFINE_MUTEX(cx8802_mutex);
 /* ------------------------------------------------------------------ */
 
 static int cx8802_start_dma(struct cx8802_dev    *dev,
@@ -689,6 +690,8 @@ int cx8802_register_driver(struct cx8802_driver *drv)
 		return err;
 	}
 
+	mutex_lock(&cx8802_mutex);
+
 	list_for_each_entry(dev, &cx8802_devlist, devlist) {
 		printk(KERN_INFO
 		       "%s/2: subsystem: %04x:%04x, board: %s [card=%d]\n",
@@ -698,8 +701,10 @@ int cx8802_register_driver(struct cx8802_driver *drv)
 
 		/* Bring up a new struct for each driver instance */
 		driver = kzalloc(sizeof(*drv),GFP_KERNEL);
-		if (driver == NULL)
-			return -ENOMEM;
+		if (driver == NULL) {
+			err = -ENOMEM;
+			goto out;
+		}
 
 		/* Snapshot of the driver registration data */
 		drv->core = dev->core;
@@ -722,7 +727,10 @@ int cx8802_register_driver(struct cx8802_driver *drv)
 		mutex_unlock(&drv->core->lock);
 	}
 
-	return i ? 0 : -ENODEV;
+	err = i ? 0 : -ENODEV;
+out:
+	mutex_unlock(&cx8802_mutex);
+	return err;
 }
 
 int cx8802_unregister_driver(struct cx8802_driver *drv)
@@ -736,6 +744,8 @@ int cx8802_unregister_driver(struct cx8802_driver *drv)
 	       drv->type_id == CX88_MPEG_DVB ? "dvb" : "blackbird",
 	       drv->hw_access == CX8802_DRVCTL_SHARED ? "shared" : "exclusive");
 
+	mutex_lock(&cx8802_mutex);
+
 	list_for_each_entry(dev, &cx8802_devlist, devlist) {
 		printk(KERN_INFO
 		       "%s/2: subsystem: %04x:%04x, board: %s [card=%d]\n",
@@ -762,6 +772,8 @@ int cx8802_unregister_driver(struct cx8802_driver *drv)
 		mutex_unlock(&dev->core->lock);
 	}
 
+	mutex_unlock(&cx8802_mutex);
+
 	return err;
 }
 
@@ -799,7 +811,9 @@ static int __devinit cx8802_probe(struct pci_dev *pci_dev,
 		goto fail_free;
 
 	INIT_LIST_HEAD(&dev->drvlist);
+	mutex_lock(&cx8802_mutex);
 	list_add_tail(&dev->devlist,&cx8802_devlist);
+	mutex_unlock(&cx8802_mutex);
 
 	/* now autoload cx88-dvb or cx88-blackbird */
 	request_modules(dev);
-- 
1.7.5.rc0

[PATCH 5/7] [media] cx88: gracefully reject attempts to use unregistered cx88-blackbird driver

[Jonathan Nieder]

It should not be possible to enter mpeg_open and acquire core->lock
without the blackbird driver being registered, so just error out if it
is not.  This makes the code more readable and should prevent the bug
fixed by the patch "hold device lock during sub-driver initialization"
from resurfacing again.

Similarly, if we enter mpeg_release and acquire core->lock then either
the blackbird driver is registered (since open files keep it loaded)
or the sysadmin forced the driver's removal.  In the latter case the
state will be inconsistent and this is worth a loud warning.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 drivers/media/video/cx88/cx88-blackbird.c |   25 ++++++++++++++-----------
 1 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/drivers/media/video/cx88/cx88-blackbird.c b/drivers/media/video/cx88/cx88-blackbird.c
index f637d34..fa8e347 100644
--- a/drivers/media/video/cx88/cx88-blackbird.c
+++ b/drivers/media/video/cx88/cx88-blackbird.c
@@ -1060,18 +1060,21 @@ static int mpeg_open(struct file *file)
 
 	/* Make sure we can acquire the hardware */
 	drv = cx8802_get_driver(dev, CX88_MPEG_BLACKBIRD);
-	if (drv) {
-		err = drv->request_acquire(drv);
-		if(err != 0) {
-			dprintk(1,"%s: Unable to acquire hardware, %d\n", __func__, err);
-			mutex_unlock(&dev->core->lock);
-			return err;
-		}
+	if (!drv) {
+		dprintk(1, "%s: blackbird driver is not loaded\n", __func__);
+		mutex_unlock(&dev->core->lock);
+		return -ENODEV;
+	}
+
+	err = drv->request_acquire(drv);
+	if (err != 0) {
+		dprintk(1,"%s: Unable to acquire hardware, %d\n", __func__, err);
+		mutex_unlock(&dev->core->lock);
+		return err;
 	}
 
 	if (!atomic_read(&dev->core->mpeg_users) && blackbird_initialize_codec(dev) < 0) {
-		if (drv)
-			drv->request_release(drv);
+		drv->request_release(drv);
 		mutex_unlock(&dev->core->lock);
 		return -EINVAL;
 	}
@@ -1080,8 +1083,7 @@ static int mpeg_open(struct file *file)
 	/* allocate + initialize per filehandle data */
 	fh = kzalloc(sizeof(*fh),GFP_KERNEL);
 	if (NULL == fh) {
-		if (drv)
-			drv->request_release(drv);
+		drv->request_release(drv);
 		mutex_unlock(&dev->core->lock);
 		return -ENOMEM;
 	}
@@ -1125,6 +1127,7 @@ static int mpeg_release(struct file *file)
 
 	/* Make sure we release the hardware */
 	drv = cx8802_get_driver(dev, CX88_MPEG_BLACKBIRD);
+	WARN_ON(!drv);
 	if (drv)
 		drv->request_release(drv);
 
-- 
1.7.5.rc0

[PATCH 6/7] [media] cx88: don't use atomic_t for core->mpeg_users

[Jonathan Nieder]

mpeg_users is always read or written with core->lock held except
in mpeg_release (where it looks like a bug).  A plain int is simpler
and faster.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 drivers/media/video/cx88/cx88-blackbird.c |   11 ++++++-----
 drivers/media/video/cx88/cx88.h           |    2 +-
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/media/video/cx88/cx88-blackbird.c b/drivers/media/video/cx88/cx88-blackbird.c
index fa8e347..11e49bb 100644
--- a/drivers/media/video/cx88/cx88-blackbird.c
+++ b/drivers/media/video/cx88/cx88-blackbird.c
@@ -1073,7 +1073,7 @@ static int mpeg_open(struct file *file)
 		return err;
 	}
 
-	if (!atomic_read(&dev->core->mpeg_users) && blackbird_initialize_codec(dev) < 0) {
+	if (!dev->core->mpeg_users && blackbird_initialize_codec(dev) < 0) {
 		drv->request_release(drv);
 		mutex_unlock(&dev->core->lock);
 		return -EINVAL;
@@ -1101,7 +1101,7 @@ static int mpeg_open(struct file *file)
 	cx88_set_scale(dev->core, dev->width, dev->height,
 			fh->mpegq.field);
 
-	atomic_inc(&dev->core->mpeg_users);
+	dev->core->mpeg_users++;
 	mutex_unlock(&dev->core->lock);
 	return 0;
 }
@@ -1112,7 +1112,9 @@ static int mpeg_release(struct file *file)
 	struct cx8802_dev *dev = fh->dev;
 	struct cx8802_driver *drv = NULL;
 
-	if (dev->mpeg_active && atomic_read(&dev->core->mpeg_users) == 1)
+	mutex_lock(&dev->core->lock);
+
+	if (dev->mpeg_active && dev->core->mpeg_users == 1)
 		blackbird_stop_codec(dev);
 
 	cx8802_cancel_buffers(fh->dev);
@@ -1121,7 +1123,6 @@ static int mpeg_release(struct file *file)
 
 	videobuf_mmap_free(&fh->mpegq);
 
-	mutex_lock(&dev->core->lock);
 	file->private_data = NULL;
 	kfree(fh);
 
@@ -1131,7 +1132,7 @@ static int mpeg_release(struct file *file)
 	if (drv)
 		drv->request_release(drv);
 
-	atomic_dec(&dev->core->mpeg_users);
+	dev->core->mpeg_users--;
 
 	mutex_unlock(&dev->core->lock);
 
diff --git a/drivers/media/video/cx88/cx88.h b/drivers/media/video/cx88/cx88.h
index 3d32f4a..9e8176e 100644
--- a/drivers/media/video/cx88/cx88.h
+++ b/drivers/media/video/cx88/cx88.h
@@ -390,7 +390,7 @@ struct cx88_core {
 	/* various v4l controls */
 	u32                        freq;
 	atomic_t		   users;
-	atomic_t                   mpeg_users;
+	int                        mpeg_users;
 
 	/* cx88-video needs to access cx8802 for hybrid tuner pll access. */
 	struct cx8802_dev          *dvbdev;
-- 
1.7.5.rc0

[PATCH 7/7] [mpeg] cx88: don't use atomic_t for core->users

[Jonathan Nieder]

users is always read or written with core->lock held.  A plain int is
simpler and faster.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Thanks for reading.

 drivers/media/video/cx88/cx88-video.c |    5 +++--
 drivers/media/video/cx88/cx88.h       |    2 +-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/media/video/cx88/cx88-video.c b/drivers/media/video/cx88/cx88-video.c
index 287a41e..d719d12 100644
--- a/drivers/media/video/cx88/cx88-video.c
+++ b/drivers/media/video/cx88/cx88-video.c
@@ -824,7 +824,7 @@ static int video_open(struct file *file)
 		call_all(core, tuner, s_radio);
 	}
 
-	atomic_inc(&core->users);
+	core->users++;
 	mutex_unlock(&core->lock);
 
 	return 0;
@@ -922,7 +922,8 @@ static int video_release(struct file *file)
 	file->private_data = NULL;
 	kfree(fh);
 
-	if(atomic_dec_and_test(&dev->core->users))
+	dev->core->users--;
+	if (!dev->core->users)
 		call_all(dev->core, core, s_power, 0);
 	mutex_unlock(&dev->core->lock);
 
diff --git a/drivers/media/video/cx88/cx88.h b/drivers/media/video/cx88/cx88.h
index 9e8176e..a399a8b 100644
--- a/drivers/media/video/cx88/cx88.h
+++ b/drivers/media/video/cx88/cx88.h
@@ -389,7 +389,7 @@ struct cx88_core {
 	struct mutex               lock;
 	/* various v4l controls */
 	u32                        freq;
-	atomic_t		   users;
+	int                        users;
 	int                        mpeg_users;
 
 	/* cx88-video needs to access cx8802 for hybrid tuner pll access. */
-- 
1.7.5.rc0

Re: [PATCH 7/7] [mpeg] cx88: don't use atomic_t for core->users

[Jonathan Nieder]

Jonathan Nieder wrote:

> [Subject: [PATCH 7/7] [mpeg] cx88: don't use atomic_t for core->users]

The subject should say [media] rather than [mpeg].  I fixed it locally
and then sent the wrong patch; sorry for the nonsense.

Re: [RFC/PATCH v2 0/7] locking fixes for cx88

[Jonathan Nieder]

Jonathan Nieder wrote:

> Jonathan Nieder (7):
>   [media] cx88: protect per-device driver list with device lock
>   [media] cx88: fix locking of sub-driver operations
>   [media] cx88: hold device lock during sub-driver initialization
>   [media] cx88: use a mutex to protect cx8802_devlist
>   [media] cx88: handle attempts to use unregistered cx88-blackbird
>     driver
>   [media] cx88: don't use atomic_t for core->mpeg_users
>   [media] cx88: don't use atomic_t for core->users

Good news: Andreas Huber <hobrom@gmx.at> tested on a PC with 2
Hauppauge HVR1300 TV cards.

He writes:

> Hi Jonathan, I'm glad to say it works! No more deadlocks,
> no more reference count issues.
>
> I did some stress testing on the driver load and unload mechanism
> with and without the video devices being in use. And it was
> handled all very well.
>
> Great job, thanks!