Re: [PATCH v6] winebus.sys: Watch for hid raw device addition and removal

[Sebastian Lackner]

On 29.09.2016 20:48, Aric Stewart wrote:
> v2: Correct poll timeout
>     Style changes
> v3: vtable style instead of a callback
> v4: Suggestions from Sebastian Lackner
> v5: Fixing handling of other native in the compare function
> v6: Tweaks suggested by Sebastian Lackner
>
> Includes an implementation of a common bus_remove_hid_device
> Signed-off-by: Aric Stewart
>
> dlls/winebus.sys/bus.h      |  13 +++++-
>  dlls/winebus.sys/bus_udev.c | 106 +++++++++++++++++++++++++++++++++++++++++++-
>  dlls/winebus.sys/main.c     |  64 +++++++++++++++++++++-----
>  3 files changed, 169 insertions(+), 14 deletions(-)
>
> v6-0001-winebus.sys-Watch-for-hid-raw-device-addition-and-r.txt
>
> diff --git a/dlls/winebus.sys/bus.h b/dlls/winebus.sys/bus.h
> index dcb50f2..719d8fd 100644
> --- a/dlls/winebus.sys/bus.h
> +++ b/dlls/winebus.sys/bus.h
> @@ -19,8 +19,17 @@
>  /* Busses */
>  NTSTATUS WINAPI udev_driver_init(DRIVER_OBJECT *driver, UNICODE_STRING *registry_path) DECLSPEC_HIDDEN;
>
> +/* Native device function table */
> +typedef struct {
> - int(*compare_platform_device)(DEVICE_OBJECT *device, void *platform_dev);
>
> +} platform_vtbl;
> -
>
> +void *get_platform_private(DEVICE_OBJECT *device) DECLSPEC_HIDDEN;
> +DEVICE_OBJECT *find_hid_device(const platform_vtbl* vtbl, void *device) DECLSPEC_HIDDEN;
> -
>
> /* HID Plug and Play Bus */
>  NTSTATUS WINAPI common_pnp_dispatch(DEVICE_OBJECT *device, IRP *irp) DECLSPEC_HIDDEN;
> -DEVICE_OBJECT *bus_create_hid_device(DRIVER_OBJECT *driver, const WCHAR *busidW, void *native, WORD vid,
> +DEVICE_OBJECT *bus_create_hid_device(DRIVER_OBJECT *driver, const WCHAR *busidW, WORD vid,
>                                       WORD pid, DWORD version, DWORD uid, const WCHAR *serialW, BOOL is_gamepad,
> -
>                                 const GUID *class) DECLSPEC_HIDDEN;
> -
>                                 const GUID *class, const platform_vtbl* vtbl, DWORD platform_data_size) DECLSPEC_HIDDEN;
>
> +void bus_remove_hid_device(DEVICE_OBJECT *device) DECLSPEC_HIDDEN;
> diff --git a/dlls/winebus.sys/bus_udev.c b/dlls/winebus.sys/bus_udev.c
> index 1525861..b7995bc 100644
> --- a/dlls/winebus.sys/bus_udev.c
> +++ b/dlls/winebus.sys/bus_udev.c
> @@ -26,6 +26,11 @@
>  #ifdef HAVE_UNISTD_H
>  # include <unistd.h>
>  #endif
> -
>
> +#ifdef HAVE_SYS_POLL_H
> +# include <sys/poll.h>
> +#endif
> -
>
> #ifdef HAVE_LIBUDEV_H
>  # include <libudev.h>
>  #endif
> @@ -55,6 +60,10 @@ static const WCHAR hidraw_busidW[] = {'H','I','D','R','A','W',0};
>  #include "initguid.h"
>  DEFINE_GUID(GUID_DEVCLASS_HIDRAW, 0x3def44ad,0x242e,0x46e5,0x82,0x6d,0x70,0x72,0x13,0xf3,0xaa,0x81);
>
> +struct platform_private {
> - struct udev_device *udev_device;
>
> +};
> -
>
> static DWORD get_sysattr_dword(struct udev_device *dev, const char *sysattr, int base)
>  {
>      const char *attr = udev_device_get_sysattr_value(dev, sysattr);
> @@ -82,6 +91,17 @@ static WCHAR *get_sysattr_string(struct udev_device *dev, const char *sysattr)
>      return dst;
>  }
>
> +static int compare_platform_device(DEVICE_OBJECT *device, void *platform_dev)
> +{
> - struct udev_device *dev1 = ((struct platform_private*)get_platform_private(device))->udev_device;

There is a whitespace issue in the line above. Also, I would suggest to use a wrapper like for COM
interfaces to avoid having this conversion at multiple places. For example:

static inline platform_private *impl_from_DEVICE_OBJECT(DEVICE_OBJECT *device) { ... }
> - struct udev_device *dev2 = platform_dev;
> - return (strcmp(udev_device_get_syspath(dev1), udev_device_get_syspath(dev2)));
>
> +}
> -
>
> +static const platform_vtbl hidraw_vtbl = {
> - compare_platform_device,
>
> +};
> -
>
> static void try_add_device(struct udev_device *dev)
>  {
>      DWORD vid = 0, pid = 0, version = 0;
> @@ -117,12 +137,16 @@ static void try_add_device(struct udev_device *dev)
>      subsystem = udev_device_get_subsystem(dev);
>      if (strcmp(subsystem, "hidraw") == 0)
>      {
> -
>    device = bus_create_hid_device(udev_driver_obj, hidraw_busidW, dev, vid, pid,
> -
>                                   version, 0, serial, FALSE, &GUID_DEVCLASS_HIDRAW);
> -
>    device = bus_create_hid_device(udev_driver_obj, hidraw_busidW, vid, pid,
> -
>                                   version, 0, serial, FALSE, &GUID_DEVCLASS_HIDRAW, &hidraw_vtbl, sizeof(struct platform_private));
>
> }
>
> if (device)
> -
>
> {
> -
>    ((struct platform_private*)get_platform_private(device))->udev_device = dev;
>    udev_device_ref(dev);

udev_device_ref returns the device, so you could also do the assignment and incref in one line if you prefer. ;)
Not really an issue though.
> -
>    IoInvalidateDeviceRelations(device, BusRelations);
> - }
> else
>     WARN("Ignoring device %s with subsystem %s\n", debugstr_a(devnode), subsystem);
>
> @@ -164,6 +188,77 @@ static void build_initial_deviceset(void)
>      udev_enumerate_unref(enumerate);
>  }
>
> +/* This is our main even loop for reading devices */
> +static DWORD CALLBACK deviceloop_thread(void *args)
> +{
> - struct pollfd plfds[1];
> - struct udev_monitor *monitor = NULL;
> -
> - monitor = udev_monitor_new_from_netlink(udev_context, "udev");
> - if (!monitor)
> - {
> -
>    WARN("Unable to get udev monitor, device insertion and removal cannot be tracked\n");
> -
>    goto exit;
> - }
> - if (udev_monitor_filter_add_match_subsystem_devtype(monitor, "hidraw", NULL) < 0)
> - {
> -
>    WARN("Failed to add 'hidraw' subsystem to monitor\n");
> -
>    goto exit;
> - }
> -
> - if (udev_monitor_enable_receiving(monitor) < 0)
> - {
> -
>    WARN("Failed to start monitoring\n");
> -
>    goto exit;
> - }
> -
> - plfds[0].fd = udev_monitor_get_fd(monitor);
> - plfds[0].events = POLLIN;
> - if (plfds[0].fd < 0)
> - {
> -
>    WARN("Failed to get monitor fd\n");
> -
>    goto exit;
> - }
> - /* Run the event loop */
> - while (1)
> - {
> -
>    struct udev_device *dev;
> -
>    const char* action;
> -
> -
>    if (poll(plfds, 1, -1) <= 0) continue;
> -
> -
>    TRACE("udev reports device changes\n");
> -
>    dev = udev_monitor_receive_device(monitor);
> -
>    if (!dev)
> -
>    {
> -
>        FIXME("Failed to get device that has changed\n");
> -
>        continue;
> -
>    }
> -
>    action = udev_device_get_action(dev);
> -
>    if (!action)
> -
>        WARN("No action recieved\n");
> -
>    else if (strcmp(action, "add") == 0)
> -
>        try_add_device(dev);
> -
>    else if (strcmp(action, "remove") == 0)
> -
>    {
> -
>        DEVICE_OBJECT *device = find_hid_device(&hidraw_vtbl, dev);
> -
>        if (device)
> -
>        {
> -
>            struct udev_device *original = ((struct platform_private*)get_platform_private(device))->udev_device;
> -
>            bus_remove_hid_device(device);
> -
>            udev_device_unref(original);
> -
>        }
> -
>    }
> -
>    udev_device_unref(dev);
> - }
> -
>
> +exit:
> - TRACE("Monitor thread exiting\n");
> - if (monitor)
> -
>    udev_monitor_unref(monitor);
> - return 1;
>
> +}
> -
>
> NTSTATUS WINAPI udev_driver_init(DRIVER_OBJECT *driver, UNICODE_STRING *registry_path)
>  {
>      TRACE("(%p, %s)\n", driver, debugstr_w(registry_path->Buffer));
> @@ -178,6 +273,13 @@ NTSTATUS WINAPI udev_driver_init(DRIVER_OBJECT *driver, UNICODE_STRING *registry
>      driver->MajorFunction[IRP_MJ_PNP] = common_pnp_dispatch;
>  build_initial_deviceset();
> -
> - if (!CreateThread(NULL, 0, deviceloop_thread, NULL, 0, NULL))
> - {
> -
>    ERR("Unable to create udev device thread\n");
> -
>    return STATUS_UNSUCCESSFUL;

Wouldn't it make sense to proceed anyway? The initial device set could still
be working properly (and, you are not cleaning up already added drivers here).
> - }
> - return STATUS_SUCCESS;
>
> }
>
> diff --git a/dlls/winebus.sys/main.c b/dlls/winebus.sys/main.c
> index 1364eab..1cb8031 100644
> --- a/dlls/winebus.sys/main.c
> +++ b/dlls/winebus.sys/main.c
> @@ -47,13 +47,15 @@ struct pnp_device
>
> struct device_extension
>  {
> - void *native;  /* Must be the first member of the structure */
> - WORD vid, pid;
>    DWORD uid, version, index;
>    BOOL is_gamepad;
>    WCHAR *serial;
>    const WCHAR *busid;  /* Expected to be a static constant */
> - struct pnp_device *pnp_device;
> - const platform_vtbl *vtbl;
> -
> - BYTE platform_private[1];
>
> };
>
> static CRITICAL_SECTION device_list_cs;
> @@ -157,11 +159,11 @@ static WCHAR *get_compatible_ids(DEVICE_OBJECT *device)
>      return dst;
>  }
>
> -DEVICE_OBJECT *bus_create_hid_device(DRIVER_OBJECT *driver, const WCHAR *busidW, void *native, WORD vid,
> +DEVICE_OBJECT *bus_create_hid_device(DRIVER_OBJECT *driver, const WCHAR *busidW, WORD vid,
>                                       WORD pid, DWORD version, DWORD uid, const WCHAR *serialW, BOOL is_gamepad,
> -
>                                 const GUID *class)
> -
>                                 const GUID *class, const platform_vtbl *vtbl, DWORD platform_data_size)
>
> {
> - static const WCHAR device_name_fmtW[] = {'\','D','e','v','i','c','e','\','%','s','#','%','p',0};
> - static const WCHAR device_name_fmtW[] = {'\','D','e','v','i','c','e','\','%','s','#','%','p','#','%','x',0};
> struct device_extension *ext;
> struct pnp_device *pnp_dev;
> DEVICE_OBJECT *device;
>
> @@ -169,16 +171,18 @@ DEVICE_OBJECT *bus_create_hid_device(DRIVER_OBJECT *driver, const WCHAR *busidW,
>      WCHAR dev_name[256];
>      HDEVINFO devinfo;
>      NTSTATUS status;
> - DWORD length;
> - TRACE("(%p, %s, %p, %04x, %04x, %u, %u, %s, %u, %s)\n", driver, debugstr_w(busidW), native,
> - TRACE("(%p, %s, %04x, %04x, %u, %u, %s, %u, %s)\n", driver, debugstr_w(busidW),
>       vid, pid, version, uid, debugstr_w(serialW), is_gamepad, debugstr_guid(class));

You forgot to add the vtbl and platform_data_size parameters to the TRACE.
>  if (!(pnp_dev = HeapAlloc(GetProcessHeap(), 0, sizeof(*pnp_dev))))
>      return NULL;
>
> - sprintfW(dev_name, device_name_fmtW, busidW, native);
> - sprintfW(dev_name, device_name_fmtW, busidW, pnp_dev, GetTickCount());

Do you think the memory pointer (pnp_dev) alone is not sufficient? If not, it was already
a problem before because "native" is also just a memory address for UDEV.
>  RtlInitUnicodeString(&nameW, dev_name);
> - status = IoCreateDevice(driver, sizeof(*ext), &nameW, 0, 0, FALSE, &device);
> - length = FIELD_OFFSET(struct device_extension, platform_private[platform_data_size]);
> - status = IoCreateDevice(driver, length, &nameW, 0, 0, FALSE, &device);
> if (status)
> {
>     FIXME("failed to create device error %x\n", status);
>
> @@ -190,7 +194,6 @@ DEVICE_OBJECT *bus_create_hid_device(DRIVER_OBJECT *driver, const WCHAR *busidW,
>  /* fill out device_extension struct */
>  ext = (struct device_extension *)device->DeviceExtension;
> - ext->native     = native;
> ext->vid        = vid;
> ext->pid        = pid;
> ext->uid        = uid;
>
> @@ -199,6 +202,8 @@ DEVICE_OBJECT *bus_create_hid_device(DRIVER_OBJECT *driver, const WCHAR *busidW,
>      ext->is_gamepad = is_gamepad;
>      ext->serial     = strdupW(serialW);
>      ext->busid      = busidW;
> -
>
> ext->pnp_device = pnp_dev;
> -
>
> ext->vtbl       = vtbl;
>
> /* add to list of pnp devices */
> pnp_dev->device = device;
>
> @@ -226,7 +231,6 @@ DEVICE_OBJECT *bus_create_hid_device(DRIVER_OBJECT *driver, const WCHAR *busidW,
>      else
>          ERR("failed to get ClassDevs: %x\n", GetLastError());
> - IoInvalidateDeviceRelations(device, BusRelations);
> return device;
>
> }
>
> @@ -265,6 +269,46 @@ static NTSTATUS handle_IRP_MN_QUERY_ID(DEVICE_OBJECT *device, IRP *irp)
>      return status;
>  }
>
> +void *get_platform_private(DEVICE_OBJECT *device)
> +{
> - struct device_extension *ext = (struct device_extension*)device->DeviceExtension;
> - return ext->platform_private;
>
> +}
> -
>
> +DEVICE_OBJECT *find_hid_device(const platform_vtbl* vtbl, void *device)
> +{
> - struct pnp_device *dev, *ptr;
> - DEVICE_OBJECT *ret = NULL;
> -
> - EnterCriticalSection(&device_list_cs);
> - LIST_FOR_EACH_ENTRY_SAFE(dev, ptr, &pnp_devset, struct pnp_device, entry)

You do no longer need the _SAFE enumeration here because the deletion is done separately.
> - {
> -
>    struct device_extension *ext = (struct device_extension*)dev->device->DeviceExtension;
> -
>    if ((vtbl == ext->vtbl) &&
> -
>        (ext->vtbl->compare_platform_device(dev->device, device) == 0))
> -
>    {
> -
>        TRACE("Found as device %p\n", dev->device);
> -
>        ret = dev->device;
> -
>        break;
> -
>    }
> - }
> - LeaveCriticalSection(&device_list_cs);
> - return ret;
>
> +}
> -
>
> +void bus_remove_hid_device(DEVICE_OBJECT *device)
> +{
> - struct device_extension *ext = (struct device_extension*)device->DeviceExtension;
> - TRACE("Remove hid device %p\n", device);
> - EnterCriticalSection(&device_list_cs);
> - list_remove(&ext->pnp_device->entry);
> - LeaveCriticalSection(&device_list_cs);
> - IoInvalidateDeviceRelations(device, RemovalRelations);
> - HeapFree(GetProcessHeap(), 0, ext->serial);
> - HeapFree(GetProcessHeap(), 0, ext->pnp_device);
> - IoDeleteDevice(device);
>
> +}
> -
>
> NTSTATUS WINAPI common_pnp_dispatch(DEVICE_OBJECT *device, IRP *irp)
>  {
>      NTSTATUS status = irp->IoStatus.u.Status;

[PATCH v6] winebus.sys: Watch for hid raw device addition and removal

[Aric Stewart]

Thanks for the review, just 1 comment

On 9/29/16 2:44 PM, Sebastian Lackner wrote:
> On 29.09.2016 20:48, Aric Stewart wrote:
> > v2: Correct poll timeout
> >     Style changes
> > v3: vtable style instead of a callback
> > v4: Suggestions from Sebastian Lackner
> > v5: Fixing handling of other native in the compare function
> > v6: Tweaks suggested by Sebastian Lackner
> >
> > Includes an implementation of a common bus_remove_hid_device
> > Signed-off-by: Aric Stewart
> >
> > dlls/winebus.sys/bus.h      |  13 +++++-
> >  dlls/winebus.sys/bus_udev.c | 106 +++++++++++++++++++++++++++++++++++++++++++-
> >  dlls/winebus.sys/main.c     |  64 +++++++++++++++++++++-----
> >  3 files changed, 169 insertions(+), 14 deletions(-)
> >
> > v6-0001-winebus.sys-Watch-for-hid-raw-device-addition-and-r.txt
> >
> > diff --git a/dlls/winebus.sys/bus.h b/dlls/winebus.sys/bus.h

.
.
.
> > -DEVICE_OBJECT *bus_create_hid_device(DRIVER_OBJECT *driver, const WCHAR *busidW, void *native, WORD vid,
> > +DEVICE_OBJECT *bus_create_hid_device(DRIVER_OBJECT *driver, const WCHAR *busidW, WORD vid,
> >                                       WORD pid, DWORD version, DWORD uid, const WCHAR *serialW, BOOL is_gamepad,
> > -
> >                                 const GUID *class)
> > -
> >                                 const GUID *class, const platform_vtbl *vtbl, DWORD platform_data_size)
> >
> > {
> > - static const WCHAR device_name_fmtW[] = {'\','D','e','v','i','c','e','\','%','s','#','%','p',0};
> > - static const WCHAR device_name_fmtW[] = {'\','D','e','v','i','c','e','\','%','s','#','%','p','#','%','x',0};
> > struct device_extension *ext;
> > struct pnp_device *pnp_dev;
> > DEVICE_OBJECT *device;
> >
> > @@ -169,16 +171,18 @@ DEVICE_OBJECT *bus_create_hid_device(DRIVER_OBJECT *driver, const WCHAR *busidW,
> >      WCHAR dev_name[256];
> >      HDEVINFO devinfo;
> >      NTSTATUS status;
> > - DWORD length;
> > - TRACE("(%p, %s, %p, %04x, %04x, %u, %u, %s, %u, %s)\n", driver, debugstr_w(busidW), native,
> > - TRACE("(%p, %s, %04x, %04x, %u, %u, %s, %u, %s)\n", driver, debugstr_w(busidW),
> >       vid, pid, version, uid, debugstr_w(serialW), is_gamepad, debugstr_guid(class));
>
> You forgot to add the vtbl and platform_data_size parameters to the TRACE.
> >  if (!(pnp_dev = HeapAlloc(GetProcessHeap(), 0, sizeof(*pnp_dev))))
> >      return NULL;
> >
> > - sprintfW(dev_name, device_name_fmtW, busidW, native);
> > - sprintfW(dev_name, device_name_fmtW, busidW, pnp_dev, GetTickCount());
>
> Do you think the memory pointer (pnp_dev) alone is not sufficient? If not, it was already
> a problem before because "native" is also just a memory address for UDEV.

This was mostly because I know that when a memory address is freed wine can give out the same address quite easily. I was unsure how robust our device deletion creation process was for identically named devices so I wanted to add just that one more element of uniqueness so as to avoid this possible problem.

I am sure just using native would exhibit the same potential issues.

-aric
> >  RtlInitUnicodeString(&nameW, dev_name);
> > - status = IoCreateDevice(driver, sizeof(*ext), &nameW, 0, 0, FALSE, &device);
> > - length = FIELD_OFFSET(struct device_extension, platform_private[platform_data_size]);
> > - status = IoCreateDevice(driver, length, &nameW, 0, 0, FALSE, &device);
> > if (status)
> > {
> >     FIXME("failed to create device error %x\n", status);
> >
> > @@ -190,7 +194,6 @@ DEVICE_OBJECT *bus_create_hid_device(DRIVER_OBJECT *driver, const WCHAR *busidW,
> >  /* fill out device_extension struct */
> >  ext = (struct device_extension *)device->DeviceExtension;
> > - ext->native     = native;
> > ext->vid        = vid;
> > ext->pid        = pid;
> > ext->uid        = uid;
> >
> > @@ -199,6 +202,8 @@ DEVICE_OBJECT *bus_create_hid_device(DRIVER_OBJECT *driver, const WCHAR *busidW,
> >      ext->is_gamepad = is_gamepad;
> >      ext->serial     = strdupW(serialW);
> >      ext->busid      = busidW;
> > -
> >
> > ext->pnp_device = pnp_dev;
> > -
> >
> > ext->vtbl       = vtbl;
> >
> > /* add to list of pnp devices */
> > pnp_dev->device = device;
> >
> > @@ -226,7 +231,6 @@ DEVICE_OBJECT *bus_create_hid_device(DRIVER_OBJECT *driver, const WCHAR *busidW,
> >      else
> >          ERR("failed to get ClassDevs: %x\n", GetLastError());
> > - IoInvalidateDeviceRelations(device, BusRelations);
> > return device;
> >
> > }
> >
> > @@ -265,6 +269,46 @@ static NTSTATUS handle_IRP_MN_QUERY_ID(DEVICE_OBJECT *device, IRP *irp)
> >      return status;
> >  }
> >
> > +void *get_platform_private(DEVICE_OBJECT *device)
> > +{
> > - struct device_extension *ext = (struct device_extension*)device->DeviceExtension;
> > - return ext->platform_private;
> >
> > +}
> > -
> >
> > +DEVICE_OBJECT *find_hid_device(const platform_vtbl* vtbl, void *device)
> > +{
> > - struct pnp_device *dev, *ptr;
> > - DEVICE_OBJECT *ret = NULL;
> > -
> > - EnterCriticalSection(&device_list_cs);
> > - LIST_FOR_EACH_ENTRY_SAFE(dev, ptr, &pnp_devset, struct pnp_device, entry)
>
> You do no longer need the _SAFE enumeration here because the deletion is done separately.
> > - {
> > -
> >    struct device_extension *ext = (struct device_extension*)dev->device->DeviceExtension;
> > -
> >    if ((vtbl == ext->vtbl) &&
> > -
> >        (ext->vtbl->compare_platform_device(dev->device, device) == 0))
> > -
> >    {
> > -
> >        TRACE("Found as device %p\n", dev->device);
> > -
> >        ret = dev->device;
> > -
> >        break;
> > -
> >    }
> > - }
> > - LeaveCriticalSection(&device_list_cs);
> > - return ret;
> >
> > +}
> > -
> >
> > +void bus_remove_hid_device(DEVICE_OBJECT *device)
> > +{
> > - struct device_extension *ext = (struct device_extension*)device->DeviceExtension;
> > - TRACE("Remove hid device %p\n", device);
> > - EnterCriticalSection(&device_list_cs);
> > - list_remove(&ext->pnp_device->entry);
> > - LeaveCriticalSection(&device_list_cs);
> > - IoInvalidateDeviceRelations(device, RemovalRelations);
> > - HeapFree(GetProcessHeap(), 0, ext->serial);
> > - HeapFree(GetProcessHeap(), 0, ext->pnp_device);
> > - IoDeleteDevice(device);
> >
> > +}
> > -
> >
> > NTSTATUS WINAPI common_pnp_dispatch(DEVICE_OBJECT *device, IRP *irp)
> >  {
> >      NTSTATUS status = irp->IoStatus.u.Status;

[PATCH v6] winebus.sys: Watch for hid raw device addition and removal

[Alexandre Julliard]

Sebastian Lackner  writes:
> > @@ -178,6 +273,13 @@ NTSTATUS WINAPI udev_driver_init(DRIVER_OBJECT *driver, UNICODE_STRING *registry
> >      driver->MajorFunction[IRP_MJ_PNP] = common_pnp_dispatch;
> >  build_initial_deviceset();
> > -
> > - if (!CreateThread(NULL, 0, deviceloop_thread, NULL, 0, NULL))
> > - {
> > -
> >    ERR("Unable to create udev device thread\n");
> > -
> >    return STATUS_UNSUCCESSFUL;
>
> Wouldn't it make sense to proceed anyway? The initial device set could still
> be working properly (and, you are not cleaning up already added drivers here).

Actually, you probably want to load the initial device set from the
thread, since it has to be done after setting up the monitoring.