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 [email protected] ?
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]
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(-)
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 <[email protected]>
Signed-off-by: Jonathan Nieder <[email protected]>
Cc: Andi Huber <[email protected]>
Cc: [email protected]
---
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
From: Ben Hutchings <[email protected]>
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 <[email protected]>
Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=31962
Signed-off-by: Ben Hutchings <[email protected]>
Signed-off-by: Jonathan Nieder <[email protected]>
Cc: [email protected]
---
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
From: Ben Hutchings <[email protected]>
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 <[email protected]>
Signed-off-by: Jonathan Nieder <[email protected]>
---
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
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.
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.
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.
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
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
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
>
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 <[email protected]>
Signed-off-by: Jonathan Nieder <[email protected]>
---
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
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(-)
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 <[email protected]>.
Signed-off-by: Jonathan Nieder <[email protected]>
Cc: Andi Huber <[email protected]>
Cc: [email protected]
---
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
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 <[email protected]>.
Reported-by: Andi Huber <[email protected]>
Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=31962
Signed-off-by: Jonathan Nieder <[email protected]>
Cc: [email protected]
---
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
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 <[email protected]>
Signed-off-by: Jonathan Nieder <[email protected]>
---
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
Add and use a mutex to protect the cx88-mpeg device list. Previously
the BKL prevented races.
Based on a patch by Ben Hutchings <[email protected]>.
Signed-off-by: Jonathan Nieder <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
users is always read or written with core->lock held. A plain int is
simpler and faster.
Signed-off-by: Jonathan Nieder <[email protected]>
---
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
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.
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 <[email protected]> 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!