[PATCH 1/5] android/gatt: Fix MTU exchange

[PATCH 1/5] android/gatt: Fix MTU exchange

[Szymon Janc]

This fix using invalid MTU on LE link. Previous attempt to fix this
"android/gatt: Fix initial setting of MTU" was not correct since
on LE default MTU is always 23 and we are not allowed to send bigger
requests before MTU exchange was performed.

This was affecting multiple PTS GATT test cases.
---
 android/gatt.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/android/gatt.c b/android/gatt.c
index 5f3050f..f754f30 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -1490,7 +1490,7 @@ static void connect_cb(GIOChannel *io, GError *gerr, gpointer user_data)
 	uint32_t status;
 	GError *err = NULL;
 	GAttrib *attrib;
-	uint16_t mtu;
+	uint16_t mtu, cid;
 
 	if (dev->state != DEVICE_CONNECT_READY) {
 		error("gatt: Device not in a connecting state!?");
@@ -1510,14 +1510,21 @@ static void connect_cb(GIOChannel *io, GError *gerr, gpointer user_data)
 		goto reply;
 	}
 
-	if (!bt_io_get(io, &err, BT_IO_OPT_IMTU, &mtu, BT_IO_OPT_INVALID)) {
-		error("gatt: Could not get imtu: %s", err->message);
+	if (!bt_io_get(io, &err, BT_IO_OPT_IMTU, &mtu, BT_IO_OPT_CID, &cid,
+							BT_IO_OPT_INVALID)) {
+		error("gatt: Could not get imtu or cid: %s", err->message);
 		device_set_state(dev, DEVICE_DISCONNECTED);
 		status = GATT_FAILURE;
 		g_error_free(err);
 		goto reply;
 	}
 
+	DBG("mtu %u cid %u", mtu, cid);
+
+	/* on LE we always start with default MTU */
+	if (cid == ATT_CID)
+		mtu = ATT_DEFAULT_LE_MTU;
+
 	attrib = g_attrib_new(io, mtu);
 	if (!attrib) {
 		error("gatt: unable to create new GAttrib instance");
-- 
1.9.3

[PATCH 2/5] android/gatt: Exchange MTU only on LE link

[Szymon Janc]

As per Core Specification Vol 3. Part G. 4.3.1 ATT MTU exchange
procedure shall not be used on BR/EDR link.
---
 android/gatt.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/android/gatt.c b/android/gatt.c
index f754f30..49fff6f 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -1550,7 +1550,10 @@ static void connect_cb(GIOChannel *io, GError *gerr, gpointer user_data)
 
 	/* Send exchange mtu request as we assume being client and server */
 	/* TODO: Dont exchange mtu if no client apps */
-	send_exchange_mtu_request(dev);
+
+	/* MTU exchange shall not be used on BR/EDR - Vol 3. Part G. 4.3.1 */
+	if (cid == ATT_CID)
+		send_exchange_mtu_request(dev);
 
 	/*
 	 * Service Changed Characteristic and CCC Descriptor handles
@@ -6120,7 +6123,7 @@ static uint8_t read_request(const uint8_t *cmd, uint16_t cmd_len,
 static uint8_t mtu_att_handle(const uint8_t *cmd, uint16_t cmd_len,
 							struct gatt_device *dev)
 {
-	uint16_t mtu, imtu, omtu;
+	uint16_t mtu, imtu, omtu, cid;
 	size_t length;
 	GIOChannel *io;
 	GError *gerr = NULL;
@@ -6139,6 +6142,7 @@ static uint8_t mtu_att_handle(const uint8_t *cmd, uint16_t cmd_len,
 	io = g_attrib_get_channel(dev->attrib);
 
 	bt_io_get(io, &gerr,
+			BT_IO_OPT_CID, &cid,
 			BT_IO_OPT_IMTU, &imtu,
 			BT_IO_OPT_OMTU, &omtu,
 			BT_IO_OPT_INVALID);
@@ -6148,6 +6152,10 @@ static uint8_t mtu_att_handle(const uint8_t *cmd, uint16_t cmd_len,
 		return ATT_ECODE_UNLIKELY;
 	}
 
+	/* MTU exchange shall not be used on BR/EDR - Vol 3. Part G. 4.3.1 */
+	if (cid != ATT_CID)
+		return ATT_ECODE_UNLIKELY;
+
 	rsp = g_attrib_get_buffer(dev->attrib, &length);
 
 	/* Respond with our IMTU */
-- 
1.9.3

[PATCH 3/5] android/gatt: Disconnect ATT if MTU is too small on BR/EDR

[Szymon Janc]

On BR/EDR MTU is negotiated using L2CAP configuration and not by
ATT MTU exchange procedure.
---
 android/gatt.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/android/gatt.c b/android/gatt.c
index 49fff6f..45d70fd 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -1519,6 +1519,14 @@ static void connect_cb(GIOChannel *io, GError *gerr, gpointer user_data)
 		goto reply;
 	}
 
+	/* on BR/EDR MTU must not be less then minimal allowed MTU */
+	if (cid != ATT_CID && mtu < ATT_DEFAULT_L2CAP_MTU) {
+		error("gatt: MTU too small (%u bytes)", mtu);
+		device_set_state(dev, DEVICE_DISCONNECTED);
+		status = GATT_FAILURE;
+		goto reply;
+	}
+
 	DBG("mtu %u cid %u", mtu, cid);
 
 	/* on LE we always start with default MTU */
-- 
1.9.3

[PATCH 4/5] android/gatt: Add helper for getting local MTU

[Szymon Janc]

This helper makes code easier to follow. Also local MTU is now
MIN(IMTU,OMTU) to avoid situation that where local OMTU is
lower than negotiated MTU.
---
 android/gatt.c | 97 ++++++++++++++++++++++++++++------------------------------
 1 file changed, 46 insertions(+), 51 deletions(-)

diff --git a/android/gatt.c b/android/gatt.c
index 45d70fd..d5f3d7d 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -954,15 +954,38 @@ static gboolean disconnected_cb(GIOChannel *io, GIOCondition cond,
 	return FALSE;
 }
 
+static bool get_local_mtu(struct gatt_device *dev, uint16_t *mtu)
+{
+	GIOChannel *io;
+	uint16_t imtu, omtu;
+
+	io = g_attrib_get_channel(dev->attrib);
+
+	if (!bt_io_get(io, NULL, BT_IO_OPT_IMTU, &imtu, BT_IO_OPT_OMTU, &omtu,
+							BT_IO_OPT_INVALID)) {
+		error("gatt: Failed to get local MTU");
+		return false;
+	}
+
+	/*
+	 * Limit MTU to  MIN(IMTU, OMTU). This is to avoid situation where
+	 * local OMTU < MIN(remote MTU, IMTU)
+	 */
+	if (mtu)
+		*mtu = MIN(imtu, omtu);
+
+	DBG("mtu %u", *mtu);
+
+	return true;
+}
+
 static void att_handler(const uint8_t *ipdu, uint16_t len, gpointer user_data);
 
 static void exchange_mtu_cb(guint8 status, const guint8 *pdu, guint16 plen,
 							gpointer user_data)
 {
 	struct gatt_device *device = user_data;
-	GIOChannel *io;
-	GError *gerr = NULL;
-	uint16_t rmtu, mtu, imtu;
+	uint16_t rmtu, lmtu, mtu;
 
 	if (status) {
 		error("gatt: MTU exchange: %s", att_ecode2str(status));
@@ -975,28 +998,22 @@ static void exchange_mtu_cb(guint8 status, const guint8 *pdu, guint16 plen,
 	}
 
 	if (rmtu < ATT_DEFAULT_LE_MTU) {
-		error("gatt: MTU exchange: mtu error");
+		error("gatt: MTU exchange: remote MTU invalid (%u)", rmtu);
 		goto failed;
 	}
 
-	io = g_attrib_get_channel(device->attrib);
+	if (!get_local_mtu(device, &lmtu))
+		goto failed;
 
-	bt_io_get(io, &gerr, BT_IO_OPT_IMTU, &imtu, BT_IO_OPT_INVALID);
-	if (gerr) {
-		error("gatt: Could not get imtu: %s", gerr->message);
-		g_error_free(gerr);
+	mtu = MIN(lmtu, rmtu);
 
-		return;
-	}
-
-	mtu = MIN(rmtu, imtu);
-	if (mtu != imtu && !g_attrib_set_mtu(device->attrib, mtu)) {
+	if (mtu != ATT_DEFAULT_LE_MTU &&
+				!g_attrib_set_mtu(device->attrib, mtu)) {
 		error("gatt: MTU exchange failed");
 		goto failed;
 	}
 
-	DBG("MTU exchange succeeded: rmtu:%d, old mtu:%d, new mtu:%d", rmtu,
-								imtu, mtu);
+	DBG("MTU exchange succeeded: remote mtu:%d local mtu:%d", rmtu, lmtu);
 
 failed:
 	device_unref(device);
@@ -1004,21 +1021,12 @@ failed:
 
 static void send_exchange_mtu_request(struct gatt_device *device)
 {
-	GIOChannel *io;
-	GError *gerr = NULL;
-	uint16_t imtu;
-
-	io = g_attrib_get_channel(device->attrib);
-
-	bt_io_get(io, &gerr, BT_IO_OPT_IMTU, &imtu, BT_IO_OPT_INVALID);
-	if (gerr) {
-		error("gatt: Could not get imtu: %s", gerr->message);
-		g_error_free(gerr);
+	uint16_t mtu;
 
+	if (!get_local_mtu(device, &mtu))
 		return;
-	}
 
-	if (!gatt_exchange_mtu(device->attrib, imtu, exchange_mtu_cb,
+	if (!gatt_exchange_mtu(device->attrib, mtu, exchange_mtu_cb,
 							device_ref(device)))
 		device_unref(device);
 }
@@ -6131,50 +6139,37 @@ static uint8_t read_request(const uint8_t *cmd, uint16_t cmd_len,
 static uint8_t mtu_att_handle(const uint8_t *cmd, uint16_t cmd_len,
 							struct gatt_device *dev)
 {
-	uint16_t mtu, imtu, omtu, cid;
+	uint16_t rmtu, mtu, len;
 	size_t length;
-	GIOChannel *io;
-	GError *gerr = NULL;
-	uint16_t len;
 	uint8_t *rsp;
 
 	DBG("");
 
-	len = dec_mtu_req(cmd, cmd_len, &mtu);
+	len = dec_mtu_req(cmd, cmd_len, &rmtu);
 	if (!len)
 		return ATT_ECODE_INVALID_PDU;
 
-	if (mtu < ATT_DEFAULT_LE_MTU)
+	if (rmtu < ATT_DEFAULT_LE_MTU)
 		return ATT_ECODE_REQ_NOT_SUPP;
 
-	io = g_attrib_get_channel(dev->attrib);
-
-	bt_io_get(io, &gerr,
-			BT_IO_OPT_CID, &cid,
-			BT_IO_OPT_IMTU, &imtu,
-			BT_IO_OPT_OMTU, &omtu,
-			BT_IO_OPT_INVALID);
-	if (gerr) {
-		error("bt_io_get: %s", gerr->message);
-		g_error_free(gerr);
+	/* MTU exchange shall not be used on BR/EDR - Vol 3. Part G. 4.3.1 */
+	if (get_cid(dev) != ATT_CID)
 		return ATT_ECODE_UNLIKELY;
-	}
 
-	/* MTU exchange shall not be used on BR/EDR - Vol 3. Part G. 4.3.1 */
-	if (cid != ATT_CID)
+	if (!get_local_mtu(dev, &mtu))
 		return ATT_ECODE_UNLIKELY;
 
 	rsp = g_attrib_get_buffer(dev->attrib, &length);
 
-	/* Respond with our IMTU */
-	len = enc_mtu_resp(imtu, rsp, length);
+	/* Respond with our MTU */
+	len = enc_mtu_resp(mtu, rsp, length);
 	if (!len)
 		return ATT_ECODE_UNLIKELY;
 
 	g_attrib_send(dev->attrib, 0, rsp, len, NULL, NULL, NULL);
 
-	/* Limit OMTU to received value */
-	mtu = MIN(mtu, omtu);
+	/* Limit MTU to received value */
+	mtu = MIN(mtu, rmtu);
 	g_attrib_set_mtu(dev->attrib, mtu);
 
 	return 0;
-- 
1.9.3

[PATCH 5/5] android/gatt: Add helper for updating MTU

[Szymon Janc]

This makes code cleaner and also put all the MTU checks in single
function.
---
 android/gatt.c | 59 +++++++++++++++++++++++++++++++++-------------------------
 1 file changed, 34 insertions(+), 25 deletions(-)

diff --git a/android/gatt.c b/android/gatt.c
index d5f3d7d..a36922c 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -979,13 +979,42 @@ static bool get_local_mtu(struct gatt_device *dev, uint16_t *mtu)
 	return true;
 }
 
+static bool update_mtu(struct gatt_device *device, uint16_t rmtu)
+{
+	uint16_t mtu, lmtu;
+
+	DBG("%u", rmtu);
+
+	if (rmtu < ATT_DEFAULT_LE_MTU) {
+		error("gatt: remote MTU invalid (%u bytes)", rmtu);
+		return false;
+	}
+
+	if (!get_local_mtu(device, &lmtu))
+		return false;
+
+	mtu = MIN(lmtu, rmtu);
+
+	if (mtu == ATT_DEFAULT_LE_MTU)
+		return true;
+
+	if (!g_attrib_set_mtu(device->attrib, mtu)) {
+		error("gatt: Failed to set MTU");
+		return false;
+	}
+
+	DBG("remote_mtu:%d local_mtu:%d", rmtu, lmtu);
+
+	return true;
+}
+
 static void att_handler(const uint8_t *ipdu, uint16_t len, gpointer user_data);
 
 static void exchange_mtu_cb(guint8 status, const guint8 *pdu, guint16 plen,
 							gpointer user_data)
 {
 	struct gatt_device *device = user_data;
-	uint16_t rmtu, lmtu, mtu;
+	uint16_t rmtu;
 
 	if (status) {
 		error("gatt: MTU exchange: %s", att_ecode2str(status));
@@ -997,23 +1026,7 @@ static void exchange_mtu_cb(guint8 status, const guint8 *pdu, guint16 plen,
 		goto failed;
 	}
 
-	if (rmtu < ATT_DEFAULT_LE_MTU) {
-		error("gatt: MTU exchange: remote MTU invalid (%u)", rmtu);
-		goto failed;
-	}
-
-	if (!get_local_mtu(device, &lmtu))
-		goto failed;
-
-	mtu = MIN(lmtu, rmtu);
-
-	if (mtu != ATT_DEFAULT_LE_MTU &&
-				!g_attrib_set_mtu(device->attrib, mtu)) {
-		error("gatt: MTU exchange failed");
-		goto failed;
-	}
-
-	DBG("MTU exchange succeeded: remote mtu:%d local mtu:%d", rmtu, lmtu);
+	update_mtu(device, rmtu);
 
 failed:
 	device_unref(device);
@@ -6149,9 +6162,6 @@ static uint8_t mtu_att_handle(const uint8_t *cmd, uint16_t cmd_len,
 	if (!len)
 		return ATT_ECODE_INVALID_PDU;
 
-	if (rmtu < ATT_DEFAULT_LE_MTU)
-		return ATT_ECODE_REQ_NOT_SUPP;
-
 	/* MTU exchange shall not be used on BR/EDR - Vol 3. Part G. 4.3.1 */
 	if (get_cid(dev) != ATT_CID)
 		return ATT_ECODE_UNLIKELY;
@@ -6166,11 +6176,10 @@ static uint8_t mtu_att_handle(const uint8_t *cmd, uint16_t cmd_len,
 	if (!len)
 		return ATT_ECODE_UNLIKELY;
 
-	g_attrib_send(dev->attrib, 0, rsp, len, NULL, NULL, NULL);
+	if (!update_mtu(dev, rmtu))
+		return ATT_ECODE_UNLIKELY;
 
-	/* Limit MTU to received value */
-	mtu = MIN(mtu, rmtu);
-	g_attrib_set_mtu(dev->attrib, mtu);
+	g_attrib_send(dev->attrib, 0, rsp, len, NULL, NULL, NULL);
 
 	return 0;
 }
-- 
1.9.3

Re: [PATCH 1/5] android/gatt: Fix MTU exchange

[Szymon Janc]

On Saturday 07 of February 2015 16:18:14 Szymon Janc wrote:
> This fix using invalid MTU on LE link. Previous attempt to fix this
> "android/gatt: Fix initial setting of MTU" was not correct since
> on LE default MTU is always 23 and we are not allowed to send bigger
> requests before MTU exchange was performed.
> 
> This was affecting multiple PTS GATT test cases.
> ---
>  android/gatt.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/android/gatt.c b/android/gatt.c
> index 5f3050f..f754f30 100644
> --- a/android/gatt.c
> +++ b/android/gatt.c
> @@ -1490,7 +1490,7 @@ static void connect_cb(GIOChannel *io, GError *gerr,
> gpointer user_data) uint32_t status;
>  	GError *err = NULL;
>  	GAttrib *attrib;
> -	uint16_t mtu;
> +	uint16_t mtu, cid;
> 
>  	if (dev->state != DEVICE_CONNECT_READY) {
>  		error("gatt: Device not in a connecting state!?");
> @@ -1510,14 +1510,21 @@ static void connect_cb(GIOChannel *io, GError *gerr,
> gpointer user_data) goto reply;
>  	}
> 
> -	if (!bt_io_get(io, &err, BT_IO_OPT_IMTU, &mtu, BT_IO_OPT_INVALID)) {
> -		error("gatt: Could not get imtu: %s", err->message);
> +	if (!bt_io_get(io, &err, BT_IO_OPT_IMTU, &mtu, BT_IO_OPT_CID, &cid,
> +							BT_IO_OPT_INVALID)) {
> +		error("gatt: Could not get imtu or cid: %s", err->message);
>  		device_set_state(dev, DEVICE_DISCONNECTED);
>  		status = GATT_FAILURE;
>  		g_error_free(err);
>  		goto reply;
>  	}
> 
> +	DBG("mtu %u cid %u", mtu, cid);
> +
> +	/* on LE we always start with default MTU */
> +	if (cid == ATT_CID)
> +		mtu = ATT_DEFAULT_LE_MTU;
> +
>  	attrib = g_attrib_new(io, mtu);
>  	if (!attrib) {
>  		error("gatt: unable to create new GAttrib instance");

Applied.

-- 
BR
Szymon Janc