Skip to content
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

bluetooth: buf: Add a callback for freed buffer in rx pool #81646

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 29 additions & 8 deletions include/zephyr/bluetooth/buf.h
Original file line number Diff line number Diff line change
Expand Up @@ -28,22 +28,22 @@
extern "C" {
#endif

/** Possible types of buffers passed around the Bluetooth stack */
/** Possible types of buffers passed around the Bluetooth stack in a form of bitmask. */
enum bt_buf_type {
/** HCI command */
BT_BUF_CMD,
BT_BUF_CMD = BIT(0),
/** HCI event */
BT_BUF_EVT,
BT_BUF_EVT = BIT(1),
/** Outgoing ACL data */
BT_BUF_ACL_OUT,
BT_BUF_ACL_OUT = BIT(2),
/** Incoming ACL data */
BT_BUF_ACL_IN,
BT_BUF_ACL_IN = BIT(3),
/** Outgoing ISO data */
BT_BUF_ISO_OUT,
BT_BUF_ISO_OUT = BIT(4),
/** Incoming ISO data */
BT_BUF_ISO_IN,
BT_BUF_ISO_IN = BIT(5),
/** H:4 data */
BT_BUF_H4,
BT_BUF_H4 = BIT(6),
};

/** @brief This is a base type for bt_buf user data. */
Expand Down Expand Up @@ -138,6 +138,27 @@ BUILD_ASSERT(BT_BUF_ACL_RX_COUNT <= BT_BUF_ACL_RX_COUNT_MAX,
*/
struct net_buf *bt_buf_get_rx(enum bt_buf_type type, k_timeout_t timeout);

/** A callback to notify about freed buffer in the incoming data pool.
*
* This callback is called when a buffer of a given type is freed and can be requested through the
* @ref bt_buf_get_rx function. However, this callback is called from the context of the buffer
* freeing operation and must not attempt to allocate a new buffer from the same pool.
*
* @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).
*
* @param type_mask A bit mask of buffer types that have been freed.
*/
typedef void (*bt_buf_rx_freed_cb_t)(enum bt_buf_type type_mask);

/** Set the callback to notify about freed buffer in the incoming data pool.
*
* @param cb Callback to notify about freed buffer in the incoming data pool. If NULL, the callback
* is disabled.
*/
void bt_buf_rx_freed_cb_set(bt_buf_rx_freed_cb_t cb);

/** Allocate a buffer for outgoing data
*
* This will set the buffer type so bt_buf_set_type() does not need to
Expand Down
71 changes: 62 additions & 9 deletions subsys/bluetooth/host/buf.c
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,26 @@ LOG_MODULE_REGISTER(bt_buf, CONFIG_BT_LOG_LEVEL);
*/
#define SYNC_EVT_SIZE (BT_BUF_RESERVE + BT_HCI_EVT_HDR_SIZE + 255)

static bt_buf_rx_freed_cb_t buf_rx_freed_cb;

static void buf_rx_freed_notify(enum bt_buf_type mask)
{
k_sched_lock();

if (buf_rx_freed_cb) {
buf_rx_freed_cb(mask);
}

k_sched_unlock();
}

#if defined(CONFIG_BT_ISO_RX)
static void iso_rx_freed_cb(void)
{
buf_rx_freed_notify(BT_BUF_ISO_IN);
}
#endif

/* Pool for RX HCI buffers that are always freed by `bt_recv`
* before it returns.
*
Expand All @@ -40,17 +60,37 @@ NET_BUF_POOL_FIXED_DEFINE(discardable_pool, CONFIG_BT_BUF_EVT_DISCARDABLE_COUNT,
sizeof(struct bt_buf_data), NULL);

#if defined(CONFIG_BT_HCI_ACL_FLOW_CONTROL)
NET_BUF_POOL_DEFINE(acl_in_pool, BT_BUF_ACL_RX_COUNT,
BT_BUF_ACL_SIZE(CONFIG_BT_BUF_ACL_RX_SIZE),
sizeof(struct acl_data), bt_hci_host_num_completed_packets);
static void acl_in_pool_destroy(struct net_buf *buf)
{
bt_hci_host_num_completed_packets(buf);
buf_rx_freed_notify(BT_BUF_ACL_IN);
}

NET_BUF_POOL_FIXED_DEFINE(evt_pool, CONFIG_BT_BUF_EVT_RX_COUNT,
BT_BUF_EVT_RX_SIZE, sizeof(struct bt_buf_data),
NULL);
static void evt_pool_destroy(struct net_buf *buf)
{
net_buf_destroy(buf);
buf_rx_freed_notify(BT_BUF_EVT);
}

NET_BUF_POOL_DEFINE(acl_in_pool, BT_BUF_ACL_RX_COUNT, BT_BUF_ACL_SIZE(CONFIG_BT_BUF_ACL_RX_SIZE),
sizeof(struct acl_data), acl_in_pool_destroy);

NET_BUF_POOL_FIXED_DEFINE(evt_pool, CONFIG_BT_BUF_EVT_RX_COUNT, BT_BUF_EVT_RX_SIZE,
sizeof(struct bt_buf_data), evt_pool_destroy);
#else
NET_BUF_POOL_FIXED_DEFINE(hci_rx_pool, BT_BUF_RX_COUNT,
BT_BUF_RX_SIZE, sizeof(struct acl_data),
NULL);
static void hci_rx_pool_destroy(struct net_buf *buf)
{
net_buf_destroy(buf);

/* When ACL Flow Control is disabled, a single pool is used for events and acl data.
* Therefore the callback will always notify about both types of buffers, BT_BUF_EVT and
* BT_BUF_ACL_IN.
*/
buf_rx_freed_notify(BT_BUF_EVT | BT_BUF_ACL_IN);
}

NET_BUF_POOL_FIXED_DEFINE(hci_rx_pool, BT_BUF_RX_COUNT, BT_BUF_RX_SIZE, sizeof(struct acl_data),
hci_rx_pool_destroy);
#endif /* CONFIG_BT_HCI_ACL_FLOW_CONTROL */

struct net_buf *bt_buf_get_rx(enum bt_buf_type type, k_timeout_t timeout)
Expand Down Expand Up @@ -82,6 +122,19 @@ struct net_buf *bt_buf_get_rx(enum bt_buf_type type, k_timeout_t timeout)
return buf;
}

void bt_buf_rx_freed_cb_set(bt_buf_rx_freed_cb_t cb)
{
k_sched_lock();

buf_rx_freed_cb = cb;

#if defined(CONFIG_BT_ISO_RX)
bt_iso_buf_rx_freed_cb_set(cb != NULL ? iso_rx_freed_cb : NULL);
#endif

k_sched_unlock();
}

struct net_buf *bt_buf_get_evt(uint8_t evt, bool discardable,
k_timeout_t timeout)
{
Expand Down
21 changes: 19 additions & 2 deletions subsys/bluetooth/host/hci_raw.c
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,20 @@ static uint8_t raw_mode = BT_HCI_RAW_MODE_H4;
static uint8_t raw_mode;
#endif

NET_BUF_POOL_FIXED_DEFINE(hci_rx_pool, BT_BUF_RX_COUNT,
BT_BUF_RX_SIZE, sizeof(struct bt_buf_data), NULL);
static bt_buf_rx_freed_cb_t buf_rx_freed_cb;

static void hci_rx_buf_destroy(struct net_buf *buf)
{
net_buf_destroy(buf);

if (buf_rx_freed_cb) {
/* bt_buf_get_rx is used for all types of RX buffers */
buf_rx_freed_cb(BT_BUF_EVT | BT_BUF_ACL_IN | BT_BUF_ISO_IN);
}
}

NET_BUF_POOL_FIXED_DEFINE(hci_rx_pool, BT_BUF_RX_COUNT, BT_BUF_RX_SIZE, sizeof(struct bt_buf_data),
hci_rx_buf_destroy);
NET_BUF_POOL_FIXED_DEFINE(hci_cmd_pool, CONFIG_BT_BUF_CMD_TX_COUNT,
BT_BUF_CMD_SIZE(CONFIG_BT_BUF_CMD_TX_SIZE),
sizeof(struct bt_buf_data), NULL);
Expand Down Expand Up @@ -93,6 +105,11 @@ struct net_buf *bt_buf_get_rx(enum bt_buf_type type, k_timeout_t timeout)
return buf;
}

void bt_buf_rx_freed_cb_set(bt_buf_rx_freed_cb_t cb)
{
buf_rx_freed_cb = cb;
}

struct net_buf *bt_buf_get_tx(enum bt_buf_type type, k_timeout_t timeout,
const void *data, size_t size)
{
Expand Down
19 changes: 18 additions & 1 deletion subsys/bluetooth/host/iso.c
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,20 @@ LOG_MODULE_REGISTER(bt_iso, CONFIG_BT_ISO_LOG_LEVEL);
#define iso_chan(_iso) ((_iso)->iso.chan);

#if defined(CONFIG_BT_ISO_RX)
static bt_iso_buf_rx_freed_cb_t buf_rx_freed_cb;

static void iso_rx_buf_destroy(struct net_buf *buf)
{
net_buf_destroy(buf);

if (buf_rx_freed_cb) {
buf_rx_freed_cb();
}
}

NET_BUF_POOL_FIXED_DEFINE(iso_rx_pool, CONFIG_BT_ISO_RX_BUF_COUNT,
BT_ISO_SDU_BUF_SIZE(CONFIG_BT_ISO_RX_MTU), sizeof(struct iso_data), NULL);
BT_ISO_SDU_BUF_SIZE(CONFIG_BT_ISO_RX_MTU), sizeof(struct iso_data),
iso_rx_buf_destroy);

static struct bt_iso_recv_info iso_info_data[CONFIG_BT_ISO_RX_BUF_COUNT];
#define iso_info(buf) (&iso_info_data[net_buf_id(buf)])
Expand Down Expand Up @@ -553,6 +565,11 @@ struct net_buf *bt_iso_get_rx(k_timeout_t timeout)
return buf;
}

void bt_iso_buf_rx_freed_cb_set(bt_iso_buf_rx_freed_cb_t cb)
{
buf_rx_freed_cb = cb;
}

void bt_iso_recv(struct bt_conn *iso, struct net_buf *buf, uint8_t flags)
{
struct bt_hci_iso_sdu_hdr *hdr;
Expand Down
10 changes: 10 additions & 0 deletions subsys/bluetooth/host/iso_internal.h
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,16 @@ void hci_iso(struct net_buf *buf);
/* Allocates RX buffer */
struct net_buf *bt_iso_get_rx(k_timeout_t timeout);

/** A callback used to notify about freed buffer in the iso rx pool. */
typedef void (*bt_iso_buf_rx_freed_cb_t)(void);

/** Set a callback to notify about freed buffer in the iso rx pool.
*
* @param cb Callback to notify about freed buffer in the iso rx pool. If NULL, the callback is
* disabled.
*/
void bt_iso_buf_rx_freed_cb_set(bt_iso_buf_rx_freed_cb_t cb);

/* Process CIS Established event */
void hci_le_cis_established(struct net_buf *buf);

Expand Down
8 changes: 8 additions & 0 deletions tests/bluetooth/buf/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
# SPDX-License-Identifier: Apache-2.0

cmake_minimum_required(VERSION 3.20.0)

find_package(Zephyr REQUIRED HINTS $ENV{ZEPHYR_BASE})
project(buf)

target_sources(app PRIVATE src/main.c)
10 changes: 10 additions & 0 deletions tests/bluetooth/buf/prj.conf
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
CONFIG_TEST=y
CONFIG_ZTEST=y

CONFIG_BT=y
CONFIG_BT_CTLR=n
CONFIG_BT_H4=n

# Needed to enable and test the iso rx pool
CONFIG_BT_OBSERVER=y
CONFIG_BT_ISO_SYNC_RECEIVER=y
72 changes: 72 additions & 0 deletions tests/bluetooth/buf/src/main.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
/*
* Copyright (c) 2024 Nordic Semiconductor ASA
*
* SPDX-License-Identifier: Apache-2.0
*/

#include <zephyr/kernel.h>

#include <errno.h>
#include <zephyr/tc_util.h>
#include <zephyr/ztest.h>

#include <zephyr/bluetooth/hci.h>
#include <zephyr/bluetooth/buf.h>
#include <zephyr/bluetooth/bluetooth.h>
#include <zephyr/drivers/bluetooth.h>
#include <zephyr/sys/byteorder.h>

static enum bt_buf_type freed_buf_type;
static K_SEM_DEFINE(rx_sem, 0, 1);

void bt_buf_rx_freed_cb(enum bt_buf_type type)
{
freed_buf_type = type;
k_sem_give(&rx_sem);
}

ZTEST_SUITE(test_buf_data_api, NULL, NULL, NULL, NULL, NULL);

ZTEST(test_buf_data_api, test_buf_freed_cb)
{
struct net_buf *buf;
int err;

bt_buf_rx_freed_cb_set(bt_buf_rx_freed_cb);

/* Test that the callback is called for the BT_BUF_EVT type */
buf = bt_buf_get_rx(BT_BUF_EVT, K_NO_WAIT);
zassert_not_null(buf, "Failed to get event buffer");

net_buf_unref(buf);
/* The freed buf cb is called from net_buf_unref, therefore the semaphore should
* already by given.
*/
err = k_sem_take(&rx_sem, K_NO_WAIT);
zassert_equal(err, 0, "Timeout while waiting for event buffer to be freed");
zassert_equal(BT_BUF_EVT, BT_BUF_EVT & freed_buf_type, "Event buffer wasn't freed");

/* Test that the callback is called for the BT_BUF_ACL_IN type */
buf = bt_buf_get_rx(BT_BUF_ACL_IN, K_NO_WAIT);
zassert_not_null(buf, "Failed to get ACL buffer");

net_buf_unref(buf);
/* The freed buf cb is called from net_buf_unref, therefore the semaphore should
* already by given.
*/
err = k_sem_take(&rx_sem, K_NO_WAIT);
zassert_equal(err, 0, "Timeout while waiting for ACL buffer to be freed");
zassert_equal(BT_BUF_ACL_IN, BT_BUF_ACL_IN & freed_buf_type, "ACL buffer wasn't freed");

/* Test that the callback is called for the BT_BUF_ISO_IN type */
buf = bt_buf_get_rx(BT_BUF_ISO_IN, K_NO_WAIT);
zassert_not_null(buf, "Failed to get ISO buffer");

net_buf_unref(buf);
/* The freed buf cb is called from net_buf_unref, therefore the semaphore should
* already by given.
*/
err = k_sem_take(&rx_sem, K_NO_WAIT);
zassert_equal(err, 0, "Timeout while waiting for ISO buffer to be freed");
zassert_equal(BT_BUF_ISO_IN, BT_BUF_ISO_IN & freed_buf_type, "ISO buffer wasn't freed");
}
30 changes: 30 additions & 0 deletions tests/bluetooth/buf/testcase.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
common:
tags:
- bluetooth
- host

tests:
bluetooth.buf:
platform_allow:
- native_sim
- native_sim/native/64
integration_platforms:
- native_sim
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add extra_configs here as well to make sure the config is as we expect, no matter what the default is?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, though if we check the exact returned value, this may not be needed. I added CONFIG_BT_HCI_ACL_FLOW_CONTROL=y any way.

extra_configs:
- CONFIG_BT_HCI_ACL_FLOW_CONTROL=y
bluetooth.buf.no_acl_flow_control:
platform_allow:
- native_sim
- native_sim/native/64
integration_platforms:
- native_sim
extra_configs:
- CONFIG_BT_HCI_ACL_FLOW_CONTROL=n
bluetooth.buf.hci_raw:
platform_allow:
- native_sim
- native_sim/native/64
integration_platforms:
- native_sim
extra_configs:
- CONFIG_BT_HCI_RAW=y
Loading