[PATCH 0/4] ERTM state machine changes, part 2

[PATCH 0/4] ERTM state machine changes, part 2

[Mat Martineau]

This is the second patch series reworking the ERTM state machine and
closely related streaming mode code.  These patches include bug fixes
and segmentation of outgoing L2CAP data.

RFCv1:   Four of eight patches were merged.
RFCv2:   Fixed the send lock patch, found and fixed a few more issues
         with locking, reference counting, and unused code.  Four of
         eight patches were merged.
PATCHv1: Confirmed ERTM operation with locking and segmentation updates


Mat Martineau (4):
  Bluetooth: Fix a redundant and problematic incoming MTU check
  Bluetooth: Restore locking semantics when looking up L2CAP channels
  Bluetooth: Lock the L2CAP channel when sending
  Bluetooth: Refactor L2CAP ERTM and streaming transmit segmentation

 include/net/bluetooth/bluetooth.h |    2 -
 include/net/bluetooth/l2cap.h     |    1 +
 net/bluetooth/l2cap_core.c        |  184 +++++++++++++++++++------------------
 net/bluetooth/l2cap_sock.c        |   15 +--
 4 files changed, 103 insertions(+), 99 deletions(-)

-- 
1.7.10

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

[PATCH 1/4] Bluetooth: Fix a redundant and problematic incoming MTU check

[Mat Martineau]

The L2CAP MTU for incoming data is verified differently depending on
the L2CAP mode, so the check is best performed in a mode-specific
context.  Checking the incoming MTU before HCI fragment reassembly is
a layer violation and assumes all bytes after the standard L2CAP
header are L2CAP data.

This approach causes issues with unsegmented ERTM or streaming mode
frames, where there are additional enhanced or extended headers before
the data payload and possible FCS bytes after the data payload.  A
valid frame could be as many as 10 bytes larger than the MTU.

Removing this code is the best fix, because the MTU is checked later
on for all L2CAP data frames (connectionless, basic, ERTM, and
streaming).  This also gets rid of outdated locking (socket instead of
l2cap_chan) and an extra lookup of the channel ID.

Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
---
 net/bluetooth/l2cap_core.c |   20 --------------------
 1 file changed, 20 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 870116a..5d556b0 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -4953,8 +4953,6 @@ int l2cap_recv_acldata(struct hci_conn *hcon, struct sk_buff *skb, u16 flags)
 
 	if (!(flags & ACL_CONT)) {
 		struct l2cap_hdr *hdr;
-		struct l2cap_chan *chan;
-		u16 cid;
 		int len;
 
 		if (conn->rx_len) {
@@ -4974,7 +4972,6 @@ int l2cap_recv_acldata(struct hci_conn *hcon, struct sk_buff *skb, u16 flags)
 
 		hdr = (struct l2cap_hdr *) skb->data;
 		len = __le16_to_cpu(hdr->len) + L2CAP_HDR_SIZE;
-		cid = __le16_to_cpu(hdr->cid);
 
 		if (len == skb->len) {
 			/* Complete frame received */
@@ -4991,23 +4988,6 @@ int l2cap_recv_acldata(struct hci_conn *hcon, struct sk_buff *skb, u16 flags)
 			goto drop;
 		}
 
-		chan = l2cap_get_chan_by_scid(conn, cid);
-
-		if (chan && chan->sk) {
-			struct sock *sk = chan->sk;
-			lock_sock(sk);
-
-			if (chan->imtu < len - L2CAP_HDR_SIZE) {
-				BT_ERR("Frame exceeding recv MTU (len %d, "
-							"MTU %d)", len,
-							chan->imtu);
-				release_sock(sk);
-				l2cap_conn_unreliable(conn, ECOMM);
-				goto drop;
-			}
-			release_sock(sk);
-		}
-
 		/* Allocate skb for the complete frame (with header) */
 		conn->rx_skb = bt_skb_alloc(len, GFP_ATOMIC);
 		if (!conn->rx_skb)
-- 
1.7.10

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

[PATCH 2/4] Bluetooth: Restore locking semantics when looking up L2CAP channels

[Mat Martineau]

As the comment for l2cap_get_chan_by_scid indicated, the function used
to return a locked socket.  The lock for the socket was acquired while
the channel list was also locked.

When locking was moved over to the l2cap_chan structure, the channel
lock was no longer acquired with the channel list still locked.  This
made it possible for the l2cap_chan to be deleted after
conn->chan_lock was released but before l2cap_chan_lock was called.
Making the call to l2cap_chan_lock before releasing conn->chan_lock
makes it impossible for the l2cap_chan to be deleted at the wrong
time.

Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
---
 net/bluetooth/l2cap_core.c |   10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 5d556b0..813cf06 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -98,13 +98,15 @@ static struct l2cap_chan *__l2cap_get_chan_by_scid(struct l2cap_conn *conn, u16
 }
 
 /* Find channel with given SCID.
- * Returns locked socket */
+ * Returns locked channel. */
 static struct l2cap_chan *l2cap_get_chan_by_scid(struct l2cap_conn *conn, u16 cid)
 {
 	struct l2cap_chan *c;
 
 	mutex_lock(&conn->chan_lock);
 	c = __l2cap_get_chan_by_scid(conn, cid);
+	if (c)
+		l2cap_chan_lock(c);
 	mutex_unlock(&conn->chan_lock);
 
 	return c;
@@ -3141,8 +3143,6 @@ static inline int l2cap_config_req(struct l2cap_conn *conn, struct l2cap_cmd_hdr
 	if (!chan)
 		return -ENOENT;
 
-	l2cap_chan_lock(chan);
-
 	if (chan->state != BT_CONFIG && chan->state != BT_CONNECT2) {
 		struct l2cap_cmd_rej_cid rej;
 
@@ -3255,8 +3255,6 @@ static inline int l2cap_config_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hdr
 	if (!chan)
 		return 0;
 
-	l2cap_chan_lock(chan);
-
 	switch (result) {
 	case L2CAP_CONF_SUCCESS:
 		l2cap_conf_rfc_get(chan, rsp->data, len);
@@ -4589,8 +4587,6 @@ static inline int l2cap_data_channel(struct l2cap_conn *conn, u16 cid, struct sk
 		return 0;
 	}
 
-	l2cap_chan_lock(chan);
-
 	BT_DBG("chan %p, len %d", chan, skb->len);
 
 	if (chan->state != BT_CONNECTED)
-- 
1.7.10

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

[PATCH 3/4] Bluetooth: Lock the L2CAP channel when sending

[Mat Martineau]

The ERTM and streaming mode transmit queue must only be accessed while
the L2CAP channel lock is held.  Locking the channel before calling
l2cap_chan_send ensures that multiple threads cannot simultaneously
manipulate the queue when sending and receiving concurrently.

L2CAP channel locking had previously moved to the l2cap_chan struct
instead of the associated socket, so some of the old socket locking
can also be removed in this patch.

Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
---
 include/net/bluetooth/bluetooth.h |    2 --
 net/bluetooth/l2cap_sock.c        |   15 ++++++++-------
 2 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
index 2fb268f..90678a9 100644
--- a/include/net/bluetooth/bluetooth.h
+++ b/include/net/bluetooth/bluetooth.h
@@ -256,12 +256,10 @@ static inline struct sk_buff *bt_skb_send_alloc(struct sock *sk,
 {
 	struct sk_buff *skb;
 
-	release_sock(sk);
 	if ((skb = sock_alloc_send_skb(sk, len + BT_SKB_RESERVE, nb, err))) {
 		skb_reserve(skb, BT_SKB_RESERVE);
 		bt_cb(skb)->incoming  = 0;
 	}
-	lock_sock(sk);
 
 	if (!skb && *err)
 		return NULL;
diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index 82b6368..ac8ce10 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -716,16 +716,13 @@ static int l2cap_sock_sendmsg(struct kiocb *iocb, struct socket *sock, struct ms
 	if (msg->msg_flags & MSG_OOB)
 		return -EOPNOTSUPP;
 
-	lock_sock(sk);
-
-	if (sk->sk_state != BT_CONNECTED) {
-		release_sock(sk);
+	if (sk->sk_state != BT_CONNECTED)
 		return -ENOTCONN;
-	}
 
+	l2cap_chan_lock(chan);
 	err = l2cap_chan_send(chan, msg, len, sk->sk_priority);
+	l2cap_chan_unlock(chan);
 
-	release_sock(sk);
 	return err;
 }
 
@@ -936,9 +933,13 @@ static struct sk_buff *l2cap_sock_alloc_skb_cb(struct l2cap_chan *chan,
 	struct sk_buff *skb;
 	int err;
 
+	l2cap_chan_unlock(chan);
+
 	skb = bt_skb_send_alloc(chan->sk, len, nb, &err);
 	if (!skb)
-		return (ERR_PTR(err));
+		skb = ERR_PTR(err);
+
+	l2cap_chan_lock(chan);
 
 	return skb;
 }
-- 
1.7.10

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

[PATCH 4/4] Bluetooth: Refactor L2CAP ERTM and streaming transmit segmentation

[Mat Martineau]

Use more common code for ERTM and streaming mode segmentation and
transmission, and begin using skb control block data for delaying
extended or enhanced header generation until just before the packet is
transmitted.  This code is also better suited for resegmentation,
which is needed when L2CAP links are reconfigured after an AMP channel
move.

Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
---
 include/net/bluetooth/l2cap.h |    1 +
 net/bluetooth/l2cap_core.c    |  154 ++++++++++++++++++++++++-----------------
 2 files changed, 92 insertions(+), 63 deletions(-)

diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index 92c0423..88c128a 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -44,6 +44,7 @@
 #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_DISC_TIMEOUT		msecs_to_jiffies(100)
 #define L2CAP_DISC_REJ_TIMEOUT		msecs_to_jiffies(5000)
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 813cf06..5e3d016 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -1636,6 +1636,7 @@ static void l2cap_streaming_send(struct l2cap_chan *chan)
 	while ((skb = skb_dequeue(&chan->tx_q))) {
 		control = __get_control(chan, skb->data + L2CAP_HDR_SIZE);
 		control |= __set_txseq(chan, chan->next_tx_seq);
+		control |= __set_ctrl_sar(chan, bt_cb(skb)->control.sar);
 		__put_control(chan, control, skb->data + L2CAP_HDR_SIZE);
 
 		if (chan->fcs == L2CAP_FCS_CRC16) {
@@ -1708,6 +1709,9 @@ static int l2cap_ertm_send(struct l2cap_chan *chan)
 	if (chan->state != BT_CONNECTED)
 		return -ENOTCONN;
 
+	if (test_bit(CONN_REMOTE_BUSY, &chan->conn_state))
+		return 0;
+
 	while ((skb = chan->tx_send_head) && (!l2cap_tx_window_full(chan))) {
 
 		if (chan->remote_max_tx &&
@@ -1728,6 +1732,7 @@ static int l2cap_ertm_send(struct l2cap_chan *chan)
 
 		control |= __set_reqseq(chan, chan->buffer_seq);
 		control |= __set_txseq(chan, chan->next_tx_seq);
+		control |= __set_ctrl_sar(chan, bt_cb(skb)->control.sar);
 
 		__put_control(chan, control, tx_skb->data + L2CAP_HDR_SIZE);
 
@@ -1923,7 +1928,7 @@ static struct sk_buff *l2cap_create_basic_pdu(struct l2cap_chan *chan,
 
 static struct sk_buff *l2cap_create_iframe_pdu(struct l2cap_chan *chan,
 						struct msghdr *msg, size_t len,
-						u32 control, u16 sdulen)
+						u16 sdulen)
 {
 	struct l2cap_conn *conn = chan->conn;
 	struct sk_buff *skb;
@@ -1958,7 +1963,7 @@ static struct sk_buff *l2cap_create_iframe_pdu(struct l2cap_chan *chan,
 	lh->cid = cpu_to_le16(chan->dcid);
 	lh->len = cpu_to_le16(len + (hlen - L2CAP_HDR_SIZE));
 
-	__put_control(chan, control, skb_put(skb, __ctrl_size(chan)));
+	__put_control(chan, 0, skb_put(skb, __ctrl_size(chan)));
 
 	if (sdulen)
 		put_unaligned_le16(sdulen, skb_put(skb, L2CAP_SDULEN_SIZE));
@@ -1976,57 +1981,78 @@ static struct sk_buff *l2cap_create_iframe_pdu(struct l2cap_chan *chan,
 	return skb;
 }
 
-static int l2cap_sar_segment_sdu(struct l2cap_chan *chan, struct msghdr *msg, size_t len)
+static int l2cap_segment_sdu(struct l2cap_chan *chan,
+			     struct sk_buff_head *seg_queue,
+			     struct msghdr *msg, size_t len)
 {
 	struct sk_buff *skb;
-	struct sk_buff_head sar_queue;
-	u32 control;
-	size_t size = 0;
+	u16 sdu_len;
+	size_t pdu_len;
+	int err = 0;
+	u8 sar;
 
-	skb_queue_head_init(&sar_queue);
-	control = __set_ctrl_sar(chan, L2CAP_SAR_START);
-	skb = l2cap_create_iframe_pdu(chan, msg, chan->remote_mps, control, len);
-	if (IS_ERR(skb))
-		return PTR_ERR(skb);
+	BT_DBG("chan %p, msg %p, len %d", chan, msg, (int)len);
 
-	__skb_queue_tail(&sar_queue, skb);
-	len -= chan->remote_mps;
-	size += chan->remote_mps;
+	/* It is critical that ERTM PDUs fit in a single HCI fragment,
+	 * so fragmented skbs are not used.  The HCI layer's handling
+	 * of fragmented skbs is not compatible with ERTM's queueing.
+	 */
+
+	/* PDU size is derived from the HCI MTU */
+	pdu_len = chan->conn->mtu;
+
+	pdu_len = min_t(size_t, pdu_len, L2CAP_BREDR_MAX_PAYLOAD);
+
+	/* Adjust for largest possible L2CAP overhead. */
+	pdu_len -= L2CAP_EXT_HDR_SIZE + L2CAP_FCS_SIZE;
+
+	/* Remote device may have requested smaller PDUs */
+	pdu_len = min_t(size_t, pdu_len, chan->remote_mps);
+
+	if (len <= pdu_len) {
+		sar = L2CAP_SAR_UNSEGMENTED;
+		sdu_len = 0;
+		pdu_len = len;
+	} else {
+		sar = L2CAP_SAR_START;
+		sdu_len = len;
+		pdu_len -= L2CAP_SDULEN_SIZE;
+	}
 
 	while (len > 0) {
-		size_t buflen;
+		skb = l2cap_create_iframe_pdu(chan, msg, pdu_len, sdu_len);
 
-		if (len > chan->remote_mps) {
-			control = __set_ctrl_sar(chan, L2CAP_SAR_CONTINUE);
-			buflen = chan->remote_mps;
-		} else {
-			control = __set_ctrl_sar(chan, L2CAP_SAR_END);
-			buflen = len;
-		}
-
-		skb = l2cap_create_iframe_pdu(chan, msg, buflen, control, 0);
 		if (IS_ERR(skb)) {
-			skb_queue_purge(&sar_queue);
+			__skb_queue_purge(seg_queue);
 			return PTR_ERR(skb);
 		}
 
-		__skb_queue_tail(&sar_queue, skb);
-		len -= buflen;
-		size += buflen;
-	}
-	skb_queue_splice_tail(&sar_queue, &chan->tx_q);
-	if (chan->tx_send_head == NULL)
-		chan->tx_send_head = sar_queue.next;
+		bt_cb(skb)->control.sar = sar;
+		__skb_queue_tail(seg_queue, skb);
 
-	return size;
+		len -= pdu_len;
+		if (sdu_len) {
+			sdu_len = 0;
+			pdu_len += L2CAP_SDULEN_SIZE;
+		}
+
+		if (len <= pdu_len) {
+			sar = L2CAP_SAR_END;
+			pdu_len = len;
+		} else {
+			sar = L2CAP_SAR_CONTINUE;
+		}
+	}
+
+	return err;
 }
 
 int l2cap_chan_send(struct l2cap_chan *chan, struct msghdr *msg, size_t len,
 								u32 priority)
 {
 	struct sk_buff *skb;
-	u32 control;
 	int err;
+	struct sk_buff_head seg_queue;
 
 	/* Connectionless channel */
 	if (chan->chan_type == L2CAP_CHAN_CONN_LESS) {
@@ -2055,42 +2081,44 @@ int l2cap_chan_send(struct l2cap_chan *chan, struct msghdr *msg, size_t len,
 
 	case L2CAP_MODE_ERTM:
 	case L2CAP_MODE_STREAMING:
-		/* Entire SDU fits into one PDU */
-		if (len <= chan->remote_mps) {
-			control = __set_ctrl_sar(chan, L2CAP_SAR_UNSEGMENTED);
-			skb = l2cap_create_iframe_pdu(chan, msg, len, control,
-									0);
-			if (IS_ERR(skb))
-				return PTR_ERR(skb);
-
-			__skb_queue_tail(&chan->tx_q, skb);
-
-			if (chan->tx_send_head == NULL)
-				chan->tx_send_head = skb;
-
-		} else {
-			/* Segment SDU into multiples PDUs */
-			err = l2cap_sar_segment_sdu(chan, msg, len);
-			if (err < 0)
-				return err;
+		/* Check outgoing MTU */
+		if (len > chan->omtu) {
+			err = -EMSGSIZE;
+			break;
 		}
 
-		if (chan->mode == L2CAP_MODE_STREAMING) {
+		__skb_queue_head_init(&seg_queue);
+
+		/* Do segmentation before calling in to the state machine,
+		 * since it's possible to block while waiting for memory
+		 * allocation.
+		 */
+		err = l2cap_segment_sdu(chan, &seg_queue, msg, len);
+
+		/* The channel could have been closed while segmenting,
+		 * check that it is still connected.
+		 */
+		if (chan->state != BT_CONNECTED) {
+			__skb_queue_purge(&seg_queue);
+			err = -ENOTCONN;
+		}
+
+		if (err)
+			break;
+
+		skb_queue_splice_tail_init(&seg_queue, &chan->tx_q);
+		if (chan->mode == L2CAP_MODE_ERTM)
+			err = l2cap_ertm_send(chan);
+		else
 			l2cap_streaming_send(chan);
-			err = len;
-			break;
-		}
 
-		if (test_bit(CONN_REMOTE_BUSY, &chan->conn_state) &&
-				test_bit(CONN_WAIT_F, &chan->conn_state)) {
-			err = len;
-			break;
-		}
-
-		err = l2cap_ertm_send(chan);
 		if (err >= 0)
 			err = len;
 
+		/* If the skbs were not queued for sending, they'll still be in
+		 * seg_queue and need to be purged.
+		 */
+		__skb_queue_purge(&seg_queue);
 		break;
 
 	default:
-- 
1.7.10

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

Re: [PATCH 0/4] ERTM state machine changes, part 2

[Mat Martineau]

On Wed, 2 May 2012, Mat Martineau wrote:

> This is the second patch series reworking the ERTM state machine and
> closely related streaming mode code.  These patches include bug fixes
> and segmentation of outgoing L2CAP data.
>
> RFCv1:   Four of eight patches were merged.
> RFCv2:   Fixed the send lock patch, found and fixed a few more issues
>         with locking, reference counting, and unused code.  Four of
>         eight patches were merged.
> PATCHv1: Confirmed ERTM operation with locking and segmentation updates
>
>
> Mat Martineau (4):
>  Bluetooth: Fix a redundant and problematic incoming MTU check
>  Bluetooth: Restore locking semantics when looking up L2CAP channels
>  Bluetooth: Lock the L2CAP channel when sending
>  Bluetooth: Refactor L2CAP ERTM and streaming transmit segmentation
>
> include/net/bluetooth/bluetooth.h |    2 -
> include/net/bluetooth/l2cap.h     |    1 +
> net/bluetooth/l2cap_core.c        |  184 +++++++++++++++++++------------------
> net/bluetooth/l2cap_sock.c        |   15 +--
> 4 files changed, 103 insertions(+), 99 deletions(-)
>
> -- 
> 1.7.10

Patches 1, 2, and 3 might be appropriate for the bluetooth repository 
(not just bluetooth-next).

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

Re: [PATCH 1/4] Bluetooth: Fix a redundant and problematic incoming MTU check

[Ulisses Furquim]

Hi Mat,

On Wed, May 2, 2012 at 1:41 PM, Mat Martineau <mathewm@codeaurora.org> wrot=
e:
> The L2CAP MTU for incoming data is verified differently depending on
> the L2CAP mode, so the check is best performed in a mode-specific
> context. =A0Checking the incoming MTU before HCI fragment reassembly is
> a layer violation and assumes all bytes after the standard L2CAP
> header are L2CAP data.
>
> This approach causes issues with unsegmented ERTM or streaming mode
> frames, where there are additional enhanced or extended headers before
> the data payload and possible FCS bytes after the data payload. =A0A
> valid frame could be as many as 10 bytes larger than the MTU.
>
> Removing this code is the best fix, because the MTU is checked later
> on for all L2CAP data frames (connectionless, basic, ERTM, and
> streaming). =A0This also gets rid of outdated locking (socket instead of
> l2cap_chan) and an extra lookup of the channel ID.
>
> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
> ---
> =A0net/bluetooth/l2cap_core.c | =A0 20 --------------------
> =A01 file changed, 20 deletions(-)

This looks good and correct to me.

Regards,

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

Re: [PATCH 2/4] Bluetooth: Restore locking semantics when looking up L2CAP channels

[Ulisses Furquim]

Hi Mat,

On Wed, May 2, 2012 at 1:42 PM, Mat Martineau <mathewm@codeaurora.org> wrote:
> As the comment for l2cap_get_chan_by_scid indicated, the function used
> to return a locked socket.  The lock for the socket was acquired while
> the channel list was also locked.
>
> When locking was moved over to the l2cap_chan structure, the channel
> lock was no longer acquired with the channel list still locked.  This
> made it possible for the l2cap_chan to be deleted after
> conn->chan_lock was released but before l2cap_chan_lock was called.
> Making the call to l2cap_chan_lock before releasing conn->chan_lock
> makes it impossible for the l2cap_chan to be deleted at the wrong
> time.
>
> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
> ---
>  net/bluetooth/l2cap_core.c |   10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)

Looks good. I couldn't see this problem when Andrei was adding
l2cap_chan_lock and doing other changes, thanks for fixing it.

Regards,

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

Re: [PATCH 3/4] Bluetooth: Lock the L2CAP channel when sending

[Ulisses Furquim]

Hi Mat,

On Wed, May 2, 2012 at 1:42 PM, Mat Martineau <mathewm@codeaurora.org> wrot=
e:
> The ERTM and streaming mode transmit queue must only be accessed while
> the L2CAP channel lock is held. =A0Locking the channel before calling
> l2cap_chan_send ensures that multiple threads cannot simultaneously
> manipulate the queue when sending and receiving concurrently.
>
> L2CAP channel locking had previously moved to the l2cap_chan struct
> instead of the associated socket, so some of the old socket locking
> can also be removed in this patch.
>
> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
> ---
> =A0include/net/bluetooth/bluetooth.h | =A0 =A02 --
> =A0net/bluetooth/l2cap_sock.c =A0 =A0 =A0 =A0| =A0 15 ++++++++-------
> =A02 files changed, 8 insertions(+), 9 deletions(-)

Looks good. I'm just wondering if we still have issues with chan lock
versus sock lock elsewhere. Maybe you've done some auditing of this?

Regards,

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

Re: [PATCH 0/4] ERTM state machine changes, part 2

[Ulisses Furquim]

Hi Mat,

On Wed, May 2, 2012 at 6:40 PM, Mat Martineau <mathewm@codeaurora.org> wrot=
e:
>
> On Wed, 2 May 2012, Mat Martineau wrote:
>
>> This is the second patch series reworking the ERTM state machine and
>> closely related streaming mode code. =A0These patches include bug fixes
>> and segmentation of outgoing L2CAP data.
>>
>> RFCv1: =A0 Four of eight patches were merged.
>> RFCv2: =A0 Fixed the send lock patch, found and fixed a few more issues
>> =A0 =A0 =A0 =A0with locking, reference counting, and unused code. =A0Fou=
r of
>> =A0 =A0 =A0 =A0eight patches were merged.
>> PATCHv1: Confirmed ERTM operation with locking and segmentation updates
>>
>>
>> Mat Martineau (4):
>> =A0Bluetooth: Fix a redundant and problematic incoming MTU check
>> =A0Bluetooth: Restore locking semantics when looking up L2CAP channels
>> =A0Bluetooth: Lock the L2CAP channel when sending
>> =A0Bluetooth: Refactor L2CAP ERTM and streaming transmit segmentation
>>
>> include/net/bluetooth/bluetooth.h | =A0 =A02 -
>> include/net/bluetooth/l2cap.h =A0 =A0 | =A0 =A01 +
>> net/bluetooth/l2cap_core.c =A0 =A0 =A0 =A0| =A0184
>> +++++++++++++++++++------------------
>> net/bluetooth/l2cap_sock.c =A0 =A0 =A0 =A0| =A0 15 +--
>> 4 files changed, 103 insertions(+), 99 deletions(-)
>>
>> --
>> 1.7.10
>
>
> Patches 1, 2, and 3 might be appropriate for the bluetooth repository (no=
t
> just bluetooth-next).

I agree they might be candidates for bluetooth repository as well.

Regards,

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

Re: [PATCH 4/4] Bluetooth: Refactor L2CAP ERTM and streaming transmit segmentation

[Ulisses Furquim]

Hi Mat,

On Wed, May 2, 2012 at 1:42 PM, Mat Martineau <mathewm@codeaurora.org> wrot=
e:
> Use more common code for ERTM and streaming mode segmentation and
> transmission, and begin using skb control block data for delaying
> extended or enhanced header generation until just before the packet is
> transmitted. =A0This code is also better suited for resegmentation,
> which is needed when L2CAP links are reconfigured after an AMP channel
> move.
>
> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
> ---
> =A0include/net/bluetooth/l2cap.h | =A0 =A01 +
> =A0net/bluetooth/l2cap_core.c =A0 =A0| =A0154 ++++++++++++++++++++++++---=
--------------
> =A02 files changed, 92 insertions(+), 63 deletions(-)

This patch also looks good to me, but it'd be good if Andrei or
someone else could check as well.

Regards,

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

Re: [PATCH 1/4] Bluetooth: Fix a redundant and problematic incoming MTU check

[Gustavo Padovan]

Hi Mat,

* Mat Martineau <mathewm@codeaurora.org> [2012-05-02 09:41:59 -0700]:

> The L2CAP MTU for incoming data is verified differently depending on
> the L2CAP mode, so the check is best performed in a mode-specific
> context.  Checking the incoming MTU before HCI fragment reassembly is
> a layer violation and assumes all bytes after the standard L2CAP
> header are L2CAP data.
> 
> This approach causes issues with unsegmented ERTM or streaming mode
> frames, where there are additional enhanced or extended headers before
> the data payload and possible FCS bytes after the data payload.  A
> valid frame could be as many as 10 bytes larger than the MTU.
> 
> Removing this code is the best fix, because the MTU is checked later
> on for all L2CAP data frames (connectionless, basic, ERTM, and
> streaming).  This also gets rid of outdated locking (socket instead of
> l2cap_chan) and an extra lookup of the channel ID.
> 
> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
> ---
>  net/bluetooth/l2cap_core.c |   20 --------------------
>  1 file changed, 20 deletions(-)

Patch has been applied to the bluetooth tree. Thanks.

	Gustavo

Re: [PATCH 1/4] Bluetooth: Fix a redundant and problematic incoming MTU check

[Gustavo Padovan]

Hi Ulisses,

* Ulisses Furquim <ulisses@profusion.mobi> [2012-05-04 15:55:16 -0300]:

> Hi Mat,
>=20
> On Wed, May 2, 2012 at 1:41 PM, Mat Martineau <mathewm@codeaurora.org> wr=
ote:
> > The L2CAP MTU for incoming data is verified differently depending on
> > the L2CAP mode, so the check is best performed in a mode-specific
> > context. =A0Checking the incoming MTU before HCI fragment reassembly is
> > a layer violation and assumes all bytes after the standard L2CAP
> > header are L2CAP data.
> >
> > This approach causes issues with unsegmented ERTM or streaming mode
> > frames, where there are additional enhanced or extended headers before
> > the data payload and possible FCS bytes after the data payload. =A0A
> > valid frame could be as many as 10 bytes larger than the MTU.
> >
> > Removing this code is the best fix, because the MTU is checked later
> > on for all L2CAP data frames (connectionless, basic, ERTM, and
> > streaming). =A0This also gets rid of outdated locking (socket instead of
> > l2cap_chan) and an extra lookup of the channel ID.
> >
> > Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
> > ---
> > =A0net/bluetooth/l2cap_core.c | =A0 20 --------------------
> > =A01 file changed, 20 deletions(-)
>=20
> This looks good and correct to me.

Please add proper Reviewed-by tag when we are ok with a patch, it easier
to us pick it up here.

	Gustavo

Re: [PATCH 4/4] Bluetooth: Refactor L2CAP ERTM and streaming transmit segmentation

[Gustavo Padovan]

Hi Mat,

* Mat Martineau <mathewm@codeaurora.org> [2012-05-02 09:42:02 -0700]:

> Use more common code for ERTM and streaming mode segmentation and
> transmission, and begin using skb control block data for delaying
> extended or enhanced header generation until just before the packet is
> transmitted.  This code is also better suited for resegmentation,
> which is needed when L2CAP links are reconfigured after an AMP channel
> move.
> 
> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
> ---
>  include/net/bluetooth/l2cap.h |    1 +
>  net/bluetooth/l2cap_core.c    |  154 ++++++++++++++++++++++++-----------------
>  2 files changed, 92 insertions(+), 63 deletions(-)

Patch looks good to me. Applied to bluetooth-next. Thanks.

	Gustavo

Re: [PATCH 3/4] Bluetooth: Lock the L2CAP channel when sending

[Mat Martineau]

Hi Ulisses -

On Fri, 4 May 2012, Ulisses Furquim wrote:

> Hi Mat,
>
> On Wed, May 2, 2012 at 1:42 PM, Mat Martineau <mathewm@codeaurora.org> wrote:
>> The ERTM and streaming mode transmit queue must only be accessed while
>> the L2CAP channel lock is held.  Locking the channel before calling
>> l2cap_chan_send ensures that multiple threads cannot simultaneously
>> manipulate the queue when sending and receiving concurrently.
>>
>> L2CAP channel locking had previously moved to the l2cap_chan struct
>> instead of the associated socket, so some of the old socket locking
>> can also be removed in this patch.
>>
>> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
>> ---
>>  include/net/bluetooth/bluetooth.h |    2 --
>>  net/bluetooth/l2cap_sock.c        |   15 ++++++++-------
>>  2 files changed, 8 insertions(+), 9 deletions(-)
>
> Looks good. I'm just wondering if we still have issues with chan lock
> versus sock lock elsewhere. Maybe you've done some auditing of this?

I did an extensive review of the locking code with respect to ERTM 
last week, and I'm satisfied with the use of chan_lock there.

The work to decouple l2cap_chan from sockets is not yet complete, so 
there are still socket locking calls at connect and disconnect time. 
The data path looks like it is clear of socket locks now.


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

Re: [PATCH 3/4] Bluetooth: Lock the L2CAP channel when sending

[Ulisses Furquim]

Hi Mat,

On Fri, May 4, 2012 at 6:54 PM, Mat Martineau <mathewm@codeaurora.org> wrote:
>
> Hi Ulisses -
>
>
> On Fri, 4 May 2012, Ulisses Furquim wrote:
>
>> Hi Mat,
>>
>> On Wed, May 2, 2012 at 1:42 PM, Mat Martineau <mathewm@codeaurora.org>
>> wrote:
>>>
>>> The ERTM and streaming mode transmit queue must only be accessed while
>>> the L2CAP channel lock is held.  Locking the channel before calling
>>> l2cap_chan_send ensures that multiple threads cannot simultaneously
>>> manipulate the queue when sending and receiving concurrently.
>>>
>>> L2CAP channel locking had previously moved to the l2cap_chan struct
>>> instead of the associated socket, so some of the old socket locking
>>> can also be removed in this patch.
>>>
>>> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
>>> ---
>>>  include/net/bluetooth/bluetooth.h |    2 --
>>>  net/bluetooth/l2cap_sock.c        |   15 ++++++++-------
>>>  2 files changed, 8 insertions(+), 9 deletions(-)
>>
>>
>> Looks good. I'm just wondering if we still have issues with chan lock
>> versus sock lock elsewhere. Maybe you've done some auditing of this?
>
> I did an extensive review of the locking code with respect to ERTM last
> week, and I'm satisfied with the use of chan_lock there.
>
> The work to decouple l2cap_chan from sockets is not yet complete, so there
> are still socket locking calls at connect and disconnect time. The data path
> looks like it is clear of socket locks now.

Thanks, that's good to know.

Regards,

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

Re: [PATCH 4/4] Bluetooth: Refactor L2CAP ERTM and streaming transmit segmentation

[Andrei Emeltchenko]

Hi Mat,

On Wed, May 02, 2012 at 09:42:02AM -0700, Mat Martineau wrote:
> Use more common code for ERTM and streaming mode segmentation and
> transmission, and begin using skb control block data for delaying
> extended or enhanced header generation until just before the packet is
> transmitted.  This code is also better suited for resegmentation,
> which is needed when L2CAP links are reconfigured after an AMP channel
> move.

...
> @@ -1708,6 +1709,9 @@ static int l2cap_ertm_send(struct l2cap_chan *chan)
>  	if (chan->state != BT_CONNECTED)
>  		return -ENOTCONN;
>  
> +	if (test_bit(CONN_REMOTE_BUSY, &chan->conn_state))
> +		return 0;
> +
>  	while ((skb = chan->tx_send_head) && (!l2cap_tx_window_full(chan))) {

We check here chan->tx_send_head. But:

...
> -	skb_queue_splice_tail(&sar_queue, &chan->tx_q);
> -	if (chan->tx_send_head == NULL)
> -		chan->tx_send_head = sar_queue.next;
...

> -
> -			if (chan->tx_send_head == NULL)
> -				chan->tx_send_head = skb;
...

tx_send_head seems to be always NULL due to the change above. Have you
tested that it actually works?

Best regards 
Andrei Emeltchenko 

Re: [PATCH 4/4] Bluetooth: Refactor L2CAP ERTM and streaming transmit segmentation

[Mat Martineau]

Hi Andrei -

On Mon, 14 May 2012, Andrei Emeltchenko wrote:

> Hi Mat,
>
> On Wed, May 02, 2012 at 09:42:02AM -0700, Mat Martineau wrote:
>> Use more common code for ERTM and streaming mode segmentation and
>> transmission, and begin using skb control block data for delaying
>> extended or enhanced header generation until just before the packet is
>> transmitted.  This code is also better suited for resegmentation,
>> which is needed when L2CAP links are reconfigured after an AMP channel
>> move.
>
> ...
>> @@ -1708,6 +1709,9 @@ static int l2cap_ertm_send(struct l2cap_chan *chan)
>>  	if (chan->state != BT_CONNECTED)
>>  		return -ENOTCONN;
>>
>> +	if (test_bit(CONN_REMOTE_BUSY, &chan->conn_state))
>> +		return 0;
>> +
>>  	while ((skb = chan->tx_send_head) && (!l2cap_tx_window_full(chan))) {
>
> We check here chan->tx_send_head. But:
>
> ...
>> -	skb_queue_splice_tail(&sar_queue, &chan->tx_q);
>> -	if (chan->tx_send_head == NULL)
>> -		chan->tx_send_head = sar_queue.next;
> ...
>
>> -
>> -			if (chan->tx_send_head == NULL)
>> -				chan->tx_send_head = skb;
> ...
>
> tx_send_head seems to be always NULL due to the change above. Have you
> tested that it actually works?

I did some checks with l2test, but now that I look at the code I must 
have made a mistake (like accidentally using the wrong build).  Sorry 
about that, I'll send a fix ASAP.


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