[PATCH] Bluetooth: MTU configuration for LE links

[PATCH] Bluetooth: MTU configuration for LE links

[Anderson Briglia]

This patch implements MTU configuration for LE links. The previous
implementation was setting the MTU as a fixed value.

Signed-off-by: Anderson Briglia <anderson.briglia@openbossa.org>
Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@openbossa.org>
---
 net/bluetooth/l2cap_core.c |    6 +++++-
 net/bluetooth/l2cap_sock.c |   18 ++++++++++++++++--
 2 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index be6c5aa..4a8b388 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -177,7 +177,11 @@ static void __l2cap_chan_add(struct l2cap_conn *conn, struct l2cap_chan *chan)
 	if (sk->sk_type == SOCK_SEQPACKET || sk->sk_type == SOCK_STREAM) {
 		if (conn->hcon->type == LE_LINK) {
 			/* LE connection */
-			l2cap_pi(sk)->omtu = L2CAP_LE_DEFAULT_MTU;
+			if (l2cap_pi(sk)->imtu < L2CAP_LE_DEFAULT_MTU)
+				l2cap_pi(sk)->imtu = L2CAP_LE_DEFAULT_MTU;
+			if (l2cap_pi(sk)->omtu < L2CAP_LE_DEFAULT_MTU)
+				l2cap_pi(sk)->omtu = L2CAP_LE_DEFAULT_MTU;
+
 			l2cap_pi(sk)->scid = L2CAP_CID_LE_DATA;
 			l2cap_pi(sk)->dcid = L2CAP_CID_LE_DATA;
 		} else {
diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index 0059dda..b5796fd 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -530,16 +530,18 @@ static int l2cap_sock_setsockopt_old(struct socket *sock, int optname, char __us
 {
 	struct sock *sk = sock->sk;
 	struct l2cap_options opts;
-	int len, err = 0;
+	int len, le_sock, err = 0;
 	u32 opt;
 
 	BT_DBG("sk %p", sk);
 
 	lock_sock(sk);
 
+	le_sock = l2cap_pi(sk)->scid == L2CAP_CID_LE_DATA;
+
 	switch (optname) {
 	case L2CAP_OPTIONS:
-		if (sk->sk_state == BT_CONNECTED) {
+		if (sk->sk_state == BT_CONNECTED && !le_sock) {
 			err = -EINVAL;
 			break;
 		}
@@ -558,6 +560,18 @@ static int l2cap_sock_setsockopt_old(struct socket *sock, int optname, char __us
 			break;
 		}
 
+		if ((opts.imtu || opts.omtu) && le_sock &&
+				(sk->sk_state == BT_CONNECTED)) {
+			if (opts.imtu >= L2CAP_LE_DEFAULT_MTU)
+				l2cap_pi(sk)->imtu = opts.imtu;
+			if (opts.omtu >= L2CAP_LE_DEFAULT_MTU)
+				l2cap_pi(sk)->omtu = opts.omtu;
+			if (opts.imtu < L2CAP_LE_DEFAULT_MTU ||
+					opts.omtu < L2CAP_LE_DEFAULT_MTU)
+				err = -EINVAL;
+			break;
+		}
+
 		if (opts.txwin_size > L2CAP_DEFAULT_TX_WINDOW) {
 			err = -EINVAL;
 			break;
-- 
1.7.1

Re: [PATCH] Bluetooth: MTU configuration for LE links

[Marcel Holtmann]

Hi Anderson,

> This patch implements MTU configuration for LE links. The previous
> implementation was setting the MTU as a fixed value.
> 
> Signed-off-by: Anderson Briglia <anderson.briglia@openbossa.org>
> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@openbossa.org>
> ---
>  net/bluetooth/l2cap_core.c |    6 +++++-
>  net/bluetooth/l2cap_sock.c |   18 ++++++++++++++++--
>  2 files changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index be6c5aa..4a8b388 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -177,7 +177,11 @@ static void __l2cap_chan_add(struct l2cap_conn *conn, struct l2cap_chan *chan)
>  	if (sk->sk_type == SOCK_SEQPACKET || sk->sk_type == SOCK_STREAM) {
>  		if (conn->hcon->type == LE_LINK) {
>  			/* LE connection */
> -			l2cap_pi(sk)->omtu = L2CAP_LE_DEFAULT_MTU;
> +			if (l2cap_pi(sk)->imtu < L2CAP_LE_DEFAULT_MTU)
> +				l2cap_pi(sk)->imtu = L2CAP_LE_DEFAULT_MTU;
> +			if (l2cap_pi(sk)->omtu < L2CAP_LE_DEFAULT_MTU)
> +				l2cap_pi(sk)->omtu = L2CAP_LE_DEFAULT_MTU;
> +
>  			l2cap_pi(sk)->scid = L2CAP_CID_LE_DATA;
>  			l2cap_pi(sk)->dcid = L2CAP_CID_LE_DATA;
>  		} else {
> diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
> index 0059dda..b5796fd 100644
> --- a/net/bluetooth/l2cap_sock.c
> +++ b/net/bluetooth/l2cap_sock.c
> @@ -530,16 +530,18 @@ static int l2cap_sock_setsockopt_old(struct socket *sock, int optname, char __us
>  {
>  	struct sock *sk = sock->sk;
>  	struct l2cap_options opts;
> -	int len, err = 0;
> +	int len, le_sock, err = 0;
>  	u32 opt;
>  
>  	BT_DBG("sk %p", sk);
>  
>  	lock_sock(sk);
>  
> +	le_sock = l2cap_pi(sk)->scid == L2CAP_CID_LE_DATA;
> +
>  	switch (optname) {
>  	case L2CAP_OPTIONS:
> -		if (sk->sk_state == BT_CONNECTED) {
> +		if (sk->sk_state == BT_CONNECTED && !le_sock) {
>  			err = -EINVAL;
>  			break;
>  		}
> @@ -558,6 +560,18 @@ static int l2cap_sock_setsockopt_old(struct socket *sock, int optname, char __us
>  			break;
>  		}
>  
> +		if ((opts.imtu || opts.omtu) && le_sock &&
> +				(sk->sk_state == BT_CONNECTED)) {
> +			if (opts.imtu >= L2CAP_LE_DEFAULT_MTU)
> +				l2cap_pi(sk)->imtu = opts.imtu;
> +			if (opts.omtu >= L2CAP_LE_DEFAULT_MTU)
> +				l2cap_pi(sk)->omtu = opts.omtu;
> +			if (opts.imtu < L2CAP_LE_DEFAULT_MTU ||
> +					opts.omtu < L2CAP_LE_DEFAULT_MTU)
> +				err = -EINVAL;
> +			break;
> +		}
> +

so how do you expect this to work exactly? In BR/EDR L2CAP you set the
MTU details before calling connect(). With LE you expect to be connected
and then tell the kernel what the limits actually are?

This sounds highly convoluted. And especially hijacking the existing
command for it seems wrong. Using l2cap_sock_setsockopt_old() might have
given it away that it is a bad idea to re-use that socket option for a
new technology ;)

So the real fact here is that the MTU handling/negotiation for BR/EDR
and LE are different. Nothing is going to change this. So this should be
also different from a socket option point of view.

Regards

Marcel

Re: [PATCH] Bluetooth: MTU configuration for LE links

[Anderson Lizardo]

Hi Marcel,

On Tue, Apr 12, 2011 at 12:06 AM, Marcel Holtmann <marcel@holtmann.org> wrote:
> so how do you expect this to work exactly? In BR/EDR L2CAP you set the
> MTU details before calling connect(). With LE you expect to be connected
> and then tell the kernel what the limits actually are?

Yes. For LE, the MTU negotiation happens over the connected link
(using specific ATT commands).

> This sounds highly convoluted. And especially hijacking the existing
> command for it seems wrong. Using l2cap_sock_setsockopt_old() might have
> given it away that it is a bad idea to re-use that socket option for a
> new technology ;)
>
> So the real fact here is that the MTU handling/negotiation for BR/EDR
> and LE are different. Nothing is going to change this. So this should be
> also different from a socket option point of view.

One thing to consider is that there are a couple of MTU checks along
the L2CAP code. The issue which originated this patch is code such as:

                /* Check outgoing MTU */
                if (len > pi->omtu) {
                        err = -EMSGSIZE;
                        goto done;
                }

We understood that just letting omtu/imtu values on kernel reflect the
current connection MTU settings was the cleanest solution. What do you
propose? Bypassing these checks for LE?

Regards,
-- 
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil

Re: [PATCH] Bluetooth: MTU configuration for LE links

[Marcel Holtmann]

Hi Anderson,

> > so how do you expect this to work exactly? In BR/EDR L2CAP you set the
> > MTU details before calling connect(). With LE you expect to be connected
> > and then tell the kernel what the limits actually are?
> 
> Yes. For LE, the MTU negotiation happens over the connected link
> (using specific ATT commands).
> 
> > This sounds highly convoluted. And especially hijacking the existing
> > command for it seems wrong. Using l2cap_sock_setsockopt_old() might have
> > given it away that it is a bad idea to re-use that socket option for a
> > new technology ;)
> >
> > So the real fact here is that the MTU handling/negotiation for BR/EDR
> > and LE are different. Nothing is going to change this. So this should be
> > also different from a socket option point of view.
> 
> One thing to consider is that there are a couple of MTU checks along
> the L2CAP code. The issue which originated this patch is code such as:
> 
>                 /* Check outgoing MTU */
>                 if (len > pi->omtu) {
>                         err = -EMSGSIZE;
>                         goto done;
>                 }
> 
> We understood that just letting omtu/imtu values on kernel reflect the
> current connection MTU settings was the cleanest solution. What do you
> propose? Bypassing these checks for LE?

this does not look clean to me. And we might have an internal MTU
variable as part of L2CAP, but the way how it gets set and thus its
semantic differs clearly between BR/EDR and LE. So shoehorning an
existing socket option this is clearly a bad idea.

Regards

Marcel

Re: [PATCH] Bluetooth: MTU configuration for LE links

[Anderson Lizardo]

Hi Marcel,

On Tue, Apr 12, 2011 at 6:59 AM, Marcel Holtmann <marcel@holtmann.org> wrot=
e:
>> One thing to consider is that there are a couple of MTU checks along
>> the L2CAP code. The issue which originated this patch is code such as:
>>
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* Check outgoing MTU */
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (len > pi->omtu) {
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 err =3D -EMSGSIZE;
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto done;
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
>>
>> We understood that just letting omtu/imtu values on kernel reflect the
>> current connection MTU settings was the cleanest solution. What do you
>> propose? Bypassing these checks for LE?
>
> this does not look clean to me. And we might have an internal MTU
> variable as part of L2CAP, but the way how it gets set and thus its
> semantic differs clearly between BR/EDR and LE. So shoehorning an
> existing socket option this is clearly a bad idea.

Sure. What to do then if:

1) LE links have MTU (omtu specifically) hard-coded to 23 on kernel.
2) the kernel rejects sending data longer than omtu.

(this is the current situation)

This clearly needs some fix on kernel side, otherwise we can't send
PDUs longer than the LE default MTU (23), *even* if the peer device
supports a bigger MTU.

Suggestions are welcome regarding how to best approach this, without
affecting current BR/EDR MTU semantics.

Regards,
--=20
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil

Re: [PATCH] Bluetooth: MTU configuration for LE links

[Marcel Holtmann]

Hi Anderson,

> >> One thing to consider is that there are a couple of MTU checks along
> >> the L2CAP code. The issue which originated this patch is code such as:
> >>
> >>                 /* Check outgoing MTU */
> >>                 if (len > pi->omtu) {
> >>                         err = -EMSGSIZE;
> >>                         goto done;
> >>                 }
> >>
> >> We understood that just letting omtu/imtu values on kernel reflect the
> >> current connection MTU settings was the cleanest solution. What do you
> >> propose? Bypassing these checks for LE?
> >
> > this does not look clean to me. And we might have an internal MTU
> > variable as part of L2CAP, but the way how it gets set and thus its
> > semantic differs clearly between BR/EDR and LE. So shoehorning an
> > existing socket option this is clearly a bad idea.
> 
> Sure. What to do then if:
> 
> 1) LE links have MTU (omtu specifically) hard-coded to 23 on kernel.
> 2) the kernel rejects sending data longer than omtu.
> 
> (this is the current situation)
> 
> This clearly needs some fix on kernel side, otherwise we can't send
> PDUs longer than the LE default MTU (23), *even* if the peer device
> supports a bigger MTU.

I agree with that. Userspace needs to be able to change the kernel MTU
if a different one gets negotiated. That is not the problem. The problem
is trying to shoehorn this into an existing (and already declared
legacy) socket option.

> Suggestions are welcome regarding how to best approach this, without
> affecting current BR/EDR MTU semantics.

We need to have a socket option that allows us the dynamically change
the MTU of a L2CAP link. And right now this option will only be valid if
the link is actually an LE link.

Regards

Marcel

Re: [PATCH] Bluetooth: MTU configuration for LE links

[Anderson Briglia]

Hi Marcel,

On Wed, Apr 13, 2011 at 7:20 PM, Marcel Holtmann <marcel@holtmann.org> wrot=
e:
> Hi Anderson,
>
>> >> One thing to consider is that there are a couple of MTU checks along
>> >> the L2CAP code. The issue which originated this patch is code such as=
:
>> >>
>> >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* Check outgoing MTU */
>> >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (len > pi->omtu) {
>> >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 err =3D -EMSGSIZE;
>> >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto done;
>> >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
>> >>
>> >> We understood that just letting omtu/imtu values on kernel reflect th=
e
>> >> current connection MTU settings was the cleanest solution. What do yo=
u
>> >> propose? Bypassing these checks for LE?
>> >
>> > this does not look clean to me. And we might have an internal MTU
>> > variable as part of L2CAP, but the way how it gets set and thus its
>> > semantic differs clearly between BR/EDR and LE. So shoehorning an
>> > existing socket option this is clearly a bad idea.
>>
>> Sure. What to do then if:
>>
>> 1) LE links have MTU (omtu specifically) hard-coded to 23 on kernel.
>> 2) the kernel rejects sending data longer than omtu.
>>
>> (this is the current situation)
>>
>> This clearly needs some fix on kernel side, otherwise we can't send
>> PDUs longer than the LE default MTU (23), *even* if the peer device
>> supports a bigger MTU.
>
> I agree with that. Userspace needs to be able to change the kernel MTU
> if a different one gets negotiated. That is not the problem. The problem
> is trying to shoehorn this into an existing (and already declared
> legacy) socket option.
>
>> Suggestions are welcome regarding how to best approach this, without
>> affecting current BR/EDR MTU semantics.
>
> We need to have a socket option that allows us the dynamically change
> the MTU of a L2CAP link. And right now this option will only be valid if
> the link is actually an LE link.

Ok, so we have some options IMHO:

a) Do not add a new option in struct l2cap_options because it will
still be hijacking.

b) Add a new option for LE OMTU in struct l2cap_conninfo. Is it valid?

c) Add a new struct (l2cap_le_options ?) and add a omtu field there.

The problem in c) is that we will have to implement significant
modifications in userspace and the code should be redundant: we will
have two options to represent the same value.

Regards,

Anderson Briglia

>
> Regards
>
> Marcel
>
>
>



--=20
INdT - Instituto Nokia de tecnologia
+55 2126 1122
http://techblog.briglia.net

Re: [PATCH] Bluetooth: MTU configuration for LE links

[Anderson Briglia]

Hi guys,

On Tue, Apr 19, 2011 at 5:50 PM, Anderson Briglia
<anderson.briglia@openbossa.org> wrote:
> Hi Marcel,
>
> On Wed, Apr 13, 2011 at 7:20 PM, Marcel Holtmann <marcel@holtmann.org> wr=
ote:
>> Hi Anderson,
>>
>>> >> One thing to consider is that there are a couple of MTU checks along
>>> >> the L2CAP code. The issue which originated this patch is code such a=
s:
>>> >>
>>> >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* Check outgoing MTU */
>>> >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (len > pi->omtu) {
>>> >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 err =3D -EMSGSIZE;
>>> >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto done;
>>> >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
>>> >>
>>> >> We understood that just letting omtu/imtu values on kernel reflect t=
he
>>> >> current connection MTU settings was the cleanest solution. What do y=
ou
>>> >> propose? Bypassing these checks for LE?
>>> >
>>> > this does not look clean to me. And we might have an internal MTU
>>> > variable as part of L2CAP, but the way how it gets set and thus its
>>> > semantic differs clearly between BR/EDR and LE. So shoehorning an
>>> > existing socket option this is clearly a bad idea.
>>>
>>> Sure. What to do then if:
>>>
>>> 1) LE links have MTU (omtu specifically) hard-coded to 23 on kernel.
>>> 2) the kernel rejects sending data longer than omtu.
>>>
>>> (this is the current situation)
>>>
>>> This clearly needs some fix on kernel side, otherwise we can't send
>>> PDUs longer than the LE default MTU (23), *even* if the peer device
>>> supports a bigger MTU.
>>
>> I agree with that. Userspace needs to be able to change the kernel MTU
>> if a different one gets negotiated. That is not the problem. The problem
>> is trying to shoehorn this into an existing (and already declared
>> legacy) socket option.
>>
>>> Suggestions are welcome regarding how to best approach this, without
>>> affecting current BR/EDR MTU semantics.
>>
>> We need to have a socket option that allows us the dynamically change
>> the MTU of a L2CAP link. And right now this option will only be valid if
>> the link is actually an LE link.
>
> Ok, so we have some options IMHO:
>
> a) Do not add a new option in struct l2cap_options because it will
> still be hijacking.
>
> b) Add a new option for LE OMTU in struct l2cap_conninfo. Is it valid?
>
> c) Add a new struct (l2cap_le_options ?) and add a omtu field there.
>
> The problem in c) is that we will have to implement significant
> modifications in userspace and the code should be redundant: we will
> have two options to represent the same value.

Some comments about this a) and b) should be great. LE link
configuration is dependent of this MTU exchange to work properly.

>
> Regards,
>
> Anderson Briglia
>
>>
>> Regards
>>
>> Marcel
>>
>>
>>
>
>
>
> --
> INdT - Instituto Nokia de tecnologia
> +55 2126 1122
> http://techblog.briglia.net
>



--=20
INdT - Instituto Nokia de tecnologia
+55 2126 1122
http://techblog.briglia.net