From e93cce2d1d84d6f0e43086e19fc55f1e40ba0be2 Mon Sep 17 00:00:00 2001 From: Johan Hedberg Date: Mon, 17 Jun 2019 15:54:45 +0300 Subject: [PATCH 1/3] Bluetooth: Add dedicated pool for HCI_Num_Completed_Packets HCI event This event is a priority one, so it's not safe to have it use the RX buffer pool which may be depleted due to non-priority events (e.g. advertising events). Since the event is consumed synchronously it's safe to have a single-buffer pool for it. Also introduce a new bt_buf_get_evt() API for HCI drivers to simplify the driver-side code, this effectively also deprecates bt_buf_get_cmd_complete() which now has no in-tree HCI driver users anymore. Fixes #16864 Signed-off-by: Johan Hedberg --- drivers/bluetooth/hci/h4.c | 11 +++----- drivers/bluetooth/hci/h5.c | 11 +------- drivers/bluetooth/hci/ipm_stm32wb.c | 6 +---- drivers/bluetooth/hci/spi.c | 7 ++--- drivers/bluetooth/hci/userchan.c | 11 +++----- include/bluetooth/buf.h | 12 +++++++++ subsys/bluetooth/controller/hci/hci.c | 4 +-- subsys/bluetooth/host/hci_core.c | 35 +++++++++++++++++++++++++ subsys/bluetooth/host/hci_ecc.c | 2 +- subsys/bluetooth/host/hci_raw.c | 12 +++++++++ tests/bluetooth/hci_prop_evt/src/main.c | 2 +- 11 files changed, 73 insertions(+), 40 deletions(-) diff --git a/drivers/bluetooth/hci/h4.c b/drivers/bluetooth/hci/h4.c index 39d923818a2c1..315925fb5cd4d 100644 --- a/drivers/bluetooth/hci/h4.c +++ b/drivers/bluetooth/hci/h4.c @@ -161,16 +161,11 @@ static struct net_buf *get_rx(int timeout) { BT_DBG("type 0x%02x, evt 0x%02x", rx.type, rx.evt.evt); - if (rx.type == H4_EVT && (rx.evt.evt == BT_HCI_EVT_CMD_COMPLETE || - rx.evt.evt == BT_HCI_EVT_CMD_STATUS)) { - return bt_buf_get_cmd_complete(timeout); + if (rx.type == H4_EVT) { + return bt_buf_get_evt(rx.evt.evt, timeout); } - if (rx.type == H4_ACL) { - return bt_buf_get_rx(BT_BUF_ACL_IN, timeout); - } else { - return bt_buf_get_rx(BT_BUF_EVT, timeout); - } + return bt_buf_get_rx(BT_BUF_ACL_IN, timeout); } static void rx_thread(void *p1, void *p2, void *p3) diff --git a/drivers/bluetooth/hci/h5.c b/drivers/bluetooth/hci/h5.c index 34ee0b19b2081..3d5ae9a34114a 100644 --- a/drivers/bluetooth/hci/h5.c +++ b/drivers/bluetooth/hci/h5.c @@ -408,16 +408,7 @@ static inline struct net_buf *get_evt_buf(u8_t evt) { struct net_buf *buf; - switch (evt) { - case BT_HCI_EVT_CMD_COMPLETE: - case BT_HCI_EVT_CMD_STATUS: - buf = bt_buf_get_cmd_complete(K_NO_WAIT); - break; - default: - buf = bt_buf_get_rx(BT_BUF_EVT, K_NO_WAIT); - break; - } - + buf = bt_buf_get_evt(evt, K_NO_WAIT); if (buf) { net_buf_add_u8(h5.rx_buf, evt); } diff --git a/drivers/bluetooth/hci/ipm_stm32wb.c b/drivers/bluetooth/hci/ipm_stm32wb.c index ba67ef561102b..205b0f4bf7cc5 100644 --- a/drivers/bluetooth/hci/ipm_stm32wb.c +++ b/drivers/bluetooth/hci/ipm_stm32wb.c @@ -114,12 +114,8 @@ void TM_EvtReceivedCb(TL_EvtPacket_t *hcievt) BT_ERR("Unknown evtcode type 0x%02x", hcievt->evtserial.evt.evtcode); goto out; - case BT_HCI_EVT_CMD_COMPLETE: - case BT_HCI_EVT_CMD_STATUS: - buf = bt_buf_get_cmd_complete(K_FOREVER); - break; default: - buf = bt_buf_get_rx(BT_BUF_EVT, K_FOREVER); + buf = bt_buf_get_evt(evtserial.evt.evtcode, K_FOREVER); break; } net_buf_add_mem(buf, &hcievt->evtserial.evt, diff --git a/drivers/bluetooth/hci/spi.c b/drivers/bluetooth/hci/spi.c index 0694b7af4b0ec..e510a8f541dc7 100644 --- a/drivers/bluetooth/hci/spi.c +++ b/drivers/bluetooth/hci/spi.c @@ -353,12 +353,9 @@ static void bt_spi_rx_thread(void) /* Vendor events are currently unsupported */ bt_spi_handle_vendor_evt(rxmsg); continue; - case BT_HCI_EVT_CMD_COMPLETE: - case BT_HCI_EVT_CMD_STATUS: - buf = bt_buf_get_cmd_complete(K_FOREVER); - break; default: - buf = bt_buf_get_rx(BT_BUF_EVT, K_FOREVER); + buf = bt_buf_get_evt(rxmsg[EVT_HEADER_EVENT], + K_FOREVER); break; } diff --git a/drivers/bluetooth/hci/userchan.c b/drivers/bluetooth/hci/userchan.c index d6be3863a66bd..749cfab306186 100644 --- a/drivers/bluetooth/hci/userchan.c +++ b/drivers/bluetooth/hci/userchan.c @@ -57,16 +57,11 @@ static int bt_dev_index = -1; static struct net_buf *get_rx(const u8_t *buf) { - if (buf[0] == H4_EVT && (buf[1] == BT_HCI_EVT_CMD_COMPLETE || - buf[1] == BT_HCI_EVT_CMD_STATUS)) { - return bt_buf_get_cmd_complete(K_FOREVER); + if (buf[0] == H4_EVT) { + return bt_buf_get_evt(buf[1], K_FOREVER); } - if (buf[0] == H4_ACL) { - return bt_buf_get_rx(BT_BUF_ACL_IN, K_FOREVER); - } else { - return bt_buf_get_rx(BT_BUF_EVT, K_FOREVER); - } + return bt_buf_get_rx(BT_BUF_ACL_IN, K_FOREVER); } static bool uc_ready(void) diff --git a/include/bluetooth/buf.h b/include/bluetooth/buf.h index a71f14ed0e67b..f71a3672dc500 100644 --- a/include/bluetooth/buf.h +++ b/include/bluetooth/buf.h @@ -64,6 +64,18 @@ struct net_buf *bt_buf_get_rx(enum bt_buf_type type, s32_t timeout); */ struct net_buf *bt_buf_get_cmd_complete(s32_t timeout); +/** Allocate a buffer for an HCI Event + * + * This will set the buffer type so bt_buf_set_type() does not need to + * be explicitly called before bt_recv_prio() or bt_recv(). + * + * @param evt HCI event code + * @param timeout Timeout in milliseconds, or one of the special values + * K_NO_WAIT and K_FOREVER. + * @return A new buffer. + */ +struct net_buf *bt_buf_get_evt(u8_t evt, s32_t timeout); + /** Set the buffer type * * @param buf Bluetooth buffer diff --git a/subsys/bluetooth/controller/hci/hci.c b/subsys/bluetooth/controller/hci/hci.c index c52098006b8d7..5c88b9238ec34 100644 --- a/subsys/bluetooth/controller/hci/hci.c +++ b/subsys/bluetooth/controller/hci/hci.c @@ -120,7 +120,7 @@ void *hci_cmd_complete(struct net_buf **buf, u8_t plen) { struct bt_hci_evt_cmd_complete *cc; - *buf = bt_buf_get_cmd_complete(K_FOREVER); + *buf = bt_buf_get_evt(BT_HCI_EVT_CMD_COMPLETE, K_FOREVER); hci_evt_create(*buf, BT_HCI_EVT_CMD_COMPLETE, sizeof(*cc) + plen); @@ -137,7 +137,7 @@ static struct net_buf *cmd_status(u8_t status) struct bt_hci_evt_cmd_status *cs; struct net_buf *buf; - buf = bt_buf_get_cmd_complete(K_FOREVER); + buf = bt_buf_get_evt(BT_HCI_EVT_CMD_STATUS, K_FOREVER); hci_evt_create(buf, BT_HCI_EVT_CMD_STATUS, sizeof(*cs)); cs = net_buf_add(buf, sizeof(*cs)); diff --git a/subsys/bluetooth/host/hci_core.c b/subsys/bluetooth/host/hci_core.c index c2209018ccbeb..cd544692fa9ef 100644 --- a/subsys/bluetooth/host/hci_core.c +++ b/subsys/bluetooth/host/hci_core.c @@ -137,6 +137,16 @@ NET_BUF_POOL_DEFINE(hci_cmd_pool, CONFIG_BT_HCI_CMD_COUNT, NET_BUF_POOL_DEFINE(hci_rx_pool, CONFIG_BT_RX_BUF_COUNT, BT_BUF_RX_SIZE, BT_BUF_USER_DATA_MIN, NULL); +#if defined(CONFIG_BT_CONN) +/* Dedicated pool for HCI_Number_of_Completed_Packets. This event is always + * consumed synchronously by bt_recv_prio() so a single buffer is enough. + * Having a dedicated pool for it ensures that exhaustion of the RX pool + * cannot block the delivery of this priority event. + */ +NET_BUF_POOL_DEFINE(num_complete_pool, 1, BT_BUF_RX_SIZE, + BT_BUF_USER_DATA_MIN, NULL); +#endif /* CONFIG_BT_CONN */ + struct event_handler { u8_t event; u8_t min_len; @@ -5656,6 +5666,31 @@ struct net_buf *bt_buf_get_cmd_complete(s32_t timeout) return bt_buf_get_rx(BT_BUF_EVT, timeout); } +struct net_buf *bt_buf_get_evt(u8_t evt, s32_t timeout) +{ + switch (evt) { +#if defined(CONFIG_BT_CONN) + case BT_HCI_EVT_NUM_COMPLETED_PACKETS: + { + struct net_buf *buf; + + buf = net_buf_alloc(&num_complete_pool, timeout); + if (buf) { + net_buf_reserve(buf, CONFIG_BT_HCI_RESERVE); + bt_buf_set_type(buf, BT_BUF_EVT); + } + + return buf; + } +#endif /* CONFIG_BT_CONN */ + case BT_HCI_EVT_CMD_COMPLETE: + case BT_HCI_EVT_CMD_STATUS: + return bt_buf_get_cmd_complete(timeout); + default: + return bt_buf_get_rx(BT_BUF_EVT, timeout); + } +} + #if defined(CONFIG_BT_BREDR) static int br_start_inquiry(const struct bt_br_discovery_param *param) { diff --git a/subsys/bluetooth/host/hci_ecc.c b/subsys/bluetooth/host/hci_ecc.c index 683c06412dae6..75ae8d2892ebd 100644 --- a/subsys/bluetooth/host/hci_ecc.c +++ b/subsys/bluetooth/host/hci_ecc.c @@ -84,7 +84,7 @@ static void send_cmd_status(u16_t opcode, u8_t status) BT_DBG("opcode %x status %x", opcode, status); - buf = bt_buf_get_cmd_complete(K_FOREVER); + buf = bt_buf_get_evt(BT_HCI_EVT_CMD_STATUS, K_FOREVER); bt_buf_set_type(buf, BT_BUF_EVT); hdr = net_buf_add(buf, sizeof(*hdr)); diff --git a/subsys/bluetooth/host/hci_raw.c b/subsys/bluetooth/host/hci_raw.c index 6a7394627ed65..9d79f7a9fa0df 100644 --- a/subsys/bluetooth/host/hci_raw.c +++ b/subsys/bluetooth/host/hci_raw.c @@ -72,6 +72,18 @@ struct net_buf *bt_buf_get_cmd_complete(s32_t timeout) return buf; } +struct net_buf *bt_buf_get_evt(u8_t evt, s32_t timeout) +{ + struct net_buf *buf; + + buf = net_buf_alloc(&hci_rx_pool, timeout); + if (buf) { + bt_buf_set_type(buf, BT_BUF_EVT); + } + + return buf; +} + int bt_recv(struct net_buf *buf) { BT_DBG("buf %p len %u", buf, buf->len); diff --git a/tests/bluetooth/hci_prop_evt/src/main.c b/tests/bluetooth/hci_prop_evt/src/main.c index f501d16fea270..ca255523e51fe 100644 --- a/tests/bluetooth/hci_prop_evt/src/main.c +++ b/tests/bluetooth/hci_prop_evt/src/main.c @@ -53,7 +53,7 @@ static void *cmd_complete(struct net_buf **buf, u8_t plen, u16_t opcode) { struct bt_hci_evt_cmd_complete *cc; - *buf = bt_buf_get_cmd_complete(K_FOREVER); + *buf = bt_buf_get_evt(BT_HCI_EVT_CMD_COMPLETE, K_FOREVER); evt_create(*buf, BT_HCI_EVT_CMD_COMPLETE, sizeof(*cc) + plen); cc = net_buf_add(*buf, sizeof(*cc)); cc->ncmd = 1U; From e24e87f8238ab265f2a63393e969d4475ee00fbb Mon Sep 17 00:00:00 2001 From: Johan Hedberg Date: Wed, 19 Jun 2019 09:25:48 +0300 Subject: [PATCH 2/3] net: buf: Reserve upper 3 bits of flags for external use Currently net_buf only needs the lower two bits. It may be useful to let users of the API take advantage of some bits as a form of extended user data (if using user data isn't appropriate or there's no space there). Signed-off-by: Johan Hedberg --- include/net/buf.h | 11 ++++++++++- subsys/net/buf.c | 2 +- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/include/net/buf.h b/include/net/buf.h index 006eaaec232e3..e54d5b2d3a466 100644 --- a/include/net/buf.h +++ b/include/net/buf.h @@ -474,6 +474,12 @@ static inline void net_buf_simple_restore(struct net_buf_simple *buf, */ #define NET_BUF_EXTERNAL_DATA BIT(1) +/** + * Bitmask of the lower bits of buf->flags that are reserved for net_buf + * internal use. + */ +#define NET_BUF_FLAGS_MASK BIT_MASK(5) + /** * @brief Network buffer representation. * @@ -493,7 +499,10 @@ struct net_buf { /** Reference count. */ u8_t ref; - /** Bit-field of buffer flags. */ + /** Bit-field of buffer flags. Lower 5 bits are reserved for net_buf + * internal usage. Upper 3 bits can be freely used externally as + * a form of extended user data. + */ u8_t flags; /** Where the buffer should go when freed up. */ diff --git a/subsys/net/buf.c b/subsys/net/buf.c index 0abff02a5e787..2055b0718ab24 100644 --- a/subsys/net/buf.c +++ b/subsys/net/buf.c @@ -78,7 +78,7 @@ static inline struct net_buf *pool_get_uninit(struct net_buf_pool *pool, void net_buf_reset(struct net_buf *buf) { - NET_BUF_ASSERT(buf->flags == 0U); + NET_BUF_ASSERT((buf->flags & NET_BUF_FLAGS_MASK) == 0U); NET_BUF_ASSERT(buf->frags == NULL); net_buf_simple_reset(&buf->b); From be91f0ff1e7e1bf1e0f10adbdb1aafd2867df1e7 Mon Sep 17 00:00:00 2001 From: Johan Hedberg Date: Tue, 18 Jun 2019 10:29:29 +0300 Subject: [PATCH 3/3] Bluetooth: drivers/h4: Add support to discard packets from RX queue Add support to iterate the RX queue and discard (discardable) packets in case the RX buffer pool is empty. This will make potential deadlock scenarios less likely. Signed-off-by: Johan Hedberg --- drivers/bluetooth/hci/Kconfig | 14 +++++++ drivers/bluetooth/hci/h4.c | 75 +++++++++++++++++++++++++++++++++-- 2 files changed, 86 insertions(+), 3 deletions(-) diff --git a/drivers/bluetooth/hci/Kconfig b/drivers/bluetooth/hci/Kconfig index d4ec05ce02f91..68adf16ad3a0e 100644 --- a/drivers/bluetooth/hci/Kconfig +++ b/drivers/bluetooth/hci/Kconfig @@ -77,6 +77,20 @@ config BT_UART_ON_DEV_NAME This option specifies the name of UART device to be used for Bluetooth. +if BT_H4 + +config BT_H4_DISCARD_RX_WAIT + int "How many milliseconds to initially wait for RX buffers" + range -1 6000000 + default 100 + help + This option specifies how many milliseconds the H4 HCI driver + will initially wait for RX buffers to become available before + attempting to discard discardable buffers from the RX queue. Set + to -1 (K_FOREVER) to disable the feature. + +endif # BT_H4 + if BT_SPI config BT_BLUENRG_ACI diff --git a/drivers/bluetooth/hci/h4.c b/drivers/bluetooth/hci/h4.c index 315925fb5cd4d..a93bae87c83ce 100644 --- a/drivers/bluetooth/hci/h4.c +++ b/drivers/bluetooth/hci/h4.c @@ -34,6 +34,8 @@ #define H4_SCO 0x03 #define H4_EVT 0x04 +#define BUF_DISCARDABLE BIT(7) + static K_THREAD_STACK_DEFINE(rx_thread_stack, CONFIG_BT_RX_STACK_SIZE); static struct k_thread rx_thread_data; @@ -159,15 +161,82 @@ static void reset_rx(void) static struct net_buf *get_rx(int timeout) { + struct net_buf *buf; + BT_DBG("type 0x%02x, evt 0x%02x", rx.type, rx.evt.evt); if (rx.type == H4_EVT) { - return bt_buf_get_evt(rx.evt.evt, timeout); + buf = bt_buf_get_evt(rx.evt.evt, timeout); + } else { + buf = bt_buf_get_rx(BT_BUF_ACL_IN, timeout); + } + + if (buf) { + if (rx.discardable) { + buf->flags |= BUF_DISCARDABLE; + } else { + buf->flags &= ~BUF_DISCARDABLE; + } } - return bt_buf_get_rx(BT_BUF_ACL_IN, timeout); + return buf; } +#if (CONFIG_BT_H4_DISCARD_RX_WAIT >= 0) +static struct net_buf *get_rx_blocking(void) +{ + sys_slist_t list = SYS_SLIST_STATIC_INIT(&list); + bool discarded = false; + struct net_buf *buf; + + /* If we have ACL flow control enabled then ACL buffers come from a + * dedicated pool, and we cannot benefit from trying to discard + * events from the RX queue. + */ + if (IS_ENABLED(CONFIG_BT_HCI_ACL_FLOW_CONTROL) && rx.type == H4_ACL) { + return get_rx(K_FOREVER); + } + + buf = get_rx(CONFIG_BT_H4_DISCARD_RX_WAIT); + if (buf) { + return buf; + } + + BT_WARN("Attempting to discard packets from RX queue"); + + while ((buf = net_buf_get(&rx.fifo, K_NO_WAIT))) { + if (!discarded && (buf->flags & BUF_DISCARDABLE)) { + net_buf_unref(buf); + discarded = true; + } else { + /* Since we use k_fifo_put_slist() instead of + * net_buf APIs to insert back to the FIFO we + * cannot support fragmented buffers. This should + * never happen but better to "document" it in the + * form of an assert here. + */ + __ASSERT(!(buf->flags & NET_BUF_FRAGS), + "Fragmented buffers not supported"); + sys_slist_append(&list, &buf->node); + } + } + + if (!discarded) { + BT_WARN("Unable to discard anything from the RX queue"); + } + + /* Insert non-discarded packets back to the RX queue */ + k_fifo_put_slist(&rx.fifo, &list); + + return get_rx(K_FOREVER); +} +#else +static inline struct net_buf *get_rx_blocking(void) +{ + return get_rx(K_FOREVER); +} +#endif + static void rx_thread(void *p1, void *p2, void *p3) { struct net_buf *buf; @@ -186,7 +255,7 @@ static void rx_thread(void *p1, void *p2, void *p3) * original command buffer (if available). */ if (rx.have_hdr && !rx.buf) { - rx.buf = get_rx(K_FOREVER); + rx.buf = get_rx_blocking(); BT_DBG("Got rx.buf %p", rx.buf); if (rx.remaining > net_buf_tailroom(rx.buf)) { BT_ERR("Not enough space in buffer");