From e73443e942ab0ba338246b4b8ae1fddf76a424dd Mon Sep 17 00:00:00 2001 From: Lang Xie Date: Tue, 5 Nov 2024 10:36:18 +0100 Subject: [PATCH] [nrf fromlist] Bluetooth: Host: L2CAP: Add explicit MPS parameter to reconfigure API Add explicit MPS parameter to `bt_l2cap_ecred_chan_reconfigure`. This is the only straightforward way to reconfigure MPS channels that use `seg_recv`, where the application is in control of the MPS. The implementation of `bt_l2cap_ecred_chan_reconfigure` before this patch essentially had an implicit MPS parameter equal to `MIN(mtu + BT_L2CAP_SDU_HDR_SIZE, BT_L2CAP_RX_MTU)`. This choice maximizes throughput within the constrains of the Host implementation. To upgrade without change, applications must simply use that expression as the new argument, the the benefit that the choice of MPS is now explicit. I am not aware of any real-world uses of `bt_l2cap_ecred_chan_reconfigure`. I believe we can impose the small inconvenience of a search-replace without going through a deprecation cycle. For non-`seg_recv` channels, an explicit MPS parameter is useful if the application wants to ensure a small MPS. This might be useful for minimizing the impact this channel has on other channels on the same connection. Upstream PR #: 80883 Signed-off-by: Lang Xie --- include/zephyr/bluetooth/l2cap.h | 3 +- subsys/bluetooth/host/att.c | 5 +++- subsys/bluetooth/host/l2cap.c | 29 ++++++++++++------- subsys/bluetooth/shell/l2cap.c | 4 ++- .../host/l2cap/general/src/main_l2cap_ecred.c | 3 +- 5 files changed, 30 insertions(+), 14 deletions(-) diff --git a/include/zephyr/bluetooth/l2cap.h b/include/zephyr/bluetooth/l2cap.h index 0117da5ebb9..e7be8fa5f18 100644 --- a/include/zephyr/bluetooth/l2cap.h +++ b/include/zephyr/bluetooth/l2cap.h @@ -547,10 +547,11 @@ int bt_l2cap_ecred_chan_connect(struct bt_conn *conn, * @param chans Array of channel objects. Null-terminated. Elements after the * first 5 are silently ignored. * @param mtu Channel MTU to reconfigure to. + * @param mps Channel MPS to reconfigure to. * * @return 0 in case of success or negative value in case of error. */ -int bt_l2cap_ecred_chan_reconfigure(struct bt_l2cap_chan **chans, uint16_t mtu); +int bt_l2cap_ecred_chan_reconfigure(struct bt_l2cap_chan **chans, uint16_t mtu, uint16_t mps); /** @brief Connect L2CAP channel * diff --git a/subsys/bluetooth/host/att.c b/subsys/bluetooth/host/att.c index f5bca16d8d2..0f5ba2cba74 100644 --- a/subsys/bluetooth/host/att.c +++ b/subsys/bluetooth/host/att.c @@ -3773,6 +3773,7 @@ int bt_eatt_reconfigure(struct bt_conn *conn, uint16_t mtu) struct bt_l2cap_chan *chans[CONFIG_BT_EATT_MAX + 1] = {}; size_t offset = 0; size_t i = 0; + uint16_t mps; int err; SYS_SLIST_FOR_EACH_CONTAINER(&att->chans, att_chan, node) { @@ -3783,10 +3784,12 @@ int bt_eatt_reconfigure(struct bt_conn *conn, uint16_t mtu) } while (offset < i) { + mps = MIN(mtu + BT_L2CAP_SDU_HDR_SIZE, BT_L2CAP_RX_MTU); + /* bt_l2cap_ecred_chan_reconfigure() uses the first L2CAP_ECRED_CHAN_MAX_PER_REQ * elements of the array or until a null-terminator is reached. */ - err = bt_l2cap_ecred_chan_reconfigure(&chans[offset], mtu); + err = bt_l2cap_ecred_chan_reconfigure(&chans[offset], mtu, mps); if (err < 0) { return err; } diff --git a/subsys/bluetooth/host/l2cap.c b/subsys/bluetooth/host/l2cap.c index 69197326df2..067d41d5a0d 100644 --- a/subsys/bluetooth/host/l2cap.c +++ b/subsys/bluetooth/host/l2cap.c @@ -2944,12 +2944,13 @@ static struct bt_l2cap_le_chan *l2cap_find_pending_reconf(struct bt_conn *conn) return NULL; } -int bt_l2cap_ecred_chan_reconfigure(struct bt_l2cap_chan **chans, uint16_t mtu) +int bt_l2cap_ecred_chan_reconfigure(struct bt_l2cap_chan **chans, uint16_t mtu, uint16_t mps) { struct bt_l2cap_ecred_reconf_req *req; struct bt_conn *conn = NULL; struct bt_l2cap_le_chan *ch; struct net_buf *buf; + bool multiple_chan; uint8_t ident; int i; @@ -2959,6 +2960,16 @@ int bt_l2cap_ecred_chan_reconfigure(struct bt_l2cap_chan **chans, uint16_t mtu) return -EINVAL; } + if (chans[0] == NULL) { + return -EINVAL; + } + + if (mps > BT_L2CAP_RX_MTU) { + return -EINVAL; + } + + multiple_chan = chans[1] != NULL; + for (i = 0; i < L2CAP_ECRED_CHAN_MAX_PER_REQ; i++) { if (!chans[i]) { break; @@ -2977,10 +2988,13 @@ int bt_l2cap_ecred_chan_reconfigure(struct bt_l2cap_chan **chans, uint16_t mtu) if (mtu < BT_L2CAP_LE_CHAN(chans[i])->rx.mtu) { return -EINVAL; } - } - if (i == 0) { - return -EINVAL; + /* MPS is not allowed to decrease when reconfiguring multiple channels. + * Core Specification 3.A.4.27 v6.0 + */ + if (multiple_chan && mps < BT_L2CAP_LE_CHAN(chans[i])->rx.mps) { + return -EINVAL; + } } if (!conn) { @@ -3007,12 +3021,7 @@ int bt_l2cap_ecred_chan_reconfigure(struct bt_l2cap_chan **chans, uint16_t mtu) req = net_buf_add(buf, sizeof(*req)); req->mtu = sys_cpu_to_le16(mtu); - - /* MPS shall not be bigger than MTU + BT_L2CAP_SDU_HDR_SIZE - * as the remaining bytes cannot be used. - */ - req->mps = sys_cpu_to_le16(MIN(mtu + BT_L2CAP_SDU_HDR_SIZE, - BT_L2CAP_RX_MTU)); + req->mps = sys_cpu_to_le16(mps); for (int j = 0; j < i; j++) { ch = BT_L2CAP_LE_CHAN(chans[j]); diff --git a/subsys/bluetooth/shell/l2cap.c b/subsys/bluetooth/shell/l2cap.c index a1227534ab8..74e01bb1422 100644 --- a/subsys/bluetooth/shell/l2cap.c +++ b/subsys/bluetooth/shell/l2cap.c @@ -282,6 +282,7 @@ static int cmd_ecred_reconfigure(const struct shell *sh, size_t argc, char *argv { struct bt_l2cap_chan *l2cap_ecred_chans[] = { &l2ch_chan.ch.chan, NULL }; uint16_t mtu; + uint16_t mps; int err = 0; if (!default_conn) { @@ -301,7 +302,8 @@ static int cmd_ecred_reconfigure(const struct shell *sh, size_t argc, char *argv return -ENOEXEC; } - err = bt_l2cap_ecred_chan_reconfigure(l2cap_ecred_chans, mtu); + mps = MIN(mtu + BT_L2CAP_SDU_HDR_SIZE, BT_L2CAP_RX_MTU); + err = bt_l2cap_ecred_chan_reconfigure(l2cap_ecred_chans, mtu, mps); if (err < 0) { shell_error(sh, "Unable to reconfigure channel (err %d)", err); } else { diff --git a/tests/bsim/bluetooth/host/l2cap/general/src/main_l2cap_ecred.c b/tests/bsim/bluetooth/host/l2cap/general/src/main_l2cap_ecred.c index 521e1a6d633..29550955a26 100644 --- a/tests/bsim/bluetooth/host/l2cap/general/src/main_l2cap_ecred.c +++ b/tests/bsim/bluetooth/host/l2cap/general/src/main_l2cap_ecred.c @@ -426,12 +426,13 @@ static void send_sdu_concurrently(void) static int change_mtu_on_channels(int num_channels, int new_mtu) { struct bt_l2cap_chan *reconf_channels[ECRED_CHAN_MAX] = { NULL }; + uint16_t mps = MIN(new_mtu + BT_L2CAP_SDU_HDR_SIZE, BT_L2CAP_RX_MTU); for (int i = 0; i < num_channels; i++) { reconf_channels[i] = &(&channels[i])->le.chan; } - return bt_l2cap_ecred_chan_reconfigure(reconf_channels, new_mtu); + return bt_l2cap_ecred_chan_reconfigure(reconf_channels, new_mtu, mps); } static void test_peripheral_main(void)