Re: [PATCH] drivers/input/joystick/xpad.c: Add rumble support for original xbox controller

[Benjamin Valentin <benpicco@zedat.fu-berlin.de>]

[-- Attachment #1: Type: text/plain, Size: 4517 bytes --]

On Sat, 9 Jan 2010 23:56:16 -0800
Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:

> > --- /usr/src/linux-source-2.6.33/drivers/input/joystick/xpad.c
> > 2010-01-08 02:56:59.365851076 +0100 +++ xpad.c	2010-01-08
> > 03:13:38.477835651 +0100 @@ -505,7 +505,7 @@
> >  	struct usb_endpoint_descriptor *ep_irq_out;
> >  	int error = -ENOMEM;
> >  
> > -	if (xpad->xtype != XTYPE_XBOX360)
> > +	if (xpad->xtype != XTYPE_XBOX360 && xpad->xtype !=
> > XTYPE_XBOX) return 0;
> >  
> >  	xpad->odata = usb_buffer_alloc(xpad->udev, XPAD_PKT_LEN,
> > @@ -535,13 +535,13 @@
> >  
> >  static void xpad_stop_output(struct usb_xpad *xpad)
> >  {
> > -	if (xpad->xtype == XTYPE_XBOX360)
> > +	if (xpad->xtype == XTYPE_XBOX360 || xpad->xtype !=
> > XTYPE_XBOX)
> 
> This should cretainly be "... || xpad->xtype == XTYPE_XBOX)", I'll fix
> it up locally.

Thank you, this made me discover another bug that eventually leads to a
kernel oops when the device is unplugged while an effect is playing
(or the effect is somehow else interrupted).
This should be fixed by taking the mutex when modifying xpad->odata as
well as checking whether it has been freed before writing to it.

Signed-off-by: Benjamin Valentin <benpicco@zedat.fu-berlin.de>

diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
--- a/drivers/input/joystick/xpad.c
+++ b/drivers/input/joystick/xpad.c
@@ -535,16 +535,24 @@
 
 static void xpad_stop_output(struct usb_xpad *xpad)
 {
-	if (xpad->xtype == XTYPE_XBOX360 || xpad->xtype != XTYPE_XBOX)
-		usb_kill_urb(xpad->irq_out);
+	if (xpad->xtype != XTYPE_XBOX360 || xpad->xtype != XTYPE_XBOX)
+		return;
+		
+	mutex_lock(&xpad->odata_mutex);
+	usb_kill_urb(xpad->irq_out);
+	mutex_unlock(&xpad->odata_mutex);
 }
 
 static void xpad_deinit_output(struct usb_xpad *xpad)
 {
-	if (xpad->xtype == XTYPE_XBOX360 || xpad->xtype != XTYPE_XBOX) {
+	if (xpad->xtype == XTYPE_XBOX360 || xpad->xtype == XTYPE_XBOX) {
+		mutex_lock(&xpad->odata_mutex);
+		
 		usb_free_urb(xpad->irq_out);
-		usb_buffer_free(xpad->udev, XPAD_PKT_LEN,
+		usb_buffer_free(xpad->udev, XPAD_PKT_LEN, 
 				xpad->odata, xpad->odata_dma);
+				
+		mutex_unlock(&xpad->odata_mutex);		
 	}
 }
 #else
@@ -555,32 +563,46 @@
 
 #ifdef CONFIG_JOYSTICK_XPAD_FF
 static int xpad_send_rumble(struct usb_xpad *xpad, unsigned char left, unsigned char right) {
-	switch(xpad->xtype) {
-		case XTYPE_XBOX:
-			xpad->odata[0] = 0x00;
-			xpad->odata[1] = 0x06;
-			xpad->odata[2] = 0x00;
-			xpad->odata[3] = left;  // left actuator
-			xpad->odata[4] = 0x00;
-			xpad->odata[5] = right; // right actuator
-			xpad->irq_out->transfer_buffer_length = 6;
-			return usb_submit_urb(xpad->irq_out, GFP_KERNEL);
-		case XTYPE_XBOX360:
-			xpad->odata[0] = 0x00;
-			xpad->odata[1] = 0x08;
-			xpad->odata[2] = 0x00;
-			xpad->odata[3] = left;  // left actuator?
-			xpad->odata[4] = right; // right actuator?
-			xpad->odata[5] = 0x00;
-			xpad->odata[6] = 0x00;
-			xpad->odata[7] = 0x00;
-			xpad->irq_out->transfer_buffer_length = 8;
-			return usb_submit_urb(xpad->irq_out, GFP_KERNEL);
-		default:
-			dbg("%s - rumble command sent to unsupported xpad type: %d", 
-				__func__, xpad->xtype);
-			return 0;
+	int result = 0;
+	
+	mutex_lock(&xpad->odata_mutex);	
+	if(xpad->odata) {		
+		switch(xpad->xtype) {
+			case XTYPE_XBOX:
+				xpad->odata[0] = 0x00;
+				xpad->odata[1] = 0x06;
+				xpad->odata[2] = 0x00;
+				xpad->odata[3] = left;  // left actuator
+				xpad->odata[4] = 0x00;
+				xpad->odata[5] = right; // right actuator
+				xpad->irq_out->transfer_buffer_length = 6;
+				break;
+			case XTYPE_XBOX360:
+				xpad->odata[0] = 0x00;
+				xpad->odata[1] = 0x08;
+				xpad->odata[2] = 0x00;
+				xpad->odata[3] = left;  // left actuator?
+				xpad->odata[4] = right; // right actuator?
+				xpad->odata[5] = 0x00;
+				xpad->odata[6] = 0x00;
+				xpad->odata[7] = 0x00;
+				xpad->irq_out->transfer_buffer_length = 8;			
+				break;
+			default:
+				dbg("%s - rumble command sent to unsupported xpad type: %d", 
+					__func__, xpad->xtype);
+				result = -1;
+			}
+		} else {
+			dbg("%s - xpad->odata already freed", __func__);
+			result = -1;
 		}
+		
+		if(result == 0)
+			result = usb_submit_urb(xpad->irq_out, GFP_KERNEL);
+			
+		mutex_unlock(&xpad->odata_mutex);		
+		return result;
 }
 
 static int xpad_play_effect(struct input_dev *dev, void *data, struct ff_effect *effect)


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 197 bytes --]