[PATCH 0/8] Input: xpad: fix wireless pad connection and URB out

[PATCH 0/8] Input: xpad: fix wireless pad connection and URB out

[Pavel Rojtberg]

From: Pavel Rojtberg <rojtberg@gmail.com>

This series tries to finish the work of:
http://www.spinics.net/lists/linux-input/msg29445.html
Citing the original submission:
Rework the xpad driver to fix the issue where when a wireless xpad controller
is plugged in, 4 joystick devices are created, no matter how many are really 
attached to the system.
This is done by dynamically creating the devices only when they are found 
by the wireless receiver.
Along the way all usages of the out URB were guarded by the mutex and a
active flag was introduced to prevent usage while active.
This makes outbound communication more robust. (LED, FF, presence query)

Patches 1-2 clean up the x360 LED code after:
http://www.spinics.net/lists/linux-input/msg39147.html
partifularly the LED command on x360w pads is not submitted twice any more.
They are the same as already submitted here:
http://www.spinics.net/lists/linux-input/msg39438.html

Patches 3-6 implement the actual "on demand" creation/ deletion of input 
devices. To this end pad enumeration had to be changed from an monotonic
counter to a bitmask based counter. See Patch 6 for rationale.

Patches 7-8 prevent sending active URBs. This was alrady an issue before, but
is now more pressing as we always send the query packet on driver load. (x360w)

Pavel Rojtberg (6):
  Input: xpad: clarify LED enumeration
  Input: xpad: remove bulk out URB
  Input: xpad: query Wireless controller state at init
  Input: xpad: use bitmask for finding the pad_nr
  Input: xpad: factor out URB submission in xpad_play_effect
  Input: xpad: do not submit active URBs

Pierre-Loup A. Griffais (2):
  Input: xpad: move the input device creation to a new function
  Input: xpad: handle "present" and "gone" correctly

 xpad.c | 539 +++++++++++++++++++++++++++++++++++------------------------------
 1 file changed, 290 insertions(+), 249 deletions(-)

-- 
1.9.1

[PATCH 1/8] Input: xpad: clarify LED enumeration

[Pavel Rojtberg]

From: Pavel Rojtberg <rojtberg@gmail.com>

this one is pure cosmetic, however it helps understanding the code.
1. rename led_no -> pad_nr as the number stored there just gets
translated to a LED nr in xpad_identify_controller
2. move all comments regarding xpad_identify_controller to the function
definition to prevent inconsistent docs.

Signed-off-by: Pavel Rojtberg <rojtberg@gmail.com>
---
 drivers/input/joystick/xpad.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
index f8850f9..8892daf 100644
--- a/drivers/input/joystick/xpad.c
+++ b/drivers/input/joystick/xpad.c
@@ -344,7 +344,7 @@ struct usb_xpad {
 
 	int mapping;			/* map d-pad to buttons or to axes */
 	int xtype;			/* type of xbox device */
-	unsigned long led_no;		/* led to lit on xbox360 controllers */
+	unsigned long pad_nr;		/* the order x360 pads were attached */
 };
 
 /*
@@ -356,7 +356,6 @@ struct usb_xpad {
  *	The used report descriptor was taken from ITO Takayukis website:
  *	 http://euc.jp/periphs/xbox-controller.ja.html
  */
-
 static void xpad_process_packet(struct usb_xpad *xpad, u16 cmd, unsigned char *data)
 {
 	struct input_dev *dev = xpad->dev;
@@ -505,7 +504,6 @@ static void xpad_identify_controller(struct usb_xpad *xpad);
  * 01.1 - Pad state (Bytes 4+) valid
  *
  */
-
 static void xpad360w_process_packet(struct usb_xpad *xpad, u16 cmd, unsigned char *data)
 {
 	/* Presence change */
@@ -513,10 +511,6 @@ static void xpad360w_process_packet(struct usb_xpad *xpad, u16 cmd, unsigned cha
 		if (data[1] & 0x80) {
 			xpad->pad_present = 1;
 			usb_submit_urb(xpad->bulk_out, GFP_ATOMIC);
-			/*
-			 * Light up the segment corresponding to
-			 * controller number.
-			 */
 			xpad_identify_controller(xpad);
 		} else
 			xpad->pad_present = 0;
@@ -890,6 +884,7 @@ struct xpad_led {
 };
 
 /**
+ * set the LEDs on Xbox360 / Wireless Controllers
  * @param command
  *  0: off
  *  1: all blink, then previous setting
@@ -942,10 +937,12 @@ static void xpad_send_led_command(struct usb_xpad *xpad, int command)
 	mutex_unlock(&xpad->odata_mutex);
 }
 
+/*
+ * Light up the segment corresponding to the pad number on Xbox 360 Controllers
+ */
 static void xpad_identify_controller(struct usb_xpad *xpad)
 {
-	/* Light up the segment corresponding to controller number */
-	xpad_send_led_command(xpad, (xpad->led_no % 4) + 2);
+	xpad_send_led_command(xpad, (xpad->pad_nr % 4) + 2);
 }
 
 static void xpad_led_set(struct led_classdev *led_cdev,
@@ -971,9 +968,9 @@ static int xpad_led_probe(struct usb_xpad *xpad)
 	if (!led)
 		return -ENOMEM;
 
-	xpad->led_no = atomic_inc_return(&led_seq);
+	xpad->pad_nr = atomic_inc_return(&led_seq);
 
-	snprintf(led->name, sizeof(led->name), "xpad%lu", xpad->led_no);
+	snprintf(led->name, sizeof(led->name), "xpad%lu", xpad->pad_nr);
 	led->xpad = xpad;
 
 	led_cdev = &led->led_cdev;
@@ -987,7 +984,6 @@ static int xpad_led_probe(struct usb_xpad *xpad)
 		return error;
 	}
 
-	/* Light up the segment corresponding to controller number */
 	xpad_identify_controller(xpad);
 
 	return 0;
-- 
1.9.1

[PATCH 2/8] Input: xpad: remove bulk out URB

[Pavel Rojtberg]

From: Pavel Rojtberg <rojtberg@gmail.com>

this code is now redundand with xpad_send_led_command which does the
same thing using irq_out.
Furthermore it was probably wrong from the start as setting the LEDs did
not work before completing the xbox360 wireless package in
xpad_send_led_command.
Note that this change only affects the two supported wireless
controllers. Tested using the xbox360 wireless controller (PC).

Signed-off-by: Pavel Rojtberg <rojtberg@gmail.com>
---
 drivers/input/joystick/xpad.c | 79 +-----------------------------------------------------------------
 1 file changed, 1 insertion(+), 78 deletions(-)

diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
index 8892daf..b59663f 100644
--- a/drivers/input/joystick/xpad.c
+++ b/drivers/input/joystick/xpad.c
@@ -328,9 +328,6 @@ struct usb_xpad {
 	unsigned char *idata;		/* input data */
 	dma_addr_t idata_dma;
 
-	struct urb *bulk_out;
-	unsigned char *bdata;
-
 	struct urb *irq_out;		/* urb for interrupt out report */
 	unsigned char *odata;		/* output data */
 	dma_addr_t odata_dma;
@@ -510,7 +507,6 @@ static void xpad360w_process_packet(struct usb_xpad *xpad, u16 cmd, unsigned cha
 	if (data[0] & 0x08) {
 		if (data[1] & 0x80) {
 			xpad->pad_present = 1;
-			usb_submit_urb(xpad->bulk_out, GFP_ATOMIC);
 			xpad_identify_controller(xpad);
 		} else
 			xpad->pad_present = 0;
@@ -668,28 +664,6 @@ exit:
 			__func__, retval);
 }
 
-static void xpad_bulk_out(struct urb *urb)
-{
-	struct usb_xpad *xpad = urb->context;
-	struct device *dev = &xpad->intf->dev;
-
-	switch (urb->status) {
-	case 0:
-		/* success */
-		break;
-	case -ECONNRESET:
-	case -ENOENT:
-	case -ESHUTDOWN:
-		/* this urb is terminated, clean up */
-		dev_dbg(dev, "%s - urb shutting down with status: %d\n",
-			__func__, urb->status);
-		break;
-	default:
-		dev_dbg(dev, "%s - nonzero urb status received: %d\n",
-			__func__, urb->status);
-	}
-}
-
 static void xpad_irq_out(struct urb *urb)
 {
 	struct usb_xpad *xpad = urb->context;
@@ -1215,52 +1189,6 @@ static int xpad_probe(struct usb_interface *intf, const struct usb_device_id *id
 
 	if (xpad->xtype == XTYPE_XBOX360W) {
 		/*
-		 * Setup the message to set the LEDs on the
-		 * controller when it shows up
-		 */
-		xpad->bulk_out = usb_alloc_urb(0, GFP_KERNEL);
-		if (!xpad->bulk_out) {
-			error = -ENOMEM;
-			goto fail7;
-		}
-
-		xpad->bdata = kzalloc(XPAD_PKT_LEN, GFP_KERNEL);
-		if (!xpad->bdata) {
-			error = -ENOMEM;
-			goto fail8;
-		}
-
-		xpad->bdata[2] = 0x08;
-		switch (intf->cur_altsetting->desc.bInterfaceNumber) {
-		case 0:
-			xpad->bdata[3] = 0x42;
-			break;
-		case 2:
-			xpad->bdata[3] = 0x43;
-			break;
-		case 4:
-			xpad->bdata[3] = 0x44;
-			break;
-		case 6:
-			xpad->bdata[3] = 0x45;
-		}
-
-		ep_irq_in = &intf->cur_altsetting->endpoint[1].desc;
-		if (usb_endpoint_is_bulk_out(ep_irq_in)) {
-			usb_fill_bulk_urb(xpad->bulk_out, udev,
-					  usb_sndbulkpipe(udev,
-							  ep_irq_in->bEndpointAddress),
-					  xpad->bdata, XPAD_PKT_LEN,
-					  xpad_bulk_out, xpad);
-		} else {
-			usb_fill_int_urb(xpad->bulk_out, udev,
-					 usb_sndintpipe(udev,
-							ep_irq_in->bEndpointAddress),
-					 xpad->bdata, XPAD_PKT_LEN,
-					 xpad_bulk_out, xpad, 0);
-		}
-
-		/*
 		 * Submit the int URB immediately rather than waiting for open
 		 * because we get status messages from the device whether
 		 * or not any controllers are attached.  In fact, it's
@@ -1270,13 +1198,11 @@ static int xpad_probe(struct usb_interface *intf, const struct usb_device_id *id
 		xpad->irq_in->dev = xpad->udev;
 		error = usb_submit_urb(xpad->irq_in, GFP_KERNEL);
 		if (error)
-			goto fail9;
+			goto fail7;
 	}
 
 	return 0;
 
- fail9:	kfree(xpad->bdata);
- fail8:	usb_free_urb(xpad->bulk_out);
  fail7:	input_unregister_device(input_dev);
 	input_dev = NULL;
  fail6:	xpad_led_disconnect(xpad);
@@ -1300,8 +1226,6 @@ static void xpad_disconnect(struct usb_interface *intf)
 	xpad_deinit_output(xpad);
 
 	if (xpad->xtype == XTYPE_XBOX360W) {
-		usb_kill_urb(xpad->bulk_out);
-		usb_free_urb(xpad->bulk_out);
 		usb_kill_urb(xpad->irq_in);
 	}
 
@@ -1309,7 +1233,6 @@ static void xpad_disconnect(struct usb_interface *intf)
 	usb_free_coherent(xpad->udev, XPAD_PKT_LEN,
 			xpad->idata, xpad->idata_dma);
 
-	kfree(xpad->bdata);
 	kfree(xpad);
 
 	usb_set_intfdata(intf, NULL);
-- 
1.9.1

[PATCH 3/8] Input: xpad: move the input device creation to a new function

[Pavel Rojtberg]

From: "Pierre-Loup A. Griffais" <pgriffais@valvesoftware.com>

To allow us to later create / destroy the input device from the urb
callback, we need to initialize/ deinitialize the input device from a
separate function.  So pull that logic out now to make later patches
more "obvious" as to what they do.

Signed-off-by: "Pierre-Loup A. Griffais" <pgriffais@valvesoftware.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Pavel Rojtberg <rojtberg@gmail.com>
---
 drivers/input/joystick/xpad.c | 185 +++++++++++++++++++++++++++++++++++++----------------------------
 1 file changed, 106 insertions(+), 79 deletions(-)

diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
index b59663f..ed17e5d 100644
--- a/drivers/input/joystick/xpad.c
+++ b/drivers/input/joystick/xpad.c
@@ -342,6 +342,7 @@ struct usb_xpad {
 	int mapping;			/* map d-pad to buttons or to axes */
 	int xtype;			/* type of xbox device */
 	unsigned long pad_nr;		/* the order x360 pads were attached */
+	const char *name;		/* name of the device */
 };
 
 /*
@@ -1038,11 +1039,94 @@ static void xpad_set_up_abs(struct input_dev *input_dev, signed short abs)
 	}
 }
 
+static int xpad_init_input(struct usb_xpad *xpad)
+{
+	struct input_dev *input_dev;
+	int i, error;
+
+	input_dev = input_allocate_device();
+	if (!input_dev)
+		return -ENOMEM;
+
+	xpad->dev = input_dev;
+	input_dev->name = xpad->name;
+	input_dev->phys = xpad->phys;
+	usb_to_input_id(xpad->udev, &input_dev->id);
+	input_dev->dev.parent = &xpad->intf->dev;
+
+	input_set_drvdata(input_dev, xpad);
+
+	input_dev->open = xpad_open;
+	input_dev->close = xpad_close;
+
+	input_dev->evbit[0] = BIT_MASK(EV_KEY);
+
+	if (!(xpad->mapping & MAP_STICKS_TO_NULL)) {
+		input_dev->evbit[0] |= BIT_MASK(EV_ABS);
+		/* set up axes */
+		for (i = 0; xpad_abs[i] >= 0; i++)
+			xpad_set_up_abs(input_dev, xpad_abs[i]);
+	}
+
+	/* set up standard buttons */
+	for (i = 0; xpad_common_btn[i] >= 0; i++)
+		__set_bit(xpad_common_btn[i], input_dev->keybit);
+
+	/* set up model-specific ones */
+	if (xpad->xtype == XTYPE_XBOX360 || xpad->xtype == XTYPE_XBOX360W ||
+	    xpad->xtype == XTYPE_XBOXONE) {
+		for (i = 0; xpad360_btn[i] >= 0; i++)
+			__set_bit(xpad360_btn[i], input_dev->keybit);
+	} else {
+		for (i = 0; xpad_btn[i] >= 0; i++)
+			__set_bit(xpad_btn[i], input_dev->keybit);
+	}
+
+	if (xpad->mapping & MAP_DPAD_TO_BUTTONS) {
+		for (i = 0; xpad_btn_pad[i] >= 0; i++)
+			__set_bit(xpad_btn_pad[i], input_dev->keybit);
+	} else {
+		for (i = 0; xpad_abs_pad[i] >= 0; i++)
+			xpad_set_up_abs(input_dev, xpad_abs_pad[i]);
+	}
+
+	if (xpad->mapping & MAP_TRIGGERS_TO_BUTTONS) {
+		for (i = 0; xpad_btn_triggers[i] >= 0; i++)
+			__set_bit(xpad_btn_triggers[i], input_dev->keybit);
+	} else {
+		for (i = 0; xpad_abs_triggers[i] >= 0; i++)
+			xpad_set_up_abs(input_dev, xpad_abs_triggers[i]);
+	}
+
+	error = xpad_init_ff(xpad);
+	if (error)
+		goto fail_init_ff;
+
+	error = xpad_led_probe(xpad);
+	if (error)
+		goto fail_init_led;
+
+	error = input_register_device(xpad->dev);
+	if (error)
+		goto fail_input_register;
+
+	return 0;
+
+fail_input_register:
+	xpad_led_disconnect(xpad);
+
+fail_init_led:
+	input_ff_destroy(input_dev);
+
+fail_init_ff:
+	input_free_device(input_dev);
+	return error;
+}
+
 static int xpad_probe(struct usb_interface *intf, const struct usb_device_id *id)
 {
 	struct usb_device *udev = interface_to_usbdev(intf);
 	struct usb_xpad *xpad;
-	struct input_dev *input_dev;
 	struct usb_endpoint_descriptor *ep_irq_in;
 	int ep_irq_in_idx;
 	int i, error;
@@ -1064,12 +1148,14 @@ static int xpad_probe(struct usb_interface *intf, const struct usb_device_id *id
 	}
 
 	xpad = kzalloc(sizeof(struct usb_xpad), GFP_KERNEL);
-	input_dev = input_allocate_device();
-	if (!xpad || !input_dev) {
+	if (!xpad) {
 		error = -ENOMEM;
 		goto fail1;
 	}
 
+	usb_make_path(udev, xpad->phys, sizeof(xpad->phys));
+	strlcat(xpad->phys, "/input0", sizeof(xpad->phys));
+
 	xpad->idata = usb_alloc_coherent(udev, XPAD_PKT_LEN,
 					 GFP_KERNEL, &xpad->idata_dma);
 	if (!xpad->idata) {
@@ -1087,6 +1173,7 @@ static int xpad_probe(struct usb_interface *intf, const struct usb_device_id *id
 	xpad->intf = intf;
 	xpad->mapping = xpad_device[i].mapping;
 	xpad->xtype = xpad_device[i].xtype;
+	xpad->name = xpad_device[i].name;
 
 	if (xpad->xtype == XTYPE_UNKNOWN) {
 		if (intf->cur_altsetting->desc.bInterfaceClass == USB_CLASS_VENDOR_SPEC) {
@@ -1105,71 +1192,10 @@ static int xpad_probe(struct usb_interface *intf, const struct usb_device_id *id
 			xpad->mapping |= MAP_STICKS_TO_NULL;
 	}
 
-	xpad->dev = input_dev;
-	usb_make_path(udev, xpad->phys, sizeof(xpad->phys));
-	strlcat(xpad->phys, "/input0", sizeof(xpad->phys));
-
-	input_dev->name = xpad_device[i].name;
-	input_dev->phys = xpad->phys;
-	usb_to_input_id(udev, &input_dev->id);
-	input_dev->dev.parent = &intf->dev;
-
-	input_set_drvdata(input_dev, xpad);
-
-	input_dev->open = xpad_open;
-	input_dev->close = xpad_close;
-
-	input_dev->evbit[0] = BIT_MASK(EV_KEY);
-
-	if (!(xpad->mapping & MAP_STICKS_TO_NULL)) {
-		input_dev->evbit[0] |= BIT_MASK(EV_ABS);
-		/* set up axes */
-		for (i = 0; xpad_abs[i] >= 0; i++)
-			xpad_set_up_abs(input_dev, xpad_abs[i]);
-	}
-
-	/* set up standard buttons */
-	for (i = 0; xpad_common_btn[i] >= 0; i++)
-		__set_bit(xpad_common_btn[i], input_dev->keybit);
-
-	/* set up model-specific ones */
-	if (xpad->xtype == XTYPE_XBOX360 || xpad->xtype == XTYPE_XBOX360W ||
-	    xpad->xtype == XTYPE_XBOXONE) {
-		for (i = 0; xpad360_btn[i] >= 0; i++)
-			__set_bit(xpad360_btn[i], input_dev->keybit);
-	} else {
-		for (i = 0; xpad_btn[i] >= 0; i++)
-			__set_bit(xpad_btn[i], input_dev->keybit);
-	}
-
-	if (xpad->mapping & MAP_DPAD_TO_BUTTONS) {
-		for (i = 0; xpad_btn_pad[i] >= 0; i++)
-			__set_bit(xpad_btn_pad[i], input_dev->keybit);
-	} else {
-		for (i = 0; xpad_abs_pad[i] >= 0; i++)
-			xpad_set_up_abs(input_dev, xpad_abs_pad[i]);
-	}
-
-	if (xpad->mapping & MAP_TRIGGERS_TO_BUTTONS) {
-		for (i = 0; xpad_btn_triggers[i] >= 0; i++)
-			__set_bit(xpad_btn_triggers[i], input_dev->keybit);
-	} else {
-		for (i = 0; xpad_abs_triggers[i] >= 0; i++)
-			xpad_set_up_abs(input_dev, xpad_abs_triggers[i]);
-	}
-
 	error = xpad_init_output(intf, xpad);
 	if (error)
 		goto fail3;
 
-	error = xpad_init_ff(xpad);
-	if (error)
-		goto fail4;
-
-	error = xpad_led_probe(xpad);
-	if (error)
-		goto fail5;
-
 	/* Xbox One controller has in/out endpoints swapped. */
 	ep_irq_in_idx = xpad->xtype == XTYPE_XBOXONE ? 1 : 0;
 	ep_irq_in = &intf->cur_altsetting->endpoint[ep_irq_in_idx].desc;
@@ -1181,10 +1207,6 @@ static int xpad_probe(struct usb_interface *intf, const struct usb_device_id *id
 	xpad->irq_in->transfer_dma = xpad->idata_dma;
 	xpad->irq_in->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
 
-	error = input_register_device(xpad->dev);
-	if (error)
-		goto fail6;
-
 	usb_set_intfdata(intf, xpad);
 
 	if (xpad->xtype == XTYPE_XBOX360W) {
@@ -1197,32 +1219,37 @@ static int xpad_probe(struct usb_interface *intf, const struct usb_device_id *id
 		 */
 		xpad->irq_in->dev = xpad->udev;
 		error = usb_submit_urb(xpad->irq_in, GFP_KERNEL);
-		if (error)
-			goto fail7;
+		if (error) {
+			usb_kill_urb(xpad->irq_in);
+			goto fail4;
+		}
 	}
+	xpad->pad_present = 1;
+	error = xpad_init_input(xpad);
+	if (error)
+		goto fail4;
 
 	return 0;
 
- fail7:	input_unregister_device(input_dev);
-	input_dev = NULL;
- fail6:	xpad_led_disconnect(xpad);
- fail5:	if (input_dev)
-		input_ff_destroy(input_dev);
  fail4:	xpad_deinit_output(xpad);
  fail3:	usb_free_urb(xpad->irq_in);
  fail2:	usb_free_coherent(udev, XPAD_PKT_LEN, xpad->idata, xpad->idata_dma);
- fail1:	input_free_device(input_dev);
-	kfree(xpad);
+ fail1:	kfree(xpad);
 	return error;
 
 }
 
+static void xpad_deinit_input(struct usb_xpad *xpad)
+{
+	xpad_led_disconnect(xpad);
+	input_unregister_device(xpad->dev);
+}
+
 static void xpad_disconnect(struct usb_interface *intf)
 {
 	struct usb_xpad *xpad = usb_get_intfdata (intf);
 
-	xpad_led_disconnect(xpad);
-	input_unregister_device(xpad->dev);
+	xpad_deinit_input(xpad);
 	xpad_deinit_output(xpad);
 
 	if (xpad->xtype == XTYPE_XBOX360W) {
-- 
1.9.1

[PATCH 4/8] Input: xpad: query Wireless controller state at init

[Pavel Rojtberg]

From: Pavel Rojtberg <rojtberg@gmail.com>

We initializing the driver/device, we really don't know how many
controllers are connected.  So send a "query presence" command to the
base-station. (Command discovered by Zachary Lund)

Note, this means we now do not "preallocate" all 4 devices when a single
wireless base station is seen, but require the device to be properly
connected to the base station before that can happen.  The allocation of
the device happens in the next patch, not here, so in a way, this patch
breaks all wireless devices...

based on patch by
"Pierre-Loup A. Griffais" <pgriffais@valvesoftware.com>

presence packet taken from:
https://github.com/computerquip/xpad5

Signed-off-by: Pavel Rojtberg <rojtberg@gmail.com>
---
 drivers/input/joystick/xpad.c | 33 +++++++++++++++++++++++++++++----
 1 file changed, 29 insertions(+), 4 deletions(-)

diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
index ed17e5d..3349861 100644
--- a/drivers/input/joystick/xpad.c
+++ b/drivers/input/joystick/xpad.c
@@ -1223,11 +1223,36 @@ static int xpad_probe(struct usb_interface *intf, const struct usb_device_id *id
 			usb_kill_urb(xpad->irq_in);
 			goto fail4;
 		}
+
+		/*
+		 * send presence packet
+		 * This will force the controller to resend connection packets.
+		 * This is useful in the case we activate the module after the
+		 * adapter has been plugged in, as it won't automatically
+		 * send us info about the controllers.
+		 */
+		mutex_lock(&xpad->odata_mutex);
+		xpad->odata[0] = 0x08;
+		xpad->odata[1] = 0x00;
+		xpad->odata[2] = 0x0F;
+		xpad->odata[3] = 0xC0;
+		xpad->odata[4] = 0x00;
+		xpad->odata[5] = 0x00;
+		xpad->odata[6] = 0x00;
+		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;
+		usb_submit_urb(xpad->irq_out, GFP_KERNEL);
+		mutex_unlock(&xpad->odata_mutex);
+	} else {
+		xpad->pad_present = 1;
+		error = xpad_init_input(xpad);
+		if (error)
+			goto fail4;
 	}
-	xpad->pad_present = 1;
-	error = xpad_init_input(xpad);
-	if (error)
-		goto fail4;
 
 	return 0;
 
-- 
1.9.1

Re: [PATCH 4/8] Input: xpad: query Wireless controller state at init

[Dmitry Torokhov]

On Sat, Jul 11, 2015 at 01:47:44AM +0200, Pavel Rojtberg wrote:
> From: Pavel Rojtberg <rojtberg@gmail.com>
> 
> We initializing the driver/device, we really don't know how many
> controllers are connected.  So send a "query presence" command to the
> base-station. (Command discovered by Zachary Lund)
> 
> Note, this means we now do not "preallocate" all 4 devices when a single
> wireless base station is seen, but require the device to be properly
> connected to the base station before that can happen.  The allocation of
> the device happens in the next patch, not here, so in a way, this patch
> breaks all wireless devices...

Then we have to combine it with the one adding necessary handling.

> 
> based on patch by
> "Pierre-Loup A. Griffais" <pgriffais@valvesoftware.com>
> 
> presence packet taken from:
> https://github.com/computerquip/xpad5
> 
> Signed-off-by: Pavel Rojtberg <rojtberg@gmail.com>
> ---
>  drivers/input/joystick/xpad.c | 33 +++++++++++++++++++++++++++++----
>  1 file changed, 29 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
> index ed17e5d..3349861 100644
> --- a/drivers/input/joystick/xpad.c
> +++ b/drivers/input/joystick/xpad.c
> @@ -1223,11 +1223,36 @@ static int xpad_probe(struct usb_interface *intf, const struct usb_device_id *id
>  			usb_kill_urb(xpad->irq_in);
>  			goto fail4;
>  		}
> +
> +		/*
> +		 * send presence packet
> +		 * This will force the controller to resend connection packets.
> +		 * This is useful in the case we activate the module after the
> +		 * adapter has been plugged in, as it won't automatically
> +		 * send us info about the controllers.
> +		 */
> +		mutex_lock(&xpad->odata_mutex);
> +		xpad->odata[0] = 0x08;
> +		xpad->odata[1] = 0x00;
> +		xpad->odata[2] = 0x0F;
> +		xpad->odata[3] = 0xC0;
> +		xpad->odata[4] = 0x00;
> +		xpad->odata[5] = 0x00;
> +		xpad->odata[6] = 0x00;
> +		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;
> +		usb_submit_urb(xpad->irq_out, GFP_KERNEL);
> +		mutex_unlock(&xpad->odata_mutex);
> +	} else {
> +		xpad->pad_present = 1;
> +		error = xpad_init_input(xpad);
> +		if (error)
> +			goto fail4;
>  	}
> -	xpad->pad_present = 1;
> -	error = xpad_init_input(xpad);
> -	if (error)
> -		goto fail4;
>  
>  	return 0;
>  
> -- 
> 1.9.1
> 

-- 
Dmitry

[PATCH 5/8] Input: xpad: handle "present" and "gone" correctly

[Pavel Rojtberg]

From: "Pierre-Loup A. Griffais" <pgriffais@valvesoftware.com>

Handle the "a new device is present" message properly by dynamically
creating the input device at this point in time.  This requires a
workqueue as we are in interrupt context when we learn about this.

Also properly disconnect any devices that we are told are removed.

Signed-off-by: "Pierre-Loup A. Griffais" <pgriffais@valvesoftware.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Pavel Rojtberg <rojtberg@gmail.com>
---
 drivers/input/joystick/xpad.c | 43 +++++++++++++++++++++++++++++++++++--------
 1 file changed, 35 insertions(+), 8 deletions(-)

diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
index 3349861..a9ff4c2 100644
--- a/drivers/input/joystick/xpad.c
+++ b/drivers/input/joystick/xpad.c
@@ -343,8 +343,12 @@ struct usb_xpad {
 	int xtype;			/* type of xbox device */
 	unsigned long pad_nr;		/* the order x360 pads were attached */
 	const char *name;		/* name of the device */
+	struct work_struct work;	/* init/remove device from callback */
 };
 
+static int xpad_init_input(struct usb_xpad *xpad);
+static void xpad_deinit_input(struct usb_xpad *xpad);
+
 /*
  *	xpad_process_packet
  *
@@ -488,6 +492,22 @@ static void xpad360_process_packet(struct usb_xpad *xpad,
 
 static void xpad_identify_controller(struct usb_xpad *xpad);
 
+static void presence_work_function(struct work_struct *work)
+{
+	struct usb_xpad *xpad = container_of(work, struct usb_xpad, work);
+	int error;
+
+	if (xpad->pad_present) {
+		error = xpad_init_input(xpad);
+		if (error) {
+			/* complain only, not much else we can do here */
+			dev_err(&xpad->dev->dev, "unable to init device\n");
+		}
+	} else {
+		xpad_deinit_input(xpad);
+	}
+}
+
 /*
  * xpad360w_process_packet
  *
@@ -504,13 +524,16 @@ static void xpad_identify_controller(struct usb_xpad *xpad);
  */
 static void xpad360w_process_packet(struct usb_xpad *xpad, u16 cmd, unsigned char *data)
 {
+	int presence;
+
 	/* Presence change */
 	if (data[0] & 0x08) {
-		if (data[1] & 0x80) {
-			xpad->pad_present = 1;
-			xpad_identify_controller(xpad);
-		} else
-			xpad->pad_present = 0;
+		presence = (data[1] & 0x80) != 0;
+
+		if (xpad->pad_present != presence) {
+			xpad->pad_present = presence;
+			schedule_work(&xpad->work);
+		}
 	}
 
 	/* Valid pad data */
@@ -959,8 +982,6 @@ static int xpad_led_probe(struct usb_xpad *xpad)
 		return error;
 	}
 
-	xpad_identify_controller(xpad);
-
 	return 0;
 }
 
@@ -1110,6 +1131,8 @@ static int xpad_init_input(struct usb_xpad *xpad)
 	if (error)
 		goto fail_input_register;
 
+	xpad_identify_controller(xpad);
+
 	return 0;
 
 fail_input_register:
@@ -1174,6 +1197,7 @@ static int xpad_probe(struct usb_interface *intf, const struct usb_device_id *id
 	xpad->mapping = xpad_device[i].mapping;
 	xpad->xtype = xpad_device[i].xtype;
 	xpad->name = xpad_device[i].name;
+	INIT_WORK(&xpad->work, presence_work_function);
 
 	if (xpad->xtype == XTYPE_UNKNOWN) {
 		if (intf->cur_altsetting->desc.bInterfaceClass == USB_CLASS_VENDOR_SPEC) {
@@ -1274,7 +1298,8 @@ static void xpad_disconnect(struct usb_interface *intf)
 {
 	struct usb_xpad *xpad = usb_get_intfdata (intf);
 
-	xpad_deinit_input(xpad);
+	if (xpad->pad_present)
+		xpad_deinit_input(xpad);
 	xpad_deinit_output(xpad);
 
 	if (xpad->xtype == XTYPE_XBOX360W) {
@@ -1285,6 +1310,8 @@ static void xpad_disconnect(struct usb_interface *intf)
 	usb_free_coherent(xpad->udev, XPAD_PKT_LEN,
 			xpad->idata, xpad->idata_dma);
 
+	cancel_work_sync(&xpad->work);
+
 	kfree(xpad);
 
 	usb_set_intfdata(intf, NULL);
-- 
1.9.1

Re: [PATCH 5/8] Input: xpad: handle "present" and "gone" correctly

[Dmitry Torokhov]

On Sat, Jul 11, 2015 at 01:47:45AM +0200, Pavel Rojtberg wrote:
> From: "Pierre-Loup A. Griffais" <pgriffais@valvesoftware.com>
> 
> Handle the "a new device is present" message properly by dynamically
> creating the input device at this point in time.  This requires a
> workqueue as we are in interrupt context when we learn about this.
> 
> Also properly disconnect any devices that we are told are removed.
> 
> Signed-off-by: "Pierre-Loup A. Griffais" <pgriffais@valvesoftware.com>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Pavel Rojtberg <rojtberg@gmail.com>
> ---
>  drivers/input/joystick/xpad.c | 43 +++++++++++++++++++++++++++++++++++--------
>  1 file changed, 35 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
> index 3349861..a9ff4c2 100644
> --- a/drivers/input/joystick/xpad.c
> +++ b/drivers/input/joystick/xpad.c
> @@ -343,8 +343,12 @@ struct usb_xpad {
>  	int xtype;			/* type of xbox device */
>  	unsigned long pad_nr;		/* the order x360 pads were attached */
>  	const char *name;		/* name of the device */
> +	struct work_struct work;	/* init/remove device from callback */
>  };
>  
> +static int xpad_init_input(struct usb_xpad *xpad);
> +static void xpad_deinit_input(struct usb_xpad *xpad);
> +
>  /*
>   *	xpad_process_packet
>   *
> @@ -488,6 +492,22 @@ static void xpad360_process_packet(struct usb_xpad *xpad,
>  
>  static void xpad_identify_controller(struct usb_xpad *xpad);
>  
> +static void presence_work_function(struct work_struct *work)
> +{
> +	struct usb_xpad *xpad = container_of(work, struct usb_xpad, work);
> +	int error;
> +
> +	if (xpad->pad_present) {
> +		error = xpad_init_input(xpad);
> +		if (error) {
> +			/* complain only, not much else we can do here */
> +			dev_err(&xpad->dev->dev, "unable to init device\n");
> +		}
> +	} else {
> +		xpad_deinit_input(xpad);
> +	}
> +}
> +
>  /*
>   * xpad360w_process_packet
>   *
> @@ -504,13 +524,16 @@ static void xpad_identify_controller(struct usb_xpad *xpad);
>   */
>  static void xpad360w_process_packet(struct usb_xpad *xpad, u16 cmd, unsigned char *data)
>  {
> +	int presence;
> +
>  	/* Presence change */
>  	if (data[0] & 0x08) {
> -		if (data[1] & 0x80) {
> -			xpad->pad_present = 1;
> -			xpad_identify_controller(xpad);
> -		} else
> -			xpad->pad_present = 0;
> +		presence = (data[1] & 0x80) != 0;
> +
> +		if (xpad->pad_present != presence) {
> +			xpad->pad_present = presence;
> +			schedule_work(&xpad->work);
> +		}
>  	}

So what happens if we start without the input device, schedule it to be
added and then receive a valid pad data packet before work item had a
chance to run and input device is still not created? We are going crash
and burn :(

Thanks.

-- 
Dmitry

[PATCH 6/8] Input: xpad: use bitmask for finding the pad_nr

[Pavel Rojtberg]

From: Pavel Rojtberg <rojtberg@gmail.com>

The pad_nr should be consistent after disconnecting/ reconnecting a
xbox360 controllers.
Use a bitmask to track connected pads - this way we can re-assign freed
up pad numbers.

Consider the following case:
1. pad A is connected and gets pad_nr = 0
2. pad B is connected and gets pad_nr = 1
3. pad A is disconnected
4. pad A is connected again

using the bitmask controller A now correctly gets pad_nr = 0 again.

Note: this sets a limit of 32 simultaneous connected xbox360
controllers. However this should be tolerable.

Signed-off-by: Pavel Rojtberg <rojtberg@gmail.com>
---
 drivers/input/joystick/xpad.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
index a9ff4c2..e28a9c1 100644
--- a/drivers/input/joystick/xpad.c
+++ b/drivers/input/joystick/xpad.c
@@ -346,6 +346,8 @@ struct usb_xpad {
 	struct work_struct work;	/* init/remove device from callback */
 };
 
+static unsigned long xpad_pad_seq;
+
 static int xpad_init_input(struct usb_xpad *xpad);
 static void xpad_deinit_input(struct usb_xpad *xpad);
 
@@ -940,6 +942,12 @@ static void xpad_send_led_command(struct usb_xpad *xpad, int command)
  */
 static void xpad_identify_controller(struct usb_xpad *xpad)
 {
+	if (xpad->xtype != XTYPE_XBOX360 && xpad->xtype != XTYPE_XBOX360W)
+		return;
+
+	xpad->pad_nr = find_first_zero_bit(&xpad_pad_seq, 32);
+	set_bit(xpad->pad_nr, &xpad_pad_seq);
+
 	xpad_send_led_command(xpad, (xpad->pad_nr % 4) + 2);
 }
 
@@ -954,7 +962,6 @@ static void xpad_led_set(struct led_classdev *led_cdev,
 
 static int xpad_led_probe(struct usb_xpad *xpad)
 {
-	static atomic_t led_seq = ATOMIC_INIT(-1);
 	struct xpad_led *led;
 	struct led_classdev *led_cdev;
 	int error;
@@ -966,8 +973,6 @@ static int xpad_led_probe(struct usb_xpad *xpad)
 	if (!led)
 		return -ENOMEM;
 
-	xpad->pad_nr = atomic_inc_return(&led_seq);
-
 	snprintf(led->name, sizeof(led->name), "xpad%lu", xpad->pad_nr);
 	led->xpad = xpad;
 
@@ -1119,6 +1124,8 @@ static int xpad_init_input(struct usb_xpad *xpad)
 			xpad_set_up_abs(input_dev, xpad_abs_triggers[i]);
 	}
 
+	xpad_identify_controller(xpad);
+
 	error = xpad_init_ff(xpad);
 	if (error)
 		goto fail_init_ff;
@@ -1131,8 +1138,6 @@ static int xpad_init_input(struct usb_xpad *xpad)
 	if (error)
 		goto fail_input_register;
 
-	xpad_identify_controller(xpad);
-
 	return 0;
 
 fail_input_register:
@@ -1292,6 +1297,11 @@ static void xpad_deinit_input(struct usb_xpad *xpad)
 {
 	xpad_led_disconnect(xpad);
 	input_unregister_device(xpad->dev);
+
+	if (xpad->xtype != XTYPE_XBOX360 && xpad->xtype != XTYPE_XBOX360W)
+		return;
+
+	clear_bit(xpad->pad_nr, &xpad_pad_seq);
 }
 
 static void xpad_disconnect(struct usb_interface *intf)
-- 
1.9.1

Re: [PATCH 6/8] Input: xpad: use bitmask for finding the pad_nr

[Dmitry Torokhov]

Hi Pavel,

On Sat, Jul 11, 2015 at 01:47:46AM +0200, Pavel Rojtberg wrote:
> From: Pavel Rojtberg <rojtberg@gmail.com>
> 
> The pad_nr should be consistent after disconnecting/ reconnecting a
> xbox360 controllers.
> Use a bitmask to track connected pads - this way we can re-assign freed
> up pad numbers.
> 
> Consider the following case:
> 1. pad A is connected and gets pad_nr = 0
> 2. pad B is connected and gets pad_nr = 1
> 3. pad A is disconnected
> 4. pad A is connected again
> 
> using the bitmask controller A now correctly gets pad_nr = 0 again.

And what happens if I pull both out and plug B first? Why do we care
about this? If we really want stable numbering maybe it should be driven
from userspace based on connection path?

> 
> Note: this sets a limit of 32 simultaneous connected xbox360
> controllers. However this should be tolerable.
> 
> Signed-off-by: Pavel Rojtberg <rojtberg@gmail.com>
> ---
>  drivers/input/joystick/xpad.c | 20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
> index a9ff4c2..e28a9c1 100644
> --- a/drivers/input/joystick/xpad.c
> +++ b/drivers/input/joystick/xpad.c
> @@ -346,6 +346,8 @@ struct usb_xpad {
>  	struct work_struct work;	/* init/remove device from callback */
>  };
>  
> +static unsigned long xpad_pad_seq;
> +
>  static int xpad_init_input(struct usb_xpad *xpad);
>  static void xpad_deinit_input(struct usb_xpad *xpad);
>  
> @@ -940,6 +942,12 @@ static void xpad_send_led_command(struct usb_xpad *xpad, int command)
>   */
>  static void xpad_identify_controller(struct usb_xpad *xpad)
>  {
> +	if (xpad->xtype != XTYPE_XBOX360 && xpad->xtype != XTYPE_XBOX360W)
> +		return;
> +
> +	xpad->pad_nr = find_first_zero_bit(&xpad_pad_seq, 32);
> +	set_bit(xpad->pad_nr, &xpad_pad_seq);
> +

This is racy. If we really want it you might consider idr/ida.

Thanks.

-- 
Dmitry

Re: [PATCH 6/8] Input: xpad: use bitmask for finding the pad_nr

[Pavel Rojtberg]

Hi Dimitry,

2015-07-30 8:55 GMT+02:00 Dmitry Torokhov <dmitry.torokhov@gmail.com>:
> Hi Pavel,
>
> On Sat, Jul 11, 2015 at 01:47:46AM +0200, Pavel Rojtberg wrote:
>> From: Pavel Rojtberg <rojtberg@gmail.com>
>>
>> The pad_nr should be consistent after disconnecting/ reconnecting a
>> xbox360 controllers.
>> Use a bitmask to track connected pads - this way we can re-assign freed
>> up pad numbers.
>>
>> Consider the following case:
>> 1. pad A is connected and gets pad_nr = 0
>> 2. pad B is connected and gets pad_nr = 1
>> 3. pad A is disconnected
>> 4. pad A is connected again
>>
>> using the bitmask controller A now correctly gets pad_nr = 0 again.
>
> And what happens if I pull both out and plug B first? Why do we care
> about this? If we really want stable numbering maybe it should be driven
> from userspace based on connection path?
Note this is not about stable numbering, but really about ida style enumeration.
This is used only for determining which LED should be lit (range = [0..4[).
So plugging B first and having it pad_nr = 0 is actually the expected
result here.
(We do not know whether pad A will be connected at this point, so pad
B gets slot 0)

The original patch used the joypad id for enumeration:
http://www.spinics.net/lists/linux-input/msg29448.html
But the response was that this is a bad approach, this is why I came up
with the ida style enumeration.

Using userspace for enumeration was also already discussed here:
http://www.spinics.net/lists/linux-input/msg29539.html
the consensus was (and I am with it) that a daemon is overkill, just
to light some LEDs.

>> Note: this sets a limit of 32 simultaneous connected xbox360
>> controllers. However this should be tolerable.
>>
>> Signed-off-by: Pavel Rojtberg <rojtberg@gmail.com>
>> ---
>>  drivers/input/joystick/xpad.c | 20 +++++++++++++++-----
>>  1 file changed, 15 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
>> index a9ff4c2..e28a9c1 100644
>> --- a/drivers/input/joystick/xpad.c
>> +++ b/drivers/input/joystick/xpad.c
>> @@ -346,6 +346,8 @@ struct usb_xpad {
>>       struct work_struct work;        /* init/remove device from callback */
>>  };
>>
>> +static unsigned long xpad_pad_seq;
>> +
>>  static int xpad_init_input(struct usb_xpad *xpad);
>>  static void xpad_deinit_input(struct usb_xpad *xpad);
>>
>> @@ -940,6 +942,12 @@ static void xpad_send_led_command(struct usb_xpad *xpad, int command)
>>   */
>>  static void xpad_identify_controller(struct usb_xpad *xpad)
>>  {
>> +     if (xpad->xtype != XTYPE_XBOX360 && xpad->xtype != XTYPE_XBOX360W)
>> +             return;
>> +
>> +     xpad->pad_nr = find_first_zero_bit(&xpad_pad_seq, 32);
>> +     set_bit(xpad->pad_nr, &xpad_pad_seq);
>> +
>
> This is racy. If we really want it you might consider idr/ida.
Thanks, this is what I actually needed here. Will change this for v2.

>
> Thanks.
>
> --
> Dmitry

[PATCH 7/8] Input: xpad: factor out URB submission in xpad_play_effect

[Pavel Rojtberg]

From: Pavel Rojtberg <rojtberg@gmail.com>

To allow us controlling URB submission in the next patch, move submisson
logic to a single point at the end of the function.

Signed-off-by: Pavel Rojtberg <rojtberg@gmail.com>
---
 drivers/input/joystick/xpad.c | 140 ++++++++++++++++++++++++++++++++---------------------------------
 1 file changed, 69 insertions(+), 71 deletions(-)

diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
index e28a9c1..150eaaa 100644
--- a/drivers/input/joystick/xpad.c
+++ b/drivers/input/joystick/xpad.c
@@ -784,80 +784,78 @@ static void xpad_deinit_output(struct usb_xpad *xpad)
 static int xpad_play_effect(struct input_dev *dev, void *data, struct ff_effect *effect)
 {
 	struct usb_xpad *xpad = input_get_drvdata(dev);
+	__u16 strong;
+	__u16 weak;
 
-	if (effect->type == FF_RUMBLE) {
-		__u16 strong = effect->u.rumble.strong_magnitude;
-		__u16 weak = effect->u.rumble.weak_magnitude;
-
-		switch (xpad->xtype) {
-
-		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);
-
-		case XTYPE_XBOXONE:
-			xpad->odata[0] = 0x09; /* activate rumble */
-			xpad->odata[1] = 0x08;
-			xpad->odata[2] = 0x00;
-			xpad->odata[3] = 0x08; /* continuous effect */
-			xpad->odata[4] = 0x00; /* simple rumble mode */
-			xpad->odata[5] = 0x03; /* L and R actuator only */
-			xpad->odata[6] = 0x00; /* TODO: LT actuator */
-			xpad->odata[7] = 0x00; /* TODO: RT actuator */
-			xpad->odata[8] = strong / 256;	/* left actuator */
-			xpad->odata[9] = weak / 256;	/* right actuator */
-			xpad->odata[10] = 0x80;	/* length of pulse */
-			xpad->odata[11] = 0x00;	/* stop period of pulse */
-			xpad->irq_out->transfer_buffer_length = 12;
-
-			return usb_submit_urb(xpad->irq_out, GFP_ATOMIC);
-
-		default:
-			dev_dbg(&xpad->dev->dev,
-				"%s - rumble command sent to unsupported xpad type: %d\n",
-				__func__, xpad->xtype);
-			return -1;
-		}
+	if (effect->type != FF_RUMBLE)
+		return 0;
+
+	strong = effect->u.rumble.strong_magnitude;
+	weak = effect->u.rumble.weak_magnitude;
+
+	switch (xpad->xtype) {
+	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;
+		break;
+
+	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;
+		break;
+
+	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;
+		break;
+
+	case XTYPE_XBOXONE:
+		xpad->odata[0] = 0x09; /* activate rumble */
+		xpad->odata[1] = 0x08;
+		xpad->odata[2] = 0x00;
+		xpad->odata[3] = 0x08; /* continuous effect */
+		xpad->odata[4] = 0x00; /* simple rumble mode */
+		xpad->odata[5] = 0x03; /* L and R actuator only */
+		xpad->odata[6] = 0x00; /* TODO: LT actuator */
+		xpad->odata[7] = 0x00; /* TODO: RT actuator */
+		xpad->odata[8] = strong / 256;	/* left actuator */
+		xpad->odata[9] = weak / 256;	/* right actuator */
+		xpad->odata[10] = 0x80;	/* length of pulse */
+		xpad->odata[11] = 0x00;	/* stop period of pulse */
+		xpad->irq_out->transfer_buffer_length = 12;
+		break;
+
+	default:
+		dev_dbg(&xpad->dev->dev,
+			"%s - rumble command sent to unsupported xpad type: %d\n",
+			__func__, xpad->xtype);
+		return -EINVAL;
 	}
 
-	return 0;
+	return usb_submit_urb(xpad->irq_out, GFP_ATOMIC);
 }
 
 static int xpad_init_ff(struct usb_xpad *xpad)
-- 
1.9.1

[PATCH 8/8] Input: xpad: do not submit active URBs

[Pavel Rojtberg]

From: Pavel Rojtberg <rojtberg@gmail.com>

track the active status of the irq_out URB to prevent submission while
it is active. Failure to do so results in the "URB submitted while
active" warning/ stacktrace.

Also add missing mutex locking around xpad->odata usages.

This is an workaround for a suspend/ resume issue on my machine, where
after resume irq_out is completely dead.

In preliminary testing I could not observe any dropping of packets.
(controlling rumble with fftest while setting the LEDs using sysfs)
If there actually are cases where packets are dropped an extension of
this patch to queue the URBs instead of dropping is straightforward.

Signed-off-by: Pavel Rojtberg <rojtberg@gmail.com>
---
 drivers/input/joystick/xpad.c | 43 +++++++++++++++++++++++++++++++++++++++----
 1 file changed, 39 insertions(+), 4 deletions(-)

diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
index 150eaaa..3df4671 100644
--- a/drivers/input/joystick/xpad.c
+++ b/drivers/input/joystick/xpad.c
@@ -329,6 +329,7 @@ struct usb_xpad {
 	dma_addr_t idata_dma;
 
 	struct urb *irq_out;		/* urb for interrupt out report */
+	int irq_out_active;		/* we must not use an active URB */
 	unsigned char *odata;		/* output data */
 	dma_addr_t odata_dma;
 	struct mutex odata_mutex;
@@ -701,6 +702,7 @@ static void xpad_irq_out(struct urb *urb)
 	switch (status) {
 	case 0:
 		/* success */
+		xpad->irq_out_active = 0;
 		return;
 
 	case -ECONNRESET:
@@ -709,6 +711,7 @@ 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);
+		xpad->irq_out_active = 0;
 		return;
 
 	default:
@@ -786,6 +789,7 @@ static int xpad_play_effect(struct input_dev *dev, void *data, struct ff_effect
 	struct usb_xpad *xpad = input_get_drvdata(dev);
 	__u16 strong;
 	__u16 weak;
+	int retval;
 
 	if (effect->type != FF_RUMBLE)
 		return 0;
@@ -793,6 +797,8 @@ static int xpad_play_effect(struct input_dev *dev, void *data, struct ff_effect
 	strong = effect->u.rumble.strong_magnitude;
 	weak = effect->u.rumble.weak_magnitude;
 
+	mutex_lock(&xpad->odata_mutex);
+
 	switch (xpad->xtype) {
 	case XTYPE_XBOX:
 		xpad->odata[0] = 0x00;
@@ -849,13 +855,25 @@ static int xpad_play_effect(struct input_dev *dev, void *data, struct ff_effect
 		break;
 
 	default:
+		mutex_unlock(&xpad->odata_mutex);
 		dev_dbg(&xpad->dev->dev,
 			"%s - rumble command sent to unsupported xpad type: %d\n",
 			__func__, xpad->xtype);
 		return -EINVAL;
 	}
 
-	return usb_submit_urb(xpad->irq_out, GFP_ATOMIC);
+	if (!xpad->irq_out_active) {
+		retval = usb_submit_urb(xpad->irq_out, GFP_ATOMIC);
+		xpad->irq_out_active = 1;
+	} else {
+		retval = -EIO;
+		dev_dbg(&xpad->dev->dev, "%s - dropped, irq_out is active\n",
+				__func__);
+	}
+
+	mutex_unlock(&xpad->odata_mutex);
+
+	return retval;
 }
 
 static int xpad_init_ff(struct usb_xpad *xpad)
@@ -931,7 +949,13 @@ static void xpad_send_led_command(struct usb_xpad *xpad, int command)
 		break;
 	}
 
-	usb_submit_urb(xpad->irq_out, GFP_KERNEL);
+	if (!xpad->irq_out_active) {
+		usb_submit_urb(xpad->irq_out, GFP_KERNEL);
+		xpad->irq_out_active = 1;
+	} else
+		dev_dbg(&xpad->dev->dev, "%s - dropped, irq_out is active\n",
+				__func__);
+
 	mutex_unlock(&xpad->odata_mutex);
 }
 
@@ -1007,6 +1031,7 @@ static void xpad_identify_controller(struct usb_xpad *xpad) { }
 static int xpad_open(struct input_dev *dev)
 {
 	struct usb_xpad *xpad = input_get_drvdata(dev);
+	int retval;
 
 	/* URB was submitted in probe */
 	if (xpad->xtype == XTYPE_XBOX360W)
@@ -1017,11 +1042,14 @@ static int xpad_open(struct input_dev *dev)
 		return -EIO;
 
 	if (xpad->xtype == XTYPE_XBOXONE) {
+		mutex_lock(&xpad->odata_mutex);
 		/* Xbox one controller needs to be initialized. */
 		xpad->odata[0] = 0x05;
 		xpad->odata[1] = 0x20;
 		xpad->irq_out->transfer_buffer_length = 2;
-		return usb_submit_urb(xpad->irq_out, GFP_KERNEL);
+		retval = usb_submit_urb(xpad->irq_out, GFP_KERNEL);
+		mutex_unlock(&xpad->odata_mutex);
+		return retval;
 	}
 
 	return 0;
@@ -1272,7 +1300,14 @@ static int xpad_probe(struct usb_interface *intf, const struct usb_device_id *id
 		xpad->odata[10] = 0x00;
 		xpad->odata[11] = 0x00;
 		xpad->irq_out->transfer_buffer_length = 12;
-		usb_submit_urb(xpad->irq_out, GFP_KERNEL);
+
+		if (!xpad->irq_out_active) {
+			usb_submit_urb(xpad->irq_out, GFP_KERNEL);
+			xpad->irq_out_active = 1;
+		} else
+			dev_dbg(&xpad->dev->dev,
+				"%s - dropped, irq_out is active\n", __func__);
+
 		mutex_unlock(&xpad->odata_mutex);
 	} else {
 		xpad->pad_present = 1;
-- 
1.9.1

Re: [PATCH 8/8] Input: xpad: do not submit active URBs

[Dmitry Torokhov]

On Sat, Jul 11, 2015 at 01:47:48AM +0200, Pavel Rojtberg wrote:
> From: Pavel Rojtberg <rojtberg@gmail.com>
> 
> track the active status of the irq_out URB to prevent submission while
> it is active. Failure to do so results in the "URB submitted while
> active" warning/ stacktrace.
> 
> Also add missing mutex locking around xpad->odata usages.
> 
> This is an workaround for a suspend/ resume issue on my machine, where
> after resume irq_out is completely dead.
> 
> In preliminary testing I could not observe any dropping of packets.
> (controlling rumble with fftest while setting the LEDs using sysfs)
> If there actually are cases where packets are dropped an extension of
> this patch to queue the URBs instead of dropping is straightforward.

We need to implement it. If you weren't able to reproduce the race it
does not mean it does not exist.

Also you can not take mutex in xpad_play_effect as it is called under a
spinlock with interrupts disabled.

Thanks.

-- 
Dmitry

Re: [PATCH 8/8] Input: xpad: do not submit active URBs

[Pavel Rojtberg]

2015-07-30 8:59 GMT+02:00 Dmitry Torokhov <dmitry.torokhov@gmail.com>:
> On Sat, Jul 11, 2015 at 01:47:48AM +0200, Pavel Rojtberg wrote:
>> From: Pavel Rojtberg <rojtberg@gmail.com>
>>
>> track the active status of the irq_out URB to prevent submission while
>> it is active. Failure to do so results in the "URB submitted while
>> active" warning/ stacktrace.
>>
>> Also add missing mutex locking around xpad->odata usages.
>>
>> This is an workaround for a suspend/ resume issue on my machine, where
>> after resume irq_out is completely dead.
>>
>> In preliminary testing I could not observe any dropping of packets.
>> (controlling rumble with fftest while setting the LEDs using sysfs)
>> If there actually are cases where packets are dropped an extension of
>> this patch to queue the URBs instead of dropping is straightforward.
>
> We need to implement it. If you weren't able to reproduce the race it
> does not mean it does not exist.
I will probably just back out this one from v2. It is not quite clear to me
whether we are at fault here for not buffering the packets or the userspace
for submitting too many.

Furthermore I found out that I can wake up irq_out by resetting the usb device
after suspend/ resume. This would be a better fix for my issue at hand.

> Also you can not take mutex in xpad_play_effect as it is called under a
> spinlock with interrupts disabled.
Should the odata_mutex better be replaced by a spinlock then?

> Thanks.
>
> --
> Dmitry

Re: [PATCH 0/8] Input: xpad: fix wireless pad connection and URB out

[Pavel Rojtberg]

feel free to ask for details. If you have no objections, please merge.

Am 25.07.2015 um 12:51 schrieb Pavel Rojtberg:
> feel free to ask for details. If you have no objections, please merge.
>
> Pavel Rojtberg <rojtberg@gmail.com <mailto:rojtberg@gmail.com>> 
> schrieb am Sa., 11. Juli 2015 um 01:48 Uhr:
>
>     From: Pavel Rojtberg <rojtberg@gmail.com <mailto:rojtberg@gmail.com>>
>
>     This series tries to finish the work of:
>     http://www.spinics.net/lists/linux-input/msg29445.html
>     Citing the original submission:
>     Rework the xpad driver to fix the issue where when a wireless xpad
>     controller
>     is plugged in, 4 joystick devices are created, no matter how many
>     are really
>     attached to the system.
>     This is done by dynamically creating the devices only when they
>     are found
>     by the wireless receiver.
>     Along the way all usages of the out URB were guarded by the mutex
>     and a
>     active flag was introduced to prevent usage while active.
>     This makes outbound communication more robust. (LED, FF, presence
>     query)
>
>     Patches 1-2 clean up the x360 LED code after:
>     http://www.spinics.net/lists/linux-input/msg39147.html
>     partifularly the LED command on x360w pads is not submitted twice
>     any more.
>     They are the same as already submitted here:
>     http://www.spinics.net/lists/linux-input/msg39438.html
>
>     Patches 3-6 implement the actual "on demand" creation/ deletion of
>     input
>     devices. To this end pad enumeration had to be changed from an
>     monotonic
>     counter to a bitmask based counter. See Patch 6 for rationale.
>
>     Patches 7-8 prevent sending active URBs. This was alrady an issue
>     before, but
>     is now more pressing as we always send the query packet on driver
>     load. (x360w)
>
>     Pavel Rojtberg (6):
>       Input: xpad: clarify LED enumeration
>       Input: xpad: remove bulk out URB
>       Input: xpad: query Wireless controller state at init
>       Input: xpad: use bitmask for finding the pad_nr
>       Input: xpad: factor out URB submission in xpad_play_effect
>       Input: xpad: do not submit active URBs
>
>     Pierre-Loup A. Griffais (2):
>       Input: xpad: move the input device creation to a new function
>       Input: xpad: handle "present" and "gone" correctly
>
>      xpad.c | 539
>     +++++++++++++++++++++++++++++++++++------------------------------
>      1 file changed, 290 insertions(+), 249 deletions(-)
>
>     --
>     1.9.1
>