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

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

[Nick Pelly]

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

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.

Make sense? Any other suggestions?

Attached is a rough (untested) patch of what I had in mind.

See core spec 2.E.5.4.2 for reference.

Nick

[-- Attachment #2: 0001-Bluetooth-Send-non-flushable-ACL-packets-by-default.patch --]
[-- Type: application/octet-stream, Size: 3129 bytes --]

From 90f997071d3e795d6e674821c1d0f4057edb5838 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: Send non-flushable ACL packets by default.

L2CAP sockets can request flushable ACL packets with L2CAP_LM_RELIABLE

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

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index f69f015..c6f6ce5 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_FLUSHABLE	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
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..bf16a6a 100644
--- a/net/bluetooth/l2cap.c
+++ b/net/bluetooth/l2cap.c
@@ -325,7 +325,7 @@ static inline int l2cap_send_cmd(struct l2cap_conn *conn, u8 ident, u8 code, u16
 	if (!skb)
 		return -ENOMEM;
 
-	return hci_send_acl(conn->hcon, skb, 0);
+	return hci_send_acl(conn->hcon, skb, ACL_START);
 }
 
 static void l2cap_do_start(struct sock *sk)
@@ -1116,6 +1116,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 +1169,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 (l2cap_pi(sk)->force_reliable)
+		flags = ACL_START;
+	else
+		flags = ACL_START_FLUSHABLE;
+
+	if ((err = hci_send_acl(conn->hcon, skb, flags)) < 0)
 		goto fail;
 
 	return sent;
-- 
1.6.5.3

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

[Marcel Holtmann]

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.

You can not just blindly use ACL_START_FLUSHABLE without proper version
checking. This feature has been introduced later and will not work with
all stacks out there. Please go through the different spec. versions to
see the difference.

Regards

Marcel

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

[Nick Pelly]

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

On Tue, Dec 8, 2009 at 9:06 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.
>

Ok, how about introducing

#define L2CAP_LM_FLUSHABLE 0x0040

struct l2cap_pinfo {
   ...
   __u8 flushable;
}


> You can not just blindly use ACL_START_FLUSHABLE without proper version
> checking. This feature has been introduced later and will not work with
> all stacks out there. Please go through the different spec. versions to
> see the difference.
>

Will do.

Nick

[-- Attachment #2: Type: text/html, Size: 1929 bytes --]

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

[Nick Pelly]

Hi Marcel,

On Tue, Dec 8, 2009 at 9:06 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

struct l2cap_pinfo {
   ...
   __u8 flushable;
}

> You can not just blindly use ACL_START_FLUSHABLE without proper version
> checking. This feature has been introduced later and will not work with
> all stacks out there. Please go through the different spec. versions to
> see the difference.

Will do.

Nick

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

[Marcel Holtmann]

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.

Regards

Marcel

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

[Nick Pelly]

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

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

[Marcel Holtmann]

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.

can I stop you right here (without even looking at the patch). We do
have the generic SOL_BLUETOOTH that you should be using. So adding
SOL_ACL is not a viable option at all.

Remember that it would also be nice if this works with RFCOMM sockets.
Even if the usefulness is limited for these sockets.

Regards

Marcel

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

[Nick Pelly]

On Wed, Dec 16, 2009 at 3:36 PM, Marcel Holtmann <marcel@holtmann.org> wrot=
e:
> Hi Nick,
>
>> >> >> Right now Bluez always requests flushable ACL packets (but does no=
t
>> >> >> set a flush timeout, so effectively they are non-flushable):
>> >> >>
>> >> >> However it is desirable to use an ACL flush timeout on A2DP packet=
s so
>> >> >> that if the ACL packets block for some reason then the LM can flus=
h
>> >> >> them to make room for newer packets.
>> >> >>
>> >> >> Is it reasonable for Bluez to use the 0x00 ACL packet boundary fla=
g by
>> >> >> default (non-flushable packet), and let userspace request flushabl=
e
>> >> >> 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 tha=
t had
>> >> > to confirm that we can detect malformed packets and report them. Th=
ese
>> >> > 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 i=
ts
>> > own socket option. Also an ioctl() to actually trigger Enhanced flush
>> > might be needed.
>> >
>> >> struct l2cap_pinfo {
>> >> =A0 =A0...
>> >> =A0 =A0__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.

Was this proposal ok?

>> 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.
>
> can I stop you right here (without even looking at the patch). We do
> have the generic SOL_BLUETOOTH that you should be using. So adding
> SOL_ACL is not a viable option at all.

This would be in a later patch, and SOL_BLUETOOTH, ACL_FLUSH_TIMEOUT
is fine too, or whatever you prefer.

Nick

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

[Marcel Holtmann]

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.
> 
> Was this proposal ok?

Even SOL_L2CAP goes away. Use SOL_BLUETOOTH for this.

> >> 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.
> >
> > can I stop you right here (without even looking at the patch). We do
> > have the generic SOL_BLUETOOTH that you should be using. So adding
> > SOL_ACL is not a viable option at all.
> 
> This would be in a later patch, and SOL_BLUETOOTH, ACL_FLUSH_TIMEOUT
> is fine too, or whatever you prefer.

Why not just use BT_FLUSHABLE and have it always take a timeout option
and then 0 means not flushable. And advantage of having it separated?

Regards

Marcel

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

[Nick Pelly]

On Fri, Dec 18, 2009 at 3:05 PM, Marcel Holtmann <marcel@holtmann.org> wrot=
e:
>
> Hi Nick,
>
> > >> >> >> Right now Bluez always requests flushable ACL packets (but doe=
s not
> > >> >> >> set a flush timeout, so effectively they are non-flushable):
> > >> >> >>
> > >> >> >> However it is desirable to use an ACL flush timeout on A2DP pa=
ckets 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 flus=
hable
> > >> >> >> 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 deserv=
es its
> > >> > own socket option. Also an ioctl() to actually trigger Enhanced fl=
ush
> > >> > might be needed.
> > >> >
> > >> >> struct l2cap_pinfo {
> > >> >> =A0 =A0...
> > >> >> =A0 =A0__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.
> >
> > Was this proposal ok?
>
> Even SOL_L2CAP goes away. Use SOL_BLUETOOTH for this.
>
> > >> 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.
> > >
> > > can I stop you right here (without even looking at the patch). We do
> > > have the generic SOL_BLUETOOTH that you should be using. So adding
> > > SOL_ACL is not a viable option at all.
> >
> > This would be in a later patch, and SOL_BLUETOOTH, ACL_FLUSH_TIMEOUT
> > is fine too, or whatever you prefer.
>
> Why not just use BT_FLUSHABLE and have it always take a timeout option
> and then 0 means not flushable. And advantage of having it separated?

I think keeping them=A0separate=A0makes it clear that the flush timeout is
global for a given ACL link, whereas the flushable/non-flushable
boolean is specific to a L2CAP channel. (Which is why I suggested
introducing a new level SOL_ACL for the ACL_FLUSH_TIMEOUT option -
since this option applies at the ACL level in the stack).

A specific advantage of this is that flushable packets can be enabled
without over-writing a previous flush timeout that was set on a
different L2CAP socket on the same ACL link. I guess this can also be
achieved with getsockopt() but that is racy.

> Regards
>
> Marcel
>
>

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

[Marcel Holtmann]

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.
> > >
> > > Was this proposal ok?
> >
> > Even SOL_L2CAP goes away. Use SOL_BLUETOOTH for this.
> >
> > > >> 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.
> > > >
> > > > can I stop you right here (without even looking at the patch). We do
> > > > have the generic SOL_BLUETOOTH that you should be using. So adding
> > > > SOL_ACL is not a viable option at all.
> > >
> > > This would be in a later patch, and SOL_BLUETOOTH, ACL_FLUSH_TIMEOUT
> > > is fine too, or whatever you prefer.
> >
> > Why not just use BT_FLUSHABLE and have it always take a timeout option
> > and then 0 means not flushable. And advantage of having it separated?
> 
> I think keeping them separate makes it clear that the flush timeout is
> global for a given ACL link, whereas the flushable/non-flushable
> boolean is specific to a L2CAP channel. (Which is why I suggested
> introducing a new level SOL_ACL for the ACL_FLUSH_TIMEOUT option -
> since this option applies at the ACL level in the stack).
> 
> A specific advantage of this is that flushable packets can be enabled
> without over-writing a previous flush timeout that was set on a
> different L2CAP socket on the same ACL link. I guess this can also be
> achieved with getsockopt() but that is racy.

I am talking here about Enhanced Flush support and that would happen on
a per ACL handle basis. So it actually almost applies on a per L2CAP
socket level. Only exception is if you establish two or more L2CAP
connections to the same remote device and set them all to flushable.
Then of course all of them will be flushed. So strictly speaking it
might be an ACL link feature, but we don't wanna use it that way. And in
practice you won't have multiple concurrent flushable L2CAP connections
to one remote device anyway.

Regards

Marcel

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

[Nick Pelly]

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

On Fri, Dec 18, 2009 at 3:50 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.
> > > >
> > > > Was this proposal ok?
> > >
> > > Even SOL_L2CAP goes away. Use SOL_BLUETOOTH for this.
> > >
> > > > >> 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.
> > > > >
> > > > > can I stop you right here (without even looking at the patch). We
> do
> > > > > have the generic SOL_BLUETOOTH that you should be using. So adding
> > > > > SOL_ACL is not a viable option at all.
> > > >
> > > > This would be in a later patch, and SOL_BLUETOOTH, ACL_FLUSH_TIMEOUT
> > > > is fine too, or whatever you prefer.
> > >
> > > Why not just use BT_FLUSHABLE and have it always take a timeout option
> > > and then 0 means not flushable. And advantage of having it separated?
> >
> > I think keeping them separate makes it clear that the flush timeout is
> > global for a given ACL link, whereas the flushable/non-flushable
> > boolean is specific to a L2CAP channel. (Which is why I suggested
> > introducing a new level SOL_ACL for the ACL_FLUSH_TIMEOUT option -
> > since this option applies at the ACL level in the stack).
> >
> > A specific advantage of this is that flushable packets can be enabled
> > without over-writing a previous flush timeout that was set on a
> > different L2CAP socket on the same ACL link. I guess this can also be
> > achieved with getsockopt() but that is racy.
>
> I am talking here about Enhanced Flush support and that would happen on
> a per ACL handle basis. So it actually almost applies on a per L2CAP
> socket level. Only exception is if you establish two or more L2CAP
> connections to the same remote device and set them all to flushable.
> Then of course all of them will be flushed. So strictly speaking it
> might be an ACL link feature, but we don't wanna use it that way. And in
> practice you won't have multiple concurrent flushable L2CAP connections
> to one remote device anyway.
>

I agree that having 2 flush-able L2CAP channels to the same device would
probably not be common. But who knows what new profiles the Bluetooth SIG
will come up with that might also benefit from flush-able ACL data. And if a
use-case comes up, then your proposed API will require programmers to write
a racy getsockopt/setsockopt if they want to turn on flushing on one l2cap
connection without affecting the ACL flush timeout set by another
connection. Building race conditions into an API seems like a sub-optimal
design choice.

But its not worth arguing over. SOL_BLUETOOTH, BT_FLUSHABLE is fine (or
BT_FLUSH_TIMEOUT instead).

I won't be doing development for a few weeks over the holidays, so I can not
update this patch for a while.

Nick

[-- Attachment #2: Type: text/html, Size: 6952 bytes --]

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

[Marcel Holtmann]

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.
>         > > >
>         > > > Was this proposal ok?
>         > >
>         > > Even SOL_L2CAP goes away. Use SOL_BLUETOOTH for this.
>         > >
>         > > > >> 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.
>         > > > >
>         > > > > can I stop you right here (without even looking at the
>         patch). We do
>         > > > > have the generic SOL_BLUETOOTH that you should be
>         using. So adding
>         > > > > SOL_ACL is not a viable option at all.
>         > > >
>         > > > This would be in a later patch, and SOL_BLUETOOTH,
>         ACL_FLUSH_TIMEOUT
>         > > > is fine too, or whatever you prefer.
>         > >
>         > > Why not just use BT_FLUSHABLE and have it always take a
>         timeout option
>         > > and then 0 means not flushable. And advantage of having it
>         separated?
>         >
>         > I think keeping them separate makes it clear that the flush
>         timeout is
>         > global for a given ACL link, whereas the
>         flushable/non-flushable
>         > boolean is specific to a L2CAP channel. (Which is why I
>         suggested
>         > introducing a new level SOL_ACL for the ACL_FLUSH_TIMEOUT
>         option -
>         > since this option applies at the ACL level in the stack).
>         >
>         > A specific advantage of this is that flushable packets can
>         be enabled
>         > without over-writing a previous flush timeout that was set
>         on a
>         > different L2CAP socket on the same ACL link. I guess this
>         can also be
>         > achieved with getsockopt() but that is racy.
>         
>         
>         I am talking here about Enhanced Flush support and that would
>         happen on
>         a per ACL handle basis. So it actually almost applies on a per
>         L2CAP
>         socket level. Only exception is if you establish two or more
>         L2CAP
>         connections to the same remote device and set them all to
>         flushable.
>         Then of course all of them will be flushed. So strictly
>         speaking it
>         might be an ACL link feature, but we don't wanna use it that
>         way. And in
>         practice you won't have multiple concurrent flushable L2CAP
>         connections
>         to one remote device anyway.
> 
> 
> I agree that having 2 flush-able L2CAP channels to the same device
> would probably not be common. But who knows what new profiles the
> Bluetooth SIG will come up with that might also benefit from
> flush-able ACL data. And if a use-case comes up, then your proposed
> API will require programmers to write a racy getsockopt/setsockopt if
> they want to turn on flushing on one l2cap connection without
> affecting the ACL flush timeout set by another connection. Building
> race conditions into an API seems like a sub-optimal design choice.

are you expecting to change this frequently and from different parts of
the code during the lifetime of a socket. I just don't see that
happening at all actually. Either you create a "flushable" socket or you
don't. Fill me in on how you wanna actually use this feature.

> But its not worth arguing over. SOL_BLUETOOTH, BT_FLUSHABLE is fine
> (or BT_FLUSH_TIMEOUT instead).

I would call it BT_FLUSHABLE as of now. Since that is how the
specification calls it. However I do have to refresh my memory with the
actual details. I haven't read that part of the specification in a long
time.

Regards

Marcel

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

[Nick Pelly]

On Fri, Dec 18, 2009 at 4:26 PM, Marcel Holtmann <marcel@holtmann.org> wrot=
e:
> Hi Nick,
>
>> =A0 =A0 =A0 =A0 > > > >> >> >> Right now Bluez always requests flushable=
 ACL
>> =A0 =A0 =A0 =A0 packets (but does not
>> =A0 =A0 =A0 =A0 > > > >> >> >> set a flush timeout, so effectively they =
are
>> =A0 =A0 =A0 =A0 non-flushable):
>> =A0 =A0 =A0 =A0 > > > >> >> >>
>> =A0 =A0 =A0 =A0 > > > >> >> >> However it is desirable to use an ACL flu=
sh
>> =A0 =A0 =A0 =A0 timeout on A2DP packets so
>> =A0 =A0 =A0 =A0 > > > >> >> >> that if the ACL packets block for some re=
ason
>> =A0 =A0 =A0 =A0 then the LM can flush
>> =A0 =A0 =A0 =A0 > > > >> >> >> them to make room for newer packets.
>> =A0 =A0 =A0 =A0 > > > >> >> >>
>> =A0 =A0 =A0 =A0 > > > >> >> >> Is it reasonable for Bluez to use the 0x0=
0 ACL
>> =A0 =A0 =A0 =A0 packet boundary flag by
>> =A0 =A0 =A0 =A0 > > > >> >> >> default (non-flushable packet), and let
>> =A0 =A0 =A0 =A0 userspace request flushable
>> =A0 =A0 =A0 =A0 > > > >> >> >> packets on A2DP L2CAP sockets with the so=
cket
>> =A0 =A0 =A0 =A0 option
>> =A0 =A0 =A0 =A0 > > > >> >> >> L2CAP_LM_RELIABLE.
>> =A0 =A0 =A0 =A0 > > > >> >> >
>> =A0 =A0 =A0 =A0 > > > >> >> > the reliable option has a different meanin=
g. It
>> =A0 =A0 =A0 =A0 comes back from the old
>> =A0 =A0 =A0 =A0 > > > >> >> > Bluetooth 1.1 qualification days where we =
had to
>> =A0 =A0 =A0 =A0 tests on L2CAP that had
>> =A0 =A0 =A0 =A0 > > > >> >> > to confirm that we can detect malformed pa=
ckets
>> =A0 =A0 =A0 =A0 and report them. These
>> =A0 =A0 =A0 =A0 > > > >> >> > days it is just fine to drop them.
>> =A0 =A0 =A0 =A0 > > > >> >>
>> =A0 =A0 =A0 =A0 > > > >> >> Got it, how about introducing
>> =A0 =A0 =A0 =A0 > > > >> >>
>> =A0 =A0 =A0 =A0 > > > >> >> #define L2CAP_LM_FLUSHABLE 0x0040
>> =A0 =A0 =A0 =A0 > > > >> >
>> =A0 =A0 =A0 =A0 > > > >> > that l2cap_sock_setsockopt_old() sets this di=
dn't
>> =A0 =A0 =A0 =A0 give you a hint that
>> =A0 =A0 =A0 =A0 > > > >> > we might wanna deprecate this socket options =
;)
>> =A0 =A0 =A0 =A0 > > > >> >
>> =A0 =A0 =A0 =A0 > > > >> > I need to read up on the flushable stuff, but=
 in
>> =A0 =A0 =A0 =A0 the end it deserves its
>> =A0 =A0 =A0 =A0 > > > >> > own socket option. Also an ioctl() to actuall=
y
>> =A0 =A0 =A0 =A0 trigger Enhanced flush
>> =A0 =A0 =A0 =A0 > > > >> > might be needed.
>> =A0 =A0 =A0 =A0 > > > >> >
>> =A0 =A0 =A0 =A0 > > > >> >> struct l2cap_pinfo {
>> =A0 =A0 =A0 =A0 > > > >> >> =A0 =A0...
>> =A0 =A0 =A0 =A0 > > > >> >> =A0 =A0__u8 flushable;
>> =A0 =A0 =A0 =A0 > > > >> >> }
>> =A0 =A0 =A0 =A0 > > > >> >
>> =A0 =A0 =A0 =A0 > > > >> > Sure. In the long run we need to turn this in=
to a
>> =A0 =A0 =A0 =A0 bitmask. We are just
>> =A0 =A0 =A0 =A0 > > > >> > wasting memory here.
>> =A0 =A0 =A0 =A0 > > > >>
>> =A0 =A0 =A0 =A0 > > > >> Attached is an updated patch, that checks the L=
MP
>> =A0 =A0 =A0 =A0 features bitmask
>> =A0 =A0 =A0 =A0 > > > >> before using the new non-flushable packet type.
>> =A0 =A0 =A0 =A0 > > > >>
>> =A0 =A0 =A0 =A0 > > > >> I am still using L2CAP_LM_FLUSHABLE socket opti=
on in
>> =A0 =A0 =A0 =A0 > > > >> l2cap_sock_setsockopt_old(), which I don't thin=
k you
>> =A0 =A0 =A0 =A0 are happy with.
>> =A0 =A0 =A0 =A0 > > > >> So how about a new option:
>> =A0 =A0 =A0 =A0 > > > >>
>> =A0 =A0 =A0 =A0 > > > >> SOL_L2CAP, L2CAP_ACL_FLUSH
>> =A0 =A0 =A0 =A0 > > > >> which has a default value of 0, and can be set =
to 1
>> =A0 =A0 =A0 =A0 to make the ACL
>> =A0 =A0 =A0 =A0 > > > >> data sent by this L2CAP socket flushable.
>> =A0 =A0 =A0 =A0 > > >
>> =A0 =A0 =A0 =A0 > > > Was this proposal ok?
>> =A0 =A0 =A0 =A0 > >
>> =A0 =A0 =A0 =A0 > > Even SOL_L2CAP goes away. Use SOL_BLUETOOTH for this=
.
>> =A0 =A0 =A0 =A0 > >
>> =A0 =A0 =A0 =A0 > > > >> In a later commit we would then add
>> =A0 =A0 =A0 =A0 > > > >> SOL_ACL, ACL_FLUSH_TIMEOUT
>> =A0 =A0 =A0 =A0 > > > >> That is used to set an automatic flush timeout =
for
>> =A0 =A0 =A0 =A0 the ACL link on a
>> =A0 =A0 =A0 =A0 > > > >> L2CAP socket. Note that SOL_ACL is new.
>> =A0 =A0 =A0 =A0 > > > >
>> =A0 =A0 =A0 =A0 > > > > can I stop you right here (without even looking =
at the
>> =A0 =A0 =A0 =A0 patch). We do
>> =A0 =A0 =A0 =A0 > > > > have the generic SOL_BLUETOOTH that you should b=
e
>> =A0 =A0 =A0 =A0 using. So adding
>> =A0 =A0 =A0 =A0 > > > > SOL_ACL is not a viable option at all.
>> =A0 =A0 =A0 =A0 > > >
>> =A0 =A0 =A0 =A0 > > > This would be in a later patch, and SOL_BLUETOOTH,
>> =A0 =A0 =A0 =A0 ACL_FLUSH_TIMEOUT
>> =A0 =A0 =A0 =A0 > > > is fine too, or whatever you prefer.
>> =A0 =A0 =A0 =A0 > >
>> =A0 =A0 =A0 =A0 > > Why not just use BT_FLUSHABLE and have it always tak=
e a
>> =A0 =A0 =A0 =A0 timeout option
>> =A0 =A0 =A0 =A0 > > and then 0 means not flushable. And advantage of hav=
ing it
>> =A0 =A0 =A0 =A0 separated?
>> =A0 =A0 =A0 =A0 >
>> =A0 =A0 =A0 =A0 > I think keeping them separate makes it clear that the =
flush
>> =A0 =A0 =A0 =A0 timeout is
>> =A0 =A0 =A0 =A0 > global for a given ACL link, whereas the
>> =A0 =A0 =A0 =A0 flushable/non-flushable
>> =A0 =A0 =A0 =A0 > boolean is specific to a L2CAP channel. (Which is why =
I
>> =A0 =A0 =A0 =A0 suggested
>> =A0 =A0 =A0 =A0 > introducing a new level SOL_ACL for the ACL_FLUSH_TIME=
OUT
>> =A0 =A0 =A0 =A0 option -
>> =A0 =A0 =A0 =A0 > since this option applies at the ACL level in the stac=
k).
>> =A0 =A0 =A0 =A0 >
>> =A0 =A0 =A0 =A0 > A specific advantage of this is that flushable packets=
 can
>> =A0 =A0 =A0 =A0 be enabled
>> =A0 =A0 =A0 =A0 > without over-writing a previous flush timeout that was=
 set
>> =A0 =A0 =A0 =A0 on a
>> =A0 =A0 =A0 =A0 > different L2CAP socket on the same ACL link. I guess t=
his
>> =A0 =A0 =A0 =A0 can also be
>> =A0 =A0 =A0 =A0 > achieved with getsockopt() but that is racy.
>>
>>
>> =A0 =A0 =A0 =A0 I am talking here about Enhanced Flush support and that =
would
>> =A0 =A0 =A0 =A0 happen on
>> =A0 =A0 =A0 =A0 a per ACL handle basis. So it actually almost applies on=
 a per
>> =A0 =A0 =A0 =A0 L2CAP
>> =A0 =A0 =A0 =A0 socket level. Only exception is if you establish two or =
more
>> =A0 =A0 =A0 =A0 L2CAP
>> =A0 =A0 =A0 =A0 connections to the same remote device and set them all t=
o
>> =A0 =A0 =A0 =A0 flushable.
>> =A0 =A0 =A0 =A0 Then of course all of them will be flushed. So strictly
>> =A0 =A0 =A0 =A0 speaking it
>> =A0 =A0 =A0 =A0 might be an ACL link feature, but we don't wanna use it =
that
>> =A0 =A0 =A0 =A0 way. And in
>> =A0 =A0 =A0 =A0 practice you won't have multiple concurrent flushable L2=
CAP
>> =A0 =A0 =A0 =A0 connections
>> =A0 =A0 =A0 =A0 to one remote device anyway.
>>
>>
>> I agree that having 2 flush-able L2CAP channels to the same device
>> would probably not be common. But who knows what new profiles the
>> Bluetooth SIG will come up with that might also benefit from
>> flush-able ACL data. And if a use-case comes up, then your proposed
>> API will require programmers to write a racy getsockopt/setsockopt if
>> they want to turn on flushing on one l2cap connection without
>> affecting the ACL flush timeout set by another connection. Building
>> race conditions into an API seems like a sub-optimal design choice.
>
> are you expecting to change this frequently and from different parts of
> the code during the lifetime of a socket. I just don't see that
> happening at all actually. Either you create a "flushable" socket or you
> don't. Fill me in on how you wanna actually use this feature.

My use case is just for A2DP. I turn on flushing with a timeout of say
160ms just before starting streaming of A2DP data, and turn it off
when I finish. This is not a problem with either API proposal.

Where it becomes a problem is if there is a reason to have two
flush-able L2CAP connections to the same host. With your API proposal,
the second connection has no way of turning on flushing without
over-writing the flush timeout set by the first socket. You could
implement another API to read the current flush timeout, and have the
second socket read that API, but thats racy.

If this is not a use-case you care about, then ok. But I just want to
point out that this is a problem that will be baked into the API - and
will require ugly workarounds in userspace as soon as someone requires
2 flushable L2CAP connections to one host. Given the rate at which
Bluetooth changes and new profiles and use cases are added I would not
be so quick to dismiss this use case.

>
>> But its not worth arguing over. SOL_BLUETOOTH, BT_FLUSHABLE is fine
>> (or BT_FLUSH_TIMEOUT instead).
>
> I would call it BT_FLUSHABLE as of now. Since that is how the
> specification calls it. However I do have to refresh my memory with the
> actual details. I haven't read that part of the specification in a long
> time.
>
> Regards
>
> Marcel
>
>
>

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

[Marcel Holtmann]

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.
> >>         > > >
> >>         > > > Was this proposal ok?
> >>         > >
> >>         > > Even SOL_L2CAP goes away. Use SOL_BLUETOOTH for this.
> >>         > >
> >>         > > > >> 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.
> >>         > > > >
> >>         > > > > can I stop you right here (without even looking at the
> >>         patch). We do
> >>         > > > > have the generic SOL_BLUETOOTH that you should be
> >>         using. So adding
> >>         > > > > SOL_ACL is not a viable option at all.
> >>         > > >
> >>         > > > This would be in a later patch, and SOL_BLUETOOTH,
> >>         ACL_FLUSH_TIMEOUT
> >>         > > > is fine too, or whatever you prefer.
> >>         > >
> >>         > > Why not just use BT_FLUSHABLE and have it always take a
> >>         timeout option
> >>         > > and then 0 means not flushable. And advantage of having it
> >>         separated?
> >>         >
> >>         > I think keeping them separate makes it clear that the flush
> >>         timeout is
> >>         > global for a given ACL link, whereas the
> >>         flushable/non-flushable
> >>         > boolean is specific to a L2CAP channel. (Which is why I
> >>         suggested
> >>         > introducing a new level SOL_ACL for the ACL_FLUSH_TIMEOUT
> >>         option -
> >>         > since this option applies at the ACL level in the stack).
> >>         >
> >>         > A specific advantage of this is that flushable packets can
> >>         be enabled
> >>         > without over-writing a previous flush timeout that was set
> >>         on a
> >>         > different L2CAP socket on the same ACL link. I guess this
> >>         can also be
> >>         > achieved with getsockopt() but that is racy.
> >>
> >>
> >>         I am talking here about Enhanced Flush support and that would
> >>         happen on
> >>         a per ACL handle basis. So it actually almost applies on a per
> >>         L2CAP
> >>         socket level. Only exception is if you establish two or more
> >>         L2CAP
> >>         connections to the same remote device and set them all to
> >>         flushable.
> >>         Then of course all of them will be flushed. So strictly
> >>         speaking it
> >>         might be an ACL link feature, but we don't wanna use it that
> >>         way. And in
> >>         practice you won't have multiple concurrent flushable L2CAP
> >>         connections
> >>         to one remote device anyway.
> >>
> >>
> >> I agree that having 2 flush-able L2CAP channels to the same device
> >> would probably not be common. But who knows what new profiles the
> >> Bluetooth SIG will come up with that might also benefit from
> >> flush-able ACL data. And if a use-case comes up, then your proposed
> >> API will require programmers to write a racy getsockopt/setsockopt if
> >> they want to turn on flushing on one l2cap connection without
> >> affecting the ACL flush timeout set by another connection. Building
> >> race conditions into an API seems like a sub-optimal design choice.
> >
> > are you expecting to change this frequently and from different parts of
> > the code during the lifetime of a socket. I just don't see that
> > happening at all actually. Either you create a "flushable" socket or you
> > don't. Fill me in on how you wanna actually use this feature.
> 
> My use case is just for A2DP. I turn on flushing with a timeout of say
> 160ms just before starting streaming of A2DP data, and turn it off
> when I finish. This is not a problem with either API proposal.

I count this as creating socket, setting flushable and then using it.
Then closing it. And especially in A2DP case where the media socket is
brought up and taken down a lot that is a proper usage. However I do
expect that each socket should not change from flushable to
non-flushable in mid term usage. While potentially possible it don't see
its usage at all.

So we could even force the flushable option into non-changeable after
the socket has been connected. Like changing the MTU afterwards makes no
sense.

> Where it becomes a problem is if there is a reason to have two
> flush-able L2CAP connections to the same host. With your API proposal,
> the second connection has no way of turning on flushing without
> over-writing the flush timeout set by the first socket. You could
> implement another API to read the current flush timeout, and have the
> second socket read that API, but thats racy.
> 
> If this is not a use-case you care about, then ok. But I just want to
> point out that this is a problem that will be baked into the API - and
> will require ugly workarounds in userspace as soon as someone requires
> 2 flushable L2CAP connections to one host. Given the rate at which
> Bluetooth changes and new profiles and use cases are added I would not
> be so quick to dismiss this use case.

So my idea would actually be that every socket can has its own flush
timeout, but the core than picks the time to actually do the flushing of
packets. Also we can not have one socket change a socket option of
another one. It is a per socket option and not a global one.

On other possible way would be to use CMSG details to inform sockets
about flushable packets. We have to see how useful that is. Since the
flushable is only useful for the time in between the packet is hold in
the Bluetooth chip buffers and hasn't been transmitted over the air.
Once the packet is on the air, there is nothing to flush anymore. And
with L2CAP ERTM this all becomes obsolete since we can flush at any time
anyway. The retransmission takes care of any accidental flush.

Regards

Marcel

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

[Nick Pelly]

On Fri, Dec 18, 2009 at 6:05 PM, Marcel Holtmann <marcel@holtmann.org> wrot=
e:
> Hi Nick,
>
>> >> =A0 =A0 =A0 =A0 > > > >> >> >> Right now Bluez always requests flusha=
ble ACL
>> >> =A0 =A0 =A0 =A0 packets (but does not
>> >> =A0 =A0 =A0 =A0 > > > >> >> >> set a flush timeout, so effectively th=
ey are
>> >> =A0 =A0 =A0 =A0 non-flushable):
>> >> =A0 =A0 =A0 =A0 > > > >> >> >>
>> >> =A0 =A0 =A0 =A0 > > > >> >> >> However it is desirable to use an ACL =
flush
>> >> =A0 =A0 =A0 =A0 timeout on A2DP packets so
>> >> =A0 =A0 =A0 =A0 > > > >> >> >> that if the ACL packets block for some=
 reason
>> >> =A0 =A0 =A0 =A0 then the LM can flush
>> >> =A0 =A0 =A0 =A0 > > > >> >> >> them to make room for newer packets.
>> >> =A0 =A0 =A0 =A0 > > > >> >> >>
>> >> =A0 =A0 =A0 =A0 > > > >> >> >> Is it reasonable for Bluez to use the =
0x00 ACL
>> >> =A0 =A0 =A0 =A0 packet boundary flag by
>> >> =A0 =A0 =A0 =A0 > > > >> >> >> default (non-flushable packet), and le=
t
>> >> =A0 =A0 =A0 =A0 userspace request flushable
>> >> =A0 =A0 =A0 =A0 > > > >> >> >> packets on A2DP L2CAP sockets with the=
 socket
>> >> =A0 =A0 =A0 =A0 option
>> >> =A0 =A0 =A0 =A0 > > > >> >> >> L2CAP_LM_RELIABLE.
>> >> =A0 =A0 =A0 =A0 > > > >> >> >
>> >> =A0 =A0 =A0 =A0 > > > >> >> > the reliable option has a different mea=
ning. It
>> >> =A0 =A0 =A0 =A0 comes back from the old
>> >> =A0 =A0 =A0 =A0 > > > >> >> > Bluetooth 1.1 qualification days where =
we had to
>> >> =A0 =A0 =A0 =A0 tests on L2CAP that had
>> >> =A0 =A0 =A0 =A0 > > > >> >> > to confirm that we can detect malformed=
 packets
>> >> =A0 =A0 =A0 =A0 and report them. These
>> >> =A0 =A0 =A0 =A0 > > > >> >> > days it is just fine to drop them.
>> >> =A0 =A0 =A0 =A0 > > > >> >>
>> >> =A0 =A0 =A0 =A0 > > > >> >> Got it, how about introducing
>> >> =A0 =A0 =A0 =A0 > > > >> >>
>> >> =A0 =A0 =A0 =A0 > > > >> >> #define L2CAP_LM_FLUSHABLE 0x0040
>> >> =A0 =A0 =A0 =A0 > > > >> >
>> >> =A0 =A0 =A0 =A0 > > > >> > that l2cap_sock_setsockopt_old() sets this=
 didn't
>> >> =A0 =A0 =A0 =A0 give you a hint that
>> >> =A0 =A0 =A0 =A0 > > > >> > we might wanna deprecate this socket optio=
ns ;)
>> >> =A0 =A0 =A0 =A0 > > > >> >
>> >> =A0 =A0 =A0 =A0 > > > >> > I need to read up on the flushable stuff, =
but in
>> >> =A0 =A0 =A0 =A0 the end it deserves its
>> >> =A0 =A0 =A0 =A0 > > > >> > own socket option. Also an ioctl() to actu=
ally
>> >> =A0 =A0 =A0 =A0 trigger Enhanced flush
>> >> =A0 =A0 =A0 =A0 > > > >> > might be needed.
>> >> =A0 =A0 =A0 =A0 > > > >> >
>> >> =A0 =A0 =A0 =A0 > > > >> >> struct l2cap_pinfo {
>> >> =A0 =A0 =A0 =A0 > > > >> >> =A0 =A0...
>> >> =A0 =A0 =A0 =A0 > > > >> >> =A0 =A0__u8 flushable;
>> >> =A0 =A0 =A0 =A0 > > > >> >> }
>> >> =A0 =A0 =A0 =A0 > > > >> >
>> >> =A0 =A0 =A0 =A0 > > > >> > Sure. In the long run we need to turn this=
 into a
>> >> =A0 =A0 =A0 =A0 bitmask. We are just
>> >> =A0 =A0 =A0 =A0 > > > >> > wasting memory here.
>> >> =A0 =A0 =A0 =A0 > > > >>
>> >> =A0 =A0 =A0 =A0 > > > >> Attached is an updated patch, that checks th=
e LMP
>> >> =A0 =A0 =A0 =A0 features bitmask
>> >> =A0 =A0 =A0 =A0 > > > >> before using the new non-flushable packet ty=
pe.
>> >> =A0 =A0 =A0 =A0 > > > >>
>> >> =A0 =A0 =A0 =A0 > > > >> I am still using L2CAP_LM_FLUSHABLE socket o=
ption in
>> >> =A0 =A0 =A0 =A0 > > > >> l2cap_sock_setsockopt_old(), which I don't t=
hink you
>> >> =A0 =A0 =A0 =A0 are happy with.
>> >> =A0 =A0 =A0 =A0 > > > >> So how about a new option:
>> >> =A0 =A0 =A0 =A0 > > > >>
>> >> =A0 =A0 =A0 =A0 > > > >> SOL_L2CAP, L2CAP_ACL_FLUSH
>> >> =A0 =A0 =A0 =A0 > > > >> which has a default value of 0, and can be s=
et to 1
>> >> =A0 =A0 =A0 =A0 to make the ACL
>> >> =A0 =A0 =A0 =A0 > > > >> data sent by this L2CAP socket flushable.
>> >> =A0 =A0 =A0 =A0 > > >
>> >> =A0 =A0 =A0 =A0 > > > Was this proposal ok?
>> >> =A0 =A0 =A0 =A0 > >
>> >> =A0 =A0 =A0 =A0 > > Even SOL_L2CAP goes away. Use SOL_BLUETOOTH for t=
his.
>> >> =A0 =A0 =A0 =A0 > >
>> >> =A0 =A0 =A0 =A0 > > > >> In a later commit we would then add
>> >> =A0 =A0 =A0 =A0 > > > >> SOL_ACL, ACL_FLUSH_TIMEOUT
>> >> =A0 =A0 =A0 =A0 > > > >> That is used to set an automatic flush timeo=
ut for
>> >> =A0 =A0 =A0 =A0 the ACL link on a
>> >> =A0 =A0 =A0 =A0 > > > >> L2CAP socket. Note that SOL_ACL is new.
>> >> =A0 =A0 =A0 =A0 > > > >
>> >> =A0 =A0 =A0 =A0 > > > > can I stop you right here (without even looki=
ng at the
>> >> =A0 =A0 =A0 =A0 patch). We do
>> >> =A0 =A0 =A0 =A0 > > > > have the generic SOL_BLUETOOTH that you shoul=
d be
>> >> =A0 =A0 =A0 =A0 using. So adding
>> >> =A0 =A0 =A0 =A0 > > > > SOL_ACL is not a viable option at all.
>> >> =A0 =A0 =A0 =A0 > > >
>> >> =A0 =A0 =A0 =A0 > > > This would be in a later patch, and SOL_BLUETOO=
TH,
>> >> =A0 =A0 =A0 =A0 ACL_FLUSH_TIMEOUT
>> >> =A0 =A0 =A0 =A0 > > > is fine too, or whatever you prefer.
>> >> =A0 =A0 =A0 =A0 > >
>> >> =A0 =A0 =A0 =A0 > > Why not just use BT_FLUSHABLE and have it always =
take a
>> >> =A0 =A0 =A0 =A0 timeout option
>> >> =A0 =A0 =A0 =A0 > > and then 0 means not flushable. And advantage of =
having it
>> >> =A0 =A0 =A0 =A0 separated?
>> >> =A0 =A0 =A0 =A0 >
>> >> =A0 =A0 =A0 =A0 > I think keeping them separate makes it clear that t=
he flush
>> >> =A0 =A0 =A0 =A0 timeout is
>> >> =A0 =A0 =A0 =A0 > global for a given ACL link, whereas the
>> >> =A0 =A0 =A0 =A0 flushable/non-flushable
>> >> =A0 =A0 =A0 =A0 > boolean is specific to a L2CAP channel. (Which is w=
hy I
>> >> =A0 =A0 =A0 =A0 suggested
>> >> =A0 =A0 =A0 =A0 > introducing a new level SOL_ACL for the ACL_FLUSH_T=
IMEOUT
>> >> =A0 =A0 =A0 =A0 option -
>> >> =A0 =A0 =A0 =A0 > since this option applies at the ACL level in the s=
tack).
>> >> =A0 =A0 =A0 =A0 >
>> >> =A0 =A0 =A0 =A0 > A specific advantage of this is that flushable pack=
ets can
>> >> =A0 =A0 =A0 =A0 be enabled
>> >> =A0 =A0 =A0 =A0 > without over-writing a previous flush timeout that =
was set
>> >> =A0 =A0 =A0 =A0 on a
>> >> =A0 =A0 =A0 =A0 > different L2CAP socket on the same ACL link. I gues=
s this
>> >> =A0 =A0 =A0 =A0 can also be
>> >> =A0 =A0 =A0 =A0 > achieved with getsockopt() but that is racy.
>> >>
>> >>
>> >> =A0 =A0 =A0 =A0 I am talking here about Enhanced Flush support and th=
at would
>> >> =A0 =A0 =A0 =A0 happen on
>> >> =A0 =A0 =A0 =A0 a per ACL handle basis. So it actually almost applies=
 on a per
>> >> =A0 =A0 =A0 =A0 L2CAP
>> >> =A0 =A0 =A0 =A0 socket level. Only exception is if you establish two =
or more
>> >> =A0 =A0 =A0 =A0 L2CAP
>> >> =A0 =A0 =A0 =A0 connections to the same remote device and set them al=
l to
>> >> =A0 =A0 =A0 =A0 flushable.
>> >> =A0 =A0 =A0 =A0 Then of course all of them will be flushed. So strict=
ly
>> >> =A0 =A0 =A0 =A0 speaking it
>> >> =A0 =A0 =A0 =A0 might be an ACL link feature, but we don't wanna use =
it that
>> >> =A0 =A0 =A0 =A0 way. And in
>> >> =A0 =A0 =A0 =A0 practice you won't have multiple concurrent flushable=
 L2CAP
>> >> =A0 =A0 =A0 =A0 connections
>> >> =A0 =A0 =A0 =A0 to one remote device anyway.
>> >>
>> >>
>> >> I agree that having 2 flush-able L2CAP channels to the same device
>> >> would probably not be common. But who knows what new profiles the
>> >> Bluetooth SIG will come up with that might also benefit from
>> >> flush-able ACL data. And if a use-case comes up, then your proposed
>> >> API will require programmers to write a racy getsockopt/setsockopt if
>> >> they want to turn on flushing on one l2cap connection without
>> >> affecting the ACL flush timeout set by another connection. Building
>> >> race conditions into an API seems like a sub-optimal design choice.
>> >
>> > are you expecting to change this frequently and from different parts o=
f
>> > the code during the lifetime of a socket. I just don't see that
>> > happening at all actually. Either you create a "flushable" socket or y=
ou
>> > don't. Fill me in on how you wanna actually use this feature.
>>
>> My use case is just for A2DP. I turn on flushing with a timeout of say
>> 160ms just before starting streaming of A2DP data, and turn it off
>> when I finish. This is not a problem with either API proposal.
>
> I count this as creating socket, setting flushable and then using it.
> Then closing it. And especially in A2DP case where the media socket is
> brought up and taken down a lot that is a proper usage. However I do
> expect that each socket should not change from flushable to
> non-flushable in mid term usage. While potentially possible it don't see
> its usage at all.
>
> So we could even force the flushable option into non-changeable after
> the socket has been connected. Like changing the MTU afterwards makes no
> sense.
>
>> Where it becomes a problem is if there is a reason to have two
>> flush-able L2CAP connections to the same host. With your API proposal,
>> the second connection has no way of turning on flushing without
>> over-writing the flush timeout set by the first socket. You could
>> implement another API to read the current flush timeout, and have the
>> second socket read that API, but thats racy.
>>
>> If this is not a use-case you care about, then ok. But I just want to
>> point out that this is a problem that will be baked into the API - and
>> will require ugly workarounds in userspace as soon as someone requires
>> 2 flushable L2CAP connections to one host. Given the rate at which
>> Bluetooth changes and new profiles and use cases are added I would not
>> be so quick to dismiss this use case.
>
> So my idea would actually be that every socket can has its own flush
> timeout, but the core than picks the time to actually do the flushing of
> packets. Also we can not have one socket change a socket option of
> another one. It is a per socket option and not a global one.

I think you are confused. This patch does not implement HCI Enhance
Flush Command. The flush timeout that I am referring to is passed to
the Bluetooth Chipset with the HCI Write Automatic Flush Timeout
command. Which is why it is global for the ACL link.

>
> On other possible way would be to use CMSG details to inform sockets
> about flushable packets. We have to see how useful that is. Since the
> flushable is only useful for the time in between the packet is hold in
> the Bluetooth chip buffers and hasn't been transmitted over the air.
> Once the packet is on the air, there is nothing to flush anymore. And
> with L2CAP ERTM this all becomes obsolete since we can flush at any time
> anyway. The retransmission takes care of any accidental flush.
>
> Regards
>
> Marcel
>
>
>

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

[Marcel Holtmann]

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.
> >> >>         > > >
> >> >>         > > > Was this proposal ok?
> >> >>         > >
> >> >>         > > Even SOL_L2CAP goes away. Use SOL_BLUETOOTH for this.
> >> >>         > >
> >> >>         > > > >> 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.
> >> >>         > > > >
> >> >>         > > > > can I stop you right here (without even looking at the
> >> >>         patch). We do
> >> >>         > > > > have the generic SOL_BLUETOOTH that you should be
> >> >>         using. So adding
> >> >>         > > > > SOL_ACL is not a viable option at all.
> >> >>         > > >
> >> >>         > > > This would be in a later patch, and SOL_BLUETOOTH,
> >> >>         ACL_FLUSH_TIMEOUT
> >> >>         > > > is fine too, or whatever you prefer.
> >> >>         > >
> >> >>         > > Why not just use BT_FLUSHABLE and have it always take a
> >> >>         timeout option
> >> >>         > > and then 0 means not flushable. And advantage of having it
> >> >>         separated?
> >> >>         >
> >> >>         > I think keeping them separate makes it clear that the flush
> >> >>         timeout is
> >> >>         > global for a given ACL link, whereas the
> >> >>         flushable/non-flushable
> >> >>         > boolean is specific to a L2CAP channel. (Which is why I
> >> >>         suggested
> >> >>         > introducing a new level SOL_ACL for the ACL_FLUSH_TIMEOUT
> >> >>         option -
> >> >>         > since this option applies at the ACL level in the stack).
> >> >>         >
> >> >>         > A specific advantage of this is that flushable packets can
> >> >>         be enabled
> >> >>         > without over-writing a previous flush timeout that was set
> >> >>         on a
> >> >>         > different L2CAP socket on the same ACL link. I guess this
> >> >>         can also be
> >> >>         > achieved with getsockopt() but that is racy.
> >> >>
> >> >>
> >> >>         I am talking here about Enhanced Flush support and that would
> >> >>         happen on
> >> >>         a per ACL handle basis. So it actually almost applies on a per
> >> >>         L2CAP
> >> >>         socket level. Only exception is if you establish two or more
> >> >>         L2CAP
> >> >>         connections to the same remote device and set them all to
> >> >>         flushable.
> >> >>         Then of course all of them will be flushed. So strictly
> >> >>         speaking it
> >> >>         might be an ACL link feature, but we don't wanna use it that
> >> >>         way. And in
> >> >>         practice you won't have multiple concurrent flushable L2CAP
> >> >>         connections
> >> >>         to one remote device anyway.
> >> >>
> >> >>
> >> >> I agree that having 2 flush-able L2CAP channels to the same device
> >> >> would probably not be common. But who knows what new profiles the
> >> >> Bluetooth SIG will come up with that might also benefit from
> >> >> flush-able ACL data. And if a use-case comes up, then your proposed
> >> >> API will require programmers to write a racy getsockopt/setsockopt if
> >> >> they want to turn on flushing on one l2cap connection without
> >> >> affecting the ACL flush timeout set by another connection. Building
> >> >> race conditions into an API seems like a sub-optimal design choice.
> >> >
> >> > are you expecting to change this frequently and from different parts of
> >> > the code during the lifetime of a socket. I just don't see that
> >> > happening at all actually. Either you create a "flushable" socket or you
> >> > don't. Fill me in on how you wanna actually use this feature.
> >>
> >> My use case is just for A2DP. I turn on flushing with a timeout of say
> >> 160ms just before starting streaming of A2DP data, and turn it off
> >> when I finish. This is not a problem with either API proposal.
> >
> > I count this as creating socket, setting flushable and then using it.
> > Then closing it. And especially in A2DP case where the media socket is
> > brought up and taken down a lot that is a proper usage. However I do
> > expect that each socket should not change from flushable to
> > non-flushable in mid term usage. While potentially possible it don't see
> > its usage at all.
> >
> > So we could even force the flushable option into non-changeable after
> > the socket has been connected. Like changing the MTU afterwards makes no
> > sense.
> >
> >> Where it becomes a problem is if there is a reason to have two
> >> flush-able L2CAP connections to the same host. With your API proposal,
> >> the second connection has no way of turning on flushing without
> >> over-writing the flush timeout set by the first socket. You could
> >> implement another API to read the current flush timeout, and have the
> >> second socket read that API, but thats racy.
> >>
> >> If this is not a use-case you care about, then ok. But I just want to
> >> point out that this is a problem that will be baked into the API - and
> >> will require ugly workarounds in userspace as soon as someone requires
> >> 2 flushable L2CAP connections to one host. Given the rate at which
> >> Bluetooth changes and new profiles and use cases are added I would not
> >> be so quick to dismiss this use case.
> >
> > So my idea would actually be that every socket can has its own flush
> > timeout, but the core than picks the time to actually do the flushing of
> > packets. Also we can not have one socket change a socket option of
> > another one. It is a per socket option and not a global one.
> 
> I think you are confused. This patch does not implement HCI Enhance
> Flush Command. The flush timeout that I am referring to is passed to
> the Bluetooth Chipset with the HCI Write Automatic Flush Timeout
> command. Which is why it is global for the ACL link.

I was clearly talking about Enhanced Flush support and not the automatic
flush timeout. The automatic flush timeout should clearly not be a L2CAP
socket option. That would be just wrong. It would be great if they had a
default variant so we can just make this a main.conf option, but that is
out of the question.

So personally I think using the automatic flush timeout would be not a
really good choice for us. Just implemented Enhanced Flush would allow
us to achieve exactly the same with a lot of more control over what is
going on.

Regards

Marcel

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

[Perelet, Oleg]

> Since the flushable is only useful for the time in between the packet is =
hold in
> the Bluetooth chip buffers and hasn't been transmitted over the air.
> Once the packet is on the air, there is nothing to flush anymore. And
> with L2CAP ERTM this all becomes obsolete since we can flush at any time
> anyway. The retransmission takes care of any accidental flush.

Marcel, by the time L2CAP ERTM will get in to real life products whole blob=
 of new surprises/problems will show up:)

What Nick did - improves A2DP  audio quality rather dramatically especially=
 for (low performance) embedded devices working with real life Headsets. Cu=
rrent BlueZ A2DP is good proof of concept,  but to make it usable quite few=
  things are needed. One of them is this RFC.=20

Thanks!
Oleg.
 =20

________________________________________
From: linux-bluetooth-owner@vger.kernel.org [linux-bluetooth-owner@vger.ker=
nel.org] On Behalf Of Marcel Holtmann [marcel@holtmann.org]
Sent: Friday, December 18, 2009 8:05 PM
To: Nick Pelly
Cc: linux-bluetooth@vger.kernel.org
Subject: Re: RFC: Allow Bluez to select flushable or non-flushable ACL pack=
ets  with L2CAP_LM_RELIABLE

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.
> >>         > > >
> >>         > > > Was this proposal ok?
> >>         > >
> >>         > > Even SOL_L2CAP goes away. Use SOL_BLUETOOTH for this.
> >>         > >
> >>         > > > >> 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.
> >>         > > > >
> >>         > > > > can I stop you right here (without even looking at the
> >>         patch). We do
> >>         > > > > have the generic SOL_BLUETOOTH that you should be
> >>         using. So adding
> >>         > > > > SOL_ACL is not a viable option at all.
> >>         > > >
> >>         > > > This would be in a later patch, and SOL_BLUETOOTH,
> >>         ACL_FLUSH_TIMEOUT
> >>         > > > is fine too, or whatever you prefer.
> >>         > >
> >>         > > Why not just use BT_FLUSHABLE and have it always take a
> >>         timeout option
> >>         > > and then 0 means not flushable. And advantage of having it
> >>         separated?
> >>         >
> >>         > I think keeping them separate makes it clear that the flush
> >>         timeout is
> >>         > global for a given ACL link, whereas the
> >>         flushable/non-flushable
> >>         > boolean is specific to a L2CAP channel. (Which is why I
> >>         suggested
> >>         > introducing a new level SOL_ACL for the ACL_FLUSH_TIMEOUT
> >>         option -
> >>         > since this option applies at the ACL level in the stack).
> >>         >
> >>         > A specific advantage of this is that flushable packets can
> >>         be enabled
> >>         > without over-writing a previous flush timeout that was set
> >>         on a
> >>         > different L2CAP socket on the same ACL link. I guess this
> >>         can also be
> >>         > achieved with getsockopt() but that is racy.
> >>
> >>
> >>         I am talking here about Enhanced Flush support and that would
> >>         happen on
> >>         a per ACL handle basis. So it actually almost applies on a per
> >>         L2CAP
> >>         socket level. Only exception is if you establish two or more
> >>         L2CAP
> >>         connections to the same remote device and set them all to
> >>         flushable.
> >>         Then of course all of them will be flushed. So strictly
> >>         speaking it
> >>         might be an ACL link feature, but we don't wanna use it that
> >>         way. And in
> >>         practice you won't have multiple concurrent flushable L2CAP
> >>         connections
> >>         to one remote device anyway.
> >>
> >>
> >> I agree that having 2 flush-able L2CAP channels to the same device
> >> would probably not be common. But who knows what new profiles the
> >> Bluetooth SIG will come up with that might also benefit from
> >> flush-able ACL data. And if a use-case comes up, then your proposed
> >> API will require programmers to write a racy getsockopt/setsockopt if
> >> they want to turn on flushing on one l2cap connection without
> >> affecting the ACL flush timeout set by another connection. Building
> >> race conditions into an API seems like a sub-optimal design choice.
> >
> > are you expecting to change this frequently and from different parts of
> > the code during the lifetime of a socket. I just don't see that
> > happening at all actually. Either you create a "flushable" socket or yo=
u
> > don't. Fill me in on how you wanna actually use this feature.
>
> My use case is just for A2DP. I turn on flushing with a timeout of say
> 160ms just before starting streaming of A2DP data, and turn it off
> when I finish. This is not a problem with either API proposal.

I count this as creating socket, setting flushable and then using it.
Then closing it. And especially in A2DP case where the media socket is
brought up and taken down a lot that is a proper usage. However I do
expect that each socket should not change from flushable to
non-flushable in mid term usage. While potentially possible it don't see
its usage at all.

So we could even force the flushable option into non-changeable after
the socket has been connected. Like changing the MTU afterwards makes no
sense.

> Where it becomes a problem is if there is a reason to have two
> flush-able L2CAP connections to the same host. With your API proposal,
> the second connection has no way of turning on flushing without
> over-writing the flush timeout set by the first socket. You could
> implement another API to read the current flush timeout, and have the
> second socket read that API, but thats racy.
>
> If this is not a use-case you care about, then ok. But I just want to
> point out that this is a problem that will be baked into the API - and
> will require ugly workarounds in userspace as soon as someone requires
> 2 flushable L2CAP connections to one host. Given the rate at which
> Bluetooth changes and new profiles and use cases are added I would not
> be so quick to dismiss this use case.

So my idea would actually be that every socket can has its own flush
timeout, but the core than picks the time to actually do the flushing of
packets. Also we can not have one socket change a socket option of
another one. It is a per socket option and not a global one.

On other possible way would be to use CMSG details to inform sockets
about flushable packets. We have to see how useful that is. Since the
flushable is only useful for the time in between the packet is hold in
the Bluetooth chip buffers and hasn't been transmitted over the air.
Once the packet is on the air, there is nothing to flush anymore. And
with L2CAP ERTM this all becomes obsolete since we can flush at any time
anyway. The retransmission takes care of any accidental flush.

Regards

Marcel


--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" =
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Johan Hedberg]

Hi Marcel,

On Fri, Dec 18, 2009, Marcel Holtmann wrote:
> > > are you expecting to change this frequently and from different parts of
> > > the code during the lifetime of a socket. I just don't see that
> > > happening at all actually. Either you create a "flushable" socket or you
> > > don't. Fill me in on how you wanna actually use this feature.
> > 
> > My use case is just for A2DP. I turn on flushing with a timeout of say
> > 160ms just before starting streaming of A2DP data, and turn it off
> > when I finish. This is not a problem with either API proposal.
> 
> I count this as creating socket, setting flushable and then using it.
> Then closing it. And especially in A2DP case where the media socket is
> brought up and taken down a lot that is a proper usage. However I do
> expect that each socket should not change from flushable to
> non-flushable in mid term usage. While potentially possible it don't see
> its usage at all.

I hope you remember that the A2DP media transport socket is kept open
much longer periods than e.g. the SCO socket in HFP/HSP since suspending
an A2DP stream only means stopping sending data but still keeping the
media socket open. Only when the configured codec parameters aren't
suitable for the audio data would you close, reconfigure and recreate
the media socket. So is it ok to keep the flushable setting even while
the A2DP stream is suspended?

Johan

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

[Nick Pelly]

Hi Marcel,

On Fri, Dec 18, 2009 at 3:50 PM, Marcel Holtmann <marcel@holtmann.org> wrot=
e:
>
> 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 A2D=
P 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 boun=
dary 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 L=
2CAP 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 h=
int that
> > > > >> > we might wanna deprecate this socket options ;)
> > > > >> >
> > > > >> > I need to read up on the flushable stuff, but in the end it de=
serves its
> > > > >> > own socket option. Also an ioctl() to actually trigger Enhance=
d flush
> > > > >> > might be needed.
> > > > >> >
> > > > >> >> struct l2cap_pinfo {
> > > > >> >> =A0 =A0...
> > > > >> >> =A0 =A0__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 bitma=
sk
> > > > >> 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 w=
ith.
> > > > >> 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.
> > > >
> > > > Was this proposal ok?
> > >
> > > Even SOL_L2CAP goes away. Use SOL_BLUETOOTH for this.
> > >
> > > > >> 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.
> > > > >
> > > > > can I stop you right here (without even looking at the patch). We=
 do
> > > > > have the generic SOL_BLUETOOTH that you should be using. So addin=
g
> > > > > SOL_ACL is not a viable option at all.
> > > >
> > > > This would be in a later patch, and SOL_BLUETOOTH, ACL_FLUSH_TIMEOU=
T
> > > > is fine too, or whatever you prefer.
> > >
> > > Why not just use BT_FLUSHABLE and have it always take a timeout optio=
n
> > > and then 0 means not flushable. And advantage of having it separated?
> >
> > I think keeping them separate makes it clear that the flush timeout is
> > global for a given ACL link, whereas the flushable/non-flushable
> > boolean is specific to a L2CAP channel. (Which is why I suggested
> > introducing a new level SOL_ACL for the ACL_FLUSH_TIMEOUT option -
> > since this option applies at the ACL level in the stack).
> >
> > A specific advantage of this is that flushable packets can be enabled
> > without over-writing a previous flush timeout that was set on a
> > different L2CAP socket on the same ACL link. I guess this can also be
> > achieved with getsockopt() but that is racy.
>
> I am talking here about Enhanced Flush support and that would happen on
> a per ACL handle basis. So it actually almost applies on a per L2CAP
> socket level. Only exception is if you establish two or more L2CAP
> connections to the same remote device and set them all to flushable.
> Then of course all of them will be flushed. So strictly speaking it
> might be an ACL link feature, but we don't wanna use it that way. And in
> practice you won't have multiple concurrent flushable L2CAP connections
> to one remote device anyway.
>

I agree that having 2=A0flush-able=A0L2CAP channels to the same device
would probably not be common. But who knows what new profiles the
Bluetooth SIG will come up with that might also benefit from
flush-able=A0ACL data. And when this happens, with your proposed API,
programmers will not be able to avoid a race condition between
getsockopt() then setsockopt() if they want to turn on flushing
without changing the ACL flush timeout.
But its not worth arguing over.=A0SOL_BLUETOOTH, BT_FLUSHABLE is fine
(or BT_FLUSH_TIMEOUT instead).

I won't be doing development for a few weeks over the holidays, so I
can not update this patch for a while.

Nick

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

[Nick Pelly]

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

On Wed, Dec 16, 2009 at 1:59 PM, Nick Pelly <npelly@google.com> wrote:
> 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.

Attached an updated patch for 2.6.32 kernel. We've been using this
patch successfully on production devices.

Nick

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

From 80ea1ef6d82e5bcbde85c23c8585ccac820f5515 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            |   25 ++++++++++++++++++++++---
 5 files changed, 33 insertions(+), 5 deletions(-)

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index 9716e5a..bfd23ae 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -145,11 +145,14 @@ enum {
 			EDR_ESCO_MASK)
 
 /* 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
@@ -188,6 +191,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 cbcc5b1..f5da4e6 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -479,6 +479,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 9516f4b..23f37b4 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -70,6 +70,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
@@ -316,6 +317,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 e1da8f6..84a9d75 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1239,7 +1239,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 */
@@ -1256,12 +1256,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 6e3b596..4529e99 100644
--- a/net/bluetooth/l2cap.c
+++ b/net/bluetooth/l2cap.c
@@ -325,13 +325,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->hcon->hdev))
+		flags = ACL_START_NO_FLUSH;
+	else
+		flags = ACL_START;
+
+	return hci_send_acl(conn->hcon, skb, flags);
 }
 
 static inline int l2cap_send_sframe(struct l2cap_pinfo *pi, u16 control)
@@ -770,6 +776,7 @@ 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;
@@ -778,6 +785,7 @@ static void l2cap_sock_init(struct sock *sk, struct sock *parent)
 		pi->sec_level = BT_SECURITY_LOW;
 		pi->role_switch = 0;
 		pi->force_reliable = 0;
+		pi->flushable = 0;
 	}
 
 	/* Default config options */
@@ -1258,11 +1266,18 @@ static void l2cap_drop_acked_frames(struct sock *sk)
 static inline int l2cap_do_send(struct sock *sk, struct sk_buff *skb)
 {
 	struct l2cap_pinfo *pi = l2cap_pi(sk);
+	struct hci_conn *hcon = pi->conn->hcon;
 	int err;
+	u16 flags;
 
 	BT_DBG("sk %p, skb %p len %d", sk, skb, skb->len);
 
-	err = hci_send_acl(pi->conn->hcon, skb, 0);
+	if (lmp_no_flush_capable(hcon->hdev) && !l2cap_pi(sk)->flushable)
+		flags = ACL_START_NO_FLUSH;
+	else
+		flags = ACL_START;
+
+	err = hci_send_acl(hcon, skb, flags);
 	if (err < 0)
 		kfree_skb(skb);
 
@@ -1747,6 +1762,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:
@@ -1874,6 +1890,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;
@@ -3801,7 +3820,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

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

[Marcel Holtmann]

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.
> 
> Attached an updated patch for 2.6.32 kernel. We've been using this
> patch successfully on production devices.

can see anything wrong with that patch. However we need to use
SOL_BLUETOOTH for it of course. So we need to come up with something to
make this simple.

An additional change I like to see is to use flags for booleans like
flushable in the structures. Can you work on changing that.

Also do we have decoding support for this in hcidump. It might be nice
to include some really simple examples in the commit message.

Regards

Marcel

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

[Luiz Augusto von Dentz]

Hi,

On Tue, Mar 9, 2010 at 11:45 PM, Marcel Holtmann <marcel@holtmann.org> wrot=
e:
> Hi Nick,
>
>> >>> >> Right now Bluez always requests flushable ACL packets (but does n=
ot
>> >>> >> set a flush timeout, so effectively they are non-flushable):
>> >>> >>
>> >>> >> However it is desirable to use an ACL flush timeout on A2DP packe=
ts so
>> >>> >> that if the ACL packets block for some reason then the LM can flu=
sh
>> >>> >> them to make room for newer packets.
>> >>> >>
>> >>> >> Is it reasonable for Bluez to use the 0x00 ACL packet boundary fl=
ag by
>> >>> >> default (non-flushable packet), and let userspace request flushab=
le
>> >>> >> packets on A2DP L2CAP sockets with the socket option
>> >>> >> L2CAP_LM_RELIABLE.
>> >>> >
>> >>> > the reliable option has a different meaning. It comes back from th=
e old
>> >>> > Bluetooth 1.1 qualification days where we had to tests on L2CAP th=
at had
>> >>> > to confirm that we can detect malformed packets and report them. T=
hese
>> >>> > 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 tha=
t
>> >> 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 {
>> >>> =A0 =A0...
>> >>> =A0 =A0__u8 flushable;
>> >>> }
>> >>
>> >> Sure. In the long run we need to turn this into a bitmask. We are jus=
t
>> >> 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.
>>
>> Attached an updated patch for 2.6.32 kernel. We've been using this
>> patch successfully on production devices.
>
> can see anything wrong with that patch. However we need to use
> SOL_BLUETOOTH for it of course. So we need to come up with something to
> make this simple.
>
> An additional change I like to see is to use flags for booleans like
> flushable in the structures. Can you work on changing that.
>
> Also do we have decoding support for this in hcidump. It might be nice
> to include some really simple examples in the commit message.
>
> Regards

I would like to play a little bit with this, so is there any missing update=
s?

--=20
Luiz Augusto von Dentz
Computer Engineer

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

[Suraj]

Hi Luis,

On 6/16/2010 5:10 PM, Luiz Augusto von Dentz wrote:
> Hi,
>
> On Tue, Mar 9, 2010 at 11:45 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.
>>>
>>> Attached an updated patch for 2.6.32 kernel. We've been using this
>>> patch successfully on production devices.
>>
>> can see anything wrong with that patch. However we need to use
>> SOL_BLUETOOTH for it of course. So we need to come up with something to
>> make this simple.
>>
>> An additional change I like to see is to use flags for booleans like
>> flushable in the structures. Can you work on changing that.
>>
>> Also do we have decoding support for this in hcidump. It might be nice
>> to include some really simple examples in the commit message.
>>
>> Regards
>
> I would like to play a little bit with this, so is there any missing updates?
>
This is not exactly something related to your question, but there is 
another side effect for the current implementation.

Assume you have 2 ACL links, FTP and A2DP. A2DP streaming and FTP doing 
FTP Put.
When the A2DP packets start blocking, it effectively start blocking the 
packets available for FTP too. But, the host has no idea about it and 
keep pumping in A2DP data until all available buffers are blocked. 
Effectively blocking both A2DP and FTP.

So at the user level you will see your FTP connection stalling as long 
your A2DP connection is stalled (out of range). FTP will restart as soon 
as A2DP comes back in range.

I had raised this issue sometime before, but could not follow it up.

Regards
Suraj

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

[Luiz Augusto von Dentz]

Hi,

On Wed, Jun 16, 2010 at 3:04 PM, Suraj <suraj@atheros.com> wrote:
> Hi Luis,
>
> On 6/16/2010 5:10 PM, Luiz Augusto von Dentz wrote:
>>
>> Hi,
>>
>> On Tue, Mar 9, 2010 at 11:45 PM, Marcel Holtmann<marcel@holtmann.org>
>> =A0wrote:
>>>
>>> Hi Nick,
>>>
>>>>>>>>> Right now Bluez always requests flushable ACL packets (but does n=
ot
>>>>>>>>> set a flush timeout, so effectively they are non-flushable):
>>>>>>>>>
>>>>>>>>> However it is desirable to use an ACL flush timeout on A2DP packe=
ts
>>>>>>>>> so
>>>>>>>>> that if the ACL packets block for some reason then the LM can flu=
sh
>>>>>>>>> them to make room for newer packets.
>>>>>>>>>
>>>>>>>>> Is it reasonable for Bluez to use the 0x00 ACL packet boundary fl=
ag
>>>>>>>>> by
>>>>>>>>> default (non-flushable packet), and let userspace request flushab=
le
>>>>>>>>> packets on A2DP L2CAP sockets with the socket option
>>>>>>>>> L2CAP_LM_RELIABLE.
>>>>>>>>
>>>>>>>> the reliable option has a different meaning. It comes back from th=
e
>>>>>>>> old
>>>>>>>> Bluetooth 1.1 qualification days where we had to tests on L2CAP th=
at
>>>>>>>> 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 th=
at
>>>>>> 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 flus=
h
>>>>>> might be needed.
>>>>>>
>>>>>>> struct l2cap_pinfo {
>>>>>>> =A0 =A0...
>>>>>>> =A0 =A0__u8 flushable;
>>>>>>> }
>>>>>>
>>>>>> Sure. In the long run we need to turn this into a bitmask. We are ju=
st
>>>>>> 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.
>>>>
>>>> Attached an updated patch for 2.6.32 kernel. We've been using this
>>>> patch successfully on production devices.
>>>
>>> can see anything wrong with that patch. However we need to use
>>> SOL_BLUETOOTH for it of course. So we need to come up with something to
>>> make this simple.
>>>
>>> An additional change I like to see is to use flags for booleans like
>>> flushable in the structures. Can you work on changing that.
>>>
>>> Also do we have decoding support for this in hcidump. It might be nice
>>> to include some really simple examples in the commit message.
>>>
>>> Regards
>>
>> I would like to play a little bit with this, so is there any missing
>> updates?
>>
> This is not exactly something related to your question, but there is anot=
her
> side effect for the current implementation.
>
> Assume you have 2 ACL links, FTP and A2DP. A2DP streaming and FTP doing F=
TP
> Put.
> When the A2DP packets start blocking, it effectively start blocking the
> packets available for FTP too. But, the host has no idea about it and kee=
p
> pumping in A2DP data until all available buffers are blocked. Effectively
> blocking both A2DP and FTP.
>
> So at the user level you will see your FTP connection stalling as long yo=
ur
> A2DP connection is stalled (out of range). FTP will restart as soon as A2=
DP
> comes back in range.
>
> I had raised this issue sometime before, but could not follow it up.

I got the impression that we can still control which packets are
Automatically-Flushable and which are not, so even thought we set the
timeout in a per ACL link fashion we can still mark which packets
should be flushable in a per socket basis.

Is that correct, Nick?


--=20
Luiz Augusto von Dentz
Computer Engineer

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

[Suraj]

Hi,
On 6/16/2010 8:44 PM, Luiz Augusto von Dentz wrote:
> Hi,
>
> On Wed, Jun 16, 2010 at 3:04 PM, Suraj<suraj@atheros.com>  wrote:
>> Hi Luis,
>>
>> On 6/16/2010 5:10 PM, Luiz Augusto von Dentz wrote:
>>>
>>> Hi,
>>>
>>> On Tue, Mar 9, 2010 at 11:45 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.
>>>>>
>>>>> Attached an updated patch for 2.6.32 kernel. We've been using this
>>>>> patch successfully on production devices.
>>>>
>>>> can see anything wrong with that patch. However we need to use
>>>> SOL_BLUETOOTH for it of course. So we need to come up with something to
>>>> make this simple.
>>>>
>>>> An additional change I like to see is to use flags for booleans like
>>>> flushable in the structures. Can you work on changing that.
>>>>
>>>> Also do we have decoding support for this in hcidump. It might be nice
>>>> to include some really simple examples in the commit message.
>>>>
>>>> Regards
>>>
>>> I would like to play a little bit with this, so is there any missing
>>> updates?
>>>
>> This is not exactly something related to your question, but there is another
>> side effect for the current implementation.
>>
>> Assume you have 2 ACL links, FTP and A2DP. A2DP streaming and FTP doing FTP
>> Put.
>> When the A2DP packets start blocking, it effectively start blocking the
>> packets available for FTP too. But, the host has no idea about it and keep
>> pumping in A2DP data until all available buffers are blocked. Effectively
>> blocking both A2DP and FTP.
>>
>> So at the user level you will see your FTP connection stalling as long your
>> A2DP connection is stalled (out of range). FTP will restart as soon as A2DP
>> comes back in range.
>>
>> I had raised this issue sometime before, but could not follow it up.
>
> I got the impression that we can still control which packets are
> Automatically-Flushable and which are not, so even thought we set the
> timeout in a per ACL link fashion we can still mark which packets
> should be flushable in a per socket basis.
>
> Is that correct, Nick?
>

Yes, it is possible to flush packets selectively. But the buffer 
management is between Host and Controller, not per ACL link.

The root cause of the issue is that A2DP packets are not at all flushed 
and keeps clogging the available buffers.

So, Implementing option to actually flush A2DP packet should solve the 
above mentioned problem.

Regards
Suraj

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

[Nick Pelly]

On Wed, Jun 16, 2010 at 8:14 AM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi,
>
> On Wed, Jun 16, 2010 at 3:04 PM, Suraj <suraj@atheros.com> wrote:
>> Hi Luis,
>>
>> On 6/16/2010 5:10 PM, Luiz Augusto von Dentz wrote:
>>>
>>> Hi,
>>>
>>> On Tue, Mar 9, 2010 at 11:45 PM, Marcel Holtmann<marcel@holtmann.org>
>>> =A0wrote:
>>>>
>>>> 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 pack=
ets
>>>>>>>>>> so
>>>>>>>>>> that if the ACL packets block for some reason then the LM can fl=
ush
>>>>>>>>>> them to make room for newer packets.
>>>>>>>>>>
>>>>>>>>>> Is it reasonable for Bluez to use the 0x00 ACL packet boundary f=
lag
>>>>>>>>>> by
>>>>>>>>>> default (non-flushable packet), and let userspace request flusha=
ble
>>>>>>>>>> packets on A2DP L2CAP sockets with the socket option
>>>>>>>>>> L2CAP_LM_RELIABLE.
>>>>>>>>>
>>>>>>>>> the reliable option has a different meaning. It comes back from t=
he
>>>>>>>>> old
>>>>>>>>> Bluetooth 1.1 qualification days where we had to tests on L2CAP t=
hat
>>>>>>>>> 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 t=
hat
>>>>>>> we might wanna deprecate this socket options ;)
>>>>>>>
>>>>>>> I need to read up on the flushable stuff, but in the end it deserve=
s
>>>>>>> its
>>>>>>> own socket option. Also an ioctl() to actually trigger Enhanced flu=
sh
>>>>>>> might be needed.
>>>>>>>
>>>>>>>> struct l2cap_pinfo {
>>>>>>>> =A0 =A0...
>>>>>>>> =A0 =A0__u8 flushable;
>>>>>>>> }
>>>>>>>
>>>>>>> Sure. In the long run we need to turn this into a bitmask. We are j=
ust
>>>>>>> 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.
>>>>>
>>>>> Attached an updated patch for 2.6.32 kernel. We've been using this
>>>>> patch successfully on production devices.
>>>>
>>>> can see anything wrong with that patch. However we need to use
>>>> SOL_BLUETOOTH for it of course. So we need to come up with something t=
o
>>>> make this simple.
>>>>
>>>> An additional change I like to see is to use flags for booleans like
>>>> flushable in the structures. Can you work on changing that.
>>>>
>>>> Also do we have decoding support for this in hcidump. It might be nice
>>>> to include some really simple examples in the commit message.
>>>>
>>>> Regards
>>>
>>> I would like to play a little bit with this, so is there any missing
>>> updates?
>>>
>> This is not exactly something related to your question, but there is ano=
ther
>> side effect for the current implementation.
>>
>> Assume you have 2 ACL links, FTP and A2DP. A2DP streaming and FTP doing =
FTP
>> Put.
>> When the A2DP packets start blocking, it effectively start blocking the
>> packets available for FTP too. But, the host has no idea about it and ke=
ep
>> pumping in A2DP data until all available buffers are blocked. Effectivel=
y
>> blocking both A2DP and FTP.
>>
>> So at the user level you will see your FTP connection stalling as long y=
our
>> A2DP connection is stalled (out of range). FTP will restart as soon as A=
2DP
>> comes back in range.
>>
>> I had raised this issue sometime before, but could not follow it up.
>
> I got the impression that we can still control which packets are
> Automatically-Flushable and which are not, so even thought we set the
> timeout in a per ACL link fashion we can still mark which packets
> should be flushable in a per socket basis.
>
> Is that correct, Nick?

Yes, as Suraj explains, my patch will solve the problem of a stalled
A2DP link preventing FTP packets on another ACL - because the A2DP
packets will be marked flushable.

However its worth bringing up that my patch does not solve the reverse
problem - if an FTP link is stalled then all ACL packets will be
temporarily stalled - because you would not normally mark FTP packets
as flushable. For this you would need the kernel to set a high level
watermark on a per-ACL-link or per-socket basis, so that a single ACL
link or a single socket can not use all available ACL buffers
regardless of whether they are flushable or not.

Nick

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

[Suraj]

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

Hi,
On 6/16/2010 9:56 PM, Nick Pelly wrote:
> On Wed, Jun 16, 2010 at 8:14 AM, Luiz Augusto von Dentz
> <luiz.dentz@gmail.com>  wrote:
>> Hi,
>>
>> On Wed, Jun 16, 2010 at 3:04 PM, Suraj<suraj@atheros.com>  wrote:
>>> Hi Luis,
>>>
>>> On 6/16/2010 5:10 PM, Luiz Augusto von Dentz wrote:
>>>>
>>>> Hi,
>>>>
>>>> On Tue, Mar 9, 2010 at 11:45 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.
>>>>>>
>>>>>> Attached an updated patch for 2.6.32 kernel. We've been using this
>>>>>> patch successfully on production devices.
>>>>>
>>>>> can see anything wrong with that patch. However we need to use
>>>>> SOL_BLUETOOTH for it of course. So we need to come up with something to
>>>>> make this simple.
>>>>>
>>>>> An additional change I like to see is to use flags for booleans like
>>>>> flushable in the structures. Can you work on changing that.
>>>>>
>>>>> Also do we have decoding support for this in hcidump. It might be nice
>>>>> to include some really simple examples in the commit message.
>>>>>
>>>>> Regards
>>>>
>>>> I would like to play a little bit with this, so is there any missing
>>>> updates?
>>>>
>>> This is not exactly something related to your question, but there is another
>>> side effect for the current implementation.
>>>
>>> Assume you have 2 ACL links, FTP and A2DP. A2DP streaming and FTP doing FTP
>>> Put.
>>> When the A2DP packets start blocking, it effectively start blocking the
>>> packets available for FTP too. But, the host has no idea about it and keep
>>> pumping in A2DP data until all available buffers are blocked. Effectively
>>> blocking both A2DP and FTP.
>>>
>>> So at the user level you will see your FTP connection stalling as long your
>>> A2DP connection is stalled (out of range). FTP will restart as soon as A2DP
>>> comes back in range.
>>>
>>> I had raised this issue sometime before, but could not follow it up.
>>
>> I got the impression that we can still control which packets are
>> Automatically-Flushable and which are not, so even thought we set the
>> timeout in a per ACL link fashion we can still mark which packets
>> should be flushable in a per socket basis.
>>
>> Is that correct, Nick?
>
> Yes, as Suraj explains, my patch will solve the problem of a stalled
> A2DP link preventing FTP packets on another ACL - because the A2DP
> packets will be marked flushable.
>
> However its worth bringing up that my patch does not solve the reverse
> problem - if an FTP link is stalled then all ACL packets will be
> temporarily stalled - because you would not normally mark FTP packets
> as flushable. For this you would need the kernel to set a high level
> watermark on a per-ACL-link or per-socket basis, so that a single ACL
> link or a single socket can not use all available ACL buffers
> regardless of whether they are flushable or not.
>
> Nick

Exactly, this was the problem I raised sometime before.
Attached is a patch that works for me.
Not sure if this is the best solution. I can send a formal patch if this 
is worth a try.

Regards
Suraj


[-- Attachment #2: 0001-Fix-for-Link-starvation-if-device-goes-out-of-range.patch --]
[-- Type: text/plain, Size: 2214 bytes --]

>From cbcb5fd9d126e490a7b12469a0a1e1fb09ce535b Mon Sep 17 00:00:00 2001
From: root <root@localhost.localdomain>
Date: Tue, 4 May 2010 13:52:04 +0530
Subject: [PATCH] Fix for Link starvation if device goes out of range

---
 include/net/bluetooth/hci_core.h |    2 ++
 net/bluetooth/hci_core.c         |    5 +++++
 net/bluetooth/hci_event.c        |    4 ++++
 3 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 25b8a03..4216a7b 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -178,6 +178,8 @@ struct hci_conn {
 
 	unsigned int	 sent;
 
+	unsigned int	 avail_buffer;
+
 	struct sk_buff_head data_q;
 
 	struct timer_list disc_timer;
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 406ad07..f8a99cf 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1325,6 +1325,8 @@ static inline struct hci_conn *hci_low_sent(struct hci_dev *hdev, __u8 type, int
 
 		if (c->state != BT_CONNECTED && c->state != BT_CONFIG)
 			continue;
+		if (c->avail_buffer == 0)
+			continue;
 
 		num++;
 
@@ -1390,6 +1392,9 @@ static inline void hci_sched_acl(struct hci_dev *hdev)
 
 			hdev->acl_cnt--;
 			conn->sent++;
+
+			if (conn->avail_buffer != 0)
+				conn->avail_buffer--;
 		}
 	}
 }
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index e99fe38..2f3b6dc 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -900,6 +900,9 @@ static inline void hci_conn_complete_evt(struct hci_dev *hdev, struct sk_buff *s
 		if (conn->type == ACL_LINK) {
 			struct hci_cp_read_remote_features cp;
 			cp.handle = ev->handle;
+
+			conn->avail_buffer = hdev->acl_pkts;
+
 			hci_send_cmd(hdev, HCI_OP_READ_REMOTE_FEATURES,
 							sizeof(cp), &cp);
 		}
@@ -1447,6 +1450,7 @@ static inline void hci_num_comp_pkts_evt(struct hci_dev *hdev, struct sk_buff *s
 			if (conn->type == ACL_LINK) {
 				if ((hdev->acl_cnt += count) > hdev->acl_pkts)
 					hdev->acl_cnt = hdev->acl_pkts;
+				conn->avail_buffer = count;
 			} else {
 				if ((hdev->sco_cnt += count) > hdev->sco_pkts)
 					hdev->sco_cnt = hdev->sco_pkts;
-- 
1.6.0.6

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

[Nick Pelly]

On Wed, Jun 16, 2010 at 4:40 AM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi,
>
> On Tue, Mar 9, 2010 at 11:45 PM, Marcel Holtmann <marcel@holtmann.org> wr=
ote:
>> 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 pack=
ets so
>>> >>> >> that if the ACL packets block for some reason then the LM can fl=
ush
>>> >>> >> them to make room for newer packets.
>>> >>> >>
>>> >>> >> Is it reasonable for Bluez to use the 0x00 ACL packet boundary f=
lag by
>>> >>> >> default (non-flushable packet), and let userspace request flusha=
ble
>>> >>> >> packets on A2DP L2CAP sockets with the socket option
>>> >>> >> L2CAP_LM_RELIABLE.
>>> >>> >
>>> >>> > the reliable option has a different meaning. It comes back from t=
he old
>>> >>> > Bluetooth 1.1 qualification days where we had to tests on L2CAP t=
hat 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 th=
at
>>> >> 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 flus=
h
>>> >> might be needed.
>>> >>
>>> >>> struct l2cap_pinfo {
>>> >>> =A0 =A0...
>>> >>> =A0 =A0__u8 flushable;
>>> >>> }
>>> >>
>>> >> Sure. In the long run we need to turn this into a bitmask. We are ju=
st
>>> >> 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.
>>>
>>> Attached an updated patch for 2.6.32 kernel. We've been using this
>>> patch successfully on production devices.
>>
>> can see anything wrong with that patch. However we need to use
>> SOL_BLUETOOTH for it of course. So we need to come up with something to
>> make this simple.
>>
>> An additional change I like to see is to use flags for booleans like
>> flushable in the structures. Can you work on changing that.
>>
>> Also do we have decoding support for this in hcidump. It might be nice
>> to include some really simple examples in the commit message.
>>
>> Regards
>
> I would like to play a little bit with this, so is there any missing upda=
tes?

Nope, that is our most recent version.

Nick

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

[Andrei Emeltchenko]

Hi All,

On Wed, Jun 16, 2010 at 5:15 PM, Nick Pelly <npelly@google.com> wrote:
> On Wed, Jun 16, 2010 at 4:40 AM, Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
>> Hi,
>>
>> On Tue, Mar 9, 2010 at 11:45 PM, Marcel Holtmann <marcel@holtmann.org> w=
rote:
>>> 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 pac=
kets so
>>>> >>> >> that if the ACL packets block for some reason then the LM can f=
lush
>>>> >>> >> 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 flush=
able
>>>> >>> >> 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 t=
hat
>>>> >> we might wanna deprecate this socket options ;)
>>>> >>
>>>> >> I need to read up on the flushable stuff, but in the end it deserve=
s its
>>>> >> own socket option. Also an ioctl() to actually trigger Enhanced flu=
sh
>>>> >> might be needed.
>>>> >>
>>>> >>> struct l2cap_pinfo {
>>>> >>> =A0 =A0...
>>>> >>> =A0 =A0__u8 flushable;
>>>> >>> }
>>>> >>
>>>> >> Sure. In the long run we need to turn this into a bitmask. We are j=
ust
>>>> >> 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.
>>>>
>>>> Attached an updated patch for 2.6.32 kernel. We've been using this
>>>> patch successfully on production devices.
>>>
>>> can see anything wrong with that patch. However we need to use
>>> SOL_BLUETOOTH for it of course. So we need to come up with something to
>>> make this simple.

Nick are you going to take Marcel comments? Otherwise I could take
care about the patch as it seems that it might help in some
situations.

>>> An additional change I like to see is to use flags for booleans like
>>> flushable in the structures. Can you work on changing that.
>>>
>>> Also do we have decoding support for this in hcidump. It might be nice
>>> to include some really simple examples in the commit message.

At least wireshark which I use understands those packets.

>> I would like to play a little bit with this, so is there any missing upd=
ates?
>
> Nope, that is our most recent version.

Nick, do you know headset which could help to hear the real
difference? I was trying to use Sony DR-BT22 headset which has some
issues with A2DP but the solution did not help much.

Regards,
Andrei

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

[Nick Pelly]

On Thu, Dec 9, 2010 at 2:37 AM, Andrei Emeltchenko
<andrei.emeltchenko.news@gmail.com> wrote:
>
> Hi All,
>
> On Wed, Jun 16, 2010 at 5:15 PM, Nick Pelly <npelly@google.com> wrote:
> > On Wed, Jun 16, 2010 at 4:40 AM, Luiz Augusto von Dentz
> > <luiz.dentz@gmail.com> wrote:
> >> Hi,
> >>
> >> On Tue, Mar 9, 2010 at 11:45 PM, Marcel Holtmann <marcel@holtmann.org>=
 wrote:
> >>> Hi Nick,
> >>>
> >>>> >>> >> Right now Bluez always requests flushable ACL packets (but do=
es not
> >>>> >>> >> set a flush timeout, so effectively they are non-flushable):
> >>>> >>> >>
> >>>> >>> >> However it is desirable to use an ACL flush timeout on A2DP p=
ackets 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 boundar=
y flag by
> >>>> >>> >> default (non-flushable packet), and let userspace request flu=
shable
> >>>> >>> >> packets on A2DP L2CAP sockets with the socket option
> >>>> >>> >> L2CAP_LM_RELIABLE.
> >>>> >>> >
> >>>> >>> > the reliable option has a different meaning. It comes back fro=
m the old
> >>>> >>> > Bluetooth 1.1 qualification days where we had to tests on L2CA=
P that had
> >>>> >>> > to confirm that we can detect malformed packets and report the=
m. 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 deser=
ves its
> >>>> >> own socket option. Also an ioctl() to actually trigger Enhanced f=
lush
> >>>> >> might be needed.
> >>>> >>
> >>>> >>> struct l2cap_pinfo {
> >>>> >>> =A0 =A0...
> >>>> >>> =A0 =A0__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 wit=
h.
> >>>> > 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 AC=
L
> >>>> > 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 you=
r
> >>>> > advice before I implement this.
> >>>>
> >>>> Attached an updated patch for 2.6.32 kernel. We've been using this
> >>>> patch successfully on production devices.
> >>>
> >>> can see anything wrong with that patch. However we need to use
> >>> SOL_BLUETOOTH for it of course. So we need to come up with something =
to
> >>> make this simple.
>
> Nick are you going to take Marcel comments? Otherwise I could take
> care about the patch as it seems that it might help in some
> situations.

I'm not actively working on this patch.

> >>> An additional change I like to see is to use flags for booleans like
> >>> flushable in the structures. Can you work on changing that.
> >>>
> >>> Also do we have decoding support for this in hcidump. It might be nic=
e
> >>> to include some really simple examples in the commit message.
>
> At least wireshark which I use understands those packets.
>
> >> I would like to play a little bit with this, so is there any missing u=
pdates?
> >
> > Nope, that is our most recent version.
>
> Nick, do you know headset which could help to hear the real
> difference? I was trying to use Sony DR-BT22 headset which has some
> issues with A2DP but the solution did not help much.

It becomes essential in non-ideal radio bandwidth conditions such as
single antenna wifi co-existence. We also had some headsets that
exacerbated the problem (presumably they had less logic to 'catch-up'
through late packets) but I can't remember off hand.

Nick

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

[Suraj Sumangala]

Hi,

On 12/9/2010 10:25 PM, Nick Pelly wrote:
> On Thu, Dec 9, 2010 at 2:37 AM, Andrei Emeltchenko
> <andrei.emeltchenko.news@gmail.com>  wrote:
>>
>> Hi All,
>>
>> On Wed, Jun 16, 2010 at 5:15 PM, Nick Pelly<npelly@google.com>  wrote:
>>> On Wed, Jun 16, 2010 at 4:40 AM, Luiz Augusto von Dentz
>>> <luiz.dentz@gmail.com>  wrote:
>>>> Hi,
>>>>
>>>> On Tue, Mar 9, 2010 at 11:45 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.
>>>>>>
>>>>>> Attached an updated patch for 2.6.32 kernel. We've been using this
>>>>>> patch successfully on production devices.
>>>>>
>>>>> can see anything wrong with that patch. However we need to use
>>>>> SOL_BLUETOOTH for it of course. So we need to come up with something to
>>>>> make this simple.
>>
>> Nick are you going to take Marcel comments? Otherwise I could take
>> care about the patch as it seems that it might help in some
>> situations.
>
> I'm not actively working on this patch.
>
>>>>> An additional change I like to see is to use flags for booleans like
>>>>> flushable in the structures. Can you work on changing that.
>>>>>
>>>>> Also do we have decoding support for this in hcidump. It might be nice
>>>>> to include some really simple examples in the commit message.
>>
>> At least wireshark which I use understands those packets.
>>
>>>> I would like to play a little bit with this, so is there any missing updates?
>>>
>>> Nope, that is our most recent version.
>>
>> Nick, do you know headset which could help to hear the real
>> difference? I was trying to use Sony DR-BT22 headset which has some
>> issues with A2DP but the solution did not help much.
>
> It becomes essential in non-ideal radio bandwidth conditions such as
> single antenna wifi co-existence. We also had some headsets that
> exacerbated the problem (presumably they had less logic to 'catch-up'
> through late packets) but I can't remember off hand.

I think you should be able to reproduce the same issue using any A2DP 
headset.

Use a CSR dongle as local controller and start streaming A2DP data.
Parallel to that initiate an FTP connection to another device.
Take the headset out of range, or keep it in a shield box.

You should be able to see that the FTP link also stalls as the A2DP 
packets are stalled in controller without getting flushed.

>
> Nick
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Regards
Suraj