Skip to content

Commit

Permalink
bluetooth: hci_driver: Fix deadlock in MPSL workq
Browse files Browse the repository at this point in the history 
This commit fixes deadlock in MPSL workq caused by bt_buf_get_rx with
K_FOREVER.

Previously, calling bt_buf_get_rx with K_FOREVER could block
indefinitely if the requested pool had no available buffers. This
blocking affected the MPSL work queue, preventing it from processing
other work items, including critical timeslot events like
MPSL_TIMESLOT_SIGNAL_CANCELLED and MPSL_TIMESLOT_SIGNAL_BLOCKED.

The issue becomes more severe if a background flash operation coincides
with this scenario. Flash operations often execute on the system work
queue (sysworkq), which can delay host work items responsible for
freeing buffers in the RX pool. In such cases:

- Flash operations stall, waiting for a timeslot event from the MPSL
  work queue.
- The MPSL work queue remains blocked by bt_buf_get_rx.
- This results in a deadlock, causing the flash operation to timeout and
  triggering a warning.

This commit modifies the HCI driver to call bt_buf_get_rx with
K_NO_WAIT. If no buffer is immediately available, it relies on a
callback to notify when a buffer is freed. This change ensures the MPSL
work queue remains unblocked, allowing other work items to execute while
waiting for a buffer.

Signed-off-by: Pavel Vasilyev <[email protected]>
  • Loading branch information
@PavelVPV
PavelVPV committed Jan 7, 2025
1 parent 7b49c65 commit 231ada2
Showing 1 changed file with 80 additions and 35 deletions.
115 changes: 80 additions & 35 deletions subsys/bluetooth/controller/hci_driver.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -328,6 +328,23 @@ static inline void receive_signal_raise(void)
mpsl_work_submit(&receive_work);
}

/** Storage for HCI packets from controller to host */
static struct {
/* Buffer for the HCI packet. */
uint8_t buf[HCI_RX_BUF_SIZE];
/* Type of the HCI packet the buffer contains. */
sdc_hci_msg_type_t type;
} rx_hci_msg;

static void bt_buf_rx_freed_cb(enum bt_buf_type type_mask)
{
if (((rx_hci_msg.type == SDC_HCI_MSG_TYPE_EVT && (type_mask & BT_BUF_EVT) != 0u) ||
(rx_hci_msg.type == SDC_HCI_MSG_TYPE_DATA && (type_mask & BT_BUF_ACL_IN) != 0u) ||
(rx_hci_msg.type == SDC_HCI_MSG_TYPE_ISO && (type_mask & BT_BUF_ISO_IN) != 0u))) {
receive_signal_raise();
}
}

static int cmd_handle(struct net_buf *cmd)
{
LOG_DBG("");
Expand Down Expand Up @@ -429,16 +446,16 @@ static int hci_driver_send(const struct device *dev, struct net_buf *buf)
return err;
}

static void data_packet_process(const struct device *dev, uint8_t *hci_buf)
static int data_packet_process(const struct device *dev, uint8_t *hci_buf)
{
struct net_buf *data_buf = bt_buf_get_rx(BT_BUF_ACL_IN, K_FOREVER);
struct net_buf *data_buf = bt_buf_get_rx(BT_BUF_ACL_IN, K_NO_WAIT);
struct bt_hci_acl_hdr *hdr = (void *)hci_buf;
uint16_t hf, handle, len;
uint8_t flags, pb, bc;

if (!data_buf) {
LOG_ERR("No data buffer available");
return;
LOG_DBG("No data buffer available");
return -ENOBUFS;
}

len = sys_le16_to_cpu(hdr->len);
Expand All @@ -452,8 +469,7 @@ static void data_packet_process(const struct device *dev, uint8_t *hci_buf)
LOG_ERR("Event buffer too small. %u > %u",
len + sizeof(*hdr),
HCI_RX_BUF_SIZE);
k_panic();
return;
return -ENOMEM;
}

LOG_DBG("Data: handle (0x%02x), PB(%01d), BC(%01d), len(%u)", handle,
Expand All @@ -464,28 +480,36 @@ static void data_packet_process(const struct device *dev, uint8_t *hci_buf)
struct hci_driver_data *driver_data = dev->data;

driver_data->recv_func(dev, data_buf);

return 0;
}

static void iso_data_packet_process(const struct device *dev, uint8_t *hci_buf)
static int iso_data_packet_process(const struct device *dev, uint8_t *hci_buf)
{
struct net_buf *data_buf = bt_buf_get_rx(BT_BUF_ISO_IN, K_FOREVER);
struct net_buf *data_buf = bt_buf_get_rx(BT_BUF_ISO_IN, K_NO_WAIT);
struct bt_hci_iso_hdr *hdr = (void *)hci_buf;

uint16_t len = sys_le16_to_cpu(hdr->len);

if (!data_buf) {
LOG_DBG("No data buffer available");
return -ENOBUFS;
}

if (len + sizeof(*hdr) > HCI_RX_BUF_SIZE) {
LOG_ERR("Event buffer too small. %u > %u",
len + sizeof(*hdr),
HCI_RX_BUF_SIZE);
k_panic();
return;
return -ENOMEM;
}

net_buf_add_mem(data_buf, &hci_buf[0], len + sizeof(*hdr));

struct hci_driver_data *driver_data = dev->data;

driver_data->recv_func(dev, data_buf);
(void)driver_data->recv_func(dev, data_buf);

return 0;
}

static bool event_packet_is_discardable(const uint8_t *hci_buf)
Expand Down Expand Up @@ -532,7 +556,7 @@ static bool event_packet_is_discardable(const uint8_t *hci_buf)
}
}

static void event_packet_process(const struct device *dev, uint8_t *hci_buf)
static int event_packet_process(const struct device *dev, uint8_t *hci_buf)
{
bool discardable = event_packet_is_discardable(hci_buf);
struct bt_hci_evt_hdr *hdr = (void *)hci_buf;
Expand All @@ -542,8 +566,7 @@ static void event_packet_process(const struct device *dev, uint8_t *hci_buf)
LOG_ERR("Event buffer too small. %u > %u",
hdr->len + sizeof(*hdr),
HCI_RX_BUF_SIZE);
k_panic();
return;
return -ENOMEM;
}

if (hdr->evt == BT_HCI_EVT_LE_META_EVENT) {
Expand All @@ -569,67 +592,85 @@ static void event_packet_process(const struct device *dev, uint8_t *hci_buf)
LOG_DBG("Event (0x%02x) len %u", hdr->evt, hdr->len);
}

evt_buf = bt_buf_get_evt(hdr->evt, discardable,
discardable ? K_NO_WAIT : K_FOREVER);
evt_buf = bt_buf_get_evt(hdr->evt, discardable, K_NO_WAIT);

if (!evt_buf) {
if (discardable) {
LOG_DBG("Discarding event");
return;
return 0;
}

LOG_ERR("No event buffer available");
return;
LOG_DBG("No event buffer available");
return -ENOBUFS;
}

net_buf_add_mem(evt_buf, &hci_buf[0], hdr->len + sizeof(*hdr));

struct hci_driver_data *driver_data = dev->data;

driver_data->recv_func(dev, evt_buf);
(void)driver_data->recv_func(dev, evt_buf);

return 0;
}

static bool fetch_and_process_hci_msg(const struct device *dev, uint8_t *p_hci_buffer)
static int fetch_hci_msg(uint8_t *p_hci_buffer, sdc_hci_msg_type_t *msg_type)
{
int errcode;
sdc_hci_msg_type_t msg_type;

errcode = MULTITHREADING_LOCK_ACQUIRE();
if (!errcode) {
errcode = hci_internal_msg_get(p_hci_buffer, &msg_type);
errcode = hci_internal_msg_get(p_hci_buffer, msg_type);
MULTITHREADING_LOCK_RELEASE();
}

if (errcode) {
return false;
}
return errcode;
}

static int process_hci_msg(const struct device *dev, uint8_t *p_hci_buffer,
sdc_hci_msg_type_t msg_type)
{
int err;

if (msg_type == SDC_HCI_MSG_TYPE_EVT) {
event_packet_process(dev, p_hci_buffer);
err = event_packet_process(dev, p_hci_buffer);
} else if (msg_type == SDC_HCI_MSG_TYPE_DATA) {
data_packet_process(dev, p_hci_buffer);
err = data_packet_process(dev, p_hci_buffer);
} else if (msg_type == SDC_HCI_MSG_TYPE_ISO) {
iso_data_packet_process(dev, p_hci_buffer);
err = iso_data_packet_process(dev, p_hci_buffer);
} else {
if (!IS_ENABLED(CONFIG_BT_CTLR_SDC_SILENCE_UNEXPECTED_MSG_TYPE)) {
LOG_ERR("Unexpected msg_type: %u. This if-else needs a new branch",
msg_type);
}
err = 0;
}

return true;
return err;
}

void hci_driver_receive_process(void)
{
static uint8_t hci_buf[HCI_RX_BUF_SIZE];

const struct device *dev = DEVICE_DT_GET(DT_DRV_INST(0));
int err;

if (fetch_and_process_hci_msg(dev, &hci_buf[0])) {
/* Let other threads of same priority run in between. */
receive_signal_raise();
if (rx_hci_msg.type == SDC_HCI_MSG_TYPE_NONE &&
fetch_hci_msg(&rx_hci_msg.buf[0], &rx_hci_msg.type) != 0) {
return;
}

err = process_hci_msg(dev, &rx_hci_msg.buf[0], rx_hci_msg.type);
if (err == 0) {
rx_hci_msg.type = SDC_HCI_MSG_TYPE_NONE;
} else if (err == -ENOBUFS) {
/* If we got -ENOBUFS, wait for the signal from the host. */
return;
} else {
LOG_ERR("Unknown error when processing hci message %d", err);
k_panic();
}

/* Let other threads of same priority run in between. */
receive_signal_raise();
}

static void receive_work_handler(struct k_work *work)
Expand Down Expand Up @@ -1410,6 +1451,8 @@ static int hci_driver_open(const struct device *dev, bt_hci_recv_t recv_func)

driver_data->recv_func = recv_func;

bt_buf_rx_freed_cb_set(bt_buf_rx_freed_cb);

return 0;
}

Expand Down Expand Up @@ -1442,6 +1485,8 @@ static int hci_driver_close(const struct device *dev)

MULTITHREADING_LOCK_RELEASE();

bt_buf_rx_freed_cb_set(NULL);

return err;
}

Expand Down

0 comments on commit 231ada2

Please sign in to comment.