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.
Hi Mat,
* Mat Martineau <[email protected]> [2010-08-12 16:36:57 -0700]:
>
>
> On Wed, 11 Aug 2010, Gustavo F. Padovan wrote:
>
> >Hi Mat,
> >
> >* Gustavo F. Padovan <[email protected]> [2010-08-11 00:35:41 -0300]:
> >
> >>* Mat Martineau <[email protected]> [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 <[email protected]>
> >>>---
> >>> 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.
>
> That's what the code does, without violating the HCI MTU.
Yes, you are right!
>
> >>>+ 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.
> >
>
> 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.
Ok, and do the other way around in the if:
if (l2cap_pi(sk)->fcs != L2CAP_FCS_CRC16)
return sent;
if (skb_tailroom(...
--
Gustavo F. Padovan
http://padovan.org
Gustavo -
On Wed, 11 Aug 2010, Gustavo F. Padovan wrote:
> Hi Mat,
>
> * Mat Martineau <[email protected]> [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 <[email protected]>
>> ---
>> 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()
Ok.
>
>> +{
>> + /* 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.
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.
>
>> 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).
>
Ok.
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
On Wed, 11 Aug 2010, Gustavo F. Padovan wrote:
> Hi Mat,
>
> * Mat Martineau <[email protected]> [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 <[email protected]>
>> ---
>> 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?
skb_headlen() returns the length of the first fragment.
>> +
>> + 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.
>> +
>> + 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.
>
>> + } 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;
}
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
On Wed, 11 Aug 2010, Gustavo F. Padovan wrote:
> Hi Mat,
>
> * Gustavo F. Padovan <[email protected]> [2010-08-11 00:35:41 -0300]:
>
>> * Mat Martineau <[email protected]> [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 <[email protected]>
>>> ---
>>> 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.
That's what the code does, without violating the HCI 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);
>
> 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
On Wed, 11 Aug 2010, Gustavo F. Padovan wrote:
> * Mat Martineau <[email protected]> [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 <[email protected]>
>> ---
>> 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 <="
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.
>> + 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
Hi Mat,
* Mat Martineau <[email protected]> [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 <[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.
--
Gustavo F. Padovan
http://padovan.org
Hi Mat,
* Mat Martineau <[email protected]> [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 <[email protected]>
> ---
> 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
Hi Mat,
* Mat Martineau <[email protected]> [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 <[email protected]>
> ---
> 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
Hi Mat,
* Marcel Holtmann <[email protected]> [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 <[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.
Same from me.
--
Gustavo F. Padovan
http://padovan.org
Hi Mat,
* Gustavo F. Padovan <[email protected]> [2010-08-11 00:35:41 -0300]:
> * Mat Martineau <[email protected]> [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 <[email protected]>
> > ---
> > 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
* Mat Martineau <[email protected]> [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 <[email protected]>
> ---
> 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
Hi Mat,
* Mat Martineau <[email protected]> [2010-08-10 19:24:20 -0700]:
>
>
> Gustavo -
>
> On Tue, 10 Aug 2010, Gustavo F. Padovan wrote:
>
> >Hi Mat,
> >
> >* Mat Martineau <[email protected]> [2010-08-10 12:14:59 -0700]:
> >
> >>
> >>Signed-off-by: Mat Martineau <[email protected]>
> >>---
> >> 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
Gustavo -
On Tue, 10 Aug 2010, Gustavo F. Padovan wrote:
> Hi Mat,
>
> * Mat Martineau <[email protected]> [2010-08-10 12:14:59 -0700]:
>
>>
>> Signed-off-by: Mat Martineau <[email protected]>
>> ---
>> 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
Hi Mat,
* Mat Martineau <[email protected]> [2010-08-10 12:14:59 -0700]:
>
> Signed-off-by: Mat Martineau <[email protected]>
> ---
> 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
Hi Mat,
* Mat Martineau <[email protected]> [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 <[email protected]>
> ---
> 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
Hi Mat,
> Signed-off-by: Mat Martineau <[email protected]>
> ---
> 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
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 <[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
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 <[email protected]>
> ---
> 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
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 <[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
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(-)
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
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(-)
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
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(-)
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
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(-)
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
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(-)
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
Signed-off-by: Mat Martineau <[email protected]>
---
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
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(-)
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