All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] usb input pipelining patches v3
@ 2012-10-31 12:47 Hans de Goede
  2012-10-31 12:47 ` [Qemu-devel] [PATCH 1/8] usb: Add packet combining functions Hans de Goede
                   ` (7 more replies)
  0 siblings, 8 replies; 19+ messages in thread
From: Hans de Goede @ 2012-10-31 12:47 UTC (permalink / raw)
  To: Gerd Hoffmann; +Cc: qemu-devel

Here the not yet merged input pipelining patches. Please pull these into
your tree for Anthony.

Changes from v2:
-dropped the special combined packet device methods, as these are not
 necessary (as discussed on the list)

Thanks & Regards,

Hans

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [Qemu-devel] [PATCH 1/8] usb: Add packet combining functions
  2012-10-31 12:47 [Qemu-devel] usb input pipelining patches v3 Hans de Goede
@ 2012-10-31 12:47 ` Hans de Goede
  2012-11-01 10:08   ` Gerd Hoffmann
  2012-10-31 12:47 ` [Qemu-devel] [PATCH 2/8] combined-packet: Add a workaround for Linux usbfs + live migration Hans de Goede
                   ` (6 subsequent siblings)
  7 siblings, 1 reply; 19+ messages in thread
From: Hans de Goede @ 2012-10-31 12:47 UTC (permalink / raw)
  To: Gerd Hoffmann; +Cc: Hans de Goede, qemu-devel

Currently we only do pipelining for output endpoints, since to properly
support short-not-ok semantics we can only have one outstanding input
packet. Since the ehci and uhci controllers have a limited per td packet
size guests will split large input transfers to into multiple packets,
and since we don't pipeline these, this comes with a serious performance
penalty.

This patch adds helper functions to (re-)combine packets which belong to 1
transfer at the guest device-driver level into 1 large transger. This can be
used by (redirection) usb-devices to enable pipelining for input endpoints.

This patch will combine packets together until a transfer terminating packet
is encountered. A terminating packet is a packet which meets one or more of
the following conditions:
1) The packet size is *not* a multiple of the endpoint max packet size
2) The packet does *not* have its short-not-ok flag set
3) The packet has its interrupt-on-complete flag set

The short-not-ok flag of the combined packet is that of the terminating packet.
Multiple combined packets may be submitted to the device, if the combined
packets do not have their short-not-ok flag set, enabling true pipelining.

If a combined packet does have its short-not-ok flag set the queue will
wait with submitting further packets to the device until that packet has
completed.

Once enabled in the usb-redir and ehci code, this improves the speed (MB/s)
of a Linux guest reading from a USB mass storage device by a factor of
1.2 - 1.5.

And the main reason why I started working on this, when reading from a pl2303
USB<->serial converter, it combines the previous 4 packets submitted per
device-driver level read into 1 big read, reducing the number of packets / sec
by a factor 4, and it allows to have multiple reads outstanding. This allows
for much better latency tolerance without the pl2303's internal buffer
overflowing (which was happening at 115200 bps, without serial flow control).

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb.h                 |  13 ++++
 hw/usb/Makefile.objs     |   2 +-
 hw/usb/combined-packet.c | 179 +++++++++++++++++++++++++++++++++++++++++++++++
 hw/usb/core.c            |   1 +
 4 files changed, 194 insertions(+), 1 deletion(-)
 create mode 100644 hw/usb/combined-packet.c

diff --git a/hw/usb.h b/hw/usb.h
index 3a6cc84..a6bae3c 100644
--- a/hw/usb.h
+++ b/hw/usb.h
@@ -160,6 +160,7 @@ typedef struct USBBusOps USBBusOps;
 typedef struct USBPort USBPort;
 typedef struct USBDevice USBDevice;
 typedef struct USBPacket USBPacket;
+typedef struct USBCombinedPacket USBCombinedPacket;
 typedef struct USBEndpoint USBEndpoint;
 
 typedef struct USBDesc USBDesc;
@@ -356,7 +357,15 @@ struct USBPacket {
     int result; /* transfer length or USB_RET_* status code */
     /* Internal use by the USB layer.  */
     USBPacketState state;
+    USBCombinedPacket *combined;
     QTAILQ_ENTRY(USBPacket) queue;
+    QTAILQ_ENTRY(USBPacket) combined_entry;
+};
+
+struct USBCombinedPacket {
+    USBPacket *first;
+    QTAILQ_HEAD(packets_head, USBPacket) packets;
+    QEMUIOVector iov;
 };
 
 void usb_packet_init(USBPacket *p);
@@ -399,6 +408,10 @@ void usb_ep_set_pipeline(USBDevice *dev, int pid, int ep, bool enabled);
 USBPacket *usb_ep_find_packet_by_id(USBDevice *dev, int pid, int ep,
                                     uint64_t id);
 
+void usb_ep_combine_input_packets(USBEndpoint *ep);
+void usb_combined_input_packet_complete(USBDevice *dev, USBPacket *p);
+void usb_combined_packet_cancel(USBDevice *dev, USBPacket *p);
+
 void usb_attach(USBPort *port);
 void usb_detach(USBPort *port);
 void usb_port_reset(USBPort *port);
diff --git a/hw/usb/Makefile.objs b/hw/usb/Makefile.objs
index 4225136..b193321 100644
--- a/hw/usb/Makefile.objs
+++ b/hw/usb/Makefile.objs
@@ -7,7 +7,7 @@ hw-obj-y += libhw.o
 hw-obj-$(CONFIG_SMARTCARD) += dev-smartcard-reader.o
 hw-obj-$(CONFIG_USB_REDIR) += redirect.o
 
-common-obj-y += core.o bus.o desc.o dev-hub.o
+common-obj-y += core.o combined-packet.o bus.o desc.o dev-hub.o
 common-obj-y += host-$(HOST_USB).o dev-bluetooth.o
 common-obj-y += dev-hid.o dev-storage.o dev-wacom.o
 common-obj-y += dev-serial.o dev-network.o dev-audio.o
diff --git a/hw/usb/combined-packet.c b/hw/usb/combined-packet.c
new file mode 100644
index 0000000..652bf02
--- /dev/null
+++ b/hw/usb/combined-packet.c
@@ -0,0 +1,179 @@
+/*
+ * QEMU USB packet combining code (for input pipelining)
+ *
+ * Copyright(c) 2012 Red Hat, Inc.
+ *
+ * Red Hat Authors:
+ * Hans de Goede <hdegoede@redhat.com>
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or(at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+#include "qemu-common.h"
+#include "hw/usb.h"
+#include "iov.h"
+#include "trace.h"
+
+static void usb_combined_packet_add(USBCombinedPacket *combined, USBPacket *p)
+{
+    qemu_iovec_concat(&combined->iov, &p->iov, 0, p->iov.size);
+    QTAILQ_INSERT_TAIL(&combined->packets, p, combined_entry);
+    p->combined = combined;
+}
+
+static void usb_combined_packet_remove(USBCombinedPacket *combined,
+                                       USBPacket *p)
+{
+    assert(p->combined == combined);
+    p->combined = NULL;
+    QTAILQ_REMOVE(&combined->packets, p, combined_entry);
+}
+
+/* Also handles completion of non combined packets for pipelined input eps */
+void usb_combined_input_packet_complete(USBDevice *dev, USBPacket *p)
+{
+    USBCombinedPacket *combined = p->combined;
+    USBEndpoint *ep = p->ep;
+    USBPacket *next;
+    enum { completing, complete, leftover };
+    int result, state = completing;
+    bool short_not_ok;
+
+    if (combined == NULL) {
+        usb_packet_complete_one(dev, p);
+        goto leave;
+    }
+
+    assert(combined->first == p && p == QTAILQ_FIRST(&combined->packets));
+
+    result = combined->first->result;
+    short_not_ok = QTAILQ_LAST(&combined->packets, packets_head)->short_not_ok;
+
+    QTAILQ_FOREACH_SAFE(p, &combined->packets, combined_entry, next) {
+        if (state == completing) {
+            /* Distribute data over uncombined packets */
+            if (result >= p->iov.size) {
+                p->result = p->iov.size;
+            } else {
+                /* Send short or error packet to complete the transfer */
+                p->result = result;
+                state = complete;
+            }
+            p->short_not_ok = short_not_ok;
+            usb_combined_packet_remove(combined, p);
+            usb_packet_complete_one(dev, p);
+            result -= p->result;
+        } else {
+            /* Remove any leftover packets from the queue */
+            state = leftover;
+            p->result = USB_RET_REMOVE_FROM_QUEUE;
+            dev->port->ops->complete(dev->port, p);
+        }
+    }
+    /*
+     * If we had leftover packets the hcd driver will have cancelled them
+     * and usb_combined_packet_cancel has already freed combined!
+     */
+    if (state != leftover) {
+        g_free(combined);
+    }
+leave:
+    /* Check if there are packets in the queue waiting for our completion */
+    usb_ep_combine_input_packets(ep);
+}
+
+/* May only be called for combined packets! */
+void usb_combined_packet_cancel(USBDevice *dev, USBPacket *p)
+{
+    USBCombinedPacket *combined = p->combined;
+    assert(combined != NULL);
+
+    usb_combined_packet_remove(combined, p);
+    if (p == combined->first) {
+        usb_device_cancel_packet(dev, p);
+    }
+    if (QTAILQ_EMPTY(&combined->packets)) {
+        g_free(combined);
+    }
+}
+
+/*
+ * Large input transfers can get split into multiple input packets, this
+ * function recombines them, removing the short_not_ok checks which all but
+ * the last packet of such splits transfers have, thereby allowing input
+ * transfer pipelining (which we cannot do on short_not_ok transfers)
+ */
+void usb_ep_combine_input_packets(USBEndpoint *ep)
+{
+    USBPacket *p, *u, *next, *prev = NULL, *first = NULL;
+    USBPort *port = ep->dev->port;
+    int ret;
+
+    assert(ep->pipeline);
+    assert(ep->pid == USB_TOKEN_IN);
+
+    QTAILQ_FOREACH_SAFE(p, &ep->queue, queue, next) {
+        /* Empty the queue on a halt */
+        if (ep->halted) {
+            p->result = USB_RET_REMOVE_FROM_QUEUE;
+            port->ops->complete(port, p);
+            continue;
+        }
+
+        /* Skip packets already submitted to the device */
+        if (p->state == USB_PACKET_ASYNC) {
+            prev = p;
+            continue;
+        }
+        usb_packet_check_state(p, USB_PACKET_QUEUED);
+
+        /*
+         * If the previous (combined) packet has the short_not_ok flag set
+         * stop, as we must not submit packets to the device after a transfer
+         * ending with short_not_ok packet.
+         */
+        if (prev && prev->short_not_ok) {
+            break;
+        }
+
+        if (first) {
+            if (first->combined == NULL) {
+                USBCombinedPacket *combined = g_new0(USBCombinedPacket, 1);
+
+                combined->first = first;
+                QTAILQ_INIT(&combined->packets);
+                qemu_iovec_init(&combined->iov, 2);
+                usb_combined_packet_add(combined, first);
+            }
+            usb_combined_packet_add(first->combined, p);
+        } else {
+            first = p;
+        }
+
+        /* Is this packet the last one of a (combined) transfer? */
+        if ((p->iov.size % ep->max_packet_size) != 0 || !p->short_not_ok ||
+                next == NULL) {
+            ret = usb_device_handle_data(ep->dev, first);
+            assert(ret == USB_RET_ASYNC);
+            if (first->combined) {
+                QTAILQ_FOREACH(u, &first->combined->packets, combined_entry) {
+                    usb_packet_set_state(u, USB_PACKET_ASYNC);
+                }
+            } else {
+                usb_packet_set_state(first, USB_PACKET_ASYNC);
+            }
+            first = NULL;
+            prev = p;
+        }
+    }
+}
diff --git a/hw/usb/core.c b/hw/usb/core.c
index 632a8ef..ab37f6f 100644
--- a/hw/usb/core.c
+++ b/hw/usb/core.c
@@ -545,6 +545,7 @@ void usb_packet_setup(USBPacket *p, int pid, USBEndpoint *ep, uint64_t id,
     p->parameter = 0;
     p->short_not_ok = short_not_ok;
     p->int_req = int_req;
+    p->combined = NULL;
     qemu_iovec_reset(&p->iov);
     usb_packet_set_state(p, USB_PACKET_SETUP);
 }
-- 
1.7.12.1

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [Qemu-devel] [PATCH 2/8] combined-packet: Add a workaround for Linux usbfs + live migration
  2012-10-31 12:47 [Qemu-devel] usb input pipelining patches v3 Hans de Goede
  2012-10-31 12:47 ` [Qemu-devel] [PATCH 1/8] usb: Add packet combining functions Hans de Goede
@ 2012-10-31 12:47 ` Hans de Goede
  2012-10-31 12:47 ` [Qemu-devel] [PATCH 3/8] usb-redir: Add support for 32 bits bulk packet length Hans de Goede
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 19+ messages in thread
From: Hans de Goede @ 2012-10-31 12:47 UTC (permalink / raw)
  To: Gerd Hoffmann; +Cc: Hans de Goede, qemu-devel

Older versions (anything but the latest) of Linux usbfs + libusb(x),
will submit larger (bulk) transfers split into multiple 16k submissions,
which means that rather then all tds getting linked into the queue in
one atomic operarion they get linked in a bunch at a time, which could
cause problems if:
1) We scan the queue while libusb is in the middle of submitting a split
   bulk transfer
2) While this bulk transfer is pending we migrate to another host.

The problem is that after 2, the new host will rescan the queue and
combine the packets in one large transfer, where as 1) has caused the
original host to see them as 2 transfers. This patch fixes this by stopping
combinging if we detect a 16k transfer with its int_req flag set.

This should not adversely effect performance for other cases as:
1) Linux never sets the interrupt flag on packets other then the last
2) Windows does set the in_req flag on each td, but will submit large
transfers in 20k tds thus never triggering the check

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb/combined-packet.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/hw/usb/combined-packet.c b/hw/usb/combined-packet.c
index 652bf02..3904e71 100644
--- a/hw/usb/combined-packet.c
+++ b/hw/usb/combined-packet.c
@@ -117,7 +117,7 @@ void usb_ep_combine_input_packets(USBEndpoint *ep)
 {
     USBPacket *p, *u, *next, *prev = NULL, *first = NULL;
     USBPort *port = ep->dev->port;
-    int ret;
+    int ret, totalsize;
 
     assert(ep->pipeline);
     assert(ep->pid == USB_TOKEN_IN);
@@ -161,8 +161,11 @@ void usb_ep_combine_input_packets(USBEndpoint *ep)
         }
 
         /* Is this packet the last one of a (combined) transfer? */
+        totalsize = (p->combined) ? p->combined->iov.size : p->iov.size;
         if ((p->iov.size % ep->max_packet_size) != 0 || !p->short_not_ok ||
-                next == NULL) {
+                next == NULL ||
+                /* Work around for Linux usbfs bulk splitting + migration */
+                (totalsize == 16348 && p->int_req)) {
             ret = usb_device_handle_data(ep->dev, first);
             assert(ret == USB_RET_ASYNC);
             if (first->combined) {
-- 
1.7.12.1

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [Qemu-devel] [PATCH 3/8] usb-redir: Add support for 32 bits bulk packet length
  2012-10-31 12:47 [Qemu-devel] usb input pipelining patches v3 Hans de Goede
  2012-10-31 12:47 ` [Qemu-devel] [PATCH 1/8] usb: Add packet combining functions Hans de Goede
  2012-10-31 12:47 ` [Qemu-devel] [PATCH 2/8] combined-packet: Add a workaround for Linux usbfs + live migration Hans de Goede
@ 2012-10-31 12:47 ` Hans de Goede
  2012-10-31 12:47 ` [Qemu-devel] [PATCH 4/8] usb-redir: Add support for input pipelining Hans de Goede
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 19+ messages in thread
From: Hans de Goede @ 2012-10-31 12:47 UTC (permalink / raw)
  To: Gerd Hoffmann; +Cc: Hans de Goede, qemu-devel

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 configure         | 2 +-
 hw/usb/redirect.c | 7 ++++++-
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/configure b/configure
index 248b871..036c38a 100755
--- a/configure
+++ b/configure
@@ -2768,7 +2768,7 @@ fi
 
 # check for usbredirparser for usb network redirection support
 if test "$usb_redir" != "no" ; then
-    if $pkg_config --atleast-version=0.5 libusbredirparser-0.5 >/dev/null 2>&1 ; then
+    if $pkg_config --atleast-version=0.5.3 libusbredirparser-0.5 >/dev/null 2>&1 ; then
         usb_redir="yes"
         usb_redir_cflags=$($pkg_config --cflags libusbredirparser-0.5 2>/dev/null)
         usb_redir_libs=$($pkg_config --libs libusbredirparser-0.5 2>/dev/null)
diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
index 84b9705..a24ff63 100644
--- a/hw/usb/redirect.c
+++ b/hw/usb/redirect.c
@@ -585,6 +585,10 @@ static int usbredir_handle_bulk_data(USBRedirDevice *dev, USBPacket *p,
     bulk_packet.endpoint  = ep;
     bulk_packet.length    = p->iov.size;
     bulk_packet.stream_id = 0;
+    bulk_packet.length_high = p->iov.size >> 16;
+    assert(bulk_packet.length_high == 0 ||
+           usbredirparser_peer_has_cap(dev->parser,
+                                       usb_redir_cap_32bits_bulk_length));
 
     if (ep & USB_DIR_IN) {
         usbredirparser_send_bulk_packet(dev->parser, p->id,
@@ -906,6 +910,7 @@ static void usbredir_create_parser(USBRedirDevice *dev)
     usbredirparser_caps_set_cap(caps, usb_redir_cap_filter);
     usbredirparser_caps_set_cap(caps, usb_redir_cap_ep_info_max_packet_size);
     usbredirparser_caps_set_cap(caps, usb_redir_cap_64bits_ids);
+    usbredirparser_caps_set_cap(caps, usb_redir_cap_32bits_bulk_length);
 
     if (runstate_check(RUN_STATE_INMIGRATE)) {
         flags |= usbredirparser_fl_no_hello;
@@ -1475,7 +1480,7 @@ static void usbredir_bulk_packet(void *priv, uint64_t id,
 {
     USBRedirDevice *dev = priv;
     uint8_t ep = bulk_packet->endpoint;
-    int len = bulk_packet->length;
+    int len = (bulk_packet->length_high << 16) | bulk_packet->length;
     USBPacket *p;
 
     DPRINTF("bulk-in status %d ep %02X len %d id %"PRIu64"\n",
-- 
1.7.12.1

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [Qemu-devel] [PATCH 4/8] usb-redir: Add support for input pipelining
  2012-10-31 12:47 [Qemu-devel] usb input pipelining patches v3 Hans de Goede
                   ` (2 preceding siblings ...)
  2012-10-31 12:47 ` [Qemu-devel] [PATCH 3/8] usb-redir: Add support for 32 bits bulk packet length Hans de Goede
@ 2012-10-31 12:47 ` Hans de Goede
  2012-11-01 10:10   ` Gerd Hoffmann
  2012-10-31 12:47 ` [Qemu-devel] [PATCH 5/8] usb-redir: Add an usbredir_setup_usb_eps() helper function Hans de Goede
                   ` (3 subsequent siblings)
  7 siblings, 1 reply; 19+ messages in thread
From: Hans de Goede @ 2012-10-31 12:47 UTC (permalink / raw)
  To: Gerd Hoffmann; +Cc: Hans de Goede, qemu-devel

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb/redirect.c | 59 +++++++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 49 insertions(+), 10 deletions(-)

diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
index a24ff63..d7a08b2 100644
--- a/hw/usb/redirect.c
+++ b/hw/usb/redirect.c
@@ -29,6 +29,7 @@
 #include "qemu-timer.h"
 #include "monitor.h"
 #include "sysemu.h"
+#include "iov.h"
 
 #include <dirent.h>
 #include <sys/ioctl.h>
@@ -322,6 +323,11 @@ static void usbredir_cancel_packet(USBDevice *udev, USBPacket *p)
 {
     USBRedirDevice *dev = DO_UPCAST(USBRedirDevice, dev, udev);
 
+    if (p->combined) {
+        usb_combined_packet_cancel(udev, p);
+        return;
+    }
+
     packet_id_queue_add(&dev->cancelled, p->id);
     usbredirparser_send_cancel_data_packet(dev->parser, p->id);
     usbredirparser_do_write(dev->parser);
@@ -575,17 +581,18 @@ static int usbredir_handle_bulk_data(USBRedirDevice *dev, USBPacket *p,
                                       uint8_t ep)
 {
     struct usb_redir_bulk_packet_header bulk_packet;
+    size_t size = (p->combined) ? p->combined->iov.size : p->iov.size;
 
-    DPRINTF("bulk-out ep %02X len %zd id %"PRIu64"\n", ep, p->iov.size, p->id);
+    DPRINTF("bulk-out ep %02X len %zd id %"PRIu64"\n", ep, size, p->id);
 
     if (usbredir_already_in_flight(dev, p->id)) {
         return USB_RET_ASYNC;
     }
 
     bulk_packet.endpoint  = ep;
-    bulk_packet.length    = p->iov.size;
+    bulk_packet.length    = size;
     bulk_packet.stream_id = 0;
-    bulk_packet.length_high = p->iov.size >> 16;
+    bulk_packet.length_high = size >> 16;
     assert(bulk_packet.length_high == 0 ||
            usbredirparser_peer_has_cap(dev->parser,
                                        usb_redir_cap_32bits_bulk_length));
@@ -594,11 +601,16 @@ static int usbredir_handle_bulk_data(USBRedirDevice *dev, USBPacket *p,
         usbredirparser_send_bulk_packet(dev->parser, p->id,
                                         &bulk_packet, NULL, 0);
     } else {
-        uint8_t buf[p->iov.size];
-        usb_packet_copy(p, buf, p->iov.size);
-        usbredir_log_data(dev, "bulk data out:", buf, p->iov.size);
+        uint8_t buf[size];
+        if (p->combined) {
+            iov_to_buf(p->combined->iov.iov, p->combined->iov.niov,
+                       0, buf, size);
+        } else {
+            usb_packet_copy(p, buf, size);
+        }
+        usbredir_log_data(dev, "bulk data out:", buf, size);
         usbredirparser_send_bulk_packet(dev->parser, p->id,
-                                        &bulk_packet, buf, p->iov.size);
+                                        &bulk_packet, buf, size);
     }
     usbredirparser_do_write(dev->parser);
     return USB_RET_ASYNC;
@@ -715,6 +727,10 @@ static int usbredir_handle_data(USBDevice *udev, USBPacket *p)
     case USB_ENDPOINT_XFER_ISOC:
         return usbredir_handle_iso_data(dev, p, ep);
     case USB_ENDPOINT_XFER_BULK:
+        if (p->state == USB_PACKET_SETUP && p->pid == USB_TOKEN_IN &&
+                p->ep->pipeline) {
+            return USB_RET_ADD_TO_QUEUE;
+        }
         return usbredir_handle_bulk_data(dev, p, ep);
     case USB_ENDPOINT_XFER_INT:
         return usbredir_handle_interrupt_data(dev, p, ep);
@@ -725,6 +741,13 @@ static int usbredir_handle_data(USBDevice *udev, USBPacket *p)
     }
 }
 
+static void usbredir_flush_ep_queue(USBDevice *dev, USBEndpoint *ep)
+{
+    if (ep->pid == USB_TOKEN_IN && ep->pipeline) {
+        usb_ep_combine_input_packets(ep);
+    }
+}
+
 static int usbredir_set_config(USBRedirDevice *dev, USBPacket *p,
                                 int config)
 {
@@ -1306,6 +1329,11 @@ static void usbredir_set_pipeline(USBRedirDevice *dev, struct USBEndpoint *uep)
     if (uep->pid == USB_TOKEN_OUT) {
         uep->pipeline = true;
     }
+    if (uep->pid == USB_TOKEN_IN && uep->max_packet_size != 0 &&
+        usbredirparser_peer_has_cap(dev->parser,
+                                    usb_redir_cap_32bits_bulk_length)) {
+        uep->pipeline = true;
+    }
 }
 
 static void usbredir_ep_info(void *priv,
@@ -1488,11 +1516,17 @@ static void usbredir_bulk_packet(void *priv, uint64_t id,
 
     p = usbredir_find_packet_by_id(dev, ep, id);
     if (p) {
+        size_t size = (p->combined) ? p->combined->iov.size : p->iov.size;
         len = usbredir_handle_status(dev, bulk_packet->status, len);
         if (len > 0) {
             usbredir_log_data(dev, "bulk data in:", data, data_len);
-            if (data_len <= p->iov.size) {
-                usb_packet_copy(p, data, data_len);
+            if (data_len <= size) {
+                if (p->combined) {
+                    iov_from_buf(p->combined->iov.iov, p->combined->iov.niov,
+                                 0, data, data_len);
+                } else {
+                    usb_packet_copy(p, data, data_len);
+                }
             } else {
                 ERROR("bulk got more data then requested (%d > %zd)\n",
                       data_len, p->iov.size);
@@ -1500,7 +1534,11 @@ static void usbredir_bulk_packet(void *priv, uint64_t id,
             }
         }
         p->result = len;
-        usb_packet_complete(&dev->dev, p);
+        if (p->pid == USB_TOKEN_IN && p->ep->pipeline) {
+            usb_combined_input_packet_complete(&dev->dev, p);
+        } else {
+            usb_packet_complete(&dev->dev, p);
+        }
     }
     free(data);
 }
@@ -1907,6 +1945,7 @@ static void usbredir_class_initfn(ObjectClass *klass, void *data)
     uc->handle_reset   = usbredir_handle_reset;
     uc->handle_data    = usbredir_handle_data;
     uc->handle_control = usbredir_handle_control;
+    uc->flush_ep_queue = usbredir_flush_ep_queue;
     dc->vmsd           = &usbredir_vmstate;
     dc->props          = usbredir_properties;
 }
-- 
1.7.12.1

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [Qemu-devel] [PATCH 5/8] usb-redir: Add an usbredir_setup_usb_eps() helper function
  2012-10-31 12:47 [Qemu-devel] usb input pipelining patches v3 Hans de Goede
                   ` (3 preceding siblings ...)
  2012-10-31 12:47 ` [Qemu-devel] [PATCH 4/8] usb-redir: Add support for input pipelining Hans de Goede
@ 2012-10-31 12:47 ` Hans de Goede
  2012-10-31 12:47 ` [Qemu-devel] [PATCH 6/8] usb-redir: Use reject rather the disconnect on bad ep info Hans de Goede
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 19+ messages in thread
From: Hans de Goede @ 2012-10-31 12:47 UTC (permalink / raw)
  To: Gerd Hoffmann; +Cc: Hans de Goede, qemu-devel

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb/redirect.c | 45 ++++++++++++++++++++++-----------------------
 1 file changed, 22 insertions(+), 23 deletions(-)

diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
index d7a08b2..ade5cc6 100644
--- a/hw/usb/redirect.c
+++ b/hw/usb/redirect.c
@@ -1336,17 +1336,35 @@ static void usbredir_set_pipeline(USBRedirDevice *dev, struct USBEndpoint *uep)
     }
 }
 
+static void usbredir_setup_usb_eps(USBRedirDevice *dev)
+{
+    struct USBEndpoint *usb_ep;
+    int i, pid;
+
+    for (i = 0; i < MAX_ENDPOINTS; i++) {
+        pid = (i & 0x10) ? USB_TOKEN_IN : USB_TOKEN_OUT;
+        usb_ep = usb_ep_get(&dev->dev, pid, i & 0x0f);
+        usb_ep->type = dev->endpoint[i].type;
+        usb_ep->ifnum = dev->endpoint[i].interface;
+        usb_ep->max_packet_size = dev->endpoint[i].max_packet_size;
+        usbredir_set_pipeline(dev, usb_ep);
+    }
+}
+
 static void usbredir_ep_info(void *priv,
     struct usb_redir_ep_info_header *ep_info)
 {
     USBRedirDevice *dev = priv;
-    struct USBEndpoint *usb_ep;
     int i;
 
     for (i = 0; i < MAX_ENDPOINTS; i++) {
         dev->endpoint[i].type = ep_info->type[i];
         dev->endpoint[i].interval = ep_info->interval[i];
         dev->endpoint[i].interface = ep_info->interface[i];
+        if (usbredirparser_peer_has_cap(dev->parser,
+                                     usb_redir_cap_ep_info_max_packet_size)) {
+            dev->endpoint[i].max_packet_size = ep_info->max_packet_size[i];
+        }
         switch (dev->endpoint[i].type) {
         case usb_redir_type_invalid:
             break;
@@ -1367,18 +1385,8 @@ static void usbredir_ep_info(void *priv,
             usbredir_device_disconnect(dev);
             return;
         }
-        usb_ep = usb_ep_get(&dev->dev,
-                            (i & 0x10) ? USB_TOKEN_IN : USB_TOKEN_OUT,
-                            i & 0x0f);
-        usb_ep->type = dev->endpoint[i].type;
-        usb_ep->ifnum = dev->endpoint[i].interface;
-        if (usbredirparser_peer_has_cap(dev->parser,
-                                     usb_redir_cap_ep_info_max_packet_size)) {
-            dev->endpoint[i].max_packet_size =
-                usb_ep->max_packet_size = ep_info->max_packet_size[i];
-        }
-        usbredir_set_pipeline(dev, usb_ep);
     }
+    usbredir_setup_usb_eps(dev);
 }
 
 static void usbredir_configuration_status(void *priv, uint64_t id,
@@ -1620,8 +1628,6 @@ static void usbredir_pre_save(void *priv)
 static int usbredir_post_load(void *priv, int version_id)
 {
     USBRedirDevice *dev = priv;
-    struct USBEndpoint *usb_ep;
-    int i;
 
     switch (dev->device_info.speed) {
     case usb_redir_speed_low:
@@ -1641,15 +1647,8 @@ static int usbredir_post_load(void *priv, int version_id)
     }
     dev->dev.speedmask = (1 << dev->dev.speed);
 
-    for (i = 0; i < MAX_ENDPOINTS; i++) {
-        usb_ep = usb_ep_get(&dev->dev,
-                            (i & 0x10) ? USB_TOKEN_IN : USB_TOKEN_OUT,
-                            i & 0x0f);
-        usb_ep->type = dev->endpoint[i].type;
-        usb_ep->ifnum = dev->endpoint[i].interface;
-        usb_ep->max_packet_size = dev->endpoint[i].max_packet_size;
-        usbredir_set_pipeline(dev, usb_ep);
-    }
+    usbredir_setup_usb_eps(dev);
+
     return 0;
 }
 
-- 
1.7.12.1

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [Qemu-devel] [PATCH 6/8] usb-redir: Use reject rather the disconnect on bad ep info
  2012-10-31 12:47 [Qemu-devel] usb input pipelining patches v3 Hans de Goede
                   ` (4 preceding siblings ...)
  2012-10-31 12:47 ` [Qemu-devel] [PATCH 5/8] usb-redir: Add an usbredir_setup_usb_eps() helper function Hans de Goede
@ 2012-10-31 12:47 ` Hans de Goede
  2012-10-31 12:47 ` [Qemu-devel] [PATCH 7/8] usb-redir: Allow to attach USB 2.0 devices to 1.1 host controller Hans de Goede
  2012-10-31 12:47 ` [Qemu-devel] [PATCH 8/8] usb-redir: Allow redirecting super speed devices to high speed controllers Hans de Goede
  7 siblings, 0 replies; 19+ messages in thread
From: Hans de Goede @ 2012-10-31 12:47 UTC (permalink / raw)
  To: Gerd Hoffmann; +Cc: Hans de Goede, qemu-devel

So that the client gets a notification about us disconnecting the device.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb/redirect.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
index ade5cc6..a3ce815 100644
--- a/hw/usb/redirect.c
+++ b/hw/usb/redirect.c
@@ -1372,7 +1372,8 @@ static void usbredir_ep_info(void *priv,
         case usb_redir_type_interrupt:
             if (dev->endpoint[i].interval == 0) {
                 ERROR("Received 0 interval for isoc or irq endpoint\n");
-                usbredir_device_disconnect(dev);
+                usbredir_reject_device(dev);
+                return;
             }
             /* Fall through */
         case usb_redir_type_control:
@@ -1382,7 +1383,7 @@ static void usbredir_ep_info(void *priv,
             break;
         default:
             ERROR("Received invalid endpoint type\n");
-            usbredir_device_disconnect(dev);
+            usbredir_reject_device(dev);
             return;
         }
     }
-- 
1.7.12.1

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [Qemu-devel] [PATCH 7/8] usb-redir: Allow to attach USB 2.0 devices to 1.1 host controller
  2012-10-31 12:47 [Qemu-devel] usb input pipelining patches v3 Hans de Goede
                   ` (5 preceding siblings ...)
  2012-10-31 12:47 ` [Qemu-devel] [PATCH 6/8] usb-redir: Use reject rather the disconnect on bad ep info Hans de Goede
@ 2012-10-31 12:47 ` Hans de Goede
  2012-10-31 12:47 ` [Qemu-devel] [PATCH 8/8] usb-redir: Allow redirecting super speed devices to high speed controllers Hans de Goede
  7 siblings, 0 replies; 19+ messages in thread
From: Hans de Goede @ 2012-10-31 12:47 UTC (permalink / raw)
  To: Gerd Hoffmann; +Cc: Jan Kiszka, qemu-devel, Hans de Goede

From: Jan Kiszka <jan.kiszka@siemens.com>

This follows the logic of host-linux: If a 2.0 device has no ISO
endpoint and no interrupt endpoint with a packet size > 64, we can
attach it also to an 1.1 host controller. In case the redir server does
not report endpoint sizes, play safe and remove the 1.1 compatibility as
well. Moreover, if we detect a conflicting change in the configuration
after the device was already attached, it will be disconnected
immediately.

HdG: Several small cleanups and fixes

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb/redirect.c | 30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
index a3ce815..8f9c42e 100644
--- a/hw/usb/redirect.c
+++ b/hw/usb/redirect.c
@@ -106,6 +106,7 @@ struct USBRedirDevice {
     struct usb_redir_interface_info_header interface_info;
     struct usbredirfilter_rule *filter_rules;
     int filter_rules_count;
+    int compatible_speedmask;
 };
 
 static void usbredir_hello(void *priv, struct usb_redir_hello_header *h);
@@ -968,6 +969,7 @@ static void usbredir_do_attach(void *opaque)
     }
 
     if (usb_device_attach(&dev->dev) != 0) {
+        WARNING("rejecting device due to speed mismatch\n");
         usbredir_reject_device(dev);
     }
 }
@@ -1088,6 +1090,9 @@ static int usbredir_initfn(USBDevice *udev)
     /* We'll do the attach once we receive the speed from the usb-host */
     udev->auto_attach = 0;
 
+    /* Will be cleared during setup when we find conflicts */
+    dev->compatible_speedmask = USB_SPEED_MASK_FULL;
+
     /* Let the backend know we are ready */
     qemu_chr_fe_open(dev->cs);
     qemu_chr_add_handlers(dev->cs, &usbredir_chr_handlers, dev);
@@ -1227,6 +1232,7 @@ static void usbredir_device_connect(void *priv,
     case usb_redir_speed_low:
         speed = "low speed";
         dev->dev.speed = USB_SPEED_LOW;
+        dev->compatible_speedmask &= ~USB_SPEED_MASK_FULL;
         break;
     case usb_redir_speed_full:
         speed = "full speed";
@@ -1260,7 +1266,7 @@ static void usbredir_device_connect(void *priv,
              device_connect->device_class);
     }
 
-    dev->dev.speedmask = (1 << dev->dev.speed);
+    dev->dev.speedmask = (1 << dev->dev.speed) | dev->compatible_speedmask;
     dev->device_info = *device_connect;
 
     if (usbredir_check_filter(dev)) {
@@ -1300,6 +1306,7 @@ static void usbredir_device_disconnect(void *priv)
     dev->interface_info.interface_count = NO_INTERFACE_INFO;
     dev->dev.addr = 0;
     dev->dev.speed = 0;
+    dev->compatible_speedmask = USB_SPEED_MASK_FULL;
 }
 
 static void usbredir_interface_info(void *priv,
@@ -1321,6 +1328,12 @@ static void usbredir_interface_info(void *priv,
     }
 }
 
+static void usbredir_mark_speed_incompatible(USBRedirDevice *dev, int speed)
+{
+    dev->compatible_speedmask &= ~(1 << speed);
+    dev->dev.speedmask = (1 << dev->dev.speed) | dev->compatible_speedmask;
+}
+
 static void usbredir_set_pipeline(USBRedirDevice *dev, struct USBEndpoint *uep)
 {
     if (uep->type != USB_ENDPOINT_XFER_BULK) {
@@ -1369,7 +1382,14 @@ static void usbredir_ep_info(void *priv,
         case usb_redir_type_invalid:
             break;
         case usb_redir_type_iso:
+            usbredir_mark_speed_incompatible(dev, USB_SPEED_FULL);
+            /* Fall through */
         case usb_redir_type_interrupt:
+            if (!usbredirparser_peer_has_cap(dev->parser,
+                                     usb_redir_cap_ep_info_max_packet_size) ||
+                    ep_info->max_packet_size[i] > 64) {
+                usbredir_mark_speed_incompatible(dev, USB_SPEED_FULL);
+            }
             if (dev->endpoint[i].interval == 0) {
                 ERROR("Received 0 interval for isoc or irq endpoint\n");
                 usbredir_reject_device(dev);
@@ -1387,6 +1407,14 @@ static void usbredir_ep_info(void *priv,
             return;
         }
     }
+    /* The new ep info may have caused a speed incompatibility, recheck */
+    if (dev->dev.attached &&
+            !(dev->dev.port->speedmask & dev->dev.speedmask)) {
+        ERROR("Device no longer matches speed after endpoint info change, "
+              "disconnecting!\n");
+        usbredir_reject_device(dev);
+        return;
+    }
     usbredir_setup_usb_eps(dev);
 }
 
-- 
1.7.12.1

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [Qemu-devel] [PATCH 8/8] usb-redir: Allow redirecting super speed devices to high speed controllers
  2012-10-31 12:47 [Qemu-devel] usb input pipelining patches v3 Hans de Goede
                   ` (6 preceding siblings ...)
  2012-10-31 12:47 ` [Qemu-devel] [PATCH 7/8] usb-redir: Allow to attach USB 2.0 devices to 1.1 host controller Hans de Goede
@ 2012-10-31 12:47 ` Hans de Goede
  7 siblings, 0 replies; 19+ messages in thread
From: Hans de Goede @ 2012-10-31 12:47 UTC (permalink / raw)
  To: Gerd Hoffmann; +Cc: Hans de Goede, qemu-devel

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb/redirect.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
index 8f9c42e..7904a12 100644
--- a/hw/usb/redirect.c
+++ b/hw/usb/redirect.c
@@ -1091,7 +1091,7 @@ static int usbredir_initfn(USBDevice *udev)
     udev->auto_attach = 0;
 
     /* Will be cleared during setup when we find conflicts */
-    dev->compatible_speedmask = USB_SPEED_MASK_FULL;
+    dev->compatible_speedmask = USB_SPEED_MASK_FULL | USB_SPEED_MASK_HIGH;
 
     /* Let the backend know we are ready */
     qemu_chr_fe_open(dev->cs);
@@ -1233,10 +1233,12 @@ static void usbredir_device_connect(void *priv,
         speed = "low speed";
         dev->dev.speed = USB_SPEED_LOW;
         dev->compatible_speedmask &= ~USB_SPEED_MASK_FULL;
+        dev->compatible_speedmask &= ~USB_SPEED_MASK_HIGH;
         break;
     case usb_redir_speed_full:
         speed = "full speed";
         dev->dev.speed = USB_SPEED_FULL;
+        dev->compatible_speedmask &= ~USB_SPEED_MASK_HIGH;
         break;
     case usb_redir_speed_high:
         speed = "high speed";
@@ -1306,7 +1308,7 @@ static void usbredir_device_disconnect(void *priv)
     dev->interface_info.interface_count = NO_INTERFACE_INFO;
     dev->dev.addr = 0;
     dev->dev.speed = 0;
-    dev->compatible_speedmask = USB_SPEED_MASK_FULL;
+    dev->compatible_speedmask = USB_SPEED_MASK_FULL | USB_SPEED_MASK_HIGH;
 }
 
 static void usbredir_interface_info(void *priv,
@@ -1383,6 +1385,7 @@ static void usbredir_ep_info(void *priv,
             break;
         case usb_redir_type_iso:
             usbredir_mark_speed_incompatible(dev, USB_SPEED_FULL);
+            usbredir_mark_speed_incompatible(dev, USB_SPEED_HIGH);
             /* Fall through */
         case usb_redir_type_interrupt:
             if (!usbredirparser_peer_has_cap(dev->parser,
@@ -1390,6 +1393,11 @@ static void usbredir_ep_info(void *priv,
                     ep_info->max_packet_size[i] > 64) {
                 usbredir_mark_speed_incompatible(dev, USB_SPEED_FULL);
             }
+            if (!usbredirparser_peer_has_cap(dev->parser,
+                                     usb_redir_cap_ep_info_max_packet_size) ||
+                    ep_info->max_packet_size[i] > 1024) {
+                usbredir_mark_speed_incompatible(dev, USB_SPEED_HIGH);
+            }
             if (dev->endpoint[i].interval == 0) {
                 ERROR("Received 0 interval for isoc or irq endpoint\n");
                 usbredir_reject_device(dev);
@@ -1520,6 +1528,17 @@ static void usbredir_control_packet(void *priv, uint64_t id,
     DPRINTF("ctrl-in status %d len %d id %"PRIu64"\n", control_packet->status,
             len, id);
 
+    /* Fix up USB-3 ep0 maxpacket size to allow superspeed connected devices
+     * to work redirected to a not superspeed capable hcd */
+    if (dev->dev.speed == USB_SPEED_SUPER &&
+            !((dev->dev.port->speedmask & USB_SPEED_MASK_SUPER)) &&
+            control_packet->requesttype == 0x80 &&
+            control_packet->request == 6 &&
+            control_packet->value == 0x100 && control_packet->index == 0 &&
+            data_len >= 18 && data[7] == 9) {
+        data[7] = 64;
+    }
+
     p = usbredir_find_packet_by_id(dev, 0, id);
     if (p) {
         len = usbredir_handle_status(dev, control_packet->status, len);
-- 
1.7.12.1

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [Qemu-devel] [PATCH 1/8] usb: Add packet combining functions
  2012-10-31 12:47 ` [Qemu-devel] [PATCH 1/8] usb: Add packet combining functions Hans de Goede
@ 2012-11-01 10:08   ` Gerd Hoffmann
  2012-11-01 13:14     ` Hans de Goede
  2012-11-01 13:59     ` Hans de Goede
  0 siblings, 2 replies; 19+ messages in thread
From: Gerd Hoffmann @ 2012-11-01 10:08 UTC (permalink / raw)
  To: Hans de Goede; +Cc: qemu-devel

On 10/31/12 13:47, Hans de Goede wrote:
> +    /*
> +     * If we had leftover packets the hcd driver will have cancelled them
> +     * and usb_combined_packet_cancel has already freed combined!
> +     */
> +    if (state != leftover) {
> +        g_free(combined);
> +    }

This calls for reference-counting USBCombinedPacket IMHO.

cheers,
  Gerd

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Qemu-devel] [PATCH 4/8] usb-redir: Add support for input pipelining
  2012-10-31 12:47 ` [Qemu-devel] [PATCH 4/8] usb-redir: Add support for input pipelining Hans de Goede
@ 2012-11-01 10:10   ` Gerd Hoffmann
  2012-11-01 13:16     ` Hans de Goede
  0 siblings, 1 reply; 19+ messages in thread
From: Gerd Hoffmann @ 2012-11-01 10:10 UTC (permalink / raw)
  To: Hans de Goede; +Cc: qemu-devel

On 10/31/12 13:47, Hans de Goede wrote:
> +        if (p->combined) {
> +            iov_to_buf(p->combined->iov.iov, p->combined->iov.niov,
> +                       0, buf, size);
> +        } else {
> +            usb_packet_copy(p, buf, size);
> +        }

Should we make usb_packet_copy & friends aware of combined packets?

cheers,
  Gerd

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Qemu-devel] [PATCH 1/8] usb: Add packet combining functions
  2012-11-01 10:08   ` Gerd Hoffmann
@ 2012-11-01 13:14     ` Hans de Goede
  2012-11-01 13:28       ` Hans de Goede
  2012-11-01 13:59     ` Hans de Goede
  1 sibling, 1 reply; 19+ messages in thread
From: Hans de Goede @ 2012-11-01 13:14 UTC (permalink / raw)
  To: Gerd Hoffmann; +Cc: qemu-devel

Hi,

On 11/01/2012 11:08 AM, Gerd Hoffmann wrote:
> On 10/31/12 13:47, Hans de Goede wrote:
>> +    /*
>> +     * If we had leftover packets the hcd driver will have cancelled them
>> +     * and usb_combined_packet_cancel has already freed combined!
>> +     */
>> +    if (state != leftover) {
>> +        g_free(combined);
>> +    }
>
> This calls for reference-counting USBCombinedPacket IMHO.

Why? We call packet_complete with a status if USB_RET_REMOVE_FROM_QUEUE
if we've left-over packets, the hcd code will cancel these, and
usb_combined_packet_cancel will free the combined packet when the
last packet of it gets cancelled, which *will* happen as we're
always processing *all* packets in combined here. There is no
scenario here where one or the other party wants to keep the
combined packet around any longer...

The only reason this is a bit non straightforward is that
normally packets get freed either on completion or cancellation,
but here we've a partial completion and a partial cancellation.

Also can you please just do one review and then point out all the
issues you see? Esp. since the feature freeze for 1.3 is *today*

Regards,

Hans

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Qemu-devel] [PATCH 4/8] usb-redir: Add support for input pipelining
  2012-11-01 10:10   ` Gerd Hoffmann
@ 2012-11-01 13:16     ` Hans de Goede
  0 siblings, 0 replies; 19+ messages in thread
From: Hans de Goede @ 2012-11-01 13:16 UTC (permalink / raw)
  To: Gerd Hoffmann; +Cc: qemu-devel

Hi,

On 11/01/2012 11:10 AM, Gerd Hoffmann wrote:
> On 10/31/12 13:47, Hans de Goede wrote:
>> +        if (p->combined) {
>> +            iov_to_buf(p->combined->iov.iov, p->combined->iov.niov,
>> +                       0, buf, size);
>> +        } else {
>> +            usb_packet_copy(p, buf, size);
>> +        }
>
> Should we make usb_packet_copy & friends aware of combined packets?

That sounds like something worth looking into, but can we please first
get this merged as is, and then do that as a follow up patch?

I'll put doing this on my to-do list.

Thanks & Regards,

Hans

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Qemu-devel] [PATCH 1/8] usb: Add packet combining functions
  2012-11-01 13:14     ` Hans de Goede
@ 2012-11-01 13:28       ` Hans de Goede
  0 siblings, 0 replies; 19+ messages in thread
From: Hans de Goede @ 2012-11-01 13:28 UTC (permalink / raw)
  To: Gerd Hoffmann; +Cc: qemu-devel

Hi,

On 11/01/2012 02:14 PM, Hans de Goede wrote:
> Hi,
>
> On 11/01/2012 11:08 AM, Gerd Hoffmann wrote:
>> On 10/31/12 13:47, Hans de Goede wrote:
>>> +    /*
>>> +     * If we had leftover packets the hcd driver will have cancelled them
>>> +     * and usb_combined_packet_cancel has already freed combined!
>>> +     */
>>> +    if (state != leftover) {
>>> +        g_free(combined);
>>> +    }
>>
>> This calls for reference-counting USBCombinedPacket IMHO.
>
> Why? We call packet_complete with a status if USB_RET_REMOVE_FROM_QUEUE
> if we've left-over packets, the hcd code will cancel these, and
> usb_combined_packet_cancel will free the combined packet when the
> last packet of it gets cancelled, which *will* happen as we're
> always processing *all* packets in combined here. There is no
> scenario here where one or the other party wants to keep the
> combined packet around any longer...
>
> The only reason this is a bit non straightforward is that
> normally packets get freed either on completion or cancellation,
> but here we've a partial completion and a partial cancellation.

Also note that reference counting will not make the special case
go away, as for combined packets without any leftover packets the
packet_complete (status == USB_RET_REMOVE_FROM_QUEUE) -> cancel
-> free/unref will never happen.

So simply taking a ref at the beginning of usb_combined_input_packet_complete
and then doing unref at the end will not help. Because for combined-packets
where all packets where used we then would need to do unref twice, once
to drop the local ref, and once to drop the final ref.

Regards,

Hans

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Qemu-devel] [PATCH 1/8] usb: Add packet combining functions
  2012-11-01 10:08   ` Gerd Hoffmann
  2012-11-01 13:14     ` Hans de Goede
@ 2012-11-01 13:59     ` Hans de Goede
  2012-11-01 14:23       ` Gerd Hoffmann
  1 sibling, 1 reply; 19+ messages in thread
From: Hans de Goede @ 2012-11-01 13:59 UTC (permalink / raw)
  To: Gerd Hoffmann; +Cc: qemu-devel

[-- Attachment #1: Type: text/plain, Size: 573 bytes --]

Hi,

On 11/01/2012 11:08 AM, Gerd Hoffmann wrote:
> On 10/31/12 13:47, Hans de Goede wrote:
>> +    /*
>> +     * If we had leftover packets the hcd driver will have cancelled them
>> +     * and usb_combined_packet_cancel has already freed combined!
>> +     */
>> +    if (state != leftover) {
>> +        g_free(combined);
>> +    }
>
> This calls for reference-counting USBCombinedPacket IMHO.

Here is a patch changing the way this is handled to something which
hopefully will be more to your liking. Feel free to squash this
into the original commit.

Regards,

Hans

[-- Attachment #2: 0001-usb-combined-packet-Move-freeing-of-combined-to-usb_.patch --]
[-- Type: text/x-patch, Size: 4087 bytes --]

>From fbcfdf12cbf28c2d1b0e11e290701376aaea554c Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@redhat.com>
Date: Thu, 1 Nov 2012 14:46:31 +0100
Subject: [PATCH] usb/combined-packet: Move freeing of combined to
 usb_combined_packet_remove()

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb/combined-packet.c | 33 +++++++++++++++------------------
 1 file changed, 15 insertions(+), 18 deletions(-)

diff --git a/hw/usb/combined-packet.c b/hw/usb/combined-packet.c
index e722198..4a0c299 100644
--- a/hw/usb/combined-packet.c
+++ b/hw/usb/combined-packet.c
@@ -31,12 +31,16 @@ static void usb_combined_packet_add(USBCombinedPacket *combined, USBPacket *p)
     p->combined = combined;
 }
 
+/* Note will free combined when the last packet gets removed */
 static void usb_combined_packet_remove(USBCombinedPacket *combined,
                                        USBPacket *p)
 {
     assert(p->combined == combined);
     p->combined = NULL;
     QTAILQ_REMOVE(&combined->packets, p, combined_entry);
+    if (QTAILQ_EMPTY(&combined->packets)) {
+        g_free(combined);
+    }
 }
 
 /* Also handles completion of non combined packets for pipelined input eps */
@@ -45,9 +49,8 @@ void usb_combined_input_packet_complete(USBDevice *dev, USBPacket *p)
     USBCombinedPacket *combined = p->combined;
     USBEndpoint *ep = p->ep;
     USBPacket *next;
-    enum { completing, complete, leftover };
-    int status, actual_length, state = completing;
-    bool short_not_ok;
+    int status, actual_length;
+    bool short_not_ok, done = false;
 
     if (combined == NULL) {
         usb_packet_complete_one(dev, p);
@@ -61,39 +64,34 @@ void usb_combined_input_packet_complete(USBDevice *dev, USBPacket *p)
     short_not_ok = QTAILQ_LAST(&combined->packets, packets_head)->short_not_ok;
 
     QTAILQ_FOREACH_SAFE(p, &combined->packets, combined_entry, next) {
-        if (state == completing) {
+        if (!done) {
             /* Distribute data over uncombined packets */
             if (actual_length >= p->iov.size) {
                 p->actual_length = p->iov.size;
             } else {
                 /* Send short or error packet to complete the transfer */
                 p->actual_length = actual_length;
-                state = complete;
+                done = true;
             }
             /* Report status on the last packet */
-            if (state == complete || next == NULL) {
+            if (done || next == NULL) {
                 p->status = status;
             } else {
                 p->status = USB_RET_SUCCESS;
             }
             p->short_not_ok = short_not_ok;
+            /* Note will free combined when the last packet gets removed! */
             usb_combined_packet_remove(combined, p);
             usb_packet_complete_one(dev, p);
             actual_length -= p->actual_length;
         } else {
             /* Remove any leftover packets from the queue */
-            state = leftover;
             p->status = USB_RET_REMOVE_FROM_QUEUE;
+            /* Note will free combined on the last packet! */
             dev->port->ops->complete(dev->port, p);
         }
     }
-    /*
-     * If we had leftover packets the hcd driver will have cancelled them
-     * and usb_combined_packet_cancel has already freed combined!
-     */
-    if (state != leftover) {
-        g_free(combined);
-    }
+    /* Do not use combined here, it has been freed! */
 leave:
     /* Check if there are packets in the queue waiting for our completion */
     usb_ep_combine_input_packets(ep);
@@ -104,14 +102,13 @@ void usb_combined_packet_cancel(USBDevice *dev, USBPacket *p)
 {
     USBCombinedPacket *combined = p->combined;
     assert(combined != NULL);
+    USBPacket *first = p->combined->first;
 
+    /* Note will free combined on the last packet! */
     usb_combined_packet_remove(combined, p);
-    if (p == combined->first) {
+    if (p == first) {
         usb_device_cancel_packet(dev, p);
     }
-    if (QTAILQ_EMPTY(&combined->packets)) {
-        g_free(combined);
-    }
 }
 
 /*
-- 
1.7.12.1


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [Qemu-devel] [PATCH 1/8] usb: Add packet combining functions
  2012-11-01 13:59     ` Hans de Goede
@ 2012-11-01 14:23       ` Gerd Hoffmann
  2012-11-01 16:17         ` Hans de Goede
  0 siblings, 1 reply; 19+ messages in thread
From: Gerd Hoffmann @ 2012-11-01 14:23 UTC (permalink / raw)
  To: Hans de Goede; +Cc: qemu-devel

  Hi,

> Here is a patch changing the way this is handled to something which
> hopefully will be more to your liking. Feel free to squash this
> into the original commit.

Looks good but doesn't apply.  Likewise the "split packet result into
actual_length + status" patch series.

Pushed current usb patch queue to
http://www.kraxel.org/cgit/qemu/log/?h=rebase/usb-next, please rebase
and resend.

thanks,
  Gerd

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Qemu-devel] [PATCH 1/8] usb: Add packet combining functions
  2012-11-01 14:23       ` Gerd Hoffmann
@ 2012-11-01 16:17         ` Hans de Goede
  2012-11-01 16:42           ` Gerd Hoffmann
  0 siblings, 1 reply; 19+ messages in thread
From: Hans de Goede @ 2012-11-01 16:17 UTC (permalink / raw)
  To: Gerd Hoffmann; +Cc: qemu-devel

Hi,

On 11/01/2012 03:23 PM, Gerd Hoffmann wrote:
>    Hi,
>
>> Here is a patch changing the way this is handled to something which
>> hopefully will be more to your liking. Feel free to squash this
>> into the original commit.
>
> Looks good but doesn't apply.  Likewise the "split packet result into
> actual_length + status" patch series.
>
> Pushed current usb patch queue to
> http://www.kraxel.org/cgit/qemu/log/?h=rebase/usb-next, please rebase
> and resend.

Rebased and re-rested (a bit), new version send.

I see you've just done a pull-req for what was in usb-next already,
I really hope you can do another one with the status + length splitting
patches in there, so that those can go into 1.3, this will make it
a lot easier to cherry pick any later bugfixes from master to 1.3

Thanks & Regards,

Hans

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Qemu-devel] [PATCH 1/8] usb: Add packet combining functions
  2012-11-01 16:17         ` Hans de Goede
@ 2012-11-01 16:42           ` Gerd Hoffmann
  2012-11-01 19:07             ` Hans de Goede
  0 siblings, 1 reply; 19+ messages in thread
From: Gerd Hoffmann @ 2012-11-01 16:42 UTC (permalink / raw)
  To: Hans de Goede; +Cc: qemu-devel

  Hi,

> Rebased and re-rested (a bit), new version send.

Added to the queue.

> I see you've just done a pull-req for what was in usb-next already,
> I really hope you can do another one with the status + length splitting
> patches in there, so that those can go into 1.3, this will make it
> a lot easier to cherry pick any later bugfixes from master to 1.3

It's soft freeze only, not hard freeze yet (see
http://wiki.qemu.org/Planning/1.3).  It's a bit invasive (although local
to usb), but it isn't a major change.  So merging this a bit later
should be no problem.  I'll try to get a incremental pull out of the
door before kvm forum nevertheless.

cheers,
  Gerd

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Qemu-devel] [PATCH 1/8] usb: Add packet combining functions
  2012-11-01 16:42           ` Gerd Hoffmann
@ 2012-11-01 19:07             ` Hans de Goede
  0 siblings, 0 replies; 19+ messages in thread
From: Hans de Goede @ 2012-11-01 19:07 UTC (permalink / raw)
  To: Gerd Hoffmann; +Cc: qemu-devel

Hi,

On 11/01/2012 05:42 PM, Gerd Hoffmann wrote:
>    Hi,
>
>> Rebased and re-rested (a bit), new version send.
>
> Added to the queue.
>
>> I see you've just done a pull-req for what was in usb-next already,
>> I really hope you can do another one with the status + length splitting
>> patches in there, so that those can go into 1.3, this will make it
>> a lot easier to cherry pick any later bugfixes from master to 1.3
>
> It's soft freeze only, not hard freeze yet (see
> http://wiki.qemu.org/Planning/1.3).  It's a bit invasive (although local
> to usb), but it isn't a major change.  So merging this a bit later
> should be no problem.  I'll try to get a incremental pull out of the
> door before kvm forum nevertheless.

Ok, thanks!

Regards,

Hans

^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2012-11-01 19:05 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-10-31 12:47 [Qemu-devel] usb input pipelining patches v3 Hans de Goede
2012-10-31 12:47 ` [Qemu-devel] [PATCH 1/8] usb: Add packet combining functions Hans de Goede
2012-11-01 10:08   ` Gerd Hoffmann
2012-11-01 13:14     ` Hans de Goede
2012-11-01 13:28       ` Hans de Goede
2012-11-01 13:59     ` Hans de Goede
2012-11-01 14:23       ` Gerd Hoffmann
2012-11-01 16:17         ` Hans de Goede
2012-11-01 16:42           ` Gerd Hoffmann
2012-11-01 19:07             ` Hans de Goede
2012-10-31 12:47 ` [Qemu-devel] [PATCH 2/8] combined-packet: Add a workaround for Linux usbfs + live migration Hans de Goede
2012-10-31 12:47 ` [Qemu-devel] [PATCH 3/8] usb-redir: Add support for 32 bits bulk packet length Hans de Goede
2012-10-31 12:47 ` [Qemu-devel] [PATCH 4/8] usb-redir: Add support for input pipelining Hans de Goede
2012-11-01 10:10   ` Gerd Hoffmann
2012-11-01 13:16     ` Hans de Goede
2012-10-31 12:47 ` [Qemu-devel] [PATCH 5/8] usb-redir: Add an usbredir_setup_usb_eps() helper function Hans de Goede
2012-10-31 12:47 ` [Qemu-devel] [PATCH 6/8] usb-redir: Use reject rather the disconnect on bad ep info Hans de Goede
2012-10-31 12:47 ` [Qemu-devel] [PATCH 7/8] usb-redir: Allow to attach USB 2.0 devices to 1.1 host controller Hans de Goede
2012-10-31 12:47 ` [Qemu-devel] [PATCH 8/8] usb-redir: Allow redirecting super speed devices to high speed controllers Hans de Goede

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.