[PATCH] Fix MTU value used on MTU exchange response

[PATCH] Fix MTU value used on MTU exchange response

[Bruna Moreira]

The new MTU value only should be applied in server side after sending
the ATT_MTU_RESP so encode the response using the old MTU value.
---
 src/attrib-server.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/src/attrib-server.c b/src/attrib-server.c
index 62c10f4..9de4c81 100644
--- a/src/attrib-server.c
+++ b/src/attrib-server.c
@@ -773,9 +773,11 @@ static uint16_t write_value(struct gatt_channel *channel, uint16_t handle,
 static uint16_t mtu_exchange(struct gatt_channel *channel, uint16_t mtu,
 		uint8_t *pdu, int len)
 {
+	guint old_mtu = channel->mtu;
+
 	channel->mtu = MIN(mtu, channel->mtu);
 
-	return enc_mtu_resp(channel->mtu, pdu, len);
+	return enc_mtu_resp(old_mtu, pdu, len);
 }
 
 static void channel_disconnect(void *user_data)
-- 
1.7.0.4

Re: [PATCH] Fix MTU value used on MTU exchange response

[Brian Gix]

On 2/24/2011 1:37 PM, Bruna Moreira wrote:
> The new MTU value only should be applied in server side after sending
> the ATT_MTU_RESP so encode the response using the old MTU value.
> ---
>   src/attrib-server.c |    4 +++-
>   1 files changed, 3 insertions(+), 1 deletions(-)
>
> diff --git a/src/attrib-server.c b/src/attrib-server.c
> index 62c10f4..9de4c81 100644
> --- a/src/attrib-server.c
> +++ b/src/attrib-server.c
> @@ -773,9 +773,11 @@ static uint16_t write_value(struct gatt_channel *channel, uint16_t handle,
>   static uint16_t mtu_exchange(struct gatt_channel *channel, uint16_t mtu,
>   		uint8_t *pdu, int len)
>   {
> +	guint old_mtu = channel->mtu;
> +
>   	channel->mtu = MIN(mtu, channel->mtu);
>
> -	return enc_mtu_resp(channel->mtu, pdu, len);
> +	return enc_mtu_resp(old_mtu, pdu, len);
>   }
>
>   static void channel_disconnect(void *user_data)


I don't think this is correct.  The change has us replying with our old 
MTU.  What part of the specification justifies this change?

-- 
Brian Gix
bgix@codeaurora.org
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

Re: [PATCH] Fix MTU value used on MTU exchange response

[Bruna Moreira]

Hi Brian,

On 2/24/11, Brian Gix <bgix@codeaurora.org> wrote:
> On 2/24/2011 1:37 PM, Bruna Moreira wrote:
>> The new MTU value only should be applied in server side after sending
>> the ATT_MTU_RESP so encode the response using the old MTU value.
>> ---
>>   src/attrib-server.c |    4 +++-
>>   1 files changed, 3 insertions(+), 1 deletions(-)
>>
>> diff --git a/src/attrib-server.c b/src/attrib-server.c
>> index 62c10f4..9de4c81 100644
>> --- a/src/attrib-server.c
>> +++ b/src/attrib-server.c
>> @@ -773,9 +773,11 @@ static uint16_t write_value(struct gatt_channel
>> *channel, uint16_t handle,
>>   static uint16_t mtu_exchange(struct gatt_channel *channel, uint16_t mtu,
>>   		uint8_t *pdu, int len)
>>   {
>> +	guint old_mtu = channel->mtu;
>> +
>>   	channel->mtu = MIN(mtu, channel->mtu);
>>
>> -	return enc_mtu_resp(channel->mtu, pdu, len);
>> +	return enc_mtu_resp(old_mtu, pdu, len);
>>   }
>>
>>   static void channel_disconnect(void *user_data)
>
>
> I don't think this is correct.  The change has us replying with our old
> MTU.  What part of the specification justifies this change?

>From "3.4.2.2 Exchange MTU Response" section:

"This ATT_MTU value shall be applied in the server after this response has
been sent and before any other attribute protocol PDU is sent."

BR,
-- 
Bruna Moreira
Instituto Nokia de Tecnologia (INdT)
Manaus - Brazil

Re: [PATCH] Fix MTU value used on MTU exchange response

[Brian Gix]

Hi Bruna,

On 2/24/2011 1:58 PM, Bruna Moreira wrote:
> Hi Brian,
>
> On 2/24/11, Brian Gix<bgix@codeaurora.org>  wrote:
>> On 2/24/2011 1:37 PM, Bruna Moreira wrote:
>>> The new MTU value only should be applied in server side after sending
>>> the ATT_MTU_RESP so encode the response using the old MTU value.
>>> ---
>>>    src/attrib-server.c |    4 +++-
>>>    1 files changed, 3 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/src/attrib-server.c b/src/attrib-server.c
>>> index 62c10f4..9de4c81 100644
>>> --- a/src/attrib-server.c
>>> +++ b/src/attrib-server.c
>>> @@ -773,9 +773,11 @@ static uint16_t write_value(struct gatt_channel
>>> *channel, uint16_t handle,
>>>    static uint16_t mtu_exchange(struct gatt_channel *channel, uint16_t mtu,
>>>    		uint8_t *pdu, int len)
>>>    {
>>> +	guint old_mtu = channel->mtu;
>>> +
>>>    	channel->mtu = MIN(mtu, channel->mtu);
>>>
>>> -	return enc_mtu_resp(channel->mtu, pdu, len);
>>> +	return enc_mtu_resp(old_mtu, pdu, len);
>>>    }
>>>
>>>    static void channel_disconnect(void *user_data)
>>
>>
>> I don't think this is correct.  The change has us replying with our old
>> MTU.  What part of the specification justifies this change?
>
>  From "3.4.2.2 Exchange MTU Response" section:
>
> "This ATT_MTU value shall be applied in the server after this response has
> been sent and before any other attribute protocol PDU is sent."
>
> BR,


OK,  I re-read the MTU section, and agree that your reading appears to 
be correct.  I wonder if we should also be testing for the default 
(minimum) MTUs of 23 and 48 for LE and BR/EDR respectively here as well, 
per:

"If either Client Rx MTU or Service Rx MTU are incorrectly less than the 
default ATT_MTU, then the ATT_MTU shall not be changed and the ATT_MTU 
shall be the default ATT_MTU."

-- 
Brian Gix
bgix@codeaurora.org
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

[PATCHv2 1/2] Use CID to infer transport type

[Bruna Moreira]

Instead of comparing GIOChannel pointers, use the CID returned by
bt_io_get() to infer the transport type. LE uses the fixed CID for GATT.
---
 src/attrib-server.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/src/attrib-server.c b/src/attrib-server.c
index b980b28..61db851 100644
--- a/src/attrib-server.c
+++ b/src/attrib-server.c
@@ -899,7 +899,7 @@ done:
 static void connect_event(GIOChannel *io, GError *err, void *user_data)
 {
 	struct gatt_channel *channel;
-	GIOChannel **server_io = user_data;
+	uint16_t cid;
 	GError *gerr = NULL;
 
 	if (err) {
@@ -912,6 +912,7 @@ static void connect_event(GIOChannel *io, GError *err, void *user_data)
 	bt_io_get(io, BT_IO_L2CAP, &gerr,
 			BT_IO_OPT_SOURCE_BDADDR, &channel->src,
 			BT_IO_OPT_DEST_BDADDR, &channel->dst,
+			BT_IO_OPT_CID, &cid,
 			BT_IO_OPT_INVALID);
 	if (gerr) {
 		error("bt_io_get: %s", gerr->message);
@@ -921,7 +922,7 @@ static void connect_event(GIOChannel *io, GError *err, void *user_data)
 		return;
 	}
 
-	if (server_io == &l2cap_io)
+	if (cid != GATT_CID)
 		channel->mtu = ATT_DEFAULT_L2CAP_MTU;
 	else
 		channel->mtu = ATT_DEFAULT_LE_MTU;
@@ -1046,7 +1047,7 @@ int attrib_server_init(void)
 
 	/* BR/EDR socket */
 	l2cap_io = bt_io_listen(BT_IO_L2CAP, NULL, confirm_event,
-					&l2cap_io, NULL, &gerr,
+					NULL, NULL, &gerr,
 					BT_IO_OPT_SOURCE_BDADDR, BDADDR_ANY,
 					BT_IO_OPT_PSM, GATT_PSM,
 					BT_IO_OPT_SEC_LEVEL, BT_IO_SEC_LOW,
-- 
1.7.0.4

Re: [PATCHv2 1/2] Use CID to infer transport type

[Bruna Moreira]

On 3/2/11, Bruna Moreira <bruna.moreira@openbossa.org> wrote:
> Instead of comparing GIOChannel pointers, use the CID returned by
> bt_io_get() to infer the transport type. LE uses the fixed CID for GATT.
> ---
>  src/attrib-server.c |    7 ++++---
>  1 files changed, 4 insertions(+), 3 deletions(-)
>

Please ignore this series, I will send a fixed one (I found a bug
after running more tests).

BR,
-- 
Bruna Moreira
Instituto Nokia de Tecnologia (INdT)
Manaus - Brazil

[PATCHv2 2/2] Fix MTU value used on MTU exchange response

[Bruna Moreira]

ATT_MTU_RESP shall send the original MTU value prior to the exchange. If
client sends an invalid value, the server shall use the ATT_DEFAULT_LE_MTU
value. This operation is not supported in BR/EDR, so if any client sends
this request it will be ignored by server and it will reply with
"Request not supported".

For BR/EDR, the MTU is limited to ATT_MAX_MTU (currently 256). This
avoids allocating a big buffer when most ATT PDUs are small.

Note: the kernel currently limits the minimum MTU to 48, regardless of
transport type. This limit is valid only for BR/EDR, for LE the minimum
is 23. This bug will be fixed, but it does not affect the outgoing MTU
for new created LE sockets, which are correctly set to 23.
---
 src/attrib-server.c |   23 +++++++++++++++++++----
 1 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/src/attrib-server.c b/src/attrib-server.c
index 61db851..cc4b601 100644
--- a/src/attrib-server.c
+++ b/src/attrib-server.c
@@ -59,6 +59,7 @@ struct gatt_channel {
 	GSList *indicate;
 	GAttrib *attrib;
 	guint mtu;
+	gboolean le;
 	guint id;
 	gboolean encrypted;
 };
@@ -759,9 +760,14 @@ static uint16_t write_value(struct gatt_channel *channel, uint16_t handle,
 static uint16_t mtu_exchange(struct gatt_channel *channel, uint16_t mtu,
 		uint8_t *pdu, int len)
 {
-	channel->mtu = MIN(mtu, channel->mtu);
+	guint old_mtu = channel->mtu;
 
-	return enc_mtu_resp(channel->mtu, pdu, len);
+	if (mtu < ATT_DEFAULT_LE_MTU)
+		channel->mtu = ATT_DEFAULT_LE_MTU;
+	else
+		channel->mtu = MIN(mtu, channel->mtu);
+
+	return enc_mtu_resp(old_mtu, pdu, len);
 }
 
 static void channel_disconnect(void *user_data)
@@ -831,6 +837,11 @@ static void channel_handler(const uint8_t *ipdu, uint16_t len,
 		length = read_blob(channel, start, offset, opdu, channel->mtu);
 		break;
 	case ATT_OP_MTU_REQ:
+		if (!channel->le) {
+			status = ATT_ECODE_REQ_NOT_SUPP;
+			goto done;
+		}
+
 		length = dec_mtu_req(ipdu, len, &mtu);
 		if (length == 0) {
 			status = ATT_ECODE_INVALID_PDU;
@@ -913,6 +924,7 @@ static void connect_event(GIOChannel *io, GError *err, void *user_data)
 			BT_IO_OPT_SOURCE_BDADDR, &channel->src,
 			BT_IO_OPT_DEST_BDADDR, &channel->dst,
 			BT_IO_OPT_CID, &cid,
+			BT_IO_OPT_OMTU, &channel->mtu,
 			BT_IO_OPT_INVALID);
 	if (gerr) {
 		error("bt_io_get: %s", gerr->message);
@@ -922,10 +934,13 @@ static void connect_event(GIOChannel *io, GError *err, void *user_data)
 		return;
 	}
 
+	if (channel->mtu > ATT_MAX_MTU)
+		channel->mtu = ATT_MAX_MTU;
+
 	if (cid != GATT_CID)
-		channel->mtu = ATT_DEFAULT_L2CAP_MTU;
+		channel->le = FALSE;
 	else
-		channel->mtu = ATT_DEFAULT_LE_MTU;
+		channel->le = TRUE;
 
 	channel->attrib = g_attrib_new(io);
 	g_io_channel_unref(io);
-- 
1.7.0.4

[PATCHv3 1/2] Use CID to infer transport type

[Bruna Moreira]

Instead of comparing GIOChannel pointers, use the CID returned by
bt_io_get() to infer the transport type. LE uses the fixed CID for GATT.
---
 src/attrib-server.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/src/attrib-server.c b/src/attrib-server.c
index b980b28..61db851 100644
--- a/src/attrib-server.c
+++ b/src/attrib-server.c
@@ -899,7 +899,7 @@ done:
 static void connect_event(GIOChannel *io, GError *err, void *user_data)
 {
 	struct gatt_channel *channel;
-	GIOChannel **server_io = user_data;
+	uint16_t cid;
 	GError *gerr = NULL;
 
 	if (err) {
@@ -912,6 +912,7 @@ static void connect_event(GIOChannel *io, GError *err, void *user_data)
 	bt_io_get(io, BT_IO_L2CAP, &gerr,
 			BT_IO_OPT_SOURCE_BDADDR, &channel->src,
 			BT_IO_OPT_DEST_BDADDR, &channel->dst,
+			BT_IO_OPT_CID, &cid,
 			BT_IO_OPT_INVALID);
 	if (gerr) {
 		error("bt_io_get: %s", gerr->message);
@@ -921,7 +922,7 @@ static void connect_event(GIOChannel *io, GError *err, void *user_data)
 		return;
 	}
 
-	if (server_io == &l2cap_io)
+	if (cid != GATT_CID)
 		channel->mtu = ATT_DEFAULT_L2CAP_MTU;
 	else
 		channel->mtu = ATT_DEFAULT_LE_MTU;
@@ -1046,7 +1047,7 @@ int attrib_server_init(void)
 
 	/* BR/EDR socket */
 	l2cap_io = bt_io_listen(BT_IO_L2CAP, NULL, confirm_event,
-					&l2cap_io, NULL, &gerr,
+					NULL, NULL, &gerr,
 					BT_IO_OPT_SOURCE_BDADDR, BDADDR_ANY,
 					BT_IO_OPT_PSM, GATT_PSM,
 					BT_IO_OPT_SEC_LEVEL, BT_IO_SEC_LOW,
-- 
1.7.0.4

Re: [PATCHv3 1/2] Use CID to infer transport type

[Johan Hedberg]

Hi Bruna,

On Fri, Mar 04, 2011, Bruna Moreira wrote:
> Instead of comparing GIOChannel pointers, use the CID returned by
> bt_io_get() to infer the transport type. LE uses the fixed CID for GATT.
> ---
>  src/attrib-server.c |    7 ++++---
>  1 files changed, 4 insertions(+), 3 deletions(-)

Both patches have been pushes upstream. Thanks.

Johan

[PATCHv3 2/2] Fix MTU value used on MTU exchange response

[Bruna Moreira]

ATT_MTU_RESP shall send the original MTU value prior to the exchange. If
client sends an invalid value, the server shall use the ATT_DEFAULT_LE_MTU
value. This operation is not supported in BR/EDR, so if any client sends
this request it will be ignored by server and it will reply with
"Request not supported".

For BR/EDR, the MTU is limited to ATT_MAX_MTU (currently 256). This
avoids allocating a big buffer when most ATT PDUs are small.

Note: the kernel currently limits the minimum MTU to 48, regardless of
transport type. This limit is valid only for BR/EDR, for LE the minimum
is 23. This bug will be fixed, but it does not affect the outgoing MTU
for new created LE sockets, which are correctly set to 23.
---
 src/attrib-server.c |   27 +++++++++++++++++++++++----
 1 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/src/attrib-server.c b/src/attrib-server.c
index 61db851..8464d2a 100644
--- a/src/attrib-server.c
+++ b/src/attrib-server.c
@@ -59,6 +59,7 @@ struct gatt_channel {
 	GSList *indicate;
 	GAttrib *attrib;
 	guint mtu;
+	gboolean le;
 	guint id;
 	gboolean encrypted;
 };
@@ -759,9 +760,18 @@ static uint16_t write_value(struct gatt_channel *channel, uint16_t handle,
 static uint16_t mtu_exchange(struct gatt_channel *channel, uint16_t mtu,
 		uint8_t *pdu, int len)
 {
-	channel->mtu = MIN(mtu, channel->mtu);
+	guint old_mtu = channel->mtu;
 
-	return enc_mtu_resp(channel->mtu, pdu, len);
+	if (mtu < ATT_DEFAULT_LE_MTU)
+		channel->mtu = ATT_DEFAULT_LE_MTU;
+	else
+		channel->mtu = MIN(mtu, channel->mtu);
+
+	bt_io_set(le_io, BT_IO_L2CAP, NULL,
+			BT_IO_OPT_OMTU, channel->mtu,
+			BT_IO_OPT_INVALID);
+
+	return enc_mtu_resp(old_mtu, pdu, len);
 }
 
 static void channel_disconnect(void *user_data)
@@ -831,6 +841,11 @@ static void channel_handler(const uint8_t *ipdu, uint16_t len,
 		length = read_blob(channel, start, offset, opdu, channel->mtu);
 		break;
 	case ATT_OP_MTU_REQ:
+		if (!channel->le) {
+			status = ATT_ECODE_REQ_NOT_SUPP;
+			goto done;
+		}
+
 		length = dec_mtu_req(ipdu, len, &mtu);
 		if (length == 0) {
 			status = ATT_ECODE_INVALID_PDU;
@@ -913,6 +928,7 @@ static void connect_event(GIOChannel *io, GError *err, void *user_data)
 			BT_IO_OPT_SOURCE_BDADDR, &channel->src,
 			BT_IO_OPT_DEST_BDADDR, &channel->dst,
 			BT_IO_OPT_CID, &cid,
+			BT_IO_OPT_OMTU, &channel->mtu,
 			BT_IO_OPT_INVALID);
 	if (gerr) {
 		error("bt_io_get: %s", gerr->message);
@@ -922,10 +938,13 @@ static void connect_event(GIOChannel *io, GError *err, void *user_data)
 		return;
 	}
 
+	if (channel->mtu > ATT_MAX_MTU)
+		channel->mtu = ATT_MAX_MTU;
+
 	if (cid != GATT_CID)
-		channel->mtu = ATT_DEFAULT_L2CAP_MTU;
+		channel->le = FALSE;
 	else
-		channel->mtu = ATT_DEFAULT_LE_MTU;
+		channel->le = TRUE;
 
 	channel->attrib = g_attrib_new(io);
 	g_io_channel_unref(io);
-- 
1.7.0.4