Re: RFC: Allow Bluez to select flushable or non-flushable ACL packets with L2CAP_LM_RELIABLE

[Nick Pelly <npelly@google.com>]

[-- Attachment #1: Type: text/plain, Size: 2236 bytes --]

Hi Marcel,

On Thu, Dec 10, 2009 at 2:03 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Nick,
>
>> >> Right now Bluez always requests flushable ACL packets (but does not
>> >> set a flush timeout, so effectively they are non-flushable):
>> >>
>> >> However it is desirable to use an ACL flush timeout on A2DP packets so
>> >> that if the ACL packets block for some reason then the LM can flush
>> >> them to make room for newer packets.
>> >>
>> >> Is it reasonable for Bluez to use the 0x00 ACL packet boundary flag by
>> >> default (non-flushable packet), and let userspace request flushable
>> >> packets on A2DP L2CAP sockets with the socket option
>> >> L2CAP_LM_RELIABLE.
>> >
>> > the reliable option has a different meaning. It comes back from the old
>> > Bluetooth 1.1 qualification days where we had to tests on L2CAP that had
>> > to confirm that we can detect malformed packets and report them. These
>> > days it is just fine to drop them.
>>
>> Got it, how about introducing
>>
>> #define L2CAP_LM_FLUSHABLE 0x0040
>
> that l2cap_sock_setsockopt_old() sets this didn't give you a hint that
> we might wanna deprecate this socket options ;)
>
> I need to read up on the flushable stuff, but in the end it deserves its
> own socket option. Also an ioctl() to actually trigger Enhanced flush
> might be needed.
>
>> struct l2cap_pinfo {
>>    ...
>>    __u8 flushable;
>> }
>
> Sure. In the long run we need to turn this into a bitmask. We are just
> wasting memory here.

Attached is an updated patch, that checks the LMP features bitmask
before using the new non-flushable packet type.

I am still using L2CAP_LM_FLUSHABLE socket option in
l2cap_sock_setsockopt_old(), which I don't think you are happy with.
So how about a new option:

SOL_L2CAP, L2CAP_ACL_FLUSH
which has a default value of 0, and can be set to 1 to make the ACL
data sent by this L2CAP socket flushable.

In a later commit we would then add
SOL_ACL, ACL_FLUSH_TIMEOUT
That is used to set an automatic flush timeout for the ACL link on a
L2CAP socket. Note that SOL_ACL is new.

But maybe this is not what you had in mind, so I'm looking for your
advice before I implement this.

Nick

[-- Attachment #2: 0001-Bluetooth-Use-non-flushable-pb-flag-by-default-for-A.patch --]
[-- Type: application/octet-stream, Size: 6871 bytes --]

From ce1d3ab4dfe3fc58f1e187581b4f8c72fb48a306 Mon Sep 17 00:00:00 2001
From: Nick Pelly <npelly@google.com>
Date: Tue, 8 Dec 2009 19:42:21 -0800
Subject: [PATCH] Bluetooth: Use non-flushable pb flag by default for ACL data on capable chipsets.

With Bluetooth 2.1 ACL packets can be flushable or non-flushable. This commit
makes ACL data packets non-flushable by default on compatible chipsets, and
adds the L2CAP_LM_FLUSHABLE socket option to explicitly request flushable ACL
data packets for a given L2CAP socket. This is useful for A2DP data which can
be safely discarded if it can not be delivered within a short time (while
other ACL data should not be discarded).

Note that making ACL data flushable has no effect unless the automatic flush
timeout for that ACL link is changed from its default of 0 (infinite).

Signed-off-by: Nick Pelly <npelly@google.com>
---
 include/net/bluetooth/hci.h      |    4 ++++
 include/net/bluetooth/hci_core.h |    1 +
 include/net/bluetooth/l2cap.h    |    2 ++
 net/bluetooth/hci_core.c         |    6 ++++--
 net/bluetooth/l2cap.c            |   24 +++++++++++++++++++++---
 5 files changed, 32 insertions(+), 5 deletions(-)

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index 5dbd6a4..44bb34c 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -142,11 +142,14 @@ enum {
 #define EDR_ESCO_MASK  (ESCO_2EV3 | ESCO_3EV3 | ESCO_2EV5 | ESCO_3EV5)
 
 /* ACL flags */
+#define ACL_START_NO_FLUSH	0x00
 #define ACL_CONT		0x01
 #define ACL_START		0x02
 #define ACL_ACTIVE_BCAST	0x04
 #define ACL_PICO_BCAST		0x08
 
+#define ACL_PB_MASK	(ACL_CONT | ACL_START)
+
 /* Baseband links */
 #define SCO_LINK	0x00
 #define ACL_LINK	0x01
@@ -185,6 +188,7 @@ enum {
 #define LMP_EDR_ESCO_3M	0x40
 #define LMP_EDR_3S_ESCO	0x80
 
+#define LMP_NO_FLUSH	0x01
 #define LMP_SIMPLE_PAIR	0x08
 
 /* Connection modes */
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index ed16efb..9072d64 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -468,6 +468,7 @@ void hci_conn_del_sysfs(struct hci_conn *conn);
 #define lmp_sniffsubr_capable(dev) ((dev)->features[5] & LMP_SNIFF_SUBR)
 #define lmp_esco_capable(dev)      ((dev)->features[3] & LMP_ESCO)
 #define lmp_ssp_capable(dev)       ((dev)->features[6] & LMP_SIMPLE_PAIR)
+#define lmp_no_flush_capable(dev)  ((dev)->features[6] & LMP_NO_FLUSH)
 
 /* ----- HCI protocols ----- */
 struct hci_proto {
diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index f566aa1..9e58e43 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -62,6 +62,7 @@ struct l2cap_conninfo {
 #define L2CAP_LM_TRUSTED	0x0008
 #define L2CAP_LM_RELIABLE	0x0010
 #define L2CAP_LM_SECURE		0x0020
+#define L2CAP_LM_FLUSHABLE	0x0040
 
 /* L2CAP command codes */
 #define L2CAP_COMMAND_REJ 0x01
@@ -245,6 +246,7 @@ struct l2cap_pinfo {
 	__u8            sec_level;
 	__u8		role_switch;
 	__u8            force_reliable;
+	__u8            flushable;
 
 	__u8		conf_req[64];
 	__u8		conf_len;
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index cd06151..aeaf07f 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1200,7 +1200,7 @@ int hci_send_acl(struct hci_conn *conn, struct sk_buff *skb, __u16 flags)
 
 	skb->dev = (void *) hdev;
 	bt_cb(skb)->pkt_type = HCI_ACLDATA_PKT;
-	hci_add_acl_hdr(skb, conn->handle, flags | ACL_START);
+	hci_add_acl_hdr(skb, conn->handle, flags);
 
 	if (!(list = skb_shinfo(skb)->frag_list)) {
 		/* Non fragmented */
@@ -1217,12 +1217,14 @@ int hci_send_acl(struct hci_conn *conn, struct sk_buff *skb, __u16 flags)
 		spin_lock_bh(&conn->data_q.lock);
 
 		__skb_queue_tail(&conn->data_q, skb);
+		flags &= ~ACL_PB_MASK;
+		flags |= ACL_CONT;
 		do {
 			skb = list; list = list->next;
 
 			skb->dev = (void *) hdev;
 			bt_cb(skb)->pkt_type = HCI_ACLDATA_PKT;
-			hci_add_acl_hdr(skb, conn->handle, flags | ACL_CONT);
+			hci_add_acl_hdr(skb, conn->handle, flags);
 
 			BT_DBG("%s frag %p len %d", hdev->name, skb, skb->len);
 
diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
index ca4d3b4..90dfaf3 100644
--- a/net/bluetooth/l2cap.c
+++ b/net/bluetooth/l2cap.c
@@ -319,13 +319,19 @@ static inline u8 l2cap_get_ident(struct l2cap_conn *conn)
 static inline int 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);
+	u8 flags;
 
 	BT_DBG("code 0x%2.2x", code);
 
 	if (!skb)
 		return -ENOMEM;
 
-	return hci_send_acl(conn->hcon, skb, 0);
+	if (lmp_no_flush_capable(conn->hdev))
+		flags = ACL_START_NO_FLUSH;
+	else
+		flags = ACL_START;
+
+	return hci_send_acl(conn->hcon, skb, flags);
 }
 
 static void l2cap_do_start(struct sock *sk)
@@ -714,12 +720,14 @@ static void l2cap_sock_init(struct sock *sk, struct sock *parent)
 		pi->sec_level = l2cap_pi(parent)->sec_level;
 		pi->role_switch = l2cap_pi(parent)->role_switch;
 		pi->force_reliable = l2cap_pi(parent)->force_reliable;
+		pi->flushable = l2cap_pi(parent)->flushable;
 	} else {
 		pi->imtu = L2CAP_DEFAULT_MTU;
 		pi->omtu = 0;
 		pi->sec_level = BT_SECURITY_LOW;
 		pi->role_switch = 0;
 		pi->force_reliable = 0;
+		pi->flushable = 0;
 	}
 
 	/* Default config options */
@@ -1116,6 +1124,7 @@ static inline int l2cap_do_send(struct sock *sk, struct msghdr *msg, int len)
 	struct sk_buff *skb, **frag;
 	int err, hlen, count, sent=0;
 	struct l2cap_hdr *lh;
+	u16 flags;
 
 	BT_DBG("sk %p len %d", sk, len);
 
@@ -1168,7 +1177,12 @@ static inline int l2cap_do_send(struct sock *sk, struct msghdr *msg, int len)
 		frag = &(*frag)->next;
 	}
 
-	if ((err = hci_send_acl(conn->hcon, skb, 0)) < 0)
+	if (lmp_no_flush_capable(conn->hdev) && !l2cap_pi(sk)->flushable)
+		flags = ACL_START_NO_FLUSH;
+	else
+		flags = ACL_START;
+
+	if ((err = hci_send_acl(conn->hcon, skb, flags)) < 0)
 		goto fail;
 
 	return sent;
@@ -1277,6 +1291,7 @@ static int l2cap_sock_setsockopt_old(struct socket *sock, int optname, char __us
 
 		l2cap_pi(sk)->role_switch    = (opt & L2CAP_LM_MASTER);
 		l2cap_pi(sk)->force_reliable = (opt & L2CAP_LM_RELIABLE);
+		l2cap_pi(sk)->flushable = (opt & L2CAP_LM_FLUSHABLE);
 		break;
 
 	default:
@@ -1403,6 +1418,9 @@ static int l2cap_sock_getsockopt_old(struct socket *sock, int optname, char __us
 		if (l2cap_pi(sk)->force_reliable)
 			opt |= L2CAP_LM_RELIABLE;
 
+		if (l2cap_pi(sk)->flushable)
+			opt |= L2CAP_LM_FLUSHABLE;
+
 		if (put_user(opt, (u32 __user *) optval))
 			err = -EFAULT;
 		break;
@@ -2613,7 +2631,7 @@ static int l2cap_recv_acldata(struct hci_conn *hcon, struct sk_buff *skb, u16 fl
 
 	BT_DBG("conn %p len %d flags 0x%x", conn, skb->len, flags);
 
-	if (flags & ACL_START) {
+	if (!(flags & ACL_CONT)) {
 		struct l2cap_hdr *hdr;
 		int len;
 
-- 
1.6.5.3