bluetooth: buf: Fix callback protection for ISR context #100308
Conversation
PavelVPV
force-pushed
the
fix_calling_sync_prim_on_isr
branch
from
870a19a to
b13376b
Compare
zephyrbot
added
area: Bluetooth ISO
zephyrbot
requested review from
HaavardRei,
Thalley,
alwa-nordic,
cvinayak,
hermabe,
jhedberg,
rugeGerritsen and
sjanc
There was a problem hiding this comment.
Pull request overview
This PR fixes an assertion failure when buffer freed callbacks are invoked from ISR context by replacing scheduler locks with atomic pointer operations for thread-safe callback access. The key change enables buf_rx_freed_notify() to be safely called from both thread and ISR contexts.
- Replaced direct callback pointer access with atomic pointer operations (
atomic_ptr_t,atomic_ptr_get,atomic_ptr_set) - Made scheduler locking conditional based on execution context (thread vs ISR)
- Updated documentation to clarify behavior differences between thread and ISR contexts
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| subsys/bluetooth/host/buf.c | Implements ISR-safe callback handling by using atomic pointer operations and conditional scheduler locking based on execution context |
| subsys/bluetooth/host/iso.c | Updates ISO buffer callback to use atomic pointer operations for consistency with buf.c changes |
| include/zephyr/bluetooth/buf.h | Updates callback documentation to clarify scheduler lock behavior differs between thread and ISR contexts |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * callee must not perform any action that makes the current thread unready. When called from ISR | ||
| * context, the callback runs without scheduler lock. | ||
| * | ||
| * |
There was a problem hiding this comment.
Extra blank comment line. Remove this line to maintain consistent documentation formatting.
| * |
include/zephyr/bluetooth/buf.h
Outdated
| * @warning When this callback is called, the scheduler is locked and the callee must not perform | ||
| * any action that makes the current thread unready. This callback must only be used for very | ||
| * short non-blocking operation (e.g. submitting a work item). | ||
| * @warning This callback must only be used for very short non-blocking operations (e.g. submitting |
There was a problem hiding this comment.
Extra space between "operations" and "(e.g.".
| * @warning This callback must only be used for very short non-blocking operations (e.g. submitting | |
| * @warning This callback must only be used for very short non-blocking operations (e.g. submitting |
Thalley
left a comment
Thalley
left a comment
There was a problem hiding this comment.
Minor comments w.r.t. complying with https://docs.zephyrproject.org/latest/contribute/coding_guidelines/index.html rule 85, but otherwise I think this is good. Not sure if the out-of-order change is something that needs to be considered?
PavelVPV
dismissed stale reviews from KyraLengfeld and HaavardRei
via
bf6ee42
PavelVPV
force-pushed
the
fix_calling_sync_prim_on_isr
branch
2 times, most recently
from
bf6ee42 to
8110d9d
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| bool in_isr = k_is_in_isr(); | ||
|
|
||
| if (buf_rx_freed_cb) { | ||
| buf_rx_freed_cb(mask); | ||
| if (!in_isr) { | ||
| k_sched_lock(); | ||
| } | ||
|
|
||
| k_sched_unlock(); | ||
| cb = (bt_buf_rx_freed_cb_t)atomic_ptr_get(&buf_rx_freed_cb); |
There was a problem hiding this comment.
Race condition: The ISR status check is performed before acquiring the scheduler lock and reading the callback pointer. If the thread is preempted between checking in_isr and calling k_sched_lock(), the callback pointer could be modified. Move the atomic_ptr_get call before the k_sched_lock() to ensure atomic read happens first, then protect the callback execution with the scheduler lock.
There was a problem hiding this comment.
Calling atomic_ptr_get before k_sched_lock will increase the window where the thread could be preempted before k_sched_lock is called, thus will brake backward compatibility. It is perfectly fine if callback has been changed before k_sched_lock is called.
include/zephyr/bluetooth/buf.h
Outdated
| * Don't unset a callback while it might be executing. Don't free callback resources until after | ||
| * unregistering. |
There was a problem hiding this comment.
The guidance 'Don't unset a callback while it might be executing' is insufficient given the atomic operations now used. The documentation should specify the proper synchronization mechanism or grace period needed (e.g., ensuring all pending callbacks complete before unregistering, or using a specific barrier). Without this, users may not understand how to safely unregister callbacks.
| * Don't unset a callback while it might be executing. Don't free callback resources until after | |
| * unregistering. | |
| * Do not unset (unregister) a callback while it might be executing. Before unregistering or freeing | |
| * callback resources, you must ensure that no callback is currently executing and that no further | |
| * callbacks can be invoked. This typically requires appropriate synchronization, such as disabling | |
| * interrupts, using a barrier, or waiting for all pending callbacks to complete, depending on your | |
| * application context and the execution environment. |
There was a problem hiding this comment.
This is too verbose IMHO.
Fix assertion failure when buf_rx_freed_notify() is called from ISR by replacing k_sched_lock with atomic_ptr operations for callback pointer access. The scheduler lock is retained during callback execution in thread context to maintain backward compatibility, but is skipped in ISR context where it's not available. Updated documentation to clarify the behavior difference between thread and ISR contexts. Signed-off-by: Pavel Vasilyev <[email protected]>
PavelVPV
dismissed stale reviews from KyraLengfeld, HaavardRei, and Thalley
via
e530b15
PavelVPV
force-pushed
the
fix_calling_sync_prim_on_isr
branch
from
8110d9d to
e530b15
Compare
PavelVPV
requested review from
HaavardRei,
KyraLengfeld,
Thalley and
alwa-nordic
|
7 participants
Fix assertion failure when buf_rx_freed_notify() is called from ISR by replacing k_sched_lock with atomic_ptr operations for callback pointer access.
The scheduler lock is retained during callback execution in thread context to maintain backward compatibility, but is skipped in ISR context where it's not available.
Updated documentation to clarify the behavior difference between thread and ISR contexts.