All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH 0/1] USB: bugfix on interrupt xfers with usb-redir
@ 2019-07-24 12:58 Martin Cerveny
  2019-07-24 12:58 ` [Qemu-devel] [PATCH 1/1] usb-redir: merge interrupt packets Martin Cerveny
  0 siblings, 1 reply; 3+ messages in thread
From: Martin Cerveny @ 2019-07-24 12:58 UTC (permalink / raw)
  To: Gerd Hoffmann; +Cc: qemu-devel, Martin Cerveny

I have problem in xen with qemu xhci with usbredir backend.
Windows bluetooth (BCM20703) driver does not work without proposed patch.
Interrupt EP does not work as expected and described in USB spec.

usb_20.pdf/5.7.3 Interrupt Transfer Packet Size Constraint:
----
An endpoint must always transmit data payloads with a data field less than or equal to the endpoint’s
wMaxPacketSize value. A device can move data via an interrupt pipe that is larger than wMaxPacketSize.
A software client can accept this data via an IRP for the interrupt transfer that requires multiple bus
transactions without requiring an IRP-complete notification per transaction. This can be achieved by
specifying a buffer that can hold the desired data size. The size of the buffer is a multiple of
wMaxPacketSize with some remainder. The endpoint must transfer each transaction except the last as
wMaxPacketSize and the last transaction is the remainder. The multiple data transactions are moved over
the bus at the period established for the pipe.
When an interrupt transfer involves more data than can fit in one data payload of the currently established
maximum size, all data payloads are required to be maximum-sized except for the last data payload, which
will contain the remaining data. An interrupt transfer is complete when the endpoint does one of the
following:
• Has transferred exactly the amount of data expected
• Transfers a packet with a payload size less than wMaxPacketSize or transfers a zero-length packet
----

Examples of affected device on windows usbpcap decoded with wireshark:

- snip of configuration descriptor:
----
ENDPOINT DESCRIPTOR
    bLength: 7
    bDescriptorType: 0x05 (ENDPOINT)
    bEndpointAddress: 0x81  IN  Endpoint:1
        1... .... = Direction: IN Endpoint
        .... 0001 = Endpoint Number: 0x1
    bmAttributes: 0x03
        .... ..11 = Transfertype: Interrupt-Transfer (0x3)
    wMaxPacketSize: 16
        ...0 0... .... .... = Transactions per microframe: 1 (0)
        .... ..00 0001 0000 = Maximum Packet Size: 16
    bInterval: 1
----


- snip of two correct URB interrupts (len 70 and len 16) from non-virtualized communication and patched qemu:
----
USB URB
    [Source: 1.6.1]
    [Destination: host]
    USBPcap pseudoheader length: 27
    IRP ID: 0xffffa901ed380050
    IRP USBD_STATUS: USBD_STATUS_SUCCESS (0x00000000)
    URB Function: URB_FUNCTION_BULK_OR_INTERRUPT_TRANSFER (0x0009)
    IRP information: 0x01, Direction: PDO -> FDO
        0000 000. = Reserved: 0x00
        .... ...1 = Direction: PDO -> FDO (0x1)
    URB bus id: 1
    Device address: 6
    Endpoint: 0x81, Direction: IN
        1... .... = Direction: IN (1)
        .... 0001 = Endpoint number: 1
    URB transfer type: URB_INTERRUPT (0x01)
    Packet Data Length: 70
    [Request in: 43377]
    [Time from request: 0.006005000 seconds]
    [bInterfaceClass: Vendor Specific (0xff)]
Leftover Capture Data: 0e4401021000ffffff03ccffefffffffec1ff20fe8fe3ff7...
USB URB
    [Source: 1.6.1]
    [Destination: host]
    USBPcap pseudoheader length: 27
    IRP ID: 0xffffa901ed380050
    IRP USBD_STATUS: USBD_STATUS_SUCCESS (0x00000000)
    URB Function: URB_FUNCTION_BULK_OR_INTERRUPT_TRANSFER (0x0009)
    IRP information: 0x01, Direction: PDO -> FDO
        0000 000. = Reserved: 0x00
        .... ...1 = Direction: PDO -> FDO (0x1)
    URB bus id: 1
    Device address: 6
    Endpoint: 0x81, Direction: IN
        1... .... = Direction: IN (1)
        .... 0001 = Endpoint number: 1
    URB transfer type: URB_INTERRUPT (0x01)
    Packet Data Length: 16
    [Request in: 43405]
    [Time from request: 0.002952000 seconds]
    [bInterfaceClass: Vendor Specific (0xff)]
Leftover Capture Data: 0e0e0104100001020000000000000000
----


- snip of the same two (more URB 70=16+16+16+16+6, 16=16+0) in actual qemu:
----
USB URB
    [Source: 1.4.1]
    [Destination: host]
    USBPcap pseudoheader length: 27
    IRP ID: 0xffffc5062ede69f0
    IRP USBD_STATUS: USBD_STATUS_SUCCESS (0x00000000)
    URB Function: URB_FUNCTION_BULK_OR_INTERRUPT_TRANSFER (0x0009)
    IRP information: 0x01, Direction: PDO -> FDO
        0000 000. = Reserved: 0x00
        .... ...1 = Direction: PDO -> FDO (0x1)
    URB bus id: 1
    Device address: 4
    Endpoint: 0x81, Direction: IN
        1... .... = Direction: IN (1)
        .... 0001 = Endpoint number: 1
    URB transfer type: URB_INTERRUPT (0x01)
    Packet Data Length: 16
    [Request in: 72930]
    [Time from request: 0.004881000 seconds]
    [bInterfaceClass: Vendor Specific (0xff)]
Leftover Capture Data: 0e4401021000ffffff03ccffefffffff
USB URB
    [Source: 1.4.1]
    [Destination: host]
    USBPcap pseudoheader length: 27
    IRP ID: 0xffffc5062ede69f0
    IRP USBD_STATUS: USBD_STATUS_SUCCESS (0x00000000)
    URB Function: URB_FUNCTION_BULK_OR_INTERRUPT_TRANSFER (0x0009)
    IRP information: 0x01, Direction: PDO -> FDO
        0000 000. = Reserved: 0x00
        .... ...1 = Direction: PDO -> FDO (0x1)
    URB bus id: 1
    Device address: 4
    Endpoint: 0x81, Direction: IN
        1... .... = Direction: IN (1)
        .... 0001 = Endpoint number: 1
    URB transfer type: URB_INTERRUPT (0x01)
    Packet Data Length: 16
    [Request in: 72947]
    [Time from request: 0.004244000 seconds]
    [bInterfaceClass: Vendor Specific (0xff)]
Leftover Capture Data: ec1ff20fe8fe3ff78fff1c00040061f7
USB URB
    [Source: 1.4.1]
    [Destination: host]
    USBPcap pseudoheader length: 27
    IRP ID: 0xffffc5062ede69f0
    IRP USBD_STATUS: USBD_STATUS_SUCCESS (0x00000000)
    URB Function: URB_FUNCTION_BULK_OR_INTERRUPT_TRANSFER (0x0009)
    IRP information: 0x01, Direction: PDO -> FDO
        0000 000. = Reserved: 0x00
        .... ...1 = Direction: PDO -> FDO (0x1)
    URB bus id: 1
    Device address: 4
    Endpoint: 0x81, Direction: IN
        1... .... = Direction: IN (1)
        .... 0001 = Endpoint number: 1
    URB transfer type: URB_INTERRUPT (0x01)
    Packet Data Length: 16
    [Request in: 72957]
    [Time from request: 0.000073000 seconds]
    [bInterfaceClass: Vendor Specific (0xff)]
Leftover Capture Data: ffff7ff8ffffff3f0000000000000000
USB URB
    [Source: 1.4.1]
    [Destination: host]
    USBPcap pseudoheader length: 27
    IRP ID: 0xffffc5062ede69f0
    IRP USBD_STATUS: USBD_STATUS_SUCCESS (0x00000000)
    URB Function: URB_FUNCTION_BULK_OR_INTERRUPT_TRANSFER (0x0009)
    IRP information: 0x01, Direction: PDO -> FDO
        0000 000. = Reserved: 0x00
        .... ...1 = Direction: PDO -> FDO (0x1)
    URB bus id: 1
    Device address: 4
    Endpoint: 0x81, Direction: IN
        1... .... = Direction: IN (1)
        .... 0001 = Endpoint number: 1
    URB transfer type: URB_INTERRUPT (0x01)
    Packet Data Length: 16
    [Request in: 72959]
    [Time from request: 0.001875000 seconds]
    [bInterfaceClass: Vendor Specific (0xff)]
Leftover Capture Data: 00000000000000000000000000000000
USB URB
    [Source: 1.4.1]
    [Destination: host]
    USBPcap pseudoheader length: 27
    IRP ID: 0xffffc5062ede69f0
    IRP USBD_STATUS: USBD_STATUS_SUCCESS (0x00000000)
    URB Function: URB_FUNCTION_BULK_OR_INTERRUPT_TRANSFER (0x0009)
    IRP information: 0x01, Direction: PDO -> FDO
        0000 000. = Reserved: 0x00
        .... ...1 = Direction: PDO -> FDO (0x1)
    URB bus id: 1
    Device address: 4
    Endpoint: 0x81, Direction: IN
        1... .... = Direction: IN (1)
        .... 0001 = Endpoint number: 1
    URB transfer type: URB_INTERRUPT (0x01)
    Packet Data Length: 6
    [Request in: 72967]
    [Time from request: 0.000144000 seconds]
    [bInterfaceClass: Vendor Specific (0xff)]
Leftover Capture Data: 000000000000


USB URB
    [Source: 1.4.1]
    [Destination: host]
    USBPcap pseudoheader length: 27
    IRP ID: 0xffffc5062ede69f0
    IRP USBD_STATUS: USBD_STATUS_SUCCESS (0x00000000)
    URB Function: URB_FUNCTION_BULK_OR_INTERRUPT_TRANSFER (0x0009)
    IRP information: 0x01, Direction: PDO -> FDO
        0000 000. = Reserved: 0x00
        .... ...1 = Direction: PDO -> FDO (0x1)
    URB bus id: 1
    Device address: 4
    Endpoint: 0x81, Direction: IN
        1... .... = Direction: IN (1)
        .... 0001 = Endpoint number: 1
    URB transfer type: URB_INTERRUPT (0x01)
    Packet Data Length: 16
    [Request in: 73298]
    [Time from request: 0.005657000 seconds]
    [bInterfaceClass: Vendor Specific (0xff)]
Leftover Capture Data: 0e0e0104100001020700000000000000
USB URB
    [Source: 1.4.1]
    [Destination: host]
    USBPcap pseudoheader length: 27
    IRP ID: 0xffffc5062ede69f0
    IRP USBD_STATUS: USBD_STATUS_SUCCESS (0x00000000)
    URB Function: URB_FUNCTION_BULK_OR_INTERRUPT_TRANSFER (0x0009)
    IRP information: 0x01, Direction: PDO -> FDO
        0000 000. = Reserved: 0x00
        .... ...1 = Direction: PDO -> FDO (0x1)
    URB bus id: 1
    Device address: 4
    Endpoint: 0x81, Direction: IN
        1... .... = Direction: IN (1)
        .... 0001 = Endpoint number: 1
    URB transfer type: URB_INTERRUPT (0x01)
    Packet Data Length: 0
    [Request in: 73314]
    [Time from request: 0.001614000 seconds]
    [bInterfaceClass: Vendor Specific (0xff)]
----

I am not regular contributor. Maintainers should check and correct code or propose different solution.
Code is tested with qemu-xen (qemu-xen-4.12.0).

Regards, 

Martin 

Martin Cerveny (1):
  usb-redir: merge interrupt packets

 hw/usb/redirect.c | 69 ++++++++++++++++++++++++++++++++---------------
 1 file changed, 48 insertions(+), 21 deletions(-)

-- 
2.20.1



^ permalink raw reply	[flat|nested] 3+ messages in thread
* [Qemu-devel] [PATCH 1/1] usb-redir: merge interrupt packets
  2019-07-24 12:58 [Qemu-devel] [PATCH 0/1] USB: bugfix on interrupt xfers with usb-redir Martin Cerveny
@ 2019-07-24 12:58 ` Martin Cerveny
  2019-08-14 12:12   ` Gerd Hoffmann
  0 siblings, 1 reply; 3+ messages in thread
From: Martin Cerveny @ 2019-07-24 12:58 UTC (permalink / raw)
  To: Gerd Hoffmann; +Cc: qemu-devel, Martin Cerveny

Interrupt packets (limited by wMaxPacketSize) should be buffered and merged
by algorithm described in USB spec.
(see usb_20.pdf/5.7.3 Interrupt Transfer Packet Size Constraints).

Signed-off-by: Martin Cerveny <M.Cerveny@computer.org>
---
 hw/usb/redirect.c | 69 ++++++++++++++++++++++++++++++++---------------
 1 file changed, 48 insertions(+), 21 deletions(-)

diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
index 998fc6e4b0..0c0c86878c 100644
--- a/hw/usb/redirect.c
+++ b/hw/usb/redirect.c
@@ -815,8 +815,8 @@ static void usbredir_handle_interrupt_in_data(USBRedirDevice *dev,
                                               USBPacket *p, uint8_t ep)
 {
     /* Input interrupt endpoint, buffered packet input */
-    struct buf_packet *intp;
-    int status, len;
+    struct buf_packet *intp, *intp_to_free;
+    int status, len, sum;
 
     if (!dev->endpoint[EP2I(ep)].interrupt_started &&
             !dev->endpoint[EP2I(ep)].interrupt_error) {
@@ -835,9 +835,17 @@ static void usbredir_handle_interrupt_in_data(USBRedirDevice *dev,
         dev->endpoint[EP2I(ep)].bufpq_dropping_packets = 0;
     }
 
-    intp = QTAILQ_FIRST(&dev->endpoint[EP2I(ep)].bufpq);
+    /* check for completed interrupt message (with all fragments) */
+    sum = 0;
+    QTAILQ_FOREACH(intp, &dev->endpoint[EP2I(ep)].bufpq, next) {
+        sum += intp->len;
+        if (intp->len < dev->endpoint[EP2I(ep)].max_packet_size ||
+            sum >= p->iov.size)
+            break;
+    }
+
     if (intp == NULL) {
-        DPRINTF2("interrupt-token-in ep %02X, no intp\n", ep);
+        DPRINTF2("interrupt-token-in ep %02X, no intp, buffered %d\n", ep, sum);
         /* Check interrupt_error for stream errors */
         status = dev->endpoint[EP2I(ep)].interrupt_error;
         dev->endpoint[EP2I(ep)].interrupt_error = 0;
@@ -848,18 +856,42 @@ static void usbredir_handle_interrupt_in_data(USBRedirDevice *dev,
         }
         return;
     }
-    DPRINTF("interrupt-token-in ep %02X status %d len %d\n", ep,
-            intp->status, intp->len);
 
-    status = intp->status;
-    len = intp->len;
-    if (len > p->iov.size) {
-        ERROR("received int data is larger then packet ep %02X\n", ep);
-        len = p->iov.size;
-        status = usb_redir_babble;
+    /* copy of completed interrupt message */
+    sum = 0;
+    status = usb_redir_success;
+    intp_to_free = NULL;
+    QTAILQ_FOREACH(intp, &dev->endpoint[EP2I(ep)].bufpq, next) {
+        if (intp_to_free) {
+            bufp_free(dev, intp_to_free, ep);
+        }
+        DPRINTF("interrupt-token-in ep %02X fragment status %d len %d\n", ep,
+                intp->status, intp->len);
+
+        sum += intp->len;
+        len = intp->len;
+        if (status == usb_redir_success) {
+            status = intp->status;
+        }
+        if (sum > p->iov.size) {
+            ERROR("received int data is larger then packet ep %02X\n", ep);
+            len -= (sum - p->iov.size);
+            sum = p->iov.size;
+            status = usb_redir_babble;
+        }
+
+        usb_packet_copy(p, intp->data, len);
+
+        intp_to_free = intp;
+        if (intp->len < dev->endpoint[EP2I(ep)].max_packet_size ||
+            sum >= p->iov.size)
+            break;
     }
-    usb_packet_copy(p, intp->data, len);
-    bufp_free(dev, intp, ep);
+    if (intp_to_free) {
+        bufp_free(dev, intp_to_free, ep);
+    }
+    DPRINTF("interrupt-token-in ep %02X summary status %d len %d\n", ep,
+            status, sum);
     usbredir_handle_status(dev, p, status);
 }
 
@@ -2032,22 +2064,17 @@ static void usbredir_interrupt_packet(void *priv, uint64_t id,
     }
 
     if (ep & USB_DIR_IN) {
-        bool q_was_empty;
-
         if (dev->endpoint[EP2I(ep)].interrupt_started == 0) {
             DPRINTF("received int packet while not started ep %02X\n", ep);
             free(data);
             return;
         }
 
-        q_was_empty = QTAILQ_EMPTY(&dev->endpoint[EP2I(ep)].bufpq);
-
         /* bufp_alloc also adds the packet to the ep queue */
         bufp_alloc(dev, data, data_len, interrupt_packet->status, ep, data);
 
-        if (q_was_empty) {
-            usb_wakeup(usb_ep_get(&dev->dev, USB_TOKEN_IN, ep & 0x0f), 0);
-        }
+        /* insufficient data solved with USB_RET_NAK */
+        usb_wakeup(usb_ep_get(&dev->dev, USB_TOKEN_IN, ep & 0x0f), 0);
     } else {
         /*
          * We report output interrupt packets as completed directly upon
-- 
2.20.1



^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH 1/1] usb-redir: merge interrupt packets
  2019-07-24 12:58 ` [Qemu-devel] [PATCH 1/1] usb-redir: merge interrupt packets Martin Cerveny
@ 2019-08-14 12:12   ` Gerd Hoffmann
  0 siblings, 0 replies; 3+ messages in thread
From: Gerd Hoffmann @ 2019-08-14 12:12 UTC (permalink / raw)
  To: Martin Cerveny; +Cc: qemu-devel

On Wed, Jul 24, 2019 at 02:58:59PM +0200, Martin Cerveny wrote:
> Interrupt packets (limited by wMaxPacketSize) should be buffered and merged
> by algorithm described in USB spec.
> (see usb_20.pdf/5.7.3 Interrupt Transfer Packet Size Constraints).

Added to usb patch queue.

thanks,
  Gerd



^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-08-14 12:12 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-24 12:58 [Qemu-devel] [PATCH 0/1] USB: bugfix on interrupt xfers with usb-redir Martin Cerveny
2019-07-24 12:58 ` [Qemu-devel] [PATCH 1/1] usb-redir: merge interrupt packets Martin Cerveny
2019-08-14 12:12   ` Gerd Hoffmann

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.