cx88-blackbird broken (since 2.6.37)
A message that is not in the archive (sent off-list or not yet received): [email protected]
Hi Andi,
Huber Andreas wrote[1]:
Quoted 2 lines
[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
Quoted 43 lines
[Cause]
Nestetmutex_locks(which are not allowed) result in a deadlock.[Details]
Source-File:drivers/media/video/cx88/cx88-blackbird.c
Function: intmpeg_open(struct file *file)
Problem: the calls todrv->request_acquire(drv); anddrv->request_release(drv); will hang because they try to lock a mutex that has already been locked by a previouse call tomutex_lock(&dev->core->lock)...1050 static int
mpeg_open(struct file *file)
1051 {
[...]
1060mutex_lock(&dev->core->lock); // MUTEX LOCKED !!!!!!!!!!!!!!!!
10611062 /* 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]
cover locking fixes for cx88
Hi,
Huber Andreas wrote[1]:
Quoted 1 line
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 cx88: protect per-device driver list with device lock
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.
--- 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(-)
drivers/media/video/cx88/cx88-blackbird.c +7 -1
@@ -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);
drivers/media/video/cx88/cx88-dvb.c +8 -0
@@ -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;
drivers/media/video/cx88/cx88-mpeg.c +7 -4
@@ -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 ;
drivers/media/video/cx88/cx88.h +8 -1
@@ -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);/* ----------------------------------------------------------- */
Signature · 1 line
1.7.5.rc0
PATCH 2/3 cx88: fix locking of sub-driver operations
In reply to Jonathan NiederFrom: 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, andadvise_release: all require the caller to hold the device lock now.
[jn: split from a larger patch, with new commit message]
--- 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(-)
drivers/media/video/cx88/cx88-blackbird.c +2 -7
@@ -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 ; }
drivers/media/video/cx88/cx88-dvb.c +1 -7
@@ -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; }
drivers/media/video/cx88/cx88-mpeg.c +0 -4
@@ -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 ; }
drivers/media/video/cx88/cx88.h +2 -1
@@ -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 */
Signature · 1 line
1.7.5.rc0
PATCH 3/3 cx88: use a mutex to protect cx8802_devlist
In reply to Jonathan NiederFrom: 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]
---
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(-)
drivers/media/video/cx88/cx88-mpeg.c +17 -3
@@ -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);
Signature · 1 line
1.7.5.rc0
A message that is not in the archive (sent off-list or not yet received): [email protected]
Re: [PATCH 3/3] [media] cx88: use a mutex to protect cx8802_devlist
Hi Andreas,
(please turn off HTML mail.)
Quoted 3 lines · Andreas Huber
There is a reference count bug in the driver code. The driver's
active_refcount 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
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.
Quoted 32 lines · Jonathan Nieder
Hi Andreas,
(please turn off HTML mail.)
Quoted 3 lines · Andreas Huber
There is a reference count bug in the driver code. The driver's
active_refcount may become negative which leads to unpredictable behavior. (mpeg video device inaccessible, etc ...)Hmm, the patchset didn't touch
active_refhandling.
active_refwas 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_acquireare balancedwith calls to cx8802_driver::request_release;*
cx8802_driver::advise_acquireis non-null if and only ifcx8802_driver::advise_releaseis (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 toactive_refwas previously protected by the BKL and now it is protected bycore->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_refbecomes negative)Thanks for reporting.
Jonathan
Re: [PATCH 3/3] [media] cx88: use a mutex to protect cx8802_devlist
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
Quoted 38 lines · 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.Quoted 32 lines · Jonathan Nieder
Hi Andreas,
(please turn off HTML mail.)
Quoted 3 lines · Andreas Huber
There is a reference count bug in the driver code. The driver's
active_refcount may become negative which leads to unpredictable behavior. (mpeg video device inaccessible, etc ...)Hmm, the patchset didn't touch
active_refhandling.
active_refwas 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_acquireare balancedwith calls to cx8802_driver::request_release;*
cx8802_driver::advise_acquireis non-null if and only ifcx8802_driver::advise_releaseis (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 toactive_refwas previously protected by the BKL and now it is protected bycore->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_refbecomes negative)Thanks for reporting.
Jonathan
Re: [PATCH 3/3] [media] cx88: use a mutex to protect cx8802_devlist
Hi again,
Quoted 4 lines · Andreas Huber
[...]
/* 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
--- 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(-)
drivers/media/video/cx88/cx88-blackbird.c +0 -2
@@ -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 ;
drivers/media/video/cx88/cx88-mpeg.c +2 -3
@@ -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 ;
drivers/media/video/cx88/cx88.h +2 -5
@@ -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 */
Signature · 1 line
1.7.5.rc0
Re: [RFC/PATCH 0/3] locking fixes for cx88
In reply to Jonathan NiederQuoted 16 lines · Jonathan Nieder <jrnieder <at> gmail.com> writes:
Hi,
Huber Andreas wrote[1]:
Quoted 1 line
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 protectcx8802_devlist
Quoted 9 lines
Jonathan Nieder (1):
[media] cx88: protect per-device driver list with device lockdrivers/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.cb/drivers/media/video/cx88/cx88-mpeg.c
drivers/media/video/cx88/cx88-mpeg.c +1 -0
@@ -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
In reply to Jonathan NiederQuoted 10 lines · Jonathan Nieder
Hi,
Huber Andreas wrote[1]:
Quoted 1 line
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.
Quoted 13 lines
Ben Hutchings (2):
[media] cx88: fix locking of sub-driver operations [media] cx88: use a mutex to protectcx8802_devlistJonathan Nieder (1):
[media] cx88: protect per-device driver list with device lockdrivers/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(-)
Signature · 2 lines
Ben Hutchings Once a job is fouled up, anything done to improve it makes it worse.
Re: [RFC/PATCH 0/3] locking fixes for cx88
Quoted 3 lines · Ben Hutchings
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.
v2 cover locking fixes for cx88
In reply to Jonathan NiederHi again,
Quoted 8 lines · Jonathan Nieder
Huber Andreas wrote[1]:
Quoted 1 line
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 v2 1/7 cx88: protect per-device driver list with device lock
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]>.
--- 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(-)
drivers/media/video/cx88/cx88-blackbird.c +2 -1
@@ -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);
drivers/media/video/cx88/cx88-dvb.c +3 -0
@@ -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;
drivers/media/video/cx88/cx88-mpeg.c +7 -4
@@ -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 ;
drivers/media/video/cx88/cx88.h +8 -1
@@ -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);/* ----------------------------------------------------------- */
Signature · 1 line
1.7.5.rc0
PATCH v2 2/7 cx88: fix locking of sub-driver operations
In reply to Jonathan NiederThe 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]>.
--- 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(-)
drivers/media/video/cx88/cx88-blackbird.c +2 -2
@@ -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 ; }
drivers/media/video/cx88/cx88-dvb.c +1 -2
@@ -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; }
drivers/media/video/cx88/cx88-mpeg.c +0 -4
@@ -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 ; }
drivers/media/video/cx88/cx88.h +2 -1
@@ -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 */
Signature · 1 line
1.7.5.rc0
PATCH v2 3/7 cx88: hold device lock during sub-driver initialization
In reply to Jonathan Niedercx8802_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.
--- 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(-)
drivers/media/video/cx88/cx88-blackbird.c +0 -2
@@ -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 ;
drivers/media/video/cx88/cx88-mpeg.c +2 -3
@@ -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 ;
drivers/media/video/cx88/cx88.h +2 -5
@@ -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 */
Signature · 1 line
1.7.5.rc0
PATCH v2 4/7 cx88: use a mutex to protect cx8802_devlist
In reply to Jonathan NiederAdd 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]>.
--- drivers/media/video/cx88/cx88-mpeg.c | 20 +++++++++++++++++--- 1 files changed, 17 insertions(+), 3 deletions(-)
drivers/media/video/cx88/cx88-mpeg.c +17 -3
@@ -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);
Signature · 1 line
1.7.5.rc0
PATCH v2 5/7 cx88: gracefully reject attempts to use unregistered cx88-blackbird driver
In reply to Jonathan NiederIt 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.
--- drivers/media/video/cx88/cx88-blackbird.c | 25 ++++++++++++++----------- 1 files changed, 14 insertions(+), 11 deletions(-)
drivers/media/video/cx88/cx88-blackbird.c +14 -11
@@ -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);
Signature · 1 line
1.7.5.rc0
PATCH v2 6/7 cx88: don't use atomic_t for core->mpeg_users
In reply to Jonathan Niedermpeg_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.
--- drivers/media/video/cx88/cx88-blackbird.c | 11 ++++++----- drivers/media/video/cx88/cx88.h | 2 +- 2 files changed, 7 insertions(+), 6 deletions(-)
drivers/media/video/cx88/cx88-blackbird.c +6 -5
@@ -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);
drivers/media/video/cx88/cx88.h +1 -1
@@ -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;
Signature · 1 line
1.7.5.rc0
PATCH v2 7/7 cx88: don't use atomic_t for core->users
In reply to Jonathan Niederusers is always read or written with core->lock held. A plain int is simpler and faster.
---
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(-)
drivers/media/video/cx88/cx88-video.c +3 -2
@@ -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);
drivers/media/video/cx88/cx88.h +1 -1
@@ -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. */
Signature · 1 line
1.7.5.rc0
Re: [PATCH 7/7] [mpeg] cx88: don't use atomic_t for core->users
Quoted 1 line · Jonathan Nieder
[Subject: [PATCH 7/7] [mpeg] cx88: don't use
atomic_tforcore->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
In reply to Jonathan NiederQuoted 9 lines · Jonathan Nieder
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 protectcx8802_devlist[media] cx88: handle attempts to use unregistered cx88-blackbird
driver
[media] cx88: don't useatomic_tforcore->mpeg_users[media] cx88: don't useatomic_tforcore->users
Good news: Andreas Huber <[email protected]> tested on a PC with 2 Hauppauge HVR1300 TV cards.
Quoted 8 lines · He
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!