bluetooth: services: ras: fix RRSP disconnect deadlock - #30805
Conversation
b422f8b to
d426c32
Compare
CI InformationTo view the history of this post, click the 'edited' button above Inputs:Sources:sdk-nrf: PR head: a3d949abdc332f50584b45b655f2ba80a9000578 more detailssdk-nrf:
Github labels
List of changed files detected by CI (2)Outputs:ToolchainVersion: e214c2a47c Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped;
|
There was a problem hiding this comment.
Pull request overview
Defers RRSP teardown to its work queue to prevent disconnect deadlocks.
Changes:
- Adds asynchronous teardown and disconnect-state guards.
- Documents delayed instance reuse.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
subsys/bluetooth/services/ras/rrsp/ras_rrsp.c |
Implements deferred teardown and work suppression. |
include/bluetooth/services/ras.h |
Documents asynchronous teardown behavior. |
Suppressed comments (2)
subsys/bluetooth/services/ras/rrsp/ras_rrsp.c:151
- Setting
conntoNULLmakes this slot allocatable while itsteardown_workhandler is still executing. A concurrent allocation for another connection can select the slot,memsetit, and reinitialize the same runningk_work; it can also overwrite instance state before this handler returns. Keep pool availability synchronized with work completion, for example by initializing work objects once and managing slot state separately under a lock.
bt_conn_unref(rrsp->conn);
rrsp->conn = NULL;
rrsp->disconnecting = false;
subsys/bluetooth/services/ras/rrsp/ras_rrsp.c:173
- The disconnected instance continues occupying its pool slot until this low-priority queue reaches teardown. With auto-allocation enabled and
CONFIG_BT_RAS_RRSP_MAX_ACTIVE_CONN < CONFIG_BT_MAX_CONN, a new connection arriving in that interval gets-ENOMEMfromconnected()'s unchecked allocation and is never assigned an RRSP instance. Add an allocation handoff/retry after teardown or release/reserve pool capacity synchronously.
(void)k_work_submit_to_queue(&rrsp_wq, &rrsp->teardown_work);
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| (void)k_work_cancel(&rrsp->send_data_work); | ||
| (void)k_work_cancel(&rrsp->rascp_work); | ||
| (void)k_work_cancel(&rrsp->status_work); | ||
|
|
||
| if (rrsp->active_buf) { |
There was a problem hiding this comment.
I think this can be dismissed, as the timer is stopped in bt_ras_rrsp_free before the teardown work is submitted to the queue.
There was a problem hiding this comment.
I moved stopping of the timer to the teardown, that will catch the case where rd_segment_send starts the timer it gets preempted by the disconnected callback too, because the teardown will happen after rd_segment_send complets.
| if (rrsp->active_buf) { | ||
| bt_ras_rd_buffer_release(rrsp->active_buf); |
There was a problem hiding this comment.
I think this comment wsa good, i removed this code, bt_ras_rd_buffer_release wasnt called in cleanup before this commit either . so this was a mistake to add here.
|
You can find the documentation preview for this PR here. |
KyraLengfeld
left a comment
KyraLengfeld
left a comment
There was a problem hiding this comment.
Generally looks good, and I think the one copilot comment can be dismissed (commented there), but the possibly overwritten buffer concern should be checked. (I haven't looked at the file copilot is quoting there, let me know if I should).
Jenkins is also failing please have a look at it. (wfh today and the vpn app is refusing me right now, so cannot check what)
Also, this seems known-issue worthy, but would leave it to @alexstanoev-nordic how it is handled for services. (In any case a label needs to be added).
| (void)k_work_cancel(&rrsp->send_data_work); | ||
| (void)k_work_cancel(&rrsp->rascp_work); | ||
| (void)k_work_cancel(&rrsp->status_work); | ||
|
|
||
| if (rrsp->active_buf) { |
There was a problem hiding this comment.
I think this can be dismissed, as the timer is stopped in bt_ras_rrsp_free before the teardown work is submitted to the queue.
Defer instance teardown to rrsp_wq instead of calling k_work_queue_drain() from the connection disconnect callback. Blocking sysworkq on rrsp_wq could deadlock when send_data_work was waiting for att_pool buffers that are only recycled on sysworkq. Stop new RRSP work after disconnect with a disconnecting flag. Signed-off-by: Martin Tverdal <martin.tverdal@nordicsemi.no>
d426c32 to
a3d949a
Compare
Defer instance teardown to rrsp_wq instead of calling k_work_queue_drain() from the connection disconnect callback.
Blocking sysworkq on rrsp_wq could deadlock when send_data_work was waiting for att_pool buffers that are only recycled on sysworkq. Stop new RRSP work after disconnect with a disconnecting flag.