Skip to content

rp2: Run USB stack task exclusively from core 0. #8836

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

rp2: Run USB stack task exclusively from core 0. #8836

wants to merge 1 commit into from

Conversation

jimmo
Copy link
Member

@jimmo jimmo commented Jun 29, 2022

The goal is to avoid a situation where core 1 is shut down while holding
the tinyusb spinlock, which could happen during soft reset if
mp_thread_deinit is called while core1 is running tud_task().

This also fixes a latent race where the two cores are competing to
decrement and compare vm_hook_divisor with no mem fence or atomic
protection -- only core0 will now do this.

This addresses the first comment on #8494 (comment)

It's easy to reproduce with exactly the minimal example from #8494. If I copy that over as main.py, then I get a lock up about 4 times out of 5 on soft reset at the repl.

Some notes for discussion:

  • There are potentially other ways that core1 could acquire the usb lock. Perhaps we should solve this more like rp2: Ensure core1 doesn't hold gc lock in deinit. #8835 where we require core0 to acquire the tinyusb lock during mp_thread_deinit.
  • Are there any disadvantages to only running tud_task on core0?
  • Can we solve this more generally, given that there are other locks that need to survive soft reset? (e.g. LWIP is the next on my list to investigate). Something along the lines of having core1 permanently hold a mutex that it bounces in MICROPY_EVENT_POLL_HOOK or MICROPY_VM_HOOK_POLL, and deinit acquires that lock before shutdown. Therefore it would only ever be shutdown in a "safe" location.

This was referenced Jun 29, 2022
Closed
Closed
@dpgeorge
Copy link
Member

I did want to see if it's possible to run the TinyUSB ISR code from the IRQ itself, which would completely remove the hook code from the VM. But that is a task for later. For now the fix here looks OK.

There are potentially other ways that core1 could acquire the usb lock

Yes, and I guess one of those is if core1 writes out to the USB CDC (tud_cdc_write and tud_cdc_write_flush). That code path should probably be checked, eg a tight loop with both cores printing out stuff.

Are there any disadvantages to only running tud_task on core0?

Not that I can think of. This call is not time critical, so as long as it's eventually called it should be OK.

Can we solve this more generally, given that there are other locks that need to survive soft reset? (e.g. LWIP is the next on my list to investigate). Something along the lines of having core1 permanently hold a mutex that it bounces

Doing it more generally does sound good. But bouncing it in MICROPY_VM_HOOK_POLL may be expensive. Would need to profile it.

@dpgeorge
Copy link
Member

Instead of bouncing a lock, maybe we can have a single, global flag which is set by either core to indicate that it wants to synchronise? Then there can be a single uniform check of this flag in MICROPY_VM_HOOK_POLL, that is independent of which core is running the code.

@jimmo jimmo mentioned this pull request Jun 30, 2022
Closed
dpgeorge
dpgeorge reviewed Jul 5, 2022
View reviewed changes
ports/rp2/mpconfigport.h Outdated
@@ -238,7 +238,7 @@ extern const struct _mod_network_nic_type_t mod_network_nic_type_wiznet5k;
extern void mp_handle_pending(bool); \
mp_handle_pending(true); \
best_effort_wfe_or_timeout(make_timeout_time_ms(1)); \
MICROPY_HW_USBDEV_TASK_HOOK \
if (get_core_num() == 0) { MICROPY_HW_USBDEV_TASK_HOOK } \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are other calls to MICROPY_HW_USBDEV_TASK_HOOK which I think also need to be handled this way.

I'll do a refactor first.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #8863.

@dpgeorge dpgeorge mentioned this pull request Jul 5, 2022
Merged
@dpgeorge
Copy link
Member

dpgeorge commented Jul 5, 2022

Please rebase on latest master.

@jimmo
rp2: Run USB stack task exclusively from core 0.
c37e703 
The goal is to avoid a situation where core 1 is shut down while holding
the tinyusb spinlock, which could happen during soft reset if
mp_thread_deinit is called while core1 is running tud_task().

This also fixes a latent race where the two cores are competing to
decrement and compare `vm_hook_divisor` with no mem fence or atomic
protection -- only core0 will now do this.

Signed-off-by: Jim Mussared <jim.mussared@gmail.com>
@jimmo
Copy link
Member Author

jimmo commented Jul 6, 2022

Done. #8845 should probably be merged first though.

@dpgeorge
Copy link
Member

dpgeorge commented Jul 6, 2022

Done. #8845 should probably be merged first though.

Thanks. Yes I agree 8845 should be merged first (but see comments on that PR).

@dpgeorge
Copy link
Member

Merged in daff597

@dpgeorge dpgeorge closed this Jul 12, 2022
tannewt pushed a commit to tannewt/circuitpython that referenced this pull request Jan 25, 2024
@dhalbert
Merge pull request micropython#8836 from supcik/picomo
b2c7e14 
Add support for the PicoMo board
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dpgeorge dpgeorge dpgeorge left review comments

Assignees
No one assigned
Labels
port-rp2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jimmo @dpgeorge