* [PATCH 1/4] Bluetooth: Add definitions and struct members for new ERTM state machine
2012-03-23 23:56 [PATCH 0/4] ERTM state machine changes, part 1 Mat Martineau
@ 2012-03-23 23:56 ` Mat Martineau
2012-03-24 18:30 ` Marcel Holtmann
2012-03-23 23:56 ` [PATCH 2/4] Bluetooth: Add a structure to carry ERTM data in skb control blocks Mat Martineau
` (2 subsequent siblings)
3 siblings, 1 reply; 14+ messages in thread
From: Mat Martineau @ 2012-03-23 23:56 UTC (permalink / raw)
To: linux-bluetooth; +Cc: padovan, pkrystad, marcel, Mat Martineau
Adds some missing values for control field parsing, additional data
for the new state machine, and enumerations for states, incoming
packet classification, and state machine events.
Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
---
include/net/bluetooth/l2cap.h | 49 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 49 insertions(+)
diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index 9b242c6..ed36f00 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -139,6 +139,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 +154,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 +190,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;
@@ -446,6 +452,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;
@@ -456,9 +465,11 @@ struct l2cap_chan {
__u16 buffer_seq;
__u16 buffer_seq_srej;
__u16 srej_save_reqseq;
+ __u16 last_acked_seq;
__u16 frames_sent;
__u16 unacked_frames;
__u8 retry_count;
+ __u16 srej_queue_next;
__u8 num_acked;
__u16 sdu_len;
struct sk_buff *sdu;
@@ -600,6 +611,44 @@ enum {
FLAG_EFS_ENABLE,
};
+enum {
+ L2CAP_TX_STATE_XMIT,
+ L2CAP_TX_STATE_WAIT_F,
+};
+
+enum {
+ L2CAP_RX_STATE_RECV,
+ L2CAP_RX_STATE_SREJ_SENT,
+};
+
+enum {
+ L2CAP_TXSEQ_EXPECTED,
+ L2CAP_TXSEQ_EXPECTED_SREJ,
+ L2CAP_TXSEQ_UNEXPECTED,
+ L2CAP_TXSEQ_UNEXPECTED_SREJ,
+ L2CAP_TXSEQ_DUPLICATE,
+ L2CAP_TXSEQ_DUPLICATE_SREJ,
+ L2CAP_TXSEQ_INVALID,
+ L2CAP_TXSEQ_INVALID_IGNORE,
+};
+
+enum {
+ L2CAP_EV_DATA_REQUEST,
+ L2CAP_EV_LOCAL_BUSY_DETECTED,
+ L2CAP_EV_LOCAL_BUSY_CLEAR,
+ L2CAP_EV_RECV_REQSEQ_AND_FBIT,
+ L2CAP_EV_RECV_FBIT,
+ L2CAP_EV_RETRANS_TO,
+ L2CAP_EV_MONITOR_TO,
+ L2CAP_EV_EXPLICIT_POLL,
+ L2CAP_EV_RECV_IFRAME,
+ L2CAP_EV_RECV_RR,
+ L2CAP_EV_RECV_REJ,
+ L2CAP_EV_RECV_RNR,
+ L2CAP_EV_RECV_SREJ,
+ L2CAP_EV_RECV_FRAME,
+};
+
static inline void l2cap_chan_hold(struct l2cap_chan *c)
{
atomic_inc(&c->refcnt);
--
1.7.9.4
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH 1/4] Bluetooth: Add definitions and struct members for new ERTM state machine
2012-03-23 23:56 ` [PATCH 1/4] Bluetooth: Add definitions and struct members for new ERTM state machine Mat Martineau
@ 2012-03-24 18:30 ` Marcel Holtmann
0 siblings, 0 replies; 14+ messages in thread
From: Marcel Holtmann @ 2012-03-24 18:30 UTC (permalink / raw)
To: Mat Martineau; +Cc: linux-bluetooth, padovan, pkrystad
Hi Mat,
> Adds some missing values for control field parsing, additional data
> for the new state machine, and enumerations for states, incoming
> packet classification, and state machine events.
>
> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
> ---
> include/net/bluetooth/l2cap.h | 49 +++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 49 insertions(+)
looks fine to me. I assume that all entries here will be used in the
future.
Acked-by: Marcel Holtmann <marcel@holtmann.org>
Regards
Marcel
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/4] Bluetooth: Add a structure to carry ERTM data in skb control blocks
2012-03-23 23:56 [PATCH 0/4] ERTM state machine changes, part 1 Mat Martineau
2012-03-23 23:56 ` [PATCH 1/4] Bluetooth: Add definitions and struct members for new ERTM state machine Mat Martineau
@ 2012-03-23 23:56 ` Mat Martineau
2012-03-24 18:34 ` Marcel Holtmann
2012-03-23 23:56 ` [PATCH 3/4] Bluetooth: Add the l2cap_seq_list structure for tracking frames Mat Martineau
2012-03-23 23:56 ` [PATCH 4/4] Bluetooth: Functions parsing or generating ERTM control fields Mat Martineau
3 siblings, 1 reply; 14+ messages in thread
From: Mat Martineau @ 2012-03-23 23:56 UTC (permalink / raw)
To: linux-bluetooth; +Cc: padovan, pkrystad, marcel, Mat Martineau
Every field from ERTM control headers is now carried in the control
block so it only has to be parsed or generated once, and can be
efficiently accessed throughout the ERTM code.
Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
---
include/net/bluetooth/bluetooth.h | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
index 262ebd1..f2c8bdf 100644
--- a/include/net/bluetooth/bluetooth.h
+++ b/include/net/bluetooth/bluetooth.h
@@ -215,6 +215,18 @@ void bt_accept_unlink(struct sock *sk);
struct sock *bt_accept_dequeue(struct sock *parent, struct socket *newsock);
/* Skb helpers */
+struct l2cap_ctrl {
+ unsigned int sframe : 1,
+ poll : 1,
+ final : 1,
+ fcs : 1,
+ sar : 2,
+ super : 2;
+ __u16 reqseq;
+ __u16 txseq;
+ __u8 retries;
+};
+
struct bt_skb_cb {
__u8 pkt_type;
__u8 incoming;
@@ -223,6 +235,7 @@ struct bt_skb_cb {
__u8 retries;
__u8 sar;
__u8 force_active;
+ struct l2cap_ctrl control;
};
#define bt_cb(skb) ((struct bt_skb_cb *)((skb)->cb))
--
1.7.9.4
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH 2/4] Bluetooth: Add a structure to carry ERTM data in skb control blocks
2012-03-23 23:56 ` [PATCH 2/4] Bluetooth: Add a structure to carry ERTM data in skb control blocks Mat Martineau
@ 2012-03-24 18:34 ` Marcel Holtmann
0 siblings, 0 replies; 14+ messages in thread
From: Marcel Holtmann @ 2012-03-24 18:34 UTC (permalink / raw)
To: Mat Martineau; +Cc: linux-bluetooth, padovan, pkrystad
Hi Mat,
> Every field from ERTM control headers is now carried in the control
> block so it only has to be parsed or generated once, and can be
> efficiently accessed throughout the ERTM code.
>
> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
> ---
> include/net/bluetooth/bluetooth.h | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
> diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
> index 262ebd1..f2c8bdf 100644
> --- a/include/net/bluetooth/bluetooth.h
> +++ b/include/net/bluetooth/bluetooth.h
> @@ -215,6 +215,18 @@ void bt_accept_unlink(struct sock *sk);
> struct sock *bt_accept_dequeue(struct sock *parent, struct socket *newsock);
>
> /* Skb helpers */
> +struct l2cap_ctrl {
> + unsigned int sframe : 1,
> + poll : 1,
> + final : 1,
> + fcs : 1,
> + sar : 2,
> + super : 2;
> + __u16 reqseq;
> + __u16 txseq;
> + __u8 retries;
> +};
> +
> struct bt_skb_cb {
> __u8 pkt_type;
> __u8 incoming;
> @@ -223,6 +235,7 @@ struct bt_skb_cb {
> __u8 retries;
> __u8 sar;
> __u8 force_active;
> + struct l2cap_ctrl control;
> };
> #define bt_cb(skb) ((struct bt_skb_cb *)((skb)->cb))
>
would it make sense in the future to split this into bt_skb_cb and
bt_skb_l2cap_cb? However that can be done later.
Acked-by: Marcel Holtmann <marcel@holtmann.org>
Regards
Marcel
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 3/4] Bluetooth: Add the l2cap_seq_list structure for tracking frames
2012-03-23 23:56 [PATCH 0/4] ERTM state machine changes, part 1 Mat Martineau
2012-03-23 23:56 ` [PATCH 1/4] Bluetooth: Add definitions and struct members for new ERTM state machine Mat Martineau
2012-03-23 23:56 ` [PATCH 2/4] Bluetooth: Add a structure to carry ERTM data in skb control blocks Mat Martineau
@ 2012-03-23 23:56 ` Mat Martineau
2012-03-24 18:43 ` Marcel Holtmann
2012-03-23 23:56 ` [PATCH 4/4] Bluetooth: Functions parsing or generating ERTM control fields Mat Martineau
3 siblings, 1 reply; 14+ messages in thread
From: Mat Martineau @ 2012-03-23 23:56 UTC (permalink / raw)
To: linux-bluetooth; +Cc: padovan, pkrystad, marcel, Mat Martineau
A sequence lists is a data structure used to track frames that need to
be retransmitted, and frames that have been requested for
retransmission by the remote device. It can compactly represent a
list of sequence numbers within the ERTM transmit window. Memory for
the list is allocated once at connection time, and common operations
in ERTM are O(1).
Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
---
include/net/bluetooth/l2cap.h | 13 +++++
net/bluetooth/l2cap_core.c | 128 +++++++++++++++++++++++++++++++++++++++++
2 files changed, 141 insertions(+)
diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index ed36f00..bc3c8d5 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -407,6 +407,17 @@ struct l2cap_conn_param_update_rsp {
#define L2CAP_CONN_PARAM_REJECTED 0x0001
/* ----- L2CAP channels and connections ----- */
+struct l2cap_seq_list {
+ __u16 head;
+ __u16 tail;
+ __u16 size;
+ __u16 mask;
+ __u16 *list;
+};
+
+#define L2CAP_SEQ_LIST_CLEAR 0xFFFF
+#define L2CAP_SEQ_LIST_TAIL 0x8000
+
struct srej_list {
__u16 tx_seq;
struct list_head list;
@@ -501,6 +512,8 @@ struct l2cap_chan {
struct sk_buff *tx_send_head;
struct sk_buff_head tx_q;
struct sk_buff_head srej_q;
+ struct l2cap_seq_list srej_list;
+ struct l2cap_seq_list retrans_list;
struct list_head srej_l;
struct list_head list;
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 9c86d73..ad73696 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -233,6 +233,130 @@ static inline void l2cap_chan_set_err(struct l2cap_chan *chan, int err)
release_sock(sk);
}
+static inline struct sk_buff *l2cap_ertm_seq_in_queue(struct sk_buff_head *head,
+ u16 seq)
+{
+ struct sk_buff *skb;
+
+ skb_queue_walk(head, skb) {
+ if (bt_cb(skb)->control.txseq == seq)
+ return skb;
+ }
+
+ return NULL;
+}
+
+static int l2cap_seq_list_init(struct l2cap_seq_list *seq_list, u16 size)
+{
+ u16 allocSize = 1;
+ int err = 0;
+ int i;
+
+ /* Actual allocated size must be a power of 2 */
+ while (allocSize && allocSize <= size)
+ allocSize <<= 1;
+ if (!allocSize)
+ return -ENOMEM;
+
+ seq_list->list = kzalloc(sizeof(u16) * allocSize, GFP_KERNEL);
+ if (!seq_list->list)
+ return -ENOMEM;
+
+ seq_list->size = allocSize;
+ seq_list->mask = allocSize - 1;
+ seq_list->head = L2CAP_SEQ_LIST_CLEAR;
+ seq_list->tail = L2CAP_SEQ_LIST_CLEAR;
+ for (i = 0; i < allocSize; i++)
+ seq_list->list[i] = L2CAP_SEQ_LIST_CLEAR;
+
+ return err;
+}
+
+static inline void l2cap_seq_list_free(struct l2cap_seq_list *seq_list)
+{
+ kfree(seq_list->list);
+}
+
+static inline bool l2cap_seq_list_contains(struct l2cap_seq_list *seq_list,
+ u16 seq)
+{
+ return seq_list->list[seq & seq_list->mask] != L2CAP_SEQ_LIST_CLEAR;
+}
+
+static u16 l2cap_seq_list_remove(struct l2cap_seq_list *seq_list, u16 seq)
+{
+ u16 mask = seq_list->mask;
+
+ BT_DBG("seq_list %p, seq %d", seq_list, (int) seq);
+
+ if (seq_list->head == L2CAP_SEQ_LIST_CLEAR) {
+ /* In case someone tries to pop the head of an empty list */
+ BT_DBG("List empty");
+ return L2CAP_SEQ_LIST_CLEAR;
+ } else if (seq_list->head == seq) {
+ /* Head can be removed quickly */
+ BT_DBG("Remove head");
+ seq_list->head = seq_list->list[seq & mask];
+ seq_list->list[seq & mask] = L2CAP_SEQ_LIST_CLEAR;
+
+ if (seq_list->head == L2CAP_SEQ_LIST_TAIL) {
+ seq_list->head = L2CAP_SEQ_LIST_CLEAR;
+ seq_list->tail = L2CAP_SEQ_LIST_CLEAR;
+ }
+ } else {
+ /* Non-head item must be found first */
+ u16 prev = seq_list->head;
+ BT_DBG("Find and remove");
+ while (seq_list->list[prev & mask] != seq) {
+ prev = seq_list->list[prev & mask];
+ if (prev == L2CAP_SEQ_LIST_TAIL) {
+ BT_DBG("seq %d not in list", (int) seq);
+ return L2CAP_SEQ_LIST_CLEAR;
+ }
+ }
+
+ seq_list->list[prev & mask] = seq_list->list[seq & mask];
+ seq_list->list[seq & mask] = L2CAP_SEQ_LIST_CLEAR;
+ if (seq_list->tail == seq)
+ seq_list->tail = prev;
+ }
+ return seq;
+}
+
+static inline u16 l2cap_seq_list_pop(struct l2cap_seq_list *seq_list)
+{
+ return l2cap_seq_list_remove(seq_list, seq_list->head);
+}
+
+static void l2cap_seq_list_clear(struct l2cap_seq_list *seq_list)
+{
+ if (seq_list->head != L2CAP_SEQ_LIST_CLEAR) {
+ u16 i;
+ for (i = 0; i < seq_list->size; i++)
+ seq_list->list[i] = L2CAP_SEQ_LIST_CLEAR;
+
+ seq_list->head = L2CAP_SEQ_LIST_CLEAR;
+ seq_list->tail = L2CAP_SEQ_LIST_CLEAR;
+ }
+}
+
+static void l2cap_seq_list_append(struct l2cap_seq_list *seq_list, u16 seq)
+{
+ u16 mask = seq_list->mask;
+
+ BT_DBG("seq_list %p, seq %d", seq_list, (int) seq);
+
+ if (seq_list->list[seq & mask] == L2CAP_SEQ_LIST_CLEAR) {
+ if (seq_list->tail == L2CAP_SEQ_LIST_CLEAR)
+ seq_list->head = seq;
+ else
+ seq_list->list[seq_list->tail & mask] = seq;
+
+ seq_list->tail = seq;
+ seq_list->list[seq & mask] = L2CAP_SEQ_LIST_TAIL;
+ }
+}
+
static void l2cap_chan_timeout(struct work_struct *work)
{
struct l2cap_chan *chan = container_of(work, struct l2cap_chan,
@@ -406,6 +530,8 @@ static void l2cap_chan_del(struct l2cap_chan *chan, int err)
skb_queue_purge(&chan->srej_q);
+ l2cap_seq_list_free(&chan->srej_list);
+ l2cap_seq_list_free(&chan->retrans_list);
list_for_each_entry_safe(l, tmp, &chan->srej_l, list) {
list_del(&l->list);
kfree(l);
@@ -2051,6 +2177,8 @@ static inline void l2cap_ertm_init(struct l2cap_chan *chan)
skb_queue_head_init(&chan->srej_q);
+ l2cap_seq_list_init(&chan->srej_list, chan->tx_win);
+ l2cap_seq_list_init(&chan->retrans_list, chan->remote_tx_win);
INIT_LIST_HEAD(&chan->srej_l);
}
--
1.7.9.4
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH 3/4] Bluetooth: Add the l2cap_seq_list structure for tracking frames
2012-03-23 23:56 ` [PATCH 3/4] Bluetooth: Add the l2cap_seq_list structure for tracking frames Mat Martineau
@ 2012-03-24 18:43 ` Marcel Holtmann
2012-03-27 17:54 ` Mat Martineau
0 siblings, 1 reply; 14+ messages in thread
From: Marcel Holtmann @ 2012-03-24 18:43 UTC (permalink / raw)
To: Mat Martineau; +Cc: linux-bluetooth, padovan, pkrystad
Hi Mat,
> A sequence lists is a data structure used to track frames that need to
> be retransmitted, and frames that have been requested for
> retransmission by the remote device. It can compactly represent a
> list of sequence numbers within the ERTM transmit window. Memory for
> the list is allocated once at connection time, and common operations
> in ERTM are O(1).
>
> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
> ---
> include/net/bluetooth/l2cap.h | 13 +++++
> net/bluetooth/l2cap_core.c | 128 +++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 141 insertions(+)
>
> diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
> index ed36f00..bc3c8d5 100644
> --- a/include/net/bluetooth/l2cap.h
> +++ b/include/net/bluetooth/l2cap.h
> @@ -407,6 +407,17 @@ struct l2cap_conn_param_update_rsp {
> #define L2CAP_CONN_PARAM_REJECTED 0x0001
>
> /* ----- L2CAP channels and connections ----- */
> +struct l2cap_seq_list {
> + __u16 head;
> + __u16 tail;
> + __u16 size;
> + __u16 mask;
> + __u16 *list;
> +};
> +
> +#define L2CAP_SEQ_LIST_CLEAR 0xFFFF
> +#define L2CAP_SEQ_LIST_TAIL 0x8000
> +
> struct srej_list {
> __u16 tx_seq;
> struct list_head list;
> @@ -501,6 +512,8 @@ struct l2cap_chan {
> struct sk_buff *tx_send_head;
> struct sk_buff_head tx_q;
> struct sk_buff_head srej_q;
> + struct l2cap_seq_list srej_list;
> + struct l2cap_seq_list retrans_list;
> struct list_head srej_l;
>
> struct list_head list;
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index 9c86d73..ad73696 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -233,6 +233,130 @@ static inline void l2cap_chan_set_err(struct l2cap_chan *chan, int err)
> release_sock(sk);
> }
>
> +static inline struct sk_buff *l2cap_ertm_seq_in_queue(struct sk_buff_head *head,
> + u16 seq)
we better should just stop over using inline in L2CAP and let the
compiler make all the decisions here.
> +{
> + struct sk_buff *skb;
> +
> + skb_queue_walk(head, skb) {
> + if (bt_cb(skb)->control.txseq == seq)
> + return skb;
> + }
> +
> + return NULL;
> +}
> +
> +static int l2cap_seq_list_init(struct l2cap_seq_list *seq_list, u16 size)
> +{
> + u16 allocSize = 1;
Please use alloc_size here. No camel case please.
> + int err = 0;
> + int i;
> +
> + /* Actual allocated size must be a power of 2 */
> + while (allocSize && allocSize <= size)
> + allocSize <<= 1;
> + if (!allocSize)
> + return -ENOMEM;
> +
> + seq_list->list = kzalloc(sizeof(u16) * allocSize, GFP_KERNEL);
> + if (!seq_list->list)
> + return -ENOMEM;
> +
> + seq_list->size = allocSize;
> + seq_list->mask = allocSize - 1;
Is the mask ever changing? If not, we not use a define here instead of
using extra memory.
> + seq_list->head = L2CAP_SEQ_LIST_CLEAR;
> + seq_list->tail = L2CAP_SEQ_LIST_CLEAR;
> + for (i = 0; i < allocSize; i++)
> + seq_list->list[i] = L2CAP_SEQ_LIST_CLEAR;
> +
> + return err;
> +}
> +
> +static inline void l2cap_seq_list_free(struct l2cap_seq_list *seq_list)
> +{
> + kfree(seq_list->list);
> +}
> +
> +static inline bool l2cap_seq_list_contains(struct l2cap_seq_list *seq_list,
> + u16 seq)
> +{
> + return seq_list->list[seq & seq_list->mask] != L2CAP_SEQ_LIST_CLEAR;
> +}
> +
> +static u16 l2cap_seq_list_remove(struct l2cap_seq_list *seq_list, u16 seq)
> +{
> + u16 mask = seq_list->mask;
> +
> + BT_DBG("seq_list %p, seq %d", seq_list, (int) seq);
> +
> + if (seq_list->head == L2CAP_SEQ_LIST_CLEAR) {
> + /* In case someone tries to pop the head of an empty list */
> + BT_DBG("List empty");
> + return L2CAP_SEQ_LIST_CLEAR;
> + } else if (seq_list->head == seq) {
> + /* Head can be removed quickly */
> + BT_DBG("Remove head");
> + seq_list->head = seq_list->list[seq & mask];
> + seq_list->list[seq & mask] = L2CAP_SEQ_LIST_CLEAR;
> +
> + if (seq_list->head == L2CAP_SEQ_LIST_TAIL) {
> + seq_list->head = L2CAP_SEQ_LIST_CLEAR;
> + seq_list->tail = L2CAP_SEQ_LIST_CLEAR;
> + }
> + } else {
> + /* Non-head item must be found first */
> + u16 prev = seq_list->head;
> + BT_DBG("Find and remove");
> + while (seq_list->list[prev & mask] != seq) {
> + prev = seq_list->list[prev & mask];
> + if (prev == L2CAP_SEQ_LIST_TAIL) {
> + BT_DBG("seq %d not in list", (int) seq);
> + return L2CAP_SEQ_LIST_CLEAR;
> + }
> + }
> +
> + seq_list->list[prev & mask] = seq_list->list[seq & mask];
> + seq_list->list[seq & mask] = L2CAP_SEQ_LIST_CLEAR;
> + if (seq_list->tail == seq)
> + seq_list->tail = prev;
> + }
> + return seq;
> +}
> +
> +static inline u16 l2cap_seq_list_pop(struct l2cap_seq_list *seq_list)
> +{
> + return l2cap_seq_list_remove(seq_list, seq_list->head);
> +}
> +
> +static void l2cap_seq_list_clear(struct l2cap_seq_list *seq_list)
> +{
> + if (seq_list->head != L2CAP_SEQ_LIST_CLEAR) {
> + u16 i;
> + for (i = 0; i < seq_list->size; i++)
> + seq_list->list[i] = L2CAP_SEQ_LIST_CLEAR;
> +
> + seq_list->head = L2CAP_SEQ_LIST_CLEAR;
> + seq_list->tail = L2CAP_SEQ_LIST_CLEAR;
> + }
> +}
> +
> +static void l2cap_seq_list_append(struct l2cap_seq_list *seq_list, u16 seq)
> +{
> + u16 mask = seq_list->mask;
> +
> + BT_DBG("seq_list %p, seq %d", seq_list, (int) seq);
> +
> + if (seq_list->list[seq & mask] == L2CAP_SEQ_LIST_CLEAR) {
> + if (seq_list->tail == L2CAP_SEQ_LIST_CLEAR)
> + seq_list->head = seq;
> + else
> + seq_list->list[seq_list->tail & mask] = seq;
> +
> + seq_list->tail = seq;
> + seq_list->list[seq & mask] = L2CAP_SEQ_LIST_TAIL;
> + }
> +}
> +
> static void l2cap_chan_timeout(struct work_struct *work)
> {
> struct l2cap_chan *chan = container_of(work, struct l2cap_chan,
> @@ -406,6 +530,8 @@ static void l2cap_chan_del(struct l2cap_chan *chan, int err)
>
> skb_queue_purge(&chan->srej_q);
>
> + l2cap_seq_list_free(&chan->srej_list);
> + l2cap_seq_list_free(&chan->retrans_list);
> list_for_each_entry_safe(l, tmp, &chan->srej_l, list) {
> list_del(&l->list);
> kfree(l);
> @@ -2051,6 +2177,8 @@ static inline void l2cap_ertm_init(struct l2cap_chan *chan)
>
> skb_queue_head_init(&chan->srej_q);
>
> + l2cap_seq_list_init(&chan->srej_list, chan->tx_win);
> + l2cap_seq_list_init(&chan->retrans_list, chan->remote_tx_win);
> INIT_LIST_HEAD(&chan->srej_l);
> }
>
I think you need to add a bit more documentation on what this is
actually doing, but overall it seems fine to me.
The only question that I still have is if we wanna leave it in l2cap.h
and not just put it in l2cap_core.c. Or if we wanna keep this away from
the core, maybe just create a l2cap_list.h or similar.
Regards
Marcel
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 3/4] Bluetooth: Add the l2cap_seq_list structure for tracking frames
2012-03-24 18:43 ` Marcel Holtmann
@ 2012-03-27 17:54 ` Mat Martineau
0 siblings, 0 replies; 14+ messages in thread
From: Mat Martineau @ 2012-03-27 17:54 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth, padovan, pkrystad
On Sat, 24 Mar 2012, Marcel Holtmann wrote:
> Hi Mat,
>
>> A sequence lists is a data structure used to track frames that need to
>> be retransmitted, and frames that have been requested for
>> retransmission by the remote device. It can compactly represent a
>> list of sequence numbers within the ERTM transmit window. Memory for
>> the list is allocated once at connection time, and common operations
>> in ERTM are O(1).
>>
>> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
>> ---
>> include/net/bluetooth/l2cap.h | 13 +++++
>> net/bluetooth/l2cap_core.c | 128 +++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 141 insertions(+)
>>
>> diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
>> index ed36f00..bc3c8d5 100644
>> --- a/include/net/bluetooth/l2cap.h
>> +++ b/include/net/bluetooth/l2cap.h
>> @@ -407,6 +407,17 @@ struct l2cap_conn_param_update_rsp {
>> #define L2CAP_CONN_PARAM_REJECTED 0x0001
>>
>> /* ----- L2CAP channels and connections ----- */
>> +struct l2cap_seq_list {
>> + __u16 head;
>> + __u16 tail;
>> + __u16 size;
>> + __u16 mask;
>> + __u16 *list;
>> +};
>> +
>> +#define L2CAP_SEQ_LIST_CLEAR 0xFFFF
>> +#define L2CAP_SEQ_LIST_TAIL 0x8000
>> +
>> struct srej_list {
>> __u16 tx_seq;
>> struct list_head list;
>> @@ -501,6 +512,8 @@ struct l2cap_chan {
>> struct sk_buff *tx_send_head;
>> struct sk_buff_head tx_q;
>> struct sk_buff_head srej_q;
>> + struct l2cap_seq_list srej_list;
>> + struct l2cap_seq_list retrans_list;
>> struct list_head srej_l;
>>
>> struct list_head list;
>> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
>> index 9c86d73..ad73696 100644
>> --- a/net/bluetooth/l2cap_core.c
>> +++ b/net/bluetooth/l2cap_core.c
>> @@ -233,6 +233,130 @@ static inline void l2cap_chan_set_err(struct l2cap_chan *chan, int err)
>> release_sock(sk);
>> }
>>
>> +static inline struct sk_buff *l2cap_ertm_seq_in_queue(struct sk_buff_head *head,
>> + u16 seq)
>
> we better should just stop over using inline in L2CAP and let the
> compiler make all the decisions here.
Ok.
>> +{
>> + struct sk_buff *skb;
>> +
>> + skb_queue_walk(head, skb) {
>> + if (bt_cb(skb)->control.txseq == seq)
>> + return skb;
>> + }
>> +
>> + return NULL;
>> +}
>> +
>> +static int l2cap_seq_list_init(struct l2cap_seq_list *seq_list, u16 size)
>> +{
>> + u16 allocSize = 1;
>
> Please use alloc_size here. No camel case please.
Ok.
>> + int err = 0;
>> + int i;
>> +
>> + /* Actual allocated size must be a power of 2 */
>> + while (allocSize && allocSize <= size)
>> + allocSize <<= 1;
>> + if (!allocSize)
>> + return -ENOMEM;
>> +
>> + seq_list->list = kzalloc(sizeof(u16) * allocSize, GFP_KERNEL);
>> + if (!seq_list->list)
>> + return -ENOMEM;
>> +
>> + seq_list->size = allocSize;
>> + seq_list->mask = allocSize - 1;
>
> Is the mask ever changing? If not, we not use a define here instead of
> using extra memory.
'size' is only used in one place, I'll keep the mask and calculate the
size.
>> + seq_list->head = L2CAP_SEQ_LIST_CLEAR;
>> + seq_list->tail = L2CAP_SEQ_LIST_CLEAR;
>> + for (i = 0; i < allocSize; i++)
>> + seq_list->list[i] = L2CAP_SEQ_LIST_CLEAR;
>> +
>> + return err;
>> +}
>> +
>> +static inline void l2cap_seq_list_free(struct l2cap_seq_list *seq_list)
>> +{
>> + kfree(seq_list->list);
>> +}
>> +
>> +static inline bool l2cap_seq_list_contains(struct l2cap_seq_list *seq_list,
>> + u16 seq)
>> +{
>> + return seq_list->list[seq & seq_list->mask] != L2CAP_SEQ_LIST_CLEAR;
>> +}
>> +
>> +static u16 l2cap_seq_list_remove(struct l2cap_seq_list *seq_list, u16 seq)
>> +{
>> + u16 mask = seq_list->mask;
>> +
>> + BT_DBG("seq_list %p, seq %d", seq_list, (int) seq);
>> +
>> + if (seq_list->head == L2CAP_SEQ_LIST_CLEAR) {
>> + /* In case someone tries to pop the head of an empty list */
>> + BT_DBG("List empty");
>> + return L2CAP_SEQ_LIST_CLEAR;
>> + } else if (seq_list->head == seq) {
>> + /* Head can be removed quickly */
>> + BT_DBG("Remove head");
>> + seq_list->head = seq_list->list[seq & mask];
>> + seq_list->list[seq & mask] = L2CAP_SEQ_LIST_CLEAR;
>> +
>> + if (seq_list->head == L2CAP_SEQ_LIST_TAIL) {
>> + seq_list->head = L2CAP_SEQ_LIST_CLEAR;
>> + seq_list->tail = L2CAP_SEQ_LIST_CLEAR;
>> + }
>> + } else {
>> + /* Non-head item must be found first */
>> + u16 prev = seq_list->head;
>> + BT_DBG("Find and remove");
>> + while (seq_list->list[prev & mask] != seq) {
>> + prev = seq_list->list[prev & mask];
>> + if (prev == L2CAP_SEQ_LIST_TAIL) {
>> + BT_DBG("seq %d not in list", (int) seq);
>> + return L2CAP_SEQ_LIST_CLEAR;
>> + }
>> + }
>> +
>> + seq_list->list[prev & mask] = seq_list->list[seq & mask];
>> + seq_list->list[seq & mask] = L2CAP_SEQ_LIST_CLEAR;
>> + if (seq_list->tail == seq)
>> + seq_list->tail = prev;
>> + }
>> + return seq;
>> +}
>> +
>> +static inline u16 l2cap_seq_list_pop(struct l2cap_seq_list *seq_list)
>> +{
>> + return l2cap_seq_list_remove(seq_list, seq_list->head);
>> +}
>> +
>> +static void l2cap_seq_list_clear(struct l2cap_seq_list *seq_list)
>> +{
>> + if (seq_list->head != L2CAP_SEQ_LIST_CLEAR) {
>> + u16 i;
>> + for (i = 0; i < seq_list->size; i++)
>> + seq_list->list[i] = L2CAP_SEQ_LIST_CLEAR;
>> +
>> + seq_list->head = L2CAP_SEQ_LIST_CLEAR;
>> + seq_list->tail = L2CAP_SEQ_LIST_CLEAR;
>> + }
>> +}
>> +
>> +static void l2cap_seq_list_append(struct l2cap_seq_list *seq_list, u16 seq)
>> +{
>> + u16 mask = seq_list->mask;
>> +
>> + BT_DBG("seq_list %p, seq %d", seq_list, (int) seq);
>> +
>> + if (seq_list->list[seq & mask] == L2CAP_SEQ_LIST_CLEAR) {
>> + if (seq_list->tail == L2CAP_SEQ_LIST_CLEAR)
>> + seq_list->head = seq;
>> + else
>> + seq_list->list[seq_list->tail & mask] = seq;
>> +
>> + seq_list->tail = seq;
>> + seq_list->list[seq & mask] = L2CAP_SEQ_LIST_TAIL;
>> + }
>> +}
>> +
>> static void l2cap_chan_timeout(struct work_struct *work)
>> {
>> struct l2cap_chan *chan = container_of(work, struct l2cap_chan,
>> @@ -406,6 +530,8 @@ static void l2cap_chan_del(struct l2cap_chan *chan, int err)
>>
>> skb_queue_purge(&chan->srej_q);
>>
>> + l2cap_seq_list_free(&chan->srej_list);
>> + l2cap_seq_list_free(&chan->retrans_list);
>> list_for_each_entry_safe(l, tmp, &chan->srej_l, list) {
>> list_del(&l->list);
>> kfree(l);
>> @@ -2051,6 +2177,8 @@ static inline void l2cap_ertm_init(struct l2cap_chan *chan)
>>
>> skb_queue_head_init(&chan->srej_q);
>>
>> + l2cap_seq_list_init(&chan->srej_list, chan->tx_win);
>> + l2cap_seq_list_init(&chan->retrans_list, chan->remote_tx_win);
>> INIT_LIST_HEAD(&chan->srej_l);
>> }
>>
>
> I think you need to add a bit more documentation on what this is
> actually doing, but overall it seems fine to me.
I'll add some more comments.
> The only question that I still have is if we wanna leave it in l2cap.h
> and not just put it in l2cap_core.c. Or if we wanna keep this away from
> the core, maybe just create a l2cap_list.h or similar.
There's just the one data structure in l2cap.h, and it is used in
l2cap_chan. The code is all in l2cap_core.c.
Regards,
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 4/4] Bluetooth: Functions parsing or generating ERTM control fields
2012-03-23 23:56 [PATCH 0/4] ERTM state machine changes, part 1 Mat Martineau
` (2 preceding siblings ...)
2012-03-23 23:56 ` [PATCH 3/4] Bluetooth: Add the l2cap_seq_list structure for tracking frames Mat Martineau
@ 2012-03-23 23:56 ` Mat Martineau
2012-03-24 18:48 ` Marcel Holtmann
` (2 more replies)
3 siblings, 3 replies; 14+ messages in thread
From: Mat Martineau @ 2012-03-23 23:56 UTC (permalink / raw)
To: linux-bluetooth; +Cc: padovan, pkrystad, marcel, Mat Martineau
These functions encode or decode ERTM control fields (extended or
enhanced) to or from the new l2cap_control structure.
Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
---
net/bluetooth/l2cap_core.c | 120 ++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 120 insertions(+)
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index ad73696..890cfb9 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -787,6 +787,118 @@ static inline void l2cap_send_rr_or_rnr(struct l2cap_chan *chan, u32 control)
l2cap_send_sframe(chan, control);
}
+static u16 __pack_enhanced_control(struct l2cap_ctrl *control)
+{
+ u16 packed;
+
+ packed = (control->reqseq << L2CAP_CTRL_REQSEQ_SHIFT) &
+ L2CAP_CTRL_REQSEQ;
+ packed |= (control->final << L2CAP_CTRL_FINAL_SHIFT) &
+ L2CAP_CTRL_FINAL;
+
+ if (control->sframe) {
+ packed |= (control->poll << L2CAP_CTRL_POLL_SHIFT) &
+ L2CAP_CTRL_POLL;
+ packed |= (control->super << L2CAP_CTRL_SUPER_SHIFT) &
+ L2CAP_CTRL_SUPERVISE;
+ packed |= L2CAP_CTRL_FRAME_TYPE;
+ } else {
+ packed |= (control->sar << L2CAP_CTRL_SAR_SHIFT) &
+ L2CAP_CTRL_SAR;
+ packed |= (control->txseq << L2CAP_CTRL_TXSEQ_SHIFT) &
+ L2CAP_CTRL_TXSEQ;
+ }
+
+ return packed;
+}
+
+static void __get_enhanced_control(u16 enhanced,
+ struct l2cap_ctrl *control)
+{
+ control->reqseq = (enhanced & L2CAP_CTRL_REQSEQ) >>
+ L2CAP_CTRL_REQSEQ_SHIFT;
+ control->final = (enhanced & L2CAP_CTRL_FINAL) >>
+ L2CAP_CTRL_FINAL_SHIFT;
+
+ if (enhanced & L2CAP_CTRL_FRAME_TYPE) {
+ /* S-Frame */
+ control->sframe = 1;
+ control->poll = (enhanced & L2CAP_CTRL_POLL) >>
+ L2CAP_CTRL_POLL_SHIFT;
+ control->super = (enhanced & L2CAP_CTRL_SUPERVISE) >>
+ L2CAP_CTRL_SUPER_SHIFT;
+
+ control->sar = 0;
+ control->txseq = 0;
+ } else {
+ /* I-Frame */
+ control->sframe = 0;
+ control->sar = (enhanced & L2CAP_CTRL_SAR) >>
+ L2CAP_CTRL_SAR_SHIFT;
+ control->txseq = (enhanced & L2CAP_CTRL_TXSEQ) >>
+ L2CAP_CTRL_TXSEQ_SHIFT;
+
+ control->poll = 0;
+ control->super = 0;
+ }
+}
+
+static u32 __pack_extended_control(struct l2cap_ctrl *control)
+{
+ u32 packed;
+
+ packed = (control->reqseq << L2CAP_EXT_CTRL_REQSEQ_SHIFT) &
+ L2CAP_EXT_CTRL_REQSEQ;
+ packed |= (control->final << L2CAP_EXT_CTRL_FINAL_SHIFT) &
+ L2CAP_EXT_CTRL_FINAL;
+
+ if (control->sframe) {
+ packed |= (control->poll << L2CAP_EXT_CTRL_POLL_SHIFT) &
+ L2CAP_EXT_CTRL_POLL;
+ packed |= (control->super << L2CAP_EXT_CTRL_SUPER_SHIFT) &
+ L2CAP_EXT_CTRL_SUPERVISE;
+ packed |= L2CAP_EXT_CTRL_FRAME_TYPE;
+ } else {
+ packed |= (control->sar << L2CAP_EXT_CTRL_SAR_SHIFT) &
+ L2CAP_EXT_CTRL_SAR;
+ packed |= (control->txseq << L2CAP_EXT_CTRL_TXSEQ_SHIFT) &
+ L2CAP_EXT_CTRL_TXSEQ;
+ }
+
+ return packed;
+}
+
+static void __get_extended_control(u32 extended,
+ struct l2cap_ctrl *control)
+{
+ control->reqseq = (extended & L2CAP_EXT_CTRL_REQSEQ) >>
+ L2CAP_EXT_CTRL_REQSEQ_SHIFT;
+ control->final = (extended & L2CAP_EXT_CTRL_FINAL) >>
+ L2CAP_EXT_CTRL_FINAL_SHIFT;
+
+ if (extended & L2CAP_EXT_CTRL_FRAME_TYPE) {
+ /* S-Frame */
+ control->sframe = 1;
+ control->poll = (extended & L2CAP_EXT_CTRL_POLL) >>
+ L2CAP_EXT_CTRL_POLL_SHIFT;
+ control->super = (extended & L2CAP_EXT_CTRL_SUPERVISE) >>
+ L2CAP_EXT_CTRL_SUPER_SHIFT;
+
+ control->sar = 0;
+ control->txseq = 0;
+ } else {
+ /* I-Frame */
+ control->sframe = 0;
+ control->sar = (extended & L2CAP_EXT_CTRL_SAR) >>
+ L2CAP_EXT_CTRL_SAR_SHIFT;
+ control->txseq = (extended & L2CAP_EXT_CTRL_TXSEQ) >>
+ L2CAP_EXT_CTRL_TXSEQ_SHIFT;
+
+ control->poll = 0;
+ control->super = 0;
+ }
+}
+
static inline int __l2cap_no_conn_pending(struct l2cap_chan *chan)
{
return !test_bit(CONF_CONNECT_PEND, &chan->conf_state);
@@ -4345,6 +4457,14 @@ static int l2cap_ertm_data_rcv(struct l2cap_chan *chan, struct sk_buff *skb)
u16 req_seq;
int len, next_tx_seq_offset, req_seq_offset;
+ if (test_bit(FLAG_EXT_CTRL, &chan->flags)) {
+ __get_extended_control(get_unaligned_le32(skb->data),
+ &bt_cb(skb)->control);
+ } else {
+ __get_enhanced_control(get_unaligned_le16(skb->data),
+ &bt_cb(skb)->control);
+ }
+
control = __get_control(chan, skb->data);
skb_pull(skb, __ctrl_size(chan));
len = skb->len;
--
1.7.9.4
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH 4/4] Bluetooth: Functions parsing or generating ERTM control fields
2012-03-23 23:56 ` [PATCH 4/4] Bluetooth: Functions parsing or generating ERTM control fields Mat Martineau
@ 2012-03-24 18:48 ` Marcel Holtmann
2012-03-25 16:34 ` Gustavo Padovan
2012-03-26 15:20 ` Andrei Emeltchenko
2 siblings, 0 replies; 14+ messages in thread
From: Marcel Holtmann @ 2012-03-24 18:48 UTC (permalink / raw)
To: Mat Martineau; +Cc: linux-bluetooth, padovan, pkrystad
Hi Mat,
> These functions encode or decode ERTM control fields (extended or
> enhanced) to or from the new l2cap_control structure.
>
> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
> ---
> net/bluetooth/l2cap_core.c | 120 ++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 120 insertions(+)
>
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index ad73696..890cfb9 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -787,6 +787,118 @@ static inline void l2cap_send_rr_or_rnr(struct l2cap_chan *chan, u32 control)
> l2cap_send_sframe(chan, control);
> }
>
> +static u16 __pack_enhanced_control(struct l2cap_ctrl *control)
> +{
> + u16 packed;
> +
> + packed = (control->reqseq << L2CAP_CTRL_REQSEQ_SHIFT) &
> + L2CAP_CTRL_REQSEQ;
> + packed |= (control->final << L2CAP_CTRL_FINAL_SHIFT) &
> + L2CAP_CTRL_FINAL;
this looks all good, but with the latest complaints from davem about
coding style, I have no idea on how to format these and similar ones
properly.
So you might have to redo these, but in general all good.
Acked-by: Marcel Holtmann <marcel@holtmann.org>
Regards
Marcel
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 4/4] Bluetooth: Functions parsing or generating ERTM control fields
2012-03-23 23:56 ` [PATCH 4/4] Bluetooth: Functions parsing or generating ERTM control fields Mat Martineau
2012-03-24 18:48 ` Marcel Holtmann
@ 2012-03-25 16:34 ` Gustavo Padovan
2012-03-27 17:57 ` Mat Martineau
2012-03-26 15:20 ` Andrei Emeltchenko
2 siblings, 1 reply; 14+ messages in thread
From: Gustavo Padovan @ 2012-03-25 16:34 UTC (permalink / raw)
To: Mat Martineau; +Cc: linux-bluetooth, pkrystad, marcel
Hi Mat,
* Mat Martineau <mathewm@codeaurora.org> [2012-03-23 16:56:58 -0700]:
> These functions encode or decode ERTM control fields (extended or
> enhanced) to or from the new l2cap_control structure.
>
> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
> ---
> net/bluetooth/l2cap_core.c | 120 ++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 120 insertions(+)
>
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index ad73696..890cfb9 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -787,6 +787,118 @@ static inline void l2cap_send_rr_or_rnr(struct l2cap_chan *chan, u32 control)
> l2cap_send_sframe(chan, control);
> }
>
> +static u16 __pack_enhanced_control(struct l2cap_ctrl *control)
> +{
> + u16 packed;
> +
> + packed = (control->reqseq << L2CAP_CTRL_REQSEQ_SHIFT) &
> + L2CAP_CTRL_REQSEQ;
> + packed |= (control->final << L2CAP_CTRL_FINAL_SHIFT) &
> + L2CAP_CTRL_FINAL;
> +
> + if (control->sframe) {
> + packed |= (control->poll << L2CAP_CTRL_POLL_SHIFT) &
> + L2CAP_CTRL_POLL;
> + packed |= (control->super << L2CAP_CTRL_SUPER_SHIFT) &
> + L2CAP_CTRL_SUPERVISE;
> + packed |= L2CAP_CTRL_FRAME_TYPE;
> + } else {
> + packed |= (control->sar << L2CAP_CTRL_SAR_SHIFT) &
> + L2CAP_CTRL_SAR;
> + packed |= (control->txseq << L2CAP_CTRL_TXSEQ_SHIFT) &
> + L2CAP_CTRL_TXSEQ;
> + }
> +
> + return packed;
> +}
> +
> +static void __get_enhanced_control(u16 enhanced,
> + struct l2cap_ctrl *control)
wrong style here.
> +{
> + control->reqseq = (enhanced & L2CAP_CTRL_REQSEQ) >>
> + L2CAP_CTRL_REQSEQ_SHIFT;
> + control->final = (enhanced & L2CAP_CTRL_FINAL) >>
> + L2CAP_CTRL_FINAL_SHIFT;
> +
> + if (enhanced & L2CAP_CTRL_FRAME_TYPE) {
> + /* S-Frame */
> + control->sframe = 1;
> + control->poll = (enhanced & L2CAP_CTRL_POLL) >>
> + L2CAP_CTRL_POLL_SHIFT;
> + control->super = (enhanced & L2CAP_CTRL_SUPERVISE) >>
> + L2CAP_CTRL_SUPER_SHIFT;
> +
> + control->sar = 0;
> + control->txseq = 0;
> + } else {
> + /* I-Frame */
> + control->sframe = 0;
> + control->sar = (enhanced & L2CAP_CTRL_SAR) >>
> + L2CAP_CTRL_SAR_SHIFT;
> + control->txseq = (enhanced & L2CAP_CTRL_TXSEQ) >>
> + L2CAP_CTRL_TXSEQ_SHIFT;
> +
> + control->poll = 0;
> + control->super = 0;
> + }
> +}
> +
> +static u32 __pack_extended_control(struct l2cap_ctrl *control)
> +{
> + u32 packed;
> +
> + packed = (control->reqseq << L2CAP_EXT_CTRL_REQSEQ_SHIFT) &
> + L2CAP_EXT_CTRL_REQSEQ;
> + packed |= (control->final << L2CAP_EXT_CTRL_FINAL_SHIFT) &
> + L2CAP_EXT_CTRL_FINAL;
> +
> + if (control->sframe) {
> + packed |= (control->poll << L2CAP_EXT_CTRL_POLL_SHIFT) &
> + L2CAP_EXT_CTRL_POLL;
> + packed |= (control->super << L2CAP_EXT_CTRL_SUPER_SHIFT) &
> + L2CAP_EXT_CTRL_SUPERVISE;
> + packed |= L2CAP_EXT_CTRL_FRAME_TYPE;
> + } else {
> + packed |= (control->sar << L2CAP_EXT_CTRL_SAR_SHIFT) &
> + L2CAP_EXT_CTRL_SAR;
> + packed |= (control->txseq << L2CAP_EXT_CTRL_TXSEQ_SHIFT) &
> + L2CAP_EXT_CTRL_TXSEQ;
> + }
> +
> + return packed;
> +}
> +
> +static void __get_extended_control(u32 extended,
> + struct l2cap_ctrl *control)
wrong style here
Gustavo
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 4/4] Bluetooth: Functions parsing or generating ERTM control fields
2012-03-25 16:34 ` Gustavo Padovan
@ 2012-03-27 17:57 ` Mat Martineau
0 siblings, 0 replies; 14+ messages in thread
From: Mat Martineau @ 2012-03-27 17:57 UTC (permalink / raw)
To: Gustavo Padovan; +Cc: linux-bluetooth, pkrystad, marcel
Gustavo -
On Sun, 25 Mar 2012, Gustavo Padovan wrote:
> Hi Mat,
>
> * Mat Martineau <mathewm@codeaurora.org> [2012-03-23 16:56:58 -0700]:
>
>> These functions encode or decode ERTM control fields (extended or
>> enhanced) to or from the new l2cap_control structure.
>>
>> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
>> ---
>> net/bluetooth/l2cap_core.c | 120 ++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 120 insertions(+)
>>
>> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
>> index ad73696..890cfb9 100644
>> --- a/net/bluetooth/l2cap_core.c
>> +++ b/net/bluetooth/l2cap_core.c
>> @@ -787,6 +787,118 @@ static inline void l2cap_send_rr_or_rnr(struct l2cap_chan *chan, u32 control)
>> l2cap_send_sframe(chan, control);
>> }
>>
>> +static u16 __pack_enhanced_control(struct l2cap_ctrl *control)
>> +{
>> + u16 packed;
>> +
>> + packed = (control->reqseq << L2CAP_CTRL_REQSEQ_SHIFT) &
>> + L2CAP_CTRL_REQSEQ;
>> + packed |= (control->final << L2CAP_CTRL_FINAL_SHIFT) &
>> + L2CAP_CTRL_FINAL;
>> +
>> + if (control->sframe) {
>> + packed |= (control->poll << L2CAP_CTRL_POLL_SHIFT) &
>> + L2CAP_CTRL_POLL;
>> + packed |= (control->super << L2CAP_CTRL_SUPER_SHIFT) &
>> + L2CAP_CTRL_SUPERVISE;
>> + packed |= L2CAP_CTRL_FRAME_TYPE;
>> + } else {
>> + packed |= (control->sar << L2CAP_CTRL_SAR_SHIFT) &
>> + L2CAP_CTRL_SAR;
>> + packed |= (control->txseq << L2CAP_CTRL_TXSEQ_SHIFT) &
>> + L2CAP_CTRL_TXSEQ;
>> + }
>> +
>> + return packed;
>> +}
>> +
>> +static void __get_enhanced_control(u16 enhanced,
>> + struct l2cap_ctrl *control)
>
> wrong style here.
>
>> +{
>> + control->reqseq = (enhanced & L2CAP_CTRL_REQSEQ) >>
>> + L2CAP_CTRL_REQSEQ_SHIFT;
>> + control->final = (enhanced & L2CAP_CTRL_FINAL) >>
>> + L2CAP_CTRL_FINAL_SHIFT;
>> +
>> + if (enhanced & L2CAP_CTRL_FRAME_TYPE) {
>> + /* S-Frame */
>> + control->sframe = 1;
>> + control->poll = (enhanced & L2CAP_CTRL_POLL) >>
>> + L2CAP_CTRL_POLL_SHIFT;
>> + control->super = (enhanced & L2CAP_CTRL_SUPERVISE) >>
>> + L2CAP_CTRL_SUPER_SHIFT;
>> +
>> + control->sar = 0;
>> + control->txseq = 0;
>> + } else {
>> + /* I-Frame */
>> + control->sframe = 0;
>> + control->sar = (enhanced & L2CAP_CTRL_SAR) >>
>> + L2CAP_CTRL_SAR_SHIFT;
>> + control->txseq = (enhanced & L2CAP_CTRL_TXSEQ) >>
>> + L2CAP_CTRL_TXSEQ_SHIFT;
>> +
>> + control->poll = 0;
>> + control->super = 0;
>> + }
>> +}
>> +
>> +static u32 __pack_extended_control(struct l2cap_ctrl *control)
>> +{
>> + u32 packed;
>> +
>> + packed = (control->reqseq << L2CAP_EXT_CTRL_REQSEQ_SHIFT) &
>> + L2CAP_EXT_CTRL_REQSEQ;
>> + packed |= (control->final << L2CAP_EXT_CTRL_FINAL_SHIFT) &
>> + L2CAP_EXT_CTRL_FINAL;
>> +
>> + if (control->sframe) {
>> + packed |= (control->poll << L2CAP_EXT_CTRL_POLL_SHIFT) &
>> + L2CAP_EXT_CTRL_POLL;
>> + packed |= (control->super << L2CAP_EXT_CTRL_SUPER_SHIFT) &
>> + L2CAP_EXT_CTRL_SUPERVISE;
>> + packed |= L2CAP_EXT_CTRL_FRAME_TYPE;
>> + } else {
>> + packed |= (control->sar << L2CAP_EXT_CTRL_SAR_SHIFT) &
>> + L2CAP_EXT_CTRL_SAR;
>> + packed |= (control->txseq << L2CAP_EXT_CTRL_TXSEQ_SHIFT) &
>> + L2CAP_EXT_CTRL_TXSEQ;
>> + }
>> +
>> + return packed;
>> +}
>> +
>> +static void __get_extended_control(u32 extended,
>> + struct l2cap_ctrl *control)
>
> wrong style here
>
Thanks for the review - I made a pass over my code to fix up the
function indentation, but I guess I missed these. I'll update and
re-submit.
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 4/4] Bluetooth: Functions parsing or generating ERTM control fields
2012-03-23 23:56 ` [PATCH 4/4] Bluetooth: Functions parsing or generating ERTM control fields Mat Martineau
2012-03-24 18:48 ` Marcel Holtmann
2012-03-25 16:34 ` Gustavo Padovan
@ 2012-03-26 15:20 ` Andrei Emeltchenko
2012-03-27 17:59 ` Mat Martineau
2 siblings, 1 reply; 14+ messages in thread
From: Andrei Emeltchenko @ 2012-03-26 15:20 UTC (permalink / raw)
To: Mat Martineau; +Cc: linux-bluetooth, padovan, pkrystad, marcel
Hi Mat,
On Fri, Mar 23, 2012 at 04:56:58PM -0700, Mat Martineau wrote:
> These functions encode or decode ERTM control fields (extended or
> enhanced) to or from the new l2cap_control structure.
>
> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
> ---
> net/bluetooth/l2cap_core.c | 120 ++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 120 insertions(+)
>
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index ad73696..890cfb9 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -787,6 +787,118 @@ static inline void l2cap_send_rr_or_rnr(struct l2cap_chan *chan, u32 control)
> l2cap_send_sframe(chan, control);
> }
>
> +static u16 __pack_enhanced_control(struct l2cap_ctrl *control)
> +{
> + u16 packed;
> +
> + packed = (control->reqseq << L2CAP_CTRL_REQSEQ_SHIFT) &
> + L2CAP_CTRL_REQSEQ;
> + packed |= (control->final << L2CAP_CTRL_FINAL_SHIFT) &
> + L2CAP_CTRL_FINAL;
Is "& L2CAP_CTRL_FINAL" needed here and for other similar values?
Best regards
Andrei Emeltchenko
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 4/4] Bluetooth: Functions parsing or generating ERTM control fields
2012-03-26 15:20 ` Andrei Emeltchenko
@ 2012-03-27 17:59 ` Mat Martineau
0 siblings, 0 replies; 14+ messages in thread
From: Mat Martineau @ 2012-03-27 17:59 UTC (permalink / raw)
To: Andrei Emeltchenko; +Cc: linux-bluetooth, padovan, pkrystad, marcel
On Mon, 26 Mar 2012, Andrei Emeltchenko wrote:
> Hi Mat,
>
> On Fri, Mar 23, 2012 at 04:56:58PM -0700, Mat Martineau wrote:
>> These functions encode or decode ERTM control fields (extended or
>> enhanced) to or from the new l2cap_control structure.
>>
>> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
>> ---
>> net/bluetooth/l2cap_core.c | 120 ++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 120 insertions(+)
>>
>> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
>> index ad73696..890cfb9 100644
>> --- a/net/bluetooth/l2cap_core.c
>> +++ b/net/bluetooth/l2cap_core.c
>> @@ -787,6 +787,118 @@ static inline void l2cap_send_rr_or_rnr(struct l2cap_chan *chan, u32 control)
>> l2cap_send_sframe(chan, control);
>> }
>>
>> +static u16 __pack_enhanced_control(struct l2cap_ctrl *control)
>> +{
>> + u16 packed;
>> +
>> + packed = (control->reqseq << L2CAP_CTRL_REQSEQ_SHIFT) &
>> + L2CAP_CTRL_REQSEQ;
>> + packed |= (control->final << L2CAP_CTRL_FINAL_SHIFT) &
>> + L2CAP_CTRL_FINAL;
>
> Is "& L2CAP_CTRL_FINAL" needed here and for other similar values?
Since many of the values in l2cap_ctrl are now bitfields, the extra
masking in the __pack functions does seem like overkill.
Thanks,
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
^ permalink raw reply [flat|nested] 14+ messages in thread