From: Pavel Rojtberg <rojtberg@gmail.com>
To: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: linux-input@vger.kernel.org,
"Pierre-Loup A. Griffais" <pgriffais@valvesoftware.com>,
Greg KH <gregkh@linuxfoundation.org>,
Laura Abbott <labbott@fedoraproject.org>
Subject: Re: [PATCH 3/5] Input: xpad: re-submit pending ff and led requests
Date: Sat, 26 Dec 2015 00:37:55 +0100 [thread overview]
Message-ID: <CA+b0ujdg4YxLkg3v8DBVip47-4v_0Ttej4cFXjFz-ZPiaPb8+g@mail.gmail.com> (raw)
In-Reply-To: <20151210064032.GA35505@dtor-ws>
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;
> }
next prev parent reply other threads:[~2015-12-25 23:38 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
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-12-10 7:02 ` Dmitry Torokhov
2015-12-14 23:29 ` Dmitry Torokhov
2015-12-18 2:01 ` Pavel Rojtberg
2015-11-01 15:31 ` [PATCH 2/5] Input: xpad: do not submit active URBs Pavel Rojtberg
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 [this message]
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
2015-11-01 15:31 ` [PATCH 5/5] Input: xpad: update Xbox One Force Feedback Support Pavel Rojtberg
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CA+b0ujdg4YxLkg3v8DBVip47-4v_0Ttej4cFXjFz-ZPiaPb8+g@mail.gmail.com \
--to=rojtberg@gmail.com \
--cc=dmitry.torokhov@gmail.com \
--cc=gregkh@linuxfoundation.org \
--cc=labbott@fedoraproject.org \
--cc=linux-input@vger.kernel.org \
--cc=pgriffais@valvesoftware.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.