Skip to contentSkip to conversation

L2CAP fragmentation changes

RFC In review Mat Martineau 3 participants 27 messages Aug 10, 2010 – Aug 13, 2010

Subsystems (3)
Arm Qualcomm mailing list Bluetooth drivers Bluetooth
lore
Share & tools
lore

Mat Martineau

author
#0 lore raw

cover L2CAP fragmentation changes

Since the previous L2CAP patch set has not been fully merged, I'm only posting these patches for review at this time.

ERTM and streaming mode currently have a limitation where the trasmitted PDU size cannot exceed the HCI MTU. This is a problem with some basebands with small HCI MTU values (some are around 300 bytes), since it is not possible to use larger BR/EDR packets over the air when the PDUs are too small. Bandwidth is also wasted with extra L2CAP header overhead. Patches 1-3 add the capability to calculate checksums on PDUs that have HCI continuation fragments.

Patches 4-6 change the way ERTM PDUs are reassembled, to avoid extra data copying at receive time. At higher AMP data rates, this efficiency improvement becomes more important. Each PDU is linked together using skbuff fragments, similar to the way outgoing data uses skbuff frag_lists for HCI continuations. This way, L2CAP must only copy the data to a linear buffer when it is copied out to userspace.

Patch 7 changes the max PDU size configuration to allow larger PDU sizes to be set up, enabling ERTM and streaming mode to send larger PDUs.

author
#1 lore raw

RFC 1/7 Bluetooth: Calculate L2CAP FCS on fragmented skbuffs.

When L2CAP PDUs are longer than the HCI MTU, they are stored in fragmented skbuffs. This adds a function to calculate the FCS on any skbuff (fragmented or not), and replaces direct calls to crc16 with calls to the new apply_fcs() function.

Signed-off-by Mat Martineau <[email protected]>
---
 net/bluetooth/l2cap.c |   57 ++++++++++++++++++++++++++++++++----------------
 1 files changed, 38 insertions(+), 19 deletions(-)
net/bluetooth/l2cap.c +38 -19
index 8d362d7..5e78c18 100644
@@ -351,6 +351,33 @@ static inline u8 l2cap_get_ident(struct l2cap_conn *conn)
 	return id;
 }
 
+static void apply_fcs(struct sk_buff *skb)
+{
+	size_t len;
+	u16 partial_crc;
+	struct sk_buff *iter;
+	struct sk_buff *final_frag = skb;
+
+	if (skb_has_frags(skb))
+		len = skb_headlen(skb);
+	else
+		len = skb->len - L2CAP_FCS_SIZE;
+
+	partial_crc = crc16(0, (u8 *) skb->data, len);
+
+	skb_walk_frags(skb, iter) {
+		len = iter->len;
+		if (!iter->next)
+			len -= L2CAP_FCS_SIZE;
+
+		partial_crc = crc16(partial_crc, iter->data, len);
+		final_frag = iter;
+	}
+
+	put_unaligned_le16(partial_crc,
+		final_frag->data + final_frag->len - L2CAP_FCS_SIZE);
+}
+
 static inline void l2cap_send_cmd(struct l2cap_conn *conn, u8 ident, u8 code, u16 len, void *data)
 {
 	struct sk_buff *skb = l2cap_build_cmd(conn, code, ident, len, data);

@@ -401,10 +428,8 @@ static inline void l2cap_send_sframe(struct l2cap_pinfo *pi, u16 control)
 	lh->cid = cpu_to_le16(pi->dcid);
 	put_unaligned_le16(control, skb_put(skb, 2));
 
-	if (pi->fcs == L2CAP_FCS_CRC16) {
-		u16 fcs = crc16(0, (u8 *)lh, count - 2);
-		put_unaligned_le16(fcs, skb_put(skb, 2));
-	}
+	if (pi->fcs == L2CAP_FCS_CRC16)
+		apply_fcs(skb);
 
 	hci_send_acl(pi->conn->hcon, skb, 0);
 }

@@ -1458,7 +1483,7 @@ static void l2cap_streaming_send(struct sock *sk)
 {
 	struct sk_buff *skb, *tx_skb;
 	struct l2cap_pinfo *pi = l2cap_pi(sk);
-	u16 control, fcs;
+	u16 control;
 
 	while ((skb = sk->sk_send_head)) {
 		tx_skb = skb_clone(skb, GFP_ATOMIC);

@@ -1467,10 +1492,8 @@ static void l2cap_streaming_send(struct sock *sk)
 		control |= pi->next_tx_seq << L2CAP_CTRL_TXSEQ_SHIFT;
 		put_unaligned_le16(control, tx_skb->data + L2CAP_HDR_SIZE);
 
-		if (pi->fcs == L2CAP_FCS_CRC16) {
-			fcs = crc16(0, (u8 *)tx_skb->data, tx_skb->len - 2);
-			put_unaligned_le16(fcs, tx_skb->data + tx_skb->len - 2);
-		}
+		if (pi->fcs == L2CAP_FCS_CRC16)
+			apply_fcs(tx_skb);
 
 		l2cap_do_send(sk, tx_skb);
 

@@ -1490,7 +1513,7 @@ static void l2cap_retransmit_one_frame(struct sock *sk, u8 tx_seq)
 {
 	struct l2cap_pinfo *pi = l2cap_pi(sk);
 	struct sk_buff *skb, *tx_skb;
-	u16 control, fcs;
+	u16 control;
 
 	skb = skb_peek(TX_QUEUE(sk));
 	if (!skb)

@@ -1525,10 +1548,8 @@ static void l2cap_retransmit_one_frame(struct sock *sk, u8 tx_seq)
 
 	put_unaligned_le16(control, tx_skb->data + L2CAP_HDR_SIZE);
 
-	if (pi->fcs == L2CAP_FCS_CRC16) {
-		fcs = crc16(0, (u8 *)tx_skb->data, tx_skb->len - 2);
-		put_unaligned_le16(fcs, tx_skb->data + tx_skb->len - 2);
-	}
+	if (pi->fcs == L2CAP_FCS_CRC16)
+		apply_fcs(tx_skb);
 
 	l2cap_do_send(sk, tx_skb);
 }

@@ -1537,7 +1558,7 @@ static int l2cap_ertm_send(struct sock *sk)
 {
 	struct sk_buff *skb, *tx_skb;
 	struct l2cap_pinfo *pi = l2cap_pi(sk);
-	u16 control, fcs;
+	u16 control;
 	int nsent = 0;
 
 	if (sk->sk_state != BT_CONNECTED)

@@ -1567,10 +1588,8 @@ static int l2cap_ertm_send(struct sock *sk)
 		put_unaligned_le16(control, tx_skb->data + L2CAP_HDR_SIZE);
 
 
-		if (pi->fcs == L2CAP_FCS_CRC16) {
-			fcs = crc16(0, (u8 *)skb->data, tx_skb->len - 2);
-			put_unaligned_le16(fcs, skb->data + tx_skb->len - 2);
-		}
+		if (pi->fcs == L2CAP_FCS_CRC16)
+			apply_fcs(tx_skb);
 
 		l2cap_do_send(sk, tx_skb);
Signature · 6 lines
1.7.1

--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
maintainer
#2 lore raw

Re: [RFC 1/7] Bluetooth: Calculate L2CAP FCS on fragmented skbuffs.

Hi Mat,

Quoted 9 lines

When L2CAP PDUs are longer than the HCI MTU, they are stored in fragmented skbuffs. This adds a function to calculate the FCS on any skbuff (fragmented or not), and replaces direct calls to crc16 with calls to the new apply_fcs() function.

Signed-off-by Mat Martineau <[email protected]>
---
 net/bluetooth/l2cap.c |   57 ++++++++++++++++++++++++++++++++----------------
 1 files changed, 38 insertions(+), 19 deletions(-)

looks fine to me. Just lets get the other ones in first and then we can apply this one.

Regards

Marcel

#3 lore raw

Re: [RFC 1/7] Bluetooth: Calculate L2CAP FCS on fragmented skbuffs.

In reply to Mat Martineau

Hi Mat,

Quoted 15 lines · Mat Martineau

When L2CAP PDUs are longer than the HCI MTU, they are stored in fragmented skbuffs. This adds a function to calculate the FCS on any skbuff (fragmented or not), and replaces direct calls to crc16 with calls to the new apply_fcs() function.

Signed-off-by Mat Martineau <[email protected]>
---
 net/bluetooth/l2cap.c |   57 ++++++++++++++++++++++++++++++++----------------
 1 files changed, 38 insertions(+), 19 deletions(-)
net/bluetooth/l2cap.c +0 -0
index 8d362d7..5e78c18 100644

net/bluetooth/l2cap.c static inline u8 l2cap_get_ident(struct l2cap_conn *conn) · line 354

Quoted 4 lines
net/bluetooth/l2cap.c +1 -0
@@ -351,6 +351,33 @@ static inline u8 l2cap_get_ident(struct l2cap_conn *conn)
 	return id;
 }
 
+static void apply_fcs(struct sk_buff *skb)

I would call this l2cap_apply_fcs().

Signature · 2 lines
Gustavo F. Padovan
http://padovan.org
author
#4 lore raw

RFC 2/7 Bluetooth: Use enhanced L2CAP header structure and symbolic values.

In reply to Mat Martineau
Signed-off-by Mat Martineau <[email protected]>
---
 net/bluetooth/l2cap.c |   23 +++++++++++++----------
 1 files changed, 13 insertions(+), 10 deletions(-)
net/bluetooth/l2cap.c +13 -10
index 5e78c18..aa69c84 100644
@@ -1753,33 +1753,36 @@ static struct sk_buff *l2cap_create_iframe_pdu(struct sock *sk, struct msghdr *m
 {
 	struct l2cap_conn *conn = l2cap_pi(sk)->conn;
 	struct sk_buff *skb;
-	int err, count, hlen = L2CAP_HDR_SIZE + 2;
-	struct l2cap_hdr *lh;
+	int err, count, hlen = L2CAP_ENHANCED_HDR_SIZE;
+	struct l2cap_enhanced_hdr *lh;
 
-	BT_DBG("sk %p len %d", sk, (int)len);
+	BT_DBG("sk %p, msg %p, len %d, control %x, sdulen %d",
+		sk, msg, (int)len, control, (int)sdulen);
 
 	if (!conn)
 		return ERR_PTR(-ENOTCONN);
 
 	if (sdulen)
-		hlen += 2;
+		hlen += L2CAP_SDULEN_SIZE;
 
 	if (l2cap_pi(sk)->fcs == L2CAP_FCS_CRC16)
-		hlen += 2;
+		hlen += L2CAP_FCS_SIZE;
 
 	count = min_t(unsigned int, (conn->mtu - hlen), len);
+
 	skb = bt_skb_send_alloc(sk, count + hlen,
 			msg->msg_flags & MSG_DONTWAIT, &err);
 	if (!skb)
-		return ERR_PTR(-ENOMEM);
+		return ERR_PTR(err);
 
 	/* Create L2CAP header */
-	lh = (struct l2cap_hdr *) skb_put(skb, L2CAP_HDR_SIZE);
+	lh = (struct l2cap_enhanced_hdr *) skb_put(skb,
+						L2CAP_ENHANCED_HDR_SIZE);
 	lh->cid = cpu_to_le16(l2cap_pi(sk)->dcid);
-	lh->len = cpu_to_le16(len + (hlen - L2CAP_HDR_SIZE));
-	put_unaligned_le16(control, skb_put(skb, 2));
+	lh->len = cpu_to_le16(len + hlen - L2CAP_HDR_SIZE);
+	lh->control = cpu_to_le16(control);
 	if (sdulen)
-		put_unaligned_le16(sdulen, skb_put(skb, 2));
+		put_unaligned_le16(sdulen, skb_put(skb, L2CAP_SDULEN_SIZE));
 
 	err = l2cap_skbuff_fromiovec(sk, msg, len, count, skb);
 	if (unlikely(err < 0)) {
Signature · 6 lines
1.7.1

--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
maintainer
#5 lore raw

Re: [RFC 2/7] Bluetooth: Use enhanced L2CAP header structure and symbolic values.

Hi Mat,

Quoted 10 lines
Signed-off-by Mat Martineau <[email protected]>
---
 net/bluetooth/l2cap.c |   23 +++++++++++++----------
 1 files changed, 13 insertions(+), 10 deletions(-)
net/bluetooth/l2cap.c +0 -0
index 5e78c18..aa69c84 100644

net/bluetooth/l2cap.c static struct sk_buff *l2cap_create_iframe_pdu(struct sock *sk, struct msghdr *m · line 1757

Quoted 7 lines
net/bluetooth/l2cap.c +2 -2
@@ -1753,33 +1753,36 @@ static struct sk_buff *l2cap_create_iframe_pdu(struct sock *sk, struct msghdr *m
 {
 	struct l2cap_conn *conn = l2cap_pi(sk)->conn;
 	struct sk_buff *skb;
-	int err, count, hlen = L2CAP_HDR_SIZE + 2;
-	struct l2cap_hdr *lh;
+	int err, count, hlen = L2CAP_ENHANCED_HDR_SIZE;
+	struct l2cap_enhanced_hdr *lh;

so where is that patch that actually introduces L2CAP_ENHANCED_HDR_SIZE etc. I can't see that.

Regards

Marcel

#6 lore raw

Re: [RFC 2/7] Bluetooth: Use enhanced L2CAP header structure and symbolic values.

In reply to Mat Martineau

Hi Mat,

Quoted 11 lines · Mat Martineau
Signed-off-by Mat Martineau <[email protected]>
---
 net/bluetooth/l2cap.c |   23 +++++++++++++----------
 1 files changed, 13 insertions(+), 10 deletions(-)
net/bluetooth/l2cap.c +0 -0
index 5e78c18..aa69c84 100644

net/bluetooth/l2cap.c static struct sk_buff *l2cap_create_iframe_pdu(struct sock *sk, struct msghdr *m · line 1757

Quoted 7 lines
net/bluetooth/l2cap.c +2 -2
@@ -1753,33 +1753,36 @@ static struct sk_buff *l2cap_create_iframe_pdu(struct sock *sk, struct msghdr *m
 {
 	struct l2cap_conn *conn = l2cap_pi(sk)->conn;
 	struct sk_buff *skb;
-	int err, count, hlen = L2CAP_HDR_SIZE + 2;
-	struct l2cap_hdr *lh;
+	int err, count, hlen = L2CAP_ENHANCED_HDR_SIZE;
+	struct l2cap_enhanced_hdr *lh;

This patch shouldn't even compile. Since there no definition for struct l2cap_enhanced_hdr. Anyway we should not have that struct, keep the code to handle this as it is.
We'll have to change this code again to add the extended control bit field.

net/bluetooth/l2cap.c static struct sk_buff *l2cap_create_iframe_pdu(struct sock *sk, struct msghdr *m · line 1766

Quoted 11 lines
net/bluetooth/l2cap.c +3 -2
-	BT_DBG("sk %p len %d", sk, (int)len);
+	BT_DBG("sk %p, msg %p, len %d, control %x, sdulen %d",
+		sk, msg, (int)len, control, (int)sdulen);
 
 	if (!conn)
 		return ERR_PTR(-ENOTCONN);
 
 	if (sdulen)
-		hlen += 2;
+		hlen += L2CAP_SDULEN_SIZE;

Separated patch to add L2CAP_SDULEN_SIZE

net/bluetooth/l2cap.c static struct sk_buff *l2cap_create_iframe_pdu(struct sock *sk, struct msghdr *m · line 1769

Quoted 4 lines
net/bluetooth/l2cap.c +1 -1
 	if (l2cap_pi(sk)->fcs == L2CAP_FCS_CRC16)
-		hlen += 2;
+		hlen += L2CAP_FCS_SIZE;

Separated patch to ad L2CAP_FCS_SIZE

net/bluetooth/l2cap.c static struct sk_buff *l2cap_create_iframe_pdu(struct sock *sk, struct msghdr *m · line 1776

Quoted 8 lines
net/bluetooth/l2cap.c +2 -1
 	count = min_t(unsigned int, (conn->mtu - hlen), len);
+
 	skb = bt_skb_send_alloc(sk, count + hlen,
 			msg->msg_flags & MSG_DONTWAIT, &err);
 	if (!skb)
-		return ERR_PTR(-ENOMEM);
+		return ERR_PTR(err);

This also should not be in this patch.

Signature · 2 lines
Gustavo F. Padovan
http://padovan.org
author
#7 lore raw

Re: [RFC 2/7] Bluetooth: Use enhanced L2CAP header structure and symbolic values.

Gustavo -

Quoted 20 lines · Gustavo F. Padovan

Hi Mat,

Quoted 16 lines · Mat Martineau
Signed-off-by Mat Martineau <[email protected]>
---
 net/bluetooth/l2cap.c |   23 +++++++++++++----------
 1 files changed, 13 insertions(+), 10 deletions(-)
net/bluetooth/l2cap.c +0 -2
index 5e78c18..aa69c84 100644
@@ -1753,33 +1753,36 @@ static struct sk_buff *l2cap_create_iframe_pdu(struct sock *sk, struct msghdr *m
 {
 	struct l2cap_conn *conn = l2cap_pi(sk)->conn;
 	struct sk_buff *skb;
-	int err, count, hlen = L2CAP_HDR_SIZE + 2;
-	struct l2cap_hdr *lh;
Quoted 8 lines

net/bluetooth/l2cap.c static struct sk_buff *l2cap_create_iframe_pdu(struct sock *sk, struct msghdr *m · line 1757

Quoted 2 lines
net/bluetooth/l2cap.c +2 -0
+	int err, count, hlen = L2CAP_ENHANCED_HDR_SIZE;
+	struct l2cap_enhanced_hdr *lh;

This patch shouldn't even compile. Since there no definition for struct l2cap_enhanced_hdr. Anyway we should not have that struct, keep the code to handle this as it is.
We'll have to change this code again to add the extended control bit field.

Sorry about that - I did my git format-patch starting one commit too late.

I also have an l2cap_extended_hdr struct to add later, but it sounds like you don't want that one either.

Quoted 6 lines
Quoted 6 lines
net/bluetooth/l2cap.c +2 -1
-	BT_DBG("sk %p len %d", sk, (int)len);
+	BT_DBG("sk %p, msg %p, len %d, control %x, sdulen %d",
+		sk, msg, (int)len, control, (int)sdulen);
Quoted 8 lines

net/bluetooth/l2cap.c static struct sk_buff *l2cap_create_iframe_pdu(struct sock *sk, struct msghdr *m · line 1766

Quoted 6 lines
net/bluetooth/l2cap.c +1 -1
 	if (!conn)
 		return ERR_PTR(-ENOTCONN);
 
 	if (sdulen)
-		hlen += 2;
+		hlen += L2CAP_SDULEN_SIZE;

Separated patch to add L2CAP_SDULEN_SIZE

Ok.

Quoted 6 lines

net/bluetooth/l2cap.c static struct sk_buff *l2cap_create_iframe_pdu(struct sock *sk, struct msghdr *m · line 1769

Quoted 4 lines
net/bluetooth/l2cap.c +1 -1
 	if (l2cap_pi(sk)->fcs == L2CAP_FCS_CRC16)
-		hlen += 2;
+		hlen += L2CAP_FCS_SIZE;

Separated patch to ad L2CAP_FCS_SIZE

Ok. Does this need to be separate from the L2CAP_SDULEN_SIZE patch?

Quoted 11 lines

net/bluetooth/l2cap.c static struct sk_buff *l2cap_create_iframe_pdu(struct sock *sk, struct msghdr *m · line 1776

Quoted 9 lines
net/bluetooth/l2cap.c +2 -1
 	count = min_t(unsigned int, (conn->mtu - hlen), len);
+
 	skb = bt_skb_send_alloc(sk, count + hlen,
 			msg->msg_flags & MSG_DONTWAIT, &err);
 	if (!skb)
-		return ERR_PTR(-ENOMEM);
+		return ERR_PTR(err);

This also should not be in this patch.

No problem.

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

#8 lore raw

Re: [RFC 2/7] Bluetooth: Use enhanced L2CAP header structure and symbolic values.

Hi Mat,

Quoted 35 lines · Mat Martineau

Gustavo -

Quoted 20 lines · Gustavo F. Padovan

Hi Mat,

Quoted 16 lines · Mat Martineau
Signed-off-by Mat Martineau <[email protected]>

---
net/bluetooth/l2cap.c | 23 +++++++++++++----------

1 files changed, 13 insertions(+), 10 deletions(-)
net/bluetooth/l2cap.c +0 -2
index 5e78c18..aa69c84 100644
@@ -1753,33 +1753,36 @@ static struct sk_buff *l2cap_create_iframe_pdu(struct sock *sk, struct msghdr *m
 {
 	struct l2cap_conn *conn = l2cap_pi(sk)->conn;
 	struct sk_buff *skb;
-	int err, count, hlen = L2CAP_HDR_SIZE + 2;
-	struct l2cap_hdr *lh;
Quoted 8 lines

net/bluetooth/l2cap.c static struct sk_buff *l2cap_create_iframe_pdu(struct sock *sk, struct msghdr *m · line 1757

Quoted 2 lines
net/bluetooth/l2cap.c +2 -0
+	int err, count, hlen = L2CAP_ENHANCED_HDR_SIZE;
+	struct l2cap_enhanced_hdr *lh;

This patch shouldn't even compile. Since there no definition for struct l2cap_enhanced_hdr. Anyway we should not have that struct, keep the code to handle this as it is.
We'll have to change this code again to add the extended control bit field.

Quoted 5 lines

Sorry about that - I did my git format-patch starting one commit too late.

I also have an l2cap_extended_hdr struct to add later, but it sounds like you don't want that one either.

Get rid of that one too. It's better do not have that extra complication.

By the way, I think we can hide the handle of extended and enhanced control fields inside the macros to get control bit fields. So we won't have to care about that in the code. We still have to talk about that.

Quoted 19 lines
Quoted 7 lines
Quoted 7 lines
net/bluetooth/l2cap.c +2 -1
-	BT_DBG("sk %p len %d", sk, (int)len);
+	BT_DBG("sk %p, msg %p, len %d, control %x, sdulen %d",
+		sk, msg, (int)len, control, (int)sdulen);
Quoted 8 lines

net/bluetooth/l2cap.c static struct sk_buff *l2cap_create_iframe_pdu(struct sock *sk, struct msghdr *m · line 1766

Quoted 6 lines
net/bluetooth/l2cap.c +1 -1
 	if (!conn)
 		return ERR_PTR(-ENOTCONN);
 
 	if (sdulen)
-		hlen += 2;
+		hlen += L2CAP_SDULEN_SIZE;

Separated patch to add L2CAP_SDULEN_SIZE

Ok.

Quoted 1 line
Quoted 1 line
Quoted 7 lines
Quoted 5 lines

net/bluetooth/l2cap.c

Quoted 3 lines
	if (l2cap_pi(sk)->fcs == L2CAP_FCS_CRC16)
net/bluetooth/l2cap.c +1 -1
-		hlen += 2;
+		hlen += L2CAP_FCS_SIZE;

Separated patch to ad L2CAP_FCS_SIZE

Ok. Does this need to be separate from the L2CAP_SDULEN_SIZE patch?

It depends. If we have few occurrence of that macros, they could be in same patch.

Signature · 2 lines
Gustavo F. Padovan
http://padovan.org
author
#9 lore raw

RFC 3/7 Bluetooth: Add FCS awareness to L2CAP HCI fragmentation.

In reply to Mat Martineau

In order to not limit ERTM and streaming mode PDUs to the HCI MTU size, L2CAP must be able to split PDUs in to multple HCI fragments. This is done by allocating space for the FCS in the last fragment.

Signed-off-by Mat Martineau <[email protected]>
---
 net/bluetooth/l2cap.c |   39 ++++++++++++++++++++++++++++++++++-----
 1 files changed, 34 insertions(+), 5 deletions(-)
net/bluetooth/l2cap.c +34 -5
index aa69c84..b485c4a 100644
@@ -1664,31 +1664,63 @@ static inline int l2cap_skbuff_fromiovec(struct sock *sk, struct msghdr *msg, in
 {
 	struct l2cap_conn *conn = l2cap_pi(sk)->conn;
 	struct sk_buff **frag;
+	struct sk_buff *final;
 	int err, sent = 0;
 
+	BT_DBG("sk %p, msg %p, len %d, count %d, skb %p", sk,
+		msg, (int)len, (int)count, skb);
+
 	if (memcpy_fromiovec(skb_put(skb, count), msg->msg_iov, count))
 		return -EFAULT;
 
 	sent += count;
 	len  -= count;
+	final = skb;
 
 	/* Continuation fragments (no L2CAP header) */
 	frag = &skb_shinfo(skb)->frag_list;
 	while (len) {
+		int skblen;
 		count = min_t(unsigned int, conn->mtu, len);
 
-		*frag = bt_skb_send_alloc(sk, count, msg->msg_flags & MSG_DONTWAIT, &err);
+		/* Add room for the FCS if it fits */
+		if (l2cap_pi(sk)->fcs == L2CAP_FCS_CRC16 &&
+		    len + L2CAP_FCS_SIZE <= conn->mtu)
+			skblen = count + L2CAP_FCS_SIZE;
+		else
+			skblen = count;
+
+		*frag = bt_skb_send_alloc(sk, skblen,
+					msg->msg_flags & MSG_DONTWAIT, &err);
 		if (!*frag)
 			return -EFAULT;
-		if (memcpy_fromiovec(skb_put(*frag, count), msg->msg_iov, count))
+
+		if (memcpy_fromiovec(skb_put(*frag, count),
+				msg->msg_iov, count))
 			return -EFAULT;
 
 		sent += count;
 		len  -= count;
 
+		final = *frag;
+
 		frag = &(*frag)->next;
 	}
 
+	if (l2cap_pi(sk)->fcs == L2CAP_FCS_CRC16) {
+		if (skb_tailroom(final) < L2CAP_FCS_SIZE) {
+			*frag = bt_skb_send_alloc(sk, L2CAP_FCS_SIZE,
+						msg->msg_flags & MSG_DONTWAIT,
+						&err);
+			if (!*frag)
+				return -EFAULT;
+
+			final = *frag;
+		}
+
+		skb_put(final, L2CAP_FCS_SIZE);
+	}
+
 	return sent;
 }
 

@@ -1790,9 +1822,6 @@ static struct sk_buff *l2cap_create_iframe_pdu(struct sock *sk, struct msghdr *m
 		return ERR_PTR(err);
 	}
 
-	if (l2cap_pi(sk)->fcs == L2CAP_FCS_CRC16)
-		put_unaligned_le16(0, skb_put(skb, 2));
-
 	bt_cb(skb)->retries = 0;
 	return skb;
 }
Signature · 6 lines
1.7.1

--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
maintainer
#10 lore raw

Re: [RFC 3/7] Bluetooth: Add FCS awareness to L2CAP HCI fragmentation.

Hi Mat,

Quoted 25 lines

In order to not limit ERTM and streaming mode PDUs to the HCI MTU size, L2CAP must be able to split PDUs in to multple HCI fragments. This is done by allocating space for the FCS in the last fragment.

Signed-off-by Mat Martineau <[email protected]>
---
 net/bluetooth/l2cap.c |   39 ++++++++++++++++++++++++++++++++++-----
 1 files changed, 34 insertions(+), 5 deletions(-)
net/bluetooth/l2cap.c +4 -0
index aa69c84..b485c4a 100644
@@ -1664,31 +1664,63 @@ static inline int l2cap_skbuff_fromiovec(struct sock *sk, struct msghdr *msg, in
 {
 	struct l2cap_conn *conn = l2cap_pi(sk)->conn;
 	struct sk_buff **frag;
+	struct sk_buff *final;
 	int err, sent = 0;
 
+	BT_DBG("sk %p, msg %p, len %d, count %d, skb %p", sk,
+		msg, (int)len, (int)count, skb);
+
 	if (memcpy_fromiovec(skb_put(skb, count), msg->msg_iov, count))
 		return -EFAULT;

net/bluetooth/l2cap.c static inline int l2cap_skbuff_fromiovec(struct sock *sk, struct msghdr *msg, in · line 1684

Quoted 10 lines
net/bluetooth/l2cap.c +2 -0
 
 	sent += count;
 	len  -= count;
+	final = skb;
 
 	/* Continuation fragments (no L2CAP header) */
 	frag = &skb_shinfo(skb)->frag_list;
 	while (len) {
+		int skblen;
 		count = min_t(unsigned int, conn->mtu, len);

I prefer that you either make the int skblen global or add an empty line. There should be always an empty line between variable declaration and code.

net/bluetooth/l2cap.c static inline int l2cap_skbuff_fromiovec(struct sock *sk, struct msghdr *msg, in · line 1689

Quoted 6 lines
net/bluetooth/l2cap.c +4 -1
-		*frag = bt_skb_send_alloc(sk, count, msg->msg_flags & MSG_DONTWAIT, &err);
+		/* Add room for the FCS if it fits */
+		if (l2cap_pi(sk)->fcs == L2CAP_FCS_CRC16 &&
+		    len + L2CAP_FCS_SIZE <= conn->mtu)
+			skblen = count + L2CAP_FCS_SIZE;

So in general we do double indent for the second line of the if:

		if (l2cap_pi ...
				len + ...
			skblen = count ...

This is not strictly enforced and every kernel code does it a little bit different.

The rest looks just fine.

Regards

Marcel

#11 lore raw

Re: [RFC 3/7] Bluetooth: Add FCS awareness to L2CAP HCI fragmentation.

In reply to Mat Martineau
Quoted 30 lines · Mat Martineau

In order to not limit ERTM and streaming mode PDUs to the HCI MTU size, L2CAP must be able to split PDUs in to multple HCI fragments. This is done by allocating space for the FCS in the last fragment.

Signed-off-by Mat Martineau <[email protected]>
---
 net/bluetooth/l2cap.c |   39 ++++++++++++++++++++++++++++++++++-----
 1 files changed, 34 insertions(+), 5 deletions(-)
net/bluetooth/l2cap.c +5 -0
index aa69c84..b485c4a 100644
@@ -1664,31 +1664,63 @@ static inline int l2cap_skbuff_fromiovec(struct sock *sk, struct msghdr *msg, in
 {
 	struct l2cap_conn *conn = l2cap_pi(sk)->conn;
 	struct sk_buff **frag;
+	struct sk_buff *final;
 	int err, sent = 0;
 
+	BT_DBG("sk %p, msg %p, len %d, count %d, skb %p", sk,
+		msg, (int)len, (int)count, skb);
+
 	if (memcpy_fromiovec(skb_put(skb, count), msg->msg_iov, count))
 		return -EFAULT;
 
 	sent += count;
 	len  -= count;
+	final = skb;

net/bluetooth/l2cap.c static inline int l2cap_skbuff_fromiovec(struct sock *sk, struct msghdr *msg, in · line 1688

Quoted 10 lines
net/bluetooth/l2cap.c +4 -1
 	/* Continuation fragments (no L2CAP header) */
 	frag = &skb_shinfo(skb)->frag_list;
 	while (len) {
+		int skblen;
 		count = min_t(unsigned int, conn->mtu, len);
 
-		*frag = bt_skb_send_alloc(sk, count, msg->msg_flags & MSG_DONTWAIT, &err);
+		/* Add room for the FCS if it fits */
+		if (l2cap_pi(sk)->fcs == L2CAP_FCS_CRC16 &&
+		    len + L2CAP_FCS_SIZE <= conn->mtu)

You don't need to check for (len + L2CAP_FCS_SIZE <= conn=mtu) here. Section 5.1 point that:

"Unlike the B-Frame length field, the I-frame length field may be greater than the configured MTU because it includes the octet lengths of the Control, L2CAP SDU Length (when present), and frame check sequence fields as well as the Information octets."

Quoted 59 lines

From that I understand "len <=" and not "len + L2CAP_FCS_SIZE <="

net/bluetooth/l2cap.c +25 -4
+			skblen = count + L2CAP_FCS_SIZE;
+		else
+			skblen = count;
+
+		*frag = bt_skb_send_alloc(sk, skblen,
+					msg->msg_flags & MSG_DONTWAIT, &err);
 		if (!*frag)
 			return -EFAULT;
-		if (memcpy_fromiovec(skb_put(*frag, count), msg->msg_iov, count))
+
+		if (memcpy_fromiovec(skb_put(*frag, count),
+				msg->msg_iov, count))
 			return -EFAULT;
 
 		sent += count;
 		len  -= count;
 
+		final = *frag;
+
 		frag = &(*frag)->next;
 	}
 
+	if (l2cap_pi(sk)->fcs == L2CAP_FCS_CRC16) {
+		if (skb_tailroom(final) < L2CAP_FCS_SIZE) {
+			*frag = bt_skb_send_alloc(sk, L2CAP_FCS_SIZE,
+						msg->msg_flags & MSG_DONTWAIT,
+						&err);
+			if (!*frag)
+				return -EFAULT;
+
+			final = *frag;
+		}
+
+		skb_put(final, L2CAP_FCS_SIZE);
+	}
+
 	return sent;
 }

@@ -1790,9 +1822,6 @@ static struct sk_buff *l2cap_create_iframe_pdu(struct sock *sk, struct msghdr *m
 		return ERR_PTR(err);
 	}
 
-	if (l2cap_pi(sk)->fcs == L2CAP_FCS_CRC16)
-		put_unaligned_le16(0, skb_put(skb, 2));
-
 	bt_cb(skb)->retries = 0;
 	return skb;
 }
Signature · 6 lines
1.7.1

--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
Signature · 2 lines
Gustavo F. Padovan
http://padovan.org
#12 lore raw

Re: [RFC 3/7] Bluetooth: Add FCS awareness to L2CAP HCI fragmentation.

Hi Mat,

Quoted 46 lines · Gustavo F. Padovan
Quoted 30 lines · Mat Martineau

In order to not limit ERTM and streaming mode PDUs to the HCI MTU size, L2CAP must be able to split PDUs in to multple HCI fragments. This is done by allocating space for the FCS in the last fragment.

Signed-off-by Mat Martineau <[email protected]>
---
 net/bluetooth/l2cap.c |   39 ++++++++++++++++++++++++++++++++++-----
 1 files changed, 34 insertions(+), 5 deletions(-)
net/bluetooth/l2cap.c +5 -0
index aa69c84..b485c4a 100644
@@ -1664,31 +1664,63 @@ static inline int l2cap_skbuff_fromiovec(struct sock *sk, struct msghdr *msg, in
 {
 	struct l2cap_conn *conn = l2cap_pi(sk)->conn;
 	struct sk_buff **frag;
+	struct sk_buff *final;
 	int err, sent = 0;
 
+	BT_DBG("sk %p, msg %p, len %d, count %d, skb %p", sk,
+		msg, (int)len, (int)count, skb);
+
 	if (memcpy_fromiovec(skb_put(skb, count), msg->msg_iov, count))
 		return -EFAULT;
 
 	sent += count;
 	len  -= count;
+	final = skb;

net/bluetooth/l2cap.c static inline int l2cap_skbuff_fromiovec(struct sock *sk, struct msghdr *msg, in · line 1688

Quoted 10 lines
net/bluetooth/l2cap.c +4 -1
 	/* Continuation fragments (no L2CAP header) */
 	frag = &skb_shinfo(skb)->frag_list;
 	while (len) {
+		int skblen;
 		count = min_t(unsigned int, conn->mtu, len);
 
-		*frag = bt_skb_send_alloc(sk, count, msg->msg_flags & MSG_DONTWAIT, &err);
+		/* Add room for the FCS if it fits */
+		if (l2cap_pi(sk)->fcs == L2CAP_FCS_CRC16 &&
+		    len + L2CAP_FCS_SIZE <= conn->mtu)

You don't need to check for (len + L2CAP_FCS_SIZE <= conn=mtu) here. Section 5.1 point that:

Quoted 6 lines

"Unlike the B-Frame length field, the I-frame length field may be greater than the configured MTU because it includes the octet lengths of the Control, L2CAP SDU Length (when present), and frame check sequence fields as well as the Information octets."

From that I understand "len <=" and not "len + L2CAP_FCS_SIZE <="

So here you might want to the check if we support FCS, and then add 2 to skblen.

Quoted 24 lines
Quoted 24 lines
net/bluetooth/l2cap.c +11 -1
+			skblen = count + L2CAP_FCS_SIZE;
+		else
+			skblen = count;
+
+		*frag = bt_skb_send_alloc(sk, skblen,
+					msg->msg_flags & MSG_DONTWAIT, &err);
 		if (!*frag)
 			return -EFAULT;
-		if (memcpy_fromiovec(skb_put(*frag, count), msg->msg_iov, count))
+
+		if (memcpy_fromiovec(skb_put(*frag, count),
+				msg->msg_iov, count))
 			return -EFAULT;
 
 		sent += count;
 		len  -= count;
 
+		final = *frag;
+
 		frag = &(*frag)->next;
 	}
Quoted 5 lines

net/bluetooth/l2cap.c static inline int l2cap_skbuff_fromiovec(struct sock *sk, struct msghdr *msg, in · line 1714

Quoted 5 lines
net/bluetooth/l2cap.c +5 -0
+	if (l2cap_pi(sk)->fcs == L2CAP_FCS_CRC16) {
+		if (skb_tailroom(final) < L2CAP_FCS_SIZE) {
+			*frag = bt_skb_send_alloc(sk, L2CAP_FCS_SIZE,
+						msg->msg_flags & MSG_DONTWAIT,
+						&err);

Why do we need to check for FCS again? We already added it required space to the last fragment.

Signature · 2 lines
Gustavo F. Padovan
http://padovan.org
author
#13 lore raw

Re: [RFC 3/7] Bluetooth: Add FCS awareness to L2CAP HCI fragmentation.

Quoted 55 lines · Gustavo F. Padovan

Hi Mat,

Quoted 51 lines · Gustavo F. Padovan
Quoted 30 lines · Mat Martineau

In order to not limit ERTM and streaming mode PDUs to the HCI MTU size, L2CAP must be able to split PDUs in to multple HCI fragments. This is done by allocating space for the FCS in the last fragment.

Signed-off-by Mat Martineau <[email protected]>
---
 net/bluetooth/l2cap.c |   39 ++++++++++++++++++++++++++++++++++-----
 1 files changed, 34 insertions(+), 5 deletions(-)
net/bluetooth/l2cap.c +5 -0
index aa69c84..b485c4a 100644
@@ -1664,31 +1664,63 @@ static inline int l2cap_skbuff_fromiovec(struct sock *sk, struct msghdr *msg, in
 {
 	struct l2cap_conn *conn = l2cap_pi(sk)->conn;
 	struct sk_buff **frag;
+	struct sk_buff *final;
 	int err, sent = 0;
 
+	BT_DBG("sk %p, msg %p, len %d, count %d, skb %p", sk,
+		msg, (int)len, (int)count, skb);
+
 	if (memcpy_fromiovec(skb_put(skb, count), msg->msg_iov, count))
 		return -EFAULT;
 
 	sent += count;
 	len  -= count;
+	final = skb;

net/bluetooth/l2cap.c static inline int l2cap_skbuff_fromiovec(struct sock *sk, struct msghdr *msg, in · line 1688

Quoted 10 lines
net/bluetooth/l2cap.c +4 -1
 	/* Continuation fragments (no L2CAP header) */
 	frag = &skb_shinfo(skb)->frag_list;
 	while (len) {
+		int skblen;
 		count = min_t(unsigned int, conn->mtu, len);
 
-		*frag = bt_skb_send_alloc(sk, count, msg->msg_flags & MSG_DONTWAIT, &err);
+		/* Add room for the FCS if it fits */
+		if (l2cap_pi(sk)->fcs == L2CAP_FCS_CRC16 &&
+		    len + L2CAP_FCS_SIZE <= conn->mtu)

You don't need to check for (len + L2CAP_FCS_SIZE <= conn=mtu) here. Section 5.1 point that:

"Unlike the B-Frame length field, the I-frame length field may be greater than the configured MTU because it includes the octet lengths of the Control, L2CAP SDU Length (when present), and frame check sequence fields as well as the Information octets."

Quoted 4 lines
Quoted 1 line

From that I understand "len <=" and not "len + L2CAP_FCS_SIZE <="

So here you might want to the check if we support FCS, and then add 2 to skblen.

That's what the code does, without violating the HCI MTU.

Quoted 22 lines
Quoted 22 lines
Quoted 22 lines
net/bluetooth/l2cap.c +11 -1
+			skblen = count + L2CAP_FCS_SIZE;
+		else
+			skblen = count;
+
+		*frag = bt_skb_send_alloc(sk, skblen,
+					msg->msg_flags & MSG_DONTWAIT, &err);
 		if (!*frag)
 			return -EFAULT;
-		if (memcpy_fromiovec(skb_put(*frag, count), msg->msg_iov, count))
+
+		if (memcpy_fromiovec(skb_put(*frag, count),
+				msg->msg_iov, count))
 			return -EFAULT;
 
 		sent += count;
 		len  -= count;
 
+		final = *frag;
+
 		frag = &(*frag)->next;
 	}
Quoted 9 lines
Quoted 5 lines

net/bluetooth/l2cap.c static inline int l2cap_skbuff_fromiovec(struct sock *sk, struct msghdr *msg, in · line 1714

Quoted 5 lines
net/bluetooth/l2cap.c +5 -0
+	if (l2cap_pi(sk)->fcs == L2CAP_FCS_CRC16) {
+		if (skb_tailroom(final) < L2CAP_FCS_SIZE) {
+			*frag = bt_skb_send_alloc(sk, L2CAP_FCS_SIZE,
+						msg->msg_flags & MSG_DONTWAIT,
+						&err);

Why do we need to check for FCS again? We already added it required space to the last fragment.

If there was room in the HCI fragment, count+2 bytes were allocated with bt_skb_send_alloc. However, only count bytes were used by skb_put.

This final block of code is doing two things:

1. Allocating a final HCI fragment for the FCS if there was not room
for the FCS in the last data fragment.

2. Doing the skb_put for the FCS bytes.

I will add some comments to this code to help clarify what's going on.

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

#14 lore raw

Re: [RFC 3/7] Bluetooth: Add FCS awareness to L2CAP HCI fragmentation.

Hi Mat,

Quoted 59 lines · Mat Martineau
Quoted 55 lines · Gustavo F. Padovan

Hi Mat,

Quoted 51 lines · Gustavo F. Padovan
Quoted 30 lines · Mat Martineau

In order to not limit ERTM and streaming mode PDUs to the HCI MTU size, L2CAP must be able to split PDUs in to multple HCI fragments. This is done by allocating space for the FCS in the last fragment.

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

---
net/bluetooth/l2cap.c | 39 ++++++++++++++++++++++++++++++++++-----

1 files changed, 34 insertions(+), 5 deletions(-)
net/bluetooth/l2cap.c +5 -0
index aa69c84..b485c4a 100644
@@ -1664,31 +1664,63 @@ static inline int l2cap_skbuff_fromiovec(struct sock *sk, struct msghdr *msg, in
 {
 	struct l2cap_conn *conn = l2cap_pi(sk)->conn;
 	struct sk_buff **frag;
+	struct sk_buff *final;
 	int err, sent = 0;
 
+	BT_DBG("sk %p, msg %p, len %d, count %d, skb %p", sk,
+		msg, (int)len, (int)count, skb);
+
 	if (memcpy_fromiovec(skb_put(skb, count), msg->msg_iov, count))
 		return -EFAULT;
 
 	sent += count;
 	len  -= count;
+	final = skb;

net/bluetooth/l2cap.c static inline int l2cap_skbuff_fromiovec(struct sock *sk, struct msghdr *msg, in · line 1688

Quoted 10 lines
net/bluetooth/l2cap.c +4 -1
 	/* Continuation fragments (no L2CAP header) */
 	frag = &skb_shinfo(skb)->frag_list;
 	while (len) {
+		int skblen;
 		count = min_t(unsigned int, conn->mtu, len);
 
-		*frag = bt_skb_send_alloc(sk, count, msg->msg_flags & MSG_DONTWAIT, &err);
+		/* Add room for the FCS if it fits */
+		if (l2cap_pi(sk)->fcs == L2CAP_FCS_CRC16 &&
+		    len + L2CAP_FCS_SIZE <= conn->mtu)

You don't need to check for (len + L2CAP_FCS_SIZE <= conn=mtu) here. Section 5.1 point that:

"Unlike the B-Frame length field, the I-frame length field may be greater than the configured MTU because it includes the octet lengths of the Control, L2CAP SDU Length (when present), and frame check sequence fields as well as the Information octets."

Quoted 6 lines
Quoted 4 lines
Quoted 1 line

From that I understand "len <=" and not "len + L2CAP_FCS_SIZE <="

So here you might want to the check if we support FCS, and then add 2 to skblen.

That's what the code does, without violating the HCI MTU.

Yes, you are right!

Quoted 37 lines
Quoted 23 lines
Quoted 23 lines
Quoted 23 lines
net/bluetooth/l2cap.c +11 -1
+			skblen = count + L2CAP_FCS_SIZE;
+		else
+			skblen = count;
+
+		*frag = bt_skb_send_alloc(sk, skblen,
+					msg->msg_flags & MSG_DONTWAIT, &err);
 		if (!*frag)
 			return -EFAULT;
-		if (memcpy_fromiovec(skb_put(*frag, count), msg->msg_iov, count))
+
+		if (memcpy_fromiovec(skb_put(*frag, count),
+				msg->msg_iov, count))
 			return -EFAULT;
 
 		sent += count;
 		len  -= count;
 
+		final = *frag;
+
 		frag = &(*frag)->next;
 	}
Quoted 9 lines
Quoted 5 lines

net/bluetooth/l2cap.c static inline int l2cap_skbuff_fromiovec(struct sock *sk, struct msghdr *msg, in · line 1714

Quoted 5 lines
net/bluetooth/l2cap.c +5 -0
+	if (l2cap_pi(sk)->fcs == L2CAP_FCS_CRC16) {
+		if (skb_tailroom(final) < L2CAP_FCS_SIZE) {
+			*frag = bt_skb_send_alloc(sk, L2CAP_FCS_SIZE,
+						msg->msg_flags & MSG_DONTWAIT,
+						&err);

Why do we need to check for FCS again? We already added it required space to the last fragment.

If there was room in the HCI fragment, count+2 bytes were allocated with bt_skb_send_alloc. However, only count bytes were used by skb_put.

Quoted 8 lines

This final block of code is doing two things:

1. Allocating a final HCI fragment for the FCS if there was not
room for the FCS in the last data fragment.

2. Doing the skb_put for the FCS bytes.

I will add some comments to this code to help clarify what's going on.

Ok, and do the other way around in the if:

	if (l2cap_pi(sk)->fcs != L2CAP_FCS_CRC16)
		return sent;

	if (skb_tailroom(...
Signature · 2 lines
Gustavo F. Padovan
http://padovan.org
author
#15 lore raw

Re: [RFC 3/7] Bluetooth: Add FCS awareness to L2CAP HCI fragmentation.

In reply to Gustavo F. Padovan
Quoted 46 lines · Gustavo F. Padovan
Quoted 30 lines · Mat Martineau

In order to not limit ERTM and streaming mode PDUs to the HCI MTU size, L2CAP must be able to split PDUs in to multple HCI fragments. This is done by allocating space for the FCS in the last fragment.

Signed-off-by Mat Martineau <[email protected]>
---
 net/bluetooth/l2cap.c |   39 ++++++++++++++++++++++++++++++++++-----
 1 files changed, 34 insertions(+), 5 deletions(-)
net/bluetooth/l2cap.c +5 -0
index aa69c84..b485c4a 100644
@@ -1664,31 +1664,63 @@ static inline int l2cap_skbuff_fromiovec(struct sock *sk, struct msghdr *msg, in
 {
 	struct l2cap_conn *conn = l2cap_pi(sk)->conn;
 	struct sk_buff **frag;
+	struct sk_buff *final;
 	int err, sent = 0;
 
+	BT_DBG("sk %p, msg %p, len %d, count %d, skb %p", sk,
+		msg, (int)len, (int)count, skb);
+
 	if (memcpy_fromiovec(skb_put(skb, count), msg->msg_iov, count))
 		return -EFAULT;
 
 	sent += count;
 	len  -= count;
+	final = skb;

net/bluetooth/l2cap.c static inline int l2cap_skbuff_fromiovec(struct sock *sk, struct msghdr *msg, in · line 1688

Quoted 10 lines
net/bluetooth/l2cap.c +4 -1
 	/* Continuation fragments (no L2CAP header) */
 	frag = &skb_shinfo(skb)->frag_list;
 	while (len) {
+		int skblen;
 		count = min_t(unsigned int, conn->mtu, len);
 
-		*frag = bt_skb_send_alloc(sk, count, msg->msg_flags & MSG_DONTWAIT, &err);
+		/* Add room for the FCS if it fits */
+		if (l2cap_pi(sk)->fcs == L2CAP_FCS_CRC16 &&
+		    len + L2CAP_FCS_SIZE <= conn->mtu)

You don't need to check for (len + L2CAP_FCS_SIZE <= conn=mtu) here. Section 5.1 point that:

Quoted 6 lines

"Unlike the B-Frame length field, the I-frame length field may be greater than the configured MTU because it includes the octet lengths of the Control, L2CAP SDU Length (when present), and frame check sequence fields as well as the Information octets."

From that I understand "len <=" and not "len + L2CAP_FCS_SIZE <="

This is checking against the HCI MTU, not the L2CAP MTU. Any L2CAP payload (headers, data, or FCS) needs to fit in the HCI fragment. The idea here is to see if len is short enough that the FCS can be included in this HCI fragment after the L2CAP data.

Quoted 43 lines
Quoted 43 lines
net/bluetooth/l2cap.c +25 -1
+			skblen = count + L2CAP_FCS_SIZE;
+		else
+			skblen = count;
+
+		*frag = bt_skb_send_alloc(sk, skblen,
+					msg->msg_flags & MSG_DONTWAIT, &err);
 		if (!*frag)
 			return -EFAULT;
-		if (memcpy_fromiovec(skb_put(*frag, count), msg->msg_iov, count))
+
+		if (memcpy_fromiovec(skb_put(*frag, count),
+				msg->msg_iov, count))
 			return -EFAULT;
 
 		sent += count;
 		len  -= count;
 
+		final = *frag;
+
 		frag = &(*frag)->next;
 	}
 
+	if (l2cap_pi(sk)->fcs == L2CAP_FCS_CRC16) {
+		if (skb_tailroom(final) < L2CAP_FCS_SIZE) {
+			*frag = bt_skb_send_alloc(sk, L2CAP_FCS_SIZE,
+						msg->msg_flags & MSG_DONTWAIT,
+						&err);
+			if (!*frag)
+				return -EFAULT;
+
+			final = *frag;
+		}
+
+		skb_put(final, L2CAP_FCS_SIZE);
+	}
+
 	return sent;
 }
 

@@ -1790,9 +1822,6 @@ static struct sk_buff *l2cap_create_iframe_pdu(struct sock *sk, struct msghdr *m
 		return ERR_PTR(err);
 	}
Quoted 8 lines
Quoted 8 lines
net/bluetooth/l2cap.c +0 -4
-	if (l2cap_pi(sk)->fcs == L2CAP_FCS_CRC16)
-		put_unaligned_le16(0, skb_put(skb, 2));
-
 	bt_cb(skb)->retries = 0;
 	return skb;
 }
--

1.7.1

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

author
#16 lore raw

RFC 4/7 Bluetooth: Linearize received L2CAP skbuffs.

In reply to Mat Martineau

Fragmented skbs are only encountered by in-kernel protocols and profiles when receiving ERTM or streaming mode L2CAP data. BNEP, CMTP, HIDP, and RFCOMM generally use basic mode, so will not generally need the extra memcpy's necessary to put the L2CAP payload in to a linear buffer. However, it is possible to use enhanced L2CAP in these modes so the possibility needs to be handled.

Signed-off-by Mat Martineau <[email protected]>
---
 net/bluetooth/bnep/core.c   |    5 ++++-
 net/bluetooth/cmtp/core.c   |    5 ++++-
 net/bluetooth/hidp/core.c   |   10 ++++++++--
 net/bluetooth/rfcomm/core.c |    5 ++++-
 4 files changed, 20 insertions(+), 5 deletions(-)
net/bluetooth/bnep/core.c +4 -1
index f10b41f..9f60443 100644
@@ -481,7 +481,10 @@ static int bnep_session(void *arg)
 		// RX
 		while ((skb = skb_dequeue(&sk->sk_receive_queue))) {
 			skb_orphan(skb);
-			bnep_rx_frame(s, skb);
+			if (!skb_linearize(skb))
+				bnep_rx_frame(s, skb);
+			else
+				kfree_skb(skb);
 		}
 
 		if (sk->sk_state != BT_CONNECTED)
net/bluetooth/cmtp/core.c +4 -1
index d4c6af0..48ac812 100644
@@ -293,7 +293,10 @@ static int cmtp_session(void *arg)
 
 		while ((skb = skb_dequeue(&sk->sk_receive_queue))) {
 			skb_orphan(skb);
-			cmtp_recv_frame(session, skb);
+			if (!skb_linearize(skb))
+				cmtp_recv_frame(session, skb);
+			else
+				kfree_skb(skb);
 		}
 
 		cmtp_process_transmit(session);
net/bluetooth/hidp/core.c +8 -2
index bfe641b..097ff25 100644
@@ -571,12 +571,18 @@ static int hidp_session(void *arg)
 
 		while ((skb = skb_dequeue(&ctrl_sk->sk_receive_queue))) {
 			skb_orphan(skb);
-			hidp_recv_ctrl_frame(session, skb);
+			if (!skb_linearize(skb))
+				hidp_recv_ctrl_frame(session, skb);
+			else
+				kfree_skb(skb);
 		}
 
 		while ((skb = skb_dequeue(&intr_sk->sk_receive_queue))) {
 			skb_orphan(skb);
-			hidp_recv_intr_frame(session, skb);
+			if (!skb_linearize(skb))
+				hidp_recv_intr_frame(session, skb);
+			else
+				kfree_skb(skb);
 		}
 
 		hidp_process_transmit(session);
net/bluetooth/rfcomm/core.c +4 -1
index 7dca91b..ee27f59 100644
@@ -1845,7 +1845,10 @@ static inline void rfcomm_process_rx(struct rfcomm_session *s)
 	/* Get data directly from socket receive queue without copying it. */
 	while ((skb = skb_dequeue(&sk->sk_receive_queue))) {
 		skb_orphan(skb);
-		rfcomm_recv_frame(s, skb);
+		if (!skb_linearize(skb))
+			rfcomm_recv_frame(s, skb);
+		else
+			kfree_skb(skb);
 	}
 
 	if (sk->sk_state == BT_CLOSED) {
Signature · 6 lines
1.7.1

--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
maintainer
#17 lore raw

Re: [RFC 4/7] Bluetooth: Linearize received L2CAP skbuffs.

Hi Mat,

Quoted 8 lines

Fragmented skbs are only encountered by in-kernel protocols and profiles when receiving ERTM or streaming mode L2CAP data. BNEP, CMTP, HIDP, and RFCOMM generally use basic mode, so will not generally need the extra memcpy's necessary to put the L2CAP payload in to a linear buffer. However, it is possible to use enhanced L2CAP in these modes so the possibility needs to be handled.

Quoted 7 lines
Signed-off-by Mat Martineau <[email protected]>
---
 net/bluetooth/bnep/core.c   |    5 ++++-
 net/bluetooth/cmtp/core.c   |    5 ++++-
 net/bluetooth/hidp/core.c   |   10 ++++++++--
 net/bluetooth/rfcomm/core.c |    5 ++++-
 4 files changed, 20 insertions(+), 5 deletions(-)

in general I am fine with this.

Regards

Marcel

#18 lore raw

Re: [RFC 4/7] Bluetooth: Linearize received L2CAP skbuffs.

Hi Mat,

Quoted 18 lines · Marcel Holtmann

Hi Mat,

Quoted 8 lines

Fragmented skbs are only encountered by in-kernel protocols and profiles when receiving ERTM or streaming mode L2CAP data. BNEP, CMTP, HIDP, and RFCOMM generally use basic mode, so will not generally need the extra memcpy's necessary to put the L2CAP payload in to a linear buffer. However, it is possible to use enhanced L2CAP in these modes so the possibility needs to be handled.

Quoted 7 lines
Signed-off-by Mat Martineau <[email protected]>
---
 net/bluetooth/bnep/core.c   |    5 ++++-
 net/bluetooth/cmtp/core.c   |    5 ++++-
 net/bluetooth/hidp/core.c   |   10 ++++++++--
 net/bluetooth/rfcomm/core.c |    5 ++++-
 4 files changed, 20 insertions(+), 5 deletions(-)
Quoted 1 line

in general I am fine with this.

Same from me.

Signature · 2 lines
Gustavo F. Padovan
http://padovan.org
author
#19 lore raw

RFC 5/7 Bluetooth: Handle fragmented skbs in bt_sock_stream_recvmsg()

In reply to Mat Martineau

When reading L2CAP skbuffs, add the ability to copy from fragmented skbuffs generated during ERTM or streaming mode reassembly. This defers extra copying of L2CAP payloads until the final, unavoidable copy to userspace.

Signed-off-by Mat Martineau <[email protected]>
---
 net/bluetooth/af_bluetooth.c |   30 ++++++++++++++++++++++++++++--
 1 files changed, 28 insertions(+), 2 deletions(-)
net/bluetooth/af_bluetooth.c +28 -2
index 77a26fe..5e0375b 100644
@@ -342,7 +342,7 @@ int bt_sock_stream_recvmsg(struct kiocb *iocb, struct socket *sock,
 		}
 
 		chunk = min_t(unsigned int, skb->len, size);
-		if (memcpy_toiovec(msg->msg_iov, skb->data, chunk)) {
+		if (skb_copy_datagram_iovec(skb, 0, msg->msg_iov, chunk)) {
 			skb_queue_head(&sk->sk_receive_queue, skb);
 			if (!copied)
 				copied = -EFAULT;

@@ -354,7 +354,33 @@ int bt_sock_stream_recvmsg(struct kiocb *iocb, struct socket *sock,
 		sock_recv_ts_and_drops(msg, sk, skb);
 
 		if (!(flags & MSG_PEEK)) {
-			skb_pull(skb, chunk);
+			int skb_len = skb_headlen(skb);
+
+			if (chunk <= skb_len) {
+				__skb_pull(skb, chunk);
+			} else {
+				struct sk_buff *frag;
+
+				__skb_pull(skb, skb_len);
+				chunk -= skb_len;
+
+				skb_walk_frags(skb, frag) {
+					if (chunk <= frag->len) {
+						/* Pulling partial data */
+						skb->len -= chunk;
+						skb->data_len -= chunk;
+						__skb_pull(frag, chunk);
+						break;
+					} else if (frag->len) {
+						/* Pulling all frag data */
+						chunk -= frag->len;
+						skb->len -= frag->len;
+						skb->data_len -= frag->len;
+						__skb_pull(frag, frag->len);
+					}
+				}
+			}
+
 			if (skb->len) {
 				skb_queue_head(&sk->sk_receive_queue, skb);
 				break;
Signature · 6 lines
1.7.1

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

Re: [RFC 5/7] Bluetooth: Handle fragmented skbs in bt_sock_stream_recvmsg()

Hi Mat,

Quoted 24 lines · Mat Martineau

When reading L2CAP skbuffs, add the ability to copy from fragmented skbuffs generated during ERTM or streaming mode reassembly. This defers extra copying of L2CAP payloads until the final, unavoidable copy to userspace.

Signed-off-by Mat Martineau <[email protected]>
---
 net/bluetooth/af_bluetooth.c |   30 ++++++++++++++++++++++++++++--
 1 files changed, 28 insertions(+), 2 deletions(-)
net/bluetooth/af_bluetooth.c +1 -1
index 77a26fe..5e0375b 100644
@@ -342,7 +342,7 @@ int bt_sock_stream_recvmsg(struct kiocb *iocb, struct socket *sock,
 		}
 
 		chunk = min_t(unsigned int, skb->len, size);
-		if (memcpy_toiovec(msg->msg_iov, skb->data, chunk)) {
+		if (skb_copy_datagram_iovec(skb, 0, msg->msg_iov, chunk)) {
 			skb_queue_head(&sk->sk_receive_queue, skb);
 			if (!copied)
 				copied = -EFAULT;

net/bluetooth/af_bluetooth.c int bt_sock_stream_recvmsg(struct kiocb *iocb, struct socket *sock, · line 357

Quoted 5 lines
net/bluetooth/af_bluetooth.c +1 -1
@@ -354,7 +354,33 @@ int bt_sock_stream_recvmsg(struct kiocb *iocb, struct socket *sock,
 		sock_recv_ts_and_drops(msg, sk, skb);
 
 		if (!(flags & MSG_PEEK)) {
-			skb_pull(skb, chunk);
+			int skb_len = skb_headlen(skb);

Why are you using the header length here?

net/bluetooth/af_bluetooth.c int bt_sock_stream_recvmsg(struct kiocb *iocb, struct socket *sock, · line 365

Quoted 8 lines
net/bluetooth/af_bluetooth.c +8 -0
+
+			if (chunk <= skb_len) {
+				__skb_pull(skb, chunk);
+			} else {
+				struct sk_buff *frag;
+
+				__skb_pull(skb, skb_len);
+				chunk -= skb_len;

Why do we have this __skb_pull() here? I think skb_walk_frags() can handle everything.

net/bluetooth/af_bluetooth.c int bt_sock_stream_recvmsg(struct kiocb *iocb, struct socket *sock, · line 373

Quoted 8 lines
net/bluetooth/af_bluetooth.c +8 -0
+
+				skb_walk_frags(skb, frag) {
+					if (chunk <= frag->len) {
+						/* Pulling partial data */
+						skb->len -= chunk;
+						skb->data_len -= chunk;
+						__skb_pull(frag, chunk);
+						break;

If we break here what will happen whit the rest of the data to be pulled.

Quoted 20 lines
net/bluetooth/af_bluetooth.c +10 -0
+					} else if (frag->len) {
+						/* Pulling all frag data */
+						chunk -= frag->len;
+						skb->len -= frag->len;
+						skb->data_len -= frag->len;
+						__skb_pull(frag, frag->len);
+					}
+				}
+			}
+
 			if (skb->len) {
 				skb_queue_head(&sk->sk_receive_queue, skb);
 				break;
Signature · 6 lines
1.7.1

--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
Signature · 2 lines
Gustavo F. Padovan
http://padovan.org
author
#21 lore raw

Re: [RFC 5/7] Bluetooth: Handle fragmented skbs in bt_sock_stream_recvmsg()

Quoted 30 lines · Gustavo F. Padovan

Hi Mat,

Quoted 26 lines · Mat Martineau

When reading L2CAP skbuffs, add the ability to copy from fragmented skbuffs generated during ERTM or streaming mode reassembly. This defers extra copying of L2CAP payloads until the final, unavoidable copy to userspace.

Signed-off-by Mat Martineau <[email protected]>
---
 net/bluetooth/af_bluetooth.c |   30 ++++++++++++++++++++++++++++--
 1 files changed, 28 insertions(+), 2 deletions(-)
net/bluetooth/af_bluetooth.c +1 -1
index 77a26fe..5e0375b 100644
@@ -342,7 +342,7 @@ int bt_sock_stream_recvmsg(struct kiocb *iocb, struct socket *sock,
 		}
 
 		chunk = min_t(unsigned int, skb->len, size);
-		if (memcpy_toiovec(msg->msg_iov, skb->data, chunk)) {
+		if (skb_copy_datagram_iovec(skb, 0, msg->msg_iov, chunk)) {
 			skb_queue_head(&sk->sk_receive_queue, skb);
 			if (!copied)
 				copied = -EFAULT;

@@ -354,7 +354,33 @@ int bt_sock_stream_recvmsg(struct kiocb *iocb, struct socket *sock,
 		sock_recv_ts_and_drops(msg, sk, skb);
Quoted 5 lines

net/bluetooth/af_bluetooth.c int bt_sock_stream_recvmsg(struct kiocb *iocb, struct socket *sock, · line 357

Quoted 3 lines
net/bluetooth/af_bluetooth.c +1 -1
 		if (!(flags & MSG_PEEK)) {
-			skb_pull(skb, chunk);
+			int skb_len = skb_headlen(skb);

Why are you using the header length here?

skb_headlen() returns the length of the first fragment.

Quoted 11 lines

net/bluetooth/af_bluetooth.c int bt_sock_stream_recvmsg(struct kiocb *iocb, struct socket *sock, · line 365

Quoted 8 lines
net/bluetooth/af_bluetooth.c +8 -0
+
+			if (chunk <= skb_len) {
+				__skb_pull(skb, chunk);
+			} else {
+				struct sk_buff *frag;
+
+				__skb_pull(skb, skb_len);
+				chunk -= skb_len;

Why do we have this __skb_pull() here? I think skb_walk_frags() can handle everything.

That first __skb_pull() is necessary to deal with data in the first skbuff, and the skb_walk_frags() deals with anything in skb_shinfo(skb)->frag_list. The linked skb list looks roughly like this:

skb
unsigned char *data -> (L2CAP data)
struct sk_buff *frag_list -> 2nd frag

                                unsigned char *data -> (more data)
                                struct sk_buff *next -> 3rd frag
                                                          (etc.)

So the first frag is special - the pointer to the fragment is frag_list. Each linked fragment after that uses the "next" pointer in the sk_buff struct. The skb_walk_frags() macro starts with the sk_buff pointed to by frag_list, then follows the "next" links.

Quoted 11 lines

net/bluetooth/af_bluetooth.c int bt_sock_stream_recvmsg(struct kiocb *iocb, struct socket *sock, · line 373

Quoted 8 lines
net/bluetooth/af_bluetooth.c +8 -0
+
+				skb_walk_frags(skb, frag) {
+					if (chunk <= frag->len) {
+						/* Pulling partial data */
+						skb->len -= chunk;
+						skb->data_len -= chunk;
+						__skb_pull(frag, chunk);
+						break;

If we break here what will happen whit the rest of the data to be pulled.

The data is left to be pulled later. The skb is not freed until all the fragments are empty, and this is handled by existing code. I've added the next couple of lines of context below to help explain.

Quoted 14 lines
Quoted 4 lines
net/bluetooth/af_bluetooth.c +3 -0
+					} else if (frag->len) {
+						/* Pulling all frag data */
+						chunk -= frag->len;

net/bluetooth/af_bluetooth.c int bt_sock_stream_recvmsg(struct kiocb *iocb, struct socket *sock, · line 386

Quoted 10 lines
net/bluetooth/af_bluetooth.c +7 -0
+						skb->len -= frag->len;
+						skb->data_len -= frag->len;
+						__skb_pull(frag, frag->len);
+					}
+				}
+			}
+
 			if (skb->len) {
 				skb_queue_head(&sk->sk_receive_queue, skb);
 				break;

                        }
                        kfree_skb(skb);

If the skb is not empty, it's pushed back on the head of the receive queue. If it is empty, it's freed.

I based this approach on some code I found in the SCTP protocol.

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

author
#22 lore raw

RFC 6/7 Bluetooth: Reassemble enhanced L2CAP PDUs using skb fragments.

In reply to Mat Martineau

As enhanced L2CAP PDUs arrive, it is not necessary to copy them in to a separate skbuff. Instead, the skbuffs can be linked together as fragments, only being copied in to a linear buffer when the data is copied to userspace. This avoids the need to allocate additional buffers for incoming data, and eliminates copying of data payloads during SDU reassembly. This is of greater concern with high-speed AMP links than with BR/EDR.

Signed-off-by Mat Martineau <[email protected]>
---
 include/net/bluetooth/l2cap.h |    1 +
 net/bluetooth/l2cap.c         |   66 +++++++++++++++++++++-------------------
 2 files changed, 36 insertions(+), 31 deletions(-)
include/net/bluetooth/l2cap.h +1 -0
index 2f3222f..9384e87 100644
@@ -357,6 +357,7 @@ struct l2cap_pinfo {
 	__u16		sdu_len;
 	__u16		partial_sdu_len;
 	struct sk_buff	*sdu;
+	struct sk_buff	*sdu_last_frag;
 
 	__u8		ident;
net/bluetooth/l2cap.c +35 -31

122 lines not on this page · raw message

Signature · 6 lines
1.7.1

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

Re: [RFC 6/7] Bluetooth: Reassemble enhanced L2CAP PDUs using skb fragments.

Hi Mat,

Quoted 42 lines · Mat Martineau

As enhanced L2CAP PDUs arrive, it is not necessary to copy them in to a separate skbuff. Instead, the skbuffs can be linked together as fragments, only being copied in to a linear buffer when the data is copied to userspace. This avoids the need to allocate additional buffers for incoming data, and eliminates copying of data payloads during SDU reassembly. This is of greater concern with high-speed AMP links than with BR/EDR.

Signed-off-by Mat Martineau <[email protected]>
---
 include/net/bluetooth/l2cap.h |    1 +
 net/bluetooth/l2cap.c         |   66 +++++++++++++++++++++-------------------
 2 files changed, 36 insertions(+), 31 deletions(-)
include/net/bluetooth/l2cap.h +1 -0
index 2f3222f..9384e87 100644
@@ -357,6 +357,7 @@ struct l2cap_pinfo {
 	__u16		sdu_len;
 	__u16		partial_sdu_len;
 	struct sk_buff	*sdu;
+	struct sk_buff	*sdu_last_frag;
 
 	__u8		ident;
net/bluetooth/l2cap.c +3 -0
index b485c4a..0212035 100644
@@ -290,6 +290,9 @@ static void l2cap_chan_del(struct sock *sk, int err)
 		skb_queue_purge(SREJ_QUEUE(sk));
 		skb_queue_purge(BUSY_QUEUE(sk));
 
+		if (l2cap_pi(sk)->sdu)
+			kfree_skb(l2cap_pi(sk)->sdu);
+
 		list_for_each_entry_safe(l, tmp, SREJ_LIST(sk), list) {
 			list_del(&l->list);
 			kfree(l);

net/bluetooth/l2cap.c static int l2cap_add_to_srej_queue(struct sock *sk, struct sk_buff *skb, u8 tx_s · line 3642

Quoted 5 lines
net/bluetooth/l2cap.c +2 -0
@@ -3635,6 +3638,27 @@ static int l2cap_add_to_srej_queue(struct sock *sk, struct sk_buff *skb, u8 tx_s
 	return 0;
 }
 
+static inline void append_skb_frag(struct sk_buff *skb,
+			struct sk_buff *new_frag, struct sk_buff **last_frag)

Call this l2cap_append_skb_frag()

Quoted 9 lines
net/bluetooth/l2cap.c +9 -0
+{
+	/* skb->len reflects data in skb as well as all fragments
+	   skb->data_len reflects only data in fragments
+	 */
+	BT_DBG("skb %p, new_frag %p, *last_frag %p", skb, new_frag, *last_frag);
+
+	if (!skb_has_frags(skb))
+		skb_shinfo(skb)->frag_list = new_frag;
+

net/bluetooth/l2cap.c static int l2cap_add_to_srej_queue(struct sock *sk, struct sk_buff *skb, u8 tx_s · line 3661

Quoted 10 lines
net/bluetooth/l2cap.c +10 -0
+	new_frag->next = NULL;
+
+	(*last_frag)->next = new_frag;
+	*last_frag = new_frag;
+
+	skb->len += new_frag->len;
+	skb->data_len += new_frag->len;
+	skb->truesize += new_frag->truesize;
+}
+

Wondering if it is possible to do that append more simple, but I looked through the kernel code it's not possible, we have to keep last frag pointer.

This change should go to l2cap_streaming_reassembly_sdu() as well, then you can get ride of partial_sdu_len in the struct l2cap_pinfo.

net/bluetooth/l2cap.c static int l2cap_ertm_reassembly_sdu(struct sock *sk, struct sk_buff *skb, u16 c · line 3670

Quoted 9 lines
net/bluetooth/l2cap.c +1 -1
 static int l2cap_ertm_reassembly_sdu(struct sock *sk, struct sk_buff *skb, u16 control)
 {
 	struct l2cap_pinfo *pi = l2cap_pi(sk);

@@ -3643,7 +3667,7 @@ static int l2cap_ertm_reassembly_sdu(struct sock *sk, struct sk_buff *skb, u16 c
 
 	switch (control & L2CAP_CTRL_SAR) {
 	case L2CAP_SDU_UNSEGMENTED:
-		if (pi->conn_state & L2CAP_CONN_SAR_SDU)
+		if (pi->sdu)

pi->sdu can do the work work of pi->conn_state & L2CAP_CONN_SAR_SDU, so a separated patch for that change sounds better (you can change that for the Streaming mode at the same time, it's a similar code, but more simple).

Signature · 2 lines
Gustavo F. Padovan
http://padovan.org
author
#24 lore raw

Re: [RFC 6/7] Bluetooth: Reassemble enhanced L2CAP PDUs using skb fragments.

Gustavo -

Quoted 49 lines · Gustavo F. Padovan

Hi Mat,

Quoted 45 lines · Mat Martineau

As enhanced L2CAP PDUs arrive, it is not necessary to copy them in to a separate skbuff. Instead, the skbuffs can be linked together as fragments, only being copied in to a linear buffer when the data is copied to userspace. This avoids the need to allocate additional buffers for incoming data, and eliminates copying of data payloads during SDU reassembly. This is of greater concern with high-speed AMP links than with BR/EDR.

Signed-off-by Mat Martineau <[email protected]>
---
 include/net/bluetooth/l2cap.h |    1 +
 net/bluetooth/l2cap.c         |   66 +++++++++++++++++++++-------------------
 2 files changed, 36 insertions(+), 31 deletions(-)
include/net/bluetooth/l2cap.h +1 -0
index 2f3222f..9384e87 100644
@@ -357,6 +357,7 @@ struct l2cap_pinfo {
 	__u16		sdu_len;
 	__u16		partial_sdu_len;
 	struct sk_buff	*sdu;
+	struct sk_buff	*sdu_last_frag;
 
 	__u8		ident;
net/bluetooth/l2cap.c +3 -0
index b485c4a..0212035 100644
@@ -290,6 +290,9 @@ static void l2cap_chan_del(struct sock *sk, int err)
 		skb_queue_purge(SREJ_QUEUE(sk));
 		skb_queue_purge(BUSY_QUEUE(sk));
 
+		if (l2cap_pi(sk)->sdu)
+			kfree_skb(l2cap_pi(sk)->sdu);
+
 		list_for_each_entry_safe(l, tmp, SREJ_LIST(sk), list) {
 			list_del(&l->list);
 			kfree(l);

@@ -3635,6 +3638,27 @@ static int l2cap_add_to_srej_queue(struct sock *sk, struct sk_buff *skb, u8 tx_s
 	return 0;
 }
Quoted 4 lines

net/bluetooth/l2cap.c static int l2cap_add_to_srej_queue(struct sock *sk, struct sk_buff *skb, u8 tx_s · line 3642

Quoted 2 lines
net/bluetooth/l2cap.c +2 -0
+static inline void append_skb_frag(struct sk_buff *skb,
+			struct sk_buff *new_frag, struct sk_buff **last_frag)

Call this l2cap_append_skb_frag()

Ok.

Quoted 25 lines
Quoted 10 lines
net/bluetooth/l2cap.c +9 -0
+{
+	/* skb->len reflects data in skb as well as all fragments
+	   skb->data_len reflects only data in fragments
+	 */
+	BT_DBG("skb %p, new_frag %p, *last_frag %p", skb, new_frag, *last_frag);
+
+	if (!skb_has_frags(skb))
+		skb_shinfo(skb)->frag_list = new_frag;
+

net/bluetooth/l2cap.c static int l2cap_add_to_srej_queue(struct sock *sk, struct sk_buff *skb, u8 tx_s · line 3661

Quoted 10 lines
net/bluetooth/l2cap.c +10 -0
+	new_frag->next = NULL;
+
+	(*last_frag)->next = new_frag;
+	*last_frag = new_frag;
+
+	skb->len += new_frag->len;
+	skb->data_len += new_frag->len;
+	skb->truesize += new_frag->truesize;
+}
+

Wondering if it is possible to do that append more simple, but I looked through the kernel code it's not possible, we have to keep last frag pointer.

Quoted 2 lines

This change should go to l2cap_streaming_reassembly_sdu() as well, then you can get ride of partial_sdu_len in the struct l2cap_pinfo.

I haven't had a chance to test streaming mode, so I was avoiding changes to it so far. I agree that the same technique can be applied there.

Quoted 6 lines
Quoted 6 lines
net/bluetooth/l2cap.c +0 -0
 static int l2cap_ertm_reassembly_sdu(struct sock *sk, struct sk_buff *skb, u16 control)
 {
 	struct l2cap_pinfo *pi = l2cap_pi(sk);

@@ -3643,7 +3667,7 @@ static int l2cap_ertm_reassembly_sdu(struct sock *sk, struct sk_buff *skb, u16 c
Quoted 9 lines

net/bluetooth/l2cap.c static int l2cap_ertm_reassembly_sdu(struct sock *sk, struct sk_buff *skb, u16 c · line 3670

Quoted 4 lines
net/bluetooth/l2cap.c +1 -1
 	switch (control & L2CAP_CTRL_SAR) {
 	case L2CAP_SDU_UNSEGMENTED:
-		if (pi->conn_state & L2CAP_CONN_SAR_SDU)
+		if (pi->sdu)

pi->sdu can do the work work of pi->conn_state & L2CAP_CONN_SAR_SDU, so a separated patch for that change sounds better (you can change that for the Streaming mode at the same time, it's a similar code, but more simple).

Ok.

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

author
#25 lore raw

RFC 7/7 Bluetooth: Do not limit enhanced L2CAP max PDU size to HCI MTU.

In reply to Mat Martineau

HCI MTU and PDU size have no relationship, since ERTM and streaming PDUs can be fragmented over HCI just like other L2CAP frames. The only applicable limit for PDU size is the L2CAP MTU, which is now checked during L2CAP configuration.

Signed-off-by Mat Martineau <[email protected]>
---
 net/bluetooth/l2cap.c |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)
net/bluetooth/l2cap.c +8 -8
index 0212035..15fbb1d 100644
@@ -2621,8 +2621,8 @@ done:
 		rfc.retrans_timeout = 0;
 		rfc.monitor_timeout = 0;
 		rfc.max_pdu_size    = cpu_to_le16(L2CAP_DEFAULT_MAX_PDU_SIZE);
-		if (L2CAP_DEFAULT_MAX_PDU_SIZE > pi->conn->mtu - 10)
-			rfc.max_pdu_size = cpu_to_le16(pi->conn->mtu - 10);
+		if (L2CAP_DEFAULT_MAX_PDU_SIZE > pi->imtu)
+			rfc.max_pdu_size = cpu_to_le16(pi->imtu);
 
 		l2cap_add_conf_opt(&ptr, L2CAP_CONF_RFC, sizeof(rfc),
 							(unsigned long) &rfc);

@@ -2644,8 +2644,8 @@ done:
 		rfc.retrans_timeout = 0;
 		rfc.monitor_timeout = 0;
 		rfc.max_pdu_size    = cpu_to_le16(L2CAP_DEFAULT_MAX_PDU_SIZE);
-		if (L2CAP_DEFAULT_MAX_PDU_SIZE > pi->conn->mtu - 10)
-			rfc.max_pdu_size = cpu_to_le16(pi->conn->mtu - 10);
+		if (L2CAP_DEFAULT_MAX_PDU_SIZE > pi->imtu)
+			rfc.max_pdu_size = cpu_to_le16(pi->imtu);
 
 		l2cap_add_conf_opt(&ptr, L2CAP_CONF_RFC, sizeof(rfc),
 							(unsigned long) &rfc);

@@ -2778,8 +2778,8 @@ done:
 			pi->remote_tx_win = rfc.txwin_size;
 			pi->remote_max_tx = rfc.max_transmit;
 
-			if (le16_to_cpu(rfc.max_pdu_size) > pi->conn->mtu - 10)
-				rfc.max_pdu_size = cpu_to_le16(pi->conn->mtu - 10);
+			if (le16_to_cpu(rfc.max_pdu_size) > pi->omtu)
+				rfc.max_pdu_size = cpu_to_le16(pi->omtu);
 
 			pi->remote_mps = le16_to_cpu(rfc.max_pdu_size);
 

@@ -2796,8 +2796,8 @@ done:
 			break;
 
 		case L2CAP_MODE_STREAMING:
-			if (le16_to_cpu(rfc.max_pdu_size) > pi->conn->mtu - 10)
-				rfc.max_pdu_size = cpu_to_le16(pi->conn->mtu - 10);
+			if (le16_to_cpu(rfc.max_pdu_size) > pi->omtu)
+				rfc.max_pdu_size = cpu_to_le16(pi->omtu);
 
 			pi->remote_mps = le16_to_cpu(rfc.max_pdu_size);
Signature · 6 lines
1.7.1

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

Re: [RFC 7/7] Bluetooth: Do not limit enhanced L2CAP max PDU size to HCI MTU.

Hi Mat,

Quoted 9 lines · Mat Martineau

HCI MTU and PDU size have no relationship, since ERTM and streaming PDUs can be fragmented over HCI just like other L2CAP frames. The only applicable limit for PDU size is the L2CAP MTU, which is now checked during L2CAP configuration.

Signed-off-by Mat Martineau <[email protected]>
---
 net/bluetooth/l2cap.c |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)

This one is fine after the changes in the previous patches.

Signature · 2 lines
Gustavo F. Padovan
http://padovan.org