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

[Matthias Kaehlcke <matthias.kaehlcke@gmail.com>]

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

> Whoops ...
> 
> 
> On Mon, 30 Jul 2007, Satyam Sharma wrote:
> 
> > On Mon, 30 Jul 2007, Michael Buesch wrote:
> > 
> > > On Sunday 29 July 2007 23:34, Matthias Kaehlcke wrote:
> > > > The Host AP driver uses a semaphore as mutex. Use the mutex API
> > > > instead of the (binary) semaphore.
> > > > 
> > > > Signed-off-by: Matthias Kaehlcke <matthias.kaehlcke@gmail.com>
> > 
> > [ Something seems to have gone wrong with your diff / patch / script.
> >   There was no diff header here, which should have been. ]
> > 
> > > > -	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);
> > > >  
> > > > -	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.
> > 
> > > 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.
> 
> 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 <matthias.kaehlcke@gmail.com>

--

diff --git a/drivers/net/wireless/hostap/hostap_hw.c b/drivers/net/wireless/hostap/hostap_hw.c
index 959887b..d3dacbc 100644
--- a/drivers/net/wireless/hostap/hostap_hw.c
+++ b/drivers/net/wireless/hostap/hostap_hw.c
@@ -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;
diff --git a/drivers/net/wireless/hostap/hostap_wlan.h b/drivers/net/wireless/hostap/hostap_wlan.h
index 87a54aa..a42325c 100644
--- a/drivers/net/wireless/hostap/hostap_wlan.h
+++ b/drivers/net/wireless/hostap/hostap_wlan.h
@@ -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 */

-- 
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                  `-