[PATCH v2 0/2] MR11158: xinput1_3: Rescan controllers before reporting disconnection to callers.
If a requested controller slot is not currently open, rescan the controllers before reporting the slot as disconnected. This matches native Windows behaviour. The game RiME (and possible others), upon receiving a device addition WM_DEVICECHANGE message, immediately queries all four slots with XInputGetState(), and gives up if none are unavailable. Without the rescan, the game's query can race with the xinput update thread and cause hotplugging failure. -- v2: xinput1_3: Rescan controllers before reporting disconnection to callers. xinput1_3: Implement thread-safe controller addition and removal. https://gitlab.winehq.org/wine/wine/-/merge_requests/11158
From: Shaun Ren <sren@codeweavers.com> This is needed in order to allow user call paths to rescan the controllers themselves, which is implemented in the following commit. --- dlls/xinput1_3/main.c | 145 +++++++++++++++++++++++++++++++----------- 1 file changed, 109 insertions(+), 36 deletions(-) diff --git a/dlls/xinput1_3/main.c b/dlls/xinput1_3/main.c index f6329a22b8d..26098209020 100644 --- a/dlls/xinput1_3/main.c +++ b/dlls/xinput1_3/main.c @@ -323,24 +323,53 @@ static DWORD HID_set_state(struct xinput_controller *controller, XINPUT_VIBRATIO return ERROR_SUCCESS; } -static void controller_destroy(struct xinput_controller *controller, BOOL already_removed); +static void controllers_lock_all(void) +{ + int i; + + for (i = 0; i < XUSER_MAX_COUNT; i++) + EnterCriticalSection(&controllers[i].crit); +} -static void controller_enable(struct xinput_controller *controller) +static void controllers_unlock_all_except(int slot) +{ + int i; + + for (i = XUSER_MAX_COUNT; i > 0; i--) + { + if (i - 1 != slot) LeaveCriticalSection(&controllers[i - 1].crit); + } +} + +static void controllers_unlock_all(void) +{ + controllers_unlock_all_except(-1); +} + +static BOOL controller_enable(struct xinput_controller *controller) { ULONG report_len = controller->hid.caps.InputReportByteLength; char *report_buf = controller->hid.input_report_buf; XINPUT_VIBRATION state = controller->vibration; BOOL ret; - if (controller->enabled) return; + if (controller->enabled) return TRUE; if (controller->caps.Flags & XINPUT_CAPS_FFB_SUPPORTED) HID_set_state(controller, &state); - controller->enabled = TRUE; memset(&controller->hid.read_ovl, 0, sizeof(controller->hid.read_ovl)); controller->hid.read_ovl.hEvent = controller->read_event; ret = ReadFile(controller->device, report_buf, report_len, NULL, &controller->hid.read_ovl); - if (!ret && GetLastError() != ERROR_IO_PENDING) controller_destroy(controller, TRUE); - else SetEvent(update_event); + if (!ret && GetLastError() != ERROR_IO_PENDING) return FALSE; + + controller->enabled = TRUE; + SetEvent(update_event); + return TRUE; +} + +static void controller_cancel_pending_read(struct xinput_controller *controller) +{ + CancelIoEx(controller->device, &controller->hid.read_ovl); + WaitForSingleObject(controller->hid.read_ovl.hEvent, INFINITE); } static void controller_disable(struct xinput_controller *controller) @@ -351,8 +380,7 @@ static void controller_disable(struct xinput_controller *controller) if (controller->caps.Flags & XINPUT_CAPS_FFB_SUPPORTED) HID_set_state(controller, &state); controller->enabled = FALSE; - CancelIoEx(controller->device, &controller->hid.read_ovl); - WaitForSingleObject(controller->hid.read_ovl.hEvent, INFINITE); + controller_cancel_pending_read(controller); SetEvent(update_event); } @@ -374,17 +402,15 @@ static BOOL controller_init(struct xinput_controller *controller, PHIDP_PREPARSE lstrcpynW(controller->device_path, device_path, MAX_PATH); controller->enabled = FALSE; - EnterCriticalSection(&controller->crit); controller->device = device; - controller_enable(controller); - LeaveCriticalSection(&controller->crit); - return TRUE; + if (controller_enable(controller)) return TRUE; failed: free(controller->hid.input_report_buf); free(controller->hid.output_report_buf); free(controller->hid.feature_report_buf); memset(&controller->hid, 0, sizeof(controller->hid)); + controller->device = NULL; return FALSE; } @@ -470,6 +496,7 @@ static BOOL open_device_at_index(const WCHAR *device_path, int index) return TRUE; } +/* Caller must hold all controller locks before calling this function. */ static BOOL find_opened_device(const WCHAR *device_path, int *slot) { int i; @@ -489,19 +516,53 @@ static BOOL find_opened_device(const WCHAR *device_path, int *slot) static BOOL try_add_device(const WCHAR *device_path) { + BOOL ret = FALSE; int i; - if (find_opened_device(device_path, &i)) return TRUE; /* already opened */ - if (i == XUSER_MAX_COUNT) return FALSE; /* no more slots */ - return open_device_at_index(device_path, i); + controllers_lock_all(); + + if (find_opened_device(device_path, &i)) /* already opened */ + { + controllers_unlock_all(); + return TRUE; + } + if (i == XUSER_MAX_COUNT) /* no more slots */ + { + controllers_unlock_all(); + return FALSE; + } + + controllers_unlock_all_except(i); + ret = open_device_at_index(device_path, i); + LeaveCriticalSection(&controllers[i].crit); + return ret; } +static void controller_destroy(struct xinput_controller *controller); + static void try_remove_device(const WCHAR *device_path) { int i; - if (find_opened_device(device_path, &i)) - controller_destroy(&controllers[i], TRUE); + controllers_lock_all(); + + if (!find_opened_device(device_path, &i)) + { + controllers_unlock_all(); + return; + } + + controllers_unlock_all_except(i); + + if (controllers[i].enabled) + { + controllers[i].enabled = FALSE; + controller_cancel_pending_read(&controllers[i]); + } + controller_destroy(&controllers[i]); + + LeaveCriticalSection(&controllers[i].crit); + SetEvent(update_event); } static void update_controller_list(void) @@ -530,13 +591,10 @@ static void update_controller_list(void) SetupDiDestroyDeviceInfoList(set); } -static void controller_destroy(struct xinput_controller *controller, BOOL already_removed) +static void controller_destroy(struct xinput_controller *controller) { - EnterCriticalSection(&controller->crit); - if (controller->device) { - if (!already_removed) controller_disable(controller); CloseHandle(controller->device); controller->device = NULL; @@ -545,9 +603,10 @@ static void controller_destroy(struct xinput_controller *controller, BOOL alread free(controller->hid.feature_report_buf); HidD_FreePreparsedData(controller->hid.preparsed); memset(&controller->hid, 0, sizeof(controller->hid)); - } - LeaveCriticalSection(&controller->crit); + controller->enabled = FALSE; + controller->device_path[0] = 0; + } } static LONG sign_extend(ULONG value, const HIDP_VALUE_CAPS *caps) @@ -567,26 +626,39 @@ static LONG scale_value(ULONG value, const HIDP_VALUE_CAPS *caps, LONG min, LONG static void read_controller_state(struct xinput_controller *controller) { - ULONG read_len, report_len = controller->hid.caps.InputReportByteLength; - char *report_buf = controller->hid.input_report_buf; + ULONG read_len, report_len; + char *report_buf; XINPUT_STATE state; NTSTATUS status; USAGE buttons[11]; ULONG i, button_length, value; BOOL ret; - if (!GetOverlappedResult(controller->device, &controller->hid.read_ovl, &read_len, TRUE)) + EnterCriticalSection(&controller->crit); + + if (!controller->device) + { + LeaveCriticalSection(&controller->crit); + return; + } + + if (!GetOverlappedResult(controller->device, &controller->hid.read_ovl, &read_len, FALSE)) { - if (GetLastError() == ERROR_OPERATION_ABORTED) return; if (GetLastError() == ERROR_ACCESS_DENIED || GetLastError() == ERROR_INVALID_HANDLE || GetLastError() == ERROR_DEVICE_NOT_CONNECTED) { - controller_destroy(controller, TRUE); + controller_destroy(controller); } - else ERR("Failed to read input report, GetOverlappedResult failed with error %lu\n", GetLastError()); + else if (GetLastError() != ERROR_OPERATION_ABORTED) + ERR("Failed to read input report, GetOverlappedResult failed with error %lu\n", GetLastError()); + + LeaveCriticalSection(&controller->crit); return; } + report_len = controller->hid.caps.InputReportByteLength; + report_buf = controller->hid.input_report_buf; + button_length = ARRAY_SIZE(buttons); status = HidP_GetUsages(HidP_Input, HID_USAGE_PAGE_BUTTON, 0, buttons, &button_length, controller->hid.preparsed, report_buf, report_len); if (status != HIDP_STATUS_SUCCESS) WARN("HidP_GetUsages HID_USAGE_PAGE_BUTTON returned %#lx\n", status); @@ -656,7 +728,6 @@ static void read_controller_state(struct xinput_controller *controller) if (status != HIDP_STATUS_SUCCESS) WARN("HidP_GetUsageValue HID_USAGE_PAGE_GENERIC / HID_USAGE_GENERIC_Z returned %#lx\n", status); else state.Gamepad.bLeftTrigger = scale_value(value, &controller->hid.lt_caps, 0, 255); - EnterCriticalSection(&controller->crit); if (controller->enabled) { state.dwPacketNumber = controller->state.dwPacketNumber + 1; @@ -664,8 +735,9 @@ static void read_controller_state(struct xinput_controller *controller) memset(&controller->hid.read_ovl, 0, sizeof(controller->hid.read_ovl)); controller->hid.read_ovl.hEvent = controller->read_event; ret = ReadFile(controller->device, report_buf, report_len, NULL, &controller->hid.read_ovl); - if (!ret && GetLastError() != ERROR_IO_PENDING) controller_destroy(controller, TRUE); + if (!ret && GetLastError() != ERROR_IO_PENDING) controller_destroy(controller); } + LeaveCriticalSection(&controller->crit); } @@ -722,9 +794,8 @@ static DWORD WINAPI hid_update_thread_proc(void *param) count = 0; for (i = 0; i < XUSER_MAX_COUNT; ++i) { - if (!controllers[i].device) continue; EnterCriticalSection(&controllers[i].crit); - if (controllers[i].enabled) + if (controllers[i].device && controllers[i].enabled) { devices[count] = controllers + i; events[count] = controllers[i].read_event; @@ -783,8 +854,6 @@ static void start_update_thread(void) static BOOL controller_lock(struct xinput_controller *controller) { - if (!controller->device) return FALSE; - EnterCriticalSection(&controller->crit); if (!controller->device) @@ -830,7 +899,11 @@ void WINAPI DECLSPEC_HOTPATCH XInputEnable(BOOL enable) for (index = 0; index < XUSER_MAX_COUNT; index++) { if (!controller_lock(&controllers[index])) continue; - if (enable) controller_enable(&controllers[index]); + if (enable) + { + if (!controller_enable(&controllers[index])) + controller_destroy(&controllers[index]); + } else controller_disable(&controllers[index]); controller_unlock(&controllers[index]); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/11158
From: Shaun Ren <sren@codeweavers.com> If a requested controller slot is not currently open, rescan the controllers before reporting the slot as disconnected. This matches native Windows behaviour. The game RiME (and possibly others), upon receiving a device addition WM_DEVICECHANGE message, immediately queries all four slots with XInputGetState(), and gives up if none are unavailable. Without the rescan, the game's query can race with the xinput update thread and cause hotplugging failure. --- dlls/xinput1_3/main.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/dlls/xinput1_3/main.c b/dlls/xinput1_3/main.c index 26098209020..95ed2d8b639 100644 --- a/dlls/xinput1_3/main.c +++ b/dlls/xinput1_3/main.c @@ -865,6 +865,13 @@ static BOOL controller_lock(struct xinput_controller *controller) return TRUE; } +static BOOL controller_lock_or_rescan(struct xinput_controller *controller) +{ + if (controller_lock(controller)) return TRUE; + update_controller_list(); + return controller_lock(controller); +} + static void controller_unlock(struct xinput_controller *controller) { LeaveCriticalSection(&controller->crit); @@ -918,7 +925,7 @@ DWORD WINAPI DECLSPEC_HOTPATCH XInputSetState(DWORD index, XINPUT_VIBRATION *vib start_update_thread(); if (index >= XUSER_MAX_COUNT) return ERROR_BAD_ARGUMENTS; - if (!controller_lock(&controllers[index])) return ERROR_DEVICE_NOT_CONNECTED; + if (!controller_lock_or_rescan(&controllers[index])) return ERROR_DEVICE_NOT_CONNECTED; ret = HID_set_state(&controllers[index], vibration); @@ -936,7 +943,7 @@ static DWORD xinput_get_state(DWORD index, XINPUT_STATE *state) start_update_thread(); if (index >= XUSER_MAX_COUNT) return ERROR_BAD_ARGUMENTS; - if (!controller_lock(&controllers[index])) return ERROR_DEVICE_NOT_CONNECTED; + if (!controller_lock_or_rescan(&controllers[index])) return ERROR_DEVICE_NOT_CONNECTED; *state = controllers[index].state; controller_unlock(&controllers[index]); @@ -1223,7 +1230,7 @@ DWORD WINAPI DECLSPEC_HOTPATCH XInputGetCapabilitiesEx(DWORD unk, DWORD index, D if (index >= XUSER_MAX_COUNT) return ERROR_BAD_ARGUMENTS; - if (!controller_lock(&controllers[index])) return ERROR_DEVICE_NOT_CONNECTED; + if (!controller_lock_or_rescan(&controllers[index])) return ERROR_DEVICE_NOT_CONNECTED; if (flags & XINPUT_FLAG_GAMEPAD && controllers[index].caps.SubType != XINPUT_DEVSUBTYPE_GAMEPAD) ret = ERROR_DEVICE_NOT_CONNECTED; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/11158
Rémi Bernon (@rbernon) commented about dlls/xinput1_3/main.c:
+} + +static void controllers_unlock_all_except(int slot) +{ + int i; + + for (i = XUSER_MAX_COUNT; i > 0; i--) + { + if (i - 1 != slot) LeaveCriticalSection(&controllers[i - 1].crit); + } +} + +static void controllers_unlock_all(void) +{ + controllers_unlock_all_except(-1); +} What about just replacing all the CS with a global static one?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/11158#note_143232
On Tue Jun 16 11:43:05 2026 +0000, Rémi Bernon wrote:
What about just replacing all the CS with a global static one? We could, but this could increase contention when there are multiple controllers connected. I'm not sure how important this is though.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/11158#note_143262
On Tue Jun 16 16:43:13 2026 +0000, Shaun Ren wrote:
We could, but this could increase contention when there are multiple controllers connected. I'm not sure how important this is though. I don't think it can be worse than locking four different CS.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/11158#note_143266
On Tue Jun 16 18:40:02 2026 +0000, Rémi Bernon wrote:
I don't think it can be worse than locking four different CS. We're only locking all of them for addition/removal path; it is possible for the update thread to be in `read_controller_state()` for a controller while the game calls GetState for another controller, and using a global CS here would cause the game's GetState call to stall.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/11158#note_143275
On Tue Jun 16 19:02:48 2026 +0000, Shaun Ren wrote:
We're only locking all of them for addition/removal path; it is possible for the update thread to be in `read_controller_state()` for a controller while the game calls GetState for another controller, and using a global CS here would cause the game's GetState call to stall. I did some basic instrumentation on this. During my one minute testing with RiME, the times took for `read_controller_state()` to complete is on average 23+/-32 us, however there were 11 spikes above 500 us, with the maximum almost 2 ms.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/11158#note_143302
On Tue Jun 16 23:45:19 2026 +0000, Shaun Ren wrote:
I did some basic instrumentation on this. During my one minute testing with RiME, the times took for `read_controller_state()` to complete is on average 23+/-32 us, however there were 11 spikes above 500 us, with the maximum almost 2 ms. What is taking that long? Is this the ReadFile call that initiates an async read? Could we somehow move it out of the CS?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/11158#note_143317
participants (3)
-
Rémi Bernon (＠rbernon) -
Shaun Ren
-
Shaun Ren (＠shaunren)