New L2CAP ERTM state machine

This change affects data structures storing ERTM state and control fields, and adds new definitions for states and events. An l2cap_seq_list structure is added for tracking ERTM sequence numbers without repeated memory allocations. Control fields are carried in the bt_skb_cb struct rather than constantly doing shift and mask operations.

Signed-off-by Mat Martineau <[email protected]>

---
 include/net/bluetooth/bluetooth.h |   14 ++-
 include/net/bluetooth/l2cap.h     |  260 +++++++++----------------------------
 2 files changed, 73 insertions(+), 201 deletions(-)

Hi Mat,

It is better to have normal patches for a better review. I think that we can minimize amount of changes by redefining defines when they cannot be used.

I also think think that patches shall be logically split like:
- change control field handling
- working with FCS, etc which do not affect state machine
- adding states

Also check some comments below: (I copied some code from the link you sen=

Quoted 7 lines

This change affects data structures storing ERTM state and control fields, and adds new definitions for states and events. =A0An l2cap_seq_list structure is added for tracking ERTM sequence numbers without repeated memory allocations. =A0Control fields are carried in the bt_skb_cb struct rather than constantly doing shift and mask operations.

Quoted 4 lines

Signed-off-by Mat Martineau <[email protected]>

---
=A0include/net/bluetooth/bluetooth.h | =A0 14 ++- =A0include/net/bluetooth/l2cap.h =A0 =A0 | =A0260 +++++++++-------------=

Quoted 1 line

=A02 files changed, 73 insertions(+), 201 deletions(-)

...

include/net/bluetooth/l2cap.h static inline bool l2cap_clear_timer(struct l2cap_chan *chan, · line 684

Quoted 11 lines

include/net/bluetooth/l2cap.h +0 -11

-static inline int l2cap_tx_window_full(struct l2cap_chan *ch)
-{
- =A0 =A0 int sub;
-
- =A0 =A0 sub =3D (ch->next_tx_seq - ch->expected_ack_seq) % 64;
-
- =A0 =A0 if (sub < 0)
- =A0 =A0 =A0 =A0 =A0 =A0 sub +=3D 64;
-
- =A0 =A0 return sub =3D=3D ch->remote_tx_win;
-}

BTW: was it already changed? What is the status with Luiz's patch?

...

Review

Quoted 7 lines

(quoted diff) +0 -7

-static inline __u32 __get_control(struct l2cap_chan *chan, void *p)
-{
- =A0 =A0 if (test_bit(FLAG_EXT_CTRL, &chan->flags))
- =A0 =A0 =A0 =A0 =A0 =A0 return get_unaligned_le32(p);
- =A0 =A0 else
- =A0 =A0 =A0 =A0 =A0 =A0 return get_unaligned_le16(p);
-}

Cannot it still be used?

+ =A0 =A0 =A0 if (test_bit(FLAG_EXT_CTRL, &chan->flags)) {
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 __get_extended_control(get_unaligned_le32(s=

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 skb_pull(skb, L2CAP_EXT_CTRL_SIZE);
+ =A0 =A0 =A0 } else {
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 __get_enhanced_control(get_unaligned_le16(s=

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 skb_pull(skb, L2CAP_ENH_CTRL_SIZE);
+ =A0 =A0 =A0 }

- =A0 =A0 =A0 control =3D __get_control(chan, skb->data);
- =A0 =A0 =A0 skb_pull(skb, __ctrl_size(chan));

...

Quoted 1 line

-static inline void __put_control(struct l2cap_chan *chan, __u32 control=

Quoted 1 line

- =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =

Review

Quoted 6 lines

(quoted diff) +0 -6

-{
- =A0 =A0 if (test_bit(FLAG_EXT_CTRL, &chan->flags))
- =A0 =A0 =A0 =A0 =A0 =A0 return put_unaligned_le32(control, p);
- =A0 =A0 else
- =A0 =A0 =A0 =A0 =A0 =A0 return put_unaligned_le16(control, p);
-}

Can it be used in the code below:

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (test_bit(FLAG_EXT_CTRL, &chan->flags)) =

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 put_unaligned_le32(__pack_e=

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 } else {
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 put_unaligned_le16(__pack_e=

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 }

and for example here:

- =A0 =A0 =A0 __put_control(chan, control, skb_put(skb, __ctrl_size(chan)=

+ =A0 =A0 =A0 /* Control header is populated later */
+ =A0 =A0 =A0 if (test_bit(FLAG_EXT_CTRL, &chan->flags))
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 put_unaligned_le32(0, skb_put(skb, 4));
+ =A0 =A0 =A0 else
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 put_unaligned_le16(0, skb_put(skb, 2));

include/net/bluetooth/l2cap.h static inline bool l2cap_clear_timer(struct l2cap_chan *chan, · line 841

Quoted 8 lines

include/net/bluetooth/l2cap.h +0 -8

-
-static inline __u8 __ctrl_size(struct l2cap_chan *chan)
-{
- =A0 =A0 if (test_bit(FLAG_EXT_CTRL, &chan->flags))
- =A0 =A0 =A0 =A0 =A0 =A0 return L2CAP_EXT_HDR_SIZE - L2CAP_HDR_SIZE;
- =A0 =A0 else
- =A0 =A0 =A0 =A0 =A0 =A0 return L2CAP_ENH_HDR_SIZE - L2CAP_HDR_SIZE;
-}

Hi Andrei,

Quoted 3 lines · Andrei Emeltchenko

Hi Mat,

It is better to have normal patches for a better review.

Quoted 2 lines

I think that we can minimize amount of changes by redefining defines when they cannot be used.

Quoted 5 lines

I also think think that patches shall be logically split like:
- change control field handling
- working with FCS, etc which do not affect state machine
- adding states

Quoted 19 lines

Also check some comments below: (I copied some code from the link you sent)

Quoted 12 lines · Mat Martineau

This change affects data structures storing ERTM state and control fields, and adds new definitions for states and events. An l2cap_seq_list structure is added for tracking ERTM sequence numbers without repeated memory allocations. Control fields are carried in the bt_skb_cb struct rather than constantly doing shift and mask operations.

Signed-off-by Mat Martineau<[email protected]>

---
  include/net/bluetooth/bluetooth.h |   14 ++-
  include/net/bluetooth/l2cap.h     |  260 +++++++++----------------------------
  2 files changed, 73 insertions(+), 201 deletions(-)

...

Quoted 5 lines

Quoted 5 lines

include/net/bluetooth/l2cap.h +0 -5

-static inline int l2cap_tx_window_full(struct l2cap_chan *ch)
-{
-     int sub;
-
-     sub = (ch->next_tx_seq - ch->expected_ack_seq) % 64;

Quoted 8 lines

include/net/bluetooth/l2cap.h static inline bool l2cap_clear_timer(struct l2cap_chan *chan, · line 684

Quoted 6 lines

include/net/bluetooth/l2cap.h +0 -6

-
-     if (sub<  0)
-             sub += 64;
-
-     return sub == ch->remote_tx_win;
-}

BTW: was it already changed? What is the status with Luiz's patch?

Quoted 12 lines

...

include/net/bluetooth/l2cap.h static inline bool l2cap_clear_timer(struct l2cap_chan *chan, · line 824

Quoted 7 lines

include/net/bluetooth/l2cap.h +0 -7

-static inline __u32 __get_control(struct l2cap_chan *chan, void *p)
-{
-     if (test_bit(FLAG_EXT_CTRL,&chan->flags))
-             return get_unaligned_le32(p);
-     else
-             return get_unaligned_le16(p);
-}

Cannot it still be used?

Quoted 27 lines

(quoted diff) +9 -2

+       if (test_bit(FLAG_EXT_CTRL,&chan->flags)) {
+               __get_extended_control(get_unaligned_le32(skb->data),
+                                      control);
+               skb_pull(skb, L2CAP_EXT_CTRL_SIZE);
+       } else {
+               __get_enhanced_control(get_unaligned_le16(skb->data),
+                                      control);
+               skb_pull(skb, L2CAP_ENH_CTRL_SIZE);
+       }
 
-       control = __get_control(chan, skb->data);
-       skb_pull(skb, __ctrl_size(chan));
 
...

include/net/bluetooth/l2cap.h static inline bool l2cap_clear_timer(struct l2cap_chan *chan, · line 833

Quoted 9 lines

include/net/bluetooth/l2cap.h +0 -8

-static inline void __put_control(struct l2cap_chan *chan, __u32 control,
-                                                             void *p)
-{
-     if (test_bit(FLAG_EXT_CTRL,&chan->flags))
-             return put_unaligned_le32(control, p);
-     else
-             return put_unaligned_le16(control, p);
-}

Can it be used in the code below:

Review

Quoted 8 lines

(quoted diff) +7 -0

+               if (test_bit(FLAG_EXT_CTRL,&chan->flags)) {
+                       put_unaligned_le32(__pack_extended_control(control),
+                                       skb->data + L2CAP_HDR_SIZE);
+               } else {
+                       put_unaligned_le16(__pack_enhanced_control(control),
+                                       skb->data + L2CAP_HDR_SIZE);
+               }

Review

Quoted 8 lines

and for example here:

(quoted diff) +5 -1

-       __put_control(chan, control, skb_put(skb, __ctrl_size(chan)));
+       /* Control header is populated later */
+       if (test_bit(FLAG_EXT_CTRL,&chan->flags))
+               put_unaligned_le32(0, skb_put(skb, 4));
+       else
+               put_unaligned_le16(0, skb_put(skb, 2));

Quoted 9 lines

include/net/bluetooth/l2cap.h static inline bool l2cap_clear_timer(struct l2cap_chan *chan, · line 841

Quoted 9 lines

include/net/bluetooth/l2cap.h +0 -8

-
-static inline __u8 __ctrl_size(struct l2cap_chan *chan)
-{
-     if (test_bit(FLAG_EXT_CTRL,&chan->flags))
-             return L2CAP_EXT_HDR_SIZE - L2CAP_HDR_SIZE;
-     else
-             return L2CAP_ENH_HDR_SIZE - L2CAP_HDR_SIZE;
-}

Do we have that many places with this test in Mat's code? If not, then we might not need to bother having all of these helpers, I think. And if we add them, I do think it makes sense to add them to l2cap_core.c than in l2cap.h, right?

Andrei and Ulisses -

Quoted 7 lines · Ulisses Furquim

Hi Andrei,

Quoted 2 lines · On Fri, Feb 24, 2012 at 7:48 AM, Andrei Emeltchenko…

Hi Mat,

Quoted 3 lines

Quoted 3 lines

and for example here:

- =A0 =A0 =A0 __put_control(chan, control, skb_put(skb, __ctrl_size(cha=

Quoted 6 lines

Quoted 6 lines

+ =A0 =A0 =A0 /* Control header is populated later */
+ =A0 =A0 =A0 if (test_bit(FLAG_EXT_CTRL,&chan->flags))

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 put_unaligned_le32(0, skb_put(skb, 4));
+ =A0 =A0 =A0 else
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 put_unaligned_le16(0, skb_put(skb, 2));

This is the only place the macro would drop in cleanly - but it seems lik=

macro should be used in more than one place unless it's hiding some reall=

distracting syntax. =A0In this case, I think it's clearer to see what's g=

on.

And - I forgot to fix some magic numbers in those skb_puts.

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

include/net/bluetooth/l2cap.h +0 -4

-
-static inline __u8 __ctrl_size(struct l2cap_chan *chan)
-{
- =A0 =A0 if (test_bit(FLAG_EXT_CTRL,&chan->flags))

Quoted 9 lines

Quoted 4 lines

Quoted 4 lines

- =A0 =A0 =A0 =A0 =A0 =A0 return L2CAP_EXT_HDR_SIZE - L2CAP_HDR_SIZE;
- =A0 =A0 else
- =A0 =A0 =A0 =A0 =A0 =A0 return L2CAP_ENH_HDR_SIZE - L2CAP_HDR_SIZE; -}

Do we have that many places with this test in Mat's code? If not, then we might not need to bother having all of these helpers, I think. And if we add them, I do think it makes sense to add them to l2cap_core.c than in l2cap.h, right?

Not too many places with the FLAG_EXT_CTRL test, with use of struct bt_control in most of the code. =A0I would rather have helpers in l2cap_c=

if they're needed.

Quoted 2 lines

Quoted 2 lines

I think that we can minimize amount of changes by redefining defines when they cannot be used.

Do you mean that it would be better to modify existing macros rather than remove them?

Quoted 4 lines

Quoted 4 lines

I also think think that patches shall be logically split like:
- change control field handling
- working with FCS, etc which do not affect state machine
- adding states

Sounds like a good way to divide things up.

Quoted 11 lines

Quoted 11 lines · Mat Martineau

include/net/bluetooth/l2cap.h static inline bool l2cap_clear_timer(struct l2cap_chan *chan, · line 824

Quoted 8 lines

This change affects data structures storing ERTM state and control

include/net/bluetooth/l2cap.h +0 -7

-static inline __u32 __get_control(struct l2cap_chan *chan, void *p)
-{
-     if (test_bit(FLAG_EXT_CTRL,&chan->flags))
-             return get_unaligned_le32(p);
-     else
-             return get_unaligned_le16(p);
-}

Cannot it still be used?

It's not needed. The control header is only parsed in one place now at the beginning of l2cap_ertm_data_rcv(), and this macro isn't a good fit for the code there.

Quoted 5 lines

Review

Quoted 5 lines

and for example here:

(quoted diff) +2 -1

-       __put_control(chan, control, skb_put(skb, __ctrl_size(chan)));
+       /* Control header is populated later */
+       if (test_bit(FLAG_EXT_CTRL,&chan->flags))

Quoted 3 lines

Review

Quoted 3 lines

(quoted diff) +3 -0

+               put_unaligned_le32(0, skb_put(skb, 4));
+       else
+               put_unaligned_le16(0, skb_put(skb, 2));

This is the only place the macro would drop in cleanly - but it seems like a macro should be used in more than one place unless it's hiding some really distracting syntax. In this case, I think it's clearer to see what's going on.

And - I forgot to fix some magic numbers in those skb_puts.

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

include/net/bluetooth/l2cap.h +0 -4

-static inline __u8 __ctrl_size(struct l2cap_chan *chan)
-{
-     if (test_bit(FLAG_EXT_CTRL,&chan->flags))
-             return L2CAP_EXT_HDR_SIZE - L2CAP_HDR_SIZE;

Quoted 8 lines

Quoted 3 lines

include/net/bluetooth/l2cap.h static inline bool l2cap_clear_timer(struct l2cap_chan *chan, · line 841

Quoted 3 lines

include/net/bluetooth/l2cap.h +0 -3

-     else
-             return L2CAP_ENH_HDR_SIZE - L2CAP_HDR_SIZE;
-}

Do we have that many places with this test in Mat's code? If not, then we might not need to bother having all of these helpers, I think. And if we add them, I do think it makes sense to add them to l2cap_core.c than in l2cap.h, right?

Not too many places with the FLAG_EXT_CTRL test, with use of struct bt_control in most of the code. I would rather have helpers in l2cap_core.c if they're needed.

Thanks for the reviews!

This change affects data structures storing ERTM state and control fields, and adds new definitions for states and events. =A0An l2cap_seq_list structure is added for tracking ERTM sequence numbers without repeated memory allocations. =A0Control fields are carried in the bt_skb_cb struct rather than constantly doing shift and mask operations.

Signed-off-by Mat Martineau <[email protected]>

---
=A0include/net/bluetooth/bluetooth.h | =A0 14 ++- =A0include/net/bluetooth/l2cap.h =A0 =A0 | =A0260 +++++++++--------------=

=A02 files changed, 73 insertions(+), 201 deletions(-)

index d6d8ec8..a499b60 100644

include/net/bluetooth/l2cap.h +0 -0

Hi Mat,

Quoted 12 lines · Mat Martineau

This change affects data structures storing ERTM state and control fields, and adds new definitions for states and events. An l2cap_seq_list structure is added for tracking ERTM sequence numbers without repeated memory allocations. Control fields are carried in the bt_skb_cb struct rather than constantly doing shift and mask operations.

Signed-off-by Mat Martineau<[email protected]>

---
  include/net/bluetooth/bluetooth.h |   14 ++-
  include/net/bluetooth/l2cap.h     |  260 +++++++++----------------------------
  2 files changed, 73 insertions(+), 201 deletions(-)

<snip>

Quoted 4 lines

include/net/bluetooth/l2cap.h +0 -0

index d6d8ec8..a499b60 100644

<snip>

include/net/bluetooth/l2cap.h static inline bool l2cap_clear_timer(struct l2cap_chan *chan, · line 666

Quoted 4 lines

include/net/bluetooth/l2cap.h +0 -0

@@ -645,200 +664,43 @@ static inline bool l2cap_clear_timer(struct l2cap_chan *chan,
 
  #define __set_chan_timer(c, t) l2cap_set_timer(c,&c->chan_timer, (t))
  #define __clear_chan_timer(c) l2cap_clear_timer(c,&c->chan_timer)

Are these two still needed? I saw you moved others to l2cap_core.c which is fine but what about these?

Ulisses -

Quoted 32 lines · Ulisses Furquim

Hi Mat,

Quoted 14 lines · On Thu, Feb 23, 2012 at 6:37 PM, Mat Martineau<[email protected]> =A0wrote:

This change affects data structures storing ERTM state and control fields, and adds new definitions for states and events. =A0An l2cap_seq_list structure is added for tracking ERTM sequence numbers without repeated memory allocations. =A0Control fields are carried in the bt_skb_cb struct rather than constantly doing shift and mask operations.

Signed-off-by Mat Martineau<[email protected]>

---
=A0include/net/bluetooth/bluetooth.h | =A0 14 ++- =A0include/net/bluetooth/l2cap.h =A0 =A0 | =A0260
+++++++++----------------------------
=A02 files changed, 73 insertions(+), 201 deletions(-)

<snip>

Quoted 5 lines

include/net/bluetooth/l2cap.h +0 -0

index d6d8ec8..a499b60 100644

<snip>

Quoted 4 lines

Quoted 4 lines

include/net/bluetooth/l2cap.h +0 -0

@@ -645,200 +664,43 @@ static inline bool l2cap_clear_timer(struct
 l2cap_chan *chan,
 

=A0#define __set_chan_timer(c, t) l2cap_set_timer(c,&c->chan_timer, (t)=

Quoted 5 lines

Quoted 1 line

=A0#define __clear_chan_timer(c) l2cap_clear_timer(c,&c->chan_timer)

Are these two still needed? I saw you moved others to l2cap_core.c which is fine but what about these?

Since these macros are unrelated to ERTM, that would be a separate patch.

I think all the macros traditionally land in the header files because the=

are no #defines in the l2cap*.c files. =A0However, I see that many other =

files in net/bluetooth do have #defines.

It's not that I "moved" the other macros, as much as my ported code had static functions instead. =A0I tried to minimize changes to the ported co=

rather than minimize changes to the upstream code.

This change affects data structures storing ERTM state and control fields, and adds new definitions for states and events. An l2cap_seq_list structure is added for tracking ERTM sequence numbers without repeated memory allocations. Control fields are carried in the bt_skb_cb struct rather than constantly doing shift and mask operations.

Signed-off-by Mat Martineau <[email protected]>

---
 include/net/bluetooth/bluetooth.h |   14 ++-
 include/net/bluetooth/l2cap.h     |  260 +++++++++----------------------------
 2 files changed, 73 insertions(+), 201 deletions(-)

include/net/bluetooth/bluetooth.h +0 -0

index 262ebd1..a667cb8 100644

include/net/bluetooth/bluetooth.h +1 -1

-	__u8 sar;
 	__u8 force_active;
+	struct bt_l2cap_control control;
 };
 #define bt_cb(skb) ((struct bt_skb_cb *)((skb)->cb))
 

include/net/bluetooth/l2cap.h +0 -0

index d6d8ec8..a499b60 100644

include/net/bluetooth/l2cap.h +9 -0

    Written 2000,2001 by Maxim Krasnyansky <[email protected]>
 

@@ -44,6 +45,9 @@
 #define L2CAP_DEFAULT_MAX_SDU_SIZE	0xFFFF
 #define L2CAP_DEFAULT_SDU_ITIME		0xFFFFFFFF
 #define L2CAP_DEFAULT_ACC_LAT		0xFFFFFFFF
+#define L2CAP_BREDR_MAX_PAYLOAD		1019    /* 3-DH5 packet */
+#define L2CAP_MAX_ERTM_QUEUED		5
+#define L2CAP_MIN_ERTM_QUEUED		2
 
 #define L2CAP_DISC_TIMEOUT             (100)
 #define L2CAP_DISC_REJ_TIMEOUT         (5000)  /*  5 seconds */

@@ -139,6 +143,8 @@ struct l2cap_conninfo {
 
 #define L2CAP_CTRL_TXSEQ_SHIFT		1
 #define L2CAP_CTRL_SUPER_SHIFT		2
+#define L2CAP_CTRL_POLL_SHIFT		4
+#define L2CAP_CTRL_FINAL_SHIFT		7
 #define L2CAP_CTRL_REQSEQ_SHIFT		8
 #define L2CAP_CTRL_SAR_SHIFT		14
 

@@ -152,9 +158,11 @@ struct l2cap_conninfo {
 #define L2CAP_EXT_CTRL_FINAL		0x00000002
 #define L2CAP_EXT_CTRL_FRAME_TYPE	0x00000001 /* I- or S-Frame */
 
+#define L2CAP_EXT_CTRL_FINAL_SHIFT	1
 #define L2CAP_EXT_CTRL_REQSEQ_SHIFT	2
 #define L2CAP_EXT_CTRL_SAR_SHIFT	16
 #define L2CAP_EXT_CTRL_SUPER_SHIFT	16
+#define L2CAP_EXT_CTRL_POLL_SHIFT	18
 #define L2CAP_EXT_CTRL_TXSEQ_SHIFT	18
 
 /* L2CAP Supervisory Function */

@@ -186,6 +194,8 @@ struct l2cap_hdr {
 #define L2CAP_FCS_SIZE		2
 #define L2CAP_SDULEN_SIZE	2
 #define L2CAP_PSMLEN_SIZE	2
+#define L2CAP_ENH_CTRL_SIZE	2
+#define L2CAP_EXT_CTRL_SIZE	4
 
 struct l2cap_cmd_hdr {
 	__u8       code;

@@ -401,9 +411,12 @@ struct l2cap_conn_param_update_rsp {
 #define L2CAP_CONN_PARAM_REJECTED	0x0001
 
 /* ----- L2CAP channels and connections ----- */

include/net/bluetooth/l2cap.h +6 -2

 struct l2cap_chan {

@@ -446,6 +459,9 @@ struct l2cap_chan {
 	__u16		monitor_timeout;
 	__u16		mps;
 
+	__u8		tx_state;
+	__u8		rx_state;
+
 	unsigned long	conf_state;
 	unsigned long	conn_state;
 	unsigned long	flags;

@@ -454,15 +470,16 @@ struct l2cap_chan {
 	__u16		expected_ack_seq;
 	__u16		expected_tx_seq;
 	__u16		buffer_seq;
-	__u16		buffer_seq_srej;
 	__u16		srej_save_reqseq;
+	__u16		last_acked_seq;
 	__u16		frames_sent;
 	__u16		unacked_frames;
 	__u8		retry_count;
-	__u8		num_acked;
+	__u16		srej_queue_next;
 	__u16		sdu_len;
 	struct sk_buff	*sdu;
 	struct sk_buff	*sdu_last_frag;
+	atomic_t	ertm_queued;
 
 	__u16		remote_tx_win;
 	__u8		remote_max_tx;

include/net/bluetooth/l2cap.h +2 -1

 	struct sk_buff		*tx_send_head;
 	struct sk_buff_head	tx_q;
 	struct sk_buff_head	srej_q;
-	struct list_head	srej_l;
+	struct l2cap_seq_list	srej_list;
+	struct l2cap_seq_list	retrans_list;
 
 	struct list_head list;
 	struct list_head global_l;

@@ -645,200 +664,43 @@ static inline bool l2cap_clear_timer(struct l2cap_chan *chan,
 
 #define __set_chan_timer(c, t) l2cap_set_timer(c, &c->chan_timer, (t))

include/net/bluetooth/l2cap.h +0 -174

-
-static inline int __seq_offset(struct l2cap_chan *chan, __u16 seq1, __u16 seq2)
-{
-	int offset;
-
-	offset = (seq1 - seq2) % (chan->tx_win_max + 1);
-	if (offset < 0)
-		offset += (chan->tx_win_max + 1);
-
-	return offset;
-}
-
-static inline __u16 __next_seq(struct l2cap_chan *chan, __u16 seq)
-{
-	return (seq + 1) % (chan->tx_win_max + 1);
-}
-
-static inline int l2cap_tx_window_full(struct l2cap_chan *ch)
-{
-	int sub;
-
-	sub = (ch->next_tx_seq - ch->expected_ack_seq) % 64;
-
-	if (sub < 0)
-		sub += 64;
-
-	return sub == ch->remote_tx_win;
-}
-
-static inline __u16 __get_reqseq(struct l2cap_chan *chan, __u32 ctrl)
-{
-	if (test_bit(FLAG_EXT_CTRL, &chan->flags))
-		return (ctrl & L2CAP_EXT_CTRL_REQSEQ) >>
-						L2CAP_EXT_CTRL_REQSEQ_SHIFT;
-	else
-		return (ctrl & L2CAP_CTRL_REQSEQ) >> L2CAP_CTRL_REQSEQ_SHIFT;
-}
-
-static inline __u32 __set_reqseq(struct l2cap_chan *chan, __u32 reqseq)
-{
-	if (test_bit(FLAG_EXT_CTRL, &chan->flags))
-		return (reqseq << L2CAP_EXT_CTRL_REQSEQ_SHIFT) &
-							L2CAP_EXT_CTRL_REQSEQ;
-	else
-		return (reqseq << L2CAP_CTRL_REQSEQ_SHIFT) & L2CAP_CTRL_REQSEQ;
-}
-
-static inline __u16 __get_txseq(struct l2cap_chan *chan, __u32 ctrl)
-{
-	if (test_bit(FLAG_EXT_CTRL, &chan->flags))
-		return (ctrl & L2CAP_EXT_CTRL_TXSEQ) >>
-						L2CAP_EXT_CTRL_TXSEQ_SHIFT;
-	else
-		return (ctrl & L2CAP_CTRL_TXSEQ) >> L2CAP_CTRL_TXSEQ_SHIFT;
-}
-
-static inline __u32 __set_txseq(struct l2cap_chan *chan, __u32 txseq)
-{
-	if (test_bit(FLAG_EXT_CTRL, &chan->flags))
-		return (txseq << L2CAP_EXT_CTRL_TXSEQ_SHIFT) &
-							L2CAP_EXT_CTRL_TXSEQ;
-	else
-		return (txseq << L2CAP_CTRL_TXSEQ_SHIFT) & L2CAP_CTRL_TXSEQ;
-}
-
-static inline bool __is_sframe(struct l2cap_chan *chan, __u32 ctrl)
-{
-	if (test_bit(FLAG_EXT_CTRL, &chan->flags))
-		return ctrl & L2CAP_EXT_CTRL_FRAME_TYPE;
-	else
-		return ctrl & L2CAP_CTRL_FRAME_TYPE;
-}
-
-static inline __u32 __set_sframe(struct l2cap_chan *chan)
-{
-	if (test_bit(FLAG_EXT_CTRL, &chan->flags))
-		return L2CAP_EXT_CTRL_FRAME_TYPE;
-	else
-		return L2CAP_CTRL_FRAME_TYPE;
-}
-
-static inline __u8 __get_ctrl_sar(struct l2cap_chan *chan, __u32 ctrl)
-{
-	if (test_bit(FLAG_EXT_CTRL, &chan->flags))
-		return (ctrl & L2CAP_EXT_CTRL_SAR) >> L2CAP_EXT_CTRL_SAR_SHIFT;
-	else
-		return (ctrl & L2CAP_CTRL_SAR) >> L2CAP_CTRL_SAR_SHIFT;
-}
 
-static inline __u32 __set_ctrl_sar(struct l2cap_chan *chan, __u32 sar)
-{
-	if (test_bit(FLAG_EXT_CTRL, &chan->flags))
-		return (sar << L2CAP_EXT_CTRL_SAR_SHIFT) & L2CAP_EXT_CTRL_SAR;
-	else
-		return (sar << L2CAP_CTRL_SAR_SHIFT) & L2CAP_CTRL_SAR;
-}
-
-static inline bool __is_sar_start(struct l2cap_chan *chan, __u32 ctrl)
-{
-	return __get_ctrl_sar(chan, ctrl) == L2CAP_SAR_START;
-}
-
-static inline __u32 __get_sar_mask(struct l2cap_chan *chan)
-{
-	if (test_bit(FLAG_EXT_CTRL, &chan->flags))
-		return L2CAP_EXT_CTRL_SAR;
-	else
-		return L2CAP_CTRL_SAR;
-}
-
-static inline __u8 __get_ctrl_super(struct l2cap_chan *chan, __u32 ctrl)
-{
-	if (test_bit(FLAG_EXT_CTRL, &chan->flags))
-		return (ctrl & L2CAP_EXT_CTRL_SUPERVISE) >>
-						L2CAP_EXT_CTRL_SUPER_SHIFT;
-	else
-		return (ctrl & L2CAP_CTRL_SUPERVISE) >> L2CAP_CTRL_SUPER_SHIFT;
-}
-
-static inline __u32 __set_ctrl_super(struct l2cap_chan *chan, __u32 super)
-{
-	if (test_bit(FLAG_EXT_CTRL, &chan->flags))
-		return (super << L2CAP_EXT_CTRL_SUPER_SHIFT) &
-						L2CAP_EXT_CTRL_SUPERVISE;
-	else
-		return (super << L2CAP_CTRL_SUPER_SHIFT) &
-							L2CAP_CTRL_SUPERVISE;
-}
-
-static inline __u32 __set_ctrl_final(struct l2cap_chan *chan)
-{
-	if (test_bit(FLAG_EXT_CTRL, &chan->flags))
-		return L2CAP_EXT_CTRL_FINAL;
-	else
-		return L2CAP_CTRL_FINAL;
-}
-
-static inline bool __is_ctrl_final(struct l2cap_chan *chan, __u32 ctrl)
-{
-	if (test_bit(FLAG_EXT_CTRL, &chan->flags))
-		return ctrl & L2CAP_EXT_CTRL_FINAL;
-	else
-		return ctrl & L2CAP_CTRL_FINAL;
-}
-
-static inline __u32 __set_ctrl_poll(struct l2cap_chan *chan)
-{
-	if (test_bit(FLAG_EXT_CTRL, &chan->flags))
-		return L2CAP_EXT_CTRL_POLL;
-	else
-		return L2CAP_CTRL_POLL;
-}
-
-static inline bool __is_ctrl_poll(struct l2cap_chan *chan, __u32 ctrl)
-{
-	if (test_bit(FLAG_EXT_CTRL, &chan->flags))
-		return ctrl & L2CAP_EXT_CTRL_POLL;
-	else
-		return ctrl & L2CAP_CTRL_POLL;
-}
-
-static inline __u32 __get_control(struct l2cap_chan *chan, void *p)
-{
-	if (test_bit(FLAG_EXT_CTRL, &chan->flags))
-		return get_unaligned_le32(p);
-	else
-		return get_unaligned_le16(p);
-}
-
-static inline void __put_control(struct l2cap_chan *chan, __u32 control,
-								void *p)
-{
-	if (test_bit(FLAG_EXT_CTRL, &chan->flags))
-		return put_unaligned_le32(control, p);
-	else

include/net/bluetooth/l2cap.h +14 -0

+#define L2CAP_SEQ_LIST_CLEAR       0xFFFF
+#define L2CAP_SEQ_LIST_TAIL        0x8000
+
+#define L2CAP_ERTM_TX_STATE_XMIT          0x01
+#define L2CAP_ERTM_TX_STATE_WAIT_F        0x02
+
+#define L2CAP_ERTM_RX_STATE_RECV                    0x01
+#define L2CAP_ERTM_RX_STATE_SREJ_SENT               0x02
+
+#define L2CAP_ERTM_TXSEQ_EXPECTED        0x00
+#define L2CAP_ERTM_TXSEQ_EXPECTED_SREJ   0x01
+#define L2CAP_ERTM_TXSEQ_UNEXPECTED      0x02
+#define L2CAP_ERTM_TXSEQ_UNEXPECTED_SREJ 0x03
+#define L2CAP_ERTM_TXSEQ_DUPLICATE       0x04

Hi Mat,

Quoted 20 lines · Mat Martineau

This change affects data structures storing ERTM state and control fields, and adds new definitions for states and events. An l2cap_seq_list structure is added for tracking ERTM sequence numbers without repeated memory allocations. Control fields are carried in the bt_skb_cb struct rather than constantly doing shift and mask operations.

Signed-off-by Mat Martineau <[email protected]>

---
 include/net/bluetooth/bluetooth.h |   14 ++-
 include/net/bluetooth/l2cap.h     |  260 +++++++++----------------------------
 2 files changed, 73 insertions(+), 201 deletions(-)

include/net/bluetooth/bluetooth.h +0 -0

index 262ebd1..a667cb8 100644
@@ -215,14 +215,24 @@ void bt_accept_unlink(struct sock *sk);
 struct sock *bt_accept_dequeue(struct sock *parent, struct socket *newsock);
 

include/net/bluetooth/bluetooth.h void bt_accept_unlink(struct sock *sk); · line 218

Quoted 2 lines

include/net/bluetooth/bluetooth.h +1 -0

 /* Skb helpers */
+struct bt_l2cap_control {

I would call this struct l2cap_ctrl only. Much smaller.

include/net/bluetooth/bluetooth.h void bt_accept_unlink(struct sock *sk); · line 219

Quoted 1 line

include/net/bluetooth/bluetooth.h +1 -0

+	__u8  frame_type;

This could be frame_type:1, only one bit is needed here.

include/net/bluetooth/bluetooth.h void bt_accept_unlink(struct sock *sk); · line 220

Quoted 1 line

include/net/bluetooth/bluetooth.h +1 -0

+	__u8  final;

final:1

include/net/bluetooth/bluetooth.h void bt_accept_unlink(struct sock *sk); · line 221

Quoted 1 line

include/net/bluetooth/bluetooth.h +1 -0

+	__u8  sar;

sar:2

include/net/bluetooth/bluetooth.h void bt_accept_unlink(struct sock *sk); · line 222

Quoted 1 line

include/net/bluetooth/bluetooth.h +1 -0

+	__u8  super;

super:2

include/net/bluetooth/bluetooth.h void bt_accept_unlink(struct sock *sk); · line 223

Quoted 1 line

include/net/bluetooth/bluetooth.h +1 -0

+	__u16 reqseq;

go on..

Quoted 6 lines

include/net/bluetooth/bluetooth.h +5 -0

+	__u16 txseq;
+	__u8  poll;
+	__u8  fcs;
+};
+
 struct bt_skb_cb {

include/net/bluetooth/bluetooth.h void bt_accept_unlink(struct sock *sk); · line 233

Quoted 5 lines

include/net/bluetooth/bluetooth.h +0 -1

 	__u8 pkt_type;
 	__u8 incoming;
 	__u16 expect;
-	__u16 tx_seq;
 	__u8 retries;

I would put retries inside l2cap_ctrl. It says how many times we sent that frame, looks a type of information that fits there.

Quoted 6 lines

include/net/bluetooth/bluetooth.h +1 -1

-	__u8 sar;
 	__u8 force_active;
+	struct bt_l2cap_control control;
 };
 #define bt_cb(skb) ((struct bt_skb_cb *)((skb)->cb))
 

include/net/bluetooth/l2cap.h line 6

Quoted 9 lines

include/net/bluetooth/l2cap.h +1 -0

index d6d8ec8..a499b60 100644
@@ -3,6 +3,7 @@
    Copyright (C) 2000-2001 Qualcomm Incorporated
    Copyright (C) 2009-2010 Gustavo F. Padovan <[email protected]>
    Copyright (C) 2010 Google Inc.
+   Copyright (c) 2012 Code Aurora Forum.  All rights reserved.

Leave the Copyright note to when you really add code to l2cap.c

Quoted 46 lines

include/net/bluetooth/l2cap.h +9 -0

    Written 2000,2001 by Maxim Krasnyansky <[email protected]>
 

@@ -44,6 +45,9 @@
 #define L2CAP_DEFAULT_MAX_SDU_SIZE	0xFFFF
 #define L2CAP_DEFAULT_SDU_ITIME		0xFFFFFFFF
 #define L2CAP_DEFAULT_ACC_LAT		0xFFFFFFFF
+#define L2CAP_BREDR_MAX_PAYLOAD		1019    /* 3-DH5 packet */
+#define L2CAP_MAX_ERTM_QUEUED		5
+#define L2CAP_MIN_ERTM_QUEUED		2
 
 #define L2CAP_DISC_TIMEOUT             (100)
 #define L2CAP_DISC_REJ_TIMEOUT         (5000)  /*  5 seconds */

@@ -139,6 +143,8 @@ struct l2cap_conninfo {
 
 #define L2CAP_CTRL_TXSEQ_SHIFT		1
 #define L2CAP_CTRL_SUPER_SHIFT		2
+#define L2CAP_CTRL_POLL_SHIFT		4
+#define L2CAP_CTRL_FINAL_SHIFT		7
 #define L2CAP_CTRL_REQSEQ_SHIFT		8
 #define L2CAP_CTRL_SAR_SHIFT		14
 

@@ -152,9 +158,11 @@ struct l2cap_conninfo {
 #define L2CAP_EXT_CTRL_FINAL		0x00000002
 #define L2CAP_EXT_CTRL_FRAME_TYPE	0x00000001 /* I- or S-Frame */
 
+#define L2CAP_EXT_CTRL_FINAL_SHIFT	1
 #define L2CAP_EXT_CTRL_REQSEQ_SHIFT	2
 #define L2CAP_EXT_CTRL_SAR_SHIFT	16
 #define L2CAP_EXT_CTRL_SUPER_SHIFT	16
+#define L2CAP_EXT_CTRL_POLL_SHIFT	18
 #define L2CAP_EXT_CTRL_TXSEQ_SHIFT	18
 
 /* L2CAP Supervisory Function */

@@ -186,6 +194,8 @@ struct l2cap_hdr {
 #define L2CAP_FCS_SIZE		2
 #define L2CAP_SDULEN_SIZE	2
 #define L2CAP_PSMLEN_SIZE	2
+#define L2CAP_ENH_CTRL_SIZE	2
+#define L2CAP_EXT_CTRL_SIZE	4
 
 struct l2cap_cmd_hdr {
 	__u8       code;

@@ -401,9 +411,12 @@ struct l2cap_conn_param_update_rsp {
 #define L2CAP_CONN_PARAM_REJECTED	0x0001
 

include/net/bluetooth/l2cap.h struct l2cap_conn_param_update_rsp { · line 420

Quoted 11 lines

include/net/bluetooth/l2cap.h +6 -3

 /* ----- L2CAP channels and connections ----- */
-struct srej_list {
-	__u16	tx_seq;
-	struct list_head list;
+struct l2cap_seq_list {
+	__u16 head;
+	__u16 tail;
+	__u16 size;
+	__u16 mask;
+	__u16 *list;
 };

So I looked to the code and saw the big kalloc you do in the list element here that is 256KB if we use the maximum tx_win in both directions. I'm open to ideas here, I don't have any better.

Quoted 29 lines

include/net/bluetooth/l2cap.h +6 -2

 struct l2cap_chan {

@@ -446,6 +459,9 @@ struct l2cap_chan {
 	__u16		monitor_timeout;
 	__u16		mps;
 
+	__u8		tx_state;
+	__u8		rx_state;
+
 	unsigned long	conf_state;
 	unsigned long	conn_state;
 	unsigned long	flags;

@@ -454,15 +470,16 @@ struct l2cap_chan {
 	__u16		expected_ack_seq;
 	__u16		expected_tx_seq;
 	__u16		buffer_seq;
-	__u16		buffer_seq_srej;
 	__u16		srej_save_reqseq;
+	__u16		last_acked_seq;
 	__u16		frames_sent;
 	__u16		unacked_frames;
 	__u8		retry_count;
-	__u8		num_acked;
+	__u16		srej_queue_next;
 	__u16		sdu_len;
 	struct sk_buff	*sdu;
 	struct sk_buff	*sdu_last_frag;
+	atomic_t	ertm_queued;
 

include/net/bluetooth/l2cap.h struct l2cap_chan { · line 506

Quoted 7 lines

include/net/bluetooth/l2cap.h +1 -0

 	__u16		remote_tx_win;
 	__u8		remote_max_tx;

@@ -486,11 +503,13 @@ struct l2cap_chan {
 	struct delayed_work	retrans_timer;
 	struct delayed_work	monitor_timer;
 	struct delayed_work	ack_timer;
+	struct work_struct	tx_work;

Do we really need another work in the L2CAP code?

Quoted 12 lines

include/net/bluetooth/l2cap.h +2 -1

 	struct sk_buff		*tx_send_head;
 	struct sk_buff_head	tx_q;
 	struct sk_buff_head	srej_q;
-	struct list_head	srej_l;
+	struct l2cap_seq_list	srej_list;
+	struct l2cap_seq_list	retrans_list;
 
 	struct list_head list;
 	struct list_head global_l;

@@ -645,200 +664,43 @@ static inline bool l2cap_clear_timer(struct l2cap_chan *chan,
 

include/net/bluetooth/l2cap.h static inline bool l2cap_clear_timer(struct l2cap_chan *chan, · line 656

Quoted 11 lines

include/net/bluetooth/l2cap.h +0 -9

 #define __set_chan_timer(c, t) l2cap_set_timer(c, &c->chan_timer, (t))
 #define __clear_chan_timer(c) l2cap_clear_timer(c, &c->chan_timer)
-#define __set_retrans_timer(c) l2cap_set_timer(c, &c->retrans_timer, \
-		msecs_to_jiffies(L2CAP_DEFAULT_RETRANS_TO));
-#define __clear_retrans_timer(c) l2cap_clear_timer(c, &c->retrans_timer)
-#define __set_monitor_timer(c) l2cap_set_timer(c, &c->monitor_timer, \
-		msecs_to_jiffies(L2CAP_DEFAULT_MONITOR_TO));
-#define __clear_monitor_timer(c) l2cap_clear_timer(c, &c->monitor_timer)
-#define __set_ack_timer(c) l2cap_set_timer(c, &chan->ack_timer, \
-		msecs_to_jiffies(L2CAP_DEFAULT_ACK_TO));
-#define __clear_ack_timer(c) l2cap_clear_timer(c, &c->ack_timer)

So, I was expecting a patch that we could build without the l2cap.c code so we could apply it and move on, but this one completely breaks the build.

Also I noted that you just turned some of macros here in functions in l2cap.c. Why? Keep those as is for the next round of patches, no unnecessary changes here.

Quoted 179 lines

include/net/bluetooth/l2cap.h +0 -178

-
-static inline int __seq_offset(struct l2cap_chan *chan, __u16 seq1, __u16 seq2)
-{
-	int offset;
-
-	offset = (seq1 - seq2) % (chan->tx_win_max + 1);
-	if (offset < 0)
-		offset += (chan->tx_win_max + 1);
-
-	return offset;
-}
-
-static inline __u16 __next_seq(struct l2cap_chan *chan, __u16 seq)
-{
-	return (seq + 1) % (chan->tx_win_max + 1);
-}
-
-static inline int l2cap_tx_window_full(struct l2cap_chan *ch)
-{
-	int sub;
-
-	sub = (ch->next_tx_seq - ch->expected_ack_seq) % 64;
-
-	if (sub < 0)
-		sub += 64;
-
-	return sub == ch->remote_tx_win;
-}
-
-static inline __u16 __get_reqseq(struct l2cap_chan *chan, __u32 ctrl)
-{
-	if (test_bit(FLAG_EXT_CTRL, &chan->flags))
-		return (ctrl & L2CAP_EXT_CTRL_REQSEQ) >>
-						L2CAP_EXT_CTRL_REQSEQ_SHIFT;
-	else
-		return (ctrl & L2CAP_CTRL_REQSEQ) >> L2CAP_CTRL_REQSEQ_SHIFT;
-}
-
-static inline __u32 __set_reqseq(struct l2cap_chan *chan, __u32 reqseq)
-{
-	if (test_bit(FLAG_EXT_CTRL, &chan->flags))
-		return (reqseq << L2CAP_EXT_CTRL_REQSEQ_SHIFT) &
-							L2CAP_EXT_CTRL_REQSEQ;
-	else
-		return (reqseq << L2CAP_CTRL_REQSEQ_SHIFT) & L2CAP_CTRL_REQSEQ;
-}
-
-static inline __u16 __get_txseq(struct l2cap_chan *chan, __u32 ctrl)
-{
-	if (test_bit(FLAG_EXT_CTRL, &chan->flags))
-		return (ctrl & L2CAP_EXT_CTRL_TXSEQ) >>
-						L2CAP_EXT_CTRL_TXSEQ_SHIFT;
-	else
-		return (ctrl & L2CAP_CTRL_TXSEQ) >> L2CAP_CTRL_TXSEQ_SHIFT;
-}
-
-static inline __u32 __set_txseq(struct l2cap_chan *chan, __u32 txseq)
-{
-	if (test_bit(FLAG_EXT_CTRL, &chan->flags))
-		return (txseq << L2CAP_EXT_CTRL_TXSEQ_SHIFT) &
-							L2CAP_EXT_CTRL_TXSEQ;
-	else
-		return (txseq << L2CAP_CTRL_TXSEQ_SHIFT) & L2CAP_CTRL_TXSEQ;
-}
-
-static inline bool __is_sframe(struct l2cap_chan *chan, __u32 ctrl)
-{
-	if (test_bit(FLAG_EXT_CTRL, &chan->flags))
-		return ctrl & L2CAP_EXT_CTRL_FRAME_TYPE;
-	else
-		return ctrl & L2CAP_CTRL_FRAME_TYPE;
-}
-
-static inline __u32 __set_sframe(struct l2cap_chan *chan)
-{
-	if (test_bit(FLAG_EXT_CTRL, &chan->flags))
-		return L2CAP_EXT_CTRL_FRAME_TYPE;
-	else
-		return L2CAP_CTRL_FRAME_TYPE;
-}
-
-static inline __u8 __get_ctrl_sar(struct l2cap_chan *chan, __u32 ctrl)
-{
-	if (test_bit(FLAG_EXT_CTRL, &chan->flags))
-		return (ctrl & L2CAP_EXT_CTRL_SAR) >> L2CAP_EXT_CTRL_SAR_SHIFT;
-	else
-		return (ctrl & L2CAP_CTRL_SAR) >> L2CAP_CTRL_SAR_SHIFT;
-}
 
-static inline __u32 __set_ctrl_sar(struct l2cap_chan *chan, __u32 sar)
-{
-	if (test_bit(FLAG_EXT_CTRL, &chan->flags))
-		return (sar << L2CAP_EXT_CTRL_SAR_SHIFT) & L2CAP_EXT_CTRL_SAR;
-	else
-		return (sar << L2CAP_CTRL_SAR_SHIFT) & L2CAP_CTRL_SAR;
-}
-
-static inline bool __is_sar_start(struct l2cap_chan *chan, __u32 ctrl)
-{
-	return __get_ctrl_sar(chan, ctrl) == L2CAP_SAR_START;
-}
-
-static inline __u32 __get_sar_mask(struct l2cap_chan *chan)
-{
-	if (test_bit(FLAG_EXT_CTRL, &chan->flags))
-		return L2CAP_EXT_CTRL_SAR;
-	else
-		return L2CAP_CTRL_SAR;
-}
-
-static inline __u8 __get_ctrl_super(struct l2cap_chan *chan, __u32 ctrl)
-{
-	if (test_bit(FLAG_EXT_CTRL, &chan->flags))
-		return (ctrl & L2CAP_EXT_CTRL_SUPERVISE) >>
-						L2CAP_EXT_CTRL_SUPER_SHIFT;
-	else
-		return (ctrl & L2CAP_CTRL_SUPERVISE) >> L2CAP_CTRL_SUPER_SHIFT;
-}
-
-static inline __u32 __set_ctrl_super(struct l2cap_chan *chan, __u32 super)
-{
-	if (test_bit(FLAG_EXT_CTRL, &chan->flags))
-		return (super << L2CAP_EXT_CTRL_SUPER_SHIFT) &
-						L2CAP_EXT_CTRL_SUPERVISE;
-	else
-		return (super << L2CAP_CTRL_SUPER_SHIFT) &
-							L2CAP_CTRL_SUPERVISE;
-}
-
-static inline __u32 __set_ctrl_final(struct l2cap_chan *chan)
-{
-	if (test_bit(FLAG_EXT_CTRL, &chan->flags))
-		return L2CAP_EXT_CTRL_FINAL;
-	else
-		return L2CAP_CTRL_FINAL;
-}
-
-static inline bool __is_ctrl_final(struct l2cap_chan *chan, __u32 ctrl)
-{
-	if (test_bit(FLAG_EXT_CTRL, &chan->flags))
-		return ctrl & L2CAP_EXT_CTRL_FINAL;
-	else
-		return ctrl & L2CAP_CTRL_FINAL;
-}
-
-static inline __u32 __set_ctrl_poll(struct l2cap_chan *chan)
-{
-	if (test_bit(FLAG_EXT_CTRL, &chan->flags))
-		return L2CAP_EXT_CTRL_POLL;
-	else
-		return L2CAP_CTRL_POLL;
-}
-
-static inline bool __is_ctrl_poll(struct l2cap_chan *chan, __u32 ctrl)
-{
-	if (test_bit(FLAG_EXT_CTRL, &chan->flags))
-		return ctrl & L2CAP_EXT_CTRL_POLL;
-	else
-		return ctrl & L2CAP_CTRL_POLL;
-}
-
-static inline __u32 __get_control(struct l2cap_chan *chan, void *p)
-{
-	if (test_bit(FLAG_EXT_CTRL, &chan->flags))
-		return get_unaligned_le32(p);
-	else
-		return get_unaligned_le16(p);
-}
-
-static inline void __put_control(struct l2cap_chan *chan, __u32 control,
-								void *p)
-{
-	if (test_bit(FLAG_EXT_CTRL, &chan->flags))
-		return put_unaligned_le32(control, p);
-	else
-		return put_unaligned_le16(control, p);
-}
-
-static inline __u8 __ctrl_size(struct l2cap_chan *chan)

include/net/bluetooth/l2cap.h static inline bool l2cap_clear_timer(struct l2cap_chan *chan, · line 841

Quoted 6 lines

include/net/bluetooth/l2cap.h +0 -6

-{
-	if (test_bit(FLAG_EXT_CTRL, &chan->flags))
-		return L2CAP_EXT_HDR_SIZE - L2CAP_HDR_SIZE;
-	else
-		return L2CAP_ENH_HDR_SIZE - L2CAP_HDR_SIZE;
-}

Are we going to replace the Extended Window Size implemetation too?

Quoted 18 lines

include/net/bluetooth/l2cap.h +18 -0

+#define L2CAP_SEQ_LIST_CLEAR       0xFFFF
+#define L2CAP_SEQ_LIST_TAIL        0x8000
+
+#define L2CAP_ERTM_TX_STATE_XMIT          0x01
+#define L2CAP_ERTM_TX_STATE_WAIT_F        0x02
+
+#define L2CAP_ERTM_RX_STATE_RECV                    0x01
+#define L2CAP_ERTM_RX_STATE_SREJ_SENT               0x02
+
+#define L2CAP_ERTM_TXSEQ_EXPECTED        0x00
+#define L2CAP_ERTM_TXSEQ_EXPECTED_SREJ   0x01
+#define L2CAP_ERTM_TXSEQ_UNEXPECTED      0x02
+#define L2CAP_ERTM_TXSEQ_UNEXPECTED_SREJ 0x03
+#define L2CAP_ERTM_TXSEQ_DUPLICATE       0x04
+#define L2CAP_ERTM_TXSEQ_DUPLICATE_SREJ  0x05
+#define L2CAP_ERTM_TXSEQ_INVALID         0x06
+#define L2CAP_ERTM_TXSEQ_INVALID_IGNORE  0x07
+

include/net/bluetooth/l2cap.h static inline bool l2cap_clear_timer(struct l2cap_chan *chan, · line 691

Quoted 6 lines

include/net/bluetooth/l2cap.h +6 -0

+#define L2CAP_ERTM_EVENT_DATA_REQUEST          0x01
+#define L2CAP_ERTM_EVENT_LOCAL_BUSY_DETECTED   0x02
+#define L2CAP_ERTM_EVENT_LOCAL_BUSY_CLEAR      0x03
+#define L2CAP_ERTM_EVENT_RECV_REQSEQ_AND_FBIT  0x04
+#define L2CAP_ERTM_EVENT_RECV_FBIT             0x05
+#define L2CAP_ERTM_EVENT_RETRANS_TIMER_EXPIRES 0x06

			...RETRANS_TO

include/net/bluetooth/l2cap.h static inline bool l2cap_clear_timer(struct l2cap_chan *chan, · line 692

Quoted 1 line

include/net/bluetooth/l2cap.h +1 -0

+#define L2CAP_ERTM_EVENT_MONITOR_TIMER_EXPIRES 0x07

			...MONITOR_TO

include/net/bluetooth/l2cap.h static inline bool l2cap_clear_timer(struct l2cap_chan *chan, · line 699

Quoted 7 lines

include/net/bluetooth/l2cap.h +7 -0

+#define L2CAP_ERTM_EVENT_EXPLICIT_POLL         0x08
+#define L2CAP_ERTM_EVENT_RECV_IFRAME           0x09
+#define L2CAP_ERTM_EVENT_RECV_RR               0x0a
+#define L2CAP_ERTM_EVENT_RECV_REJ              0x0b
+#define L2CAP_ERTM_EVENT_RECV_RNR              0x0c
+#define L2CAP_ERTM_EVENT_RECV_SREJ             0x0d
+#define L2CAP_ERTM_EVENT_RECV_FRAME            0x0e

When we started ERTM we decided to let "_ERTM_" out of the macro names, can we go that way? Shorter names are better.

Also s/EVENT/EV/

The previous ERTM implementation had a handler for each frame type, and each handler had to figure out what the current state was and handle each frame accordingly.

This implementation has a state machine that chooses an execution path first based on the receive or transmit state, then each state has handlers for the frame types. This is easier to match up with the spec, which is defined similarly.

Signed-off-by Mat Martineau <[email protected]>

Hi Mat,

Quoted 10 lines · Mat Martineau

The previous ERTM implementation had a handler for each frame type, and each handler had to figure out what the current state was and handle each frame accordingly.

This implementation has a state machine that chooses an execution path first based on the receive or transmit state, then each state has handlers for the frame types. This is easier to match up with the spec, which is defined similarly.

Signed-off-by Mat Martineau<[email protected]>

<snip>

(quoted diff) +2 -1

@@ -1245,7 +1457,8 @@ int __l2cap_wait_ack(struct sock *sk)
 
         add_wait_queue(sk_sleep(sk),&wait);
         set_current_state(TASK_INTERRUPTIBLE);
-       while (chan->unacked_frames>  0&&  chan->conn) {
+       while (chan->unacked_frames>  0&&  chan->conn&&
+               atomic_read(&chan->ertm_queued)) {
                 if (!timeo)
                         timeo = HZ/5;
 

Can we have unacked_frames> 0 and nothing queued? Or have I misinterpreted?

<snip>

(quoted diff) +4 -1

+               BT_DBG("Sent txseq %d", (int)control->txseq);
 
-               skb = skb_queue_next(&chan->tx_q, skb);
+               chan->next_tx_seq = __next_seq(chan->next_tx_seq, chan);
+               chan->frames_sent += 1;
+               sent += 1;

Nitpick here. frames_sent++ and sent++ ? Happens in other places as well so I won't copy all of them here.

<snip>

(quoted diff) +3 -2

-               if (bt_cb(skb)->retries == 1) {
-                       chan->unacked_frames++;
+               l2cap_chan_hold(chan);
+               sock_hold(chan->sk);
+               tx_skb->sk = chan->sk;

Do we really need these? Do we always have chan->sk? (We have that in l2cap_ertm_send() and l2cap_ertm_resend()).

<snip>

+ keep_sk = schedule_work(&chan->tx_work);

Would make sense to schedule this in hdev->workqueue?

<snip>

+static void l2cap_ertm_tx_worker(struct work_struct *work)
{

Why do we need this worker?

(quoted diff) +6 -6

-       int ret;
+       struct l2cap_chan *chan =
+               container_of(work, struct l2cap_chan, tx_work);
 
-       if (!skb_queue_empty(&chan->tx_q))
-               chan->tx_send_head = chan->tx_q.next;
+       BT_DBG("%p", chan);
 
-       chan->next_tx_seq = chan->expected_ack_seq;
-       ret = l2cap_ertm_send(chan);
-       return ret;
+       lock_sock(chan->sk);
+       l2cap_ertm_send(chan);
+       release_sock(chan->sk);

Can't we just use l2cap_chan_lock()/l2cap_chan_unlock() here? (I'm assuming you saw the patches creating a lock in l2cap_chan to protect it) Do we always have sk?

(quoted diff) +23 -0

+       sock_put(chan->sk);
+       l2cap_chan_put(chan);
  }
 
<snip>
 
+static void l2cap_monitor_timeout(struct work_struct *work)
+{
+       struct l2cap_chan *chan = container_of(work, struct l2cap_chan,
+                                                       monitor_timer.work);
+       struct sock *sk = chan->sk;
+
+       BT_DBG("chan %p", chan);
+
+       lock_sock(sk);
+
+       if (!chan->conn) {
+               release_sock(sk);
+               l2cap_chan_put(chan);
+               return;
+       }
+
+       l2cap_ertm_tx(chan, 0, 0, L2CAP_ERTM_EVENT_MONITOR_TIMER_EXPIRES);
+
+       release_sock(sk);
+       l2cap_chan_put(chan);
+}

Here we might need to use l2cap_chan_lock/unlock instead of lock_sock/release_sock.

(quoted diff) +19 -0

+static void l2cap_retrans_timeout(struct work_struct *work)
+{
+       struct l2cap_chan *chan = container_of(work, struct l2cap_chan,
+                                                       retrans_timer.work);
+       struct sock *sk = chan->sk;
+
+       BT_DBG("chan %p", chan);
+
+       lock_sock(sk);
+
+       if (!chan->conn) {
+               release_sock(sk);
+               l2cap_chan_put(chan);
+               return;
+       }
+
+       l2cap_ertm_tx(chan, 0, 0, L2CAP_ERTM_EVENT_RETRANS_TIMER_EXPIRES);
+       release_sock(sk);
+       l2cap_chan_put(chan);

And here as well.

Ok, that's it for now. Have you run this code through PTS or any other test? I haven't checked the actual bits of ERTM but since we're replacing the current state machine code we need to be somewhat sure this code is as good as or even better than the current one. Introducing too many regressions would be really bad IMHO.

Quoted 31 lines

Hi Mat,

Quoted 11 lines · On Thu, Feb 23, 2012 at 6:37 PM, Mat Martineau<[email protected]> =A0wrote:

The previous ERTM implementation had a handler for each frame type, and each handler had to figure out what the current state was and handle each frame accordingly.

This implementation has a state machine that chooses an execution path first based on the receive or transmit state, then each state has handlers for the frame types. This is easier to match up with the spec, which is defined similarly.

Signed-off-by Mat Martineau<[email protected]>

<snip>

(quoted diff) +2 -1

@@ -1245,7 +1457,8 @@ int __l2cap_wait_ack(struct sock *sk)
 
 =A0 =A0 =A0 =A0 add_wait_queue(sk_sleep(sk),&wait);
 =A0 =A0 =A0 =A0 set_current_state(TASK_INTERRUPTIBLE);
- =A0 =A0 =A0 while (chan->unacked_frames> =A00&& =A0chan->conn) {
 
+ =A0 =A0 =A0 while (chan->unacked_frames> =A00&& =A0chan->conn&&
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 atomic_read(&chan->ertm_queued)) {
 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (!timeo)
 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 timeo =3D HZ/5;

Quoted 2 lines

Can we have unacked_frames> =A00 and nothing queued? Or have I misinterpreted?

In normal operation, there should be unacked frames when ertm_queued is non-zero. =A0I think I ran in to a corner case with AMP, where unacked_fr=

can be forced to 0 during a channel move. =A0There are AMP components to =

state machine that are not included in this patch.

Quoted 8 lines

<snip>

- =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (bt_cb(skb)->retries =3D=3D 1) {
- =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 chan->unacked_frames++;
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 l2cap_chan_hold(chan);
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 sock_hold(chan->sk);
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 tx_skb->sk =3D chan->sk;

Quoted 2 lines

Do we really need these? Do we always have chan->sk? (We have that in l2cap_ertm_send() and l2cap_ertm_resend()).

The upstream ERTM code still relies on having chan->sk, so I didn't try t=

finish splitting channels & sockets within this patch. =A0skb destructors expect to have an sk pointer, so it is critical to modify these reference counts so the socket and chan are around when the skb leaves the hci tx queue.

Quoted 6 lines

<snip>

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 keep_sk =3D schedule_work(&chan->tx_work);

Would make sense to schedule this in hdev->workqueue?

It's a tradeoff. =A0If this is scheduled on hdev->workqueue, then that workqueue can get blocked waiting for the socket lock. =A0If these are scheduled on the system workqueue, there is potential for more latency (b=

it hasn't been a problem in practice, even with AMP data rates).

Quoted 7 lines

<snip>

+static void l2cap_ertm_tx_worker(struct work_struct *work) =A0{

Why do we need this worker?

To control the depth of the hci tx queue. =A0Without this, you end up wit=

entire tx window of iframes queued up at the hci layer. =A0When an sframe shows up from the remote device and you have to retransmit, or when an sframe needs to be sent, then retransmissions and sframes have to get que=

behind that full tx window of iframes. =A0It adds a HUGE amount of latenc=

those situations, which leads to ERTM timeouts and disconnects that are n=

necessary. =A0ERTM works much, much better with lossy connections (like A=

if it does not flood the hci tx queue.

We had a discussion on the list about how to solve this. =A0The idea is t=

push most queuing up to the L2CAP layer, and have the hci scheduler call =

to L2CAP to fetch frames. =A0However, this hasn't been implemented yet.

Quoted 4 lines

- =A0 =A0 =A0 int ret;
+ =A0 =A0 =A0 struct l2cap_chan *chan =3D
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 container_of(work, struct l2cap_chan, tx_w=

Quoted 12 lines

- =A0 =A0 =A0 if (!skb_queue_empty(&chan->tx_q))
- =A0 =A0 =A0 =A0 =A0 =A0 =A0 chan->tx_send_head =3D chan->tx_q.next;
+ =A0 =A0 =A0 BT_DBG("%p", chan);

- =A0 =A0 =A0 chan->next_tx_seq =3D chan->expected_ack_seq;
- =A0 =A0 =A0 ret =3D l2cap_ertm_send(chan);
- =A0 =A0 =A0 return ret;
+ =A0 =A0 =A0 lock_sock(chan->sk);
+ =A0 =A0 =A0 l2cap_ertm_send(chan);
+ =A0 =A0 =A0 release_sock(chan->sk);

Quoted 3 lines

Can't we just use l2cap_chan_lock()/l2cap_chan_unlock() here? (I'm assuming you saw the patches creating a lock in l2cap_chan to protect it) Do we always have sk?

l2cap_chan_lock() is pretty new! =A0Yes, I should use that to guard the E=

state.

Right now, we do still always have sk, but that is changing (as it should=

Review

Quoted 9 lines

+ =A0 =A0 =A0 sock_put(chan->sk);
+ =A0 =A0 =A0 l2cap_chan_put(chan);
=A0}

<snip>

(quoted diff) +3 -0

+static void l2cap_monitor_timeout(struct work_struct *work)
+{
+ =A0 =A0 =A0 struct l2cap_chan *chan =3D container_of(work, struct l2ca=

Quoted 7 lines

+
monitor_timer.work);
+ =A0 =A0 =A0 struct sock *sk =3D chan->sk;

(quoted diff) +4 -0

+
+ =A0 =A0 =A0 BT_DBG("chan %p", chan);
+
+ =A0 =A0 =A0 lock_sock(sk);

Review

Quoted 8 lines

(quoted diff) +8 -0

+
+ =A0 =A0 =A0 if (!chan->conn) {
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 release_sock(sk);
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 l2cap_chan_put(chan);
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 return;
+ =A0 =A0 =A0 }
+
+ =A0 =A0 =A0 l2cap_ertm_tx(chan, 0, 0, L2CAP_ERTM_EVENT_MONITOR_TIMER_E=

Quoted 7 lines

(quoted diff) +4 -0

+
+ =A0 =A0 =A0 release_sock(sk);
+ =A0 =A0 =A0 l2cap_chan_put(chan);
+}

Here we might need to use l2cap_chan_lock/unlock instead of lock_sock/release_sock.

Agreed.

Quoted 1 line

Review

Quoted 3 lines

(quoted diff) +3 -0

+static void l2cap_retrans_timeout(struct work_struct *work)
+{
+ =A0 =A0 =A0 struct l2cap_chan *chan =3D container_of(work, struct l2ca=

Quoted 7 lines

+
retrans_timer.work);
+ =A0 =A0 =A0 struct sock *sk =3D chan->sk;

(quoted diff) +4 -0

+
+ =A0 =A0 =A0 BT_DBG("chan %p", chan);
+
+ =A0 =A0 =A0 lock_sock(sk);

Review

Quoted 8 lines

(quoted diff) +8 -0

+
+ =A0 =A0 =A0 if (!chan->conn) {
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 release_sock(sk);
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 l2cap_chan_put(chan);
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 return;
+ =A0 =A0 =A0 }
+
+ =A0 =A0 =A0 l2cap_ertm_tx(chan, 0, 0, L2CAP_ERTM_EVENT_RETRANS_TIMER_E=

Quoted 10 lines

+ =A0 =A0 =A0 release_sock(sk);
+ =A0 =A0 =A0 l2cap_chan_put(chan);

And here as well.

Ok, that's it for now. Have you run this code through PTS or any other test? I haven't checked the actual bits of ERTM but since we're replacing the current state machine code we need to be somewhat sure this code is as good as or even better than the current one. Introducing too many regressions would be really bad IMHO.

The kernel I'm porting from is qualified, listed, interop'd, UPF'd, and heavily tested -- but I haven't validated this port yet. =A0I will defini=

run this through PTS and test against other ERTM implementations before w=

merge the changes.

Quoted 8 lines

Quoted 8 lines

<snip>

-               if (bt_cb(skb)->retries == 1) {
-                       chan->unacked_frames++;
+               l2cap_chan_hold(chan);
+               sock_hold(chan->sk);
+               tx_skb->sk = chan->sk;

Quoted 8 lines

Quoted 2 lines

Do we really need these? Do we always have chan->sk? (We have that in l2cap_ertm_send() and l2cap_ertm_resend()).

The upstream ERTM code still relies on having chan->sk, so I didn't try to finish splitting channels & sockets within this patch.  skb destructors expect to have an sk pointer, so it is critical to modify these reference counts so the socket and chan are around when the skb leaves the hci tx queue.

If I'm not mistaken the skb destructor relies on sk only to be able to access chan, right? It'd be good if we could not rely on sk here. Andrei, what do you think?

Quoted 19 lines

Quoted 18 lines

-       int ret;
+       struct l2cap_chan *chan =
+               container_of(work, struct l2cap_chan, tx_work);

-       if (!skb_queue_empty(&chan->tx_q))
-               chan->tx_send_head = chan->tx_q.next;
+       BT_DBG("%p", chan);

-       chan->next_tx_seq = chan->expected_ack_seq;
-       ret = l2cap_ertm_send(chan);
-       return ret;
+       lock_sock(chan->sk);
+       l2cap_ertm_send(chan);
+       release_sock(chan->sk);

Can't we just use l2cap_chan_lock()/l2cap_chan_unlock() here? (I'm assuming you saw the patches creating a lock in l2cap_chan to protect it) Do we always have sk?

Quoted 4 lines

l2cap_chan_lock() is pretty new!  Yes, I should use that to guard the ERTM state.

Right now, we do still always have sk, but that is changing (as it should).

Ok. If we could not rely on sk here as well would be good.

Quoted 26 lines · Ulisses Furquim

Hi Mat,

Quoted 10 lines · Mat Martineau

The previous ERTM implementation had a handler for each frame type, and each handler had to figure out what the current state was and handle each frame accordingly.

This implementation has a state machine that chooses an execution path first based on the receive or transmit state, then each state has handlers for the frame types. This is easier to match up with the spec, which is defined similarly.

Signed-off-by Mat Martineau<[email protected]>

<snip>

(quoted diff) +2 -1

@@ -1245,7 +1457,8 @@ int __l2cap_wait_ack(struct sock *sk)
 
        add_wait_queue(sk_sleep(sk),&wait);
        set_current_state(TASK_INTERRUPTIBLE);
-       while (chan->unacked_frames>  0&&  chan->conn) {
+       while (chan->unacked_frames>  0&&  chan->conn&&
+               atomic_read(&chan->ertm_queued)) {
                if (!timeo)
                        timeo = HZ/5;
 

Quoted 1 line

Can we have unacked_frames> 0 and nothing queued? Or have I misinterpreted?

In normal operation, there should be unacked frames when ertm_queued is non-zero. I think I ran in to a corner case with AMP, where unacked_frames can be forced to 0 during a channel move. There are AMP components to the state machine that are not included in this patch.

Quoted 10 lines

<snip>

(quoted diff) +4 -1

+               BT_DBG("Sent txseq %d", (int)control->txseq);
 
-               skb = skb_queue_next(&chan->tx_q, skb);
+               chan->next_tx_seq = __next_seq(chan->next_tx_seq, chan);
+               chan->frames_sent += 1;
+               sent += 1;

Quoted 2 lines

Nitpick here. frames_sent++ and sent++ ? Happens in other places as well so I won't copy all of them here.

Ok, will fix.

Quoted 11 lines

<snip>

(quoted diff) +3 -2

-               if (bt_cb(skb)->retries == 1) {
-                       chan->unacked_frames++;
+               l2cap_chan_hold(chan);
+               sock_hold(chan->sk);
+               tx_skb->sk = chan->sk;

Do we really need these? Do we always have chan->sk? (We have that in l2cap_ertm_send() and l2cap_ertm_resend()).

The upstream ERTM code still relies on having chan->sk, so I didn't try to finish splitting channels & sockets within this patch. skb destructors expect to have an sk pointer, so it is critical to modify these reference counts so the socket and chan are around when the skb leaves the hci tx queue.

Quoted 6 lines

<snip>

+ keep_sk = schedule_work(&chan->tx_work);

Would make sense to schedule this in hdev->workqueue?

It's a tradeoff. If this is scheduled on hdev->workqueue, then that workqueue can get blocked waiting for the socket lock. If these are scheduled on the system workqueue, there is potential for more latency (but it hasn't been a problem in practice, even with AMP data rates).

Quoted 7 lines

<snip>

+static void l2cap_ertm_tx_worker(struct work_struct *work)
{

Why do we need this worker?

To control the depth of the hci tx queue. Without this, you end up with an entire tx window of iframes queued up at the hci layer. When an sframe shows up from the remote device and you have to retransmit, or when an sframe needs to be sent, then retransmissions and sframes have to get queued behind that full tx window of iframes. It adds a HUGE amount of latency in those situations, which leads to ERTM timeouts and disconnects that are not necessary. ERTM works much, much better with lossy connections (like AMP) if it does not flood the hci tx queue.

We had a discussion on the list about how to solve this. The idea is to push most queuing up to the L2CAP layer, and have the hci scheduler call up to L2CAP to fetch frames. However, this hasn't been implemented yet.

Hi Mat,

Quoted 99 lines · Mat Martineau

Quoted 26 lines · Ulisses Furquim

Hi Mat,

Quoted 10 lines · Mat Martineau

The previous ERTM implementation had a handler for each frame type, and each handler had to figure out what the current state was and handle each frame accordingly.

This implementation has a state machine that chooses an execution path first based on the receive or transmit state, then each state has handlers for the frame types. This is easier to match up with the spec, which is defined similarly.

Signed-off-by Mat Martineau<[email protected]>

<snip>

(quoted diff) +2 -1

@@ -1245,7 +1457,8 @@ int __l2cap_wait_ack(struct sock *sk)
 
        add_wait_queue(sk_sleep(sk),&wait);
        set_current_state(TASK_INTERRUPTIBLE);
-       while (chan->unacked_frames>  0&&  chan->conn) {
+       while (chan->unacked_frames>  0&&  chan->conn&&
+               atomic_read(&chan->ertm_queued)) {
                if (!timeo)
                        timeo = HZ/5;
 

Quoted 1 line

Can we have unacked_frames> 0 and nothing queued? Or have I misinterpreted?

In normal operation, there should be unacked frames when ertm_queued is non-zero. I think I ran in to a corner case with AMP, where unacked_frames can be forced to 0 during a channel move. There are AMP components to the state machine that are not included in this patch.

Quoted 10 lines

<snip>

(quoted diff) +4 -1

+               BT_DBG("Sent txseq %d", (int)control->txseq);
 
-               skb = skb_queue_next(&chan->tx_q, skb);
+               chan->next_tx_seq = __next_seq(chan->next_tx_seq, chan);
+               chan->frames_sent += 1;
+               sent += 1;

Quoted 2 lines

Nitpick here. frames_sent++ and sent++ ? Happens in other places as well so I won't copy all of them here.

Ok, will fix.

Quoted 11 lines

<snip>

(quoted diff) +3 -2

-               if (bt_cb(skb)->retries == 1) {
-                       chan->unacked_frames++;
+               l2cap_chan_hold(chan);
+               sock_hold(chan->sk);
+               tx_skb->sk = chan->sk;

Do we really need these? Do we always have chan->sk? (We have that in l2cap_ertm_send() and l2cap_ertm_resend()).

The upstream ERTM code still relies on having chan->sk, so I didn't try to finish splitting channels & sockets within this patch. skb destructors expect to have an sk pointer, so it is critical to modify these reference counts so the socket and chan are around when the skb leaves the hci tx queue.

Quoted 6 lines

<snip>

+ keep_sk = schedule_work(&chan->tx_work);

Would make sense to schedule this in hdev->workqueue?

It's a tradeoff. If this is scheduled on hdev->workqueue, then that workqueue can get blocked waiting for the socket lock. If these are scheduled on the system workqueue, there is potential for more latency (but it hasn't been a problem in practice, even with AMP data rates).

Quoted 7 lines

<snip>

+static void l2cap_ertm_tx_worker(struct work_struct *work)
{

Why do we need this worker?

To control the depth of the hci tx queue. Without this, you end up with an entire tx window of iframes queued up at the hci layer. When an sframe shows up from the remote device and you have to retransmit, or when an sframe needs to be sent, then retransmissions and sframes have to get queued behind that full tx window of iframes. It adds a HUGE amount of latency in those situations, which leads to ERTM timeouts and disconnects that are not necessary. ERTM works much, much better with lossy connections (like AMP) if it does not flood the hci tx queue.

Quoted 4 lines

We had a discussion on the list about how to solve this. The idea is to push most queuing up to the L2CAP layer, and have the hci scheduler call up to L2CAP to fetch frames. However, this hasn't been implemented yet.

Ok, I can see why you added tx_work now. We really need to move to the new proposal here.

Also I think the most important thing here is not point out wrong stuff in this huge patch but find ways to split it in many patches. If we could get a cleaner and rebased patch fixing the unnecessary renames and functions move, etc. that would be good for us. :)

Quoted 101 lines

Quoted 100 lines

Quoted 24 lines · Mat Martineau

Quoted 10 lines

The previous ERTM implementation had a handler for each frame type, and each handler had to figure out what the current state was and handle each frame accordingly.

This implementation has a state machine that chooses an execution path first based on the receive or transmit state, then each state has handlers for the frame types. This is easier to match up with the spec, which is defined similarly.

Signed-off-by Mat Martineau<[email protected]>

<snip>

(quoted diff) +2 -1

@@ -1245,7 +1457,8 @@ int __l2cap_wait_ack(struct sock *sk)
 
        add_wait_queue(sk_sleep(sk),&wait);
        set_current_state(TASK_INTERRUPTIBLE);
-       while (chan->unacked_frames>  0&&  chan->conn) {
+       while (chan->unacked_frames>  0&&  chan->conn&&
+               atomic_read(&chan->ertm_queued)) {
                if (!timeo)
                        timeo = HZ/5;
 

Quoted 1 line

Can we have unacked_frames> 0 and nothing queued? Or have I misinterpreted?

In normal operation, there should be unacked frames when ertm_queued is non-zero. I think I ran in to a corner case with AMP, where unacked_frames can be forced to 0 during a channel move. There are AMP components to the state machine that are not included in this patch.

Quoted 10 lines

<snip>

(quoted diff) +4 -1

+               BT_DBG("Sent txseq %d", (int)control->txseq);
 
-               skb = skb_queue_next(&chan->tx_q, skb);
+               chan->next_tx_seq = __next_seq(chan->next_tx_seq, chan);
+               chan->frames_sent += 1;
+               sent += 1;

Quoted 2 lines

Nitpick here. frames_sent++ and sent++ ? Happens in other places as well so I won't copy all of them here.

Ok, will fix.

Quoted 11 lines

<snip>

(quoted diff) +3 -2

-               if (bt_cb(skb)->retries == 1) {
-                       chan->unacked_frames++;
+               l2cap_chan_hold(chan);
+               sock_hold(chan->sk);
+               tx_skb->sk = chan->sk;

Do we really need these? Do we always have chan->sk? (We have that in l2cap_ertm_send() and l2cap_ertm_resend()).

The upstream ERTM code still relies on having chan->sk, so I didn't try to finish splitting channels & sockets within this patch. skb destructors expect to have an sk pointer, so it is critical to modify these reference counts so the socket and chan are around when the skb leaves the hci tx queue.

Quoted 6 lines

<snip>

+ keep_sk = schedule_work(&chan->tx_work);

Would make sense to schedule this in hdev->workqueue?

It's a tradeoff. If this is scheduled on hdev->workqueue, then that workqueue can get blocked waiting for the socket lock. If these are scheduled on the system workqueue, there is potential for more latency (but it hasn't been a problem in practice, even with AMP data rates).

Quoted 7 lines

<snip>

+static void l2cap_ertm_tx_worker(struct work_struct *work)
{

Why do we need this worker?

To control the depth of the hci tx queue. Without this, you end up with an entire tx window of iframes queued up at the hci layer. When an sframe shows up from the remote device and you have to retransmit, or when an sframe needs to be sent, then retransmissions and sframes have to get queued behind that full tx window of iframes. It adds a HUGE amount of latency in those situations, which leads to ERTM timeouts and disconnects that are not necessary. ERTM works much, much better with lossy connections (like AMP) if it does not flood the hci tx queue.

We had a discussion on the list about how to solve this. The idea is to push most queuing up to the L2CAP layer, and have the hci scheduler call up to L2CAP to fetch frames. However, this hasn't been implemented yet.

Quoted 2 lines

Ok, I can see why you added tx_work now. We really need to move to the new proposal here.

I was hoping we could use this callback method for now, then implement the new proposal. It seems better to have ERTM working well from the start. Both designs have callbacks to L2CAP (this one actually makes fewer callbacks). If you want me to rip out the skb destructor callbacks, I can do it now.

Quoted 4 lines

Also I think the most important thing here is not point out wrong stuff in this huge patch but find ways to split it in many patches. If we could get a cleaner and rebased patch fixing the unnecessary renames and functions move, etc. that would be good for us. :)

Now that I have the input for l2cap.h, I'll update l2cap_core.c and start splitting it up. My understanding from earlier discussion of this ERTM change is that there are a few independent changes that can be added without breaking the current ERTM implementation, but the big changes will have to add inactive code in parallel. After all the code is there, we will make it active and remove the old code. Let me know if I've misunderstood.

Hi Mat,

Quoted 101 lines

Quoted 101 lines

Quoted 100 lines

Quoted 24 lines · Mat Martineau

Quoted 10 lines

The previous ERTM implementation had a handler for each frame type, and each handler had to figure out what the current state was and handle each frame accordingly.

This implementation has a state machine that chooses an execution path first based on the receive or transmit state, then each state has handlers for the frame types. This is easier to match up with the spec, which is defined similarly.

Signed-off-by Mat Martineau<[email protected]>

<snip>

(quoted diff) +2 -1

@@ -1245,7 +1457,8 @@ int __l2cap_wait_ack(struct sock *sk)
 
        add_wait_queue(sk_sleep(sk),&wait);
        set_current_state(TASK_INTERRUPTIBLE);
-       while (chan->unacked_frames>  0&&  chan->conn) {
+       while (chan->unacked_frames>  0&&  chan->conn&&
+               atomic_read(&chan->ertm_queued)) {
                if (!timeo)
                        timeo = HZ/5;
 

Quoted 1 line

Can we have unacked_frames> 0 and nothing queued? Or have I misinterpreted?

In normal operation, there should be unacked frames when ertm_queued is non-zero. I think I ran in to a corner case with AMP, where unacked_frames can be forced to 0 during a channel move. There are AMP components to the state machine that are not included in this patch.

Quoted 10 lines

<snip>

(quoted diff) +4 -1

+               BT_DBG("Sent txseq %d", (int)control->txseq);
 
-               skb = skb_queue_next(&chan->tx_q, skb);
+               chan->next_tx_seq = __next_seq(chan->next_tx_seq, chan);
+               chan->frames_sent += 1;
+               sent += 1;

Quoted 2 lines

Nitpick here. frames_sent++ and sent++ ? Happens in other places as well so I won't copy all of them here.

Ok, will fix.

Quoted 11 lines

<snip>

(quoted diff) +3 -2

-               if (bt_cb(skb)->retries == 1) {
-                       chan->unacked_frames++;
+               l2cap_chan_hold(chan);
+               sock_hold(chan->sk);
+               tx_skb->sk = chan->sk;

Do we really need these? Do we always have chan->sk? (We have that in l2cap_ertm_send() and l2cap_ertm_resend()).

The upstream ERTM code still relies on having chan->sk, so I didn't try to finish splitting channels & sockets within this patch. skb destructors expect to have an sk pointer, so it is critical to modify these reference counts so the socket and chan are around when the skb leaves the hci tx queue.

Quoted 6 lines

<snip>

+ keep_sk = schedule_work(&chan->tx_work);

Would make sense to schedule this in hdev->workqueue?

It's a tradeoff. If this is scheduled on hdev->workqueue, then that workqueue can get blocked waiting for the socket lock. If these are scheduled on the system workqueue, there is potential for more latency (but it hasn't been a problem in practice, even with AMP data rates).

Quoted 7 lines

<snip>

+static void l2cap_ertm_tx_worker(struct work_struct *work)
{

Why do we need this worker?

To control the depth of the hci tx queue. Without this, you end up with an entire tx window of iframes queued up at the hci layer. When an sframe shows up from the remote device and you have to retransmit, or when an sframe needs to be sent, then retransmissions and sframes have to get queued behind that full tx window of iframes. It adds a HUGE amount of latency in those situations, which leads to ERTM timeouts and disconnects that are not necessary. ERTM works much, much better with lossy connections (like AMP) if it does not flood the hci tx queue.

We had a discussion on the list about how to solve this. The idea is to push most queuing up to the L2CAP layer, and have the hci scheduler call up to L2CAP to fetch frames. However, this hasn't been implemented yet.

Quoted 8 lines

Quoted 2 lines

Ok, I can see why you added tx_work now. We really need to move to the new proposal here.

I was hoping we could use this callback method for now, then implement the new proposal. It seems better to have ERTM working well from the start. Both designs have callbacks to L2CAP (this one actually makes fewer callbacks). If you want me to rip out the skb destructor callbacks, I can do it now.

how much effort would it be? I am fine with doing this a little bit later on if this would take too much time. Give me an idea how complicated the change would be at this time.

Quoted 5 lines

Quoted 4 lines

Also I think the most important thing here is not point out wrong stuff in this huge patch but find ways to split it in many patches. If we could get a cleaner and rebased patch fixing the unnecessary renames and functions move, etc. that would be good for us. :)

Quoted 7 lines

Now that I have the input for l2cap.h, I'll update l2cap_core.c and start splitting it up. My understanding from earlier discussion of this ERTM change is that there are a few independent changes that can be added without breaking the current ERTM implementation, but the big changes will have to add inactive code in parallel. After all the code is there, we will make it active and remove the old code. Let me know if I've misunderstood.

That sounds perfectly fine to. Small changes that can be done right now without breaking the current code should go in right away. Like some renaming, moving of code, cleanup and so.

After that, lets add the extra code you need, activate it, and then delete the unused code.