[patch 1/2] echoaudio semaphore to mutex

[patch 1/2] echoaudio semaphore to mutex

[Kevin Winchester]

[-- Attachment #1: echoaudio-sem-to-mutex.patch --]
[-- Type: text/plain, Size: 3533 bytes --]

Convert the semaphore to a mutex in echoaudio.c

Signed-off-by: Kevin Winchester <kjwinchester@gmail.com>

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

Index: v2.6.24-rc4/sound/pci/echoaudio/echoaudio.c
===================================================================
--- v2.6.24-rc4.orig/sound/pci/echoaudio/echoaudio.c
+++ 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 ! */
Index: v2.6.24-rc4/sound/pci/echoaudio/echoaudio.h
===================================================================
--- v2.6.24-rc4.orig/sound/pci/echoaudio/echoaudio.h
+++ 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;

-- 

Re: [patch 1/2] echoaudio semaphore to mutex

[Giuliano Pochini]

On Sun, 9 Dec 2007, Kevin Winchester wrote:

> Convert the semaphore to a mutex in echoaudio.c
>
> Signed-off-by: Kevin Winchester <kjwinchester@gmail.com>

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

Acked-by: Giuliano Pochini <pochini@shiny.it>


--
Giuliano.

Re: [patch 1/2] echoaudio semaphore to mutex

[Takashi Iwai]

At Sun, 09 Dec 2007 13:15:10 -0400,
Kevin Winchester wrote:
> 
> Convert the semaphore to a mutex in echoaudio.c
> 
> Signed-off-by: Kevin Winchester <kjwinchester@gmail.com>

It was already fixed on ALSA tree :)

Thanks anyway.


Takashi

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

[Kevin Winchester]

[-- Attachment #1: 9p-util-sem-to-mutex.patch --]
[-- Type: text/plain, Size: 1537 bytes --]

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

Signed-off-by: Kevin Winchester <kjwinchester@gmail.com>

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

Index: v2.6.24-rc4/net/9p/util.c
===================================================================
--- v2.6.24-rc4.orig/net/9p/util.c
+++ 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);
 

-- 

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

[Andrew Morton]

On Sun, 09 Dec 2007 13:15:11 -0400 Kevin Winchester <kjwinchester@gmail.com> wrote:

> Convert the semaphore to a mutex in net/9p/util.c
> 
> Signed-off-by: Kevin Winchester <kjwinchester@gmail.com>
> 
> ---
>  net/9p/util.c |   12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> Index: v2.6.24-rc4/net/9p/util.c
> ===================================================================
> --- v2.6.24-rc4.orig/net/9p/util.c
> +++ 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);

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 <akpm@linux-foundation.org>

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

Cc: Kevin Winchester <kjwinchester@gmail.com>
Cc: Eric Van Hensbergen <ericvh@gmail.com>
Cc: Ron Minnich <rminnich@sandia.gov>
Cc: Latchesar Ionkov <lucho@ionkov.net>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

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

diff -puN net/9p/util.c~9p-util-fix-semaphore-handling net/9p/util.c
--- a/net/9p/util.c~9p-util-fix-semaphore-handling
+++ a/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);
 }
_