&#34;kill the Big Kernel Lock (BKL)&#34; tree

Quoted 5 lines · Ingo Molnar

Linus, Alan: the increased visibility and debuggability of the BKL already uncovered a rather serious regression in upstream -git. You might want to cherry pick this single fix, it will apply just fine to current -git:

Ok, so I'm obviously happy. This is exactly the kind of thing I would want to see.

That said, the way it is now set up, it's unreasonable to merge anything directly, and while I can cherry-pick obvious fixes this way, I do think we could do things better.

It should be possible to set things up so that it's a config option, and we can mark it EXPERIMENTAL but still merge it into the standard kernel, so that we'd have the debug stuff there. That would get a lot more coverage, especially if it all still *works*, even if the debug stuff then complains (ie it would be nicer if the lock itself didn't start breaking).

So for example, have CONFIG_DEBUG_BKL turn it into a mutex (and select mutex debugging), and get all the debug coverage that way, but then when somebody enters the scheduler with the lock held, first complain, but then auto-release it anyway. That way, bugs get found and complained about, but hopefully the machine still ends up working.

Linus

Ingo Molnar

author

2008-05-14 19:44 UTC #15 lore raw

Quoted 11 lines · Linus Torvalds

Quoted 4 lines
Linus, Alan: the increased visibility and debuggability of the BKL already uncovered a rather serious regression in upstream -git. You might want to cherry pick this single fix, it will apply just fine to current -git:
Ok, so I'm obviously happy. This is exactly the kind of thing I would want to see.
That said, the way it is now set up, it's unreasonable to merge anything directly, and while I can cherry-pick obvious fixes this way, I do think we could do things better.

yeah. This is just a first approximation. It might be v2.6.27 stuff, if it stabilizes fast enough.

Quoted 6 lines

It should be possible to set things up so that it's a config option, and we can mark it EXPERIMENTAL but still merge it into the standard kernel, so that we'd have the debug stuff there. That would get a lot more coverage, especially if it all still *works*, even if the debug stuff then complains (ie it would be nicer if the lock itself didn't start breaking).

yeah. Will try to reshape it like that.

Quoted 5 lines

So for example, have CONFIG_DEBUG_BKL turn it into a mutex (and select mutex debugging), and get all the debug coverage that way, but then when somebody enters the scheduler with the lock held, first complain, but then auto-release it anyway. That way, bugs get found and complained about, but hopefully the machine still ends up working.

hm, we'll got more ideas about other debug helpers, but i dont think warning in the scheduler is realistic or useful: lots and lots of code _does_ reschedule with the BKL held and always did - we never knew this before in a reliable way due to the auto-release. Sleeping locks that purely nest inside the BKL are the norm in the VFS, in the tty code and in most other places - they should be fine and are frequently taken in BKL sections (and frequently produce scheduling there).

As the BKL gets pushed inside subsystems, so do inner locks vanish from its scope - and at the final stage it can become a spinlock or mutex, depending on what the actual use is.

The main point with the mutex is to make the BKL _stricter_ and more defined - this hurts BKL using code more (see the many fixes that were needed), but it also makes things much more visible and much more fixable IMO. This tree turns the BKL into "just another mutex", with a tiny bit of self-recursion glue on top of it.

Btw., often there's potential scheduling at points where BKL using code does not expect it. So this series might also _fix_ some rare races.

The fact that this also makes BKL critical sections involuntarily preemptible is a side-effect (which is one of my main motivations to do this whole thing), and it's a pretty much unavoidable side-effect.

Also, turning it into a more or less simple mutex with no scheduler smarts at all, it all fits into our "how do we remove a serializing lock" workflow rather well. Even if for some piece of code not much changes in reality, it becomes more familar, less mystic and more trustable to fix and improve.

Btw., while i hacked on this today, i _think_ i've got most of the worst problems mapped out already. I needed two fixes to get it to boot to a ssh shell prompt without hanging. I needed 10 more fixes to solve all the dependencies that lockdep found. Another 5 fixes were exposed in more directed randconfig based testing in the second half of the day.

I've got a full desktop running on SMP on two boxes with lots of services enabled. There are three known problem areas:

- reiser3. I've got three patches for but they are not pretty - see them below. One of them widens BKL locking to the VFS. I'm not sure it's worth fixing - we could declare reiser3 legacy && make it depend on !DEBUG_BKL?

- NFS. Even with "remove the BKL: restructure NFS code" there's a lockdep splat when mounting NFS. Havent looked into it yet, Peter says it's hairy code.

- racy procfs dir entry creation methods. These will not result in outright hangs, but need to be reviewed then fixed or annotated away because they are potentially racy - they'll show up as WARN_ON()s in fs/proc/.

More will be found i'm sure, but also, about 80% of the fixes were not actual hangs but were proactive fixes based on lockdep warnings. Only 3 out of the ~17 fixes were hang-induced. So i think even the current early form of it is quite hackable and debuggable.

Ingo

---------------------
Subject: remove bkl: annotate reiserfs3
From: Ingo Molnar <[email protected]>
Date: Wed May 14 15:22:01 CEST 2008

reiserfs uses proc_create_data() with the BKL held;

WARNING: at fs/proc/generic.c:701 proc_create_data+0x33/0xc3()
Modules linked in:
Pid: 3193, comm: mount Not tainted 2.6.26-rc2-sched-devel.git #478
 [<c013d2ed>] warn_on_slowpath+0x41/0x6d
 [<c01571c0>] ? save_trace+0x37/0x8a
 [<c0157277>] ? add_lock_to_list+0x64/0x8a
 [<c01c7fea>] ? proc_register+0x2e/0x12e
 [<c04ae22c>] ? _spin_unlock+0x27/0x3c
 [<c01c811d>] proc_create_data+0x33/0xc3
 [<c01f6bd2>] add_file+0x23/0x2a
 [<c01f6c73>] ? show_version+0x0/0x3b
 [<c01f749a>] reiserfs_proc_info_init+0xab/0x136
 [<c01e4a3a>] reiserfs_fill_super+0xb97/0xc7d
 [<c02687f8>] ? vsnprintf+0x265/0x3fc
 [<c01cbd02>] ? disk_name+0x25/0x67
 [<c0195997>] get_sb_bdev+0xcd/0x10b
 [<c0190030>] ? cache_alloc_refill+0x53c/0x632
 [<c01a7609>] ? alloc_vfsmnt+0xe3/0x10b
 [<c01a7609>] ? alloc_vfsmnt+0xe3/0x10b
 [<c01e1f3d>] get_super_block+0x13/0x15
 [<c01e3ea3>] ? reiserfs_fill_super+0x0/0xc7d
 [<c019557f>] vfs_kern_mount+0x81/0xf7
 [<c0195639>] do_kern_mount+0x32/0xba
 [<c01a863d>] do_new_mount+0x46/0x74
 [<c01a8802>] do_mount+0x197/0x1b5
 [<c01586a7>] ? trace_hardirqs_on_caller+0xe0/0x115
 [<c04ace60>] ? mutex_lock_nested+0x222/0x22a
 [<c04ae4ab>] ? lock_kernel+0x1e/0x25
 [<c01a8884>] sys_mount+0x64/0x9b
 [<c0119a8a>] sysenter_past_esp+0x6a/0xa4
 =======================

but its use of proc_create_data() is safe here. Annotate that by dropping the BKL around the procfs ops.

Signed-off-by Ingo Molnar <[email protected]>

---
 fs/reiserfs/procfs.c |   12 ++++++++++++
 1 file changed, 12 insertions(+)

fs/reiserfs/procfs.c +12 -0

Index: linux/fs/reiserfs/procfs.c
===================================================================
@@ -494,6 +494,15 @@ int reiserfs_proc_info_init(struct super
 	spin_lock_init(&__PINFO(sb).lock);
 	REISERFS_SB(sb)->procdir = proc_mkdir(b, proc_info_root);
 	if (REISERFS_SB(sb)->procdir) {
+		int saved_lock_depth = current->lock_depth;
+
+		/*
+		 * This is in essence an annotation that tells procfs that
+		 * it is fine to call it with the BKL held (it causes
+		 * the kernel_locked() check to not trigger):
+		 */
+		current->lock_depth = -1;
+
 		REISERFS_SB(sb)->procdir->owner = THIS_MODULE;
 		REISERFS_SB(sb)->procdir->data = sb;
 		add_file(sb, "version", show_version);

@@ -503,6 +512,9 @@ int reiserfs_proc_info_init(struct super
 		add_file(sb, "on-disk-super", show_on_disk_super);
 		add_file(sb, "oidmap", show_oidmap);
 		add_file(sb, "journal", show_journal);
+
+		current->lock_depth = saved_lock_depth;
+
 		return 0;
 	}
 	reiserfs_warning(sb, "reiserfs: cannot create /proc/%s/%s",

------------>
Subject: remove: bkl sync supers dependency
From: Ingo Molnar <[email protected]>
Date: Wed May 14 16:10:36 CEST 2008

untangle this dependency:

=======================================================
[ INFO: possible circular locking dependency detected ] 2.6.26-rc2-sched-devel.git #480
-------------------------------------------------------
pdflush/303 is trying to acquire lock:
(kernel_mutex){--..}, at: [<c04ae4cb>] lock_kernel+0x1e/0x25

but task is already holding lock:
(&type->s_lock_key#8){--..}, at: [<c0194b95>] lock_super+0x1b/0x1d

which lock already depends on the new lock.

the existing dependency chain (in reverse order) is:

-> #2 (&type->s_lock_key#8){--..}:

       [<c0159405>] __lock_acquire+0x97d/0xae6
       [<c01598da>] lock_acquire+0x4e/0x6c
       [<c04acd20>] mutex_lock_nested+0xc2/0x22a
       [<c0194b95>] lock_super+0x1b/0x1d
       [<c01956ff>] sync_supers+0x3e/0x99
       [<c017a3b9>] wb_kupdate+0x2a/0xdd
       [<c017a89d>] pdflush+0xf8/0x18d
       [<c014d5b4>] kthread+0x3b/0x63
       [<c011a737>] kernel_thread_helper+0x7/0x10
       [<ffffffff>] 0xffffffff

-> #1 (&type->s_umount_key#15){----}:

       [<c0159405>] __lock_acquire+0x97d/0xae6
       [<c01598da>] lock_acquire+0x4e/0x6c
       [<c04ad28a>] down_write+0x28/0x44
       [<c0194f67>] sget+0x1fd/0x339
       [<c0195910>] get_sb_bdev+0x46/0x10b
       [<c01e1f3d>] get_super_block+0x13/0x15
       [<c019557f>] vfs_kern_mount+0x81/0xf7
       [<c0195639>] do_kern_mount+0x32/0xba
       [<c01a863d>] do_new_mount+0x46/0x74
       [<c01a8802>] do_mount+0x197/0x1b5
       [<c01a8884>] sys_mount+0x64/0x9b
       [<c06dba90>] mount_block_root+0xa3/0x1e6
       [<c06dbc1f>] mount_root+0x4c/0x54
       [<c06dbd72>] prepare_namespace+0x14b/0x172
       [<c06db565>] kernel_init+0x217/0x226
       [<c011a737>] kernel_thread_helper+0x7/0x10
       [<ffffffff>] 0xffffffff

-> #0 (kernel_mutex){--..}:

       [<c015932c>] __lock_acquire+0x8a4/0xae6
       [<c01598da>] lock_acquire+0x4e/0x6c
       [<c04acd20>] mutex_lock_nested+0xc2/0x22a
       [<c04ae4cb>] lock_kernel+0x1e/0x25
       [<c01e2839>] reiserfs_sync_fs+0x15/0x5b
       [<c01e288c>] reiserfs_write_super+0xd/0xf
       [<c0195719>] sync_supers+0x58/0x99
       [<c017a3b9>] wb_kupdate+0x2a/0xdd
       [<c017a89d>] pdflush+0xf8/0x18d
       [<c014d5b4>] kthread+0x3b/0x63
       [<c011a737>] kernel_thread_helper+0x7/0x10
       [<ffffffff>] 0xffffffff

other info that might help us debug this:

2 locks held by pdflush/303:
#0: (&type->s_umount_key#15){----}, at: [<c01956f8>] sync_supers+0x37/0x99 #1: (&type->s_lock_key#8){--..}, at: [<c0194b95>] lock_super+0x1b/0x1d

stack backtrace:
Pid: 303, comm: pdflush Not tainted 2.6.26-rc2-sched-devel.git #480
 [<c0157af7>] print_circular_bug_tail+0x5b/0x66
 [<c015743f>] ? print_circular_bug_entry+0x39/0x43
 [<c015932c>] __lock_acquire+0x8a4/0xae6
 [<c0158040>] ? find_usage_backwards+0x97/0xb6
 [<c01598da>] lock_acquire+0x4e/0x6c
 [<c04ae4cb>] ? lock_kernel+0x1e/0x25
 [<c04acd20>] mutex_lock_nested+0xc2/0x22a
 [<c04ae4cb>] ? lock_kernel+0x1e/0x25
 [<c04ae4cb>] ? lock_kernel+0x1e/0x25
 [<c04ae4cb>] lock_kernel+0x1e/0x25
 [<c01e2839>] reiserfs_sync_fs+0x15/0x5b
 [<c0194b95>] ? lock_super+0x1b/0x1d
 [<c01e288c>] reiserfs_write_super+0xd/0xf
 [<c0195719>] sync_supers+0x58/0x99
 [<c017a3b9>] wb_kupdate+0x2a/0xdd
 [<c01586e7>] ? trace_hardirqs_on+0xb/0xd
 [<c017a7a5>] ? pdflush+0x0/0x18d
 [<c017a89d>] pdflush+0xf8/0x18d
 [<c017a38f>] ? wb_kupdate+0x0/0xdd
 [<c014d5b4>] kthread+0x3b/0x63
 [<c014d579>] ? kthread+0x0/0x63
 [<c011a737>] kernel_thread_helper+0x7/0x10
 =======================

it's a hack, because it widens the BKL's scope. But it's needed for every filesystem that takes the BKL, up until the point that SB code can stop using the BKL.

NOT-Signed-off-by Ingo Molnar <[email protected]>

---
 fs/quota.c |    2 ++
 fs/super.c |    4 ++++
 2 files changed, 6 insertions(+)

fs/quota.c +2 -0

Index: linux/fs/quota.c
===================================================================
@@ -206,10 +206,12 @@ restart:
 			continue;
 		sb->s_count++;
 		spin_unlock(&sb_lock);
+		lock_kernel();
 		down_read(&sb->s_umount);
 		if (sb->s_root && sb->s_qcop->quota_sync)
 			quota_sync_sb(sb, type);
 		up_read(&sb->s_umount);
+		unlock_kernel();
 		spin_lock(&sb_lock);
 		if (__put_super_and_need_restart(sb))
 			goto restart;

fs/super.c +4 -0

Index: linux/fs/super.c
===================================================================
@@ -408,9 +408,11 @@ restart:
 		if (sb->s_dirt) {
 			sb->s_count++;
 			spin_unlock(&sb_lock);
+			lock_kernel();
 			down_read(&sb->s_umount);
 			write_super(sb);
 			up_read(&sb->s_umount);
+			unlock_kernel();
 			spin_lock(&sb_lock);
 			if (__put_super_and_need_restart(sb))
 				goto restart;

@@ -459,10 +461,12 @@ restart:
 			continue;	/* hm.  Was remounted r/o meanwhile */
 		sb->s_count++;
 		spin_unlock(&sb_lock);
+		lock_kernel();
 		down_read(&sb->s_umount);
 		if (sb->s_root && (wait || sb->s_dirt))
 			sb->s_op->sync_fs(sb, wait);
 		up_read(&sb->s_umount);
+		unlock_kernel();
 		/* restart only when sb is no longer on the list */
 		spin_lock(&sb_lock);
 		if (__put_super_and_need_restart(sb))

------------->

Subject: remove bkl: reiserfs fix
From: Ingo Molnar <[email protected]>
Date: Wed May 14 16:26:36 CEST 2008

avoid j_commit_lock deadlock. Since the down() can block it is safe to drop the BKL here.

Signed-off-by Ingo Molnar <[email protected]>

---
 fs/reiserfs/journal.c |    2 ++
 fs/super.c            |    2 ++
 2 files changed, 4 insertions(+)

fs/reiserfs/journal.c +2 -0

Index: linux/fs/reiserfs/journal.c
===================================================================
@@ -1044,8 +1044,10 @@ static int flush_commit_list(struct supe
 		}
 	}
 
+//	unlock_kernel();
 	/* make sure nobody is trying to flush this one at the same time */
 	down(&jl->j_commit_lock);
+//	lock_kernel();
 	if (!journal_list_still_alive(s, trans_id)) {
 		up(&jl->j_commit_lock);
 		goto put_jl;

fs/super.c +2 -0

Index: linux/fs/super.c
===================================================================
@@ -180,10 +180,12 @@ void deactivate_super(struct super_block
 		s->s_count -= S_BIAS-1;
 		spin_unlock(&sb_lock);
 		DQUOT_OFF(s, 0);
+		lock_kernel();
 		down_write(&s->s_umount);
 		fs->kill_sb(s);
 		put_filesystem(fs);
 		put_super(s);
+		unlock_kernel();
 	}
 }

Frederik Deweerdt

2008-05-14 20:06 UTC #16 lore raw

Hi Ingo,

Quoted 18 lines · Ingo Molnar

Subject: remove bkl: reiserfs fix
From: Ingo Molnar <[email protected]>
Date: Wed May 14 16:26:36 CEST 2008
avoid j_commit_lock deadlock. Since the down() can block it is safe to drop the BKL here.
Signed-off-by Ingo Molnar <[email protected]>
---
 fs/reiserfs/journal.c |    2 ++
 fs/super.c            |    2 ++
 2 files changed, 4 insertions(+)
Index: linux/fs/reiserfs/journal.c
===================================================================

fs/reiserfs/journal.c static int flush_commit_list(struct supe · line 1047

Quoted 4 lines

fs/reiserfs/journal.c +1 -0

@@ -1044,8 +1044,10 @@ static int flush_commit_list(struct supe
 		}
 	}
 
+//	unlock_kernel();

^^^^

fs/reiserfs/journal.c static int flush_commit_list(struct supe · line 1050

Quoted 3 lines

fs/reiserfs/journal.c +1 -0

 	/* make sure nobody is trying to flush this one at the same time */
 	down(&jl->j_commit_lock);
+//	lock_kernel();

^^^^

fs/reiserfs/journal.c static int flush_commit_list(struct supe · line 1053

Quoted 3 lines

fs/reiserfs/journal.c +0 -0

 	if (!journal_list_still_alive(s, trans_id)) {
 		up(&jl->j_commit_lock);
 		goto put_jl;

Must be a typo?

Regards,
Frederik

Jonathan Corbet

2008-05-14 21:45 UTC #17 lore raw

Sez Ingo:

Quoted 3 lines

This task is not easy at all. 12 years after Linux has been converted to an SMP OS we still have 1300+ legacy BKL using sites. There are 400+ lock_kernel() critical sections and 800+ ioctls.

There's also every char device open() method - a rather long list in its own right. I'd be surprised if one in ten of them really needs it, but one has to look...

I've been looking at the chrdev code anyway, and pondering on how this might be addressed. Here's some thoughts on alternatives, I'd be curious what people think:

1: We could add an unlocked_open() to the file_operations structure;
drivers could be converted over as they are verified not to need the BKL on open. Disadvantages are that it grows this structure for a relatively rare case - most open() calls already don't need the BKL. But it's a relatively easy path without flag days.

2: Create a char_dev_ops structure for char devs and use it instead of
file_operations. I vaguely remember seeing Al mutter about that a while back. Quite a while back. This mirrors what was done with block devices, and makes some sense - there's a lot of stuff in struct file_operations which is not really applicable to char devs. Then struct char_dev_ops could have open() and locked_open(), with the latter destined for removal sometime around 2015 or so.

Advantages are that it's cleaner and separates out some things which perhaps shouldn't be mixed anyway. Disadvantage is...well...a fair amount of code churn. It would also require chrdev-specific wrappers to map straight file_operations calls in the VFS to the new callbacks.

3: Provide a new form of cdev_add() which lets the driver indicate
that the BKL is not needed on open (or anything else?). At a minimum, it could just be a new parameter on cdev_add which has a value of zero or FIXME_I_STILL_NEED_BKL. Still some churn but easier to script and smaller because a lot of drivers are still using register_chrdev() - something else worth fixing.

A more involved form might provide a new chardev_add() which takes the new char_dev_ops structure too. Mapping between new and old operations vectors would be done internally to avoid breaking older drivers before they can be fixed.

4: Just find every char dev open() function and shove in lock_kernel()
calls, then remove the call from chrdev_open(). The disadvantage here is that, beyond lots of work and churn, there's no way to know which ones you missed.

I kind of like the combination of 2 and 3, done in such a way that there's no "every driver must change" flag day. This could be an interesting project, even... Thoughts?

jon

Alan Cox

2008-05-14 21:51 UTC #18 lore raw

Quoted 3 lines

I kind of like the combination of 2 and 3, done in such a way that there's no "every driver must change" flag day. This could be an interesting project, even... Thoughts?

Its beginning to sound like should start 2.7 ;)

Alan

Linus Torvalds

2008-05-14 21:58 UTC #19 lore raw

Quoted 4 lines · Jonathan Corbet

There's also every char device open() method - a rather long list in its own right. I'd be surprised if one in ten of them really needs it, but one has to look...

I don't think there are *that* many. I found only 83 instances of "register_chrdev()" in the kernel, so the open methods should be pretty limited.

Of course, some open methods call other sub-registrations, but you'd start off by moving the lock_kernel() down just *one* stage.

So it literally should be:
- remove one lock_kernel/unlock_kernel pair in fs/char_dev.c
- add max 83 pairs in the places that register those things
- external modules will need to add it themselves some day.

Quoted 5 lines

1: We could add an unlocked_open() to the file_operations structure;
drivers could be converted over as they are verified not to need the BKL on open. Disadvantages are that it grows this structure for a relatively rare case - most open() calls already don't need the BKL. But it's a relatively easy path without flag days.

I really don't think it's worth the pain. See above. The numbers aren't that huge, and external modules simply aren't a pressing enough issue.

Linus

Jonathan Corbet

2008-05-14 22:07 UTC #20 lore raw

Quoted 3 lines · Linus Torvalds

I don't think there are *that* many. I found only 83 instances of "register_chrdev()" in the kernel, so the open methods should be pretty limited.

There's the drivers calling cdev_add() directly as well - another 40ish. Still not a huge list, I guess.

Quoted 4 lines

So it literally should be:
- remove one lock_kernel/unlock_kernel pair in fs/char_dev.c
- add max 83 pairs in the places that register those things
- external modules will need to add it themselves some day.

This is all certainly doable, but it leaves me with one concern: there will be no signal to external module maintainers that the change needs to be made. So, beyond doubt, quite a few of them will just continue to be shipped unfixed - and they will still run. If any of them actually *need* the BKL, something awful may happen to somebody someday.

jon

Linus Torvalds

2008-05-14 22:15 UTC #21 lore raw

Quoted 6 lines · Jonathan Corbet

This is all certainly doable, but it leaves me with one concern: there will be no signal to external module maintainers that the change needs to be made. So, beyond doubt, quite a few of them will just continue to be shipped unfixed - and they will still run. If any of them actually *need* the BKL, something awful may happen to somebody someday.

External modules have bugs because interfaces change. Film at 11.

It's true, but it definitely shouldn't keep us from just doing it. Especially since well-maintained external modules (ie the authors follow big discussions like this) can just take the kernel lock regardless of kernel version, since it won't even be broken with old kernels.

Of course, well-maintained kernel modules wouldn't depend on the BKL in the first place. Oh, well.

Linus

Alan Cox

2008-05-22 20:33 UTC #22 lore raw

In reply to Linus Torvalds

Quoted 5 lines

This is all certainly doable, but it leaves me with one concern: there will be no signal to external module maintainers that the change needs to be made. So, beyond doubt, quite a few of them will just continue to be shipped unfixed - and they will still run. If any of them actually *need* the BKL, something awful may happen to somebody someday.

I now have a large patch and my full x86-32 build tree building without ->ioctl() in file_operations. Its a 350K patch and took all day so I'll begin splitting it out and sending chunks to tree maintainers.

Alan

Jonathan Corbet

2008-05-16 15:44 UTC #23 lore raw

RFC PATCH 1/1 char dev BKL pushdown

Quoted 3 lines · Linus

So it literally should be:
- remove one lock_kernel/unlock_kernel pair in fs/char_dev.c
- add max 83 pairs in the places that register those things

...and so that's what I've done. My approach was to find every register_chrdev() and cdev_add() call, look at the associated file_operations, then go back to the open() function, if any. Unless it was almost immediately obvious to me that the function was either (1) so trivial as to not require locking (quite few of them are "return 0;"), or
(2) clearly doing its own locking, I wrapped the code in the BKL.

Finally, I removed the BKL from chrdev_open().

Allmodconfig and allyesconfig makes work here, and this kernel runs on my dual-core desktop. But, clearly, I don't have all of this hardware. Actually, I wonder if some of it still exists outside of museums. So there's probably something stupid in there somewhere.

The result is available at:

	git://git.lwn.net/linux-2.6.git cdev

I'll put a shortlog and diffstat at the end of this message. For completeness, there's also a list of files I examined and did *not* change.

Assuming nobody tells me I'm completely off-base, I guess my next step is to start running individual patches past maintainers. Some of them, probably (I hope), will tell me that I've been wasting my time and that their code doesn't need the BKL. In such cases, I'll gladly drop the associated patch. But there's a fair amount of stuff here which clearly *does* need it still. If all seems well, maybe this tree should get into linux-next at some point too.

Comments?

The changes come out to this:

 arch/cris/arch-v10/drivers/gpio.c         |    3 ++
 arch/cris/arch-v10/drivers/sync_serial.c  |   34 ++++++++++++++++----------
 arch/cris/arch-v32/drivers/mach-a3/gpio.c |    4 +++
 arch/cris/arch-v32/drivers/mach-fs/gpio.c |    5 +++
 arch/cris/arch-v32/drivers/sync_serial.c  |   33 +++++++++++++++-----------
 arch/mips/kernel/rtlx.c                   |    7 ++++-
 arch/mips/kernel/vpe.c                    |   12 +++++++--
 arch/mips/sibyte/common/sb_tbprof.c       |   25 ++++++++++++++-----
 arch/sh/boards/landisk/gio.c              |   10 +++++--
 arch/x86/kernel/cpuid.c                   |   25 +++++++++++++------
 arch/x86/kernel/msr.c                     |   16 +++++++++---
 block/bsg.c                               |    7 ++++-
 drivers/block/aoe/aoechr.c                |    7 ++++-
 drivers/block/paride/pg.c                 |   22 ++++++++++++-----
 drivers/block/paride/pt.c                 |    8 +++++-
 drivers/char/drm/drm_fops.c               |    9 +++++--
 drivers/char/ipmi/ipmi_devintf.c          |    8 ++++--
 drivers/char/lp.c                         |   38 ++++++++++++++++++++----------
 drivers/char/mbcs.c                       |    5 +++
 drivers/char/mem.c                        |   10 ++++++-
 drivers/char/misc.c                       |    3 ++
 drivers/char/pcmcia/cm4000_cs.c           |   26 +++++++++++++++-----
 drivers/char/pcmcia/cm4040_cs.c           |   23 +++++++++++++-----
 drivers/char/snsc.c                       |    5 +++
 drivers/char/tty_io.c                     |   27 +++++++++++++++++++--
 drivers/char/viotape.c                    |    3 ++
 drivers/firewire/fw-cdev.c                |   16 +++++++++---
 drivers/hid/hidraw.c                      |    3 ++
 drivers/i2c/i2c-dev.c                     |   22 ++++++++++++-----
 drivers/ide/ide-tape.c                    |    7 ++++-
 drivers/ieee1394/dv1394.c                 |    6 +++-
 drivers/ieee1394/raw1394.c                |    3 ++
 drivers/ieee1394/video1394.c              |   18 ++++++++++----
 drivers/input/input.c                     |   16 +++++++++---
 drivers/isdn/i4l/isdn_common.c            |    3 +-
 drivers/media/dvb/dvb-core/dvbdev.c       |    4 +++
 drivers/mtd/mtdchar.c                     |   22 ++++++++++++-----
 drivers/mtd/ubi/cdev.c                    |    7 ++++-
 drivers/net/wan/cosa.c                    |   22 ++++++++++++-----
 drivers/pcmcia/pcmcia_ioctl.c             |   25 ++++++++++++++-----
 drivers/rtc/rtc-dev.c                     |   12 +++++++--
 drivers/s390/char/fs3270.c                |   23 ++++++++++++------
 drivers/s390/char/tape_char.c             |   12 +++++++--
 drivers/s390/char/vmlogrdr.c              |    8 +++++-
 drivers/s390/char/vmur.c                  |   12 +++++++--
 drivers/scsi/aacraid/linit.c              |    3 ++
 drivers/scsi/gdth.c                       |    3 ++
 drivers/scsi/osst.c                       |   15 +++++++++++
 drivers/scsi/sg.c                         |   16 ++++++++++--
 drivers/scsi/st.c                         |   11 +++++++-
 drivers/telephony/phonedev.c              |    3 ++
 drivers/uio/uio.c                         |   17 +++++++++----
 drivers/usb/core/file.c                   |    3 ++
 drivers/video/fbmem.c                     |   15 ++++++++---
 fs/char_dev.c                             |    8 ++----
 sound/core/sound.c                        |   15 +++++++++++
 sound/sound_core.c                        |    5 +++
 57 files changed, 554 insertions(+), 176 deletions(-)

The associated shortlog is:

Jonathan Corbet (43):
bsg: cdev lock_kernel() pushdown
cris: cdev lock_kernel() pushdown
mips: cdev lock_kernel() pushdown
sh: cdev lock_kernel() pushdown
x86: cdev lock_kernel() pushdown
i2c: cdev lock_kernel() pushdown
cosa: cdev lock_kernel() pushdown
pcmcia: cdev lock_kernel() pushdown
ieee1394: cdev lock_kernel() pushdown
rtc: cdev lock_kernel() pushdown drivers/s390: cdev lock_kernel() pushdown
AoE: cdev lock_kernel() pushdown
paride: cdev lock_kernel() pushdown
mtdchar: cdev lock_kernel() pushdown
UBI: cdev lock_kernel() pushdown
firewire: cdev lock_kernel() pushdown
HID: cdev lock_kernel() pushdown
Input: cdev lock_kernel() pushdown
UIO: cdev lock_kernel() pushdown
cm40x0: cdev lock_kernel() pushdown
ipmi: cdev lock_kernel() pushdown
mem: cdev lock_kernel() pushdown
misc: cdev lock_kernel() pushdown
viotape: cdev lock_kernel pushdown
mbcs: cdev lock_kernel() pushdown
lp: cdev lock_kernel() pushdown
drm: cdev lock_kernel() pushdown
phonedev: cdev lock_kernel() pushdown
ide-tape: cdev lock_kernel() pushdown
sg: cdev lock_kernel() pushdown
osst: cdev lock_kernel() pushdown.
aacraid: cdev lock_kernel() pushdown
st: cdev lock_kernel() pushdown
gdth: cdev lock_kernel() pushdown
isdn: cdev lock_kernel() pushdown
usbcore: cdev lock_kernel() pushdown
dvb: cdev lock_kernel() pushdown
fbmem: cdev lock_kernel() pushdown
sound: cdev lock_kernel() pushdown
snsc: cdev lock_kernel() pushdown
tty: cdev lock_kernel() pushdown
Remove the lock_kernel() call from chrdev_open() Add a comment in chrdev_open()

Char device source files which I did *not* change:

arch/cris/arch-v32/drivers/pcf8563.c (no open function) arch/cris/arch-v32/drivers/i2c.c (empty open function) arch/cris/arch-v32/drivers/cryptocop.c (almost-empty open function) arch/cris/arch-v10/drivers/pcf8563.c (no open function) arch/cris/arch-v10/drivers/i2c.c (empty open function) arch/cris/arch-v10/drivers/ds1302.c (no open function) arch/cris/arch-v10/drivers/eeprom.c (trivial) drivers/net/ppp_generic.c (trivial) drivers/ieee1394/ieee1394_core.c (almost-trivial) drivers/spi/spidev.c (locking looks right) drivers/char/cs5535_gpio.c (trivial) drivers/char/dtlk.c (trivial, broken open) drivers/char/ds1302.c (no open) drivers/char/pc8736x_gpio.c (trivial) drivers/char/stallion.c (no open)
drivers/char/tb0219.c (trivial open) drivers/char/vc_screen.c (trivial open) drivers/char/ppdev.c (trivial open) drivers/char/ip2/ip2main.c (trivial open - does not do anything!) drivers/char/scx200_gpio.c (trivial open) drivers/char/xilinx_hwicap/xilinx_hwicap.c (locking looks right) drivers/char/istallion.c (no open) drivers/char/vr41xx_giu.c (trivial) drivers/char/tlclk.c (locking looks right) drivers/char/raw.c (obvious locking w/raw_mutex) drivers/char/dsp56k.c (single-use locking) drivers/infiniband/hw/ipath/ipath_file_ops.c (trivial open) drivers/infiniband/core/user_mad.c (appears to have good locking) drivers/infiniband/core/uverbs_main.c (ditto) drivers/infiniband/core/ucm.c (trivial open) drivers/misc/phantom.c (has locking) drivers/sbus/char/bpp.c (has locking) drivers/sbus/char/vfc_dev.c (has locking) drivers/scsi/dpt_i2o.c (has locking) drivers/scsi/3w-xxxx.c (trivial open) drivers/scsi/3w-9xxx.c (trivial open) drivers/scsi/megaraid/megaraid_sas.c (trivial open) drivers/scsi/ch.c (has locking)
drivers/scsi/megaraid.c (trivial open) drivers/isdn/capi/capi.c (semi-trivial open) drivers/isdn/hardware/eicon/divasi.c (empty open) drivers/isdn/hardware/eicon/divamnt.c (single-use open) drivers/isdn/hardware/eicon/divasmain.c (empty open) drivers/macintosh/adb.c (trivial open) drivers/usb/gadget/printer.c (has locking) drivers/usb/mon/mon_bin.c (has locking) drivers/usb/core/endpoint.c (no opens) drivers/usb/core/devio.c (has locking) drivers/media/video/videodev.c (has locking) fs/coda/psdev.c (already has lock_kernel() calls)

jon

Christoph Hellwig

2008-05-16 15:50 UTC #24 lore raw

Re: [PATCH, RFC] char dev BKL pushdown

Quoted 1 line · Jonathan Corbet

trivial as to not require locking (quite few of them are "return 0;"), or

If they literaly are 'return 0' you can just remove them, as a non-existing open op will just be fine.

Quoted 1 line

(2) clearly doing its own locking, I wrapped the code in the BKL.

Even if clearly does it's own locking please add the BKL for now and let the maintainers sort it out later, better be safe then sorry.

Except for that thanks a lot, this is the kind of work that's more productive than all these discussions here :)

For some reason about 80 instances seem awfully few, but we've move a lot of device into subsystems from beeing plain chardevs so this might actually be correct.

Christoph Hellwig

2008-05-16 16:03 UTC #25 lore raw

PATCH 1/1 kill empty chardev open/release methods

Quoted 5 lines · Christoph Hellwig

Quoted 1 line · Jonathan Corbet
trivial as to not require locking (quite few of them are "return 0;"), or
If they literaly are 'return 0' you can just remove them, as a non-existing open op will just be fine.

And here's a patch to do just that: remove all empty chardev open/release methods. Based on the list compiled by Jonathan.

(and yeah, ip2_ipl_open is not technically empty at the source level,
but only at the binary level :))

Signed-off-by Christoph Hellwig <[email protected]>

arch/cris/arch-v10/drivers/i2c.c +0 ~~-14~~

Index: linux-2.6/arch/cris/arch-v10/drivers/i2c.c
===================================================================
@@ -563,18 +563,6 @@ i2c_readreg(unsigned char theSlave, unsi
 	return b;
 }
 
-static int
-i2c_open(struct inode *inode, struct file *filp)
-{
-	return 0;
-}
-
-static int
-i2c_release(struct inode *inode, struct file *filp)
-{
-	return 0;
-}
-
 /* Main device API. ioctl's to write or read to/from i2c registers.
  */

@@ -619,8 +607,6 @@ i2c_ioctl(struct inode *inode, struct fi
 static const struct file_operations i2c_fops = {
 	.owner    = THIS_MODULE,
 	.ioctl    = i2c_ioctl,
-	.open     = i2c_open,
-	.release  = i2c_release,
 };
 
 int __init

arch/cris/arch-v32/drivers/i2c.c +0 ~~-14~~

Index: linux-2.6/arch/cris/arch-v32/drivers/i2c.c
===================================================================
@@ -633,18 +633,6 @@ i2c_readreg(unsigned char theSlave, unsi
 	return b;
 }
 
-static int
-i2c_open(struct inode *inode, struct file *filp)
-{
-	return 0;
-}
-
-static int
-i2c_release(struct inode *inode, struct file *filp)
-{
-	return 0;
-}
-
 /* Main device API. ioctl's to write or read to/from i2c registers.
  */

@@ -689,8 +677,6 @@ i2c_ioctl(struct inode *inode, struct fi
 static const struct file_operations i2c_fops = {
 	.owner =    THIS_MODULE,
 	.ioctl =    i2c_ioctl,
-	.open =     i2c_open,
-	.release =  i2c_release,
 };
 
 static int __init i2c_init(void)

drivers/char/ip2/ip2main.c +0 ~~-54~~

72 lines not on this page · raw message

drivers/isdn/hardware/eicon/divasi.c +0 -8

Index: linux-2.6/drivers/isdn/hardware/eicon/divasi.c
===================================================================
@@ -74,7 +74,6 @@ static ssize_t um_idi_read(struct file *
 static ssize_t um_idi_write(struct file *file, const char __user *buf,
 			    size_t count, loff_t * offset);
 static unsigned int um_idi_poll(struct file *file, poll_table * wait);
-static int um_idi_open(struct inode *inode, struct file *file);
 static int um_idi_release(struct inode *inode, struct file *file);
 static int remove_entity(void *entity);
 static void diva_um_timer_function(unsigned long data);

@@ -136,7 +135,6 @@ static const struct file_operations diva
 	.read    = um_idi_read,
 	.write   = um_idi_write,
 	.poll    = um_idi_poll,
-	.open    = um_idi_open,
 	.release = um_idi_release
 };

@@ -398,12 +396,6 @@ static unsigned int um_idi_poll(struct f
 	return (POLLIN | POLLRDNORM);
 }
 
-static int um_idi_open(struct inode *inode, struct file *file)
-{
-	return (0);
-}
-
-
 static int um_idi_release(struct inode *inode, struct file *file)
 {
 	diva_um_idi_os_context_t *p_os;

drivers/isdn/hardware/eicon/divasmain.c +0 -5

Index: linux-2.6/drivers/isdn/hardware/eicon/divasmain.c
===================================================================
@@ -578,10 +578,6 @@ xdi_copy_from_user(void *os_handle, void
 /*
  * device node operations
  */
-static int divas_open(struct inode *inode, struct file *file)
-{
-	return (0);
-}
 
 static int divas_release(struct inode *inode, struct file *file)
 {

@@ -667,7 +663,6 @@ static const struct file_operations diva
 	.read    = divas_read,
 	.write   = divas_write,
 	.poll    = divas_poll,
-	.open    = divas_open,
 	.release = divas_release
 };

Alan Cox

2008-05-16 16:42 UTC #26 lore raw

Re: [PATCH] kill empty chardev open/release methods

Quoted 1 line

Index: linux-2.6/drivers/char/ip2/ip2main.c

Looks fine but please send it via my tty tree so it doesn't collide with the tty work.

Alan Cox

2008-05-16 21:07 UTC #27 lore raw

Re: [PATCH] kill empty chardev open/release methods

In reply to Christoph Hellwig

Quoted 5 lines

Quoted 2 lines
If they literaly are 'return 0' you can just remove them, as a non-existing open op will just be fine.
And here's a patch to do just that: remove all empty chardev open/release methods. Based on the list compiled by Jonathan.

Actually it turns out you can introduce bugs doing this when the BKL is pushed down.

The problem is the methods are not NULL, they (with the lock pushed down are)

{
	lock_kernel();
	unlock_kernel();
}

And we have drivers with setup code that does things in the wrong order but under the BKL. eg one I just fixed did

	misc_register()
	init locks
	allocate memory
	do stuff
	return 0;

The lock/unlock in the open happens to save your butt against the wrong order of intialisation because the open cannot occur before the lock is taken, and thanks to the BKL it cannot make any progress until the setup is completed. Fun too - udev loves opening things as they appear so in some cases we might actually trigger them too.

So when you remove the _open() empty methods *please* make sure you have verified the correctness and ordering of the entire registration path. I've found three examples of this so far just cleaning up drivers/watchdog.

Alan

Jonathan Corbet

2008-05-18 19:46 UTC #28 lore raw

Re: [PATCH] kill empty chardev open/release methods

Quoted 11 lines · Alan Cox

Actually it turns out you can introduce bugs doing this when the BKL is pushed down.
The problem is the methods are not NULL, they (with the lock pushed down are)
{
	lock_kernel();
	unlock_kernel();
}

Quoted 8 lines

And we have drivers with setup code that does things in the wrong order but under the BKL. eg one I just fixed did
	misc_register()
	init locks
	allocate memory
	do stuff
	return 0;

Hmph.

As it turns out, a misc driver will still be OK because the BKL has not (yet) been pushed past misc_open(). What this does mean, though, is that all of those empty and trivial open functions need to be revisited. I thought this looked too easy the first time through...

jon

Alan Cox

2008-05-18 20:10 UTC #29 lore raw

Re: [PATCH] kill empty chardev open/release methods

Quoted 5 lines

As it turns out, a misc driver will still be OK because the BKL has not (yet) been pushed past misc_open(). What this does mean, though, is that all of those empty and trivial open functions need to be revisited. I thought this looked too easy the first time through...

I think it would be best to make them lock/unlock kernel in the first pass and then work through them. The BKL can be subtle and evil, but as I brought it into the world I guess I must banish it ;)

Alan

Linus Torvalds

2008-05-16 16:31 UTC #30 lore raw

Re: [PATCH, RFC] char dev BKL pushdown

Quoted 3 lines · Jonathan Corbet

I'll put a shortlog and diffstat at the end of this message. For completeness, there's also a list of files I examined and did *not* change.

May I suggest just adding a comment in those files, just saying something like

	/* This does not need the BKL, because .. */

where even the "because" part could be dropped when it's really obvious.

That way that "list of files I examined and did *not* change" would be obvious in the patch itself, and we also have some documentation that somebody actually looked at the path.

Quoted 5 lines

Assuming nobody tells me I'm completely off-base, I guess my next step is to start running individual patches past maintainers. Some of them, probably (I hope), will tell me that I've been wasting my time and that their code doesn't need the BKL. In such cases, I'll gladly drop the associated patch.

Same deal - just document the fact that the BKL isn't needed.

Yeah, in the long run that kind of documentation is worthless and we may want to get rid of it again in a year or two, but in the short run it's a good idea. If only to help people who want to review your patches.

Btw, do you have gitweb running anywhere?

Linus

Jonathan Corbet

2008-05-16 16:44 UTC #31 lore raw

Re: [PATCH, RFC] char dev BKL pushdown

Quoted 4 lines · Linus Torvalds

May I suggest just adding a comment in those files, just saying something like
	/* This does not need the BKL, because .. */

OK, I'll make another pass shortly and fill that in.

Quoted 1 line

Btw, do you have gitweb running anywhere?

No, I guess I need to figure out how to set it up. Either that or get one of those kernel.org accounts and put things there.

Thanks,

jon

Alan Cox

2008-05-16 16:41 UTC #32 lore raw

Re: [PATCH, RFC] char dev BKL pushdown

Quoted 3 lines

probably (I hope), will tell me that I've been wasting my time and that their code doesn't need the BKL. In such cases, I'll gladly drop the associated patch. But there's a fair amount of stuff here which clearly

You have to be careful before assuming but yes - seems sensible.

I'm currently munching my way through the watchdog drivers fixing them up for unlocked_ioctl/BKL drops and finding various things needing fixing anyway.

Quoted 2 lines

*does* need it still. If all seems well, maybe this tree should get into linux-next at some point too.

Definitely

Arnd Bergmann

maintainer

2008-05-17 21:15 UTC #33 lore raw

Re: [PATCH, RFC] char dev BKL pushdown

Quoted 3 lines · Jonathan Corbet

...and so that's what I've done. My approach was to find every register_chrdev() and cdev_add() call, look at the associated file_operations, then go back to the open() function, if any.

Note that the majority of drivers use (grep suggests up to 165 of them) uses misc_register instead of register_chrdev/cdev_add. Your patches are still correct, because you pushed the BKL into the misc_open function, but there is an obvious next step in pushing it further into the misc drivers.
There are probably a few more subsystems with minor number specific open() functions, misc is just the obvious one.

Quoted 8 lines

Quoted 1 line
...and so that's what I've done. My approach was to find every
register_chrdev() and cdev_add() call, look at the associated file_operations, then go back to the open() function, if any. Unless it was almost immediately obvious to me that the function was either (1) so trivial as to not require locking (quite few of them are "return 0;"), or
(2) clearly doing its own locking, I wrapped the code in the BKL.
Finally, I removed the BKL from chrdev_open().

In your current git tree, this change is no longer the final one, so bisecting the series may cause other bugs. You should probably reorder the patches at some point to avoid this.

	Arnd <><

Jonathan Corbet

2008-05-18 20:27 UTC #34 lore raw

Re: [PATCH, RFC] char dev BKL pushdown

Quoted 5 lines · Arnd Bergmann

Note that the majority of drivers use (grep suggests up to 165 of them) uses misc_register instead of register_chrdev/cdev_add. Your patches are still correct, because you pushed the BKL into the misc_open function, but there is an obvious next step in pushing it further into the misc drivers.

There's a few intermediate dispatcher levels like this, actually. Lots of video drivers get called behind video_open(), usb drivers from usb_open(), etc. Not much to be done but to push things down one level at a time.

Quoted 3 lines

In your current git tree, this change is no longer the final one, so bisecting the series may cause other bugs. You should probably reorder the patches at some point to avoid this.

Bisection is going to be problem regardless - if a problem turns up, it's going to be the chrdev_open() change which gets fingered. I bet, though, that it will be a rare BKL-related problem which is reproducible enough to be easily bisectable.

But, yes, I do need to reorganize the patch series once I'm done adding on changes.

jon

Arnd Bergmann

maintainer

2008-05-19 23:45 UTC #35 lore raw

Re: [PATCH, RFC] char dev BKL pushdown

Quoted 4 lines · Jonathan Corbet

There's a few intermediate dispatcher levels like this, actually. Lots of video drivers get called behind video_open(), usb drivers from usb_open(), etc. Not much to be done but to push things down one level at a time.

I've given it a try for all the misc drivers that have an open() function. The vast majority of them are actually watchdog drivers, all of which register as a misc device by themselves. You seem to already have a script to turn per-file changes into a patch each, so I'm sending you two patches:
one for all the watchdog drivers (maybe Wim can take care of that as well) and one for all the other misc drivers (this one needs to be split).

	Arnd <><

Arnd Bergmann

maintainer

2008-05-19 23:46 UTC #36 lore raw

RFC PATCH 1/3 misc char dev BKL pushdown

The Big Kernel Lock has been pushed down from chardev_open to misc_open, this change moves it to the individual misc driver open functions.

As before, the change was purely mechanical, most drivers should actually not need the BKL. In particular, we still hold the misc_mtx() while calling the open() function The patch should probably be split into one changeset per driver.

Signed-off-by Arnd Bergmann <[email protected]>

---

arch/arm/common/rtctime.c +3 -0

Index: linux-2.6/arch/arm/common/rtctime.c
===================================================================
@@ -16,6 +16,7 @@
 #include <linux/poll.h>
 #include <linux/proc_fs.h>
 #include <linux/miscdevice.h>
+#include <linux/smp_lock.h>
 #include <linux/spinlock.h>
 #include <linux/capability.h>
 #include <linux/device.h>

@@ -282,6 +283,7 @@ static int rtc_open(struct inode *inode,
 {
 	int ret;
 
+	lock_kernel();
 	mutex_lock(&rtc_mutex);
 
 	if (rtc_inuse) {

@@ -301,6 +303,7 @@ static int rtc_open(struct inode *inode,
 		}
 	}
 	mutex_unlock(&rtc_mutex);
+	unlock_kernel();
 
 	return ret;
 }

arch/blackfin/mach-bf561/coreb.c +4 -0

Index: linux-2.6/arch/blackfin/mach-bf561/coreb.c
===================================================================
@@ -32,6 +32,7 @@
 #include <linux/device.h>
 #include <linux/ioport.h>
 #include <linux/module.h>
+#include <linux/smp_lock.h>
 #include <linux/uaccess.h>
 #include <linux/fs.h>
 #include <asm/dma.h>

@@ -196,6 +197,7 @@ static loff_t coreb_lseek(struct file *f
 
 static int coreb_open(struct inode *inode, struct file *file)
 {
+	lock_kernel();
 	spin_lock_irq(&coreb_lock);
 
 	if (coreb_status & COREB_IS_OPEN)

@@ -204,10 +206,12 @@ static int coreb_open(struct inode *inod
 	coreb_status |= COREB_IS_OPEN;
 
 	spin_unlock_irq(&coreb_lock);
+	unlock_kernel();
 	return 0;
 
  out_busy:
 	spin_unlock_irq(&coreb_lock);
+	unlock_kernel();
 	return -EBUSY;
 }

arch/m68k/bvme6000/rtc.c +6 -1

Index: linux-2.6/arch/m68k/bvme6000/rtc.c
===================================================================
@@ -10,6 +10,7 @@
 #include <linux/errno.h>
 #include <linux/miscdevice.h>
 #include <linux/slab.h>
+#include <linux/smp_lock.h>
 #include <linux/ioport.h>
 #include <linux/capability.h>
 #include <linux/fcntl.h>

@@ -140,10 +141,14 @@ static int rtc_ioctl(struct inode *inode
 
 static int rtc_open(struct inode *inode, struct file *file)
 {
-	if(rtc_status)
+	lock_kernel();
+	if(rtc_status) {
+		unlock_kernel();
 		return -EBUSY;
+	}
 
 	rtc_status = 1;
+	unlock_kernel();
 	return 0;
 }

arch/m68k/mvme16x/rtc.c +4 -0

Index: linux-2.6/arch/m68k/mvme16x/rtc.c
===================================================================
@@ -10,6 +10,7 @@
 #include <linux/errno.h>
 #include <linux/miscdevice.h>
 #include <linux/slab.h>
+#include <linux/smp_lock.h>
 #include <linux/ioport.h>
 #include <linux/capability.h>
 #include <linux/fcntl.h>

@@ -127,11 +128,14 @@ static int rtc_ioctl(struct inode *inode
 
 static int rtc_open(struct inode *inode, struct file *file)
 {
+	lock_kernel();
 	if( !atomic_dec_and_test(&rtc_ready) )
 	{
 		atomic_inc( &rtc_ready );
+		unlock_kernel();
 		return -EBUSY;
 	}
+	unlock_kernel();
 
 	return 0;
 }

arch/mips/basler/excite/excite_iodev.c +8 -1

Index: linux-2.6/arch/mips/basler/excite/excite_iodev.c
===================================================================
@@ -26,6 +26,7 @@
 #include <linux/interrupt.h>
 #include <linux/platform_device.h>
 #include <linux/miscdevice.h>
+#include <linux/smp_lock.h>
 
 #include "excite_iodev.h"

@@ -110,8 +111,14 @@ static int __exit iodev_remove(struct de
 
 static int iodev_open(struct inode *i, struct file *f)
 {
-	return request_irq(iodev_irq, iodev_irqhdl, IRQF_DISABLED,
+	int ret;
+
+	lock_kernel();
+	ret = request_irq(iodev_irq, iodev_irqhdl, IRQF_DISABLED,
 			   iodev_name, &miscdev);
+	unlock_kernel();
+
+	return ret;
 }
 
 static int iodev_release(struct inode *i, struct file *f)

arch/parisc/kernel/perf.c +4 -0

Index: linux-2.6/arch/parisc/kernel/perf.c
===================================================================
@@ -46,6 +46,7 @@
 #include <linux/init.h>
 #include <linux/proc_fs.h>
 #include <linux/miscdevice.h>
+#include <linux/smp_lock.h>
 #include <linux/spinlock.h>
 
 #include <asm/uaccess.h>

@@ -260,13 +261,16 @@ printk("Preparing to start counters\n");
  */
 static int perf_open(struct inode *inode, struct file *file)
 {
+	lock_kernel();
 	spin_lock(&perf_lock);
 	if (perf_enabled) {
 		spin_unlock(&perf_lock);
+		unlock_kernel();
 		return -EBUSY;
 	}
 	perf_enabled = 1;
  	spin_unlock(&perf_lock);
+	unlock_kernel();
 
 	return 0;
 }

arch/s390/crypto/prng.c +2 -0

Index: linux-2.6/arch/s390/crypto/prng.c
===================================================================
@@ -6,6 +6,7 @@
 #include <linux/fs.h>
 #include <linux/init.h>
 #include <linux/kernel.h>
+#include <linux/smp_lock.h>
 #include <linux/miscdevice.h>
 #include <linux/module.h>
 #include <linux/moduleparam.h>

@@ -48,6 +49,7 @@ static unsigned char parm_block[32] = {
 
 static int prng_open(struct inode *inode, struct file *file)
 {
+	cycle_kernel_lock();
 	return nonseekable_open(inode, file);
 }

arch/sparc/kernel/apc.c +2 -0

Index: linux-2.6/arch/sparc/kernel/apc.c
===================================================================
@@ -10,6 +10,7 @@
 #include <linux/errno.h>
 #include <linux/init.h>
 #include <linux/miscdevice.h>
+#include <linux/smp_lock.h>
 #include <linux/pm.h>
 
 #include <asm/io.h>

@@ -75,6 +76,7 @@ static inline void apc_free(void)
 
 static int apc_open(struct inode *inode, struct file *f)
 {
+	cycle_kernel_lock();
 	return 0;
 }

arch/sparc64/kernel/time.c +6 -1

Index: linux-2.6/arch/sparc64/kernel/time.c
===================================================================
@@ -11,6 +11,7 @@
 #include <linux/errno.h>
 #include <linux/module.h>
 #include <linux/sched.h>
+#include <linux/smp_lock.h>
 #include <linux/kernel.h>
 #include <linux/param.h>
 #include <linux/string.h>

@@ -1659,10 +1660,14 @@ static int mini_rtc_ioctl(struct inode *
 
 static int mini_rtc_open(struct inode *inode, struct file *file)
 {
-	if (mini_rtc_status & RTC_IS_OPEN)
+	lock_kernel();
+	if (mini_rtc_status & RTC_IS_OPEN) {
+		unlock_kernel();
 		return -EBUSY;
+	}
 
 	mini_rtc_status |= RTC_IS_OPEN;
+	unlock_kernel();
 
 	return 0;
 }

arch/um/drivers/harddog_kern.c +3 -0

Index: linux-2.6/arch/um/drivers/harddog_kern.c
===================================================================
@@ -66,6 +66,7 @@ static int harddog_open(struct inode *in
 	int err = -EBUSY;
 	char *sock = NULL;
 
+	lock_kernel();
 	spin_lock(&lock);
 	if(timer_alive)
 		goto err;

@@ -82,9 +83,11 @@ static int harddog_open(struct inode *in
 
 	timer_alive = 1;
 	spin_unlock(&lock);
+	unlock_kernel();
 	return nonseekable_open(inode, file);
 err:
 	spin_unlock(&lock);
+	unlock_kernel();
 	return err;
 }

arch/um/drivers/mmapper_kern.c +2 -0

Index: linux-2.6/arch/um/drivers/mmapper_kern.c
===================================================================
@@ -16,6 +16,7 @@
 #include <linux/miscdevice.h>
 #include <linux/module.h>
 #include <linux/mm.h>
+#include <linux/smp_lock.h>
 #include <asm/uaccess.h>
 #include "mem_user.h"

@@ -77,6 +78,7 @@ out:
 
 static int mmapper_open(struct inode *inode, struct file *file)
 {
+	cycle_kernel_lock();
 	return 0;
 }

arch/um/drivers/random.c +3 -0

Index: linux-2.6/arch/um/drivers/random.c
===================================================================
@@ -7,6 +7,7 @@
  * of the GNU General Public License, incorporated herein by reference.
  */
 #include <linux/sched.h>
+#include <linux/smp_lock.h>
 #include <linux/module.h>
 #include <linux/fs.h>
 #include <linux/interrupt.h>

@@ -33,6 +34,8 @@ static DECLARE_WAIT_QUEUE_HEAD(host_read
 
 static int rng_dev_open (struct inode *inode, struct file *filp)
 {
+	cycle_kernel_lock();
+
 	/* enforce read-only access to this chrdev */
 	if ((filp->f_mode & FMODE_READ) == 0)
 		return -EINVAL;

arch/x86/kernel/apm_32.c +4 -0

Index: linux-2.6/arch/x86/kernel/apm_32.c
===================================================================
@@ -204,6 +204,7 @@
 #include <linux/module.h>
 
 #include <linux/poll.h>
+#include <linux/smp_lock.h>
 #include <linux/types.h>
 #include <linux/stddef.h>
 #include <linux/timer.h>

@@ -1544,10 +1545,12 @@ static int do_open(struct inode *inode, 
 {
 	struct apm_user *as;
 
+	lock_kernel();
 	as = kmalloc(sizeof(*as), GFP_KERNEL);
 	if (as == NULL) {
 		printk(KERN_ERR "apm: cannot allocate struct of size %d bytes\n",
 		       sizeof(*as));
+		       unlock_kernel();
 		return -ENOMEM;
 	}
 	as->magic = APM_BIOS_MAGIC;

@@ -1569,6 +1572,7 @@ static int do_open(struct inode *inode, 
 	user_list = as;
 	spin_unlock(&user_list_lock);
 	filp->private_data = as;
+	unlock_kernel();
 	return 0;
 }

arch/x86/kernel/cpu/mcheck/mce_64.c +4 -0

Index: linux-2.6/arch/x86/kernel/cpu/mcheck/mce_64.c
===================================================================
@@ -9,6 +9,7 @@
 #include <linux/types.h>
 #include <linux/kernel.h>
 #include <linux/sched.h>
+#include <linux/smp_lock.h>
 #include <linux/string.h>
 #include <linux/rcupdate.h>
 #include <linux/kallsyms.h>

@@ -527,10 +528,12 @@ static int open_exclu;	/* already open e
 
 static int mce_open(struct inode *inode, struct file *file)
 {
+	lock_kernel();
 	spin_lock(&mce_state_lock);
 
 	if (open_exclu || (open_count && (file->f_flags & O_EXCL))) {
 		spin_unlock(&mce_state_lock);
+		unlock_kernel();
 		return -EBUSY;
 	}

@@ -539,6 +542,7 @@ static int mce_open(struct inode *inode,
 	open_count++;
 
 	spin_unlock(&mce_state_lock);
+	unlock_kernel();
 
 	return nonseekable_open(inode, file);
 }

arch/x86/kernel/microcode.c +2 -0

Index: linux-2.6/arch/x86/kernel/microcode.c
===================================================================
@@ -75,6 +75,7 @@
 #include <linux/kernel.h>
 #include <linux/init.h>
 #include <linux/sched.h>
+#include <linux/smp_lock.h>
 #include <linux/cpumask.h>
 #include <linux/module.h>
 #include <linux/slab.h>

@@ -422,6 +423,7 @@ out:
 
 static int microcode_open (struct inode *unused1, struct file *unused2)
 {
+	cycle_kernel_lock();
 	return capable(CAP_SYS_RAWIO) ? 0 : -EPERM;
 }

drivers/bluetooth/hci_vhci.c +5 -0

Index: linux-2.6/drivers/bluetooth/hci_vhci.c
===================================================================
@@ -28,6 +28,7 @@
 #include <linux/kernel.h>
 #include <linux/init.h>
 #include <linux/slab.h>
+#include <linux/smp_lock.h>
 #include <linux/types.h>
 #include <linux/errno.h>
 #include <linux/sched.h>

@@ -263,9 +264,11 @@ static int vhci_open(struct inode *inode
 	skb_queue_head_init(&data->readq);
 	init_waitqueue_head(&data->read_wait);
 
+	lock_kernel();
 	hdev = hci_alloc_dev();
 	if (!hdev) {
 		kfree(data);
+		unlock_kernel();
 		return -ENOMEM;
 	}

@@ -286,10 +289,12 @@ static int vhci_open(struct inode *inode
 		BT_ERR("Can't register HCI device");
 		kfree(data);
 		hci_free_dev(hdev);
+		unlock_kernel();
 		return -EBUSY;
 	}
 
 	file->private_data = data;
+	unlock_kernel();
 
 	return nonseekable_open(inode, file);
 }

drivers/char/agp/frontend.c +4 -0

Index: linux-2.6/drivers/char/agp/frontend.c
===================================================================
@@ -39,6 +39,7 @@
 #include <linux/mm.h>
 #include <linux/fs.h>
 #include <linux/sched.h>
+#include <linux/smp_lock.h>
 #include <asm/uaccess.h>
 #include <asm/pgtable.h>
 #include "agp.h"

@@ -677,6 +678,7 @@ static int agp_open(struct inode *inode,
 	struct agp_client *client;
 	int rc = -ENXIO;
 
+	lock_kernel();
 	mutex_lock(&(agp_fe.agp_mutex));
 
 	if (minor != AGPGART_MINOR)

@@ -703,12 +705,14 @@ static int agp_open(struct inode *inode,
 	agp_insert_file_private(priv);
 	DBG("private=%p, client=%p", priv, client);
 	mutex_unlock(&(agp_fe.agp_mutex));
+	unlock_kernel();
 	return 0;
 
 err_out_nomem:
 	rc = -ENOMEM;
 err_out:
 	mutex_unlock(&(agp_fe.agp_mutex));
+	unlock_kernel();
 	return rc;
 }

drivers/char/apm-emulation.c +3 -0

Index: linux-2.6/drivers/char/apm-emulation.c
===================================================================
@@ -13,6 +13,7 @@
 #include <linux/module.h>
 #include <linux/poll.h>
 #include <linux/slab.h>
+#include <linux/smp_lock.h>
 #include <linux/proc_fs.h>
 #include <linux/seq_file.h>
 #include <linux/miscdevice.h>

@@ -416,6 +417,7 @@ static int apm_open(struct inode * inode
 {
 	struct apm_user *as;
 
+	lock_kernel();
 	as = kzalloc(sizeof(*as), GFP_KERNEL);
 	if (as) {
 		/*

@@ -435,6 +437,7 @@ static int apm_open(struct inode * inode
 
 		filp->private_data = as;
 	}
+	unlock_kernel();
 
 	return as ? 0 : -ENOMEM;
 }

drivers/char/briq_panel.c +7 -2

Index: linux-2.6/drivers/char/briq_panel.c
===================================================================
@@ -6,6 +6,7 @@
 
 #include <linux/module.h>
 
+#include <linux/smp_lock.h>
 #include <linux/types.h>
 #include <linux/errno.h>
 #include <linux/tty.h>

@@ -67,11 +68,15 @@ static void set_led(char state)
 
 static int briq_panel_open(struct inode *ino, struct file *filep)
 {
-	/* enforce single access */
-	if (vfd_is_open)
+	lock_kernel();
+	/* enforce single access, vfd_is_open is protected by BKL */
+	if (vfd_is_open) {
+		unlock_kernel();
 		return -EBUSY;
+	}
 	vfd_is_open = 1;
 
+	unlock_kernel();
 	return 0;
 }

drivers/char/ds1286.c +4 -0

Index: linux-2.6/drivers/char/ds1286.c
===================================================================
@@ -27,6 +27,7 @@
  * option) any later version.
  */
 #include <linux/ds1286.h>
+#include <linux/smp_lock.h>
 #include <linux/types.h>
 #include <linux/errno.h>
 #include <linux/miscdevice.h>

@@ -252,6 +253,7 @@ static int ds1286_ioctl(struct inode *in
 
 static int ds1286_open(struct inode *inode, struct file *file)
 {
+	lock_kernel();
 	spin_lock_irq(&ds1286_lock);
 
 	if (ds1286_status & RTC_IS_OPEN)

@@ -260,10 +262,12 @@ static int ds1286_open(struct inode *ino
 	ds1286_status |= RTC_IS_OPEN;
 
 	spin_unlock_irq(&ds1286_lock);
+	unlock_kernel();
 	return 0;
 
 out_busy:
 	spin_lock_irq(&ds1286_lock);
+	unlock_kernel();
 	return -EBUSY;
 }

drivers/char/ds1620.c +8 -1

Index: linux-2.6/drivers/char/ds1620.c
===================================================================
@@ -8,6 +8,7 @@
 #include <linux/proc_fs.h>
 #include <linux/capability.h>
 #include <linux/init.h>
+#include <linux/smp_lock.h>
 
 #include <asm/hardware.h>
 #include <asm/mach-types.h>

@@ -208,6 +209,12 @@ static void ds1620_read_state(struct the
 	therm->hi = cvt_9_to_int(ds1620_in(THERM_READ_TH, 9));
 }
 
+static int ds1620_open(struct inode *inode, struct file *file)
+{
+	cycle_kernel_lock();
+	return nonseekable_open(inode, file);
+}
+
 static ssize_t
 ds1620_read(struct file *file, char __user *buf, size_t count, loff_t *ptr)
 {

@@ -336,7 +343,7 @@ static struct proc_dir_entry *proc_therm
 
 static const struct file_operations ds1620_fops = {
 	.owner		= THIS_MODULE,
-	.open		= nonseekable_open,
+	.open		= ds1620_open,
 	.read		= ds1620_read,
 	.ioctl		= ds1620_ioctl,
 };

drivers/char/efirtc.c +2 -0

Index: linux-2.6/drivers/char/efirtc.c
===================================================================
@@ -28,6 +28,7 @@
  */
 
 
+#include <linux/smp_lock.h>
 #include <linux/types.h>
 #include <linux/errno.h>
 #include <linux/miscdevice.h>

@@ -272,6 +273,7 @@ efi_rtc_open(struct inode *inode, struct
 	 * We do accept multiple open files at the same time as we
 	 * synchronize on the per call operation.
 	 */
+	cycle_kernel_lock();
 	return 0;
 }

drivers/char/genrtc.c +6 -1

Index: linux-2.6/drivers/char/genrtc.c
===================================================================
@@ -51,6 +51,7 @@
 #include <linux/init.h>
 #include <linux/poll.h>
 #include <linux/proc_fs.h>
+#include <linux/smp_lock.h>
 #include <linux/workqueue.h>
 
 #include <asm/uaccess.h>

@@ -338,12 +339,16 @@ static int gen_rtc_ioctl(struct inode *i
 
 static int gen_rtc_open(struct inode *inode, struct file *file)
 {
-	if (gen_rtc_status & RTC_IS_OPEN)
+	lock_kernel();
+	if (gen_rtc_status & RTC_IS_OPEN) {
+		unlock_kernel();
 		return -EBUSY;
+	}
 
 	gen_rtc_status |= RTC_IS_OPEN;
 	gen_rtc_irq_data = 0;
 	irq_active = 0;
+	unlock_kernel();
 
 	return 0;
 }

drivers/char/hpet.c +4 -0

Index: linux-2.6/drivers/char/hpet.c
===================================================================
@@ -14,6 +14,7 @@
 #include <linux/interrupt.h>
 #include <linux/module.h>
 #include <linux/kernel.h>
+#include <linux/smp_lock.h>
 #include <linux/types.h>
 #include <linux/miscdevice.h>
 #include <linux/major.h>

@@ -193,6 +194,7 @@ static int hpet_open(struct inode *inode
 	if (file->f_mode & FMODE_WRITE)
 		return -EINVAL;
 
+	lock_kernel();
 	spin_lock_irq(&hpet_lock);
 
 	for (devp = NULL, hpetp = hpets; hpetp && !devp; hpetp = hpetp->hp_next)

@@ -207,6 +209,7 @@ static int hpet_open(struct inode *inode
 
 	if (!devp) {
 		spin_unlock_irq(&hpet_lock);
+		unlock_kernel();
 		return -EBUSY;
 	}

@@ -214,6 +217,7 @@ static int hpet_open(struct inode *inode
 	devp->hd_irqdata = 0;
 	devp->hd_flags |= HPET_OPEN;
 	spin_unlock_irq(&hpet_lock);
+	unlock_kernel();
 
 	return 0;
 }

drivers/char/hw_random/core.c +2 -0

Index: linux-2.6/drivers/char/hw_random/core.c
===================================================================
@@ -37,6 +37,7 @@
 #include <linux/kernel.h>
 #include <linux/fs.h>
 #include <linux/sched.h>
+#include <linux/smp_lock.h>
 #include <linux/init.h>
 #include <linux/miscdevice.h>
 #include <linux/delay.h>

@@ -86,6 +87,7 @@ static int rng_dev_open(struct inode *in
 		return -EINVAL;
 	if (filp->f_mode & FMODE_WRITE)
 		return -EINVAL;
+	cycle_kernel_lock();
 	return 0;
 }

drivers/char/ip27-rtc.c +4 -0

Index: linux-2.6/drivers/char/ip27-rtc.c
===================================================================
@@ -27,6 +27,7 @@
 #include <linux/bcd.h>
 #include <linux/module.h>
 #include <linux/kernel.h>
+#include <linux/smp_lock.h>
 #include <linux/types.h>
 #include <linux/miscdevice.h>
 #include <linux/ioport.h>

@@ -163,15 +164,18 @@ static long rtc_ioctl(struct file *filp,
 
 static int rtc_open(struct inode *inode, struct file *file)
 {
+	lock_kernel();
 	spin_lock_irq(&rtc_lock);
 
 	if (rtc_status & RTC_IS_OPEN) {
 		spin_unlock_irq(&rtc_lock);
+		unlock_kernel();
 		return -EBUSY;
 	}
 
 	rtc_status |= RTC_IS_OPEN;
 	spin_unlock_irq(&rtc_lock);
+	unlock_kernel();
 
 	return 0;
 }

drivers/char/ipmi/ipmi_watchdog.c +3 -0

Index: linux-2.6/drivers/char/ipmi/ipmi_watchdog.c
===================================================================
@@ -35,6 +35,7 @@
 #include <linux/moduleparam.h>
 #include <linux/ipmi.h>
 #include <linux/ipmi_smi.h>
+#include <linux/smp_lock.h>
 #include <linux/watchdog.h>
 #include <linux/miscdevice.h>
 #include <linux/init.h>

@@ -819,6 +820,8 @@ static int ipmi_open(struct inode *ino, 
 		if (test_and_set_bit(0, &ipmi_wdog_open))
 			return -EBUSY;
 
+		cycle_kernel_lock();
+
 		/*
 		 * Don't start the timer now, let it start on the
 		 * first heartbeat.

drivers/char/lcd.c +3 -0

Index: linux-2.6/drivers/char/lcd.c
===================================================================
@@ -20,6 +20,7 @@
 #include <linux/mc146818rtc.h>
 #include <linux/netdevice.h>
 #include <linux/sched.h>
+#include <linux/smp_lock.h>
 #include <linux/delay.h>
 
 #include <asm/io.h>

@@ -414,6 +415,8 @@ static int lcd_ioctl(struct inode *inode
 
 static int lcd_open(struct inode *inode, struct file *file)
 {
+	cycle_kernel_lock();
+
 	if (!lcd_present)
 		return -ENXIO;
 	else

drivers/char/mwave/mwavedd.c +2 -0

Index: linux-2.6/drivers/char/mwave/mwavedd.c
===================================================================
@@ -56,6 +56,7 @@
 #include <linux/serial.h>
 #include <linux/sched.h>
 #include <linux/spinlock.h>
+#include <linux/smp_lock.h>
 #include <linux/delay.h>
 #include <linux/serial_8250.h>
 #include "smapi.h"

@@ -100,6 +101,7 @@ static int mwave_open(struct inode *inod
 	PRINTK_2(TRACE_MWAVE,
 		"mwavedd::mwave_open, exit return retval %x\n", retval);
 
+	cycle_kernel_lock();
 	return retval;
 }

drivers/char/nvram.c +4 -0

Index: linux-2.6/drivers/char/nvram.c
===================================================================
@@ -107,6 +107,7 @@
 #include <linux/init.h>
 #include <linux/proc_fs.h>
 #include <linux/spinlock.h>
+#include <linux/smp_lock.h>
 
 #include <asm/io.h>
 #include <asm/uaccess.h>

@@ -333,12 +334,14 @@ nvram_ioctl(struct inode *inode, struct 
 static int
 nvram_open(struct inode *inode, struct file *file)
 {
+	lock_kernel();
 	spin_lock(&nvram_state_lock);
 
 	if ((nvram_open_cnt && (file->f_flags & O_EXCL)) ||
 	    (nvram_open_mode & NVRAM_EXCL) ||
 	    ((file->f_mode & 2) && (nvram_open_mode & NVRAM_WRITE))) {
 		spin_unlock(&nvram_state_lock);
+		unlock_kernel();
 		return -EBUSY;
 	}

@@ -349,6 +352,7 @@ nvram_open(struct inode *inode, struct f
 	nvram_open_cnt++;
 
 	spin_unlock(&nvram_state_lock);
+	unlock_kernel();
 
 	return 0;
 }

drivers/char/rtc.c +4 -0

Index: linux-2.6/drivers/char/rtc.c
===================================================================
@@ -73,6 +73,7 @@
 #include <linux/proc_fs.h>
 #include <linux/seq_file.h>
 #include <linux/spinlock.h>
+#include <linux/smp_lock.h>
 #include <linux/sysctl.h>
 #include <linux/wait.h>
 #include <linux/bcd.h>

@@ -733,6 +734,7 @@ static int rtc_ioctl(struct inode *inode
  * needed here. Or anywhere else in this driver. */
 static int rtc_open(struct inode *inode, struct file *file)
 {
+	lock_kernel();
 	spin_lock_irq(&rtc_lock);
 
 	if (rtc_status & RTC_IS_OPEN)

@@ -742,10 +744,12 @@ static int rtc_open(struct inode *inode,
 
 	rtc_irq_data = 0;
 	spin_unlock_irq(&rtc_lock);
+	unlock_kernel();
 	return 0;
 
 out_busy:
 	spin_unlock_irq(&rtc_lock);
+	unlock_kernel();
 	return -EBUSY;
 }

drivers/char/sonypi.c +3 -0

Index: linux-2.6/drivers/char/sonypi.c
===================================================================
@@ -49,6 +49,7 @@
 #include <linux/err.h>
 #include <linux/kfifo.h>
 #include <linux/platform_device.h>
+#include <linux/smp_lock.h>
 
 #include <asm/uaccess.h>
 #include <asm/io.h>

@@ -906,12 +907,14 @@ static int sonypi_misc_release(struct in
 
 static int sonypi_misc_open(struct inode *inode, struct file *file)
 {
+	lock_kernel();
 	mutex_lock(&sonypi_device.lock);
 	/* Flush input queue on first open */
 	if (!sonypi_device.open_count)
 		kfifo_reset(sonypi_device.fifo);
 	sonypi_device.open_count++;
 	mutex_unlock(&sonypi_device.lock);
+	unlock_kernel();
 	return 0;
 }

drivers/char/tpm/tpm.c +5 -0

Index: linux-2.6/drivers/char/tpm/tpm.c
===================================================================
@@ -26,6 +26,7 @@
 #include <linux/poll.h>
 #include <linux/mutex.h>
 #include <linux/spinlock.h>
+#include <linux/smp_lock.h>
 
 #include "tpm.h"

@@ -897,6 +898,7 @@ int tpm_open(struct inode *inode, struct
 	int rc = 0, minor = iminor(inode);
 	struct tpm_chip *chip = NULL, *pos;
 
+	lock_kernel();
 	spin_lock(&driver_lock);
 
 	list_for_each_entry(pos, &tpm_chip_list, list) {

@@ -926,16 +928,19 @@ int tpm_open(struct inode *inode, struct
 	if (chip->data_buffer == NULL) {
 		chip->num_opens--;
 		put_device(chip->dev);
+		unlock_kernel();
 		return -ENOMEM;
 	}
 
 	atomic_set(&chip->data_pending, 0);
 
 	file->private_data = chip;
+	unlock_kernel();
 	return 0;
 
 err_out:
 	spin_unlock(&driver_lock);
+	unlock_kernel();
 	return rc;
 }
 EXPORT_SYMBOL_GPL(tpm_open);

drivers/infiniband/core/ucma.c +3 -0

Index: linux-2.6/drivers/infiniband/core/ucma.c
===================================================================
@@ -38,6 +38,7 @@
 #include <linux/in.h>
 #include <linux/in6.h>
 #include <linux/miscdevice.h>
+#include <linux/smp_lock.h>
 
 #include <rdma/rdma_user_cm.h>
 #include <rdma/ib_marshall.h>

@@ -1156,6 +1157,7 @@ static int ucma_open(struct inode *inode
 	if (!file)
 		return -ENOMEM;
 
+	lock_kernel();
 	INIT_LIST_HEAD(&file->event_list);
 	INIT_LIST_HEAD(&file->ctx_list);
 	init_waitqueue_head(&file->poll_wait);

@@ -1163,6 +1165,7 @@ static int ucma_open(struct inode *inode
 
 	filp->private_data = file;
 	file->filp = filp;
+	unlock_kernel();
 	return 0;
 }

drivers/input/misc/hp_sdc_rtc.c +2 -0

Index: linux-2.6/drivers/input/misc/hp_sdc_rtc.c
===================================================================
@@ -35,6 +35,7 @@
 
 #include <linux/hp_sdc.h>
 #include <linux/errno.h>
+#include <linux/smp_lock.h>
 #include <linux/types.h>
 #include <linux/init.h>
 #include <linux/module.h>

@@ -408,6 +409,7 @@ static unsigned int hp_sdc_rtc_poll(stru
 
 static int hp_sdc_rtc_open(struct inode *inode, struct file *file)
 {
+	cycle_kernel_lock();
         return 0;
 }

drivers/input/misc/uinput.c +3 -0

Index: linux-2.6/drivers/input/misc/uinput.c
===================================================================
@@ -37,6 +37,7 @@
 #include <linux/fs.h>
 #include <linux/miscdevice.h>
 #include <linux/uinput.h>
+#include <linux/smp_lock.h>
 
 static int uinput_dev_event(struct input_dev *dev, unsigned int type, unsigned int code, int value)
 {

@@ -222,6 +223,7 @@ static int uinput_open(struct inode *ino
 	if (!newdev)
 		return -ENOMEM;
 
+	lock_kernel();
 	mutex_init(&newdev->mutex);
 	spin_lock_init(&newdev->requests_lock);
 	init_waitqueue_head(&newdev->requests_waitq);

@@ -229,6 +231,7 @@ static int uinput_open(struct inode *ino
 	newdev->state = UIST_NEW_DEVICE;
 
 	file->private_data = newdev;
+	unlock_kernel();
 
 	return 0;
 }

drivers/input/mousedev.c +10 -2

Index: linux-2.6/drivers/input/mousedev.c
===================================================================
@@ -14,6 +14,7 @@
 #define MOUSEDEV_MIX		31
 
 #include <linux/slab.h>
+#include <linux/smp_lock.h>
 #include <linux/poll.h>
 #include <linux/module.h>
 #include <linux/init.h>

@@ -545,16 +546,21 @@ static int mousedev_open(struct inode *i
 	if (i >= MOUSEDEV_MINORS)
 		return -ENODEV;
 
+	lock_kernel();
 	error = mutex_lock_interruptible(&mousedev_table_mutex);
-	if (error)
+	if (error) {
+		unlock_kernel();
 		return error;
+	}
 	mousedev = mousedev_table[i];
 	if (mousedev)
 		get_device(&mousedev->dev);
 	mutex_unlock(&mousedev_table_mutex);
 
-	if (!mousedev)
+	if (!mousedev) {
+		unlock_kernel();
 		return -ENODEV;
+	}
 
 	client = kzalloc(sizeof(struct mousedev_client), GFP_KERNEL);
 	if (!client) {

@@ -573,6 +579,7 @@ static int mousedev_open(struct inode *i
 		goto err_free_client;
 
 	file->private_data = client;
+	unlock_kernel();
 	return 0;
 
  err_free_client:

@@ -580,6 +587,7 @@ static int mousedev_open(struct inode *i
 	kfree(client);
  err_put_mousedev:
 	put_device(&mousedev->dev);
+	unlock_kernel();
 	return error;
 }

drivers/input/serio/serio_raw.c +5 -1

Index: linux-2.6/drivers/input/serio/serio_raw.c
===================================================================
@@ -10,6 +10,7 @@
  */
 
 #include <linux/slab.h>
+#include <linux/smp_lock.h>
 #include <linux/poll.h>
 #include <linux/module.h>
 #include <linux/serio.h>

@@ -81,9 +82,10 @@ static int serio_raw_open(struct inode *
 	struct serio_raw_list *list;
 	int retval = 0;
 
+	lock_kernel();
 	retval = mutex_lock_interruptible(&serio_raw_mutex);
 	if (retval)
-		return retval;
+		goto out_bkl;
 
 	if (!(serio_raw = serio_raw_locate(iminor(inode)))) {
 		retval = -ENODEV;

@@ -108,6 +110,8 @@ static int serio_raw_open(struct inode *
 
 out:
 	mutex_unlock(&serio_raw_mutex);
+out_bkl:
+	unlock_kernel();
 	return retval;
 }

drivers/macintosh/ans-lcd.c +2 -0

Index: linux-2.6/drivers/macintosh/ans-lcd.c
===================================================================
@@ -3,6 +3,7 @@
  */
 
 #include <linux/types.h>
+#include <linux/smp_lock.h>
 #include <linux/errno.h>
 #include <linux/kernel.h>
 #include <linux/miscdevice.h>

@@ -119,6 +120,7 @@ anslcd_ioctl( struct inode * inode, stru
 static int
 anslcd_open( struct inode * inode, struct file * file )
 {
+	cycle_kernel_lock();
 	return 0;
 }

drivers/macintosh/smu.c +3 -0

Index: linux-2.6/drivers/macintosh/smu.c
===================================================================
@@ -19,6 +19,7 @@
  *    the userland interface
  */
 
+#include <linux/smp_lock.h>
 #include <linux/types.h>
 #include <linux/kernel.h>
 #include <linux/device.h>

@@ -1083,10 +1084,12 @@ static int smu_open(struct inode *inode,
 	pp->mode = smu_file_commands;
 	init_waitqueue_head(&pp->wait);
 
+	lock_kernel();
 	spin_lock_irqsave(&smu_clist_lock, flags);
 	list_add(&pp->list, &smu_clist);
 	spin_unlock_irqrestore(&smu_clist_lock, flags);
 	file->private_data = pp;
+	unlock_kernel();
 
 	return 0;
 }

drivers/macintosh/via-pmu.c +3 -0

Index: linux-2.6/drivers/macintosh/via-pmu.c
===================================================================
@@ -18,6 +18,7 @@
  *
  */
 #include <stdarg.h>
+#include <linux/smp_lock.h>
 #include <linux/types.h>
 #include <linux/errno.h>
 #include <linux/kernel.h>

@@ -2047,6 +2048,7 @@ pmu_open(struct inode *inode, struct fil
 	pp->rb_get = pp->rb_put = 0;
 	spin_lock_init(&pp->lock);
 	init_waitqueue_head(&pp->wait);
+	lock_kernel();
 	spin_lock_irqsave(&all_pvt_lock, flags);
 #if defined(CONFIG_INPUT_ADBHID) && defined(CONFIG_PMAC_BACKLIGHT)
 	pp->backlight_locker = 0;

@@ -2054,6 +2056,7 @@ pmu_open(struct inode *inode, struct fil
 	list_add(&pp->list, &all_pmu_pvt);
 	spin_unlock_irqrestore(&all_pvt_lock, flags);
 	file->private_data = pp;
+	unlock_kernel();
 	return 0;
 }

drivers/media/radio/miropcm20-rds.c +4 -0

Index: linux-2.6/drivers/media/radio/miropcm20-rds.c
===================================================================
@@ -12,6 +12,7 @@
 #include <linux/module.h>
 #include <linux/init.h>
 #include <linux/slab.h>
+#include <linux/smp_lock.h>
 #include <linux/fs.h>
 #include <linux/miscdevice.h>
 #include <linux/delay.h>

@@ -27,13 +28,16 @@ static int rds_f_open(struct inode *in, 
 	if (rds_users)
 		return -EBUSY;
 
+	lock_kernel();
 	rds_users++;
 	if ((text_buffer=kmalloc(66, GFP_KERNEL)) == 0) {
 		rds_users--;
 		printk(KERN_NOTICE "aci-rds: Out of memory by open()...\n");
+		unlock_kernel();
 		return -ENOMEM;
 	}
 
+	unlock_kernel();
 	return 0;
 }

drivers/message/i2o/i2o_config.c +2 -0

Index: linux-2.6/drivers/message/i2o/i2o_config.c
===================================================================
@@ -1061,6 +1061,7 @@ static int cfg_open(struct inode *inode,
 	if (!tmp)
 		return -ENOMEM;
 
+	lock_kernel();
 	file->private_data = (void *)(i2o_cfg_info_id++);
 	tmp->fp = file;
 	tmp->fasync = NULL;

@@ -1074,6 +1075,7 @@ static int cfg_open(struct inode *inode,
 	spin_lock_irqsave(&i2o_config_lock, flags);
 	open_files = tmp;
 	spin_unlock_irqrestore(&i2o_config_lock, flags);
+	unlock_kernel();
 
 	return 0;
 }

drivers/misc/hdpuftrs/hdpu_cpustate.c +8 -1

Index: linux-2.6/drivers/misc/hdpuftrs/hdpu_cpustate.c
===================================================================
@@ -17,6 +17,7 @@
 #include <linux/module.h>
 #include <linux/kernel.h>
 #include <linux/spinlock.h>
+#include <linux/smp_lock.h>
 #include <linux/miscdevice.h>
 #include <linux/proc_fs.h>
 #include <linux/hdpu_features.h>

@@ -151,7 +152,13 @@ static ssize_t cpustate_write(struct fil
 
 static int cpustate_open(struct inode *inode, struct file *file)
 {
-	return cpustate_get_ref((file->f_flags & O_EXCL));
+	int ret;
+
+	lock_kernel();
+	ret = cpustate_get_ref((file->f_flags & O_EXCL));
+	unlock_kernel();
+
+	return ret;
 }
 
 static int cpustate_release(struct inode *inode, struct file *file)

drivers/misc/sony-laptop.c +3 -0

Index: linux-2.6/drivers/misc/sony-laptop.c
===================================================================
@@ -46,6 +46,7 @@
 #include <linux/module.h>
 #include <linux/moduleparam.h>
 #include <linux/init.h>
+#include <linux/smp_lock.h>
 #include <linux/types.h>
 #include <linux/backlight.h>
 #include <linux/platform_device.h>

@@ -1927,8 +1928,10 @@ static int sonypi_misc_release(struct in
 static int sonypi_misc_open(struct inode *inode, struct file *file)
 {
 	/* Flush input queue on first open */
+	lock_kernel();
 	if (atomic_inc_return(&sonypi_compat.open_count) == 1)
 		kfifo_reset(sonypi_compat.fifo);
+	unlock_kernel();
 	return 0;
 }

drivers/net/tun.c +2 -0

Index: linux-2.6/drivers/net/tun.c
===================================================================
@@ -48,6 +48,7 @@
 #include <linux/kernel.h>
 #include <linux/major.h>
 #include <linux/slab.h>
+#include <linux/smp_lock.h>
 #include <linux/poll.h>
 #include <linux/fcntl.h>
 #include <linux/init.h>

@@ -797,6 +798,7 @@ static int tun_chr_fasync(int fd, struct
 
 static int tun_chr_open(struct inode *inode, struct file * file)
 {
+	cycle_kernel_lock();
 	DBG1(KERN_INFO "tunX: tun_chr_open\n");
 	file->private_data = NULL;
 	return 0;

drivers/parisc/eisa_eeprom.c +3 -0

Index: linux-2.6/drivers/parisc/eisa_eeprom.c
===================================================================
@@ -24,6 +24,7 @@
 #include <linux/kernel.h>
 #include <linux/miscdevice.h>
 #include <linux/slab.h>
+#include <linux/smp_lock.h>
 #include <linux/fs.h>
 #include <asm/io.h>
 #include <asm/uaccess.h>

@@ -83,6 +84,8 @@ static int eisa_eeprom_ioctl(struct inod
 
 static int eisa_eeprom_open(struct inode *inode, struct file *file)
 {
+	cycle_kernel_lock();
+
 	if (file->f_mode & 2)
 		return -EINVAL;

drivers/rtc/rtc-m41t80.c +6 -1

Index: linux-2.6/drivers/rtc/rtc-m41t80.c
===================================================================
@@ -17,6 +17,7 @@
 #include <linux/init.h>
 #include <linux/kernel.h>
 #include <linux/slab.h>
+#include <linux/smp_lock.h>
 #include <linux/string.h>
 #include <linux/i2c.h>
 #include <linux/rtc.h>

@@ -655,12 +656,16 @@ static int wdt_ioctl(struct inode *inode
 static int wdt_open(struct inode *inode, struct file *file)
 {
 	if (MINOR(inode->i_rdev) == WATCHDOG_MINOR) {
-		if (test_and_set_bit(0, &wdt_is_open))
+		lock_kernel();
+		if (test_and_set_bit(0, &wdt_is_open)) {
+			unlock_kernel();
 			return -EBUSY;
+		}
 		/*
 		 *	Activate
 		 */
 		wdt_is_open = 1;
+		unlock_kernel();
 		return 0;
 	}
 	return -ENODEV;

drivers/s390/block/dasd_eer.c +6 -0

Index: linux-2.6/drivers/s390/block/dasd_eer.c
===================================================================
@@ -15,6 +15,7 @@
 #include <linux/device.h>
 #include <linux/poll.h>
 #include <linux/mutex.h>
+#include <linux/smp_lock.h>
 
 #include <asm/uaccess.h>
 #include <asm/atomic.h>

@@ -525,6 +526,7 @@ static int dasd_eer_open(struct inode *i
 	eerb = kzalloc(sizeof(struct eerbuffer), GFP_KERNEL);
 	if (!eerb)
 		return -ENOMEM;
+	lock_kernel();
 	eerb->buffer_page_count = eer_pages;
 	if (eerb->buffer_page_count < 1 ||
 	    eerb->buffer_page_count > INT_MAX / PAGE_SIZE) {

@@ -532,6 +534,7 @@ static int dasd_eer_open(struct inode *i
 		MESSAGE(KERN_WARNING, "can't open device since module "
 			"parameter eer_pages is smaller then 1 or"
 			" bigger then %d", (int)(INT_MAX / PAGE_SIZE));
+		unlock_kernel();
 		return -EINVAL;
 	}
 	eerb->buffersize = eerb->buffer_page_count * PAGE_SIZE;

@@ -539,12 +542,14 @@ static int dasd_eer_open(struct inode *i
 			       GFP_KERNEL);
         if (!eerb->buffer) {
 		kfree(eerb);
+		unlock_kernel();
                 return -ENOMEM;
 	}
 	if (dasd_eer_allocate_buffer_pages(eerb->buffer,
 					   eerb->buffer_page_count)) {
 		kfree(eerb->buffer);
 		kfree(eerb);
+		unlock_kernel();
 		return -ENOMEM;
 	}
 	filp->private_data = eerb;

@@ -552,6 +557,7 @@ static int dasd_eer_open(struct inode *i
 	list_add(&eerb->list, &bufferlist);
 	spin_unlock_irqrestore(&bufferlock, flags);
 
+	unlock_kernel();
 	return nonseekable_open(inp,filp);
 }

drivers/s390/char/monreader.c +3 -0

Index: linux-2.6/drivers/s390/char/monreader.c
===================================================================
@@ -340,6 +340,7 @@ static int mon_open(struct inode *inode,
 	/*
 	 * only one user allowed
 	 */
+	lock_kernel();
 	rc = -EBUSY;
 	if (test_and_set_bit(MON_IN_USE, &mon_in_use))
 		goto out;

@@ -377,6 +378,7 @@ static int mon_open(struct inode *inode,
 	}
 	P_INFO("open, established connection to *MONITOR service\n\n");
 	filp->private_data = monpriv;
+	unlock_kernel();
 	return nonseekable_open(inode, filp);
 
 out_path:

@@ -386,6 +388,7 @@ out_priv:
 out_use:
 	clear_bit(MON_IN_USE, &mon_in_use);
 out:
+	unlock_kernel();
 	return rc;
 }

drivers/s390/char/monwriter.c +3 -0

Index: linux-2.6/drivers/s390/char/monwriter.c
===================================================================
@@ -12,6 +12,7 @@
 #include <linux/moduleparam.h>
 #include <linux/init.h>
 #include <linux/errno.h>
+#include <linux/smp_lock.h>
 #include <linux/types.h>
 #include <linux/kernel.h>
 #include <linux/miscdevice.h>

@@ -179,10 +180,12 @@ static int monwrite_open(struct inode *i
 	monpriv = kzalloc(sizeof(struct mon_private), GFP_KERNEL);
 	if (!monpriv)
 		return -ENOMEM;
+	lock_kernel();
 	INIT_LIST_HEAD(&monpriv->list);
 	monpriv->hdr_to_read = sizeof(monpriv->hdr);
 	mutex_init(&monpriv->thread_mutex);
 	filp->private_data = monpriv;
+	unlock_kernel();
 	return nonseekable_open(inode, filp);
 }

drivers/s390/char/vmcp.c +4 -0

Index: linux-2.6/drivers/s390/char/vmcp.c
===================================================================
@@ -16,6 +16,7 @@
 #include <linux/kernel.h>
 #include <linux/miscdevice.h>
 #include <linux/module.h>
+#include <linux/smp_lock.h>
 #include <asm/cpcmd.h>
 #include <asm/debug.h>
 #include <asm/uaccess.h>

@@ -39,11 +40,14 @@ static int vmcp_open(struct inode *inode
 	session = kmalloc(sizeof(*session), GFP_KERNEL);
 	if (!session)
 		return -ENOMEM;
+
+	lock_kernel();
 	session->bufsize = PAGE_SIZE;
 	session->response = NULL;
 	session->resp_size = 0;
 	mutex_init(&session->mutex);
 	file->private_data = session;
+	unlock_kernel();
 	return nonseekable_open(inode, file);
 }

drivers/s390/char/vmwatchdog.c +6 -1

Index: linux-2.6/drivers/s390/char/vmwatchdog.c
===================================================================
@@ -13,6 +13,7 @@
 #include <linux/module.h>
 #include <linux/moduleparam.h>
 #include <linux/watchdog.h>
+#include <linux/smp_lock.h>
 
 #include <asm/ebcdic.h>
 #include <asm/io.h>

@@ -131,11 +132,15 @@ static int __init vmwdt_probe(void)
 static int vmwdt_open(struct inode *i, struct file *f)
 {
 	int ret;
-	if (test_and_set_bit(0, &vmwdt_is_open))
+	lock_kernel();
+	if (test_and_set_bit(0, &vmwdt_is_open)) {
+		unlock_kernel();
 		return -EBUSY;
+	}
 	ret = vmwdt_keepalive();
 	if (ret)
 		clear_bit(0, &vmwdt_is_open);
+	unlock_kernel();
 	return ret ? ret : nonseekable_open(i, f);
 }

drivers/s390/crypto/zcrypt_api.c +3 -0

Index: linux-2.6/drivers/s390/crypto/zcrypt_api.c
===================================================================
@@ -34,6 +34,7 @@
 #include <linux/fs.h>
 #include <linux/proc_fs.h>
 #include <linux/compat.h>
+#include <linux/smp_lock.h>
 #include <asm/atomic.h>
 #include <asm/uaccess.h>
 #include <linux/hw_random.h>

@@ -300,7 +301,9 @@ static ssize_t zcrypt_write(struct file 
  */
 static int zcrypt_open(struct inode *inode, struct file *filp)
 {
+	lock_kernel();
 	atomic_inc(&zcrypt_open_count);
+	unlock_kernel();
 	return 0;
 }

drivers/sbus/char/cpwatchdog.c +4 -0

Index: linux-2.6/drivers/sbus/char/cpwatchdog.c
===================================================================
@@ -279,6 +279,7 @@ static inline int wd_opt_timeout(void)
 
 static int wd_open(struct inode *inode, struct file *f)
 {
+	lock_kernel();
 	switch(iminor(inode))
 	{
 		case WD0_MINOR:

@@ -291,6 +292,7 @@ static int wd_open(struct inode *inode, 
 			f->private_data = &wd_dev.watchdog[WD2_ID];
 			break;
 		default:
+			unlock_kernel();
 			return(-ENODEV);
 	}

@@ -304,11 +306,13 @@ static int wd_open(struct inode *inode, 
 						(void *)wd_dev.regs)) {
 			printk("%s: Cannot register IRQ %d\n", 
 				WD_OBPNAME, wd_dev.irq);
+			unlock_kernel();
 			return(-EBUSY);
 		}
 		wd_dev.initialized = 1;
 	}
 
+	unlock_kernel();
 	return(nonseekable_open(inode, f));
 }

drivers/sbus/char/display7seg.c +1 -0

Index: linux-2.6/drivers/sbus/char/display7seg.c
===================================================================
@@ -94,6 +94,7 @@ static int d7s_open(struct inode *inode,
 {
 	if (D7S_MINOR != iminor(inode))
 		return -ENODEV;
+	cycle_kernel_lock();
 	atomic_inc(&d7s_users);
 	return 0;
 }

drivers/sbus/char/envctrl.c +2 -0

Index: linux-2.6/drivers/sbus/char/envctrl.c
===================================================================
@@ -27,6 +27,7 @@
 #include <linux/miscdevice.h>
 #include <linux/kmod.h>
 #include <linux/reboot.h>
+#include <linux/smp_lock.h>
 
 #include <asm/ebus.h>
 #include <asm/uaccess.h>

@@ -694,6 +695,7 @@ envctrl_ioctl(struct file *file, unsigne
 static int
 envctrl_open(struct inode *inode, struct file *file)
 {
+	cycle_kernel_lock();
 	file->private_data = NULL;
 	return 0;
 }

drivers/sbus/char/flash.c +5 -1

Index: linux-2.6/drivers/sbus/char/flash.c
===================================================================
@@ -127,9 +127,13 @@ flash_read(struct file * file, char __us
 static int
 flash_open(struct inode *inode, struct file *file)
 {
-	if (test_and_set_bit(0, (void *)&flash.busy) != 0)
+	lock_kernel();
+	if (test_and_set_bit(0, (void *)&flash.busy) != 0) {
+		unlock_kernel();
 		return -EBUSY;
+	}
 
+	unlock_kernel();
 	return 0;
 }

drivers/sbus/char/jsflash.c +10 -3

Index: linux-2.6/drivers/sbus/char/jsflash.c
===================================================================
@@ -27,6 +27,7 @@
  */
 
 #include <linux/module.h>
+#include <linux/smp_lock.h>
 #include <linux/types.h>
 #include <linux/errno.h>
 #include <linux/miscdevice.h>

@@ -417,11 +418,17 @@ static int jsf_mmap(struct file * file, 
 
 static int jsf_open(struct inode * inode, struct file * filp)
 {
-
-	if (jsf0.base == 0) return -ENXIO;
-	if (test_and_set_bit(0, (void *)&jsf0.busy) != 0)
+	lock_kernel();
+	if (jsf0.base == 0) {
+		unlock_kernel();
+		return -ENXIO;
+	}
+	if (test_and_set_bit(0, (void *)&jsf0.busy) != 0) {
+		unlock_kernel();
 		return -EBUSY;
+	}
 
+	unlock_kernel();
 	return 0;	/* XXX What security? */
 }

drivers/sbus/char/openprom.c +3 -0

Index: linux-2.6/drivers/sbus/char/openprom.c
===================================================================
@@ -33,6 +33,7 @@
 #include <linux/kernel.h>
 #include <linux/errno.h>
 #include <linux/slab.h>
+#include <linux/smp_lock.h>
 #include <linux/string.h>
 #include <linux/miscdevice.h>
 #include <linux/init.h>

@@ -689,9 +690,11 @@ static int openprom_open(struct inode * 
 	if (!data)
 		return -ENOMEM;
 
+	lock_kernel();
 	data->current_node = of_find_node_by_path("/");
 	data->lastnode = data->current_node;
 	file->private_data = (void *) data;
+	unlock_kernel();
 
 	return 0;
 }

drivers/sbus/char/riowatchdog.c +2 -0

Index: linux-2.6/drivers/sbus/char/riowatchdog.c
===================================================================
@@ -11,6 +11,7 @@
 #include <linux/errno.h>
 #include <linux/init.h>
 #include <linux/miscdevice.h>
+#include <linux/smp_lock.h>
 
 #include <asm/io.h>
 #include <asm/ebus.h>

@@ -116,6 +117,7 @@ static void riowd_starttimer(void)
 
 static int riowd_open(struct inode *inode, struct file *filp)
 {
+	cycle_kernel_lock();
 	nonseekable_open(inode, filp);
 	return 0;
 }

drivers/sbus/char/rtc.c +3 -0

Index: linux-2.6/drivers/sbus/char/rtc.c
===================================================================
@@ -12,6 +12,7 @@
  */
 
 #include <linux/module.h>
+#include <linux/smp_lock.h>
 #include <linux/types.h>
 #include <linux/errno.h>
 #include <linux/miscdevice.h>

@@ -213,6 +214,7 @@ static int rtc_open(struct inode *inode,
 {
 	int ret;
 
+	lock_kernel();
 	spin_lock_irq(&mostek_lock);
 	if (rtc_busy) {
 		ret = -EBUSY;

@@ -221,6 +223,7 @@ static int rtc_open(struct inode *inode,
 		ret = 0;
 	}
 	spin_unlock_irq(&mostek_lock);
+	unlock_kernel();
 
 	return ret;
 }

drivers/sbus/char/uctrl.c +3 -0

Index: linux-2.6/drivers/sbus/char/uctrl.c
===================================================================
@@ -9,6 +9,7 @@
 #include <linux/delay.h>
 #include <linux/interrupt.h>
 #include <linux/slab.h>
+#include <linux/smp_lock.h>
 #include <linux/ioport.h>
 #include <linux/init.h>
 #include <linux/miscdevice.h>

@@ -211,8 +212,10 @@ uctrl_ioctl(struct inode *inode, struct 
 static int
 uctrl_open(struct inode *inode, struct file *file)
 {
+	lock_kernel();
 	uctrl_get_event_status();
 	uctrl_get_external_status();
+	unlock_kernel();
 	return 0;
 }

drivers/scsi/megaraid/megaraid_mm.c +2 -0

Index: linux-2.6/drivers/scsi/megaraid/megaraid_mm.c
===================================================================
@@ -15,6 +15,7 @@
  * Common management module
  */
 #include <linux/sched.h>
+#include <linux/smp_lock.h>
 #include "megaraid_mm.h"

@@ -96,6 +97,7 @@ mraid_mm_open(struct inode *inode, struc
 	 */
 	if (!capable(CAP_SYS_ADMIN)) return (-EACCES);
 
+	cycle_kernel_lock();
 	return 0;
 }

drivers/scsi/scsi_tgt_if.c +2 -0

Index: linux-2.6/drivers/scsi/scsi_tgt_if.c
===================================================================
@@ -21,6 +21,7 @@
  */
 #include <linux/miscdevice.h>
 #include <linux/file.h>
+#include <linux/smp_lock.h>
 #include <net/tcp.h>
 #include <scsi/scsi.h>
 #include <scsi/scsi_cmnd.h>

@@ -321,6 +322,7 @@ static int tgt_open(struct inode *inode,
 {
 	tx_ring.tr_idx = rx_ring.tr_idx = 0;
 
+	cycle_kernel_lock();
 	return 0;
 }

fs/dlm/user.c +8 -1

Index: linux-2.6/fs/dlm/user.c
===================================================================
@@ -15,6 +15,7 @@
 #include <linux/poll.h>
 #include <linux/signal.h>
 #include <linux/spinlock.h>
+#include <linux/smp_lock.h>
 #include <linux/dlm.h>
 #include <linux/dlm_device.h>

@@ -618,13 +619,17 @@ static int device_open(struct inode *ino
 	struct dlm_user_proc *proc;
 	struct dlm_ls *ls;
 
+	lock_kernel();
 	ls = dlm_find_lockspace_device(iminor(inode));
-	if (!ls)
+	if (!ls) {
+		unlock_kernel();
 		return -ENOENT;
+	}
 
 	proc = kzalloc(sizeof(struct dlm_user_proc), GFP_KERNEL);
 	if (!proc) {
 		dlm_put_lockspace(ls);
+		unlock_kernel();
 		return -ENOMEM;
 	}

@@ -636,6 +641,7 @@ static int device_open(struct inode *ino
 	spin_lock_init(&proc->locks_spin);
 	init_waitqueue_head(&proc->wait);
 	file->private_data = proc;
+	unlock_kernel();
 
 	return 0;
 }

@@ -870,6 +876,7 @@ static unsigned int device_poll(struct f
 
 static int ctl_device_open(struct inode *inode, struct file *file)
 {
+	cycle_kernel_lock();
 	file->private_data = NULL;
 	return 0;
 }

fs/ocfs2/stack_user.c +3 -0

Index: linux-2.6/fs/ocfs2/stack_user.c
===================================================================
@@ -21,6 +21,7 @@
 #include <linux/fs.h>
 #include <linux/miscdevice.h>
 #include <linux/mutex.h>
+#include <linux/smp_lock.h>
 #include <linux/reboot.h>
 #include <asm/uaccess.h>

@@ -619,10 +620,12 @@ static int ocfs2_control_open(struct ino
 		return -ENOMEM;
 	p->op_this_node = -1;
 
+	lock_kernel();
 	mutex_lock(&ocfs2_control_lock);
 	file->private_data = p;
 	list_add(&p->op_list, &ocfs2_control_private_list);
 	mutex_unlock(&ocfs2_control_lock);
+	unlock_kernel();
 
 	return 0;
 }

kernel/pm_qos_params.c +5 -1

Index: linux-2.6/kernel/pm_qos_params.c
===================================================================
@@ -358,15 +358,19 @@ static int pm_qos_power_open(struct inod
 	int ret;
 	long pm_qos_class;
 
+	lock_kernel();
 	pm_qos_class = find_pm_qos_object_by_minor(iminor(inode));
 	if (pm_qos_class >= 0) {
 		filp->private_data = (void *)pm_qos_class;
 		sprintf(name, "process_%d", current->pid);
 		ret = pm_qos_add_requirement(pm_qos_class, name,
 					PM_QOS_DEFAULT_VALUE);
-		if (ret >= 0)
+		if (ret >= 0) {
+			unlock_kernel();
 			return 0;
+		}
 	}
+	unlock_kernel();
 
 	return -EPERM;
 }

kernel/power/user.c +9 -1

Index: linux-2.6/kernel/power/user.c
===================================================================
@@ -9,6 +9,7 @@
  *
  */
 
+#include <linux/smp_lock.h>
 #include <linux/suspend.h>
 #include <linux/syscalls.h>
 #include <linux/reboot.h>

@@ -69,15 +70,20 @@ static int snapshot_open(struct inode *i
 	struct snapshot_data *data;
 	int error;
 
-	if (!atomic_add_unless(&snapshot_device_available, -1, 0))
+	lock_kernel();
+	if (!atomic_add_unless(&snapshot_device_available, -1, 0)) {
+		unlock_kernel();
 		return -EBUSY;
+	}
 
 	if ((filp->f_flags & O_ACCMODE) == O_RDWR) {
 		atomic_inc(&snapshot_device_available);
+		unlock_kernel();
 		return -ENOSYS;
 	}
 	if(create_basic_memory_bitmaps()) {
 		atomic_inc(&snapshot_device_available);
+		unlock_kernel();
 		return -ENOMEM;
 	}
 	nonseekable_open(inode, filp);

@@ -100,11 +106,13 @@ static int snapshot_open(struct inode *i
 	}
 	if (error) {
 		atomic_inc(&snapshot_device_available);
+		unlock_kernel();
 		return error;
 	}
 	data->frozen = 0;
 	data->ready = 0;
 	data->platform_support = 0;
+	unlock_kernel();
 
 	return 0;
 }

net/irda/irnet/irnet.h +1 -0

Index: linux-2.6/net/irda/irnet/irnet.h
===================================================================
@@ -241,6 +241,7 @@
 #include <linux/module.h>
 
 #include <linux/kernel.h>
+#include <linux/smp_lock.h>
 #include <linux/skbuff.h>
 #include <linux/tty.h>
 #include <linux/proc_fs.h>

net/irda/irnet/irnet_ppp.c +3 -0

Index: linux-2.6/net/irda/irnet/irnet_ppp.c
===================================================================
@@ -479,6 +479,7 @@ dev_irnet_open(struct inode *	inode,
   ap = kzalloc(sizeof(*ap), GFP_KERNEL);
   DABORT(ap == NULL, -ENOMEM, FS_ERROR, "Can't allocate struct irnet...\n");
 
+  lock_kernel();
   /* initialize the irnet structure */
   ap->file = file;

@@ -500,6 +501,7 @@ dev_irnet_open(struct inode *	inode,
     {
       DERROR(FS_ERROR, "Can't setup IrDA link...\n");
       kfree(ap);
+      unlock_kernel();
       return err;
     }

@@ -510,6 +512,7 @@ dev_irnet_open(struct inode *	inode,
   file->private_data = ap;
 
   DEXIT(FS_TRACE, " - ap=0x%p\n", ap);
+  unlock_kernel();
   return 0;
 }

Mike Frysinger

2008-05-20 00:08 UTC #37 lore raw

Re: [PATCH 1/3, RFC] misc char dev BKL pushdown

Quoted 30 lines · Arnd Bergmann

The Big Kernel Lock has been pushed down from chardev_open to misc_open, this change moves it to the individual misc driver open functions.
As before, the change was purely mechanical, most drivers should actually not need the BKL. In particular, we still hold the misc_mtx() while calling the open() function The patch should probably be split into one changeset per driver.
arch/blackfin/mach-bf561/coreb.c +2 -0
@@ -32,6 +32,7 @@
 #include <linux/device.h>
 #include <linux/ioport.h>
 #include <linux/module.h>
+#include <linux/smp_lock.h>
 #include <linux/uaccess.h>
 #include <linux/fs.h>
 #include <asm/dma.h>
@@ -196,6 +197,7 @@ static loff_t coreb_lseek(struct file *f
 
 static int coreb_open(struct inode *inode, struct file *file)
 {
+       lock_kernel();
       spin_lock_irq(&coreb_lock);
 
       if (coreb_status & COREB_IS_OPEN)
@@ -204,10 +206,12 @@ static int coreb_open(struct inode *inod
       coreb_status |= COREB_IS_OPEN;

arch/blackfin/mach-bf561/coreb.c static int coreb_open(struct inode *inod · line 216

Quoted 10 lines

arch/blackfin/mach-bf561/coreb.c +2 -0

 
       spin_unlock_irq(&coreb_lock);
+       unlock_kernel();
       return 0;
 
 out_busy:
       spin_unlock_irq(&coreb_lock);
+       unlock_kernel();
       return -EBUSY;
 }

this open func already has a spinlock protecting it. doesnt that mean we dont need the bkl in it ?
-mike

Jonathan Corbet

2008-05-20 00:21 UTC #38 lore raw

Re: [PATCH 1/3, RFC] misc char dev BKL pushdown

Quoted 2 lines · Mike Frysinger

this open func already has a spinlock protecting it. doesnt that mean we dont need the bkl in it ?

The existence of a spinlock is a good sign. But, until somebody has looked at the code and verified that said lock is really protecting everything, it's best to leave the BKL protection (which has always been there, just at a higher level) in place.

jon

Mike Frysinger

2008-05-20 00:46 UTC #39 lore raw

Re: [PATCH 1/3, RFC] misc char dev BKL pushdown

Quoted 8 lines · Jonathan Corbet

Quoted 2 lines · Mike Frysinger
this open func already has a spinlock protecting it. doesnt that mean we dont need the bkl in it ?
The existence of a spinlock is a good sign. But, until somebody has looked at the code and verified that said lock is really protecting everything, it's best to leave the BKL protection (which has always been there, just at a higher level) in place.

if the spinlock doesnt do what it's advertising (preventing mutual access), then the BKL is needed. if there's some UP behavior i'm not aware of, then the BKL is needed. otherwise, the BKL is not needed in this driver.

i should prob rewrite this driver anyways ... the open code could easily be replaced with some atomic funcs.
-mike

Alan Cox

Acked-by on v1 1/3

2008-05-20 08:59 UTC #40 lore raw

Re: [PATCH 1/3, RFC] misc char dev BKL pushdown

Quoted 11 lines · On Tue, 20 May 2008 01:26:28 +0200 Arnd Bergmann <[email protected]> wrote:

The Big Kernel Lock has been pushed down from chardev_open to misc_open, this change moves it to the individual misc driver open functions.
As before, the change was purely mechanical, most drivers should actually not need the BKL. In particular, we still hold the misc_mtx() while calling the open() function The patch should probably be split into one changeset per driver.
Signed-off-by Arnd Bergmann <[email protected]>

Acked-by Alan Cox <[email protected]>

Mike Frysinger

2008-05-20 23:01 UTC #41 lore raw

Re: [PATCH 1/3, RFC] misc char dev BKL pushdown

Quoted 35 lines · Arnd Bergmann

The Big Kernel Lock has been pushed down from chardev_open to misc_open, this change moves it to the individual misc driver open functions.
As before, the change was purely mechanical, most drivers should actually not need the BKL. In particular, we still hold the misc_mtx() while calling the open() function The patch should probably be split into one changeset per driver.
Signed-off-by Arnd Bergmann <[email protected]>
---
Index: linux-2.6/arch/blackfin/mach-bf561/coreb.c
===================================================================
arch/blackfin/mach-bf561/coreb.c +2 -0
@@ -32,6 +32,7 @@
 #include <linux/device.h>
 #include <linux/ioport.h>
 #include <linux/module.h>
+#include <linux/smp_lock.h>
 #include <linux/uaccess.h>
 #include <linux/fs.h>
 #include <asm/dma.h>
@@ -196,6 +197,7 @@ static loff_t coreb_lseek(struct file *f
 
 static int coreb_open(struct inode *inode, struct file *file)
 {
+       lock_kernel();
       spin_lock_irq(&coreb_lock);
 
       if (coreb_status & COREB_IS_OPEN)
@@ -204,10 +206,12 @@ static int coreb_open(struct inode *inod
       coreb_status |= COREB_IS_OPEN;

arch/blackfin/mach-bf561/coreb.c static int coreb_open(struct inode *inod · line 216

Quoted 10 lines

arch/blackfin/mach-bf561/coreb.c +2 -0

 
       spin_unlock_irq(&coreb_lock);
+       unlock_kernel();
       return 0;
 
 out_busy:
       spin_unlock_irq(&coreb_lock);
+       unlock_kernel();
       return -EBUSY;
 }

please drop the coreb.c changes from your patch
-mike

Jonathan Corbet

2008-05-20 23:25 UTC #42 lore raw

Re: [PATCH 1/3, RFC] misc char dev BKL pushdown

Quoted 1 line · Mike Frysinger

please drop the coreb.c changes from your patch

At a minimum, I would hope such a request would say something like "I've looked at the driver's locking and am convinced that the BKL is not needed." Have you done that? There is a certain leap of faith involved in removing that protection from a driver.

I decided to take a quick look...

- You use spin_lock_irq(&coreb_lock) in a number of places, but you do not take the lock in the interrupt handler. You also do not take the lock in coreb_write() or coreb_read(), so those can race with the interrupt handler, with ioctl(), and with each other.

- coreb_write() and coreb_read() do interruptible waits, but do not check to see whether they were interrupted. They will, in fact, continue in their I/O loops after a signal.

- In both functions you have:

	unsigned long p = *ppos;

	if (p + count > coreb_size)
		return -EFAULT;

that calculation can overflow.

- You also do this:

static ssize_t coreb_write(struct file *file, const char *buf, size_t count,
loff_t * ppos)
/* ... */
set_dma_start_addr(CH_MEM_STREAM2_SRC, (unsigned long)buf);

In other words, the DMA is done directly to/from a user-space address. Maybe that's safe on Blackfin, I don't know...

- I have no idea why some of your functions are using d_inode->i_mutex.

- In coreb_ioctl():

		spin_lock_irq(&coreb_lock);
		if (coreb_status & COREB_IS_RUNNING) {
			retval = -EBUSY;
			break;
		}

this will exit the function with the spinlock still held and interrupts disabled.

	case CMD_COREB_RESET:
		printk(KERN_INFO "Resetting Core B\n");
		bfin_write_SICB_SYSCR(bfin_read_SICB_SYSCR() | 0x0080);
		break;

You do not acquire the lock here, so this can race against other ioctl() calls. And ioctl() can race against read() and write().

Registration and such seem reasonable, so I can't come up with a scenario where loss of BKL protection will create trouble. Given the other problems there, though, I'll confess to being a bit nervous about it.

jon

Mike Frysinger

2008-05-21 16:22 UTC #43 lore raw

Re: [PATCH 1/3, RFC] misc char dev BKL pushdown

Quoted 8 lines · Jonathan Corbet

Quoted 1 line · Mike Frysinger
please drop the coreb.c changes from your patch
At a minimum, I would hope such a request would say something like "I've looked at the driver's locking and am convinced that the BKL is not needed." Have you done that? There is a certain leap of faith involved in removing that protection from a driver.

Quoted 6 lines

I decided to take a quick look...
- You use spin_lock_irq(&coreb_lock) in a number of places, but you do not take the lock in the interrupt handler. You also do not take the lock in coreb_write() or coreb_read(), so those can race with the interrupt handler, with ioctl(), and with each other.

the lock is to protect one thing: coreb_status. we lock around any access to it, so it not being grabbed in the irq handler or any other function where coreb_status is not utilized is irrelevant. that means the BKL is not needed in the driver.

the rest of your comments are more or less on target, but again irrelevant to the topic of the BKL. i'll keep them in mind when i rewrite the driver, thanks.
-mike

Arnd Bergmann

maintainer

2008-05-19 23:45 UTC #44 lore raw

RFC PATCH 3/3 remove BKL from misc_open()

In reply to Christoph Hellwig

Since all misc drivers that have an open() function now take the BKL in there, there is no longer the need to take it in the common misc_open() function.

Signed-off-by Arnd Bergmann <[email protected]>

---

drivers/char/misc.c +0 -3

Index: linux-2.6/drivers/char/misc.c
===================================================================
@@ -50,7 +50,6 @@
 #include <linux/device.h>
 #include <linux/tty.h>
 #include <linux/kmod.h>
-#include <linux/smp_lock.h>
 
 /*
  * Head entry for the doubly linked miscdevice list

@@ -121,7 +120,6 @@ static int misc_open(struct inode * inod
 	int err = -ENODEV;
 	const struct file_operations *old_fops, *new_fops = NULL;
 	
-	lock_kernel();
 	mutex_lock(&misc_mtx);
 	
 	list_for_each_entry(c, &misc_list, list) {

@@ -159,7 +157,6 @@ static int misc_open(struct inode * inod
 	fops_put(old_fops);
 fail:
 	mutex_unlock(&misc_mtx);
-	unlock_kernel();
 	return err;
 }

A message that is not in the archive (sent off-list or not yet received): [email protected]

Arnd Bergmann

maintainer

2008-05-19 23:46 UTC #46 lore raw

RFC PATCH 2/3 watchdog dev BKL pushdown

The Big Kernel Lock has been pushed down from chardev_open to misc_open, this change moves it to the individual watchdog driver open functions.

As before, the change was purely mechanical, most drivers should actually not need the BKL.

Signed-off-by Arnd Bergmann <[email protected]>

drivers/watchdog/acquirewdt.c +6 -1

Index: linux-2.6/drivers/watchdog/acquirewdt.c
===================================================================
@@ -64,6 +64,7 @@
 #include <linux/ioport.h>		/* For io-port access */
 #include <linux/platform_device.h>	/* For platform_driver framework */
 #include <linux/init.h>			/* For __init/__exit/... */
+#include <linux/smp_lock.h>		/* For lock_kernel() */
 
 #include <asm/uaccess.h>		/* For copy_to_user/put_user/... */
 #include <asm/io.h>			/* For inb/outb/... */

@@ -195,14 +196,18 @@ static int acq_ioctl(struct inode *inode
 
 static int acq_open(struct inode *inode, struct file *file)
 {
-	if (test_and_set_bit(0, &acq_is_open))
+	lock_kernel();
+	if (test_and_set_bit(0, &acq_is_open)) {
+		unlock_kernel();
 		return -EBUSY;
+	}
 
 	if (nowayout)
 		__module_get(THIS_MODULE);
 
 	/* Activate */
 	acq_keepalive();
+	unlock_kernel();
 	return nonseekable_open(inode, file);
 }

drivers/watchdog/advantechwdt.c +6 -1

Index: linux-2.6/drivers/watchdog/advantechwdt.c
===================================================================
@@ -30,6 +30,7 @@
 
 #include <linux/module.h>
 #include <linux/moduleparam.h>
+#include <linux/smp_lock.h>
 #include <linux/types.h>
 #include <linux/miscdevice.h>
 #include <linux/watchdog.h>

@@ -198,13 +199,17 @@ advwdt_ioctl(struct inode *inode, struct
 static int
 advwdt_open(struct inode *inode, struct file *file)
 {
-	if (test_and_set_bit(0, &advwdt_is_open))
+	lock_kernel();
+	if (test_and_set_bit(0, &advwdt_is_open)) {
+		unlock_kernel();
 		return -EBUSY;
+	}
 	/*
 	 *	Activate
 	 */
 
 	advwdt_ping();
+	unlock_kernel();
 	return nonseekable_open(inode, file);
 }

drivers/watchdog/alim1535_wdt.c +6 -1

Index: linux-2.6/drivers/watchdog/alim1535_wdt.c
===================================================================
@@ -9,6 +9,7 @@
 
 #include <linux/module.h>
 #include <linux/moduleparam.h>
+#include <linux/smp_lock.h>
 #include <linux/types.h>
 #include <linux/miscdevice.h>
 #include <linux/watchdog.h>

@@ -252,11 +253,15 @@ static int ali_ioctl(struct inode *inode
 static int ali_open(struct inode *inode, struct file *file)
 {
 	/* /dev/watchdog can only be opened once */
-	if (test_and_set_bit(0, &ali_is_open))
+	lock_kernel();
+	if (test_and_set_bit(0, &ali_is_open)) {
+		unlock_kernel();
 		return -EBUSY;
+	}
 
 	/* Activate */
 	ali_start();
+	unlock_kernel();
 	return nonseekable_open(inode, file);
 }

drivers/watchdog/alim7101_wdt.c +6 -1

Index: linux-2.6/drivers/watchdog/alim7101_wdt.c
===================================================================
@@ -21,6 +21,7 @@
 
 #include <linux/module.h>
 #include <linux/moduleparam.h>
+#include <linux/smp_lock.h>
 #include <linux/types.h>
 #include <linux/timer.h>
 #include <linux/miscdevice.h>

@@ -198,10 +199,14 @@ static ssize_t fop_write(struct file * f
 static int fop_open(struct inode * inode, struct file * file)
 {
 	/* Just in case we're already talking to someone... */
-	if(test_and_set_bit(0, &wdt_is_open))
+	lock_kernel();
+	if(test_and_set_bit(0, &wdt_is_open)) {
+		unlock_kernel();
 		return -EBUSY;
+	}
 	/* Good, fire up the show */
 	wdt_startup();
+	unlock_kernel();
 	return nonseekable_open(inode, file);
 }

drivers/watchdog/ar7_wdt.c +6 -1

Index: linux-2.6/drivers/watchdog/ar7_wdt.c
===================================================================
@@ -28,6 +28,7 @@
 #include <linux/errno.h>
 #include <linux/init.h>
 #include <linux/miscdevice.h>
+#include <linux/smp_lock.h>
 #include <linux/watchdog.h>
 #include <linux/notifier.h>
 #include <linux/reboot.h>

@@ -179,11 +180,15 @@ static void ar7_wdt_disable_wdt(void)
 static int ar7_wdt_open(struct inode *inode, struct file *file)
 {
 	/* only allow one at a time */
-	if (down_trylock(&open_semaphore))
+	lock_kernel();
+	if (down_trylock(&open_semaphore)) {
+		unlock_kernel();
 		return -EBUSY;
+	}
 	ar7_wdt_enable_wdt();
 	expect_close = 0;
 
+	unlock_kernel();
 	return nonseekable_open(inode, file);
 }

drivers/watchdog/at32ap700x_wdt.c +6 -1

Index: linux-2.6/drivers/watchdog/at32ap700x_wdt.c
===================================================================
@@ -32,6 +32,7 @@
 #include <linux/uaccess.h>
 #include <linux/io.h>
 #include <linux/spinlock.h>
+#include <linux/smp_lock.h>
 
 #define TIMEOUT_MIN		1
 #define TIMEOUT_MAX		2

@@ -131,10 +132,14 @@ static inline void at32_wdt_pat(void)
  */
 static int at32_wdt_open(struct inode *inode, struct file *file)
 {
-	if (test_and_set_bit(1, &wdt->users))
+	lock_kernel();
+	if (test_and_set_bit(1, &wdt->users)) {
+		unlock_kernel();
 		return -EBUSY;
+	}
 
 	at32_wdt_start();
+	unlock_kernel();
 	return nonseekable_open(inode, file);
 }

drivers/watchdog/at91rm9200_wdt.c +6 -1

Index: linux-2.6/drivers/watchdog/at91rm9200_wdt.c
===================================================================
@@ -18,6 +18,7 @@
 #include <linux/module.h>
 #include <linux/moduleparam.h>
 #include <linux/platform_device.h>
+#include <linux/smp_lock.h>
 #include <linux/types.h>
 #include <linux/watchdog.h>
 #include <asm/uaccess.h>

@@ -75,10 +76,14 @@ static void inline at91_wdt_reload(void)
  */
 static int at91_wdt_open(struct inode *inode, struct file *file)
 {
-	if (test_and_set_bit(0, &at91wdt_busy))
+	lock_kernel();
+	if (test_and_set_bit(0, &at91wdt_busy)) {
+		unlock_kernel();
 		return -EBUSY;
+	}
 
 	at91_wdt_start();
+	unlock_kernel();
 	return nonseekable_open(inode, file);
 }

drivers/watchdog/bfin_wdt.c +6 -1

Index: linux-2.6/drivers/watchdog/bfin_wdt.c
===================================================================
@@ -15,6 +15,7 @@
 #include <linux/platform_device.h>
 #include <linux/module.h>
 #include <linux/moduleparam.h>
+#include <linux/smp_lock.h>
 #include <linux/types.h>
 #include <linux/timer.h>
 #include <linux/miscdevice.h>

@@ -165,10 +166,13 @@ static int bfin_wdt_set_timeout(unsigned
  */
 static int bfin_wdt_open(struct inode *inode, struct file *file)
 {
+	lock_kernel();
 	stampit();
 
-	if (test_and_set_bit(0, &open_check))
+	if (test_and_set_bit(0, &open_check)) {
+		unlock_kernel();
 		return -EBUSY;
+	}
 
 	if (nowayout)
 		__module_get(THIS_MODULE);

@@ -176,6 +180,7 @@ static int bfin_wdt_open(struct inode *i
 	bfin_wdt_keepalive();
 	bfin_wdt_start();
 
+	unlock_kernel();
 	return nonseekable_open(inode, file);
 }

drivers/watchdog/booke_wdt.c +3 -0

Index: linux-2.6/drivers/watchdog/booke_wdt.c
===================================================================
@@ -18,6 +18,7 @@
 #include <linux/fs.h>
 #include <linux/miscdevice.h>
 #include <linux/notifier.h>
+#include <linux/smp_lock.h>
 #include <linux/watchdog.h>
 
 #include <asm/reg_booke.h>

@@ -137,12 +138,14 @@ static int booke_wdt_ioctl (struct inode
  */
 static int booke_wdt_open (struct inode *inode, struct file *file)
 {
+	lock_kernel();
 	if (booke_wdt_enabled == 0) {
 		booke_wdt_enabled = 1;
 		booke_wdt_enable();
 		printk (KERN_INFO "PowerPC Book-E Watchdog Timer Enabled (wdt_period=%d)\n",
 				booke_wdt_period);
 	}
+	unlock_kernel();
 
 	return nonseekable_open(inode, file);
 }

drivers/watchdog/cpu5wdt.c +6 -1

Index: linux-2.6/drivers/watchdog/cpu5wdt.c
===================================================================
@@ -21,6 +21,7 @@
 
 #include <linux/module.h>
 #include <linux/moduleparam.h>
+#include <linux/smp_lock.h>
 #include <linux/types.h>
 #include <linux/errno.h>
 #include <linux/miscdevice.h>

@@ -130,9 +131,13 @@ static int cpu5wdt_stop(void)
 
 static int cpu5wdt_open(struct inode *inode, struct file *file)
 {
-	if ( test_and_set_bit(0, &cpu5wdt_device.inuse) )
+	lock_kernel();
+	if ( test_and_set_bit(0, &cpu5wdt_device.inuse) ) {
+		unlock_kernel();
 		return -EBUSY;
+	}
 
+	unlock_kernel();
 	return nonseekable_open(inode, file);
 }

drivers/watchdog/davinci_wdt.c +6 -1

Index: linux-2.6/drivers/watchdog/davinci_wdt.c
===================================================================
@@ -13,6 +13,7 @@
 
 #include <linux/module.h>
 #include <linux/moduleparam.h>
+#include <linux/smp_lock.h>
 #include <linux/types.h>
 #include <linux/kernel.h>
 #include <linux/fs.h>

@@ -120,10 +121,14 @@ static void wdt_enable(void)
 
 static int davinci_wdt_open(struct inode *inode, struct file *file)
 {
-	if (test_and_set_bit(WDT_IN_USE, &wdt_status))
+	lock_kernel();
+	if (test_and_set_bit(WDT_IN_USE, &wdt_status)) {
+		unlock_kernel();
 		return -EBUSY;
+	}
 
 	wdt_enable();
+	unlock_kernel();
 
 	return nonseekable_open(inode, file);
 }

drivers/watchdog/ep93xx_wdt.c +6 -1

Index: linux-2.6/drivers/watchdog/ep93xx_wdt.c
===================================================================
@@ -26,6 +26,7 @@
 #include <linux/module.h>
 #include <linux/fs.h>
 #include <linux/miscdevice.h>
+#include <linux/smp_lock.h>
 #include <linux/watchdog.h>
 #include <linux/timer.h>

@@ -93,13 +94,17 @@ static void wdt_keepalive(void)
 
 static int ep93xx_wdt_open(struct inode *inode, struct file *file)
 {
-	if (test_and_set_bit(WDT_IN_USE, &wdt_status))
+	lock_kernel();
+	if (test_and_set_bit(WDT_IN_USE, &wdt_status)) {
+		unlock_kernel();
 		return -EBUSY;
+	}
 
 	clear_bit(WDT_OK_TO_CLOSE, &wdt_status);
 
 	wdt_startup();
 
+	unlock_kernel();
 	return nonseekable_open(inode, file);
 }

drivers/watchdog/eurotechwdt.c +6 -1

Index: linux-2.6/drivers/watchdog/eurotechwdt.c
===================================================================
@@ -48,6 +48,7 @@
 #include <linux/interrupt.h>
 #include <linux/module.h>
 #include <linux/moduleparam.h>
+#include <linux/smp_lock.h>
 #include <linux/types.h>
 #include <linux/miscdevice.h>
 #include <linux/watchdog.h>

@@ -300,11 +301,15 @@ static int eurwdt_ioctl(struct inode *in
 
 static int eurwdt_open(struct inode *inode, struct file *file)
 {
-	if (test_and_set_bit(0, &eurwdt_is_open))
+	lock_kernel();
+	if (test_and_set_bit(0, &eurwdt_is_open)) {
+		unlock_kernel();
 		return -EBUSY;
+	}
 	eurwdt_timeout = WDT_TIMEOUT;	/* initial timeout */
 	/* Activate the WDT */
 	eurwdt_activate_timer();
+	unlock_kernel();
 	return nonseekable_open(inode, file);
 }

drivers/watchdog/hpwdt.c +6 -1

Index: linux-2.6/drivers/watchdog/hpwdt.c
===================================================================
@@ -30,6 +30,7 @@
 #include <linux/pci_ids.h>
 #include <linux/reboot.h>
 #include <linux/sched.h>
+#include <linux/smp_lock.h>
 #include <linux/timer.h>
 #include <linux/types.h>
 #include <linux/uaccess.h>

@@ -486,12 +487,16 @@ static int hpwdt_change_timer(int new_ma
 static int hpwdt_open(struct inode *inode, struct file *file)
 {
 	/* /dev/watchdog can only be opened once */
-	if (test_and_set_bit(0, &hpwdt_is_open))
+	lock_kernel();
+	if (test_and_set_bit(0, &hpwdt_is_open)) {
+		unlock_kernel();
 		return -EBUSY;
+	}
 
 	/* Start the watchdog */
 	hpwdt_start();
 	hpwdt_ping();
+	unlock_kernel();
 
 	return nonseekable_open(inode, file);
 }

drivers/watchdog/i6300esb.c +6 -1

Index: linux-2.6/drivers/watchdog/i6300esb.c
===================================================================
@@ -28,6 +28,7 @@
  */
 
 #include <linux/module.h>
+#include <linux/smp_lock.h>
 #include <linux/types.h>
 #include <linux/kernel.h>
 #include <linux/fs.h>

@@ -198,12 +199,16 @@ static int esb_timer_read (void)
 static int esb_open (struct inode *inode, struct file *file)
 {
         /* /dev/watchdog can only be opened once */
-        if (test_and_set_bit(0, &timer_alive))
+	lock_kernel();
+        if (test_and_set_bit(0, &timer_alive)) {
+		unlock_kernel();
                 return -EBUSY;
+	}
 
         /* Reload and activate timer */
         esb_timer_keepalive ();
         esb_timer_start ();
+	unlock_kernel();
 
 	return nonseekable_open(inode, file);
 }

drivers/watchdog/iTCO_wdt.c +6 -1

Index: linux-2.6/drivers/watchdog/iTCO_wdt.c
===================================================================
@@ -62,6 +62,7 @@
 /* Includes */
 #include <linux/module.h>		/* For module specific items */
 #include <linux/moduleparam.h>		/* For new moduleparam's */
+#include <linux/smp_lock.h>		/* For lock_kernel */
 #include <linux/types.h>		/* For standard types (like size_t) */
 #include <linux/errno.h>		/* For the -ENODEV/... values */
 #include <linux/kernel.h>		/* For printk/panic/... */

@@ -453,14 +454,18 @@ static int iTCO_wdt_get_timeleft (int *t
 static int iTCO_wdt_open (struct inode *inode, struct file *file)
 {
 	/* /dev/watchdog can only be opened once */
-	if (test_and_set_bit(0, &is_active))
+	lock_kernel();
+	if (test_and_set_bit(0, &is_active)) {
+		unlock_kernel();
 		return -EBUSY;
+	}
 
 	/*
 	 *      Reload and activate timer
 	 */
 	iTCO_wdt_keepalive();
 	iTCO_wdt_start();
+	unlock_kernel();
 	return nonseekable_open(inode, file);
 }

drivers/watchdog/ib700wdt.c +4 -0

Index: linux-2.6/drivers/watchdog/ib700wdt.c
===================================================================
@@ -32,6 +32,7 @@
  */
 
 #include <linux/module.h>
+#include <linux/smp_lock.h>
 #include <linux/types.h>
 #include <linux/miscdevice.h>
 #include <linux/watchdog.h>

@@ -256,7 +257,9 @@ ibwdt_ioctl(struct inode *inode, struct 
 static int
 ibwdt_open(struct inode *inode, struct file *file)
 {
+	lock_kernel();
 	if (test_and_set_bit(0, &ibwdt_is_open)) {
+		unlock_kernel();
 		return -EBUSY;
 	}
 	if (nowayout)

@@ -264,6 +267,7 @@ ibwdt_open(struct inode *inode, struct f
 
 	/* Activate */
 	ibwdt_ping();
+	unlock_kernel();
 	return nonseekable_open(inode, file);
 }

drivers/watchdog/ibmasr.c +6 -1

Index: linux-2.6/drivers/watchdog/ibmasr.c
===================================================================
@@ -13,6 +13,7 @@
 #include <linux/fs.h>
 #include <linux/kernel.h>
 #include <linux/slab.h>
+#include <linux/smp_lock.h>
 #include <linux/module.h>
 #include <linux/pci.h>
 #include <linux/timer.h>

@@ -300,11 +301,15 @@ static int asr_ioctl(struct inode *inode
 
 static int asr_open(struct inode *inode, struct file *file)
 {
-	if(test_and_set_bit(0, &asr_is_open))
+	lock_kernel();
+	if(test_and_set_bit(0, &asr_is_open)) {
+		unlock_kernel();
 		return -EBUSY;
+	}
 
 	asr_toggle();
 	asr_enable();
+	unlock_kernel();
 
 	return nonseekable_open(inode, file);
 }

drivers/watchdog/indydog.c +6 -1

Index: linux-2.6/drivers/watchdog/indydog.c
===================================================================
@@ -13,6 +13,7 @@
 
 #include <linux/module.h>
 #include <linux/moduleparam.h>
+#include <linux/smp_lock.h>
 #include <linux/types.h>
 #include <linux/kernel.h>
 #include <linux/fs.h>

@@ -62,8 +63,11 @@ static void indydog_ping(void)
  */
 static int indydog_open(struct inode *inode, struct file *file)
 {
-	if (indydog_alive)
+	lock_kernel();
+	if (indydog_alive) {
+		unlock_kernel();
 		return -EBUSY;
+	}
 
 	if (nowayout)
 		__module_get(THIS_MODULE);

@@ -74,6 +78,7 @@ static int indydog_open(struct inode *in
 
 	indydog_alive = 1;
 	printk(KERN_INFO "Started watchdog timer.\n");
+	unlock_kernel();
 
 	return nonseekable_open(inode, file);
 }

drivers/watchdog/iop_wdt.c +6 -1

Index: linux-2.6/drivers/watchdog/iop_wdt.c
===================================================================
@@ -30,6 +30,7 @@
 #include <linux/init.h>
 #include <linux/device.h>
 #include <linux/miscdevice.h>
+#include <linux/smp_lock.h>
 #include <linux/watchdog.h>
 #include <linux/uaccess.h>
 #include <asm/hardware.h>

@@ -88,14 +89,18 @@ static int wdt_disable(void)
 
 static int iop_wdt_open(struct inode *inode, struct file *file)
 {
-	if (test_and_set_bit(WDT_IN_USE, &wdt_status))
+	lock_kernel();
+	if (test_and_set_bit(WDT_IN_USE, &wdt_status)) {
+		unlock_kernel();
 		return -EBUSY;
+	}
 
 	clear_bit(WDT_OK_TO_CLOSE, &wdt_status);
 
 	wdt_enable();
 
 	set_bit(WDT_ENABLED, &wdt_status);
+	unlock_kernel();
 
 	return nonseekable_open(inode, file);
 }

drivers/watchdog/it8712f_wdt.c +6 -1

Index: linux-2.6/drivers/watchdog/it8712f_wdt.c
===================================================================
@@ -30,6 +30,7 @@
 #include <linux/fs.h>
 #include <linux/pci.h>
 #include <linux/spinlock.h>
+#include <linux/smp_lock.h>
 
 #include <asm/uaccess.h>
 #include <asm/io.h>

@@ -305,10 +306,14 @@ it8712f_wdt_ioctl(struct inode *inode, s
 static int
 it8712f_wdt_open(struct inode *inode, struct file *file)
 {
+	lock_kernel();
 	/* only allow one at a time */
-	if (down_trylock(&it8712f_wdt_sem))
+	if (down_trylock(&it8712f_wdt_sem)) {
+		unlock_kernel();
 		return -EBUSY;
+	}
 	it8712f_wdt_enable();
+	unlock_kernel();
 
 	return nonseekable_open(inode, file);
 }

drivers/watchdog/ixp2000_wdt.c +6 -1

Index: linux-2.6/drivers/watchdog/ixp2000_wdt.c
===================================================================
@@ -18,6 +18,7 @@
 
 #include <linux/module.h>
 #include <linux/moduleparam.h>
+#include <linux/smp_lock.h>
 #include <linux/types.h>
 #include <linux/kernel.h>
 #include <linux/fs.h>

@@ -62,12 +63,16 @@ wdt_keepalive(void)
 static int
 ixp2000_wdt_open(struct inode *inode, struct file *file)
 {
-	if (test_and_set_bit(WDT_IN_USE, &wdt_status))
+	lock_kernel();
+	if (test_and_set_bit(WDT_IN_USE, &wdt_status)) {
+		unlock_kernel();
 		return -EBUSY;
+	}
 
 	clear_bit(WDT_OK_TO_CLOSE, &wdt_status);
 
 	wdt_enable();
+	unlock_kernel();
 
 	return nonseekable_open(inode, file);
 }

drivers/watchdog/ixp4xx_wdt.c +6 -1

Index: linux-2.6/drivers/watchdog/ixp4xx_wdt.c
===================================================================
@@ -15,6 +15,7 @@
 
 #include <linux/module.h>
 #include <linux/moduleparam.h>
+#include <linux/smp_lock.h>
 #include <linux/types.h>
 #include <linux/kernel.h>
 #include <linux/fs.h>

@@ -57,12 +58,16 @@ wdt_disable(void)
 static int
 ixp4xx_wdt_open(struct inode *inode, struct file *file)
 {
-	if (test_and_set_bit(WDT_IN_USE, &wdt_status))
+	lock_kernel();
+	if (test_and_set_bit(WDT_IN_USE, &wdt_status)) {
+		unlock_kernel();
 		return -EBUSY;
+	}
 
 	clear_bit(WDT_OK_TO_CLOSE, &wdt_status);
 
 	wdt_enable();
+	unlock_kernel();
 
 	return nonseekable_open(inode, file);
 }

drivers/watchdog/ks8695_wdt.c +6 -1

Index: linux-2.6/drivers/watchdog/ks8695_wdt.c
===================================================================
@@ -17,6 +17,7 @@
 #include <linux/module.h>
 #include <linux/moduleparam.h>
 #include <linux/platform_device.h>
+#include <linux/smp_lock.h>
 #include <linux/types.h>
 #include <linux/watchdog.h>
 #include <asm/io.h>

@@ -114,10 +115,14 @@ static int ks8695_wdt_settimeout(int new
  */
 static int ks8695_wdt_open(struct inode *inode, struct file *file)
 {
-	if (test_and_set_bit(0, &ks8695wdt_busy))
+	lock_kernel();
+	if (test_and_set_bit(0, &ks8695wdt_busy)) {
+		unlock_kernel();
 		return -EBUSY;
+	}
 
 	ks8695_wdt_start();
+	unlock_kernel();
 	return nonseekable_open(inode, file);
 }

drivers/watchdog/machzwd.c +4 -0

Index: linux-2.6/drivers/watchdog/machzwd.c
===================================================================
@@ -30,6 +30,7 @@
 
 #include <linux/module.h>
 #include <linux/moduleparam.h>
+#include <linux/smp_lock.h>
 #include <linux/types.h>
 #include <linux/timer.h>
 #include <linux/jiffies.h>

@@ -337,9 +338,11 @@ static int zf_ioctl(struct inode *inode,
 
 static int zf_open(struct inode *inode, struct file *file)
 {
+	lock_kernel();
 	spin_lock(&zf_lock);
 	if(test_and_set_bit(0, &zf_is_open)) {
 		spin_unlock(&zf_lock);
+		unlock_kernel();
 		return -EBUSY;
 	}

@@ -349,6 +352,7 @@ static int zf_open(struct inode *inode, 
 	spin_unlock(&zf_lock);
 
 	zf_timer_on();
+	unlock_kernel();
 
 	return nonseekable_open(inode, file);
 }

drivers/watchdog/mixcomwd.c +4 -0

Index: linux-2.6/drivers/watchdog/mixcomwd.c
===================================================================
@@ -44,6 +44,7 @@
 
 #include <linux/module.h>
 #include <linux/moduleparam.h>
+#include <linux/smp_lock.h>
 #include <linux/types.h>
 #include <linux/miscdevice.h>
 #include <linux/ioport.h>

@@ -129,7 +130,9 @@ static void mixcomwd_timerfun(unsigned l
 
 static int mixcomwd_open(struct inode *inode, struct file *file)
 {
+	lock_kernel();
 	if(test_and_set_bit(0,&mixcomwd_opened)) {
+		unlock_kernel();
 		return -EBUSY;
 	}
 	mixcomwd_ping();

@@ -147,6 +150,7 @@ static int mixcomwd_open(struct inode *i
 			mixcomwd_timer_alive=0;
 		}
 	}
+	unlock_kernel();
 	return nonseekable_open(inode, file);
 }

drivers/watchdog/mpc5200_wdt.c +6 -1

Index: linux-2.6/drivers/watchdog/mpc5200_wdt.c
===================================================================
@@ -1,6 +1,7 @@
 #include <linux/init.h>
 #include <linux/module.h>
 #include <linux/miscdevice.h>
+#include <linux/smp_lock.h>
 #include <linux/watchdog.h>
 #include <linux/io.h>
 #include <linux/spinlock.h>

@@ -137,14 +138,18 @@ static int mpc5200_wdt_ioctl(struct inod
 }
 static int mpc5200_wdt_open(struct inode *inode, struct file *file)
 {
+	lock_kernel();
 	/* /dev/watchdog can only be opened once */
-	if (test_and_set_bit(0, &is_active))
+	if (test_and_set_bit(0, &is_active)) {
+		unlock_kernel();
 		return -EBUSY;
+	}
 
 	/* Set and activate the watchdog */
 	mpc5200_wdt_set_timeout(wdt_global, 30);
 	mpc5200_wdt_start(wdt_global);
 	file->private_data = wdt_global;
+	unlock_kernel();
 	return nonseekable_open(inode, file);
 }
 static int mpc5200_wdt_release(struct inode *inode, struct file *file)

drivers/watchdog/mpc83xx_wdt.c +6 -1

Index: linux-2.6/drivers/watchdog/mpc83xx_wdt.c
===================================================================
@@ -21,6 +21,7 @@
 #include <linux/miscdevice.h>
 #include <linux/platform_device.h>
 #include <linux/module.h>
+#include <linux/smp_lock.h>
 #include <linux/watchdog.h>
 #include <asm/io.h>
 #include <asm/uaccess.h>

@@ -78,8 +79,11 @@ static ssize_t mpc83xx_wdt_write(struct 
 static int mpc83xx_wdt_open(struct inode *inode, struct file *file)
 {
 	u32 tmp = SWCRR_SWEN;
-	if (test_and_set_bit(0, &wdt_is_open))
+	lock_kernel();
+	if (test_and_set_bit(0, &wdt_is_open)) {
+		unlock_kernel();
 		return -EBUSY;
+	}
 
 	/* Once we start the watchdog we can't stop it */
 	__module_get(THIS_MODULE);

@@ -93,6 +97,7 @@ static int mpc83xx_wdt_open(struct inode
 	tmp |= timeout << 16;
 
 	out_be32(&wd_base->swcrr, tmp);
+	unlock_kernel();
 
 	return nonseekable_open(inode, file);
 }

drivers/watchdog/mpc8xx_wdt.c +6 -1

Index: linux-2.6/drivers/watchdog/mpc8xx_wdt.c
===================================================================
@@ -14,6 +14,7 @@
 #include <linux/kernel.h>
 #include <linux/miscdevice.h>
 #include <linux/module.h>
+#include <linux/smp_lock.h>
 #include <linux/watchdog.h>
 #include <asm/8xx_immap.h>
 #include <asm/uaccess.h>

@@ -51,11 +52,15 @@ static void mpc8xx_wdt_handler_enable(vo
 
 static int mpc8xx_wdt_open(struct inode *inode, struct file *file)
 {
-	if (test_and_set_bit(0, &wdt_opened))
+	lock_kernel();
+	if (test_and_set_bit(0, &wdt_opened)) {
+		unlock_kernel();
 		return -EBUSY;
+	}
 
 	m8xx_wdt_reset();
 	mpc8xx_wdt_handler_disable();
+	unlock_kernel();
 
 	return nonseekable_open(inode, file);
 }

drivers/watchdog/mpcore_wdt.c +6 -1

Index: linux-2.6/drivers/watchdog/mpcore_wdt.c
===================================================================
@@ -21,6 +21,7 @@
  */
 #include <linux/module.h>
 #include <linux/moduleparam.h>
+#include <linux/smp_lock.h>
 #include <linux/types.h>
 #include <linux/miscdevice.h>
 #include <linux/watchdog.h>

@@ -140,8 +141,11 @@ static int mpcore_wdt_open(struct inode 
 {
 	struct mpcore_wdt *wdt = platform_get_drvdata(mpcore_wdt_dev);
 
-	if (test_and_set_bit(0, &wdt->timer_alive))
+	lock_kernel();
+	if (test_and_set_bit(0, &wdt->timer_alive)) {
+		unlock_kernel();
 		return -EBUSY;
+	}
 
 	if (nowayout)
 		__module_get(THIS_MODULE);

@@ -152,6 +156,7 @@ static int mpcore_wdt_open(struct inode 
 	 *	Activate timer
 	 */
 	mpcore_wdt_start(wdt);
+	unlock_kernel();
 
 	return nonseekable_open(inode, file);
 }

drivers/watchdog/mtx-1_wdt.c +6 -1

Index: linux-2.6/drivers/watchdog/mtx-1_wdt.c
===================================================================
@@ -35,6 +35,7 @@
 
 #include <linux/module.h>
 #include <linux/moduleparam.h>
+#include <linux/smp_lock.h>
 #include <linux/types.h>
 #include <linux/errno.h>
 #include <linux/miscdevice.h>

@@ -120,8 +121,12 @@ static int mtx1_wdt_stop(void)
 
 static int mtx1_wdt_open(struct inode *inode, struct file *file)
 {
-	if (test_and_set_bit(0, &mtx1_wdt_device.inuse))
+	lock_kernel();
+	if (test_and_set_bit(0, &mtx1_wdt_device.inuse)) {
+		unlock_kernel();
 		return -EBUSY;
+	}
+	unlock_kernel();
 
 	return nonseekable_open(inode, file);
 }

drivers/watchdog/mv64x60_wdt.c +6 -1

Index: linux-2.6/drivers/watchdog/mv64x60_wdt.c
===================================================================
@@ -20,6 +20,7 @@
 #include <linux/kernel.h>
 #include <linux/miscdevice.h>
 #include <linux/module.h>
+#include <linux/smp_lock.h>
 #include <linux/watchdog.h>
 #include <linux/platform_device.h>

@@ -122,13 +123,17 @@ static void mv64x60_wdt_set_timeout(unsi
 
 static int mv64x60_wdt_open(struct inode *inode, struct file *file)
 {
-	if (test_and_set_bit(MV64x60_WDOG_FLAG_OPENED, &wdt_flags))
+	lock_kernel();
+	if (test_and_set_bit(MV64x60_WDOG_FLAG_OPENED, &wdt_flags)) {
+		unlock_kernel();
 		return -EBUSY;
+	}
 
 	if (nowayout)
 		__module_get(THIS_MODULE);
 
 	mv64x60_wdt_handler_enable();
+	unlock_kernel();
 
 	return nonseekable_open(inode, file);
 }

drivers/watchdog/omap_wdt.c +6 -1

Index: linux-2.6/drivers/watchdog/omap_wdt.c
===================================================================
@@ -27,6 +27,7 @@
  */
 
 #include <linux/module.h>
+#include <linux/smp_lock.h>
 #include <linux/types.h>
 #include <linux/kernel.h>
 #include <linux/fs.h>

@@ -122,8 +123,11 @@ static void omap_wdt_set_timeout(void)
 
 static int omap_wdt_open(struct inode *inode, struct file *file)
 {
-	if (test_and_set_bit(1, (unsigned long *)&omap_wdt_users))
+	lock_kernel();
+	if (test_and_set_bit(1, (unsigned long *)&omap_wdt_users)) {
+		unlock_kernel();
 		return -EBUSY;
+	}
 
 	if (cpu_is_omap16xx())
 		clk_enable(armwdt_ck);	/* Enable the clock */

@@ -142,6 +146,7 @@ static int omap_wdt_open(struct inode *i
 
 	omap_wdt_set_timeout();
 	omap_wdt_enable();
+	unlock_kernel();
 	return nonseekable_open(inode, file);
 }

drivers/watchdog/pc87413_wdt.c +6 -1

Index: linux-2.6/drivers/watchdog/pc87413_wdt.c
===================================================================
@@ -19,6 +19,7 @@
  */
 
 #include <linux/module.h>
+#include <linux/smp_lock.h>
 #include <linux/types.h>
 #include <linux/miscdevice.h>
 #include <linux/watchdog.h>

@@ -305,8 +306,11 @@ static int pc87413_open(struct inode *in
 {
 	/* /dev/watchdog can only be opened once */
 
-	if (test_and_set_bit(0, &timer_enabled))
+	lock_kernel();
+	if (test_and_set_bit(0, &timer_enabled)) {
+		unlock_kernel();
 		return -EBUSY;
+	}
 
 	if (nowayout)
 		__module_get(THIS_MODULE);

@@ -316,6 +320,7 @@ static int pc87413_open(struct inode *in
 
 	printk(KERN_INFO MODNAME "Watchdog enabled. Timeout set to"
 	                         " %d minute(s).\n", timeout);
+	unlock_kernel();
 
 	return nonseekable_open(inode, file);
 }

drivers/watchdog/pcwd.c +4 -0

Index: linux-2.6/drivers/watchdog/pcwd.c
===================================================================
@@ -51,6 +51,7 @@
 
 #include <linux/module.h>	/* For module specific items */
 #include <linux/moduleparam.h>	/* For new moduleparam's */
+#include <linux/smp_lock.h>	/* For lock_kernel */
 #include <linux/types.h>	/* For standard types (like size_t) */
 #include <linux/errno.h>	/* For the -ENODEV/... values */
 #include <linux/kernel.h>	/* For printk/panic/... */

@@ -682,10 +683,12 @@ static ssize_t pcwd_write(struct file *f
 
 static int pcwd_open(struct inode *inode, struct file *file)
 {
+	lock_kernel();
 	if (!atomic_dec_and_test(&open_allowed) ) {
 		if (debug >= VERBOSE)
 			printk(KERN_ERR PFX "Attempt to open already opened device.\n");
 		atomic_inc( &open_allowed );
+		unlock_kernel();
 		return -EBUSY;
 	}

@@ -695,6 +698,7 @@ static int pcwd_open(struct inode *inode
 	/* Activate */
 	pcwd_start();
 	pcwd_keepalive();
+	unlock_kernel();
 	return nonseekable_open(inode, file);
 }

drivers/watchdog/pcwd_pci.c +4 -0

Index: linux-2.6/drivers/watchdog/pcwd_pci.c
===================================================================
@@ -33,6 +33,7 @@
 
 #include <linux/module.h>	/* For module specific items */
 #include <linux/moduleparam.h>	/* For new moduleparam's */
+#include <linux/smp_lock.h>	/* For lock_kernel */
 #include <linux/types.h>	/* For standard types (like size_t) */
 #include <linux/errno.h>	/* For the -ENODEV/... values */
 #include <linux/kernel.h>	/* For printk/panic/... */

@@ -563,15 +564,18 @@ static int pcipcwd_ioctl(struct inode *i
 static int pcipcwd_open(struct inode *inode, struct file *file)
 {
 	/* /dev/watchdog can only be opened once */
+	lock_kernel();
 	if (test_and_set_bit(0, &is_active)) {
 		if (debug >= VERBOSE)
 			printk(KERN_ERR PFX "Attempt to open already opened device.\n");
+		unlock_kernel();
 		return -EBUSY;
 	}
 
 	/* Activate */
 	pcipcwd_start();
 	pcipcwd_keepalive();
+	unlock_kernel();
 	return nonseekable_open(inode, file);
 }

drivers/watchdog/pcwd_usb.c +6 -1

Index: linux-2.6/drivers/watchdog/pcwd_usb.c
===================================================================
@@ -26,6 +26,7 @@
 
 #include <linux/module.h>	/* For module specific items */
 #include <linux/moduleparam.h>	/* For new moduleparam's */
+#include <linux/smp_lock.h>	/* For lock_kernel */
 #include <linux/types.h>	/* For standard types (like size_t) */
 #include <linux/errno.h>	/* For the -ENODEV/... values */
 #include <linux/kernel.h>	/* For printk/panic/... */

@@ -460,12 +461,16 @@ static int usb_pcwd_ioctl(struct inode *
 static int usb_pcwd_open(struct inode *inode, struct file *file)
 {
 	/* /dev/watchdog can only be opened once */
-	if (test_and_set_bit(0, &is_active))
+	lock_kernel();
+	if (test_and_set_bit(0, &is_active)) {
+		unlock_kernel();
 		return -EBUSY;
+	}
 
 	/* Activate */
 	usb_pcwd_start(usb_pcwd_device);
 	usb_pcwd_keepalive(usb_pcwd_device);
+	unlock_kernel();
 	return nonseekable_open(inode, file);
 }

drivers/watchdog/pnx4008_wdt.c +6 -1

Index: linux-2.6/drivers/watchdog/pnx4008_wdt.c
===================================================================
@@ -16,6 +16,7 @@
 
 #include <linux/module.h>
 #include <linux/moduleparam.h>
+#include <linux/smp_lock.h>
 #include <linux/types.h>
 #include <linux/kernel.h>
 #include <linux/fs.h>

@@ -134,12 +135,16 @@ static void wdt_disable(void)
 
 static int pnx4008_wdt_open(struct inode *inode, struct file *file)
 {
-	if (test_and_set_bit(WDT_IN_USE, &wdt_status))
+	lock_kernel();
+	if (test_and_set_bit(WDT_IN_USE, &wdt_status)) {
+		unlock_kernel();
 		return -EBUSY;
+	}
 
 	clear_bit(WDT_OK_TO_CLOSE, &wdt_status);
 
 	wdt_enable();
+	unlock_kernel();
 
 	return nonseekable_open(inode, file);
 }

drivers/watchdog/rm9k_wdt.c +9 -2

Index: linux-2.6/drivers/watchdog/rm9k_wdt.c
===================================================================
@@ -28,6 +28,7 @@
 #include <linux/reboot.h>
 #include <linux/notifier.h>
 #include <linux/miscdevice.h>
+#include <linux/smp_lock.h>
 #include <linux/watchdog.h>
 #include <asm/io.h>
 #include <asm/atomic.h>

@@ -182,8 +183,11 @@ static int wdt_gpi_open(struct inode *in
 {
 	int res;
 
-	if (unlikely(atomic_dec_if_positive(&opencnt) < 0))
+	lock_kernel();
+	if (unlikely(atomic_dec_if_positive(&opencnt) < 0)) {
+		unlock_kernel();
 		return -EBUSY;
+	}
 
 	expect_close = 0;
 	if (locked) {

@@ -194,14 +198,17 @@ static int wdt_gpi_open(struct inode *in
 
 	res = request_irq(wd_irq, wdt_gpi_irqhdl, IRQF_SHARED | IRQF_DISABLED,
 			  wdt_gpi_name, &miscdev);
-	if (unlikely(res))
+	if (unlikely(res)) {
+		unlock_kernel();
 		return res;
+	}
 
 	wdt_gpi_set_timeout(timeout);
 	wdt_gpi_start();
 
 	printk(KERN_INFO "%s: watchdog started, timeout = %u seconds\n",
 		wdt_gpi_name, timeout);
+	unlock_kernel();
 	return nonseekable_open(inode, file);
 }

drivers/watchdog/s3c2410_wdt.c +6 -1

Index: linux-2.6/drivers/watchdog/s3c2410_wdt.c
===================================================================
@@ -37,6 +37,7 @@
 
 #include <linux/module.h>
 #include <linux/moduleparam.h>
+#include <linux/smp_lock.h>
 #include <linux/types.h>
 #include <linux/timer.h>
 #include <linux/miscdevice.h>

@@ -211,8 +212,11 @@ static int s3c2410wdt_set_heartbeat(int 
 
 static int s3c2410wdt_open(struct inode *inode, struct file *file)
 {
-	if(down_trylock(&open_lock))
+	lock_kernel();
+	if(down_trylock(&open_lock)) {
+		unlock_kernel();
 		return -EBUSY;
+	}
 
 	if (nowayout)
 		__module_get(THIS_MODULE);

@@ -221,6 +225,7 @@ static int s3c2410wdt_open(struct inode 
 
 	/* start the timer */
 	s3c2410wdt_start();
+	unlock_kernel();
 	return nonseekable_open(inode, file);
 }

drivers/watchdog/sa1100_wdt.c +6 -1

Index: linux-2.6/drivers/watchdog/sa1100_wdt.c
===================================================================
@@ -19,6 +19,7 @@
  */
 #include <linux/module.h>
 #include <linux/moduleparam.h>
+#include <linux/smp_lock.h>
 #include <linux/types.h>
 #include <linux/kernel.h>
 #include <linux/fs.h>

@@ -45,14 +46,18 @@ static int boot_status;
  */
 static int sa1100dog_open(struct inode *inode, struct file *file)
 {
-	if (test_and_set_bit(1,&sa1100wdt_users))
+	lock_kernel();
+	if (test_and_set_bit(1,&sa1100wdt_users)) {
+		unlock_kernel();
 		return -EBUSY;
+	}
 
 	/* Activate SA1100 Watchdog timer */
 	OSMR3 = OSCR + pre_margin;
 	OSSR = OSSR_M3;
 	OWER = OWER_WME;
 	OIER |= OIER_E3;
+	unlock_kernel();
 	return nonseekable_open(inode, file);
 }

drivers/watchdog/sb_wdog.c +4 -0

Index: linux-2.6/drivers/watchdog/sb_wdog.c
===================================================================
@@ -45,6 +45,7 @@
  */
 #include <linux/module.h>
 #include <linux/io.h>
+#include <linux/smp_lock.h>
 #include <linux/uaccess.h>
 #include <linux/fs.h>
 #include <linux/reboot.h>

@@ -96,8 +97,10 @@ static struct watchdog_info ident = {
  */
 static int sbwdog_open(struct inode *inode, struct file *file)
 {
+	lock_kernel();
 	nonseekable_open(inode, file);
 	if (test_and_set_bit(0, &sbwdog_gate)) {
+		unlock_kernel();
 		return -EBUSY;
 	}
 	__module_get(THIS_MODULE);

@@ -107,6 +110,7 @@ static int sbwdog_open(struct inode *ino
 	 */
 	sbwdog_set(user_dog, timeout);
 	__raw_writeb(1, user_dog);
+	unlock_kernel();
 
 	return 0;
 }

drivers/watchdog/sbc60xxwdt.c +6 -1

Index: linux-2.6/drivers/watchdog/sbc60xxwdt.c
===================================================================
@@ -46,6 +46,7 @@
 
 #include <linux/module.h>
 #include <linux/moduleparam.h>
+#include <linux/smp_lock.h>
 #include <linux/types.h>
 #include <linux/timer.h>
 #include <linux/jiffies.h>

@@ -191,15 +192,19 @@ static ssize_t fop_write(struct file * f
 
 static int fop_open(struct inode * inode, struct file * file)
 {
+	lock_kernel();
 	/* Just in case we're already talking to someone... */
-	if(test_and_set_bit(0, &wdt_is_open))
+	if(test_and_set_bit(0, &wdt_is_open)) {
+		unlock_kernel();
 		return -EBUSY;
+	}
 
 	if (nowayout)
 		__module_get(THIS_MODULE);
 
 	/* Good, fire up the show */
 	wdt_startup();
+	unlock_kernel();
 	return nonseekable_open(inode, file);
 }

drivers/watchdog/sbc7240_wdt.c +6 -1

Index: linux-2.6/drivers/watchdog/sbc7240_wdt.c
===================================================================
@@ -25,6 +25,7 @@
 #include <linux/miscdevice.h>
 #include <linux/notifier.h>
 #include <linux/reboot.h>
+#include <linux/smp_lock.h>
 #include <linux/types.h>
 #include <linux/watchdog.h>
 #include <asm/atomic.h>

@@ -136,10 +137,14 @@ static ssize_t fop_write(struct file *fi
 
 static int fop_open(struct inode *inode, struct file *file)
 {
-	if (test_and_set_bit(SBC7240_OPEN_STATUS_BIT, &wdt_status))
+	lock_kernel();
+	if (test_and_set_bit(SBC7240_OPEN_STATUS_BIT, &wdt_status)) {
+		unlock_kernel();
 		return -EBUSY;
+	}
 
 	wdt_enable();
+	unlock_kernel();
 
 	return nonseekable_open(inode, file);
 }

drivers/watchdog/sbc8360.c +4 -0

Index: linux-2.6/drivers/watchdog/sbc8360.c
===================================================================
@@ -37,6 +37,7 @@
  */
 
 #include <linux/module.h>
+#include <linux/smp_lock.h>
 #include <linux/types.h>
 #include <linux/miscdevice.h>
 #include <linux/watchdog.h>

@@ -257,9 +258,11 @@ static ssize_t sbc8360_write(struct file
 
 static int sbc8360_open(struct inode *inode, struct file *file)
 {
+	lock_kernel();
 	spin_lock(&sbc8360_lock);
 	if (test_and_set_bit(0, &sbc8360_is_open)) {
 		spin_unlock(&sbc8360_lock);
+		unlock_kernel();
 		return -EBUSY;
 	}
 	if (nowayout)

@@ -269,6 +272,7 @@ static int sbc8360_open(struct inode *in
 	spin_unlock(&sbc8360_lock);
 	sbc8360_activate();
 	sbc8360_ping();
+	unlock_kernel();
 	return nonseekable_open(inode, file);
 }

drivers/watchdog/sbc_epx_c3.c +6 -1

Index: linux-2.6/drivers/watchdog/sbc_epx_c3.c
===================================================================
@@ -15,6 +15,7 @@
 
 #include <linux/module.h>
 #include <linux/moduleparam.h>
+#include <linux/smp_lock.h>
 #include <linux/types.h>
 #include <linux/kernel.h>
 #include <linux/fs.h>

@@ -63,8 +64,11 @@ static void epx_c3_pet(void)
  */
 static int epx_c3_open(struct inode *inode, struct file *file)
 {
-	if (epx_c3_alive)
+	lock_kernel();
+	if (epx_c3_alive) {
+		unlock_kernel();
 		return -EBUSY;
+	}
 
 	if (nowayout)
 		__module_get(THIS_MODULE);

@@ -75,6 +79,7 @@ static int epx_c3_open(struct inode *ino
 
 	epx_c3_alive = 1;
 	printk(KERN_INFO "Started watchdog timer.\n");
+	unlock_kernel();
 
 	return nonseekable_open(inode, file);
 }

drivers/watchdog/sc1200wdt.c +6 -1

Index: linux-2.6/drivers/watchdog/sc1200wdt.c
===================================================================
@@ -33,6 +33,7 @@
 #include <linux/watchdog.h>
 #include <linux/ioport.h>
 #include <linux/spinlock.h>
+#include <linux/smp_lock.h>
 #include <linux/notifier.h>
 #include <linux/reboot.h>
 #include <linux/init.h>

@@ -151,14 +152,18 @@ static inline int sc1200wdt_status(void)
 static int sc1200wdt_open(struct inode *inode, struct file *file)
 {
 	/* allow one at a time */
-	if (down_trylock(&open_sem))
+	lock_kernel();
+	if (down_trylock(&open_sem)) {
+		unlock_kernel();
 		return -EBUSY;
+	}
 
 	if (timeout > MAX_TIMEOUT)
 		timeout = MAX_TIMEOUT;
 
 	sc1200wdt_start();
 	printk(KERN_INFO PFX "Watchdog enabled, timeout = %d min(s)", timeout);
+	unlock_kernel();
 
 	return nonseekable_open(inode, file);
 }

drivers/watchdog/sc520_wdt.c +6 -1

Index: linux-2.6/drivers/watchdog/sc520_wdt.c
===================================================================
@@ -55,6 +55,7 @@
 #include <linux/module.h>
 #include <linux/moduleparam.h>
 #include <linux/types.h>
+#include <linux/smp_lock.h>
 #include <linux/timer.h>
 #include <linux/miscdevice.h>
 #include <linux/watchdog.h>

@@ -249,13 +250,17 @@ static ssize_t fop_write(struct file * f
 static int fop_open(struct inode * inode, struct file * file)
 {
 	/* Just in case we're already talking to someone... */
-	if(test_and_set_bit(0, &wdt_is_open))
+	lock_kernel();
+	if(test_and_set_bit(0, &wdt_is_open)) {
+		unlock_kernel();
 		return -EBUSY;
+	}
 	if (nowayout)
 		__module_get(THIS_MODULE);
 
 	/* Good, fire up the show */
 	wdt_startup();
+	unlock_kernel();
 	return nonseekable_open(inode, file);
 }

drivers/watchdog/scx200_wdt.c +6 -1

Index: linux-2.6/drivers/watchdog/scx200_wdt.c
===================================================================
@@ -21,6 +21,7 @@
 #include <linux/moduleparam.h>
 #include <linux/init.h>
 #include <linux/miscdevice.h>
+#include <linux/smp_lock.h>
 #include <linux/watchdog.h>
 #include <linux/notifier.h>
 #include <linux/reboot.h>

@@ -92,9 +93,13 @@ static void scx200_wdt_disable(void)
 static int scx200_wdt_open(struct inode *inode, struct file *file)
 {
 	/* only allow one at a time */
-	if (down_trylock(&open_semaphore))
+	lock_kernel();
+	if (down_trylock(&open_semaphore)) {
+		unlock_kernel();
 		return -EBUSY;
+	}
 	scx200_wdt_enable();
+	unlock_kernel();
 
 	return nonseekable_open(inode, file);
 }

drivers/watchdog/shwdt.c +6 -1

Index: linux-2.6/drivers/watchdog/shwdt.c
===================================================================
@@ -22,6 +22,7 @@
 #include <linux/init.h>
 #include <linux/types.h>
 #include <linux/miscdevice.h>
+#include <linux/smp_lock.h>
 #include <linux/watchdog.h>
 #include <linux/reboot.h>
 #include <linux/notifier.h>

@@ -194,12 +195,16 @@ static void sh_wdt_ping(unsigned long da
  */
 static int sh_wdt_open(struct inode *inode, struct file *file)
 {
-	if (test_and_set_bit(0, &shwdt_is_open))
+	lock_kernel();
+	if (test_and_set_bit(0, &shwdt_is_open)) {
+		unlock_kernel();
 		return -EBUSY;
+	}
 	if (nowayout)
 		__module_get(THIS_MODULE);
 
 	sh_wdt_start();
+	unlock_kernel();
 
 	return nonseekable_open(inode, file);
 }

drivers/watchdog/smsc37b787_wdt.c +6 -1

Index: linux-2.6/drivers/watchdog/smsc37b787_wdt.c
===================================================================
@@ -45,6 +45,7 @@
 
 #include <linux/module.h>
 #include <linux/moduleparam.h>
+#include <linux/smp_lock.h>
 #include <linux/types.h>
 #include <linux/miscdevice.h>
 #include <linux/watchdog.h>

@@ -346,8 +347,11 @@ static int wb_smsc_wdt_open(struct inode
 {
 	/* /dev/watchdog can only be opened once */
 
-	if (test_and_set_bit(0, &timer_enabled))
+	lock_kernel();
+	if (test_and_set_bit(0, &timer_enabled)) {
+		unlock_kernel();
 		return -EBUSY;
+	}
 
 	if (nowayout)
 		__module_get(THIS_MODULE);

@@ -356,6 +360,7 @@ static int wb_smsc_wdt_open(struct inode
 	wb_smsc_wdt_enable();
 
 	printk(KERN_INFO MODNAME "Watchdog enabled. Timeout set to %d %s.\n", timeout, (unit == UNIT_SECOND) ? "second(s)" : "minute(s)");
+	unlock_kernel();
 
 	return nonseekable_open(inode, file);
 }

drivers/watchdog/softdog.c +6 -1

Index: linux-2.6/drivers/watchdog/softdog.c
===================================================================
@@ -38,6 +38,7 @@
 
 #include <linux/module.h>
 #include <linux/moduleparam.h>
+#include <linux/smp_lock.h>
 #include <linux/types.h>
 #include <linux/timer.h>
 #include <linux/miscdevice.h>

@@ -132,14 +133,18 @@ static int softdog_set_heartbeat(int t)
 
 static int softdog_open(struct inode *inode, struct file *file)
 {
-	if (test_and_set_bit(0, &driver_open))
+	lock_kernel();
+	if (test_and_set_bit(0, &driver_open)) {
+		unlock_kernel();
 		return -EBUSY;
+	}
 	if (!test_and_clear_bit(0, &orphan_timer))
 		__module_get(THIS_MODULE);
 	/*
 	 *	Activate timer
 	 */
 	softdog_keepalive();
+	unlock_kernel();
 	return nonseekable_open(inode, file);
 }

drivers/watchdog/txx9wdt.c +6 -1

Index: linux-2.6/drivers/watchdog/txx9wdt.c
===================================================================
@@ -9,6 +9,7 @@
  */
 #include <linux/module.h>
 #include <linux/moduleparam.h>
+#include <linux/smp_lock.h>
 #include <linux/types.h>
 #include <linux/miscdevice.h>
 #include <linux/watchdog.h>

@@ -70,8 +71,11 @@ static void txx9wdt_stop(void)
 
 static int txx9wdt_open(struct inode *inode, struct file *file)
 {
-	if (test_and_set_bit(0, &txx9wdt_alive))
+	lock_kernel();
+	if (test_and_set_bit(0, &txx9wdt_alive)) {
+		unlock_kernel();
 		return -EBUSY;
+	}
 
 	if (__raw_readl(&txx9wdt_reg->tcr) & TXx9_TMTCR_TCE) {
 		clear_bit(0, &txx9wdt_alive);

@@ -82,6 +86,7 @@ static int txx9wdt_open(struct inode *in
 		__module_get(THIS_MODULE);
 
 	txx9wdt_start();
+	unlock_kernel();
 	return nonseekable_open(inode, file);
 }

drivers/watchdog/w83627hf_wdt.c +6 -1

Index: linux-2.6/drivers/watchdog/w83627hf_wdt.c
===================================================================
@@ -28,6 +28,7 @@
 
 #include <linux/module.h>
 #include <linux/moduleparam.h>
+#include <linux/smp_lock.h>
 #include <linux/types.h>
 #include <linux/miscdevice.h>
 #include <linux/watchdog.h>

@@ -256,13 +257,17 @@ wdt_ioctl(struct inode *inode, struct fi
 static int
 wdt_open(struct inode *inode, struct file *file)
 {
-	if (test_and_set_bit(0, &wdt_is_open))
+	lock_kernel();
+	if (test_and_set_bit(0, &wdt_is_open)) {
+		unlock_kernel();
 		return -EBUSY;
+	}
 	/*
 	 *	Activate
 	 */
 
 	wdt_ping();
+	unlock_kernel();
 	return nonseekable_open(inode, file);
 }

drivers/watchdog/w83697hf_wdt.c +6 -1

Index: linux-2.6/drivers/watchdog/w83697hf_wdt.c
===================================================================
@@ -27,6 +27,7 @@
 
 #include <linux/module.h>
 #include <linux/moduleparam.h>
+#include <linux/smp_lock.h>
 #include <linux/types.h>
 #include <linux/miscdevice.h>
 #include <linux/watchdog.h>

@@ -280,13 +281,17 @@ wdt_ioctl(struct inode *inode, struct fi
 static int
 wdt_open(struct inode *inode, struct file *file)
 {
-	if (test_and_set_bit(0, &wdt_is_open))
+	lock_kernel();
+	if (test_and_set_bit(0, &wdt_is_open)) {
+		unlock_kernel();
 		return -EBUSY;
+	}
 	/*
 	 *	Activate
 	 */
 
 	wdt_enable();
+	unlock_kernel();
 	return nonseekable_open(inode, file);
 }

drivers/watchdog/w83877f_wdt.c +6 -1

Index: linux-2.6/drivers/watchdog/w83877f_wdt.c
===================================================================
@@ -41,6 +41,7 @@
 
 #include <linux/module.h>
 #include <linux/moduleparam.h>
+#include <linux/smp_lock.h>
 #include <linux/types.h>
 #include <linux/timer.h>
 #include <linux/jiffies.h>

@@ -214,11 +215,15 @@ static ssize_t fop_write(struct file * f
 static int fop_open(struct inode * inode, struct file * file)
 {
 	/* Just in case we're already talking to someone... */
-	if(test_and_set_bit(0, &wdt_is_open))
+	lock_kernel();
+	if(test_and_set_bit(0, &wdt_is_open)) {
+		unlock_kernel();
 		return -EBUSY;
+	}
 
 	/* Good, fire up the show */
 	wdt_startup();
+	unlock_kernel();
 	return nonseekable_open(inode, file);
 }

drivers/watchdog/w83977f_wdt.c +6 -1

Index: linux-2.6/drivers/watchdog/w83977f_wdt.c
===================================================================
@@ -17,6 +17,7 @@
 
 #include <linux/module.h>
 #include <linux/moduleparam.h>
+#include <linux/smp_lock.h>
 #include <linux/types.h>
 #include <linux/kernel.h>
 #include <linux/fs.h>

@@ -290,13 +291,17 @@ static int wdt_get_status(int *status)
 static int wdt_open(struct inode *inode, struct file *file)
 {
 	/* If the watchdog is alive we don't need to start it again */
-	if( test_and_set_bit(0, &timer_alive) )
+	lock_kernel();
+	if( test_and_set_bit(0, &timer_alive) ) {
+		unlock_kernel();
 		return -EBUSY;
+	}
 
 	if (nowayout)
 		__module_get(THIS_MODULE);
 
 	wdt_start();
+	unlock_kernel();
 	return nonseekable_open(inode, file);
 }

drivers/watchdog/wafer5823wdt.c +6 -1

Index: linux-2.6/drivers/watchdog/wafer5823wdt.c
===================================================================
@@ -29,6 +29,7 @@
 #include <linux/module.h>
 #include <linux/moduleparam.h>
 #include <linux/miscdevice.h>
+#include <linux/smp_lock.h>
 #include <linux/watchdog.h>
 #include <linux/fs.h>
 #include <linux/ioport.h>

@@ -181,13 +182,17 @@ static int wafwdt_ioctl(struct inode *in
 
 static int wafwdt_open(struct inode *inode, struct file *file)
 {
-	if (test_and_set_bit(0, &wafwdt_is_open))
+	lock_kernel();
+	if (test_and_set_bit(0, &wafwdt_is_open)) {
+		unlock_kernel();
 		return -EBUSY;
+	}
 
 	/*
 	 *      Activate
 	 */
 	wafwdt_start();
+	unlock_kernel();
 	return nonseekable_open(inode, file);
 }

drivers/watchdog/wdrtas.c +4 -0

Index: linux-2.6/drivers/watchdog/wdrtas.c
===================================================================
@@ -33,6 +33,7 @@
 #include <linux/module.h>
 #include <linux/notifier.h>
 #include <linux/reboot.h>
+#include <linux/smp_lock.h>
 #include <linux/types.h>
 #include <linux/watchdog.h>

@@ -403,13 +404,16 @@ static int
 wdrtas_open(struct inode *inode, struct file *file)
 {
 	/* only open once */
+	lock_kernel();
 	if (atomic_inc_return(&wdrtas_miscdev_open) > 1) {
 		atomic_dec(&wdrtas_miscdev_open);
+		unlock_kernel();
 		return -EBUSY;
 	}
 
 	wdrtas_timer_start();
 	wdrtas_timer_keepalive();
+	unlock_kernel();
 
 	return nonseekable_open(inode, file);
 }

drivers/watchdog/wdt.c +6 -1

Index: linux-2.6/drivers/watchdog/wdt.c
===================================================================
@@ -34,6 +34,7 @@
 #include <linux/interrupt.h>
 #include <linux/module.h>
 #include <linux/moduleparam.h>
+#include <linux/smp_lock.h>
 #include <linux/types.h>
 #include <linux/miscdevice.h>
 #include <linux/watchdog.h>

@@ -405,12 +406,16 @@ static int wdt_ioctl(struct inode *inode
 
 static int wdt_open(struct inode *inode, struct file *file)
 {
-	if(test_and_set_bit(0, &wdt_is_open))
+	lock_kernel();
+	if(test_and_set_bit(0, &wdt_is_open)) {
+		unlock_kernel();
 		return -EBUSY;
+	}
 	/*
 	 *	Activate
 	 */
 	wdt_start();
+	unlock_kernel();
 	return nonseekable_open(inode, file);
 }

drivers/watchdog/wdt285.c +9 -2

Index: linux-2.6/drivers/watchdog/wdt285.c
===================================================================
@@ -18,6 +18,7 @@
 #include <linux/module.h>
 #include <linux/moduleparam.h>
 #include <linux/types.h>
+#include <linux/smp_lock.h>
 #include <linux/kernel.h>
 #include <linux/fs.h>
 #include <linux/mm.h>

@@ -69,11 +70,16 @@ static int watchdog_open(struct inode *i
 {
 	int ret;
 
-	if (*CSR_SA110_CNTL & (1 << 13))
+	lock_kernel();
+	if (*CSR_SA110_CNTL & (1 << 13)) {
+		unlock_kernel();
 		return -EBUSY;
+	}
 
-	if (test_and_set_bit(1, &timer_alive))
+	if (test_and_set_bit(1, &timer_alive)) {
+		unlock_kernel();
 		return -EBUSY;
+	}
 
 	reload = soft_margin * (mem_fclk_21285 / 256);

@@ -98,6 +104,7 @@ static int watchdog_open(struct inode *i
 	ret = 0;
 #endif
 	nonseekable_open(inode, file);
+	unlock_kernel();
 	return ret;
 }

drivers/watchdog/wdt977.c +6 -1

Index: linux-2.6/drivers/watchdog/wdt977.c
===================================================================
@@ -24,6 +24,7 @@
 
 #include <linux/module.h>
 #include <linux/moduleparam.h>
+#include <linux/smp_lock.h>
 #include <linux/types.h>
 #include <linux/kernel.h>
 #include <linux/fs.h>

@@ -258,13 +259,17 @@ static int wdt977_get_status(int *status
 static int wdt977_open(struct inode *inode, struct file *file)
 {
 	/* If the watchdog is alive we don't need to start it again */
-	if( test_and_set_bit(0,&timer_alive) )
+	lock_kernel();
+	if (test_and_set_bit(0,&timer_alive)) {
+		unlock_kernel();
 		return -EBUSY;
+	}
 
 	if (nowayout)
 		__module_get(THIS_MODULE);
 
 	wdt977_start();
+	unlock_kernel();
 	return nonseekable_open(inode, file);
 }

drivers/watchdog/wdt_pci.c +6 -1

Index: linux-2.6/drivers/watchdog/wdt_pci.c
===================================================================
@@ -38,6 +38,7 @@
 #include <linux/interrupt.h>
 #include <linux/module.h>
 #include <linux/moduleparam.h>
+#include <linux/smp_lock.h>
 #include <linux/types.h>
 #include <linux/miscdevice.h>
 #include <linux/watchdog.h>

@@ -426,8 +427,11 @@ static int wdtpci_ioctl(struct inode *in
 
 static int wdtpci_open(struct inode *inode, struct file *file)
 {
-	if (down_trylock(&open_sem))
+	lock_kernel();
+	if (down_trylock(&open_sem)) {
+		unlock_kernel();
 		return -EBUSY;
+	}
 
 	if (nowayout) {
 		__module_get(THIS_MODULE);

@@ -436,6 +440,7 @@ static int wdtpci_open(struct inode *ino
 	 *	Activate
 	 */
 	wdtpci_start();
+	unlock_kernel();
 	return nonseekable_open(inode, file);
 }

Christoph Hellwig

2008-05-20 06:21 UTC #47 lore raw

Re: [PATCH 2/3, RFC] watchdog dev BKL pushdown

Quoted 6 lines · Arnd Bergmann

The Big Kernel Lock has been pushed down from chardev_open to misc_open, this change moves it to the individual watchdog driver open functions.
As before, the change was purely mechanical, most drivers should actually not need the BKL.

Actually I'd prefer to fix this for real. This single open stuff aswell as same set of ioctls are duplicated all over the watchdog drivers. We'd be much better off introducing a simple watchdog layer that handles this plus proper locking and convert drivers over to it gradually.

Arnd Bergmann

maintainer

2008-05-20 08:33 UTC #48 lore raw

Re: [PATCH 2/3, RFC] watchdog dev BKL pushdown

Quoted 5 lines · Christoph Hellwig

Quoted 4 lines · Arnd Bergmann
The Big Kernel Lock has been pushed down from chardev_open to misc_open, this change moves it to the individual watchdog driver open functions.

Quoted 7 lines

Quoted 2 lines
As before, the change was purely mechanical, most drivers should actually not need the BKL.
Actually I'd prefer to fix this for real. This single open stuff aswell as same set of ioctls are duplicated all over the watchdog drivers. We'd be much better off introducing a simple watchdog layer that handles this plus proper locking and convert drivers over to it gradually.

I fully agree, I thought the same thing when I did the patches. I remember that Wim had a git tree doing this, which is still active at
http://git.kernel.org/?p=linux/kernel/git/wim/linux-2.6-watchdog-experimental.git;a=commitdiff;h=732c54027e6c866f98857c4a6d1c6c466459dcd5

Unfortunately, it hasn't seen much activitity over the last two years, and the number of watchdog drivers seems to have exploded: I count 67 of them, including some outside of drivers/watchdog.

Wim, was there anything preventing you from integrating the generic watchdog layer back then?

	Arnd <><

Wim Van Sebroeck

2008-05-20 15:47 UTC #49 lore raw

Re: [PATCH 2/3, RFC] watchdog dev BKL pushdown

Hi Arnd,

Quoted 9 lines

Quoted 4 lines
Actually I'd prefer to fix this for real. This single open stuff aswell as same set of ioctls are duplicated all over the watchdog drivers. We'd be much better off introducing a simple watchdog layer that handles this plus proper locking and convert drivers over to it gradually.
I fully agree, I thought the same thing when I did the patches. I remember that Wim had a git tree doing this, which is still active at
http://git.kernel.org/?p=linux/kernel/git/wim/linux-2.6-watchdog-experimental.git;a=commitdiff;h=732c54027e6c866f98857c4a6d1c6c466459dcd5

Quoted 6 lines

Unfortunately, it hasn't seen much activitity over the last two years, and the number of watchdog drivers seems to have exploded: I count 67 of them, including some outside of drivers/watchdog.
Wim, was there anything preventing you from integrating the generic watchdog layer back then?

check the linux-2.6-watchdog-mm tree. You'll see the patches sitting there as the uniform watchdog driver. They are thus also in the -mm tree. I'll need to change it also to an unlocked_ioctl (and add documentation!) but I'll attach the core code below (This does not seem to be the latest code, because I know there was a request to change the alloc_watchdogdev code so that it could also allocate a private data-area/space).
But it gives you an idea where I was going to. Second step would then be to add a sysfs interface so that we can start handling mutiple devices.

Greetings,
Wim.

Quoted 1 line

From a223c170e5e7e63e5dd55f56837318db6ad0807f Mon Sep 17 00:00:00 2001

From: Wim Van Sebroeck <[email protected]>
Date: Sun, 19 Aug 2007 19:44:24 +0000
Subject: [PATCH] [WATCHDOG] Uniform Watchdog Device Driver

The Uniform Watchdog Device Driver is a frame-work that contains the common code for all watchdog-driver's. It also introduces a watchdog device structure and the operations that go with it.

Signed-off-by Wim Van Sebroeck <[email protected]>

---
 drivers/watchdog/Kconfig              |    2 +
 drivers/watchdog/Makefile             |    2 +
 drivers/watchdog/core/Kconfig         |   34 +++
 drivers/watchdog/core/Makefile        |   11 +
 drivers/watchdog/core/watchdog_core.c |  187 +++++++++++++
 drivers/watchdog/core/watchdog_dev.c  |  463 +++++++++++++++++++++++++++++++++
 include/linux/watchdog.h              |   49 ++++
 7 files changed, 748 insertions(+), 0 deletions(-)
 create mode 100644 drivers/watchdog/core/Kconfig
 create mode 100644 drivers/watchdog/core/Makefile
 create mode 100644 drivers/watchdog/core/watchdog_core.c
 create mode 100644 drivers/watchdog/core/watchdog_dev.c

drivers/watchdog/Kconfig +2 -0

index 37bddc1..a458e87 100644
@@ -37,6 +37,8 @@ config WATCHDOG_NOWAYOUT
 	  get killed. If you say Y here, the watchdog cannot be stopped once
 	  it has been started.
 
+source "drivers/watchdog/core/Kconfig"
+
 #
 # General Watchdog drivers
 #

drivers/watchdog/Makefile +2 -0

index 389f8b1..b61c103 100644
@@ -2,6 +2,8 @@
 # Makefile for the WatchDog device drivers.
 #
 
+obj-$(CONFIG_WATCHDOG)	+= core/
+
 # Only one watchdog can succeed. We probe the ISA/PCI/USB based
 # watchdog-cards first, then the architecture specific watchdog
 # drivers and then the architecture independant "softdog" driver.

drivers/watchdog/core/Kconfig new file +34 -0

new file mode 100644
index 0000000..a9a23a9
@@ -0,0 +1,34 @@
+#
+# Watchdog device driver core
+#
+
+if WATCHDOG
+
+config WATCHDOG_CORE
+	tristate "Uniform Watchdog Device Driver"
+	depends on EXPERIMENTAL
+	default m
+	---help---
+	  Say Y here if you want to use the new uniform watchdog device
+	  driver. This driver provides a framework for all watchdog
+	  device drivers and gives them the /dev/watchdog interface (and
+	  later also the sysfs interface).
+
+	  At this moment we have no watchdog device drivers using this new
+	  framework.
+
+	  To compile this driver as a module, choose M here: the module will
+	  be called watchdog_core.
+
+config WATCHDOG_DEBUG_CORE
+	bool "Uniform Watchdog Device Driver debugging output"
+	depends on WATCHDOG_CORE
+	default n
+	---help---
+	  Say Y here if you want the Uniform Watchdog Device Driver to
+	  produce debugging information. Select this if you are having a
+	  problem with the uniform watchdog device driver and want to see
+	  more of what is really happening.
+
+endif # WATCHDOG
+

drivers/watchdog/core/Makefile new file +7 -0

new file mode 100644
index 0000000..7554d35
@@ -0,0 +1,11 @@
+#
+# Makefile for the Watchdog Device Drivers generic core.
+#
+
+# The Generic Watchdog Driver
+obj-$(CONFIG_WATCHDOG_CORE)		+= watchdog_core.o watchdog_dev.o
+

drivers/watchdog/core/watchdog_core.c new file +187 -0

187 lines not on this page · raw message

+MODULE_SUPPORTED_DEVICE("watchdog");
+

drivers/watchdog/core/watchdog_dev.c new file +463 -0

463 lines not on this page · raw message

+MODULE_LICENSE("GPL");
+

include/linux/watchdog.h +49 -0

61 lines not on this page · raw message

Signature · 1 line

1.5.3.4

Alan Cox

2008-05-20 18:45 UTC #50 lore raw

Re: [PATCH 2/3, RFC] watchdog dev BKL pushdown

Review

Quoted 11 lines

(quoted diff) +11 -0

+int register_watchdogdevice(struct watchdog_device *dev, struct device *parent)
+{
+	int ret;
+
+	if (dev == NULL ||
+	    dev->watchdog_ops == NULL)
+		return -ENODATA;
+
+	if (dev->watchdog_ops->start == NULL ||
+	    dev->watchdog_ops->stop == NULL ||
+	    dev->watchdog_ops->keepalive == NULL)

Some watchdogs have no stop method so you need to allow for that (it means your device is always going to close 'nowayout' style).

Review

Quoted 5 lines

(quoted diff) +5 -0

+	if (dev->watchdog_ops->set_heartbeat) {
+		dev->options |= WDIOF_SETTIMEOUT;
+	} else {
+		dev->options &= ~WDIOF_SETTIMEOUT;
+	}

Should have been done by the driver anyway - so this is a WARN check really for debug.

Review

Quoted 3 lines

(quoted diff) +3 -0

+static int watchdog_ioctl(struct inode *inode, struct file *file,
+				unsigned int cmd, unsigned long arg)
+{

This ioctl code is racy

Quoted 4 lines

(quoted diff) +4 -0

+		if (new_options & WDIOS_DISABLECARD) {
+			/* only try to stop the watchdog if it's allready running */
+			if (watchdogdev->watchdog_state == WATCHDOG_STARTED) {
+				err =  watchdogdev->watchdog_ops->stop(watchdogdev);

Review

Quoted 10 lines

(quoted diff) +10 -0

+				if (err == 0) {
+					watchdogdev->watchdog_state = WATCHDOG_STOPPED;
+				} else {
+					printk(KERN_CRIT PFX "WDIOS_DISABLECARD not successfull! (err=%d)",
+						err);
+					return -EFAULT;
+				}
+			}
+		}
+

Consider two of these happening at once

Review

Quoted 7 lines

(quoted diff) +7 -0

+	/* only open if we have a valid watchdog device */
+	if (!watchdogdev ||
+	    !watchdogdev->watchdog_ops ||
+	    !watchdogdev->watchdog_ops->start ||
+	    !watchdogdev->watchdog_ops->stop ||
+	    !watchdogdev->watchdog_ops->keepalive)
+		return -EBUSY;

Races register - plus you don't need to check these cases

Also there is a problem with module locking as you don't lock down the driver module as your fops are owned by the watchdog_dev.

I'd actually been thinking along the same lines but not paid attention to your tree (was a bit busy without poking watchdog).

The patch below is actually quite similar in parts to yours except

- It handles the temperature device
- You pass it the ident and other structs
- It handles stop not being supported (and stop/start fails)
- It does all the reboot, nowayout and module locking and housekeeping
- I've used static objects for most things so they can be initialised by the compiler making more of the code "table filling".

The API is actually pretty similar so I think we agree on most parts but it might be worth working out which are the best bits of each implementation and putting together something nicer still:

Current patch (& example conversions) below. I'm still debating whether the watchdog core should implement its own mutex locking for the drivers so that only special case users (eg those with IRQ handlers) need to do their own private locking. That would further reduce mistakes by authors.

 drivers/watchdog/Kconfig        |   15 -
 drivers/watchdog/Makefile       |    8 -
 drivers/watchdog/alim1535_wdt.c |  289 ++++---------------------
 drivers/watchdog/alim7101_wdt.c |  218 +++----------------
 drivers/watchdog/softdog.c      |  211 ++++--------------
 drivers/watchdog/watchdog.c     |  296 ++++++++++++++++++++++++++
 drivers/watchdog/watchdog.h     |   35 +++
 drivers/watchdog/wdt.c          |  447 ++++++++-------------------------------
 8 files changed, 556 insertions(+), 963 deletions(-)
 create mode 100644 drivers/watchdog/watchdog.c
 create mode 100644 drivers/watchdog/watchdog.h

drivers/watchdog/Kconfig +0 ~~-15~~

index 254d115..b592ede 100644
@@ -828,21 +828,6 @@ config WDT
 	  To compile this driver as a module, choose M here: the
 	  module will be called wdt.
 
-config WDT_501
-	bool "WDT501 features"
-	depends on WDT
-	help
-	  Saying Y here and creating a character special file /dev/temperature
-	  with major number 10 and minor number 131 ("man mknod") will give
-	  you a thermometer inside your computer: reading from
-	  /dev/temperature yields one byte, the temperature in degrees
-	  Fahrenheit. This works only if you have a WDT501P watchdog board
-	  installed.
-
-	  If you want to enable the Fan Tachometer on the WDT501P, then you
-	  can do this via the tachometer parameter. Only do this if you have a
-	  fan tachometer actually set up.
-
 #
 # PCI-based Watchdog Cards
 #

drivers/watchdog/Makefile +4 -4

index f3fb170..1a9f2b7 100644
@@ -13,7 +13,7 @@
 # ISA-based Watchdog Cards
 obj-$(CONFIG_PCWATCHDOG) += pcwd.o
 obj-$(CONFIG_MIXCOMWD) += mixcomwd.o
-obj-$(CONFIG_WDT) += wdt.o
+obj-$(CONFIG_WDT) += wdt.o watchdog.o
 
 # PCI-based Watchdog Cards
 obj-$(CONFIG_PCIPCWATCHDOG) += pcwd_pci.o

@@ -57,8 +57,8 @@ obj-$(CONFIG_BFIN_WDT) += bfin_wdt.o
 # X86 (i386 + ia64 + x86_64) Architecture
 obj-$(CONFIG_ACQUIRE_WDT) += acquirewdt.o
 obj-$(CONFIG_ADVANTECH_WDT) += advantechwdt.o
-obj-$(CONFIG_ALIM1535_WDT) += alim1535_wdt.o
-obj-$(CONFIG_ALIM7101_WDT) += alim7101_wdt.o
+obj-$(CONFIG_ALIM1535_WDT) += alim1535_wdt.o softdog.o
+obj-$(CONFIG_ALIM7101_WDT) += alim7101_wdt.o softdog.o
 obj-$(CONFIG_SC520_WDT) += sc520_wdt.o
 obj-$(CONFIG_EUROTECH_WDT) += eurotechwdt.o
 obj-$(CONFIG_IB700_WDT) += ib700wdt.o

@@ -123,4 +123,4 @@ obj-$(CONFIG_SH_WDT) += shwdt.o
 # XTENSA Architecture
 
 # Architecture Independant
-obj-$(CONFIG_SOFT_WATCHDOG) += softdog.o
+obj-$(CONFIG_SOFT_WATCHDOG) += softdog.o watchdog.o

drivers/watchdog/alim1535_wdt.c +47 ~~-242~~

379 lines not on this page · raw message

drivers/watchdog/alim7101_wdt.c +39 ~~-179~~

284 lines not on this page · raw message

drivers/watchdog/softdog.c +41 ~~-170~~

276 lines not on this page · raw message

drivers/watchdog/watchdog.c new file +296 -0

296 lines not on this page · raw message

drivers/watchdog/watchdog.h new file +35 -0

new file mode 100644
index 0000000..0d22eaa
@@ -0,0 +1,35 @@
+struct watchdog;
+
+struct watchdog_ops
+{
+	int (*start)(struct watchdog *w);
+	int (*stop)(struct watchdog *w);
+	void (*ping)(struct watchdog *w);
+	int (*status)(struct watchdog *w);
+	int (*temperature)(struct watchdog *w);
+	int (*set_timeout)(struct watchdog *w, int t);
+	int (*reboot)(struct watchdog *w);
+	long (*ioctl)(struct watchdog *w, unsigned int cmd, unsigned long arg);
+};
+
+struct watchdog
+{
+	char *name;
+	const struct watchdog_info *info;
+	const struct watchdog_ops *ops;
+	int timeout;
+	int boot_status;
+	long status;
+#define WDOG_OPEN		0
+#define WDOG_EXPECT_RELEASE	1
+#define WDOG_ORPHAN		2
+#define WDOG_NO_WAY_OUT		3
+	struct module *owner;
+};
+
+
+extern void watchdog_unregister(struct watchdog *w);
+extern int watchdog_register(struct watchdog *w, int nwo);
+
+
+

drivers/watchdog/wdt.c +94 ~~-353~~

584 lines not on this page · raw message

Arnd Bergmann

maintainer

2008-05-20 21:01 UTC #51 lore raw

Re: [PATCH 2/3, RFC] watchdog dev BKL pushdown

Quoted 1 line · Alan Cox

Current patch (& example conversions) below.

Very nice code!

drivers/watchdog/watchdog.c line 35

Quoted 5 lines

drivers/watchdog/watchdog.c new file +5 -0

+       if (w->ops->ioctl) {
+               r = w->ops->ioctl(w, cmd, arg);
+               if (r != -ENOIOCTLCMD)
+                       return r;
+       }

Are you planning this as a transitional method for converting drivers, or are you aware of any driver that actually needs its own ioctl method?

drivers/watchdog/watchdog.c line 195

Quoted 8 lines

drivers/watchdog/watchdog.c new file +8 -0

+static const struct file_operations watchdog_fops = {
+       .owner          = THIS_MODULE,
+       .llseek         = no_llseek,
+       .write          = watchdog_write,
+       .unlocked_ioctl = watchdog_ioctl,
+       .open           = watchdog_open,
+       .release        = watchdog_release,
+};

All the ioctl numbers are compatible, so it would be good to register the watchdog ioctl function as compat_ioctl as well. Once all drivers are using the common abstraction, we can also kill their COMPATIBLE_IOCTL() entries in fs/compat_ioctl.c.

drivers/watchdog/watchdog.h

Quoted 2 lines

drivers/watchdog/watchdog.h new file +0 -0

There are a few watchdog drivers living outside of drivers/watchdog/, I could find:

* arch/um/drivers/harddog_kern.c
* drivers/char/ipmi/ipmi_watchdog.c
* drivers/rtc/rtc-m41t80.c
* drivers/s390/char/vmwatchdog.c
* drivers/sbus/char/cpwatchdog.c
* drivers/sbus/char/riowatchdog.c

In order to conver those to the new model, you either have to move them to the right place, or move the new declarations to include/linux/watchdog.h.

	Arnd <><

Alan Cox

2008-05-22 09:48 UTC #52 lore raw

Re: [PATCH 2/3, RFC] watchdog dev BKL pushdown

Quoted 3 lines

Are you planning this as a transitional method for converting drivers, or are you aware of any driver that actually needs its own ioctl method?

I added it "in case" and to allow for special cases later. We may not need it for any existing devices.

Quoted 5 lines

All the ioctl numbers are compatible, so it would be good to register the watchdog ioctl function as compat_ioctl as well. Once all drivers are using the common abstraction, we can also kill their COMPATIBLE_IOCTL() entries in fs/compat_ioctl.c.

Good point. Wim will no doubt comment on all this once he has finished his more pressing jobs.

Alan

Alan Cox

2008-05-20 09:27 UTC #53 lore raw

Re: [PATCH 2/3, RFC] watchdog dev BKL pushdown

Quoted 5 lines · On Tue, 20 May 2008 02:20:56 -0400 Christoph Hellwig <[email protected]> wrote:

Quoted 4 lines · Arnd Bergmann
The Big Kernel Lock has been pushed down from chardev_open to misc_open, this change moves it to the individual watchdog driver open functions.

Quoted 7 lines

Quoted 2 lines
As before, the change was purely mechanical, most drivers should actually not need the BKL.
Actually I'd prefer to fix this for real. This single open stuff aswell as same set of ioctls are duplicated all over the watchdog drivers. We'd be much better off introducing a simple watchdog layer that handles this plus proper locking and convert drivers over to it gradually.

In progress in two ways

- Wim was (is ?) working on a proper device layer

- I've cleaned up all the drivers and am now testing a watchdog driver supporting library which is taking about 50% of the code out of each driver I convert.

Alan

Alan Cox

2008-05-20 08:56 UTC #54 lore raw

Re: [PATCH 2/3, RFC] watchdog dev BKL pushdown

Quoted 3 lines · On Tue, 20 May 2008 01:14:23 +0200 Arnd Bergmann <[email protected]> wrote:

The Big Kernel Lock has been pushed down from chardev_open to misc_open, this change moves it to the individual watchdog driver open functions.

NAK. I've posted set of bigger patches which actually fix up all the watchdog locking code - and simply adding lock/unlock kernel isn't enough because of all the bugs in the code (it won't make it worse but it won't fix it either).

Alan

Jonathan Corbet

2008-05-20 15:13 UTC #55 lore raw

Re: [PATCH, RFC] char dev BKL pushdown

Quoted 3 lines · Arnd Bergmann

I've given it a try for all the misc drivers that have an open() function. The vast majority of them are actually watchdog drivers, all of which register as a misc device by themselves.

OK, it looks like the "misc" misc drivers patch can go into the bkl-removal tree, while the watchdog patches should not. What that means, I guess, is that the final misc_open() patch cannot go in at this point; Alan's watchdog stuff needs to find its way in first. Make sense?

Quoted 4 lines

You seem to already have a script to turn per-file changes into a patch each, so I'm sending you two patches: one for all the watchdog drivers (maybe Wim can take care of that as well) and one for all the other misc drivers (this one needs to be split).

Alas, I have no such script. I just committed each change as I made it
- each one required individual attention anyway. The misc changes look pretty straightforward, so I could probably hack up such a thing pretty quickly if you don't have a tree with broken out patches.

Thanks,

jon

Arnd Bergmann

maintainer

2008-05-20 17:22 UTC #56 lore raw

Re: [PATCH, RFC] char dev BKL pushdown

Quoted 11 lines · Jonathan Corbet

Quoted 3 lines · Arnd Bergmann
I've given it a try for all the misc drivers that have an open() function. The vast majority of them are actually watchdog drivers, all of which register as a misc device by themselves.
OK, it looks like the "misc" misc drivers patch can go into the bkl-removal tree, while the watchdog patches should not. What that means, I guess, is that the final misc_open() patch cannot go in at this point; Alan's watchdog stuff needs to find its way in first. Make sense?

Right, unless Alan or Wim are confident enough that removing the BKL won't break the drivers (more than they are today). Almost all of the open functions go along the lines of

int open(struct file *f, struct inode *i)

{
	if (wd_is_open)
		return -EBUSY;
	wd_is_open = 1;
	
	start_wd();

	return nonseekable_open(f, i);
}

nonseekable_open doesn't need the BKL by itself, and the wd_is_open variable is protected by the misc_mtx mutex.
I can't see any scenario in which start_wd() would need the BKL, or where a watchdog driver needs cycle_kernel_lock(), but I was't confident enough about that assessment, because I'm not really familiar with the drivers.

Quoted 9 lines

Quoted 4 lines
You seem to already have a script to turn per-file changes into a patch each, so I'm sending you two patches: one for all the watchdog drivers (maybe Wim can take care of that as well) and one for all the other misc drivers (this one needs to be split).
Alas, I have no such script. I just committed each change as I made it
- each one required individual attention anyway. The misc changes look pretty straightforward, so I could probably hack up such a thing pretty quickly if you don't have a tree with broken out patches.

I've done a semi-automated split and applied the patches on top of your tree. You can pull these from

git://git.kernel.org/pub/scm/linux/kernel/git/arnd/cell-2.6 bkl-removal

(I guess I should do a separate tree for it, will do that if more stuff comes up.)

	Arnd <><

Alan Cox

2008-05-20 19:05 UTC #57 lore raw

Re: [PATCH, RFC] char dev BKL pushdown

Quoted 15 lines

Right, unless Alan or Wim are confident enough that removing the BKL won't break the drivers (more than they are today). Almost all of the open functions go along the lines of
int open(struct file *f, struct inode *i)
{
	if (wd_is_open)
		return -EBUSY;
	wd_is_open = 1;
	
	start_wd();

	return nonseekable_open(f, i);
}

Quoted 3 lines

nonseekable_open doesn't need the BKL by itself, and the wd_is_open variable is protected by the misc_mtx mutex.
I can't see any scenario in which start_wd() would need the BKL, or

You need to review the use of misc_register(). Which is what I did already and sorted out for each watchdog - the job is done and completed and the various problem cases fixed. Watchdog has already been made BKL removal safe in the patch series I sent.

Alan

Linus Torvalds

2008-05-17 21:59 UTC #58 lore raw

Re: [PATCH, RFC] char dev BKL pushdown

Btw, Jonathan, would you be willing to maintain some kind of tree of these BKL removal patches?

This is different from the work Ingo is doing in the sense that these things should be (a) safe and reasonably obvious and thus (b) presumably ready to be merged in the next merge window. Ingo's BKL debugging tree is likely a good thing to use to find places that need work, but actually removing the BKL from some subsystem is a different issue.

(And when I say "safe and reasonably obvious" I obviously don't mean that there can't be bugs. Mistakes happen, and some BKL use might be overly subtle like the issue that Alan pointed out with an empty ->open routine almost accidentally serializing with initialization, but that's why I'd not merge these things after -rc2 anyway, but in the next merge window).

Because if you're willing to maintain a BKL-cleanup tree that gets merged into linux-next etc, I'd submit my VFAT/MSDOS BKL removal patch to you. The reason I did that one was that Thomas actually reported that to be a major source of latency problems on one of his embedded systems (80ms latency!), so it would be nice to have that patch in some place where it might get tested.

Linus

Jonathan Corbet

2008-05-18 20:07 UTC #59 lore raw

Re: [PATCH, RFC] char dev BKL pushdown

Quoted 2 lines · Linus Torvalds

Btw, Jonathan, would you be willing to maintain some kind of tree of these BKL removal patches?

Sure, I can do that - as long as people don't mind that committed to being with the in-laws and off the net for the first couple of weeks in June. I'll try to get the first version up shortly, after yet another pass over the chardev pushdown stuff.

jon

Andi Kleen

2008-05-14 22:12 UTC #60 lore raw

Quoted 13 lines · [email protected] (Jonathan Corbet) writes:

3: Provide a new form of cdev_add() which lets the driver indicate
that the BKL is not needed on open (or anything else?). At a minimum, it could just be a new parameter on cdev_add which has a value of zero or FIXME_I_STILL_NEED_BKL. Still some churn but easier to script and smaller because a lot of drivers are still using register_chrdev() - something else worth fixing.
A more involved form might provide a new chardev_add() which takes the new char_dev_ops structure too. Mapping between new and old operations vectors would be done internally to avoid breaking older drivers before they can be fixed.

Quoted 4 lines

4: Just find every char dev open() function and shove in lock_kernel()
calls, then remove the call from chrdev_open(). The disadvantage here is that, beyond lots of work and churn, there's no way to know which ones you missed.

In general when changing semantics drastically you should force compile errors by renaming the respective entry point. That has been the standard Linux method for this for years.

I've been also pondering a variant of 1, but 3 might be better.

Quoted 3 lines

I kind of like the combination of 2 and 3, done in such a way that there's no "every driver must change" flag day. This could be an interesting project, even... Thoughts?

I doubt it will be very interesting, but it would be useful. The goal less being to get rid of BKL in old drivers, but not requiring BKL in new drivers. Basically all BKL assumptions in interfaces really should go.

-Andi

Linus Torvalds

2008-05-14 22:17 UTC #61 lore raw

Quoted 4 lines · Andi Kleen

The goal less being to get rid of BKL in old drivers, but not requiring BKL in new drivers. Basically all BKL assumptions in interfaces really should go.

No, we really do want to get rid of BKL in old drivers too. Or at least in the interfaces.

Linus

Andi Kleen

2008-05-14 22:22 UTC #62 lore raw

Quoted 8 lines · Linus Torvalds

Quoted 3 lines · Andi Kleen
The goal less being to get rid of BKL in old drivers, but not requiring BKL in new drivers. Basically all BKL assumptions in interfaces really should go.
No, we really do want to get rid of BKL in old drivers too. Or at least in the interfaces.

In the interfaces definitely yes and all subsystems should have their own lock_kernel calls, but why in the old drivers? For those it's very unlikely they are used on any SMP system anyways (e.g. anything depending on CONFIG_ISA) or if they do only on 2 CPU systems.

Of course if you can find someone to do the work it wouldn't be bad, just wouldn't seem like a particularly useful investment of time to me.

Also it would be bad if the people who did such conversions didn't actually test it and that's a great danger with many old drivers because nearly nobody has the hardware (and if they do it won't be in a SMP system)

-Andi

Alan Cox

2008-05-15 13:42 UTC #63 lore raw

Quoted 3 lines

own lock_kernel calls, but why in the old drivers? For those it's very unlikely they are used on any SMP system anyways (e.g. anything depending on CONFIG_ISA) or if they do only on 2 CPU systems.

Because

- You need to verify the locking assumptions that remain are entirely driver internal
- At the point you achieve that you've done *ALL* the work required to add a driver specific lock

John Stoffel

2008-05-15 15:20 UTC #64 lore raw

In reply to Andi Kleen

Quoted 1 line

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Andi" == Andi Kleen <[email protected]> writes:

Quoted 8 lines · Andi> Linus Torvalds wrote:

Quoted 8 lines · Andi Kleen
Quoted 3 lines
The goal less being to get rid of BKL in old drivers, but not requiring BKL in new drivers. Basically all BKL assumptions in interfaces really should go.
No, we really do want to get rid of BKL in old drivers too. Or at least in the interfaces.

Andi> In the interfaces definitely yes and all subsystems should have Andi> their own lock_kernel calls, but why in the old drivers? For Andi> those it's very unlikely they are used on any SMP system anyways Andi> (e.g. anything depending on CONFIG_ISA) or if they do only on 2 Andi> CPU systems.

I'm still running an SMP server with ISA slots. I'd love to contribute testing and possibly coding to this effort, but realisticlly I'll be able to compile and boot stuff.

John

Andi Kleen

2008-05-15 15:11 UTC #65 lore raw

Quoted 10 lines · John Stoffel

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Andi" == Andi Kleen <[email protected]> writes:
Quoted 6 lines · Andi> Linus Torvalds wrote:
Quoted 6 lines · Andi Kleen
Quoted 3 lines
The goal less being to get rid of BKL in old drivers, but not requiring BKL in new drivers. Basically all BKL assumptions in interfaces really should go.
No, we really do want to get rid of BKL in old drivers too. Or at least in the interfaces.

Quoted 7 lines

Andi> In the interfaces definitely yes and all subsystems should have Andi> their own lock_kernel calls, but why in the old drivers? For Andi> those it's very unlikely they are used on any SMP system anyways Andi> (e.g. anything depending on CONFIG_ISA) or if they do only on 2 Andi> CPU systems.
I'm still running an SMP server with ISA slots.

I do too (although one CPU has died recently), but how many ISA devices do you use in it? Mine used to have a ISA ISDN card, but that was it and then no ISA anymore even though the slots are still in there.

Also on 2 CPU systems BKL is not that critical anyways. It only starts to hurt on larger CPU counts.

-Andi

John Stoffel

2008-05-15 15:27 UTC #66 lore raw

Quoted 1 line

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Andi" == Andi Kleen <[email protected]> writes:

Quoted 2 lines · Andi> John Stoffel wrote:

Quoted 2 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Andi" == Andi Kleen <[email protected]> writes:

Quoted 7 lines · Andi> Linus Torvalds wrote:

Quoted 7 lines
Quoted 6 lines
Quoted 6 lines · Andi Kleen
Quoted 3 lines
The goal less being to get rid of BKL in old drivers, but not requiring BKL in new drivers. Basically all BKL assumptions in interfaces really should go.
No, we really do want to get rid of BKL in old drivers too. Or at least in the interfaces.

Quoted 2 lines

Quoted 2 lines
I'm still running an SMP server with ISA slots.

Andi> I do too (although one CPU has died recently), but how many ISA Andi> devices do you use in it? Mine used to have a ISA ISDN card, but Andi> that was it and then no ISA anymore even though the slots are Andi> still in there.

I must admit I used to have an ISA Cyclades 8 port serial card running in there, but now it's all PCI stuff. It only had one ISA slot. So yes, ISA SMP boxes are slowly dying, but they'll be around for a long time to come.

Andi> Also on 2 CPU systems BKL is not that critical anyways. It only Andi> starts to hurt on larger CPU counts.

True, but with the growing number of multicore systems, esp on desktops, it's going to be an issue. How will the BKL work on quad core boxes?

I'm going to shutup now, since I don't have the knowledge to really contribute much to the discussion.

John

Andi Kleen

2008-05-15 15:38 UTC #67 lore raw

Quoted 4 lines

I must admit I used to have an ISA Cyclades 8 port serial card running in there, but now it's all PCI stuff. It only had one ISA slot. So yes, ISA SMP boxes are slowly dying, but they'll be around for a long time to come.

My point was that for those few users it's actually better to keep the BKL. If you try to remove it on some old driver you cannot test due to lack of hardware the risk of breaking the driver is higher than the gain you would get from removing it. The best you can do in this legacy code is to keep it running with minimal changes in the old tested state (although for some of it it's doubtful it still does actually)

Quoted 6 lines

Andi> Also on 2 CPU systems BKL is not that critical anyways. It only Andi> starts to hurt on larger CPU counts.
True, but with the growing number of multicore systems, esp on desktops, it's going to be an issue.

Multicore systems don't have ISA slots.

[yes I'm sure someone will tell me now about the ISA-over-USB device that exists. Don't bother, it doesn't add anything to the point]

-Andi

Jan Engelhardt

2008-05-15 08:45 UTC #68 lore raw

Quoted 6 lines · Jonathan Corbet

Sez Ingo:
1: We could add an unlocked_open() to the file_operations structure;
drivers could be converted over as they are verified not to need the BKL on open. Disadvantages are that it grows this structure for a relatively rare case - most open() calls already don't need the BKL. But it's a relatively easy path without flag days.

1b: add a .locked_open and move all BKL-requiring code to use that. When time comes and BKL is gone, .locked_open can be removed again, and no rename was ever done for BKL-free code.

Quoted 7 lines

2: Create a char_dev_ops structure for char devs and use it instead of
file_operations. I vaguely remember seeing Al mutter about that a while back. Quite a while back. This mirrors what was done with block devices, and makes some sense - there's a lot of stuff in struct file_operations which is not really applicable to char devs. Then struct char_dev_ops could have open() and locked_open(), with the latter destined for removal sometime around 2015 or so.

Iff you create a new char_dev_ops, don't clutter it with the old stuff. BKL-using code could continue using file_operations, would not it?

Quoted 2 lines

3: Provide a new form of cdev_add() which lets the driver indicate
that the BKL is not needed on open (or anything else?). At a

This is the BSD/Solaris tactic, heh :)

Diego Calleja

2008-05-15 14:54 UTC #69 lore raw

Quoted 3 lines · Jan Engelhardt

1b: add a .locked_open and move all BKL-requiring code to use that. When time comes and BKL is gone, .locked_open can be removed again, and no rename was ever done for BKL-free code.

1c: make the BKL-unsafe drivers depend on !SMP && !PREEMPT.

Alan Cox

2008-05-14 21:57 UTC #70 lore raw

Out of amusement I took the watchdog drivers and started looking for large cans of worms in the BKL drop arena.

Here is a fun one for general discussion - right now driver probe functions request resources. We have no ordering on the requests so we have deadlocks if two drivers do resource requests for conflicting resources in reverse order.

"Discuss"

Alan

Linus Torvalds

2008-05-14 22:13 UTC #71 lore raw

Quoted 5 lines · Alan Cox

Here is a fun one for general discussion - right now driver probe functions request resources. We have no ordering on the requests so we have deadlocks if two drivers do resource requests for conflicting resources in reverse order.

resource requests aren't blocking, so it wouldn't actually be a deadlock. It would just be a "both failed, try again".

That said, two drivers shouldn't be probing the same hardware at the same time regardless, so I can't imagine that it's much of a problem in real life.

Linus

Andi Kleen

2008-05-14 22:15 UTC #72 lore raw

In reply to Alan Cox

Quoted 7 lines · Alan Cox

Out of amusement I took the watchdog drivers and started looking for large cans of worms in the BKL drop arena.
Here is a fun one for general discussion - right now driver probe functions request resources. We have no ordering on the requests so we have deadlocks if two drivers do resource requests for conflicting resources in reverse order.

What deadlocks? resource allocation normally doesn't block. So if there's a ordering issue one of them will fail and should bail out.

That said if you have conflicting resources then failing is the correct behavior anyways.

-Andi

Linus Torvalds

2008-05-15 17:42 UTC #73 lore raw

So looking a bit more at your trivial fixups, I'd suggest strongly that they be re-organized a bit.

I cherry-picked your tty layer thing, because it was a real fix.

Quoted 10 lines · Ingo Molnar

net/sunrpc/sched.c +1 -0

index 6eab9bf..e12e571 100644
@@ -224,9 +224,15 @@ EXPORT_SYMBOL_GPL(rpc_destroy_wait_queue);
 
 static int rpc_wait_bit_killable(void *word)
 {
+	int bkl = kernel_locked();

net/sunrpc/sched.c EXPORT_SYMBOL_GPL(rpc_destroy_wait_queue); · line 237

Quoted 10 lines

net/sunrpc/sched.c +5 -0

+
 	if (fatal_signal_pending(current))
 		return -ERESTARTSYS;
+	if (bkl)
+		unlock_kernel();
 	schedule();
+	if (bkl)
+		lock_kernel();
 	return 0;
 }

The above doesn't even work in general. It depends on having just a single level of locking, and is ugly to boot. So wow about we just expose some version of

	depth = release_kernel_lock()
	..
	reacquire_kernel_lock(depth);

to existing BKL users as a way to safely release and re-aquire it regardless of depth. That makes the code more generic, but it *also* makes it more readable than that "if (bkl) [un]lock_kernel()" sequence.

Quoted 5 lines

commit 89c25297465376321cf54438d86441a5947bbd11
Author: Ingo Molnar <[email protected]>
Date:   Wed May 14 15:10:37 2008 +0200
remove the BKL: do not take the BKL in init code

I think this one should be changed - that comment says "do not take", but in fact you still do take it, you just release it earlier. So we should just not start out with the kernel locked in the first place. The BKL doesn't do anything for the init sequence anyway, since all of this is for code that runs before there even are any other threads (not counting the idle thread).

I don't see anything in there that could *possibly* depend on the kernel lock, and if there is anything, it would need to be fixed anyway.

Quoted 5 lines

commit 5fff2843de609b77d4590e87de5c976b8ac1aacd
Author: Ingo Molnar <[email protected]>
Date:   Wed May 14 14:30:33 2008 +0200
remove the BKL: procfs debug helper and BKL elimination

ACK. Code that relies on this is broken anyway.

We used to have lots of "proc_create()" followed by setup code that could race with "proc_lookup()", but they were fundamentally racy anyway, so this should happen regardless of any other BKL removal.

Quoted 5 lines

commit b07e615cf0f731d53a3ab431f44b1fe6ef4576e6
Author: Ingo Molnar <[email protected]>
Date:   Wed May 14 14:19:52 2008 +0200
remove the BKL: request_module() debug helper

See the above comment about "release/reaquire_kernel_lock()".

Quoted 5 lines

commit d31eec64e76a4b0795b5a6b57f2925d57aeefda5
Author: Ingo Molnar <[email protected]>
Date:   Wed May 14 13:47:58 2008 +0200
remove the BKL: tty updates

Same thing.

Quoted 5 lines

commit 3a0bf25bb160233b902962457ce917df27550850
Author: Ingo Molnar <[email protected]>
Date:   Wed May 14 11:34:13 2008 +0200
remove the BKL: reduce misc_open() BKL dependency

Same thing.

Quoted 5 lines

commit 79b2b296c31fa07e8868a6c622d766bb567f6655
Author: Ingo Molnar <[email protected]>
Date:   Wed May 14 11:30:35 2008 +0200
remove the BKL: change get_fs_type() BKL dependency

Same thing.

Quoted 5 lines

commit fc6f051a95c8774abb950f287b4b5e7f710f6977
Author: Ingo Molnar <[email protected]>
Date:   Wed May 14 09:51:42 2008 +0200
revert ("BKL: revert back to the old spinlock implementation")

And obviously this one I'd never take. It would need to work with a working BKL implementation.

Linus

Arjan van de Ven

2008-05-15 20:27 UTC #74 lore raw

Quoted 29 lines · On Thu, 15 May 2008 10:41:54 -0700 (PDT) Linus Torvalds…

So looking a bit more at your trivial fixups, I'd suggest strongly that they be re-organized a bit.
Quoted 10 lines
net/sunrpc/sched.c +1 -0
index 6eab9bf..e12e571 100644
@@ -224,9 +224,15 @@ EXPORT_SYMBOL_GPL(rpc_destroy_wait_queue);
 
 static int rpc_wait_bit_killable(void *word)
 {
+	int bkl = kernel_locked();
net/sunrpc/sched.c EXPORT_SYMBOL_GPL(rpc_destroy_wait_queue); · line 237
Quoted 10 lines
net/sunrpc/sched.c +5 -0
+
 	if (fatal_signal_pending(current))
 		return -ERESTARTSYS;
+	if (bkl)
+		unlock_kernel();
 	schedule();
+	if (bkl)
+		lock_kernel();
 	return 0;
 }
The above doesn't even work in general. It depends on having just a single level of locking, and is ugly to boot. So wow about we just expose some version of

Quoted 8 lines

	depth = release_kernel_lock()
	..
	reacquire_kernel_lock(depth);
to existing BKL users as a way to safely release and re-aquire it regardless of depth. That makes the code more generic, but it *also* makes it more readable than that "if (bkl) [un]lock_kernel()" sequence.

can we make this even more specific/restricted? Like having something like

call_bkl_unlocked(function_pointer, argument);

or something that will internally do the full unlock and then the function call. The last thing we need is another nailgun that BKL using code can use to staple themselves to something big and fast moving. By having a more restricted interface... less likely.
Maybe we can even get away with only a

drop_bkl_and_schedule();

and nothing else.

Peter Zijlstra

2008-05-15 20:46 UTC #75 lore raw

Quoted 53 lines · Arjan van de Ven

Quoted 29 lines · On Thu, 15 May 2008 10:41:54 -0700 (PDT) Linus Torvalds…
So looking a bit more at your trivial fixups, I'd suggest strongly that they be re-organized a bit.
Quoted 10 lines
net/sunrpc/sched.c +1 -0
index 6eab9bf..e12e571 100644
@@ -224,9 +224,15 @@ EXPORT_SYMBOL_GPL(rpc_destroy_wait_queue);
 
 static int rpc_wait_bit_killable(void *word)
 {
+	int bkl = kernel_locked();
net/sunrpc/sched.c EXPORT_SYMBOL_GPL(rpc_destroy_wait_queue); · line 237
Quoted 10 lines
net/sunrpc/sched.c +5 -0
+
 	if (fatal_signal_pending(current))
 		return -ERESTARTSYS;
+	if (bkl)
+		unlock_kernel();
 	schedule();
+	if (bkl)
+		lock_kernel();
 	return 0;
 }
The above doesn't even work in general. It depends on having just a single level of locking, and is ugly to boot. So wow about we just expose some version of
Quoted 8 lines
	depth = release_kernel_lock()
	..
	reacquire_kernel_lock(depth);
to existing BKL users as a way to safely release and re-aquire it regardless of depth. That makes the code more generic, but it *also* makes it more readable than that "if (bkl) [un]lock_kernel()" sequence.
can we make this even more specific/restricted? Like having something like
call_bkl_unlocked(function_pointer, argument);
or something that will internally do the full unlock and then the function call. The last thing we need is another nailgun that BKL using code can use to staple themselves to something big and fast moving. By having a more restricted interface... less likely.
Maybe we can even get away with only a

Quoted 3 lines

drop_bkl_and_schedule();
and nothing else.

No, that would defeat the whole purpose of the exercise. This drop on schedule property makes it possible to have inverse lock order and not deadlock.

That also makes the manual re-acquire on a different level pretty ugly and deadlock prone.

The whole purpose of this patch series was to get rid of this exact problem so that the BKL turns into something that resembles a normal lock within the regular locking hierarchy.

Arjan van de Ven

2008-05-15 21:22 UTC #76 lore raw

Quoted 54 lines · On Thu, 15 May 2008 22:45:55 +0200 Peter Zijlstra <[email protected]>…

Quoted 53 lines · Arjan van de Ven
Quoted 29 lines · On Thu, 15 May 2008 10:41:54 -0700 (PDT) Linus Torvalds…
So looking a bit more at your trivial fixups, I'd suggest strongly that they be re-organized a bit.
Quoted 10 lines
net/sunrpc/sched.c +1 -0
index 6eab9bf..e12e571 100644
@@ -224,9 +224,15 @@ EXPORT_SYMBOL_GPL(rpc_destroy_wait_queue);
 
 static int rpc_wait_bit_killable(void *word)
 {
+	int bkl = kernel_locked();
net/sunrpc/sched.c EXPORT_SYMBOL_GPL(rpc_destroy_wait_queue); · line 237
Quoted 10 lines
net/sunrpc/sched.c +5 -0
+
 	if (fatal_signal_pending(current))
 		return -ERESTARTSYS;
+	if (bkl)
+		unlock_kernel();
 	schedule();
+	if (bkl)
+		lock_kernel();
 	return 0;
 }
The above doesn't even work in general. It depends on having just a single level of locking, and is ugly to boot. So wow about we just expose some version of
Quoted 8 lines
	depth = release_kernel_lock()
	..
	reacquire_kernel_lock(depth);
to existing BKL users as a way to safely release and re-aquire it regardless of depth. That makes the code more generic, but it *also* makes it more readable than that "if (bkl) [un]lock_kernel()" sequence.
can we make this even more specific/restricted? Like having something like
call_bkl_unlocked(function_pointer, argument);
or something that will internally do the full unlock and then the function call. The last thing we need is another nailgun that BKL using code can use to staple themselves to something big and fast moving. By having a more restricted interface... less likely. Maybe we can even get away with only a

Quoted 7 lines

Quoted 3 lines
drop_bkl_and_schedule();
and nothing else.
No, that would defeat the whole purpose of the exercise. This drop on schedule property makes it possible to have inverse lock order and not deadlock.

I would totally agree with you, except that all these patches effectively do it manually again ANYWAY :(

so what I propose is make it explicit drop_bkl_and_schedule() call only, and only do them as a very very last resort.

For 99% of the rest it does give exactly the regular benefits you describe. And we can then prioritize these ugly cases to get de-bkl'd first.

Kevin Winchester

2008-05-17 00:15 UTC #77 lore raw