[v2] Release usb devices on shutdown and usb_del command

When closig Vm or removing usb on guest via usb_del monitor command,
qemu does not return the control to the host, the user have to
unplug and plug the device in order to use it on the host.

v2:
added empty methods to usb-bsd and usb-stub.
release usb devices when main is out.

Signed-off-by: Shahar Havivi <shaharh@redhat.com>
---
 hw/usb-bus.c |    4 ++++
 hw/usb.h     |    2 ++
 usb-bsd.c    |   10 ++++++++++
 usb-linux.c  |   21 +++++++++++++++++++++
 usb-stub.c   |   10 ++++++++++
 vl.c         |    1 +
 6 files changed, 48 insertions(+), 0 deletions(-)

On 05/19/2010 12:10 PM, Shahar Havivi wrote:
> When closig Vm or removing usb on guest via usb_del monitor command,
> qemu does not return the control to the host, the user have to
> unplug and plug the device in order to use it on the host.
> 
> v2:
> added empty methods to usb-bsd and usb-stub.
> release usb devices when main is out.
> 
> Signed-off-by: Shahar Havivi <shaharh@redhat.com>
> ---
>  hw/usb-bus.c |    4 ++++
>  hw/usb.h     |    2 ++
>  usb-bsd.c    |   10 ++++++++++
>  usb-linux.c  |   21 +++++++++++++++++++++
>  usb-stub.c   |   10 ++++++++++
>  vl.c         |    1 +
>  6 files changed, 48 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/usb-bus.c b/hw/usb-bus.c
> index b692503..75dc819 100644
> --- a/hw/usb-bus.c
> +++ b/hw/usb-bus.c
> @@ -207,6 +207,10 @@ int usb_device_delete_addr(int busnr, int addr)
>          return -1;
>      dev = port->dev;
>  
> +    if (!strcmp(dev->info->usbdevice_name, "host")) {
> +        usb_host_device_release(dev);
> +    }
> +

Shouldn't this be done through a callback -- say usbdevice_release
similar to usbdevice_init -- instead of embedding host specifics here?
You wouldn't need the bsd and stub stubs then.

David


>      qdev_free(&dev->qdev);
>      return 0;
>  }
> diff --git a/hw/usb.h b/hw/usb.h
> index 00d2802..08c48d2 100644
> --- a/hw/usb.h
> +++ b/hw/usb.h
> @@ -258,6 +258,8 @@ void usb_send_msg(USBDevice *dev, int msg);
>  USBDevice *usb_host_device_open(const char *devname);
>  int usb_host_device_close(const char *devname);
>  void usb_host_info(Monitor *mon);
> +int usb_host_device_release(USBDevice *dev);
> +void usb_cleanup(void);
>  
>  /* usb-hid.c */
>  void usb_hid_datain_cb(USBDevice *dev, void *opaque, void (*datain)(void *));
> diff --git a/usb-bsd.c b/usb-bsd.c
> index 48567a3..fc9ea80 100644
> --- a/usb-bsd.c
> +++ b/usb-bsd.c
> @@ -634,3 +634,13 @@ int usb_host_device_close(const char *devname)
>  {
>      return 0;
>  }
> +
> +int usb_host_device_release(USBDevice *dev)
> +{
> +    return 0;
> +}
> +
> +void usb_cleanup(void)
> +{
> +    return 0;
> +}
> diff --git a/usb-linux.c b/usb-linux.c
> index 88273ff..cea5b84 100644
> --- a/usb-linux.c
> +++ b/usb-linux.c
> @@ -286,6 +286,27 @@ static void async_cancel(USBPacket *unused, void *opaque)
>      }
>  }
>  
> +void usb_cleanup(void)
> +{
> +    struct USBHostDevice *s;
> +
> +    QTAILQ_FOREACH(s, &hostdevs, next) {
> +        if (s->fd != -1) {
> +            usb_host_device_release((USBDevice*)s);
> +        }
> +    }
> +}
> +
> +int usb_host_device_release(USBDevice *dev)
> +{
> +    int ret;
> +
> +    USBHostDevice *s = DO_UPCAST(USBHostDevice, dev, dev);
> +    ret = ioctl(s->fd, USBDEVFS_RESET);
> +
> +    return ret;
> +}
> +
>  static int usb_host_claim_interfaces(USBHostDevice *dev, int configuration)
>  {
>      int dev_descr_len, config_descr_len;
> diff --git a/usb-stub.c b/usb-stub.c
> index 9c3fcea..4432c2e 100644
> --- a/usb-stub.c
> +++ b/usb-stub.c
> @@ -50,3 +50,13 @@ int usb_host_device_close(const char *devname)
>  {
>      return 0;
>  }
> +
> +int usb_host_device_release(USBDevice *dev)
> +{
> +    return 0;
> +}
> +
> +void usb_cleanup(void)
> +{
> +    return 0;
> +}
> diff --git a/vl.c b/vl.c
> index d77b47c..e3f4dc9 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -3914,6 +3914,7 @@ int main(int argc, char **argv, char **envp)
>      main_loop();
>      quit_timers();
>      net_cleanup();
> +    usb_cleanup();
>  
>      return 0;
>  }

"David S. Ahern" <daahern@cisco.com> writes:

> On 05/19/2010 12:10 PM, Shahar Havivi wrote:
>> When closig Vm or removing usb on guest via usb_del monitor command,
>> qemu does not return the control to the host, the user have to
>> unplug and plug the device in order to use it on the host.
>> 
>> v2:
>> added empty methods to usb-bsd and usb-stub.
>> release usb devices when main is out.
>> 
>> Signed-off-by: Shahar Havivi <shaharh@redhat.com>
>> ---
>>  hw/usb-bus.c |    4 ++++
>>  hw/usb.h     |    2 ++
>>  usb-bsd.c    |   10 ++++++++++
>>  usb-linux.c  |   21 +++++++++++++++++++++
>>  usb-stub.c   |   10 ++++++++++
>>  vl.c         |    1 +
>>  6 files changed, 48 insertions(+), 0 deletions(-)
>> 
>> diff --git a/hw/usb-bus.c b/hw/usb-bus.c
>> index b692503..75dc819 100644
>> --- a/hw/usb-bus.c
>> +++ b/hw/usb-bus.c
>> @@ -207,6 +207,10 @@ int usb_device_delete_addr(int busnr, int addr)
>>          return -1;
>>      dev = port->dev;
>>  
>> +    if (!strcmp(dev->info->usbdevice_name, "host")) {
>> +        usb_host_device_release(dev);
>> +    }
>> +
>
> Shouldn't this be done through a callback -- say usbdevice_release
> similar to usbdevice_init -- instead of embedding host specifics here?
> You wouldn't need the bsd and stub stubs then.
>
> David

What about the existing callbacks?  Could handle_destroy do?

Note: usbdevice_init() is not for general initialization, just for
dealing with the legacy -usbdevice command line.

>>      qdev_free(&dev->qdev);
>>      return 0;
>>  }
[...]
>> diff --git a/usb-linux.c b/usb-linux.c
>> index 88273ff..cea5b84 100644
>> --- a/usb-linux.c
>> +++ b/usb-linux.c
>> @@ -286,6 +286,27 @@ static void async_cancel(USBPacket *unused, void *opaque)
>>      }
>>  }
>>  
>> +void usb_cleanup(void)
>> +{
>> +    struct USBHostDevice *s;
>> +
>> +    QTAILQ_FOREACH(s, &hostdevs, next) {
>> +        if (s->fd != -1) {
>> +            usb_host_device_release((USBDevice*)s);
>> +        }
>> +    }
>> +}
>> +
>> +int usb_host_device_release(USBDevice *dev)
>> +{
>> +    int ret;
>> +
>> +    USBHostDevice *s = DO_UPCAST(USBHostDevice, dev, dev);
>> +    ret = ioctl(s->fd, USBDEVFS_RESET);
>> +
>> +    return ret;
>> +}
>> +
>>  static int usb_host_claim_interfaces(USBHostDevice *dev, int configuration)
>>  {
>>      int dev_descr_len, config_descr_len;
[...]
>> diff --git a/vl.c b/vl.c
>> index d77b47c..e3f4dc9 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -3914,6 +3914,7 @@ int main(int argc, char **argv, char **envp)
>>      main_loop();
>>      quit_timers();
>>      net_cleanup();
>> +    usb_cleanup();
>>  
>>      return 0;
>>  }

Figure we'd have to clean up the qdev tree on exit.  Gerd?

Hi,

> What about the existing callbacks?  Could handle_destroy do?

For hot-unplug it should do.

>>> --- a/vl.c
>>> +++ b/vl.c
>>> @@ -3914,6 +3914,7 @@ int main(int argc, char **argv, char **envp)
>>>       main_loop();
>>>       quit_timers();
>>>       net_cleanup();
>>> +    usb_cleanup();
>>>
>>>       return 0;
>>>   }
>
> Figure we'd have to clean up the qdev tree on exit.  Gerd?

Hmm, yes.  Question is how to do that best.  There is qdev_free(). 
Today this is used for hot-unplug only.  Using it on exit() too could 
have unwanted guest-visible side effects as it doesn't just release 
ressources, but also unplugs the device if possible.

Maybe it is better to add a exit notifier ...

cheers,
   Gerd