[PATCH 1/5] winebus.sys: Report WINEBUS\WINE_COMP_HID compatible id.
And match it in winehid.sys instead of individual bus ids. Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- This series should apply smoothly with the previous winebus.sys. I'm also sending it now because it may need a bit more consideration. The rationale of the design is described below: The idea here is to create a new xinput.sys driver, which will match xinput-compatible winebus.sys devices, and create two device nodes on top of the underlying device. The first one will be the public "&IG_00" XInput bogus HID gamepad, as native has, and as Win32 SDL expects to see. The other will be a pass-through to the underlying device, but although it's going to be an HID compatible device too, it will need to be exposed on a different interface class GUID in order to be used only by Wine XInput. The second node will filter its read requests, and then translate them into the corresponding bogus XInput HID report, passing them to the first one for its read request completion. This design removes the need to really care about XInput gamepad compat in winebus.sys, making it possible to extend the HID reports there to include the full capabilities of the unix devices: * It will allow the user-space XInput implementation to have access to the full capabilities of the underlying device, and not be limited by the bogus HID compatibility anymore. * It also removes an eventual need to duplicate the winebus devices, as the two nodes will only need one underlying device (the bogus XInput node will never actually request reads to its underlying PDO). * And finally it makes it possible to have XInput compatibility with any underlying device, not just with SDL or evdev where we handle the report creation. Even hidraw or iohid device reports can be translated to the XInput HID reports. dlls/winebus.sys/main.c | 22 ++++++++++++++++++++-- dlls/winehid.sys/winehid.inf | 7 +------ 2 files changed, 21 insertions(+), 8 deletions(-) diff --git a/dlls/winebus.sys/main.c b/dlls/winebus.sys/main.c index e372ec6db3f..e360416b5dd 100644 --- a/dlls/winebus.sys/main.c +++ b/dlls/winebus.sys/main.c @@ -225,7 +225,7 @@ static WCHAR *get_device_id(DEVICE_OBJECT *device) return dst; } -static WCHAR *get_compatible_ids(DEVICE_OBJECT *device) +static WCHAR *get_hardware_ids(DEVICE_OBJECT *device) { struct device_extension *ext = (struct device_extension *)device->DeviceExtension; WCHAR *dst; @@ -239,6 +239,24 @@ static WCHAR *get_compatible_ids(DEVICE_OBJECT *device) return dst; } +static WCHAR *get_compatible_ids(DEVICE_OBJECT *device) +{ + static const WCHAR hid_compat[] = + { + 'W','I','N','E','B','U','S','\\','W','I','N','E','_','C','O','M','P','_','H','I','D',0 + }; + DWORD len = strlenW(hid_compat); + WCHAR *dst; + + if ((dst = ExAllocatePool(PagedPool, (len + 2) * sizeof(WCHAR)))) + { + strcpyW(dst, hid_compat); + dst[len + 1] = 0; + } + + return dst; +} + static void remove_pending_irps(DEVICE_OBJECT *device) { struct device_extension *ext = device->DeviceExtension; @@ -441,7 +459,7 @@ static NTSTATUS handle_IRP_MN_QUERY_ID(DEVICE_OBJECT *device, IRP *irp) { case BusQueryHardwareIDs: TRACE("BusQueryHardwareIDs\n"); - irp->IoStatus.Information = (ULONG_PTR)get_compatible_ids(device); + irp->IoStatus.Information = (ULONG_PTR)get_hardware_ids(device); break; case BusQueryCompatibleIDs: TRACE("BusQueryCompatibleIDs\n"); diff --git a/dlls/winehid.sys/winehid.inf b/dlls/winehid.sys/winehid.inf index f9ed4091217..c1d8a1cf999 100644 --- a/dlls/winehid.sys/winehid.inf +++ b/dlls/winehid.sys/winehid.inf @@ -7,12 +7,7 @@ Class=HIDClass Wine=mfg_section [mfg_section] -Wine hidraw device=device_section,HIDRAW -Wine IOHID device=device_section,IOHID -Wine libevent device=device_section,LNXEV -Wine SDL HID device=device_section,SDLJOY -Wine mouse device=device_section,WINEMOUSE -Wine keyboard device=device_section,WINEKEYBOARD +Wine HID compatible device=device_section,WINEBUS\WINE_COMP_HID [device_section.Services] AddService = winehid,0x2,svc_section -- 2.32.0
Currently only acting as a pass-through driver, matching any device with a WINEBUS\WINE_COMP_XINPUT compatible id. This creates new PDO on the bus, adding the &IG_ device ID suffix to the original device ID (replacing an eventual &MI_ suffix), and removes the need to set the suffix on winebus.sys side. Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- configure.ac | 1 + dlls/winebus.sys/main.c | 27 +-- dlls/xinput.sys/Makefile.in | 8 + dlls/xinput.sys/main.c | 393 ++++++++++++++++++++++++++++++++ dlls/xinput.sys/xinput.inf | 22 ++ dlls/xinput.sys/xinput.rc | 20 ++ dlls/xinput.sys/xinput.sys.spec | 1 + loader/wine.inf.in | 1 + 8 files changed, 457 insertions(+), 16 deletions(-) create mode 100644 dlls/xinput.sys/Makefile.in create mode 100644 dlls/xinput.sys/main.c create mode 100644 dlls/xinput.sys/xinput.inf create mode 100644 dlls/xinput.sys/xinput.rc create mode 100644 dlls/xinput.sys/xinput.sys.spec diff --git a/configure.ac b/configure.ac index 7237289a2ad..7ff7869b37e 100644 --- a/configure.ac +++ b/configure.ac @@ -3895,6 +3895,7 @@ WINE_CONFIG_MAKEFILE(dlls/xaudio2_7) WINE_CONFIG_MAKEFILE(dlls/xaudio2_7/tests) WINE_CONFIG_MAKEFILE(dlls/xaudio2_8) WINE_CONFIG_MAKEFILE(dlls/xaudio2_9) +WINE_CONFIG_MAKEFILE(dlls/xinput.sys) WINE_CONFIG_MAKEFILE(dlls/xinput1_1) WINE_CONFIG_MAKEFILE(dlls/xinput1_2) WINE_CONFIG_MAKEFILE(dlls/xinput1_3) diff --git a/dlls/winebus.sys/main.c b/dlls/winebus.sys/main.c index e360416b5dd..17720bffbc5 100644 --- a/dlls/winebus.sys/main.c +++ b/dlls/winebus.sys/main.c @@ -152,8 +152,6 @@ static CRITICAL_SECTION device_list_cs = { &critsect_debug, -1, 0, 0, 0, 0 }; static struct list pnp_devset = LIST_INIT(pnp_devset); static const WCHAR zero_serialW[]= {'0','0','0','0',0}; -static const WCHAR miW[] = {'M','I',0}; -static const WCHAR igW[] = {'I','G',0}; static inline WCHAR *strdupW(const WCHAR *src) { @@ -201,25 +199,17 @@ static WCHAR *get_instance_id(DEVICE_OBJECT *device) static WCHAR *get_device_id(DEVICE_OBJECT *device) { + static const WCHAR input_formatW[] = {'&','M','I','_','%','0','2','u',0}; static const WCHAR formatW[] = {'%','s','\\','v','i','d','_','%','0','4','x', '&','p','i','d','_','%','0','4','x',0}; - static const WCHAR format_inputW[] = {'%','s','\\','v','i','d','_','%','0','4','x', - '&','p','i','d','_','%','0','4','x','&','%','s','_','%','0','2','i',0}; struct device_extension *ext = (struct device_extension *)device->DeviceExtension; DWORD len = strlenW(ext->busid) + 34; - WCHAR *dst; + WCHAR *dst, *tmp; if ((dst = ExAllocatePool(PagedPool, len * sizeof(WCHAR)))) { - if (ext->input == (WORD)-1) - { - sprintfW(dst, formatW, ext->busid, ext->vid, ext->pid); - } - else - { - sprintfW(dst, format_inputW, ext->busid, ext->vid, ext->pid, - ext->is_gamepad ? igW : miW, ext->input); - } + tmp = dst + sprintfW(dst, formatW, ext->busid, ext->vid, ext->pid); + if (ext->input != (WORD)-1) sprintfW(tmp, input_formatW, ext->input); } return dst; @@ -245,12 +235,17 @@ static WCHAR *get_compatible_ids(DEVICE_OBJECT *device) { 'W','I','N','E','B','U','S','\\','W','I','N','E','_','C','O','M','P','_','H','I','D',0 }; - DWORD len = strlenW(hid_compat); + static const WCHAR xinput_compat[] = + { + 'W','I','N','E','B','U','S','\\','W','I','N','E','_','C','O','M','P','_','X','I','N','P','U','T',0 + }; + struct device_extension *ext = (struct device_extension *)device->DeviceExtension; + DWORD len = strlenW(ext->is_gamepad ? xinput_compat : hid_compat); WCHAR *dst; if ((dst = ExAllocatePool(PagedPool, (len + 2) * sizeof(WCHAR)))) { - strcpyW(dst, hid_compat); + strcpyW(dst, ext->is_gamepad ? xinput_compat : hid_compat); dst[len + 1] = 0; } diff --git a/dlls/xinput.sys/Makefile.in b/dlls/xinput.sys/Makefile.in new file mode 100644 index 00000000000..52b00ef0528 --- /dev/null +++ b/dlls/xinput.sys/Makefile.in @@ -0,0 +1,8 @@ +MODULE = xinput.sys +IMPORTS = ntoskrnl +EXTRADLLFLAGS = -mno-cygwin -Wl,--subsystem,native + +C_SRCS = \ + main.c + +RC_SRCS = xinput.rc diff --git a/dlls/xinput.sys/main.c b/dlls/xinput.sys/main.c new file mode 100644 index 00000000000..b97ec49b325 --- /dev/null +++ b/dlls/xinput.sys/main.c @@ -0,0 +1,393 @@ +/* + * WINE XInput device driver + * + * Copyright 2021 Rémi Bernon for CodeWeavers + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA + */ + +#include <stdarg.h> +#include <stdlib.h> + +#include "ntstatus.h" +#define WIN32_NO_STATUS +#include "windef.h" +#include "winbase.h" +#include "winternl.h" +#include "winioctl.h" + +#include "cfgmgr32.h" +#include "ddk/wdm.h" +#include "ddk/hidport.h" + +#include "wine/debug.h" + +WINE_DEFAULT_DEBUG_CHANNEL(xinput); + +#if defined(__i386__) && !defined(_WIN32) +extern void *WINAPI wrap_fastcall_func1(void *func, const void *a); +__ASM_STDCALL_FUNC(wrap_fastcall_func1, 8, + "popl %ecx\n\t" + "popl %eax\n\t" + "xchgl (%esp),%ecx\n\t" + "jmp *%eax"); +#define call_fastcall_func1(func,a) wrap_fastcall_func1(func,a) +#else +#define call_fastcall_func1(func,a) func(a) +#endif + +struct device_state +{ + DEVICE_OBJECT *bus_pdo; + DEVICE_OBJECT *xinput_pdo; + + WCHAR bus_id[MAX_DEVICE_ID_LEN]; + WCHAR instance_id[MAX_DEVICE_ID_LEN]; +}; + +struct device_extension +{ + BOOL is_fdo; + BOOL removed; + + WCHAR device_id[MAX_DEVICE_ID_LEN]; + struct device_state *state; +}; + +static inline struct device_extension *ext_from_DEVICE_OBJECT(DEVICE_OBJECT *device) +{ + return (struct device_extension *)device->DeviceExtension; +} + +static NTSTATUS WINAPI internal_ioctl(DEVICE_OBJECT *device, IRP *irp) +{ + struct device_extension *ext = ext_from_DEVICE_OBJECT(device); + IO_STACK_LOCATION *stack = IoGetCurrentIrpStackLocation(irp); + ULONG code = stack->Parameters.DeviceIoControl.IoControlCode; + struct device_state *state = ext->state; + + if (InterlockedOr(&ext->removed, FALSE)) + { + irp->IoStatus.Status = STATUS_DELETE_PENDING; + irp->IoStatus.Information = 0; + IoCompleteRequest(irp, IO_NO_INCREMENT); + } + + TRACE("device %p, irp %p, code %#x, bus_pdo %p.\n", device, irp, code, state->bus_pdo); + + IoSkipCurrentIrpStackLocation(irp); + return IoCallDriver(state->bus_pdo, irp); +} + +static WCHAR *query_instance_id(DEVICE_OBJECT *device) +{ + struct device_extension *ext = ext_from_DEVICE_OBJECT(device); + struct device_state *state = ext->state; + DWORD len = wcslen(state->instance_id); + WCHAR *dst; + + if ((dst = ExAllocatePool(PagedPool, len * sizeof(WCHAR)))) + wcscpy(dst, state->instance_id); + + return dst; +} + +static WCHAR *query_device_id(DEVICE_OBJECT *device) +{ + struct device_extension *ext = ext_from_DEVICE_OBJECT(device); + DWORD len = wcslen(ext->device_id); + WCHAR *dst; + + if ((dst = ExAllocatePool(PagedPool, len * sizeof(WCHAR)))) + wcscpy(dst, ext->device_id); + + return dst; +} + +static WCHAR *query_hardware_ids(DEVICE_OBJECT *device) +{ + struct device_extension *ext = (struct device_extension *)device->DeviceExtension; + DWORD len = wcslen(ext->device_id); + WCHAR *dst; + + if ((dst = ExAllocatePool(PagedPool, (len + 2) * sizeof(WCHAR)))) + { + wcscpy(dst, ext->device_id); + dst[len + 1] = 0; + } + + return dst; +} + +static WCHAR *query_compatible_ids(DEVICE_OBJECT *device) +{ + DWORD len = wcslen(L"WINEBUS\\WINE_COMP_HID"); + WCHAR *dst; + + if ((dst = ExAllocatePool(PagedPool, (len + 2) * sizeof(WCHAR)))) + { + wcscpy(dst, L"WINEBUS\\WINE_COMP_HID"); + dst[len + 1] = 0; + } + + return dst; +} + +static NTSTATUS WINAPI pdo_pnp(DEVICE_OBJECT *device, IRP *irp) +{ + struct device_extension *ext = ext_from_DEVICE_OBJECT(device); + IO_STACK_LOCATION *stack = IoGetCurrentIrpStackLocation(irp); + struct device_state *state = ext->state; + ULONG code = stack->MinorFunction; + NTSTATUS status; + + TRACE("device %p, irp %p, code %#x, bus_pdo %p.\n", device, irp, code, state->bus_pdo); + + switch (code) + { + case IRP_MN_START_DEVICE: + status = STATUS_SUCCESS; + break; + + case IRP_MN_SURPRISE_REMOVAL: + status = STATUS_SUCCESS; + if (InterlockedExchange(&ext->removed, TRUE)) break; + break; + + case IRP_MN_REMOVE_DEVICE: + irp->IoStatus.Status = STATUS_SUCCESS; + IoCompleteRequest(irp, IO_NO_INCREMENT); + IoDeleteDevice(device); + return STATUS_SUCCESS; + + case IRP_MN_QUERY_ID: + switch (stack->Parameters.QueryId.IdType) + { + case BusQueryHardwareIDs: + irp->IoStatus.Information = (ULONG_PTR)query_hardware_ids(device); + break; + case BusQueryCompatibleIDs: + irp->IoStatus.Information = (ULONG_PTR)query_compatible_ids(device); + break; + case BusQueryDeviceID: + irp->IoStatus.Information = (ULONG_PTR)query_device_id(device); + break; + case BusQueryInstanceID: + irp->IoStatus.Information = (ULONG_PTR)query_instance_id(device); + break; + default: + IoSkipCurrentIrpStackLocation(irp); + return IoCallDriver(state->bus_pdo, irp); + } + + if (!irp->IoStatus.Information) status = STATUS_NO_MEMORY; + else status = STATUS_SUCCESS; + break; + + default: + IoSkipCurrentIrpStackLocation(irp); + return IoCallDriver(state->bus_pdo, irp); + } + + irp->IoStatus.Status = status; + IoCompleteRequest(irp, IO_NO_INCREMENT); + return status; +} + +static void create_child_pdos(DEVICE_OBJECT *fdo) +{ + struct device_extension *fdo_ext = fdo->DeviceExtension, *pdo_ext; + struct device_state *state = fdo_ext->state; + DEVICE_OBJECT *xinput_pdo; + UNICODE_STRING string; + WCHAR *tmp, pdo_name[255]; + NTSTATUS status; + + swprintf(pdo_name, ARRAY_SIZE(pdo_name), L"\\Device\\%s#%p&%p", state->bus_id, fdo->DriverObject, state->bus_pdo); + RtlInitUnicodeString(&string, pdo_name); + if ((status = IoCreateDevice(fdo->DriverObject, sizeof(*pdo_ext), &string, 0, 0, FALSE, &xinput_pdo))) + { + ERR( "failed to create xinput PDO, status %#x.\n", status ); + return; + } + + pdo_ext = xinput_pdo->DeviceExtension; + pdo_ext->is_fdo = FALSE; + pdo_ext->state = state; + wcscpy(pdo_ext->device_id, fdo_ext->device_id); + if ((tmp = wcsstr(pdo_ext->device_id, L"&MI_"))) memcpy(tmp, L"&IG", 6); + else wcscat(pdo_ext->device_id, L"&IG_00"); + + state->xinput_pdo = xinput_pdo; + + TRACE("fdo %p, xinput PDO %p.\n", fdo, xinput_pdo); + + IoInvalidateDeviceRelations(state->bus_pdo, BusRelations); +} + +static NTSTATUS WINAPI fdo_pnp(DEVICE_OBJECT *device, IRP *irp) +{ + struct device_extension *ext = ext_from_DEVICE_OBJECT(device); + IO_STACK_LOCATION *stack = IoGetCurrentIrpStackLocation(irp); + struct device_state *state = ext->state; + ULONG code = stack->MinorFunction; + DEVICE_RELATIONS *devices; + DEVICE_OBJECT *child; + NTSTATUS status; + + TRACE("device %p, irp %p, code %#x, bus_pdo %p.\n", device, irp, code, state->bus_pdo); + + switch (stack->MinorFunction) + { + case IRP_MN_QUERY_DEVICE_RELATIONS: + if (stack->Parameters.QueryDeviceRelations.Type == BusRelations) + { + if (!(devices = ExAllocatePool(PagedPool, offsetof(DEVICE_RELATIONS, Objects[2])))) + { + irp->IoStatus.Status = STATUS_NO_MEMORY; + IoCompleteRequest(irp, IO_NO_INCREMENT); + return STATUS_NO_MEMORY; + } + + devices->Count = 0; + if ((child = state->xinput_pdo)) + { + devices->Objects[devices->Count] = child; + call_fastcall_func1(ObfReferenceObject, child); + devices->Count++; + } + + irp->IoStatus.Information = (ULONG_PTR)devices; + irp->IoStatus.Status = STATUS_SUCCESS; + } + + IoSkipCurrentIrpStackLocation(irp); + return IoCallDriver(state->bus_pdo, irp); + + case IRP_MN_START_DEVICE: + IoSkipCurrentIrpStackLocation(irp); + if (!(status = IoCallDriver(state->bus_pdo, irp))) + create_child_pdos(device); + return status; + + case IRP_MN_REMOVE_DEVICE: + IoSkipCurrentIrpStackLocation(irp); + status = IoCallDriver(state->bus_pdo, irp); + IoDetachDevice(state->bus_pdo); + IoDeleteDevice(device); + return status; + + default: + IoSkipCurrentIrpStackLocation(irp); + return IoCallDriver(state->bus_pdo, irp); + } + + return STATUS_SUCCESS; +} + +static NTSTATUS WINAPI driver_pnp(DEVICE_OBJECT *device, IRP *irp) +{ + struct device_extension *ext = ext_from_DEVICE_OBJECT(device); + + if (ext->is_fdo) return fdo_pnp(device, irp); + return pdo_pnp(device, irp); +} + +static NTSTATUS get_device_id(DEVICE_OBJECT *device, BUS_QUERY_ID_TYPE type, WCHAR *id) +{ + IO_STACK_LOCATION *stack; + IO_STATUS_BLOCK io; + KEVENT event; + IRP *irp; + + KeInitializeEvent(&event, NotificationEvent, FALSE); + irp = IoBuildSynchronousFsdRequest(IRP_MJ_PNP, device, NULL, 0, NULL, &event, &io); + if (irp == NULL) return STATUS_NO_MEMORY; + + stack = IoGetNextIrpStackLocation(irp); + stack->MinorFunction = IRP_MN_QUERY_ID; + stack->Parameters.QueryId.IdType = type; + + if (IoCallDriver(device, irp) == STATUS_PENDING) + KeWaitForSingleObject(&event, Executive, KernelMode, FALSE, NULL); + + wcscpy(id, (WCHAR *)io.Information); + ExFreePool((WCHAR *)io.Information); + return io.Status; +} + +static NTSTATUS WINAPI add_device(DRIVER_OBJECT *driver, DEVICE_OBJECT *bus_pdo) +{ + WCHAR bus_id[MAX_DEVICE_ID_LEN], *device_id, instance_id[MAX_DEVICE_ID_LEN]; + struct device_extension *ext; + struct device_state *state; + DEVICE_OBJECT *fdo; + NTSTATUS status; + + TRACE("driver %p, bus_pdo %p.\n", driver, bus_pdo); + + if ((status = get_device_id(bus_pdo, BusQueryDeviceID, bus_id))) + { + ERR("failed to get PDO device id, status %#x.\n", status); + return status; + } + + if ((device_id = wcsrchr(bus_id, '\\'))) *device_id++ = 0; + else + { + ERR("unexpected device id %s\n", debugstr_w(bus_id)); + return STATUS_UNSUCCESSFUL; + } + + if ((status = get_device_id(bus_pdo, BusQueryInstanceID, instance_id))) + { + ERR("failed to get PDO instance id, status %#x.\n", status); + return status; + } + + if ((status = IoCreateDevice(driver, sizeof(*ext) + sizeof(struct device_state), NULL, + FILE_DEVICE_BUS_EXTENDER, 0, FALSE, &fdo))) + { + ERR("failed to create bus FDO, status %#x.\n", status); + return status; + } + + ext = fdo->DeviceExtension; + ext->is_fdo = TRUE; + ext->state = (struct device_state *)(ext + 1); + + state = ext->state; + state->bus_pdo = bus_pdo; + wcscpy(state->bus_id, bus_id); + swprintf(ext->device_id, MAX_DEVICE_ID_LEN, L"%s\\%s", bus_id, device_id); + wcscpy(state->instance_id, instance_id); + + TRACE("fdo %p, bus %s, device %s, instance %s.\n", fdo, debugstr_w(state->bus_id), debugstr_w(ext->device_id), debugstr_w(state->instance_id)); + + IoAttachDeviceToDeviceStack(fdo, bus_pdo); + fdo->Flags &= ~DO_DEVICE_INITIALIZING; + return STATUS_SUCCESS; +} + +NTSTATUS WINAPI DriverEntry(DRIVER_OBJECT *driver, UNICODE_STRING *path) +{ + TRACE("driver %p, path %s.\n", driver, debugstr_w(path->Buffer)); + + driver->MajorFunction[IRP_MJ_INTERNAL_DEVICE_CONTROL] = internal_ioctl; + driver->MajorFunction[IRP_MJ_PNP] = driver_pnp; + driver->DriverExtension->AddDevice = add_device; + + return STATUS_SUCCESS; +} diff --git a/dlls/xinput.sys/xinput.inf b/dlls/xinput.sys/xinput.inf new file mode 100644 index 00000000000..dcc5c87166f --- /dev/null +++ b/dlls/xinput.sys/xinput.inf @@ -0,0 +1,22 @@ +[Version] +Signature="$CHICAGO$" +ClassGuid={4d36e97d-e325-11ce-bfc1-08002be10318} +Class=System + +[Manufacturer] +Wine=mfg_section + +[mfg_section] +Wine XInput compatible device=device_section,WINEBUS\WINE_COMP_XINPUT + +[device_section.Services] +AddService = xinput,0x2,svc_section + +[svc_section] +Description="Wine XInput device driver" +DisplayName="Wine XInput" +ServiceBinary="%12%\xinput.sys" +LoadOrderGroup="WinePlugPlay" +ServiceType=1 +StartType=3 +ErrorControl=1 diff --git a/dlls/xinput.sys/xinput.rc b/dlls/xinput.sys/xinput.rc new file mode 100644 index 00000000000..791dca8fccb --- /dev/null +++ b/dlls/xinput.sys/xinput.rc @@ -0,0 +1,20 @@ +/* + * Copyright 2021 Rémi Bernon for CodeWeavers + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA + */ + +/* @makedep: xinput.inf */ +1 WINE_DATA_FILE xinput.inf diff --git a/dlls/xinput.sys/xinput.sys.spec b/dlls/xinput.sys/xinput.sys.spec new file mode 100644 index 00000000000..76421d7e35b --- /dev/null +++ b/dlls/xinput.sys/xinput.sys.spec @@ -0,0 +1 @@ +# nothing to export diff --git a/loader/wine.inf.in b/loader/wine.inf.in index 49e4f45da27..9b8ec938c2f 100644 --- a/loader/wine.inf.in +++ b/loader/wine.inf.in @@ -4060,6 +4060,7 @@ services,"@%11%\ws2_32.dll,-4" winebus.inf,"@%12%\winebus.sys,-1" winehid.inf,"@%12%\winehid.sys,-1" wineusb.inf,"@%12%\wineusb.sys,-1" +xinput.inf,"@%12%\xinput.sys,-1" [NlsFiles] c_037.nls -- 2.32.0
On 8/18/21 2:31 AM, Rémi Bernon wrote:
Currently only acting as a pass-through driver, matching any device with a WINEBUS\WINE_COMP_XINPUT compatible id.
This creates new PDO on the bus, adding the &IG_ device ID suffix to the original device ID (replacing an eventual &MI_ suffix), and removes the need to set the suffix on winebus.sys side.
Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com>
Can we find something else to name this, so it's less ambiguous what "xinput" refers to? Maybe "winexinput"; this is a wine-specific DLL after all.
--- configure.ac | 1 + dlls/winebus.sys/main.c | 27 +-- dlls/xinput.sys/Makefile.in | 8 + dlls/xinput.sys/main.c | 393 ++++++++++++++++++++++++++++++++ dlls/xinput.sys/xinput.inf | 22 ++ dlls/xinput.sys/xinput.rc | 20 ++ dlls/xinput.sys/xinput.sys.spec | 1 + loader/wine.inf.in | 1 + 8 files changed, 457 insertions(+), 16 deletions(-) create mode 100644 dlls/xinput.sys/Makefile.in create mode 100644 dlls/xinput.sys/main.c create mode 100644 dlls/xinput.sys/xinput.inf create mode 100644 dlls/xinput.sys/xinput.rc create mode 100644 dlls/xinput.sys/xinput.sys.spec
diff --git a/configure.ac b/configure.ac index 7237289a2ad..7ff7869b37e 100644 --- a/configure.ac +++ b/configure.ac @@ -3895,6 +3895,7 @@ WINE_CONFIG_MAKEFILE(dlls/xaudio2_7) WINE_CONFIG_MAKEFILE(dlls/xaudio2_7/tests) WINE_CONFIG_MAKEFILE(dlls/xaudio2_8) WINE_CONFIG_MAKEFILE(dlls/xaudio2_9) +WINE_CONFIG_MAKEFILE(dlls/xinput.sys) WINE_CONFIG_MAKEFILE(dlls/xinput1_1) WINE_CONFIG_MAKEFILE(dlls/xinput1_2) WINE_CONFIG_MAKEFILE(dlls/xinput1_3) diff --git a/dlls/winebus.sys/main.c b/dlls/winebus.sys/main.c index e360416b5dd..17720bffbc5 100644 --- a/dlls/winebus.sys/main.c +++ b/dlls/winebus.sys/main.c @@ -152,8 +152,6 @@ static CRITICAL_SECTION device_list_cs = { &critsect_debug, -1, 0, 0, 0, 0 }; static struct list pnp_devset = LIST_INIT(pnp_devset);
static const WCHAR zero_serialW[]= {'0','0','0','0',0}; -static const WCHAR miW[] = {'M','I',0}; -static const WCHAR igW[] = {'I','G',0};
static inline WCHAR *strdupW(const WCHAR *src) { @@ -201,25 +199,17 @@ static WCHAR *get_instance_id(DEVICE_OBJECT *device)
static WCHAR *get_device_id(DEVICE_OBJECT *device) { + static const WCHAR input_formatW[] = {'&','M','I','_','%','0','2','u',0}; static const WCHAR formatW[] = {'%','s','\\','v','i','d','_','%','0','4','x', '&','p','i','d','_','%','0','4','x',0}; - static const WCHAR format_inputW[] = {'%','s','\\','v','i','d','_','%','0','4','x', - '&','p','i','d','_','%','0','4','x','&','%','s','_','%','0','2','i',0}; struct device_extension *ext = (struct device_extension *)device->DeviceExtension; DWORD len = strlenW(ext->busid) + 34; - WCHAR *dst; + WCHAR *dst, *tmp;
if ((dst = ExAllocatePool(PagedPool, len * sizeof(WCHAR)))) { - if (ext->input == (WORD)-1) - { - sprintfW(dst, formatW, ext->busid, ext->vid, ext->pid); - } - else - { - sprintfW(dst, format_inputW, ext->busid, ext->vid, ext->pid, - ext->is_gamepad ? igW : miW, ext->input); - } + tmp = dst + sprintfW(dst, formatW, ext->busid, ext->vid, ext->pid); + if (ext->input != (WORD)-1) sprintfW(tmp, input_formatW, ext->input); }
return dst; @@ -245,12 +235,17 @@ static WCHAR *get_compatible_ids(DEVICE_OBJECT *device) { 'W','I','N','E','B','U','S','\\','W','I','N','E','_','C','O','M','P','_','H','I','D',0 }; - DWORD len = strlenW(hid_compat); + static const WCHAR xinput_compat[] = + { + 'W','I','N','E','B','U','S','\\','W','I','N','E','_','C','O','M','P','_','X','I','N','P','U','T',0 + }; + struct device_extension *ext = (struct device_extension *)device->DeviceExtension; + DWORD len = strlenW(ext->is_gamepad ? xinput_compat : hid_compat); WCHAR *dst;
if ((dst = ExAllocatePool(PagedPool, (len + 2) * sizeof(WCHAR)))) { - strcpyW(dst, hid_compat); + strcpyW(dst, ext->is_gamepad ? xinput_compat : hid_compat); dst[len + 1] = 0; }
diff --git a/dlls/xinput.sys/Makefile.in b/dlls/xinput.sys/Makefile.in new file mode 100644 index 00000000000..52b00ef0528 --- /dev/null +++ b/dlls/xinput.sys/Makefile.in @@ -0,0 +1,8 @@ +MODULE = xinput.sys +IMPORTS = ntoskrnl +EXTRADLLFLAGS = -mno-cygwin -Wl,--subsystem,native + +C_SRCS = \ + main.c + +RC_SRCS = xinput.rc diff --git a/dlls/xinput.sys/main.c b/dlls/xinput.sys/main.c new file mode 100644 index 00000000000..b97ec49b325 --- /dev/null +++ b/dlls/xinput.sys/main.c @@ -0,0 +1,393 @@ +/* + * WINE XInput device driver + * + * Copyright 2021 Rémi Bernon for CodeWeavers + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA + */ + +#include <stdarg.h> +#include <stdlib.h> + +#include "ntstatus.h" +#define WIN32_NO_STATUS +#include "windef.h" +#include "winbase.h" +#include "winternl.h" +#include "winioctl.h" + +#include "cfgmgr32.h" +#include "ddk/wdm.h" +#include "ddk/hidport.h" + +#include "wine/debug.h" + +WINE_DEFAULT_DEBUG_CHANNEL(xinput); + +#if defined(__i386__) && !defined(_WIN32) +extern void *WINAPI wrap_fastcall_func1(void *func, const void *a); +__ASM_STDCALL_FUNC(wrap_fastcall_func1, 8, + "popl %ecx\n\t" + "popl %eax\n\t" + "xchgl (%esp),%ecx\n\t" + "jmp *%eax"); +#define call_fastcall_func1(func,a) wrap_fastcall_func1(func,a) +#else +#define call_fastcall_func1(func,a) func(a) +#endif + +struct device_state
This is a weird thing to call this, because it's not dynamic at all. Also, maybe a slightly more idiomatic structure would be to hold a pointer to the parent device as a struct device_extension, and then access its members through that pointer?
+{ + DEVICE_OBJECT *bus_pdo; + DEVICE_OBJECT *xinput_pdo;
This is ambiguous, how about "child_pdo"?
+ + WCHAR bus_id[MAX_DEVICE_ID_LEN]; + WCHAR instance_id[MAX_DEVICE_ID_LEN]; +}; + +struct device_extension
Can we please not call this "device_extension"? I know we use that in other DLLs, but I think it's rather unclear. "struct device" seems better to me.
+{ + BOOL is_fdo; + BOOL removed; + + WCHAR device_id[MAX_DEVICE_ID_LEN]; + struct device_state *state; +}; + +static inline struct device_extension *ext_from_DEVICE_OBJECT(DEVICE_OBJECT *device) +{ + return (struct device_extension *)device->DeviceExtension; +} + +static NTSTATUS WINAPI internal_ioctl(DEVICE_OBJECT *device, IRP *irp) +{ + struct device_extension *ext = ext_from_DEVICE_OBJECT(device); + IO_STACK_LOCATION *stack = IoGetCurrentIrpStackLocation(irp); + ULONG code = stack->Parameters.DeviceIoControl.IoControlCode; + struct device_state *state = ext->state; + + if (InterlockedOr(&ext->removed, FALSE)) + { + irp->IoStatus.Status = STATUS_DELETE_PENDING; + irp->IoStatus.Information = 0; + IoCompleteRequest(irp, IO_NO_INCREMENT); + } + + TRACE("device %p, irp %p, code %#x, bus_pdo %p.\n", device, irp, code, state->bus_pdo); + + IoSkipCurrentIrpStackLocation(irp); + return IoCallDriver(state->bus_pdo, irp); +} + +static WCHAR *query_instance_id(DEVICE_OBJECT *device) +{ + struct device_extension *ext = ext_from_DEVICE_OBJECT(device); + struct device_state *state = ext->state; + DWORD len = wcslen(state->instance_id); + WCHAR *dst; + + if ((dst = ExAllocatePool(PagedPool, len * sizeof(WCHAR)))) + wcscpy(dst, state->instance_id); + + return dst; +} + +static WCHAR *query_device_id(DEVICE_OBJECT *device) +{ + struct device_extension *ext = ext_from_DEVICE_OBJECT(device); + DWORD len = wcslen(ext->device_id); + WCHAR *dst; + + if ((dst = ExAllocatePool(PagedPool, len * sizeof(WCHAR)))) + wcscpy(dst, ext->device_id); + + return dst; +} + +static WCHAR *query_hardware_ids(DEVICE_OBJECT *device) +{ + struct device_extension *ext = (struct device_extension *)device->DeviceExtension; + DWORD len = wcslen(ext->device_id); + WCHAR *dst; + + if ((dst = ExAllocatePool(PagedPool, (len + 2) * sizeof(WCHAR)))) + { + wcscpy(dst, ext->device_id); + dst[len + 1] = 0; + } + + return dst; +} + +static WCHAR *query_compatible_ids(DEVICE_OBJECT *device) +{ + DWORD len = wcslen(L"WINEBUS\\WINE_COMP_HID");
Nitpick, but it strikes me as odd to define "len" instead of defining the string constant itself.
+ WCHAR *dst; + + if ((dst = ExAllocatePool(PagedPool, (len + 2) * sizeof(WCHAR)))) + { + wcscpy(dst, L"WINEBUS\\WINE_COMP_HID");
Also, you've just used "len", so this should be memcmp. Same for the others.
+ dst[len + 1] = 0; + } + + return dst; +} + +static NTSTATUS WINAPI pdo_pnp(DEVICE_OBJECT *device, IRP *irp) +{ + struct device_extension *ext = ext_from_DEVICE_OBJECT(device); + IO_STACK_LOCATION *stack = IoGetCurrentIrpStackLocation(irp); + struct device_state *state = ext->state; + ULONG code = stack->MinorFunction; + NTSTATUS status; + + TRACE("device %p, irp %p, code %#x, bus_pdo %p.\n", device, irp, code, state->bus_pdo); + + switch (code) + { + case IRP_MN_START_DEVICE: + status = STATUS_SUCCESS; + break; + + case IRP_MN_SURPRISE_REMOVAL: + status = STATUS_SUCCESS; + if (InterlockedExchange(&ext->removed, TRUE)) break; + break; + + case IRP_MN_REMOVE_DEVICE: + irp->IoStatus.Status = STATUS_SUCCESS; + IoCompleteRequest(irp, IO_NO_INCREMENT); + IoDeleteDevice(device); + return STATUS_SUCCESS; + + case IRP_MN_QUERY_ID: + switch (stack->Parameters.QueryId.IdType) + { + case BusQueryHardwareIDs: + irp->IoStatus.Information = (ULONG_PTR)query_hardware_ids(device); + break; + case BusQueryCompatibleIDs: + irp->IoStatus.Information = (ULONG_PTR)query_compatible_ids(device); + break; + case BusQueryDeviceID: + irp->IoStatus.Information = (ULONG_PTR)query_device_id(device); + break; + case BusQueryInstanceID: + irp->IoStatus.Information = (ULONG_PTR)query_instance_id(device); + break; + default: + IoSkipCurrentIrpStackLocation(irp); + return IoCallDriver(state->bus_pdo, irp); + } + + if (!irp->IoStatus.Information) status = STATUS_NO_MEMORY; + else status = STATUS_SUCCESS; + break; + + default: + IoSkipCurrentIrpStackLocation(irp); + return IoCallDriver(state->bus_pdo, irp); + } + + irp->IoStatus.Status = status; + IoCompleteRequest(irp, IO_NO_INCREMENT); + return status; +} + +static void create_child_pdos(DEVICE_OBJECT *fdo) +{ + struct device_extension *fdo_ext = fdo->DeviceExtension, *pdo_ext; + struct device_state *state = fdo_ext->state; + DEVICE_OBJECT *xinput_pdo; + UNICODE_STRING string; + WCHAR *tmp, pdo_name[255]; + NTSTATUS status; + + swprintf(pdo_name, ARRAY_SIZE(pdo_name), L"\\Device\\%s#%p&%p", state->bus_id, fdo->DriverObject, state->bus_pdo); + RtlInitUnicodeString(&string, pdo_name); + if ((status = IoCreateDevice(fdo->DriverObject, sizeof(*pdo_ext), &string, 0, 0, FALSE, &xinput_pdo))) + { + ERR( "failed to create xinput PDO, status %#x.\n", status ); + return; + } + + pdo_ext = xinput_pdo->DeviceExtension; + pdo_ext->is_fdo = FALSE; + pdo_ext->state = state; + wcscpy(pdo_ext->device_id, fdo_ext->device_id); + if ((tmp = wcsstr(pdo_ext->device_id, L"&MI_"))) memcpy(tmp, L"&IG", 6); + else wcscat(pdo_ext->device_id, L"&IG_00");
I hate to nitpick, but can we please add some newlines here? This is not easy to read.
+ + state->xinput_pdo = xinput_pdo; + + TRACE("fdo %p, xinput PDO %p.\n", fdo, xinput_pdo); + + IoInvalidateDeviceRelations(state->bus_pdo, BusRelations); +} + +static NTSTATUS WINAPI fdo_pnp(DEVICE_OBJECT *device, IRP *irp) +{ + struct device_extension *ext = ext_from_DEVICE_OBJECT(device); + IO_STACK_LOCATION *stack = IoGetCurrentIrpStackLocation(irp); + struct device_state *state = ext->state; + ULONG code = stack->MinorFunction; + DEVICE_RELATIONS *devices; + DEVICE_OBJECT *child; + NTSTATUS status; + + TRACE("device %p, irp %p, code %#x, bus_pdo %p.\n", device, irp, code, state->bus_pdo); + + switch (stack->MinorFunction) + { + case IRP_MN_QUERY_DEVICE_RELATIONS: + if (stack->Parameters.QueryDeviceRelations.Type == BusRelations) + { + if (!(devices = ExAllocatePool(PagedPool, offsetof(DEVICE_RELATIONS, Objects[2])))) + { + irp->IoStatus.Status = STATUS_NO_MEMORY; + IoCompleteRequest(irp, IO_NO_INCREMENT); + return STATUS_NO_MEMORY; + } + + devices->Count = 0; + if ((child = state->xinput_pdo)) + { + devices->Objects[devices->Count] = child; + call_fastcall_func1(ObfReferenceObject, child); + devices->Count++; + } + + irp->IoStatus.Information = (ULONG_PTR)devices; + irp->IoStatus.Status = STATUS_SUCCESS; + } + + IoSkipCurrentIrpStackLocation(irp); + return IoCallDriver(state->bus_pdo, irp); + + case IRP_MN_START_DEVICE: + IoSkipCurrentIrpStackLocation(irp); + if (!(status = IoCallDriver(state->bus_pdo, irp))) + create_child_pdos(device); + return status; + + case IRP_MN_REMOVE_DEVICE: + IoSkipCurrentIrpStackLocation(irp); + status = IoCallDriver(state->bus_pdo, irp); + IoDetachDevice(state->bus_pdo); + IoDeleteDevice(device); + return status; + + default: + IoSkipCurrentIrpStackLocation(irp); + return IoCallDriver(state->bus_pdo, irp); + } + + return STATUS_SUCCESS; +} + +static NTSTATUS WINAPI driver_pnp(DEVICE_OBJECT *device, IRP *irp) +{ + struct device_extension *ext = ext_from_DEVICE_OBJECT(device); + + if (ext->is_fdo) return fdo_pnp(device, irp); + return pdo_pnp(device, irp); +} + +static NTSTATUS get_device_id(DEVICE_OBJECT *device, BUS_QUERY_ID_TYPE type, WCHAR *id) +{ + IO_STACK_LOCATION *stack; + IO_STATUS_BLOCK io; + KEVENT event; + IRP *irp; + + KeInitializeEvent(&event, NotificationEvent, FALSE); + irp = IoBuildSynchronousFsdRequest(IRP_MJ_PNP, device, NULL, 0, NULL, &event, &io); + if (irp == NULL) return STATUS_NO_MEMORY; + + stack = IoGetNextIrpStackLocation(irp); + stack->MinorFunction = IRP_MN_QUERY_ID; + stack->Parameters.QueryId.IdType = type; + + if (IoCallDriver(device, irp) == STATUS_PENDING) + KeWaitForSingleObject(&event, Executive, KernelMode, FALSE, NULL); + + wcscpy(id, (WCHAR *)io.Information); + ExFreePool((WCHAR *)io.Information); + return io.Status; +} + +static NTSTATUS WINAPI add_device(DRIVER_OBJECT *driver, DEVICE_OBJECT *bus_pdo) +{ + WCHAR bus_id[MAX_DEVICE_ID_LEN], *device_id, instance_id[MAX_DEVICE_ID_LEN]; + struct device_extension *ext; + struct device_state *state; + DEVICE_OBJECT *fdo; + NTSTATUS status; + + TRACE("driver %p, bus_pdo %p.\n", driver, bus_pdo); + + if ((status = get_device_id(bus_pdo, BusQueryDeviceID, bus_id))) + { + ERR("failed to get PDO device id, status %#x.\n", status); + return status; + } + + if ((device_id = wcsrchr(bus_id, '\\'))) *device_id++ = 0; + else + { + ERR("unexpected device id %s\n", debugstr_w(bus_id)); + return STATUS_UNSUCCESSFUL; + } + + if ((status = get_device_id(bus_pdo, BusQueryInstanceID, instance_id))) + { + ERR("failed to get PDO instance id, status %#x.\n", status); + return status; + } + + if ((status = IoCreateDevice(driver, sizeof(*ext) + sizeof(struct device_state), NULL, + FILE_DEVICE_BUS_EXTENDER, 0, FALSE, &fdo))) + { + ERR("failed to create bus FDO, status %#x.\n", status); + return status; + } + + ext = fdo->DeviceExtension; + ext->is_fdo = TRUE; + ext->state = (struct device_state *)(ext + 1); + + state = ext->state; + state->bus_pdo = bus_pdo; + wcscpy(state->bus_id, bus_id); + swprintf(ext->device_id, MAX_DEVICE_ID_LEN, L"%s\\%s", bus_id, device_id); + wcscpy(state->instance_id, instance_id); + + TRACE("fdo %p, bus %s, device %s, instance %s.\n", fdo, debugstr_w(state->bus_id), debugstr_w(ext->device_id), debugstr_w(state->instance_id)); + + IoAttachDeviceToDeviceStack(fdo, bus_pdo); + fdo->Flags &= ~DO_DEVICE_INITIALIZING; + return STATUS_SUCCESS; +} + +NTSTATUS WINAPI DriverEntry(DRIVER_OBJECT *driver, UNICODE_STRING *path) +{ + TRACE("driver %p, path %s.\n", driver, debugstr_w(path->Buffer)); + + driver->MajorFunction[IRP_MJ_INTERNAL_DEVICE_CONTROL] = internal_ioctl; + driver->MajorFunction[IRP_MJ_PNP] = driver_pnp; + driver->DriverExtension->AddDevice = add_device; + + return STATUS_SUCCESS; +}
Missing a DriverUnload; that'll prevent the driver from being unloaded.
diff --git a/dlls/xinput.sys/xinput.inf b/dlls/xinput.sys/xinput.inf new file mode 100644 index 00000000000..dcc5c87166f --- /dev/null +++ b/dlls/xinput.sys/xinput.inf @@ -0,0 +1,22 @@ +[Version] +Signature="$CHICAGO$" +ClassGuid={4d36e97d-e325-11ce-bfc1-08002be10318} +Class=System + +[Manufacturer] +Wine=mfg_section + +[mfg_section] +Wine XInput compatible device=device_section,WINEBUS\WINE_COMP_XINPUT + +[device_section.Services] +AddService = xinput,0x2,svc_section + +[svc_section] +Description="Wine XInput device driver" +DisplayName="Wine XInput" +ServiceBinary="%12%\xinput.sys" +LoadOrderGroup="WinePlugPlay" +ServiceType=1 +StartType=3 +ErrorControl=1 diff --git a/dlls/xinput.sys/xinput.rc b/dlls/xinput.sys/xinput.rc new file mode 100644 index 00000000000..791dca8fccb --- /dev/null +++ b/dlls/xinput.sys/xinput.rc @@ -0,0 +1,20 @@ +/* + * Copyright 2021 Rémi Bernon for CodeWeavers + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA + */ + +/* @makedep: xinput.inf */ +1 WINE_DATA_FILE xinput.inf diff --git a/dlls/xinput.sys/xinput.sys.spec b/dlls/xinput.sys/xinput.sys.spec new file mode 100644 index 00000000000..76421d7e35b --- /dev/null +++ b/dlls/xinput.sys/xinput.sys.spec @@ -0,0 +1 @@ +# nothing to export diff --git a/loader/wine.inf.in b/loader/wine.inf.in index 49e4f45da27..9b8ec938c2f 100644 --- a/loader/wine.inf.in +++ b/loader/wine.inf.in @@ -4060,6 +4060,7 @@ services,"@%11%\ws2_32.dll,-4" winebus.inf,"@%12%\winebus.sys,-1" winehid.inf,"@%12%\winehid.sys,-1" wineusb.inf,"@%12%\wineusb.sys,-1" +xinput.inf,"@%12%\xinput.sys,-1"
[NlsFiles] c_037.nls
On 8/23/21 6:40 PM, Zebediah Figura (she/her) wrote:
On 8/18/21 2:31 AM, Rémi Bernon wrote:
Currently only acting as a pass-through driver, matching any device with a WINEBUS\WINE_COMP_XINPUT compatible id.
This creates new PDO on the bus, adding the &IG_ device ID suffix to the original device ID (replacing an eventual &MI_ suffix), and removes the need to set the suffix on winebus.sys side.
Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com>
Can we find something else to name this, so it's less ambiguous what "xinput" refers to?
Maybe "winexinput"; this is a wine-specific DLL after all.
Well, I initially thought about naming it like the native XInput driver, xusb22.sys or something like that, but then I saw there are several of them, and some even named xinput.sys (I think). Sure it's kind of Wine-specific, although a similar native driver exists, and naming it winexinput would make it live among the growing list of wine-something DLLs, which is imho starting to be painful to distinguish. It seemed clearer to name it xinput.sys, so it would be listed right alongside its user-space xinput1_* counterparts, which felt convenient when looking at the code. The driver is only meant for these DLLs, and some of the code will even be very similar. -- Rémi Bernon <rbernon(a)codeweavers.com>
On 8/18/21 2:31 AM, Rémi Bernon wrote:
Currently only acting as a pass-through driver, matching any device with a WINEBUS\WINE_COMP_XINPUT compatible id.
This creates new PDO on the bus, adding the &IG_ device ID suffix to the original device ID (replacing an eventual &MI_ suffix), and removes the need to set the suffix on winebus.sys side.
Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- configure.ac | 1 + dlls/winebus.sys/main.c | 27 +-- dlls/xinput.sys/Makefile.in | 8 + dlls/xinput.sys/main.c | 393 ++++++++++++++++++++++++++++++++ dlls/xinput.sys/xinput.inf | 22 ++ dlls/xinput.sys/xinput.rc | 20 ++ dlls/xinput.sys/xinput.sys.spec | 1 + loader/wine.inf.in | 1 + 8 files changed, 457 insertions(+), 16 deletions(-) create mode 100644 dlls/xinput.sys/Makefile.in create mode 100644 dlls/xinput.sys/main.c create mode 100644 dlls/xinput.sys/xinput.inf create mode 100644 dlls/xinput.sys/xinput.rc create mode 100644 dlls/xinput.sys/xinput.sys.spec
diff --git a/configure.ac b/configure.ac index 7237289a2ad..7ff7869b37e 100644 --- a/configure.ac +++ b/configure.ac @@ -3895,6 +3895,7 @@ WINE_CONFIG_MAKEFILE(dlls/xaudio2_7) WINE_CONFIG_MAKEFILE(dlls/xaudio2_7/tests) WINE_CONFIG_MAKEFILE(dlls/xaudio2_8) WINE_CONFIG_MAKEFILE(dlls/xaudio2_9) +WINE_CONFIG_MAKEFILE(dlls/xinput.sys) WINE_CONFIG_MAKEFILE(dlls/xinput1_1) WINE_CONFIG_MAKEFILE(dlls/xinput1_2) WINE_CONFIG_MAKEFILE(dlls/xinput1_3) diff --git a/dlls/winebus.sys/main.c b/dlls/winebus.sys/main.c index e360416b5dd..17720bffbc5 100644 --- a/dlls/winebus.sys/main.c +++ b/dlls/winebus.sys/main.c @@ -152,8 +152,6 @@ static CRITICAL_SECTION device_list_cs = { &critsect_debug, -1, 0, 0, 0, 0 }; static struct list pnp_devset = LIST_INIT(pnp_devset);
static const WCHAR zero_serialW[]= {'0','0','0','0',0}; -static const WCHAR miW[] = {'M','I',0}; -static const WCHAR igW[] = {'I','G',0};
static inline WCHAR *strdupW(const WCHAR *src) { @@ -201,25 +199,17 @@ static WCHAR *get_instance_id(DEVICE_OBJECT *device)
static WCHAR *get_device_id(DEVICE_OBJECT *device) { + static const WCHAR input_formatW[] = {'&','M','I','_','%','0','2','u',0}; static const WCHAR formatW[] = {'%','s','\\','v','i','d','_','%','0','4','x', '&','p','i','d','_','%','0','4','x',0}; - static const WCHAR format_inputW[] = {'%','s','\\','v','i','d','_','%','0','4','x', - '&','p','i','d','_','%','0','4','x','&','%','s','_','%','0','2','i',0}; struct device_extension *ext = (struct device_extension *)device->DeviceExtension; DWORD len = strlenW(ext->busid) + 34; - WCHAR *dst; + WCHAR *dst, *tmp;
if ((dst = ExAllocatePool(PagedPool, len * sizeof(WCHAR)))) { - if (ext->input == (WORD)-1) - { - sprintfW(dst, formatW, ext->busid, ext->vid, ext->pid); - } - else - { - sprintfW(dst, format_inputW, ext->busid, ext->vid, ext->pid, - ext->is_gamepad ? igW : miW, ext->input); - } + tmp = dst + sprintfW(dst, formatW, ext->busid, ext->vid, ext->pid); + if (ext->input != (WORD)-1) sprintfW(tmp, input_formatW, ext->input); }
return dst; @@ -245,12 +235,17 @@ static WCHAR *get_compatible_ids(DEVICE_OBJECT *device) { 'W','I','N','E','B','U','S','\\','W','I','N','E','_','C','O','M','P','_','H','I','D',0 }; - DWORD len = strlenW(hid_compat); + static const WCHAR xinput_compat[] = + { + 'W','I','N','E','B','U','S','\\','W','I','N','E','_','C','O','M','P','_','X','I','N','P','U','T',0 + }; + struct device_extension *ext = (struct device_extension *)device->DeviceExtension; + DWORD len = strlenW(ext->is_gamepad ? xinput_compat : hid_compat); WCHAR *dst;
if ((dst = ExAllocatePool(PagedPool, (len + 2) * sizeof(WCHAR)))) { - strcpyW(dst, hid_compat); + strcpyW(dst, ext->is_gamepad ? xinput_compat : hid_compat); dst[len + 1] = 0; }
It also occurs to me that you could put the winebus parts in a separate patch, or even combine them with patch 3/5.
On 8/23/21 6:41 PM, Zebediah Figura (she/her) wrote:
It also occurs to me that you could put the winebus parts in a separate patch, or even combine them with patch 3/5.
Sure, altough in that case the driver code will not be actually executed until PATCH 3. -- Rémi Bernon <rbernon(a)codeweavers.com>
The &IG_00 suffix is now automatically added by xinput.sys, and we use a compatible id instead to match devices. Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/winebus.sys/bus_iohid.c | 5 +---- dlls/winebus.sys/bus_sdl.c | 5 +---- dlls/winebus.sys/bus_udev.c | 2 -- 3 files changed, 2 insertions(+), 10 deletions(-) diff --git a/dlls/winebus.sys/bus_iohid.c b/dlls/winebus.sys/bus_iohid.c index 4eb0ea0e4b3..8ba90714071 100644 --- a/dlls/winebus.sys/bus_iohid.c +++ b/dlls/winebus.sys/bus_iohid.c @@ -289,7 +289,6 @@ static void handle_DeviceMatchingCallback(void *context, IOReturn result, void * CFStringRef str = NULL; WCHAR serial_string[256]; BOOL is_gamepad = FALSE; - WORD input = -1; TRACE("OS/X IOHID Device Added %p\n", IOHIDDevice); @@ -351,10 +350,8 @@ static void handle_DeviceMatchingCallback(void *context, IOReturn result, void * is_gamepad = (axes == 6 && buttons >= 14); } } - if (is_gamepad) - input = 0; - device = bus_create_hid_device(busidW, vid, pid, input, + device = bus_create_hid_device(busidW, vid, pid, -1, version, uid, str ? serial_string : NULL, is_gamepad, &iohid_vtbl, sizeof(struct platform_private)); if (!device) diff --git a/dlls/winebus.sys/bus_sdl.c b/dlls/winebus.sys/bus_sdl.c index 84e3ef20664..c2b0ad17c95 100644 --- a/dlls/winebus.sys/bus_sdl.c +++ b/dlls/winebus.sys/bus_sdl.c @@ -735,7 +735,6 @@ static void try_add_device(unsigned int index) WCHAR serial[34] = {0}; char guid_str[34]; BOOL is_xbox_gamepad; - WORD input = -1; SDL_Joystick* joystick; SDL_JoystickID id; @@ -786,10 +785,8 @@ static void try_add_device(unsigned int index) button_count = pSDL_JoystickNumButtons(joystick); is_xbox_gamepad = (axis_count == 6 && button_count >= 14); } - if (is_xbox_gamepad) - input = 0; - device = bus_create_hid_device(sdl_busidW, vid, pid, input, version, index, + device = bus_create_hid_device(sdl_busidW, vid, pid, -1, version, index, serial, is_xbox_gamepad, &sdl_vtbl, sizeof(struct platform_private)); if (device) diff --git a/dlls/winebus.sys/bus_udev.c b/dlls/winebus.sys/bus_udev.c index 77a242a6087..1a51a813a35 100644 --- a/dlls/winebus.sys/bus_udev.c +++ b/dlls/winebus.sys/bus_udev.c @@ -1118,8 +1118,6 @@ static void try_add_device(struct udev_device *dev) is_gamepad = (axes == 6 && buttons >= 14); } #endif - if (input == (WORD)-1 && is_gamepad) - input = 0; TRACE("Found udev device %s (vid %04x, pid %04x, version %04x, input %d, serial %s)\n", debugstr_a(devnode), vid, pid, version, input, debugstr_w(serial)); -- 2.32.0
This internal PDO is an HID compatible pass-through device, but it needs to be kept private and is listed on the internal XINPUT device interface class, instead of the public HID device interface class. This is a Wine extension for convenience and native XInput driver uses a different, undocumented, device interface. This internal PDO filters the report read requests and copies the input reports to the pending xinput PDO read requests, completing them at the same time. Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/hidclass.sys/hid.h | 1 + dlls/hidclass.sys/pnp.c | 6 +- dlls/xinput.sys/Makefile.in | 2 +- dlls/xinput.sys/main.c | 230 +++++++++++++++++++++++++++++++++++- 4 files changed, 233 insertions(+), 6 deletions(-) diff --git a/dlls/hidclass.sys/hid.h b/dlls/hidclass.sys/hid.h index 1ca6a926872..4fa9e72e615 100644 --- a/dlls/hidclass.sys/hid.h +++ b/dlls/hidclass.sys/hid.h @@ -84,6 +84,7 @@ typedef struct _BASE_DEVICE_EXTENSION * for convenience. */ WCHAR device_id[MAX_DEVICE_ID_LEN]; WCHAR instance_id[MAX_DEVICE_ID_LEN]; + const GUID *interface_guid; BOOL is_fdo; } BASE_DEVICE_EXTENSION; diff --git a/dlls/hidclass.sys/pnp.c b/dlls/hidclass.sys/pnp.c index db45aea2fd3..60aaa2e4a6f 100644 --- a/dlls/hidclass.sys/pnp.c +++ b/dlls/hidclass.sys/pnp.c @@ -38,6 +38,7 @@ WINE_DEFAULT_DEBUG_CHANNEL(hid); DEFINE_DEVPROPKEY(DEVPROPKEY_HID_HANDLE, 0xbc62e415, 0xf4fe, 0x405c, 0x8e, 0xda, 0x63, 0x6f, 0xb5, 0x9f, 0x08, 0x98, 2); +DEFINE_GUID(GUID_DEVINTERFACE_XINPUT, 0xec87f1e3, 0xc13b, 0x4100, 0xb5, 0xf7, 0x8b, 0x84, 0xd5, 0x42, 0x60, 0xcb); #if defined(__i386__) && !defined(_WIN32) @@ -165,6 +166,8 @@ static NTSTATUS WINAPI driver_add_device(DRIVER_OBJECT *driver, DEVICE_OBJECT *b ext->u.fdo.hid_ext.NextDeviceObject = bus_pdo; swprintf(ext->device_id, ARRAY_SIZE(ext->device_id), L"HID\\%s", wcsrchr(device_id, '\\') + 1); wcscpy(ext->instance_id, instance_id); + ext->interface_guid = !wcsncmp(device_id, L"XINPUT\\", 7) ? &GUID_DEVINTERFACE_XINPUT + : &GUID_DEVINTERFACE_HID; status = minidriver->AddDevice(minidriver->minidriver.DriverObject, fdo); if (status != STATUS_SUCCESS) @@ -220,6 +223,7 @@ static void create_child(minidriver *minidriver, DEVICE_OBJECT *fdo) KeInitializeSpinLock(&pdo_ext->u.pdo.irp_queue_lock); wcscpy(pdo_ext->device_id, fdo_ext->device_id); wcscpy(pdo_ext->instance_id, fdo_ext->instance_id); + pdo_ext->interface_guid = fdo_ext->interface_guid; pdo_ext->u.pdo.information.VendorID = attr.VendorID; pdo_ext->u.pdo.information.ProductID = attr.ProductID; @@ -445,7 +449,7 @@ static NTSTATUS pdo_pnp(DEVICE_OBJECT *device, IRP *irp) } case IRP_MN_START_DEVICE: - if ((status = IoRegisterDeviceInterface(device, &GUID_DEVINTERFACE_HID, NULL, &ext->u.pdo.link_name))) + if ((status = IoRegisterDeviceInterface(device, ext->interface_guid, NULL, &ext->u.pdo.link_name))) { ERR("Failed to register interface, status %#x.\n", status); break; diff --git a/dlls/xinput.sys/Makefile.in b/dlls/xinput.sys/Makefile.in index 52b00ef0528..f75c92a189f 100644 --- a/dlls/xinput.sys/Makefile.in +++ b/dlls/xinput.sys/Makefile.in @@ -1,5 +1,5 @@ MODULE = xinput.sys -IMPORTS = ntoskrnl +IMPORTS = ntoskrnl hidparse EXTRADLLFLAGS = -mno-cygwin -Wl,--subsystem,native C_SRCS = \ diff --git a/dlls/xinput.sys/main.c b/dlls/xinput.sys/main.c index b97ec49b325..aaf4b78dc17 100644 --- a/dlls/xinput.sys/main.c +++ b/dlls/xinput.sys/main.c @@ -31,6 +31,7 @@ #include "cfgmgr32.h" #include "ddk/wdm.h" #include "ddk/hidport.h" +#include "ddk/hidpddi.h" #include "wine/debug.h" @@ -52,17 +53,26 @@ struct device_state { DEVICE_OBJECT *bus_pdo; DEVICE_OBJECT *xinput_pdo; + DEVICE_OBJECT *internal_pdo; WCHAR bus_id[MAX_DEVICE_ID_LEN]; WCHAR instance_id[MAX_DEVICE_ID_LEN]; + + /* everything below requires holding the cs */ + CRITICAL_SECTION cs; + IRP *pending_read; + ULONG report_len; + char *report_buf; }; struct device_extension { BOOL is_fdo; + BOOL is_xinput; BOOL removed; WCHAR device_id[MAX_DEVICE_ID_LEN]; + struct device_state *state; }; @@ -71,12 +81,114 @@ static inline struct device_extension *ext_from_DEVICE_OBJECT(DEVICE_OBJECT *dev return (struct device_extension *)device->DeviceExtension; } +static NTSTATUS sync_ioctl(DEVICE_OBJECT *device, DWORD code, void *in_buf, DWORD in_len, void *out_buf, DWORD out_len) +{ + IO_STATUS_BLOCK io; + KEVENT event; + IRP *irp; + + KeInitializeEvent(&event, NotificationEvent, FALSE); + irp = IoBuildDeviceIoControlRequest(code, device, in_buf, in_len, out_buf, out_len, TRUE, &event, &io); + if (IoCallDriver(device, irp) == STATUS_PENDING) KeWaitForSingleObject(&event, Executive, KernelMode, FALSE, NULL); + return io.Status; +} + +static BOOL set_pending_read(struct device_state *state, IRP *irp) +{ + IRP *previous; + + RtlEnterCriticalSection(&state->cs); + if (!(previous = state->pending_read)) + { + state->pending_read = irp; + IoMarkIrpPending(irp); + } + RtlLeaveCriticalSection(&state->cs); + + return previous == NULL; +} + +static IRP *pop_pending_read(struct device_state *state) +{ + IRP *pending; + + RtlEnterCriticalSection(&state->cs); + pending = state->pending_read; + state->pending_read = NULL; + RtlLeaveCriticalSection(&state->cs); + + return pending; +} + +static NTSTATUS WINAPI xinput_ioctl(DEVICE_OBJECT *device, IRP *irp) +{ + struct device_extension *ext = ext_from_DEVICE_OBJECT(device); + IO_STACK_LOCATION *stack = IoGetCurrentIrpStackLocation(irp); + ULONG code = stack->Parameters.DeviceIoControl.IoControlCode; + struct device_state *state = ext->state; + + TRACE("device %p, irp %p, code %#x, bus_pdo %p.\n", device, irp, code, state->bus_pdo); + + switch (code) + { + case IOCTL_HID_READ_REPORT: + if (!set_pending_read(state, irp)) + { + ERR("another read IRP was already pending!\n"); + irp->IoStatus.Status = STATUS_UNSUCCESSFUL; + IoCompleteRequest(irp, IO_NO_INCREMENT); + return STATUS_UNSUCCESSFUL; + } + return STATUS_PENDING; + + case IOCTL_HID_GET_INPUT_REPORT: + { + HID_XFER_PACKET *packet = (HID_XFER_PACKET *)irp->UserBuffer; + + RtlEnterCriticalSection(&state->cs); + memcpy(packet->reportBuffer, state->report_buf, state->report_len); + irp->IoStatus.Information = state->report_len; + RtlLeaveCriticalSection(&state->cs); + + irp->IoStatus.Status = STATUS_SUCCESS; + IoCompleteRequest(irp, IO_NO_INCREMENT); + return STATUS_SUCCESS; + } + + default: + IoSkipCurrentIrpStackLocation(irp); + return IoCallDriver(state->bus_pdo, irp); + } + + return STATUS_SUCCESS; +} + +static NTSTATUS WINAPI hid_read_report_completion(DEVICE_OBJECT *device, IRP *irp, void *context) +{ + struct device_extension *ext = ext_from_DEVICE_OBJECT(device); + ULONG read_len = irp->IoStatus.Information; + struct device_state *state = ext->state; + char *read_buf = irp->UserBuffer; + + TRACE("device %p, irp %p, bus_pdo %p.\n", device, irp, state->bus_pdo); + + RtlEnterCriticalSection(&state->cs); + if (!state->report_buf) WARN("report buffer not created yet.\n"); + else if (read_len <= state->report_len) memcpy(state->report_buf, read_buf, read_len); + else ERR("report length mismatch %u, expected %u\n", read_len, state->report_len); + RtlLeaveCriticalSection(&state->cs); + + if (irp->PendingReturned) IoMarkIrpPending(irp); + return STATUS_SUCCESS; +} + static NTSTATUS WINAPI internal_ioctl(DEVICE_OBJECT *device, IRP *irp) { struct device_extension *ext = ext_from_DEVICE_OBJECT(device); IO_STACK_LOCATION *stack = IoGetCurrentIrpStackLocation(irp); ULONG code = stack->Parameters.DeviceIoControl.IoControlCode; struct device_state *state = ext->state; + IRP *pending; if (InterlockedOr(&ext->removed, FALSE)) { @@ -85,12 +197,86 @@ static NTSTATUS WINAPI internal_ioctl(DEVICE_OBJECT *device, IRP *irp) IoCompleteRequest(irp, IO_NO_INCREMENT); } + if (ext->is_xinput) return xinput_ioctl(device, irp); + TRACE("device %p, irp %p, code %#x, bus_pdo %p.\n", device, irp, code, state->bus_pdo); - IoSkipCurrentIrpStackLocation(irp); + if (code == IOCTL_HID_READ_REPORT) + { + /* we completed the previous internal read, send the fixed up report to xinput pdo */ + if ((pending = pop_pending_read(state))) + { + RtlEnterCriticalSection(&state->cs); + memcpy(pending->UserBuffer, state->report_buf, state->report_len); + pending->IoStatus.Information = state->report_len; + RtlLeaveCriticalSection(&state->cs); + + pending->IoStatus.Status = irp->IoStatus.Status; + IoCompleteRequest(pending, IO_NO_INCREMENT); + } + + IoCopyCurrentIrpStackLocationToNext(irp); + IoSetCompletionRoutine(irp, hid_read_report_completion, NULL, TRUE, FALSE, FALSE); + } + else IoSkipCurrentIrpStackLocation(irp); return IoCallDriver(state->bus_pdo, irp); } +static NTSTATUS xinput_pdo_start_device(DEVICE_OBJECT *device) +{ + struct device_extension *ext = ext_from_DEVICE_OBJECT(device); + struct device_state *state = ext->state; + PHIDP_REPORT_DESCRIPTOR report_desc; + PHIDP_PREPARSED_DATA preparsed; + HIDP_DEVICE_DESC device_desc; + HID_DESCRIPTOR hid_desc; + ULONG report_desc_len; + NTSTATUS status; + HIDP_CAPS caps; + + if ((status = sync_ioctl(state->bus_pdo, IOCTL_HID_GET_DEVICE_DESCRIPTOR, NULL, 0, &hid_desc, sizeof(hid_desc)))) + return status; + + if (!(report_desc_len = hid_desc.DescriptorList[0].wReportLength)) return STATUS_UNSUCCESSFUL; + if (!(report_desc = malloc(report_desc_len))) return STATUS_NO_MEMORY; + + status = sync_ioctl(state->bus_pdo, IOCTL_HID_GET_REPORT_DESCRIPTOR, NULL, 0, report_desc, report_desc_len); + if (!status) status = HidP_GetCollectionDescription(report_desc, report_desc_len, PagedPool, &device_desc); + free(report_desc); + if (status != HIDP_STATUS_SUCCESS) return status; + + preparsed = device_desc.CollectionDesc->PreparsedData; + status = HidP_GetCaps(preparsed, &caps); + if (status != HIDP_STATUS_SUCCESS) WARN("HidP_GetCaps returned %#x\n", status); + + status = STATUS_SUCCESS; + RtlEnterCriticalSection(&state->cs); + state->report_len = caps.InputReportByteLength; + if (!device_desc.ReportIDs[0].ReportID) state->report_len--; + if (!(state->report_buf = malloc(state->report_len))) status = STATUS_NO_MEMORY; + RtlLeaveCriticalSection(&state->cs); + + HidP_FreeCollectionDescription(&device_desc); + return status; +} + +static NTSTATUS xinput_pdo_remove_device(DEVICE_OBJECT *device) +{ + struct device_extension *ext = ext_from_DEVICE_OBJECT(device); + struct device_state *state = ext->state; + char *report_buf; + + RtlEnterCriticalSection(&state->cs); + report_buf = state->report_buf; + state->report_buf = NULL; + state->report_len = 0; + RtlLeaveCriticalSection(&state->cs); + + free(report_buf); + + return STATUS_SUCCESS; +} + static WCHAR *query_instance_id(DEVICE_OBJECT *device) { struct device_extension *ext = ext_from_DEVICE_OBJECT(device); @@ -152,21 +338,33 @@ static NTSTATUS WINAPI pdo_pnp(DEVICE_OBJECT *device, IRP *irp) struct device_state *state = ext->state; ULONG code = stack->MinorFunction; NTSTATUS status; + IRP *pending; TRACE("device %p, irp %p, code %#x, bus_pdo %p.\n", device, irp, code, state->bus_pdo); switch (code) { case IRP_MN_START_DEVICE: - status = STATUS_SUCCESS; + if (ext->is_xinput) status = xinput_pdo_start_device(device); + else status = STATUS_SUCCESS; break; case IRP_MN_SURPRISE_REMOVAL: status = STATUS_SUCCESS; if (InterlockedExchange(&ext->removed, TRUE)) break; + + if (!ext->is_xinput) break; + + if ((pending = pop_pending_read(state))) + { + pending->IoStatus.Status = STATUS_DELETE_PENDING; + pending->IoStatus.Information = 0; + IoCompleteRequest(pending, IO_NO_INCREMENT); + } break; case IRP_MN_REMOVE_DEVICE: + if (ext->is_xinput) xinput_pdo_remove_device(device); irp->IoStatus.Status = STATUS_SUCCESS; IoCompleteRequest(irp, IO_NO_INCREMENT); IoDeleteDevice(device); @@ -210,7 +408,7 @@ static void create_child_pdos(DEVICE_OBJECT *fdo) { struct device_extension *fdo_ext = fdo->DeviceExtension, *pdo_ext; struct device_state *state = fdo_ext->state; - DEVICE_OBJECT *xinput_pdo; + DEVICE_OBJECT *internal_pdo, *xinput_pdo; UNICODE_STRING string; WCHAR *tmp, pdo_name[255]; NTSTATUS status; @@ -223,16 +421,33 @@ static void create_child_pdos(DEVICE_OBJECT *fdo) return; } + swprintf(pdo_name, ARRAY_SIZE(pdo_name), L"\\Device\\XINPUT#%p&%p", fdo->DriverObject, state->bus_pdo); + RtlInitUnicodeString(&string, pdo_name); + if ((status = IoCreateDevice(fdo->DriverObject, sizeof(*pdo_ext), &string, 0, 0, FALSE, &internal_pdo))) + { + ERR( "failed to create internal PDO, status %#x.\n", status ); + IoDeleteDevice(xinput_pdo); + return; + } + pdo_ext = xinput_pdo->DeviceExtension; pdo_ext->is_fdo = FALSE; + pdo_ext->is_xinput = TRUE; pdo_ext->state = state; wcscpy(pdo_ext->device_id, fdo_ext->device_id); if ((tmp = wcsstr(pdo_ext->device_id, L"&MI_"))) memcpy(tmp, L"&IG", 6); else wcscat(pdo_ext->device_id, L"&IG_00"); + pdo_ext = internal_pdo->DeviceExtension; + pdo_ext->is_fdo = FALSE; + pdo_ext->is_xinput = FALSE; + pdo_ext->state = state; + swprintf(pdo_ext->device_id, MAX_DEVICE_ID_LEN, L"XINPUT\\%s", wcsrchr(fdo_ext->device_id, '\\') + 1); + state->xinput_pdo = xinput_pdo; + state->internal_pdo = internal_pdo; - TRACE("fdo %p, xinput PDO %p.\n", fdo, xinput_pdo); + TRACE("fdo %p, internal PDO %p, xinput PDO %p.\n", fdo, internal_pdo, xinput_pdo); IoInvalidateDeviceRelations(state->bus_pdo, BusRelations); } @@ -262,6 +477,12 @@ static NTSTATUS WINAPI fdo_pnp(DEVICE_OBJECT *device, IRP *irp) } devices->Count = 0; + if ((child = state->internal_pdo)) + { + devices->Objects[devices->Count] = child; + call_fastcall_func1(ObfReferenceObject, child); + devices->Count++; + } if ((child = state->xinput_pdo)) { devices->Objects[devices->Count] = child; @@ -373,6 +594,7 @@ static NTSTATUS WINAPI add_device(DRIVER_OBJECT *driver, DEVICE_OBJECT *bus_pdo) wcscpy(state->bus_id, bus_id); swprintf(ext->device_id, MAX_DEVICE_ID_LEN, L"%s\\%s", bus_id, device_id); wcscpy(state->instance_id, instance_id); + RtlInitializeCriticalSection(&state->cs); TRACE("fdo %p, bus %s, device %s, instance %s.\n", fdo, debugstr_w(state->bus_id), debugstr_w(ext->device_id), debugstr_w(state->instance_id)); -- 2.32.0
On 8/18/21 2:31 AM, Rémi Bernon wrote:
+static NTSTATUS WINAPI xinput_ioctl(DEVICE_OBJECT *device, IRP *irp)
Can we please keep "internal" in the name?
+{ + struct device_extension *ext = ext_from_DEVICE_OBJECT(device); + IO_STACK_LOCATION *stack = IoGetCurrentIrpStackLocation(irp); + ULONG code = stack->Parameters.DeviceIoControl.IoControlCode; + struct device_state *state = ext->state; + + TRACE("device %p, irp %p, code %#x, bus_pdo %p.\n", device, irp, code, state->bus_pdo); + + switch (code) + { + case IOCTL_HID_READ_REPORT: + if (!set_pending_read(state, irp)) + { + ERR("another read IRP was already pending!\n"); + irp->IoStatus.Status = STATUS_UNSUCCESSFUL; + IoCompleteRequest(irp, IO_NO_INCREMENT); + return STATUS_UNSUCCESSFUL;
Do we really need to handle this case? Can't we depend on hidclass acting sanely? At most I imagine printing an ERR is reasonable.
+ } + return STATUS_PENDING; + + case IOCTL_HID_GET_INPUT_REPORT: + { + HID_XFER_PACKET *packet = (HID_XFER_PACKET *)irp->UserBuffer; + + RtlEnterCriticalSection(&state->cs); + memcpy(packet->reportBuffer, state->report_buf, state->report_len); + irp->IoStatus.Information = state->report_len; + RtlLeaveCriticalSection(&state->cs); + + irp->IoStatus.Status = STATUS_SUCCESS; + IoCompleteRequest(irp, IO_NO_INCREMENT); + return STATUS_SUCCESS; + } + + default: + IoSkipCurrentIrpStackLocation(irp); + return IoCallDriver(state->bus_pdo, irp); + } + + return STATUS_SUCCESS; +} + +static NTSTATUS WINAPI hid_read_report_completion(DEVICE_OBJECT *device, IRP *irp, void *context) +{ + struct device_extension *ext = ext_from_DEVICE_OBJECT(device); + ULONG read_len = irp->IoStatus.Information; + struct device_state *state = ext->state; + char *read_buf = irp->UserBuffer; + + TRACE("device %p, irp %p, bus_pdo %p.\n", device, irp, state->bus_pdo); + + RtlEnterCriticalSection(&state->cs); + if (!state->report_buf) WARN("report buffer not created yet.\n"); + else if (read_len <= state->report_len) memcpy(state->report_buf, read_buf, read_len); + else ERR("report length mismatch %u, expected %u\n", read_len, state->report_len); + RtlLeaveCriticalSection(&state->cs); + + if (irp->PendingReturned) IoMarkIrpPending(irp); + return STATUS_SUCCESS; +} + static NTSTATUS WINAPI internal_ioctl(DEVICE_OBJECT *device, IRP *irp) { struct device_extension *ext = ext_from_DEVICE_OBJECT(device); IO_STACK_LOCATION *stack = IoGetCurrentIrpStackLocation(irp); ULONG code = stack->Parameters.DeviceIoControl.IoControlCode; struct device_state *state = ext->state; + IRP *pending;
if (InterlockedOr(&ext->removed, FALSE)) { @@ -85,12 +197,86 @@ static NTSTATUS WINAPI internal_ioctl(DEVICE_OBJECT *device, IRP *irp) IoCompleteRequest(irp, IO_NO_INCREMENT); }
+ if (ext->is_xinput) return xinput_ioctl(device, irp); + TRACE("device %p, irp %p, code %#x, bus_pdo %p.\n", device, irp, code, state->bus_pdo);
- IoSkipCurrentIrpStackLocation(irp); + if (code == IOCTL_HID_READ_REPORT) + { + /* we completed the previous internal read, send the fixed up report to xinput pdo */ + if ((pending = pop_pending_read(state))) + { + RtlEnterCriticalSection(&state->cs); + memcpy(pending->UserBuffer, state->report_buf, state->report_len); + pending->IoStatus.Information = state->report_len; + RtlLeaveCriticalSection(&state->cs); + + pending->IoStatus.Status = irp->IoStatus.Status; + IoCompleteRequest(pending, IO_NO_INCREMENT); + } + + IoCopyCurrentIrpStackLocationToNext(irp); + IoSetCompletionRoutine(irp, hid_read_report_completion, NULL, TRUE, FALSE, FALSE); + }
Doesn't this break if you get more than one ioctl on the "internal" PDO without any on the "xinput" PDO in between?
+ else IoSkipCurrentIrpStackLocation(irp); return IoCallDriver(state->bus_pdo, irp); }
...
@@ -152,21 +338,33 @@ static NTSTATUS WINAPI pdo_pnp(DEVICE_OBJECT *device, IRP *irp) struct device_state *state = ext->state; ULONG code = stack->MinorFunction; NTSTATUS status; + IRP *pending;
TRACE("device %p, irp %p, code %#x, bus_pdo %p.\n", device, irp, code, state->bus_pdo);
switch (code) { case IRP_MN_START_DEVICE: - status = STATUS_SUCCESS; + if (ext->is_xinput) status = xinput_pdo_start_device(device); + else status = STATUS_SUCCESS; break;
case IRP_MN_SURPRISE_REMOVAL: status = STATUS_SUCCESS; if (InterlockedExchange(&ext->removed, TRUE)) break; + + if (!ext->is_xinput) break; + + if ((pending = pop_pending_read(state))) + { + pending->IoStatus.Status = STATUS_DELETE_PENDING; + pending->IoStatus.Information = 0; + IoCompleteRequest(pending, IO_NO_INCREMENT); + }
Now that you're doing something nontrivial in your remove, using atomic functions to check "removed" is no longer thread-safe. In this case you can get a pending read queued after this IRP_MN_SURPRISE_REMOVAL returns.
On 8/23/21 6:58 PM, Zebediah Figura (she/her) wrote:
On 8/18/21 2:31 AM, Rémi Bernon wrote:
+static NTSTATUS WINAPI xinput_ioctl(DEVICE_OBJECT *device, IRP *irp)
Can we please keep "internal" in the name?
Sure, I may have to find a better name for the other "internal" PDO then, I named it xinput_ioctl as opposed to the other internal_ioctl.
+{ + struct device_extension *ext = ext_from_DEVICE_OBJECT(device); + IO_STACK_LOCATION *stack = IoGetCurrentIrpStackLocation(irp); + ULONG code = stack->Parameters.DeviceIoControl.IoControlCode; + struct device_state *state = ext->state; + + TRACE("device %p, irp %p, code %#x, bus_pdo %p.\n", device, irp, code, state->bus_pdo); + + switch (code) + { + case IOCTL_HID_READ_REPORT: + if (!set_pending_read(state, irp)) + { + ERR("another read IRP was already pending!\n"); + irp->IoStatus.Status = STATUS_UNSUCCESSFUL; + IoCompleteRequest(irp, IO_NO_INCREMENT); + return STATUS_UNSUCCESSFUL;
Do we really need to handle this case? Can't we depend on hidclass acting sanely? At most I imagine printing an ERR is reasonable.
Well, probably not, and now that I know hidclass.sys is supposed to check about everything about the HID report buffers I'm tempted to remove any kind of checks from the lower-level drivers. However, this means eventual bugs in hidclass.sys may then go unnoticed or trigger ugly crashes. I'm personally a fan of asserts for such cases, and I would find it better than an error message without error propagation, if that's alright?
+ } + return STATUS_PENDING; + + case IOCTL_HID_GET_INPUT_REPORT: + { + HID_XFER_PACKET *packet = (HID_XFER_PACKET *)irp->UserBuffer; + + RtlEnterCriticalSection(&state->cs); + memcpy(packet->reportBuffer, state->report_buf, state->report_len); + irp->IoStatus.Information = state->report_len; + RtlLeaveCriticalSection(&state->cs); + + irp->IoStatus.Status = STATUS_SUCCESS; + IoCompleteRequest(irp, IO_NO_INCREMENT); + return STATUS_SUCCESS; + } + + default: + IoSkipCurrentIrpStackLocation(irp); + return IoCallDriver(state->bus_pdo, irp); + } + + return STATUS_SUCCESS; +} + +static NTSTATUS WINAPI hid_read_report_completion(DEVICE_OBJECT *device, IRP *irp, void *context) +{ + struct device_extension *ext = ext_from_DEVICE_OBJECT(device); + ULONG read_len = irp->IoStatus.Information; + struct device_state *state = ext->state; + char *read_buf = irp->UserBuffer; + + TRACE("device %p, irp %p, bus_pdo %p.\n", device, irp, state->bus_pdo); + + RtlEnterCriticalSection(&state->cs); + if (!state->report_buf) WARN("report buffer not created yet.\n"); + else if (read_len <= state->report_len) memcpy(state->report_buf, read_buf, read_len); + else ERR("report length mismatch %u, expected %u\n", read_len, state->report_len); + RtlLeaveCriticalSection(&state->cs); + + if (irp->PendingReturned) IoMarkIrpPending(irp); + return STATUS_SUCCESS; +} + static NTSTATUS WINAPI internal_ioctl(DEVICE_OBJECT *device, IRP *irp) { struct device_extension *ext = ext_from_DEVICE_OBJECT(device); IO_STACK_LOCATION *stack = IoGetCurrentIrpStackLocation(irp); ULONG code = stack->Parameters.DeviceIoControl.IoControlCode; struct device_state *state = ext->state; + IRP *pending; if (InterlockedOr(&ext->removed, FALSE)) { @@ -85,12 +197,86 @@ static NTSTATUS WINAPI internal_ioctl(DEVICE_OBJECT *device, IRP *irp) IoCompleteRequest(irp, IO_NO_INCREMENT); } + if (ext->is_xinput) return xinput_ioctl(device, irp); + TRACE("device %p, irp %p, code %#x, bus_pdo %p.\n", device, irp, code, state->bus_pdo); - IoSkipCurrentIrpStackLocation(irp); + if (code == IOCTL_HID_READ_REPORT) + { + /* we completed the previous internal read, send the fixed up report to xinput pdo */ + if ((pending = pop_pending_read(state))) + { + RtlEnterCriticalSection(&state->cs); + memcpy(pending->UserBuffer, state->report_buf, state->report_len); + pending->IoStatus.Information = state->report_len; + RtlLeaveCriticalSection(&state->cs); + + pending->IoStatus.Status = irp->IoStatus.Status; + IoCompleteRequest(pending, IO_NO_INCREMENT); + } + + IoCopyCurrentIrpStackLocationToNext(irp); + IoSetCompletionRoutine(irp, hid_read_report_completion, NULL, TRUE, FALSE, FALSE); + }
Doesn't this break if you get more than one ioctl on the "internal" PDO without any on the "xinput" PDO in between?
I'm not sure to see how? The "internal" read will just complete normally and skip notifying the xinput PDO, which will possibly miss a report but hopefully will catch up on the next read. If you mean the two should run in lock-step so that xinput never misses a report, then it will need to be done differently.
+ else IoSkipCurrentIrpStackLocation(irp); return IoCallDriver(state->bus_pdo, irp); }
...
@@ -152,21 +338,33 @@ static NTSTATUS WINAPI pdo_pnp(DEVICE_OBJECT *device, IRP *irp) struct device_state *state = ext->state; ULONG code = stack->MinorFunction; NTSTATUS status; + IRP *pending; TRACE("device %p, irp %p, code %#x, bus_pdo %p.\n", device, irp, code, state->bus_pdo); switch (code) { case IRP_MN_START_DEVICE: - status = STATUS_SUCCESS; + if (ext->is_xinput) status = xinput_pdo_start_device(device); + else status = STATUS_SUCCESS; break; case IRP_MN_SURPRISE_REMOVAL: status = STATUS_SUCCESS; if (InterlockedExchange(&ext->removed, TRUE)) break; + + if (!ext->is_xinput) break; + + if ((pending = pop_pending_read(state))) + { + pending->IoStatus.Status = STATUS_DELETE_PENDING; + pending->IoStatus.Information = 0; + IoCompleteRequest(pending, IO_NO_INCREMENT); + }
Now that you're doing something nontrivial in your remove, using atomic functions to check "removed" is no longer thread-safe. In this case you can get a pending read queued after this IRP_MN_SURPRISE_REMOVAL returns.
Yeah of course... -- Rémi Bernon <rbernon(a)codeweavers.com>
On 8/23/21 12:16 PM, Rémi Bernon wrote:
+{ + struct device_extension *ext = ext_from_DEVICE_OBJECT(device); + IO_STACK_LOCATION *stack = IoGetCurrentIrpStackLocation(irp); + ULONG code = stack->Parameters.DeviceIoControl.IoControlCode; + struct device_state *state = ext->state; + + TRACE("device %p, irp %p, code %#x, bus_pdo %p.\n", device, irp, code, state->bus_pdo); + + switch (code) + { + case IOCTL_HID_READ_REPORT: + if (!set_pending_read(state, irp)) + { + ERR("another read IRP was already pending!\n"); + irp->IoStatus.Status = STATUS_UNSUCCESSFUL; + IoCompleteRequest(irp, IO_NO_INCREMENT); + return STATUS_UNSUCCESSFUL;
Do we really need to handle this case? Can't we depend on hidclass acting sanely? At most I imagine printing an ERR is reasonable.
Well, probably not, and now that I know hidclass.sys is supposed to check about everything about the HID report buffers I'm tempted to remove any kind of checks from the lower-level drivers.
However, this means eventual bugs in hidclass.sys may then go unnoticed or trigger ugly crashes. I'm personally a fan of asserts for such cases, and I would find it better than an error message without error propagation, if that's alright?
An assert sounds reasonable to me too, yeah.
+ } + return STATUS_PENDING; + + case IOCTL_HID_GET_INPUT_REPORT: + { + HID_XFER_PACKET *packet = (HID_XFER_PACKET *)irp->UserBuffer; + + RtlEnterCriticalSection(&state->cs); + memcpy(packet->reportBuffer, state->report_buf, state->report_len); + irp->IoStatus.Information = state->report_len; + RtlLeaveCriticalSection(&state->cs); + + irp->IoStatus.Status = STATUS_SUCCESS; + IoCompleteRequest(irp, IO_NO_INCREMENT); + return STATUS_SUCCESS; + } + + default: + IoSkipCurrentIrpStackLocation(irp); + return IoCallDriver(state->bus_pdo, irp); + } + + return STATUS_SUCCESS; +} + +static NTSTATUS WINAPI hid_read_report_completion(DEVICE_OBJECT *device, IRP *irp, void *context) +{ + struct device_extension *ext = ext_from_DEVICE_OBJECT(device); + ULONG read_len = irp->IoStatus.Information; + struct device_state *state = ext->state; + char *read_buf = irp->UserBuffer; + + TRACE("device %p, irp %p, bus_pdo %p.\n", device, irp, state->bus_pdo); + + RtlEnterCriticalSection(&state->cs); + if (!state->report_buf) WARN("report buffer not created yet.\n"); + else if (read_len <= state->report_len) memcpy(state->report_buf, read_buf, read_len); + else ERR("report length mismatch %u, expected %u\n", read_len, state->report_len); + RtlLeaveCriticalSection(&state->cs); + + if (irp->PendingReturned) IoMarkIrpPending(irp); + return STATUS_SUCCESS; +} + static NTSTATUS WINAPI internal_ioctl(DEVICE_OBJECT *device, IRP *irp) { struct device_extension *ext = ext_from_DEVICE_OBJECT(device); IO_STACK_LOCATION *stack = IoGetCurrentIrpStackLocation(irp); ULONG code = stack->Parameters.DeviceIoControl.IoControlCode; struct device_state *state = ext->state; + IRP *pending; if (InterlockedOr(&ext->removed, FALSE)) { @@ -85,12 +197,86 @@ static NTSTATUS WINAPI internal_ioctl(DEVICE_OBJECT *device, IRP *irp) IoCompleteRequest(irp, IO_NO_INCREMENT); } + if (ext->is_xinput) return xinput_ioctl(device, irp); + TRACE("device %p, irp %p, code %#x, bus_pdo %p.\n", device, irp, code, state->bus_pdo); - IoSkipCurrentIrpStackLocation(irp); + if (code == IOCTL_HID_READ_REPORT) + { + /* we completed the previous internal read, send the fixed up report to xinput pdo */ + if ((pending = pop_pending_read(state))) + { + RtlEnterCriticalSection(&state->cs); + memcpy(pending->UserBuffer, state->report_buf, state->report_len); + pending->IoStatus.Information = state->report_len; + RtlLeaveCriticalSection(&state->cs); + + pending->IoStatus.Status = irp->IoStatus.Status; + IoCompleteRequest(pending, IO_NO_INCREMENT); + } + + IoCopyCurrentIrpStackLocationToNext(irp); + IoSetCompletionRoutine(irp, hid_read_report_completion, NULL, TRUE, FALSE, FALSE); + }
Doesn't this break if you get more than one ioctl on the "internal" PDO without any on the "xinput" PDO in between?
I'm not sure to see how? The "internal" read will just complete normally and skip notifying the xinput PDO, which will possibly miss a report but hopefully will catch up on the next read.
If you mean the two should run in lock-step so that xinput never misses a report, then it will need to be done differently.
I'm not familiar with the way hidclass works, but I'm assuming that it needs to see "every" report for both devices? If not, I'm not sure why we need to bother waiting for the "xinput" ioctl in order to complete the "internal" ioctl.
On 8/23/21 7:23 PM, Zebediah Figura (she/her) wrote:
On 8/23/21 12:16 PM, Rémi Bernon wrote:
+{ + struct device_extension *ext = ext_from_DEVICE_OBJECT(device); + IO_STACK_LOCATION *stack = IoGetCurrentIrpStackLocation(irp); + ULONG code = stack->Parameters.DeviceIoControl.IoControlCode; + struct device_state *state = ext->state; + + TRACE("device %p, irp %p, code %#x, bus_pdo %p.\n", device, irp, code, state->bus_pdo); + + switch (code) + { + case IOCTL_HID_READ_REPORT: + if (!set_pending_read(state, irp)) + { + ERR("another read IRP was already pending!\n"); + irp->IoStatus.Status = STATUS_UNSUCCESSFUL; + IoCompleteRequest(irp, IO_NO_INCREMENT); + return STATUS_UNSUCCESSFUL;
Do we really need to handle this case? Can't we depend on hidclass acting sanely? At most I imagine printing an ERR is reasonable.
Well, probably not, and now that I know hidclass.sys is supposed to check about everything about the HID report buffers I'm tempted to remove any kind of checks from the lower-level drivers.
However, this means eventual bugs in hidclass.sys may then go unnoticed or trigger ugly crashes. I'm personally a fan of asserts for such cases, and I would find it better than an error message without error propagation, if that's alright?
An assert sounds reasonable to me too, yeah.
+ } + return STATUS_PENDING; + + case IOCTL_HID_GET_INPUT_REPORT: + { + HID_XFER_PACKET *packet = (HID_XFER_PACKET *)irp->UserBuffer; + + RtlEnterCriticalSection(&state->cs); + memcpy(packet->reportBuffer, state->report_buf, state->report_len); + irp->IoStatus.Information = state->report_len; + RtlLeaveCriticalSection(&state->cs); + + irp->IoStatus.Status = STATUS_SUCCESS; + IoCompleteRequest(irp, IO_NO_INCREMENT); + return STATUS_SUCCESS; + } + + default: + IoSkipCurrentIrpStackLocation(irp); + return IoCallDriver(state->bus_pdo, irp); + } + + return STATUS_SUCCESS; +} + +static NTSTATUS WINAPI hid_read_report_completion(DEVICE_OBJECT *device, IRP *irp, void *context) +{ + struct device_extension *ext = ext_from_DEVICE_OBJECT(device); + ULONG read_len = irp->IoStatus.Information; + struct device_state *state = ext->state; + char *read_buf = irp->UserBuffer; + + TRACE("device %p, irp %p, bus_pdo %p.\n", device, irp, state->bus_pdo); + + RtlEnterCriticalSection(&state->cs); + if (!state->report_buf) WARN("report buffer not created yet.\n"); + else if (read_len <= state->report_len) memcpy(state->report_buf, read_buf, read_len); + else ERR("report length mismatch %u, expected %u\n", read_len, state->report_len); + RtlLeaveCriticalSection(&state->cs); + + if (irp->PendingReturned) IoMarkIrpPending(irp); + return STATUS_SUCCESS; +} + static NTSTATUS WINAPI internal_ioctl(DEVICE_OBJECT *device, IRP *irp) { struct device_extension *ext = ext_from_DEVICE_OBJECT(device); IO_STACK_LOCATION *stack = IoGetCurrentIrpStackLocation(irp); ULONG code = stack->Parameters.DeviceIoControl.IoControlCode; struct device_state *state = ext->state; + IRP *pending; if (InterlockedOr(&ext->removed, FALSE)) { @@ -85,12 +197,86 @@ static NTSTATUS WINAPI internal_ioctl(DEVICE_OBJECT *device, IRP *irp) IoCompleteRequest(irp, IO_NO_INCREMENT); } + if (ext->is_xinput) return xinput_ioctl(device, irp); + TRACE("device %p, irp %p, code %#x, bus_pdo %p.\n", device, irp, code, state->bus_pdo); - IoSkipCurrentIrpStackLocation(irp); + if (code == IOCTL_HID_READ_REPORT) + { + /* we completed the previous internal read, send the fixed up report to xinput pdo */ + if ((pending = pop_pending_read(state))) + { + RtlEnterCriticalSection(&state->cs); + memcpy(pending->UserBuffer, state->report_buf, state->report_len); + pending->IoStatus.Information = state->report_len; + RtlLeaveCriticalSection(&state->cs); + + pending->IoStatus.Status = irp->IoStatus.Status; + IoCompleteRequest(pending, IO_NO_INCREMENT); + } + + IoCopyCurrentIrpStackLocationToNext(irp); + IoSetCompletionRoutine(irp, hid_read_report_completion, NULL, TRUE, FALSE, FALSE); + }
Doesn't this break if you get more than one ioctl on the "internal" PDO without any on the "xinput" PDO in between?
I'm not sure to see how? The "internal" read will just complete normally and skip notifying the xinput PDO, which will possibly miss a report but hopefully will catch up on the next read.
If you mean the two should run in lock-step so that xinput never misses a report, then it will need to be done differently.
I'm not familiar with the way hidclass works, but I'm assuming that it needs to see "every" report for both devices? If not, I'm not sure why we need to bother waiting for the "xinput" ioctl in order to complete the "internal" ioctl.
Well, each report contains the full state of the device, hidclass.sys just pass them to the clients and doesn't care about missing reports. The way it's done here it is possible that the "internal" PDO will get more reports than the "xinput" PDO, and applications reading the "xinput" devices may then miss some input events as a consequence. In general I think we should avoid as much as possible cases where reports could be missed or dropped, to avoid missing input events. Some people already reported that sometimes Wine misses a button press for instance. (FWIW I suspect it may come from winebus.sys current design, which already makes it possible to miss reports, and which I'm planning to fix at some point.) There's a ring-buffer for every reader with a user-space configurable size in hidclass.sys and I believe that's the only place were we're allowed to drop a report. Then, in this case I considered that the bogus "xinput" devices were already bogus so didn't deserve much effort, but as some code seems to rely on them maybe we should avoid losing reports in all cases. And so we'll have to make both run in lock-step. -- Rémi Bernon <rbernon(a)codeweavers.com>
Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/xinput1_3/main.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/dlls/xinput1_3/main.c b/dlls/xinput1_3/main.c index 4770b64681e..bc5d91109b0 100644 --- a/dlls/xinput1_3/main.c +++ b/dlls/xinput1_3/main.c @@ -43,6 +43,8 @@ #include "wine/debug.h" +DEFINE_GUID(GUID_DEVINTERFACE_XINPUT, 0xec87f1e3, 0xc13b, 0x4100, 0xb5, 0xf7, 0x8b, 0x84, 0xd5, 0x42, 0x60, 0xcb); + /* Not defined in the headers, used only by XInputGetStateEx */ #define XINPUT_GAMEPAD_GUIDE 0x0400 @@ -324,7 +326,7 @@ static void update_controller_list(void) GUID guid; int i; - HidD_GetHidGuid(&guid); + guid = GUID_DEVINTERFACE_XINPUT; set = SetupDiGetClassDevsW(&guid, NULL, NULL, DIGCF_DEVICEINTERFACE | DIGCF_PRESENT); detail->cbSize = sizeof(*detail); @@ -335,8 +337,6 @@ static void update_controller_list(void) if (!SetupDiGetDeviceInterfaceDetailW(set, &iface, detail, sizeof(buffer), NULL, NULL)) continue; - if (!wcsstr(detail->DevicePath, L"IG_")) continue; - if (find_opened_device(detail, &i)) continue; /* already opened */ if (i == XUSER_MAX_COUNT) break; /* no more slots */ -- 2.32.0
On 8/18/21 2:31 AM, Rémi Bernon wrote:
And match it in winehid.sys instead of individual bus ids.
Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> ---
This series should apply smoothly with the previous winebus.sys. I'm also sending it now because it may need a bit more consideration. The rationale of the design is described below:
The idea here is to create a new xinput.sys driver, which will match xinput-compatible winebus.sys devices, and create two device nodes on top of the underlying device.
The first one will be the public "&IG_00" XInput bogus HID gamepad, as native has, and as Win32 SDL expects to see.
The other will be a pass-through to the underlying device, but although it's going to be an HID compatible device too, it will need to be exposed on a different interface class GUID in order to be used only by Wine XInput.
The second node will filter its read requests, and then translate them into the corresponding bogus XInput HID report, passing them to the first one for its read request completion.
This design removes the need to really care about XInput gamepad compat in winebus.sys, making it possible to extend the HID reports there to include the full capabilities of the unix devices:
* It will allow the user-space XInput implementation to have access to the full capabilities of the underlying device, and not be limited by the bogus HID compatibility anymore.
* It also removes an eventual need to duplicate the winebus devices, as the two nodes will only need one underlying device (the bogus XInput node will never actually request reads to its underlying PDO).
* And finally it makes it possible to have XInput compatibility with any underlying device, not just with SDL or evdev where we handle the report creation. Even hidraw or iohid device reports can be translated to the XInput HID reports.
I have a few questions: * How will dinput (or winmm) expose these devices? * How will we expose composite vs. non-composite devices from winebus? * What potential need is there to duplicate winebus devices that you refer to? As far as I understand we don't currently duplicate anything. * And perhaps more generally: from my time working on Proton, although I haven't worked on controller support recently, I vaguely understand/remember that some games are very picky about what is and isn't reported as an xinput device, and also whether the same device is reported via dinput. Would it be possible for you to summarize the relevant problems there, so that we have that additional context? * In this patch specifically, is there any reason to keep the old hardware IDs around?
On 8/18/21 5:52 PM, Zebediah Figura (she/her) wrote:
I have a few questions:
* How will dinput (or winmm) expose these devices?
For dinput, it should enumerate the HID device interface class, so for XInput compatible devices, it will enumerate the bogus "&IG_" devices, in addition to non-XInput compatible devices. Applications are expected to check this marker to de-duplicate the XInput gamepads, as described in [1]. I don't know about winmm, but if it can be implemented on top of HID then it'll be the same. Both will then be limited by the XInput bogus HID support, like on Windows. And they won't have force-feedback support for them for instance (although XInput, using the internal device, will have it). We could decide to overcome that limitation by enumerating the XINPUT device interface class, but I don't think it's how it's supposed to work.
* How will we expose composite vs. non-composite devices from winebus?
I don't know, is there going to be any? On winebus.sys I think composite devices are just going to be enumerated on the unix bus level as separate devices, with different "input" numbers. So we should see a "&MI_00", "&MI_01", "&MI_02" winebus.sys device and so on. If one of those is supposed to be XInput compatible, the xinput.sys driver should match the compatible ID, and take it over instead of winehid.sys, create a corresponding "&IG_00" (or "&IG_01", etc) HID device, and an internal device on the XINPUT\ bus.
* What potential need is there to duplicate winebus devices that you refer to? As far as I understand we don't currently duplicate anything.
We don't, but then we have to decide whether a device is exposed as a "mapped" gamepad, or not. The "mapped" version is supposed to mimic the XInput bogus HID devices, in order for applications to be happy when the look at the HID capabilities and reports. Because of this, the "mapped" devices are limited, possibly not exposing all their axes and buttons in the HID report descriptor, and changing anything there may break applications. Duplicating the devices, or forking them like done here, let us have a full featured HID device for internal use, while exposing the same bogus thing native exposes publicly (and not use it ourselves).
* And perhaps more generally: from my time working on Proton, although I haven't worked on controller support recently, I vaguely understand/remember that some games are very picky about what is and isn't reported as an xinput device, and also whether the same device is reported via dinput. Would it be possible for you to summarize the relevant problems there, so that we have that additional context?
Well as far as I could see there's all sorts of issues, starting with SDL enabling various hacks when they detect XInput devices. I believe they support the XInput API, but not only, and they also have support for the XInput HID devices through WM_INPUT messages. They are trying very hard to support their bogus HID reports, and I think we can expect other input middleware to do as well. More specifically, they try to workaround two issues: * The guide button of the XBox controller, which is not supposed to be exposed on the bogus HID reports, but should be obviously supported internally by XInput. * The two triggers axes, which are supposed to be combined together in the bogus HID report, in a single Z axis. This allows compatibility for XBox gamepads with old games still using DInput, and where the triggers were effectively combined in a single DInput axis. However, XInput needs to have the two separate triggers internally, so we already report them separately, as an incompatible hack, and implementing our DInput on top of that would break such old games which depend on the triggers to be combined.
* In this patch specifically, is there any reason to keep the old hardware IDs around?
The internal bus hardware IDs are probably not really useful, and I intend to replace them with a common "WINEBUS\VID_xxxx&PID_xxxx" hardware IDs later on, in order to match these in xinput.sys and remove the need to check for XBox gamepad VID/PID in winebus.sys. The internal bus names in the device IDs may be helpful to determine which bus a device comes from, although I don't really think it matters. I was kind of going to keep them around though. [1] https://docs.microsoft.com/en-us/windows/win32/xinput/xinput-and-directinput -- Rémi Bernon <rbernon(a)codeweavers.com>
On 8/18/21 11:50 AM, Rémi Bernon wrote:
On 8/18/21 5:52 PM, Zebediah Figura (she/her) wrote:
I have a few questions:
* How will dinput (or winmm) expose these devices?
For dinput, it should enumerate the HID device interface class, so for XInput compatible devices, it will enumerate the bogus "&IG_" devices, in addition to non-XInput compatible devices. Applications are expected to check this marker to de-duplicate the XInput gamepads, as described in [1].
I don't know about winmm, but if it can be implemented on top of HID then it'll be the same.
Both will then be limited by the XInput bogus HID support, like on Windows. And they won't have force-feedback support for them for instance (although XInput, using the internal device, will have it).
We could decide to overcome that limitation by enumerating the XINPUT device interface class, but I don't think it's how it's supposed to work.
* How will we expose composite vs. non-composite devices from winebus?
I don't know, is there going to be any?
On winebus.sys I think composite devices are just going to be enumerated on the unix bus level as separate devices, with different "input" numbers. So we should see a "&MI_00", "&MI_01", "&MI_02" winebus.sys device and so on.
If one of those is supposed to be XInput compatible, the xinput.sys driver should match the compatible ID, and take it over instead of winehid.sys, create a corresponding "&IG_00" (or "&IG_01", etc) HID device, and an internal device on the XINPUT\ bus.
I think I need some further clarification, sorry. By "composite" I rather mean HID devices which comprise a single interface on a composite USB device, as opposed to a HID device which comprises an entire non-composite USB device (or a non-USB device, I guess.) Actually, I think I'm missing even more basic context. Is xinput exposing HID devices? Where does hidclass fit in for xinput-compatible gamepads? As far as I can tell your xinput.sys is attaching to winebus, and it isn't actually exposing any HID devices. It's just exposing the "internal" HID device for xinput, but it's breaking anything else. I.e. it creates a device named e.g. HIDRAW\VID_####&PID_####&IG_##, but it doesn't actually register any interfaces for the device, or handle HID ioctls.
On 8/18/21 11:02 PM, Zebediah Figura (she/her) wrote:
On 8/18/21 11:50 AM, Rémi Bernon wrote:
On 8/18/21 5:52 PM, Zebediah Figura (she/her) wrote: I don't know, is there going to be any?
On winebus.sys I think composite devices are just going to be enumerated on the unix bus level as separate devices, with different "input" numbers. So we should see a "&MI_00", "&MI_01", "&MI_02" winebus.sys device and so on.
If one of those is supposed to be XInput compatible, the xinput.sys driver should match the compatible ID, and take it over instead of winehid.sys, create a corresponding "&IG_00" (or "&IG_01", etc) HID device, and an internal device on the XINPUT\ bus.
I think I need some further clarification, sorry.
By "composite" I rather mean HID devices which comprise a single interface on a composite USB device, as opposed to a HID device which comprises an entire non-composite USB device (or a non-USB device, I guess.)
Well, like I said, I believe the interfaces are exposed by winebus.sys as separate devices.
Actually, I think I'm missing even more basic context. Is xinput exposing HID devices? Where does hidclass fit in for xinput-compatible gamepads?
It exposes the xinput PDO as the same kind of device as winebus.sys devices, and the internal PDO as well. Both return a WINEBUS\WINE_COMP_HID compatible ID, so winehid.sys will match and handle both. Then winehid.sys registers with hidclass.sys as usual, which creates the corresponding FDO and PDO, and starts a device thread for each. However, the internal PDO created from xinput.sys is only an HID device for convenience, and it should not be enumerated as the other HID devices. So in order to do that, xinput.sys returns a XINPUT\ prefixed device ID for this PDO only, and hidclass.sys knows that it needs to register its HID PDO on the XINPUT device interface class (first 3 hunks of PATCH 4).
As far as I can tell your xinput.sys is attaching to winebus, and it isn't actually exposing any HID devices. It's just exposing the "internal" HID device for xinput, but it's breaking anything else. I.e. it creates a device named e.g. HIDRAW\VID_####&PID_####&IG_##, but it doesn't actually register any interfaces for the device, or handle HID ioctls.
It's working surprisingly very well then ;) (and I made sure it worked on every single patch). The internal PDO is mostly just a pass-through, and the HID ioctls are still handled by the underlying winebus.sys device. It has to intercept the read requests through, so we can translate them and complete the corresponding xinput PDO read requests (which doesn't pass them through). (This description is valid for after PATCH 4, where the internal PDO is created, before it the xinput PDO acts as the pass-through device). -- Rémi Bernon <rbernon(a)codeweavers.com>
On 8/18/21 4:40 PM, Rémi Bernon wrote:
On 8/18/21 11:02 PM, Zebediah Figura (she/her) wrote:
On 8/18/21 11:50 AM, Rémi Bernon wrote:
On 8/18/21 5:52 PM, Zebediah Figura (she/her) wrote: I don't know, is there going to be any?
On winebus.sys I think composite devices are just going to be enumerated on the unix bus level as separate devices, with different "input" numbers. So we should see a "&MI_00", "&MI_01", "&MI_02" winebus.sys device and so on.
If one of those is supposed to be XInput compatible, the xinput.sys driver should match the compatible ID, and take it over instead of winehid.sys, create a corresponding "&IG_00" (or "&IG_01", etc) HID device, and an internal device on the XINPUT\ bus.
I think I need some further clarification, sorry.
By "composite" I rather mean HID devices which comprise a single interface on a composite USB device, as opposed to a HID device which comprises an entire non-composite USB device (or a non-USB device, I guess.)
Well, like I said, I believe the interfaces are exposed by winebus.sys as separate devices.
Actually, I think I'm missing even more basic context. Is xinput exposing HID devices? Where does hidclass fit in for xinput-compatible gamepads?
It exposes the xinput PDO as the same kind of device as winebus.sys devices, and the internal PDO as well.
Both return a WINEBUS\WINE_COMP_HID compatible ID, so winehid.sys will match and handle both.
Then winehid.sys registers with hidclass.sys as usual, which creates the corresponding FDO and PDO, and starts a device thread for each.
However, the internal PDO created from xinput.sys is only an HID device for convenience, and it should not be enumerated as the other HID devices.
So in order to do that, xinput.sys returns a XINPUT\ prefixed device ID for this PDO only, and hidclass.sys knows that it needs to register its HID PDO on the XINPUT device interface class (first 3 hunks of PATCH 4).
As far as I can tell your xinput.sys is attaching to winebus, and it isn't actually exposing any HID devices. It's just exposing the "internal" HID device for xinput, but it's breaking anything else. I.e. it creates a device named e.g. HIDRAW\VID_####&PID_####&IG_##, but it doesn't actually register any interfaces for the device, or handle HID ioctls.
It's working surprisingly very well then ;) (and I made sure it worked on every single patch).
The internal PDO is mostly just a pass-through, and the HID ioctls are still handled by the underlying winebus.sys device.
It has to intercept the read requests through, so we can translate them and complete the corresponding xinput PDO read requests (which doesn't pass them through).
(This description is valid for after PATCH 4, where the internal PDO is created, before it the xinput PDO acts as the pass-through device).
Ah, I see what I was missing. So it's kind of a poor man's filter DO, I guess. That is, it effectively inserts itself into the device stack between winebus and winehid, and exposes the HID interface from winebus directly using an internal GUID, while reporting a different HID descriptor (and report data, etc.) to winehid? I must still be missing something, though, because if that's the case I don't see why you would need to touch hidclass at all. I guess the only reason we can't do this all from winehid.sys is because hidclass doesn't let anyone open the device directly, right?
On 8/18/21 11:51 PM, Zebediah Figura (she/her) wrote:
Ah, I see what I was missing. So it's kind of a poor man's filter DO, I guess. That is, it effectively inserts itself into the device stack between winebus and winehid, and exposes the HID interface from winebus directly using an internal GUID, while reporting a different HID descriptor (and report data, etc.) to winehid?
Yes. I quickly looked at filter drivers but didn't find anything explicit and it looked like we don't have the necessary bricks to make it work. And I really don't think it's worth the trouble for something as simple as this driver.
I must still be missing something, though, because if that's the case I don't see why you would need to touch hidclass at all.
I guess the only reason we can't do this all from winehid.sys is because hidclass doesn't let anyone open the device directly, right?
Well I don't know how to tell hidclass.sys not register the device on the HID GUID, and register it instead on another GUID without changing hidclass.sys. And yes, opening any HID device goes through hidclass.sys and we really want it to be on top and handle all the ugly HID details (and it also guarantees that we will only ever have a single read request at a time, which will be handy). -- Rémi Bernon <rbernon(a)codeweavers.com>
On 8/18/21 5:04 PM, Rémi Bernon wrote:
On 8/18/21 11:51 PM, Zebediah Figura (she/her) wrote:
Ah, I see what I was missing. So it's kind of a poor man's filter DO, I guess. That is, it effectively inserts itself into the device stack between winebus and winehid, and exposes the HID interface from winebus directly using an internal GUID, while reporting a different HID descriptor (and report data, etc.) to winehid?
Yes. I quickly looked at filter drivers but didn't find anything explicit and it looked like we don't have the necessary bricks to make it work. And I really don't think it's worth the trouble for something as simple as this driver.
Filter drivers wouldn't help anyway, because of the mentioned problem with IRP_MJ_CREATE. As far as I know, if we could do it with a filter driver, we could do it with winehid.
I must still be missing something, though, because if that's the case I don't see why you would need to touch hidclass at all.
I guess the only reason we can't do this all from winehid.sys is because hidclass doesn't let anyone open the device directly, right?
Well I don't know how to tell hidclass.sys not register the device on the HID GUID, and register it instead on another GUID without changing hidclass.sys.
And yes, opening any HID device goes through hidclass.sys and we really want it to be on top and handle all the ugly HID details (and it also guarantees that we will only ever have a single read request at a time, which will be handy).
Mmh, I see the problem now. Hmm. I guess we really do want both the real and xinput devices to go through winehid + hidclass. It should come as no surprise that the hidclass hack isn't great. But I can't particularly hate it, because this solution is in general about as close as we can get to native architecture. I'm almost tempted to propose shoving large parts of hidclass into xinput and/or xinput.sys (making use of that new hidparse, probably—by the way, could that be a static library instead of using PARENTSRC?) but it's probably too much work to be worthwhile. How bad would it be if we exposed the real device using GUID_DEVINTERFACE_HID anyway? Would it be worse than duplicate devices in some games? Not that that's particularly desirable to begin with, granted. Otherwise I'll probably start reviewing the actual contents of this patch series instead of trying to wrap my head around what it's actually doing ;-)
On 8/19/21 5:29 AM, Zebediah Figura (she/her) wrote:
On 8/18/21 5:04 PM, Rémi Bernon wrote:
On 8/18/21 11:51 PM, Zebediah Figura (she/her) wrote:
Ah, I see what I was missing. So it's kind of a poor man's filter DO, I guess. That is, it effectively inserts itself into the device stack between winebus and winehid, and exposes the HID interface from winebus directly using an internal GUID, while reporting a different HID descriptor (and report data, etc.) to winehid?
Yes. I quickly looked at filter drivers but didn't find anything explicit and it looked like we don't have the necessary bricks to make it work. And I really don't think it's worth the trouble for something as simple as this driver.
Filter drivers wouldn't help anyway, because of the mentioned problem with IRP_MJ_CREATE. As far as I know, if we could do it with a filter driver, we could do it with winehid.
I must still be missing something, though, because if that's the case I don't see why you would need to touch hidclass at all.
I guess the only reason we can't do this all from winehid.sys is because hidclass doesn't let anyone open the device directly, right?
Well I don't know how to tell hidclass.sys not register the device on the HID GUID, and register it instead on another GUID without changing hidclass.sys.
And yes, opening any HID device goes through hidclass.sys and we really want it to be on top and handle all the ugly HID details (and it also guarantees that we will only ever have a single read request at a time, which will be handy).
Mmh, I see the problem now. Hmm.
I guess we really do want both the real and xinput devices to go through winehid + hidclass.
It should come as no surprise that the hidclass hack isn't great. But I can't particularly hate it, because this solution is in general about as close as we can get to native architecture.
I agree, I sadly couldn't find another way. I guess it's the drawback of using HID to communicate with the xinput internal device as it should normally not be that way.
I'm almost tempted to propose shoving large parts of hidclass into xinput and/or xinput.sys (making use of that new hidparse, probably—by the way, could that be a static library instead of using PARENTSRC?) but it's probably too much work to be worthwhile.
FWIW native hidparse.sys really exists, I don't know how much it matters to stay close to native in the world of drivers though. I don't really like the idea of making xinput having to re-implement similar logic as hidclass.sys, even if most of the code could be re-imported. The way it's done here make it possible to keep the driver code very simple and I don't think we want it to become complicated.
How bad would it be if we exposed the real device using GUID_DEVINTERFACE_HID anyway? Would it be worse than duplicate devices in some games? Not that that's particularly desirable to begin with, granted.
Yeah, I don't really now but I suspect it's not a good idea. Also we have to have a way to differentiate xinput-compatible HID devices from xinput-incompatible gamepads in xinput itself (although it could very well be done with another kind of suffix).
Otherwise I'll probably start reviewing the actual contents of this patch series instead of trying to wrap my head around what it's actually doing ;-)
Thanks! -- Rémi Bernon <rbernon(a)codeweavers.com>
participants (2)
-
Rémi Bernon -
Zebediah Figura (she/her)