Skip to content

Potentially blocking call within critical section #2004

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.

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

Open
gmarull opened this issue Mar 7, 2025 · 2 comments
Open

Potentially blocking call within critical section #2004

gmarull opened this issue Mar 7, 2025 · 2 comments
Labels

Comments

@gmarull
Copy link
Contributor

gmarull commented Mar 7, 2025

In the following controller code:

void
ble_ll_rfmgmt_release(void)
{
struct ble_ll_rfmgmt_data *rfmgmt = &g_ble_ll_rfmgmt_data;
os_sr_t sr;
OS_ENTER_CRITICAL(sr);
ble_ll_event_remove(&rfmgmt->release_ev);
if (g_ble_ll_rfmgmt_data.state != RFMGMT_STATE_OFF) {
ble_ll_event_add(&rfmgmt->release_ev);
}
OS_EXIT_CRITICAL(sr);
}

ble_ll_event_add is called while inside a critical section. This calls:

void
ble_ll_event_add(struct ble_npl_event *ev)
{
ble_npl_eventq_put(&g_ble_ll_data.ll_evq, ev);
}

In case of FreeRTOS,

static inline void
ble_npl_eventq_put(struct ble_npl_eventq *evq, struct ble_npl_event *ev)
{
npl_freertos_eventq_put(evq, ev);
}

void
npl_freertos_eventq_put(struct ble_npl_eventq *evq, struct ble_npl_event *ev)
{
BaseType_t woken;
BaseType_t ret;
if (ev->queued) {
return;
}
ev->queued = true;
if (in_isr()) {
ret = xQueueSendToBackFromISR(evq->q, &ev, &woken);
portYIELD_FROM_ISR(woken);
} else {
ret = xQueueSendToBack(evq->q, &ev, portMAX_DELAY);
}
assert(ret == pdPASS);
}

As you can see above, xQueueSendToBack is called with portMAX_DELAY. I guess at the very least, we should add a zero delay if we're within a critical section. Also, see https://forums.freertos.org/t/critical-sections-freertos-api-calls/17352.

@sjanc sjanc added the porting label Mar 7, 2025
@andrzej-kaczmarek
Copy link
Contributor

I think we could try to remove critical sections from rfmgmt code entirely. The only usage of rfmgmt outside LL task context seems to be the rfmgmt timer, at least this is the only I could find after a quick scan though the code. If we change that to either send an event to LL queue in order to serialize rfmgmt calls or replace the timer with a callout, critical sections should not be necessary.

@andrzej-kaczmarek
Copy link
Contributor

andrzej-kaczmarek commented Mar 7, 2025

Ah, I've just realized that sending an event from timer callback is not great idea as this would delay rfmgmt enable a bit which is not what we want there...

gmarull added a commit to teslabs/pebble-firmware that referenced this issue Mar 7, 2025
So that we do not hit assertions when called from critical section
context.

See apache/mynewt-nimble#2004

Signed-off-by: Gerard Marull-Paretas <gerard@teslabs.com>
gmarull added a commit to pebble-dev/pebble-firmware that referenced this issue Mar 7, 2025
So that we do not hit assertions when called from critical section
context.

See apache/mynewt-nimble#2004

Signed-off-by: Gerard Marull-Paretas <gerard@teslabs.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants