[Qemu-devel] Re: [PATCH 2/5] husb: support for USB host device auto connect.

[Max Krasnyansky <maxk@kernel.org>]

Anthony Liguori wrote:
> Max Krasnyansky wrote:
>> Anyway, it's implemented using a periodic timer that scans host devices
>> and grabs those that match the filter. Timer is started when the first
>> filter is added.
>>   
> 
> Again, there has to be a way to get notified of usb device add/remove in
> Linux.

Yes the best way is probably registering with HAL via dbus.
Do you mind if we do something like that incrementally ?
ie Merge the current patch then change polling to HAL registration.
That would simplify testing/patch stacking for me. Existing users are
not affected anyway since the timer is activated only when new filter is added.

>> +static USBHostDevice *hostdev_find(int bus_num, int addr)
>> +{
>> +    USBHostDevice *s = hostdev_list;
>> +    while (s) {
>> +        if (s->bus_num == bus_num && s->addr == addr)
>> +            return s;
>> +        s = s->next;
>> +    }
>> +    return NULL;
>> +}
>>   
> 
> There are generic list macros in sys-queue.h.  They aren't universally
> used though so it's up to you whether you think it's appropriate.
Oh, I looked around (mostly USB code) and did not see any generic lists.
I'd say ideally we should clean things up to use generic stuff as much as
possible. There seems to be a lot of duplication currently.

>> +struct USBAutoFilter {
>> +    struct USBAutoFilter *next;
>> +    int bus_num;
>> +    int addr;
>> +    int vendor_id;
>> +    int product_id;
>> +};
>> +
>> +static QEMUTimer *usb_auto_timer;
>> +static struct USBAutoFilter *usb_auto_filter;
>> +
>> +static int usb_host_auto_scan(void *opaque, int bus_num, int addr,
>> +                     int class_id, int vendor_id, int product_id,
>> +                     const char *product_name, int speed)
>> +{
>> +    struct USBAutoFilter *f;
>> +    struct USBDevice *dev;
>> +
>> +    /* Ignore hubs */
>> +    if (class_id == 9)
>> +        return 0;
>> +
>> +    for (f = usb_auto_filter; f; f = f->next) {
>> +        // printf("Auto match: bus_num %d addr %d vid %d pid %d\n",
>> +    //    bus_num, addr, vendor_id, product_id);
>>   
> 
> An #ifdef guard would be nicer.
Yep. Missed that one.

>> +    if (f->bus_num >= 0 && f->bus_num != bus_num)
>> +            continue;
>> +
>> +    if (f->addr >= 0 && f->addr != addr)
>> +            continue;
>> +
>> +    if (f->vendor_id >= 0 && f->vendor_id != vendor_id)
>> +            continue;
>> +
>> +    if (f->product_id >= 0 && f->product_id != product_id)
>> +            continue;
>> +
>> +        /* We got a match */
>> +
>> +        /* Allredy attached ? */
>> +        if (hostdev_find(bus_num, addr))
>> +            return 0;
>> +
>> +        printf("Auto open: bus_num %d addr %d\n", bus_num, addr);
>>   
> 
> Please make this debug too.
Will do.


> 
>> +    dev = usb_host_device_open_addr(bus_num, addr, product_name);
>> +    if (dev)
>> +        usb_device_add_dev(dev);
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static void usb_host_auto_timer(void *unused)
>> +{
>> +    usb_host_scan(NULL, usb_host_auto_scan);
>> +    qemu_mod_timer(usb_auto_timer, qemu_get_clock(rt_clock) + 2000);
>> +}
>> +
>> +/*
>> + * Add autoconnect filter
>> + * -1 means 'any' (device, vendor, etc)
>> + */
>> +static void usb_host_auto_add(int bus_num, int addr, int vendor_id,
>> int product_id)
>> +{
>> +    struct USBAutoFilter *f = qemu_mallocz(sizeof(*f));
>> +    if (!f) {
>> +        printf("Failed to allocate auto filter\n");
>> +    return;
>> +    }
>> +
>> +    f->bus_num = bus_num;
>> +    f->addr    = addr;
>> +    f->vendor_id  = vendor_id;
>> +    f->product_id = product_id;
>> +
>> +    if (!usb_auto_filter) {
>> +        /*
>> +         * First entry. Init and start the monitor.
>> +         * Right now we're using timer to check for new devices.
>> +         * If this turns out to be too expensive we can move that
>> into a +         * separate thread.
>> +         */
>> +    usb_auto_timer = qemu_new_timer(rt_clock, usb_host_auto_timer,
>> NULL);
>> +    if (!usb_auto_timer) {
>> +            printf("Failed to allocate timer\n");
>>   
> 
> Please print errors to stderr.

Will do.

Max