From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:34468) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TLMMf-0007Ik-Qk for qemu-devel@nongnu.org; Mon, 08 Oct 2012 19:03:59 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TLMMc-0005mz-Hf for qemu-devel@nongnu.org; Mon, 08 Oct 2012 19:03:57 -0400 Received: from mx1.redhat.com ([209.132.183.28]:21815) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TLMMc-0005mV-9G for qemu-devel@nongnu.org; Mon, 08 Oct 2012 19:03:54 -0400 Message-ID: <50735C33.70703@redhat.com> Date: Tue, 09 Oct 2012 01:05:23 +0200 From: Hans de Goede MIME-Version: 1.0 References: <505D84F3.3070106@web.de> In-Reply-To: <505D84F3.3070106@web.de> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [v2] usb-redir: Allow to attach USB 2.0 devices to 1.1 host controller List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jan Kiszka Cc: qemu-devel , Gerd Hoffmann Hi, Some comments inline, I've a version with all the comments fixed here: http://cgit.freedesktop.org/~jwrdegoede/qemu/commit/?h=qemu-kvm-1.2-usbredir&id=5e342d2d2186757cc41b2f834e3503cf10e98c2b On 09/22/2012 01:29 AM, Jan Kiszka wrote: > From: Jan Kiszka > > 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. > > Signed-off-by: Jan Kiszka > > --- > Changes in v2: > - fix incompatibility marking via introduction of compatible_speedmask > - disconnect device if incompatibility is detected when already > attached > > hw/usb/redirect.c | 24 +++++++++++++++++++++++- > 1 files changed, 23 insertions(+), 1 deletions(-) > > diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c > index b10241a..2099ea4 100644 > --- a/hw/usb/redirect.c > +++ b/hw/usb/redirect.c > @@ -105,6 +105,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); > @@ -1037,6 +1038,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; > + This needs to be done in disconnect to, as that resets the device state, as the same usb-redir device may be reused to later redirect another device. > /* Let the backend know we are ready */ > qemu_chr_fe_open(dev->cs); > qemu_chr_add_handlers(dev->cs, usbredir_chardev_can_read, > @@ -1177,10 +1181,12 @@ static void usbredir_device_connect(void *priv, > case usb_redir_speed_low: > speed = "low speed"; > dev->dev.speed = USB_SPEED_LOW; > + dev->dev.speedmask = 0; > break; > case usb_redir_speed_full: > speed = "full speed"; > dev->dev.speed = USB_SPEED_FULL; > + dev->dev.speedmask = 0; > break; > case usb_redir_speed_high: > speed = "high speed"; > @@ -1189,6 +1195,7 @@ static void usbredir_device_connect(void *priv, > case usb_redir_speed_super: > speed = "super speed"; > dev->dev.speed = USB_SPEED_SUPER; > + dev->dev.speedmask = 0; > break; > default: > speed = "unknown speed"; All the above setting to 0 of speedmask are not necessary, as immediately afterwards we do: > @@ -1210,7 +1217,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)) { > @@ -1271,6 +1278,14 @@ static void usbredir_interface_info(void *priv, > } > } > > +static void usbredir_mark_fullspeed_incompatible(USBRedirDevice *dev) > +{ > + dev->compatible_speedmask &= ~USB_SPEED_MASK_FULL; > + if (dev->dev.attached && dev->dev.port->dev->speed == USB_SPEED_FULL) { This test won't work as for a high speed dev dev->dev.port->dev->speed == USB_SPEED_HIGH Also this is not the right place to test, as this gets called from middle in the loop over the endpoints, so any ep state cleanup usbredir_device_disconnect() does will then get partially undone by the rest of the loop. > + usbredir_device_disconnect(dev); It would be better to use usbredir_device_reject here, this way the spice-client / usbredirserver also gets notified of the device getting disconnected by qemu. > + } > +} > + > static void usbredir_ep_info(void *priv, > struct usb_redir_ep_info_header *ep_info) > { > @@ -1286,7 +1301,14 @@ static void usbredir_ep_info(void *priv, > case usb_redir_type_invalid: > break; > case usb_redir_type_iso: > + usbredir_mark_fullspeed_incompatible(dev); > + /* 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_fullspeed_incompatible(dev); > + } > if (dev->endpoint[i].interval == 0) { > ERROR("Received 0 interval for isoc or irq endpoint\n"); > usbredir_device_disconnect(dev); > Regards, Hans