Patchwork usb: selective endpoint initialization

login
register
mail settings
Submitter Gerd Hoffmann
Date July 3, 2012, 8:21 a.m.
Message ID <4FF2AB70.5080307@redhat.com>
Download mbox | patch
Permalink /patch/168730/
State New
Headers show

Comments

Gerd Hoffmann - July 3, 2012, 8:21 a.m.
On 07/03/12 09:47, Jan Kiszka wrote:
> On 2012-07-02 18:16, Gerd Hoffmann wrote:
>> Add support for (re-)initializing endpoints which belong to a specific
>> interface only.  Use this in usb-host when changing altsetting for an
>> interface, so other interfaces are not disturbed.
>>
> 
> qemu-system-x86_64: /data/qemu/hw/usb/host-linux.c:1220:
> usb_linux_update_endp_table: Assertion `usb_ep_get_type(&s->dev, pid,
> ep) == 255' failed.
> 
> Do you need a trace again?

Don't think so.  Alternative fix attached.

>     Interface Descriptor:
>       bInterfaceNumber        3
>       bInterfaceClass         3 Human Interface Device

This interface is the one the packet comes from, guess the headset has
some button(s) which this HID interface is intended for.

cheers,
  Gerd
From 7d00f8996f7981bb118b6986813ce407b31f2b70 Mon Sep 17 00:00:00 2001
From: Gerd Hoffmann <kraxel@redhat.com>
Date: Tue, 3 Jul 2012 10:11:21 +0200
Subject: [PATCH] usb: split endpoint init and reset

Create a new usb_ep_reset() function to reset endpoint state, without
re-initialiting the queues, so we don't unlink in-flight packets just
because usb-host has to re-parse the descriptor tables.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb.h            |    1 +
 hw/usb/core.c       |   13 +++++++++++--
 hw/usb/host-linux.c |    5 +++--
 3 files changed, 15 insertions(+), 4 deletions(-)
Jan Kiszka - July 3, 2012, 8:43 a.m.
On 2012-07-03 10:21, Gerd Hoffmann wrote:
> On 07/03/12 09:47, Jan Kiszka wrote:
>> On 2012-07-02 18:16, Gerd Hoffmann wrote:
>>> Add support for (re-)initializing endpoints which belong to a specific
>>> interface only.  Use this in usb-host when changing altsetting for an
>>> interface, so other interfaces are not disturbed.
>>>
>>
>> qemu-system-x86_64: /data/qemu/hw/usb/host-linux.c:1220:
>> usb_linux_update_endp_table: Assertion `usb_ep_get_type(&s->dev, pid,
>> ep) == 255' failed.
>>
>> Do you need a trace again?
> 
> Don't think so.  Alternative fix attached.

Looks good here. No crashes so far, sound is playing.

> 
>>     Interface Descriptor:
>>       bInterfaceNumber        3
>>       bInterfaceClass         3 Human Interface Device
> 
> This interface is the one the packet comes from, guess the headset has
> some button(s) which this HID interface is intended for.

Yes, there are several buttons.

Thanks,
Jan
Jan Kiszka - July 3, 2012, 8:51 a.m.
On 2012-07-03 10:43, Jan Kiszka wrote:
> On 2012-07-03 10:21, Gerd Hoffmann wrote:
>> On 07/03/12 09:47, Jan Kiszka wrote:
>>> On 2012-07-02 18:16, Gerd Hoffmann wrote:
>>>> Add support for (re-)initializing endpoints which belong to a specific
>>>> interface only.  Use this in usb-host when changing altsetting for an
>>>> interface, so other interfaces are not disturbed.
>>>>
>>>
>>> qemu-system-x86_64: /data/qemu/hw/usb/host-linux.c:1220:
>>> usb_linux_update_endp_table: Assertion `usb_ep_get_type(&s->dev, pid,
>>> ep) == 255' failed.
>>>
>>> Do you need a trace again?
>>
>> Don't think so.  Alternative fix attached.
> 
> Looks good here. No crashes so far, sound is playing.
> 

BTW, there are still plenty of "husb: out of buffers for iso stream"
messages. Can we do anything about it, or does the the guest selects too
few buffers here (for a virtualized setup)?

Jan
Gerd Hoffmann - July 3, 2012, 9:17 a.m.
Hi,

> BTW, there are still plenty of "husb: out of buffers for iso stream"
> messages. Can we do anything about it, or does the the guest selects too
> few buffers here (for a virtualized setup)?

Try increase isobufs (usb-host property, default is 4).

cheers,
  Gerd
Jan Kiszka - July 3, 2012, 11:39 a.m.
On 2012-07-03 11:17, Gerd Hoffmann wrote:
>   Hi,
> 
>> BTW, there are still plenty of "husb: out of buffers for iso stream"
>> messages. Can we do anything about it, or does the the guest selects too
>> few buffers here (for a virtualized setup)?
> 
> Try increase isobufs (usb-host property, default is 4).

I gave up at isobufs=10000 - it apparently has no influence on this issue.

Rather, it looks like it is triggered when some sound effect played by
Windows ends (e.g. the feedback on volume changes).

Jan
Gerd Hoffmann - July 3, 2012, 11:55 a.m.
On 07/03/12 13:39, Jan Kiszka wrote:
> On 2012-07-03 11:17, Gerd Hoffmann wrote:
>>   Hi,
>>
>>> BTW, there are still plenty of "husb: out of buffers for iso stream"
>>> messages. Can we do anything about it, or does the the guest selects too
>>> few buffers here (for a virtualized setup)?
>>
>> Try increase isobufs (usb-host property, default is 4).
> 
> I gave up at isobufs=10000 - it apparently has no influence on this issue.
>
> Rather, it looks like it is triggered when some sound effect played by
> Windows ends (e.g. the feedback on volume changes).

Makes sense.  /me should probably turn those messages into
usb_host_iso_* tracepoints so usb-host is quiet by default.

cheers,
  Gerd

Patch

diff --git a/hw/usb.h b/hw/usb.h
index a5623d3..9cd2f89 100644
--- a/hw/usb.h
+++ b/hw/usb.h
@@ -363,6 +363,7 @@  void usb_packet_complete(USBDevice *dev, USBPacket *p);
 void usb_cancel_packet(USBPacket * p);
 
 void usb_ep_init(USBDevice *dev);
+void usb_ep_reset(USBDevice *dev);
 void usb_ep_dump(USBDevice *dev);
 struct USBEndpoint *usb_ep_get(USBDevice *dev, int pid, int ep);
 uint8_t usb_ep_get_type(USBDevice *dev, int pid, int ep);
diff --git a/hw/usb/core.c b/hw/usb/core.c
index 0e02da7..fe15be0 100644
--- a/hw/usb/core.c
+++ b/hw/usb/core.c
@@ -550,7 +550,7 @@  void usb_packet_cleanup(USBPacket *p)
     qemu_iovec_destroy(&p->iov);
 }
 
-void usb_ep_init(USBDevice *dev)
+void usb_ep_reset(USBDevice *dev)
 {
     int ep;
 
@@ -559,7 +559,6 @@  void usb_ep_init(USBDevice *dev)
     dev->ep_ctl.ifnum = 0;
     dev->ep_ctl.dev = dev;
     dev->ep_ctl.pipeline = false;
-    QTAILQ_INIT(&dev->ep_ctl.queue);
     for (ep = 0; ep < USB_MAX_ENDPOINTS; ep++) {
         dev->ep_in[ep].nr = ep + 1;
         dev->ep_out[ep].nr = ep + 1;
@@ -573,6 +572,16 @@  void usb_ep_init(USBDevice *dev)
         dev->ep_out[ep].dev = dev;
         dev->ep_in[ep].pipeline = false;
         dev->ep_out[ep].pipeline = false;
+    }
+}
+
+void usb_ep_init(USBDevice *dev)
+{
+    int ep;
+
+    usb_ep_reset(dev);
+    QTAILQ_INIT(&dev->ep_ctl.queue);
+    for (ep = 0; ep < USB_MAX_ENDPOINTS; ep++) {
         QTAILQ_INIT(&dev->ep_in[ep].queue);
         QTAILQ_INIT(&dev->ep_out[ep].queue);
     }
diff --git a/hw/usb/host-linux.c b/hw/usb/host-linux.c
index 5479fb5..9ba8925 100644
--- a/hw/usb/host-linux.c
+++ b/hw/usb/host-linux.c
@@ -1136,7 +1136,7 @@  static int usb_linux_update_endp_table(USBHostDevice *s)
     USBDescriptor *d;
     bool active = false;
 
-    usb_ep_init(&s->dev);
+    usb_ep_reset(&s->dev);
 
     for (i = 0;; i += d->bLength) {
         if (i+2 >= s->descr_len) {
@@ -1239,7 +1239,7 @@  static int usb_linux_update_endp_table(USBHostDevice *s)
     return 0;
 
 error:
-    usb_ep_init(&s->dev);
+    usb_ep_reset(&s->dev);
     return 1;
 }
 
@@ -1326,6 +1326,7 @@  static int usb_host_open(USBHostDevice *dev, int bus_num,
         goto fail;
     }
 
+    usb_ep_init(&dev->dev);
     ret = usb_linux_update_endp_table(dev);
     if (ret) {
         goto fail;