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@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");elsesystems++;- }
+#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");elsesystems++;- }
+#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};