Fixed standalone locking bugs in HID host idle get/set#264
Merged
Conversation
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>
fdesbiens
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two bugs discovered while reviewing and fixing thread-safety issues in the new
ux_host_class_hid_protocol_get/setfunctions (PR #244).Changes
1.
idle_get.c: wrong operator acquires no lock in standalone modeLine 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 returnsUX_BUSYwhen 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 inUX_HOST_STANDALONEmode, making the mutual-exclusion check a no-op and leavingidle_getunprotected against concurrent calls.2.
idle_set.c: standalone path used a blocking spin-loopThe standalone branch called
_ux_host_class_hid_idle_set_run()in ado/whileloop, 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 directUX_DISABLE/UX_RESTOREatomic flag pattern and returnUX_BUSYif the instance or device endpoint is already locked.Replaced with the same inline standalone locking pattern used by the other functions: atomic
HID_FLAG_LOCKacquire, atomicDEVICE_FLAG_LOCKacquire withAUTO_DEVICE_UNLOCK+UX_TRANSFER_STATE_RESET, and anAUTO_WAITcheck at completion consistent withidle_getbehavior.