Re: [PATCH 1/5] husb: support for USB host device auto disconnect.

[Anthony Liguori <anthony@codemonkey.ws>]

Max Krasnyansky wrote:
> I got really annoyed by the fact that you have to manually do
> usb_del in the monitor when host device is unplugged and decided
> to fix it :)
>   

Yes, I've felt the same annoyance.

> Basically we now automatically remove guest USB device
> when the actual host device is disconnected.
>
> At first I've extended set_fd_handlerX() stuff to support checking
> for exceptions on fds. But unfortunately usbfs code does not wake up
> user-space process when device is removed, which means we need a
> timer to periodically check if device is still there. So I removed
> fd exception stuff and implemented it with the timer.
>   

I'm surprised there isn't an EOF when the device is disconnected.  There 
has to be some sort of userspace notification that a device has been 
unplugged.  I don't really like the idea of polling but it wouldn't 
surprise me if we had to.

> Signed-off-by: Max Krasnyansky <maxk@kernel.org>
> ---
>  hw/usb.h    |    1 +
>  usb-linux.c |   56 +++++++++++++++++++++++++++++++++++++++++++++++---------
>  vl.c        |   26 ++++++++++++++++++--------
>  3 files changed, 66 insertions(+), 17 deletions(-)
>
> diff --git a/hw/usb.h b/hw/usb.h
> index 8bdc68d..2edb982 100644
> --- a/hw/usb.h
> +++ b/hw/usb.h
> @@ -197,6 +197,7 @@ static inline void usb_cancel_packet(USBPacket * p)
>      p->cancel_cb(p, p->cancel_opaque);
>  }
>  
> +int usb_device_del_addr(int bus_num, int addr);
>  void usb_attach(USBPort *port, USBDevice *dev);
>  int usb_generic_handle_packet(USBDevice *s, USBPacket *p);
>  int set_usb_string(uint8_t *buf, const char *str);
> diff --git a/usb-linux.c b/usb-linux.c
> index d3e4e2e..0023c1d 100644
> --- a/usb-linux.c
> +++ b/usb-linux.c
> @@ -22,6 +22,7 @@
>   * THE SOFTWARE.
>   */
>  #include "qemu-common.h"
> +#include "qemu-timer.h"
>  #include "hw/usb.h"
>  #include "console.h"
>  
> @@ -77,6 +78,7 @@ typedef struct USBHostDevice {
>      uint8_t descr[1024];
>      int descr_len;
>      int urbs_ready;
> +    QEMUTimer *timer;
>  } USBHostDevice;
>  
>  typedef struct PendingURB {
> @@ -165,7 +167,11 @@ static int usb_host_update_interfaces(USBHostDevice *dev, int configuration)
>          }
>          config_descr_len = dev->descr[i];
>  
> -        if (configuration == dev->descr[i + 5])
> +#ifdef DEBUG
> +	printf("config #%d need %d\n", dev->descr[i + 5], configuration); 
> +#endif
> +
> +        if (configuration < 0 || configuration == dev->descr[i + 5])
>              break;
>  
>          i += config_descr_len;
> @@ -230,8 +236,11 @@ static void usb_host_handle_destroy(USBDevice *dev)
>  {
>      USBHostDevice *s = (USBHostDevice *)dev;
>  
> +    qemu_del_timer(s->timer);
> +
>   

qemu_del_timer() only removes a pending timer.  You need 
qemu_free_timer() to actually free the memory associated with it.

>      dev->urbs_ready = 0;
>      return (USBDevice *)dev;
>  fail:
> -    if (dev)
> +    if (dev) {
> +	if (dev->timer)
> +		qemu_del_timer(dev->timer);
>   

Here too.

Regards,

Anthony Liguori