diff mbox

[14/14] usb-host: add timeout handler

Message ID 1335282691-26864-15-git-send-email-kraxel@redhat.com
State New
Headers show

Commit Message

Gerd Hoffmann April 24, 2012, 3:51 p.m. UTC 
Add a timeout handler.  In case bulk transfers take too long to finish
the request will be canceled.  The timeout is tunable via property, by
default it is 5 seconds.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/host-linux.c |   27 +++++++++++++++++++++++++++
 trace-events        |    1 +
 2 files changed, 28 insertions(+), 0 deletions(-)

Comments

Johannes Stezenbach April 26, 2012, 9:51 a.m. UTC | #1 
Hi,

On Tue, Apr 24, 2012 at 05:51:31PM +0200, Gerd Hoffmann wrote:
> Add a timeout handler.  In case bulk transfers take too long to finish
> the request will be canceled.  The timeout is tunable via property, by
> default it is 5 seconds.

This patch causes an issue for my firmware download.  The
protocol used is very simple and uses a single bulk out endpoint:
- send start packet
- send firmware image
- send end packet

When the "end" packet is received the actual flashing starts,
which can take minutes.  The device will not ACK the packet
until flashing completes.  Thus I now get an error from the
flash tool (while flashing completes on the device).
Qemu says:

  husb: urb timeout (5 secs, #1)

I think 5 seconds default timeout may also cause issues
with slow devices (hard disk spinning up, CDROM reading
disc TOC etc.).  It seems unneccessary since the guest OS
handles timeouts specified by drivers/applications
on URB submission, and will cancel transfers on timeout.
Has this been added to fix an actual problem?


Thanks,
Johannes
Gerd Hoffmann April 26, 2012, 10:22 a.m. UTC | #2 
Hi,

> When the "end" packet is received the actual flashing starts,
> which can take minutes.  The device will not ACK the packet
> until flashing completes.  Thus I now get an error from the
> flash tool (while flashing completes on the device).
> Qemu says:
> 
>   husb: urb timeout (5 secs, #1)
> 
> I think 5 seconds default timeout may also cause issues
> with slow devices (hard disk spinning up, CDROM reading
> disc TOC etc.).  It seems unneccessary since the guest OS
> handles timeouts specified by drivers/applications
> on URB submission, and will cancel transfers on timeout.
> Has this been added to fix an actual problem?

Has been added to detect requests getting stuck.  I'm seeing this now
and then with linux guests polling usb sticks (test unit ready every few
seconds).  Seems to be a bug somewhere, not root-caused yet.

/me wanted qemu handle it and warn about this (like it warns already in
case device reset takes unusual long).  But, yes, I can see how this can
cause more harm than it helps as usb-host has no way to figure how long
a certain operation will take.

Pushed usb.49 with this patch dropped.

cheers,
  Gerd
diff mbox

Patch

diff --git a/hw/usb/host-linux.c b/hw/usb/host-linux.c
index 048f8ff..616e51c 100644
--- a/hw/usb/host-linux.c
+++ b/hw/usb/host-linux.c
@@ -110,6 +110,8 @@  typedef struct USBHostDevice {
     int       closing;
     uint32_t  iso_urb_count;
     uint32_t  options;
+    uint32_t  timeout_secs;
+    uint32_t  timeout_count;
     Notifier  exit;
 
     struct endp_data ep_in[USB_MAX_ENDPOINTS];
@@ -276,6 +278,7 @@  struct AsyncURB
     struct usbdevfs_iso_packet_desc isocpd[ISO_FRAME_DESC_PER_URB];
     USBHostDevice *hdev;
     QLIST_ENTRY(AsyncURB) next;
+    QEMUTimer *timeout;
 
     /* For regular async urbs */
     USBPacket     *packet;
@@ -285,16 +288,34 @@  struct AsyncURB
     int iso_frame_idx; /* -1 means in flight */
 };
 
+static void async_timeout(void *opaque)
+{
+    AsyncURB *aurb = opaque;
+    USBHostDevice *s = aurb->hdev;
+
+    s->timeout_count++;
+    if (s->timeout_count < 10 ||
+        s->timeout_count % 10 == 0) {
+        fprintf(stderr, "husb: urb timeout (%d secs, #%d)\n",
+                s->timeout_secs, s->timeout_count);
+    }
+    trace_usb_host_urb_timeout(s->bus_num, s->addr, aurb);
+    ioctl(s->fd, USBDEVFS_DISCARDURB, aurb);
+}
+
 static AsyncURB *async_alloc(USBHostDevice *s)
 {
     AsyncURB *aurb = g_malloc0(sizeof(AsyncURB));
     aurb->hdev = s;
     QLIST_INSERT_HEAD(&s->aurbs, aurb, next);
+    aurb->timeout = qemu_new_timer_ns(vm_clock, async_timeout, aurb);
     return aurb;
 }
 
 static void async_free(AsyncURB *aurb)
 {
+    qemu_del_timer(aurb->timeout);
+    qemu_free_timer(aurb->timeout);
     QLIST_REMOVE(aurb, next);
     g_free(aurb);
 }
@@ -938,6 +959,11 @@  static int usb_host_handle_data(USBDevice *dev, USBPacket *p)
                 return USB_RET_STALL;
             }
         }
+        if (urb->type == USBDEVFS_URB_TYPE_BULK) {
+            qemu_mod_timer(aurb->timeout, qemu_get_clock_ns(vm_clock)
+                           + s->timeout_secs * get_ticks_per_sec());
+        }
+
     } while (rem > 0);
 
     return USB_RET_ASYNC;
@@ -1444,6 +1470,7 @@  static Property usb_host_dev_properties[] = {
     DEFINE_PROP_HEX32("vendorid",  USBHostDevice, match.vendor_id,  0),
     DEFINE_PROP_HEX32("productid", USBHostDevice, match.product_id, 0),
     DEFINE_PROP_UINT32("isobufs",  USBHostDevice, iso_urb_count,    4),
+    DEFINE_PROP_UINT32("timeout",  USBHostDevice, timeout_secs,     5),
     DEFINE_PROP_INT32("bootindex", USBHostDevice, bootindex,        -1),
     DEFINE_PROP_BIT("pipeline",    USBHostDevice, options,
                     USB_HOST_OPT_PIPELINE, true),
diff --git a/trace-events b/trace-events
index 87cb96c..710eb28 100644
--- a/trace-events
+++ b/trace-events
@@ -329,6 +329,7 @@  usb_host_req_canceled(int bus, int addr, void *p) "dev %d:%d, packet %p"
 usb_host_urb_submit(int bus, int addr, void *aurb, int length, int more) "dev %d:%d, aurb %p, length %d, more %d"
 usb_host_urb_complete(int bus, int addr, void *aurb, int status, int length, int more) "dev %d:%d, aurb %p, status %d, length %d, more %d"
 usb_host_urb_canceled(int bus, int addr, void *aurb) "dev %d:%d, aurb %p"
+usb_host_urb_timeout(int bus, int addr, void *aurb) "dev %d:%d, aurb %p"
 usb_host_ep_set_halt(int bus, int addr, int ep) "dev %d:%d, ep %d"
 usb_host_ep_clear_halt(int bus, int addr, int ep) "dev %d:%d, ep %d"
 usb_host_ep_start_iso(int bus, int addr, int ep) "dev %d:%d, ep %d"