Hi,
When detaching a port from the client side (usbip --detach 0),
the event thread, on the server side, is going to deadlock.
The "eh" server thread is getting USBIP_EH_RESET event and calls:
-> stub_device_reset() -> usb_reset_device()
the USB framework is then calling back _in the same "eh" thread_ :
-> stub_disconnect() -> usbip_stop_eh() -> wait_for_completion()
the "eh" thread is being asleep forever, waiting for its own completion.
The patch checks if "eh" is the current thread, in usbip_stop_eh().
Please Cc me in reply, I'm not in the list.
b.r.
------------------
diff -Nur linux-2.6.34-rc5/drivers/staging/usbip/usbip_event.c
linux-2.6.34-rc5.new/drivers/staging/usbip/usbip_event.c
--- linux-2.6.34-rc5/drivers/staging/usbip/usbip_event.c 2010-04-20
01:29:56.000000000 +0200
+++ linux-2.6.34-rc5.new/drivers/staging/usbip/usbip_event.c
2010-04-22 17:07:36.249588273 +0200
@@ -116,6 +116,13 @@
void usbip_stop_eh(struct usbip_device *ud)
{
struct usbip_task *eh = &ud->eh;
+ int i_am_eh;
+
+ lock_kernel();
+ i_am_eh = (eh->thread == current);
+ unlock_kernel();
+ if (i_am_eh)
+ return; /* do not wait for myself */
wait_for_completion(&eh->thread_done);
usbip_dbg_eh("usbip_eh has finished\n");
On Thu, Apr 22, 2010 at 06:54:02PM +0200, Eric Lescouet wrote:
> Hi,
> When detaching a port from the client side (usbip --detach 0),
> the event thread, on the server side, is going to deadlock.
> The "eh" server thread is getting USBIP_EH_RESET event and calls:
> -> stub_device_reset() -> usb_reset_device()
> the USB framework is then calling back _in the same "eh" thread_ :
> -> stub_disconnect() -> usbip_stop_eh() -> wait_for_completion()
> the "eh" thread is being asleep forever, waiting for its own completion.
> The patch checks if "eh" is the current thread, in usbip_stop_eh().
Ok, but does the lock_kernel() call you just made actually prevent this
from happening? Isn't there some other lock you can use instead?
> Please Cc me in reply, I'm not in the list.
Also, to have patches that we can apply, we need a "Signed-off-by:"
line. See the file, Documentation/SubmittingPatches for what this
means.
>
> b.r.
>
> ------------------
>
> diff -Nur linux-2.6.34-rc5/drivers/staging/usbip/usbip_event.c
> linux-2.6.34-rc5.new/drivers/staging/usbip/usbip_event.c
> --- linux-2.6.34-rc5/drivers/staging/usbip/usbip_event.c
> 2010-04-20 01:29:56.000000000 +0200
Your email client wrapped the patch and made it unappliable :(
thanks,
greg k-h
Greg KH wrote:
[...]
> Ok, but does the lock_kernel() call you just made actually prevent this
> from happening? Isn't there some other lock you can use instead?
>
Right. lock_kernel() is not needed (was copy/paste).
The deadlock is avoided by not calling wait_for_completion()
when the code is executed by the "eh" thread itself.
> Also, to have patches that we can apply, we need a "Signed-off-by:"
> line. See the file, Documentation/SubmittingPatches for what this
> means.
Understood.
> Your email client wrapped the patch and made it unappliable :(
>
> thanks,
>
> greg k-h
Sorry about that. Please find below an hopefully better one.
Thanks,
Eric.
Signed-off-by: Eric Lescouet <[email protected]>
-------------
diff -Nur linux-2.6.34-rc5/drivers/staging/usbip/usbip_event.c linux-2.6.34-rc5.new/drivers/staging/usbip/usbip_event.c
--- linux-2.6.34-rc5/drivers/staging/usbip/usbip_event.c 2010-04-20 01:29:56.000000000 +0200
+++ linux-2.6.34-rc5.new/drivers/staging/usbip/usbip_event.c 2010-04-22 19:19:52.997889126 +0200
@@ -117,6 +117,9 @@
{
struct usbip_task *eh = &ud->eh;
+ if (eh->thread == current)
+ return; /* do not wait for myself */
+
wait_for_completion(&eh->thread_done);
usbip_dbg_eh("usbip_eh has finished\n");
}
On Thu, Apr 22, 2010 at 08:12:05PM +0200, Eric Lescouet wrote:
> Greg KH wrote:
> [...]
> >Ok, but does the lock_kernel() call you just made actually prevent this
> >from happening? Isn't there some other lock you can use instead?
> >
>
> Right. lock_kernel() is not needed (was copy/paste).
> The deadlock is avoided by not calling wait_for_completion()
> when the code is executed by the "eh" thread itself.
>
> >Also, to have patches that we can apply, we need a "Signed-off-by:"
> >line. See the file, Documentation/SubmittingPatches for what this
> >means.
>
> Understood.
>
> >Your email client wrapped the patch and made it unappliable :(
> >
> >thanks,
> >
> >greg k-h
>
> Sorry about that. Please find below an hopefully better one.
> Thanks,
> Eric.
>
>
> Signed-off-by: Eric Lescouet <[email protected]>
> -------------
>
> diff -Nur linux-2.6.34-rc5/drivers/staging/usbip/usbip_event.c
> linux-2.6.34-rc5.new/drivers/staging/usbip/usbip_event.c
> --- linux-2.6.34-rc5/drivers/staging/usbip/usbip_event.c
> 2010-04-20 01:29:56.000000000 +0200
> +++ linux-2.6.34-rc5.new/drivers/staging/usbip/usbip_event.c
> 2010-04-22 19:19:52.997889126 +0200
> @@ -117,6 +117,9 @@
> {
> struct usbip_task *eh = &ud->eh;
> + if (eh->thread == current)
> + return; /* do not wait for myself */
> +
> wait_for_completion(&eh->thread_done);
> usbip_dbg_eh("usbip_eh has finished\n");
Odd, the patch has all leading tabs and spaces stripped out of it, is
linewrapped, and the tabs are converted to spaces :(
Care to try again, third time's a charm...
thanks,
greg k-h
> Odd, the patch has all leading tabs and spaces stripped out of it, is
> linewrapped, and the tabs are converted to spaces :(
>
> Care to try again, third time's a charm...
OK. It looked better on my box and on the list as well ...
... but let's try another client.
Eric.
Signed-off-by: Eric Lescouet <[email protected]>
-------------
diff -Nur linux-2.6.34-rc5/drivers/staging/usbip/usbip_event.c linux-2.6.34-rc5.new/drivers/staging/usbip/usbip_event.c
--- linux-2.6.34-rc5/drivers/staging/usbip/usbip_event.c 2010-04-20 01:29:56.000000000 +0200
+++ linux-2.6.34-rc5.new/drivers/staging/usbip/usbip_event.c 2010-04-22 19:19:52.997889126 +0200
@@ -117,6 +117,9 @@
{
struct usbip_task *eh = &ud->eh;
+ if (eh->thread == current)
+ return; /* do not wait for myself */
+
wait_for_completion(&eh->thread_done);
usbip_dbg_eh("usbip_eh has finished\n");
}