2.5.50: sleeping function called from illegal context

2.5.50: sleeping function called from illegal context

[Duncan Sands]

Got this with today 2.5 BK tree:

Debug: sleeping function called from illegal context at include/asm/semaphore.h:119
Call Trace:
 [<c0113f1a>] __might_sleep+0x52/0x58
 [<c024291a>] snd_cs46xx_iec958_put+0x36/0xf8
 [<c0217f28>] snd_ctl_elem_write+0xe0/0x1a4
 [<c0218360>] snd_ctl_ioctl+0x184/0x2c8
 [<c01462e6>] sys_ioctl+0x1fa/0x244
 [<c01088f7>] syscall_call+0x7/0xb


-------------------------------------------------------
This SF.net email is sponsored by: Get the new Palm Tungsten T 
handheld. Power & Color in a compact size! 
http://ads.sourceforge.net/cgi-bin/redirect.pl?palm0002en

Re: 2.5.50: sleeping function called from illegal context

[Takashi Iwai]

At Tue, 3 Dec 2002 04:07:52 +0100,
Duncan Sands wrote:
> 
> Got this with today 2.5 BK tree:
> 
> Debug: sleeping function called from illegal context at include/asm/semaphore.h:119
> Call Trace:
>  [<c0113f1a>] __might_sleep+0x52/0x58
>  [<c024291a>] snd_cs46xx_iec958_put+0x36/0xf8
>  [<c0217f28>] snd_ctl_elem_write+0xe0/0x1a4
>  [<c0218360>] snd_ctl_ioctl+0x184/0x2c8
>  [<c01462e6>] sys_ioctl+0x1fa/0x244
>  [<c01088f7>] syscall_call+0x7/0xb

ouch, we are using rwlock in the control ioctls.

mutex is necessary for many controls, so we cannot suppress the use of
mutex in control callbacks.
but temporary unlocking looks ad-hoc, too...


Takashi


-------------------------------------------------------
This SF.net email is sponsored by: Get the new Palm Tungsten T 
handheld. Power & Color in a compact size! 
http://ads.sourceforge.net/cgi-bin/redirect.pl?palm0002en

Re: 2.5.50: sleeping function called from illegal context

[Duncan Sands]

On Tuesday 03 December 2002 13:01, Takashi Iwai wrote:
> At Tue, 3 Dec 2002 04:07:52 +0100,
>
> Duncan Sands wrote:
> > Got this with today 2.5 BK tree:
> >
> > Debug: sleeping function called from illegal context at
> > include/asm/semaphore.h:119 Call Trace:
> >  [<c0113f1a>] __might_sleep+0x52/0x58
> >  [<c024291a>] snd_cs46xx_iec958_put+0x36/0xf8
> >  [<c0217f28>] snd_ctl_elem_write+0xe0/0x1a4
> >  [<c0218360>] snd_ctl_ioctl+0x184/0x2c8
> >  [<c01462e6>] sys_ioctl+0x1fa/0x244
> >  [<c01088f7>] syscall_call+0x7/0xb
>
> ouch, we are using rwlock in the control ioctls.
>
> mutex is necessary for many controls, so we cannot suppress the use of
> mutex in control callbacks.
> but temporary unlocking looks ad-hoc, too...

If I understand right, the problem is that snd_ctl_elem_write
acquires control_rwlock, which is a rw spinlock.  It then calls
snd_cs46xx_iec958_put which acquires chip->spos_mutex,
which is a semaphore.  Thus the message.  Now I deduce
from the fact that  you don't use read_lock_irqsave that the
data structure is not read from interrupt context.  That means
you are only protecting against other CPUs.  So why not use
a semaphore instead of a spinlock?

All the best,

Duncan.


-------------------------------------------------------
This SF.net email is sponsored by: Get the new Palm Tungsten T 
handheld. Power & Color in a compact size! 
http://ads.sourceforge.net/cgi-bin/redirect.pl?palm0002en

Re: 2.5.50: sleeping function called from illegal context

[Takashi Iwai]

At Tue, 3 Dec 2002 04:55:15 +0100,
Duncan Sands wrote:
> 
> On Tuesday 03 December 2002 13:01, Takashi Iwai wrote:
> > At Tue, 3 Dec 2002 04:07:52 +0100,
> >
> > Duncan Sands wrote:
> > > Got this with today 2.5 BK tree:
> > >
> > > Debug: sleeping function called from illegal context at
> > > include/asm/semaphore.h:119 Call Trace:
> > >  [<c0113f1a>] __might_sleep+0x52/0x58
> > >  [<c024291a>] snd_cs46xx_iec958_put+0x36/0xf8
> > >  [<c0217f28>] snd_ctl_elem_write+0xe0/0x1a4
> > >  [<c0218360>] snd_ctl_ioctl+0x184/0x2c8
> > >  [<c01462e6>] sys_ioctl+0x1fa/0x244
> > >  [<c01088f7>] syscall_call+0x7/0xb
> >
> > ouch, we are using rwlock in the control ioctls.
> >
> > mutex is necessary for many controls, so we cannot suppress the use of
> > mutex in control callbacks.
> > but temporary unlocking looks ad-hoc, too...
> 
> If I understand right, the problem is that snd_ctl_elem_write
> acquires control_rwlock, which is a rw spinlock.  It then calls
> snd_cs46xx_iec958_put which acquires chip->spos_mutex,
> which is a semaphore.  Thus the message.  Now I deduce
> from the fact that  you don't use read_lock_irqsave that the
> data structure is not read from interrupt context.  That means
> you are only protecting against other CPUs.  So why not use
> a semaphore instead of a spinlock?

hmm, there are some places calling with irq lock.
for example, snd_ctl_notify() can be called from the interrupt handler
for some interrupts like h/w volume change.

however, it seems that a single rwlock is used for the management of
two different lists.  and snd_ctl_elem_write() uses card->controls
only, whereas snd_ctl_notify() uses card->ctl_files only.
hence, we can merge two locks, rwsem for card->controls and rwlock for
card->ctl_files.  i'll give a try.


ciao,

Takashi


-------------------------------------------------------
This SF.net email is sponsored by: Get the new Palm Tungsten T 
handheld. Power & Color in a compact size! 
http://ads.sourceforge.net/cgi-bin/redirect.pl?palm0002en

Re: 2.5.50: sleeping function called from illegal context

[Duncan Sands]

On Tuesday 03 December 2002 14:56, Takashi Iwai wrote:
> At Tue, 3 Dec 2002 04:55:15 +0100,
>
> Duncan Sands wrote:
> > On Tuesday 03 December 2002 13:01, Takashi Iwai wrote:
> > > At Tue, 3 Dec 2002 04:07:52 +0100,
> > >
> > > Duncan Sands wrote:
> > > > Got this with today 2.5 BK tree:
> > > >
> > > > Debug: sleeping function called from illegal context at
> > > > include/asm/semaphore.h:119 Call Trace:
> > > >  [<c0113f1a>] __might_sleep+0x52/0x58
> > > >  [<c024291a>] snd_cs46xx_iec958_put+0x36/0xf8
> > > >  [<c0217f28>] snd_ctl_elem_write+0xe0/0x1a4
> > > >  [<c0218360>] snd_ctl_ioctl+0x184/0x2c8
> > > >  [<c01462e6>] sys_ioctl+0x1fa/0x244
> > > >  [<c01088f7>] syscall_call+0x7/0xb
> > >
> > > ouch, we are using rwlock in the control ioctls.
> > >
> > > mutex is necessary for many controls, so we cannot suppress the use of
> > > mutex in control callbacks.
> > > but temporary unlocking looks ad-hoc, too...
> >
> > If I understand right, the problem is that snd_ctl_elem_write
> > acquires control_rwlock, which is a rw spinlock.  It then calls
> > snd_cs46xx_iec958_put which acquires chip->spos_mutex,
> > which is a semaphore.  Thus the message.  Now I deduce
> > from the fact that  you don't use read_lock_irqsave that the
> > data structure is not read from interrupt context.  That means
> > you are only protecting against other CPUs.  So why not use
> > a semaphore instead of a spinlock?
>
> hmm, there are some places calling with irq lock.
> for example, snd_ctl_notify() can be called from the interrupt handler
> for some interrupts like h/w volume change.

Uh oh!

> however, it seems that a single rwlock is used for the management of
> two different lists.  and snd_ctl_elem_write() uses card->controls
> only, whereas snd_ctl_notify() uses card->ctl_files only.
> hence, we can merge two locks, rwsem for card->controls and rwlock for
> card->ctl_files.  i'll give a try.

I guess another way of dealing with this kind of problem is to use a
semaphore rather than a spinlock, and a workqueue: when the interrupt
comes in, the call to snd_ctl_notify is put on the queue, where it will later
be run in process context, and can safely take the semaphore.

Duncan.


-------------------------------------------------------
This SF.net email is sponsored by: Get the new Palm Tungsten T 
handheld. Power & Color in a compact size! 
http://ads.sourceforge.net/cgi-bin/redirect.pl?palm0002en

Re: 2.5.50: sleeping function called from illegal context

[Takashi Iwai]

[-- Attachment #1: Type: text/plain, Size: 2620 bytes --]

At Tue, 3 Dec 2002 06:57:45 +0100,
Duncan Sands wrote:
> 
> On Tuesday 03 December 2002 14:56, Takashi Iwai wrote:
> > At Tue, 3 Dec 2002 04:55:15 +0100,
> >
> > Duncan Sands wrote:
> > > On Tuesday 03 December 2002 13:01, Takashi Iwai wrote:
> > > > At Tue, 3 Dec 2002 04:07:52 +0100,
> > > >
> > > > Duncan Sands wrote:
> > > > > Got this with today 2.5 BK tree:
> > > > >
> > > > > Debug: sleeping function called from illegal context at
> > > > > include/asm/semaphore.h:119 Call Trace:
> > > > >  [<c0113f1a>] __might_sleep+0x52/0x58
> > > > >  [<c024291a>] snd_cs46xx_iec958_put+0x36/0xf8
> > > > >  [<c0217f28>] snd_ctl_elem_write+0xe0/0x1a4
> > > > >  [<c0218360>] snd_ctl_ioctl+0x184/0x2c8
> > > > >  [<c01462e6>] sys_ioctl+0x1fa/0x244
> > > > >  [<c01088f7>] syscall_call+0x7/0xb
> > > >
> > > > ouch, we are using rwlock in the control ioctls.
> > > >
> > > > mutex is necessary for many controls, so we cannot suppress the use of
> > > > mutex in control callbacks.
> > > > but temporary unlocking looks ad-hoc, too...
> > >
> > > If I understand right, the problem is that snd_ctl_elem_write
> > > acquires control_rwlock, which is a rw spinlock.  It then calls
> > > snd_cs46xx_iec958_put which acquires chip->spos_mutex,
> > > which is a semaphore.  Thus the message.  Now I deduce
> > > from the fact that  you don't use read_lock_irqsave that the
> > > data structure is not read from interrupt context.  That means
> > > you are only protecting against other CPUs.  So why not use
> > > a semaphore instead of a spinlock?
> >
> > hmm, there are some places calling with irq lock.
> > for example, snd_ctl_notify() can be called from the interrupt handler
> > for some interrupts like h/w volume change.
> 
> Uh oh!
> 
> > however, it seems that a single rwlock is used for the management of
> > two different lists.  and snd_ctl_elem_write() uses card->controls
> > only, whereas snd_ctl_notify() uses card->ctl_files only.
> > hence, we can merge two locks, rwsem for card->controls and rwlock for
> > card->ctl_files.  i'll give a try.
> 
> I guess another way of dealing with this kind of problem is to use a
> semaphore rather than a spinlock, and a workqueue: when the interrupt
> comes in, the call to snd_ctl_notify is put on the queue, where it will later
> be run in process context, and can safely take the semaphore.

yes, basically what snd_ctl_notify() does is the same.
it queues an event and wakes up the sleepers.
thus, it's ok to separate the stuff from the semaphore.

the attached is a patch to rewrite the locks with rwsem.
please check whether it works for you.


thanks,

Takashi

[-- Attachment #2: control-lock-fix.dif --]
[-- Type: application/octet-stream, Size: 20990 bytes --]

Index: alsa-kernel/include/core.h
===================================================================
RCS file: /suse/tiwai/cvs/alsa/alsa-kernel/include/core.h,v
retrieving revision 1.26
diff -u -r1.26 core.h
--- alsa-kernel/include/core.h	2 Dec 2002 09:25:18 -0000	1.26
+++ alsa-kernel/include/core.h	3 Dec 2002 14:29:58 -0000
@@ -24,6 +24,7 @@
 
 #include <linux/sched.h>		/* wake_up() */
 #include <asm/semaphore.h>		/* struct semaphore */
+#include <linux/rwsem.h>		/* struct rw_semaphore */
 
 /* Typedef's */
 typedef struct timeval snd_timestamp_t;
@@ -141,8 +142,8 @@
 	struct list_head devices;	/* devices */
 
 	unsigned int last_numid;	/* last used numeric ID */
-	rwlock_t control_rwlock;	/* control list lock */
-	rwlock_t control_owner_lock;	/* control list lock */
+	struct rw_semaphore controls_rwsem;	/* controls list lock */
+	rwlock_t ctl_files_rwlock;	/* ctl_files list lock */
 	int controls_count;		/* count of all controls */
 	struct list_head controls;	/* all controls for this card */
 	struct list_head ctl_files;	/* active control files */
Index: alsa-kernel/core/control.c
===================================================================
RCS file: /suse/tiwai/cvs/alsa/alsa-kernel/core/control.c,v
retrieving revision 1.17
diff -u -r1.17 control.c
--- alsa-kernel/core/control.c	2 Dec 2002 09:25:17 -0000	1.17
+++ alsa-kernel/core/control.c	3 Dec 2002 14:45:15 -0000
@@ -37,7 +37,7 @@
 
 #define snd_kctl_ioctl(n) list_entry(n, snd_kctl_ioctl_t, list)
 
-static rwlock_t snd_ioctl_rwlock = RW_LOCK_UNLOCKED;
+static DECLARE_RWSEM(snd_ioctl_rwsem);
 static LIST_HEAD(snd_control_ioctls);
 
 static inline void dec_mod_count(struct module *module)
@@ -82,9 +82,9 @@
 	ctl->card = card;
 	ctl->pid = current->pid;
 	file->private_data = ctl;
-	write_lock_irqsave(&card->control_rwlock, flags);
+	write_lock_irqsave(&card->ctl_files_rwlock, flags);
 	list_add_tail(&ctl->list, &card->ctl_files);
-	write_unlock_irqrestore(&card->control_rwlock, flags);
+	write_unlock_irqrestore(&card->ctl_files_rwlock, flags);
 	return 0;
 
       __error:
@@ -123,16 +123,16 @@
 	fasync_helper(-1, file, 0, &ctl->fasync);
 	file->private_data = NULL;
 	card = ctl->card;
-	write_lock_irqsave(&card->control_rwlock, flags);
+	write_lock_irqsave(&card->ctl_files_rwlock, flags);
 	list_del(&ctl->list);
-	write_lock(&card->control_owner_lock);
+	write_unlock_irqrestore(&card->ctl_files_rwlock, flags);
+	down_write(&card->controls_rwsem);
 	list_for_each(list, &card->controls) {
 		control = snd_kcontrol(list);
 		if (control->owner == ctl)
 			control->owner = NULL;
 	}
-	write_unlock(&card->control_owner_lock);
-	write_unlock_irqrestore(&card->control_rwlock, flags);
+	up_write(&card->controls_rwsem);
 	snd_ctl_empty_read_queue(ctl);
 	snd_magic_kfree(ctl);
 	dec_mod_count(card->module);
@@ -151,7 +151,7 @@
 	snd_kctl_event_t *ev;
 	
 	snd_runtime_check(card != NULL && id != NULL, return);
-	read_lock_irqsave(&card->control_rwlock, flags);
+	read_lock(&card->ctl_files_rwlock);
 #if defined(CONFIG_SND_MIXER_OSS) || defined(CONFIG_SND_MIXER_OSS_MODULE)
 	card->mixer_oss_change_count++;
 #endif
@@ -160,7 +160,7 @@
 		ctl = snd_ctl_file(flist);
 		if (!ctl->subscribed)
 			continue;
-		spin_lock(&ctl->read_lock);
+		spin_lock_irqsave(&ctl->read_lock, flags);
 		list_for_each(elist, &ctl->events) {
 			ev = snd_kctl_event(elist);
 			if (ev->id.numid == id->numid) {
@@ -179,9 +179,9 @@
 	_found:
 		wake_up(&ctl->change_sleep);
 		kill_fasync(&ctl->fasync, SIGIO, POLL_IN);
-		spin_unlock(&ctl->read_lock);
+		spin_unlock_irqrestore(&ctl->read_lock, flags);
 	}
-	read_unlock_irqrestore(&card->control_rwlock, flags);
+	read_unlock(&card->ctl_files_rwlock);
 }
 
 snd_kcontrol_t *snd_ctl_new(snd_kcontrol_t * control)
@@ -233,11 +233,11 @@
 	snd_assert(kcontrol->info != NULL, return -EINVAL);
 	snd_assert(!(kcontrol->access & SNDRV_CTL_ELEM_ACCESS_READ) || kcontrol->get != NULL, return -EINVAL);
 	snd_assert(!(kcontrol->access & SNDRV_CTL_ELEM_ACCESS_WRITE) || kcontrol->put != NULL, return -EINVAL);
-	write_lock(&card->control_rwlock);
+	down_write(&card->controls_rwsem);
 	list_add_tail(&kcontrol->list, &card->controls);
 	card->controls_count++;
 	kcontrol->id.numid = ++card->last_numid;
-	write_unlock(&card->control_rwlock);
+	up_write(&card->controls_rwsem);
 	snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_ADD, &kcontrol->id);
 	return 0;
 }
@@ -245,10 +245,10 @@
 int snd_ctl_remove(snd_card_t * card, snd_kcontrol_t * kcontrol)
 {
 	snd_runtime_check(card != NULL && kcontrol != NULL, return -EINVAL);
-	write_lock(&card->control_rwlock);
+	down_write(&card->controls_rwsem);
 	list_del(&kcontrol->list);
 	card->controls_count--;
-	write_unlock(&card->control_rwlock);
+	up_write(&card->controls_rwsem);
 	snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_REMOVE, &kcontrol->id);
 	snd_ctl_free_one(kcontrol);
 	return 0;
@@ -264,47 +264,46 @@
 	return snd_ctl_remove(card, kctl);
 }
 
+static snd_kcontrol_t *_ctl_find_id(snd_card_t * card, snd_ctl_elem_id_t *id); /* w/o lock */
+
 int snd_ctl_rename_id(snd_card_t * card, snd_ctl_elem_id_t *src_id, snd_ctl_elem_id_t *dst_id)
 {
 	snd_kcontrol_t *kctl;
 
-	kctl = snd_ctl_find_id(card, src_id);
-	if (kctl == NULL)
+	down_write(&card->controls_rwsem);
+	kctl = _ctl_find_id(card, src_id);
+	if (kctl == NULL) {
+		up_write(&card->controls_rwsem);
 		return -ENOENT;
-	write_lock(&card->control_rwlock);
+	}
 	kctl->id = *dst_id;
 	kctl->id.numid = ++card->last_numid;
-	write_unlock(&card->control_rwlock);
+	up_write(&card->controls_rwsem);
 	return 0;
 }
 
-snd_kcontrol_t *snd_ctl_find_numid(snd_card_t * card, unsigned int numid)
+static snd_kcontrol_t *_ctl_find_numid(snd_card_t * card, unsigned int numid)
 {
 	struct list_head *list;
 	snd_kcontrol_t *kctl;
 
 	snd_runtime_check(card != NULL && numid != 0, return NULL);
-	read_lock(&card->control_rwlock);
 	list_for_each(list, &card->controls) {
 		kctl = snd_kcontrol(list);
-		if (kctl->id.numid == numid) {
-			read_unlock(&card->control_rwlock);
+		if (kctl->id.numid == numid)
 			return kctl;
-		}
 	}
-	read_unlock(&card->control_rwlock);
 	return NULL;
 }
 
-snd_kcontrol_t *snd_ctl_find_id(snd_card_t * card, snd_ctl_elem_id_t *id)
+static snd_kcontrol_t *_ctl_find_id(snd_card_t * card, snd_ctl_elem_id_t *id)
 {
 	struct list_head *list;
 	snd_kcontrol_t *kctl;
 
 	snd_runtime_check(card != NULL && id != NULL, return NULL);
 	if (id->numid != 0)
-		return snd_ctl_find_numid(card, id->numid);
-	read_lock(&card->control_rwlock);
+		return _ctl_find_numid(card, id->numid);
 	list_for_each(list, &card->controls) {
 		kctl = snd_kcontrol(list);
 		if (kctl->id.iface != id->iface)
@@ -317,20 +316,38 @@
 			continue;
 		if (kctl->id.index != id->index)
 			continue;
-		read_unlock(&card->control_rwlock);
 		return kctl;
 	}
-	read_unlock(&card->control_rwlock);
 	return NULL;
 }
 
+/* exported: with read lock */
+snd_kcontrol_t *snd_ctl_find_id(snd_card_t * card, snd_ctl_elem_id_t *id)
+{
+	snd_kcontrol_t *kctl;
+	down_read(&card->controls_rwsem);
+	kctl = _ctl_find_id(card, id);
+	up_read(&card->controls_rwsem);
+	return kctl;
+}
+
+/* exported: with read lock */
+snd_kcontrol_t *snd_ctl_find_numid(snd_card_t * card, unsigned int numid)
+{
+	snd_kcontrol_t *kctl;
+	down_read(&card->controls_rwsem);
+	kctl = _ctl_find_numid(card, numid);
+	up_read(&card->controls_rwsem);
+	return kctl;
+}
+
 static int snd_ctl_card_info(snd_card_t * card, snd_ctl_file_t * ctl,
 			     unsigned int cmd, unsigned long arg)
 {
 	snd_ctl_card_info_t info;
 
 	memset(&info, 0, sizeof(info));
-	read_lock(&snd_ioctl_rwlock);
+	down_read(&snd_ioctl_rwsem);
 	info.card = card->number;
 	strncpy(info.id, card->id, sizeof(info.id) - 1);
 	strncpy(info.driver, card->driver, sizeof(info.driver) - 1);
@@ -338,7 +355,7 @@
 	strncpy(info.longname, card->longname, sizeof(info.longname) - 1);
 	strncpy(info.mixername, card->mixername, sizeof(info.mixername) - 1);
 	strncpy(info.components, card->components, sizeof(info.components) - 1);
-	read_unlock(&snd_ioctl_rwlock);
+	up_read(&snd_ioctl_rwsem);
 	if (copy_to_user((void *) arg, &info, sizeof(snd_ctl_card_info_t)))
 		return -EFAULT;
 	return 0;
@@ -364,7 +381,7 @@
 		dst = vmalloc(space * sizeof(snd_ctl_elem_id_t));
 		if (dst == NULL)
 			return -ENOMEM;
-		read_lock(&card->control_rwlock);
+		down_read(&card->controls_rwsem);
 		list.count = card->controls_count;
 		plist = card->controls.next;
 		while (offset-- > 0 && plist != &card->controls)
@@ -379,14 +396,14 @@
 			space--;
 			list.used++;
 		}
-		read_unlock(&card->control_rwlock);
+		up_read(&card->controls_rwsem);
 		if (list.used > 0 && copy_to_user(list.pids, dst, list.used * sizeof(snd_ctl_elem_id_t)))
 			return -EFAULT;
 		vfree(dst);
 	} else {
-		read_lock(&card->control_rwlock);
+		down_read(&card->controls_rwsem);
 		list.count = card->controls_count;
-		read_unlock(&card->control_rwlock);
+		up_read(&card->controls_rwsem);
 	}
 	if (copy_to_user(_list, &list, sizeof(list)))
 		return -EFAULT;
@@ -402,10 +419,10 @@
 	
 	if (copy_from_user(&info, _info, sizeof(info)))
 		return -EFAULT;
-	read_lock(&card->control_rwlock);
-	kctl = snd_ctl_find_id(card, &info.id);
+	down_read(&card->controls_rwsem);
+	kctl = _ctl_find_id(card, &info.id);
 	if (kctl == NULL) {
-		read_unlock(&card->control_rwlock);
+		up_read(&card->controls_rwsem);
 		return -ENOENT;
 	}
 #ifdef CONFIG_SND_DEBUG
@@ -425,7 +442,7 @@
 			info.owner = -1;
 		}
 	}
-	read_unlock(&card->control_rwlock);
+	up_read(&card->controls_rwsem);
 	if (result >= 0)
 		if (copy_to_user(_info, &info, sizeof(info)))
 			return -EFAULT;
@@ -443,8 +460,8 @@
 		return -ENOMEM;	
 	if (copy_from_user(control, _control, sizeof(*control)))
 		return -EFAULT;
-	read_lock(&card->control_rwlock);
-	kctl = snd_ctl_find_id(card, &control->id);
+	down_read(&card->controls_rwsem);
+	kctl = _ctl_find_id(card, &control->id);
 	if (kctl == NULL) {
 		result = -ENOENT;
 	} else {
@@ -460,7 +477,7 @@
 				result = -EPERM;
 		}
 	}
-	read_unlock(&card->control_rwlock);
+	up_read(&card->controls_rwsem);
 	if (result >= 0)
 		if (copy_to_user(_control, control, sizeof(*control)))
 			return -EFAULT;
@@ -480,8 +497,8 @@
 		return -ENOMEM;	
 	if (copy_from_user(control, _control, sizeof(*control)))
 		return -EFAULT;
-	read_lock(&card->control_rwlock);
-	kctl = snd_ctl_find_id(card, &control->id);
+	down_read(&card->controls_rwsem);
+	kctl = _ctl_find_id(card, &control->id);
 	if (kctl == NULL) {
 		result = -ENOENT;
 	} else {
@@ -489,7 +506,6 @@
 		if (control->indirect != indirect) {
 			result = -EACCES;
 		} else {
-			read_lock(&card->control_owner_lock);
 			if (!(kctl->access & SNDRV_CTL_ELEM_ACCESS_WRITE) ||
 			    kctl->put == NULL ||
 			    (kctl->owner != NULL && kctl->owner != file)) {
@@ -499,16 +515,15 @@
 				if (result >= 0)
 					control->id = kctl->id;
 			}
-			read_unlock(&card->control_owner_lock);
 			if (result > 0) {
-				read_unlock(&card->control_rwlock);
+				up_read(&card->controls_rwsem);
 				snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_VALUE, &kctl->id);
 				result = 0;
 				goto __unlocked;
 			}
 		}
 	}
-	read_unlock(&card->control_rwlock);
+	up_read(&card->controls_rwsem);
       __unlocked:
 	if (result >= 0)
 		if (copy_to_user(_control, control, sizeof(*control)))
@@ -526,12 +541,11 @@
 	
 	if (copy_from_user(&id, _id, sizeof(id)))
 		return -EFAULT;
-	read_lock(&card->control_rwlock);
-	kctl = snd_ctl_find_id(card, &id);
+	down_write(&card->controls_rwsem);
+	kctl = _ctl_find_id(card, &id);
 	if (kctl == NULL) {
 		result = -ENOENT;
 	} else {
-		write_lock(&card->control_owner_lock);
 		if (kctl->owner != NULL)
 			result = -EBUSY;
 		else {
@@ -539,9 +553,8 @@
 			kctl->owner_pid = current->pid;
 			result = 0;
 		}
-		write_unlock(&card->control_owner_lock);
 	}
-	read_unlock(&card->control_rwlock);
+	up_write(&card->controls_rwsem);
 	return result;
 }
 
@@ -554,12 +567,11 @@
 	
 	if (copy_from_user(&id, _id, sizeof(id)))
 		return -EFAULT;
-	read_lock(&card->control_rwlock);
-	kctl = snd_ctl_find_id(card, &id);
+	down_write(&card->controls_rwsem);
+	kctl = _ctl_find_id(card, &id);
 	if (kctl == NULL) {
 		result = -ENOENT;
 	} else {
-		write_lock(&card->control_owner_lock);
 		if (kctl->owner == NULL)
 			result = -EINVAL;
 		else if (kctl->owner != file)
@@ -569,9 +581,8 @@
 			kctl->owner_pid = 0;
 			result = 0;
 		}
-		write_unlock(&card->control_owner_lock);
 	}
-	read_unlock(&card->control_rwlock);
+	up_write(&card->controls_rwsem);
 	return result;
 }
 
@@ -646,16 +657,16 @@
 		return put_user(SNDRV_CTL_POWER_D0, (int *)arg) ? -EFAULT : 0;
 #endif
 	}
-	read_lock(&snd_ioctl_rwlock);
+	down_read(&snd_ioctl_rwsem);
 	list_for_each(list, &snd_control_ioctls) {
 		p = list_entry(list, snd_kctl_ioctl_t, list);
 		err = p->fioctl(card, ctl, cmd, arg);
 		if (err != -ENOIOCTLCMD) {
-			read_unlock(&snd_ioctl_rwlock);
+			up_read(&snd_ioctl_rwsem);
 			return err;
 		}
 	}
-	read_unlock(&snd_ioctl_rwlock);
+	up_read(&snd_ioctl_rwsem);
 	snd_printd("unknown ioctl = 0x%x\n", cmd);
 	return -ENOTTY;
 }
@@ -740,9 +751,9 @@
 	if (pn == NULL)
 		return -ENOMEM;
 	pn->fioctl = fcn;
-	write_lock(&snd_ioctl_rwlock);
+	down_write(&snd_ioctl_rwsem);
 	list_add_tail(&pn->list, &snd_control_ioctls);
-	write_unlock(&snd_ioctl_rwlock);
+	up_write(&snd_ioctl_rwsem);
 	return 0;
 }
 
@@ -752,17 +763,17 @@
 	snd_kctl_ioctl_t *p;
 
 	snd_runtime_check(fcn != NULL, return -EINVAL);
-	write_lock(&snd_ioctl_rwlock);
+	down_write(&snd_ioctl_rwsem);
 	list_for_each(list, &snd_control_ioctls) {
 		p = list_entry(list, snd_kctl_ioctl_t, list);
 		if (p->fioctl == fcn) {
 			list_del(&p->list);
-			write_unlock(&snd_ioctl_rwlock);
+			up_write(&snd_ioctl_rwsem);
 			kfree(p);
 			return 0;
 		}
 	}
-	write_unlock(&snd_ioctl_rwlock);
+	up_write(&snd_ioctl_rwsem);
 	snd_BUG();
 	return -EINVAL;
 }
@@ -821,13 +832,13 @@
 	struct list_head *flist;
 	snd_ctl_file_t *ctl;
 
-	read_lock_irq(&card->control_rwlock);
+	down_read(&card->controls_rwsem);
 	list_for_each(flist, &card->ctl_files) {
 		ctl = snd_ctl_file(flist);
 		wake_up(&ctl->change_sleep);
 		kill_fasync(&ctl->fasync, SIGIO, POLL_ERR);
 	}
-	read_unlock_irq(&card->control_rwlock);
+	up_read(&card->controls_rwsem);
 	return 0;
 }
 
Index: alsa-kernel/core/init.c
===================================================================
RCS file: /suse/tiwai/cvs/alsa/alsa-kernel/core/init.c,v
retrieving revision 1.17
diff -u -r1.17 init.c
--- alsa-kernel/core/init.c	3 Dec 2002 11:02:36 -0000	1.17
+++ alsa-kernel/core/init.c	3 Dec 2002 14:30:16 -0000
@@ -99,8 +99,8 @@
 	card->number = idx;
 	card->module = module;
 	INIT_LIST_HEAD(&card->devices);
-	rwlock_init(&card->control_rwlock);
-	rwlock_init(&card->control_owner_lock);
+	init_rwsem(&card->controls_rwsem);
+	rwlock_init(&card->ctl_files_rwlock);
 	INIT_LIST_HEAD(&card->controls);
 	INIT_LIST_HEAD(&card->ctl_files);
 	spin_lock_init(&card->files_lock);
Index: alsa-kernel/core/pcm.c
===================================================================
RCS file: /suse/tiwai/cvs/alsa/alsa-kernel/core/pcm.c,v
retrieving revision 1.19
diff -u -r1.19 pcm.c
--- alsa-kernel/core/pcm.c	2 Dec 2002 09:25:17 -0000	1.19
+++ alsa-kernel/core/pcm.c	3 Dec 2002 14:28:54 -0000
@@ -698,7 +698,7 @@
 		return -ENODEV;
 
 	card = pcm->card;
-	read_lock(&card->control_rwlock);
+	down_read(&card->controls_rwsem);
 	list_for_each(list, &card->ctl_files) {
 		kctl = snd_ctl_file(list);
 		if (kctl->pid == current->pid) {
@@ -706,7 +706,7 @@
 			break;
 		}
 	}
-	read_unlock(&card->control_rwlock);
+	up_read(&card->controls_rwsem);
 
 	if (pstr->substream_count == 0)
 		return -ENODEV;
Index: alsa-kernel/core/rawmidi.c
===================================================================
RCS file: /suse/tiwai/cvs/alsa/alsa-kernel/core/rawmidi.c,v
retrieving revision 1.22
diff -u -r1.22 rawmidi.c
--- alsa-kernel/core/rawmidi.c	2 Dec 2002 09:25:18 -0000	1.22
+++ alsa-kernel/core/rawmidi.c	3 Dec 2002 14:31:09 -0000
@@ -412,7 +412,7 @@
 	add_wait_queue(&rmidi->open_wait, &wait);
 	while (1) {
 		subdevice = -1;
-		read_lock(&card->control_rwlock);
+		down_read(&card->controls_rwsem);
 		list_for_each(list, &card->ctl_files) {
 			kctl = snd_ctl_file(list);
 			if (kctl->pid == current->pid) {
@@ -420,7 +420,7 @@
 				break;
 			}
 		}
-		read_unlock(&card->control_rwlock);
+		up_read(&card->controls_rwsem);
 		err = snd_rawmidi_kernel_open(cardnum, device, subdevice, fflags, rawmidi_file);
 		if (err >= 0)
 			break;
Index: alsa-kernel/core/oss/mixer_oss.c
===================================================================
RCS file: /suse/tiwai/cvs/alsa/alsa-kernel/core/oss/mixer_oss.c,v
retrieving revision 1.14
diff -u -r1.14 mixer_oss.c
--- alsa-kernel/core/oss/mixer_oss.c	2 Dec 2002 09:25:18 -0000	1.14
+++ alsa-kernel/core/oss/mixer_oss.c	3 Dec 2002 14:36:03 -0000
@@ -562,7 +562,7 @@
 	struct slot *slot = (struct slot *)pslot->private_data;
 	
 	*left = *right = 100;
-	read_lock(&card->control_rwlock);
+	down_read(&card->controls_rwsem);
 	if (slot->present & SNDRV_MIXER_OSS_PRESENT_PVOLUME) {
 		snd_mixer_oss_get_volume1_vol(fmixer, pslot, slot->kcontrol[SNDRV_MIXER_OSS_ITEM_PVOLUME], left, right);
 	} else if (slot->present & SNDRV_MIXER_OSS_PRESENT_GVOLUME) {
@@ -579,7 +579,7 @@
 	} else if (slot->present & SNDRV_MIXER_OSS_PRESENT_GROUTE) {
 		snd_mixer_oss_get_volume1_sw(fmixer, pslot, slot->kcontrol[SNDRV_MIXER_OSS_ITEM_GROUTE], left, right, 1);
 	}
-	read_unlock(&card->control_rwlock);
+	up_read(&card->controls_rwsem);
 	return 0;
 }
 
@@ -655,7 +655,7 @@
 	snd_card_t *card = fmixer->card;
 	struct slot *slot = (struct slot *)pslot->private_data;
 	
-	read_lock(&card->control_rwlock);
+	down_read(&card->controls_rwsem);
 	if (slot->present & SNDRV_MIXER_OSS_PRESENT_PVOLUME) {
 		snd_mixer_oss_put_volume1_vol(fmixer, pslot, slot->kcontrol[SNDRV_MIXER_OSS_ITEM_PVOLUME], left, right);
 		if (slot->present & SNDRV_MIXER_OSS_PRESENT_CVOLUME)
@@ -685,7 +685,7 @@
 			snd_mixer_oss_put_volume1_sw(fmixer, pslot, slot->kcontrol[SNDRV_MIXER_OSS_ITEM_GROUTE], left, right, 1);
 		}
 	}
-	read_unlock(&card->control_rwlock);
+	up_read(&card->controls_rwsem);
 	return 0;
 }
 
@@ -698,9 +698,9 @@
 	int left, right;
 	
 	left = right = 1;
-	read_lock(&card->control_rwlock);
+	down_read(&card->controls_rwsem);
 	snd_mixer_oss_get_volume1_sw(fmixer, pslot, slot->kcontrol[SNDRV_MIXER_OSS_ITEM_CSWITCH], &left, &right, 0);
-	read_unlock(&card->control_rwlock);
+	up_read(&card->controls_rwsem);
 	*active = (left || right) ? 1 : 0;
 	return 0;
 }
@@ -714,9 +714,9 @@
 	int left, right;
 	
 	left = right = 1;
-	read_lock(&card->control_rwlock);
+	down_read(&card->controls_rwsem);
 	snd_mixer_oss_get_volume1_sw(fmixer, pslot, slot->kcontrol[SNDRV_MIXER_OSS_ITEM_CROUTE], &left, &right, 1);
-	read_unlock(&card->control_rwlock);
+	up_read(&card->controls_rwsem);
 	*active = (left || right) ? 1 : 0;
 	return 0;
 }
@@ -728,9 +728,9 @@
 	snd_card_t *card = fmixer->card;
 	struct slot *slot = (struct slot *)pslot->private_data;
 	
-	read_lock(&card->control_rwlock);
+	down_read(&card->controls_rwsem);
 	snd_mixer_oss_put_volume1_sw(fmixer, pslot, slot->kcontrol[SNDRV_MIXER_OSS_ITEM_CSWITCH], active, active, 0);
-	read_unlock(&card->control_rwlock);
+	up_read(&card->controls_rwsem);
 	return 0;
 }
 
@@ -741,9 +741,9 @@
 	snd_card_t *card = fmixer->card;
 	struct slot *slot = (struct slot *)pslot->private_data;
 	
-	read_lock(&card->control_rwlock);
+	down_read(&card->controls_rwsem);
 	snd_mixer_oss_put_volume1_sw(fmixer, pslot, slot->kcontrol[SNDRV_MIXER_OSS_ITEM_CROUTE], active, active, 1);
-	read_unlock(&card->control_rwlock);
+	up_read(&card->controls_rwsem);
 	return 0;
 }
 
@@ -764,12 +764,11 @@
 		err = -ENOMEM;
 		goto __unlock;
 	}
-	read_lock(&card->control_rwlock);
+	down_read(&card->controls_rwsem);
 	kctl = snd_mixer_oss_test_id(mixer, "Capture Source", 0);
 	snd_runtime_check(kctl != NULL, err = -ENOENT; goto __unlock);
 	snd_runtime_check(!(err = kctl->info(kctl, uinfo)), goto __unlock);
 	snd_runtime_check(!(err = kctl->get(kctl, uctl)), goto __unlock);
-	read_unlock(&card->control_rwlock);
 	for (idx = 0; idx < 32; idx++) {
 		if (!(mixer->mask_recsrc & (1 << idx)))
 			continue;
@@ -785,10 +784,8 @@
 		}
 	}
 	err = 0;
-	goto __unalloc;
       __unlock:
-      	read_unlock(&card->control_rwlock);
-      __unalloc:
+     	up_read(&card->controls_rwsem);
       	if (uctl)
       		kfree(uctl);
       	if (uinfo)
@@ -813,7 +810,7 @@
 		err = -ENOMEM;
 		goto __unlock;
 	}
-	read_lock(&card->control_rwlock);
+	down_read(&card->controls_rwsem);
 	kctl = snd_mixer_oss_test_id(mixer, "Capture Source", 0);
 	snd_runtime_check(kctl != NULL, err = -ENOENT; goto __unlock);
 	snd_runtime_check(!(err = kctl->info(kctl, uinfo)), goto __unlock);
@@ -838,7 +835,7 @@
 		snd_ctl_notify(fmixer->card, SNDRV_CTL_EVENT_MASK_VALUE, &kctl->id);
 	err = 0;
       __unlock:
-	read_unlock(&card->control_rwlock);
+	up_read(&card->controls_rwsem);
 	if (uctl)
 		kfree(uctl);
 	if (uinfo)

Re: 2.5.50: sleeping function called from illegal context

[Duncan Sands]

> yes, basically what snd_ctl_notify() does is the same.
> it queues an event and wakes up the sleepers.
> thus, it's ok to separate the stuff from the semaphore.
>
> the attached is a patch to rewrite the locks with rwsem.
> please check whether it works for you.

This is against alsa cvs and not 2.5.50, right?  That means
it will take me a little longer to test it (but I will test it).

Thanks a lot,

Duncan.


-------------------------------------------------------
This SF.net email is sponsored by: Get the new Palm Tungsten T 
handheld. Power & Color in a compact size! 
http://ads.sourceforge.net/cgi-bin/redirect.pl?palm0002en

Re: 2.5.50: sleeping function called from illegal context

[Duncan Sands]

> > I guess another way of dealing with this kind of problem is to use a
> > semaphore rather than a spinlock, and a workqueue: when the interrupt
> > comes in, the call to snd_ctl_notify is put on the queue, where it will
> > later be run in process context, and can safely take the semaphore.
>
> yes, basically what snd_ctl_notify() does is the same.
> it queues an event and wakes up the sleepers.
> thus, it's ok to separate the stuff from the semaphore.
>
> the attached is a patch to rewrite the locks with rwsem.
> please check whether it works for you.

It seems to work fine.

Thanks a lot,

Duncan.


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Re: 2.5.50: sleeping function called from illegal context

[Paul Davis]

>I guess another way of dealing with this kind of problem is to use a
>semaphore rather than a spinlock, and a workqueue: when the interrupt
>comes in, the call to snd_ctl_notify is put on the queue, where it will later
>be run in process context, and can safely take the semaphore.

can i get a pointer to some code that does this? we need to do this
for handling MIDI I/O in the hammerfall dsp driver. doing it from
the interrupt handler causes audio dropouts because the midi i/o is so
much slower.

--p


-------------------------------------------------------
This SF.net email is sponsored by: Microsoft Visual Studio.NET 
comprehensive development tool, built to increase your 
productivity. Try a free online hosted session at:
http://ads.sourceforge.net/cgi-bin/redirect.pl?micr0003en

Re: 2.5.50: sleeping function called from illegal context

[Duncan Sands]

On Tuesday 03 December 2002 19:34, Paul Davis wrote:
> >I guess another way of dealing with this kind of problem is to use a
> >semaphore rather than a spinlock, and a workqueue: when the interrupt
> >comes in, the call to snd_ctl_notify is put on the queue, where it will
> > later be run in process context, and can safely take the semaphore.
>
> can i get a pointer to some code that does this? we need to do this
> for handling MIDI I/O in the hammerfall dsp driver. doing it from
> the interrupt handler causes audio dropouts because the midi i/o is so
> much slower.

For the use of workqueues (introduced in 2.5), see
http://lwn.net/Articles/11360/
and
http://lwn.net/Articles/10963/

For the use of task queues (now gone) and tasklets,
see for example "Linux device drivers" by Rubini and
Corbet.

If you need to go to sleep, you need to use a workqueue.
If you are happy running in interrupt context, a tasklet may
be enough.

I hope this helps,

Duncan.


-------------------------------------------------------
This SF.net email is sponsored by: Microsoft Visual Studio.NET 
comprehensive development tool, built to increase your 
productivity. Try a free online hosted session at:
http://ads.sourceforge.net/cgi-bin/redirect.pl?micr0003en

Re: 2.5.50: sleeping function called from illegal context

[Takashi Iwai]

At Tue, 3 Dec 2002 13:21:41 +0100,
Duncan Sands wrote:
> 
> On Tuesday 03 December 2002 19:34, Paul Davis wrote:
> > >I guess another way of dealing with this kind of problem is to use a
> > >semaphore rather than a spinlock, and a workqueue: when the interrupt
> > >comes in, the call to snd_ctl_notify is put on the queue, where it will
> > > later be run in process context, and can safely take the semaphore.
> >
> > can i get a pointer to some code that does this? we need to do this
> > for handling MIDI I/O in the hammerfall dsp driver. doing it from
> > the interrupt handler causes audio dropouts because the midi i/o is so
> > much slower.
> 
> For the use of workqueues (introduced in 2.5), see
> http://lwn.net/Articles/11360/
> and
> http://lwn.net/Articles/10963/

thanks for pointers!

i believe the tasklet would be enough for a task like the midi
handler, too.


regarding to the workqueue, it can be used for a new stuff to shutdown
the module asynchronously for hotplug, which Jaroslav added recently.
the current implementation is invoking a kernel thread directly.
with the workqueue, the code would be much simpler (although there is
another issue for compatibility with older kernels :)


Takashi


-------------------------------------------------------
This SF.net email is sponsored by: Microsoft Visual Studio.NET 
comprehensive development tool, built to increase your 
productivity. Try a free online hosted session at:
http://ads.sourceforge.net/cgi-bin/redirect.pl?micr0003en