Re: [RFC 1/2] Bluetooth: Header changes for ERTM state machine replacement
From: Gustavo Padovan <hidden>
Date: 2012-02-28 23:33:05
Hi Mat, * Mat Martineau [off-list ref] [2012-02-23 12:37:48 -0800]:
quoted hunk ↗ jump to hunk
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 <redacted> --- 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.
quoted hunk ↗ jump to hunk
- __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
quoted hunk ↗ jump to hunk
Written 2000,2001 by Maxim Krasnyansky [off-list ref]@@ -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.
quoted hunk ↗ jump to hunk
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?
quoted hunk ↗ jump to hunk
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