[PATCH 1/4] Bluetooth: Fix packet size provided to the controller

[PATCH 1/4] Bluetooth: Fix packet size provided to the controller

[Gustavo Padovan]

When building fragmented skb's skb->len keeps track of the size of head
plus all fragments combined, however when queueing the skb for sending we
need to report the head size instead of the total size, so we just set
skb->len to skb_headlen().

This bug appeared when implementing MSG_MORE support for L2CAP sockets, it
never showed up before because l2cap_skbuff_fromiovec() never accounted skb
size correctly. A following patch will fix this.

Signed-off-by: Gustavo Padovan <gustavo@padovan.org>
Reviewed-by: Mat Martineau <mathewm@codeaurora.org>
---
 net/bluetooth/hci_core.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 83d3d35..6b220c2 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -2162,6 +2162,12 @@ static void hci_queue_acl(struct hci_conn *conn, struct sk_buff_head *queue,
 	struct hci_dev *hdev = conn->hdev;
 	struct sk_buff *list;
 
+	skb->len = skb_headlen(skb);
+	skb->data_len = 0;
+
+	bt_cb(skb)->pkt_type = HCI_ACLDATA_PKT;
+	hci_add_acl_hdr(skb, conn->handle, flags);
+
 	list = skb_shinfo(skb)->frag_list;
 	if (!list) {
 		/* Non fragmented */
@@ -2205,8 +2211,6 @@ void hci_send_acl(struct hci_chan *chan, struct sk_buff *skb, __u16 flags)
 	BT_DBG("%s chan %p flags 0x%x", hdev->name, chan, flags);
 
 	skb->dev = (void *) hdev;
-	bt_cb(skb)->pkt_type = HCI_ACLDATA_PKT;
-	hci_add_acl_hdr(skb, conn->handle, flags);
 
 	hci_queue_acl(conn, &chan->data_q, skb, flags);
 
-- 
1.7.10.1

[PATCH 2/4] Bluetooth: Fix skb length calculation

[Gustavo Padovan]

When we add a fragment to a skb, len and data_len  fields need
to be updated.

Signed-off-by: Gustavo Padovan <gustavo@padovan.org>
---
 net/bluetooth/l2cap_core.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 2b30bd7..e5a4fd9 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -1851,6 +1851,9 @@ static inline int l2cap_skbuff_fromiovec(struct l2cap_chan *chan,
 		sent += count;
 		len  -= count;
 
+		skb->len += (*frag)->len;
+		skb->data_len += (*frag)->len;
+
 		frag = &(*frag)->next;
 	}
 
-- 
1.7.10.1

[PATCH 3/4] Bluetooth: Add MSG_MORE support to L2CAP sockets

[Gustavo Padovan]

MSG_MORE enables us to save buffer space in userspace, the packet are
built directly in the kernel and sent when their size reaches the Output
MTU value.

Only MSG_MORE for Basic Mode is supported right now. chan->skb_more keeps
a pointer to the L2CAP packet that is being build through many calls to
send().

Signed-off-by: Gustavo Padovan <gustavo@padovan.org>
---
 include/net/bluetooth/l2cap.h |    2 +
 net/bluetooth/l2cap_core.c    |  116 ++++++++++++++++++++++++++++++++++++++---
 2 files changed, 110 insertions(+), 8 deletions(-)

diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index 1c7d1cd..5f2845d 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -520,6 +520,8 @@ struct l2cap_chan {
 	struct list_head	list;
 	struct list_head	global_l;
 
+	struct sk_buff		*skb_more;
+
 	void			*data;
 	struct l2cap_ops	*ops;
 	struct mutex		lock;
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index e5a4fd9..73bf8a8 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -1827,6 +1827,8 @@ static inline int l2cap_skbuff_fromiovec(struct l2cap_chan *chan,
 	struct sk_buff **frag;
 	int sent = 0;
 
+	count = min_t(unsigned int, count, len);
+
 	if (memcpy_fromiovec(skb_put(skb, count), msg->msg_iov, count))
 		return -EFAULT;
 
@@ -1903,9 +1905,13 @@ static struct sk_buff *l2cap_create_basic_pdu(struct l2cap_chan *chan,
 	int err, count;
 	struct l2cap_hdr *lh;
 
-	BT_DBG("chan %p len %d", chan, (int)len);
+	BT_DBG("chan %p len %d aclmtu %d" , chan, (int)len, conn->mtu);
 
-	count = min_t(unsigned int, (conn->mtu - L2CAP_HDR_SIZE), len);
+	if (msg->msg_flags & MSG_MORE)
+		count = min_t(unsigned int, (conn->mtu - L2CAP_HDR_SIZE),
+			      chan->omtu);
+	else
+		count = min_t(unsigned int, (conn->mtu - L2CAP_HDR_SIZE), len);
 
 	skb = chan->ops->alloc_skb(chan, count + L2CAP_HDR_SIZE,
 				   msg->msg_flags & MSG_DONTWAIT);
@@ -2048,6 +2054,75 @@ static int l2cap_segment_sdu(struct l2cap_chan *chan,
 	return err;
 }
 
+static int l2cap_append_pdu(struct l2cap_chan *chan, struct msghdr *msg,
+			    size_t len)
+{
+	struct l2cap_conn *conn = chan->conn;
+	struct sk_buff **frag, *skb = chan->skb_more;
+	int sent = 0;
+	unsigned int count;
+	struct l2cap_hdr *lh;
+
+	BT_DBG("chan %p len %ld", chan, len);
+
+	frag = &skb_shinfo(skb)->frag_list;
+	if (*frag)
+		goto frags;
+
+	count = min_t(unsigned int, (conn->mtu - L2CAP_HDR_SIZE),
+		      chan->omtu);
+	count = count - skb->len + L2CAP_HDR_SIZE;
+	count = min_t(unsigned int, count, len);
+
+	if (memcpy_fromiovec(skb_put(skb, count), msg->msg_iov, count))
+		return -EFAULT;
+
+	sent += count;
+	len  -= count;
+
+frags:
+	while (*frag)
+		frag = &(*frag)->next;
+
+	while (len) {
+		count = chan->omtu - skb->len + L2CAP_HDR_SIZE;
+		count = min_t(unsigned int, count, len);
+		count = min_t(unsigned int, conn->mtu, count);
+
+		*frag = chan->ops->alloc_skb(chan, count,
+					     msg->msg_flags & MSG_DONTWAIT);
+		if (IS_ERR(*frag))
+			return PTR_ERR(*frag);
+
+		if (memcpy_fromiovec(skb_put(*frag, count), msg->msg_iov,
+				     count))
+			return -EFAULT;
+
+		(*frag)->priority = skb->priority;
+
+		sent += count;
+		len  -= count;
+
+		skb->len += (*frag)->len;
+		skb->data_len += (*frag)->len;
+
+		frag = &(*frag)->next;
+
+		if (skb->len == chan->omtu + L2CAP_HDR_SIZE)
+			break;
+	}
+
+	lh = (struct l2cap_hdr *) skb->data;
+	lh->len = cpu_to_le16(skb->len - L2CAP_HDR_SIZE);
+
+	if (skb->len == chan->omtu + L2CAP_HDR_SIZE) {
+		l2cap_do_send(chan, skb);
+		chan->skb_more = NULL;
+	}
+
+	return sent;
+}
+
 int l2cap_chan_send(struct l2cap_chan *chan, struct msghdr *msg, size_t len,
 								u32 priority)
 {
@@ -2068,16 +2143,41 @@ int l2cap_chan_send(struct l2cap_chan *chan, struct msghdr *msg, size_t len,
 	switch (chan->mode) {
 	case L2CAP_MODE_BASIC:
 		/* Check outgoing MTU */
-		if (len > chan->omtu)
+		if (len > chan->omtu) {
+			kfree_skb(chan->skb_more);
 			return -EMSGSIZE;
+		}
 
-		/* Create a basic PDU */
-		skb = l2cap_create_basic_pdu(chan, msg, len, priority);
-		if (IS_ERR(skb))
+		err = len;
+		if (chan->skb_more) {
+			int sent = l2cap_append_pdu(chan, msg, len);
+
+			if (sent < 0) {
+				kfree_skb(chan->skb_more);
+				return sent;
+			}
+
+			len -= sent;
+		}
+
+		if (len)
+			skb = l2cap_create_basic_pdu(chan, msg, len, priority);
+		else
+			skb = chan->skb_more;
+
+		if (IS_ERR(skb)) {
+			kfree_skb(chan->skb_more);
 			return PTR_ERR(skb);
+		}
+
+		if (!skb)
+			return err;
+
+		if (msg->msg_flags & MSG_MORE && skb->len < chan->omtu)
+			chan->skb_more = skb;
+		else
+			l2cap_do_send(chan, skb);
 
-		l2cap_do_send(chan, skb);
-		err = len;
 		break;
 
 	case L2CAP_MODE_ERTM:
-- 
1.7.10.1

[PATCH 4/4] Bluetooth: add a timer to L2CAP MSG_MORE code

[Gustavo Padovan]

There is now a 200 milliseconds limit for which L2CAP will wait for
which L2CAP will wait before send the already queued data to the
controller.

Signed-off-by: Gustavo Padovan <gustavo@padovan.org>
---
 include/net/bluetooth/l2cap.h |    5 ++++
 net/bluetooth/l2cap_core.c    |   65 ++++++++++++++++++++++++++++-------------
 2 files changed, 49 insertions(+), 21 deletions(-)

diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index 5f2845d..e8bbb2e 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -40,6 +40,7 @@
 #define L2CAP_DEFAULT_MONITOR_TO	12000   /* 12 seconds */
 #define L2CAP_DEFAULT_MAX_PDU_SIZE	1009    /* Sized for 3-DH5 packet */
 #define L2CAP_DEFAULT_ACK_TO		200
+#define L2CAP_MSG_MORE_TO		200
 #define L2CAP_LE_DEFAULT_MTU		23
 #define L2CAP_DEFAULT_MAX_SDU_SIZE	0xFFFF
 #define L2CAP_DEFAULT_SDU_ITIME		0xFFFFFFFF
@@ -521,6 +522,7 @@ struct l2cap_chan {
 	struct list_head	global_l;
 
 	struct sk_buff		*skb_more;
+	struct delayed_work	more_timer;
 
 	void			*data;
 	struct l2cap_ops	*ops;
@@ -724,6 +726,9 @@ static inline bool l2cap_clear_timer(struct l2cap_chan *chan,
 #define __set_ack_timer(c) l2cap_set_timer(c, &chan->ack_timer, \
 		msecs_to_jiffies(L2CAP_DEFAULT_ACK_TO));
 #define __clear_ack_timer(c) l2cap_clear_timer(c, &c->ack_timer)
+#define __set_more_timer(c) l2cap_set_timer(c, &chan->more_timer, \
+		msecs_to_jiffies(L2CAP_MSG_MORE_TO));
+#define __clear_more_timer(c) l2cap_clear_timer(c, &c->more_timer)
 
 static inline int __seq_offset(struct l2cap_chan *chan, __u16 seq1, __u16 seq2)
 {
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 73bf8a8..4f3ccf8 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -337,6 +337,24 @@ static void l2cap_seq_list_append(struct l2cap_seq_list *seq_list, u16 seq)
 	}
 }
 
+static void l2cap_do_send(struct l2cap_chan *chan, struct sk_buff *skb)
+{
+	struct hci_conn *hcon = chan->conn->hcon;
+	u16 flags;
+
+	BT_DBG("chan %p, skb %p len %d priority %u", chan, skb, skb->len,
+							skb->priority);
+
+	if (!test_bit(FLAG_FLUSHABLE, &chan->flags) &&
+					lmp_no_flush_capable(hcon->hdev))
+		flags = ACL_START_NO_FLUSH;
+	else
+		flags = ACL_START;
+
+	bt_cb(skb)->force_active = test_bit(FLAG_FORCE_ACTIVE, &chan->flags);
+	hci_send_acl(chan->conn->hchan, skb, flags);
+}
+
 static void l2cap_chan_timeout(struct work_struct *work)
 {
 	struct l2cap_chan *chan = container_of(work, struct l2cap_chan,
@@ -367,6 +385,22 @@ static void l2cap_chan_timeout(struct work_struct *work)
 	l2cap_chan_put(chan);
 }
 
+static void l2cap_msg_more_timeout(struct work_struct *work)
+{
+	struct l2cap_chan *chan = container_of(work, struct l2cap_chan,
+					       more_timer.work);
+
+	BT_DBG("chan %p", chan);
+
+	l2cap_chan_lock(chan);
+
+	l2cap_do_send(chan, chan->skb_more);
+	chan->skb_more = NULL;
+
+	l2cap_chan_unlock(chan);
+	l2cap_chan_put(chan);
+}
+
 struct l2cap_chan *l2cap_chan_create(void)
 {
 	struct l2cap_chan *chan;
@@ -382,6 +416,7 @@ struct l2cap_chan *l2cap_chan_create(void)
 	write_unlock(&chan_list_lock);
 
 	INIT_DELAYED_WORK(&chan->chan_timer, l2cap_chan_timeout);
+	INIT_DELAYED_WORK(&chan->more_timer, l2cap_msg_more_timeout);
 
 	chan->state = BT_OPEN;
 
@@ -696,24 +731,6 @@ static void l2cap_send_cmd(struct l2cap_conn *conn, u8 ident, u8 code, u16 len,
 	hci_send_acl(conn->hchan, skb, flags);
 }
 
-static void l2cap_do_send(struct l2cap_chan *chan, struct sk_buff *skb)
-{
-	struct hci_conn *hcon = chan->conn->hcon;
-	u16 flags;
-
-	BT_DBG("chan %p, skb %p len %d priority %u", chan, skb, skb->len,
-							skb->priority);
-
-	if (!test_bit(FLAG_FLUSHABLE, &chan->flags) &&
-					lmp_no_flush_capable(hcon->hdev))
-		flags = ACL_START_NO_FLUSH;
-	else
-		flags = ACL_START;
-
-	bt_cb(skb)->force_active = test_bit(FLAG_FORCE_ACTIVE, &chan->flags);
-	hci_send_acl(chan->conn->hchan, skb, flags);
-}
-
 static void __unpack_enhanced_control(u16 enh, struct l2cap_ctrl *control)
 {
 	control->reqseq = (enh & L2CAP_CTRL_REQSEQ) >> L2CAP_CTRL_REQSEQ_SHIFT;
@@ -2142,6 +2159,9 @@ int l2cap_chan_send(struct l2cap_chan *chan, struct msghdr *msg, size_t len,
 
 	switch (chan->mode) {
 	case L2CAP_MODE_BASIC:
+
+		__clear_more_timer(chan);
+
 		/* Check outgoing MTU */
 		if (len > chan->omtu) {
 			kfree_skb(chan->skb_more);
@@ -2173,10 +2193,13 @@ int l2cap_chan_send(struct l2cap_chan *chan, struct msghdr *msg, size_t len,
 		if (!skb)
 			return err;
 
-		if (msg->msg_flags & MSG_MORE && skb->len < chan->omtu)
+		if (msg->msg_flags & MSG_MORE && skb->len < chan->omtu) {
 			chan->skb_more = skb;
-		else
-			l2cap_do_send(chan, skb);
+			__set_more_timer(chan);
+			return err;
+		}
+
+		l2cap_do_send(chan, skb);
 
 		break;
 
-- 
1.7.10.1

Re: [PATCH 4/4] Bluetooth: add a timer to L2CAP MSG_MORE code

[Mat Martineau]

On Fri, 11 May 2012, Gustavo Padovan wrote:

> There is now a 200 milliseconds limit for which L2CAP will wait for
> which L2CAP will wait before send the already queued data to the
> controller.
>
> Signed-off-by: Gustavo Padovan <gustavo@padovan.org>

A timeout like this makes sense with a streaming protocol, but not 
with a datagram protocol like L2CAP.  The application could be waiting 
on file I/O (especially on a flash filesystem), be waiting for another 
blocking call to return, or another resource-intensive application 
could be hogging the processor for a short time.  If an incomplete SDU 
gets sent out because of a timeout like this, the profile on the other 
side will see it as corrupt.  Better to keep the queued data 
indefinitely.

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

Re: [PATCH 4/4] Bluetooth: add a timer to L2CAP MSG_MORE code

[Gustavo Padovan]

* Mat Martineau <mathewm@codeaurora.org> [2012-05-11 11:38:15 -0700]:

> 
> On Fri, 11 May 2012, Gustavo Padovan wrote:
> 
> >There is now a 200 milliseconds limit for which L2CAP will wait for
> >which L2CAP will wait before send the already queued data to the
> >controller.
> >
> >Signed-off-by: Gustavo Padovan <gustavo@padovan.org>
> 
> A timeout like this makes sense with a streaming protocol, but not
> with a datagram protocol like L2CAP.  The application could be
> waiting on file I/O (especially on a flash filesystem), be waiting
> for another blocking call to return, or another resource-intensive
> application could be hogging the processor for a short time.  If an
> incomplete SDU gets sent out because of a timeout like this, the
> profile on the other side will see it as corrupt.  Better to keep
> the queued data indefinitely.

I don't think this would case problems, before this gets to the profile the
ACL frame needs to be built and it won't be until we get the all pieces of the
L2CAP frame.

I'm following the TCP behaviour with MSG_MORE here.

	Gustavo

Re: [PATCH 4/4] Bluetooth: add a timer to L2CAP MSG_MORE code

[Mat Martineau]

On Fri, 11 May 2012, Gustavo Padovan wrote:

> * Mat Martineau <mathewm@codeaurora.org> [2012-05-11 11:38:15 -0700]:
>
>>
>> On Fri, 11 May 2012, Gustavo Padovan wrote:
>>
>>> There is now a 200 milliseconds limit for which L2CAP will wait for
>>> which L2CAP will wait before send the already queued data to the
>>> controller.
>>>
>>> Signed-off-by: Gustavo Padovan <gustavo@padovan.org>
>>
>> A timeout like this makes sense with a streaming protocol, but not
>> with a datagram protocol like L2CAP.  The application could be
>> waiting on file I/O (especially on a flash filesystem), be waiting
>> for another blocking call to return, or another resource-intensive
>> application could be hogging the processor for a short time.  If an
>> incomplete SDU gets sent out because of a timeout like this, the
>> profile on the other side will see it as corrupt.  Better to keep
>> the queued data indefinitely.
>
> I don't think this would case problems, before this gets to the 
> profile the ACL frame needs to be built and it won't be until we get 
> the all pieces of the L2CAP frame.

This doesn't have anything to do with ACL fragments.  When the timeout 
expires, you're sending an L2CAP frame that will get passed up to the 
remote profile right away.  The remote L2CAP protocol will not wait 
for any more data before passing the frame up to the profile, because 
L2CAP doesn't know any more data is coming.

> I'm following the TCP behaviour with MSG_MORE here.

TCP is a stream, like RFCOMM.  UDP and L2CAP are based on datagrams. 
The 200ms timeout makes sense for a stream, because message boundaries 
have no meaning in a stream like TCP.  Splitting up an L2CAP PDU like 
this breaks all kinds of profiles that depend on message boundaries.

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

Re: [PATCH 3/4] Bluetooth: Add MSG_MORE support to L2CAP sockets

[Mat Martineau]

Hi Gustavo -

On Fri, 11 May 2012, Gustavo Padovan wrote:

> MSG_MORE enables us to save buffer space in userspace, the packet are
> built directly in the kernel and sent when their size reaches the Output
> MTU value.
>
> Only MSG_MORE for Basic Mode is supported right now. chan->skb_more keeps
> a pointer to the L2CAP packet that is being build through many calls to
> send().

Could you explain more about how you expect it to work?

I would assume the application would do a series of sends:

send(fd, buf, len, MSG_MORE);
...
send(fd, buf, len, MSG_MORE);
...
send(fd, buf, len, MSG_MORE);
...
send(fd, buf, len, 0);

and the SDU would be sent the first time there is no MSG_MORE flag. 
If the MTU is exceeded, the SDU is not sent and an error is returned.

What should happen if a send() with MSG_MORE completely fills an SDU 
(length of data sent is equal to MTU)?  Does it make sense to treat it 
like a normal send, or return an error so that application knows that 
later calls with MSG_MORE will not append?  Or does the full SDU not 
get sent, and a zero-length send() with no MSG_MORE would trigger the 
transmission?

>
> Signed-off-by: Gustavo Padovan <gustavo@padovan.org>
> ---
> include/net/bluetooth/l2cap.h |    2 +
> net/bluetooth/l2cap_core.c    |  116 ++++++++++++++++++++++++++++++++++++++---
> 2 files changed, 110 insertions(+), 8 deletions(-)
>
> diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
> index 1c7d1cd..5f2845d 100644
> --- a/include/net/bluetooth/l2cap.h
> +++ b/include/net/bluetooth/l2cap.h
> @@ -520,6 +520,8 @@ struct l2cap_chan {
> 	struct list_head	list;
> 	struct list_head	global_l;
>
> +	struct sk_buff		*skb_more;
> +
> 	void			*data;
> 	struct l2cap_ops	*ops;
> 	struct mutex		lock;
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index e5a4fd9..73bf8a8 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -1827,6 +1827,8 @@ static inline int l2cap_skbuff_fromiovec(struct l2cap_chan *chan,
> 	struct sk_buff **frag;
> 	int sent = 0;
>
> +	count = min_t(unsigned int, count, len);
> +
> 	if (memcpy_fromiovec(skb_put(skb, count), msg->msg_iov, count))
> 		return -EFAULT;
>
> @@ -1903,9 +1905,13 @@ static struct sk_buff *l2cap_create_basic_pdu(struct l2cap_chan *chan,
> 	int err, count;
> 	struct l2cap_hdr *lh;
>
> -	BT_DBG("chan %p len %d", chan, (int)len);
> +	BT_DBG("chan %p len %d aclmtu %d" , chan, (int)len, conn->mtu);
>
> -	count = min_t(unsigned int, (conn->mtu - L2CAP_HDR_SIZE), len);
> +	if (msg->msg_flags & MSG_MORE)
> +		count = min_t(unsigned int, (conn->mtu - L2CAP_HDR_SIZE),
> +			      chan->omtu);
> +	else
> +		count = min_t(unsigned int, (conn->mtu - L2CAP_HDR_SIZE), len);
>
> 	skb = chan->ops->alloc_skb(chan, count + L2CAP_HDR_SIZE,
> 				   msg->msg_flags & MSG_DONTWAIT);
> @@ -2048,6 +2054,75 @@ static int l2cap_segment_sdu(struct l2cap_chan *chan,
> 	return err;
> }
>
> +static int l2cap_append_pdu(struct l2cap_chan *chan, struct msghdr *msg,
> +			    size_t len)
> +{
> +	struct l2cap_conn *conn = chan->conn;
> +	struct sk_buff **frag, *skb = chan->skb_more;
> +	int sent = 0;
> +	unsigned int count;
> +	struct l2cap_hdr *lh;
> +
> +	BT_DBG("chan %p len %ld", chan, len);
> +
> +	frag = &skb_shinfo(skb)->frag_list;
> +	if (*frag)
> +		goto frags;

I think this would be more readable without the goto - just use a 
normal if statement with a code block.  There's only one nested if 
statement.

> +
> +	count = min_t(unsigned int, (conn->mtu - L2CAP_HDR_SIZE),
> +		      chan->omtu);
> +	count = count - skb->len + L2CAP_HDR_SIZE;
> +	count = min_t(unsigned int, count, len);
> +
> +	if (memcpy_fromiovec(skb_put(skb, count), msg->msg_iov, count))
> +		return -EFAULT;
> +
> +	sent += count;
> +	len  -= count;
> +
> +frags:
> +	while (*frag)
> +		frag = &(*frag)->next;
> +
> +	while (len) {
> +		count = chan->omtu - skb->len + L2CAP_HDR_SIZE;
> +		count = min_t(unsigned int, count, len);
> +		count = min_t(unsigned int, conn->mtu, count);
> +
> +		*frag = chan->ops->alloc_skb(chan, count,
> +					     msg->msg_flags & MSG_DONTWAIT);
> +		if (IS_ERR(*frag))
> +			return PTR_ERR(*frag);
> +
> +		if (memcpy_fromiovec(skb_put(*frag, count), msg->msg_iov,
> +				     count))
> +			return -EFAULT;
> +
> +		(*frag)->priority = skb->priority;
> +
> +		sent += count;
> +		len  -= count;
> +
> +		skb->len += (*frag)->len;
> +		skb->data_len += (*frag)->len;
> +
> +		frag = &(*frag)->next;
> +
> +		if (skb->len == chan->omtu + L2CAP_HDR_SIZE)
> +			break;

Don't you want to return -EMSGSIZE if the data doesn't fit in one SDU?

> +	}
> +
> +	lh = (struct l2cap_hdr *) skb->data;
> +	lh->len = cpu_to_le16(skb->len - L2CAP_HDR_SIZE);
> +
> +	if (skb->len == chan->omtu + L2CAP_HDR_SIZE) {
> +		l2cap_do_send(chan, skb);

I don't think it's good to put a send in here.  Let the calling 
function do the send, so it's in one place.

> +		chan->skb_more = NULL;
> +	}
> +
> +	return sent;
> +}
> +
> int l2cap_chan_send(struct l2cap_chan *chan, struct msghdr *msg, size_t len,
> 								u32 priority)
> {
> @@ -2068,16 +2143,41 @@ int l2cap_chan_send(struct l2cap_chan *chan, struct msghdr *msg, size_t len,
> 	switch (chan->mode) {
> 	case L2CAP_MODE_BASIC:
> 		/* Check outgoing MTU */
> -		if (len > chan->omtu)
> +		if (len > chan->omtu) {
> +			kfree_skb(chan->skb_more);

Set skb_more to NULL after freeing.

> 			return -EMSGSIZE;
> +		}
>
> -		/* Create a basic PDU */
> -		skb = l2cap_create_basic_pdu(chan, msg, len, priority);
> -		if (IS_ERR(skb))
> +		err = len;
> +		if (chan->skb_more) {
> +			int sent = l2cap_append_pdu(chan, msg, len);
> +
> +			if (sent < 0) {
> +				kfree_skb(chan->skb_more);
> +				return sent;
> +			}
> +
> +			len -= sent;
> +		}
> +
> +		if (len)
> +			skb = l2cap_create_basic_pdu(chan, msg, len, priority);

Shouldn't this be the 'else' clause for the above if statement?  You 
should either call l2cap_append_pdu or l2cap_create_basic_pdu, but 
never both.  Better to structure the logic so that they are obviously 
mutually exclusive.

> +		else
> +			skb = chan->skb_more;
> +
> +		if (IS_ERR(skb)) {
> +			kfree_skb(chan->skb_more);

Set skb_more to NULL after freeing.

> 			return PTR_ERR(skb);
> +		}
> +
> +		if (!skb)
> +			return err;
> +
> +		if (msg->msg_flags & MSG_MORE && skb->len < chan->omtu)
> +			chan->skb_more = skb;
> +		else
> +			l2cap_do_send(chan, skb);

I think l2cap_do_send() should be called if and only if MSG_MORE is 
not set, unless there is an MTU problem.

Also, do you need to account for L2CAP_HDR_SIZE when checking 
skb->len?

>
> -		l2cap_do_send(chan, skb);
> -		err = len;
> 		break;
>
> 	case L2CAP_MODE_ERTM:
> -- 
> 1.7.10.1

Overall, I would suggest that l2cap_chan_send should be kept simple 
and most of the MSG_MORE code should be in a function called by 
l2cap_chan_send.

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

Re: [PATCH 3/4] Bluetooth: Add MSG_MORE support to L2CAP sockets

[Mat Martineau]

Gustavo -

On Fri, 11 May 2012, Mat Martineau wrote:

>
> Hi Gustavo -
>
> On Fri, 11 May 2012, Gustavo Padovan wrote:
>
>> MSG_MORE enables us to save buffer space in userspace, the packet are
>> built directly in the kernel and sent when their size reaches the Output
>> MTU value.
>> 
>> Only MSG_MORE for Basic Mode is supported right now. chan->skb_more keeps
>> a pointer to the L2CAP packet that is being build through many calls to
>> send().
>
> Could you explain more about how you expect it to work?
>
> I would assume the application would do a series of sends:
>
> send(fd, buf, len, MSG_MORE);
> ...
> send(fd, buf, len, MSG_MORE);
> ...
> send(fd, buf, len, MSG_MORE);
> ...
> send(fd, buf, len, 0);
>
> and the SDU would be sent the first time there is no MSG_MORE flag. If the 
> MTU is exceeded, the SDU is not sent and an error is returned.
>
> What should happen if a send() with MSG_MORE completely fills an SDU (length 
> of data sent is equal to MTU)?  Does it make sense to treat it like a normal 
> send, or return an error so that application knows that later calls with 
> MSG_MORE will not append?  Or does the full SDU not get sent, and a 
> zero-length send() with no MSG_MORE would trigger the transmission?
>
>> 
>> Signed-off-by: Gustavo Padovan <gustavo@padovan.org>
>> ---
>> include/net/bluetooth/l2cap.h |    2 +
>> net/bluetooth/l2cap_core.c    |  116 
>> ++++++++++++++++++++++++++++++++++++++---
>> 2 files changed, 110 insertions(+), 8 deletions(-)
>> 
>> diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
>> index 1c7d1cd..5f2845d 100644
>> --- a/include/net/bluetooth/l2cap.h
>> +++ b/include/net/bluetooth/l2cap.h
>> @@ -520,6 +520,8 @@ struct l2cap_chan {
>> 	struct list_head	list;
>> 	struct list_head	global_l;
>> 
>> +	struct sk_buff		*skb_more;
>> +
>> 	void			*data;
>> 	struct l2cap_ops	*ops;
>> 	struct mutex		lock;
>> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
>> index e5a4fd9..73bf8a8 100644
>> --- a/net/bluetooth/l2cap_core.c
>> +++ b/net/bluetooth/l2cap_core.c
>> @@ -1827,6 +1827,8 @@ static inline int l2cap_skbuff_fromiovec(struct 
>> l2cap_chan *chan,
>> 	struct sk_buff **frag;
>> 	int sent = 0;
>> 
>> +	count = min_t(unsigned int, count, len);
>> +
>> 	if (memcpy_fromiovec(skb_put(skb, count), msg->msg_iov, count))
>> 		return -EFAULT;
>> 
>> @@ -1903,9 +1905,13 @@ static struct sk_buff *l2cap_create_basic_pdu(struct 
>> l2cap_chan *chan,
>> 	int err, count;
>> 	struct l2cap_hdr *lh;
>> 
>> -	BT_DBG("chan %p len %d", chan, (int)len);
>> +	BT_DBG("chan %p len %d aclmtu %d" , chan, (int)len, conn->mtu);
>> 
>> -	count = min_t(unsigned int, (conn->mtu - L2CAP_HDR_SIZE), len);
>> +	if (msg->msg_flags & MSG_MORE)
>> +		count = min_t(unsigned int, (conn->mtu - L2CAP_HDR_SIZE),
>> +			      chan->omtu);
>> +	else
>> +		count = min_t(unsigned int, (conn->mtu - L2CAP_HDR_SIZE), 
>> len);
>>
>> 	skb = chan->ops->alloc_skb(chan, count + L2CAP_HDR_SIZE,
>> 				   msg->msg_flags & MSG_DONTWAIT);
>> @@ -2048,6 +2054,75 @@ static int l2cap_segment_sdu(struct l2cap_chan 
>> *chan,
>> 	return err;
>> }
>> 
>> +static int l2cap_append_pdu(struct l2cap_chan *chan, struct msghdr *msg,
>> +			    size_t len)
>> +{
>> +	struct l2cap_conn *conn = chan->conn;
>> +	struct sk_buff **frag, *skb = chan->skb_more;
>> +	int sent = 0;
>> +	unsigned int count;
>> +	struct l2cap_hdr *lh;
>> +
>> +	BT_DBG("chan %p len %ld", chan, len);
>> +
>> +	frag = &skb_shinfo(skb)->frag_list;
>> +	if (*frag)
>> +		goto frags;
>
> I think this would be more readable without the goto - just use a normal if 
> statement with a code block.  There's only one nested if statement.
>
>> +
>> +	count = min_t(unsigned int, (conn->mtu - L2CAP_HDR_SIZE),
>> +		      chan->omtu);
>> +	count = count - skb->len + L2CAP_HDR_SIZE;
>> +	count = min_t(unsigned int, count, len);
>> +
>> +	if (memcpy_fromiovec(skb_put(skb, count), msg->msg_iov, count))
>> +		return -EFAULT;
>> +
>> +	sent += count;
>> +	len  -= count;
>> +
>> +frags:
>> +	while (*frag)
>> +		frag = &(*frag)->next;
>> +
>> +	while (len) {
>> +		count = chan->omtu - skb->len + L2CAP_HDR_SIZE;
>> +		count = min_t(unsigned int, count, len);
>> +		count = min_t(unsigned int, conn->mtu, count);
>> +
>> +		*frag = chan->ops->alloc_skb(chan, count,
>> +					     msg->msg_flags & MSG_DONTWAIT);
>> +		if (IS_ERR(*frag))
>> +			return PTR_ERR(*frag);
>> +
>> +		if (memcpy_fromiovec(skb_put(*frag, count), msg->msg_iov,
>> +				     count))
>> +			return -EFAULT;
>> +
>> +		(*frag)->priority = skb->priority;
>> +
>> +		sent += count;
>> +		len  -= count;
>> +
>> +		skb->len += (*frag)->len;
>> +		skb->data_len += (*frag)->len;
>> +
>> +		frag = &(*frag)->next;
>> +
>> +		if (skb->len == chan->omtu + L2CAP_HDR_SIZE)
>> +			break;
>
> Don't you want to return -EMSGSIZE if the data doesn't fit in one SDU?
>
>> +	}
>> +
>> +	lh = (struct l2cap_hdr *) skb->data;
>> +	lh->len = cpu_to_le16(skb->len - L2CAP_HDR_SIZE);
>> +
>> +	if (skb->len == chan->omtu + L2CAP_HDR_SIZE) {
>> +		l2cap_do_send(chan, skb);
>
> I don't think it's good to put a send in here.  Let the calling function do 
> the send, so it's in one place.
>
>> +		chan->skb_more = NULL;
>> +	}
>> +
>> +	return sent;
>> +}
>> +
>> int l2cap_chan_send(struct l2cap_chan *chan, struct msghdr *msg, size_t 
>> len,
>> 								u32 priority)
>> {
>> @@ -2068,16 +2143,41 @@ int l2cap_chan_send(struct l2cap_chan *chan, struct 
>> msghdr *msg, size_t len,
>> 	switch (chan->mode) {
>> 	case L2CAP_MODE_BASIC:
>> 		/* Check outgoing MTU */
>> -		if (len > chan->omtu)
>> +		if (len > chan->omtu) {
>> +			kfree_skb(chan->skb_more);
>
> Set skb_more to NULL after freeing.
>
>> 			return -EMSGSIZE;
>> +		}
>> 
>> -		/* Create a basic PDU */
>> -		skb = l2cap_create_basic_pdu(chan, msg, len, priority);
>> -		if (IS_ERR(skb))
>> +		err = len;
>> +		if (chan->skb_more) {
>> +			int sent = l2cap_append_pdu(chan, msg, len);
>> +
>> +			if (sent < 0) {
>> +				kfree_skb(chan->skb_more);
>> +				return sent;
>> +			}
>> +
>> +			len -= sent;
>> +		}
>> +
>> +		if (len)
>> +			skb = l2cap_create_basic_pdu(chan, msg, len, 
>> priority);
>
> Shouldn't this be the 'else' clause for the above if statement?  You should 
> either call l2cap_append_pdu or l2cap_create_basic_pdu, but never both. 
> Better to structure the logic so that they are obviously mutually exclusive.
>
>> +		else
>> +			skb = chan->skb_more;
>> +
>> +		if (IS_ERR(skb)) {
>> +			kfree_skb(chan->skb_more);
>
> Set skb_more to NULL after freeing.
>
>> 			return PTR_ERR(skb);
>> +		}
>> +
>> +		if (!skb)
>> +			return err;
>> +
>> +		if (msg->msg_flags & MSG_MORE && skb->len < chan->omtu)
>> +			chan->skb_more = skb;
>> +		else
>> +			l2cap_do_send(chan, skb);
>
> I think l2cap_do_send() should be called if and only if MSG_MORE is not set, 
> unless there is an MTU problem.
>
> Also, do you need to account for L2CAP_HDR_SIZE when checking skb->len?
>
>> 
>> -		l2cap_do_send(chan, skb);
>> -		err = len;
>> 		break;
>>
>> 	case L2CAP_MODE_ERTM:
>> -- 
>> 1.7.10.1
>
> Overall, I would suggest that l2cap_chan_send should be kept simple and most 
> of the MSG_MORE code should be in a function called by l2cap_chan_send.

One more thing: also make sure that chan->skb_more is freed when the 
channel is deleted, since there may be data queued when an L2CAP 
channel, ACL, or socket is disconnected.

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

Re: [PATCH 3/4] Bluetooth: Add MSG_MORE support to L2CAP sockets

[Gustavo Padovan]

Hi Mat.

* Mat Martineau <mathewm@codeaurora.org> [2012-05-11 11:31:50 -0700]:

> 
> Hi Gustavo -
> 
> On Fri, 11 May 2012, Gustavo Padovan wrote:
> 
> >MSG_MORE enables us to save buffer space in userspace, the packet are
> >built directly in the kernel and sent when their size reaches the Output
> >MTU value.
> >
> >Only MSG_MORE for Basic Mode is supported right now. chan->skb_more keeps
> >a pointer to the L2CAP packet that is being build through many calls to
> >send().
> 
> Could you explain more about how you expect it to work?
> 
> I would assume the application would do a series of sends:
> 
> send(fd, buf, len, MSG_MORE);
> ...
> send(fd, buf, len, MSG_MORE);
> ...
> send(fd, buf, len, MSG_MORE);
> ...
> send(fd, buf, len, 0);
> 
> and the SDU would be sent the first time there is no MSG_MORE flag.
> If the MTU is exceeded, the SDU is not sent and an error is
> returned.

No, a PDU is sent every time the buffered data achieves the size of the
channel MTU, then skb_more starts to buffer the next pdu to be sent.
A send with the MSG_MORE flag unset would tell to send everything that is
queued.

> What should happen if a send() with MSG_MORE completely fills an SDU
> (length of data sent is equal to MTU)?  Does it make sense to treat
> it like a normal send, or return an error so that application knows
> that later calls with MSG_MORE will not append?  Or does the full
> SDU not get sent, and a zero-length send() with no MSG_MORE would
> trigger the transmission?

It get sent right away. Also I don't think the application needs to know such
kind of information. It deliveries the data to the kernel and the kernel
decides when to send the data.

> >
> >Signed-off-by: Gustavo Padovan <gustavo@padovan.org>
> >---
> >include/net/bluetooth/l2cap.h |    2 +
> >net/bluetooth/l2cap_core.c    |  116 ++++++++++++++++++++++++++++++++++++++---
> >2 files changed, 110 insertions(+), 8 deletions(-)
> >
> >diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
> >index 1c7d1cd..5f2845d 100644
> >--- a/include/net/bluetooth/l2cap.h
> >+++ b/include/net/bluetooth/l2cap.h
> >@@ -520,6 +520,8 @@ struct l2cap_chan {
> >	struct list_head	list;
> >	struct list_head	global_l;
> >
> >+	struct sk_buff		*skb_more;
> >+
> >	void			*data;
> >	struct l2cap_ops	*ops;
> >	struct mutex		lock;
> >diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> >index e5a4fd9..73bf8a8 100644
> >--- a/net/bluetooth/l2cap_core.c
> >+++ b/net/bluetooth/l2cap_core.c
> >@@ -1827,6 +1827,8 @@ static inline int l2cap_skbuff_fromiovec(struct l2cap_chan *chan,
> >	struct sk_buff **frag;
> >	int sent = 0;
> >
> >+	count = min_t(unsigned int, count, len);
> >+
> >	if (memcpy_fromiovec(skb_put(skb, count), msg->msg_iov, count))
> >		return -EFAULT;
> >
> >@@ -1903,9 +1905,13 @@ static struct sk_buff *l2cap_create_basic_pdu(struct l2cap_chan *chan,
> >	int err, count;
> >	struct l2cap_hdr *lh;
> >
> >-	BT_DBG("chan %p len %d", chan, (int)len);
> >+	BT_DBG("chan %p len %d aclmtu %d" , chan, (int)len, conn->mtu);
> >
> >-	count = min_t(unsigned int, (conn->mtu - L2CAP_HDR_SIZE), len);
> >+	if (msg->msg_flags & MSG_MORE)
> >+		count = min_t(unsigned int, (conn->mtu - L2CAP_HDR_SIZE),
> >+			      chan->omtu);
> >+	else
> >+		count = min_t(unsigned int, (conn->mtu - L2CAP_HDR_SIZE), len);
> >
> >	skb = chan->ops->alloc_skb(chan, count + L2CAP_HDR_SIZE,
> >				   msg->msg_flags & MSG_DONTWAIT);
> >@@ -2048,6 +2054,75 @@ static int l2cap_segment_sdu(struct l2cap_chan *chan,
> >	return err;
> >}
> >
> >+static int l2cap_append_pdu(struct l2cap_chan *chan, struct msghdr *msg,
> >+			    size_t len)
> >+{
> >+	struct l2cap_conn *conn = chan->conn;
> >+	struct sk_buff **frag, *skb = chan->skb_more;
> >+	int sent = 0;
> >+	unsigned int count;
> >+	struct l2cap_hdr *lh;
> >+
> >+	BT_DBG("chan %p len %ld", chan, len);
> >+
> >+	frag = &skb_shinfo(skb)->frag_list;
> >+	if (*frag)
> >+		goto frags;
> 
> I think this would be more readable without the goto - just use a
> normal if statement with a code block.  There's only one nested if
> statement.
> 
> >+
> >+	count = min_t(unsigned int, (conn->mtu - L2CAP_HDR_SIZE),
> >+		      chan->omtu);
> >+	count = count - skb->len + L2CAP_HDR_SIZE;
> >+	count = min_t(unsigned int, count, len);
> >+
> >+	if (memcpy_fromiovec(skb_put(skb, count), msg->msg_iov, count))
> >+		return -EFAULT;
> >+
> >+	sent += count;
> >+	len  -= count;
> >+
> >+frags:
> >+	while (*frag)
> >+		frag = &(*frag)->next;
> >+
> >+	while (len) {
> >+		count = chan->omtu - skb->len + L2CAP_HDR_SIZE;
> >+		count = min_t(unsigned int, count, len);
> >+		count = min_t(unsigned int, conn->mtu, count);
> >+
> >+		*frag = chan->ops->alloc_skb(chan, count,
> >+					     msg->msg_flags & MSG_DONTWAIT);
> >+		if (IS_ERR(*frag))
> >+			return PTR_ERR(*frag);
> >+
> >+		if (memcpy_fromiovec(skb_put(*frag, count), msg->msg_iov,
> >+				     count))
> >+			return -EFAULT;
> >+
> >+		(*frag)->priority = skb->priority;
> >+
> >+		sent += count;
> >+		len  -= count;
> >+
> >+		skb->len += (*frag)->len;
> >+		skb->data_len += (*frag)->len;
> >+
> >+		frag = &(*frag)->next;
> >+
> >+		if (skb->len == chan->omtu + L2CAP_HDR_SIZE)
> >+			break;
> 
> Don't you want to return -EMSGSIZE if the data doesn't fit in one SDU?

You are reading it wrong. This checks if the buffered achieve the OMTU size,
if yes we break and send the data. Later l2cap_create_basic_pdu() is called to
deal with the remaining data.

> 
> >+	}
> >+
> >+	lh = (struct l2cap_hdr *) skb->data;
> >+	lh->len = cpu_to_le16(skb->len - L2CAP_HDR_SIZE);
> >+
> >+	if (skb->len == chan->omtu + L2CAP_HDR_SIZE) {
> >+		l2cap_do_send(chan, skb);
> 
> I don't think it's good to put a send in here.  Let the calling
> function do the send, so it's in one place.

This make the logic much more simple, otherwise extra checks will be needed in
l2cap_chan_send()

> 
> >+		chan->skb_more = NULL;
> >+	}
> >+
> >+	return sent;
> >+}
> >+
> >int l2cap_chan_send(struct l2cap_chan *chan, struct msghdr *msg, size_t len,
> >								u32 priority)
> >{
> >@@ -2068,16 +2143,41 @@ int l2cap_chan_send(struct l2cap_chan *chan, struct msghdr *msg, size_t len,
> >	switch (chan->mode) {
> >	case L2CAP_MODE_BASIC:
> >		/* Check outgoing MTU */
> >-		if (len > chan->omtu)
> >+		if (len > chan->omtu) {
> >+			kfree_skb(chan->skb_more);
> 
> Set skb_more to NULL after freeing.

Yes.

> 
> >			return -EMSGSIZE;
> >+		}
> >
> >-		/* Create a basic PDU */
> >-		skb = l2cap_create_basic_pdu(chan, msg, len, priority);
> >-		if (IS_ERR(skb))
> >+		err = len;
> >+		if (chan->skb_more) {
> >+			int sent = l2cap_append_pdu(chan, msg, len);
> >+
> >+			if (sent < 0) {
> >+				kfree_skb(chan->skb_more);
> >+				return sent;
> >+			}
> >+
> >+			len -= sent;
> >+		}
> >+
> >+		if (len)
> >+			skb = l2cap_create_basic_pdu(chan, msg, len, priority);
> 
> Shouldn't this be the 'else' clause for the above if statement?  You
> should either call l2cap_append_pdu or l2cap_create_basic_pdu, but
> never both.  Better to structure the logic so that they are
> obviously mutually exclusive.

No, that is the continuation of what I send above, if we call
l2cap_append_pdu(), completely fill a pdu and send it we would new a call to
l2cap_create_basic_pdu() if there is more data to queue (len > 0)

> 
> >+		else
> >+			skb = chan->skb_more;
> >+
> >+		if (IS_ERR(skb)) {
> >+			kfree_skb(chan->skb_more);
> 
> Set skb_more to NULL after freeing.

Yes.

> 
> >			return PTR_ERR(skb);
> >+		}
> >+
> >+		if (!skb)
> >+			return err;
> >+
> >+		if (msg->msg_flags & MSG_MORE && skb->len < chan->omtu)
> >+			chan->skb_more = skb;
> >+		else
> >+			l2cap_do_send(chan, skb);
> 
> I think l2cap_do_send() should be called if and only if MSG_MORE is
> not set, unless there is an MTU problem.

If the queued data equals the omtu then we send it.

> 
> Also, do you need to account for L2CAP_HDR_SIZE when checking
> skb->len?

I think so.

	Gustavo

Re: [PATCH 3/4] Bluetooth: Add MSG_MORE support to L2CAP sockets

[Mat Martineau]

Gustavo -

I've added Luiz as a cc: because I assume he's interested in MSG_MORE 
for OBEX over L2CAP and might have an opinion about the behavior of 
this feature.

On Fri, 11 May 2012, Gustavo Padovan wrote:

> Hi Mat.
>
> * Mat Martineau <mathewm@codeaurora.org> [2012-05-11 11:31:50 -0700]:
>
>>
>> Hi Gustavo -
>>
>> On Fri, 11 May 2012, Gustavo Padovan wrote:
>>
>>> MSG_MORE enables us to save buffer space in userspace, the packet are
>>> built directly in the kernel and sent when their size reaches the Output
>>> MTU value.
>>>
>>> Only MSG_MORE for Basic Mode is supported right now. chan->skb_more keeps
>>> a pointer to the L2CAP packet that is being build through many calls to
>>> send().
>>
>> Could you explain more about how you expect it to work?
>>
>> I would assume the application would do a series of sends:
>>
>> send(fd, buf, len, MSG_MORE);
>> ...
>> send(fd, buf, len, MSG_MORE);
>> ...
>> send(fd, buf, len, MSG_MORE);
>> ...
>> send(fd, buf, len, 0);
>>
>> and the SDU would be sent the first time there is no MSG_MORE flag.
>> If the MTU is exceeded, the SDU is not sent and an error is
>> returned.
>
> No, a PDU is sent every time the buffered data achieves the size of the
> channel MTU, then skb_more starts to buffer the next pdu to be sent.
> A send with the MSG_MORE flag unset would tell to send everything that is
> queued.

Given that L2CAP is a datagram protocol, I don't think triggering a 
send every time a PDU is filled is the right thing to do.

>> What should happen if a send() with MSG_MORE completely fills an SDU
>> (length of data sent is equal to MTU)?  Does it make sense to treat
>> it like a normal send, or return an error so that application knows
>> that later calls with MSG_MORE will not append?  Or does the full
>> SDU not get sent, and a zero-length send() with no MSG_MORE would
>> trigger the transmission?
>
> It get sent right away. Also I don't think the application needs to 
> know such kind of information. It deliveries the data to the kernel 
> and the kernel decides when to send the data.

It is critical for the application to exactly control the beginning 
and end of each PDU.  When other remote stacks receive data and pass 
it up to a Bluetooth profile, those profiles depend on having a 
complete PDU for the profile to work with.  An L2CAP connection is not 
a stream, the specifications don't treat it that way and neither do 
other Bluetooth stacks.

Consider A2DP.  An SBC media packet has a header byte, followed by a 
number of SBC frames.  The header specifies the number of complete 
frames that follow in that PDU.

Here's a simplified example, and assume the MTU is 500 bytes.

send(fd, header_buf, 1, MSG_MORE); // header says there are 5 frames
send(fd, frame1, 100, MSG_MORE); // SBC frame 1
send(fd, frame2, 100, MSG_MORE); // SBC frame 2
send(fd, frame3, 100, MSG_MORE); // SBC frame 3
send(fd, frame4, 100, MSG_MORE); // SBC frame 4
send(fd, frame5, 100, 0);        // SBC frame 5

This would send out two PDUs:

* First PDU is 500 bytes.  The last SBC frame is incomplete (which 
would trigger a decoder error in SBC).  The remote device would not 
simply look for more data in the next PDU.

* Second PDU is one byte.  It is not a valid header because it is the 
last byte of SBC data.  The header would essentially be random data, 
and the receiver would experience a second error.

I'm suggesting that it's better for the "SBC frame 5" send to return 
EMSGSIZE, exactly like what would have happened if the 501-byte frame 
had been sent using a single call to send() with no MSG_MORE.  The 
profile code is doing something invalid, and it should know that there 
was an error.  Having the kernel silently split up PDUs does not help 
anybody.

>>>
>>> Signed-off-by: Gustavo Padovan <gustavo@padovan.org>
>>> ---
>>> include/net/bluetooth/l2cap.h |    2 +
>>> net/bluetooth/l2cap_core.c    |  116 ++++++++++++++++++++++++++++++++++++++---
>>> 2 files changed, 110 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
>>> index 1c7d1cd..5f2845d 100644
>>> --- a/include/net/bluetooth/l2cap.h
>>> +++ b/include/net/bluetooth/l2cap.h
>>> @@ -520,6 +520,8 @@ struct l2cap_chan {
>>> 	struct list_head	list;
>>> 	struct list_head	global_l;
>>>
>>> +	struct sk_buff		*skb_more;
>>> +
>>> 	void			*data;
>>> 	struct l2cap_ops	*ops;
>>> 	struct mutex		lock;
>>> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
>>> index e5a4fd9..73bf8a8 100644
>>> --- a/net/bluetooth/l2cap_core.c
>>> +++ b/net/bluetooth/l2cap_core.c
>>> @@ -1827,6 +1827,8 @@ static inline int l2cap_skbuff_fromiovec(struct l2cap_chan *chan,
>>> 	struct sk_buff **frag;
>>> 	int sent = 0;
>>>
>>> +	count = min_t(unsigned int, count, len);
>>> +
>>> 	if (memcpy_fromiovec(skb_put(skb, count), msg->msg_iov, count))
>>> 		return -EFAULT;
>>>
>>> @@ -1903,9 +1905,13 @@ static struct sk_buff *l2cap_create_basic_pdu(struct l2cap_chan *chan,
>>> 	int err, count;
>>> 	struct l2cap_hdr *lh;
>>>
>>> -	BT_DBG("chan %p len %d", chan, (int)len);
>>> +	BT_DBG("chan %p len %d aclmtu %d" , chan, (int)len, conn->mtu);
>>>
>>> -	count = min_t(unsigned int, (conn->mtu - L2CAP_HDR_SIZE), len);
>>> +	if (msg->msg_flags & MSG_MORE)
>>> +		count = min_t(unsigned int, (conn->mtu - L2CAP_HDR_SIZE),
>>> +			      chan->omtu);
>>> +	else
>>> +		count = min_t(unsigned int, (conn->mtu - L2CAP_HDR_SIZE), len);
>>>
>>> 	skb = chan->ops->alloc_skb(chan, count + L2CAP_HDR_SIZE,
>>> 				   msg->msg_flags & MSG_DONTWAIT);
>>> @@ -2048,6 +2054,75 @@ static int l2cap_segment_sdu(struct l2cap_chan *chan,
>>> 	return err;
>>> }
>>>
>>> +static int l2cap_append_pdu(struct l2cap_chan *chan, struct msghdr *msg,
>>> +			    size_t len)
>>> +{
>>> +	struct l2cap_conn *conn = chan->conn;
>>> +	struct sk_buff **frag, *skb = chan->skb_more;
>>> +	int sent = 0;
>>> +	unsigned int count;
>>> +	struct l2cap_hdr *lh;
>>> +
>>> +	BT_DBG("chan %p len %ld", chan, len);
>>> +
>>> +	frag = &skb_shinfo(skb)->frag_list;
>>> +	if (*frag)
>>> +		goto frags;
>>
>> I think this would be more readable without the goto - just use a
>> normal if statement with a code block.  There's only one nested if
>> statement.
>>
>>> +
>>> +	count = min_t(unsigned int, (conn->mtu - L2CAP_HDR_SIZE),
>>> +		      chan->omtu);
>>> +	count = count - skb->len + L2CAP_HDR_SIZE;
>>> +	count = min_t(unsigned int, count, len);
>>> +
>>> +	if (memcpy_fromiovec(skb_put(skb, count), msg->msg_iov, count))
>>> +		return -EFAULT;
>>> +
>>> +	sent += count;
>>> +	len  -= count;
>>> +
>>> +frags:
>>> +	while (*frag)
>>> +		frag = &(*frag)->next;
>>> +
>>> +	while (len) {
>>> +		count = chan->omtu - skb->len + L2CAP_HDR_SIZE;
>>> +		count = min_t(unsigned int, count, len);
>>> +		count = min_t(unsigned int, conn->mtu, count);
>>> +
>>> +		*frag = chan->ops->alloc_skb(chan, count,
>>> +					     msg->msg_flags & MSG_DONTWAIT);
>>> +		if (IS_ERR(*frag))
>>> +			return PTR_ERR(*frag);
>>> +
>>> +		if (memcpy_fromiovec(skb_put(*frag, count), msg->msg_iov,
>>> +				     count))
>>> +			return -EFAULT;
>>> +
>>> +		(*frag)->priority = skb->priority;
>>> +
>>> +		sent += count;
>>> +		len  -= count;
>>> +
>>> +		skb->len += (*frag)->len;
>>> +		skb->data_len += (*frag)->len;
>>> +
>>> +		frag = &(*frag)->next;
>>> +
>>> +		if (skb->len == chan->omtu + L2CAP_HDR_SIZE)
>>> +			break;
>>
>> Don't you want to return -EMSGSIZE if the data doesn't fit in one SDU?
>
> You are reading it wrong. This checks if the buffered achieve the OMTU size,
> if yes we break and send the data. Later l2cap_create_basic_pdu() is called to
> deal with the remaining data.

Again, this doesn't keep the boundaries between PDUs that profiles 
expect.

>
>>
>>> +	}
>>> +
>>> +	lh = (struct l2cap_hdr *) skb->data;
>>> +	lh->len = cpu_to_le16(skb->len - L2CAP_HDR_SIZE);
>>> +
>>> +	if (skb->len == chan->omtu + L2CAP_HDR_SIZE) {
>>> +		l2cap_do_send(chan, skb);
>>
>> I don't think it's good to put a send in here.  Let the calling
>> function do the send, so it's in one place.
>
> This make the logic much more simple, otherwise extra checks will be needed in
> l2cap_chan_send()
>
>>
>>> +		chan->skb_more = NULL;
>>> +	}
>>> +
>>> +	return sent;
>>> +}
>>> +
>>> int l2cap_chan_send(struct l2cap_chan *chan, struct msghdr *msg, size_t len,
>>> 								u32 priority)
>>> {
>>> @@ -2068,16 +2143,41 @@ int l2cap_chan_send(struct l2cap_chan *chan, struct msghdr *msg, size_t len,
>>> 	switch (chan->mode) {
>>> 	case L2CAP_MODE_BASIC:
>>> 		/* Check outgoing MTU */
>>> -		if (len > chan->omtu)
>>> +		if (len > chan->omtu) {
>>> +			kfree_skb(chan->skb_more);
>>
>> Set skb_more to NULL after freeing.
>
> Yes.
>
>>
>>> 			return -EMSGSIZE;
>>> +		}
>>>
>>> -		/* Create a basic PDU */
>>> -		skb = l2cap_create_basic_pdu(chan, msg, len, priority);
>>> -		if (IS_ERR(skb))
>>> +		err = len;
>>> +		if (chan->skb_more) {
>>> +			int sent = l2cap_append_pdu(chan, msg, len);
>>> +
>>> +			if (sent < 0) {
>>> +				kfree_skb(chan->skb_more);
>>> +				return sent;
>>> +			}
>>> +
>>> +			len -= sent;
>>> +		}
>>> +
>>> +		if (len)
>>> +			skb = l2cap_create_basic_pdu(chan, msg, len, priority);
>>
>> Shouldn't this be the 'else' clause for the above if statement?  You
>> should either call l2cap_append_pdu or l2cap_create_basic_pdu, but
>> never both.  Better to structure the logic so that they are
>> obviously mutually exclusive.
>
> No, that is the continuation of what I send above, if we call
> l2cap_append_pdu(), completely fill a pdu and send it we would new a call to
> l2cap_create_basic_pdu() if there is more data to queue (len > 0)
>
>>
>>> +		else
>>> +			skb = chan->skb_more;
>>> +
>>> +		if (IS_ERR(skb)) {
>>> +			kfree_skb(chan->skb_more);
>>
>> Set skb_more to NULL after freeing.
>
> Yes.
>
>>
>>> 			return PTR_ERR(skb);
>>> +		}
>>> +
>>> +		if (!skb)
>>> +			return err;
>>> +
>>> +		if (msg->msg_flags & MSG_MORE && skb->len < chan->omtu)
>>> +			chan->skb_more = skb;
>>> +		else
>>> +			l2cap_do_send(chan, skb);
>>
>> I think l2cap_do_send() should be called if and only if MSG_MORE is
>> not set, unless there is an MTU problem.
>
> If the queued data equals the omtu then we send it.
>
>>
>> Also, do you need to account for L2CAP_HDR_SIZE when checking
>> skb->len?
>
> I think so.


Can you provide a use case where letting the kernel pick the PDU 
boundaries based on the MTU helps a profile or application?  It seems 
like only buggy profiles would attempt to send more data than the MTU 
allows, so it doesn't make sense to let them do that.

If you only send a PDU when the MSG_MORE flag is unset and return 
EMSGSIZE whenever the MTU is exceeded, you get simpler kernel code and 
less confusion among profile developers.

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

Re: [PATCH 1/4] Bluetooth: Fix packet size provided to the controller

[Johan Hedberg]

Hi Gustavo,

On Fri, May 11, 2012, Gustavo Padovan wrote:
> When building fragmented skb's skb->len keeps track of the size of head
> plus all fragments combined, however when queueing the skb for sending we
> need to report the head size instead of the total size, so we just set
> skb->len to skb_headlen().
> 
> This bug appeared when implementing MSG_MORE support for L2CAP sockets, it
> never showed up before because l2cap_skbuff_fromiovec() never accounted skb
> size correctly. A following patch will fix this.
> 
> Signed-off-by: Gustavo Padovan <gustavo@padovan.org>
> Reviewed-by: Mat Martineau <mathewm@codeaurora.org>
> ---
>  net/bluetooth/hci_core.c |    8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)

Patches 1 and 2 have been applied to bluetooth-next. Thanks.

Johan