[RFC 0/7] L2CAP fragmentation changes

[RFC 0/7] L2CAP fragmentation changes

[Mat Martineau]

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.

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

[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 <mathewm@codeaurora.org>
---
 net/bluetooth/l2cap.c |   57 ++++++++++++++++++++++++++++++++----------------
 1 files changed, 38 insertions(+), 19 deletions(-)

diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
index 8d362d7..5e78c18 100644
--- a/net/bluetooth/l2cap.c
+++ b/net/bluetooth/l2cap.c
@@ -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);
 
-- 
1.7.1

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

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

[Marcel Holtmann]

Hi Mat,

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

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

[Gustavo F. Padovan]

Hi Mat,

* Mat Martineau <mathewm@codeaurora.org> [2010-08-10 12:14:58 -0700]:

> 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 <mathewm@codeaurora.org>
> ---
>  net/bluetooth/l2cap.c |   57 ++++++++++++++++++++++++++++++++----------------
>  1 files changed, 38 insertions(+), 19 deletions(-)
> 
> diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
> index 8d362d7..5e78c18 100644
> --- a/net/bluetooth/l2cap.c
> +++ b/net/bluetooth/l2cap.c
> @@ -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().


-- 
Gustavo F. Padovan
http://padovan.org

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

[Mat Martineau]

Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
---
 net/bluetooth/l2cap.c |   23 +++++++++++++----------
 1 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
index 5e78c18..aa69c84 100644
--- a/net/bluetooth/l2cap.c
+++ b/net/bluetooth/l2cap.c
@@ -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)) {
-- 
1.7.1

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

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

[Marcel Holtmann]

Hi Mat,

> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
> ---
>  net/bluetooth/l2cap.c |   23 +++++++++++++----------
>  1 files changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
> index 5e78c18..aa69c84 100644
> --- a/net/bluetooth/l2cap.c
> +++ b/net/bluetooth/l2cap.c
> @@ -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

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

[Gustavo F. Padovan]

Hi Mat,

* Mat Martineau <mathewm@codeaurora.org> [2010-08-10 12:14:59 -0700]:

> 
> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
> ---
>  net/bluetooth/l2cap.c |   23 +++++++++++++----------
>  1 files changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
> index 5e78c18..aa69c84 100644
> --- a/net/bluetooth/l2cap.c
> +++ b/net/bluetooth/l2cap.c
> @@ -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.

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

>  
>  	if (l2cap_pi(sk)->fcs == L2CAP_FCS_CRC16)
> -		hlen += 2;
> +		hlen += L2CAP_FCS_SIZE;

Separated patch to ad 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);

This also should not be in this patch.

-- 
Gustavo F. Padovan
http://padovan.org

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

[Mat Martineau]

Gustavo -

On Tue, 10 Aug 2010, Gustavo F. Padovan wrote:

> Hi Mat,
>
> * Mat Martineau <mathewm@codeaurora.org> [2010-08-10 12:14:59 -0700]:
>
>>
>> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
>> ---
>>  net/bluetooth/l2cap.c |   23 +++++++++++++----------
>>  1 files changed, 13 insertions(+), 10 deletions(-)
>>
>> diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
>> index 5e78c18..aa69c84 100644
>> --- a/net/bluetooth/l2cap.c
>> +++ b/net/bluetooth/l2cap.c
>> @@ -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.

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.

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

Ok.

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

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

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

[Gustavo F. Padovan]

Hi Mat,

* Mat Martineau <mathewm@codeaurora.org> [2010-08-10 19:24:20 -0700]:

> 
> 
> Gustavo -
> 
> On Tue, 10 Aug 2010, Gustavo F. Padovan wrote:
> 
> >Hi Mat,
> >
> >* Mat Martineau <mathewm@codeaurora.org> [2010-08-10 12:14:59 -0700]:
> >
> >>
> >>Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
> >>---
> >> net/bluetooth/l2cap.c |   23 +++++++++++++----------
> >> 1 files changed, 13 insertions(+), 10 deletions(-)
> >>
> >>diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
> >>index 5e78c18..aa69c84 100644
> >>--- a/net/bluetooth/l2cap.c
> >>+++ b/net/bluetooth/l2cap.c
> >>@@ -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.
> 
> 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.

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

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


-- 
Gustavo F. Padovan
http://padovan.org

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

[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 <mathewm@codeaurora.org>
---
 net/bluetooth/l2cap.c |   39 ++++++++++++++++++++++++++++++++++-----
 1 files changed, 34 insertions(+), 5 deletions(-)

diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
index aa69c84..b485c4a 100644
--- a/net/bluetooth/l2cap.c
+++ b/net/bluetooth/l2cap.c
@@ -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;
 }
-- 
1.7.1

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

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

[Marcel Holtmann]

Hi Mat,

> 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 <mathewm@codeaurora.org>
> ---
>  net/bluetooth/l2cap.c |   39 ++++++++++++++++++++++++++++++++++-----
>  1 files changed, 34 insertions(+), 5 deletions(-)
> 
> diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
> index aa69c84..b485c4a 100644
> --- a/net/bluetooth/l2cap.c
> +++ b/net/bluetooth/l2cap.c
> @@ -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);

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.

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

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

[Gustavo F. Padovan]

* Mat Martineau <mathewm@codeaurora.org> [2010-08-10 12:15:00 -0700]:

> 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 <mathewm@codeaurora.org>
> ---
>  net/bluetooth/l2cap.c |   39 ++++++++++++++++++++++++++++++++++-----
>  1 files changed, 34 insertions(+), 5 deletions(-)
> 
> diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
> index aa69c84..b485c4a 100644
> --- a/net/bluetooth/l2cap.c
> +++ b/net/bluetooth/l2cap.c
> @@ -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)

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

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


> +			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;
>  }
> -- 
> 1.7.1
> 
> --
> Mat Martineau
> Employee of Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

-- 
Gustavo F. Padovan
http://padovan.org

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

[Gustavo F. Padovan]

Hi Mat,

* Gustavo F. Padovan <gustavo@padovan.org> [2010-08-11 00:35:41 -0300]:

> * Mat Martineau <mathewm@codeaurora.org> [2010-08-10 12:15:00 -0700]:
> 
> > 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 <mathewm@codeaurora.org>
> > ---
> >  net/bluetooth/l2cap.c |   39 ++++++++++++++++++++++++++++++++++-----
> >  1 files changed, 34 insertions(+), 5 deletions(-)
> > 
> > diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
> > index aa69c84..b485c4a 100644
> > --- a/net/bluetooth/l2cap.c
> > +++ b/net/bluetooth/l2cap.c
> > @@ -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)
> 
> 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."
> 
> 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.

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

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


-- 
Gustavo F. Padovan
http://padovan.org

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

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

diff --git a/net/bluetooth/bnep/core.c b/net/bluetooth/bnep/core.c
index f10b41f..9f60443 100644
--- a/net/bluetooth/bnep/core.c
+++ b/net/bluetooth/bnep/core.c
@@ -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)
diff --git a/net/bluetooth/cmtp/core.c b/net/bluetooth/cmtp/core.c
index d4c6af0..48ac812 100644
--- a/net/bluetooth/cmtp/core.c
+++ b/net/bluetooth/cmtp/core.c
@@ -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);
diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index bfe641b..097ff25 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -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);
diff --git a/net/bluetooth/rfcomm/core.c b/net/bluetooth/rfcomm/core.c
index 7dca91b..ee27f59 100644
--- a/net/bluetooth/rfcomm/core.c
+++ b/net/bluetooth/rfcomm/core.c
@@ -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) {
-- 
1.7.1

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

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

[Marcel Holtmann]

Hi Mat,

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

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

[Gustavo F. Padovan]

Hi Mat,

* Marcel Holtmann <marcel@holtmann.org> [2010-08-10 17:38:33 -0400]:

> Hi Mat,
> 
> > 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 <mathewm@codeaurora.org>
> > ---
> >  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.

Same from me.

-- 
Gustavo F. Padovan
http://padovan.org

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

[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 <mathewm@codeaurora.org>
---
 net/bluetooth/af_bluetooth.c |   30 ++++++++++++++++++++++++++++--
 1 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/net/bluetooth/af_bluetooth.c b/net/bluetooth/af_bluetooth.c
index 77a26fe..5e0375b 100644
--- a/net/bluetooth/af_bluetooth.c
+++ b/net/bluetooth/af_bluetooth.c
@@ -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;
-- 
1.7.1

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

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

[Gustavo F. Padovan]

Hi Mat,

* Mat Martineau <mathewm@codeaurora.org> [2010-08-10 12:15:02 -0700]:

> 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 <mathewm@codeaurora.org>
> ---
>  net/bluetooth/af_bluetooth.c |   30 ++++++++++++++++++++++++++++--
>  1 files changed, 28 insertions(+), 2 deletions(-)
> 
> diff --git a/net/bluetooth/af_bluetooth.c b/net/bluetooth/af_bluetooth.c
> index 77a26fe..5e0375b 100644
> --- a/net/bluetooth/af_bluetooth.c
> +++ b/net/bluetooth/af_bluetooth.c
> @@ -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);

Why are you using the header length here?

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

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

> +					} 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;
> -- 
> 1.7.1
> 
> --
> Mat Martineau
> Employee of Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

-- 
Gustavo F. Padovan
http://padovan.org

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

[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 <mathewm@codeaurora.org>
---
 include/net/bluetooth/l2cap.h |    1 +
 net/bluetooth/l2cap.c         |   66 +++++++++++++++++++++-------------------
 2 files changed, 36 insertions(+), 31 deletions(-)

diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index 2f3222f..9384e87 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -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;
 
diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
index b485c4a..0212035 100644
--- a/net/bluetooth/l2cap.c
+++ b/net/bluetooth/l2cap.c
@@ -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;
 }
 
+static inline void append_skb_frag(struct sk_buff *skb,
+			struct sk_buff *new_frag, struct sk_buff **last_frag)
+{
+	/* 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;
+
+	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;
+}
+
 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)
 			goto drop;
 
 		err = sock_queue_rcv_skb(sk, skb);
@@ -3653,61 +3677,42 @@ static int l2cap_ertm_reassembly_sdu(struct sock *sk, struct sk_buff *skb, u16 c
 		break;
 
 	case L2CAP_SDU_START:
-		if (pi->conn_state & L2CAP_CONN_SAR_SDU)
+		if (pi->sdu)
 			goto drop;
 
 		pi->sdu_len = get_unaligned_le16(skb->data);
+		skb_pull(skb, 2);
 
 		if (pi->sdu_len > pi->imtu)
 			goto disconnect;
 
-		pi->sdu = bt_skb_alloc(pi->sdu_len, GFP_ATOMIC);
-		if (!pi->sdu)
-			return -ENOMEM;
-
-		/* pull sdu_len bytes only after alloc, because of Local Busy
-		 * condition we have to be sure that this will be executed
-		 * only once, i.e., when alloc does not fail */
-		skb_pull(skb, 2);
-
-		memcpy(skb_put(pi->sdu, skb->len), skb->data, skb->len);
-
-		pi->conn_state |= L2CAP_CONN_SAR_SDU;
-		pi->partial_sdu_len = skb->len;
+		pi->sdu = skb;
+		pi->sdu_last_frag = skb;
 		break;
 
 	case L2CAP_SDU_CONTINUE:
-		if (!(pi->conn_state & L2CAP_CONN_SAR_SDU))
-			goto disconnect;
-
 		if (!pi->sdu)
 			goto disconnect;
 
-		pi->partial_sdu_len += skb->len;
-		if (pi->partial_sdu_len > pi->sdu_len)
-			goto drop;
+		append_skb_frag(pi->sdu, skb, &pi->sdu_last_frag);
 
-		memcpy(skb_put(pi->sdu, skb->len), skb->data, skb->len);
+		if (pi->sdu->len > pi->sdu_len)
+			goto drop;
 
 		break;
 
 	case L2CAP_SDU_END:
-		if (!(pi->conn_state & L2CAP_CONN_SAR_SDU))
-			goto disconnect;
-
 		if (!pi->sdu)
 			goto disconnect;
 
 		if (!(pi->conn_state & L2CAP_CONN_SAR_RETRY)) {
-			pi->partial_sdu_len += skb->len;
+			append_skb_frag(pi->sdu, skb, &pi->sdu_last_frag);
 
-			if (pi->partial_sdu_len > pi->imtu)
+			if (pi->sdu->len > pi->sdu_len)
 				goto drop;
 
-			if (pi->partial_sdu_len != pi->sdu_len)
+			if (pi->sdu->len != pi->sdu_len)
 				goto drop;
-
-			memcpy(skb_put(pi->sdu, skb->len), skb->data, skb->len);
 		}
 
 		_skb = skb_clone(pi->sdu, GFP_ATOMIC);
@@ -3724,7 +3729,6 @@ static int l2cap_ertm_reassembly_sdu(struct sock *sk, struct sk_buff *skb, u16 c
 		}
 
 		pi->conn_state &= ~L2CAP_CONN_SAR_RETRY;
-		pi->conn_state &= ~L2CAP_CONN_SAR_SDU;
 
 		kfree_skb(pi->sdu);
 		break;
-- 
1.7.1

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

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

[Gustavo F. Padovan]

Hi Mat,

* Mat Martineau <mathewm@codeaurora.org> [2010-08-10 12:15:03 -0700]:

> 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 <mathewm@codeaurora.org>
> ---
>  include/net/bluetooth/l2cap.h |    1 +
>  net/bluetooth/l2cap.c         |   66 +++++++++++++++++++++-------------------
>  2 files changed, 36 insertions(+), 31 deletions(-)
> 
> diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
> index 2f3222f..9384e87 100644
> --- a/include/net/bluetooth/l2cap.h
> +++ b/include/net/bluetooth/l2cap.h
> @@ -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;
>  
> diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
> index b485c4a..0212035 100644
> --- a/net/bluetooth/l2cap.c
> +++ b/net/bluetooth/l2cap.c
> @@ -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;
>  }
>  
> +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()

> +{
> +	/* 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;
> +
> +	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.

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


-- 
Gustavo F. Padovan
http://padovan.org

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

[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 <mathewm@codeaurora.org>
---
 net/bluetooth/l2cap.c |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
index 0212035..15fbb1d 100644
--- a/net/bluetooth/l2cap.c
+++ b/net/bluetooth/l2cap.c
@@ -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);
 
-- 
1.7.1

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

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

[Gustavo F. Padovan]

Hi Mat,

* Mat Martineau <mathewm@codeaurora.org> [2010-08-10 12:15:04 -0700]:

> 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 <mathewm@codeaurora.org>
> ---
>  net/bluetooth/l2cap.c |   16 ++++++++--------
>  1 files changed, 8 insertions(+), 8 deletions(-)

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

-- 
Gustavo F. Padovan
http://padovan.org