[PATCH] usbmouse: random freeze/hangup; synchronize irq completion

[PATCH] usbmouse: random freeze/hangup; synchronize irq completion

[Jordi Pujol]

[-- Attachment #1: Type: Text/Plain, Size: 289 bytes --]

Hello,

usbmouse: synchonize the irq completion for each urb.

Without this patch the mouse was a bit erratic and when
using the mouse the system gets freezed/hangup randomly (one or two times a 
day).

Jordi Pujol

Live never ending Tale
GNU/Linux Live forever!
http://livenet.selfip.com

[-- Attachment #2: usb_mouse_synchronize irq_urb.patch --]
[-- Type: text/x-patch, Size: 2008 bytes --]

 usbmouse: synchonize the irq completion

Signed-off-by: Jordi Pujol <jordipujolp AT gmail DOT com>

--- linux-2.6.37-old/include/linux/usb.h	2011-01-05 01:50:19.000000000 +0100
+++ linux-2.6.37/include/linux/usb.h	2011-02-10 09:31:30.098752709 +0100
@@ -1214,6 +1214,7 @@ struct urb {
 	int error_count;		/* (return) number of ISO errors */
 	void *context;			/* (in) context for completion */
 	usb_complete_t complete;	/* (in) completion routine */
+	struct mutex		complete_mutex;		/* synchronize completion */
 	struct usb_iso_packet_descriptor iso_frame_desc[0];
 					/* (in) ISO ONLY */
 };
@@ -1250,6 +1251,7 @@ static inline void usb_fill_control_urb(
 	urb->transfer_buffer_length = buffer_length;
 	urb->complete = complete_fn;
 	urb->context = context;
+	mutex_init(&urb->complete_mutex);
 }

 /**
@@ -1279,6 +1281,7 @@ static inline void usb_fill_bulk_urb(str
 	urb->transfer_buffer_length = buffer_length;
 	urb->complete = complete_fn;
 	urb->context = context;
+	mutex_init(&urb->complete_mutex);
 }

 /**
@@ -1326,6 +1329,7 @@ static inline void usb_fill_int_urb(stru
 	else
 		urb->interval = interval;
 	urb->start_frame = -1;
+	mutex_init(&urb->complete_mutex);
 }

 extern void usb_init_urb(struct urb *urb);
--- linux-2.6.37-old/drivers/hid/usbhid/usbmouse.c
+++ linux-2.6.37/drivers/hid/usbhid/usbmouse.c	2011-02-06 16:15:10.432559673 +0100
@@ -66,13 +71,15 @@ static void usb_mouse_irq(struct urb *ur
 	struct input_dev *dev = mouse->dev;
 	int status;

+	mutex_lock(&urb->complete_mutex);
+
 	switch (urb->status) {
 	case 0:			/* success */
 		break;
 	case -ECONNRESET:	/* unlink */
 	case -ENOENT:
 	case -ESHUTDOWN:
-		return;
+		goto out;
 	/* -EPIPE:  should clear the halt */
 	default:		/* error */
 		goto resubmit;
@@ -95,6 +102,9 @@ resubmit:
 		err ("can't resubmit intr, %s-%s/input0, status %d",
 				mouse->usbdev->bus->bus_name,
 				mouse->usbdev->devpath, status);
+
+out:
+	mutex_unlock(&urb->complete_mutex);
 }

 static int usb_mouse_open(struct input_dev *dev)

Re: [PATCH] usbmouse: random freeze/hangup; synchronize irq completion

[Alan Stern]

On Mon, 14 Feb 2011, Jordi Pujol wrote:

> Hello,
> 
> usbmouse: synchonize the irq completion for each urb.
> 
> Without this patch the mouse was a bit erratic and when
> using the mouse the system gets freezed/hangup randomly (one or two times a 
> day).

Your patch is no good, for quite a few reasons.  I'll state only two of 
them:

	It calls mutex_lock(), which can sleep, from within an URB
	completion routine, which runs with interrupts disabled.

	It doesn't synchronize the completion _with_ anything else,
	i.e., it calls mutex_lock() in only one place.

Whatever is causing your problem, this is not the proper solution.

Alan Stern

Re: [PATCH] usbmouse: random freeze/hangup; synchronize irq completion

[Jordi Pujol]

A Dilluns 14 Febrer 2011 16:00:24, Alan Stern va escriure:
> 	It calls mutex_lock(), which can sleep, from within an URB
> 	completion routine, which runs with interrupts disabled.
> 
> 	It doesn't synchronize the completion _with_ anything else,
> 	i.e., it calls mutex_lock() in only one place.
this routine is synchronized with himself.
I said synchronize thinking about synchronized routines that are pieces of 
code scheduled to be executed only in a single thread simultaneously; 
therefore a new call to this routine can not be executed until a previous call 
has been completed.

> 
> Whatever is causing your problem, this is not the proper solution.
Yes, that this is not a patch to be included in the kernel, it's only a 
testing patch, but it solves the hangup. and so this probes that there is some 
kind of problem related to the urb update process; I think the problem should 
be external to this routine.

searching the web, some people has found problems like this, example:
- [ubuntu] Ubuntu 10.04 (Lucid Lynx) Random Freeze / Hang-up

Consider this as an idea to point out the problem and, if possible, someone 
more expert help to look for a good solution.

Jordi Pujol

Live never ending Tale
GNU/Linux Live forever!
http://livenet.selfip.com

Re: [PATCH] usbmouse: random freeze/hangup; synchronize irq completion

[Alan Stern]

On Tue, 15 Feb 2011, Jordi Pujol wrote:

> A Dilluns 14 Febrer 2011 16:00:24, Alan Stern va escriure:
> > 	It calls mutex_lock(), which can sleep, from within an URB
> > 	completion routine, which runs with interrupts disabled.
> > 
> > 	It doesn't synchronize the completion _with_ anything else,
> > 	i.e., it calls mutex_lock() in only one place.
> this routine is synchronized with himself.

No.  The code you added does not perform _any_ additional 
synchronization.  If it did, you'd see a big fat error message in the 
kernel log because the mutex_lock call would try to sleep while in 
interrupt context.

> I said synchronize thinking about synchronized routines that are pieces of 
> code scheduled to be executed only in a single thread simultaneously; 
> therefore a new call to this routine can not be executed until a previous call 
> has been completed.

That is true even without your patch.  Go ahead and add a pair of
debugging printouts to the usb_mouse_irq() routine (without your
patch); print the value of urb upon entry and exit.  You'll see that
there are never two instances of the routine running comcurrently for
the same URB.  In fact, if you have only one USB mouse, you'll see that
there are never two instances of the routine running concurrently at
all.

> > Whatever is causing your problem, this is not the proper solution.
> Yes, that this is not a patch to be included in the kernel, it's only a 
> testing patch, but it solves the hangup. and so this probes that there is some 
> kind of problem related to the urb update process; I think the problem should 
> be external to this routine.
> 
> searching the web, some people has found problems like this, example:
> - [ubuntu] Ubuntu 10.04 (Lucid Lynx) Random Freeze / Hang-up
> 
> Consider this as an idea to point out the problem and, if possible, someone 
> more expert help to look for a good solution.

More to the point, why are you using the usbmouse driver in the first
place?  You should be using usbhid instead.  See the warnings in the
Kconfig help text for USB_MOUSE.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usbmouse: random freeze/hangup; synchronize irq completion

[Greg KH]

On Mon, Feb 14, 2011 at 09:51:00AM +0100, Jordi Pujol wrote:
> Hello,
> 
> usbmouse: synchonize the irq completion for each urb.
> 
> Without this patch the mouse was a bit erratic and when
> using the mouse the system gets freezed/hangup randomly (one or two times a 
> day).
> 
> Jordi Pujol
> 
> Live never ending Tale
> GNU/Linux Live forever!
> http://livenet.selfip.com

>  usbmouse: synchonize the irq completion
> 
> Signed-off-by: Jordi Pujol <jordipujolp AT gmail DOT com>
> 
> --- linux-2.6.37-old/include/linux/usb.h	2011-01-05 01:50:19.000000000 +0100
> +++ linux-2.6.37/include/linux/usb.h	2011-02-10 09:31:30.098752709 +0100
> @@ -1214,6 +1214,7 @@ struct urb {
>  	int error_count;		/* (return) number of ISO errors */
>  	void *context;			/* (in) context for completion */
>  	usb_complete_t complete;	/* (in) completion routine */
> +	struct mutex		complete_mutex;		/* synchronize completion */

Ick, no, this should never be needed.

Please explain why the existing procedure for this isn't working for
you?

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html