[Core] Cleanups to USB suspend logic #21537

KarlK90 · 2023-07-15T16:00:40Z

Description

This is a followup on #19780.

TBA.

Types of Changes

Issues Fixed or Closed by This PR

Checklist

My code follows the code style of this project: C, Python
I have read the PR Checklist document and have made the appropriate changes.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
I have tested the changes and verified that they work and don't break anything (as well as I can manage).

platforms/chibios/suspend.c

tmk_core/protocol/chibios/usb_main.c

tmk_core/protocol/chibios/chibios.c

sigprof · 2023-07-15T18:11:27Z

tmk_core/protocol/chibios/usb_main.c

@@ -534,40 +530,35 @@ static void usb_event_cb(USBDriver *usbp, usbevent_t event) {
                }
                qmkusbConfigureHookI(&drivers.array[i].driver);
            }
-            osalSysUnlockFromISR();
            if (last_suspend_state) {
                usb_event_queue_enqueue(USB_EVENT_WAKEUP);
            }


One remaining problem here is that last_suspend_state is set in usb_event_queue_task() (and without any locking too). If usb_event_queue_task() does not get called in time (which may happen if someone sticks a large delay into some code), we may potentially get here with a stale value for last_suspend_state, and therefore lose the wakeup event in case the wakeup goes through the full reconfiguration. Although getting that case after the other changes in the PR would probably be harder than before (with the older code you could get this broken state for any resume that ends up reconfiguring devices, because USB_EVENT_SUSPEND was not processed in time).

The options here are:

Move all handling of last_suspend_state into usb_event_cb() (set it on USB_EVENT_SUSPEND and USB_EVENT_WAKEUP, and also after generating the USB_EVENT_WAKEUP event in the USB_EVENT_CONFIGURED path).

Move all handling of last_suspend_state into usb_event_queue_task() (if USB_EVENT_CONFIGURED comes after USB_EVENT_SUSPEND, but without USB_EVENT_WAKEUP, perform wakeup processing too).

Thanks for spotting! I did the first approach and moved it into the callback and hopefully didn't miss anything.

dexter93 · 2023-07-24T22:07:09Z

tmk_core/protocol/chibios/usb_main.c

        case USB_EVENT_UNCONFIGURED:
-            /* Falls into.*/


maybe keep these comments for readability?

Hmmn, I'm not convinced that it actually adds readability. Though that is ofc subjective.

zvecr · 2023-07-28T00:06:15Z

Having issues with the current code on macos, will update this comment with more context asap.

suspend_wakeup_init isn't called from an ISR context anymore, thus we can use the regular clear_keyboard function in this context, which will also send a report to the host to reset all keys which might still be registered as held.

Otherwise a incoming USB_EVENT_SUSPEND event might not be processed before falling into the sleeping loop.

The queue is written from an ISR context and read from the main thread which can lead to race conditions if the main thread is pre-empted while accessing the queue. Therefore all interactions are moved into critical sections.

As this was previously accessed from both ISR and main thread, which can lead to race conditions as well.

...at this point the USB driver is already suspended so the message will only be printed after resuming the operation, which is kind of pointless.

github-actions · 2023-11-12T01:50:54Z

Thank you for your contribution!
This pull request has been automatically marked as stale because it has not had activity in the last 45 days. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, or re-open when it's ready.
For maintainers: Please label with bug, awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

github-actions · 2023-12-13T01:50:06Z

Thank you for your contribution!
This pull request has been automatically closed because it has not had activity in the last 30 days. Please feel free to give a status update now, ping for review, or re-open when it's ready.
// [stale-action-closed]

KarlK90 requested a review from a team July 15, 2023 16:00

github-actions bot added the core label Jul 15, 2023

KarlK90 requested review from sigprof and zvecr July 15, 2023 16:00

sigprof requested changes Jul 15, 2023

View reviewed changes

KarlK90 force-pushed the maintenance/usb-suspend-logic branch 2 times, most recently from c963338 to 6916f10 Compare July 15, 2023 18:54

KarlK90 requested a review from sigprof July 15, 2023 18:55

dexter93 reviewed Jul 24, 2023

View reviewed changes

github-actions bot added the keyboard label Jul 25, 2023

KarlK90 force-pushed the maintenance/usb-suspend-logic branch 3 times, most recently from f4f713a to 6a88a07 Compare July 27, 2023 18:17

github-actions bot removed the keyboard label Jul 27, 2023

drashna approved these changes Jul 27, 2023

View reviewed changes

drashna requested a review from a team July 27, 2023 19:47

dexter93 mentioned this pull request Aug 2, 2023

[Core] Refactor ChibiOS USB endpoints to be fully async #21656

Merged

14 tasks

KarlK90 added 6 commits August 2, 2023 17:45

Clear keyboard after wakeup

4f1caf2

suspend_wakeup_init isn't called from an ISR context anymore, thus we can use the regular clear_keyboard function in this context, which will also send a report to the host to reset all keys which might still be registered as held.

Move USB events processing before suspend handling

ede94b0

Otherwise a incoming USB_EVENT_SUSPEND event might not be processed before falling into the sleeping loop.

Move USB event queue access into critical section

ea232b0

The queue is written from an ISR context and read from the main thread which can lead to race conditions if the main thread is pre-empted while accessing the queue. Therefore all interactions are moved into critical sections.

Add GPL license header to suspend.c

ea44ac3

Move additional wakeup event generation into ISR

37b3194

As this was previously accessed from both ISR and main thread, which can lead to race conditions as well.

Remove 'suspending keyboard' log line

9abba9d

...at this point the USB driver is already suspended so the message will only be printed after resuming the operation, which is kind of pointless.

KarlK90 force-pushed the maintenance/usb-suspend-logic branch from 6a88a07 to 9abba9d Compare August 2, 2023 15:45

github-actions bot added the stale Issues or pull requests that have become inactive without resolution. label Nov 12, 2023

github-actions bot closed this Dec 13, 2023

KarlK90 reopened this Dec 13, 2023

KarlK90 added awaiting changes awaiting_pr Relies on another PR to be merged first in progress labels Dec 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Core] Cleanups to USB suspend logic #21537

[Core] Cleanups to USB suspend logic #21537

KarlK90 commented Jul 15, 2023 •

edited

Loading

sigprof Jul 15, 2023

KarlK90 Jul 15, 2023

dexter93 Jul 24, 2023

KarlK90 Jul 25, 2023

zvecr commented Jul 28, 2023

github-actions bot commented Nov 12, 2023

github-actions bot commented Dec 13, 2023

[Core] Cleanups to USB suspend logic #21537

Are you sure you want to change the base?

[Core] Cleanups to USB suspend logic #21537

Conversation

KarlK90 commented Jul 15, 2023 • edited Loading

Description

Types of Changes

Issues Fixed or Closed by This PR

Checklist

sigprof Jul 15, 2023

Choose a reason for hiding this comment

KarlK90 Jul 15, 2023

Choose a reason for hiding this comment

dexter93 Jul 24, 2023

Choose a reason for hiding this comment

KarlK90 Jul 25, 2023

Choose a reason for hiding this comment

zvecr commented Jul 28, 2023

github-actions bot commented Nov 12, 2023

github-actions bot commented Dec 13, 2023

KarlK90 commented Jul 15, 2023 •

edited

Loading