The xpad_send_led_command() and xpad_play_effect() functions submit urb requests, but do not wait for the previous urb request to complete before using the same resources to submit a new request. This can lead to unpredictable undesirable effects, including memory corruption and dereferencing of NULL pointers(and needless to say, kernel panics).
Fix the issue by introducing a busy flag set on submission of the urb request, and cleared on urb request completion. If this flag is set while in xpad_send_led_command() or xpad_play_effect(), the led/rumble packet data is buffered, and will be sent from the urb request completion routine when the current urb request finishes. Patch tested with rumble against a Logitech F510 and an X-Box v1 pad. Signed-off-by: Sarah Bessmer <[email protected]> --- v2: -fixed introduced race in xpad_irq_out() --- linux-3.14.4/drivers/input/joystick/xpad.c.orig 2014-05-15 13:13:39.000000000 -0700 +++ linux-3.14.4/drivers/input/joystick/xpad.c 2014-05-16 02:00:56.000000000 -0700 @@ -78,6 +78,7 @@ #include <linux/stat.h> #include <linux/module.h> #include <linux/usb/input.h> +#include <linux/spinlock.h> #define DRIVER_AUTHOR "Marko Friedemann <[email protected]>" #define DRIVER_DESC "X-Box pad driver" @@ -282,7 +283,15 @@ struct usb_xpad { struct urb *irq_out; /* urb for interrupt out report */ unsigned char *odata; /* output data */ dma_addr_t odata_dma; - struct mutex odata_mutex; + int odata_busy; + + spinlock_t pend_lock; + + unsigned pend_rum; + unsigned char rum_data[XPAD_PKT_LEN]; + + unsigned pend_led; + unsigned char led_data[XPAD_PKT_LEN]; #endif #if defined(CONFIG_JOYSTICK_XPAD_LEDS) @@ -541,12 +550,37 @@ static void xpad_irq_out(struct urb *urb struct usb_xpad *xpad = urb->context; struct device *dev = &xpad->intf->dev; int retval, status; + unsigned long flags; status = urb->status; switch (status) { case 0: /* success */ + spin_lock_irqsave(&xpad->pend_lock, flags); + xpad->irq_out->transfer_buffer_length = 0; + + if (xpad->pend_rum != 0) { + memcpy(xpad->odata, xpad->rum_data, xpad->pend_rum); + xpad->irq_out->transfer_buffer_length = xpad->pend_rum; + xpad->pend_rum = 0; + } else if (xpad->pend_led != 0) { + memcpy(xpad->odata, xpad->led_data, xpad->pend_led); + xpad->irq_out->transfer_buffer_length = xpad->pend_led; + xpad->pend_led = 0; + } + + if (xpad->irq_out->transfer_buffer_length != 0) { + spin_unlock_irqrestore(&xpad->pend_lock, flags); + if (usb_submit_urb(xpad->irq_out, GFP_ATOMIC) != 0) { + spin_lock_irqsave(&xpad->pend_lock, flags); + xpad->odata_busy = 0; + spin_unlock_irqrestore(&xpad->pend_lock, flags); + } + } else { + xpad->odata_busy = 0; + spin_unlock_irqrestore(&xpad->pend_lock, flags); + } return; case -ECONNRESET: @@ -555,19 +589,27 @@ static void xpad_irq_out(struct urb *urb /* this urb is terminated, clean up */ dev_dbg(dev, "%s - urb shutting down with status: %d\n", __func__, status); + + spin_lock_irqsave(&xpad->pend_lock, flags); + xpad->odata_busy = 0; + spin_unlock_irqrestore(&xpad->pend_lock, flags); return; default: dev_dbg(dev, "%s - nonzero urb status received: %d\n", __func__, status); - goto exit; - } -exit: - retval = usb_submit_urb(urb, GFP_ATOMIC); - if (retval) - dev_err(dev, "%s - usb_submit_urb failed with result %d\n", - __func__, retval); + retval = usb_submit_urb(urb, GFP_ATOMIC); + if (retval) { + dev_err(dev, "%s - usb_submit_urb failed with result %d\n", + __func__, retval); + spin_lock_irqsave(&xpad->pend_lock, flags); + xpad->odata_busy = 0; + spin_unlock_irqrestore(&xpad->pend_lock, flags); + } + + return; + } } static int xpad_init_output(struct usb_interface *intf, struct usb_xpad *xpad) @@ -585,7 +627,12 @@ static int xpad_init_output(struct usb_i goto fail1; } - mutex_init(&xpad->odata_mutex); + xpad->odata_busy = 0; + + spin_lock_init(&xpad->pend_lock); + + xpad->pend_rum = 0; + xpad->pend_led = 0; xpad->irq_out = usb_alloc_urb(0, GFP_KERNEL); if (!xpad->irq_out) { @@ -628,61 +675,91 @@ static void xpad_stop_output(struct usb_ #endif #ifdef CONFIG_JOYSTICK_XPAD_FF +static int xpad_make_rum_data(struct usb_xpad *xpad, __u16 strong, __u16 weak) +{ + switch (xpad->xtype) { + + case XTYPE_XBOX: + xpad->rum_data[0] = 0x00; + xpad->rum_data[1] = 0x06; + xpad->rum_data[2] = 0x00; + xpad->rum_data[3] = strong / 256; /* left actuator */ + xpad->rum_data[4] = 0x00; + xpad->rum_data[5] = weak / 256; /* right actuator */ + xpad->pend_rum = 6; + return 0; + + + case XTYPE_XBOX360: + xpad->rum_data[0] = 0x00; + xpad->rum_data[1] = 0x08; + xpad->rum_data[2] = 0x00; + xpad->rum_data[3] = strong / 256; /* left actuator? */ + xpad->rum_data[4] = weak / 256; /* right actuator? */ + xpad->rum_data[5] = 0x00; + xpad->rum_data[6] = 0x00; + xpad->rum_data[7] = 0x00; + xpad->pend_rum = 8; + return 0; + + case XTYPE_XBOX360W: + xpad->rum_data[0] = 0x00; + xpad->rum_data[1] = 0x01; + xpad->rum_data[2] = 0x0F; + xpad->rum_data[3] = 0xC0; + xpad->rum_data[4] = 0x00; + xpad->rum_data[5] = strong / 256; + xpad->rum_data[6] = weak / 256; + xpad->rum_data[7] = 0x00; + xpad->rum_data[8] = 0x00; + xpad->rum_data[9] = 0x00; + xpad->rum_data[10] = 0x00; + xpad->rum_data[11] = 0x00; + xpad->pend_rum = 12; + return 0; + + } + return -1; +} + static int xpad_play_effect(struct input_dev *dev, void *data, struct ff_effect *effect) { struct usb_xpad *xpad = input_get_drvdata(dev); if (effect->type == FF_RUMBLE) { - __u16 strong = effect->u.rumble.strong_magnitude; - __u16 weak = effect->u.rumble.weak_magnitude; - - switch (xpad->xtype) { + unsigned long flags; + int mrdrv; - case XTYPE_XBOX: - xpad->odata[0] = 0x00; - xpad->odata[1] = 0x06; - xpad->odata[2] = 0x00; - xpad->odata[3] = strong / 256; /* left actuator */ - xpad->odata[4] = 0x00; - xpad->odata[5] = weak / 256; /* right actuator */ - xpad->irq_out->transfer_buffer_length = 6; - - return usb_submit_urb(xpad->irq_out, GFP_ATOMIC); - - case XTYPE_XBOX360: - xpad->odata[0] = 0x00; - xpad->odata[1] = 0x08; - xpad->odata[2] = 0x00; - xpad->odata[3] = strong / 256; /* left actuator? */ - xpad->odata[4] = weak / 256; /* 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_ATOMIC); - - case XTYPE_XBOX360W: - xpad->odata[0] = 0x00; - xpad->odata[1] = 0x01; - xpad->odata[2] = 0x0F; - xpad->odata[3] = 0xC0; - xpad->odata[4] = 0x00; - xpad->odata[5] = strong / 256; - xpad->odata[6] = weak / 256; - xpad->odata[7] = 0x00; - xpad->odata[8] = 0x00; - xpad->odata[9] = 0x00; - xpad->odata[10] = 0x00; - xpad->odata[11] = 0x00; - xpad->irq_out->transfer_buffer_length = 12; - - return usb_submit_urb(xpad->irq_out, GFP_ATOMIC); + spin_lock_irqsave(&xpad->pend_lock, flags); + mrdrv = xpad_make_rum_data(xpad, effect->u.rumble.strong_magnitude, + effect->u.rumble.weak_magnitude); + + if (mrdrv == 0 && !xpad->odata_busy) { + memcpy(xpad->odata, xpad->rum_data, xpad->pend_rum); + xpad->irq_out->transfer_buffer_length = xpad->pend_rum; + xpad->pend_rum = 0; + xpad->odata_busy = 1; + spin_unlock_irqrestore(&xpad->pend_lock, flags); + + if (usb_submit_urb(xpad->irq_out, GFP_ATOMIC) != 0) { + spin_lock_irqsave(&xpad->pend_lock, flags); + xpad->odata_busy = 0; + spin_unlock_irqrestore(&xpad->pend_lock, flags); + } + } else { + spin_unlock_irqrestore(&xpad->pend_lock, flags); + + if (mrdrv == 0) { + dev_dbg(&xpad->dev->dev, + "%s - rumble while urb busy\n", __func__); + } + } - default: + if (mrdrv != 0) { dev_dbg(&xpad->dev->dev, "%s - rumble command sent to unsupported xpad type: %d\n", __func__, xpad->xtype); + return -1; } } @@ -716,13 +793,30 @@ struct xpad_led { static void xpad_send_led_command(struct usb_xpad *xpad, int command) { if (command >= 0 && command < 14) { - mutex_lock(&xpad->odata_mutex); - xpad->odata[0] = 0x01; - xpad->odata[1] = 0x03; - xpad->odata[2] = command; - xpad->irq_out->transfer_buffer_length = 3; - usb_submit_urb(xpad->irq_out, GFP_KERNEL); - mutex_unlock(&xpad->odata_mutex); + unsigned long flags; + + spin_lock_irqsave(&xpad->pend_lock, flags); + xpad->led_data[0] = 0x01; + xpad->led_data[1] = 0x03; + xpad->led_data[2] = command; + xpad->pend_led = 3; + + if (!xpad->odata_busy) { + memcpy(xpad->odata, xpad->led_data, xpad->pend_led); + xpad->irq_out->transfer_buffer_length = xpad->pend_led; + xpad->pend_led = 0; + xpad->odata_busy = 1; + spin_unlock_irqrestore(&xpad->pend_lock, flags); + + if (usb_submit_urb(xpad->irq_out, GFP_KERNEL) != 0) { + spin_lock_irqsave(&xpad->pend_lock, flags); + xpad->odata_busy = 0; + spin_unlock_irqrestore(&xpad->pend_lock, flags); + } + } else { + spin_unlock_irqrestore(&xpad->pend_lock, flags); + dev_dbg(&xpad->dev->dev, "%s - led while urb busy\n", __func__); + } } } -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
