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@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.