[PATCH] android/gatt: Set attrib MTU correctly

[PATCH] android/gatt: Set attrib MTU correctly

[Marcin Kraglak]

We should set g_attrib mtu with MIN of two values: Remote Rx MTU
and local Tx MTU. In previous solution once we set g_attrib mtu, we
could only reduce MTU (because we took previously set g_attrib MTU
and Remote Rx MTU). It affected cases when remote wanted to increase
MTU.
---
 android/gatt.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/android/gatt.c b/android/gatt.c
index 429181f..5bf7694 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -4857,8 +4857,8 @@ 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;
-	size_t omtu;
+	uint16_t mtu, imtu, omtu;
+	size_t length;
 	GIOChannel *io;
 	GError *gerr = NULL;
 	uint16_t len;
@@ -4877,6 +4877,7 @@ static uint8_t mtu_att_handle(const uint8_t *cmd, uint16_t cmd_len,
 
 	bt_io_get(io, &gerr,
 			BT_IO_OPT_IMTU, &imtu,
+			BT_IO_OPT_OMTU, &omtu,
 			BT_IO_OPT_INVALID);
 	if (gerr) {
 		error("bt_io_get: %s", gerr->message);
@@ -4884,10 +4885,10 @@ static uint8_t mtu_att_handle(const uint8_t *cmd, uint16_t cmd_len,
 		return ATT_ECODE_UNLIKELY;
 	}
 
-	rsp = g_attrib_get_buffer(dev->attrib, &omtu);
+	rsp = g_attrib_get_buffer(dev->attrib, &length);
 
 	/* Respond with our IMTU */
-	len = enc_mtu_resp(imtu, rsp, omtu);
+	len = enc_mtu_resp(imtu, rsp, length);
 	if (!len)
 		return ATT_ECODE_UNLIKELY;
 
-- 
1.9.0

Re: [PATCH] android/gatt: Set attrib MTU correctly

[Szymon Janc]

Hi Marcin,

On Wednesday 04 of June 2014 14:54:08 Marcin Kraglak wrote:
> We should set g_attrib mtu with MIN of two values: Remote Rx MTU
> and local Tx MTU. In previous solution once we set g_attrib mtu, we
> could only reduce MTU (because we took previously set g_attrib MTU
> and Remote Rx MTU). It affected cases when remote wanted to increase
> MTU

According to spec:
section 3.4.2.1: "This [Exchange MTU] request shall only be sent once during a 
connection by the client."
section 3.4.2.2: "A device's Exchange MTU Request shall contain the same MTU 
as the device's Exchange MTU Response (i.e. the MTU shall be symmetric)."

So if I get the spec correctly it is illegal to increase (or decrease) MTU 
after it was exchanged (we should probably respond with error if client is 
sending request more than one).

Please correct me if I'm wrong.

> .
> ---
>  android/gatt.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/android/gatt.c b/android/gatt.c
> index 429181f..5bf7694 100644
> --- a/android/gatt.c
> +++ b/android/gatt.c
> @@ -4857,8 +4857,8 @@ 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;
> -	size_t omtu;
> +	uint16_t mtu, imtu, omtu;
> +	size_t length;
>  	GIOChannel *io;
>  	GError *gerr = NULL;
>  	uint16_t len;
> @@ -4877,6 +4877,7 @@ static uint8_t mtu_att_handle(const uint8_t *cmd,
> uint16_t cmd_len,
> 
>  	bt_io_get(io, &gerr,
>  			BT_IO_OPT_IMTU, &imtu,
> +			BT_IO_OPT_OMTU, &omtu,
>  			BT_IO_OPT_INVALID);
>  	if (gerr) {
>  		error("bt_io_get: %s", gerr->message);
> @@ -4884,10 +4885,10 @@ static uint8_t mtu_att_handle(const uint8_t *cmd,
> uint16_t cmd_len, return ATT_ECODE_UNLIKELY;
>  	}
> 
> -	rsp = g_attrib_get_buffer(dev->attrib, &omtu);
> +	rsp = g_attrib_get_buffer(dev->attrib, &length);
> 
>  	/* Respond with our IMTU */
> -	len = enc_mtu_resp(imtu, rsp, omtu);
> +	len = enc_mtu_resp(imtu, rsp, length);
>  	if (!len)
>  		return ATT_ECODE_UNLIKELY;

-- 
BR
Szymon Janc

Re: [PATCH] android/gatt: Set attrib MTU correctly

[Marcin Kraglak]

Hi Szymon,

On 4 June 2014 19:38, Szymon Janc <szymon.janc@tieto.com> wrote:
> Hi Marcin,
>
> On Wednesday 04 of June 2014 14:54:08 Marcin Kraglak wrote:
>> We should set g_attrib mtu with MIN of two values: Remote Rx MTU
>> and local Tx MTU. In previous solution once we set g_attrib mtu, we
>> could only reduce MTU (because we took previously set g_attrib MTU
>> and Remote Rx MTU). It affected cases when remote wanted to increase
>> MTU
>
> According to spec:
> section 3.4.2.1: "This [Exchange MTU] request shall only be sent once during a
> connection by the client."
> section 3.4.2.2: "A device's Exchange MTU Request shall contain the same MTU
> as the device's Exchange MTU Response (i.e. the MTU shall be symmetric)."
>
> So if I get the spec correctly it is illegal to increase (or decrease) MTU
> after it was exchanged (we should probably respond with error if client is
> sending request more than one).
>
> Please correct me if I'm wrong.
>

That is not the case. We set g_attrib MTU when g_attrib_new() is called,
and then it is set to default value - 23 (gattrib.c:499). While
exchanging MTU we
get MIN value of g_attrib MTU, which was previously set to 23 and remote MTU.
So it was set to 23 again. (It means that we always used default MTU).
Now we get remote MTU and local output MTU.

Increasing or decreasing MTU even if it was previously exchanged can be a point
of discussion, however PTS exchange it two times in test case TP/GAC/SR/BV-01-C.
We can create an issue for this case.

>> .
>> ---
>>  android/gatt.c | 9 +++++----
>>  1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/android/gatt.c b/android/gatt.c
>> index 429181f..5bf7694 100644
>> --- a/android/gatt.c
>> +++ b/android/gatt.c
>> @@ -4857,8 +4857,8 @@ 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;
>> -     size_t omtu;
>> +     uint16_t mtu, imtu, omtu;
>> +     size_t length;
>>       GIOChannel *io;
>>       GError *gerr = NULL;
>>       uint16_t len;
>> @@ -4877,6 +4877,7 @@ static uint8_t mtu_att_handle(const uint8_t *cmd,
>> uint16_t cmd_len,
>>
>>       bt_io_get(io, &gerr,
>>                       BT_IO_OPT_IMTU, &imtu,
>> +                     BT_IO_OPT_OMTU, &omtu,
>>                       BT_IO_OPT_INVALID);
>>       if (gerr) {
>>               error("bt_io_get: %s", gerr->message);
>> @@ -4884,10 +4885,10 @@ static uint8_t mtu_att_handle(const uint8_t *cmd,
>> uint16_t cmd_len, return ATT_ECODE_UNLIKELY;
>>       }
>>
>> -     rsp = g_attrib_get_buffer(dev->attrib, &omtu);
>> +     rsp = g_attrib_get_buffer(dev->attrib, &length);
>>
>>       /* Respond with our IMTU */
>> -     len = enc_mtu_resp(imtu, rsp, omtu);
>> +     len = enc_mtu_resp(imtu, rsp, length);
>>       if (!len)
>>               return ATT_ECODE_UNLIKELY;
>
> --
> BR
> Szymon Janc

BR
Marcin

Re: [PATCH] android/gatt: Set attrib MTU correctly

[Szymon Janc]

Hi Marcin,

On Wednesday 04 of June 2014 20:31:56 Marcin Kraglak wrote:
> Hi Szymon,
> 
> On 4 June 2014 19:38, Szymon Janc <szymon.janc@tieto.com> wrote:
> > Hi Marcin,
> > 
> > On Wednesday 04 of June 2014 14:54:08 Marcin Kraglak wrote:
> >> We should set g_attrib mtu with MIN of two values: Remote Rx MTU
> >> and local Tx MTU. In previous solution once we set g_attrib mtu, we
> >> could only reduce MTU (because we took previously set g_attrib MTU
> >> and Remote Rx MTU). It affected cases when remote wanted to increase
> >> MTU
> > 
> > According to spec:
> > section 3.4.2.1: "This [Exchange MTU] request shall only be sent once
> > during a connection by the client."
> > section 3.4.2.2: "A device's Exchange MTU Request shall contain the same
> > MTU as the device's Exchange MTU Response (i.e. the MTU shall be
> > symmetric)."
> > 
> > So if I get the spec correctly it is illegal to increase (or decrease) MTU
> > after it was exchanged (we should probably respond with error if client is
> > sending request more than one).
> > 
> > Please correct me if I'm wrong.
> 
> That is not the case. We set g_attrib MTU when g_attrib_new() is called,
> and then it is set to default value - 23 (gattrib.c:499). While
> exchanging MTU we
> get MIN value of g_attrib MTU, which was previously set to 23 and remote
> MTU. So it was set to 23 again. (It means that we always used default MTU).
> Now we get remote MTU and local output MTU.

Fair enough. This patch is now applied, thanks.

> 
> Increasing or decreasing MTU even if it was previously exchanged can be a
> point of discussion, however PTS exchange it two times in test case
> TP/GAC/SR/BV-01-C. We can create an issue for this case.

I guess PTS errata should be filled to clear this up.

> 
> >> .
> >> ---
> >> 
> >>  android/gatt.c | 9 +++++----
> >>  1 file changed, 5 insertions(+), 4 deletions(-)
> >> 
> >> diff --git a/android/gatt.c b/android/gatt.c
> >> index 429181f..5bf7694 100644
> >> --- a/android/gatt.c
> >> +++ b/android/gatt.c
> >> @@ -4857,8 +4857,8 @@ 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;
> >> -     size_t omtu;
> >> +     uint16_t mtu, imtu, omtu;
> >> +     size_t length;
> >> 
> >>       GIOChannel *io;
> >>       GError *gerr = NULL;
> >>       uint16_t len;
> >> 
> >> @@ -4877,6 +4877,7 @@ static uint8_t mtu_att_handle(const uint8_t *cmd,
> >> uint16_t cmd_len,
> >> 
> >>       bt_io_get(io, &gerr,
> >>       
> >>                       BT_IO_OPT_IMTU, &imtu,
> >> 
> >> +                     BT_IO_OPT_OMTU, &omtu,
> >> 
> >>                       BT_IO_OPT_INVALID);
> >>       
> >>       if (gerr) {
> >>       
> >>               error("bt_io_get: %s", gerr->message);
> >> 
> >> @@ -4884,10 +4885,10 @@ static uint8_t mtu_att_handle(const uint8_t *cmd,
> >> uint16_t cmd_len, return ATT_ECODE_UNLIKELY;
> >> 
> >>       }
> >> 
> >> -     rsp = g_attrib_get_buffer(dev->attrib, &omtu);
> >> +     rsp = g_attrib_get_buffer(dev->attrib, &length);
> >> 
> >>       /* Respond with our IMTU */
> >> 
> >> -     len = enc_mtu_resp(imtu, rsp, omtu);
> >> +     len = enc_mtu_resp(imtu, rsp, length);
> >> 
> >>       if (!len)
> >>       
> >>               return ATT_ECODE_UNLIKELY;
> > 
> > --
> > BR
> > Szymon Janc
> 
> BR
> Marcin

-- 
BR
Szymon Janc