Return-Path: Date: Thu, 12 Aug 2010 16:36:57 -0700 (PDT) From: Mat Martineau To: "Gustavo F. Padovan" cc: linux-bluetooth@vger.kernel.org, marcel@holtmann.org, rshaffer@codeaurora.org Subject: Re: [RFC 3/7] Bluetooth: Add FCS awareness to L2CAP HCI fragmentation. In-Reply-To: <20100811035654.GA9293@vigoh> Message-ID: References: <1281467704-5378-1-git-send-email-mathewm@codeaurora.org> <1281467704-5378-4-git-send-email-mathewm@codeaurora.org> <20100811033540.GA9151@vigoh> <20100811035654.GA9293@vigoh> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed List-ID: On Wed, 11 Aug 2010, Gustavo F. Padovan wrote: > Hi Mat, > > * Gustavo F. Padovan [2010-08-11 00:35:41 -0300]: > >> * Mat Martineau [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 >>> --- >>> 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