mcumgr: smp_bt: wrong notify MTU calculation with CONFIG_BT_GATT_NOTIFY_MULTIPLE

[LeBlue]

Describe the bug
It is probably not the source of the bug, but currently the MTU calculation for fragmentet bt_smp packets fails to calculate the correct avaliable data length. The length is calculated as MTU - 3

zephyr/subsys/mgmt/smp_bt.c

Line 136 in 1b1d728

return mtu - 3;

but bt_gatt_notify needs 5 additional bytes instead of 3 if CONFIG_BT_GATT_NOTIFY_MULTIPLE is set.

zephyr/subsys/bluetooth/host/gatt.c

Line 1721 in 1b1d728

sizeof(*nfy) + params->len);

Discovered with the mcumgr image list command that generates a response of length 258, when both slots are used (see below for patch with additional log messages).

[00:00:17.537,872] <wrn> bt_att: No ATT channel for MTU 263, max 261
[00:00:17.537,872] <wrn> bt_gatt: No buffer available to send notification, mult

I assume the bug is actually using the gatt_notify_mult, even though bt_gatt_notify is called with the conn obj in

zephyr/subsys/mgmt/smp_bt.c

Line 101 in 1b1d728

return bt_gatt_notify(conn, smp_bt_attrs + 2, data, len);

?

Workarounds

1. Increase CONFIG_BT_L2CAP_TX_MTU to prevent fragmentation of bt_smp responses (>= 263?)
2. Disable CONFIG_BT_GATT_NOTIFY_MULTIPLE
3. Change mtu - 3 to mtu - 5 in bt_smp.c:smp_bt_get_mtu

To Reproduce
Steps to reproduce the behavior:

1. mkdir build; cd build
2. cmake -DBOARD=board_xyz samples/subsys/mgmt/mcumgr/smp_svr/
3. make
4. mcumgr image list
5. see error

Expected behavior
bt_smp does not (always) fail to send notifications/command responses

Impact
None for me, as single connection only does not need notify multiple. Otherwise pretty bad.

Screenshots or console output

CONFIG_BT_GATT_NOTIFY_MULTIPLE=y
CONFIG_BT_L2CAP_TX_MTU=261

[00:00:17.537,841] <wrn> bt_att: MTU: 261
[00:00:17.537,841] <wrn> mcumgr: conn 0x200026ec, len: 258
[00:00:17.537,872] <wrn> bt_gatt: Buff len(nfy) 4 + params len: 258
[00:00:17.537,872] <wrn> bt_att: No ATT channel for MTU 263, max 261
[00:00:17.537,872] <wrn> bt_gatt: No buffer available to send notification, mult

or

# CONFIG_BT_GATT_NOTIFY_MULTIPLE is not set
CONFIG_BT_L2CAP_TX_MTU=261

[00:00:36.543,884] <wrn> bt_att: MTU: 261
[00:00:36.543,884] <err> mcumgr: conn 0x200026e4, len: 258

Environment (please complete the following information):

• OS: MacOS
• Toolchain gnuarmemb
• zephyr v2.3.0 and master@1b1d728a18

Additional context
Changes for logging:

diff --git a/subsys/bluetooth/host/att.c b/subsys/bluetooth/host/att.c
index 06a7fd0daa..6490b5cc7c 100644
--- a/subsys/bluetooth/host/att.c
+++ b/subsys/bluetooth/host/att.c
@@ -2357,6 +2357,7 @@ struct net_buf *bt_att_create_pdu(struct bt_conn *conn, u8_t op, size_t len)
 {
 	struct bt_att *att;
 	struct bt_att_chan *chan, *tmp;
+	u16_t max_mtu = 0;

 	att = att_get(conn);
 	if (!att) {
@@ -2365,13 +2366,14 @@ struct net_buf *bt_att_create_pdu(struct bt_conn *conn, u8_t op, size_t len)

 	SYS_SLIST_FOR_EACH_CONTAINER_SAFE(&att->chans, chan, tmp, node) {
 		if (len + sizeof(op) > chan->chan.tx.mtu) {
+			max_mtu = MAX(chan->chan.tx.mtu, max_mtu);
 			continue;
 		}

 		return bt_att_chan_create_pdu(chan, op, len);
 	}

-	BT_WARN("No ATT channel for MTU %zu", len + sizeof(op));
+	BT_WARN("No ATT channel for MTU %zu, max %d", len + sizeof(op), max_mtu);

 	return NULL;
 }
@@ -2783,7 +2785,7 @@ u16_t bt_att_get_mtu(struct bt_conn *conn)
 			mtu = chan->chan.tx.mtu;
 		}
 	}
-
+	BT_WARN("MTU: %d", mtu);
 	return mtu;
 }

diff --git a/subsys/bluetooth/host/gatt.c b/subsys/bluetooth/host/gatt.c
index 12d3d88542..f649f3ce5b 100644
--- a/subsys/bluetooth/host/gatt.c
+++ b/subsys/bluetooth/host/gatt.c
@@ -1715,12 +1715,12 @@ static int gatt_notify_mult(struct bt_conn *conn, u16_t handle,
 			return ret;
 		}
 	}
-
+	BT_WARN("Buff len(nfy) %d + params len: %d", sizeof(*nfy),  params->len);
 	if (!*buf) {
 		*buf = bt_att_create_pdu(conn, BT_ATT_OP_NOTIFY_MULT,
 					 sizeof(*nfy) + params->len);
 		if (!*buf) {
-			BT_WARN("No buffer available to send notification");
+			BT_WARN("No buffer available to send notification, mult");
 			return -ENOMEM;
 		}
 		/* Set user_data so it can be restored when sending */
@@ -1769,7 +1769,7 @@ static int gatt_notify(struct bt_conn *conn, u16_t handle,
 	buf = bt_att_create_pdu(conn, BT_ATT_OP_NOTIFY,
 				sizeof(*nfy) + params->len);
 	if (!buf) {
-		BT_WARN("No buffer available to send notification");
+		BT_WARN("No buffer available to send notification, single");
 		return -ENOMEM;
 	}

diff --git a/subsys/mgmt/smp_bt.c b/subsys/mgmt/smp_bt.c
index d76de20b28..b758efda40 100644
--- a/subsys/mgmt/smp_bt.c
+++ b/subsys/mgmt/smp_bt.c
@@ -21,6 +21,9 @@

 #include <mgmt/smp.h>

+#include <logging/log.h>
+LOG_MODULE_REGISTER(mcumgr, CONFIG_MCUMGR_LOG_LEVEL);
+
 struct device;

 struct smp_bt_user_data {
@@ -170,6 +173,7 @@ static int smp_bt_tx_pkt(struct zephyr_smp_transport *zst, struct net_buf *nb)
 	if (conn == NULL) {
 		rc = -1;
 	} else {
+		LOG_ERR("conn %p, len: %d", conn, nb->len);
 		rc = smp_bt_tx_rsp(conn, nb->data, nb->len);
 		bt_conn_unref(conn);
 	}

[carlescufi]

@de-nordic can you please take a look?
@LeBlue would you mind sending a PR?

[joerchan]

@LeBlue Thank you for a detailed bug-report. The new GATT feature notify multiple should not break the API in this way. I have created a fix that sets it as default off, since it changes the behavior in a way that breaks existing applications.

[LeBlue]

@joerchan The question remains if the bt_smp.c:smp_bt_get_mtu still must be adjusted, if the CONFIG_BT_GATT_NOTIFY_MULTIPLE is enabled (see workaround 3. above). This would be fixing the 'update' application to the change buffer requirements.

[joerchan]

@LeBlue Yes if you wish to use GATT NOTIFY MULTIPLE. But there does not appear to be a good way for the application to determine if notify multiple will be used. We need to expose this information to the application in some way.

[Vudentz]

Hardcoding -3 is also a bad idea, because applications may actually want to set notify multiple then that would be broken anyway, so perhaps having a check fo IS_ENABLED(CONFIG_BT_GATT_NOTIFY_MULTIPLE) return mtu - 5; instead, but Im afraid we really want a runtime option if the application want to use the entire payload for doing streaming, but that would probably need an API change so the stack can avoid the grouping logic.

[joerchan]

Hardcoding -3 is also a bad idea, because applications may actually want to set notify multiple then that would be broken anyway, so perhaps having a check fo IS_ENABLED(CONFIG_BT_GATT_NOTIFY_MULTIPLE) return mtu - 5

But there is an MTU per L2CAP channel. I think the best is to leave it as is, MTU - 3 would give the best throughput when the application wants to send large data. The notify multiple seems to be best suited for many small notify values.