* [PATCH] android/gatt: Set attrib MTU correctly
@ 2014-06-04 12:54 Marcin Kraglak
2014-06-04 17:38 ` Szymon Janc
0 siblings, 1 reply; 4+ messages in thread
From: Marcin Kraglak @ 2014-06-04 12:54 UTC (permalink / raw)
To: linux-bluetooth
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
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] android/gatt: Set attrib MTU correctly
2014-06-04 12:54 [PATCH] android/gatt: Set attrib MTU correctly Marcin Kraglak
@ 2014-06-04 17:38 ` Szymon Janc
2014-06-04 18:31 ` Marcin Kraglak
0 siblings, 1 reply; 4+ messages in thread
From: Szymon Janc @ 2014-06-04 17:38 UTC (permalink / raw)
To: Marcin Kraglak; +Cc: linux-bluetooth
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
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] android/gatt: Set attrib MTU correctly
2014-06-04 17:38 ` Szymon Janc
@ 2014-06-04 18:31 ` Marcin Kraglak
2014-06-06 4:10 ` Szymon Janc
0 siblings, 1 reply; 4+ messages in thread
From: Marcin Kraglak @ 2014-06-04 18:31 UTC (permalink / raw)
To: Szymon Janc; +Cc: linux-bluetooth@vger.kernel.org development
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
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] android/gatt: Set attrib MTU correctly
2014-06-04 18:31 ` Marcin Kraglak
@ 2014-06-06 4:10 ` Szymon Janc
0 siblings, 0 replies; 4+ messages in thread
From: Szymon Janc @ 2014-06-06 4:10 UTC (permalink / raw)
To: Marcin Kraglak; +Cc: linux-bluetooth@vger.kernel.org development
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-06-06 4:10 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-06-04 12:54 [PATCH] android/gatt: Set attrib MTU correctly Marcin Kraglak
2014-06-04 17:38 ` Szymon Janc
2014-06-04 18:31 ` Marcin Kraglak
2014-06-06 4:10 ` Szymon Janc
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.