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.

Sign up for GitHub

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: conn: check for disconnected earlier when sending #68748

Merged
merged 2 commits into from
Feb 9, 2024
Merged

Bluetooth: conn: check for disconnected earlier when sending #68748

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b6dd3ef
Bluetooth: conn: check for disconnected earlier when sending
jori-nordic Feb 8, 2024
bde60d5
Bluetooth: conn: document obfuscated function: `send_buf()`
jori-nordic Feb 8, 2024
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: 31 additions & 6 deletions subsys/bluetooth/host/conn.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -586,12 +586,6 @@ static int do_send_frag(struct bt_conn *conn, struct net_buf *buf, uint8_t flags
unsigned int key;
int err = 0;

/* Check for disconnection while waiting for pkts_sem */
if (conn->state != BT_CONN_CONNECTED) {
err = -ENOTCONN;
goto fail;
}

LOG_DBG("conn %p buf %p len %u flags 0x%02x", conn, buf, buf->len,
flags);

Expand Down Expand Up @@ -669,6 +663,14 @@ static int send_frag(struct bt_conn *conn,
return -ENOBUFS;
}

/* Check for disconnection. It can't be done higher up (ie `send_buf`)
* as `create_frag` blocks with K_FOREVER and the connection could
* change state after waiting.
*/
if (conn->state != BT_CONN_CONNECTED) {
return -ENOTCONN;
}

/* Add the data to the buffer */
if (frag) {
uint16_t frag_len = MIN(conn_mtu(conn), net_buf_tailroom(frag));
Expand Down Expand Up @@ -720,6 +722,23 @@ static struct net_buf *create_frag(struct bt_conn *conn, struct net_buf *buf)
return frag;
}

/* Tentatively send a buffer to the HCI driver.
*
* This is designed to be async, as in most failures due to lack of resources
* are not fatal. The caller should call `send_buf()` again later.
*
* Return values:
*
* - 0: `buf` sent. `buf` ownership transferred to lower layers.
*
* - -EIO: buffer failed to send due to HCI error. `buf` ownership returned to
* caller BUT `buf` is popped from the TX queue. The caller shall destroy
* `buf` and its TX context.
Comment on lines +734 to +736
Copy link
Collaborator

Choose a reason for hiding this comment

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

If the caller shall destroy on this error, couldn't this function do that?

Copy link
Collaborator Author

@jori-nordic jori-nordic Feb 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

ownership is a bit messy regarding those 3 functions.
I am currently rewriting the whole TX flow so I'd rather quick-fix this particular issue and reserve the in-depth review for the refactor if that's alright.

*
* - Any other error: buffer failed to send. `buf` ownership returned to caller
* and `buf` is still the head of the TX queue
*
*/
static int send_buf(struct bt_conn *conn, struct net_buf *buf)
{
struct net_buf *frag;
Expand Down Expand Up @@ -929,6 +948,12 @@ void bt_conn_process_tx(struct bt_conn *conn)
err = send_buf(conn, buf);
net_buf_unref(buf);

/* HCI driver error. `buf` may have been popped from `tx_queue` and
* should be destroyed.
*
* TODO: In that case we might want to disable Bluetooth or at the very
* least tear down the connection.
*/
if (err == -EIO) {
struct bt_conn_tx *tx = tx_data(buf)->tx;

Expand Down
Loading