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

[Hans de Goede]

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 <address@hidden>
>
> 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 <address@hidden>
>
> ---
> 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