misc_open() is already serialized with misc_mtx. Remove the BKL
locking which got there via the BKL pushdown.
Signed-off-by: Thomas Gleixner <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
---
drivers/char/misc.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
Index: linux-2.6-tip/drivers/char/misc.c
===================================================================
--- linux-2.6-tip.orig/drivers/char/misc.c
+++ linux-2.6-tip/drivers/char/misc.c
@@ -49,7 +49,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
@@ -118,8 +117,7 @@ static int misc_open(struct inode * inod
struct miscdevice *c;
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) {
@@ -157,7 +155,6 @@ static int misc_open(struct inode * inod
fops_put(old_fops);
fail:
mutex_unlock(&misc_mtx);
- unlock_kernel();
return err;
}
On Saturday 10 October 2009, Thomas Gleixner wrote:
> misc_open() is already serialized with misc_mtx. Remove the BKL
> locking which got there via the BKL pushdown.
This only works as long as all misc drivers also don't need the BKL
in /their/ open methods. I believe that we did a pushdown into them
last year, but new ones may have crept up.
I guess we really should have kill this one off back then, but somehow
it was forgotten about.
Arnd <><
Commit-ID: 40b798efe3460797a4ac928ee2e038774e2758eb
Gitweb: http://git.kernel.org/tip/40b798efe3460797a4ac928ee2e038774e2758eb
Author: Thomas Gleixner <[email protected]>
AuthorDate: Sat, 10 Oct 2009 15:35:43 +0000
Committer: Thomas Gleixner <[email protected]>
CommitDate: Wed, 14 Oct 2009 17:33:32 +0200
drivers: Remove BKL from misc_open
misc_open() is already serialized with misc_mtx. Remove the BKL
locking which got there via the BKL pushdown.
Signed-off-by: Thomas Gleixner <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
LKML-Reference: <[email protected]>
---
drivers/char/misc.c | 5 +----
1 files changed, 1 insertions(+), 4 deletions(-)
diff --git a/drivers/char/misc.c b/drivers/char/misc.c
index 07fa612..96f1cd0 100644
--- a/drivers/char/misc.c
+++ b/drivers/char/misc.c
@@ -49,7 +49,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
@@ -118,8 +117,7 @@ static int misc_open(struct inode * inode, struct file * file)
struct miscdevice *c;
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) {
@@ -157,7 +155,6 @@ static int misc_open(struct inode * inode, struct file * file)
fops_put(old_fops);
fail:
mutex_unlock(&misc_mtx);
- unlock_kernel();
return err;
}
On Wednesday 14 October 2009, tip-bot for Thomas Gleixner wrote:
> Commit-ID: 40b798efe3460797a4ac928ee2e038774e2758eb
> Gitweb: http://git.kernel.org/tip/40b798efe3460797a4ac928ee2e038774e2758eb
> Author: Thomas Gleixner <[email protected]>
> AuthorDate: Sat, 10 Oct 2009 15:35:43 +0000
> Committer: Thomas Gleixner <[email protected]>
> CommitDate: Wed, 14 Oct 2009 17:33:32 +0200
>
> drivers: Remove BKL from misc_open
>
> misc_open() is already serialized with misc_mtx. Remove the BKL
> locking which got there via the BKL pushdown.
Did you check the misc drivers to make sure this is not silently
removing the BKL from drivers that need it?
Arnd <><
On Wed, 14 Oct 2009, Arnd Bergmann wrote:
> On Wednesday 14 October 2009, tip-bot for Thomas Gleixner wrote:
> > Commit-ID: 40b798efe3460797a4ac928ee2e038774e2758eb
> > Gitweb: http://git.kernel.org/tip/40b798efe3460797a4ac928ee2e038774e2758eb
> > Author: Thomas Gleixner <[email protected]>
> > AuthorDate: Sat, 10 Oct 2009 15:35:43 +0000
> > Committer: Thomas Gleixner <[email protected]>
> > CommitDate: Wed, 14 Oct 2009 17:33:32 +0200
> >
> > drivers: Remove BKL from misc_open
> >
> > misc_open() is already serialized with misc_mtx. Remove the BKL
> > locking which got there via the BKL pushdown.
>
> Did you check the misc drivers to make sure this is not silently
> removing the BKL from drivers that need it?
Yep, they are plastered with cycle_kernel_lock and lock/unlock_kernel
already. I might have missed some, but ...
Thanks,
tglx
On Wed, 14 Oct 2009 15:47:39 GMT
tip-bot for Thomas Gleixner <[email protected]> wrote:
> Commit-ID: 40b798efe3460797a4ac928ee2e038774e2758eb
> Gitweb: http://git.kernel.org/tip/40b798efe3460797a4ac928ee2e038774e2758eb
> Author: Thomas Gleixner <[email protected]>
> AuthorDate: Sat, 10 Oct 2009 15:35:43 +0000
> Committer: Thomas Gleixner <[email protected]>
> CommitDate: Wed, 14 Oct 2009 17:33:32 +0200
>
> drivers: Remove BKL from misc_open
>
> misc_open() is already serialized with misc_mtx. Remove the BKL
> locking which got there via the BKL pushdown.
NAK.
You can't simply assume the mutex is enough - you have to either push it
down or review *every* possible called point below the lock_kernel take.
In this case the open method of the misc devices below sometimes uses the
BKL.
on Wed, 14 Oct 2009, Alan Cox wrote:
> On Wed, 14 Oct 2009 15:47:39 GMT
> tip-bot for Thomas Gleixner <[email protected]> wrote:
>
> > Commit-ID: 40b798efe3460797a4ac928ee2e038774e2758eb
> > Gitweb: http://git.kernel.org/tip/40b798efe3460797a4ac928ee2e038774e2758eb
> > Author: Thomas Gleixner <[email protected]>
> > AuthorDate: Sat, 10 Oct 2009 15:35:43 +0000
> > Committer: Thomas Gleixner <[email protected]>
> > CommitDate: Wed, 14 Oct 2009 17:33:32 +0200
> >
> > drivers: Remove BKL from misc_open
> >
> > misc_open() is already serialized with misc_mtx. Remove the BKL
> > locking which got there via the BKL pushdown.
>
> NAK.
>
> You can't simply assume the mutex is enough - you have to either push it
> down or review *every* possible called point below the lock_kernel take.
>
> In this case the open method of the misc devices below sometimes uses the
> BKL.
The BKL got pushed down into the open methods of misc dev users and we
do not need to take it twice in a row, right ?
Thanks,
tglx
On Wednesday 14 October 2009, Thomas Gleixner wrote:
> on Wed, 14 Oct 2009, Alan Cox wrote:
> > You can't simply assume the mutex is enough - you have to either push it
> > down or review *every* possible called point below the lock_kernel take.
> >
> > In this case the open method of the misc devices below sometimes uses the
> > BKL.
>
> The BKL got pushed down into the open methods of misc dev users and we
> do not need to take it twice in a row, right ?
>
I worked on the original pushdown back then and believed it to be
complete. New drivers have come in and some obviously unneeded
instances got cleared of the BKL usage. The ones with an open method
that does not take the BKL are:
arch/mips/basler/excite/excite_iodev.c
arch/x86/kernel/apm_32.c
drivers/buetooth/hci_vhci.c
drivers/char/nvram.c
drivers/char/rtc.c
drivers/gpu/vga/vgaarb.c
drivers/hwmon/fschmd.c
drivers/hwmon/lis3lv02d.c
drivers/infiniband/core/ucma.c
drivers/isdn/mISDN/timerdev.c
drivers/s390/char/vmcp.c
fs/cachefiles/daemon.c
fs/dlm/user.c
fs/fuse/cuse.c
kernel/power/user.c
net/rfkill/core.c
As well as some drivers from staging and watchdog. I believe the
BKL usage in watchdog drivers was removed shortly after the pushdown.
Arnd <><
drivers/staging/android/binder.c
drivers/staging/dream/qdsp5/audio_aac.c
drivers/staging/dream/qdsp5/audio_evrc.c
drivers/staging/dream/qdsp5/audio_in.c
drivers/staging/dream/qdsp5/audio_out.c
drivers/staging/dream/qdsp5/snd.c
drivers/staging/dream/smd/smd_qmi.c
drivers/staging/panel/panel.c
drivers/watchdog/acquirewdt.c
drivers/watchdog/adx_wdt.c
drivers/watchdog/alim7101_wdt.c
drivers/watchdog/at32ap700x_wdt.c
drivers/watchdog/at91sam9_wdt.c
drivers/watchdog/fin_wdt.c
drivers/watchdog/coh901327_wdt.c
drivers/watchdog/ep93xx_wdt.c
drivers/watchdog/gef_wdt.c
drivers/watchdog/geodewdt.c
drivers/watchdog/i6300esb.c
drivers/watchdog/b700wwdt.c
drivers/watchdog/ibmasr.c
drivers/watchdog/iop_wdt.c
drivers/watchdog/it8_wdt.c
drivers/watchdog/ixp4xx_wdt.c
drivers/watchdog/machzwd.c
drivers/watchdog/mpc5200_wdt.c
drivers/watchdog/mpcore_wdt.c
drivers/watchdog/mv64x60_wdt.c
drivers/watchdog/omap_wdt.c
drivers/watchdog/pc87413_wdt.c
drivers/watchdog/pcwd.c
drivers/watchdog/pcwd_pci.c
drivers/watchdog/pcwd_usb.c
drivers/watchdog/pnx4008_wdt.c
drivers/watchdog/rc32434_wdt.c
drivers/watchdog/s3c2410_wdt.c
drivers/watchdog/sb_wdog.c
drivers/watchdog/sbc7240_wdt.c
drivers/watchdog/sbc_epx_c3.c
drivers/watchdog/c1200wdt.c
drivers/watchdog/sch311x_wdt.c
drivers/watchdog/shwdt.c
drivers/watchdog/smsc37b787_wdt.c
drivers/watchdog/stmp3xxx_wdt.c
drivers/watchdog/txx9wdt.c
drivers/watchdog/w83697hf_wdt.c
drivers/watchdog/w83877f_wdt.c
drivers/watchdog/wafer5823wdt.c
drivers/watchdog/wdrtas.c
drivers/watchdog/wdt.c
drivers/watchdog/wdt977.c
drivers/watchdog/wdt_pci.c
drivers/watchdog/wm8350_wdt.c
On Wednesday 14 October 2009, Arnd Bergmann wrote:
> arch/mips/basler/excite/excite_iodev.c
> drivers/char/rtc.c
> drivers/infiniband/core/ucma.c
> drivers/s390/char/vmcp.c
> fs/dlm/user.c
> kernel/power/user.c
> arch/x86/kernel/apm_32.c
> drivers/char/nvram.c
> drivers/hwmon/fschmd.c
It turns out that these are all ok, the BKL was removed after
the pushdown.
> drivers/gpu/vga/vgaarb.c
> drivers/hwmon/lis3lv02d.c
> fs/cachefiles/daemon.c
> fs/fuse/cuse.c
> net/rfkill/core.c
> drivers/staging/android/binder.c
> drivers/staging/dream/qdsp5/audio_aac.c
> drivers/staging/dream/qdsp5/audio_evrc.c
> drivers/staging/dream/qdsp5/audio_in.c
> drivers/staging/dream/qdsp5/audio_out.c
> drivers/staging/dream/qdsp5/snd.c
> drivers/staging/dream/smd/smd_qmi.c
> drivers/staging/panel/panel.c
These are new drivers that were merged after the pushdown
and should be looked at, though I don't expect that any of
them to need it because none of them use the locked ioctl
method or any other form of BKL.
> drivers/isdn/mISDN/timerdev.c
This one implicitly holds the BKL in both open and ioctl,
it's probably not required but I didn't look closely.
Arnd <><
* Alan Cox <[email protected]> wrote:
> On Wed, 14 Oct 2009 15:47:39 GMT
> tip-bot for Thomas Gleixner <[email protected]> wrote:
>
> > Commit-ID: 40b798efe3460797a4ac928ee2e038774e2758eb
> > Gitweb: http://git.kernel.org/tip/40b798efe3460797a4ac928ee2e038774e2758eb
> > Author: Thomas Gleixner <[email protected]>
> > AuthorDate: Sat, 10 Oct 2009 15:35:43 +0000
> > Committer: Thomas Gleixner <[email protected]>
> > CommitDate: Wed, 14 Oct 2009 17:33:32 +0200
> >
> > drivers: Remove BKL from misc_open
> >
> > misc_open() is already serialized with misc_mtx. Remove the BKL
> > locking which got there via the BKL pushdown.
>
> NAK.
>
> You can't simply assume the mutex is enough - you have to either push
> it down or review *every* possible called point below the lock_kernel
> take.
>
> In this case the open method of the misc devices below sometimes uses
> the BKL.
We did that and it's safe in nvram.c. If you know an unsafe please let
us know.
Ingo
On Wed, 14 Oct 2009 18:16:58 +0200 (CEST)
Thomas Gleixner <[email protected]> wrote:
> on Wed, 14 Oct 2009, Alan Cox wrote:
> > On Wed, 14 Oct 2009 15:47:39 GMT
> > tip-bot for Thomas Gleixner <[email protected]> wrote:
> >
> > > Commit-ID: 40b798efe3460797a4ac928ee2e038774e2758eb
> > > Gitweb: http://git.kernel.org/tip/40b798efe3460797a4ac928ee2e038774e2758eb
> > > Author: Thomas Gleixner <[email protected]>
> > > AuthorDate: Sat, 10 Oct 2009 15:35:43 +0000
> > > Committer: Thomas Gleixner <[email protected]>
> > > CommitDate: Wed, 14 Oct 2009 17:33:32 +0200
> > >
> > > drivers: Remove BKL from misc_open
> > >
> > > misc_open() is already serialized with misc_mtx. Remove the BKL
> > > locking which got there via the BKL pushdown.
> >
> > NAK.
> >
> > You can't simply assume the mutex is enough - you have to either push it
> > down or review *every* possible called point below the lock_kernel take.
> >
> > In this case the open method of the misc devices below sometimes uses the
> > BKL.
>
> The BKL got pushed down into the open methods of misc dev users and we
> do not need to take it twice in a row, right ?
Then the comment is misleading and you need to document that you've
already pushed the BKL down into each user and checked them.
Alan
On Wed, 14 Oct 2009, Alan Cox wrote:
> On Wed, 14 Oct 2009 18:16:58 +0200 (CEST)
> Thomas Gleixner <[email protected]> wrote:
>
> > on Wed, 14 Oct 2009, Alan Cox wrote:
> > > On Wed, 14 Oct 2009 15:47:39 GMT
> > > tip-bot for Thomas Gleixner <[email protected]> wrote:
> > >
> > > > Commit-ID: 40b798efe3460797a4ac928ee2e038774e2758eb
> > > > Gitweb: http://git.kernel.org/tip/40b798efe3460797a4ac928ee2e038774e2758eb
> > > > Author: Thomas Gleixner <[email protected]>
> > > > AuthorDate: Sat, 10 Oct 2009 15:35:43 +0000
> > > > Committer: Thomas Gleixner <[email protected]>
> > > > CommitDate: Wed, 14 Oct 2009 17:33:32 +0200
> > > >
> > > > drivers: Remove BKL from misc_open
> > > >
> > > > misc_open() is already serialized with misc_mtx. Remove the BKL
> > > > locking which got there via the BKL pushdown.
> > >
> > > NAK.
> > >
> > > You can't simply assume the mutex is enough - you have to either push it
> > > down or review *every* possible called point below the lock_kernel take.
> > >
> > > In this case the open method of the misc devices below sometimes uses the
> > > BKL.
> >
> > The BKL got pushed down into the open methods of misc dev users and we
> > do not need to take it twice in a row, right ?
>
> Then the comment is misleading and you need to document that you've
> already pushed the BKL down into each user and checked them.
Fair enough. I fix that.
tglx
On Wed, 14 Oct 2009, Arnd Bergmann wrote:
> > drivers/gpu/vga/vgaarb.c
> > drivers/hwmon/lis3lv02d.c
> > fs/cachefiles/daemon.c
> > fs/fuse/cuse.c
> > net/rfkill/core.c
Sigh, will have a look.
> > drivers/staging/android/binder.c
> > drivers/staging/dream/qdsp5/audio_aac.c
> > drivers/staging/dream/qdsp5/audio_evrc.c
> > drivers/staging/dream/qdsp5/audio_in.c
> > drivers/staging/dream/qdsp5/audio_out.c
> > drivers/staging/dream/qdsp5/snd.c
> > drivers/staging/dream/smd/smd_qmi.c
> > drivers/staging/panel/panel.c
More sigh.
> These are new drivers that were merged after the pushdown
> and should be looked at, though I don't expect that any of
> them to need it because none of them use the locked ioctl
> method or any other form of BKL.
Right I looked at quite a bunch of them.
Thanks,
tglx
Hi!!
> > > drivers/staging/android/binder.c
> > > drivers/staging/dream/qdsp5/audio_aac.c
> > > drivers/staging/dream/qdsp5/audio_evrc.c
> > > drivers/staging/dream/qdsp5/audio_in.c
> > > drivers/staging/dream/qdsp5/audio_out.c
> > > drivers/staging/dream/qdsp5/snd.c
> > > drivers/staging/dream/smd/smd_qmi.c
> > > drivers/staging/panel/panel.c
>
> More sigh.
I guess you can just pretend these do not exist.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html