[PATCH 1/6] 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> --- dlls/winebus.sys/main.c | 26 ++++++++++++++++++++++++-- dlls/winehid.sys/winehid.inf | 7 +------ 2 files changed, 25 insertions(+), 8 deletions(-) diff --git a/dlls/winebus.sys/main.c b/dlls/winebus.sys/main.c index 41aef8ab018..c09ea1c2828 100644 --- a/dlls/winebus.sys/main.c +++ b/dlls/winebus.sys/main.c @@ -226,7 +226,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; @@ -240,6 +240,28 @@ 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 pos = 0, len = 0, hid_compat_len; + WCHAR *dst; + + hid_compat_len = strlenW(hid_compat); + + len += hid_compat_len + 1; + + if ((dst = ExAllocatePool(PagedPool, (len + 1) * sizeof(WCHAR)))) + { + pos += snprintfW(dst + pos, len - pos, hid_compat) + 1; + dst[pos] = 0; + } + + return dst; +} + static void remove_pending_irps(DEVICE_OBJECT *device) { struct device_extension *ext = device->DeviceExtension; @@ -444,7 +466,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.33.0
In addition, and before WINEBUS\WINE_COMP_HID, so that new xinput.sys will match first as soon as it is introduced. Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/winebus.sys/main.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/dlls/winebus.sys/main.c b/dlls/winebus.sys/main.c index c09ea1c2828..8b9aa25feb9 100644 --- a/dlls/winebus.sys/main.c +++ b/dlls/winebus.sys/main.c @@ -242,19 +242,27 @@ static WCHAR *get_hardware_ids(DEVICE_OBJECT *device) static WCHAR *get_compatible_ids(DEVICE_OBJECT *device) { + 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 + }; 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 pos = 0, len = 0, hid_compat_len; + struct device_extension *ext = (struct device_extension *)device->DeviceExtension; + DWORD pos = 0, len = 0, hid_compat_len, xinput_compat_len; WCHAR *dst; + xinput_compat_len = strlenW(xinput_compat); hid_compat_len = strlenW(hid_compat); + if (ext->is_gamepad) len += xinput_compat_len + 1; len += hid_compat_len + 1; if ((dst = ExAllocatePool(PagedPool, (len + 1) * sizeof(WCHAR)))) { + if (ext->is_gamepad) pos += snprintfW(dst + pos, len - pos, xinput_compat) + 1; pos += snprintfW(dst + pos, len - pos, hid_compat) + 1; dst[pos] = 0; } -- 2.33.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/xinput.sys/Makefile.in | 8 + dlls/xinput.sys/main.c | 394 ++++++++++++++++++++++++++++++++ dlls/xinput.sys/xinput.inf | 22 ++ dlls/xinput.sys/xinput.rc | 20 ++ dlls/xinput.sys/xinput.sys.spec | 1 + loader/wine.inf.in | 1 + 7 files changed, 447 insertions(+) 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/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..d1c547f0572 --- /dev/null +++ b/dlls/xinput.sys/main.c @@ -0,0 +1,394 @@ +/* + * 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 *gamepad_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); + return STATUS_DELETE_PENDING; + } + + 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 *gamepad_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, &gamepad_pdo))) + { + ERR( "failed to create gamepad PDO, status %#x.\n", status ); + return; + } + + pdo_ext = gamepad_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->gamepad_pdo = gamepad_pdo; + + TRACE("fdo %p, gamepad PDO %p.\n", fdo, gamepad_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->gamepad_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.33.0
On 8/26/21 12:59 AM, Rémi Bernon wrote:
+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; +} +
Missing space for a null terminator in these two. In general these could also be memcpy(), although that's a bit of a nitpick.
+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; +} +
Same comment from last time (on a different patch?) about defining a static const WCHAR[] variable for that string. Actually, it seems like quite a lot of feedback from <https://www.winehq.org/pipermail/wine-devel/2021-August/193365.html> has not been addressed, or responded to.
On 8/27/21 7:56 PM, Zebediah Figura (she/her) wrote:
On 8/26/21 12:59 AM, Rémi Bernon wrote:
+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; +} +
Missing space for a null terminator in these two.
In general these could also be memcpy(), although that's a bit of a nitpick.
Indeed.
+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; +} +
Same comment from last time (on a different patch?) about defining a static const WCHAR[] variable for that string.
Actually, it seems like quite a lot of feedback from <https://www.winehq.org/pipermail/wine-devel/2021-August/193365.html> has not been addressed, or responded to.
Indeed, I missed them somehow... sorry. -- Rémi Bernon <rbernon(a)codeweavers.com>
The &IG_00 suffix is now automatically added by xinput.sys. 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 -- dlls/winebus.sys/main.c | 18 ++++-------------- 4 files changed, 6 insertions(+), 24 deletions(-) diff --git a/dlls/winebus.sys/bus_iohid.c b/dlls/winebus.sys/bus_iohid.c index eb5eddd60fa..9b842dc81a5 100644 --- a/dlls/winebus.sys/bus_iohid.c +++ b/dlls/winebus.sys/bus_iohid.c @@ -299,7 +299,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); @@ -361,13 +360,11 @@ static void handle_DeviceMatchingCallback(void *context, IOReturn result, void * is_gamepad = (axes == 6 && buttons >= 14); } } - if (is_gamepad) - input = 0; if (!(private = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, sizeof(struct platform_private)))) return; - device = bus_create_hid_device(busidW, vid, pid, input, version, uid, str ? serial_string : NULL, + device = bus_create_hid_device(busidW, vid, pid, -1, version, uid, str ? serial_string : NULL, is_gamepad, &iohid_vtbl, &private->unix_device); if (!device) HeapFree(GetProcessHeap(), 0, private); else diff --git a/dlls/winebus.sys/bus_sdl.c b/dlls/winebus.sys/bus_sdl.c index 61d1240e05d..8259531779b 100644 --- a/dlls/winebus.sys/bus_sdl.c +++ b/dlls/winebus.sys/bus_sdl.c @@ -744,7 +744,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; @@ -795,12 +794,10 @@ 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; if (!(private = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, sizeof(*private)))) return; - device = bus_create_hid_device(sdl_busidW, vid, pid, input, version, index, serial, is_xbox_gamepad, + device = bus_create_hid_device(sdl_busidW, vid, pid, -1, version, index, serial, is_xbox_gamepad, &sdl_vtbl, &private->unix_device); if (!device) HeapFree(GetProcessHeap(), 0, private); else diff --git a/dlls/winebus.sys/bus_udev.c b/dlls/winebus.sys/bus_udev.c index bd70a66f2cd..8d631fb928e 100644 --- a/dlls/winebus.sys/bus_udev.c +++ b/dlls/winebus.sys/bus_udev.c @@ -1131,8 +1131,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)); diff --git a/dlls/winebus.sys/main.c b/dlls/winebus.sys/main.c index 8b9aa25feb9..c57e5164576 100644 --- a/dlls/winebus.sys/main.c +++ b/dlls/winebus.sys/main.c @@ -149,8 +149,6 @@ static CRITICAL_SECTION device_list_cs = { &critsect_debug, -1, 0, 0, 0, 0 }; static struct list device_list = LIST_INIT(device_list); 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 NTSTATUS winebus_call(unsigned int code, void *args) { @@ -202,25 +200,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; -- 2.33.0
On 8/26/21 12:59 AM, Rémi Bernon wrote:
The &IG_00 suffix is now automatically added by xinput.sys.
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 -- dlls/winebus.sys/main.c | 18 ++++-------------- 4 files changed, 6 insertions(+), 24 deletions(-)
Don't we still want to add the MI suffix where applicable?
On 8/27/21 7:59 PM, Zebediah Figura (she/her) wrote:
On 8/26/21 12:59 AM, Rémi Bernon wrote:
The &IG_00 suffix is now automatically added by xinput.sys.
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 -- dlls/winebus.sys/main.c | 18 ++++-------------- 4 files changed, 6 insertions(+), 24 deletions(-)
Don't we still want to add the MI suffix where applicable?
I'm not sure what you mean by "where applicable"? When we can and do detect an input number (although it's actually pretty bogus), we add the &MI_ suffix. I understand it's used for composite devices with multiple interfaces, but UDEV bus devices for instance may get an "input" number parsed from uevent even if they aren't composite. In any case I'm not completely sure how useful it is anyway. -- Rémi Bernon <rbernon(a)codeweavers.com>
On 8/27/21 2:14 PM, Rémi Bernon wrote:
On 8/27/21 7:59 PM, Zebediah Figura (she/her) wrote:
On 8/26/21 12:59 AM, Rémi Bernon wrote:
The &IG_00 suffix is now automatically added by xinput.sys.
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 -- dlls/winebus.sys/main.c | 18 ++++-------------- 4 files changed, 6 insertions(+), 24 deletions(-)
Don't we still want to add the MI suffix where applicable?
I'm not sure what you mean by "where applicable"? When we can and do detect an input number (although it's actually pretty bogus), we add the &MI_ suffix.
I understand it's used for composite devices with multiple interfaces, but UDEV bus devices for instance may get an "input" number parsed from uevent even if they aren't composite.
Oops, I misread it; I saw the lines deleted from get_device_id() and missed the ones added. The new code looks like an improvement, so I guess I can't complain too much ;-)
In any case I'm not completely sure how useful it is anyway.
Probably not very, but in theory you could have a HID driver that matches on a specific device interface.
On 8/27/21 9:47 PM, Zebediah Figura (she/her) wrote:
In any case I'm not completely sure how useful it is anyway.
Probably not very, but in theory you could have a HID driver that matches on a specific device interface.
Indeed and it actually causes me some trouble, as the UDEV devices may get spurious MI_00 suffixes, while SDL devices do not. I wanted to add the list of XBox VID/PID to xinput.inf, so that we don't have to keep a hardcoded list in winebus, but then I'll either have to add both the suffixed and not, or find a way to make things consistent (or report both hardware ids...). -- Rémi Bernon <rbernon(a)codeweavers.com>
On 8/27/21 3:21 PM, Rémi Bernon wrote:
On 8/27/21 9:47 PM, Zebediah Figura (she/her) wrote:
In any case I'm not completely sure how useful it is anyway.
Probably not very, but in theory you could have a HID driver that matches on a specific device interface.
Indeed and it actually causes me some trouble, as the UDEV devices may get spurious MI_00 suffixes, while SDL devices do not.
I suspect that the way we detect the interface number is wrong :-( (and we should probably call it "interface" or "interface_id" rather than "input" in general.)
I wanted to add the list of XBox VID/PID to xinput.inf, so that we don't have to keep a hardcoded list in winebus, but then I'll either have to add both the suffixed and not, or find a way to make things consistent (or report both hardware ids...).
Finding a way to report the right IDs sounds like the right solution to me.
This internal xinput 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. It filters the report read requests and copies the input reports to the pending gamepad PDO read requests, completing them at the same time. This is a Wine extension for convenience and native XInput driver uses a different, undocumented, device interface. 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 | 226 +++++++++++++++++++++++++++++++++++- 4 files changed, 230 insertions(+), 5 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 d1c547f0572..15d9a8b4735 100644 --- a/dlls/xinput.sys/main.c +++ b/dlls/xinput.sys/main.c @@ -20,6 +20,7 @@ #include <stdarg.h> #include <stdlib.h> +#include <assert.h> #include "ntstatus.h" #define WIN32_NO_STATUS @@ -31,6 +32,7 @@ #include "cfgmgr32.h" #include "ddk/wdm.h" #include "ddk/hidport.h" +#include "ddk/hidpddi.h" #include "wine/debug.h" @@ -52,14 +54,24 @@ struct device_state { DEVICE_OBJECT *bus_pdo; DEVICE_OBJECT *gamepad_pdo; + DEVICE_OBJECT *xinput_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; + BOOL pending_is_gamepad; + + ULONG report_len; + char *report_buf; }; struct device_extension { BOOL is_fdo; + BOOL is_gamepad; BOOL removed; WCHAR device_id[MAX_DEVICE_ID_LEN]; @@ -71,6 +83,111 @@ 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; +} + +/* check for a pending read from the other PDO, and complete both at a time. + * if there's none, save irp as pending, the other PDO will complete it. + * if the device is being removed, complete irp with an error. */ +static NTSTATUS try_complete_pending_read(DEVICE_OBJECT *device, IRP *irp) +{ + struct device_extension *ext = ext_from_DEVICE_OBJECT(device); + IRP *pending, *xinput_irp, *gamepad_irp; + struct device_state *state = ext->state; + BOOL removed, pending_is_gamepad; + NTSTATUS status; + ULONG offset; + + RtlEnterCriticalSection(&state->cs); + pending_is_gamepad = state->pending_is_gamepad; + if ((removed = ext->removed)) + pending = NULL; + else if ((pending = state->pending_read)) + state->pending_read = NULL; + else + { + state->pending_read = irp; + state->pending_is_gamepad = ext->is_gamepad; + IoMarkIrpPending(irp); + } + RtlLeaveCriticalSection(&state->cs); + + if (removed) + { + irp->IoStatus.Status = STATUS_DELETE_PENDING; + irp->IoStatus.Information = 0; + IoCompleteRequest(irp, IO_NO_INCREMENT); + return STATUS_DELETE_PENDING; + } + + if (!pending) return STATUS_PENDING; + + /* only one read at a time per device from hidclass.sys design */ + assert(pending_is_gamepad != ext->is_gamepad); + gamepad_irp = ext->is_gamepad ? irp : pending; + xinput_irp = ext->is_gamepad ? pending : irp; + + offset = state->report_buf[0] ? 0 : 1; + if (!(status = sync_ioctl(state->bus_pdo, IOCTL_HID_READ_REPORT, NULL, 0, + state->report_buf + offset, state->report_len - offset))) + { + memcpy(xinput_irp->UserBuffer, state->report_buf + offset, state->report_len - offset); + xinput_irp->IoStatus.Information = state->report_len; + memcpy(gamepad_irp->UserBuffer, state->report_buf + offset, state->report_len - offset); + gamepad_irp->IoStatus.Information = state->report_len; + } + + xinput_irp->IoStatus.Status = status; + IoCompleteRequest(xinput_irp, IO_NO_INCREMENT); + + gamepad_irp->IoStatus.Status = status; + IoCompleteRequest(gamepad_irp, IO_NO_INCREMENT); + return status; +} + +static NTSTATUS WINAPI gamepad_pdo_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; + + TRACE("device %p, irp %p, code %#x, bus_pdo %p.\n", device, irp, code, state->bus_pdo); + + switch (code) + { + 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 internal_ioctl(DEVICE_OBJECT *device, IRP *irp) { struct device_extension *ext = ext_from_DEVICE_OBJECT(device); @@ -88,10 +205,74 @@ static NTSTATUS WINAPI internal_ioctl(DEVICE_OBJECT *device, IRP *irp) TRACE("device %p, irp %p, code %#x, bus_pdo %p.\n", device, irp, code, state->bus_pdo); + if (code == IOCTL_HID_READ_REPORT) return try_complete_pending_read(device, irp); + if (ext->is_gamepad) return gamepad_pdo_internal_ioctl(device, irp); + IoSkipCurrentIrpStackLocation(irp); return IoCallDriver(state->bus_pdo, irp); } +static NTSTATUS gamepad_pdo_start_device(DEVICE_OBJECT *device) +{ + struct device_extension *ext = ext_from_DEVICE_OBJECT(device); + struct device_state *state = ext->state; + ULONG i, report_desc_len, report_count; + PHIDP_REPORT_DESCRIPTOR report_desc; + PHIDP_PREPARSED_DATA preparsed; + HIDP_DEVICE_DESC device_desc; + HIDP_REPORT_IDS *reports; + HID_DESCRIPTOR hid_desc; + 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); + + reports = device_desc.ReportIDs; + report_count = device_desc.ReportIDsLength; + for (i = 0; i < report_count; ++i) if (!reports[i].ReportID || reports[i].InputLength) break; + if (i == report_count) i = 0; /* no input report?!, just use first ID */ + + status = STATUS_SUCCESS; + RtlEnterCriticalSection(&state->cs); + state->report_len = caps.InputReportByteLength; + if (!(state->report_buf = malloc(state->report_len))) status = STATUS_NO_MEMORY; + else state->report_buf[0] = reports[i].ReportID; + RtlLeaveCriticalSection(&state->cs); + + HidP_FreeCollectionDescription(&device_desc); + return status; +} + +static NTSTATUS gamepad_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); @@ -153,21 +334,36 @@ 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_gamepad) status = gamepad_pdo_start_device(device); + else status = STATUS_SUCCESS; break; case IRP_MN_SURPRISE_REMOVAL: status = STATUS_SUCCESS; if (InterlockedExchange(&ext->removed, TRUE)) break; + + RtlEnterCriticalSection(&state->cs); + pending = state->pending_read; + state->pending_read = NULL; + RtlLeaveCriticalSection(&state->cs); + + if (pending) + { + pending->IoStatus.Status = STATUS_DELETE_PENDING; + pending->IoStatus.Information = 0; + IoCompleteRequest(pending, IO_NO_INCREMENT); + } break; case IRP_MN_REMOVE_DEVICE: + if (ext->is_gamepad) gamepad_pdo_remove_device(device); irp->IoStatus.Status = STATUS_SUCCESS; IoCompleteRequest(irp, IO_NO_INCREMENT); IoDeleteDevice(device); @@ -211,7 +407,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 *gamepad_pdo; + DEVICE_OBJECT *xinput_pdo, *gamepad_pdo; UNICODE_STRING string; WCHAR *tmp, pdo_name[255]; NTSTATUS status; @@ -224,16 +420,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, &xinput_pdo))) + { + ERR( "failed to create xinput PDO, status %#x.\n", status ); + IoDeleteDevice(gamepad_pdo); + return; + } + pdo_ext = gamepad_pdo->DeviceExtension; pdo_ext->is_fdo = FALSE; + pdo_ext->is_gamepad = 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 = xinput_pdo->DeviceExtension; + pdo_ext->is_fdo = FALSE; + pdo_ext->is_gamepad = FALSE; + pdo_ext->state = state; + swprintf(pdo_ext->device_id, MAX_DEVICE_ID_LEN, L"XINPUT\\%s", wcsrchr(fdo_ext->device_id, '\\') + 1); + state->gamepad_pdo = gamepad_pdo; + state->xinput_pdo = xinput_pdo; - TRACE("fdo %p, gamepad PDO %p.\n", fdo, gamepad_pdo); + TRACE("fdo %p, xinput PDO %p, gamepad PDO %p.\n", fdo, xinput_pdo, gamepad_pdo); IoInvalidateDeviceRelations(state->bus_pdo, BusRelations); } @@ -263,6 +476,12 @@ static NTSTATUS WINAPI fdo_pnp(DEVICE_OBJECT *device, IRP *irp) } devices->Count = 0; + if ((child = state->xinput_pdo)) + { + devices->Objects[devices->Count] = child; + call_fastcall_func1(ObfReferenceObject, child); + devices->Count++; + } if ((child = state->gamepad_pdo)) { devices->Objects[devices->Count] = child; @@ -374,6 +593,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.33.0
On 8/26/21 12:59 AM, Rémi Bernon wrote:
This internal xinput 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.
It filters the report read requests and copies the input reports to the pending gamepad PDO read requests, completing them at the same time.
This is a Wine extension for convenience and native XInput driver uses a different, undocumented, device interface.
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 | 226 +++++++++++++++++++++++++++++++++++- 4 files changed, 230 insertions(+), 5 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;
It's a matter of taste, I guess, but personally if I have to wrap a ternary operator, I take it as a sign I should be using if/else instead ;-) It also occurs to me that since this is very internal it might be a good idea to use WINEXINPUT or something rather than just XINPUT. Unlikely to make a difference, of course, but it also sort of communicates that this is internal, and more clearly tells anyone looking for the code where to look for the driver that reports WINEXINPUT devices (viz. elsewhere in the code tree.)
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;
Any chance we could put the hidclass hunks in a separate patch? ...
@@ -52,14 +54,24 @@ struct device_state { DEVICE_OBJECT *bus_pdo; DEVICE_OBJECT *gamepad_pdo; + DEVICE_OBJECT *xinput_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; + BOOL pending_is_gamepad;
Honestly after thinking about it I don't dislike "internal" as much I dislike "xinput" now. Because you'd think that "xinput" refers to the device we access from xinput, but actually it's the other way around. Not to mention that calling either device "xinput" sucks when the whole driver is named "xinput" and it just looks like an namespace prefix. Of course "gamepad" isn't great either, because they're both gamepads :-( Naming things is hard, of course, and I don't have any great suggestions. The best I can come up with after thinking about it are "real" and "fake". Or maybe "private" and "fake", which is a little less ambiguous as to which one is which.
+ + ULONG report_len; + char *report_buf; };
struct device_extension { BOOL is_fdo; + BOOL is_gamepad; BOOL removed;
WCHAR device_id[MAX_DEVICE_ID_LEN]; @@ -71,6 +83,111 @@ 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; +} + +/* check for a pending read from the other PDO, and complete both at a time. + * if there's none, save irp as pending, the other PDO will complete it. + * if the device is being removed, complete irp with an error. */ +static NTSTATUS try_complete_pending_read(DEVICE_OBJECT *device, IRP *irp) +{ + struct device_extension *ext = ext_from_DEVICE_OBJECT(device); + IRP *pending, *xinput_irp, *gamepad_irp; + struct device_state *state = ext->state; + BOOL removed, pending_is_gamepad; + NTSTATUS status; + ULONG offset; + + RtlEnterCriticalSection(&state->cs); + pending_is_gamepad = state->pending_is_gamepad; + if ((removed = ext->removed)) + pending = NULL; + else if ((pending = state->pending_read)) + state->pending_read = NULL; + else + { + state->pending_read = irp; + state->pending_is_gamepad = ext->is_gamepad; + IoMarkIrpPending(irp); + } + RtlLeaveCriticalSection(&state->cs); + + if (removed) + { + irp->IoStatus.Status = STATUS_DELETE_PENDING; + irp->IoStatus.Information = 0; + IoCompleteRequest(irp, IO_NO_INCREMENT); + return STATUS_DELETE_PENDING; + } + + if (!pending) return STATUS_PENDING; + + /* only one read at a time per device from hidclass.sys design */ + assert(pending_is_gamepad != ext->is_gamepad); + gamepad_irp = ext->is_gamepad ? irp : pending; + xinput_irp = ext->is_gamepad ? pending : irp; + + offset = state->report_buf[0] ? 0 : 1; + if (!(status = sync_ioctl(state->bus_pdo, IOCTL_HID_READ_REPORT, NULL, 0, + state->report_buf + offset, state->report_len - offset)))
This blocks in the IRP handler. I'm pretty sure this *works* in Wine, but you aren't really supposed to do this, and I could easily see future Wine changes causing problems here. (One of the many reasons that Linux really is the superior operating system...)
+ { + memcpy(xinput_irp->UserBuffer, state->report_buf + offset, state->report_len - offset); + xinput_irp->IoStatus.Information = state->report_len; + memcpy(gamepad_irp->UserBuffer, state->report_buf + offset, state->report_len - offset); + gamepad_irp->IoStatus.Information = state->report_len; + } + + xinput_irp->IoStatus.Status = status; + IoCompleteRequest(xinput_irp, IO_NO_INCREMENT); + + gamepad_irp->IoStatus.Status = status; + IoCompleteRequest(gamepad_irp, IO_NO_INCREMENT); + return status; +} +
...
@@ -374,6 +593,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));
No matching call to RtlDeleteCriticalSection; I guess that should go in FDO remove. Also, missing CS debug info.
On 8/27/21 10:31 PM, Zebediah Figura (she/her) wrote:
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;
It's a matter of taste, I guess, but personally if I have to wrap a ternary operator, I take it as a sign I should be using if/else instead ;-)
Yeah, I spent a lot of time hesitating on the best looking way to write that.
It also occurs to me that since this is very internal it might be a good idea to use WINEXINPUT or something rather than just XINPUT. Unlikely to make a difference, of course, but it also sort of communicates that this is internal, and more clearly tells anyone looking for the code where to look for the driver that reports WINEXINPUT devices (viz. elsewhere in the code tree.)
Works for me, I'm still not convinced that winexinput.sys is much clearer though, but I can probably live with it.
- 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;
Any chance we could put the hidclass hunks in a separate patch?
...
That would cause the "internal" "xinput" PDO be momentarily listed in the HID device interface class, as a duplicate device with the gamepad, and although applications aren't supposed to see or use it. Not a big deal, bug probably not very nice.
@@ -52,14 +54,24 @@ struct device_state { DEVICE_OBJECT *bus_pdo; DEVICE_OBJECT *gamepad_pdo; + DEVICE_OBJECT *xinput_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; + BOOL pending_is_gamepad;
Honestly after thinking about it I don't dislike "internal" as much I dislike "xinput" now. Because you'd think that "xinput" refers to the device we access from xinput, but actually it's the other way around.
It's still the case there, but eventually (ie: in an eventual PATCH 7/6 where xinput is switched to the internal device interface class), this is how it will indeed be. So to me it's actually intuitive.
Not to mention that calling either device "xinput" sucks when the whole driver is named "xinput" and it just looks like an namespace prefix.
Of course "gamepad" isn't great either, because they're both gamepads :-(
Naming things is hard, of course, and I don't have any great suggestions. The best I can come up with after thinking about it are "real" and "fake". Or maybe "private" and "fake", which is a little less ambiguous as to which one is which.
Well, to me fake or real are really vague concepts, especially when speaking about device. The gamepad is as real as the other, it just adds a translation layer. I don't have any better idea than gamepad/xinput right now tbh. If the driver were to be renamed to winexinput.sys, maybe xinput would be less like a namespace prefix... In any case, I don't think the driver is supposed to become more complicated than these patches (it just misses the report translation and that's it), it should be quite straightforward to get the whole picture. With a few comments maybe?
+ + /* only one read at a time per device from hidclass.sys design */ + assert(pending_is_gamepad != ext->is_gamepad); + gamepad_irp = ext->is_gamepad ? irp : pending; + xinput_irp = ext->is_gamepad ? pending : irp; + + offset = state->report_buf[0] ? 0 : 1; + if (!(status = sync_ioctl(state->bus_pdo, IOCTL_HID_READ_REPORT, NULL, 0, + state->report_buf + offset, state->report_len - offset)))
This blocks in the IRP handler. I'm pretty sure this *works* in Wine, but you aren't really supposed to do this, and I could easily see future Wine changes causing problems here. (One of the many reasons that Linux really is the superior operating system...)
I wondered about it for a bit, and it was simpler like this. I guess it could be done with completions without making it it much more complicated... I'll have a try.
@@ -374,6 +593,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));
No matching call to RtlDeleteCriticalSection; I guess that should go in FDO remove.
Also, missing CS debug info.
Sure. FWIW I don't know if we ever care (certainly not here), but setting the CS debug info causes crashes on Windows. I figured that when I was trying my HID dinput.dll there. -- Rémi Bernon <rbernon(a)codeweavers.com>
On 8/27/21 10:55 PM, Rémi Bernon wrote:
- 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;
Any chance we could put the hidclass hunks in a separate patch?
...
That would cause the "internal" "xinput" PDO be momentarily listed in the HID device interface class, as a duplicate device with the gamepad, and although applications aren't supposed to see or use it. Not a big deal, bug probably not very nice.
I guess it could come first though. BTW I used the "standard" XUSB interface class there, but now that I think of it and now that I'm moving everything to "winexinput" maybe it's not the best idea to do that. Is there a general strategy on how to define Wine-specific GUIDs? Should I just go get some randomly generated GUID and that's it? Doesn't it have to have a specific marker to tell that it's some Wine-specific one? -- Rémi Bernon <rbernon(a)codeweavers.com>
On 8/28/21 1:46 PM, Rémi Bernon wrote:
On 8/27/21 10:55 PM, Rémi Bernon wrote:
- 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;
Any chance we could put the hidclass hunks in a separate patch?
...
That would cause the "internal" "xinput" PDO be momentarily listed in the HID device interface class, as a duplicate device with the gamepad, and although applications aren't supposed to see or use it. Not a big deal, bug probably not very nice.
I guess it could come first though.
Yeah, that's what I meant.
BTW I used the "standard" XUSB interface class there, but now that I think of it and now that I'm moving everything to "winexinput" maybe it's not the best idea to do that.
Is there a general strategy on how to define Wine-specific GUIDs? Should I just go get some randomly generated GUID and that's it? Doesn't it have to have a specific marker to tell that it's some Wine-specific one?
I don't think there is, no. Personally I like to use regular-looking numbers for Data1 so that it's clear it's not a Windows GUID and nobody needs to waste time looking it up (this is even more true in tests), but there's no requirement for it.
On 8/27/21 3:55 PM, Rémi Bernon wrote:
On 8/27/21 10:31 PM, Zebediah Figura (she/her) wrote:
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;
It's a matter of taste, I guess, but personally if I have to wrap a ternary operator, I take it as a sign I should be using if/else instead ;-)
Yeah, I spent a lot of time hesitating on the best looking way to write that.
It also occurs to me that since this is very internal it might be a good idea to use WINEXINPUT or something rather than just XINPUT. Unlikely to make a difference, of course, but it also sort of communicates that this is internal, and more clearly tells anyone looking for the code where to look for the driver that reports WINEXINPUT devices (viz. elsewhere in the code tree.)
Works for me, I'm still not convinced that winexinput.sys is much clearer though, but I can probably live with it.
I'm fine with xinput.sys, but I'd rather see the device ID named WINEXINPUT.
- 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;
Any chance we could put the hidclass hunks in a separate patch?
...
That would cause the "internal" "xinput" PDO be momentarily listed in the HID device interface class, as a duplicate device with the gamepad, and although applications aren't supposed to see or use it. Not a big deal, bug probably not very nice.
Yeah, I meant putting them first, as you seem to have realized.
@@ -52,14 +54,24 @@ struct device_state { DEVICE_OBJECT *bus_pdo; DEVICE_OBJECT *gamepad_pdo; + DEVICE_OBJECT *xinput_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; + BOOL pending_is_gamepad;
Honestly after thinking about it I don't dislike "internal" as much I dislike "xinput" now. Because you'd think that "xinput" refers to the device we access from xinput, but actually it's the other way around.
It's still the case there, but eventually (ie: in an eventual PATCH 7/6 where xinput is switched to the internal device interface class), this is how it will indeed be. So to me it's actually intuitive.
Well, right, I'm talking about the eventual situation. And, I hate to say it, but "to me it's actually intuitive" isn't great :-(
Not to mention that calling either device "xinput" sucks when the whole driver is named "xinput" and it just looks like an namespace prefix.
Of course "gamepad" isn't great either, because they're both gamepads :-(
Naming things is hard, of course, and I don't have any great suggestions. The best I can come up with after thinking about it are "real" and "fake". Or maybe "private" and "fake", which is a little less ambiguous as to which one is which.
Well, to me fake or real are really vague concepts, especially when speaking about device. The gamepad is as real as the other, it just adds a translation layer. I don't have any better idea than gamepad/xinput right now tbh.
If the driver were to be renamed to winexinput.sys, maybe xinput would be less like a namespace prefix...
In any case, I don't think the driver is supposed to become more complicated than these patches (it just misses the report translation and that's it), it should be quite straightforward to get the whole picture. With a few comments maybe?
I think a comment near the top is definitely going to be necessary regardless of what terminology we eventually use. My logic with "real"/"fake" is that the "real" device reports the "real" data that we get from winebus; the "fake" device fixes it up and hides some of it. But yeah, it's clearly not unambiguous either. The best thing it has going for it is that they're clearly attributes of the child devices rather than namespace prefixes or referring to internal IOCTLs or anything else.
+ + /* only one read at a time per device from hidclass.sys design */ + assert(pending_is_gamepad != ext->is_gamepad); + gamepad_irp = ext->is_gamepad ? irp : pending; + xinput_irp = ext->is_gamepad ? pending : irp; + + offset = state->report_buf[0] ? 0 : 1; + if (!(status = sync_ioctl(state->bus_pdo, IOCTL_HID_READ_REPORT, NULL, 0, + state->report_buf + offset, state->report_len - offset)))
This blocks in the IRP handler. I'm pretty sure this *works* in Wine, but you aren't really supposed to do this, and I could easily see future Wine changes causing problems here. (One of the many reasons that Linux really is the superior operating system...)
I wondered about it for a bit, and it was simpler like this. I guess it could be done with completions without making it it much more complicated... I'll have a try.
It doesn't sound too bad from my armchair ;-)
@@ -374,6 +593,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));
No matching call to RtlDeleteCriticalSection; I guess that should go in FDO remove.
Also, missing CS debug info.
Sure. FWIW I don't know if we ever care (certainly not here), but setting the CS debug info causes crashes on Windows. I figured that when I was trying my HID dinput.dll there.
I think we usually don't worry about it. The only instance I know offhand is strmbase, which is used to build Windows tests. See strmbase_filter_init() and strmbase_filter_cleanup() for what to do in such a case.
On 8/30/21 6:22 PM, Zebediah Figura (she/her) wrote:
On 8/27/21 3:55 PM, Rémi Bernon wrote:
On 8/27/21 10:31 PM, Zebediah Figura (she/her) wrote:
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;
It's a matter of taste, I guess, but personally if I have to wrap a ternary operator, I take it as a sign I should be using if/else instead ;-)
Yeah, I spent a lot of time hesitating on the best looking way to write that.
It also occurs to me that since this is very internal it might be a good idea to use WINEXINPUT or something rather than just XINPUT. Unlikely to make a difference, of course, but it also sort of communicates that this is internal, and more clearly tells anyone looking for the code where to look for the driver that reports WINEXINPUT devices (viz. elsewhere in the code tree.)
Works for me, I'm still not convinced that winexinput.sys is much clearer though, but I can probably live with it.
I'm fine with xinput.sys, but I'd rather see the device ID named WINEXINPUT.
That's what I did in the new version, although the driver is also named winexinput.sys now. I think it'd be less confusing that way.
@@ -52,14 +54,24 @@ struct device_state { DEVICE_OBJECT *bus_pdo; DEVICE_OBJECT *gamepad_pdo; + DEVICE_OBJECT *xinput_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; + BOOL pending_is_gamepad;
Honestly after thinking about it I don't dislike "internal" as much I dislike "xinput" now. Because you'd think that "xinput" refers to the device we access from xinput, but actually it's the other way around.
It's still the case there, but eventually (ie: in an eventual PATCH 7/6 where xinput is switched to the internal device interface class), this is how it will indeed be. So to me it's actually intuitive.
Well, right, I'm talking about the eventual situation.
And, I hate to say it, but "to me it's actually intuitive" isn't great :-(
In any case, I don't think the driver is supposed to become more complicated than these patches (it just misses the report translation and that's it), it should be quite straightforward to get the whole picture. With a few comments maybe?
I think a comment near the top is definitely going to be necessary regardless of what terminology we eventually use.
My logic with "real"/"fake" is that the "real" device reports the "real" data that we get from winebus; the "fake" device fixes it up and hides some of it. But yeah, it's clearly not unambiguous either. The best thing it has going for it is that they're clearly attributes of the child devices rather than namespace prefixes or referring to internal IOCTLs or anything else.
In the new version I kept the names "gamepad_device" -for the bogus HID gamepad- with the "IG_" (as in I-something G-amepad) suffix, and "xinput_device" which will have an "XI_" (as in X-I-nput) suffix to distinguish them. Both will have the WINEXINPUT\ bus namespace. I think it's the best I can come up with that doesn't sound like something too generic (fake/real is completely generic IMHO and could mean anything depending on the reader current understanding). Cheers, -- 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.33.0
Well forgot to mention it but this is v2 of 211827-211829,211833-211834. I renamed what was "xinput PDO" to "gamepad PDO", and "internal PDO" to "xinput PDO" (as it's using a XINPUT\ bus id it made more sense). The read requests are now completed both at the same time, by issuing a sync read request downwards when both have been received. If not, the first read that comes is kept pending, and will be completed when another read arrives on the other PDO. -- Rémi Bernon <rbernon(a)codeweavers.com>
On 8/26/21 12:58 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> --- dlls/winebus.sys/main.c | 26 ++++++++++++++++++++++++-- dlls/winehid.sys/winehid.inf | 7 +------ 2 files changed, 25 insertions(+), 8 deletions(-)
diff --git a/dlls/winebus.sys/main.c b/dlls/winebus.sys/main.c index 41aef8ab018..c09ea1c2828 100644 --- a/dlls/winebus.sys/main.c +++ b/dlls/winebus.sys/main.c @@ -226,7 +226,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; @@ -240,6 +240,28 @@ 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 pos = 0, len = 0, hid_compat_len; + WCHAR *dst; + + hid_compat_len = strlenW(hid_compat); + + len += hid_compat_len + 1; + + if ((dst = ExAllocatePool(PagedPool, (len + 1) * sizeof(WCHAR)))) + { + pos += snprintfW(dst + pos, len - pos, hid_compat) + 1;
Why snprintfW() and not memcpy()? Note that memcpy() would also allow you to use sizeof().
+ dst[pos] = 0; + } + + return dst; +} +
There are a few redundancies in this function. It's kind of justified by 2/6, but maybe it makes more sense to leave off "pos" until that function.
static void remove_pending_irps(DEVICE_OBJECT *device) { struct device_extension *ext = device->DeviceExtension; @@ -444,7 +466,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
participants (2)
-
Rémi Bernon -
Zebediah Figura (she/her)