usb: split packet result into actual_length + status

Since with the ehci and xhci controllers a single packet can be larger
then maxpacketsize, it is possible for the result of a single packet
to be both having transferred some data as well as the transfer to have
an error.

An example would be an input transfer from a bulk endpoint successfully
receiving 1 or more maxpacketsize packets from the device, followed
by a packet signalling halt.

While already touching all the devices and controllers handle_packet /
handle_data / handle_control code, also change the return type of
these functions to void, solely storing the status in the packet. To
make the code paths for regular versus async packet handling more
uniform.

This patch unfortunately is somewhat invasive, since makeing the qemu
usb core deal with this requires changes everywhere. This patch only
prepares the usb core for this, all the hcd / device changes are done
in such a way that there are no functional changes.

This patch has been tested with uhci and ehci hcds, together with usb-audio,
usb-hid and usb-storage devices, as well as with usb-redir redirection
with a wide variety of real devices.

Note that there is usually no need to directly set packet->actual_length
form devices handle_data callback, as that is done by usb_packet_copy()

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

1 files changed, 69 insertions, 59 deletions

diff --git a/hw/usb/host-linux.c b/hw/usb/host-linux.c
index 3a258b4..ca3e24a 100644
--- a/hw/usb/host-linux.c
+++ b/hw/usb/host-linux.c
@@ -366,28 +366,29 @@ static void async_complete(void *opaque)
         if (p) {
             switch (aurb->urb.status) {
             case 0:
-                p->result += aurb->urb.actual_length;
+                p->actual_length = aurb->urb.actual_length;
+                p->status = USB_RET_SUCCESS; /* Clear previous ASYNC status */
                 break;
 
             case -EPIPE:
                 set_halt(s, p->pid, p->ep->nr);
-                p->result = USB_RET_STALL;
+                p->status = USB_RET_STALL;
                 break;
 
             case -EOVERFLOW:
-                p->result = USB_RET_BABBLE;
+                p->status = USB_RET_BABBLE;
                 break;
 
             default:
-                p->result = USB_RET_IOERROR;
+                p->status = USB_RET_IOERROR;
                 break;
             }
 
             if (aurb->urb.type == USBDEVFS_URB_TYPE_CONTROL) {
-                trace_usb_host_req_complete(s->bus_num, s->addr, p, p->result);
+                trace_usb_host_req_complete(s->bus_num, s->addr, p, p->status);
                 usb_generic_async_ctrl_complete(&s->dev, p);
             } else if (!aurb->more) {
-                trace_usb_host_req_complete(s->bus_num, s->addr, p, p->result);
+                trace_usb_host_req_complete(s->bus_num, s->addr, p, p->status);
                 usb_packet_complete(&s->dev, p);
             }
         }
@@ -733,27 +734,31 @@ static void usb_host_stop_n_free_iso(USBHostDevice *s, int pid, uint8_t ep)
     clear_iso_started(s, pid, ep);
 }
 
-static int urb_status_to_usb_ret(int status)
+static void urb_status_to_usb_ret(int status, USBPacket *p)
 {
     switch (status) {
     case -EPIPE:
-        return USB_RET_STALL;
+        p->status = USB_RET_STALL;
+        break;
     case -EOVERFLOW:
-        return USB_RET_BABBLE;
+        p->status = USB_RET_BABBLE;
+        break;
     default:
-        return USB_RET_IOERROR;
+        p->status = USB_RET_IOERROR;
     }
 }
 
-static int usb_host_handle_iso_data(USBHostDevice *s, USBPacket *p, int in)
+static void usb_host_handle_iso_data(USBHostDevice *s, USBPacket *p, int in)
 {
     AsyncURB *aurb;
-    int i, j, ret, max_packet_size, offset, len = 0;
+    int i, j, max_packet_size, offset, len;
     uint8_t *buf;
 
     max_packet_size = p->ep->max_packet_size;
-    if (max_packet_size == 0)
-        return USB_RET_NAK;
+    if (max_packet_size == 0) {
+        p->status = USB_RET_NAK;
+        return;
+    }
 
     aurb = get_iso_urb(s, p->pid, p->ep->nr);
     if (!aurb) {
@@ -766,18 +771,17 @@ static int usb_host_handle_iso_data(USBHostDevice *s, USBPacket *p, int in)
         if (in) {
             /* Check urb status  */
             if (aurb[i].urb.status) {
-                len = urb_status_to_usb_ret(aurb[i].urb.status);
+                urb_status_to_usb_ret(aurb[i].urb.status, p);
                 /* Move to the next urb */
                 aurb[i].iso_frame_idx = ISO_FRAME_DESC_PER_URB - 1;
             /* Check frame status */
             } else if (aurb[i].urb.iso_frame_desc[j].status) {
-                len = urb_status_to_usb_ret(
-                                        aurb[i].urb.iso_frame_desc[j].status);
+                urb_status_to_usb_ret(aurb[i].urb.iso_frame_desc[j].status, p);
             /* Check the frame fits */
             } else if (aurb[i].urb.iso_frame_desc[j].actual_length
                        > p->iov.size) {
                 printf("husb: received iso data is larger then packet\n");
-                len = USB_RET_BABBLE;
+                p->status = USB_RET_BABBLE;
             /* All good copy data over */
             } else {
                 len = aurb[i].urb.iso_frame_desc[j].actual_length;
@@ -792,7 +796,8 @@ static int usb_host_handle_iso_data(USBHostDevice *s, USBPacket *p, int in)
             /* Check the frame fits */
             if (len > max_packet_size) {
                 printf("husb: send iso data is larger then max packet size\n");
-                return USB_RET_NAK;
+                p->status = USB_RET_NAK;
+                return;
             }
 
             /* All good copy data over */
@@ -823,17 +828,16 @@ static int usb_host_handle_iso_data(USBHostDevice *s, USBPacket *p, int in)
         /* (Re)-submit all fully consumed / filled urbs */
         for (i = 0; i < s->iso_urb_count; i++) {
             if (aurb[i].iso_frame_idx == ISO_FRAME_DESC_PER_URB) {
-                ret = ioctl(s->fd, USBDEVFS_SUBMITURB, &aurb[i]);
-                if (ret < 0) {
+                if (ioctl(s->fd, USBDEVFS_SUBMITURB, &aurb[i]) < 0) {
                     perror("USBDEVFS_SUBMITURB");
-                    if (!in || len == 0) {
+                    if (!in || p->status == USB_RET_SUCCESS) {
                         switch(errno) {
                         case ETIMEDOUT:
-                            len = USB_RET_NAK;
+                            p->status = USB_RET_NAK;
                             break;
                         case EPIPE:
                         default:
-                            len = USB_RET_STALL;
+                            p->status = USB_RET_STALL;
                         }
                     }
                     break;
@@ -843,11 +847,9 @@ static int usb_host_handle_iso_data(USBHostDevice *s, USBPacket *p, int in)
             }
         }
     }
-
-    return len;
 }
 
-static int usb_host_handle_data(USBDevice *dev, USBPacket *p)
+static void usb_host_handle_data(USBDevice *dev, USBPacket *p)
 {
     USBHostDevice *s = DO_UPCAST(USBHostDevice, dev, dev);
     struct usbdevfs_urb *urb;
@@ -862,7 +864,8 @@ static int usb_host_handle_data(USBDevice *dev, USBPacket *p)
 
     if (!is_valid(s, p->pid, p->ep->nr)) {
         trace_usb_host_req_complete(s->bus_num, s->addr, p, USB_RET_NAK);
-        return USB_RET_NAK;
+        p->status = USB_RET_NAK;
+        return;
     }
 
     if (p->pid == USB_TOKEN_IN) {
@@ -877,13 +880,15 @@ static int usb_host_handle_data(USBDevice *dev, USBPacket *p)
         if (ret < 0) {
             perror("USBDEVFS_CLEAR_HALT");
             trace_usb_host_req_complete(s->bus_num, s->addr, p, USB_RET_NAK);
-            return USB_RET_NAK;
+            p->status = USB_RET_NAK;
+            return;
         }
         clear_halt(s, p->pid, p->ep->nr);
     }
 
     if (is_isoc(s, p->pid, p->ep->nr)) {
-        return usb_host_handle_iso_data(s, p, p->pid == USB_TOKEN_IN);
+        usb_host_handle_iso_data(s, p, p->pid == USB_TOKEN_IN);
+        return;
     }
 
     v = 0;
@@ -933,17 +938,19 @@ static int usb_host_handle_data(USBDevice *dev, USBPacket *p)
             case ETIMEDOUT:
                 trace_usb_host_req_complete(s->bus_num, s->addr, p,
                                             USB_RET_NAK);
-                return USB_RET_NAK;
+                p->status = USB_RET_NAK;
+                break;
             case EPIPE:
             default:
                 trace_usb_host_req_complete(s->bus_num, s->addr, p,
                                             USB_RET_STALL);
-                return USB_RET_STALL;
+                p->status = USB_RET_STALL;
             }
+            return;
         }
     } while (rem > 0);
 
-    return USB_RET_ASYNC;
+    p->status = USB_RET_ASYNC;
 }
 
 static int ctrl_error(void)
@@ -955,14 +962,13 @@ static int ctrl_error(void)
     }
 }
 
-static int usb_host_set_address(USBHostDevice *s, int addr)
+static void usb_host_set_address(USBHostDevice *s, int addr)
 {
     trace_usb_host_set_address(s->bus_num, s->addr, addr);
     s->dev.addr = addr;
-    return 0;
 }
 
-static int usb_host_set_config(USBHostDevice *s, int config)
+static void usb_host_set_config(USBHostDevice *s, int config, USBPacket *p)
 {
     int ret, first = 1;
 
@@ -987,14 +993,15 @@ again:
     }
 
     if (ret < 0) {
-        return ctrl_error();
+        p->status = ctrl_error();
+        return;
     }
     usb_host_claim_interfaces(s, config);
     usb_linux_update_endp_table(s);
-    return 0;
 }
 
-static int usb_host_set_interface(USBHostDevice *s, int iface, int alt)
+static void usb_host_set_interface(USBHostDevice *s, int iface, int alt,
+                                   USBPacket *p)
 {
     struct usbdevfs_setinterface si;
     int i, ret;
@@ -1011,7 +1018,8 @@ static int usb_host_set_interface(USBHostDevice *s, int iface, int alt)
     }
 
     if (iface >= USB_MAX_INTERFACES) {
-        return USB_RET_STALL;
+        p->status = USB_RET_STALL;
+        return;
     }
 
     si.interface  = iface;
@@ -1022,15 +1030,15 @@ static int usb_host_set_interface(USBHostDevice *s, int iface, int alt)
             iface, alt, ret, errno);
 
     if (ret < 0) {
-        return ctrl_error();
+        p->status = ctrl_error();
+        return;
     }
 
     s->dev.altsetting[iface] = alt;
     usb_linux_update_endp_table(s);
-    return 0;
 }
 
-static int usb_host_handle_control(USBDevice *dev, USBPacket *p,
+static void usb_host_handle_control(USBDevice *dev, USBPacket *p,
                int request, int value, int index, int length, uint8_t *data)
 {
     USBHostDevice *s = DO_UPCAST(USBHostDevice, dev, dev);
@@ -1048,19 +1056,19 @@ static int usb_host_handle_control(USBDevice *dev, USBPacket *p,
 
     switch (request) {
     case DeviceOutRequest | USB_REQ_SET_ADDRESS:
-        ret = usb_host_set_address(s, value);
-        trace_usb_host_req_emulated(s->bus_num, s->addr, p, ret);
-        return ret;
+        usb_host_set_address(s, value);
+        trace_usb_host_req_emulated(s->bus_num, s->addr, p, p->status);
+        return;
 
     case DeviceOutRequest | USB_REQ_SET_CONFIGURATION:
-        ret = usb_host_set_config(s, value & 0xff);
-        trace_usb_host_req_emulated(s->bus_num, s->addr, p, ret);
-        return ret;
+        usb_host_set_config(s, value & 0xff, p);
+        trace_usb_host_req_emulated(s->bus_num, s->addr, p, p->status);
+        return;
 
     case InterfaceOutRequest | USB_REQ_SET_INTERFACE:
-        ret = usb_host_set_interface(s, index, value);
-        trace_usb_host_req_emulated(s->bus_num, s->addr, p, ret);
-        return ret;
+        usb_host_set_interface(s, index, value, p);
+        trace_usb_host_req_emulated(s->bus_num, s->addr, p, p->status);
+        return;
 
     case EndpointOutRequest | USB_REQ_CLEAR_FEATURE:
         if (value == 0) { /* clear halt */
@@ -1068,17 +1076,16 @@ static int usb_host_handle_control(USBDevice *dev, USBPacket *p,
             ioctl(s->fd, USBDEVFS_CLEAR_HALT, &index);
             clear_halt(s, pid, index & 0x0f);
             trace_usb_host_req_emulated(s->bus_num, s->addr, p, 0);
-            return 0;
+            return;
         }
     }
 
     /* The rest are asynchronous */
-    assert(p && p->result == 0);
-
     if (length > sizeof(dev->data_buf)) {
         fprintf(stderr, "husb: ctrl buffer too small (%d > %zu)\n",
                 length, sizeof(dev->data_buf));
-        return USB_RET_STALL;
+        p->status = USB_RET_STALL;
+        return;
     }
 
     aurb = async_alloc(s);
@@ -1112,14 +1119,17 @@ static int usb_host_handle_control(USBDevice *dev, USBPacket *p,
 
         switch(errno) {
         case ETIMEDOUT:
-            return USB_RET_NAK;
+            p->status = USB_RET_NAK;
+            break;
         case EPIPE:
         default:
-            return USB_RET_STALL;
+            p->status = USB_RET_STALL;
+            break;
         }
+        return;
     }
 
-    return USB_RET_ASYNC;
+    p->status = USB_RET_ASYNC;
 }
 
 /* returns 1 on problem encountered or 0 for success */