* [PATCH 2/5] android/gatt: Exchange MTU only on LE link
2015-02-07 15:18 [PATCH 1/5] android/gatt: Fix MTU exchange Szymon Janc
@ 2015-02-07 15:18 ` Szymon Janc
2015-02-07 15:18 ` [PATCH 3/5] android/gatt: Disconnect ATT if MTU is too small on BR/EDR Szymon Janc
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Szymon Janc @ 2015-02-07 15:18 UTC (permalink / raw)
To: linux-bluetooth; +Cc: 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
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 3/5] android/gatt: Disconnect ATT if MTU is too small on BR/EDR
2015-02-07 15:18 [PATCH 1/5] android/gatt: Fix MTU exchange Szymon Janc
2015-02-07 15:18 ` [PATCH 2/5] android/gatt: Exchange MTU only on LE link Szymon Janc
@ 2015-02-07 15:18 ` Szymon Janc
2015-02-07 15:18 ` [PATCH 4/5] android/gatt: Add helper for getting local MTU Szymon Janc
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Szymon Janc @ 2015-02-07 15:18 UTC (permalink / raw)
To: linux-bluetooth; +Cc: 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
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 4/5] android/gatt: Add helper for getting local MTU
2015-02-07 15:18 [PATCH 1/5] android/gatt: Fix MTU exchange Szymon Janc
2015-02-07 15:18 ` [PATCH 2/5] android/gatt: Exchange MTU only on LE link Szymon Janc
2015-02-07 15:18 ` [PATCH 3/5] android/gatt: Disconnect ATT if MTU is too small on BR/EDR Szymon Janc
@ 2015-02-07 15:18 ` Szymon Janc
2015-02-07 15:18 ` [PATCH 5/5] android/gatt: Add helper for updating MTU Szymon Janc
2015-02-09 14:12 ` [PATCH 1/5] android/gatt: Fix MTU exchange Szymon Janc
4 siblings, 0 replies; 6+ messages in thread
From: Szymon Janc @ 2015-02-07 15:18 UTC (permalink / raw)
To: linux-bluetooth; +Cc: 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
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 5/5] android/gatt: Add helper for updating MTU
2015-02-07 15:18 [PATCH 1/5] android/gatt: Fix MTU exchange Szymon Janc
` (2 preceding siblings ...)
2015-02-07 15:18 ` [PATCH 4/5] android/gatt: Add helper for getting local MTU Szymon Janc
@ 2015-02-07 15:18 ` Szymon Janc
2015-02-09 14:12 ` [PATCH 1/5] android/gatt: Fix MTU exchange Szymon Janc
4 siblings, 0 replies; 6+ messages in thread
From: Szymon Janc @ 2015-02-07 15:18 UTC (permalink / raw)
To: linux-bluetooth; +Cc: 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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 1/5] android/gatt: Fix MTU exchange
2015-02-07 15:18 [PATCH 1/5] android/gatt: Fix MTU exchange Szymon Janc
` (3 preceding siblings ...)
2015-02-07 15:18 ` [PATCH 5/5] android/gatt: Add helper for updating MTU Szymon Janc
@ 2015-02-09 14:12 ` Szymon Janc
4 siblings, 0 replies; 6+ messages in thread
From: Szymon Janc @ 2015-02-09 14:12 UTC (permalink / raw)
To: linux-bluetooth
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
^ permalink raw reply [flat|nested] 6+ messages in thread