[Qemu-devel] [PATCH v2] Release usb devices on shutdown and usb_del command

[Qemu-devel] [PATCH v2] Release usb devices on shutdown and usb_del command

[Shahar Havivi]

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);
+    }
+
     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;
 }
-- 
1.7.0.4

Re: [Qemu-devel] [PATCH v2] Release usb devices on shutdown and usb_del command

[David S. Ahern]

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;
>  }

Re: [Qemu-devel] [PATCH v2] Release usb devices on shutdown and usb_del command

[Markus Armbruster]

"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?

Re: [Qemu-devel] [PATCH v2] Release usb devices on shutdown and usb_del command

[Gerd Hoffmann]

   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

[Qemu-devel] Re: [PATCH] Release usb devices on shutdown and usb_del command

[Shahar Havivi]

Remove usb_host_device_release and using usb_host_close to handle usb_del command.
Gerd, What do you think about the usb_cleanup()?
If it will be in usb-linux.c we will
have to ad atexit to each device, if we usb-bus.c we will have to implement in
bsd and stub...

Signed-off-by: Shahar Havivi <shaharh@redhat.com>
---
 hw/usb.h    |    1 +
 usb-bsd.c   |    5 +++++
 usb-linux.c |   12 ++++++++++++
 usb-stub.c  |    5 +++++
 vl.c        |    1 +
 5 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/hw/usb.h b/hw/usb.h
index 00d2802..7ddf63c 100644
--- a/hw/usb.h
+++ b/hw/usb.h
@@ -258,6 +258,7 @@ 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);
+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..c3eb891 100644
--- a/usb-bsd.c
+++ b/usb-bsd.c
@@ -634,3 +634,8 @@ int usb_host_device_close(const char *devname)
 {
     return 0;
 }
+
+void usb_cleanup(void)
+{
+    return 0;
+}
diff --git a/usb-linux.c b/usb-linux.c
index 88273ff..98909b7 100644
--- a/usb-linux.c
+++ b/usb-linux.c
@@ -286,6 +286,17 @@ static void async_cancel(USBPacket *unused, void *opaque)
     }
 }
 
+void usb_cleanup(void)
+{
+    struct USBHostDevice *s;
+
+    QTAILQ_FOREACH(s, &hostdevs, next) {
+        if (s->fd != -1) {
+            ioctl(s->fd, USBDEVFS_RESET);
+        }
+    }
+}
+
 static int usb_host_claim_interfaces(USBHostDevice *dev, int configuration)
 {
     int dev_descr_len, config_descr_len;
@@ -991,6 +1002,7 @@ static int usb_host_close(USBHostDevice *dev)
     async_complete(dev);
     dev->closing = 0;
     usb_device_detach(&dev->dev);
+    ioctl(dev->fd, USBDEVFS_RESET);
     close(dev->fd);
     dev->fd = -1;
     return 0;
diff --git a/usb-stub.c b/usb-stub.c
index 9c3fcea..bb513de 100644
--- a/usb-stub.c
+++ b/usb-stub.c
@@ -50,3 +50,8 @@ int usb_host_device_close(const char *devname)
 {
     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;
 }
-- 
1.7.0.4

Re: [Qemu-devel] [PATCH v2] Release usb devices on shutdown and usb_del command

[David S. Ahern]

> On 05/21/2010 1:33 AM, Markus Armbruster wrote:
> Note: usbdevice_init() is not for general initialization, just for
> dealing with the legacy -usbdevice command line.

Perhaps the comment before usbdevice_create is misleading or has some
conversion not completed? Right now even devices entered through the
monitor go through usbdevice_create -- which is the only place the
usbdevice_init() hook is invoked.

David

Re: [Qemu-devel] [PATCH v2] Release usb devices on shutdown and usb_del command

[Markus Armbruster]

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

>> On 05/21/2010 1:33 AM, Markus Armbruster wrote:
>> Note: usbdevice_init() is not for general initialization, just for
>> dealing with the legacy -usbdevice command line.
>
> Perhaps the comment before usbdevice_create is misleading or has some
> conversion not completed? Right now even devices entered through the
> monitor go through usbdevice_create -- which is the only place the
> usbdevice_init() hook is invoked.

... just for dealing with the -usbdevice command line and monitor
command usb_add (both legacy).

Check out docs/qdev-device-use.txt for the current way to add devices.
The monitor equivalent to -device is device_add.

[Qemu-devel] Re: [PATCH] Release usb devices on shutdown and usb_del command

[Gerd Hoffmann]

On 05/21/10 19:55, Shahar Havivi wrote:
> Remove usb_host_device_release and using usb_host_close to handle usb_del command.
> Gerd, What do you think about the usb_cleanup()?

We need a mechanism to handle this for sure.  I don't like that 
usb-specific approach very much though.

I think we should either do that at qdev level, then at exit walk the 
whole device tree and call cleanup functions (if present).  So every 
device has the chance to do cleanups when needed.

Or we could have a exit notifier, which can be used for device (and also 
other) cleanup work.

I tend to think that a exit notifier will be better.  We probably have 
only a few devices which actually have to do some cleanup work (usb 
passthrough, maybe pci passthrough too), so building qdev infrastructure 
for that feels a bit like overkill.  And exit notifiers are more 
generic, i.e. it will also work for non-device stuff.

cheers,
   Gerd

[Qemu-devel] Re: [PATCH] Release usb devices on shutdown and usb_del command

[Shahar Havivi]

On Tue, May 25, 2010 at 10:58:50AM +0200, Gerd Hoffmann wrote:
> Date: Tue, 25 May 2010 10:58:50 +0200
> From: Gerd Hoffmann <kraxel@redhat.com>
> To: Shahar Havivi <shaharh@redhat.com>
> CC: "David S. Ahern" <daahern@cisco.com>,
> 	Markus Armbruster <armbru@redhat.com>, qemu-devel@nongnu.org
> Subject: Re: [PATCH] Release usb devices on shutdown and usb_del command
> 
> On 05/21/10 19:55, Shahar Havivi wrote:
> >Remove usb_host_device_release and using usb_host_close to handle usb_del command.
> >Gerd, What do you think about the usb_cleanup()?
> 
> We need a mechanism to handle this for sure.  I don't like that
> usb-specific approach very much though.
> 
> I think we should either do that at qdev level, then at exit walk
> the whole device tree and call cleanup functions (if present).  So
> every device has the chance to do cleanups when needed.
> 
> Or we could have a exit notifier, which can be used for device (and
> also other) cleanup work.
> 
> I tend to think that a exit notifier will be better.  We probably
> have only a few devices which actually have to do some cleanup work
> (usb passthrough, maybe pci passthrough too), so building qdev
> infrastructure for that feels a bit like overkill.  And exit
> notifiers are more generic, i.e. it will also work for non-device
> stuff.
> 
> cheers,
>   Gerd
> 
We can add at exit to the first device that added in usb-linux.c via
usb_host_device_open() or usb_host_initfn() and the exit will iterate
all usb devices in linux only,
How is that solution?