Skip to content

[nrf noup] bluetooth: att: Fix CONFIG_BT_ATT_SENT_CB_AFTER_TX #3323

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
+29 −10
Open

[nrf noup] bluetooth: att: Fix CONFIG_BT_ATT_SENT_CB_AFTER_TX #3323

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d82ab7a
Revert "[nrf noup] bluetooth: conn: Skip buffer ref count check in se…
MarekPieta Oct 2, 2025
65b8128
Revert "[nrf noup] bluetooth: conn: Allow for an extra ref in bt_l2ca…
MarekPieta Oct 2, 2025
e5a694b
Revert "[nrf noup] bluetooth: att: Allow ATT sent callback after data…
MarekPieta Oct 2, 2025
5d62bf5
[nrf noup] bluetooth: att: Allow ATT sent callback after data TX is done
MarekPieta Jun 14, 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
3 changes: 0 additions & 3 deletions subsys/bluetooth/host/Kconfig.gatt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,9 +52,6 @@ config BT_ATT_SENT_CB_AFTER_TX
Zephyr fork). It is a temporary solution allowing to control flow of
GATT notifications with HID reports for HID use-case.

Enabling this option may require increasing CONFIG_BT_CONN_TX_MAX in
configuration, because ATT would use additional TX contexts.

config BT_EATT
bool "Enhanced ATT Bearers support [EXPERIMENTAL]"
depends on BT_L2CAP_ECRED
Expand Down
10 changes: 6 additions & 4 deletions subsys/bluetooth/host/conn.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -712,10 +712,12 @@ static int send_buf(struct bt_conn *conn, struct net_buf *buf,
uint16_t frag_len = MIN(conn_mtu(conn), len);

/* If ATT sent callback is delayed until data transmission
* is done by BLE controller, the transmitted buffer may
* have an additional reference. The reference is used to
* extend lifetime of the net buffer until the data
* transmission is confirmed by ACK of the remote.
* is done by BLE controller (CONFIG_BT_ATT_SENT_CB_AFTER_TX),
* the `chan_send` function from `att.c` introduces an additional
* reference. The reference is used to extend lifetime of the net
* buffer until the data transmission is confirmed by ACK of the
* remote (the reference is removed when the TX callback passed
* to `bt_l2cap_send_pdu` is called).
*
* send_buf function can be called multiple times, if buffer
* has to be fragmented over HCI. In that case, the callback
Expand Down
26 changes: 23 additions & 3 deletions subsys/bluetooth/host/l2cap.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,24 @@ static void l2cap_chan_del(struct bt_l2cap_chan *chan)
* `l2cap_chan_destroy()` as it is not called for fixed channels.
*/
while ((buf = k_fifo_get(&le_chan->tx_queue, K_NO_WAIT))) {
/* If ATT sent callback is delayed until data transmission is done by BLE controller
* (CONFIG_BT_ATT_SENT_CB_AFTER_TX), the `chan_send` function from `att.c`
* introduces an additional reference. The reference is used to extend lifetime of
* the net buffer until the data transmission is confirmed by ACK of the
* remote (the reference is removed when the TX callback passed to
* `bt_l2cap_send_pdu` is called).
*
* The TX callbacks of the buffers removed here are not called, therefore the
* additional reference needs to be removed here.
*/
if (IS_ENABLED(CONFIG_BT_ATT_SENT_CB_AFTER_TX)) {
__ASSERT_NO_MSG(buf->ref <= 2);

if (buf->ref == 2) {
net_buf_unref(buf);
}
}
Comment on lines +286 to +292
Copy link
Contributor

@alwa-nordic alwa-nordic Oct 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Detecting ATT PDUs by checking buf->ref == 2 is fragile because future changes might introduce other scenarios with extra references for different reasons. I'm not even sure that it's not the case today. Instead of using the ref count as a proxy, I suggest comparing the cb:

if (IS_ENABLED(CONFIG_BT_ATT_SENT_CB_AFTER_TX)) {
    bt_conn_tx_cb_t cb = closure_cb(buf->user_data);
    
    if (cb == chan_sent_cb) {
        void *user_data = closure_data(buf->user_data);
        /* Release the extra reference attached to user_data */
        cb(chan->conn, user_data, -ESHUTDOWN);
    }
}

But I realized it's a bug that we don't invoke the callbacks here. We should be invoking all the callbacks here just like in process_unack_tx(). I made a PR upstream to fix this just now:

zephyrproject-rtos/zephyr#97056

I suggest we cherry pick that general fix instead. Can you test that it works for you when it passes the upstream CI?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The bt_l2cap_send_pdu and bt_l2cap_dyn_chan_send both seem to demand that ref counter is 1 for buffers placed on the TX queue. Also it seems that the net_buf_unref here is expected to remove the last reference (please correct me if I overlooked something).

Using the closure_cb as in the code snippet above would require to pass the chan_sent_cb function (through header or using extern keyword). The function would no longer be static for the att.c file. I tried to avoid that as it might not be a clean solution.

Invoking the sent callback here might help. I will test your commit locally and let you know if it works.

Copy link
Contributor

Choose a reason for hiding this comment

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

The bt_l2cap_send_pdu and bt_l2cap_dyn_chan_send both seem to demand that ref counter is 1 for buffers placed on the TX queue. Also it seems that the net_buf_unref here is expected to remove the last reference (please correct me if I overlooked something).

That's just to protect against user error. We did not foresee that a stack-internal user would keep an extra reference and wait for the callback. We just saw no use for the extra reference and it is indicative of a leaked reference. The stack does not rely on this fact AFAIK. We can remove all these checks. Maybe we should.

Using the closure_cb as in the code snippet above would require to pass the chan_sent_cb function (through header or using extern keyword). The function would no longer be static for the att.c file. I tried to avoid that as it might not be a clean solution.

I prefer that. We are referring to ATT anyway (to explain the code), so including the header is just being explicit about the dependency.

Invoking the sent callback here might help. I will test your commit locally and let you know if it works.

Thanks! It passed CI checks. Invoking the sent callback is a generalization of my code snippet here, so I'm sure it will work. Let's just look out for unintentional breakage elsewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I confirmed your fix from the upstream PR. As soon as the upstream PR is merged, I will update this PR.


net_buf_unref(buf);
}

Expand Down Expand Up @@ -761,9 +779,11 @@ int bt_l2cap_send_pdu(struct bt_l2cap_le_chan *le_chan, struct net_buf *pdu,
return -ENOTCONN;
}

/* Allow for an additional buffer reference if callback is provided. This can be used to
* extend lifetime of the net buffer until the data transmission is confirmed by ACK of the
* remote.
/* If ATT sent callback is delayed until data transmission is done by BLE controller
* (CONFIG_BT_ATT_SENT_CB_AFTER_TX), the `chan_send` function from `att.c` introduces an
* additional reference. The reference is used to extend lifetime of the net buffer until
* the data transmission is confirmed by ACK of the remote (the reference is removed when
* the TX callback passed to `bt_l2cap_send_pdu` is called).
*/
if (pdu->ref > 1 + (cb ? 1 : 0)) {
/* The host may alter the buf contents when fragmenting. Higher
Expand Down
Loading