echoaudio semaphore to mutex

PATCH In review Kevin Winchester 4 participants 5 messages Dec 9, 2007 – Dec 17, 2007 1 message not in the archive

Subsystems (2)

Sound 9P file system

lore

Share & tools

lore

Kevin Winchester

author

2007-12-09 17:16 UTC #1 lore raw

PATCH 1/2 echoaudio semaphore to mutex

In reply to a message not in the archive

Convert the semaphore to a mutex in echoaudio.c

Signed-off-by Kevin Winchester <[email protected]>

---
 sound/pci/echoaudio/echoaudio.c |   18 +++++++++---------
 sound/pci/echoaudio/echoaudio.h |    2 +-
 2 files changed, 10 insertions(+), 10 deletions(-)

sound/pci/echoaudio/echoaudio.c +9 -9

Index: v2.6.24-rc4/sound/pci/echoaudio/echoaudio.c
===================================================================
@@ -378,7 +378,7 @@ static int pcm_digital_in_open(struct sn
 
 	DE_ACT(("pcm_digital_in_open\n"));
 	max_channels = num_digital_busses_in(chip) - substream->number;
-	down(&chip->mode_mutex);
+	mutex_lock(&chip->mode_mutex);
 	if (chip->digital_mode == DIGITAL_MODE_ADAT)
 		err = pcm_open(substream, max_channels);
 	else	/* If the card has ADAT, subtract the 6 channels

@@ -405,7 +405,7 @@ static int pcm_digital_in_open(struct sn
 		chip->can_set_rate=0;
 
 din_exit:
-	up(&chip->mode_mutex);
+	mutex_unlock(&chip->mode_mutex);
 	return err;
 }

@@ -420,7 +420,7 @@ static int pcm_digital_out_open(struct s
 
 	DE_ACT(("pcm_digital_out_open\n"));
 	max_channels = num_digital_busses_out(chip) - substream->number;
-	down(&chip->mode_mutex);
+	mutex_lock(&chip->mode_mutex);
 	if (chip->digital_mode == DIGITAL_MODE_ADAT)
 		err = pcm_open(substream, max_channels);
 	else	/* If the card has ADAT, subtract the 6 channels

@@ -447,7 +447,7 @@ static int pcm_digital_out_open(struct s
 	if (atomic_read(&chip->opencount) > 1 && chip->rate_set)
 		chip->can_set_rate=0;
 dout_exit:
-	up(&chip->mode_mutex);
+	mutex_unlock(&chip->mode_mutex);
 	return err;
 }

@@ -1420,7 +1420,7 @@ static int snd_echo_digital_mode_put(str
 	if (dmode != chip->digital_mode) {
 		/* mode_mutex is required to make this operation atomic wrt
 		pcm_digital_*_open() and set_input_clock() functions. */
-		down(&chip->mode_mutex);
+		mutex_lock(&chip->mode_mutex);
 
 		/* Do not allow the user to change the digital mode when a pcm
 		device is open because it also changes the number of channels

@@ -1439,7 +1439,7 @@ static int snd_echo_digital_mode_put(str
 			if (changed >= 0)
 				changed = 1;	/* No errors */
 		}
-		up(&chip->mode_mutex);
+		mutex_unlock(&chip->mode_mutex);
 	}
 	return changed;
 }

@@ -1566,12 +1566,12 @@ static int snd_echo_clock_source_put(str
 		return -EINVAL;
 	dclock = chip->clock_source_list[eclock];
 	if (chip->input_clock != dclock) {
-		down(&chip->mode_mutex);
+		mutex_lock(&chip->mode_mutex);
 		spin_lock_irq(&chip->lock);
 		if ((changed = set_input_clock(chip, dclock)) == 0)
 			changed = 1;	/* no errors */
 		spin_unlock_irq(&chip->lock);
-		up(&chip->mode_mutex);
+		mutex_unlock(&chip->mode_mutex);
 	}
 
 	if (changed < 0)

@@ -1972,7 +1972,7 @@ static __devinit int snd_echo_create(str
 		return err;
 	}
 	atomic_set(&chip->opencount, 0);
-	init_MUTEX(&chip->mode_mutex);
+	mutex_init(&chip->mode_mutex);
 	chip->can_set_rate = 1;
 	*rchip = chip;
 	/* Init done ! */

sound/pci/echoaudio/echoaudio.h +1 -1

Index: v2.6.24-rc4/sound/pci/echoaudio/echoaudio.h
===================================================================
@@ -361,7 +361,7 @@ struct echoaudio {
 	spinlock_t lock;
 	struct snd_pcm_substream *substream[DSP_MAXPIPES];
 	int last_period[DSP_MAXPIPES];
-	struct semaphore mode_mutex;
+	struct mutex mode_mutex;
 	u16 num_digital_modes, digital_mode_list[6];
 	u16 num_clock_sources, clock_source_list[10];
 	atomic_t opencount;

Kevin Winchester

author

2007-12-09 17:16 UTC #4 lore raw

PATCH 2/2 9p util semaphore to mutex

In reply to a message not in the archive

Convert the semaphore to a mutex in net/9p/util.c

Signed-off-by Kevin Winchester <[email protected]>

---
 net/9p/util.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

net/9p/util.c +6 -6

Index: v2.6.24-rc4/net/9p/util.c
===================================================================
@@ -33,7 +33,7 @@
 #include <net/9p/9p.h>
 
 struct p9_idpool {
-	struct semaphore lock;
+	struct mutex lock;
 	struct idr pool;
 };

@@ -45,7 +45,7 @@ struct p9_idpool *p9_idpool_create(void)
 	if (!p)
 		return ERR_PTR(-ENOMEM);
 
-	init_MUTEX(&p->lock);
+	mutex_init(&p->lock);
 	idr_init(&p->pool);
 
 	return p;

@@ -76,14 +76,14 @@ retry:
 	if (idr_pre_get(&p->pool, GFP_KERNEL) == 0)
 		return 0;
 
-	if (down_interruptible(&p->lock) == -EINTR) {
+	if (mutex_lock_interruptible(&p->lock) == -EINTR) {
 		P9_EPRINTK(KERN_WARNING, "Interrupted while locking\n");
 		return -1;
 	}
 
 	/* no need to store exactly p, we just need something non-null */
 	error = idr_get_new(&p->pool, p, &i);
-	up(&p->lock);
+	mutex_unlock(&p->lock);
 
 	if (error == -EAGAIN)
 		goto retry;

@@ -104,12 +104,12 @@ EXPORT_SYMBOL(p9_idpool_get);
 
 void p9_idpool_put(int id, struct p9_idpool *p)
 {
-	if (down_interruptible(&p->lock) == -EINTR) {
+	if (mutex_lock_interruptible(&p->lock) == -EINTR) {
 		P9_EPRINTK(KERN_WARNING, "Interrupted while locking\n");
 		return;
 	}
 	idr_remove(&p->pool, id);
-	up(&p->lock);
+	mutex_unlock(&p->lock);
 }
 EXPORT_SYMBOL(p9_idpool_put);

Giuliano Pochini

Acked-by on v1 1/2

2007-12-10 09:27 UTC #2 lore raw

In reply to Kevin Winchester

Quoted 3 lines · Kevin Winchester

Convert the semaphore to a mutex in echoaudio.c
Signed-off-by Kevin Winchester <[email protected]>

It's an 1:1 change. It looks fine to me.

Acked-by Giuliano Pochini <[email protected]>

--
Giuliano.

Andrew Morton

2007-12-12 02:07 UTC #5 lore raw

Re: [patch 2/2] 9p util semaphore to mutex

In reply to Kevin Winchester

Quoted 51 lines · On Sun, 09 Dec 2007 13:15:11 -0400 Kevin Winchester <[email protected]>…

Convert the semaphore to a mutex in net/9p/util.c

Signed-off-by Kevin Winchester <[email protected]>

---
 net/9p/util.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Index: v2.6.24-rc4/net/9p/util.c
===================================================================

net/9p/util.c +4 -4

@@ -33,7 +33,7 @@
 #include <net/9p/9p.h>
 
 struct p9_idpool {
-	struct semaphore lock;
+	struct mutex lock;
 	struct idr pool;
 };

@@ -45,7 +45,7 @@ struct p9_idpool *p9_idpool_create(void)
 	if (!p)
 		return ERR_PTR(-ENOMEM);
 
-	init_MUTEX(&p->lock);
+	mutex_init(&p->lock);
 	idr_init(&p->pool);
 
 	return p;

@@ -76,14 +76,14 @@ retry:
 	if (idr_pre_get(&p->pool, GFP_KERNEL) == 0)
 		return 0;
 
-	if (down_interruptible(&p->lock) == -EINTR) {
+	if (mutex_lock_interruptible(&p->lock) == -EINTR) {
 		P9_EPRINTK(KERN_WARNING, "Interrupted while locking\n");
 		return -1;
 	}
 
 	/* no need to store exactly p, we just need something non-null */
 	error = idr_get_new(&p->pool, p, &i);
-	up(&p->lock);
+	mutex_unlock(&p->lock);
 
 	if (error == -EAGAIN)
 		goto retry;

@@ -104,12 +104,12 @@ EXPORT_SYMBOL(p9_idpool_get);
 
 void p9_idpool_put(int id, struct p9_idpool *p)
 {

net/9p/util.c EXPORT_SYMBOL(p9_idpool_get); · line 114

Quoted 10 lines

net/9p/util.c +2 -2

-	if (down_interruptible(&p->lock) == -EINTR) {
+	if (mutex_lock_interruptible(&p->lock) == -EINTR) {
 		P9_EPRINTK(KERN_WARNING, "Interrupted while locking\n");
 		return;
 	}
 	idr_remove(&p->pool, id);
-	up(&p->lock);
+	mutex_unlock(&p->lock);
 }
 EXPORT_SYMBOL(p9_idpool_put);

I cannot see how the code which you are modifying could have been correct.

If the lock is contended and this task has signal_pending() then boom - we return from p9_idpool_put() without having removed the item from the IDR tree and then we just lose all track of this fact. It is at a minimum a memory leak.

I'm getting the feeling that we need to go and take a general look at the down_interuptible() and mutex_lock_interruptible() callers in the nether regions of the kernel...

Meanwhile I'd propose this instead:

From: Andrew Morton <[email protected]>

Don't use down_interruptible() in situations where we cannot handle the consequences.

Cc Kevin Winchester <[email protected]>

Cc Eric Van Hensbergen <[email protected]>

Cc Ron Minnich <[email protected]>

Cc Latchesar Ionkov <[email protected]>

Signed-off-by Andrew Morton <[email protected]>

---

 net/9p/util.c |   11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

net/9p/util.c renamed from net/9p/util.c~9p-util-fix-semaphore-handling +2 -9

diff -puN net/9p/util.c~9p-util-fix-semaphore-handling net/9p/util.c
@@ -76,12 +76,8 @@ retry:
 	if (idr_pre_get(&p->pool, GFP_KERNEL) == 0)
 		return 0;
 
-	if (down_interruptible(&p->lock) == -EINTR) {
-		P9_EPRINTK(KERN_WARNING, "Interrupted while locking\n");
-		return -1;
-	}
-
 	/* no need to store exactly p, we just need something non-null */
+	down(&p->lock);
 	error = idr_get_new(&p->pool, p, &i);
 	up(&p->lock);

@@ -104,10 +100,7 @@ EXPORT_SYMBOL(p9_idpool_get);
 
 void p9_idpool_put(int id, struct p9_idpool *p)
 {
-	if (down_interruptible(&p->lock) == -EINTR) {
-		P9_EPRINTK(KERN_WARNING, "Interrupted while locking\n");
-		return;
-	}
+	down(&p->lock);
 	idr_remove(&p->pool, id);
 	up(&p->lock);
 }

Takashi Iwai

2007-12-17 11:51 UTC #3 lore raw

In reply to Kevin Winchester

At Sun, 09 Dec 2007 13:15:10 -0400,

Quoted 4 lines · Kevin Winchester

Convert the semaphore to a mutex in echoaudio.c
Signed-off-by Kevin Winchester <[email protected]>

It was already fixed on ALSA tree :)

Thanks anyway.

Takashi