Avoid crashing after shutting down WiFi #4761
Conversation
esp-radio/src/wifi/mod.rs
Outdated
| pub(crate) fn esp_wifi_send_data(interface: wifi_interface_t, data: &mut [u8]) { | ||
| // even checking for !Uninitialized would be enough to not crash | ||
| // we won't hand out TX-tokens in this case but we might have returned a token before | ||
| if interface == wifi_interface_t_WIFI_IF_STA |
There was a problem hiding this comment.
Do we need these checks? From smoltcp's point of view, if we provide a token, we should be able to send the data.
There was a problem hiding this comment.
not too sure but my thinking was that we might hand out a token and some other thread might tear down everything after getting the token but before actually starting the transmit
we can be brave and remove it for now and see if someone runs into this problem 🤷♂️ but the check should be cheap
There was a problem hiding this comment.
not too sure but my thinking was that we might hand out a token and some other thread might tear down everything after getting the token but before actually starting the transmit
In that case this isn't a solution - if another thread can stop the wifi stack right in the middle of this, then it can stop the stack right after this check, too.
There was a problem hiding this comment.
So I guess we need to acquire an OS-mutex when we hand out a token, and release it when the packet has been sent. We need to acquire the same mutex when we try to change state.
There was a problem hiding this comment.
true - not sure if it's worth it? - lets remove it for now and see if someone manages to run into that (I left a comment there for "future us")
There was a problem hiding this comment.
(and yes - this is a consequence of detaching the interfaces from the controller's life-cycle 🙈 )
There was a problem hiding this comment.
we'll need some sort of mutexing (or rather a counting semaphore, where changing state needs to take all count) still, to prevent state change during transmission 🤷♂️
There was a problem hiding this comment.
I guess the only problematic case (having one is already enough - yes) should be de-init / uninitialized state
Other state transitions should be handled by the driver gracefully - might be worth thinking for a moment about first
bjoernQ
force-pushed
the
fix-crash-shutting-down-wifi
branch
from
234ebee to
c789893
Compare
bjoernQ
force-pushed
the
fix-crash-shutting-down-wifi
branch
from
1cce31c to
b82e764
Compare
| dump_packet_info(data); | ||
| // `esp_wifi_internal_tx` will crash if wifi is uninitialized or de-inited | ||
|
|
||
| state::locked(|| { |
There was a problem hiding this comment.
Doesn't this limit us to one in-flight packet at a time? I think we should only give back the semaphore in esp_wifi_tx_done_cb, or on an error, basically inside decrement_inflight_counter or when we would otherwise call it.
It's also a bit too late to lock here, we've already given the token, we should be able to send the packet.
There was a problem hiding this comment.
It shouldn't limit us to one? - the assumption (which is hard to prove) is that the blobs handle the inflight tx gracefully during tear-down - we just can't schedule another transmission once the buffers etc. are gone.
If that assumption is true we might not even end up in esp_wifi_tx_done_cb - oh wait - that would mean we should reset the inflight counter before creating a (new) controller 🤔
There was a problem hiding this comment.
Yeah it sounds much easier and robust to prevent shutdown if there are any tranmissions running 😅 With the added caveat that attempting to tear the whole system down should block any new packets from being sent, or network traffic can prevent the driver from being shut down.
3 participants
Thank you for your contribution!
We appreciate the time and effort you've put into this pull request.
To help us review it efficiently, please ensure you've gone through the following checklist:
Submission Checklist 📝
cargo xtask fmt-packagescommand to ensure that all changed code is formatted correctly.CHANGELOG.mdin the proper section.Extra:
Pull Request Details 📖
Description
See #4639 for a reliable reproducer
Testing
Running the reproducer