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 <[email protected]>
---
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;
}
--
On Fri, 21 Dec 2007 00:00:04 -0800 Daniel Walker <[email protected]> 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 <[email protected]>
>
> ---
> 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?
On Fri, 21 Dec 2007 00:00:04 -0800, Daniel Walker <[email protected]> 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
On Fri, 21 Dec 2007 22:24:28 -0800 Pete Zaitcev <[email protected]> wrote:
> On Fri, 21 Dec 2007 00:00:04 -0800, Daniel Walker <[email protected]> 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 ;)
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
On Sat, 22 Dec 2007 09:01:50 -0800, Daniel Walker <[email protected]> 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
On Sat, 2007-12-22 at 23:37 -0800, Pete Zaitcev wrote:
> On Sat, 22 Dec 2007 09:01:50 -0800, Daniel Walker <[email protected]> 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
On Sun, 23 Dec 2007 08:46:37 -0800, Daniel Walker <[email protected]> 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
On Mon, 2007-12-24 at 06:12 -0800, Pete Zaitcev wrote:
> On Sun, 23 Dec 2007 08:46:37 -0800, Daniel Walker <[email protected]> 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