2015-12-10 7:40 GMT+01:00 Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>:
> 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@xxxxxxxxx>
>
> 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@xxxxxxxxx>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>
> ---
> 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;
> }
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
