Bluetooth: controller: Fix Enc Setup overlap with Length Update #15958
Conversation
Fix the encryption setup queueing implementation to avoid overlapping with local initiated Length Update Procedure. Fixes zephyrproject-rtos#15733. Relates to zephyrproject-rtos#15335, and zephyrproject-rtos#15186. Signed-off-by: Vinayak Kariappa Chettimada <vich@nordicsemi.no>
cvinayak
added
bug
| #endif /* CONFIG_BT_CTLR_LE_ENC */ | ||
| 0) && | ||
| /* Encryption setup queued or data paused */ | ||
| (pause_tx || (conn->pkt_tx_head != conn->pkt_tx_ctrl)))) { |
There was a problem hiding this comment.
+1 for comments, that helps a lot.
But it is not obvious if all the corner cases of this massive-if are properly handled.
For complex branching like this, I personally like to factor out the logical meaning across const booleans, and let the booleans derive from the previous ones. With those, I can name them and comment on them to attain more readability.
There was a problem hiding this comment.
I agree this is complex (have been on my mind, and gave it almost a week before I pushed ;-) ). It is this way trying to short circuit as early as possible to reduce CPU usage. But I may be incorrect based on how the compiler/linker could optimize if the code was written in a more readable fashion.
Example, a connection with only empty) packets use less CPU in comparison to when data is in the queue. And during a active encryption procedure, the call to is_enc_req_pause_tx is completely bypassed by conn->pause_tx, etc.
I am open to alternative implementations that meet the constraints.
mped-oticon
left a comment
mped-oticon
left a comment
There was a problem hiding this comment.
I can't really tell if this fixes anything though, because the code is still complex.
But I will not gate the change because of readability.
Port the fix for the controller implementation to make start encryption queueable if there is any control procedure in progress. Refer to zephyrproject-rtos#15335. Relates to zephyrproject-rtos#15335, zephyrproject-rtos#15186, zephyrproject-rtos#15958 and zephyrproject-rtos#14636. Signed-off-by: Vinayak Kariappa Chettimada <vich@nordicsemi.no>
3 participants
Fix the encryption setup queueing implementation to avoid
overlapping with local initiated Length Update Procedure.
Fixes #15733.
Relates to #15335, and #15186.
Signed-off-by: Vinayak Kariappa Chettimada vich@nordicsemi.no