[RFC 0/2] New L2CAP ERTM state machine

[RFC 0/2] New L2CAP ERTM state machine

[Mat Martineau]

Here's an update for the ERTM state machine patch. I've made the
changes that were suggested earlier this month, and separated the
header changes and l2cap_core changes.

The first patch is a reasonable size for mailing list discussion, but
the second is too large.  In place of the second patch I'm providing a
link to the change.  Let me know if I should just try to send the
large patch anyway.

You can pull these changes from:
git://codeaurora.org/quic/bluetooth/bluetooth-next.git

Tag name: mathewm-2012-02-23-ertm


Mat Martineau (2):
  Bluetooth: Header changes for ERTM state machine replacement
  Bluetooth: L2CAP ERTM state machine replacement

 include/net/bluetooth/bluetooth.h |   14 +-
 include/net/bluetooth/l2cap.h     |  260 +---
 net/bluetooth/l2cap_core.c        | 2771 +++++++++++++++++++++++++------------
 3 files changed, 1954 insertions(+), 1091 deletions(-)

-- 
1.7.9

--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

[RFC 1/2] Bluetooth: Header changes for ERTM state machine replacement

[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 <mathewm@codeaurora.org>
---
 include/net/bluetooth/bluetooth.h |   14 ++-
 include/net/bluetooth/l2cap.h     |  260 +++++++++----------------------------
 2 files changed, 73 insertions(+), 201 deletions(-)

diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
index 262ebd1..a667cb8 100644
--- a/include/net/bluetooth/bluetooth.h
+++ b/include/net/bluetooth/bluetooth.h
@@ -215,14 +215,24 @@ void bt_accept_unlink(struct sock *sk);
 struct sock *bt_accept_dequeue(struct sock *parent, struct socket *newsock);
 
 /* Skb helpers */
+struct bt_l2cap_control {
+	__u8  frame_type;
+	__u8  final;
+	__u8  sar;
+	__u8  super;
+	__u16 reqseq;
+	__u16 txseq;
+	__u8  poll;
+	__u8  fcs;
+};
+
 struct bt_skb_cb {
 	__u8 pkt_type;
 	__u8 incoming;
 	__u16 expect;
-	__u16 tx_seq;
 	__u8 retries;
-	__u8 sar;
 	__u8 force_active;
+	struct bt_l2cap_control control;
 };
 #define bt_cb(skb) ((struct bt_skb_cb *)((skb)->cb))
 
diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index d6d8ec8..a499b60 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -3,6 +3,7 @@
    Copyright (C) 2000-2001 Qualcomm Incorporated
    Copyright (C) 2009-2010 Gustavo F. Padovan <gustavo@padovan.org>
    Copyright (C) 2010 Google Inc.
+   Copyright (c) 2012 Code Aurora Forum.  All rights reserved.
 
    Written 2000,2001 by Maxim Krasnyansky <maxk@qualcomm.com>
 
@@ -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 ----- */
-struct srej_list {
-	__u16	tx_seq;
-	struct list_head list;
+struct l2cap_seq_list {
+	__u16 head;
+	__u16 tail;
+	__u16 size;
+	__u16 mask;
+	__u16 *list;
 };
 
 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;
@@ -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;
 
 	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))
 #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)
-
-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)
-{
-	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;
-}
+#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
+
+#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
+#define L2CAP_ERTM_EVENT_MONITOR_TIMER_EXPIRES 0x07
+#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
+
+#define __delta_seq(x, y, c) ((x) >= (y) ? (x) - (y) : \
+				(c)->tx_win_max + 1 - (y) + (x))
+#define __next_seq(x, c) ((x + 1) & ((c)->tx_win_max))
 
 extern bool disable_ertm;
 
-- 
1.7.9

--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

Re: [RFC 1/2] Bluetooth: Header changes for ERTM state machine replacement

[Andrei Emeltchenko]

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 sent)

On Thu, Feb 23, 2012 at 12:37:48PM -0800, Mat Martineau wrote:
> 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 <mathewm@codeaurora.org>
> ---
>  include/net/bluetooth/bluetooth.h |   14 ++-
>  include/net/bluetooth/l2cap.h     |  260 +++++++++----------------------------
>  2 files changed, 73 insertions(+), 201 deletions(-)

...

> -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;
> -}

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

...

> -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?

+       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));

...

> -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:

+               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);
+               }

and for example here:

-       __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));


> -
> -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;
> -}

Regards,
Andrei

Re: [RFC 1/2] Bluetooth: Header changes for ERTM state machine replacement

[Ulisses Furquim]

Hi Andrei,

On Fri, Feb 24, 2012 at 7:48 AM, Andrei Emeltchenko
<andrei.emeltchenko.news@gmail.com> wrote:
> 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=
t)
>
> On Thu, Feb 23, 2012 at 12:37:48PM -0800, Mat Martineau wrote:
>> 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 <mathewm@codeaurora.org>
>> ---
>> =A0include/net/bluetooth/bluetooth.h | =A0 14 ++-
>> =A0include/net/bluetooth/l2cap.h =A0 =A0 | =A0260 +++++++++-------------=
---------------
>> =A02 files changed, 73 insertions(+), 201 deletions(-)
>
> ...
>
>> -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?
>
> ...
>
>> -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=
kb->data),
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0control);
> + =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=
kb->data),
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0control);
> + =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));
>
> ...
>
>> -static inline void __put_control(struct l2cap_chan *chan, __u32 control=
,
>> - =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 =A0 =A0 =A0 =A0 =A0 void *p)
>> -{
>> - =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=
xtended_control(control),
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0 skb->data + L2CAP_HDR_SIZE);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 } else {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 put_unaligned_le16(__pack_e=
nhanced_control(control),
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0 skb->data + L2CAP_HDR_SIZE);
> + =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));
>
>
>> -
>> -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;
>> -}

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?

Regards,

--=20
Ulisses Furquim
ProFUSION embedded systems
http://profusion.mobi
Mobile: +55 19 9250 0942
Skype: ulissesffs

Re: [RFC 1/2] Bluetooth: Header changes for ERTM state machine replacement

[Mat Martineau]

Andrei and Ulisses -

On 2/24/2012 9:42 AM, Ulisses Furquim wrote:
> Hi Andrei,
>
> On Fri, Feb 24, 2012 at 7:48 AM, Andrei Emeltchenko
> <andrei.emeltchenko.news@gmail.com>  wrote:
>> Hi Mat,
>>
>> It is better to have normal patches for a better review.

I would have preferred to have more "normal" patches, but I didn't want 
to keep you guys waiting another week.

>> 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?

>>
>> 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.

>>
>> Also check some comments below: (I copied some code from the link you sent)
>>
>> On Thu, Feb 23, 2012 at 12:37:48PM -0800, Mat Martineau wrote:
>>> 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<mathewm@codeaurora.org>
>>> ---
>>>   include/net/bluetooth/bluetooth.h |   14 ++-
>>>   include/net/bluetooth/l2cap.h     |  260 +++++++++----------------------------
>>>   2 files changed, 73 insertions(+), 201 deletions(-)
>>
>> ...
>>
>>> -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;
>>> -}
>>
>> BTW: was it already changed? What is the status with Luiz's patch?

I'm not sure about the patch status - but I do know this function isn't 
needed with the new ERTM code.

>>
>> ...
>>
>>> -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.

>>
>> +       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));
>>
>> ...
>>
>>> -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:
>>
>> +               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);
>> +               }
>>

Since the __pack function differs based on the extended/enhanced header 
type, it doesn't seem worthwhile to check FLAG_EXT_CTRL twice.

The code I'm porting in predates the __put_control macro, so I didn't 
make an effort to remove it.  In order to avoid breaking things, I'm 
trying to only change the ported code where required.  I'm open to 
making this code better, though!

>> and for example here:
>>
>> -       __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));

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.

>>
>>> -
>>> -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?

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!

-- 
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

Re: [RFC 1/2] Bluetooth: Header changes for ERTM state machine replacement

[Ulisses Furquim]

Hi Mat,

On Fri, Feb 24, 2012 at 9:21 PM, Mat Martineau <mathewm@codeaurora.org> wro=
te:
> Andrei and Ulisses -
>
>
> On 2/24/2012 9:42 AM, Ulisses Furquim wrote:
>>
>> Hi Andrei,
>>
>> On Fri, Feb 24, 2012 at 7:48 AM, Andrei Emeltchenko
>> <andrei.emeltchenko.news@gmail.com> =A0wrote:
>>>
>>> Hi Mat,

<snip>

>>> and for example here:
>>>
>>> - =A0 =A0 =A0 __put_control(chan, control, skb_put(skb, __ctrl_size(cha=
n)));
>>> + =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=
e a
> macro should be used in more than one place unless it's hiding some reall=
y
> distracting syntax. =A0In this case, I think it's clearer to see what's g=
oing
> on.

If we don't have a lot of places then I'd rather see what's going on as wel=
l.

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

This would be good to fix.

>>>> -
>>>> -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;
>>>> -}
>>
>> 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=
ore.c
> if they're needed.

Agreed.

Regards,

--=20
Ulisses Furquim
ProFUSION embedded systems
http://profusion.mobi
Mobile: +55 19 9250 0942
Skype: ulissesffs

Re: [RFC 1/2] Bluetooth: Header changes for ERTM state machine replacement

[Andrei Emeltchenko]

Hi all,

On Fri, Feb 24, 2012 at 04:21:39PM -0800, Mat Martineau wrote:
> >>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?

Only if they might be used of course.

> >>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.

Agree, this does not mean of course that the whole code shall be put in 3
patches ;) Otherwise it might be rejected because of huge size.

> >>On Thu, Feb 23, 2012 at 12:37:48PM -0800, Mat Martineau wrote:
> >>>This change affects data structures storing ERTM state and control
> >>>-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.

If this is used only once it is not needed of course.

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

BTW: Here and in all other places please put space after ",".


> >>+               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.

If the code is dealing with control size than it is OK to see what is
going on but if you use this in L2CAP code implementing e.g. ERTM then
this is not that useful.

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

Yes, we should not have them.

> >>>-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?
> 
> 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.

Agree with this.

> Thanks for the reviews!

Best regards 
Andrei Emeltchenko 

Re: [RFC 1/2] Bluetooth: Header changes for ERTM state machine replacement

[Ulisses Furquim]

Hi Mat,

On Thu, Feb 23, 2012 at 6:37 PM, Mat Martineau <mathewm@codeaurora.org> wro=
te:
> 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 <mathewm@codeaurora.org>
> ---
> =A0include/net/bluetooth/bluetooth.h | =A0 14 ++-
> =A0include/net/bluetooth/l2cap.h =A0 =A0 | =A0260 +++++++++--------------=
--------------
> =A02 files changed, 73 insertions(+), 201 deletions(-)

<snip>

> diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.=
h
> index d6d8ec8..a499b60 100644
> --- a/include/net/bluetooth/l2cap.h
> +++ b/include/net/bluetooth/l2cap.h

<snip>

> @@ -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))
> =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?

Regards,

--=20
Ulisses Furquim
ProFUSION embedded systems
http://profusion.mobi
Mobile: +55 19 9250 0942
Skype: ulissesffs

Re: [RFC 1/2] Bluetooth: Header changes for ERTM state machine replacement

[Mat Martineau]

Ulisses -

On 2/24/2012 9:39 AM, Ulisses Furquim wrote:
> Hi Mat,
>
> On Thu, Feb 23, 2012 at 6:37 PM, Mat Martineau<mathewm@codeaurora.org>  wrote:
>> 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<mathewm@codeaurora.org>
>> ---
>>   include/net/bluetooth/bluetooth.h |   14 ++-
>>   include/net/bluetooth/l2cap.h     |  260 +++++++++----------------------------
>>   2 files changed, 73 insertions(+), 201 deletions(-)
>
> <snip>
>
>> diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
>> index d6d8ec8..a499b60 100644
>> --- a/include/net/bluetooth/l2cap.h
>> +++ b/include/net/bluetooth/l2cap.h
>
> <snip>
>
>> @@ -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?

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 
there are no #defines in the l2cap*.c files.  However, I see that many 
other .c 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.  I tried to minimize changes to the ported 
code rather than minimize changes to the upstream code.

-- 
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

Re: [RFC 1/2] Bluetooth: Header changes for ERTM state machine replacement

[Ulisses Furquim]

Hi Mat,

On Fri, Feb 24, 2012 at 9:32 PM, Mat Martineau <mathewm@codeaurora.org> wro=
te:
> Ulisses -
>
>
> On 2/24/2012 9:39 AM, Ulisses Furquim wrote:
>>
>> Hi Mat,
>>
>> On Thu, Feb 23, 2012 at 6:37 PM, Mat Martineau<mathewm@codeaurora.org>
>> =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<mathewm@codeaurora.org>
>>> ---
>>> =A0include/net/bluetooth/bluetooth.h | =A0 14 ++-
>>> =A0include/net/bluetooth/l2cap.h =A0 =A0 | =A0260
>>> +++++++++----------------------------
>>> =A02 files changed, 73 insertions(+), 201 deletions(-)
>>
>>
>> <snip>
>>
>>> diff --git a/include/net/bluetooth/l2cap.h
>>> b/include/net/bluetooth/l2cap.h
>>> index d6d8ec8..a499b60 100644
>>> --- a/include/net/bluetooth/l2cap.h
>>> +++ b/include/net/bluetooth/l2cap.h
>>
>>
>> <snip>
>>
>>> @@ -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)=
)
>>> =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.

You're right, sorry.

> I think all the macros traditionally land in the header files because the=
re
> are no #defines in the l2cap*.c files. =A0However, I see that many other =
.c
> 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=
de
> rather than minimize changes to the upstream code.

Yes, I saw the static functions there and I like it better there than
in l2cap.h, really.

Regards,

--=20
Ulisses Furquim
ProFUSION embedded systems
http://profusion.mobi
Mobile: +55 19 9250 0942
Skype: ulissesffs

Re: [RFC 1/2] Bluetooth: Header changes for ERTM state machine replacement

[Gustavo Padovan]

Hi Mat,

* Mat Martineau <mathewm@codeaurora.org> [2012-02-23 12:37:48 -0800]:

> 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 <mathewm@codeaurora.org>
> ---
>  include/net/bluetooth/bluetooth.h |   14 ++-
>  include/net/bluetooth/l2cap.h     |  260 +++++++++----------------------------
>  2 files changed, 73 insertions(+), 201 deletions(-)
> 
> diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
> index 262ebd1..a667cb8 100644
> --- a/include/net/bluetooth/bluetooth.h
> +++ b/include/net/bluetooth/bluetooth.h
> @@ -215,14 +215,24 @@ void bt_accept_unlink(struct sock *sk);
>  struct sock *bt_accept_dequeue(struct sock *parent, struct socket *newsock);
>  
>  /* Skb helpers */
> +struct bt_l2cap_control {

I would call this struct l2cap_ctrl only. Much smaller.

> +	__u8  frame_type;

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

> +	__u8  final;

final:1

> +	__u8  sar;

sar:2

> +	__u8  super;

super:2 
> +	__u16 reqseq;

go on.. 

> +	__u16 txseq;
> +	__u8  poll;
> +	__u8  fcs;
> +};
> +
>  struct bt_skb_cb {
>  	__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.

> -	__u8 sar;
>  	__u8 force_active;
> +	struct bt_l2cap_control control;
>  };
>  #define bt_cb(skb) ((struct bt_skb_cb *)((skb)->cb))
>  
> diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
> index d6d8ec8..a499b60 100644
> --- a/include/net/bluetooth/l2cap.h
> +++ b/include/net/bluetooth/l2cap.h
> @@ -3,6 +3,7 @@
>     Copyright (C) 2000-2001 Qualcomm Incorporated
>     Copyright (C) 2009-2010 Gustavo F. Padovan <gustavo@padovan.org>
>     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

>  
>     Written 2000,2001 by Maxim Krasnyansky <maxk@qualcomm.com>
>  
> @@ -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 ----- */
> -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.

>  
>  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;
> @@ -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?

>  
>  	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))
>  #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.

> -
> -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)
> -{
> -	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?

> +#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
> +
> +#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

> +#define L2CAP_ERTM_EVENT_MONITOR_TIMER_EXPIRES 0x07
				
			...MONITOR_TO

> +#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/

	Gustavo

Re: [RFC 1/2] Bluetooth: Header changes for ERTM state machine replacement

[Mat Martineau]

On Tue, 28 Feb 2012, Gustavo Padovan wrote:

> Hi Mat,
>
> * Mat Martineau <mathewm@codeaurora.org> [2012-02-23 12:37:48 -0800]:
>
>> 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 <mathewm@codeaurora.org>
>> ---
>>  include/net/bluetooth/bluetooth.h |   14 ++-
>>  include/net/bluetooth/l2cap.h     |  260 +++++++++----------------------------
>>  2 files changed, 73 insertions(+), 201 deletions(-)
>>
>> diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
>> index 262ebd1..a667cb8 100644
>> --- a/include/net/bluetooth/bluetooth.h
>> +++ b/include/net/bluetooth/bluetooth.h
>> @@ -215,14 +215,24 @@ void bt_accept_unlink(struct sock *sk);
>>  struct sock *bt_accept_dequeue(struct sock *parent, struct socket *newsock);
>>
>>  /* Skb helpers */
>> +struct bt_l2cap_control {
>
> I would call this struct l2cap_ctrl only. Much smaller.

Ok.

>> +	__u8  frame_type;
>
> This could be frame_type:1, only one bit is needed here.
>
>> +	__u8  final;
>
> final:1
>
>> +	__u8  sar;
>
> sar:2
>
>> +	__u8  super;
>
> super:2
>> +	__u16 reqseq;
>
> go on..

Will convert to a bitfield.

>> +	__u16 txseq;
>> +	__u8  poll;
>> +	__u8  fcs;
>> +};
>> +
>>  struct bt_skb_cb {
>>  	__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.

Sure.  That l2cap_ctrl struct was originally just ERTM control field 
stuff, but fcs got in there.  retries might as well too.

>> -	__u8 sar;
>>  	__u8 force_active;
>> +	struct bt_l2cap_control control;
>>  };
>>  #define bt_cb(skb) ((struct bt_skb_cb *)((skb)->cb))
>>
>> diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
>> index d6d8ec8..a499b60 100644
>> --- a/include/net/bluetooth/l2cap.h
>> +++ b/include/net/bluetooth/l2cap.h
>> @@ -3,6 +3,7 @@
>>     Copyright (C) 2000-2001 Qualcomm Incorporated
>>     Copyright (C) 2009-2010 Gustavo F. Padovan <gustavo@padovan.org>
>>     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

Ok.

>>
>>     Written 2000,2001 by Maxim Krasnyansky <maxk@qualcomm.com>
>>
>> @@ -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 ----- */
>> -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.

It's pretty useless to have a tx_window any larger than a few hundred 
packets, if you're worried about maximum allocation maybe we should 
limit the the tx_window.

There is a tradeoff here, where we do a one-time allocation to ensure 
that the retransmit and srej lists can be searched in constant time 
and that list nodes don't have to be alloc'd and freed for every 
retransmitted packet.  With typical tx_windows, it's only a few 
hundred bytes of overhead.

>>
>>  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;
>> @@ -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?

I'll address this in my reply to your other email.

>>
>>  	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))
>>  #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.

My goal with this patch was to get feedback on some of the names. 
I'll aim for something independently buildable in the next iteration.

> 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.

Some of the updated functions have more code than seems appropriate 
for a macro, and it seemed best to have them all in one place.

>> -
>> -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)
>> -{
>> -	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?

The state machine code makes frequent use of the l2cap_ctrl struct, so 
all these functions are not needed.  All packing and unpacking of the 
control headers is done one time in one place.

>> +#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
>> +
>> +#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

Ok

>> +#define L2CAP_ERTM_EVENT_MONITOR_TIMER_EXPIRES 0x07
>
> 			...MONITOR_TO

Ok.

>> +#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/

Will do.  Thanks for the review.


--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

[RFC 2/2] Bluetooth: L2CAP ERTM state machine replacement

[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 <mathewm@codeaurora.org>
---

Refer to https://www.codeaurora.org/gitweb/quic/bluetooth/?p=bluetooth-next.git;a=commit;h=bf897e8a45c3eefbb72e5057b0512f4392bffdba

--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

Re: [RFC 2/2] Bluetooth: L2CAP ERTM state machine replacement

[Ulisses Furquim]

Hi Mat,

On Thu, Feb 23, 2012 at 6:37 PM, Mat Martineau <mathewm@codeaurora.org> wrote:
> 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 <mathewm@codeaurora.org>

<snip>

@@ -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>

+               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>

-               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?

-       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?

+       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.

+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.

Thanks,
Regards,

-- 
Ulisses Furquim
ProFUSION embedded systems
http://profusion.mobi
Mobile: +55 19 9250 0942
Skype: ulissesffs

Re: [RFC 2/2] Bluetooth: L2CAP ERTM state machine replacement

[Mat Martineau]

On 2/24/2012 12:13 PM, Ulisses Furquim wrote:
> Hi Mat,
>
> On Thu, Feb 23, 2012 at 6:37 PM, Mat Martineau<mathewm@codeaurora.org>  wrote:
>> 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<mathewm@codeaurora.org>
>
> <snip>
>
> @@ -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?

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.

>
> <snip>
>
> +               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.

Ok, will fix.

>
> <snip>
>
> -               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.

>
> <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).

>
> <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.

>
> -       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?

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).

> +       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.

Agreed.

>
> +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.

The kernel I'm porting from is qualified, listed, interop'd, UPF'd, and 
heavily tested -- but I haven't validated this port yet.  I will 
definitely run this through PTS and test against other ERTM 
implementations before we merge the changes.

-- 
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

Re: [RFC 2/2] Bluetooth: L2CAP ERTM state machine replacement

[Ulisses Furquim]

Hi Mat,

On Fri, Feb 24, 2012 at 10:08 PM, Mat Martineau <mathewm@codeaurora.org> wr=
ote:
> On 2/24/2012 12:13 PM, Ulisses Furquim wrote:
>>
>> Hi Mat,
>>
>> On Thu, Feb 23, 2012 at 6:37 PM, Mat Martineau<mathewm@codeaurora.org>
>> =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<mathewm@codeaurora.org>
>>
>>
>> <snip>
>>
>> @@ -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;
>>
>> 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=
ames
> can be forced to 0 during a channel move. =A0There are AMP components to =
the
> state machine that are not included in this patch.

Ok, I thought you might have a reason for that. Thanks.

>> <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;
>>
>> 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=
o
> 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.

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?

>>
>> <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=
ut
> it hasn't been a problem in practice, even with AMP data rates).

Hmm. I just was wondering if we don't have any problems with tasks in
both worqueues running simultaneously.

>>
>> <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=
h an
> 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=
ued
> behind that full tx window of iframes. =A0It adds a HUGE amount of latenc=
y in
> those situations, which leads to ERTM timeouts and disconnects that are n=
ot
> necessary. =A0ERTM works much, much better with lossy connections (like A=
MP)
> if it does not flood the hci tx queue.

Ok.

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

I see.

>>
>> - =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=
ork);
>>
>> - =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);
>>
>> 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=
RTM
> 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.

>> + =A0 =A0 =A0 sock_put(chan->sk);
>> + =A0 =A0 =A0 l2cap_chan_put(chan);
>> =A0}
>>
>> <snip>
>>
>> +static void l2cap_monitor_timeout(struct work_struct *work)
>> +{
>> + =A0 =A0 =A0 struct l2cap_chan *chan =3D container_of(work, struct l2ca=
p_chan,
>> +
>> monitor_timer.work);
>> + =A0 =A0 =A0 struct sock *sk =3D chan->sk;
>> +
>> + =A0 =A0 =A0 BT_DBG("chan %p", chan);
>> +
>> + =A0 =A0 =A0 lock_sock(sk);
>> +
>> + =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=
XPIRES);
>> +
>> + =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.
>
>
>>
>> +static void l2cap_retrans_timeout(struct work_struct *work)
>> +{
>> + =A0 =A0 =A0 struct l2cap_chan *chan =3D container_of(work, struct l2ca=
p_chan,
>> +
>> retrans_timer.work);
>> + =A0 =A0 =A0 struct sock *sk =3D chan->sk;
>> +
>> + =A0 =A0 =A0 BT_DBG("chan %p", chan);
>> +
>> + =A0 =A0 =A0 lock_sock(sk);
>> +
>> + =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=
XPIRES);
>> + =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=
tely
> run this through PTS and test against other ERTM implementations before w=
e
> merge the changes.

I'm aware your kernel is on a very good state but as you said this is
a port to upstream. And that's why I also said "too many regressions"
because some we might not be able to see now. And running through PTS
and maybe against your qualified kernel would be good, yes.

Thank you,
Best regards,

--=20
Ulisses Furquim
ProFUSION embedded systems
http://profusion.mobi
Mobile: +55 19 9250 0942
Skype: ulissesffs

Re: [RFC 2/2] Bluetooth: L2CAP ERTM state machine replacement

[Andrei Emeltchenko]

Hi all,

On Sat, Feb 25, 2012 at 12:52:04PM -0300, Ulisses Furquim wrote:
> >> <snip>
> >>
> >> -               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.
> 
> 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?

I believe that ERTM should not use sk. The places where we still use sk
shall be corrected IMO.

> >> -       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?
> >
> > 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.

Good that we agree here.

Best regards 
Andrei Emeltchenko 

Re: [RFC 2/2] Bluetooth: L2CAP ERTM state machine replacement

[Gustavo Padovan]

Hi Mat,

* Mat Martineau <mathewm@codeaurora.org> [2012-02-24 17:08:07 -0800]:

> On 2/24/2012 12:13 PM, Ulisses Furquim wrote:
> >Hi Mat,
> >
> >On Thu, Feb 23, 2012 at 6:37 PM, Mat Martineau<mathewm@codeaurora.org>  wrote:
> >>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<mathewm@codeaurora.org>
> >
> ><snip>
> >
> >@@ -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?
> 
> 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.
> 
> >
> ><snip>
> >
> >+               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.
> 
> Ok, will fix.
> 
> >
> ><snip>
> >
> >-               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.
> 
> >
> ><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).
> 
> >
> ><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.

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. :)

	Gustavo

Re: [RFC 2/2] Bluetooth: L2CAP ERTM state machine replacement

[Mat Martineau]

On Tue, 28 Feb 2012, Gustavo Padovan wrote:

> Hi Mat,
>
> * Mat Martineau <mathewm@codeaurora.org> [2012-02-24 17:08:07 -0800]:
>
>> On 2/24/2012 12:13 PM, Ulisses Furquim wrote:
>>> Hi Mat,
>>>
>>> On Thu, Feb 23, 2012 at 6:37 PM, Mat Martineau<mathewm@codeaurora.org>  wrote:
>>>> 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<mathewm@codeaurora.org>
>>>
>>> <snip>
>>>
>>> @@ -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?
>>
>> 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.
>>
>>>
>>> <snip>
>>>
>>> +               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.
>>
>> Ok, will fix.
>>
>>>
>>> <snip>
>>>
>>> -               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.
>>
>>>
>>> <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).
>>
>>>
>>> <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.
>
> 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.

> 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.

--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

Re: [RFC 2/2] Bluetooth: L2CAP ERTM state machine replacement

[Marcel Holtmann]

Hi Mat,

> >>> On Thu, Feb 23, 2012 at 6:37 PM, Mat Martineau<mathewm@codeaurora.org>  wrote:
> >>>> 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<mathewm@codeaurora.org>
> >>>
> >>> <snip>
> >>>
> >>> @@ -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?
> >>
> >> 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.
> >>
> >>>
> >>> <snip>
> >>>
> >>> +               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.
> >>
> >> Ok, will fix.
> >>
> >>>
> >>> <snip>
> >>>
> >>> -               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.
> >>
> >>>
> >>> <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).
> >>
> >>>
> >>> <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.
> >
> > 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.

> > 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.

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.

Regards

Marcel

Re: [RFC 2/2] Bluetooth: L2CAP ERTM state machine replacement

[Mat Martineau]

On Fri, 2 Mar 2012, Marcel Holtmann wrote:

> Hi Mat,
>
>>>>> On Thu, Feb 23, 2012 at 6:37 PM, Mat Martineau<mathewm@codeaurora.org>  wrote:
>>>>>> 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<mathewm@codeaurora.org>
>>>>>
>>>>> <snip>
>>>>>
>>>>> @@ -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?
>>>>
>>>> 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.
>>>>
>>>>>
>>>>> <snip>
>>>>>
>>>>> +               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.
>>>>
>>>> Ok, will fix.
>>>>
>>>>>
>>>>> <snip>
>>>>>
>>>>> -               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.
>>>>
>>>>>
>>>>> <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).
>>>>
>>>>>
>>>>> <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.
>>>
>>> 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.

It's easy to remove, but hard to replace.  ERTM does not work well 
with packet loss if this code is removed and HCI queuing is left 
alone.

>>> 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.
>
> 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.

Ok.


Regards,

--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum