[PATCH 1/4] usb microtek: remove unused semaphore

[PATCH 1/4] usb microtek: remove unused semaphore

[Daniel Walker]

[-- Attachment #1: usb-microtek-remove-unused-semaphore.patch --]
[-- Type: text/plain, Size: 1004 bytes --]

No current references, so removing it.

Signed-off-by: Daniel Walker <dwalker@mvista.com>

---
 drivers/usb/image/microtek.c |    1 -
 drivers/usb/image/microtek.h |    1 -
 2 files changed, 2 deletions(-)

Index: linux-2.6.23/drivers/usb/image/microtek.c
===================================================================
--- linux-2.6.23.orig/drivers/usb/image/microtek.c
+++ linux-2.6.23/drivers/usb/image/microtek.c
@@ -794,7 +794,6 @@ static int mts_usb_probe(struct usb_inte
 
 	new_desc->usb_dev = dev;
 	new_desc->usb_intf = intf;
-	init_MUTEX(&new_desc->lock);
 
 	/* endpoints */
 	new_desc->ep_out = ep_out;
Index: linux-2.6.23/drivers/usb/image/microtek.h
===================================================================
--- linux-2.6.23.orig/drivers/usb/image/microtek.h
+++ linux-2.6.23/drivers/usb/image/microtek.h
@@ -39,7 +39,6 @@ struct mts_desc {
 	u8 ep_image;
 
 	struct Scsi_Host * host;
-	struct semaphore lock;
 
 	struct urb *urb;
 	struct mts_transfer_context context;

-- 

[PATCH 2/4] prism54: remove questionable down_interruptible usage

[Daniel Walker]

[-- Attachment #1: wireless-prism54-fix-down_interruptible-usage.patch --]
[-- Type: text/plain, Size: 1781 bytes --]

Reviewing the semaphore usage I noticed these down_interruptible
calls. Most of these aren't returning anything, so a caller can't
tell if the operation completed or not. prism54_wpa_bss_ie_get()
returns zero, but it's treated as the function failing which doesn't
seem correct.

Signed-off-by: Daniel Walker <dwalker@mvista.com>
---
 drivers/net/wireless/prism54/isl_ioctl.c |   12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

Index: linux-2.6.23/drivers/net/wireless/prism54/isl_ioctl.c
===================================================================
--- linux-2.6.23.orig/drivers/net/wireless/prism54/isl_ioctl.c
+++ linux-2.6.23/drivers/net/wireless/prism54/isl_ioctl.c
@@ -165,8 +165,7 @@ prism54_update_stats(struct work_struct 
 	struct obj_bss bss, *bss2;
 	union oid_res_t r;
 
-	if (down_interruptible(&priv->stats_sem))
-		return;
+	down(&priv->stats_sem);
 
 /* Noise floor.
  * I'm not sure if the unit is dBm.
@@ -1793,8 +1792,7 @@ prism54_clear_mac(struct islpci_acl *acl
 	struct list_head *ptr, *next;
 	struct mac_entry *entry;
 
-	if (down_interruptible(&acl->sem))
-		return;
+	down(&acl->sem);
 
 	if (acl->size == 0) {
 		up(&acl->sem);
@@ -2116,8 +2114,7 @@ prism54_wpa_bss_ie_add(islpci_private *p
 	if (wpa_ie_len > MAX_WPA_IE_LEN)
 		wpa_ie_len = MAX_WPA_IE_LEN;
 
-	if (down_interruptible(&priv->wpa_sem))
-		return;
+	down(&priv->wpa_sem);
 
 	/* try to use existing entry */
 	list_for_each(ptr, &priv->bss_wpa_list) {
@@ -2178,8 +2175,7 @@ prism54_wpa_bss_ie_get(islpci_private *p
 	struct islpci_bss_wpa_ie *bss = NULL;
 	size_t len = 0;
 
-	if (down_interruptible(&priv->wpa_sem))
-		return 0;
+	down(&priv->wpa_sem);
 
 	list_for_each(ptr, &priv->bss_wpa_list) {
 		bss = list_entry(ptr, struct islpci_bss_wpa_ie, list);

-- 

[PATCH 3/4] docs: convert kref semaphore to mutex

[Daniel Walker]

[-- Attachment #1: docs-kref-remove-semaphore-reference.patch --]
[-- Type: text/plain, Size: 2033 bytes --]

Just converting this documentation semaphore reference, since we don't
want to promote semaphore usage.

Signed-off-by: Daniel Walker <dwalker@mvista.com>

---
 Documentation/kref.txt |   20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

Index: linux-2.6.23/Documentation/kref.txt
===================================================================
--- linux-2.6.23.orig/Documentation/kref.txt
+++ linux-2.6.23/Documentation/kref.txt
@@ -141,10 +141,10 @@ The last rule (rule 3) is the nastiest o
 instance, you have a list of items that are each kref-ed, and you wish
 to get the first one.  You can't just pull the first item off the list
 and kref_get() it.  That violates rule 3 because you are not already
-holding a valid pointer.  You must add locks or semaphores.  For
-instance:
+holding a valid pointer.  You must add a mutex (or some other lock).
+For instance:
 
-static DECLARE_MUTEX(sem);
+static DEFINE_MUTEX(mutex);
 static LIST_HEAD(q);
 struct my_data
 {
@@ -155,12 +155,12 @@ struct my_data
 static struct my_data *get_entry()
 {
 	struct my_data *entry = NULL;
-	down(&sem);
+	mutex_lock(&mutex);
 	if (!list_empty(&q)) {
 		entry = container_of(q.next, struct my_q_entry, link);
 		kref_get(&entry->refcount);
 	}
-	up(&sem);
+	mutex_unlock(&mutex);
 	return entry;
 }
 
@@ -174,9 +174,9 @@ static void release_entry(struct kref *r
 
 static void put_entry(struct my_data *entry)
 {
-	down(&sem);
+	mutex_lock(&mutex);
 	kref_put(&entry->refcount, release_entry);
-	up(&sem);
+	mutex_unlock(&mutex);
 }
 
 The kref_put() return value is useful if you do not want to hold the
@@ -191,13 +191,13 @@ static void release_entry(struct kref *r
 
 static void put_entry(struct my_data *entry)
 {
-	down(&sem);
+	mutex_lock(&mutex);
 	if (kref_put(&entry->refcount, release_entry)) {
 		list_del(&entry->link);
-		up(&sem);
+		mutex_unlock(&mutex);
 		kfree(entry);
 	} else
-		up(&sem);
+		mutex_unlock(&mutex);
 }
 
 This is really more useful if you have to call other routines as part

-- 

Re: [PATCH 3/4] docs: convert kref semaphore to mutex

[Corey Minyard]

Yes, a good idea.

Acked-by: Corey Minyard <minyard@acm.org>

Daniel Walker wrote:
> Just converting this documentation semaphore reference, since we don't
> want to promote semaphore usage.
>
> Signed-off-by: Daniel Walker <dwalker@mvista.com>
>
> ---
>  Documentation/kref.txt |   20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
>
> Index: linux-2.6.23/Documentation/kref.txt
> ===================================================================
> --- linux-2.6.23.orig/Documentation/kref.txt
> +++ linux-2.6.23/Documentation/kref.txt
> @@ -141,10 +141,10 @@ The last rule (rule 3) is the nastiest o
>  instance, you have a list of items that are each kref-ed, and you wish
>  to get the first one.  You can't just pull the first item off the list
>  and kref_get() it.  That violates rule 3 because you are not already
> -holding a valid pointer.  You must add locks or semaphores.  For
> -instance:
> +holding a valid pointer.  You must add a mutex (or some other lock).
> +For instance:
>  
> -static DECLARE_MUTEX(sem);
> +static DEFINE_MUTEX(mutex);
>  static LIST_HEAD(q);
>  struct my_data
>  {
> @@ -155,12 +155,12 @@ struct my_data
>  static struct my_data *get_entry()
>  {
>  	struct my_data *entry = NULL;
> -	down(&sem);
> +	mutex_lock(&mutex);
>  	if (!list_empty(&q)) {
>  		entry = container_of(q.next, struct my_q_entry, link);
>  		kref_get(&entry->refcount);
>  	}
> -	up(&sem);
> +	mutex_unlock(&mutex);
>  	return entry;
>  }
>  
> @@ -174,9 +174,9 @@ static void release_entry(struct kref *r
>  
>  static void put_entry(struct my_data *entry)
>  {
> -	down(&sem);
> +	mutex_lock(&mutex);
>  	kref_put(&entry->refcount, release_entry);
> -	up(&sem);
> +	mutex_unlock(&mutex);
>  }
>  
>  The kref_put() return value is useful if you do not want to hold the
> @@ -191,13 +191,13 @@ static void release_entry(struct kref *r
>  
>  static void put_entry(struct my_data *entry)
>  {
> -	down(&sem);
> +	mutex_lock(&mutex);
>  	if (kref_put(&entry->refcount, release_entry)) {
>  		list_del(&entry->link);
> -		up(&sem);
> +		mutex_unlock(&mutex);
>  		kfree(entry);
>  	} else
> -		up(&sem);
> +		mutex_unlock(&mutex);
>  }
>  
>  This is really more useful if you have to call other routines as part
>
>   

[PATCH 4/4] usb: libusual: locking cleanup

[Daniel Walker]

[-- Attachment #1: libusual-locking-cleanup.patch --]
[-- Type: text/plain, Size: 1993 bytes --]

I converted the usu_init_notify semaphore to normal mutex usage, and it 
should still prevent the request_module before the init routine is
complete. Before it acted more like a complete, now the mutex protects
two distinct section from running at the same time..

Signed-off-by: Daniel Walker <dwalker@mvista.com>

---
 drivers/usb/storage/libusual.c |   14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

Index: linux-2.6.23/drivers/usb/storage/libusual.c
===================================================================
--- linux-2.6.23.orig/drivers/usb/storage/libusual.c
+++ linux-2.6.23/drivers/usb/storage/libusual.c
@@ -9,6 +9,7 @@
 #include <linux/usb_usual.h>
 #include <linux/vmalloc.h>
 #include <linux/kthread.h>
+#include <linux/mutex.h>
 
 /*
  */
@@ -30,7 +31,7 @@ static atomic_t usu_bias = ATOMIC_INIT(U
 #define BIAS_NAME_SIZE  (sizeof("usb-storage"))
 static const char *bias_names[3] = { "none", "usb-storage", "ub" };
 
-static struct semaphore usu_init_notify;
+static DEFINE_MUTEX(usu_probe_mutex);
 static DECLARE_COMPLETION(usu_end_notify);
 static atomic_t total_threads = ATOMIC_INIT(0);
 
@@ -178,10 +179,7 @@ static int usu_probe_thread(void *arg)
 	int rc;
 	unsigned long flags;
 
-	/* A completion does not work here because it's counted. */
-	down(&usu_init_notify);
-	up(&usu_init_notify);
-
+	mutex_lock(&usu_probe_mutex);
 	rc = request_module(bias_names[type]);
 	spin_lock_irqsave(&usu_lock, flags);
 	if (rc == 0 && (st->fls & USU_MOD_FL_PRESENT) == 0) {
@@ -194,6 +192,7 @@ static int usu_probe_thread(void *arg)
 	}
 	st->fls &= ~USU_MOD_FL_THREAD;
 	spin_unlock_irqrestore(&usu_lock, flags);
+	mutex_unlock(&usu_probe_mutex);
 
 	complete_and_exit(&usu_end_notify, 0);
 }
@@ -204,10 +203,9 @@ static int __init usb_usual_init(void)
 {
 	int rc;
 
-	sema_init(&usu_init_notify, 0);
-
+	mutex_lock(&usu_probe_mutex);
 	rc = usb_register(&usu_driver);
-	up(&usu_init_notify);
+	mutex_unlock(&usu_probe_mutex);
 	return rc;
 }
 

-- 

Re: [PATCH 4/4] usb: libusual: locking cleanup

[Andrew Morton]

On Fri, 21 Dec 2007 00:00:04 -0800 Daniel Walker <dwalker@mvista.com> wrote:

> I converted the usu_init_notify semaphore to normal mutex usage, and it 
> should still prevent the request_module before the init routine is
> complete. Before it acted more like a complete, now the mutex protects
> two distinct section from running at the same time..
> 
> Signed-off-by: Daniel Walker <dwalker@mvista.com>
> 
> ---
>  drivers/usb/storage/libusual.c |   14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
> 
> Index: linux-2.6.23/drivers/usb/storage/libusual.c
> ===================================================================
> --- linux-2.6.23.orig/drivers/usb/storage/libusual.c
> +++ linux-2.6.23/drivers/usb/storage/libusual.c
> @@ -9,6 +9,7 @@
>  #include <linux/usb_usual.h>
>  #include <linux/vmalloc.h>
>  #include <linux/kthread.h>
> +#include <linux/mutex.h>
>  
>  /*
>   */
> @@ -30,7 +31,7 @@ static atomic_t usu_bias = ATOMIC_INIT(U
>  #define BIAS_NAME_SIZE  (sizeof("usb-storage"))
>  static const char *bias_names[3] = { "none", "usb-storage", "ub" };
>  
> -static struct semaphore usu_init_notify;
> +static DEFINE_MUTEX(usu_probe_mutex);
>  static DECLARE_COMPLETION(usu_end_notify);
>  static atomic_t total_threads = ATOMIC_INIT(0);
>  
> @@ -178,10 +179,7 @@ static int usu_probe_thread(void *arg)
>  	int rc;
>  	unsigned long flags;
>  
> -	/* A completion does not work here because it's counted. */
> -	down(&usu_init_notify);
> -	up(&usu_init_notify);
> -
> +	mutex_lock(&usu_probe_mutex);
>  	rc = request_module(bias_names[type]);
>  	spin_lock_irqsave(&usu_lock, flags);
>  	if (rc == 0 && (st->fls & USU_MOD_FL_PRESENT) == 0) {
> @@ -194,6 +192,7 @@ static int usu_probe_thread(void *arg)
>  	}
>  	st->fls &= ~USU_MOD_FL_THREAD;
>  	spin_unlock_irqrestore(&usu_lock, flags);
> +	mutex_unlock(&usu_probe_mutex);
>  
>  	complete_and_exit(&usu_end_notify, 0);
>  }
> @@ -204,10 +203,9 @@ static int __init usb_usual_init(void)
>  {
>  	int rc;
>  
> -	sema_init(&usu_init_notify, 0);
> -
> +	mutex_lock(&usu_probe_mutex);
>  	rc = usb_register(&usu_driver);
> -	up(&usu_init_notify);
> +	mutex_unlock(&usu_probe_mutex);
>  	return rc;
>  }

That's some pretty hairy-looking code you're playing with there.  Has this
been runtime tested?

Re: [PATCH 4/4] usb: libusual: locking cleanup

[Pete Zaitcev]

On Fri, 21 Dec 2007 00:00:04 -0800, Daniel Walker <dwalker@mvista.com> wrote:

> I converted the usu_init_notify semaphore to normal mutex usage, and it 
> should still prevent the request_module before the init routine is
> complete. Before it acted more like a complete, now the mutex protects
> two distinct section from running at the same time..

Let's see.

> @@ -178,10 +179,7 @@ static int usu_probe_thread(void *arg)
>  	int rc;
>  	unsigned long flags;
>  
> -	/* A completion does not work here because it's counted. */
> -	down(&usu_init_notify);
> -	up(&usu_init_notify);
> -
> +	mutex_lock(&usu_probe_mutex);
>  	rc = request_module(bias_names[type]);

When I tried it, usb-storage would not load with unresolved symbols.
It happens if child (usu_probe_thread) runs ahead of its parent
(usb_usual_init -> usb_register -> usu_probe). It's entirely possible,
depending on your scheduler.

I hate this down-up trick too, so if you have a better idea, I'm all ears.

-- Pete

Re: [PATCH 4/4] usb: libusual: locking cleanup

[Andrew Morton]

On Fri, 21 Dec 2007 22:24:28 -0800 Pete Zaitcev <zaitcev@redhat.com> wrote:

> On Fri, 21 Dec 2007 00:00:04 -0800, Daniel Walker <dwalker@mvista.com> wrote:
> 
> > I converted the usu_init_notify semaphore to normal mutex usage, and it 
> > should still prevent the request_module before the init routine is
> > complete. Before it acted more like a complete, now the mutex protects
> > two distinct section from running at the same time..
> 
> Let's see.
> 
> > @@ -178,10 +179,7 @@ static int usu_probe_thread(void *arg)
> >  	int rc;
> >  	unsigned long flags;
> >  
> > -	/* A completion does not work here because it's counted. */
> > -	down(&usu_init_notify);
> > -	up(&usu_init_notify);
> > -
> > +	mutex_lock(&usu_probe_mutex);
> >  	rc = request_module(bias_names[type]);
> 
> When I tried it, usb-storage would not load with unresolved symbols.
> It happens if child (usu_probe_thread) runs ahead of its parent
> (usb_usual_init -> usb_register -> usu_probe). It's entirely possible,
> depending on your scheduler.

afaict Daniel's change will fix that?

He releases usu_probe_mutex once the usb_register() has completed and this
then allows the usb_probe_thread() to start working.

I'm still wondering if that theory has been tested though.

> I hate this down-up trick too

I'd be worried if you were proud of it ;)

Re: [PATCH 4/4] usb: libusual: locking cleanup

[Daniel Walker]

On Fri, 2007-12-21 at 22:24 -0800, Pete Zaitcev wrote:

> When I tried it, usb-storage would not load with unresolved symbols.
> It happens if child (usu_probe_thread) runs ahead of its parent
> (usb_usual_init -> usb_register -> usu_probe). It's entirely possible,
> depending on your scheduler.
> 
> I hate this down-up trick too, so if you have a better idea, I'm all ears.

This is what you originally had,

static int usu_probe_thread(void *arg)
{

        /* A completion does not work here because it's counted. */
        down(&usu_init_notify);
        up(&usu_init_notify);
...
}

static int __init usb_usual_init(void)
{
        sema_init(&usu_init_notify, 0); <-- Locked init

        rc = usb_register(&usu_driver);
        up(&usu_init_notify);
        return rc;
}

The locked init can easily be an unlocked init combined with a down() ..
So your protecting usb_register() from something else. 

Then in usu_probe_thread() your basically stopping it at the start of
the function with a down(), and the up() is just ancillary .. So you
could easily move the up() further down in the function and still have
the same level of exclusion.. 

static int usu_probe_thread(void *arg)
{

        down(&usu_init_notify);
...
        up(&usu_init_notify);
}

static int __init usb_usual_init(void)
{
        sema_init(&usu_init_notify, 1); <-- Unlocked init

	down(&usu_init_notify);
        rc = usb_register(&usu_driver);
        up(&usu_init_notify);
        return rc;
}

The above protects the same way that your original code did, with the
added benefit of conforming to mutex style usage. The next step is to
convert to the mutex API..

What I've done is all suppose to be mathematical translations, I wasn't
trying to improve the code just make it use a different API..

Daniel

Re: [PATCH 4/4] usb: libusual: locking cleanup

[Pete Zaitcev]

On Sat, 22 Dec 2007 09:01:50 -0800, Daniel Walker <dwalker@mvista.com> wrote:

> Then in usu_probe_thread() your basically stopping it at the start of
> the function with a down(), and the up() is just ancillary .. So you
> could easily move the up() further down in the function and still have
> the same level of exclusion.. 

The unfortunate complication here is request_module. I didn't want to
keep a semaphore locked across it, in case child waits for something.
I wonder if there may be some deadlock that we cannot foresee.
But I guess it won't hurt to try.

I tested the patch and it seems to work ok.

-- Pete

Re: [PATCH 4/4] usb: libusual: locking cleanup

[Daniel Walker]

On Sat, 2007-12-22 at 23:37 -0800, Pete Zaitcev wrote:
> On Sat, 22 Dec 2007 09:01:50 -0800, Daniel Walker <dwalker@mvista.com> wrote:
> 
> > Then in usu_probe_thread() your basically stopping it at the start of
> > the function with a down(), and the up() is just ancillary .. So you
> > could easily move the up() further down in the function and still have
> > the same level of exclusion.. 
> 
> The unfortunate complication here is request_module. I didn't want to
> keep a semaphore locked across it, in case child waits for something.
> I wonder if there may be some deadlock that we cannot foresee.
> But I guess it won't hurt to try.

I noticed you also have a spinlock held in usu_probe_thread(), the
usu_lock.. That spinlock would preclude anything inside request_module()
from sleeping..

One thing that has bothered me is that I don't see a reason why this
couldn't become a complete, yet you have a comment which says that it
can't be a complete.. I honestly didn't understand the comment.. I would
imagine that you tried a complete , and it didn't work?

> I tested the patch and it seems to work ok.

Great, thanks ..

Daniel

Re: [PATCH 4/4] usb: libusual: locking cleanup

[Pete Zaitcev]

On Sun, 23 Dec 2007 08:46:37 -0800, Daniel Walker <dwalker@mvista.com> wrote:

> I noticed you also have a spinlock held in usu_probe_thread(), the
> usu_lock.. That spinlock would preclude anything inside request_module()
> from sleeping..

The usu_lock is not held across request_module. In fact, it can be
easily taken from inside request_module, when it invokes modprobe.
Stop scaring me :-)

> One thing that has bothered me is that I don't see a reason why this
> couldn't become a complete, yet you have a comment which says that it
> can't be a complete.. I honestly didn't understand the comment.. I would
> imagine that you tried a complete , and it didn't work?

Yes, it was a completition initially. But suppose you have two storage
devices, plugged in across a reboot. Two threads are created and have to
wait until the libusual's init function ends. Since we post one completion,
only one thread continues.

-- Pete

Re: [PATCH 4/4] usb: libusual: locking cleanup

[Daniel Walker]

On Mon, 2007-12-24 at 06:12 -0800, Pete Zaitcev wrote:
> On Sun, 23 Dec 2007 08:46:37 -0800, Daniel Walker <dwalker@mvista.com> wrote:
> 
> > I noticed you also have a spinlock held in usu_probe_thread(), the
> > usu_lock.. That spinlock would preclude anything inside request_module()
> > from sleeping..
> 
> The usu_lock is not held across request_module. In fact, it can be
> easily taken from inside request_module, when it invokes modprobe.
> Stop scaring me :-)

Your right, it's just outside .. I still don't think it could deadlock,
since I don't see a code path to recursively get back into those
libusual functions..

> > One thing that has bothered me is that I don't see a reason why this
> > couldn't become a complete, yet you have a comment which says that it
> > can't be a complete.. I honestly didn't understand the comment.. I would
> > imagine that you tried a complete , and it didn't work?
> 
> Yes, it was a completition initially. But suppose you have two storage
> devices, plugged in across a reboot. Two threads are created and have to
> wait until the libusual's init function ends. Since we post one
> completion,
> only one thread continues.

Ok .. The mutex should just prevent them from running at the same time,
but they should both run eventually.

Daniel