* [PATCH 1/5] Input: xpad: handle "present" and "gone" correctly
2015-11-01 15:31 [PATCH 0/5] Input: xpad: robustness updates Pavel Rojtberg
@ 2015-11-01 15:31 ` Pavel Rojtberg
2015-12-10 7:02 ` Dmitry Torokhov
2015-11-01 15:31 ` [PATCH 2/5] Input: xpad: do not submit active URBs Pavel Rojtberg
` (3 subsequent siblings)
4 siblings, 1 reply; 13+ messages in thread
From: Pavel Rojtberg @ 2015-11-01 15:31 UTC (permalink / raw)
To: linux-input, pgriffais, dmitry.torokhov, gregkh; +Cc: 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 means we now do
not "preallocate" all 4 devices when a single
wireless base station is seen. 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 | 66 +++++++++++++++++++++++++++++++++++++++++-------------------------
1 file changed, 41 insertions(+), 25 deletions(-)
diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
index fd4100d..23e5613 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 */
int 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
*
@@ -497,6 +501,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
*
@@ -513,17 +533,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;
- /*
- * Light up the segment corresponding to
- * controller number.
- */
- 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 */
@@ -1001,14 +1020,7 @@ static int xpad_led_probe(struct usb_xpad *xpad)
if (error)
goto err_free_id;
- if (xpad->xtype == XTYPE_XBOX360) {
- /*
- * Light up the segment corresponding to controller
- * number on wired devices. On wireless we'll do that
- * when they respond to "presence" packet.
- */
- xpad_identify_controller(xpad);
- }
+ xpad_identify_controller(xpad);
return 0;
@@ -1241,6 +1253,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) {
@@ -1277,10 +1290,6 @@ static int xpad_probe(struct usb_interface *intf, const struct usb_device_id *id
usb_set_intfdata(intf, xpad);
- error = xpad_init_input(xpad);
- if (error)
- goto err_deinit_output;
-
if (xpad->xtype == XTYPE_XBOX360W) {
/*
* Submit the int URB immediately rather than waiting for open
@@ -1292,7 +1301,7 @@ 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 err_deinit_input;
+ goto err_deinit_output;
/*
* Send presence packet.
@@ -1304,13 +1313,16 @@ static int xpad_probe(struct usb_interface *intf, const struct usb_device_id *id
error = xpad_inquiry_pad_presence(xpad);
if (error)
goto err_kill_in_urb;
+ } else {
+ xpad->pad_present = 1;
+ error = xpad_init_input(xpad);
+ if (error)
+ goto err_deinit_output;
}
return 0;
err_kill_in_urb:
usb_kill_urb(xpad->irq_in);
-err_deinit_input:
- xpad_deinit_input(xpad);
err_deinit_output:
xpad_deinit_output(xpad);
err_free_in_urb:
@@ -1327,13 +1339,17 @@ static void xpad_disconnect(struct usb_interface *intf)
{
struct usb_xpad *xpad = usb_get_intfdata (intf);
- xpad_deinit_input(xpad);
xpad_deinit_output(xpad);
if (xpad->xtype == XTYPE_XBOX360W) {
usb_kill_urb(xpad->irq_in);
}
+ cancel_work_sync(&xpad->work);
+
+ if (xpad->pad_present)
+ xpad_deinit_input(xpad);
+
usb_free_urb(xpad->irq_in);
usb_free_coherent(xpad->udev, XPAD_PKT_LEN,
xpad->idata, xpad->idata_dma);
--
1.9.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH 1/5] Input: xpad: handle "present" and "gone" correctly
2015-11-01 15:31 ` [PATCH 1/5] Input: xpad: handle "present" and "gone" correctly Pavel Rojtberg
@ 2015-12-10 7:02 ` Dmitry Torokhov
2015-12-14 23:29 ` Dmitry Torokhov
0 siblings, 1 reply; 13+ messages in thread
From: Dmitry Torokhov @ 2015-12-10 7:02 UTC (permalink / raw)
To: Pavel Rojtberg; +Cc: linux-input, pgriffais, gregkh
On Sun, Nov 01, 2015 at 04:31:35PM +0100, 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 means we now do
> not "preallocate" all 4 devices when a single
> wireless base station is seen. 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 | 66 +++++++++++++++++++++++++++++++++++++++++-------------------------
> 1 file changed, 41 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
> index fd4100d..23e5613 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 */
> int 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
> *
> @@ -497,6 +501,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
> *
> @@ -513,17 +533,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;
> - /*
> - * Light up the segment corresponding to
> - * controller number.
> - */
> - 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);
> + }
I think this is racy: we'll crash if we get motion packets before we
finish creating input device. I think we should be returning whether we
want to have xpad->irq_in URB be re-submitted and in case we scheduke
work we'd return "false" and have work resubmit IRQ when it is done
creating or destroying input device.
--
Dmitry
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 1/5] Input: xpad: handle "present" and "gone" correctly
2015-12-10 7:02 ` Dmitry Torokhov
@ 2015-12-14 23:29 ` Dmitry Torokhov
2015-12-18 2:01 ` Pavel Rojtberg
0 siblings, 1 reply; 13+ messages in thread
From: Dmitry Torokhov @ 2015-12-14 23:29 UTC (permalink / raw)
To: Pavel Rojtberg; +Cc: linux-input, pgriffais, gregkh
On Wed, Dec 09, 2015 at 11:02:39PM -0800, Dmitry Torokhov wrote:
> On Sun, Nov 01, 2015 at 04:31:35PM +0100, 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 means we now do
> > not "preallocate" all 4 devices when a single
> > wireless base station is seen. This requires a workqueue as we are in
> > interrupt context when we learn about this.
> >
> > 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;
> > - /*
> > - * Light up the segment corresponding to
> > - * controller number.
> > - */
> > - 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);
> > + }
>
> I think this is racy: we'll crash if we get motion packets before we
> finish creating input device. I think we should be returning whether we
> want to have xpad->irq_in URB be re-submitted and in case we scheduke
> work we'd return "false" and have work resubmit IRQ when it is done
> creating or destroying input device.
Hmm, after I thought about it some more I am not sure that simply
not submitting any more input URBs is sufficient: what will happen if
the device sends motion event before we send our query (I.e. user is
holding a button on the controller). I think it is safer to make sure we
are not accessing half-created or half-gone input device when
processing packet.
Could you please take a look at the new version of this patch below?
Thanks!
--
Dmitry
Input: xpad - handle "present" and "gone" correctly
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 means we now do not
"preallocate" all 4 devices when a single wireless base station is seen.
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>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/joystick/xpad.c | 107 ++++++++++++++++++++++++++---------------
1 file changed, 69 insertions(+), 38 deletions(-)
diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
index c44cbd4..1d51d24 100644
--- a/drivers/input/joystick/xpad.c
+++ b/drivers/input/joystick/xpad.c
@@ -76,6 +76,8 @@
*/
#include <linux/kernel.h>
+#include <linux/input.h>
+#include <linux/rcupdate.h>
#include <linux/slab.h>
#include <linux/stat.h>
#include <linux/module.h>
@@ -319,10 +321,12 @@ MODULE_DEVICE_TABLE(usb, xpad_table);
struct usb_xpad {
struct input_dev *dev; /* input device interface */
+ struct input_dev __rcu *x360w_dev;
struct usb_device *udev; /* usb device */
struct usb_interface *intf; /* usb interface */
- int pad_present;
+ bool pad_present;
+ bool input_created;
struct urb *irq_in; /* urb for interrupt in report */
unsigned char *idata; /* input data */
@@ -343,8 +347,12 @@ struct usb_xpad {
int xtype; /* type of xbox device */
int 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
*
@@ -424,11 +432,9 @@ static void xpad_process_packet(struct usb_xpad *xpad, u16 cmd, unsigned char *d
* http://www.free60.org/wiki/Gamepad
*/
-static void xpad360_process_packet(struct usb_xpad *xpad,
+static void xpad360_process_packet(struct usb_xpad *xpad, struct input_dev *dev,
u16 cmd, unsigned char *data)
{
- struct input_dev *dev = xpad->dev;
-
/* digital pad */
if (xpad->mapping & MAP_DPAD_TO_BUTTONS) {
/* dpad as buttons (left, right, up, down) */
@@ -495,7 +501,30 @@ static void xpad360_process_packet(struct usb_xpad *xpad,
input_sync(dev);
}
-static void xpad_identify_controller(struct usb_xpad *xpad);
+static void xpad_presence_work(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: %d\n", error);
+ } else {
+ rcu_assign_pointer(xpad->x360w_dev, xpad->dev);
+ }
+ } else {
+ RCU_INIT_POINTER(xpad->x360w_dev, NULL);
+ synchronize_rcu();
+ /*
+ * Now that we are sure xpad360w_process_packet is not
+ * using input device we can get rid of it.
+ */
+ xpad_deinit_input(xpad);
+ }
+}
/*
* xpad360w_process_packet
@@ -513,24 +542,28 @@ static void xpad_identify_controller(struct usb_xpad *xpad);
*/
static void xpad360w_process_packet(struct usb_xpad *xpad, u16 cmd, unsigned char *data)
{
+ struct input_dev *dev;
+ bool present;
+
/* Presence change */
if (data[0] & 0x08) {
- if (data[1] & 0x80) {
- xpad->pad_present = 1;
- /*
- * Light up the segment corresponding to
- * controller number.
- */
- xpad_identify_controller(xpad);
- } else
- xpad->pad_present = 0;
+ present = (data[1] & 0x80) != 0;
+
+ if (xpad->pad_present != present) {
+ xpad->pad_present = present;
+ schedule_work(&xpad->work);
+ }
}
/* Valid pad data */
if (data[1] != 0x1)
return;
- xpad360_process_packet(xpad, cmd, &data[4]);
+ rcu_read_lock();
+ dev = rcu_dereference(xpad->x360w_dev);
+ if (dev)
+ xpad360_process_packet(xpad, dev, cmd, &data[4]);
+ rcu_read_unlock();
}
/*
@@ -659,7 +692,7 @@ static void xpad_irq_in(struct urb *urb)
switch (xpad->xtype) {
case XTYPE_XBOX360:
- xpad360_process_packet(xpad, 0, xpad->idata);
+ xpad360_process_packet(xpad, xpad->dev, 0, xpad->idata);
break;
case XTYPE_XBOX360W:
xpad360w_process_packet(xpad, 0, xpad->idata);
@@ -1001,14 +1034,7 @@ static int xpad_led_probe(struct usb_xpad *xpad)
if (error)
goto err_free_id;
- if (xpad->xtype == XTYPE_XBOX360) {
- /*
- * Light up the segment corresponding to controller
- * number on wired devices. On wireless we'll do that
- * when they respond to "presence" packet.
- */
- xpad_identify_controller(xpad);
- }
+ xpad_identify_controller(xpad);
return 0;
@@ -1097,8 +1123,11 @@ static void xpad_set_up_abs(struct input_dev *input_dev, signed short abs)
static void xpad_deinit_input(struct usb_xpad *xpad)
{
- xpad_led_disconnect(xpad);
- input_unregister_device(xpad->dev);
+ if (xpad->input_created) {
+ xpad->input_created = false;
+ xpad_led_disconnect(xpad);
+ input_unregister_device(xpad->dev);
+ }
}
static int xpad_init_input(struct usb_xpad *xpad)
@@ -1181,6 +1210,7 @@ static int xpad_init_input(struct usb_xpad *xpad)
if (error)
goto err_disconnect_led;
+ xpad->input_created = true;
return 0;
err_disconnect_led:
@@ -1241,6 +1271,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, xpad_presence_work);
if (xpad->xtype == XTYPE_UNKNOWN) {
if (intf->cur_altsetting->desc.bInterfaceClass == USB_CLASS_VENDOR_SPEC) {
@@ -1277,10 +1308,6 @@ static int xpad_probe(struct usb_interface *intf, const struct usb_device_id *id
usb_set_intfdata(intf, xpad);
- error = xpad_init_input(xpad);
- if (error)
- goto err_deinit_output;
-
if (xpad->xtype == XTYPE_XBOX360W) {
/*
* Submit the int URB immediately rather than waiting for open
@@ -1292,7 +1319,7 @@ 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 err_deinit_input;
+ goto err_deinit_output;
/*
* Send presence packet.
@@ -1304,13 +1331,15 @@ static int xpad_probe(struct usb_interface *intf, const struct usb_device_id *id
error = xpad_inquiry_pad_presence(xpad);
if (error)
goto err_kill_in_urb;
+ } else {
+ error = xpad_init_input(xpad);
+ if (error)
+ goto err_deinit_output;
}
return 0;
err_kill_in_urb:
usb_kill_urb(xpad->irq_in);
-err_deinit_input:
- xpad_deinit_input(xpad);
err_deinit_output:
xpad_deinit_output(xpad);
err_free_in_urb:
@@ -1327,17 +1356,19 @@ static void xpad_disconnect(struct usb_interface *intf)
{
struct usb_xpad *xpad = usb_get_intfdata (intf);
- xpad_deinit_input(xpad);
- xpad_deinit_output(xpad);
-
- if (xpad->xtype == XTYPE_XBOX360W) {
+ if (xpad->xtype == XTYPE_XBOX360W)
usb_kill_urb(xpad->irq_in);
- }
+
+ cancel_work_sync(&xpad->work);
+
+ xpad_deinit_input(xpad);
usb_free_urb(xpad->irq_in);
usb_free_coherent(xpad->udev, XPAD_PKT_LEN,
xpad->idata, xpad->idata_dma);
+ xpad_deinit_output(xpad);
+
kfree(xpad);
usb_set_intfdata(intf, NULL);
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH 1/5] Input: xpad: handle "present" and "gone" correctly
2015-12-14 23:29 ` Dmitry Torokhov
@ 2015-12-18 2:01 ` Pavel Rojtberg
0 siblings, 0 replies; 13+ messages in thread
From: Pavel Rojtberg @ 2015-12-18 2:01 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, Pierre-Loup A. Griffais, Greg KH
2015-12-15 0:29 GMT+01:00 Dmitry Torokhov <dmitry.torokhov@gmail.com>:
> On Wed, Dec 09, 2015 at 11:02:39PM -0800, Dmitry Torokhov wrote:
>> On Sun, Nov 01, 2015 at 04:31:35PM +0100, 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 means we now do
>> > not "preallocate" all 4 devices when a single
>> > wireless base station is seen. This requires a workqueue as we are in
>> > interrupt context when we learn about this.
>> >
>> > 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;
>> > - /*
>> > - * Light up the segment corresponding to
>> > - * controller number.
>> > - */
>> > - 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);
>> > + }
>>
>> I think this is racy: we'll crash if we get motion packets before we
>> finish creating input device. I think we should be returning whether we
>> want to have xpad->irq_in URB be re-submitted and in case we scheduke
>> work we'd return "false" and have work resubmit IRQ when it is done
>> creating or destroying input device.
>
> Hmm, after I thought about it some more I am not sure that simply
> not submitting any more input URBs is sufficient: what will happen if
> the device sends motion event before we send our query (I.e. user is
> holding a button on the controller). I think it is safer to make sure we
> are not accessing half-created or half-gone input device when
> processing packet.
>
> Could you please take a look at the new version of this patch below?
>
> Thanks!
>
> --
> Dmitry
just tested it - everything still works. The approach also looks good to me.
>
> Input: xpad - handle "present" and "gone" correctly
>
> 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 means we now do not
> "preallocate" all 4 devices when a single wireless base station is seen.
> 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>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
> drivers/input/joystick/xpad.c | 107 ++++++++++++++++++++++++++---------------
> 1 file changed, 69 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
> index c44cbd4..1d51d24 100644
> --- a/drivers/input/joystick/xpad.c
> +++ b/drivers/input/joystick/xpad.c
> @@ -76,6 +76,8 @@
> */
>
> #include <linux/kernel.h>
> +#include <linux/input.h>
> +#include <linux/rcupdate.h>
> #include <linux/slab.h>
> #include <linux/stat.h>
> #include <linux/module.h>
> @@ -319,10 +321,12 @@ MODULE_DEVICE_TABLE(usb, xpad_table);
>
> struct usb_xpad {
> struct input_dev *dev; /* input device interface */
> + struct input_dev __rcu *x360w_dev;
> struct usb_device *udev; /* usb device */
> struct usb_interface *intf; /* usb interface */
>
> - int pad_present;
> + bool pad_present;
> + bool input_created;
>
> struct urb *irq_in; /* urb for interrupt in report */
> unsigned char *idata; /* input data */
> @@ -343,8 +347,12 @@ struct usb_xpad {
> int xtype; /* type of xbox device */
> int 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
> *
> @@ -424,11 +432,9 @@ static void xpad_process_packet(struct usb_xpad *xpad, u16 cmd, unsigned char *d
> * http://www.free60.org/wiki/Gamepad
> */
>
> -static void xpad360_process_packet(struct usb_xpad *xpad,
> +static void xpad360_process_packet(struct usb_xpad *xpad, struct input_dev *dev,
> u16 cmd, unsigned char *data)
> {
> - struct input_dev *dev = xpad->dev;
> -
> /* digital pad */
> if (xpad->mapping & MAP_DPAD_TO_BUTTONS) {
> /* dpad as buttons (left, right, up, down) */
> @@ -495,7 +501,30 @@ static void xpad360_process_packet(struct usb_xpad *xpad,
> input_sync(dev);
> }
>
> -static void xpad_identify_controller(struct usb_xpad *xpad);
> +static void xpad_presence_work(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: %d\n", error);
> + } else {
> + rcu_assign_pointer(xpad->x360w_dev, xpad->dev);
> + }
> + } else {
> + RCU_INIT_POINTER(xpad->x360w_dev, NULL);
> + synchronize_rcu();
> + /*
> + * Now that we are sure xpad360w_process_packet is not
> + * using input device we can get rid of it.
> + */
> + xpad_deinit_input(xpad);
> + }
> +}
>
> /*
> * xpad360w_process_packet
> @@ -513,24 +542,28 @@ static void xpad_identify_controller(struct usb_xpad *xpad);
> */
> static void xpad360w_process_packet(struct usb_xpad *xpad, u16 cmd, unsigned char *data)
> {
> + struct input_dev *dev;
> + bool present;
> +
> /* Presence change */
> if (data[0] & 0x08) {
> - if (data[1] & 0x80) {
> - xpad->pad_present = 1;
> - /*
> - * Light up the segment corresponding to
> - * controller number.
> - */
> - xpad_identify_controller(xpad);
> - } else
> - xpad->pad_present = 0;
> + present = (data[1] & 0x80) != 0;
> +
> + if (xpad->pad_present != present) {
> + xpad->pad_present = present;
> + schedule_work(&xpad->work);
> + }
> }
>
> /* Valid pad data */
> if (data[1] != 0x1)
> return;
>
> - xpad360_process_packet(xpad, cmd, &data[4]);
> + rcu_read_lock();
> + dev = rcu_dereference(xpad->x360w_dev);
> + if (dev)
> + xpad360_process_packet(xpad, dev, cmd, &data[4]);
> + rcu_read_unlock();
> }
>
> /*
> @@ -659,7 +692,7 @@ static void xpad_irq_in(struct urb *urb)
>
> switch (xpad->xtype) {
> case XTYPE_XBOX360:
> - xpad360_process_packet(xpad, 0, xpad->idata);
> + xpad360_process_packet(xpad, xpad->dev, 0, xpad->idata);
> break;
> case XTYPE_XBOX360W:
> xpad360w_process_packet(xpad, 0, xpad->idata);
> @@ -1001,14 +1034,7 @@ static int xpad_led_probe(struct usb_xpad *xpad)
> if (error)
> goto err_free_id;
>
> - if (xpad->xtype == XTYPE_XBOX360) {
> - /*
> - * Light up the segment corresponding to controller
> - * number on wired devices. On wireless we'll do that
> - * when they respond to "presence" packet.
> - */
> - xpad_identify_controller(xpad);
> - }
> + xpad_identify_controller(xpad);
>
> return 0;
>
> @@ -1097,8 +1123,11 @@ static void xpad_set_up_abs(struct input_dev *input_dev, signed short abs)
>
> static void xpad_deinit_input(struct usb_xpad *xpad)
> {
> - xpad_led_disconnect(xpad);
> - input_unregister_device(xpad->dev);
> + if (xpad->input_created) {
> + xpad->input_created = false;
> + xpad_led_disconnect(xpad);
> + input_unregister_device(xpad->dev);
> + }
> }
>
> static int xpad_init_input(struct usb_xpad *xpad)
> @@ -1181,6 +1210,7 @@ static int xpad_init_input(struct usb_xpad *xpad)
> if (error)
> goto err_disconnect_led;
>
> + xpad->input_created = true;
> return 0;
>
> err_disconnect_led:
> @@ -1241,6 +1271,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, xpad_presence_work);
>
> if (xpad->xtype == XTYPE_UNKNOWN) {
> if (intf->cur_altsetting->desc.bInterfaceClass == USB_CLASS_VENDOR_SPEC) {
> @@ -1277,10 +1308,6 @@ static int xpad_probe(struct usb_interface *intf, const struct usb_device_id *id
>
> usb_set_intfdata(intf, xpad);
>
> - error = xpad_init_input(xpad);
> - if (error)
> - goto err_deinit_output;
> -
> if (xpad->xtype == XTYPE_XBOX360W) {
> /*
> * Submit the int URB immediately rather than waiting for open
> @@ -1292,7 +1319,7 @@ 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 err_deinit_input;
> + goto err_deinit_output;
>
> /*
> * Send presence packet.
> @@ -1304,13 +1331,15 @@ static int xpad_probe(struct usb_interface *intf, const struct usb_device_id *id
> error = xpad_inquiry_pad_presence(xpad);
> if (error)
> goto err_kill_in_urb;
> + } else {
> + error = xpad_init_input(xpad);
> + if (error)
> + goto err_deinit_output;
> }
> return 0;
>
> err_kill_in_urb:
> usb_kill_urb(xpad->irq_in);
> -err_deinit_input:
> - xpad_deinit_input(xpad);
> err_deinit_output:
> xpad_deinit_output(xpad);
> err_free_in_urb:
> @@ -1327,17 +1356,19 @@ static void xpad_disconnect(struct usb_interface *intf)
> {
> struct usb_xpad *xpad = usb_get_intfdata (intf);
>
> - xpad_deinit_input(xpad);
> - xpad_deinit_output(xpad);
> -
> - if (xpad->xtype == XTYPE_XBOX360W) {
> + if (xpad->xtype == XTYPE_XBOX360W)
> usb_kill_urb(xpad->irq_in);
> - }
> +
> + cancel_work_sync(&xpad->work);
> +
> + xpad_deinit_input(xpad);
>
> usb_free_urb(xpad->irq_in);
> usb_free_coherent(xpad->udev, XPAD_PKT_LEN,
> xpad->idata, xpad->idata_dma);
>
> + xpad_deinit_output(xpad);
> +
> kfree(xpad);
>
> usb_set_intfdata(intf, NULL);
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/5] Input: xpad: do not submit active URBs
2015-11-01 15:31 [PATCH 0/5] Input: xpad: robustness updates Pavel Rojtberg
2015-11-01 15:31 ` [PATCH 1/5] Input: xpad: handle "present" and "gone" correctly Pavel Rojtberg
@ 2015-11-01 15:31 ` Pavel Rojtberg
2015-11-01 15:31 ` [PATCH 3/5] Input: xpad: re-submit pending ff and led requests Pavel Rojtberg
` (2 subsequent siblings)
4 siblings, 0 replies; 13+ messages in thread
From: Pavel Rojtberg @ 2015-11-01 15:31 UTC (permalink / raw)
To: linux-input, pgriffais, dmitry.torokhov, gregkh; +Cc: 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 locking around xpad->odata usages: as we can not use
mutexes in xpad_play_effect, replace the mutex by a spinlock.
Signed-off-by: Pavel Rojtberg <rojtberg@gmail.com>
---
drivers/input/joystick/xpad.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++----------
1 file changed, 52 insertions(+), 10 deletions(-)
diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
index 23e5613..595e3ad 100644
--- a/drivers/input/joystick/xpad.c
+++ b/drivers/input/joystick/xpad.c
@@ -329,9 +329,10 @@ 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;
+ spinlock_t odata_lock;
#if defined(CONFIG_JOYSTICK_XPAD_LEDS)
struct xpad_led *led;
@@ -708,6 +709,7 @@ static void xpad_irq_out(struct urb *urb)
switch (status) {
case 0:
/* success */
+ xpad->irq_out_active = 0;
return;
case -ECONNRESET:
@@ -716,6 +718,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:
@@ -747,7 +750,7 @@ static int xpad_init_output(struct usb_interface *intf, struct usb_xpad *xpad)
goto fail1;
}
- mutex_init(&xpad->odata_mutex);
+ spin_lock_init(&xpad->odata_lock);
xpad->irq_out = usb_alloc_urb(0, GFP_KERNEL);
if (!xpad->irq_out) {
@@ -790,8 +793,9 @@ static void xpad_deinit_output(struct usb_xpad *xpad)
static int xpad_inquiry_pad_presence(struct usb_xpad *xpad)
{
int retval;
+ unsigned long flags;
- mutex_lock(&xpad->odata_mutex);
+ spin_lock_irqsave(&xpad->odata_lock, flags);
xpad->odata[0] = 0x08;
xpad->odata[1] = 0x00;
@@ -807,9 +811,16 @@ static int xpad_inquiry_pad_presence(struct usb_xpad *xpad)
xpad->odata[11] = 0x00;
xpad->irq_out->transfer_buffer_length = 12;
- retval = usb_submit_urb(xpad->irq_out, GFP_KERNEL);
+ if (!xpad->irq_out_active) {
+ retval = 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__);
+ retval = -EIO;
+ }
- mutex_unlock(&xpad->odata_mutex);
+ spin_unlock_irqrestore(&xpad->odata_lock, flags);
return retval;
}
@@ -820,6 +831,9 @@ 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;
+
+ unsigned long flags;
if (effect->type != FF_RUMBLE)
return 0;
@@ -827,6 +841,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;
+ spin_lock_irqsave(&xpad->odata_lock, flags);
+
switch (xpad->xtype) {
case XTYPE_XBOX:
xpad->odata[0] = 0x00;
@@ -883,13 +899,25 @@ static int xpad_play_effect(struct input_dev *dev, void *data, struct ff_effect
break;
default:
+ spin_unlock_irqrestore(&xpad->odata_lock, flags);
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__);
+ }
+
+ spin_unlock_irqrestore(&xpad->odata_lock, flags);
+
+ return retval;
}
static int xpad_init_ff(struct usb_xpad *xpad)
@@ -940,9 +968,11 @@ struct xpad_led {
*/
static void xpad_send_led_command(struct usb_xpad *xpad, int command)
{
+ unsigned long flags;
+
command %= 16;
- mutex_lock(&xpad->odata_mutex);
+ spin_lock_irqsave(&xpad->odata_lock, flags);
switch (xpad->xtype) {
case XTYPE_XBOX360:
@@ -968,8 +998,14 @@ static void xpad_send_led_command(struct usb_xpad *xpad, int command)
break;
}
- usb_submit_urb(xpad->irq_out, GFP_KERNEL);
- mutex_unlock(&xpad->odata_mutex);
+ 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__);
+
+ spin_unlock_irqrestore(&xpad->odata_lock, flags);
}
/*
@@ -1051,6 +1087,9 @@ 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;
+
+ unsigned long flags;
/* URB was submitted in probe */
if (xpad->xtype == XTYPE_XBOX360W)
@@ -1061,11 +1100,14 @@ static int xpad_open(struct input_dev *dev)
return -EIO;
if (xpad->xtype == XTYPE_XBOXONE) {
+ spin_lock_irqsave(&xpad->odata_lock, flags);
/* 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);
+ spin_unlock_irqrestore(&xpad->odata_lock, flags);
+ return retval;
}
return 0;
--
1.9.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH 3/5] Input: xpad: re-submit pending ff and led requests
2015-11-01 15:31 [PATCH 0/5] Input: xpad: robustness updates Pavel Rojtberg
2015-11-01 15:31 ` [PATCH 1/5] Input: xpad: handle "present" and "gone" correctly Pavel Rojtberg
2015-11-01 15:31 ` [PATCH 2/5] Input: xpad: do not submit active URBs Pavel Rojtberg
@ 2015-11-01 15:31 ` Pavel Rojtberg
2015-12-10 6:40 ` Dmitry Torokhov
2015-11-01 15:31 ` [PATCH 4/5] Input: xpad: workaround dead irq_out after suspend/ resume Pavel Rojtberg
2015-11-01 15:31 ` [PATCH 5/5] Input: xpad: update Xbox One Force Feedback Support Pavel Rojtberg
4 siblings, 1 reply; 13+ messages in thread
From: Pavel Rojtberg @ 2015-11-01 15:31 UTC (permalink / raw)
To: linux-input, pgriffais, dmitry.torokhov, gregkh; +Cc: Pavel Rojtberg
From: Pavel Rojtberg <rojtberg@gmail.com>
Store pending brightness and FF effect in the driver structure and
replace it with the latest requests until the device is ready to process
next request. Alternate serving LED vs FF requests to make sure one does
not starve another. See [1] for discussion. Inspired by patch of Sarah
Bessmer [2].
[1]: http://www.spinics.net/lists/linux-input/msg40708.html
[2]: http://www.spinics.net/lists/linux-input/msg31450.html
Signed-off-by: Pavel Rojtberg <rojtberg@gmail.com>
---
drivers/input/joystick/xpad.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++----------
1 file changed, 56 insertions(+), 10 deletions(-)
diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
index 595e3ad..f3754c7 100644
--- a/drivers/input/joystick/xpad.c
+++ b/drivers/input/joystick/xpad.c
@@ -334,6 +334,12 @@ struct usb_xpad {
dma_addr_t odata_dma;
spinlock_t odata_lock;
+ unsigned char rum_odata[XPAD_PKT_LEN]; /* cache for rumble data */
+ unsigned char led_odata[XPAD_PKT_LEN]; /* cache for led data */
+ unsigned pend_rum; /* length of cached rumble data */
+ unsigned pend_led; /* length of cached led data */
+ int force_led; /* force send led cache next */
+
#if defined(CONFIG_JOYSTICK_XPAD_LEDS)
struct xpad_led *led;
#endif
@@ -703,14 +709,35 @@ 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 */
- xpad->irq_out_active = 0;
- return;
+ if (!xpad->pend_led && !xpad->pend_rum) {
+ xpad->irq_out_active = 0;
+ return;
+ }
+
+ spin_lock_irqsave(&xpad->odata_lock, flags);
+
+ if (xpad->pend_led && (!xpad->pend_rum || xpad->force_led)) {
+ xpad->irq_out->transfer_buffer_length = xpad->pend_led;
+ memcpy(xpad->odata, xpad->led_odata, xpad->pend_led);
+ xpad->pend_led = 0;
+ xpad->force_led = 0;
+ dev_dbg(dev, "%s - sending pending led\n", __func__);
+ break;
+ }
+
+ xpad->irq_out->transfer_buffer_length = xpad->pend_rum;
+ memcpy(xpad->odata, xpad->rum_odata, xpad->pend_rum);
+ xpad->pend_rum = 0;
+ xpad->force_led = 1;
+ dev_dbg(dev, "%s - sending pending rumble\n", __func__);
+ break;
case -ECONNRESET:
case -ENOENT:
@@ -724,11 +751,13 @@ static void xpad_irq_out(struct urb *urb)
default:
dev_dbg(dev, "%s - nonzero urb status received: %d\n",
__func__, status);
- goto exit;
+
+ spin_lock_irqsave(&xpad->odata_lock, flags);
+ break;
}
-exit:
retval = usb_submit_urb(urb, GFP_ATOMIC);
+ spin_unlock_irqrestore(&xpad->odata_lock, flags);
if (retval)
dev_err(dev, "%s - usb_submit_urb failed with result %d\n",
__func__, retval);
@@ -751,6 +780,9 @@ static int xpad_init_output(struct usb_interface *intf, struct usb_xpad *xpad)
}
spin_lock_init(&xpad->odata_lock);
+ xpad->pend_led = 0;
+ xpad->pend_rum = 0;
+ xpad->force_led = 0;
xpad->irq_out = usb_alloc_urb(0, GFP_KERNEL);
if (!xpad->irq_out) {
@@ -910,9 +942,17 @@ static int xpad_play_effect(struct input_dev *dev, void *data, struct ff_effect
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__);
+ retval = 0;
+
+ if (xpad->pend_rum) {
+ dev_dbg(&xpad->dev->dev,
+ "%s - overwriting pending\n", __func__);
+
+ retval = -EIO;
+ }
+
+ xpad->pend_rum = xpad->irq_out->transfer_buffer_length;
+ memcpy(xpad->rum_odata, xpad->odata, xpad->pend_rum);
}
spin_unlock_irqrestore(&xpad->odata_lock, flags);
@@ -1001,9 +1041,15 @@ static void xpad_send_led_command(struct usb_xpad *xpad, int command)
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__);
+ } else {
+ if (xpad->pend_led) {
+ dev_dbg(&xpad->dev->dev,
+ "%s - overwriting pending\n", __func__);
+ }
+
+ xpad->pend_led = xpad->irq_out->transfer_buffer_length;
+ memcpy(xpad->led_odata, xpad->odata, xpad->pend_led);
+ }
spin_unlock_irqrestore(&xpad->odata_lock, flags);
}
--
1.9.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH 3/5] Input: xpad: re-submit pending ff and led requests
2015-11-01 15:31 ` [PATCH 3/5] Input: xpad: re-submit pending ff and led requests Pavel Rojtberg
@ 2015-12-10 6:40 ` Dmitry Torokhov
2015-12-25 23:37 ` Pavel Rojtberg
0 siblings, 1 reply; 13+ messages in thread
From: Dmitry Torokhov @ 2015-12-10 6:40 UTC (permalink / raw)
To: Pavel Rojtberg; +Cc: linux-input, pgriffais, gregkh, Laura Abbott
Hi Pavel,
On Sun, Nov 01, 2015 at 04:31:37PM +0100, Pavel Rojtberg wrote:
> @@ -910,9 +942,17 @@ static int xpad_play_effect(struct input_dev *dev, void *data, struct ff_effect
> 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__);
> + retval = 0;
> +
> + if (xpad->pend_rum) {
> + dev_dbg(&xpad->dev->dev,
> + "%s - overwriting pending\n", __func__);
> +
> + retval = -EIO;
Why do we return -EIO here?
> + }
> +
> + xpad->pend_rum = xpad->irq_out->transfer_buffer_length;
> + memcpy(xpad->rum_odata, xpad->odata, xpad->pend_rum);
This is wrong: you first copy into xpad->odata which means that you may
alter the buffer while device is fetching the previous packet it.
Can you please try the version of the patch below? I squashed your #2
and #3 patches together and adjusted the behavior to avoid the issue
above.
The patches are also in 'xpad' branch of my tree.
Thanks!
--
Dmitry
Input: xpad - correctly handle concurrent LED and FF requests
From: Pavel Rojtberg <rojtberg@gmail.com>
Track the status of the irq_out URB to prevent submission iof new requests
while current one is active. Failure to do so results in the "URB submitted
while active" warning/stack trace.
Store pending brightness and FF effect in the driver structure and replace
it with the latest requests until the device is ready to process next
request. Alternate serving LED vs FF requests to make sure one does not
starve another. See [1] for discussion. Inspired by patch of Sarah Bessmer
[2].
[1]: http://www.spinics.net/lists/linux-input/msg40708.html
[2]: http://www.spinics.net/lists/linux-input/msg31450.html
Signed-off-by: Pavel Rojtberg <rojtberg@gmail.com>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/joystick/xpad.c | 320 ++++++++++++++++++++++++++++-------------
1 file changed, 221 insertions(+), 99 deletions(-)
diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
index 1013c5c..4a7362e 100644
--- a/drivers/input/joystick/xpad.c
+++ b/drivers/input/joystick/xpad.c
@@ -317,6 +317,19 @@ static struct usb_device_id xpad_table[] = {
MODULE_DEVICE_TABLE(usb, xpad_table);
+struct xpad_output_packet {
+ u8 data[XPAD_PKT_LEN];
+ u8 len;
+ bool pending;
+};
+
+#define XPAD_OUT_CMD_IDX 0
+#define XPAD_OUT_FF_IDX 1
+#define XPAD_OUT_LED_IDX (1 + IS_ENABLED(CONFIG_JOYSTICK_XPAD_FF))
+#define XPAD_NUM_OUT_PACKETS (1 + \
+ IS_ENABLED(CONFIG_JOYSTICK_XPAD_FF) + \
+ IS_ENABLED(CONFIG_JOYSTICK_XPAD_LEDS))
+
struct usb_xpad {
struct input_dev *dev; /* input device interface */
struct usb_device *udev; /* usb device */
@@ -329,9 +342,13 @@ struct usb_xpad {
dma_addr_t idata_dma;
struct urb *irq_out; /* urb for interrupt out report */
+ bool irq_out_active; /* we must not use an active URB */
unsigned char *odata; /* output data */
dma_addr_t odata_dma;
- struct mutex odata_mutex;
+ spinlock_t odata_lock;
+
+ struct xpad_output_packet out_packets[XPAD_NUM_OUT_PACKETS];
+ int last_out_packet;
#if defined(CONFIG_JOYSTICK_XPAD_LEDS)
struct xpad_led *led;
@@ -695,18 +712,71 @@ exit:
__func__, retval);
}
+/* Callers must hold xpad->odata_lock spinlock */
+static bool xpad_prepare_next_out_packet(struct usb_xpad *xpad)
+{
+ struct xpad_output_packet *pkt, *packet = NULL;
+ int i;
+
+ for (i = 0; i < XPAD_NUM_OUT_PACKETS; i++) {
+ if (++xpad->last_out_packet >= XPAD_NUM_OUT_PACKETS)
+ xpad->last_out_packet = 0;
+
+ pkt = &xpad->out_packets[xpad->last_out_packet];
+ if (pkt->pending) {
+ dev_dbg(&xpad->intf->dev,
+ "%s - found pending output packet %d\n",
+ __func__, xpad->last_out_packet);
+ packet = pkt;
+ break;
+ }
+ }
+
+ if (packet) {
+ memcpy(xpad->odata, packet->data, packet->len);
+ xpad->irq_out->transfer_buffer_length = packet->len;
+ return true;
+ }
+
+ return false;
+}
+
+/* Callers must hold xpad->odata_lock spinlock */
+static int xpad_try_sending_next_out_packet(struct usb_xpad *xpad)
+{
+ int error;
+
+ if (!xpad->irq_out_active && xpad_prepare_next_out_packet(xpad)) {
+ error = usb_submit_urb(xpad->irq_out, GFP_ATOMIC);
+ if (error) {
+ dev_err(&xpad->intf->dev,
+ "%s - usb_submit_urb failed with result %d\n",
+ __func__, error);
+ return -EIO;
+ }
+
+ xpad->irq_out_active = true;
+ }
+
+ return 0;
+}
+
static void xpad_irq_out(struct urb *urb)
{
struct usb_xpad *xpad = urb->context;
struct device *dev = &xpad->intf->dev;
- int retval, status;
+ int status = urb->status;
+ int error;
+ unsigned long flags;
- status = urb->status;
+ spin_lock_irqsave(&xpad->odata_lock, flags);
switch (status) {
case 0:
/* success */
- return;
+ xpad->out_packets[xpad->last_out_packet].pending = false;
+ xpad->irq_out_active = xpad_prepare_next_out_packet(xpad);
+ break;
case -ECONNRESET:
case -ENOENT:
@@ -714,19 +784,26 @@ 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);
- return;
+ xpad->irq_out_active = false;
+ break;
default:
dev_dbg(dev, "%s - nonzero urb status received: %d\n",
__func__, status);
- goto exit;
+ break;
}
-exit:
- retval = usb_submit_urb(urb, GFP_ATOMIC);
- if (retval)
- dev_err(dev, "%s - usb_submit_urb failed with result %d\n",
- __func__, retval);
+ if (xpad->irq_out_active) {
+ error = usb_submit_urb(urb, GFP_ATOMIC);
+ if (error) {
+ dev_err(dev,
+ "%s - usb_submit_urb failed with result %d\n",
+ __func__, error);
+ xpad->irq_out_active = false;
+ }
+ }
+
+ spin_unlock_irqrestore(&xpad->odata_lock, flags);
}
static int xpad_init_output(struct usb_interface *intf, struct usb_xpad *xpad)
@@ -745,7 +822,7 @@ static int xpad_init_output(struct usb_interface *intf, struct usb_xpad *xpad)
goto fail1;
}
- mutex_init(&xpad->odata_mutex);
+ spin_lock_init(&xpad->odata_lock);
xpad->irq_out = usb_alloc_urb(0, GFP_KERNEL);
if (!xpad->irq_out) {
@@ -787,27 +864,55 @@ static void xpad_deinit_output(struct usb_xpad *xpad)
static int xpad_inquiry_pad_presence(struct usb_xpad *xpad)
{
+ struct xpad_output_packet *packet =
+ &xpad->out_packets[XPAD_OUT_CMD_IDX];
+ unsigned long flags;
int retval;
- 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;
+ spin_lock_irqsave(&xpad->odata_lock, flags);
+
+ packet->data[0] = 0x08;
+ packet->data[1] = 0x00;
+ packet->data[2] = 0x0F;
+ packet->data[3] = 0xC0;
+ packet->data[4] = 0x00;
+ packet->data[5] = 0x00;
+ packet->data[6] = 0x00;
+ packet->data[7] = 0x00;
+ packet->data[8] = 0x00;
+ packet->data[9] = 0x00;
+ packet->data[10] = 0x00;
+ packet->data[11] = 0x00;
+ packet->len = 12;
+ packet->pending = true;
+
+ /* Reset the sequence so we send out presence first */
+ xpad->last_out_packet = -1;
+ retval = xpad_try_sending_next_out_packet(xpad);
+
+ spin_unlock_irqrestore(&xpad->odata_lock, flags);
+
+ return retval;
+}
+
+static int xpad_start_xbox_one(struct usb_xpad *xpad)
+{
+ struct xpad_output_packet *packet =
+ &xpad->out_packets[XPAD_OUT_CMD_IDX];
+ unsigned long flags;
+ int retval;
+
+ spin_lock_irqsave(&xpad->odata_lock, flags);
+
+ /* Xbox one controller needs to be initialized. */
+ packet->data[0] = 0x05;
+ packet->data[1] = 0x20;
+ packet->len = 2;
+ packet->pending = true;
retval = usb_submit_urb(xpad->irq_out, GFP_KERNEL);
- mutex_unlock(&xpad->odata_mutex);
+ spin_unlock_irqrestore(&xpad->odata_lock, flags);
return retval;
}
@@ -816,8 +921,11 @@ static int xpad_inquiry_pad_presence(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);
+ struct xpad_output_packet *packet = &xpad->out_packets[XPAD_OUT_FF_IDX];
__u16 strong;
__u16 weak;
+ int retval;
+ unsigned long flags;
if (effect->type != FF_RUMBLE)
return 0;
@@ -825,69 +933,80 @@ 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;
+ spin_lock_irqsave(&xpad->odata_lock, flags);
+
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;
+ packet->data[0] = 0x00;
+ packet->data[1] = 0x06;
+ packet->data[2] = 0x00;
+ packet->data[3] = strong / 256; /* left actuator */
+ packet->data[4] = 0x00;
+ packet->data[5] = weak / 256; /* right actuator */
+ packet->len = 6;
+ packet->pending = true;
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;
+ packet->data[0] = 0x00;
+ packet->data[1] = 0x08;
+ packet->data[2] = 0x00;
+ packet->data[3] = strong / 256; /* left actuator? */
+ packet->data[4] = weak / 256; /* right actuator? */
+ packet->data[5] = 0x00;
+ packet->data[6] = 0x00;
+ packet->data[7] = 0x00;
+ packet->len = 8;
+ packet->pending = true;
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;
+ packet->data[0] = 0x00;
+ packet->data[1] = 0x01;
+ packet->data[2] = 0x0F;
+ packet->data[3] = 0xC0;
+ packet->data[4] = 0x00;
+ packet->data[5] = strong / 256;
+ packet->data[6] = weak / 256;
+ packet->data[7] = 0x00;
+ packet->data[8] = 0x00;
+ packet->data[9] = 0x00;
+ packet->data[10] = 0x00;
+ packet->data[11] = 0x00;
+ packet->len = 12;
+ packet->pending = true;
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;
+ packet->data[0] = 0x09; /* activate rumble */
+ packet->data[1] = 0x08;
+ packet->data[2] = 0x00;
+ packet->data[3] = 0x08; /* continuous effect */
+ packet->data[4] = 0x00; /* simple rumble mode */
+ packet->data[5] = 0x03; /* L and R actuator only */
+ packet->data[6] = 0x00; /* TODO: LT actuator */
+ packet->data[7] = 0x00; /* TODO: RT actuator */
+ packet->data[8] = strong / 256; /* left actuator */
+ packet->data[9] = weak / 256; /* right actuator */
+ packet->data[10] = 0x80; /* length of pulse */
+ packet->data[11] = 0x00; /* stop period of pulse */
+ packet->len = 12;
+ packet->pending = true;
break;
default:
dev_dbg(&xpad->dev->dev,
"%s - rumble command sent to unsupported xpad type: %d\n",
__func__, xpad->xtype);
- return -EINVAL;
+ retval = -EINVAL;
+ goto out;
}
- return usb_submit_urb(xpad->irq_out, GFP_ATOMIC);
+ retval = xpad_try_sending_next_out_packet(xpad);
+
+out:
+ spin_unlock_irqrestore(&xpad->odata_lock, flags);
+ return retval;
}
static int xpad_init_ff(struct usb_xpad *xpad)
@@ -938,36 +1057,44 @@ struct xpad_led {
*/
static void xpad_send_led_command(struct usb_xpad *xpad, int command)
{
+ struct xpad_output_packet *packet =
+ &xpad->out_packets[XPAD_OUT_LED_IDX];
+ unsigned long flags;
+
command %= 16;
- mutex_lock(&xpad->odata_mutex);
+ spin_lock_irqsave(&xpad->odata_lock, flags);
switch (xpad->xtype) {
case XTYPE_XBOX360:
- xpad->odata[0] = 0x01;
- xpad->odata[1] = 0x03;
- xpad->odata[2] = command;
- xpad->irq_out->transfer_buffer_length = 3;
+ packet->data[0] = 0x01;
+ packet->data[1] = 0x03;
+ packet->data[2] = command;
+ packet->len = 3;
+ packet->pending = true;
break;
+
case XTYPE_XBOX360W:
- xpad->odata[0] = 0x00;
- xpad->odata[1] = 0x00;
- xpad->odata[2] = 0x08;
- xpad->odata[3] = 0x40 + command;
- 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;
+ packet->data[0] = 0x00;
+ packet->data[1] = 0x00;
+ packet->data[2] = 0x08;
+ packet->data[3] = 0x40 + command;
+ packet->data[4] = 0x00;
+ packet->data[5] = 0x00;
+ packet->data[6] = 0x00;
+ packet->data[7] = 0x00;
+ packet->data[8] = 0x00;
+ packet->data[9] = 0x00;
+ packet->data[10] = 0x00;
+ packet->data[11] = 0x00;
+ packet->len = 12;
+ packet->pending = true;
break;
}
- usb_submit_urb(xpad->irq_out, GFP_KERNEL);
- mutex_unlock(&xpad->odata_mutex);
+ xpad_try_sending_next_out_packet(xpad);
+
+ spin_unlock_irqrestore(&xpad->odata_lock, flags);
}
/*
@@ -1058,13 +1185,8 @@ static int xpad_open(struct input_dev *dev)
if (usb_submit_urb(xpad->irq_in, GFP_KERNEL))
return -EIO;
- if (xpad->xtype == XTYPE_XBOXONE) {
- /* 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);
- }
+ if (xpad->xtype == XTYPE_XBOXONE)
+ return xpad_start_xbox_one(xpad);
return 0;
}
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH 3/5] Input: xpad: re-submit pending ff and led requests
2015-12-10 6:40 ` Dmitry Torokhov
@ 2015-12-25 23:37 ` Pavel Rojtberg
0 siblings, 0 replies; 13+ messages in thread
From: Pavel Rojtberg @ 2015-12-25 23:37 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: linux-input, Pierre-Loup A. Griffais, Greg KH, Laura Abbott
2015-12-10 7:40 GMT+01:00 Dmitry Torokhov <dmitry.torokhov@gmail.com>:
> Hi Pavel,
>
> On Sun, Nov 01, 2015 at 04:31:37PM +0100, Pavel Rojtberg wrote:
>> @@ -910,9 +942,17 @@ static int xpad_play_effect(struct input_dev *dev, void *data, struct ff_effect
>> 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__);
>> + retval = 0;
>> +
>> + if (xpad->pend_rum) {
>> + dev_dbg(&xpad->dev->dev,
>> + "%s - overwriting pending\n", __func__);
>> +
>> + retval = -EIO;
>
> Why do we return -EIO here?
>
>> + }
>> +
>> + xpad->pend_rum = xpad->irq_out->transfer_buffer_length;
>> + memcpy(xpad->rum_odata, xpad->odata, xpad->pend_rum);
>
> This is wrong: you first copy into xpad->odata which means that you may
> alter the buffer while device is fetching the previous packet it.
>
> Can you please try the version of the patch below? I squashed your #2
> and #3 patches together and adjusted the behavior to avoid the issue
> above.
>
> The patches are also in 'xpad' branch of my tree.
>
> Thanks!
>
> --
> Dmitry
>
> Input: xpad - correctly handle concurrent LED and FF requests
>
> From: Pavel Rojtberg <rojtberg@gmail.com>
>
> Track the status of the irq_out URB to prevent submission iof new requests
> while current one is active. Failure to do so results in the "URB submitted
> while active" warning/stack trace.
>
> Store pending brightness and FF effect in the driver structure and replace
> it with the latest requests until the device is ready to process next
> request. Alternate serving LED vs FF requests to make sure one does not
> starve another. See [1] for discussion. Inspired by patch of Sarah Bessmer
> [2].
>
> [1]: http://www.spinics.net/lists/linux-input/msg40708.html
> [2]: http://www.spinics.net/lists/linux-input/msg31450.html
>
> Signed-off-by: Pavel Rojtberg <rojtberg@gmail.com>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
> drivers/input/joystick/xpad.c | 320 ++++++++++++++++++++++++++++-------------
> 1 file changed, 221 insertions(+), 99 deletions(-)
>
> diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
> index 1013c5c..4a7362e 100644
> --- a/drivers/input/joystick/xpad.c
> +++ b/drivers/input/joystick/xpad.c
> @@ -317,6 +317,19 @@ static struct usb_device_id xpad_table[] = {
>
> MODULE_DEVICE_TABLE(usb, xpad_table);
>
> +struct xpad_output_packet {
> + u8 data[XPAD_PKT_LEN];
> + u8 len;
> + bool pending;
> +};
> +
> +#define XPAD_OUT_CMD_IDX 0
> +#define XPAD_OUT_FF_IDX 1
> +#define XPAD_OUT_LED_IDX (1 + IS_ENABLED(CONFIG_JOYSTICK_XPAD_FF))
> +#define XPAD_NUM_OUT_PACKETS (1 + \
> + IS_ENABLED(CONFIG_JOYSTICK_XPAD_FF) + \
> + IS_ENABLED(CONFIG_JOYSTICK_XPAD_LEDS))
> +
> struct usb_xpad {
> struct input_dev *dev; /* input device interface */
> struct usb_device *udev; /* usb device */
> @@ -329,9 +342,13 @@ struct usb_xpad {
> dma_addr_t idata_dma;
>
> struct urb *irq_out; /* urb for interrupt out report */
> + bool irq_out_active; /* we must not use an active URB */
> unsigned char *odata; /* output data */
> dma_addr_t odata_dma;
> - struct mutex odata_mutex;
> + spinlock_t odata_lock;
> +
> + struct xpad_output_packet out_packets[XPAD_NUM_OUT_PACKETS];
> + int last_out_packet;
>
> #if defined(CONFIG_JOYSTICK_XPAD_LEDS)
> struct xpad_led *led;
> @@ -695,18 +712,71 @@ exit:
> __func__, retval);
> }
>
> +/* Callers must hold xpad->odata_lock spinlock */
> +static bool xpad_prepare_next_out_packet(struct usb_xpad *xpad)
> +{
> + struct xpad_output_packet *pkt, *packet = NULL;
> + int i;
> +
> + for (i = 0; i < XPAD_NUM_OUT_PACKETS; i++) {
> + if (++xpad->last_out_packet >= XPAD_NUM_OUT_PACKETS)
> + xpad->last_out_packet = 0;
> +
> + pkt = &xpad->out_packets[xpad->last_out_packet];
> + if (pkt->pending) {
> + dev_dbg(&xpad->intf->dev,
> + "%s - found pending output packet %d\n",
> + __func__, xpad->last_out_packet);
> + packet = pkt;
> + break;
> + }
> + }
> +
> + if (packet) {
> + memcpy(xpad->odata, packet->data, packet->len);
> + xpad->irq_out->transfer_buffer_length = packet->len;
> + return true;
> + }
> +
> + return false;
> +}
> +
> +/* Callers must hold xpad->odata_lock spinlock */
> +static int xpad_try_sending_next_out_packet(struct usb_xpad *xpad)
> +{
> + int error;
> +
> + if (!xpad->irq_out_active && xpad_prepare_next_out_packet(xpad)) {
> + error = usb_submit_urb(xpad->irq_out, GFP_ATOMIC);
> + if (error) {
> + dev_err(&xpad->intf->dev,
> + "%s - usb_submit_urb failed with result %d\n",
> + __func__, error);
> + return -EIO;
> + }
> +
> + xpad->irq_out_active = true;
> + }
> +
> + return 0;
> +}
> +
> static void xpad_irq_out(struct urb *urb)
> {
> struct usb_xpad *xpad = urb->context;
> struct device *dev = &xpad->intf->dev;
> - int retval, status;
> + int status = urb->status;
> + int error;
> + unsigned long flags;
>
> - status = urb->status;
> + spin_lock_irqsave(&xpad->odata_lock, flags);
>
> switch (status) {
> case 0:
> /* success */
> - return;
> + xpad->out_packets[xpad->last_out_packet].pending = false;
> + xpad->irq_out_active = xpad_prepare_next_out_packet(xpad);
> + break;
>
> case -ECONNRESET:
> case -ENOENT:
> @@ -714,19 +784,26 @@ 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);
> - return;
> + xpad->irq_out_active = false;
> + break;
>
> default:
> dev_dbg(dev, "%s - nonzero urb status received: %d\n",
> __func__, status);
> - goto exit;
> + break;
> }
>
> -exit:
> - retval = usb_submit_urb(urb, GFP_ATOMIC);
> - if (retval)
> - dev_err(dev, "%s - usb_submit_urb failed with result %d\n",
> - __func__, retval);
> + if (xpad->irq_out_active) {
> + error = usb_submit_urb(urb, GFP_ATOMIC);
> + if (error) {
> + dev_err(dev,
> + "%s - usb_submit_urb failed with result %d\n",
> + __func__, error);
> + xpad->irq_out_active = false;
> + }
> + }
> +
> + spin_unlock_irqrestore(&xpad->odata_lock, flags);
> }
>
> static int xpad_init_output(struct usb_interface *intf, struct usb_xpad *xpad)
> @@ -745,7 +822,7 @@ static int xpad_init_output(struct usb_interface *intf, struct usb_xpad *xpad)
> goto fail1;
> }
>
> - mutex_init(&xpad->odata_mutex);
> + spin_lock_init(&xpad->odata_lock);
>
> xpad->irq_out = usb_alloc_urb(0, GFP_KERNEL);
> if (!xpad->irq_out) {
> @@ -787,27 +864,55 @@ static void xpad_deinit_output(struct usb_xpad *xpad)
>
> static int xpad_inquiry_pad_presence(struct usb_xpad *xpad)
> {
> + struct xpad_output_packet *packet =
> + &xpad->out_packets[XPAD_OUT_CMD_IDX];
> + unsigned long flags;
> int retval;
>
> - 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;
> + spin_lock_irqsave(&xpad->odata_lock, flags);
> +
> + packet->data[0] = 0x08;
> + packet->data[1] = 0x00;
> + packet->data[2] = 0x0F;
> + packet->data[3] = 0xC0;
> + packet->data[4] = 0x00;
> + packet->data[5] = 0x00;
> + packet->data[6] = 0x00;
> + packet->data[7] = 0x00;
> + packet->data[8] = 0x00;
> + packet->data[9] = 0x00;
> + packet->data[10] = 0x00;
> + packet->data[11] = 0x00;
> + packet->len = 12;
> + packet->pending = true;
> +
> + /* Reset the sequence so we send out presence first */
> + xpad->last_out_packet = -1;
> + retval = xpad_try_sending_next_out_packet(xpad);
> +
> + spin_unlock_irqrestore(&xpad->odata_lock, flags);
> +
> + return retval;
> +}
> +
> +static int xpad_start_xbox_one(struct usb_xpad *xpad)
> +{
> + struct xpad_output_packet *packet =
> + &xpad->out_packets[XPAD_OUT_CMD_IDX];
> + unsigned long flags;
> + int retval;
> +
> + spin_lock_irqsave(&xpad->odata_lock, flags);
> +
> + /* Xbox one controller needs to be initialized. */
> + packet->data[0] = 0x05;
> + packet->data[1] = 0x20;
> + packet->len = 2;
> + packet->pending = true;
>
> retval = usb_submit_urb(xpad->irq_out, GFP_KERNEL);
after having taking a closer look at this part of code, you should
also replace the line above with:
xpad->last_out_packet = -1;
retval = xpad_try_sending_next_out_packet(xpad);
>
> - mutex_unlock(&xpad->odata_mutex);
> + spin_unlock_irqrestore(&xpad->odata_lock, flags);
>
> return retval;
> }
> @@ -816,8 +921,11 @@ static int xpad_inquiry_pad_presence(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);
> + struct xpad_output_packet *packet = &xpad->out_packets[XPAD_OUT_FF_IDX];
> __u16 strong;
> __u16 weak;
> + int retval;
> + unsigned long flags;
>
> if (effect->type != FF_RUMBLE)
> return 0;
> @@ -825,69 +933,80 @@ 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;
>
> + spin_lock_irqsave(&xpad->odata_lock, flags);
> +
> 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;
> + packet->data[0] = 0x00;
> + packet->data[1] = 0x06;
> + packet->data[2] = 0x00;
> + packet->data[3] = strong / 256; /* left actuator */
> + packet->data[4] = 0x00;
> + packet->data[5] = weak / 256; /* right actuator */
> + packet->len = 6;
> + packet->pending = true;
> 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;
> + packet->data[0] = 0x00;
> + packet->data[1] = 0x08;
> + packet->data[2] = 0x00;
> + packet->data[3] = strong / 256; /* left actuator? */
> + packet->data[4] = weak / 256; /* right actuator? */
> + packet->data[5] = 0x00;
> + packet->data[6] = 0x00;
> + packet->data[7] = 0x00;
> + packet->len = 8;
> + packet->pending = true;
> 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;
> + packet->data[0] = 0x00;
> + packet->data[1] = 0x01;
> + packet->data[2] = 0x0F;
> + packet->data[3] = 0xC0;
> + packet->data[4] = 0x00;
> + packet->data[5] = strong / 256;
> + packet->data[6] = weak / 256;
> + packet->data[7] = 0x00;
> + packet->data[8] = 0x00;
> + packet->data[9] = 0x00;
> + packet->data[10] = 0x00;
> + packet->data[11] = 0x00;
> + packet->len = 12;
> + packet->pending = true;
> 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;
> + packet->data[0] = 0x09; /* activate rumble */
> + packet->data[1] = 0x08;
> + packet->data[2] = 0x00;
> + packet->data[3] = 0x08; /* continuous effect */
> + packet->data[4] = 0x00; /* simple rumble mode */
> + packet->data[5] = 0x03; /* L and R actuator only */
> + packet->data[6] = 0x00; /* TODO: LT actuator */
> + packet->data[7] = 0x00; /* TODO: RT actuator */
> + packet->data[8] = strong / 256; /* left actuator */
> + packet->data[9] = weak / 256; /* right actuator */
> + packet->data[10] = 0x80; /* length of pulse */
> + packet->data[11] = 0x00; /* stop period of pulse */
> + packet->len = 12;
> + packet->pending = true;
> break;
>
> default:
> dev_dbg(&xpad->dev->dev,
> "%s - rumble command sent to unsupported xpad type: %d\n",
> __func__, xpad->xtype);
> - return -EINVAL;
> + retval = -EINVAL;
> + goto out;
> }
>
> - return usb_submit_urb(xpad->irq_out, GFP_ATOMIC);
> + retval = xpad_try_sending_next_out_packet(xpad);
> +
> +out:
> + spin_unlock_irqrestore(&xpad->odata_lock, flags);
> + return retval;
> }
>
> static int xpad_init_ff(struct usb_xpad *xpad)
> @@ -938,36 +1057,44 @@ struct xpad_led {
> */
> static void xpad_send_led_command(struct usb_xpad *xpad, int command)
> {
> + struct xpad_output_packet *packet =
> + &xpad->out_packets[XPAD_OUT_LED_IDX];
> + unsigned long flags;
> +
> command %= 16;
>
> - mutex_lock(&xpad->odata_mutex);
> + spin_lock_irqsave(&xpad->odata_lock, flags);
>
> switch (xpad->xtype) {
> case XTYPE_XBOX360:
> - xpad->odata[0] = 0x01;
> - xpad->odata[1] = 0x03;
> - xpad->odata[2] = command;
> - xpad->irq_out->transfer_buffer_length = 3;
> + packet->data[0] = 0x01;
> + packet->data[1] = 0x03;
> + packet->data[2] = command;
> + packet->len = 3;
> + packet->pending = true;
> break;
> +
> case XTYPE_XBOX360W:
> - xpad->odata[0] = 0x00;
> - xpad->odata[1] = 0x00;
> - xpad->odata[2] = 0x08;
> - xpad->odata[3] = 0x40 + command;
> - 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;
> + packet->data[0] = 0x00;
> + packet->data[1] = 0x00;
> + packet->data[2] = 0x08;
> + packet->data[3] = 0x40 + command;
> + packet->data[4] = 0x00;
> + packet->data[5] = 0x00;
> + packet->data[6] = 0x00;
> + packet->data[7] = 0x00;
> + packet->data[8] = 0x00;
> + packet->data[9] = 0x00;
> + packet->data[10] = 0x00;
> + packet->data[11] = 0x00;
> + packet->len = 12;
> + packet->pending = true;
> break;
> }
>
> - usb_submit_urb(xpad->irq_out, GFP_KERNEL);
> - mutex_unlock(&xpad->odata_mutex);
> + xpad_try_sending_next_out_packet(xpad);
> +
> + spin_unlock_irqrestore(&xpad->odata_lock, flags);
> }
>
> /*
> @@ -1058,13 +1185,8 @@ static int xpad_open(struct input_dev *dev)
> if (usb_submit_urb(xpad->irq_in, GFP_KERNEL))
> return -EIO;
>
> - if (xpad->xtype == XTYPE_XBOXONE) {
> - /* 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);
> - }
> + if (xpad->xtype == XTYPE_XBOXONE)
> + return xpad_start_xbox_one(xpad);
>
> return 0;
> }
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 4/5] Input: xpad: workaround dead irq_out after suspend/ resume
2015-11-01 15:31 [PATCH 0/5] Input: xpad: robustness updates Pavel Rojtberg
` (2 preceding siblings ...)
2015-11-01 15:31 ` [PATCH 3/5] Input: xpad: re-submit pending ff and led requests Pavel Rojtberg
@ 2015-11-01 15:31 ` Pavel Rojtberg
2015-12-10 6:41 ` Dmitry Torokhov
2015-11-01 15:31 ` [PATCH 5/5] Input: xpad: update Xbox One Force Feedback Support Pavel Rojtberg
4 siblings, 1 reply; 13+ messages in thread
From: Pavel Rojtberg @ 2015-11-01 15:31 UTC (permalink / raw)
To: linux-input, pgriffais, dmitry.torokhov, gregkh; +Cc: Pavel Rojtberg
From: Pavel Rojtberg <rojtberg@gmail.com>
the irq_out urb is dead after suspend/ resume on my x360 wr pad. (also
reproduced by Zachary Lund [0]) Work around this by resetting the usb
device on resume. Added suspend/ resume callbacks to do so.
[0]: https://github.com/paroj/xpad/issues/6
Signed-off-by: Pavel Rojtberg <rojtberg@gmail.com>
---
drivers/input/joystick/xpad.c | 30 ++++++++++++++++++++++++++----
1 file changed, 26 insertions(+), 4 deletions(-)
diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
index f3754c7..e8dcc80 100644
--- a/drivers/input/joystick/xpad.c
+++ b/drivers/input/joystick/xpad.c
@@ -1423,17 +1423,23 @@ err_free_mem:
}
-static void xpad_disconnect(struct usb_interface *intf)
+static void xpad_stop_communication(struct usb_xpad *xpad)
{
- struct usb_xpad *xpad = usb_get_intfdata (intf);
-
- xpad_deinit_output(xpad);
+ xpad_stop_output(xpad);
if (xpad->xtype == XTYPE_XBOX360W) {
usb_kill_urb(xpad->irq_in);
}
cancel_work_sync(&xpad->work);
+}
+
+static void xpad_disconnect(struct usb_interface *intf)
+{
+ struct usb_xpad *xpad = usb_get_intfdata(intf);
+
+ xpad_stop_communication(xpad);
+ xpad_deinit_output(xpad);
if (xpad->pad_present)
xpad_deinit_input(xpad);
@@ -1447,10 +1453,26 @@ static void xpad_disconnect(struct usb_interface *intf)
usb_set_intfdata(intf, NULL);
}
+static int xpad_suspend(struct usb_interface *intf, pm_message_t message)
+{
+ struct usb_xpad *xpad = usb_get_intfdata(intf);
+
+ xpad_stop_communication(xpad);
+ return 0;
+}
+
+static int xpad_resume(struct usb_interface *intf)
+{
+ usb_queue_reset_device(intf);
+ return 0;
+}
+
static struct usb_driver xpad_driver = {
.name = "xpad",
.probe = xpad_probe,
.disconnect = xpad_disconnect,
+ .suspend = xpad_suspend,
+ .resume = xpad_resume,
.id_table = xpad_table,
};
--
1.9.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH 4/5] Input: xpad: workaround dead irq_out after suspend/ resume
2015-11-01 15:31 ` [PATCH 4/5] Input: xpad: workaround dead irq_out after suspend/ resume Pavel Rojtberg
@ 2015-12-10 6:41 ` Dmitry Torokhov
2015-12-17 1:09 ` Dmitry Torokhov
0 siblings, 1 reply; 13+ messages in thread
From: Dmitry Torokhov @ 2015-12-10 6:41 UTC (permalink / raw)
To: Pavel Rojtberg; +Cc: linux-input, pgriffais, gregkh
On Sun, Nov 01, 2015 at 04:31:38PM +0100, Pavel Rojtberg wrote:
> From: Pavel Rojtberg <rojtberg@gmail.com>
>
> the irq_out urb is dead after suspend/ resume on my x360 wr pad. (also
> reproduced by Zachary Lund [0]) Work around this by resetting the usb
> device on resume. Added suspend/ resume callbacks to do so.
>
> [0]: https://github.com/paroj/xpad/issues/6
>
> Signed-off-by: Pavel Rojtberg <rojtberg@gmail.com>
> ---
> drivers/input/joystick/xpad.c | 30 ++++++++++++++++++++++++++----
> 1 file changed, 26 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
> index f3754c7..e8dcc80 100644
> --- a/drivers/input/joystick/xpad.c
> +++ b/drivers/input/joystick/xpad.c
> @@ -1423,17 +1423,23 @@ err_free_mem:
>
> }
>
> -static void xpad_disconnect(struct usb_interface *intf)
> +static void xpad_stop_communication(struct usb_xpad *xpad)
> {
> - struct usb_xpad *xpad = usb_get_intfdata (intf);
> -
> - xpad_deinit_output(xpad);
> + xpad_stop_output(xpad);
>
> if (xpad->xtype == XTYPE_XBOX360W) {
> usb_kill_urb(xpad->irq_in);
> }
>
> cancel_work_sync(&xpad->work);
> +}
> +
> +static void xpad_disconnect(struct usb_interface *intf)
> +{
> + struct usb_xpad *xpad = usb_get_intfdata(intf);
> +
> + xpad_stop_communication(xpad);
> + xpad_deinit_output(xpad);
>
> if (xpad->pad_present)
> xpad_deinit_input(xpad);
> @@ -1447,10 +1453,26 @@ static void xpad_disconnect(struct usb_interface *intf)
> usb_set_intfdata(intf, NULL);
> }
>
> +static int xpad_suspend(struct usb_interface *intf, pm_message_t message)
> +{
> + struct usb_xpad *xpad = usb_get_intfdata(intf);
> +
> + xpad_stop_communication(xpad);
> + return 0;
> +}
> +
> +static int xpad_resume(struct usb_interface *intf)
> +{
> + usb_queue_reset_device(intf);
Why do we have to force reset (and tear down the device)? I am sure we
can resume it properly.
Thanks.
> + return 0;
> +}
> +
> static struct usb_driver xpad_driver = {
> .name = "xpad",
> .probe = xpad_probe,
> .disconnect = xpad_disconnect,
> + .suspend = xpad_suspend,
> + .resume = xpad_resume,
> .id_table = xpad_table,
> };
>
> --
> 1.9.1
>
--
Dmitry
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 4/5] Input: xpad: workaround dead irq_out after suspend/ resume
2015-12-10 6:41 ` Dmitry Torokhov
@ 2015-12-17 1:09 ` Dmitry Torokhov
0 siblings, 0 replies; 13+ messages in thread
From: Dmitry Torokhov @ 2015-12-17 1:09 UTC (permalink / raw)
To: Pavel Rojtberg; +Cc: linux-input, pgriffais, gregkh
On Wed, Dec 09, 2015 at 10:41:31PM -0800, Dmitry Torokhov wrote:
> On Sun, Nov 01, 2015 at 04:31:38PM +0100, Pavel Rojtberg wrote:
> > From: Pavel Rojtberg <rojtberg@gmail.com>
...
> > +
> > +static int xpad_resume(struct usb_interface *intf)
> > +{
> > + usb_queue_reset_device(intf);
>
> Why do we have to force reset (and tear down the device)? I am sure we
> can resume it properly.
Does the patch below work for you?
--
Dmitry
Input: xpad - workaround dead irq_out after suspend/ resume
From: Pavel Rojtberg <rojtberg@gmail.com>
The irq_out urb is dead after suspend/ resume on my x360 wr pad. (also
reproduced by Zachary Lund [0]) Work around this by implementing suspend
and resume callbacks and properly shutting down URBs on suspend and
restarting them on resume.
[0]: https://github.com/paroj/xpad/issues/6
Signed-off-by: Pavel Rojtberg <rojtberg@gmail.com>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/joystick/xpad.c | 156 ++++++++++++++++++++++++++++++-----------
1 file changed, 114 insertions(+), 42 deletions(-)
diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
index f88cf00..36328b3 100644
--- a/drivers/input/joystick/xpad.c
+++ b/drivers/input/joystick/xpad.c
@@ -864,12 +864,6 @@ static int xpad_init_output(struct usb_interface *intf, struct usb_xpad *xpad)
fail1: return error;
}
-static void xpad_stop_output(struct usb_xpad *xpad)
-{
- if (xpad->xtype != XTYPE_UNKNOWN)
- usb_kill_urb(xpad->irq_out);
-}
-
static void xpad_deinit_output(struct usb_xpad *xpad)
{
if (xpad->xtype != XTYPE_UNKNOWN) {
@@ -930,7 +924,7 @@ static int xpad_start_xbox_one(struct usb_xpad *xpad)
packet->len = 5;
packet->pending = true;
- retval = usb_submit_urb(xpad->irq_out, GFP_KERNEL);
+ retval = usb_submit_urb(xpad->irq_out, GFP_KERNEL) ? -EIO : 0;
spin_unlock_irqrestore(&xpad->odata_lock, flags);
@@ -1194,32 +1188,73 @@ static void xpad_led_disconnect(struct usb_xpad *xpad) { }
static void xpad_identify_controller(struct usb_xpad *xpad) { }
#endif
-static int xpad_open(struct input_dev *dev)
+static int xpad_start_input(struct usb_xpad *xpad)
{
- struct usb_xpad *xpad = input_get_drvdata(dev);
-
- /* URB was submitted in probe */
- if (xpad->xtype == XTYPE_XBOX360W)
- return 0;
+ int error;
- xpad->irq_in->dev = xpad->udev;
if (usb_submit_urb(xpad->irq_in, GFP_KERNEL))
return -EIO;
- if (xpad->xtype == XTYPE_XBOXONE)
- return xpad_start_xbox_one(xpad);
+ if (xpad->xtype == XTYPE_XBOXONE) {
+ error = xpad_start_xbox_one(xpad);
+ if (error) {
+ usb_kill_urb(xpad->irq_in);
+ return error;
+ }
+ }
return 0;
}
-static void xpad_close(struct input_dev *dev)
+static void xpad_stop_input(struct usb_xpad *xpad)
{
- struct usb_xpad *xpad = input_get_drvdata(dev);
+ usb_kill_urb(xpad->irq_in);
+}
+
+static int xpad360w_start_input(struct usb_xpad *xpad)
+{
+ int error;
- if (xpad->xtype != XTYPE_XBOX360W)
+ error = usb_submit_urb(xpad->irq_in, GFP_KERNEL);
+ if (error)
+ return -EIO;
+
+ /*
+ * 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.
+ */
+ error = xpad_inquiry_pad_presence(xpad);
+ if (error) {
usb_kill_urb(xpad->irq_in);
+ return error;
+ }
+
+ return 0;
+}
+
+static void xpad360w_stop_input(struct usb_xpad *xpad)
+{
+ usb_kill_urb(xpad->irq_in);
+
+ /* Make sure we are done with presence work if it was scheduled */
+ flush_work(&xpad->work);
+}
+
+static int xpad_open(struct input_dev *dev)
+{
+ struct usb_xpad *xpad = input_get_drvdata(dev);
+
+ return xpad_start_input(xpad);
+}
- xpad_stop_output(xpad);
+static void xpad_close(struct input_dev *dev)
+{
+ struct usb_xpad *xpad = input_get_drvdata(dev);
+
+ xpad_stop_input(xpad);
}
static void xpad_set_up_abs(struct input_dev *input_dev, signed short abs)
@@ -1274,8 +1309,10 @@ static int xpad_init_input(struct usb_xpad *xpad)
input_set_drvdata(input_dev, xpad);
- input_dev->open = xpad_open;
- input_dev->close = xpad_close;
+ if (xpad->xtype != XTYPE_XBOX360W) {
+ input_dev->open = xpad_open;
+ input_dev->close = xpad_close;
+ }
__set_bit(EV_KEY, input_dev->evbit);
@@ -1443,21 +1480,9 @@ static int xpad_probe(struct usb_interface *intf, const struct usb_device_id *id
* exactly the message that a controller has arrived that
* we're waiting for.
*/
- xpad->irq_in->dev = xpad->udev;
- error = usb_submit_urb(xpad->irq_in, GFP_KERNEL);
+ error = xpad360w_start_input(xpad);
if (error)
goto err_deinit_output;
-
- /*
- * 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.
- */
- error = xpad_inquiry_pad_presence(xpad);
- if (error)
- goto err_kill_in_urb;
} else {
error = xpad_init_input(xpad);
if (error)
@@ -1465,8 +1490,6 @@ static int xpad_probe(struct usb_interface *intf, const struct usb_device_id *id
}
return 0;
-err_kill_in_urb:
- usb_kill_urb(xpad->irq_in);
err_deinit_output:
xpad_deinit_output(xpad);
err_free_in_urb:
@@ -1476,20 +1499,24 @@ err_free_idata:
err_free_mem:
kfree(xpad);
return error;
-
}
static void xpad_disconnect(struct usb_interface *intf)
{
- struct usb_xpad *xpad = usb_get_intfdata (intf);
+ struct usb_xpad *xpad = usb_get_intfdata(intf);
if (xpad->xtype == XTYPE_XBOX360W)
- usb_kill_urb(xpad->irq_in);
-
- cancel_work_sync(&xpad->work);
+ xpad360w_stop_input(xpad);
xpad_deinit_input(xpad);
+ /*
+ * Now that both input device and LED device are gone we can
+ * stop output URB.
+ */
+ if (xpad->xtype == XTYPE_XBOX360W)
+ usb_kill_urb(xpad->irq_out);
+
usb_free_urb(xpad->irq_in);
usb_free_coherent(xpad->udev, XPAD_PKT_LEN,
xpad->idata, xpad->idata_dma);
@@ -1501,10 +1528,55 @@ static void xpad_disconnect(struct usb_interface *intf)
usb_set_intfdata(intf, NULL);
}
+static int xpad_suspend(struct usb_interface *intf, pm_message_t message)
+{
+ struct usb_xpad *xpad = usb_get_intfdata(intf);
+ struct input_dev *input = xpad->dev;
+
+ if (xpad->xtype == XTYPE_XBOX360W) {
+ /*
+ * Wireless controllers always listen to input so
+ * they are notified when controller shows up
+ * or goes away.
+ */
+ xpad360w_stop_input(xpad);
+ } else {
+ mutex_lock(&input->mutex);
+ if (input->users)
+ xpad_stop_input(xpad);
+ mutex_unlock(&input->mutex);
+ }
+
+ if (xpad->xtype != XTYPE_UNKNOWN)
+ usb_kill_urb(xpad->irq_out);
+
+ return 0;
+}
+
+static int xpad_resume(struct usb_interface *intf)
+{
+ struct usb_xpad *xpad = usb_get_intfdata(intf);
+ struct input_dev *input = xpad->dev;
+ int retval = 0;
+
+ if (xpad->xtype == XTYPE_XBOX360W) {
+ retval = xpad360w_start_input(xpad);
+ } else {
+ mutex_lock(&input->mutex);
+ if (input->users)
+ retval = xpad_start_input(xpad);
+ mutex_unlock(&input->mutex);
+ }
+
+ return retval;
+}
+
static struct usb_driver xpad_driver = {
.name = "xpad",
.probe = xpad_probe,
.disconnect = xpad_disconnect,
+ .suspend = xpad_suspend,
+ .resume = xpad_resume,
.id_table = xpad_table,
};
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 5/5] Input: xpad: update Xbox One Force Feedback Support
2015-11-01 15:31 [PATCH 0/5] Input: xpad: robustness updates Pavel Rojtberg
` (3 preceding siblings ...)
2015-11-01 15:31 ` [PATCH 4/5] Input: xpad: workaround dead irq_out after suspend/ resume Pavel Rojtberg
@ 2015-11-01 15:31 ` Pavel Rojtberg
4 siblings, 0 replies; 13+ messages in thread
From: Pavel Rojtberg @ 2015-11-01 15:31 UTC (permalink / raw)
To: linux-input, pgriffais, dmitry.torokhov, gregkh
Cc: Pierre-Loup A. Griffais, Pavel Rojtberg
From: "Pierre-Loup A. Griffais" <githubpublic@plagman.net>
There's apparently a serial number woven into both input and output
packets; neglecting to specify a valid serial number causes the
controller to ignore the rumble packets.
The scale of the rumble was also apparently halved in the packets.
The initialization packet had to be changed to allow force feedback to
work.
see https://github.com/paroj/xpad/issues/7 for details.
Signed-off-by: Pavel Rojtberg <rojtberg@gmail.com>
---
drivers/input/joystick/xpad.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
index e8dcc80..c1107c1 100644
--- a/drivers/input/joystick/xpad.c
+++ b/drivers/input/joystick/xpad.c
@@ -351,6 +351,7 @@ struct usb_xpad {
int pad_nr; /* the order x360 pads were attached */
const char *name; /* name of the device */
struct work_struct work; /* init/remove device from callback */
+ unsigned char odata_serial; /* serial number for xbox one protocol */
};
static int xpad_init_input(struct usb_xpad *xpad);
@@ -917,17 +918,18 @@ static int xpad_play_effect(struct input_dev *dev, void *data, struct ff_effect
case XTYPE_XBOXONE:
xpad->odata[0] = 0x09; /* activate rumble */
xpad->odata[1] = 0x08;
- xpad->odata[2] = 0x00;
+ xpad->odata[2] = xpad->odata_serial++;
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[8] = strong / 512; /* left actuator */
+ xpad->odata[9] = weak / 512; /* right actuator */
xpad->odata[10] = 0x80; /* length of pulse */
xpad->odata[11] = 0x00; /* stop period of pulse */
- xpad->irq_out->transfer_buffer_length = 12;
+ xpad->odata[12] = 0x00;
+ xpad->irq_out->transfer_buffer_length = 13;
break;
default:
@@ -1150,7 +1152,10 @@ static int xpad_open(struct input_dev *dev)
/* Xbox one controller needs to be initialized. */
xpad->odata[0] = 0x05;
xpad->odata[1] = 0x20;
- xpad->irq_out->transfer_buffer_length = 2;
+ xpad->odata[2] = xpad->odata_serial++; /* packet serial */
+ xpad->odata[3] = 0x01; /* rumble bit enable? */
+ xpad->odata[4] = 0x00;
+ xpad->irq_out->transfer_buffer_length = 5;
retval = usb_submit_urb(xpad->irq_out, GFP_KERNEL);
spin_unlock_irqrestore(&xpad->odata_lock, flags);
return retval;
--
1.9.1
^ permalink raw reply related [flat|nested] 13+ messages in thread