Re: [PATCH 1/5] winebus.sys: Handle linux input event device via udev

[Sebastian Lackner]

On 27.02.2017 16:09, Aric Stewart wrote:
> Signed-off-by: Aric Stewart <aric(a)codeweavers.com>
> ---
>  dlls/winebus.sys/bus.h      |   3 +
>  dlls/winebus.sys/bus_udev.c | 150 ++++++++++++++++++++++++++++++++++++++++++--
>  dlls/winebus.sys/main.c     |  26 ++++++++
>  3 files changed, 173 insertions(+), 6 deletions(-)

I assume this is meant as a fallback, if we don't have access to hidraw, right?
If you plan to use this as a replacement for dinput, it would be better to
implement it without fixed dependency on udev.
> 0001-winebus.sys-Handle-linux-input-event-device-via-udev.txt
>
> diff --git a/dlls/winebus.sys/bus.h b/dlls/winebus.sys/bus.h
> index effbd4afd0..fed35e8c1e 100644
> --- a/dlls/winebus.sys/bus.h
> +++ b/dlls/winebus.sys/bus.h
> @@ -43,3 +43,6 @@ DEVICE_OBJECT *bus_find_hid_device(const platform_vtbl *vtbl, void *platform_dev
>  void bus_remove_hid_device(DEVICE_OBJECT *device) DECLSPEC_HIDDEN;
>  NTSTATUS WINAPI hid_internal_dispatch(DEVICE_OBJECT *device, IRP *irp) DECLSPEC_HIDDEN;
>  void process_hid_report(DEVICE_OBJECT *device, BYTE *report, DWORD length) DECLSPEC_HIDDEN;
> +
> +/* General Bus Functions */
> +DWORD check_bus_option(UNICODE_STRING *registry_path, const UNICODE_STRING *option) DECLSPEC_HIDDEN;
> diff --git a/dlls/winebus.sys/bus_udev.c b/dlls/winebus.sys/bus_udev.c
> index 415bd3db08..2dcc7fb442 100644
> --- a/dlls/winebus.sys/bus_udev.c
> +++ b/dlls/winebus.sys/bus_udev.c
> @@ -42,6 +42,14 @@
>  # include <sys/ioctl.h>
>  #endif
>
> +#ifdef HAVE_LINUX_INPUT_H
> +# include <linux/input.h>
> +# undef SW_MAX
> +# if defined(EVIOCGBIT) && defined(EV_ABS) && defined(BTN_PINKIE)
> +#  define HAS_PROPER_INPUT_HEADER
> +# endif
> +#endif
> +
>  #define NONAMELESSUNION
>
> #include "ntstatus.h"
> @@ -65,11 +73,15 @@ WINE_DECLARE_DEBUG_CHANNEL(hid_report);
>
> static struct udev *udev_context = NULL;
>  static DRIVER_OBJECT *udev_driver_obj = NULL;
> +static DWORD disable_hidraw = 0;
> +static DWORD disable_input = 0;
>
> static const WCHAR hidraw_busidW[] = {'H','I','D','R','A','W',0};
> +static const WCHAR lnxev_busidW[] = {'L','N','X','E','V',0};
>
> #include "initguid.h"
>  DEFINE_GUID(GUID_DEVCLASS_HIDRAW, 0x3def44ad,0x242e,0x46e5,0x82,0x6d,0x70,0x72,0x13,0xf3,0xaa,0x81);
> +DEFINE_GUID(GUID_DEVCLASS_LINUXEVENT, 0x1b932c0d,0xfea7,0x42cd,0x8e,0xaa,0x0e,0x48,0x79,0xb6,0x9e,0xaa);
>
> struct platform_private
>  {
> @@ -392,6 +404,52 @@ static const platform_vtbl hidraw_vtbl =
>      hidraw_set_feature_report,
>  };
>
> +#ifdef HAS_PROPER_INPUT_HEADER
> +
> +static NTSTATUS lnxev_get_reportdescriptor(DEVICE_OBJECT *device, BYTE *buffer, DWORD length, DWORD *out_length)
> +{
> +    return STATUS_NOT_IMPLEMENTED;
> +}
> +
> +static NTSTATUS lnxev_get_string(DEVICE_OBJECT *device, DWORD index, WCHAR *buffer, DWORD length)
> +{
> +    return STATUS_NOT_IMPLEMENTED;
> +}
> +
> +static NTSTATUS lnxev_begin_report_processing(DEVICE_OBJECT *device)
> +{
> +    return STATUS_NOT_IMPLEMENTED;
> +}
> +
> +static NTSTATUS lnxev_set_output_report(DEVICE_OBJECT *device, UCHAR id, BYTE *report, DWORD length, ULONG_PTR *written)
> +{
> +    *written = 0;
> +    return STATUS_NOT_IMPLEMENTED;
> +}
> +
> +static NTSTATUS lnxev_get_feature_report(DEVICE_OBJECT *device, UCHAR id, BYTE *report, DWORD length, ULONG_PTR *read)
> +{
> +    *read = 0;
> +    return STATUS_NOT_IMPLEMENTED;
> +}
> +
> +static NTSTATUS lnxev_set_feature_report(DEVICE_OBJECT *device, UCHAR id, BYTE *report, DWORD length, ULONG_PTR *written)
> +{
> +    *written = 0;
> +    return STATUS_NOT_IMPLEMENTED;
> +}
> +
> +static const platform_vtbl lnxev_vtbl = {
> +    compare_platform_device,
> +    lnxev_get_reportdescriptor,
> +    lnxev_get_string,
> +    lnxev_begin_report_processing,
> +    lnxev_set_output_report,
> +    lnxev_get_feature_report,
> +    lnxev_set_feature_report,
> +};
> +#endif
> +
>  static void try_add_device(struct udev_device *dev)
>  {
>      DWORD vid = 0, pid = 0, version = 0;
> @@ -400,6 +458,7 @@ static void try_add_device(struct udev_device *dev)
>      const char *subsystem;
>      const char *devnode;
>      WCHAR *serial = NULL;
> +    const char* gamepad = NULL;
>      int fd;
>
> if (!(devnode = udev_device_get_devnode(dev)))
> @@ -419,6 +478,26 @@ static void try_add_device(struct udev_device *dev)
>          version = get_sysattr_dword(usbdev, "version", 10);
>          serial  = get_sysattr_string(usbdev, "serial");
>      }
> +#ifdef HAS_PROPER_INPUT_HEADER
> +    else
> +    {
> +        struct input_id device_id = {0};
> +        char device_uid[255] = {0};
> +
> +        if (ioctl(fd, EVIOCGID, &device_id) == -1)
> +            WARN("ioctl(EVIOCGID) failed: %d %s\n", errno, strerror(errno));
> +        if (ioctl(fd, EVIOCGUNIQ(254), device_uid) && device_uid[0])
> +            MultiByteToWideChar(CP_UNIXCP, 0, device_uid, -1, serial, 255);

I don't think this will work as expected. serial is a NULL pointer, and the check
if ioctl() was successful is also done wrong. On error a value < 0 is returned.
> +
> +        gamepad = udev_device_get_property_value(dev, "ID_INPUT_JOYSTICK");
> +        vid = device_id.vendor;
> +        pid = device_id.product;
> +        version = device_id.version;
> +    }
> +#else
> +    else
> +        WARN("Could not get device to query VID, PID, Version and Serial\n");
> +#endif
>
> TRACE("Found udev device %s (vid %04x, pid %04x, version %u, serial %s)\n",
>            debugstr_a(devnode), vid, pid, version, debugstr_w(serial));
> @@ -429,6 +508,12 @@ static void try_add_device(struct udev_device *dev)
>          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));
>      }
> +#ifdef HAS_PROPER_INPUT_HEADER
> +    else if (strcmp(subsystem, "input") == 0)
> +    {
> +        device = bus_create_hid_device(udev_driver_obj, lnxev_busidW, vid, pid, version, 0, serial, (gamepad != NULL), &GUID_DEVCLASS_LINUXEVENT, &lnxev_vtbl, sizeof(struct platform_private));

Some linebreaks wouldn't hurt.
> +    }
> +#endif
>
> if (device)
>      {
> @@ -448,8 +533,17 @@ static void try_add_device(struct udev_device *dev)
>
> static void try_remove_device(struct udev_device *dev)
>  {
> -    DEVICE_OBJECT *device = bus_find_hid_device(&hidraw_vtbl, dev);
> -    struct platform_private *private;
> +    DEVICE_OBJECT *device = NULL;
> +    const char* subsystem;
> +    struct platform_private* private;
> +
> +    subsystem = udev_device_get_subsystem(dev);
> +    if (strcmp(subsystem, "hidraw") == 0)
> +        device = bus_find_hid_device(&hidraw_vtbl, dev);
> +#ifdef HAS_PROPER_INPUT_HEADER
> +    else if (strcmp(subsystem, "input") == 0)
> +        device = bus_find_hid_device(&lnxev_vtbl, dev);
> +#endif

It might be easier to just check both after each other, like:

device = bus_find_hid_device(...);
if (!device) device = bus_find_hid_device(...);
> if (!device) return;
>
> IoInvalidateDeviceRelations(device, RemovalRelations);
> @@ -483,8 +577,18 @@ static void build_initial_deviceset(void)
>          return;
>      }
>
> -    if (udev_enumerate_add_match_subsystem(enumerate, "hidraw") < 0)
> -        WARN("Failed to add subsystem 'hidraw' to enumeration\n");
> +    if (!disable_hidraw)
> +        if (udev_enumerate_add_match_subsystem(enumerate, "hidraw") < 0)
> +            WARN("Failed to add subsystem 'hidraw' to enumeration\n");
> +#ifdef HAS_PROPER_INPUT_HEADER
> +    if (!disable_input)
> +    {
> +        if (udev_enumerate_add_match_subsystem(enumerate, "event") < 0)
> +            WARN("Failed to add subsystem 'event' to enumeration\n");

Why do you add this match when results are not processed anywhere?
> +        if (udev_enumerate_add_match_subsystem(enumerate, "input") < 0)
> +            WARN("Failed to add subsystem 'input' to enumeration\n");
> +    }
> +#endif
>
> if (udev_enumerate_scan_devices(enumerate) < 0)
>          WARN("Enumeration scan failed\n");
> @@ -509,6 +613,7 @@ static void build_initial_deviceset(void)
>  static struct udev_monitor *create_monitor(struct pollfd *pfd)
>  {
>      struct udev_monitor *monitor;
> +    int systems = 0;
>
> monitor = udev_monitor_new_from_netlink(udev_context, "udev");
>      if (!monitor)
> @@ -517,8 +622,27 @@ static struct udev_monitor *create_monitor(struct pollfd *pfd)
>          return NULL;
>      }
>
> -    if (udev_monitor_filter_add_match_subsystem_devtype(monitor, "hidraw", NULL) < 0)
> -        WARN("Failed to add subsystem 'hidraw' to monitor\n");
> +    if (!disable_hidraw)
> +    {
> +        if (udev_monitor_filter_add_match_subsystem_devtype(monitor, "hidraw", NULL) < 0)
> +            WARN("Failed to add 'hidraw' subsystem to monitor\n");
> +        else
> +            systems++;
> +    }
> +#ifdef HAS_PROPER_INPUT_HEADER
> +    if (!disable_input)
> +    {
> +        if (udev_monitor_filter_add_match_subsystem_devtype(monitor, "input", NULL) < 0)
> +            WARN("Failed to add 'input' subsystem to monitor\n");
> +        else
> +            systems++;
> +    }
> +#endif
> +    if (systems == 0)
> +    {
> +        WARN("No subsystems added to monitor\n");
> +        goto error;
> +    }
>
> if (udev_monitor_enable_receiving(monitor) < 0)
>          goto error;
> @@ -589,6 +713,10 @@ NTSTATUS WINAPI udev_driver_init(DRIVER_OBJECT *driver, UNICODE_STRING *registry
>  {
>      HANDLE events[2];
>      DWORD result;
> +    static const WCHAR hidraw_disabledW[] = {'D','i','s','a','b','l','e',' ','h','i','d','r','a','w',0};
> +    static const UNICODE_STRING hidraw_disabled = {sizeof(hidraw_disabledW) - sizeof(WCHAR), sizeof(hidraw_disabledW), (WCHAR*)hidraw_disabledW};
> +    static const WCHAR input_disabledW[] = {'D','i','s','a','b','l','e',' ','i','n','p','u','t',0};
> +    static const UNICODE_STRING input_disabled = {sizeof(input_disabledW) - sizeof(WCHAR), sizeof(input_disabledW), (WCHAR*)input_disabledW};
>
> TRACE("(%p, %s)\n", driver, debugstr_w(registry_path->Buffer));
>
> @@ -602,6 +730,16 @@ NTSTATUS WINAPI udev_driver_init(DRIVER_OBJECT *driver, UNICODE_STRING *registry
>      driver->MajorFunction[IRP_MJ_PNP] = common_pnp_dispatch;
>      driver->MajorFunction[IRP_MJ_INTERNAL_DEVICE_CONTROL] = hid_internal_dispatch;
>
> +    disable_hidraw = check_bus_option(registry_path, &hidraw_disabled);
> +    if (disable_hidraw)
> +        TRACE("UDEV hidraw devices disabled in registry\n");
> +
> +#ifdef HAS_PROPER_INPUT_HEADER
> +    disable_input = check_bus_option(registry_path, &input_disabled);
> +    if (disable_input)
> +        TRACE("UDEV input devices disabled in registry\n");
> +#endif

Does it hurt when both hidraw and input event devices are found? If yes, it
would be better to filter duplicates somewhere else, instead of using registry
keys.
> +
>      if (!(events[0] = CreateEventW(NULL, TRUE, FALSE, NULL)))
>          goto error;
>      if (!(events[1] = CreateThread(NULL, 0, deviceloop_thread, events[0], 0, NULL)))
> diff --git a/dlls/winebus.sys/main.c b/dlls/winebus.sys/main.c
> index 978540bb17..0388f0263c 100644
> --- a/dlls/winebus.sys/main.c
> +++ b/dlls/winebus.sys/main.c
> @@ -625,6 +625,32 @@ void process_hid_report(DEVICE_OBJECT *device, BYTE *report, DWORD length)
>      LeaveCriticalSection(&ext->report_cs);
>  }
>
> +DWORD check_bus_option(UNICODE_STRING *registry_path, const UNICODE_STRING *option)
> +{
> +    OBJECT_ATTRIBUTES attr;
> +    HANDLE key;
> +    DWORD output = 0;
> +
> +    InitializeObjectAttributes(&attr, registry_path, OBJ_CASE_INSENSITIVE | OBJ_KERNEL_HANDLE, NULL, NULL);
> +    if (NtOpenKey(&key, KEY_ALL_ACCESS, &attr) == STATUS_SUCCESS)
> +    {
> +        DWORD size;
> +        char buffer[FIELD_OFFSET(KEY_VALUE_PARTIAL_INFORMATION, Data[sizeof(DWORD)])];
> +
> +        KEY_VALUE_PARTIAL_INFORMATION *info = (KEY_VALUE_PARTIAL_INFORMATION*)buffer;
> +
> +        if (NtQueryValueKey(key, option, KeyValuePartialInformation, info, sizeof(buffer), &size) == STATUS_SUCCESS)
> +        {
> +            if (info->Type == REG_DWORD)
> +                output = *(DWORD*)info->Data;
> +        }
> +
> +        NtClose(key);
> +    }
> +
> +    return output;
> +}
> +
>  NTSTATUS WINAPI DriverEntry( DRIVER_OBJECT *driver, UNICODE_STRING *path )
>  {
>      static const WCHAR udevW[] = {'\\','D','r','i','v','e','r','\\','U','D','E','V',0};

[PATCH 1/5] winebus.sys: Handle linux input event device via udev

[Aric Stewart]

Thanks for the review!


On 2/27/17 10:00 AM, Sebastian Lackner wrote:
> On 27.02.2017 16:09, Aric Stewart wrote:
> > Signed-off-by: Aric Stewart <aric(a)codeweavers.com>
> > ---
> >  dlls/winebus.sys/bus.h      |   3 +
> >  dlls/winebus.sys/bus_udev.c | 150 ++++++++++++++++++++++++++++++++++++++++++--
> >  dlls/winebus.sys/main.c     |  26 ++++++++
> >  3 files changed, 173 insertions(+), 6 deletions(-)
>
> I assume this is meant as a fallback, if we don't have access to hidraw, right?
> If you plan to use this as a replacement for dinput, it would be better to
> implement it without fixed dependency on udev.

Likely users using HID for gamepads and joysticks will end up this route by default because all the permissions are automatically setup for us. Also then user will get to leverage all the work done in the kernel to handle devices that do not behave well on the HID side or which do not show up at all using hidraw (Xbox Controllers for example)

The dependency for udev is for common device discovery. I feel like I still want that dependency unless we write our own device plug and unplug discovery system.
> > 0001-winebus.sys-Handle-linux-input-event-device-via-udev.txt
> >
> > diff --git a/dlls/winebus.sys/bus.h b/dlls/winebus.sys/bus.h
> > index effbd4afd0..fed35e8c1e 100644
> > --- a/dlls/winebus.sys/bus.h
> > +++ b/dlls/winebus.sys/bus.h
> > @@ -43,3 +43,6 @@ DEVICE_OBJECT *bus_find_hid_device(const platform_vtbl *vtbl, void *platform_dev
> >  void bus_remove_hid_device(DEVICE_OBJECT *device) DECLSPEC_HIDDEN;
> >  NTSTATUS WINAPI hid_internal_dispatch(DEVICE_OBJECT *device, IRP *irp) DECLSPEC_HIDDEN;
> >  void process_hid_report(DEVICE_OBJECT *device, BYTE *report, DWORD length) DECLSPEC_HIDDEN;
> > +
> > +/* General Bus Functions */
> > +DWORD check_bus_option(UNICODE_STRING *registry_path, const UNICODE_STRING *option) DECLSPEC_HIDDEN;
> > diff --git a/dlls/winebus.sys/bus_udev.c b/dlls/winebus.sys/bus_udev.c
> > index 415bd3db08..2dcc7fb442 100644
> > --- a/dlls/winebus.sys/bus_udev.c
> > +++ b/dlls/winebus.sys/bus_udev.c
> > @@ -42,6 +42,14 @@
> >  # include <sys/ioctl.h>
> >  #endif
> >
> > +#ifdef HAVE_LINUX_INPUT_H
> > +# include <linux/input.h>
> > +# undef SW_MAX
> > +# if defined(EVIOCGBIT) && defined(EV_ABS) && defined(BTN_PINKIE)
> > +#  define HAS_PROPER_INPUT_HEADER
> > +# endif
> > +#endif
> > +
> >  #define NONAMELESSUNION
> >
> > #include "ntstatus.h"
> > @@ -65,11 +73,15 @@ WINE_DECLARE_DEBUG_CHANNEL(hid_report);
> >
> > static struct udev *udev_context = NULL;
> >  static DRIVER_OBJECT *udev_driver_obj = NULL;
> > +static DWORD disable_hidraw = 0;
> > +static DWORD disable_input = 0;
> >
> > static const WCHAR hidraw_busidW[] = {'H','I','D','R','A','W',0};
> > +static const WCHAR lnxev_busidW[] = {'L','N','X','E','V',0};
> >
> > #include "initguid.h"
> >  DEFINE_GUID(GUID_DEVCLASS_HIDRAW, 0x3def44ad,0x242e,0x46e5,0x82,0x6d,0x70,0x72,0x13,0xf3,0xaa,0x81);
> > +DEFINE_GUID(GUID_DEVCLASS_LINUXEVENT, 0x1b932c0d,0xfea7,0x42cd,0x8e,0xaa,0x0e,0x48,0x79,0xb6,0x9e,0xaa);
> >
> > struct platform_private
> >  {
> > @@ -392,6 +404,52 @@ static const platform_vtbl hidraw_vtbl =
> >      hidraw_set_feature_report,
> >  };
> >
> > +#ifdef HAS_PROPER_INPUT_HEADER
> > +
> > +static NTSTATUS lnxev_get_reportdescriptor(DEVICE_OBJECT *device, BYTE *buffer, DWORD length, DWORD *out_length)
> > +{
> > +    return STATUS_NOT_IMPLEMENTED;
> > +}
> > +
> > +static NTSTATUS lnxev_get_string(DEVICE_OBJECT *device, DWORD index, WCHAR *buffer, DWORD length)
> > +{
> > +    return STATUS_NOT_IMPLEMENTED;
> > +}
> > +
> > +static NTSTATUS lnxev_begin_report_processing(DEVICE_OBJECT *device)
> > +{
> > +    return STATUS_NOT_IMPLEMENTED;
> > +}
> > +
> > +static NTSTATUS lnxev_set_output_report(DEVICE_OBJECT *device, UCHAR id, BYTE *report, DWORD length, ULONG_PTR *written)
> > +{
> > +    *written = 0;
> > +    return STATUS_NOT_IMPLEMENTED;
> > +}
> > +
> > +static NTSTATUS lnxev_get_feature_report(DEVICE_OBJECT *device, UCHAR id, BYTE *report, DWORD length, ULONG_PTR *read)
> > +{
> > +    *read = 0;
> > +    return STATUS_NOT_IMPLEMENTED;
> > +}
> > +
> > +static NTSTATUS lnxev_set_feature_report(DEVICE_OBJECT *device, UCHAR id, BYTE *report, DWORD length, ULONG_PTR *written)
> > +{
> > +    *written = 0;
> > +    return STATUS_NOT_IMPLEMENTED;
> > +}
> > +
> > +static const platform_vtbl lnxev_vtbl = {
> > +    compare_platform_device,
> > +    lnxev_get_reportdescriptor,
> > +    lnxev_get_string,
> > +    lnxev_begin_report_processing,
> > +    lnxev_set_output_report,
> > +    lnxev_get_feature_report,
> > +    lnxev_set_feature_report,
> > +};
> > +#endif
> > +
> >  static void try_add_device(struct udev_device *dev)
> >  {
> >      DWORD vid = 0, pid = 0, version = 0;
> > @@ -400,6 +458,7 @@ static void try_add_device(struct udev_device *dev)
> >      const char *subsystem;
> >      const char *devnode;
> >      WCHAR *serial = NULL;
> > +    const char* gamepad = NULL;
> >      int fd;
> >
> > if (!(devnode = udev_device_get_devnode(dev)))
> > @@ -419,6 +478,26 @@ static void try_add_device(struct udev_device *dev)
> >          version = get_sysattr_dword(usbdev, "version", 10);
> >          serial  = get_sysattr_string(usbdev, "serial");
> >      }
> > +#ifdef HAS_PROPER_INPUT_HEADER
> > +    else
> > +    {
> > +        struct input_id device_id = {0};
> > +        char device_uid[255] = {0};
> > +
> > +        if (ioctl(fd, EVIOCGID, &device_id) == -1)
> > +            WARN("ioctl(EVIOCGID) failed: %d %s\n", errno, strerror(errno));
> > +        if (ioctl(fd, EVIOCGUNIQ(254), device_uid) && device_uid[0])
> > +            MultiByteToWideChar(CP_UNIXCP, 0, device_uid, -1, serial, 255);
>
> I don't think this will work as expected. serial is a NULL pointer, and the check
> if ioctl() was successful is also done wrong. On error a value < 0 is returned.

Thanks I will look at that.
> > +
> > +        gamepad = udev_device_get_property_value(dev, "ID_INPUT_JOYSTICK");
> > +        vid = device_id.vendor;
> > +        pid = device_id.product;
> > +        version = device_id.version;
> > +    }
> > +#else
> > +    else
> > +        WARN("Could not get device to query VID, PID, Version and Serial\n");
> > +#endif
> >
> > TRACE("Found udev device %s (vid %04x, pid %04x, version %u, serial %s)\n",
> >            debugstr_a(devnode), vid, pid, version, debugstr_w(serial));
> > @@ -429,6 +508,12 @@ static void try_add_device(struct udev_device *dev)
> >          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));
> >      }
> > +#ifdef HAS_PROPER_INPUT_HEADER
> > +    else if (strcmp(subsystem, "input") == 0)
> > +    {
> > +        device = bus_create_hid_device(udev_driver_obj, lnxev_busidW, vid, pid, version, 0, serial, (gamepad != NULL), &GUID_DEVCLASS_LINUXEVENT, &lnxev_vtbl, sizeof(struct platform_private));
>
> Some linebreaks wouldn't hurt.

:)
> > +    }
> > +#endif
> >
> > if (device)
> >      {
> > @@ -448,8 +533,17 @@ static void try_add_device(struct udev_device *dev)
> >
> > static void try_remove_device(struct udev_device *dev)
> >  {
> > -    DEVICE_OBJECT *device = bus_find_hid_device(&hidraw_vtbl, dev);
> > -    struct platform_private *private;
> > +    DEVICE_OBJECT *device = NULL;
> > +    const char* subsystem;
> > +    struct platform_private* private;
> > +
> > +    subsystem = udev_device_get_subsystem(dev);
> > +    if (strcmp(subsystem, "hidraw") == 0)
> > +        device = bus_find_hid_device(&hidraw_vtbl, dev);
> > +#ifdef HAS_PROPER_INPUT_HEADER
> > +    else if (strcmp(subsystem, "input") == 0)
> > +        device = bus_find_hid_device(&lnxev_vtbl, dev);
> > +#endif
>
> It might be easier to just check both after each other, like:
>
> device = bus_find_hid_device(...);
> if (!device) device = bus_find_hid_device(...);

either way, seems mostly stylistic
> > if (!device) return;
> >
> > IoInvalidateDeviceRelations(device, RemovalRelations);
> > @@ -483,8 +577,18 @@ static void build_initial_deviceset(void)
> >          return;
> >      }
> >
> > -    if (udev_enumerate_add_match_subsystem(enumerate, "hidraw") < 0)
> > -        WARN("Failed to add subsystem 'hidraw' to enumeration\n");
> > +    if (!disable_hidraw)
> > +        if (udev_enumerate_add_match_subsystem(enumerate, "hidraw") < 0)
> > +            WARN("Failed to add subsystem 'hidraw' to enumeration\n");
> > +#ifdef HAS_PROPER_INPUT_HEADER
> > +    if (!disable_input)
> > +    {
> > +        if (udev_enumerate_add_match_subsystem(enumerate, "event") < 0)
> > +            WARN("Failed to add subsystem 'event' to enumeration\n");
>
> Why do you add this match when results are not processed anywhere?

legacy code. I will remove it.
> > +        if (udev_enumerate_add_match_subsystem(enumerate, "input") < 0)
> > +            WARN("Failed to add subsystem 'input' to enumeration\n");
> > +    }
> > +#endif
> >
> > if (udev_enumerate_scan_devices(enumerate) < 0)
> >          WARN("Enumeration scan failed\n");
> > @@ -509,6 +613,7 @@ static void build_initial_deviceset(void)
> >  static struct udev_monitor *create_monitor(struct pollfd *pfd)
> >  {
> >      struct udev_monitor *monitor;
> > +    int systems = 0;
> >
> > monitor = udev_monitor_new_from_netlink(udev_context, "udev");
> >      if (!monitor)
> > @@ -517,8 +622,27 @@ static struct udev_monitor *create_monitor(struct pollfd *pfd)
> >          return NULL;
> >      }
> >
> > -    if (udev_monitor_filter_add_match_subsystem_devtype(monitor, "hidraw", NULL) < 0)
> > -        WARN("Failed to add subsystem 'hidraw' to monitor\n");
> > +    if (!disable_hidraw)
> > +    {
> > +        if (udev_monitor_filter_add_match_subsystem_devtype(monitor, "hidraw", NULL) < 0)
> > +            WARN("Failed to add 'hidraw' subsystem to monitor\n");
> > +        else
> > +            systems++;
> > +    }
> > +#ifdef HAS_PROPER_INPUT_HEADER
> > +    if (!disable_input)
> > +    {
> > +        if (udev_monitor_filter_add_match_subsystem_devtype(monitor, "input", NULL) < 0)
> > +            WARN("Failed to add 'input' subsystem to monitor\n");
> > +        else
> > +            systems++;
> > +    }
> > +#endif
> > +    if (systems == 0)
> > +    {
> > +        WARN("No subsystems added to monitor\n");
> > +        goto error;
> > +    }
> >
> > if (udev_monitor_enable_receiving(monitor) < 0)
> >          goto error;
> > @@ -589,6 +713,10 @@ NTSTATUS WINAPI udev_driver_init(DRIVER_OBJECT *driver, UNICODE_STRING *registry
> >  {
> >      HANDLE events[2];
> >      DWORD result;
> > +    static const WCHAR hidraw_disabledW[] = {'D','i','s','a','b','l','e',' ','h','i','d','r','a','w',0};
> > +    static const UNICODE_STRING hidraw_disabled = {sizeof(hidraw_disabledW) - sizeof(WCHAR), sizeof(hidraw_disabledW), (WCHAR*)hidraw_disabledW};
> > +    static const WCHAR input_disabledW[] = {'D','i','s','a','b','l','e',' ','i','n','p','u','t',0};
> > +    static const UNICODE_STRING input_disabled = {sizeof(input_disabledW) - sizeof(WCHAR), sizeof(input_disabledW), (WCHAR*)input_disabledW};
> >
> > TRACE("(%p, %s)\n", driver, debugstr_w(registry_path->Buffer));
> >
> > @@ -602,6 +730,16 @@ NTSTATUS WINAPI udev_driver_init(DRIVER_OBJECT *driver, UNICODE_STRING *registry
> >      driver->MajorFunction[IRP_MJ_PNP] = common_pnp_dispatch;
> >      driver->MajorFunction[IRP_MJ_INTERNAL_DEVICE_CONTROL] = hid_internal_dispatch;
> >
> > +    disable_hidraw = check_bus_option(registry_path, &hidraw_disabled);
> > +    if (disable_hidraw)
> > +        TRACE("UDEV hidraw devices disabled in registry\n");
> > +
> > +#ifdef HAS_PROPER_INPUT_HEADER
> > +    disable_input = check_bus_option(registry_path, &input_disabled);
> > +    if (disable_input)
> > +        TRACE("UDEV input devices disabled in registry\n");
> > +#endif
>
> Does it hurt when both hidraw and input event devices are found? If yes, it
> would be better to filter duplicates somewhere else, instead of using registry
> keys.

You can get duplicate devices, on being talking to the hidraw device and one to the linux event device, but both are the same physical device. I have explored a little bit to try to find if there is a good way to detect if a device is duplicated but it has not proven to be immediately easy. Also then the question because which device is preferred if we find a duplicate.


thanks again for the review, I am seeing that I have been testing with either or and not both (hidraw/event) so you are finding very valid issues in the code I need to resolve.

I was starting to go cross-eyed looking at it all.

-aric
> > +
> >      if (!(events[0] = CreateEventW(NULL, TRUE, FALSE, NULL)))
> >          goto error;
> >      if (!(events[1] = CreateThread(NULL, 0, deviceloop_thread, events[0], 0, NULL)))
> > diff --git a/dlls/winebus.sys/main.c b/dlls/winebus.sys/main.c
> > index 978540bb17..0388f0263c 100644
> > --- a/dlls/winebus.sys/main.c
> > +++ b/dlls/winebus.sys/main.c
> > @@ -625,6 +625,32 @@ void process_hid_report(DEVICE_OBJECT *device, BYTE *report, DWORD length)
> >      LeaveCriticalSection(&ext->report_cs);
> >  }
> >
> > +DWORD check_bus_option(UNICODE_STRING *registry_path, const UNICODE_STRING *option)
> > +{
> > +    OBJECT_ATTRIBUTES attr;
> > +    HANDLE key;
> > +    DWORD output = 0;
> > +
> > +    InitializeObjectAttributes(&attr, registry_path, OBJ_CASE_INSENSITIVE | OBJ_KERNEL_HANDLE, NULL, NULL);
> > +    if (NtOpenKey(&key, KEY_ALL_ACCESS, &attr) == STATUS_SUCCESS)
> > +    {
> > +        DWORD size;
> > +        char buffer[FIELD_OFFSET(KEY_VALUE_PARTIAL_INFORMATION, Data[sizeof(DWORD)])];
> > +
> > +        KEY_VALUE_PARTIAL_INFORMATION *info = (KEY_VALUE_PARTIAL_INFORMATION*)buffer;
> > +
> > +        if (NtQueryValueKey(key, option, KeyValuePartialInformation, info, sizeof(buffer), &size) == STATUS_SUCCESS)
> > +        {
> > +            if (info->Type == REG_DWORD)
> > +                output = *(DWORD*)info->Data;
> > +        }
> > +
> > +        NtClose(key);
> > +    }
> > +
> > +    return output;
> > +}
> > +
> >  NTSTATUS WINAPI DriverEntry( DRIVER_OBJECT *driver, UNICODE_STRING *path )
> >  {
> >      static const WCHAR udevW[] = {'\\','D','r','i','v','e','r','\\','U','D','E','V',0};

[PATCH 1/5] winebus.sys: Handle linux input event device via udev

[Sebastian Lackner]

On 27.02.2017 17:22, Aric Stewart wrote:
> On 2/27/17 10:00 AM, Sebastian Lackner wrote:
> > I assume this is meant as a fallback, if we don't have access to
> > hidraw, right? If you plan to use this as a replacement for dinput,
> > it would be better to implement it without fixed dependency on
> > udev.
>
> Likely users using HID for gamepads and joysticks will end up this
> route by default because all the permissions are automatically setup
> for us. Also then user will get to leverage all the work done in the
> kernel to handle devices that do not behave well on the HID side or
> which do not show up at all using hidraw (Xbox Controllers for
> example)
>
> The dependency for udev is for common device discovery. I feel like I
> still want that dependency unless we write our own device plug and
> unplug discovery system.

Are you sure that there are no systems out there where we have linux
events, but no udev? If yes the current solution is fine, but otherwise
we should at least implement a fallback method similar to how its done
in dinput (basically, just trying to open all devices in the range 0 ..
MAX_JOYDEV).
> > Does it hurt when both hidraw and input event devices are found? If
> > yes, it would be better to filter duplicates somewhere else,
> > instead of using registry keys.
>
> You can get duplicate devices, on being talking to the hidraw device
> and one to the linux event device, but both are the same physical
> device. I have explored a little bit to try to find if there is a
> good way to detect if a device is duplicated but it has not proven to
> be immediately easy.

If it can cause problems it is probably still worth the effort. There
might also be systems where hidraw devices have access permissions by
default, and we don't want to get duplicated input events then.
> Also then the question because which device is
> preferred if we find a duplicate.

I think preferring hidraw might be slightly better. Some devices also
offer their own control utilities, and those would probably not work when
using linux input events. Nevertheless, its a matter of taste and we can
certainly discuss about that.

Best regards,
Sebastian

[PATCH 1/5] winebus.sys: Handle linux input event device via udev

[Aric Stewart]

On 2/27/17 10:52 AM, Sebastian Lackner wrote:
> On 27.02.2017 17:22, Aric Stewart wrote:
> > On 2/27/17 10:00 AM, Sebastian Lackner wrote:
> > > I assume this is meant as a fallback, if we don't have access to
> > > hidraw, right? If you plan to use this as a replacement for dinput,
> > > it would be better to implement it without fixed dependency on
> > > udev.
> >
> > Likely users using HID for gamepads and joysticks will end up this
> > route by default because all the permissions are automatically setup
> > for us. Also then user will get to leverage all the work done in the
> > kernel to handle devices that do not behave well on the HID side or
> > which do not show up at all using hidraw (Xbox Controllers for
> > example)
> >
> > The dependency for udev is for common device discovery. I feel like I
> > still want that dependency unless we write our own device plug and
> > unplug discovery system.
>
> Are you sure that there are no systems out there where we have linux
> events, but no udev? If yes the current solution is fine, but otherwise
> we should at least implement a fallback method similar to how its done
> in dinput (basically, just trying to open all devices in the range 0 ..
> MAX_JOYDEV).

I am not a linux expert but according to quick web research udev has been default since about Linux kernel version 2.6.13. That is sometime around 2006. It became part of systemd in 2012. So I am reasonably sure that any modern linux system will have it. I am not sure how much we worry about pre 2006 systems. 

-aric
> > > Does it hurt when both hidraw and input event devices are found? If
> > > yes, it would be better to filter duplicates somewhere else,
> > > instead of using registry keys.
> >
> > You can get duplicate devices, on being talking to the hidraw device
> > and one to the linux event device, but both are the same physical
> > device. I have explored a little bit to try to find if there is a
> > good way to detect if a device is duplicated but it has not proven to
> > be immediately easy.
>
> If it can cause problems it is probably still worth the effort. There
> might also be systems where hidraw devices have access permissions by
> default, and we don't want to get duplicated input events then.
> > Also then the question because which device is
> > preferred if we find a duplicate.
>
> I think preferring hidraw might be slightly better. Some devices also
> offer their own control utilities, and those would probably not work when
> using linux input events. Nevertheless, its a matter of taste and we can
> certainly discuss about that.
>
> Best regards,
> Sebastian

[PATCH 1/5] winebus.sys: Handle linux input event device via udev

[Roderick Colenbrander]

On Mon, Feb 27, 2017 at 8:52 AM, Sebastian Lackner
<sebastian(a)fds-team.de> wrote:
> On 27.02.2017 17:22, Aric Stewart wrote:
> > On 2/27/17 10:00 AM, Sebastian Lackner wrote:
> > > I assume this is meant as a fallback, if we don't have access to
> > > hidraw, right? If you plan to use this as a replacement for dinput,
> > > it would be better to implement it without fixed dependency on
> > > udev.
> >
> > Likely users using HID for gamepads and joysticks will end up this
> > route by default because all the permissions are automatically setup
> > for us. Also then user will get to leverage all the work done in the
> > kernel to handle devices that do not behave well on the HID side or
> > which do not show up at all using hidraw (Xbox Controllers for
> > example)
> >
> > The dependency for udev is for common device discovery. I feel like I
> > still want that dependency unless we write our own device plug and
> > unplug discovery system.
>
> Are you sure that there are no systems out there where we have linux
> events, but no udev? If yes the current solution is fine, but otherwise
> we should at least implement a fallback method similar to how its done
> in dinput (basically, just trying to open all devices in the range 0 ..
> MAX_JOYDEV).
> > > Does it hurt when both hidraw and input event devices are found? If
> > > yes, it would be better to filter duplicates somewhere else,
> > > instead of using registry keys.
> >
> > You can get duplicate devices, on being talking to the hidraw device
> > and one to the linux event device, but both are the same physical
> > device. I have explored a little bit to try to find if there is a
> > good way to detect if a device is duplicated but it has not proven to
> > be immediately easy.
>
> If it can cause problems it is probably still worth the effort. There
> might also be systems where hidraw devices have access permissions by
> default, and we don't want to get duplicated input events then.
> > Also then the question because which device is
> > preferred if we find a duplicate.
>
> I think preferring hidraw might be slightly better. Some devices also
> offer their own control utilities, and those would probably not work when
> using linux input events. Nevertheless, its a matter of taste and we can
> certainly discuss about that.

For some devices hidraw is a decent option, but in general my strong
recommendation is to stay the hell away from hidraw. The HID spec
itself has many holes (e.g. no good button mapping) and many vendors
implement device specific methods to do certain things (e.g. for
controller rumble, sensors, ..). The Linux kernel has a generic HID
parser, but there are so many fixups for devices, quirks and custom
handling. It is best to have the Linux kernel's evdev framework do the
work.

However the downside of evdev is that there is no 1:1 mapping between
HID devices and evdev devices. More and more devices are reported as
composite devices through evdev including Wacom devices, Wii
controllers, Sony controllers and more. It means for each HID device
there are multiple evdev nodes with the same product/vendor/version,
but different capabilities. Some separate out touch functionality,
others do it for sensors. Ideally this patch series would 'merge'
devices, maybe based on EVIOCGPHYS or something (or serial number).
> Best regards,
> Sebastian

[PATCH 1/5] winebus.sys: Handle linux input event device via udev

[Aric Stewart]

I have a few moments heads up from my current task so I can circle back here at least a bit.

On 3/2/17 12:49 AM, Roderick Colenbrander wrote:
> On Mon, Feb 27, 2017 at 8:52 AM, Sebastian Lackner
> <sebastian(a)fds-team.de> wrote:
> > On 27.02.2017 17:22, Aric Stewart wrote:
> > > On 2/27/17 10:00 AM, Sebastian Lackner wrote:
> > > > I assume this is meant as a fallback, if we don't have access to
> > > > hidraw, right? If you plan to use this as a replacement for dinput,
> > > > it would be better to implement it without fixed dependency on
> > > > udev.
> > >
> > > Likely users using HID for gamepads and joysticks will end up this
> > > route by default because all the permissions are automatically setup
> > > for us. Also then user will get to leverage all the work done in the
> > > kernel to handle devices that do not behave well on the HID side or
> > > which do not show up at all using hidraw (Xbox Controllers for
> > > example)
> > >
> > > The dependency for udev is for common device discovery. I feel like I
> > > still want that dependency unless we write our own device plug and
> > > unplug discovery system.
> >
> > Are you sure that there are no systems out there where we have linux
> > events, but no udev? If yes the current solution is fine, but otherwise
> > we should at least implement a fallback method similar to how its done
> > in dinput (basically, just trying to open all devices in the range 0 ..
> > MAX_JOYDEV).
> > > > Does it hurt when both hidraw and input event devices are found? If
> > > > yes, it would be better to filter duplicates somewhere else,
> > > > instead of using registry keys.
> > >
> > > You can get duplicate devices, on being talking to the hidraw device
> > > and one to the linux event device, but both are the same physical
> > > device. I have explored a little bit to try to find if there is a
> > > good way to detect if a device is duplicated but it has not proven to
> > > be immediately easy.
> >
> > If it can cause problems it is probably still worth the effort. There
> > might also be systems where hidraw devices have access permissions by
> > default, and we don't want to get duplicated input events then.
> > > Also then the question because which device is
> > > preferred if we find a duplicate.
> >
> > I think preferring hidraw might be slightly better. Some devices also
> > offer their own control utilities, and those would probably not work when
> > using linux input events. Nevertheless, its a matter of taste and we can
> > certainly discuss about that.

Well I am at first ecstatic that you did not read over these patches and just say "NO". I got most of this from dinput code and my very beginning reading about evdev. I am very much not a Linux or evdev developer.
> For some devices hidraw is a decent option, but in general my strong
> recommendation is to stay the hell away from hidraw. The HID spec
> itself has many holes (e.g. no good button mapping) and many vendors
> implement device specific methods to do certain things (e.g. for
> controller rumble, sensors, ..). The Linux kernel has a generic HID
> parser, but there are so many fixups for devices, quirks and custom
> handling. It is best to have the Linux kernel's evdev framework do the
> work.

My vision is that for gamepad/joystick we will likely stick mostly with evdev.  I agree that seems to have the best support and I love being able to leverage all the kernel work. Then for other hid devices, scales, golf simulators, whatever, they can be hidraw devices as likely those applications already know they need to do whatever funky things they need to do to communicate to those devices.
> However the downside of evdev is that there is no 1:1 mapping between
> HID devices and evdev devices. More and more devices are reported as
> composite devices through evdev including Wacom devices, Wii
> controllers, Sony controllers and more. It means for each HID device
> there are multiple evdev nodes with the same product/vendor/version,
> but different capabilities. Some separate out touch functionality,
> others do it for sensors. Ideally this patch series would 'merge'
> devices, maybe based on EVIOCGPHYS or something (or serial number).

Yeah, I have been aware of composite devices, but presently have done no work on handling them. Mostly because my box of test devices have no composite devices in them and because it gets really tricky really quickly.  I have been designing with it in the back  of my mind so hopefully when we get demands for composite devices to work sanely it will be able to be worked in without extensive re-writing. 


Thanks!
-aric
> > Best regards,
> > Sebastian