[PATCH] src/device.c : Fix BREDR-ATT MTU issue

[PATCH] src/device.c : Fix BREDR-ATT MTU issue

[Nagaraj D R]

For BREDR-ATT, according to spec, ATT MTU is same has
L2CAP configured MTU on which ATT is running. So, set the MTU to
L2CAP configuration and for LE-ATT adjust the ATT MTU based on
EXCHANGE_MTU request and response.
---
 src/device.c              | 2 +-
 src/shared/gatt-helpers.c | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/device.c b/src/device.c
index f693b70..cf4c8df 100644
--- a/src/device.c
+++ b/src/device.c
@@ -4922,7 +4922,7 @@ bool device_attach_att(struct btd_device *dev, GIOChannel *io)
 	}
 
 	dev->att_mtu = MIN(mtu, BT_ATT_MAX_LE_MTU);
-	attrib = g_attrib_new(io, BT_ATT_DEFAULT_LE_MTU, false);
+	attrib = g_attrib_new(io, dev->att_mtu, false);
 	if (!attrib) {
 		error("Unable to create new GAttrib instance");
 		return false;
diff --git a/src/shared/gatt-helpers.c b/src/shared/gatt-helpers.c
index 6b39bb1..8ec65be 100644
--- a/src/shared/gatt-helpers.c
+++ b/src/shared/gatt-helpers.c
@@ -517,6 +517,7 @@ static void mtu_cb(uint8_t opcode, const void *pdu, uint16_t length,
 	if (opcode == BT_ATT_OP_ERROR_RSP) {
 		success = false;
 		att_ecode = process_error(pdu, length);
+		bt_att_set_mtu(op->att, BT_ATT_DEFAULT_LE_MTU);
 		goto done;
 	}
 
-- 
1.9.1

Re: [PATCH] src/device.c : Fix BREDR-ATT MTU issue

[Luiz Augusto von Dentz]

Hi Nagaraj,
On Wed, May 2, 2018 at 11:40 AM Nagaraj D R <nagaraj.dr@samsung.com> wrote:

> For BREDR-ATT, according to spec, ATT MTU is same has
> L2CAP configured MTU on which ATT is running. So, set the MTU to
> L2CAP configuration and for LE-ATT adjust the ATT MTU based on
> EXCHANGE_MTU request and response.
> ---
>   src/device.c              | 2 +-
>   src/shared/gatt-helpers.c | 1 +
>   2 files changed, 2 insertions(+), 1 deletion(-)

> diff --git a/src/device.c b/src/device.c
> index f693b70..cf4c8df 100644
> --- a/src/device.c
> +++ b/src/device.c
> @@ -4922,7 +4922,7 @@ bool device_attach_att(struct btd_device *dev,
GIOChannel *io)
>          }

>          dev->att_mtu = MIN(mtu, BT_ATT_MAX_LE_MTU);
> -       attrib = g_attrib_new(io, BT_ATT_DEFAULT_LE_MTU, false);
> +       attrib = g_attrib_new(io, dev->att_mtu, false);

This would actually undo the following patch:

     src/device: Use BT_ATT_DEFAULT_LE_MTU as default MTU

     Use the default MTU until an MTU exchange has taken place and
     something else has been negotiated. If either side does not support
     MTU exchange, the connection shall continue to use this default
     value instead of the device maximum which was the previous behavior.

We should probably check if the cid is for LE or not and reset mtu to
BT_ATT_DEFAULT_LE_MTU that way att_mtu should be valid for either LE or
BR/EDR.

>          if (!attrib) {
>                  error("Unable to create new GAttrib instance");
>                  return false;
> diff --git a/src/shared/gatt-helpers.c b/src/shared/gatt-helpers.c
> index 6b39bb1..8ec65be 100644
> --- a/src/shared/gatt-helpers.c
> +++ b/src/shared/gatt-helpers.c
> @@ -517,6 +517,7 @@ static void mtu_cb(uint8_t opcode, const void *pdu,
uint16_t length,
>          if (opcode == BT_ATT_OP_ERROR_RSP) {
>                  success = false;
>                  att_ecode = process_error(pdu, length);
> +               bt_att_set_mtu(op->att, BT_ATT_DEFAULT_LE_MTU);

I not following this one, we only really use bt_att_set_mtu if the command
succeeds so this should not be necessary, or this is due Exchange being
used multiple times? I recall the spec not allowing that to happen, and if
does Im not sure why we would have to return to the default instead of the
value previously set in case the of error response.

>                  goto done;
>          }

> --
> 1.9.1

> --
> To unsubscribe from this list: send the line "unsubscribe
linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Luiz Augusto von Dentz

RE: Re: [PATCH] src/device.c : Fix BREDR-ATT MTU issue

[Nagaraj D R]

Hello Luiz
=C2=A0=0D=0ASender=20:=20Luiz=20Augusto=20von=20Dentz=C2=A0<luiz.dentz=40gm=
ail.com>=0D=0A>Hi=C2=A0Nagaraj,=0D=0A>On=C2=A0Wed,=C2=A0May=C2=A02,=C2=A020=
18=C2=A0at=C2=A011:40=C2=A0AM=C2=A0Nagaraj=C2=A0D=C2=A0R=C2=A0<nagaraj.dr=
=40samsung.com>=C2=A0wrote:=0D=0A=C2=A0=0D=0A>>=C2=A0For=C2=A0BREDR-ATT,=C2=
=A0according=C2=A0to=C2=A0spec,=C2=A0ATT=C2=A0MTU=C2=A0is=C2=A0same=C2=A0ha=
s=0D=0A>>=C2=A0L2CAP=C2=A0configured=C2=A0MTU=C2=A0on=C2=A0which=C2=A0ATT=
=C2=A0is=C2=A0running.=C2=A0So,=C2=A0set=C2=A0the=C2=A0MTU=C2=A0to=0D=0A>>=
=C2=A0L2CAP=C2=A0configuration=C2=A0and=C2=A0for=C2=A0LE-ATT=C2=A0adjust=C2=
=A0the=C2=A0ATT=C2=A0MTU=C2=A0based=C2=A0on=0D=0A>>=C2=A0EXCHANGE_MTU=C2=A0=
request=C2=A0and=C2=A0response.=0D=0A>>=C2=A0---=0D=0A>>=C2=A0=C2=A0=C2=A0s=
rc/device.c=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=7C=C2=A02=C2=A0+-=0D=0A>>=C2=A0=C2=A0=C2=A0src/shared=
/gatt-helpers.c=C2=A0=7C=C2=A01=C2=A0+=0D=0A>>=C2=A0=C2=A0=C2=A02=C2=A0file=
s=C2=A0changed,=C2=A02=C2=A0insertions(+),=C2=A01=C2=A0deletion(-)=0D=0A=C2=
=A0=0D=0A>>=C2=A0diff=C2=A0--git=C2=A0a/src/device.c=C2=A0b/src/device.c=0D=
=0A>>=C2=A0index=C2=A0f693b70..cf4c8df=C2=A0100644=0D=0A>>=C2=A0---=C2=A0a/=
src/device.c=0D=0A>>=C2=A0+++=C2=A0b/src/device.c=0D=0A>>=C2=A0=40=40=C2=A0=
-4922,7=C2=A0+4922,7=C2=A0=40=40=C2=A0bool=C2=A0device_attach_att(struct=C2=
=A0btd_device=C2=A0*dev,=0D=0AGIOChannel=C2=A0*io)=0D=0A>>=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=7D=0D=0A=C2=A0=0D=0A>>=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0dev->att_mtu=C2=A0=3D=
=C2=A0MIN(mtu,=C2=A0BT_ATT_MAX_LE_MTU);=0D=0A>>=C2=A0-=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0attrib=C2=A0=3D=C2=A0g_attrib_new(io,=C2=A0BT_ATT_DEFA=
ULT_LE_MTU,=C2=A0false);=0D=0A>>=C2=A0+=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0attrib=C2=A0=3D=C2=A0g_attrib_new(io,=C2=A0dev->att_mtu,=C2=A0false);=
=0D=0A=C2=A0=0D=0A>This=C2=A0would=C2=A0actually=C2=A0undo=C2=A0the=C2=A0fo=
llowing=C2=A0patch:=0D=0A=C2=A0=0D=0A>=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0src/dev=
ice:=C2=A0Use=C2=A0BT_ATT_DEFAULT_LE_MTU=C2=A0as=C2=A0default=C2=A0MTU=0D=
=0A=C2=A0=0D=0A>=C2=A0=C2=A0=C2=A0Use=C2=A0the=C2=A0default=C2=A0MTU=C2=A0u=
ntil=C2=A0an=C2=A0MTU=C2=A0exchange=C2=A0has=C2=A0taken=C2=A0place=C2=A0and=
=0D=0A>=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0something=C2=A0else=C2=A0has=C2=A0been=
=C2=A0negotiated.=C2=A0If=C2=A0either=C2=A0side=C2=A0does=C2=A0not=C2=A0sup=
port=0D=0A>=C2=A0=C2=A0=C2=A0=C2=A0MTU=C2=A0exchange,=C2=A0the=C2=A0connect=
ion=C2=A0shall=C2=A0continue=C2=A0to=C2=A0use=C2=A0this=C2=A0default=0D=0A>=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0value=C2=A0instead=C2=A0of=C2=A0the=C2=A0devi=
ce=C2=A0maximum=C2=A0which=C2=A0was=C2=A0the=C2=A0previous=C2=A0behavior.=
=0D=0A=C2=A0=0D=0A>We=C2=A0should=C2=A0probably=C2=A0check=C2=A0if=C2=A0the=
=C2=A0cid=C2=A0is=C2=A0for=C2=A0LE=C2=A0or=C2=A0not=C2=A0and=C2=A0reset=C2=
=A0mtu=C2=A0to=0D=0A>BT_ATT_DEFAULT_LE_MTU=C2=A0that=C2=A0way=C2=A0att_mtu=
=C2=A0should=C2=A0be=C2=A0valid=C2=A0for=C2=A0either=C2=A0LE=C2=A0or=0D=0A>=
BR/EDR.=0D=0A=0D=0AI=20agree,=20we=20should=20check=20for=20CID.=0D=0AIf=20=
it=20is=20BREDR-ATT=20connection=20only=20then=20consider=20L2CAP=20MTU=20o=
therwise=0D=0Aset=20the=20MTU=20to=20min=20value=20and=20set=20it=20through=
=20ATT_MTU=20exchange.=0D=0A=C2=A0=0D=0A>>=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0if=C2=A0(=21attrib)=C2=A0=7B=0D=0A>>=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0error(=22Unable=C2=A0to=C2=A0create=C2=A0new=C2=A0GAtt=
rib=C2=A0instance=22);=0D=0A>>=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0return=C2=A0=
false;=0D=0A>>=C2=A0diff=C2=A0--git=C2=A0a/src/shared/gatt-helpers.c=C2=A0b=
/src/shared/gatt-helpers.c=0D=0A>>=C2=A0index=C2=A06b39bb1..8ec65be=C2=A010=
0644=0D=0A>>=C2=A0---=C2=A0a/src/shared/gatt-helpers.c=0D=0A>>=C2=A0+++=C2=
=A0b/src/shared/gatt-helpers.c=0D=0A>>=C2=A0=40=40=C2=A0-517,6=C2=A0+517,7=
=C2=A0=40=40=C2=A0static=C2=A0void=C2=A0mtu_cb(uint8_t=C2=A0opcode,=C2=A0co=
nst=C2=A0void=C2=A0*pdu,=0D=0Auint16_t=C2=A0length,=0D=0A>>=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if=C2=A0(opcode=C2=A0=3D=3D=C2=
=A0BT_ATT_OP_ERROR_RSP)=C2=A0=7B=0D=0A>>=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
success=C2=A0=3D=C2=A0false;=0D=0A>>=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0att_ec=
ode=C2=A0=3D=C2=A0process_error(pdu,=C2=A0length);=0D=0A>>=C2=A0+=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0bt_att_set_mtu(op->att,=C2=A0BT_ATT_DEFAULT_LE_MTU);=0D=0A=C2=A0=0D=
=0A>I=C2=A0not=C2=A0following=C2=A0this=C2=A0one,=C2=A0we=C2=A0only=C2=A0re=
ally=C2=A0use=C2=A0bt_att_set_mtu=C2=A0if=C2=A0the=C2=A0command=0D=0A>succe=
eds=C2=A0so=C2=A0this=C2=A0should=C2=A0not=C2=A0be=C2=A0necessary,=C2=A0or=
=C2=A0this=C2=A0is=C2=A0due=C2=A0Exchange=C2=A0being=0D=0A>used=C2=A0multip=
le=C2=A0times?=C2=A0I=C2=A0recall=C2=A0the=C2=A0spec=C2=A0not=C2=A0allowing=
=C2=A0that=C2=A0to=C2=A0happen,=C2=A0and=C2=A0if=0D=0A>does=C2=A0Im=C2=A0no=
t=C2=A0sure=C2=A0why=C2=A0we=C2=A0would=C2=A0have=C2=A0to=C2=A0return=C2=A0=
to=C2=A0the=C2=A0default=C2=A0instead=C2=A0of=C2=A0the=0D=0A>value=C2=A0pre=
viously=C2=A0set=C2=A0in=C2=A0case=C2=A0the=C2=A0of=C2=A0error=C2=A0respons=
e.=0D=0A=0D=0AThis=20change=20was=20in=20connection=20with=20my=20proposed=
=20change.=0D=0Ai.e=20We=20set=20the=20MTU=20to=20L2CAP-MTU=20(for=20both=
=20LE-ATT=20and=20BREDR-ATT)=0D=0AIn=20case=20of=20LE-ATT,=20if=20remote=20=
does=20not=20support=20the=20ATT-MTU=20exchange=20procedure,=0D=0Athen=20it=
=20will=20return=20=22BT_ATT_OP_ERROR_RSP=22=20and=20we=20will=20fall=20bac=
k=20to=20default-MTU=0D=0A=0D=0AWith=20CID=20check=20that=20you=20advised,=
=20this=20change=20won't=20be=20necessary.=20=0D=0A=C2=A0=0D=0A=C2=A0=0D=0A

Re: Re: [PATCH] src/device.c : Fix BREDR-ATT MTU issue

[Luiz Augusto von Dentz]

Hi Nagaraj,
On Wed, May 2, 2018 at 3:18 PM Nagaraj D R <nagaraj.dr@samsung.com> wrote:

> Hello Luiz

> Sender : Luiz Augusto von Dentz <luiz.dentz@gmail.com>
> >Hi Nagaraj,
> >On Wed, May 2, 2018 at 11:40 AM Nagaraj D R <nagaraj.dr@samsung.com>
wrote:

> >> For BREDR-ATT, according to spec, ATT MTU is same has
> >> L2CAP configured MTU on which ATT is running. So, set the MTU to
> >> L2CAP configuration and for LE-ATT adjust the ATT MTU based on
> >> EXCHANGE_MTU request and response.
> >> ---
> >>   src/device.c              | 2 +-
> >>   src/shared/gatt-helpers.c | 1 +
> >>   2 files changed, 2 insertions(+), 1 deletion(-)

> >> diff --git a/src/device.c b/src/device.c
> >> index f693b70..cf4c8df 100644
> >> --- a/src/device.c
> >> +++ b/src/device.c
> >> @@ -4922,7 +4922,7 @@ bool device_attach_att(struct btd_device *dev,
> GIOChannel *io)
> >>          }

> >>          dev->att_mtu = MIN(mtu, BT_ATT_MAX_LE_MTU);
> >> -       attrib = g_attrib_new(io, BT_ATT_DEFAULT_LE_MTU, false);
> >> +       attrib = g_attrib_new(io, dev->att_mtu, false);

> >This would actually undo the following patch:

> >     src/device: Use BT_ATT_DEFAULT_LE_MTU as default MTU

> >   Use the default MTU until an MTU exchange has taken place and
> >     something else has been negotiated. If either side does not support
> >    MTU exchange, the connection shall continue to use this default
> >     value instead of the device maximum which was the previous behavior.

> >We should probably check if the cid is for LE or not and reset mtu to
> >BT_ATT_DEFAULT_LE_MTU that way att_mtu should be valid for either LE or
> >BR/EDR.

> I agree, we should check for CID.
> If it is BREDR-ATT connection only then consider L2CAP MTU otherwise
> set the MTU to min value and set it through ATT_MTU exchange.

> >>          if (!attrib) {
> >>                  error("Unable to create new GAttrib instance");
> >>                  return false;
> >> diff --git a/src/shared/gatt-helpers.c b/src/shared/gatt-helpers.c
> >> index 6b39bb1..8ec65be 100644
> >> --- a/src/shared/gatt-helpers.c
> >> +++ b/src/shared/gatt-helpers.c
> >> @@ -517,6 +517,7 @@ static void mtu_cb(uint8_t opcode, const void *pdu,
> uint16_t length,
> >>          if (opcode == BT_ATT_OP_ERROR_RSP) {
> >>                  success = false;
> >>                  att_ecode = process_error(pdu, length);
> >> +               bt_att_set_mtu(op->att, BT_ATT_DEFAULT_LE_MTU);

> >I not following this one, we only really use bt_att_set_mtu if the
command
> >succeeds so this should not be necessary, or this is due Exchange being
> >used multiple times? I recall the spec not allowing that to happen, and
if
> >does Im not sure why we would have to return to the default instead of
the
> >value previously set in case the of error response.

> This change was in connection with my proposed change.
> i.e We set the MTU to L2CAP-MTU (for both LE-ATT and BREDR-ATT)
> In case of LE-ATT, if remote does not support the ATT-MTU exchange
procedure,
> then it will return "BT_ATT_OP_ERROR_RSP" and we will fall back to
default-MTU

> With CID check that you advised, this change won't be necessary.

Ok now it is clear, so I suppose you will be sending a v2 shortly?





-- 
Luiz Augusto von Dentz