From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:34380) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TDXIA-0004bh-Go for qemu-devel@nongnu.org; Mon, 17 Sep 2012 05:07:04 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TDXI3-000336-Rn for qemu-devel@nongnu.org; Mon, 17 Sep 2012 05:06:58 -0400 Received: from mx1.redhat.com ([209.132.183.28]:40239) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TDXI3-00032l-JT for qemu-devel@nongnu.org; Mon, 17 Sep 2012 05:06:51 -0400 Message-ID: <5056E876.9010400@redhat.com> Date: Mon, 17 Sep 2012 11:08:06 +0200 From: Hans de Goede MIME-Version: 1.0 References: <5054AC57.205@web.de> In-Reply-To: <5054AC57.205@web.de> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] 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, On 09/15/2012 06:27 PM, 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. > > Signed-off-by: Jan Kiszka Interesting thanks for the patch. I like the approach you took (simple code), but unfortunately it won't work, if you look at usbredir_device_connect(), where you do the dev->dev.speedmask |= USB_SPEED_MASK_FULL, it also actually attaches the device to the controller, from which point on the guest will see the device. What happens on the wire is that the usbredir-host sends (in order): -ep_info + interface_info -device_connect message So your clearing of the speed-mask will never trigger, unless ep-info gets repeated later (which it does under certain circumstances). I suggest instead changing the code to set a "fullspeed_compatible" flag in struct USBRedirDevice from usbredir_device_disconnect(), clear that flag from usbredir_ep_info and use it to add to the mask in usbredir_device_connect(). ### Another issue is what happens if a device "grows" incompatible endpoints after being attached, ie a webcam could have no isoc endpoints in alt setting 0, and then grow an isoc endpoint on a set_interface. So we would need a check for a device becoming not fullspeed compat while being attached at fullspeed in usbredir_ep_info(), and then call usbredir_reject_device() when this happens. Although not pretty I'm ok with this, since I actually want to add similar code to allow usb-3 (superspeed) devices like a usb-3 usb-stick to work with ehci or uhci controllers :) Regards, Hans > --- > hw/usb/redirect.c | 10 ++++++++++ > 1 files changed, 10 insertions(+), 0 deletions(-) > > diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c > index 5301a69..bc36e53 100644 > --- a/hw/usb/redirect.c > +++ b/hw/usb/redirect.c > @@ -1098,6 +1098,9 @@ static void usbredir_device_connect(void *priv, > } > > dev->dev.speedmask = (1 << dev->dev.speed); > + if (dev->dev.speed == USB_SPEED_HIGH) { > + dev->dev.speedmask |= USB_SPEED_MASK_FULL; > + } > dev->device_info = *device_connect; > > if (usbredir_check_filter(dev)) { > @@ -1172,7 +1175,14 @@ static void usbredir_ep_info(void *priv, > case usb_redir_type_invalid: > break; > case usb_redir_type_iso: > + dev->dev.speedmask &= ~USB_SPEED_MASK_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) { > + dev->dev.speedmask &= ~USB_SPEED_MASK_FULL; > + } > if (dev->endpoint[i].interval == 0) { > ERROR("Received 0 interval for isoc or irq endpoint\n"); > usbredir_device_disconnect(dev); >