diff mbox

[v2] usb-redir: Allow to attach USB 2.0 devices to 1.1 host controller

Message ID 505D84F3.3070106@web.de
State New
Headers show

Commit Message

Jan Kiszka Sept. 22, 2012, 9:29 a.m. UTC 
From: Jan Kiszka <jan.kiszka@siemens.com>

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 <jan.kiszka@siemens.com>
---

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(-)

Comments

Jan Kiszka Oct. 8, 2012, 5:36 p.m. UTC | #1 
On 2012-09-22 11:29, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
> 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 <jan.kiszka@siemens.com>
> ---
> 
> 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;
> +
>      /* 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";
> @@ -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) {
> +        usbredir_device_disconnect(dev);
> +    }
> +}
> +
>  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);
> 

Any comments? Or already queued somewhere?

Jan
Hans de Goede Oct. 8, 2012, 11:05 p.m. UTC | #2 
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 <jan.kiszka@siemens.com>
>
> 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 <jan.kiszka@siemens.com>
>
> ---
> 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
Hans de Goede Oct. 9, 2012, 8:38 a.m. UTC | #3 
Hi,

On 10/09/2012 01:05 AM, Hans de Goede wrote:
> 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
>

And I now also have redirecting super-speed mass-storage devices
to an emulated EHCI controller working:
http://cgit.freedesktop.org/~jwrdegoede/qemu/commit/?h=qemu-kvm-1.2-usbredir&id=a6bdb97481c62b42e4106e7670364e61acde8037

Not sure what will happen when we actually get UAS devices in this
scenario though, will need to test, but UAS devices are very hard to
find...

Gerd, do you know where I can buy an UAS device (note a German
source is fine) ?

Regards,

Hans
Gerd Hoffmann Oct. 9, 2012, 8:40 a.m. UTC | #4 
Hi,

> Not sure what will happen when we actually get UAS devices in this
> scenario though, will need to test, but UAS devices are very hard to
> find...
> 
> Gerd, do you know where I can buy an UAS device (note a German
> source is fine) ?

No, I only have virtual ones ;)

/me got a usb3 stick, but it is a BOT device.

cheers,
  Gerd
Hans de Goede Oct. 9, 2012, 10:05 a.m. UTC | #5 
Hi,

On 10/09/2012 10:38 AM, Hans de Goede wrote:
> Hi,
>
> On 10/09/2012 01:05 AM, Hans de Goede wrote:
>> 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
>>
>
> And I now also have redirecting super-speed mass-storage devices
> to an emulated EHCI controller working:
> http://cgit.freedesktop.org/~jwrdegoede/qemu/commit/?h=qemu-kvm-1.2-usbredir&id=a6bdb97481c62b42e4106e7670364e61acde8037
>

Oops this causes full speed devices to get attached as high speed, not good,
new revision of both patches here:
http://cgit.freedesktop.org/~jwrdegoede/qemu/commit/?h=qemu-kvm-1.2-usbredir&id=76231208ad556cceb07f97db349699421d121030
http://cgit.freedesktop.org/~jwrdegoede/qemu/commit/?h=qemu-kvm-1.2-usbredir&id=7a04479a4dd1d810199fbb6d7d36eae6390c36bc

Regards,

Hans
diff mbox

Patch

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;
+
     /* 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";
@@ -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) {
+        usbredir_device_disconnect(dev);
+    }
+}
+
 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);