[linux-dvb] [PATCH] DVB: remove bogus BUG_ON in videobuf_dvb_thread
Since videobuf_waiton is called with intr=1, it can return -EINTR and therefore err may be non-zero. This happens when the system goes into the standby state. Without the BUG() occurring, there's no problem with standby mode while DVB is being used. --- int videobuf_waiton(struct videobuf_buffer *vb, int non_blocking, int intr); [ 224.561803] Stopping tasks ... 0[ cut here ] [ 224.712939] kernel BUG at drivers/media/video/video-buf-dvb.c:59! [ 224.712951] invalid opcode: [#1] [ 224.712957] PREEMPT [ 224.712969] Modules linked in: [ 224.712985] CPU:0 [ 224.712987] EIP:0060:[b03a0740]Not tainted VLI [ 224.712989] EFLAGS: 00010286 (2.6.23-rc2-git #282) [ 224.713019] EIP is at videobuf_dvb_thread+0x160/0x170 [ 224.713028] eax: fffc ebx: 0282 ecx: edx: [ 224.713038] esi: eeda63c0 edi: b1a1fa3c ebp: ea82bfcc esp: ea82bfb0 [ 224.713046] ds: 007b es: 007b fs: gs: ss: 0068 [ 224.713055] Process saa7134[0] dvb (pid: 6649, ti=ea82a000 task=eb1d5550 task.ti=ea82a000) [ 224.713064] Stack: 0009 b011ec68 b1a1fa44 b1a1fb28 b1a1fa3c b03a05e0 fffc ea82bfe0 [ 224.713103]b0135dec b0135d90 b0104e57 ea82fd70 [ 224.713140] 09010df7 862a0510 [ 224.713170] Call Trace: [ 224.713178] [b0104fca] show_trace_log_lvl+0x1a/0x30 [ 224.713206] [b010508b] show_stack_log_lvl+0x8b/0xb0 [ 224.713220] [b01052d3] show_registers+0x1c3/0x320 [ 224.713235] [b01055af] die+0xff/0x210 [ 224.713248] [b0105751] do_trap+0x91/0xd0 [ 224.713261] [b01059f8] do_invalid_op+0x88/0xa0 [ 224.713274] [b052206a] error_code+0x6a/0x70 [ 224.713296] [b0135dec] kthread+0x5c/0x90 [ 224.713313] [b0104e57] kernel_thread_helper+0x7/0x10 [ 224.713327] === [ 224.713335] Code: 8b 00 c7 04 24 c8 21 63 b0 89 44 24 04 e8 c9 29 d8 ff e9 cd fe ff ff 8b 07 c7 04 24 e8 21 63 b0 89 44 24 04 e8 b2 29 d8 ff eb c2 0f 0b eb fe 8d b6 00 00 00 00 8d bf 00 00 00 00 55 ba ea ff ff [ 224.713590] EIP: [b03a0740] videobuf_dvb_thread+0x160/0x170 SS:ESP 0068:ea82bfb0 [ 224.713893] done. drivers/media/video/video-buf-dvb.c |1 - 1 files changed, 0 insertions(+), 1 deletions(-) diff --git a/drivers/media/video/video-buf-dvb.c b/drivers/media/video/video-buf-dvb.c index e617925..d2af82d 100644 --- a/drivers/media/video/video-buf-dvb.c +++ b/drivers/media/video/video-buf-dvb.c @@ -56,7 +56,6 @@ static int videobuf_dvb_thread(void *data) struct videobuf_buffer, stream); list_del(buf-stream); err = videobuf_waiton(buf,0,1); - BUG_ON(0 != err); /* no more feeds left or stop_feed() asked us to quit */ if (0 == dvb-nfeeds) -- 1.5.0.1 -- Simon Arlott ___ linux-dvb mailing list linux-dvb@linuxtv.org http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
Re: [linux-dvb] Uwe
On 18/05/07 01:40, Allan Stirling wrote: Uwe Bugla wrote: more and more personal attacks Is there any way to disallow someone from posting to the mailing list, or does everyone have to add Uwe to their own killfile? It's possible, whether it happens or not is up to the list maintainer. Other mailing lists have no problem banning him after the first incident (e.g. MPlayer). -- Simon Arlott ___ linux-dvb mailing list linux-dvb@linuxtv.org http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
[linux-dvb] Re: 2.6.21-mm1
On 06/05/07 21:08, Andrew Morton wrote:
On Sun, 06 May 2007 15:59:53 +0100 Simon Arlott [EMAIL PROTECTED] wrote:
On 05/05/07 09:49, Andrew Morton wrote:
ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.21/2.6.21-mm1/
I'm currently in the middle of a bisect over the last week of commits to linus'
Don't accidentally switch from SLAB to SLOB.
tree, but I got the following with -mm1 that isn't showing up in the latest
2.6.21-git:
[ 15.644778] BUG: sleeping function called from invalid context at
kernel/mutex.c:86
[ 15.644873] in_atomic():1, irqs_disabled():1
[ 15.644882] 2 locks held by modprobe/2765:
[ 15.644889] #0: (devlist_lock){--..}, at: [b052dc7f] mutex_lock+0x1f/0x30
[ 15.645058] #1: (modlist_lock){}, at: [b0147b1d]
__symbol_get+0x1d/0x90
[ 15.645207] irq event stamp: 11772
[ 15.645271] hardirqs last enabled at (11771): [b016d793]
slab_free+0xc3/0x200
[ 15.645289] hardirqs last disabled at (11772): [b052f476]
_spin_lock_irqsave+0x16/0x60
[ 15.645364] softirqs last enabled at (11216): [b0125286]
__do_softirq+0x96/0xb0
[ 15.645439] softirqs last disabled at (11211): [b01069f2]
do_softirq+0x82/0x100
[ 15.645517] [b0104f0a] show_trace_log_lvl+0x1a/0x30
[ 15.645586] [b0104f32] show_trace+0x12/0x20
[ 15.645654] [b0105045] dump_stack+0x15/0x20
[ 15.645665] [b011d6ad] __might_sleep+0xcd/0xf0
[ 15.645737] [b052dc78] mutex_lock+0x18/0x30
[ 15.645806] [b01b1b7c] sysfs_create_link+0x6c/0x130
[ 15.645823] [b0146dba] use_module+0x11a/0x170
[ 15.645892] [b0147b6c] __symbol_get+0x6c/0x90
[ 15.645904] [f086ac2c] dvb_init+0x98c/0xd70 [saa7134_dvb]
[ 15.645984] [b036861d] mpeg_ops_attach+0x3d/0x50
[ 15.646058] [b0369109] saa7134_ts_register+0x29/0x70
[ 15.646070] [f086f010] dvb_register+0x10/0x12 [saa7134_dvb]
[ 15.646141] [b01491b7] sys_init_module+0xf7/0x150
[ 15.646153] [b0104172] sysenter_past_esp+0x5f/0x99
[ 15.646221] ===
Looks like a locking error in the DVB code.
And this fantastic Oops:
[ 40.965119] BUG: unable to handle kernel NULL pointer dereference at virtual
address 0080
[ 40.965252] printing eip:
[ 40.965252] b016d4a5
[ 40.965300] *pde =
[ 40.965352] Oops: [#1]
[ 40.965467] PREEMPT
It occurs when racoon is started so must be IPSEC related, I'll have more information
once I finish bisecting and merge -mm1 again.
Thanks.
Unfortunately I can't reproduce either of these anymore...
--
Simon Arlott
___
linux-dvb mailing list
linux-dvb@linuxtv.org
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
Re: [linux-dvb] [PATCH] dvb-core: Handle failures to create devices
On 06/05/07 23:50, Trent Piepho wrote: I've tested this and can confirm it works. dvb_class will be set too late without the change to subsys_initcall. On Tue, 1 May 2007, Simon Arlott wrote: dvb-core is not started early enough when device drivers that use dvb are compiled in so device_register_device fails (silently) since dvb_class is ^dvb_register_device NULL, this runs dvb_init using subsys_initcall instead of module_init. dvb_register_device will now check the return value of class_device_create. All the printks had missing level prefixes so I've fixed these too. Probably better to make this a separate patch, since it's not related. - printk (%s: could get find free device id...\n, __FUNCTION__); + printk(KERN_ERR %s: could get find free device id...\n, __FUNCTION__); couldn't find free device id If it's ok with you, I'll import your patch as two seperate patches with the spelling errors fixed? That's fine with me. + dprintk(KERN_DEBUG DVB: register adapter%d/%s%d @ minor: %i (0x%02x)\n, adap-num, dnames[type], id, nums2minor(adap-num, type, id), nums2minor(adap-num, type, id)); The dvb-core dvbdev_debug parameter does nothing but turn on this one single dprintk. I'm tempted to just delete it. Just always output that information, or remove it? (I have it enabled, although I'm not sure if there's any point). -- Simon Arlott ___ linux-dvb mailing list linux-dvb@linuxtv.org http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
Re: [linux-dvb] DST/BT878 module customization (.. was: Critical points about ...)
On 30/04/07 22:17, Markus Rechberger wrote: Trent Piepho wrote another patch for it, it just completes Uwe's patch in the end. From my side I do not see any problem with that patch, if someone else has a problem with it please state out the reason. I have no problem with the patch since it has nothing to do with my DVB card but you're only encouraging Uwe to be abusive since it seems to help get him what he wants: On 01/05/07 00:05, Uwe Bugla wrote: Piepho, you are a devil, and your links do not work at all! Uwe On 01/05/07 00:40, Uwe Bugla wrote: Go to hell, Manuel Abraham, and do not return at all to absolutely no thinkable condition at all, and never come back to this place once more again Just goto hell, you goddamn deeply asocial miserable sonofabitch!! Uwe On 4/30/07, Markus Rechberger [EMAIL PROTECTED] wrote: it's enough, I told him that I'll look at it and try to get some other people involved if it really breaks something it should get stated out; and I'll refuse any further help if he starts to write any more abusive mail. It's not working. Patches should still be applied on the basis of what they do and how, not why they were made, of course. -- Simon Arlott ___ linux-dvb mailing list linux-dvb@linuxtv.org http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
[linux-dvb] [PATCH] dvb-core: Handle failures to create devices
dvb-core is not started early enough when device drivers that use dvb are
compiled in so device_register_device fails (silently) since dvb_class is
NULL, this runs dvb_init using subsys_initclass instead of module_init.
dvb_register_device will now check the return value of class_device_create.
Signed-off-by: Simon Arlott [EMAIL PROTECTED]
---
drivers/media/dvb/dvb-core/dvbdev.c | 11 +--
1 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/media/dvb/dvb-core/dvbdev.c
b/drivers/media/dvb/dvb-core/dvbdev.c
index 7683983..b859a60 100644
--- a/drivers/media/dvb/dvb-core/dvbdev.c
+++ b/drivers/media/dvb/dvb-core/dvbdev.c
@@ -201,6 +201,7 @@ int dvb_register_device(struct dvb_adapter *adap, struct
dvb_device **pdvbdev,
struct dvb_device *dvbdev;
struct file_operations *dvbdevfops;
+ struct class_device *clsdev;
int id;
mutex_lock(dvbdev_register_lock);
@@ -242,9 +243,15 @@ int dvb_register_device(struct dvb_adapter *adap, struct
dvb_device **pdvbdev,
mutex_unlock(dvbdev_register_lock);
- class_device_create(dvb_class, NULL, MKDEV(DVB_MAJOR,
nums2minor(adap-num, type, id)),
+ clsdev = class_device_create(dvb_class, NULL, MKDEV(DVB_MAJOR,
nums2minor(adap-num, type, id)),
adap-device, dvb%d.%s%d, adap-num,
dnames[type], id);
+ if (IS_ERR(clsdev)) {
+ printk(%s: failed to create device dvb%d.%s%d (%ld)\n,
__FUNCTION__,
+ adap-num, dnames[type], id, PTR_ERR(clsdev));
+ return PTR_ERR(clsdev);
+ }
+
dprintk(DVB: register adapter%d/%s%d @ minor: %i (0x%02x)\n,
adap-num, dnames[type], id, nums2minor(adap-num, type, id),
nums2minor(adap-num, type, id));
@@ -431,7 +438,7 @@ static void __exit exit_dvbdev(void)
unregister_chrdev_region(MKDEV(DVB_MAJOR, 0), MAX_DVB_MINORS);
}
-module_init(init_dvbdev);
+subsys_initcall(init_dvbdev);
module_exit(exit_dvbdev);
MODULE_DESCRIPTION(DVB Core Driver);
--
1.5.0.1
--
Simon Arlott
___
linux-dvb mailing list
linux-dvb@linuxtv.org
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
[linux-dvb] Re: [PATCH] dvb-core: Handle failures to create devices
On 01/05/07 18:12, Simon Arlott wrote: dvb_class is NULL, this runs dvb_init using subsys_initclass instead of module_init. That should of course be subsystem_initCALL not initclass. -- Simon Arlott ___ linux-dvb mailing list linux-dvb@linuxtv.org http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
Re: [linux-dvb] Re: DST/BT878 module customization (.. was: Critical points about ...)
On 01/05/07 19:30, Uwe Bugla wrote: fewer rude comments than usual If you would avoid making inflammatory comments, the people who are trying to help you will be more inclined to fix the problems that these patches DO cause so they can be added. A while ago, you went on and on about your broken floppy drive and how people shouldn't do things that break other systems - even if they have no idea that was happening - yet here you are now demanding people do the same thing. It's easy to maintain your own changes to the kernel until equivalent changes are committed - you need to be much more patient, especially when something is being done about them. -- Simon Arlott ___ linux-dvb mailing list linux-dvb@linuxtv.org http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
Re: [linux-dvb] Re: DST/BT878 module customization (.. was: Critical points about ...)
On 01/05/07 20:34, Uwe Bugla wrote: Original-Nachricht Datum: Tue, 01 May 2007 19:50:38 +0100 Von: Simon Arlott [EMAIL PROTECTED] On 01/05/07 19:30, Uwe Bugla wrote: fewer rude comments than usual If you would avoid making inflammatory comments, the people who are trying to help you will be more inclined to fix the problems that these patches DO cause so they can be added. A while ago, you went on and on about your broken floppy drive and how people shouldn't do things that break other systems - even if they have no idea that was happening - yet here you are now demanding people do the same thing. WRONG! I offered a patch for the broken floppy, but Linus was faster and ripped the whole section out, so do not even try to misunderstand or quote me out of context! You complained that changes which break your system[1] shouldn't be added and now rudely demand your desired changes which have been shown to break others' should be added immediately. I don't believe I misunderstood you or got the wrong context - it has nothing to do with the related patch. [1] http://lkml.org/lkml/2007/2/25/127 So I say it again: The theses Mr. Chehab invents NOT TO DO SOMETHING do never carry fully transparent information with them. It's just air bubbles that he is producing, nothing else! It's always and ever the other part that is exposed to offer transparent info, but never Chehab himself! And it's exactly the same thing with Abraham or Krufky! And does that behaviour conform with democratic terms? NO WAY! So there are three mismatches around: Their names are: Abraham, Krufky, and Chehab! You're worse than that reiser4 fanatic, at least he wasn't rude when he repeated himself over and over and over. I was going to offer to help you bisect between -git1 and -git2 (it'd be trivial to run the bisect here and provide patches to test) but I won't bother if you're going to hijack every email with your abuse. -- Simon Arlott ___ linux-dvb mailing list linux-dvb@linuxtv.org http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
Re: [linux-dvb] [PATCH] dvb-core: Handle failures to create devices
dvb-core is not started early enough when device drivers that use dvb are
compiled in so device_register_device fails (silently) since dvb_class is
NULL, this runs dvb_init using subsys_initcall instead of module_init.
dvb_register_device will now check the return value of class_device_create.
All the printks had missing level prefixes so I've fixed these too.
Signed-off-by: Simon Arlott [EMAIL PROTECTED]
---
On 01/05/07 22:21, Trent Piepho wrote:
On Tue, 1 May 2007, Simon Arlott wrote:
dvb-core is not started early enough when device drivers that use dvb are
compiled in so device_register_device fails (silently) since dvb_class is
NULL, this runs dvb_init using subsys_initclass instead of module_init.
dvb_register_device will now check the return value of class_device_create.
Good catch.
Thanks, it's only been 11 months since I first pointed it out.
+ printk(%s: failed to create device dvb%d.%s%d (%ld)\n,
__FUNCTION__,
+ adap-num, dnames[type], id, PTR_ERR(clsdev));
+ return PTR_ERR(clsdev);
printk(KERN_ERR
I copied another printk from that file without thinking.
drivers/media/dvb/dvb-core/dvbdev.c | 21 ++---
1 files changed, 14 insertions(+), 7 deletions(-)
diff --git a/drivers/media/dvb/dvb-core/dvbdev.c
b/drivers/media/dvb/dvb-core/dvbdev.c
index 7683983..297da62 100644
--- a/drivers/media/dvb/dvb-core/dvbdev.c
+++ b/drivers/media/dvb/dvb-core/dvbdev.c
@@ -201,6 +201,7 @@ int dvb_register_device(struct dvb_adapter *adap, struct
dvb_device **pdvbdev,
struct dvb_device *dvbdev;
struct file_operations *dvbdevfops;
+ struct class_device *clsdev;
int id;
mutex_lock(dvbdev_register_lock);
@@ -208,7 +209,7 @@ int dvb_register_device(struct dvb_adapter *adap, struct
dvb_device **pdvbdev,
if ((id = dvbdev_get_free_id (adap, type)) 0){
mutex_unlock(dvbdev_register_lock);
*pdvbdev = NULL;
- printk (%s: could get find free device id...\n, __FUNCTION__);
+ printk(KERN_ERR %s: could get find free device id...\n,
__FUNCTION__);
return -ENFILE;
}
@@ -242,10 +243,16 @@ int dvb_register_device(struct dvb_adapter *adap, struct
dvb_device **pdvbdev,
mutex_unlock(dvbdev_register_lock);
- class_device_create(dvb_class, NULL, MKDEV(DVB_MAJOR,
nums2minor(adap-num, type, id)),
+ clsdev = class_device_create(dvb_class, NULL, MKDEV(DVB_MAJOR,
nums2minor(adap-num, type, id)),
adap-device, dvb%d.%s%d, adap-num,
dnames[type], id);
- dprintk(DVB: register adapter%d/%s%d @ minor: %i (0x%02x)\n,
+ if (IS_ERR(clsdev)) {
+ printk(KERN_ERR %s: failed to create device dvb%d.%s%d
(%ld)\n, __FUNCTION__,
+ adap-num, dnames[type], id, PTR_ERR(clsdev));
+ return PTR_ERR(clsdev);
+ }
+
+ dprintk(KERN_DEBUG DVB: register adapter%d/%s%d @ minor: %i
(0x%02x)\n,
adap-num, dnames[type], id, nums2minor(adap-num, type, id),
nums2minor(adap-num, type, id));
@@ -304,7 +311,7 @@ int dvb_register_adapter(struct dvb_adapter *adap, const
char *name, struct modu
memset (adap, 0, sizeof(struct dvb_adapter));
INIT_LIST_HEAD (adap-device_list);
- printk (DVB: registering new adapter (%s).\n, name);
+ printk(KERN_INFO DVB: registering new adapter (%s).\n, name);
adap-num = num;
adap-name = name;
@@ -400,13 +407,13 @@ static int __init init_dvbdev(void)
dev_t dev = MKDEV(DVB_MAJOR, 0);
if ((retval = register_chrdev_region(dev, MAX_DVB_MINORS, DVB)) != 0)
{
- printk(dvb-core: unable to get major %d\n, DVB_MAJOR);
+ printk(KERN_ERR dvb-core: unable to get major %d\n,
DVB_MAJOR);
return retval;
}
cdev_init(dvb_device_cdev, dvb_device_fops);
if ((retval = cdev_add(dvb_device_cdev, dev, MAX_DVB_MINORS)) != 0) {
- printk(dvb-core: unable to get major %d\n, DVB_MAJOR);
+ printk(KERN_ERR dvb-core: unable to get major %d\n,
DVB_MAJOR);
goto error;
}
@@ -431,7 +438,7 @@ static void __exit exit_dvbdev(void)
unregister_chrdev_region(MKDEV(DVB_MAJOR, 0), MAX_DVB_MINORS);
}
-module_init(init_dvbdev);
+subsys_initcall(init_dvbdev);
module_exit(exit_dvbdev);
MODULE_DESCRIPTION(DVB Core Driver);
--
1.5.0.1
--
Simon Arlott
09F911029D74E35BD84156C5635688C0
___
linux-dvb mailing list
linux-dvb@linuxtv.org
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
Re: [linux-dvb] Problem with keys repeated
On Tue, March 6, 2007 12:20, Dom H wrote: Event: time 1172928613.397507, type 1 (Key), code 108 (Down), value 1 Key Down pressed Event: time 1172928613.649279, type 1 (Key), code 108 (Down), value 2 Event: time 1172928613.685270, type 1 (Key), code 108 (Down), value 2 Event: time 1172928613.721272, type 1 (Key), code 108 (Down), value 2 Event: time 1172928613.757279, type 1 (Key), code 108 (Down), value 2 Key Down repeated Event: time 1172928613.763489, type 1 (Key), code 108 (Down), value 0 Key Down released Any idea how to rectify this? I've tried different LIRC options with no success. Are there any options for the driver that would help to ignore the repeated codes? Try updating LIRC too? -- Simon Arlott ___ linux-dvb mailing list linux-dvb@linuxtv.org http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
Re: [linux-dvb] [PATCH] dvb-core: Fix several locking related problems.
On Mon, March 5, 2007 11:19, Johannes Stezenbach wrote: On Mon, Mar 05, 2007 at 01:58:14AM +0100, Oliver Endriss wrote: Simon Arlott wrote: Is any part of the patch going to be applied? I mentioned this problem in September last year and it looks like it's existed for years (the semaphore locking did the same thing). Well, I hoped that someone more familiar with the demuxer stuff would comment on the patch. I am not very happy about using non-interruptible lock operations... Why? If there are deadlocks these should be fixed, not ignored. Anyway, I'll apply the patch to HG master if you submit an updated patch: - Please add a line of comment to each mutex_lock() stating _why_ the non-interruptible lock has to be used at this place. What's the point of doing that? IMHO using mutex_lock_interruptible() is almost always wrong. The only use it has in dvb-core is to recover from locking bugs -- if it deadlocks you can Ctrl-C out of it (instead of being left with a non-killable program - reboot). This is what lockdep is for. But with mutex_lock_interruptible() it's easy to introduce signal handling bugs, which Simon confirmed to exist. It's also easy to find examples of people needing to rmmod/modprobe because dvr0 started returning -EBUSY on open() after they closed something. Fixing those up without reverting to mutex_lock() way might be possible, but is more difficult. It'd introduce lot of unneccessary -ERESTARTSYS, just to avoid the possiblity of waiting on mutex_lock for a few msecs. -- Simon Arlott ___ linux-dvb mailing list linux-dvb@linuxtv.org http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
Re: [linux-dvb] [PATCH] dvb-core: Fix several locking related problems.
On 05/03/07 17:19, Oliver Endriss wrote: Simon Arlott wrote: On Mon, March 5, 2007 11:19, Johannes Stezenbach wrote: On Mon, Mar 05, 2007 at 01:58:14AM +0100, Oliver Endriss wrote: Simon Arlott wrote: Is any part of the patch going to be applied? I mentioned this problem in September last year and it looks like it's existed for years (the semaphore locking did the same thing). Well, I hoped that someone more familiar with the demuxer stuff would comment on the patch. I am not very happy about using non-interruptible lock operations... Why? If there are deadlocks these should be fixed, not ignored. Yes, they should be fixed, but they should not require a reboot. What!? The fix for a deadlock is not a reboot... perhaps you misunderstood what I meant - whatever ends up holding the lock forever should be fixed. From 'Linux Device Drivers' (replace 'down' by 'mutex_lock'): | ... | down decrements the value of the semaphore and waits as long as need | be. down_ interruptible does the same, but the operation is | interruptible. The interruptible version is almost always the one you | will want; it allows a user-space process that is waiting on a | semaphore to be interrupted by the user. You do not, as a general | rule, want to use noninterruptible operations unless there truly is no ^^ | alternative. Noninterruptible operations are a good way to create ^^^ | unkillable processes (the dreaded D state seen in ps), and annoy ^^ | your users. Using down_interruptible requires some extra care, ^^ I don't see this advice in Documentation/mutex-design.txt (although it doesn't advise either way) or in Documentation/ at all. ^ | however, if the operation is interrupted, the function returns a | nonzero value, and the caller does not hold the semaphore. Proper use | of down_interruptible requires always checking the return value and ^ Until you do this, you can't use down_interruptible. | responding accordingly. | ... Anyway, I'll apply the patch to HG master if you submit an updated patch: - Please add a line of comment to each mutex_lock() stating _why_ the non-interruptible lock has to be used at this place. What's the point of doing that? The point is to document why we do not use the interruptible version. Next year someone might submit a patch replacing mutex_lock by mutex_lock_interruptible, and no one remembers why mutex_lock is required at this place. There is no danger of this, if someone submits a patch replacing it with mutex_lock_interruptible and doesn't take into account every possible calling function's check of the return value then their patch needs changing before it can be accepted. IMHO using mutex_lock_interruptible() is almost always wrong. The only use it has in dvb-core is to recover from locking bugs -- if it deadlocks you can Ctrl-C out of it (instead of being left with a non-killable program - reboot). The key phrase here is locking *bugs*, if the code can lock forever it needs to be fixed. The real cause of bugs will never be found if interruptible locking is used everywhere when when it's not necessary :( Also, this is in dvb-core not actual card drivers - why would there be locking bugs in dvb-core itself? This is what lockdep is for. Is lockdep activated in distribution kernels? No, but developers could use it while testing. It only reports locking bugs by checking how locks depend on other locks (see lockdep-design.txt), I don't think it would spot code that just locked a mutex and then waited forever (which is really bad code anyway). -- Simon Arlott ___ linux-dvb mailing list linux-dvb@linuxtv.org http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
Re: [linux-dvb] [PATCH] dvb-core: Fix several locking related problems.
Is any part of the patch going to be applied? I mentioned this problem in
September last year and it looks like it's existed for years (the semaphore
locking did the same thing).
On 24/02/07 19:03, Simon Arlott wrote:
On 24/02/07 18:48, Andreas Oberritter wrote:
Hi Simon,
Simon Arlott wrote:
@@ -673,13 +672,8 @@ static int dvb_demux_open(struct inode *
static int dvb_dmxdev_filter_free(struct dmxdev *dmxdev,
struct dmxdev_filter *dmxdevfilter)
{
- if (mutex_lock_interruptible(dmxdev-mutex))
- return -ERESTARTSYS;
-
- if (mutex_lock_interruptible(dmxdevfilter-mutex)) {
- mutex_unlock(dmxdev-mutex);
- return -ERESTARTSYS;
- }
+ mutex_lock(dmxdev-mutex);
+ mutex_lock_interruptible(dmxdevfilter-mutex);
Assuming that the rest of the patch is OK, shouldn't this be a
mutex_lock(), too, if the return value will be ignored?
Argh. Sorry, I accidentally reverted my changes and quickly went through them
all again so yes, it should have been mutex_lock(). I should now change that on
my running copy... which has somehow kept working.
Regards,
Andreas
--
Simon Arlott
signature.asc
Description: OpenPGP digital signature
___
linux-dvb mailing list
linux-dvb@linuxtv.org
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
Re: [linux-dvb] [PATCH] dvb-core: Fix several locking related problems.
On 24/02/07 18:48, Andreas Oberritter wrote:
Hi Simon,
Simon Arlott wrote:
@@ -673,13 +672,8 @@ static int dvb_demux_open(struct inode *
static int dvb_dmxdev_filter_free(struct dmxdev *dmxdev,
struct dmxdev_filter *dmxdevfilter)
{
-if (mutex_lock_interruptible(dmxdev-mutex))
-return -ERESTARTSYS;
-
-if (mutex_lock_interruptible(dmxdevfilter-mutex)) {
-mutex_unlock(dmxdev-mutex);
-return -ERESTARTSYS;
-}
+mutex_lock(dmxdev-mutex);
+mutex_lock_interruptible(dmxdevfilter-mutex);
Assuming that the rest of the patch is OK, shouldn't this be a
mutex_lock(), too, if the return value will be ignored?
Argh. Sorry, I accidentally reverted my changes and quickly went through them
all again so yes, it should have been mutex_lock(). I should now change that on
my running copy... which has somehow kept working.
Regards,
Andreas
___
linux-dvb mailing list
linux-dvb@linuxtv.org
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
--
Simon Arlott
signature.asc
Description: OpenPGP digital signature
___
linux-dvb mailing list
linux-dvb@linuxtv.org
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
[linux-dvb] [PATCH] dvb-core: Fix several locking related problems.
There are several instances of dvb-core functions using
mutex_lock_interruptible and returning -ERESTARTSYS where the calling function
will either never retry or never check the return value.
dmxdev.c:
dvb_dmxdev_filter_free (via dvb_demux_release):
dvb_dvr_release:
return value ignored
This causes a race condition with dvb_dmxdev_filter_free and dvb_dvr_release,
both of which are filesystem release functions whose return value is ignored
and will never be retried. When this happens it becomes impossible to open dvr0
again (-EBUSY) since it has not been released properly. I can only reproduce
this by ^C-ing dvbrecord (which causes the bug it every time).
[ 260.767599] dvb_dmxdev_filter_free: start
[ 260.767612] dvb_dmxdev_filter_free: lock1 (dmxdev)
[ 260.767615] dvb_dmxdev_filter_free lock2 (dmxdevfilter)
[ 260.767683] dvb_dmxdev_filter_free: unlock (dmxdevfilter, dmxdev)
[ 260.767690] dvb_dmxdev_filter_free: start
[ 260.767693] dvb_dmxdev_filter_free: lock1 (dmxdev)
[ 260.767696] dvb_dmxdev_filter_free lock2 (dmxdevfilter)
[ 260.767897] dvb_dvr_release: start
[ 260.767900] dvb_dvr_release: failed (dmxdev)
[ 260.784656] dvb_dmxdev_filter_free: unlock (dmxdevfilter, dmxdev)
With the changes to dmxdev.c in this patch, the following happens instead:
[ 1194.907503] dvb_dmxdev_filter_free: start
[ 1194.907517] dvb_dmxdev_filter_free: lock1 (dmxdev)
[ 1194.907520] dvb_dmxdev_filter_free lock2 (dmxdevfilter)
[ 1194.907551] dvb_dmxdev_filter_free: unlock (dmxdevfilter, dmxdev)
[ 1194.907557] dvb_dmxdev_filter_free: start
[ 1194.907560] dvb_dmxdev_filter_free: lock1 (dmxdev)
[ 1194.907563] dvb_dmxdev_filter_free lock2 (dmxdevfilter)
[ 1194.907650] dvb_dvr_release: start
[ 1194.922452] dvb_dmxdev_filter_free: unlock (dmxdevfilter, dmxdev)
[ 1194.922716] dvb_dvr_release: lock (dmxdev)
[ 1194.922885] dvb_dvr_release: unlock (dmxdev)
The following functions also do something similar, some of which may be
responsible for a bug I have which results in dvb breaking requiring a reboot:
[384577.489000] dvb_demux_feed_del: feed not in list (type=0 state=0 pid=)
ioctl(DMX_SET_PES_FILTER) for Video PID failed (Invalid argument)
Since this happens seemingly at random (but usually 1MB into recording a TS) I
can't tell if this fixes that bug or not until it doesn't happen again for a
very long time, but I can confirm that this causes no deadlocks because I have
CONFIG_LOCKDEP enabled.
dvb_demux.c:
dmx_section_feed_stop_filtering:
dmx_section_feed_release_filter:
dmx_ts_feed_stop_filtering:
dvbdmx_connect_frontend:
dvbdmx_disconnect_frontend:
dvbdmx_release_section_feed:
dvbdmx_release_ts_feed:
return value ignored
Most of these return values get ignored by dvb-core
code itself.
dvbdev.c:
dvb_register_device:
dvb_register_adapter:
dvb_unregister_adapter:
return value ignored
Is there some important reason for these to never block?
Signed-off-by: Simon Arlott [EMAIL PROTECTED]
---
drivers/media/dvb/dvb-core/dmxdev.c| 12 +++-
drivers/media/dvb/dvb-core/dvb_demux.c | 21 +++--
drivers/media/dvb/dvb-core/dvbdev.c|9 +++--
3 files changed, 13 insertions(+), 29 deletions(-)
diff --git a/drivers/media/dvb/dvb-core/dmxdev.c
b/drivers/media/dvb/dvb-core/dmxdev.c
index fc77de4..66baaa8 100644
--- a/drivers/media/dvb/dvb-core/dmxdev.c
+++ b/drivers/media/dvb/dvb-core/dmxdev.c
@@ -180,8 +180,7 @@ static int dvb_dvr_release(struct inode
struct dvb_device *dvbdev = file-private_data;
struct dmxdev *dmxdev = dvbdev-priv;
- if (mutex_lock_interruptible(dmxdev-mutex))
- return -ERESTARTSYS;
+ mutex_lock(dmxdev-mutex);
if ((file-f_flags O_ACCMODE) == O_WRONLY) {
dmxdev-demux-disconnect_frontend(dmxdev-demux);
@@ -673,13 +672,8 @@ static int dvb_demux_open(struct inode *
static int dvb_dmxdev_filter_free(struct dmxdev *dmxdev,
struct dmxdev_filter *dmxdevfilter)
{
- if (mutex_lock_interruptible(dmxdev-mutex))
- return -ERESTARTSYS;
-
- if (mutex_lock_interruptible(dmxdevfilter-mutex)) {
- mutex_unlock(dmxdev-mutex);
- return -ERESTARTSYS;
- }
+ mutex_lock(dmxdev-mutex);
+ mutex_lock_interruptible(dmxdevfilter-mutex);
dvb_dmxdev_filter_stop(dmxdevfilter);
dvb_dmxdev_filter_reset(dmxdevfilter);
diff --git a/drivers/media/dvb/dvb-core/dvb_demux.c
b/drivers/media/dvb/dvb-core/dvb_demux.c
index fcff5ea..6d8d1c3 100644
--- a/drivers/media/dvb/dvb-core/dvb_demux.c
+++ b/drivers/media/dvb/dvb-core/dvb_demux.c
@@ -673,8 +673,7 @@ static int dmx_ts_feed_stop_filtering(st
struct dvb_demux *demux = feed-demux;
int ret;
- if (mutex_lock_interruptible(demux-mutex
[linux-dvb] drivers/media/dvb/dvb-core/dmxdev.c: dvb_dvr_release
If I have a process open /dev/dvb/adapter0/dvr0 RDONLY and then kill it (^C), the release function sometimes fails because the lock is still held: if (mutex_lock_interruptible(dmxdev-mutex)) return -ERESTARTSYS; This means that the dvbdev-readers semaphore is never updated and any future attempts to open dvr0 will fail: [ 1103.961000] function : dvb_dvr_open [ 1106.54] function : dvb_dvr_release [ 1106.54] dvb_dvr_release : readers++ [ .718000] function : dvb_dvr_open [ 1115.80] function : dvb_dvr_release [ 1118.931000] function : dvb_dvr_open [ 1118.931000] dvb_dvr_open : BUSY (dvbdev-readers=0) Everything works fine if I replace the mutex_lock_interruptible check with: mutex_lock(dmxdev-mutex); [ 1339.409000] function : dvb_dvr_open [ 1343.711000] function : dvb_dvr_release [ 1343.727000] dvb_dvr_release : readers++ [ 1344.441000] function : dvb_dvr_open [ 1350.618000] function : dvb_dvr_release [ 1350.618000] dvb_dvr_release : readers++ [ 1351.302000] function : dvb_dvr_open [ 1356.017000] function : dvb_dvr_release [ 1356.034000] dvb_dvr_release : readers++ [ 1356.781000] function : dvb_dvr_open [ 1360.145000] function : dvb_dvr_release [ 1360.164000] dvb_dvr_release : readers++ [ 1360.837000] function : dvb_dvr_open [ 1363.773000] function : dvb_dvr_release [ 1363.773000] dvb_dvr_release : readers++ [ 1498.123000] function : dvb_dvr_open [ 1501.139000] function : dvb_dvr_release [ 1501.156000] dvb_dvr_release : readers++ [ 1501.856000] function : dvb_dvr_open [ 1503.41] function : dvb_dvr_release [ 1503.427000] dvb_dvr_release : readers++ [ 1503.923000] function : dvb_dvr_open [ 1511.609000] function : dvb_dvr_release [ 1511.627000] dvb_dvr_release : readers++ -- Simon Arlott signature.asc Description: OpenPGP digital signature ___ linux-dvb mailing list linux-dvb@linuxtv.org http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
