Skip to contentSkip to conversation

use mutex instead of semaphore in several drivers

PATCH In review Matthias Kaehlcke 8 participants 24 messages Jul 29, 2007 – Jul 31, 2007

Subsystems (4)
Media (V4L/DVB) SCSI Networking drivers (wireless) SCSI tape driver
lore
Share & tools
lore

Matthias Kaehlcke

author
#0 lore raw

cover use mutex instead of semaphore in several drivers

This patchset converts semaphores that are used as mutexes to the mutex API in the following drivers/code:

Host AP driver
OnStream SCSI Tape driver
SCSI Tape driver
ISDN subsystem common functions
DVB frontend tuning interface

Signature · 12 lines
Matthias Kaehlcke
Linux Application Developer
Barcelona

        Representation of the world, like the world itself, is
        the work of men; they describe it from their own point
         of view, which they  confuse with the absolute truth
                          (Simone de Beauvoir)
                                                                 .''`.
    using free software / Debian GNU/Linux | http://debian.org  : :'  :
                                                                `. `'`
gpg --keyserver pgp.mit.edu --recv-keys 47D8E5D4                  `-
author
#1 lore raw

PATCH 1/5 Use mutex instead of semaphore in the Host AP driver

The Host AP driver uses a semaphore as mutex. Use the mutex API instead of the (binary) semaphore.

Signed-off-by Matthias Kaehlcke <[email protected]>

--

- res = down_interruptible(&local->rid_bap_sem);
+ res = mutex_lock_interruptible(&local->rid_bap_mtx);
if (res)
return res;

@@ -834,7 +834,7 @@ static int hfa384x_get_rid(struct net_device *dev, u16 rid, void *buf, int len,
printk(KERN_DEBUG "%s: hfa384x_get_rid: CMDCODE_ACCESS failed "
"(res=%d, rid=%04x, len=%d)\n",
dev->name, res, rid, len);
- up(&local->rid_bap_sem);
+ mutex_unlock(&local->rid_bap_mtx);
return res;
}

@@ -861,7 +861,7 @@ static int hfa384x_get_rid(struct net_device *dev, u16 rid, void *buf, int len,
res = hfa384x_from_bap(dev, BAP0, buf, len);

spin_unlock_bh(&local->baplock);
- up(&local->rid_bap_sem);
+ mutex_unlock(&local->rid_bap_mtx);

if (res) {
if (res != -ENODATA)
@@ -902,7 +902,7 @@ static int hfa384x_set_rid(struct net_device *dev, u16 rid, void *buf, int len)
/* RID len in words and +1 for rec.rid */
rec.len = cpu_to_le16(len / 2 + len % 2 + 1);

- res = down_interruptible(&local->rid_bap_sem);
+ res = mutex_lock_interruptible(&local->rid_bap_mtx);
if (res)
return res;

@@ -917,12 +917,12 @@ static int hfa384x_set_rid(struct net_device *dev, u16 rid, void *buf, int len)
if (res) {
printk(KERN_DEBUG "%s: hfa384x_set_rid (rid=%04x, len=%d) - "
"failed - res=%d\n", dev->name, rid, len, res);
- up(&local->rid_bap_sem);
+ mutex_unlock(&local->rid_bap_mtx);
return res;
}

res = hfa384x_cmd(dev, HFA384X_CMDCODE_ACCESS_WRITE, rid, NULL, NULL);
- up(&local->rid_bap_sem);
+ mutex_unlock(&local->rid_bap_mtx);

if (res) {
printk(KERN_DEBUG "%s: hfa384x_set_rid: CMDCODE_ACCESS_WRITE "
@@ -3171,7 +3171,7 @@ prism2_init_local_data(struct prism2_helper_functions *funcs, int card_idx,
spin_lock_init(&local->cmdlock);
spin_lock_init(&local->baplock);
spin_lock_init(&local->lock);
- init_MUTEX(&local->rid_bap_sem);
+ mutex_init(&local->rid_bap_mtx);

if (card_idx < 0 || card_idx >= MAX_PARM_DEVICES)
card_idx = 0;

drivers/net/wireless/hostap/hostap_wlan.h +2 -1
index 87a54aa..a42325c 100644
@@ -3,6 +3,7 @@
 
 #include <linux/wireless.h>
 #include <linux/netdevice.h>
+#include <linux/mutex.h>
 #include <net/iw_handler.h>
 
 #include "hostap_config.h"

@@ -641,7 +642,7 @@ struct local_info {
 			      * when removing entries from the list.
 			      * TX and RX paths can use read lock. */
 	spinlock_t cmdlock, baplock, lock;
-	struct semaphore rid_bap_sem;
+	struct mutex rid_bap_mtx;
 	u16 infofid; /* MAC buffer id for info frame */
 	/* txfid, intransmitfid, next_txtid, and next_alloc are protected by
 	 * txfidlock */
Signature · 10 lines
Matthias Kaehlcke
Linux Application Developer
Barcelona

  Si deseas mantener tu libertad, debes estar preparado para defenderla
                          (Richard Stallman)
                                                                 .''`.
    using free software / Debian GNU/Linux | http://debian.org  : :'  :
                                                                `. `'`
gpg --keyserver pgp.mit.edu --recv-keys 47D8E5D4                  `-
#2 lore raw

Re: [PATCH 1/5] Use mutex instead of semaphore in the Host AP driver

Quoted 13 lines · Matthias Kaehlcke

The Host AP driver uses a semaphore as mutex. Use the mutex API instead of the (binary) semaphore.

Signed-off-by Matthias Kaehlcke <[email protected]>

--

(quoted diff) +1 -1
-	res = down_interruptible(&local->rid_bap_sem);
+	res = mutex_lock_interruptible(&local->rid_bap_mtx);
 	if (res)
 		return res;

Review static int hfa384x_set_rid(struct net_device *dev, u16 rid, void *buf, int len) · line 908

Quoted 8 lines
(quoted diff) +1 -1
@@ -902,7 +902,7 @@ static int hfa384x_set_rid(struct net_device *dev, u16 rid, void *buf, int len)
 	/* RID len in words and +1 for rec.rid */
 	rec.len = cpu_to_le16(len / 2 + len % 2 + 1);
 
-	res = down_interruptible(&local->rid_bap_sem);
+	res = mutex_lock_interruptible(&local->rid_bap_mtx);
 	if (res)
 		return res;

Is res returned to userspace? If yes, that's not right. On a interrupted mutex allocation you should return -ERESTARTSYS to userspace.

#3 lore raw

Re: [PATCH 1/5] Use mutex instead of semaphore in the Host AP driver

Quoted 5 lines · Michael Buesch
Quoted 4 lines · Matthias Kaehlcke

The Host AP driver uses a semaphore as mutex. Use the mutex API instead of the (binary) semaphore.

Signed-off-by Matthias Kaehlcke <[email protected]>

[ Something seems to have gone wrong with your diff / patch / script.
There was no diff header here, which should have been. ]

Quoted 9 lines
Quoted 9 lines
(quoted diff) +1 -1
-	res = down_interruptible(&local->rid_bap_sem);
+	res = mutex_lock_interruptible(&local->rid_bap_mtx);
 	if (res)
 		return res;

@@ -902,7 +902,7 @@ static int hfa384x_set_rid(struct net_device *dev, u16 rid, void *buf, int len)
 	/* RID len in words and +1 for rec.rid */
 	rec.len = cpu_to_le16(len / 2 + len % 2 + 1);
Quoted 7 lines

Review static int hfa384x_set_rid(struct net_device *dev, u16 rid, void *buf, int len) · line 908

Quoted 5 lines
(quoted diff) +1 -1
-	res = down_interruptible(&local->rid_bap_sem);
+	res = mutex_lock_interruptible(&local->rid_bap_mtx);
 	if (res)
 		return res;

Is res returned to userspace? If yes, that's not right.

Yup, that's not right.

Quoted 2 lines

On a interrupted mutex allocation you should return -ERESTARTSYS to userspace.

Nope, userspace must not see ERESTARTSYS (I /think/ this is the third time I'm participating in this exact same discussion :-)

If the return would be caught by a previous in-kernel caller in the call chain, ERESTARTSYS is okay and it could try to restart the operation. However, if the return goes unfiltered directly to userspace, EINTR is the correct choice.

Satyam

#4 lore raw

Re: [PATCH 1/5] Use mutex instead of semaphore in the Host AP driver

Whoops ...

Quoted 33 lines · Satyam Sharma
Quoted 5 lines · Michael Buesch
Quoted 4 lines · Matthias Kaehlcke

The Host AP driver uses a semaphore as mutex. Use the mutex API instead of the (binary) semaphore.

Signed-off-by Matthias Kaehlcke <[email protected]>

[ Something seems to have gone wrong with your diff / patch / script.
There was no diff header here, which should have been. ]

Quoted 9 lines
Quoted 9 lines
(quoted diff) +1 -1
-	res = down_interruptible(&local->rid_bap_sem);
+	res = mutex_lock_interruptible(&local->rid_bap_mtx);
 	if (res)
 		return res;

@@ -902,7 +902,7 @@ static int hfa384x_set_rid(struct net_device *dev, u16 rid, void *buf, int len)
 	/* RID len in words and +1 for rec.rid */
 	rec.len = cpu_to_le16(len / 2 + len % 2 + 1);
Quoted 7 lines

Review static int hfa384x_set_rid(struct net_device *dev, u16 rid, void *buf, int len) · line 908

Quoted 5 lines
(quoted diff) +1 -1
-	res = down_interruptible(&local->rid_bap_sem);
+	res = mutex_lock_interruptible(&local->rid_bap_mtx);
 	if (res)
 		return res;

Is res returned to userspace? If yes, that's not right.

Yup, that's not right.

Quoted 2 lines

On a interrupted mutex allocation you should return -ERESTARTSYS to userspace.

Quoted 7 lines

Nope, userspace must not see ERESTARTSYS (I /think/ this is the third time I'm participating in this exact same discussion :-)

If the return would be caught by a previous in-kernel caller in the call chain, ERESTARTSYS is okay and it could try to restart the operation. However, if the return goes unfiltered directly to userspace, EINTR is the correct choice.

Eek, and because mutex_lock_interruptible() *does* return -EINTR when interrupted by a signal, and I noticed that hfa384x_set_rid() could be called from an ioctl(2) codepath with no intermediate caller trying to restart it, so the code is perfectly alright, actually.

But the patch doesn't have the diff header anyway, so Matthias would probably have to resend in any case :-)

Satyam

author
#5 lore raw

Re: [PATCH 1/5] Use mutex instead of semaphore in the Host AP driver

El Mon, Jul 30, 2007 at 09:17:25AM +0530 Satyam Sharma ha dit:

Quoted 46 lines

Whoops ...

Quoted 33 lines · Satyam Sharma
Quoted 5 lines · Michael Buesch
Quoted 4 lines · Matthias Kaehlcke

The Host AP driver uses a semaphore as mutex. Use the mutex API instead of the (binary) semaphore.

Signed-off-by Matthias Kaehlcke <[email protected]>

[ Something seems to have gone wrong with your diff / patch / script.
There was no diff header here, which should have been. ]

Quoted 9 lines
Quoted 9 lines
(quoted diff) +1 -1
-	res = down_interruptible(&local->rid_bap_sem);
+	res = mutex_lock_interruptible(&local->rid_bap_mtx);
 	if (res)
 		return res;

@@ -902,7 +902,7 @@ static int hfa384x_set_rid(struct net_device *dev, u16 rid, void *buf, int len)
 	/* RID len in words and +1 for rec.rid */
 	rec.len = cpu_to_le16(len / 2 + len % 2 + 1);
Quoted 7 lines

Review static int hfa384x_set_rid(struct net_device *dev, u16 rid, void *buf, int len) · line 908

Quoted 5 lines
(quoted diff) +1 -1
-	res = down_interruptible(&local->rid_bap_sem);
+	res = mutex_lock_interruptible(&local->rid_bap_mtx);
 	if (res)
 		return res;

Is res returned to userspace? If yes, that's not right.

Yup, that's not right.

Quoted 2 lines

On a interrupted mutex allocation you should return -ERESTARTSYS to userspace.

Quoted 7 lines

Nope, userspace must not see ERESTARTSYS (I /think/ this is the third time I'm participating in this exact same discussion :-)

If the return would be caught by a previous in-kernel caller in the call chain, ERESTARTSYS is okay and it could try to restart the operation. However, if the return goes unfiltered directly to userspace, EINTR is the correct choice.

Quoted 7 lines

Eek, and because mutex_lock_interruptible() *does* return -EINTR when interrupted by a signal, and I noticed that hfa384x_set_rid() could be called from an ioctl(2) codepath with no intermediate caller trying to restart it, so the code is perfectly alright, actually.

But the patch doesn't have the diff header anyway, so Matthias would probably have to resend in any case :-)

thanks for reviewing the patch and for your comments, here is the patch with the diff header (no idea what when wrong with the other one)

regards

matthias

--

The Host AP driver uses a semaphore as mutex. Use the mutex API instead of the (binary) semaphore.

Signed-off-by Matthias Kaehlcke <[email protected]>

--

drivers/net/wireless/hostap/hostap_hw.c +7 -7
index 959887b..d3dacbc 100644
@@ -825,7 +825,7 @@ static int hfa384x_get_rid(struct net_device *dev, u16 rid, void *buf, int len,
 	    local->hw_downloading)
 		return -ENODEV;
 
-	res = down_interruptible(&local->rid_bap_sem);
+	res = mutex_lock_interruptible(&local->rid_bap_mtx);
 	if (res)
 		return res;
 

@@ -834,7 +834,7 @@ static int hfa384x_get_rid(struct net_device *dev, u16 rid, void *buf, int len,
 		printk(KERN_DEBUG "%s: hfa384x_get_rid: CMDCODE_ACCESS failed "
 		       "(res=%d, rid=%04x, len=%d)\n",
 		       dev->name, res, rid, len);
-		up(&local->rid_bap_sem);
+		mutex_unlock(&local->rid_bap_mtx);
 		return res;
 	}
 

@@ -861,7 +861,7 @@ static int hfa384x_get_rid(struct net_device *dev, u16 rid, void *buf, int len,
 		res = hfa384x_from_bap(dev, BAP0, buf, len);
 
 	spin_unlock_bh(&local->baplock);
-	up(&local->rid_bap_sem);
+	mutex_unlock(&local->rid_bap_mtx);
 
 	if (res) {
 		if (res != -ENODATA)

@@ -902,7 +902,7 @@ static int hfa384x_set_rid(struct net_device *dev, u16 rid, void *buf, int len)
 	/* RID len in words and +1 for rec.rid */
 	rec.len = cpu_to_le16(len / 2 + len % 2 + 1);
 
-	res = down_interruptible(&local->rid_bap_sem);
+	res = mutex_lock_interruptible(&local->rid_bap_mtx);
 	if (res)
 		return res;
 

@@ -917,12 +917,12 @@ static int hfa384x_set_rid(struct net_device *dev, u16 rid, void *buf, int len)
 	if (res) {
 		printk(KERN_DEBUG "%s: hfa384x_set_rid (rid=%04x, len=%d) - "
 		       "failed - res=%d\n", dev->name, rid, len, res);
-		up(&local->rid_bap_sem);
+		mutex_unlock(&local->rid_bap_mtx);
 		return res;
 	}
 
 	res = hfa384x_cmd(dev, HFA384X_CMDCODE_ACCESS_WRITE, rid, NULL, NULL);
-	up(&local->rid_bap_sem);
+	mutex_unlock(&local->rid_bap_mtx);
 
 	if (res) {
 		printk(KERN_DEBUG "%s: hfa384x_set_rid: CMDCODE_ACCESS_WRITE "

@@ -3171,7 +3171,7 @@ prism2_init_local_data(struct prism2_helper_functions *funcs, int card_idx,
 	spin_lock_init(&local->cmdlock);
 	spin_lock_init(&local->baplock);
 	spin_lock_init(&local->lock);
-	init_MUTEX(&local->rid_bap_sem);
+	mutex_init(&local->rid_bap_mtx);
 
 	if (card_idx < 0 || card_idx >= MAX_PARM_DEVICES)
 		card_idx = 0;
drivers/net/wireless/hostap/hostap_wlan.h +2 -1
index 87a54aa..a42325c 100644
@@ -3,6 +3,7 @@
 
 #include <linux/wireless.h>
 #include <linux/netdevice.h>
+#include <linux/mutex.h>
 #include <net/iw_handler.h>
 
 #include "hostap_config.h"

@@ -641,7 +642,7 @@ struct local_info {
 			      * when removing entries from the list.
 			      * TX and RX paths can use read lock. */
 	spinlock_t cmdlock, baplock, lock;
-	struct semaphore rid_bap_sem;
+	struct mutex rid_bap_mtx;
 	u16 infofid; /* MAC buffer id for info frame */
 	/* txfid, intransmitfid, next_txtid, and next_alloc are protected by
 	 * txfidlock */
Signature · 10 lines
Matthias Kaehlcke
Linux Application Developer
Barcelona

             You must have a plan. If you don't have a plan,
               you'll become part of somebody else's plan
                                                                 .''`.
    using free software / Debian GNU/Linux | http://debian.org  : :'  :
                                                                `. `'`
gpg --keyserver pgp.mit.edu --recv-keys 47D8E5D4                  `-
#7 lore raw

Re: [PATCH 1/5] Use mutex instead of semaphore in the Host AP driver

Quoted 4 lines · Satyam Sharma

On Mon, 30 Jul 2007, Matthias Kaehlcke wrote:

Quoted 8 lines
Quoted 6 lines

[...]

The Host AP driver uses a semaphore as mutex. Use the mutex API instead of the (binary) semaphore.

Signed-off-by Matthias Kaehlcke <[email protected]>
Reviewed-by Satyam Sharma <[email protected]>

Is that the same as Acked-by? Or do you disagree with the patch?

Just checkin'...

John

Signature · 2 lines
John W. Linville
[email protected]
#8 lore raw

Re: [PATCH 1/5] Use mutex instead of semaphore in the Host AP driver

Quoted 7 lines · John W. Linville
Quoted 6 lines · Satyam Sharma
Quoted 4 lines · Matthias Kaehlcke

[...]
The Host AP driver uses a semaphore as mutex. Use the mutex API instead of the (binary) semaphore.

Quoted 7 lines
Quoted 3 lines
Quoted 1 line
Signed-off-by Matthias Kaehlcke <[email protected]>
Reviewed-by Satyam Sharma <[email protected]>

Is that the same as Acked-by? Or do you disagree with the patch?

Just checkin'...

Well, I'm not the maintainer, so didn't think correct to give an "Acked-by". "Reviewed-by" generally means I read the patch, and think it is correct. Probably could save time for others ...

Satyam

#9 lore raw

Re: [PATCH 1/5] Use mutex instead of semaphore in the Host AP driver

Quoted 15 lines · On Tue, 31 Jul 2007 01:44:37 +0530 (IST) Satyam Sharma wrote:
Quoted 11 lines · John W. Linville
Quoted 9 lines · Satyam Sharma
Quoted 5 lines · Matthias Kaehlcke

[...]
The Host AP driver uses a semaphore as mutex. Use the mutex API instead of the (binary) semaphore.

Signed-off-by Matthias Kaehlcke <[email protected]>
Reviewed-by Satyam Sharma <[email protected]>
Quoted 7 lines
Quoted 3 lines

Is that the same as Acked-by? Or do you disagree with the patch?

Just checkin'...

Well, I'm not the maintainer, so didn't think correct to give an "Acked-by". "Reviewed-by" generally means I read the patch, and think it is correct. Probably could save time for others ...

You should use Acked-by. See Documentation/SubmittingPatches. :)

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

#10 lore raw

Re: [PATCH 1/5] Use mutex instead of semaphore in the Host AP driver

In reply to Satyam Sharma
Quoted 34 lines · Satyam Sharma
Quoted 5 lines · Michael Buesch
Quoted 4 lines · Matthias Kaehlcke

The Host AP driver uses a semaphore as mutex. Use the mutex API instead of the (binary) semaphore.

Signed-off-by Matthias Kaehlcke <[email protected]>

[ Something seems to have gone wrong with your diff / patch / script.
There was no diff header here, which should have been. ]

Quoted 9 lines
Quoted 9 lines
(quoted diff) +1 -1
-	res = down_interruptible(&local->rid_bap_sem);
+	res = mutex_lock_interruptible(&local->rid_bap_mtx);
 	if (res)
 		return res;

@@ -902,7 +902,7 @@ static int hfa384x_set_rid(struct net_device *dev, u16 rid, void *buf, int len)
 	/* RID len in words and +1 for rec.rid */
 	rec.len = cpu_to_le16(len / 2 + len % 2 + 1);
Quoted 7 lines

Review static int hfa384x_set_rid(struct net_device *dev, u16 rid, void *buf, int len) · line 908

Quoted 5 lines
(quoted diff) +1 -1
-	res = down_interruptible(&local->rid_bap_sem);
+	res = mutex_lock_interruptible(&local->rid_bap_mtx);
 	if (res)
 		return res;

Is res returned to userspace? If yes, that's not right.

Yup, that's not right.

Quoted 2 lines

On a interrupted mutex allocation you should return -ERESTARTSYS to userspace.

Quoted 7 lines

Nope, userspace must not see ERESTARTSYS (I /think/ this is the third time I'm participating in this exact same discussion :-)

If the return would be caught by a previous in-kernel caller in the call chain, ERESTARTSYS is okay and it could try to restart the operation. However, if the return goes unfiltered directly to userspace, EINTR is the correct choice.

Last time I submitted a patch which returned EINTR to userspace, people came and said it was wrong. It was said that we must return ERESTARTSYS to the libc and the libc will convert that into either EINTR or automatically restart it immediately. (You can set that in the sigaction stuff).

So either the one who told me that last time was wrong, or you. :)

Andrew? I think you were involved in this discussion I mentioned. (It was about the HW-RNG chardev, but that doesn't matter here).

#11 lore raw

Re: [PATCH 1/5] Use mutex instead of semaphore in the Host AP driver

Quoted 49 lines · On Mon, 30 Jul 2007 19:09:38 +0200 Michael Buesch <[email protected]> wrote:
Quoted 34 lines · Satyam Sharma
Quoted 5 lines · Michael Buesch
Quoted 4 lines · Matthias Kaehlcke

The Host AP driver uses a semaphore as mutex. Use the mutex API instead of the (binary) semaphore.

Signed-off-by Matthias Kaehlcke <[email protected]>

[ Something seems to have gone wrong with your diff / patch / script.
There was no diff header here, which should have been. ]

Quoted 9 lines
Quoted 9 lines
(quoted diff) +1 -1
-	res = down_interruptible(&local->rid_bap_sem);
+	res = mutex_lock_interruptible(&local->rid_bap_mtx);
 	if (res)
 		return res;

@@ -902,7 +902,7 @@ static int hfa384x_set_rid(struct net_device *dev, u16 rid, void *buf, int len)
 	/* RID len in words and +1 for rec.rid */
 	rec.len = cpu_to_le16(len / 2 + len % 2 + 1);
Quoted 7 lines

Review static int hfa384x_set_rid(struct net_device *dev, u16 rid, void *buf, int len) · line 908

Quoted 5 lines
(quoted diff) +1 -1
-	res = down_interruptible(&local->rid_bap_sem);
+	res = mutex_lock_interruptible(&local->rid_bap_mtx);
 	if (res)
 		return res;

Is res returned to userspace? If yes, that's not right.

Yup, that's not right.

Quoted 2 lines

On a interrupted mutex allocation you should return -ERESTARTSYS to userspace.

Quoted 7 lines

Nope, userspace must not see ERESTARTSYS (I /think/ this is the third time I'm participating in this exact same discussion :-)

If the return would be caught by a previous in-kernel caller in the call chain, ERESTARTSYS is okay and it could try to restart the operation. However, if the return goes unfiltered directly to userspace, EINTR is the correct choice.

Last time I submitted a patch which returned EINTR to userspace, people came and said it was wrong. It was said that we must return ERESTARTSYS to the libc and the libc will convert that into either EINTR or automatically restart it immediately. (You can set that in the sigaction stuff).

Quoted 4 lines

So either the one who told me that last time was wrong, or you. :)

Andrew? I think you were involved in this discussion I mentioned. (It was about the HW-RNG chardev, but that doesn't matter here).

who, me? signals? Danger.

yes, I think ERESTARTSYS is correct. That gets converted to -EINTR by the arch's signal handling code (not by glibc) just before we return to userspace.

#12 lore raw

Re: [PATCH 1/5] Use mutex instead of semaphore in the Host AP driver

Quoted 42 lines · Andrew Morton
Quoted 39 lines · On Mon, 30 Jul 2007 19:09:38 +0200 Michael Buesch <[email protected]> wrote:
Quoted 22 lines · Satyam Sharma
Quoted 6 lines · Michael Buesch
Quoted 5 lines · Matthias Kaehlcke
(quoted diff) +0 -0
@@ -902,7 +902,7 @@ static int hfa384x_set_rid(struct net_device *dev, u16 rid, void *buf, int len)
 	/* RID len in words and +1 for rec.rid */
 	rec.len = cpu_to_le16(len / 2 + len % 2 + 1);
Quoted 7 lines

Review static int hfa384x_set_rid(struct net_device *dev, u16 rid, void *buf, int len) · line 908

Quoted 5 lines
(quoted diff) +1 -1
-	res = down_interruptible(&local->rid_bap_sem);
+	res = mutex_lock_interruptible(&local->rid_bap_mtx);
 	if (res)
 		return res;

Is res returned to userspace? If yes, that's not right.

Yup, that's not right.

Quoted 2 lines

On a interrupted mutex allocation you should return -ERESTARTSYS to userspace.

Quoted 7 lines

Nope, userspace must not see ERESTARTSYS (I /think/ this is the third time I'm participating in this exact same discussion :-)

If the return would be caught by a previous in-kernel caller in the call chain, ERESTARTSYS is okay and it could try to restart the operation. However, if the return goes unfiltered directly to userspace, EINTR is the correct choice.

Last time I submitted a patch which returned EINTR to userspace, people came and said it was wrong. It was said that we must return ERESTARTSYS to the libc and the libc will convert that into either EINTR or automatically restart it immediately. (You can set that in the sigaction stuff).

So either the one who told me that last time was wrong, or you. :)

Quoted 8 lines
Quoted 2 lines

Andrew? I think you were involved in this discussion I mentioned. (It was about the HW-RNG chardev, but that doesn't matter here).

who, me? signals? Danger.

yes, I think ERESTARTSYS is correct. That gets converted to -EINTR by the arch's signal handling code (not by glibc) just before we return to userspace.

Hmm? But the question is whether which of EINTR or ERESTARTSYS is correct to be returned to userspace (so, EINTR).

But now that you pointed to the automagic ERESTARTSYS-to-EINTR conversion in the arch/ code, I think there's no problem returning ERESTARTSYS from syscalls in generic kernel code when interrupted by a signal either (if all arch's do that conversion).

Probably makes sense to change down_interruptible() and mutex_lock_interruptible() to return ERESTARTSYS (and not EINTR) in that case. wait_event_interruptible() is already ERESTARTSYS, I see ...

Satyam

author
#13 lore raw

PATCH 2/5 Use mutex instead of semaphore in the OnStream SCSI Tape driver

In reply to Matthias Kaehlcke

The OnStream SCSI Tape driver uses a semaphore as mutex. Use the mutex API instead of the (binary) semaphore.

Signed-off-by Matthias Kaehlcke <[email protected]>

--

drivers/scsi/osst.c +8 -8
index 08060fb..0e2452c 100644
@@ -3298,7 +3298,7 @@ static ssize_t osst_write(struct file * filp, const char __user * buf, size_t co
 	char		    * name = tape_name(STp);
 
 
-	if (down_interruptible(&STp->lock))
+	if (mutex_lock_interruptible(&STp->lock))
 		return (-ERESTARTSYS);
 
 	/*

@@ -3600,7 +3600,7 @@ if (SRpnt) printk(KERN_ERR "%s:A: Not supposed to have SRpnt at line %d\n", name
 out:
 	if (SRpnt != NULL) osst_release_request(SRpnt);
 
-	up(&STp->lock);
+	mutex_unlock(&STp->lock);
 
 	return retval;
 }

@@ -3619,7 +3619,7 @@ static ssize_t osst_read(struct file * filp, char __user * buf, size_t count, lo
 	char		    * name  = tape_name(STp);
 
 
-	if (down_interruptible(&STp->lock))
+	if (mutex_lock_interruptible(&STp->lock))
 		return (-ERESTARTSYS);
 
 	/*

@@ -3785,7 +3785,7 @@ static ssize_t osst_read(struct file * filp, char __user * buf, size_t count, lo
 out:
 	if (SRpnt != NULL) osst_release_request(SRpnt);
 
-	up(&STp->lock);
+	mutex_unlock(&STp->lock);
 
 	return retval;
 }

@@ -4852,7 +4852,7 @@ static int osst_ioctl(struct inode * inode,struct file * file,
 	char		    * name  = tape_name(STp);
 	void	    __user  * p     = (void __user *)arg;
 
-	if (down_interruptible(&STp->lock))
+	if (mutex_lock_interruptible(&STp->lock))
 		return -ERESTARTSYS;
 
 #if DEBUG

@@ -5163,14 +5163,14 @@ static int osst_ioctl(struct inode * inode,struct file * file,
 	}
 	if (SRpnt) osst_release_request(SRpnt);
 
-	up(&STp->lock);
+	mutex_unlock(&STp->lock);
 
 	return scsi_ioctl(STp->device, cmd_in, p);
 
 out:
 	if (SRpnt) osst_release_request(SRpnt);
 
-	up(&STp->lock);
+	mutex_unlock(&STp->lock);
 
 	return retval;
 }

@@ -5866,7 +5866,7 @@ static int osst_probe(struct device *dev)
 	tpnt->modes[2].defined = 1;
 	tpnt->density_changed = tpnt->compression_changed = tpnt->blksize_changed = 0;
 
-	init_MUTEX(&tpnt->lock);
+	mutex_init(&tpnt->lock);
 	osst_nr_dev++;
 	write_unlock(&os_scsi_tapes_lock);
drivers/scsi/osst.h +2 -1
index 2cc7b5a..5aa2274 100644
@@ -4,6 +4,7 @@
 
 #include <asm/byteorder.h>
 #include <linux/completion.h>
+#include <linux/mutex.h>
 
 /*	FIXME - rename and use the following two types or delete them!
  *              and the types really should go to st.h anyway...

@@ -532,7 +533,7 @@ struct osst_tape {
   struct scsi_driver *driver;
   unsigned capacity;
   struct scsi_device *device;
-  struct semaphore lock;       /* for serialization */
+  struct mutex lock;           /* for serialization */
   struct completion wait;      /* for SCSI commands */
   struct osst_buffer * buffer;
Signature · 12 lines
Matthias Kaehlcke
Linux Application Developer
Barcelona

                Don't walk behind me, I may not lead
             Don't walk in front of me, I may not follow
                Just walk beside me and be my friend
                          (Albert Camus)
                                                                 .''`.
    using free software / Debian GNU/Linux | http://debian.org  : :'  :
                                                                `. `'`
gpg --keyserver pgp.mit.edu --recv-keys 47D8E5D4                  `-
Reviewed-by on v1 2/5
#14 lore raw

Re: [PATCH 2/5] Use mutex instead of semaphore in the OnStream SCSI Tape driver

Hi,

drivers/scsi/osst.c static ssize_t osst_write(struct file * filp, const char __user * buf, size_t co · line 3302

Quoted 12 lines · Matthias Kaehlcke

The OnStream SCSI Tape driver uses a semaphore as mutex. Use the mutex API instead of the (binary) semaphore.

Signed-off-by Matthias Kaehlcke <[email protected]>
[...]
drivers/scsi/osst.c +1 -1
@@ -3298,7 +3298,7 @@ static ssize_t osst_write(struct file * filp, const char __user * buf, size_t co
 	char		    * name = tape_name(STp);
 
 
-	if (down_interruptible(&STp->lock))
+	if (mutex_lock_interruptible(&STp->lock))
 		return (-ERESTARTSYS);

The () after return existed in the code already, but you could've felt free to remove them :-)

drivers/scsi/osst.c static ssize_t osst_read(struct file * filp, char __user * buf, size_t count, lo · line 3623

Quoted 7 lines
drivers/scsi/osst.c +1 -1
@@ -3619,7 +3619,7 @@ static ssize_t osst_read(struct file * filp, char __user * buf, size_t count, lo
 	char		    * name  = tape_name(STp);
 
 
-	if (down_interruptible(&STp->lock))
+	if (mutex_lock_interruptible(&STp->lock))
 		return (-ERESTARTSYS);

Same here.

Reviewed-by Satyam Sharma <[email protected]>
author
#15 lore raw

PATCH 3/5 Use mutex instead of semaphore in the SCSI Tape driver

In reply to Matthias Kaehlcke

The SCSI Tape driver uses a semaphore as mutex. Use the mutex API instead of the (binary) semaphore.

Signed-off-by Matthias Kaehlcke <[email protected]>

--

drivers/scsi/st.c +8 -8
index a4f7b84..73c44cb 100644
@@ -1485,7 +1485,7 @@ st_write(struct file *filp, const char __user *buf, size_t count, loff_t * ppos)
 	struct st_buffer *STbp;
 	char *name = tape_name(STp);
 
-	if (down_interruptible(&STp->lock))
+	if (mutex_lock_interruptible(&STp->lock))
 		return -ERESTARTSYS;
 
 	retval = rw_checks(STp, filp, count);

@@ -1736,7 +1736,7 @@ st_write(struct file *filp, const char __user *buf, size_t count, loff_t * ppos)
 	if (SRpnt != NULL)
 		st_release_request(SRpnt);
 	release_buffering(STp, 0);
-	up(&STp->lock);
+	mutex_unlock(&STp->lock);
 
 	return retval;
 }

@@ -1942,7 +1942,7 @@ st_read(struct file *filp, char __user *buf, size_t count, loff_t * ppos)
 	struct st_buffer *STbp = STp->buffer;
 	DEB( char *name = tape_name(STp); )
 
-	if (down_interruptible(&STp->lock))
+	if (mutex_lock_interruptible(&STp->lock))
 		return -ERESTARTSYS;
 
 	retval = rw_checks(STp, filp, count);

@@ -2069,7 +2069,7 @@ st_read(struct file *filp, char __user *buf, size_t count, loff_t * ppos)
 		release_buffering(STp, 1);
 		STbp->buffer_bytes = 0;
 	}
-	up(&STp->lock);
+	mutex_unlock(&STp->lock);
 
 	return retval;
 }

@@ -3226,7 +3226,7 @@ static int st_ioctl(struct inode *inode, struct file *file,
 	char *name = tape_name(STp);
 	void __user *p = (void __user *)arg;
 
-	if (down_interruptible(&STp->lock))
+	if (mutex_lock_interruptible(&STp->lock))
 		return -ERESTARTSYS;
 
         DEB(

@@ -3537,7 +3537,7 @@ static int st_ioctl(struct inode *inode, struct file *file,
 			retval = (-EFAULT);
 		goto out;
 	}
-	up(&STp->lock);
+	mutex_unlock(&STp->lock);
 	switch (cmd_in) {
 		case SCSI_IOCTL_GET_IDLUN:
 		case SCSI_IOCTL_GET_BUS_NUMBER:

@@ -3563,7 +3563,7 @@ static int st_ioctl(struct inode *inode, struct file *file,
 	return retval;
 
  out:
-	up(&STp->lock);
+	mutex_unlock(&STp->lock);
 	return retval;
 }
 

@@ -4029,7 +4029,7 @@ static int st_probe(struct device *dev)
 
 	tpnt->density_changed = tpnt->compression_changed =
 	    tpnt->blksize_changed = 0;
-	init_MUTEX(&tpnt->lock);
+	mutex_init(&tpnt->lock);
 
 	st_nr_dev++;
 	write_unlock(&st_dev_arr_lock);
drivers/scsi/st.h +2 -1
index 50f3deb..6c80757 100644
@@ -3,6 +3,7 @@
 #define _ST_H
 
 #include <linux/completion.h>
+#include <linux/mutex.h>
 #include <linux/kref.h>
 #include <scsi/scsi_cmnd.h>
 

@@ -98,7 +99,7 @@ struct st_partstat {
 struct scsi_tape {
 	struct scsi_driver *driver;
 	struct scsi_device *device;
-	struct semaphore lock;	/* For serialization */
+	struct mutex lock;	/* For serialization */
 	struct completion wait;	/* For SCSI commands */
 	struct st_buffer *buffer;
Signature · 15 lines
Matthias Kaehlcke
Linux Application Developer
Barcelona

    Ma patrie est où je suis, où personne ne me dérange, où personne
    ne me demande que je suis, d'où je viens et ce que je fais
                              (B. Traven)
                                                                 .''`.
    using free software / Debian GNU/Linux | http://debian.org  : :'  :
                                                                `. `'`
gpg --keyserver pgp.mit.edu --recv-keys 47D8E5D4                  `-
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
author
#18 lore raw

PATCH 4/5 Use mutex instead of semaphore in ISDN subsystem common functions

In reply to Matthias Kaehlcke

The ISDN subsystem common functions use a semaphore as mutex. Use the mutex API instead of the (binary) semaphore.

Signed-off-by Matthias Kaehlcke <[email protected]>

--

drivers/isdn/i4l/isdn_common.c +13 -13
index c97330b..a5d3db4 100644
@@ -1364,7 +1364,7 @@ isdn_ioctl(struct inode *inode, struct file *file, uint cmd, ulong arg)
 				} else {
 					s = NULL;
 				}
-				ret = down_interruptible(&dev->sem);
+				ret = mutex_lock_interruptible(&dev->mtx);
 				if( ret ) return ret;
 				if ((s = isdn_net_new(s, NULL))) {
 					if (copy_to_user(argp, s, strlen(s) + 1)){

@@ -1374,7 +1374,7 @@ isdn_ioctl(struct inode *inode, struct file *file, uint cmd, ulong arg)
 					}
 				} else
 					ret = -ENODEV;
-				up(&dev->sem);
+				mutex_unlock(&dev->mtx);
 				return ret;
 			case IIOCNETASL:
 				/* Add a slave to a network-interface */

@@ -1383,7 +1383,7 @@ isdn_ioctl(struct inode *inode, struct file *file, uint cmd, ulong arg)
 						return -EFAULT;
 				} else
 					return -EINVAL;
-				ret = down_interruptible(&dev->sem);
+				ret = mutex_lock_interruptible(&dev->mtx);
 				if( ret ) return ret;
 				if ((s = isdn_net_newslave(bname))) {
 					if (copy_to_user(argp, s, strlen(s) + 1)){

@@ -1393,17 +1393,17 @@ isdn_ioctl(struct inode *inode, struct file *file, uint cmd, ulong arg)
 					}
 				} else
 					ret = -ENODEV;
-				up(&dev->sem);
+				mutex_unlock(&dev->mtx);
 				return ret;
 			case IIOCNETDIF:
 				/* Delete a network-interface */
 				if (arg) {
 					if (copy_from_user(name, argp, sizeof(name)))
 						return -EFAULT;
-					ret = down_interruptible(&dev->sem);
+					ret = mutex_lock_interruptible(&dev->mtx);
 					if( ret ) return ret;
 					ret = isdn_net_rm(name);
-					up(&dev->sem);
+					mutex_unlock(&dev->mtx);
 					return ret;
 				} else
 					return -EINVAL;

@@ -1432,10 +1432,10 @@ isdn_ioctl(struct inode *inode, struct file *file, uint cmd, ulong arg)
 				if (arg) {
 					if (copy_from_user(&phone, argp, sizeof(phone)))
 						return -EFAULT;
-					ret = down_interruptible(&dev->sem);
+					ret = mutex_lock_interruptible(&dev->mtx);
 					if( ret ) return ret;
 					ret = isdn_net_addphone(&phone);
-					up(&dev->sem);
+					mutex_unlock(&dev->mtx);
 					return ret;
 				} else
 					return -EINVAL;

@@ -1444,10 +1444,10 @@ isdn_ioctl(struct inode *inode, struct file *file, uint cmd, ulong arg)
 				if (arg) {
 					if (copy_from_user(&phone, argp, sizeof(phone)))
 						return -EFAULT;
-					ret = down_interruptible(&dev->sem);
+					ret = mutex_lock_interruptible(&dev->mtx);
 					if( ret ) return ret;
 					ret = isdn_net_getphones(&phone, argp);
-					up(&dev->sem);
+					mutex_unlock(&dev->mtx);
 					return ret;
 				} else
 					return -EINVAL;

@@ -1456,10 +1456,10 @@ isdn_ioctl(struct inode *inode, struct file *file, uint cmd, ulong arg)
 				if (arg) {
 					if (copy_from_user(&phone, argp, sizeof(phone)))
 						return -EFAULT;
-					ret = down_interruptible(&dev->sem);
+					ret = mutex_lock_interruptible(&dev->mtx);
 					if( ret ) return ret;
 					ret = isdn_net_delphone(&phone);
-					up(&dev->sem);
+					mutex_unlock(&dev->mtx);
 					return ret;
 				} else
 					return -EINVAL;

@@ -2303,7 +2303,7 @@ static int __init isdn_init(void)
 #ifdef MODULE
 	dev->owner = THIS_MODULE;
 #endif
-	init_MUTEX(&dev->sem);
+	mutex_init(&dev->mtx);
 	init_waitqueue_head(&dev->info_waitq);
 	for (i = 0; i < ISDN_MAX_CHANNELS; i++) {
 		dev->drvmap[i] = -1;
include/linux/isdn.h +2 -1
index 3c7875b..5a77203 100644
@@ -15,6 +15,7 @@
 #define __ISDN_H__
 
 #include <linux/ioctl.h>
+#include <linux/mutex.h>
 
 #ifdef CONFIG_COBALT_MICRO_SERVER
 /* Save memory */

@@ -624,7 +625,7 @@ typedef struct isdn_devt {
 	int               v110emu[ISDN_MAX_CHANNELS]; /* V.110 emulator-mode 0=none */
 	atomic_t          v110use[ISDN_MAX_CHANNELS]; /* Usage-Semaphore for stream */
 	isdn_v110_stream  *v110[ISDN_MAX_CHANNELS];   /* V.110 private data         */
-	struct semaphore  sem;                        /* serialize list access*/
+	struct mutex      mtx;                        /* serialize list access*/
 	unsigned long     global_features;
 } isdn_dev;
Signature · 11 lines
Matthias Kaehlcke
Linux Application Developer
Barcelona

    Tant qu'il y aura sur terre des hommes pour qui existe un concept
    d' 'honneur national', la menace d'une nouvelle guerre subsistera.
                              (B. Traven)
                                                                 .''`.
    using free software / Debian GNU/Linux | http://debian.org  : :'  :
                                                                `. `'`
gpg --keyserver pgp.mit.edu --recv-keys 47D8E5D4                  `-
Acked-by on v1 4/5
#20 lore raw

Re: [PATCH 4/5] Use mutex instead of semaphore in ISDN subsystem common functions

In reply to Matthias Kaehlcke
Quoted 5 lines · Matthias Kaehlcke

The ISDN subsystem common functions use a semaphore as mutex. Use the mutex API instead of the (binary) semaphore.

Signed-off-by Matthias Kaehlcke <[email protected]>
Acked-by Karsten Keil <[email protected]>
Signature · 4 lines
Karsten Keil
SuSE Labs
ISDN and VOIP development
SUSE LINUX Products GmbH, Maxfeldstr.5 90409 Nuernberg, GF: Markus Rex, HRB 16746 (AG Nuernberg)
#21 lore raw

Re: [PATCH 4/5] Use mutex instead of semaphore in ISDN subsystem common functions

In reply to Matthias Kaehlcke

include/linux/isdn.h line 18

Quoted 7 lines · On Sun, 29 Jul 2007 23:41:05 +0200 Matthias Kaehlcke…
include/linux/isdn.h +1 -0
@@ -15,6 +15,7 @@
 #define __ISDN_H__
 
 #include <linux/ioctl.h>
+#include <linux/mutex.h>

Breaks `make headers_check'.

That was covered in sections 2 and 20 of Documentation/SubmitChecklist, which nobody reads.

author
#22 lore raw

PATCH 5/5 Use mutex instead of semaphore in the DVB frontend tuning interface

In reply to Matthias Kaehlcke

The DVB frontend tuning interface uses a semaphore as mutex. Use the mutex API instead of the (binary) semaphore.

Signed-off-by Matthias Kaehlcke <[email protected]>

--

drivers/media/dvb/dvb-core/dvb_frontend.c +5 -5
index b6c7f66..e35dd17 100644
@@ -138,7 +138,7 @@ static void dvb_frontend_add_event(struct dvb_frontend *fe, fe_status_t status)
 
 	dprintk ("%s\n", __FUNCTION__);
 
-	if (down_interruptible (&events->sem))
+	if (mutex_lock_interruptible (&events->mtx))
 		return;
 
 	wp = (events->eventw + 1) % MAX_EVENT;

@@ -159,7 +159,7 @@ static void dvb_frontend_add_event(struct dvb_frontend *fe, fe_status_t status)
 
 	events->eventw = wp;
 
-	up (&events->sem);
+	mutex_unlock(&events->mtx);
 
 	e->status = status;
 

@@ -197,7 +197,7 @@ static int dvb_frontend_get_event(struct dvb_frontend *fe,
 			return ret;
 	}
 
-	if (down_interruptible (&events->sem))
+	if (mutex_lock_interruptible (&events->mtx))
 		return -ERESTARTSYS;
 
 	memcpy (event, &events->events[events->eventr],

@@ -205,7 +205,7 @@ static int dvb_frontend_get_event(struct dvb_frontend *fe,
 
 	events->eventr = (events->eventr + 1) % MAX_EVENT;
 
-	up (&events->sem);
+	mutex_unlock(&events->mtx);
 
 	return 0;
 }

@@ -1080,7 +1080,7 @@ int dvb_register_frontend(struct dvb_adapter* dvb,
 	init_MUTEX (&fepriv->sem);
 	init_waitqueue_head (&fepriv->wait_queue);
 	init_waitqueue_head (&fepriv->events.wait_queue);
-	init_MUTEX (&fepriv->events.sem);
+	mutex_init(&fepriv->events.mtx);
 	fe->dvb = dvb;
 	fepriv->inversion = INVERSION_OFF;
drivers/media/dvb/dvb-core/dvb_frontend.h +2 -1
index a770a87..f95de63 100644
@@ -35,6 +35,7 @@
 #include <linux/module.h>
 #include <linux/errno.h>
 #include <linux/delay.h>
+#include <linux/mutex.h>
 
 #include <linux/dvb/frontend.h>
 

@@ -142,7 +143,7 @@ struct dvb_fe_events {
 	int			  eventr;
 	int			  overflow;
 	wait_queue_head_t	  wait_queue;
-	struct semaphore	  sem;
+	struct mutex		  mtx;
 };
 
 struct dvb_frontend {
Signature · 10 lines
Matthias Kaehlcke
Linux Application Developer
Barcelona

                       El camino se hace al andar
                           (Antonio Machado)
                                                                 .''`.
    using free software / Debian GNU/Linux | http://debian.org  : :'  :
                                                                `. `'`
gpg --keyserver pgp.mit.edu --recv-keys 47D8E5D4                  `-