2011-02-24 21:37:34

by Bruna Moreira

[permalink] [raw]
Subject: [PATCH] Fix MTU value used on MTU exchange response

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



2011-02-24 22:06:09

by Brian Gix

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

Hi Bruna,

On 2/24/2011 1:58 PM, Bruna Moreira wrote:
> Hi Brian,
>
> On 2/24/11, Brian Gix<[email protected]> 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
[email protected]
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

2011-02-24 21:58:11

by Bruna Moreira

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

Hi Brian,

On 2/24/11, Brian Gix <[email protected]> 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

2011-02-24 21:43:35

by Brian Gix

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

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
[email protected]
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

2011-03-10 09:09:16

by Johan Hedberg

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

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

2011-03-04 20:17:45

by Bruna Moreira

[permalink] [raw]
Subject: [PATCHv3 2/2] Fix MTU value used on MTU exchange response

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


2011-03-04 20:17:44

by Bruna Moreira

[permalink] [raw]
Subject: [PATCHv3 1/2] Use CID to infer transport type

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


2011-03-04 19:32:11

by Bruna Moreira

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

On 3/2/11, Bruna Moreira <[email protected]> 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

2011-03-02 16:34:44

by Bruna Moreira

[permalink] [raw]
Subject: [PATCHv2 2/2] Fix MTU value used on MTU exchange response

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


2011-03-02 16:34:43

by Bruna Moreira

[permalink] [raw]
Subject: [PATCHv2 1/2] Use CID to infer transport type

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