From aad73d42382617d9fdd64818208dbf2d9f0a6caa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20Desbiens?= Date: Thu, 4 Jun 2026 15:43:05 -0400 Subject: [PATCH] Fix standalone locking bugs in HID host idle get/set Two issues discovered while reviewing and fixing thread-safety issues in the new ux_host_class_hid_protocol_get/set functions (PR #244). 1. idle_get.c: wrong operator acquires no lock in standalone mode Line 104 used '&= ~UX_HOST_CLASS_HID_FLAG_LOCK' (the release/clear operation) instead of '|= UX_HOST_CLASS_HID_FLAG_LOCK' (acquire/set). The preceding check correctly returns UX_BUSY when the flag is set, but the follow-on line then immediately clears it instead of setting it. The net effect is that the HID instance is never actually locked in UX_HOST_STANDALONE mode, making the mutual-exclusion check a no-op. 2. idle_set.c: standalone path used a blocking spin-loop The standalone branch called _ux_host_class_hid_idle_set_run() in a do/while loop, blocking the caller until the transfer completed. This is inconsistent with every other inline HID control-transfer function (idle_get, report_get, report_set, protocol_get, protocol_set) which all use the direct UX_DISABLE/UX_RESTORE atomic flag pattern and return UX_BUSY if the instance or device endpoint is already locked. Replaced with the same inline standalone locking pattern used by the other functions: atomic HID FLAG_LOCK acquire, atomic DEVICE_FLAG_LOCK acquire with AUTO_DEVICE_UNLOCK + UX_TRANSFER_STATE_RESET, and an AUTO_WAIT check at completion consistent with idle_get behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../src/ux_host_class_hid_idle_get.c | 2 +- .../src/ux_host_class_hid_idle_set.c | 49 +++++++++++++------ 2 files changed, 35 insertions(+), 16 deletions(-) diff --git a/common/usbx_host_classes/src/ux_host_class_hid_idle_get.c b/common/usbx_host_classes/src/ux_host_class_hid_idle_get.c index e12088424..e39f988a9 100644 --- a/common/usbx_host_classes/src/ux_host_class_hid_idle_get.c +++ b/common/usbx_host_classes/src/ux_host_class_hid_idle_get.c @@ -101,7 +101,7 @@ UINT status; UX_RESTORE return(UX_BUSY); } - hid -> ux_host_class_hid_flags &= ~UX_HOST_CLASS_HID_FLAG_LOCK; + hid -> ux_host_class_hid_flags |= UX_HOST_CLASS_HID_FLAG_LOCK; UX_RESTORE #else diff --git a/common/usbx_host_classes/src/ux_host_class_hid_idle_set.c b/common/usbx_host_classes/src/ux_host_class_hid_idle_set.c index fe74debd9..1611e9133 100644 --- a/common/usbx_host_classes/src/ux_host_class_hid_idle_set.c +++ b/common/usbx_host_classes/src/ux_host_class_hid_idle_set.c @@ -69,14 +69,8 @@ UINT _ux_host_class_hid_idle_set(UX_HOST_CLASS_HID *hid, USHORT idle_time, USHORT report_id) { #if defined(UX_HOST_STANDALONE) -UINT status; - do - { - status = _ux_host_class_hid_idle_set_run(hid, idle_time, report_id); - } while(status == UX_STATE_WAIT || status == UX_STATE_LOCK); - return(hid -> ux_host_class_hid_status); -#else - +UX_INTERRUPT_SAVE_AREA +#endif UX_ENDPOINT *control_endpoint; UX_TRANSFER *transfer_request; UINT status; @@ -99,6 +93,16 @@ UINT status; transfer_request = &control_endpoint -> ux_endpoint_transfer_request; /* Protect thread reentry to this instance. */ +#if defined(UX_HOST_STANDALONE) + UX_DISABLE + if (hid -> ux_host_class_hid_flags & UX_HOST_CLASS_HID_FLAG_LOCK) + { + UX_RESTORE + return(UX_BUSY); + } + hid -> ux_host_class_hid_flags |= UX_HOST_CLASS_HID_FLAG_LOCK; + UX_RESTORE +#else status = _ux_host_semaphore_get(&hid -> ux_host_class_hid_semaphore, UX_WAIT_FOREVER); if (status != UX_SUCCESS) return(status); @@ -106,17 +110,27 @@ UINT status; /* Protect the control endpoint semaphore here. It will be unprotected in the transfer request function. */ status = _ux_host_semaphore_get(&hid -> ux_host_class_hid_device -> ux_device_protection_semaphore, UX_WAIT_FOREVER); - - /* Check for status. */ if (status != UX_SUCCESS) { - - /* Something went wrong. */ - /* Unprotect thread reentry to this instance. */ _ux_host_semaphore_put(&hid -> ux_host_class_hid_semaphore); - return(status); } +#endif + +#if defined(UX_HOST_STANDALONE) + /* Protect the control endpoint. It will be unprotected in the transfer request function. */ + UX_DISABLE + if (hid -> ux_host_class_hid_device -> ux_device_flags & UX_DEVICE_FLAG_LOCK) + { + _ux_host_class_hid_unlock(hid); + UX_RESTORE + return(UX_BUSY); + } + hid -> ux_host_class_hid_device -> ux_device_flags |= UX_DEVICE_FLAG_LOCK; + transfer_request -> ux_transfer_request_flags |= UX_TRANSFER_FLAG_AUTO_DEVICE_UNLOCK; + UX_TRANSFER_STATE_RESET(transfer_request); + UX_RESTORE +#endif /* Create a transfer request for the SET_IDLE request. */ transfer_request -> ux_transfer_request_data_pointer = UX_NULL; @@ -129,12 +143,17 @@ UINT status; /* Send request to HCD layer. */ status = _ux_host_stack_transfer_request(transfer_request); +#if defined(UX_HOST_STANDALONE) + if (!(transfer_request -> ux_transfer_request_flags & UX_TRANSFER_FLAG_AUTO_WAIT)) + return(status); + _ux_host_class_hid_unlock(hid); +#else /* Unprotect thread reentry to this instance. */ _ux_host_semaphore_put(&hid -> ux_host_class_hid_semaphore); +#endif /* Return the function status. */ return(status); -#endif } /**************************************************************************/