further BKL removal

PATCH In review Arnd Bergmann 5 participants 14 messages Jul 10, 2010 – Aug 24, 2010

Subsystems (6)

DRM drivers VFS Sound DRM misc +2 more subsystems

lore

Share & tools

lore

Arnd Bergmann

author

2010-07-10 21:51 UTC #0 lore raw

cover further BKL removal

With the ioctl, block, tty, vfs and llseek series
on their way into linux-next, these three patches
are attacking the hardest remaining issues.

If we get these ready for 2.6.36, the kernel should be almost usable with the BKL disabled. In all three cases, I'm cheating a bit, because the BKL still remains lurking in the dark corners of the three subsystems (i810/i830 for drm, OSS for sound and lockd for fs/locks.c).

Arnd

Cc Christoph Hellwig <[email protected]>

Cc David Airlie <[email protected]>

Cc [email protected]

Cc Frederic Weisbecker <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Jaroslav Kysela <[email protected]>

Cc "J. Bruce Fields" <[email protected]>

Cc John Kacur <[email protected]>

Cc Matthew Wilcox <[email protected]>

Cc Miklos Szeredi <[email protected]>

Cc Takashi Iwai <[email protected]>

Cc Trond Myklebust <[email protected]>

Cc [email protected]

Arnd Bergmann (2):
drm: kill BKL from common code
sound: push BKL into open functions

Matthew Wilcox (1):
Remove BKL from fs/locks.c

 arch/um/drivers/hostaudio_kern.c   |    6 ++
 drivers/gpu/drm/drm_drv.c          |    4 +-
 drivers/gpu/drm/drm_fops.c         |   23 ++++----
 drivers/gpu/drm/i810/i810_dma.c    |   44 +++++++++-----
 drivers/gpu/drm/i810/i810_drv.c    |    2 +-
 drivers/gpu/drm/i810/i810_drv.h    |    1 +
 drivers/gpu/drm/i830/i830_dma.c    |   42 +++++++++----
 drivers/gpu/drm/i830/i830_drv.c    |    2 +-
 drivers/gpu/drm/i830/i830_drv.h    |    1 +
 fs/Kconfig                         |    4 +
 fs/locks.c                         |  116 ++++++++++++++++++++++--------------
 include/drm/drmP.h                 |    2 +-
 sound/core/hwdep.c                 |   14 +++-
 sound/core/oss/mixer_oss.c         |   19 ++++---
 sound/oss/au1550_ac97.c            |   26 +++++---
 sound/oss/dmasound/dmasound_core.c |   28 +++++++--
 sound/oss/msnd_pinnacle.c          |   10 ++-
 sound/oss/sh_dac_audio.c           |    9 ++-
 sound/oss/soundcard.c              |   20 ++++---
 sound/oss/swarm_cs4297a.c          |   17 +++++-
 sound/oss/vwsnd.c                  |    8 +++
 sound/sound_core.c                 |    6 +--
 22 files changed, 267 insertions(+), 137 deletions(-)

Arnd Bergmann

author

2010-07-10 21:51 UTC #1 lore raw

PATCH 1/3 drm: kill BKL from common code

This restricts the use of the big kernel lock to the i830 and i810 device drivers. The three remaining users in common code (open, ioctl and release) get converted to a new mutex, the drm_global_mutex, making the locking stricter than the big kernel lock.

This may have a performance impact, but only in those cases that currently don't use DRM_UNLOCKED flag in the ioctl list and would benefit from that anyway.

The reason why i810 and i830 cannot use drm_global_mutex in their mmap functions is a lock-order inversion problem between the current use of the BKL and mmap_sem in these drivers. Since the BKL has release-on-sleep semantics, it's harmless but it would cause trouble if we replace the BKL with a mutex.

Instead, these drivers get their own ioctl wrappers that take the BKL around every ioctl call and then set their own handlers as DRM_UNLOCKED.

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

Cc David Airlie <[email protected]>

Cc [email protected]

---
 drivers/gpu/drm/drm_drv.c       |    4 +-
 drivers/gpu/drm/drm_fops.c      |   23 +++++++++----------
 drivers/gpu/drm/i810/i810_dma.c |   44 +++++++++++++++++++++++++-------------
 drivers/gpu/drm/i810/i810_drv.c |    2 +-
 drivers/gpu/drm/i810/i810_drv.h |    1 +
 drivers/gpu/drm/i830/i830_dma.c |   42 ++++++++++++++++++++++++------------
 drivers/gpu/drm/i830/i830_drv.c |    2 +-
 drivers/gpu/drm/i830/i830_drv.h |    1 +
 include/drm/drmP.h              |    2 +-
 9 files changed, 75 insertions(+), 46 deletions(-)

drivers/gpu/drm/drm_drv.c +2 -2

index 4a66201..76d98f4 100644
@@ -506,9 +506,9 @@ long drm_ioctl(struct file *filp,
 		if (ioctl->flags & DRM_UNLOCKED)
 			retcode = func(dev, kdata, file_priv);
 		else {
-			lock_kernel();
+			mutex_lock(&drm_global_mutex);
 			retcode = func(dev, kdata, file_priv);
-			unlock_kernel();
+			mutex_unlock(&drm_global_mutex);
 		}
 
 		if (cmd & IOC_OUT) {

drivers/gpu/drm/drm_fops.c +11 ~~-12~~

index e7aace2..2ca8df8 100644
@@ -39,6 +39,9 @@
 #include <linux/slab.h>
 #include <linux/smp_lock.h>
 
+/* from BKL pushdown: note that nothing else serializes idr_find() */
+DEFINE_MUTEX(drm_global_mutex);
+
 static int drm_open_helper(struct inode *inode, struct file *filp,
 			   struct drm_device * dev);

@@ -175,8 +178,7 @@ int drm_stub_open(struct inode *inode, struct file *filp)
 
 	DRM_DEBUG("\n");
 
-	/* BKL pushdown: note that nothing else serializes idr_find() */
-	lock_kernel();
+	mutex_lock(&drm_global_mutex);
 	minor = idr_find(&drm_minors_idr, minor_id);
 	if (!minor)
 		goto out;

@@ -197,7 +199,7 @@ int drm_stub_open(struct inode *inode, struct file *filp)
 	fops_put(old_fops);
 
 out:
-	unlock_kernel();
+	mutex_unlock(&drm_global_mutex);
 	return err;
 }

@@ -472,7 +474,7 @@ int drm_release(struct inode *inode, struct file *filp)
 	struct drm_device *dev = file_priv->minor->dev;
 	int retcode = 0;
 
-	lock_kernel();
+	mutex_lock(&drm_global_mutex);
 
 	DRM_DEBUG("open_count = %d\n", dev->open_count);

@@ -573,17 +575,14 @@ int drm_release(struct inode *inode, struct file *filp)
 		if (atomic_read(&dev->ioctl_count)) {
 			DRM_ERROR("Device busy: %d\n",
 				  atomic_read(&dev->ioctl_count));
-			spin_unlock(&dev->count_lock);
-			unlock_kernel();
-			return -EBUSY;
+			retcode = -EBUSY;
+			goto out;
 		}
-		spin_unlock(&dev->count_lock);
-		unlock_kernel();
-		return drm_lastclose(dev);
+		retcode = drm_lastclose(dev);
 	}
+out:
 	spin_unlock(&dev->count_lock);
-
-	unlock_kernel();
+	mutex_unlock(&drm_global_mutex);
 
 	return retcode;
 }

drivers/gpu/drm/i810/i810_dma.c +29 ~~-15~~

57 lines not on this page · raw message

drivers/gpu/drm/i810/i810_drv.c +1 -1

index c1e0275..b4250b2 100644
@@ -59,7 +59,7 @@ static struct drm_driver driver = {
 		 .owner = THIS_MODULE,
 		 .open = drm_open,
 		 .release = drm_release,
-		 .unlocked_ioctl = drm_ioctl,
+		 .unlocked_ioctl = i810_ioctl,
 		 .mmap = drm_mmap,
 		 .poll = drm_poll,
 		 .fasync = drm_fasync,

drivers/gpu/drm/i810/i810_drv.h +1 -0

index 21e2691..cac8f37 100644
@@ -126,6 +126,7 @@ extern void i810_driver_reclaim_buffers_locked(struct drm_device * dev,
 					       struct drm_file *file_priv);
 extern int i810_driver_device_is_agp(struct drm_device * dev);
 
+extern long i810_ioctl(struct file *file, unsigned int cmd, unsigned long arg);
 extern struct drm_ioctl_desc i810_ioctls[];
 extern int i810_max_ioctl;

drivers/gpu/drm/i830/i830_dma.c +28 ~~-14~~

55 lines not on this page · raw message

drivers/gpu/drm/i830/i830_drv.c +1 -1

index 44f990b..a5c66aa 100644
@@ -70,7 +70,7 @@ static struct drm_driver driver = {
 		 .owner = THIS_MODULE,
 		 .open = drm_open,
 		 .release = drm_release,
-		 .unlocked_ioctl = drm_ioctl,
+		 .unlocked_ioctl = i830_ioctl,
 		 .mmap = drm_mmap,
 		 .poll = drm_poll,
 		 .fasync = drm_fasync,

drivers/gpu/drm/i830/i830_drv.h +1 -0

index da82afe..d8df717 100644
@@ -122,6 +122,7 @@ typedef struct drm_i830_private {
 
 } drm_i830_private_t;
 
+long i830_ioctl(struct file *file, unsigned int cmd, unsigned long arg);
 extern struct drm_ioctl_desc i830_ioctls[];
 extern int i830_max_ioctl;

include/drm/drmP.h +1 -1

index c1b9871..cd0e56b 100644
@@ -50,7 +50,6 @@
 #include <linux/file.h>
 #include <linux/pci.h>
 #include <linux/jiffies.h>
-#include <linux/smp_lock.h>	/* For (un)lock_kernel */
 #include <linux/dma-mapping.h>
 #include <linux/mm.h>
 #include <linux/cdev.h>

@@ -1138,6 +1137,7 @@ extern long drm_compat_ioctl(struct file *filp,
 extern int drm_lastclose(struct drm_device *dev);
 
 				/* Device support (drm_fops.h) */
+extern struct mutex drm_global_mutex;
 extern int drm_open(struct inode *inode, struct file *filp);
 extern int drm_stub_open(struct inode *inode, struct file *filp);
 extern int drm_fasync(int fd, struct file *filp, int on);

Signature · 1 line

1.7.1

Andreas Schwab

2010-08-24 18:46 UTC #2 lore raw

Re: [PATCH 1/3] drm: kill BKL from common code

drivers/gpu/drm/drm_drv.c long drm_ioctl(struct file *filp, · line 511

Quoted 13 lines · Arnd Bergmann

drivers/gpu/drm/drm_drv.c +2 -2

index 4a66201..76d98f4 100644
@@ -506,9 +506,9 @@ long drm_ioctl(struct file *filp,
 		if (ioctl->flags & DRM_UNLOCKED)
 			retcode = func(dev, kdata, file_priv);
 		else {
-			lock_kernel();
+			mutex_lock(&drm_global_mutex);
 			retcode = func(dev, kdata, file_priv);
-			unlock_kernel();
+			mutex_unlock(&drm_global_mutex);

How is this supposed to work in the context of sleeping ioctls, like drm_lock?

[drm:drm_ioctl], pid=2461, cmd=0x8008642a, nr=0x2a, dev 0xe200, auth=1 [drm:drm_lock], 1 (pid 2461) requests lock (0x80000003), flags = 0x00000000 [drm:drm_ioctl], pid=2520, cmd=0x8008642b, nr=0x2b, dev 0xe200, auth=1

# ps 2461 2520
  PID TTY      STAT   TIME COMMAND
 2461 tty7     Ss+    0:01 /usr/bin/Xorg -br :0 vt7 -nolisten tcp -auth /var/lib
 2520 pts/2    D+     0:00 glxgears

Andreas.

Signature · 3 lines

Andreas Schwab, [email protected]
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Arnd Bergmann

author

2010-08-24 20:45 UTC #3 lore raw

Re: [PATCH 1/3] drm: kill BKL from common code

Quoted 19 lines · On Tuesday 24 August 2010 20:46:40 Andreas Schwab wrote:

drivers/gpu/drm/drm_drv.c long drm_ioctl(struct file *filp, · line 511

Quoted 13 lines · Arnd Bergmann

drivers/gpu/drm/drm_drv.c +2 -2

index 4a66201..76d98f4 100644
@@ -506,9 +506,9 @@ long drm_ioctl(struct file *filp,
              if (ioctl->flags & DRM_UNLOCKED)
                      retcode = func(dev, kdata, file_priv);
              else {
-                     lock_kernel();
+                     mutex_lock(&drm_global_mutex);
                      retcode = func(dev, kdata, file_priv);
-                     unlock_kernel();
+                     mutex_unlock(&drm_global_mutex);

How is this supposed to work in the context of sleeping ioctls, like drm_lock?

Quoted 8 lines

[drm:drm_ioctl], pid=2461, cmd=0x8008642a, nr=0x2a, dev 0xe200, auth=1 [drm:drm_lock], 1 (pid 2461) requests lock (0x80000003), flags = 0x00000000 [drm:drm_ioctl], pid=2520, cmd=0x8008642b, nr=0x2b, dev 0xe200, auth=1
# ps 2461 2520
  PID TTY      STAT   TIME COMMAND
 2461 tty7     Ss+    0:01 /usr/bin/Xorg -br :0 vt7 -nolisten tcp -auth /var/lib
 2520 pts/2    D+     0:00 glxgears

I was assuming that no ioctl sleeps indefinitely, which is normally the case. As I described in the changelog, all locked ioctls are serialized with the drm_global_mutex, while they used to be only serialized when not sleeping before.

I did not realize that DRM has mutexes that are held across ioctls. To restore the old behavior, apply this patch:

drivers/gpu/drm/drm_lock.c +2 -0

index e2f70a5..9bf93bc 100644
@@ -92,7 +92,9 @@ int drm_lock(struct drm_device *dev, void *data, struct drm_file *file_priv)
 		}
 
 		/* Contention */
+		mutex_unlock(&drm_global_mutex);
 		schedule();
+		mutex_lock(&drm_global_mutex);
 		if (signal_pending(current)) {
 			ret = -EINTR;
 			break;

However, it would be better instead to mark the drm_lock and drm_unlock ioctls as DRM_UNLOCKED so they are not called under drm_global_mutex to start with, like this:

drivers/gpu/drm/drm_drv.c +2 -2

index 90288ec..469bc18 100644
@@ -91,8 +91,8 @@ static struct drm_ioctl_desc drm_ioctls[] = {
 	DRM_IOCTL_DEF(DRM_IOCTL_ADD_DRAW, drm_adddraw, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY),
 	DRM_IOCTL_DEF(DRM_IOCTL_RM_DRAW, drm_rmdraw, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY),
 
-	DRM_IOCTL_DEF(DRM_IOCTL_LOCK, drm_lock, DRM_AUTH),
-	DRM_IOCTL_DEF(DRM_IOCTL_UNLOCK, drm_unlock, DRM_AUTH),
+	DRM_IOCTL_DEF(DRM_IOCTL_LOCK, drm_lock, DRM_AUTH|DRM_UNLOCKED),
+	DRM_IOCTL_DEF(DRM_IOCTL_UNLOCK, drm_unlock, DRM_AUTH|DRM_UNLOCKED),
 
 	DRM_IOCTL_DEF(DRM_IOCTL_FINISH, drm_noop, DRM_AUTH),

Ideally, all the ioctls should be marked as DRM_UNLOCKED and the path calling ioctl under drm_global_mutex be removed, but that requires auditing of each call by someone who understands the drm locking better than I do.

Apply only one of the two patches at a time, they are mutually exclusive.

Arnd

Arnd Bergmann

author

2010-07-10 21:52 UTC #4 lore raw

PATCH 2/3 Remove BKL from fs/locks.c

In reply to Arnd Bergmann

From: Matthew Wilcox <[email protected]>

I've taken a patch originally written by Matthew Wilcox and ported it to the current version. Unfortunately, the change conflicts with the use of lockd, which still heavily uses the big kernel lock.

As a workaround, I've made the behaviour configurable, it either uses the BKL when it's enabled or a spinlock when the BKL (and consequently nfs and lockd) are disabled.

Original introduction from Willy:

I've been promising to do this for about seven years now.

It seems to work well enough, but I haven't run any serious stress tests on it. This implementation uses one spinlock to protect both lock lists and all the i_flock chains. It doesn't seem worth splitting up the locking any further.

I had to move one memory allocation out from under the file_lock_lock. I hope I got that logic right. I'm rather tempted to split out the find_conflict algorithm from that function into something that can be called separately for the FL_ACCESS case.

I also have to drop and reacquire the file_lock_lock around the call to cond_resched(). This was done automatically for us before by the special BKL semantics.

I had to change vfs_setlease() as it relied on the special BKL ability to recursively acquire the same lock. The internal caller now calls __vfs_setlease and the exported interface acquires and releases the file_lock_lock around calling __vfs_setlease.

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

Cc Matthew Wilcox <[email protected]>

Cc Christoph Hellwig <[email protected]>

Cc Trond Myklebust <[email protected]>

Cc "J. Bruce Fields" <[email protected]>

Cc Miklos Szeredi <[email protected]>

Cc Frederic Weisbecker <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc John Kacur <[email protected]>

Cc [email protected]

---
 fs/Kconfig |    4 ++
 fs/locks.c |  116 +++++++++++++++++++++++++++++++++++++-----------------------
 2 files changed, 76 insertions(+), 44 deletions(-)

fs/Kconfig +4 -0

index 5f85b59..a624d92 100644
@@ -55,6 +55,10 @@ config FILE_LOCKING
           for filesystems like NFS and for the flock() system
           call. Disabling this option saves about 11k.
 
+config FLOCKS_SPINLOCK
+	bool
+	default FILE_LOCKING && !LOCK_KERNEL
+
 source "fs/notify/Kconfig"
 
 source "fs/quota/Kconfig"

fs/locks.c +72 ~~-44~~

311 lines not on this page · raw message

Signature · 1 line

1.7.1

Christoph Hellwig

2010-07-10 22:01 UTC #5 lore raw

Re: [PATCH 2/3] Remove BKL from fs/locks.c

Quoted 11 lines · Arnd Bergmann

From: Matthew Wilcox <[email protected]>
I've taken a patch originally written by Matthew Wilcox and ported it to the current version. Unfortunately, the change conflicts with the use of lockd, which still heavily uses the big kernel lock.
As a workaround, I've made the behaviour configurable, it either uses the BKL when it's enabled or a spinlock when the BKL (and consequently nfs and lockd) are disabled.

Defintively not something we want in mainline. But keep poking the nfs guys to sort the lockd mess out for real.

Arnd Bergmann

author

2010-07-11 10:31 UTC #6 lore raw

Re: [PATCH 2/3] Remove BKL from fs/locks.c

Quoted 8 lines · On Sunday 11 July 2010 00:01:10 Christoph Hellwig wrote:

Quoted 7 lines · Arnd Bergmann
From: Matthew Wilcox <[email protected]>
I've taken a patch originally written by Matthew Wilcox and ported it to the current version. Unfortunately, the change conflicts with the use of lockd, which still heavily uses the big kernel lock.

Quoted 7 lines

Quoted 4 lines
As a workaround, I've made the behaviour configurable, it either uses the BKL when it's enabled or a spinlock when the BKL (and consequently nfs and lockd) are disabled.
Defintively not something we want in mainline. But keep poking the nfs guys to sort the lockd mess out for real.

Yes, that was the idea. This is the last patch I need to run all of my machines without the BKL, so I spent a few hours looking at how to fix lockd. When I couldn't figure it out, I decided to do an evil hack that happens to work, in order to build pressure.

Arnd

Arnd Bergmann

author

2010-07-10 21:51 UTC #7 lore raw

PATCH 3/3 sound: push BKL into open functions

In reply to Arnd Bergmann

This moves the lock_kernel() call from soundcore_open to the individual OSS device drivers, where we can deal with it one driver at a time if needed, or just kill off the drivers.

All core components in ALSA already provide
adequate locking in their open()-functions
and do not require the big kernel lock, so
there is no need to add the BKL there.

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

Cc Takashi Iwai <[email protected]>

Cc Jaroslav Kysela <[email protected]>

Cc [email protected]

---
 arch/um/drivers/hostaudio_kern.c   |    6 ++++++
 sound/core/hwdep.c                 |   14 ++++++++++----
 sound/core/oss/mixer_oss.c         |   19 +++++++++++--------
 sound/oss/au1550_ac97.c            |   26 +++++++++++++++++---------
 sound/oss/dmasound/dmasound_core.c |   28 ++++++++++++++++++++++------
 sound/oss/msnd_pinnacle.c          |   10 +++++++---
 sound/oss/sh_dac_audio.c           |    9 +++++++--
 sound/oss/soundcard.c              |   20 +++++++++++---------
 sound/oss/swarm_cs4297a.c          |   17 ++++++++++++++++-
 sound/oss/vwsnd.c                  |    8 ++++++++
 sound/sound_core.c                 |    6 +-----
 11 files changed, 116 insertions(+), 47 deletions(-)

arch/um/drivers/hostaudio_kern.c +6 -0

index ae42695..68142df 100644
@@ -8,6 +8,7 @@
 #include "linux/slab.h"
 #include "linux/sound.h"
 #include "linux/soundcard.h"
+#include "linux/smp_lock.h"
 #include "asm/uaccess.h"
 #include "init.h"
 #include "os.h"

@@ -198,7 +199,10 @@ static int hostaudio_open(struct inode *inode, struct file *file)
 	if (file->f_mode & FMODE_WRITE)
 		w = 1;
 
+	lock_kernel();
 	ret = os_open_file(dsp, of_set_rw(OPENFLAGS(), r, w), 0);
+	unlock_kernel();
+
 	if (ret < 0) {
 		kfree(state);
 		return ret;

@@ -254,7 +258,9 @@ static int hostmixer_open_mixdev(struct inode *inode, struct file *file)
 	if (file->f_mode & FMODE_WRITE)
 		w = 1;
 
+	lock_kernel();
 	ret = os_open_file(mixer, of_set_rw(OPENFLAGS(), r, w), 0);
+	unlock_kernel();
 
 	if (ret < 0) {
 		printk(KERN_ERR "hostaudio_open_mixdev failed to open '%s', "

sound/core/hwdep.c +10 -4

index a70ee7f..3439587 100644
@@ -94,13 +94,18 @@ static int snd_hwdep_open(struct inode *inode, struct file * file)
 		hw = snd_lookup_oss_minor_data(iminor(inode),
 					       SNDRV_OSS_DEVICE_TYPE_DMFM);
 #endif
-	} else
-		return -ENXIO;
+	} else {
+		err = -ENXIO;
+		goto out;
+	}
+
+	err = -ENODEV;
 	if (hw == NULL)
-		return -ENODEV;
+		goto out;
 
+	err = -EFAULT;
 	if (!try_module_get(hw->card->module))
-		return -EFAULT;
+		goto out;
 
 	init_waitqueue_entry(&wait, current);
 	add_wait_queue(&hw->open_wait, &wait);

@@ -147,6 +152,7 @@ static int snd_hwdep_open(struct inode *inode, struct file * file)
 	mutex_unlock(&hw->open_mutex);
 	if (err < 0)
 		module_put(hw->card->module);
+out:
 	return err;
 }

sound/core/oss/mixer_oss.c +11 -8

index f50ebf2..c30b1f3 100644
@@ -49,17 +49,19 @@ static int snd_mixer_oss_open(struct inode *inode, struct file *file)
 
 	card = snd_lookup_oss_minor_data(iminor(inode),
 					 SNDRV_OSS_DEVICE_TYPE_MIXER);
-	if (card == NULL)
-		return -ENODEV;
-	if (card->mixer_oss == NULL)
-		return -ENODEV;
+	err = -ENODEV;
+	if (card == NULL || card->mixer_oss == NULL)
+		goto out;
+
 	err = snd_card_file_add(card, file);
 	if (err < 0)
-		return err;
+		goto out;
+
 	fmixer = kzalloc(sizeof(*fmixer), GFP_KERNEL);
 	if (fmixer == NULL) {
 		snd_card_file_remove(card, file);
-		return -ENOMEM;
+		err = -ENOMEM;
+		goto out;
 	}
 	fmixer->card = card;
 	fmixer->mixer = card->mixer_oss;

@@ -67,9 +69,10 @@ static int snd_mixer_oss_open(struct inode *inode, struct file *file)
 	if (!try_module_get(card->module)) {
 		kfree(fmixer);
 		snd_card_file_remove(card, file);
-		return -EFAULT;
+		err = -EFAULT;
 	}
-	return 0;
+out:
+	return err;
 }
 
 static int snd_mixer_oss_release(struct inode *inode, struct file *file)

sound/oss/au1550_ac97.c +17 -9

index c1070e3..47143a3 100644
@@ -43,6 +43,7 @@
 #include <linux/sound.h>
 #include <linux/slab.h>
 #include <linux/soundcard.h>
+#include <linux/smp_lock.h>
 #include <linux/init.h>
 #include <linux/interrupt.h>
 #include <linux/kernel.h>

@@ -807,7 +808,9 @@ au1550_llseek(struct file *file, loff_t offset, int origin)
 static int
 au1550_open_mixdev(struct inode *inode, struct file *file)
 {
+	lock_kernel();
 	file->private_data = &au1550_state;
+	unlock_kernel();
 	return 0;
 }

@@ -1797,21 +1800,22 @@ au1550_open(struct inode *inode, struct file *file)
 #endif
 
 	file->private_data = s;
+	lock_kernel();
 	/* wait for device to become free */
 	mutex_lock(&s->open_mutex);
 	while (s->open_mode & file->f_mode) {
-		if (file->f_flags & O_NONBLOCK) {
-			mutex_unlock(&s->open_mutex);
-			return -EBUSY;
-		}
+		ret = -EBUSY;
+		if (file->f_flags & O_NONBLOCK)
+			goto out;		
 		add_wait_queue(&s->open_wait, &wait);
 		__set_current_state(TASK_INTERRUPTIBLE);
 		mutex_unlock(&s->open_mutex);
 		schedule();
 		remove_wait_queue(&s->open_wait, &wait);
 		set_current_state(TASK_RUNNING);
+		ret = -ERESTARTSYS;
 		if (signal_pending(current))
-			return -ERESTARTSYS;
+			goto out2;
 		mutex_lock(&s->open_mutex);
 	}

@@ -1840,17 +1844,21 @@ au1550_open(struct inode *inode, struct file *file)
 
 	if (file->f_mode & FMODE_READ) {
 		if ((ret = prog_dmabuf_adc(s)))
-			return ret;
+			goto out;
 	}
 	if (file->f_mode & FMODE_WRITE) {
 		if ((ret = prog_dmabuf_dac(s)))
-			return ret;
+			goto out;
 	}
 
 	s->open_mode |= file->f_mode & (FMODE_READ | FMODE_WRITE);
-	mutex_unlock(&s->open_mutex);
 	mutex_init(&s->sem);
-	return 0;
+	ret = 0;
+out:
+	mutex_unlock(&s->open_mutex);
+out2:
+	unlock_kernel();
+	return ret;
 }
 
 static int

sound/oss/dmasound/dmasound_core.c +22 -6

index 3f3c3f7..5a4f38c 100644
@@ -323,9 +323,13 @@ static struct {
 
 static int mixer_open(struct inode *inode, struct file *file)
 {
-	if (!try_module_get(dmasound.mach.owner))
+	lock_kernel();
+	if (!try_module_get(dmasound.mach.owner)) {
+		unlock_kernel();
 		return -ENODEV;
+	}
 	mixer.busy = 1;
+	unlock_kernel();
 	return 0;
 }

@@ -737,8 +741,11 @@ static int sq_open(struct inode *inode, struct file *file)
 {
 	int rc;
 
-	if (!try_module_get(dmasound.mach.owner))
+	lock_kernel();
+	if (!try_module_get(dmasound.mach.owner)) {
+		unlock_kernel();
 		return -ENODEV;
+	}
 
 	rc = write_sq_open(file); /* checks the f_mode */
 	if (rc)

@@ -781,10 +788,11 @@ static int sq_open(struct inode *inode, struct file *file)
 		sound_set_format(AFMT_MU_LAW);
 	}
 #endif
-
+	unlock_kernel();
 	return 0;
  out:
 	module_put(dmasound.mach.owner);
+	unlock_kernel();
 	return rc;
 }

@@ -1226,12 +1234,17 @@ static int state_open(struct inode *inode, struct file *file)
 {
 	char *buffer = state.buf;
 	int len = 0;
+	int ret;
 
+	lock_kernel();
+	ret = -EBUSY;
 	if (state.busy)
-		return -EBUSY;
+		goto out;
 
+	ret = -ENODEV;
 	if (!try_module_get(dmasound.mach.owner))
-		return -ENODEV;
+		goto out;
+
 	state.ptr = 0;
 	state.busy = 1;

@@ -1293,7 +1306,10 @@ printk("dmasound: stat buffer used %d bytes\n", len) ;
 		printk(KERN_ERR "dmasound_core: stat buffer overflowed!\n");
 
 	state.len = len;
-	return 0;
+	ret = 0;
+out:
+	unlock_kernel();
+	return ret;
 }
 
 static int state_release(struct inode *inode, struct file *file)

sound/oss/msnd_pinnacle.c +7 -3

index a1e3f96..153d822 100644
@@ -756,12 +756,15 @@ static int dev_open(struct inode *inode, struct file *file)
 	int minor = iminor(inode);
 	int err = 0;
 
+	lock_kernel();
 	if (minor == dev.dsp_minor) {
 		if ((file->f_mode & FMODE_WRITE &&
 		     test_bit(F_AUDIO_WRITE_INUSE, &dev.flags)) ||
 		    (file->f_mode & FMODE_READ &&
-		     test_bit(F_AUDIO_READ_INUSE, &dev.flags)))
-			return -EBUSY;
+		     test_bit(F_AUDIO_READ_INUSE, &dev.flags))) {
+			err = -EBUSY;
+			goto out;
+		}
 
 		if ((err = dsp_open(file)) >= 0) {
 			dev.nresets = 0;

@@ -782,7 +785,8 @@ static int dev_open(struct inode *inode, struct file *file)
 		/* nothing */
 	} else
 		err = -EINVAL;
-
+out:
+	unlock_kernel();
 	return err;
 }

sound/oss/sh_dac_audio.c +7 -2

index 4153752..8f0be40 100644
@@ -16,6 +16,7 @@
 #include <linux/slab.h>
 #include <linux/fs.h>
 #include <linux/sound.h>
+#include <linux/smp_lock.h>
 #include <linux/soundcard.h>
 #include <linux/interrupt.h>
 #include <linux/hrtimer.h>

@@ -216,13 +217,17 @@ static int dac_audio_open(struct inode *inode, struct file *file)
 {
 	if (file->f_mode & FMODE_READ)
 		return -ENODEV;
-	if (in_use)
+
+	lock_kernel();
+	if (in_use) {
+		unlock_kernel();
 		return -EBUSY;
+	}
 
 	in_use = 1;
 
 	dac_audio_start();
-
+	unlock_kernel();
 	return 0;
 }

sound/oss/soundcard.c +11 -9

index 2d9c513..92aa762 100644
@@ -210,42 +210,44 @@ static int sound_open(struct inode *inode, struct file *file)
 		printk(KERN_ERR "Invalid minor device %d\n", dev);
 		return -ENXIO;
 	}
+	lock_kernel();
 	switch (dev & 0x0f) {
 	case SND_DEV_CTL:
 		dev >>= 4;
 		if (dev >= 0 && dev < MAX_MIXER_DEV && mixer_devs[dev] == NULL) {
 			request_module("mixer%d", dev);
 		}
+		retval = -ENXIO;
 		if (dev && (dev >= num_mixers || mixer_devs[dev] == NULL))
-			return -ENXIO;
+			break;
 	
 		if (!try_module_get(mixer_devs[dev]->owner))
-			return -ENXIO;
+			break;
+
+		retval = 0;
 		break;
 
 	case SND_DEV_SEQ:
 	case SND_DEV_SEQ2:
-		if ((retval = sequencer_open(dev, file)) < 0)
-			return retval;
+		retval = sequencer_open(dev, file);
 		break;
 
 	case SND_DEV_MIDIN:
-		if ((retval = MIDIbuf_open(dev, file)) < 0)
-			return retval;
+		retval = MIDIbuf_open(dev, file);
 		break;
 
 	case SND_DEV_DSP:
 	case SND_DEV_DSP16:
 	case SND_DEV_AUDIO:
-		if ((retval = audio_open(dev, file)) < 0)
-			return retval;
+		retval = audio_open(dev, file);
 		break;
 
 	default:
 		printk(KERN_ERR "Invalid minor device %d\n", dev);
-		return -ENXIO;
+		retval = -ENXIO;
 	}
 
+	unlock_kernel();
 	return 0;
 }

sound/oss/swarm_cs4297a.c +16 -1

index 3136c88..34b0838 100644
@@ -68,6 +68,7 @@
 #include <linux/delay.h>
 #include <linux/sound.h>
 #include <linux/slab.h>
+#include <linux/smp_lock.h>
 #include <linux/soundcard.h>
 #include <linux/ac97_codec.h>
 #include <linux/pci.h>

@@ -1534,6 +1535,7 @@ static int cs4297a_open_mixdev(struct inode *inode, struct file *file)
 	CS_DBGOUT(CS_FUNCTION | CS_OPEN, 4,
 		  printk(KERN_INFO "cs4297a: cs4297a_open_mixdev()+\n"));
 
+	lock_kernel();
 	list_for_each(entry, &cs4297a_devs)
 	{
 		s = list_entry(entry, struct cs4297a_state, list);

@@ -1544,6 +1546,8 @@ static int cs4297a_open_mixdev(struct inode *inode, struct file *file)
 	{
 		CS_DBGOUT(CS_FUNCTION | CS_OPEN | CS_ERROR, 2,
 			printk(KERN_INFO "cs4297a: cs4297a_open_mixdev()- -ENODEV\n"));
+
+		unlock_kernel();
 		return -ENODEV;
 	}
 	VALIDATE_STATE(s);

@@ -1551,6 +1555,7 @@ static int cs4297a_open_mixdev(struct inode *inode, struct file *file)
 
 	CS_DBGOUT(CS_FUNCTION | CS_OPEN, 4,
 		  printk(KERN_INFO "cs4297a: cs4297a_open_mixdev()- 0\n"));
+	unlock_kernel();
 
 	return nonseekable_open(inode, file);
 }

@@ -2369,7 +2374,7 @@ static int cs4297a_release(struct inode *inode, struct file *file)
 	return 0;
 }
 
-static int cs4297a_open(struct inode *inode, struct file *file)
+static int cs4297a_locked_open(struct inode *inode, struct file *file)
 {
 	int minor = iminor(inode);
 	struct cs4297a_state *s=NULL;

@@ -2486,6 +2491,16 @@ static int cs4297a_open(struct inode *inode, struct file *file)
 	return nonseekable_open(inode, file);
 }
 
+static int cs4297a_open(struct inode *inode, struct file *file)
+{
+	int ret;
+
+	lock_kernel();
+	ret = cs4297a_open(inode, file);
+	unlock_kernel();
+
+	return ret;
+}
 
 // ******************************************************************************************
 //   Wave (audio) file operations struct.

sound/oss/vwsnd.c +8 -0

index 20b3b32..99c94c4 100644
@@ -2921,6 +2921,7 @@ static int vwsnd_audio_open(struct inode *inode, struct file *file)
 
 	DBGE("(inode=0x%p, file=0x%p)\n", inode, file);
 
+	lock_kernel();
 	INC_USE_COUNT;
 	for (devc = vwsnd_dev_list; devc; devc = devc->next_dev)
 		if ((devc->audio_minor & ~0x0F) == (minor & ~0x0F))

@@ -2928,6 +2929,7 @@ static int vwsnd_audio_open(struct inode *inode, struct file *file)
 
 	if (devc == NULL) {
 		DEC_USE_COUNT;
+		unlock_kernel();
 		return -ENODEV;
 	}

@@ -2936,11 +2938,13 @@ static int vwsnd_audio_open(struct inode *inode, struct file *file)
 		mutex_unlock(&devc->open_mutex);
 		if (file->f_flags & O_NONBLOCK) {
 			DEC_USE_COUNT;
+			unlock_kernel();
 			return -EBUSY;
 		}
 		interruptible_sleep_on(&devc->open_wait);
 		if (signal_pending(current)) {
 			DEC_USE_COUNT;
+			unlock_kernel();
 			return -ERESTARTSYS;
 		}
 		mutex_lock(&devc->open_mutex);

@@ -2993,6 +2997,7 @@ static int vwsnd_audio_open(struct inode *inode, struct file *file)
 
 	file->private_data = devc;
 	DBGRV();
+	unlock_kernel();
 	return 0;
 }

@@ -3062,15 +3067,18 @@ static int vwsnd_mixer_open(struct inode *inode, struct file *file)
 	DBGEV("(inode=0x%p, file=0x%p)\n", inode, file);
 
 	INC_USE_COUNT;
+	lock_kernel();
 	for (devc = vwsnd_dev_list; devc; devc = devc->next_dev)
 		if (devc->mixer_minor == iminor(inode))
 			break;
 
 	if (devc == NULL) {
 		DEC_USE_COUNT;
+		unlock_kernel();
 		return -ENODEV;
 	}
 	file->private_data = devc;
+	unlock_kernel();
 	return 0;
 }

sound/sound_core.c +1 -5

index c8627fc..cb61317 100644
@@ -629,12 +629,8 @@ static int soundcore_open(struct inode *inode, struct file *file)
 		file->f_op = new_fops;
 		spin_unlock(&sound_loader_lock);
 
-		if (file->f_op->open) {
-			/* TODO: push down BKL into indivial open functions */
-			lock_kernel();
+		if (file->f_op->open)
 			err = file->f_op->open(inode,file);
-			unlock_kernel();
-		}
 
 		if (err) {
 			fops_put(file->f_op);

Signature · 1 line

1.7.1

Jaroslav Kysela

maintainer

2010-07-11 07:15 UTC #8 lore raw

Re: [PATCH 3/3] sound: push BKL into open functions

Quoted 65 lines · Arnd Bergmann

sound/core/hwdep.c +10 -4

@@ -94,13 +94,18 @@ static int snd_hwdep_open(struct inode *inode, struct file * file)
 		hw = snd_lookup_oss_minor_data(iminor(inode),
 					       SNDRV_OSS_DEVICE_TYPE_DMFM);
 #endif
-	} else
-		return -ENXIO;
+	} else {
+		err = -ENXIO;
+		goto out;
+	}
+
+	err = -ENODEV;
 	if (hw == NULL)
-		return -ENODEV;
+		goto out;
 
+	err = -EFAULT;
 	if (!try_module_get(hw->card->module))
-		return -EFAULT;
+		goto out;
 
 	init_waitqueue_entry(&wait, current);
 	add_wait_queue(&hw->open_wait, &wait);

@@ -147,6 +152,7 @@ static int snd_hwdep_open(struct inode *inode, struct file * file)
 	mutex_unlock(&hw->open_mutex);
 	if (err < 0)
 		module_put(hw->card->module);
+out:
 	return err;
 }

sound/core/oss/mixer_oss.c +8 -6

index f50ebf2..c30b1f3 100644
@@ -49,17 +49,19 @@ static int snd_mixer_oss_open(struct inode *inode, struct file *file)
 
 	card = snd_lookup_oss_minor_data(iminor(inode),
 					 SNDRV_OSS_DEVICE_TYPE_MIXER);
-	if (card == NULL)
-		return -ENODEV;
-	if (card->mixer_oss == NULL)
-		return -ENODEV;
+	err = -ENODEV;
+	if (card == NULL || card->mixer_oss == NULL)
+		goto out;
+
 	err = snd_card_file_add(card, file);
 	if (err < 0)
-		return err;
+		goto out;
+
 	fmixer = kzalloc(sizeof(*fmixer), GFP_KERNEL);
 	if (fmixer == NULL) {
 		snd_card_file_remove(card, file);
-		return -ENOMEM;
+		err = -ENOMEM;
+		goto out;
 	}
 	fmixer->card = card;
 	fmixer->mixer = card->mixer_oss;

@@ -67,9 +69,10 @@ static int snd_mixer_oss_open(struct inode *inode, struct file *file)
 	if (!try_module_get(card->module)) {

sound/core/oss/mixer_oss.c static int snd_mixer_oss_open(struct inode *inode, struct file *file) · line 77

Quoted 10 lines

sound/core/oss/mixer_oss.c +3 -2

 		kfree(fmixer);
 		snd_card_file_remove(card, file);
-		return -EFAULT;
+		err = -EFAULT;
 	}
-	return 0;
+out:
+	return err;
 }

I don't see any reason (benefit) to add gotos to these two functions.

Jaroslav

-----
Jaroslav Kysela <[email protected]>
Linux Kernel Sound Maintainer
ALSA Project; Red Hat, Inc.

Arnd Bergmann

author

2010-07-11 10:16 UTC #9 lore raw

Re: [PATCH 3/3] sound: push BKL into open functions

This moves the lock_kernel() call from soundcore_open to the individual OSS device drivers, where we can deal with it one driver at a time if needed, or just kill off the drivers.

All core components in ALSA already provide
adequate locking in their open()-functions
and do not require the big kernel lock, so
there is no need to add the BKL there.

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

---

Quoted 2 lines · On Sunday 11 July 2010 09:15:22 Jaroslav Kysela wrote:

I don't see any reason (benefit) to add gotos to these two functions.

Sorry, I had removed them and then forgot the git-add before creating the emails. Originally, I had two patches, one for pushing down the BKL into every sound driver (hence the goto) and a second patch to remove the BKL again from all the native alsa drivers. If you prefer, I can also give you the separate patches, but I figured that since none of the ALSA drivers needs the BKL, the combined patch would be better.

This is the corrected combined version.

Arnd

 arch/um/drivers/hostaudio_kern.c   |    6 ++++++
 sound/oss/au1550_ac97.c            |   26 +++++++++++++++++---------
 sound/oss/dmasound/dmasound_core.c |   28 ++++++++++++++++++++++------
 sound/oss/msnd_pinnacle.c          |   10 +++++++---
 sound/oss/sh_dac_audio.c           |    9 +++++++--
 sound/oss/soundcard.c              |   20 +++++++++++---------
 sound/oss/swarm_cs4297a.c          |   17 ++++++++++++++++-
 sound/oss/vwsnd.c                  |    8 ++++++++
 sound/sound_core.c                 |    6 +-----
 9 files changed, 95 insertions(+), 35 deletions(-)

arch/um/drivers/hostaudio_kern.c +6 -0

index ae42695..68142df 100644
@@ -8,6 +8,7 @@
 #include "linux/slab.h"
 #include "linux/sound.h"
 #include "linux/soundcard.h"
+#include "linux/smp_lock.h"
 #include "asm/uaccess.h"
 #include "init.h"
 #include "os.h"

@@ -198,7 +199,10 @@ static int hostaudio_open(struct inode *inode, struct file *file)
 	if (file->f_mode & FMODE_WRITE)
 		w = 1;
 
+	lock_kernel();
 	ret = os_open_file(dsp, of_set_rw(OPENFLAGS(), r, w), 0);
+	unlock_kernel();
+
 	if (ret < 0) {
 		kfree(state);
 		return ret;

@@ -254,7 +258,9 @@ static int hostmixer_open_mixdev(struct inode *inode, struct file *file)
 	if (file->f_mode & FMODE_WRITE)
 		w = 1;
 
+	lock_kernel();
 	ret = os_open_file(mixer, of_set_rw(OPENFLAGS(), r, w), 0);
+	unlock_kernel();
 
 	if (ret < 0) {
 		printk(KERN_ERR "hostaudio_open_mixdev failed to open '%s', "

sound/oss/au1550_ac97.c +17 -9

index c1070e3..47143a3 100644
@@ -43,6 +43,7 @@
 #include <linux/sound.h>
 #include <linux/slab.h>
 #include <linux/soundcard.h>
+#include <linux/smp_lock.h>
 #include <linux/init.h>
 #include <linux/interrupt.h>
 #include <linux/kernel.h>

@@ -807,7 +808,9 @@ au1550_llseek(struct file *file, loff_t offset, int origin)
 static int
 au1550_open_mixdev(struct inode *inode, struct file *file)
 {
+	lock_kernel();
 	file->private_data = &au1550_state;
+	unlock_kernel();
 	return 0;
 }

@@ -1797,21 +1800,22 @@ au1550_open(struct inode *inode, struct file *file)
 #endif
 
 	file->private_data = s;
+	lock_kernel();
 	/* wait for device to become free */
 	mutex_lock(&s->open_mutex);
 	while (s->open_mode & file->f_mode) {
-		if (file->f_flags & O_NONBLOCK) {
-			mutex_unlock(&s->open_mutex);
-			return -EBUSY;
-		}
+		ret = -EBUSY;
+		if (file->f_flags & O_NONBLOCK)
+			goto out;		
 		add_wait_queue(&s->open_wait, &wait);
 		__set_current_state(TASK_INTERRUPTIBLE);
 		mutex_unlock(&s->open_mutex);
 		schedule();
 		remove_wait_queue(&s->open_wait, &wait);
 		set_current_state(TASK_RUNNING);
+		ret = -ERESTARTSYS;
 		if (signal_pending(current))
-			return -ERESTARTSYS;
+			goto out2;
 		mutex_lock(&s->open_mutex);
 	}

@@ -1840,17 +1844,21 @@ au1550_open(struct inode *inode, struct file *file)
 
 	if (file->f_mode & FMODE_READ) {
 		if ((ret = prog_dmabuf_adc(s)))
-			return ret;
+			goto out;
 	}
 	if (file->f_mode & FMODE_WRITE) {
 		if ((ret = prog_dmabuf_dac(s)))
-			return ret;
+			goto out;
 	}
 
 	s->open_mode |= file->f_mode & (FMODE_READ | FMODE_WRITE);
-	mutex_unlock(&s->open_mutex);
 	mutex_init(&s->sem);
-	return 0;
+	ret = 0;
+out:
+	mutex_unlock(&s->open_mutex);
+out2:
+	unlock_kernel();
+	return ret;
 }
 
 static int

sound/oss/dmasound/dmasound_core.c +22 -6

index 3f3c3f7..5a4f38c 100644
@@ -323,9 +323,13 @@ static struct {
 
 static int mixer_open(struct inode *inode, struct file *file)
 {
-	if (!try_module_get(dmasound.mach.owner))
+	lock_kernel();
+	if (!try_module_get(dmasound.mach.owner)) {
+		unlock_kernel();
 		return -ENODEV;
+	}
 	mixer.busy = 1;
+	unlock_kernel();
 	return 0;
 }

@@ -737,8 +741,11 @@ static int sq_open(struct inode *inode, struct file *file)
 {
 	int rc;
 
-	if (!try_module_get(dmasound.mach.owner))
+	lock_kernel();
+	if (!try_module_get(dmasound.mach.owner)) {
+		unlock_kernel();
 		return -ENODEV;
+	}
 
 	rc = write_sq_open(file); /* checks the f_mode */
 	if (rc)

@@ -781,10 +788,11 @@ static int sq_open(struct inode *inode, struct file *file)
 		sound_set_format(AFMT_MU_LAW);
 	}
 #endif
-
+	unlock_kernel();
 	return 0;
  out:
 	module_put(dmasound.mach.owner);
+	unlock_kernel();
 	return rc;
 }

@@ -1226,12 +1234,17 @@ static int state_open(struct inode *inode, struct file *file)
 {
 	char *buffer = state.buf;
 	int len = 0;
+	int ret;
 
+	lock_kernel();
+	ret = -EBUSY;
 	if (state.busy)
-		return -EBUSY;
+		goto out;
 
+	ret = -ENODEV;
 	if (!try_module_get(dmasound.mach.owner))
-		return -ENODEV;
+		goto out;
+
 	state.ptr = 0;
 	state.busy = 1;

@@ -1293,7 +1306,10 @@ printk("dmasound: stat buffer used %d bytes\n", len) ;
 		printk(KERN_ERR "dmasound_core: stat buffer overflowed!\n");
 
 	state.len = len;
-	return 0;
+	ret = 0;
+out:
+	unlock_kernel();
+	return ret;
 }
 
 static int state_release(struct inode *inode, struct file *file)

sound/oss/msnd_pinnacle.c +7 -3

index a1e3f96..153d822 100644
@@ -756,12 +756,15 @@ static int dev_open(struct inode *inode, struct file *file)
 	int minor = iminor(inode);
 	int err = 0;
 
+	lock_kernel();
 	if (minor == dev.dsp_minor) {
 		if ((file->f_mode & FMODE_WRITE &&
 		     test_bit(F_AUDIO_WRITE_INUSE, &dev.flags)) ||
 		    (file->f_mode & FMODE_READ &&
-		     test_bit(F_AUDIO_READ_INUSE, &dev.flags)))
-			return -EBUSY;
+		     test_bit(F_AUDIO_READ_INUSE, &dev.flags))) {
+			err = -EBUSY;
+			goto out;
+		}
 
 		if ((err = dsp_open(file)) >= 0) {
 			dev.nresets = 0;

@@ -782,7 +785,8 @@ static int dev_open(struct inode *inode, struct file *file)
 		/* nothing */
 	} else
 		err = -EINVAL;
-
+out:
+	unlock_kernel();
 	return err;
 }

sound/oss/sh_dac_audio.c +7 -2

index 4153752..8f0be40 100644
@@ -16,6 +16,7 @@
 #include <linux/slab.h>
 #include <linux/fs.h>
 #include <linux/sound.h>
+#include <linux/smp_lock.h>
 #include <linux/soundcard.h>
 #include <linux/interrupt.h>
 #include <linux/hrtimer.h>

@@ -216,13 +217,17 @@ static int dac_audio_open(struct inode *inode, struct file *file)
 {
 	if (file->f_mode & FMODE_READ)
 		return -ENODEV;
-	if (in_use)
+
+	lock_kernel();
+	if (in_use) {
+		unlock_kernel();
 		return -EBUSY;
+	}
 
 	in_use = 1;
 
 	dac_audio_start();
-
+	unlock_kernel();
 	return 0;
 }

sound/oss/soundcard.c +11 -9

index 2d9c513..92aa762 100644
@@ -210,42 +210,44 @@ static int sound_open(struct inode *inode, struct file *file)
 		printk(KERN_ERR "Invalid minor device %d\n", dev);
 		return -ENXIO;
 	}
+	lock_kernel();
 	switch (dev & 0x0f) {
 	case SND_DEV_CTL:
 		dev >>= 4;
 		if (dev >= 0 && dev < MAX_MIXER_DEV && mixer_devs[dev] == NULL) {
 			request_module("mixer%d", dev);
 		}
+		retval = -ENXIO;
 		if (dev && (dev >= num_mixers || mixer_devs[dev] == NULL))
-			return -ENXIO;
+			break;
 	
 		if (!try_module_get(mixer_devs[dev]->owner))
-			return -ENXIO;
+			break;
+
+		retval = 0;
 		break;
 
 	case SND_DEV_SEQ:
 	case SND_DEV_SEQ2:
-		if ((retval = sequencer_open(dev, file)) < 0)
-			return retval;
+		retval = sequencer_open(dev, file);
 		break;
 
 	case SND_DEV_MIDIN:
-		if ((retval = MIDIbuf_open(dev, file)) < 0)
-			return retval;
+		retval = MIDIbuf_open(dev, file);
 		break;
 
 	case SND_DEV_DSP:
 	case SND_DEV_DSP16:
 	case SND_DEV_AUDIO:
-		if ((retval = audio_open(dev, file)) < 0)
-			return retval;
+		retval = audio_open(dev, file);
 		break;
 
 	default:
 		printk(KERN_ERR "Invalid minor device %d\n", dev);
-		return -ENXIO;
+		retval = -ENXIO;
 	}
 
+	unlock_kernel();
 	return 0;
 }

sound/oss/swarm_cs4297a.c +16 -1

index 3136c88..34b0838 100644
@@ -68,6 +68,7 @@
 #include <linux/delay.h>
 #include <linux/sound.h>
 #include <linux/slab.h>
+#include <linux/smp_lock.h>
 #include <linux/soundcard.h>
 #include <linux/ac97_codec.h>
 #include <linux/pci.h>

@@ -1534,6 +1535,7 @@ static int cs4297a_open_mixdev(struct inode *inode, struct file *file)
 	CS_DBGOUT(CS_FUNCTION | CS_OPEN, 4,
 		  printk(KERN_INFO "cs4297a: cs4297a_open_mixdev()+\n"));
 
+	lock_kernel();
 	list_for_each(entry, &cs4297a_devs)
 	{
 		s = list_entry(entry, struct cs4297a_state, list);

@@ -1544,6 +1546,8 @@ static int cs4297a_open_mixdev(struct inode *inode, struct file *file)
 	{
 		CS_DBGOUT(CS_FUNCTION | CS_OPEN | CS_ERROR, 2,
 			printk(KERN_INFO "cs4297a: cs4297a_open_mixdev()- -ENODEV\n"));
+
+		unlock_kernel();
 		return -ENODEV;
 	}
 	VALIDATE_STATE(s);

@@ -1551,6 +1555,7 @@ static int cs4297a_open_mixdev(struct inode *inode, struct file *file)
 
 	CS_DBGOUT(CS_FUNCTION | CS_OPEN, 4,
 		  printk(KERN_INFO "cs4297a: cs4297a_open_mixdev()- 0\n"));
+	unlock_kernel();
 
 	return nonseekable_open(inode, file);
 }

@@ -2369,7 +2374,7 @@ static int cs4297a_release(struct inode *inode, struct file *file)
 	return 0;
 }
 
-static int cs4297a_open(struct inode *inode, struct file *file)
+static int cs4297a_locked_open(struct inode *inode, struct file *file)
 {
 	int minor = iminor(inode);
 	struct cs4297a_state *s=NULL;

@@ -2486,6 +2491,16 @@ static int cs4297a_open(struct inode *inode, struct file *file)
 	return nonseekable_open(inode, file);
 }
 
+static int cs4297a_open(struct inode *inode, struct file *file)
+{
+	int ret;
+
+	lock_kernel();
+	ret = cs4297a_open(inode, file);
+	unlock_kernel();
+
+	return ret;
+}
 
 // ******************************************************************************************
 //   Wave (audio) file operations struct.

sound/oss/vwsnd.c +8 -0

index 20b3b32..99c94c4 100644
@@ -2921,6 +2921,7 @@ static int vwsnd_audio_open(struct inode *inode, struct file *file)
 
 	DBGE("(inode=0x%p, file=0x%p)\n", inode, file);
 
+	lock_kernel();
 	INC_USE_COUNT;
 	for (devc = vwsnd_dev_list; devc; devc = devc->next_dev)
 		if ((devc->audio_minor & ~0x0F) == (minor & ~0x0F))

@@ -2928,6 +2929,7 @@ static int vwsnd_audio_open(struct inode *inode, struct file *file)
 
 	if (devc == NULL) {
 		DEC_USE_COUNT;
+		unlock_kernel();
 		return -ENODEV;
 	}

@@ -2936,11 +2938,13 @@ static int vwsnd_audio_open(struct inode *inode, struct file *file)
 		mutex_unlock(&devc->open_mutex);
 		if (file->f_flags & O_NONBLOCK) {
 			DEC_USE_COUNT;
+			unlock_kernel();
 			return -EBUSY;
 		}
 		interruptible_sleep_on(&devc->open_wait);
 		if (signal_pending(current)) {
 			DEC_USE_COUNT;
+			unlock_kernel();
 			return -ERESTARTSYS;
 		}
 		mutex_lock(&devc->open_mutex);

@@ -2993,6 +2997,7 @@ static int vwsnd_audio_open(struct inode *inode, struct file *file)
 
 	file->private_data = devc;
 	DBGRV();
+	unlock_kernel();
 	return 0;
 }

@@ -3062,15 +3067,18 @@ static int vwsnd_mixer_open(struct inode *inode, struct file *file)
 	DBGEV("(inode=0x%p, file=0x%p)\n", inode, file);
 
 	INC_USE_COUNT;
+	lock_kernel();
 	for (devc = vwsnd_dev_list; devc; devc = devc->next_dev)
 		if (devc->mixer_minor == iminor(inode))
 			break;
 
 	if (devc == NULL) {
 		DEC_USE_COUNT;
+		unlock_kernel();
 		return -ENODEV;
 	}
 	file->private_data = devc;
+	unlock_kernel();
 	return 0;
 }

sound/sound_core.c +1 -5

index c8627fc..cb61317 100644
@@ -629,12 +629,8 @@ static int soundcore_open(struct inode *inode, struct file *file)
 		file->f_op = new_fops;
 		spin_unlock(&sound_loader_lock);
 
-		if (file->f_op->open) {
-			/* TODO: push down BKL into indivial open functions */
-			lock_kernel();
+		if (file->f_op->open)
 			err = file->f_op->open(inode,file);
-			unlock_kernel();
-		}
 
 		if (err) {
 			fops_put(file->f_op);

Takashi Iwai

2010-07-12 15:53 UTC #10 lore raw

Re: [PATCH 3/3] sound: push BKL into open functions

At Sun, 11 Jul 2010 12:16:36 +0200,

Quoted 24 lines · Arnd Bergmann

This moves the lock_kernel() call from soundcore_open to the individual OSS device drivers, where we can deal with it one driver at a time if needed, or just kill off the drivers.
All core components in ALSA already provide
adequate locking in their open()-functions
and do not require the big kernel lock, so
there is no need to add the BKL there.
Signed-off-by Arnd Bergmann <[email protected]>
---
Quoted 2 lines · On Sunday 11 July 2010 09:15:22 Jaroslav Kysela wrote:
I don't see any reason (benefit) to add gotos to these two functions.
Sorry, I had removed them and then forgot the git-add before creating the emails. Originally, I had two patches, one for pushing down the BKL into every sound driver (hence the goto) and a second patch to remove the BKL again from all the native alsa drivers. If you prefer, I can also give you the separate patches, but I figured that since none of the ALSA drivers needs the BKL, the combined patch would be better.

Quoted 1 line

This is the corrected combined version.

Thanks. I applied now to sound git tree (with a minor fix of coding-style in sound/oss/au1550_ac97.c).

BTW, do you have an updated patch for native_ioctl conversion wrt sound/*?

Takashi

Arnd Bergmann

author

2010-07-12 17:53 UTC #11 lore raw

PATCH 1/1 sound/oss: convert to unlocked_ioctl

These are the final conversions for the ioctl file operation so we can remove it in the next merge window.

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

Cc Jaroslav Kysela <[email protected]>

Cc Takashi Iwai <[email protected]>

Cc [email protected]

---

Quoted 3 lines · On Monday 12 July 2010 17:53:32 Takashi Iwai wrote:

BTW, do you have an updated patch for native_ioctl conversion wrt sound/*?

Almost forgot that, thanks for reminding me!

Arnd
---

 sound/oss/au1550_ac97.c            |   54 ++++++++++++++++++++++-------------
 sound/oss/dmasound/dmasound_core.c |   35 ++++++++++++++++++----
 sound/oss/msnd_pinnacle.c          |   15 ++++++---
 sound/oss/sh_dac_audio.c           |   18 ++++++++++--
 sound/oss/swarm_cs4297a.c          |   24 ++++++++++++---
 sound/oss/vwsnd.c                  |   24 ++++++++-------
 6 files changed, 119 insertions(+), 51 deletions(-)

sound/oss/au1550_ac97.c +34 ~~-20~~

index fb913e5..0fd256c 100644
@@ -827,22 +827,26 @@ mixdev_ioctl(struct ac97_codec *codec, unsigned int cmd,
 	return codec->mixer_ioctl(codec, cmd, arg);
 }
 
-static int
-au1550_ioctl_mixdev(struct inode *inode, struct file *file,
-			       unsigned int cmd, unsigned long arg)
+static long
+au1550_ioctl_mixdev(struct file *file, unsigned int cmd, unsigned long arg)
 {
 	struct au1550_state *s = (struct au1550_state *)file->private_data;
 	struct ac97_codec *codec = s->codec;
+	int ret;
+
+	lock_kernel();
+	ret = mixdev_ioctl(codec, cmd, arg);
+	unlock_kernel();
 
-	return mixdev_ioctl(codec, cmd, arg);
+	return ret;
 }
 
 static /*const */ struct file_operations au1550_mixer_fops = {
-	owner:THIS_MODULE,
-	llseek:au1550_llseek,
-	ioctl:au1550_ioctl_mixdev,
-	open:au1550_open_mixdev,
-	release:au1550_release_mixdev,
+	.owner		= THIS_MODULE,
+	.llseek		= au1550_llseek,
+	.unlocked_ioctl	= au1550_ioctl_mixdev,
+	.open		= au1550_open_mixdev,
+	.release	= au1550_release_mixdev,
 };
 
 static int

@@ -1346,8 +1350,7 @@ dma_count_done(struct dmabuf *db)
 
 
 static int
-au1550_ioctl(struct inode *inode, struct file *file, unsigned int cmd,
-							unsigned long arg)
+au1550_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
 	struct au1550_state *s = (struct au1550_state *)file->private_data;
 	unsigned long   flags;

@@ -1783,6 +1786,17 @@ au1550_ioctl(struct inode *inode, struct file *file, unsigned int cmd,
 	return mixdev_ioctl(s->codec, cmd, arg);
 }
 
+static long
+au1550_unlocked_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+{
+	int ret;
+
+	lock_kernel();
+	ret = au1550_ioctl(file, cmd, arg);
+	unlock_kernel();
+
+	return ret;
+}
 
 static int
 au1550_open(struct inode *inode, struct file *file)

@@ -1893,15 +1907,15 @@ au1550_release(struct inode *inode, struct file *file)
 }
 
 static /*const */ struct file_operations au1550_audio_fops = {
-	owner:		THIS_MODULE,
-	llseek:		au1550_llseek,
-	read:		au1550_read,
-	write:		au1550_write,
-	poll:		au1550_poll,
-	ioctl:		au1550_ioctl,
-	mmap:		au1550_mmap,
-	open:		au1550_open,
-	release:	au1550_release,
+	.owner		= THIS_MODULE,
+	.llseek		= au1550_llseek,
+	.read		= au1550_read,
+	.write		= au1550_write,
+	.poll		= au1550_poll,
+	.unlocked_ioctl	= au1550_unlocked_ioctl,
+	.mmap		= au1550_mmap,
+	.open		= au1550_open,
+	.release	= au1550_release,
 };
 
 MODULE_AUTHOR("Advanced Micro Devices (AMD), [email protected]");

sound/oss/dmasound/dmasound_core.c +28 -7

index 5a4f38c..6ecd41a 100644
@@ -341,8 +341,8 @@ static int mixer_release(struct inode *inode, struct file *file)
 	unlock_kernel();
 	return 0;
 }
-static int mixer_ioctl(struct inode *inode, struct file *file, u_int cmd,
-		       u_long arg)
+
+static int mixer_ioctl(struct file *file, u_int cmd, u_long arg)
 {
 	if (_SIOC_DIR(cmd) & _SIOC_WRITE)
 	    mixer.modify_counter++;

@@ -366,11 +366,22 @@ static int mixer_ioctl(struct inode *inode, struct file *file, u_int cmd,
 	return -EINVAL;
 }
 
+static long mixer_unlocked_ioctl(struct file *file, u_int cmd, u_long arg)
+{
+	int ret;
+
+	lock_kernel();
+	ret = mixer_ioctl(file, cmd, arg);
+	unlock_kernel();
+
+	return ret;
+}
+
 static const struct file_operations mixer_fops =
 {
 	.owner		= THIS_MODULE,
 	.llseek		= no_llseek,
-	.ioctl		= mixer_ioctl,
+	.unlocked_ioctl	= mixer_unlocked_ioctl,
 	.open		= mixer_open,
 	.release	= mixer_release,
 };

@@ -963,8 +974,7 @@ printk("dmasound_core: tried to set_queue_frags on a locked queue\n") ;
 	return 0 ;
 }
 
-static int sq_ioctl(struct inode *inode, struct file *file, u_int cmd,
-		    u_long arg)
+static int sq_ioctl(struct file *file, u_int cmd, u_long arg)
 {
 	int val, result;
 	u_long fmt;

@@ -1122,18 +1132,29 @@ static int sq_ioctl(struct inode *inode, struct file *file, u_int cmd,
 		return IOCTL_OUT(arg,val);
 
 	default:
-		return mixer_ioctl(inode, file, cmd, arg);
+		return mixer_ioctl(file, cmd, arg);
 	}
 	return -EINVAL;
 }
 
+static long sq_unlocked_ioctl(struct file *file, u_int cmd, u_long arg)
+{
+	int ret;
+
+	lock_kernel();
+	ret = sq_ioctl(file, cmd, arg);
+	unlock_kernel();
+
+	return ret;
+}
+
 static const struct file_operations sq_fops =
 {
 	.owner		= THIS_MODULE,
 	.llseek		= no_llseek,
 	.write		= sq_write,
 	.poll		= sq_poll,
-	.ioctl		= sq_ioctl,
+	.unlocked_ioctl	= sq_unlocked_ioctl,
 	.open		= sq_open,
 	.release	= sq_release,
 };

sound/oss/msnd_pinnacle.c +10 -5

index 153d822..9ffd29f 100644
@@ -639,21 +639,26 @@ static int mixer_ioctl(unsigned int cmd, unsigned long arg)
 	return -EINVAL;
 }
 
-static int dev_ioctl(struct inode *inode, struct file *file, unsigned int cmd, unsigned long arg)
+static long dev_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
 	int minor = iminor(inode);
+	int ret;
 
 	if (cmd == OSS_GETVERSION) {
 		int sound_version = SOUND_VERSION;
 		return put_user(sound_version, (int __user *)arg);
 	}
 
+	ret = -EINVAL;
+
+	lock_kernel();
 	if (minor == dev.dsp_minor)
-		return dsp_ioctl(file, cmd, arg);
+		ret = dsp_ioctl(file, cmd, arg);
 	else if (minor == dev.mixer_minor)
-		return mixer_ioctl(cmd, arg);
+		ret = mixer_ioctl(cmd, arg);
+	unlock_kernel();
 
-	return -EINVAL;
+	return ret;
 }
 
 static void dsp_write_flush(void)

@@ -1109,7 +1114,7 @@ static const struct file_operations dev_fileops = {
 	.owner		= THIS_MODULE,
 	.read		= dev_read,
 	.write		= dev_write,
-	.ioctl		= dev_ioctl,
+	.unlocked_ioctl	= dev_ioctl,
 	.open		= dev_open,
 	.release	= dev_release,
 };

sound/oss/sh_dac_audio.c +15 -3

index 8f0be40..fdb58eb 100644
@@ -15,6 +15,7 @@
 #include <linux/linkage.h>
 #include <linux/slab.h>
 #include <linux/fs.h>
+#include <linux/smp_lock.h>
 #include <linux/sound.h>
 #include <linux/smp_lock.h>
 #include <linux/soundcard.h>

@@ -93,7 +94,7 @@ static void dac_audio_set_rate(void)
 	wakeups_per_second = ktime_set(0, 1000000000 / rate);
 }
 
-static int dac_audio_ioctl(struct inode *inode, struct file *file,
+static int dac_audio_ioctl(struct file *file,
 			   unsigned int cmd, unsigned long arg)
 {
 	int val;

@@ -159,6 +160,17 @@ static int dac_audio_ioctl(struct inode *inode, struct file *file,
 	return -EINVAL;
 }
 
+static long dac_audio_unlocked_ioctl(struct file *file, u_int cmd, u_long arg)
+{
+	int ret;
+
+	lock_kernel();
+	ret = dac_audio_ioctl(file, cmd, arg);
+	unlock_kernel();
+
+	return ret;
+}
+
 static ssize_t dac_audio_write(struct file *file, const char *buf, size_t count,
 			       loff_t * ppos)
 {

@@ -242,8 +254,8 @@ static int dac_audio_release(struct inode *inode, struct file *file)
 
 const struct file_operations dac_audio_fops = {
       .read =		dac_audio_read,
-      .write =	dac_audio_write,
-      .ioctl =	dac_audio_ioctl,
+      .write =		dac_audio_write,
+      .unlocked_ioctl =	dac_audio_unlocked_ioctl,
       .open =		dac_audio_open,
       .release =	dac_audio_release,
 };

sound/oss/swarm_cs4297a.c +19 -5

index 34b0838..b15840a 100644
@@ -1571,11 +1571,15 @@ static int cs4297a_release_mixdev(struct inode *inode, struct file *file)
 }
 
 
-static int cs4297a_ioctl_mixdev(struct inode *inode, struct file *file,
+static int cs4297a_ioctl_mixdev(struct file *file,
 			       unsigned int cmd, unsigned long arg)
 {
-	return mixer_ioctl((struct cs4297a_state *) file->private_data, cmd,
+	int ret;
+	lock_kernel();
+	ret = mixer_ioctl((struct cs4297a_state *) file->private_data, cmd,
 			   arg);
+	unlock_kernel();
+	return ret;
 }

@@ -1585,7 +1589,7 @@ static int cs4297a_ioctl_mixdev(struct inode *inode, struct file *file,
 static const struct file_operations cs4297a_mixer_fops = {
 	.owner		= THIS_MODULE,
 	.llseek		= no_llseek,
-	.ioctl		= cs4297a_ioctl_mixdev,
+	.unlocked_ioctl	= cs4297a_ioctl_mixdev,
 	.open		= cs4297a_open_mixdev,
 	.release	= cs4297a_release_mixdev,
 };

@@ -1949,7 +1953,7 @@ static int cs4297a_mmap(struct file *file, struct vm_area_struct *vma)
 }
 
 
-static int cs4297a_ioctl(struct inode *inode, struct file *file,
+static int cs4297a_ioctl(struct file *file,
 			unsigned int cmd, unsigned long arg)
 {
 	struct cs4297a_state *s =

@@ -2342,6 +2346,16 @@ static int cs4297a_ioctl(struct inode *inode, struct file *file,
 	return mixer_ioctl(s, cmd, arg);
 }
 
+static long cs4297a_unlocked_ioctl(struct file *file, u_int cmd, u_long arg)
+{
+	int ret;
+
+	lock_kernel();
+	ret = cs4297a_ioctl(file, cmd, arg);
+	unlock_kernel();
+
+	return ret;
+}
 
 static int cs4297a_release(struct inode *inode, struct file *file)
 {

@@ -2511,7 +2525,7 @@ static const struct file_operations cs4297a_audio_fops = {
 	.read		= cs4297a_read,
 	.write		= cs4297a_write,
 	.poll		= cs4297a_poll,
-	.ioctl		= cs4297a_ioctl,
+	.unlocked_ioctl	= cs4297a_unlocked_ioctl,
 	.mmap		= cs4297a_mmap,
 	.open		= cs4297a_open,
 	.release	= cs4297a_release,

sound/oss/vwsnd.c +13 ~~-11~~

index 99c94c4..8cd73cd 100644
@@ -2429,8 +2429,7 @@ static unsigned int vwsnd_audio_poll(struct file *file,
 	return mask;
 }
 
-static int vwsnd_audio_do_ioctl(struct inode *inode,
-				struct file *file,
+static int vwsnd_audio_do_ioctl(struct file *file,
 				unsigned int cmd,
 				unsigned long arg)
 {

@@ -2446,8 +2445,8 @@ static int vwsnd_audio_do_ioctl(struct inode *inode,
 	int ival;
 
 	
-	DBGEV("(inode=0x%p, file=0x%p, cmd=0x%x, arg=0x%lx)\n",
-	      inode, file, cmd, arg);
+	DBGEV("(file=0x%p, cmd=0x%x, arg=0x%lx)\n",
+	      file, cmd, arg);
 	switch (cmd) {
 	case OSS_GETVERSION:		/* _SIOR ('M', 118, int) */
 		DBGX("OSS_GETVERSION\n");

@@ -2885,17 +2884,19 @@ static int vwsnd_audio_do_ioctl(struct inode *inode,
 	return -EINVAL;
 }
 
-static int vwsnd_audio_ioctl(struct inode *inode,
-				struct file *file,
+static long vwsnd_audio_ioctl(struct file *file,
 				unsigned int cmd,
 				unsigned long arg)
 {
 	vwsnd_dev_t *devc = (vwsnd_dev_t *) file->private_data;
 	int ret;
 
+	lock_kernel();
 	mutex_lock(&devc->io_mutex);
-	ret = vwsnd_audio_do_ioctl(inode, file, cmd, arg);
+	ret = vwsnd_audio_do_ioctl(file, cmd, arg);
 	mutex_unlock(&devc->io_mutex);
+	unlock_kernel();
+
 	return ret;
 }

@@ -3049,7 +3050,7 @@ static const struct file_operations vwsnd_audio_fops = {
 	.read =		vwsnd_audio_read,
 	.write =	vwsnd_audio_write,
 	.poll =		vwsnd_audio_poll,
-	.ioctl =	vwsnd_audio_ioctl,
+	.unlocked_ioctl = vwsnd_audio_ioctl,
 	.mmap =		vwsnd_audio_mmap,
 	.open =		vwsnd_audio_open,
 	.release =	vwsnd_audio_release,

@@ -3211,8 +3212,7 @@ static int mixer_write_ioctl(vwsnd_dev_t *devc, unsigned int nr, void __user *ar
 
 /* This is the ioctl entry to the mixer driver. */
 
-static int vwsnd_mixer_ioctl(struct inode *ioctl,
-			      struct file *file,
+static long vwsnd_mixer_ioctl(struct file *file,
 			      unsigned int cmd,
 			      unsigned long arg)
 {

@@ -3223,6 +3223,7 @@ static int vwsnd_mixer_ioctl(struct inode *ioctl,
 
 	DBGEV("(devc=0x%p, cmd=0x%x, arg=0x%lx)\n", devc, cmd, arg);
 
+	lock_kernel();
 	mutex_lock(&devc->mix_mutex);
 	{
 		if ((cmd & ~nrmask) == MIXER_READ(0))

@@ -3233,13 +3234,14 @@ static int vwsnd_mixer_ioctl(struct inode *ioctl,
 			retval = -EINVAL;
 	}
 	mutex_unlock(&devc->mix_mutex);
+	unlock_kernel();
 	return retval;
 }
 
 static const struct file_operations vwsnd_mixer_fops = {
 	.owner =	THIS_MODULE,
 	.llseek =	no_llseek,
-	.ioctl =	vwsnd_mixer_ioctl,
+	.unlocked_ioctl = vwsnd_mixer_ioctl,
 	.open =		vwsnd_mixer_open,
 	.release =	vwsnd_mixer_release,
 };

Signature · 1 line

1.7.1

Takashi Iwai

Applied

2010-07-12 20:38 UTC #12 lore raw

Re: [PATCH] sound/oss: convert to unlocked_ioctl

At Mon, 12 Jul 2010 19:53:18 +0200,

Quoted 8 lines · Arnd Bergmann

These are the final conversions for the ioctl file operation so we can remove it in the next merge window.
Signed-off-by Arnd Bergmann <[email protected]>
Cc Jaroslav Kysela <[email protected]>
Cc Takashi Iwai <[email protected]>
Cc [email protected]

Quoted 7 lines

---
Quoted 3 lines · On Monday 12 July 2010 17:53:32 Takashi Iwai wrote:
BTW, do you have an updated patch for native_ioctl conversion wrt sound/*?
Almost forgot that, thanks for reminding me!

Thanks, applied now.

Now the rest is eliminating each lock_kernel() in sound/oss/*.c :)

Takashi

Arnd Bergmann

author

2010-07-12 21:13 UTC #13 lore raw

Re: [PATCH] sound/oss: convert to unlocked_ioctl

Quoted 1 line · On Monday 12 July 2010 22:38:25 Takashi Iwai wrote:

Now the rest is eliminating each lock_kernel() in sound/oss/*.c :)

For other files, I've used a script (see below) to do this, it probably works with the OSS files as well, although I have not tried yet.

Of course, another option for OSS device drivers would be to remove the entire driver ;). Either way, my feeling is that the OSS drivers are not stopping anyone from building a kernel without CONFIG_BKL once we have introduced that symbol and made the drivers depend on it.

Arnd

---
#!/bin/bash
file=$1
name=$2
if grep -q lock_kernel ${file} ; then

    if grep -q 'include.*linux.mutex.h' ${file} ; then
            sed -i '/include.*<linux\/smp_lock.h>/d' ${file}
    else
            sed -i 's/include.*<linux\/smp_lock.h>.*$/include <linux\/mutex.h>/g' ${file}
    fi
    sed -i ${file} \
        -e "/^#include.*linux.mutex.h/,$ {
                1,/^\(static\|int\|long\)/ {
                     /^\(static\|int\|long\)/istatic DEFINE_MUTEX(${name}_mutex);

} }"  \
    -e "s/\(un\)*lock_kernel\>[ ]*()/mutex_\1lock(\&${name}_mutex)/g" \
    -e '/[      ]*cycle_kernel_lock();/d'
else
    sed -i -e '/include.*\<smp_lock.h\>/d' ${file}  \
                -e '/cycle_kernel_lock()/d'