[RFC] Allow Bluez to select flushable or non-flushable ACL packets

[RFC] Allow Bluez to select flushable or non-flushable ACL packets

[Emeltchenko Andrei]

From: Andrei Emeltchenko <andrei.emeltchenko@nokia.com>

Slight modification of original patch from Nick Pelly <npelly@google.com>

Changes: rebasing, simplifying logic, using SOL_BLUETOOTH instead of SOL_L2CAP.

No changes done for RFCOMM sockets since no use case exist so far but can be
added later

Andrei Emeltchenko (1):
  Bluetooth: Use non-flushable pb flag by default for ACL data on
    capable chipsets.

 include/net/bluetooth/bluetooth.h |    5 +++++
 include/net/bluetooth/hci.h       |    2 ++
 include/net/bluetooth/hci_core.h  |    1 +
 include/net/bluetooth/l2cap.h     |    2 ++
 net/bluetooth/hci_core.c          |    6 ++++--
 net/bluetooth/l2cap.c             |   33 +++++++++++++++++++++++++++++++--
 6 files changed, 45 insertions(+), 4 deletions(-)

[RFC] Bluetooth: Use non-flushable pb flag by default for ACL data on capable chipsets.

[Emeltchenko Andrei]

From: Andrei Emeltchenko <andrei.emeltchenko@nokia.com>

Modification of Nick Pelly <npelly@google.com> patch.

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 BT_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: Andrei Emeltchenko <andrei.emeltchenko@nokia.com>
---
 include/net/bluetooth/bluetooth.h |    5 +++++
 include/net/bluetooth/hci.h       |    2 ++
 include/net/bluetooth/hci_core.h  |    1 +
 include/net/bluetooth/l2cap.h     |    2 ++
 net/bluetooth/hci_core.c          |    6 ++++--
 net/bluetooth/l2cap.c             |   33 +++++++++++++++++++++++++++++++--
 6 files changed, 45 insertions(+), 4 deletions(-)

diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
index 0c5e725..ed7d775 100644
--- a/include/net/bluetooth/bluetooth.h
+++ b/include/net/bluetooth/bluetooth.h
@@ -64,6 +64,11 @@ struct bt_security {
 
 #define BT_DEFER_SETUP	7
 
+#define BT_FLUSHABLE	8
+
+#define BT_FLUSHABLE_OFF	0
+#define BT_FLUSHABLE_ON		1
+
 #define BT_INFO(fmt, arg...) printk(KERN_INFO "Bluetooth: " fmt "\n" , ## arg)
 #define BT_ERR(fmt, arg...)  printk(KERN_ERR "%s: " fmt "\n" , __func__ , ## arg)
 #define BT_DBG(fmt, arg...)  pr_debug("%s: " fmt "\n" , __func__ , ## arg)
diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index 29a7a8c..333d5cb 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -150,6 +150,7 @@ 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
@@ -193,6 +194,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 1992fac..9778bc8 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -456,6 +456,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 7ad25ca..af35711 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -75,6 +75,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
@@ -327,6 +328,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 51c61f7..c0d776b 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1380,7 +1380,7 @@ void 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);
 
 	list = skb_shinfo(skb)->frag_list;
 	if (!list) {
@@ -1398,12 +1398,14 @@ void 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_START;
+		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 c791fcd..c7f25c2 100644
--- a/net/bluetooth/l2cap.c
+++ b/net/bluetooth/l2cap.c
@@ -362,13 +362,19 @@ static inline u8 l2cap_get_ident(struct l2cap_conn *conn)
 static inline void l2cap_send_cmd(struct l2cap_conn *conn, u8 ident, u8 code, u16 len, void *data)
 {
 	struct sk_buff *skb = l2cap_build_cmd(conn, code, ident, len, data);
+	u8 flags;
 
 	BT_DBG("code 0x%2.2x", code);
 
 	if (!skb)
 		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;
+
+	hci_send_acl(conn->hcon, skb, flags);
 }
 
 static inline void l2cap_send_sframe(struct l2cap_pinfo *pi, u16 control)
@@ -900,6 +906,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;
@@ -915,6 +922,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 = BT_FLUSHABLE_OFF;
 	}
 
 	/* Default config options */
@@ -2098,6 +2106,20 @@ static int l2cap_sock_setsockopt(struct socket *sock, int level, int optname, ch
 		bt_sk(sk)->defer_setup = opt;
 		break;
 
+	case BT_FLUSHABLE:
+		if (get_user(opt, (u32 __user *) optval)) {
+			err = -EFAULT;
+			break;
+		}
+
+		if (opt > BT_FLUSHABLE_ON) {
+			err = -EINVAL;
+			break;
+		}
+
+		l2cap_pi(sk)->flushable = opt;
+		break;
+
 	default:
 		err = -ENOPROTOOPT;
 		break;
@@ -2237,6 +2259,13 @@ static int l2cap_sock_getsockopt(struct socket *sock, int level, int optname, ch
 
 		break;
 
+	case BT_FLUSHABLE:
+
+		if (put_user(l2cap_pi(sk)->flushable, (u32 __user *) optval))
+ 			err = -EFAULT;
+
+ 		break;
+
 	default:
 		err = -ENOPROTOOPT;
 		break;
@@ -4697,7 +4726,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;
 		struct sock *sk;
 		u16 cid;
-- 
1.7.1

Re: [RFC] Bluetooth: Use non-flushable pb flag by default for ACL data on capable chipsets.

[Suraj Sumangala]

Hi Andrei,

On 12/13/2010 8:08 PM, Emeltchenko Andrei wrote:
>   static inline void l2cap_send_cmd(struct l2cap_conn *conn, u8 ident, u8 code, u16 len, void *data)
>   {
>   	struct sk_buff *skb = l2cap_build_cmd(conn, code, ident, len, data);
> +	u8 flags;
>
>   	BT_DBG("code 0x%2.2x", code);
>
>   	if (!skb)
>   		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;
> +
> +	hci_send_acl(conn->hcon, skb, flags);
>   }
>
Should we send L2CAP command packets also as flushable packets? Should 
it be non-flushable?
I thought only L2CAP data frames should be sent as flushable.

Regards
Suraj

Re: [RFC] Bluetooth: Use non-flushable pb flag by default for ACL data on capable chipsets.

[Gustavo F. Padovan]

Hi Andrei,

* Emeltchenko Andrei <Andrei.Emeltchenko.news@gmail.com> [2010-12-13 16:38:25 +0200]:

> From: Andrei Emeltchenko <andrei.emeltchenko@nokia.com>
> 
> Modification of Nick Pelly <npelly@google.com> patch.
> 
> 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 BT_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: Andrei Emeltchenko <andrei.emeltchenko@nokia.com>
> ---
>  include/net/bluetooth/bluetooth.h |    5 +++++
>  include/net/bluetooth/hci.h       |    2 ++
>  include/net/bluetooth/hci_core.h  |    1 +
>  include/net/bluetooth/l2cap.h     |    2 ++
>  net/bluetooth/hci_core.c          |    6 ++++--
>  net/bluetooth/l2cap.c             |   33 +++++++++++++++++++++++++++++++--
>  6 files changed, 45 insertions(+), 4 deletions(-)
> 
> diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
> index 0c5e725..ed7d775 100644
> --- a/include/net/bluetooth/bluetooth.h
> +++ b/include/net/bluetooth/bluetooth.h
> @@ -64,6 +64,11 @@ struct bt_security {
>  
>  #define BT_DEFER_SETUP	7
>  
> +#define BT_FLUSHABLE	8
> +
> +#define BT_FLUSHABLE_OFF	0
> +#define BT_FLUSHABLE_ON		1
> +
>  #define BT_INFO(fmt, arg...) printk(KERN_INFO "Bluetooth: " fmt "\n" , ## arg)
>  #define BT_ERR(fmt, arg...)  printk(KERN_ERR "%s: " fmt "\n" , __func__ , ## arg)
>  #define BT_DBG(fmt, arg...)  pr_debug("%s: " fmt "\n" , __func__ , ## arg)
> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> index 29a7a8c..333d5cb 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -150,6 +150,7 @@ 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
> @@ -193,6 +194,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 1992fac..9778bc8 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -456,6 +456,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)

IMHO lmp_flush_capable() makes more sense. We can avoid things like
(!lmp_no_flush_capable(dev)) and add two negatives in the same comparison.

>  
>  /* ----- HCI protocols ----- */
>  struct hci_proto {
> diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
> index 7ad25ca..af35711 100644
> --- a/include/net/bluetooth/l2cap.h
> +++ b/include/net/bluetooth/l2cap.h
> @@ -75,6 +75,7 @@ struct l2cap_conninfo {
>  #define L2CAP_LM_TRUSTED	0x0008
>  #define L2CAP_LM_RELIABLE	0x0010
>  #define L2CAP_LM_SECURE		0x0020
> +#define L2CAP_LM_FLUSHABLE	0x0040

Not using this anywhere.

>  
>  /* L2CAP command codes */
>  #define L2CAP_COMMAND_REJ	0x01
> @@ -327,6 +328,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 51c61f7..c0d776b 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -1380,7 +1380,7 @@ void 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);
>  
>  	list = skb_shinfo(skb)->frag_list;
>  	if (!list) {
> @@ -1398,12 +1398,14 @@ void 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_START;
> +		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 c791fcd..c7f25c2 100644
> --- a/net/bluetooth/l2cap.c
> +++ b/net/bluetooth/l2cap.c
> @@ -362,13 +362,19 @@ static inline u8 l2cap_get_ident(struct l2cap_conn *conn)
>  static inline void l2cap_send_cmd(struct l2cap_conn *conn, u8 ident, u8 code, u16 len, void *data)
>  {
>  	struct sk_buff *skb = l2cap_build_cmd(conn, code, ident, len, data);
> +	u8 flags;
>  
>  	BT_DBG("code 0x%2.2x", code);
>  
>  	if (!skb)
>  		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;
> +
> +	hci_send_acl(conn->hcon, skb, flags);

I also agree that l2cap commands should be non-flushable. Just pass
ACL_START_NO_FLUSH to hci_send_acl() here.
You should add this check to l2cap_do_send() instead.

>  }
>  
>  static inline void l2cap_send_sframe(struct l2cap_pinfo *pi, u16 control)
> @@ -900,6 +906,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;
> @@ -915,6 +922,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 = BT_FLUSHABLE_OFF;
>  	}
>  
>  	/* Default config options */
> @@ -2098,6 +2106,20 @@ static int l2cap_sock_setsockopt(struct socket *sock, int level, int optname, ch
>  		bt_sk(sk)->defer_setup = opt;
>  		break;
>  
> +	case BT_FLUSHABLE:
> +		if (get_user(opt, (u32 __user *) optval)) {
> +			err = -EFAULT;
> +			break;
> +		}
> +
> +		if (opt > BT_FLUSHABLE_ON) {
> +			err = -EINVAL;
> +			break;
> +		}
> +
> +		l2cap_pi(sk)->flushable = opt;

You are not using this flushable value anywhere. Something is wrong here.

-- 
Gustavo F. Padovan
http://profusion.mobi

Re: [RFC] Bluetooth: Use non-flushable pb flag by default for ACL data on capable chipsets.

[Mat Martineau]

On Tue, 14 Dec 2010, Gustavo F. Padovan wrote:

> Hi Andrei,
>
> * Emeltchenko Andrei <Andrei.Emeltchenko.news@gmail.com> [2010-12-13 16:38:25 +0200]:
>
>> From: Andrei Emeltchenko <andrei.emeltchenko@nokia.com>
>>
>> Modification of Nick Pelly <npelly@google.com> patch.
>>
>> 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 BT_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).

This is a great feature to add, not only for A2DP.  Both ERTM and 
streaming mode only really make sense on unreliable links.

>> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@nokia.com>
>> ---
>>  include/net/bluetooth/bluetooth.h |    5 +++++
>>  include/net/bluetooth/hci.h       |    2 ++
>>  include/net/bluetooth/hci_core.h  |    1 +
>>  include/net/bluetooth/l2cap.h     |    2 ++
>>  net/bluetooth/hci_core.c          |    6 ++++--
>>  net/bluetooth/l2cap.c             |   33 +++++++++++++++++++++++++++++++--
>>  6 files changed, 45 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
>> index 0c5e725..ed7d775 100644
>> --- a/include/net/bluetooth/bluetooth.h
>> +++ b/include/net/bluetooth/bluetooth.h
>> @@ -64,6 +64,11 @@ struct bt_security {
>>
>>  #define BT_DEFER_SETUP	7
>>
>> +#define BT_FLUSHABLE	8
>> +
>> +#define BT_FLUSHABLE_OFF	0
>> +#define BT_FLUSHABLE_ON		1
>> +
>>  #define BT_INFO(fmt, arg...) printk(KERN_INFO "Bluetooth: " fmt "\n" , ## arg)
>>  #define BT_ERR(fmt, arg...)  printk(KERN_ERR "%s: " fmt "\n" , __func__ , ## arg)
>>  #define BT_DBG(fmt, arg...)  pr_debug("%s: " fmt "\n" , __func__ , ## arg)
>> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
>> index 29a7a8c..333d5cb 100644
>> --- a/include/net/bluetooth/hci.h
>> +++ b/include/net/bluetooth/hci.h
>> @@ -150,6 +150,7 @@ 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
>> @@ -193,6 +194,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 1992fac..9778bc8 100644
>> --- a/include/net/bluetooth/hci_core.h
>> +++ b/include/net/bluetooth/hci_core.h
>> @@ -456,6 +456,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)
>
> IMHO lmp_flush_capable() makes more sense. We can avoid things like
> (!lmp_no_flush_capable(dev)) and add two negatives in the same comparison.
>
>>
>>  /* ----- HCI protocols ----- */
>>  struct hci_proto {
>> diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
>> index 7ad25ca..af35711 100644
>> --- a/include/net/bluetooth/l2cap.h
>> +++ b/include/net/bluetooth/l2cap.h
>> @@ -75,6 +75,7 @@ struct l2cap_conninfo {
>>  #define L2CAP_LM_TRUSTED	0x0008
>>  #define L2CAP_LM_RELIABLE	0x0010
>>  #define L2CAP_LM_SECURE		0x0020
>> +#define L2CAP_LM_FLUSHABLE	0x0040
>
> Not using this anywhere.
>
>>
>>  /* L2CAP command codes */
>>  #define L2CAP_COMMAND_REJ	0x01
>> @@ -327,6 +328,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 51c61f7..c0d776b 100644
>> --- a/net/bluetooth/hci_core.c
>> +++ b/net/bluetooth/hci_core.c
>> @@ -1380,7 +1380,7 @@ void 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);
>>
>>  	list = skb_shinfo(skb)->frag_list;
>>  	if (!list) {
>> @@ -1398,12 +1398,14 @@ void 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_START;
>> +		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 c791fcd..c7f25c2 100644
>> --- a/net/bluetooth/l2cap.c
>> +++ b/net/bluetooth/l2cap.c
>> @@ -362,13 +362,19 @@ static inline u8 l2cap_get_ident(struct l2cap_conn *conn)
>>  static inline void l2cap_send_cmd(struct l2cap_conn *conn, u8 ident, u8 code, u16 len, void *data)
>>  {
>>  	struct sk_buff *skb = l2cap_build_cmd(conn, code, ident, len, data);
>> +	u8 flags;
>>
>>  	BT_DBG("code 0x%2.2x", code);
>>
>>  	if (!skb)
>>  		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;
>> +
>> +	hci_send_acl(conn->hcon, skb, flags);
>
> I also agree that l2cap commands should be non-flushable. Just pass
> ACL_START_NO_FLUSH to hci_send_acl() here.
> You should add this check to l2cap_do_send() instead.
>
>>  }
>>
>>  static inline void l2cap_send_sframe(struct l2cap_pinfo *pi, u16 control)
>> @@ -900,6 +906,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;
>> @@ -915,6 +922,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 = BT_FLUSHABLE_OFF;
>>  	}
>>
>>  	/* Default config options */
>> @@ -2098,6 +2106,20 @@ static int l2cap_sock_setsockopt(struct socket *sock, int level, int optname, ch
>>  		bt_sk(sk)->defer_setup = opt;
>>  		break;
>>
>> +	case BT_FLUSHABLE:
>> +		if (get_user(opt, (u32 __user *) optval)) {
>> +			err = -EFAULT;
>> +			break;
>> +		}
>> +
>> +		if (opt > BT_FLUSHABLE_ON) {
>> +			err = -EINVAL;
>> +			break;
>> +		}
>> +
>> +		l2cap_pi(sk)->flushable = opt;

Does it make sense to check the HCI device for flush capability here? 
If the HCI device is not capable, this option should remain false. 
This would also let the application use getsockopt() to find out if 
the link is really flushable or not.

>
> You are not using this flushable value anywhere. Something is wrong here.

I agree - like Gustavo mentioned above, l2cap_do_send() needs to be 
checking l2cap_pi(sk)->flushable and setting the flags in 
hci_send_acl() appropriately.

There is one more thing missing:  Even though there is an L2CAP socket 
option to set flush_to, and the flush_to is passed around during L2CAP 
configuration, there is no use of the "Write Automatic Flush Timeout" 
HCI command to tell the baseband what the flush timeout is!  Since the 
flush timeout is shared across all connections on the ACL, how should 
BlueZ handle the case where different flush timeouts are set on 
connections that share the same ACL?  (My guess is that either the 
longest or shortest timeout should be used, but there are good 
arguments either way)

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

Re: [RFC] Bluetooth: Use non-flushable pb flag by default for ACL data on capable chipsets.

[Andrei Emeltchenko]

Hi,

On Wed, Dec 15, 2010 at 6:35 PM, Mat Martineau <mathewm@codeaurora.org> wrote:
>
> On Tue, 14 Dec 2010, Gustavo F. Padovan wrote:
>
>> Hi Andrei,
>>
>> * Emeltchenko Andrei <Andrei.Emeltchenko.news@gmail.com> [2010-12-13
>> 16:38:25 +0200]:
>>
>>> From: Andrei Emeltchenko <andrei.emeltchenko@nokia.com>
>>>
>>> Modification of Nick Pelly <npelly@google.com> patch.
>>>
>>> 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 BT_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).
>
> This is a great feature to add, not only for A2DP.  Both ERTM and streaming
> mode only really make sense on unreliable links.
>
>>> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@nokia.com>
>>> ---
>>>  include/net/bluetooth/bluetooth.h |    5 +++++
>>>  include/net/bluetooth/hci.h       |    2 ++
>>>  include/net/bluetooth/hci_core.h  |    1 +
>>>  include/net/bluetooth/l2cap.h     |    2 ++
>>>  net/bluetooth/hci_core.c          |    6 ++++--
>>>  net/bluetooth/l2cap.c             |   33
>>> +++++++++++++++++++++++++++++++--
>>>  6 files changed, 45 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/include/net/bluetooth/bluetooth.h
>>> b/include/net/bluetooth/bluetooth.h
>>> index 0c5e725..ed7d775 100644
>>> --- a/include/net/bluetooth/bluetooth.h
>>> +++ b/include/net/bluetooth/bluetooth.h
>>> @@ -64,6 +64,11 @@ struct bt_security {
>>>
>>>  #define BT_DEFER_SETUP 7
>>>
>>> +#define BT_FLUSHABLE   8
>>> +
>>> +#define BT_FLUSHABLE_OFF       0
>>> +#define BT_FLUSHABLE_ON                1
>>> +
>>>  #define BT_INFO(fmt, arg...) printk(KERN_INFO "Bluetooth: " fmt "\n" ,
>>> ## arg)
>>>  #define BT_ERR(fmt, arg...)  printk(KERN_ERR "%s: " fmt "\n" , __func__
>>> , ## arg)
>>>  #define BT_DBG(fmt, arg...)  pr_debug("%s: " fmt "\n" , __func__ , ##
>>> arg)
>>> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
>>> index 29a7a8c..333d5cb 100644
>>> --- a/include/net/bluetooth/hci.h
>>> +++ b/include/net/bluetooth/hci.h
>>> @@ -150,6 +150,7 @@ 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
>>> @@ -193,6 +194,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 1992fac..9778bc8 100644
>>> --- a/include/net/bluetooth/hci_core.h
>>> +++ b/include/net/bluetooth/hci_core.h
>>> @@ -456,6 +456,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)
>>
>> IMHO lmp_flush_capable() makes more sense. We can avoid things like
>> (!lmp_no_flush_capable(dev)) and add two negatives in the same comparison.
>>
>>>
>>>  /* ----- HCI protocols ----- */
>>>  struct hci_proto {
>>> diff --git a/include/net/bluetooth/l2cap.h
>>> b/include/net/bluetooth/l2cap.h
>>> index 7ad25ca..af35711 100644
>>> --- a/include/net/bluetooth/l2cap.h
>>> +++ b/include/net/bluetooth/l2cap.h
>>> @@ -75,6 +75,7 @@ struct l2cap_conninfo {
>>>  #define L2CAP_LM_TRUSTED       0x0008
>>>  #define L2CAP_LM_RELIABLE      0x0010
>>>  #define L2CAP_LM_SECURE                0x0020
>>> +#define L2CAP_LM_FLUSHABLE     0x0040
>>
>> Not using this anywhere.
>>
>>>
>>>  /* L2CAP command codes */
>>>  #define L2CAP_COMMAND_REJ      0x01
>>> @@ -327,6 +328,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 51c61f7..c0d776b 100644
>>> --- a/net/bluetooth/hci_core.c
>>> +++ b/net/bluetooth/hci_core.c
>>> @@ -1380,7 +1380,7 @@ void 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);
>>>
>>>        list = skb_shinfo(skb)->frag_list;
>>>        if (!list) {
>>> @@ -1398,12 +1398,14 @@ void 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_START;
>>> +               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 c791fcd..c7f25c2 100644
>>> --- a/net/bluetooth/l2cap.c
>>> +++ b/net/bluetooth/l2cap.c
>>> @@ -362,13 +362,19 @@ static inline u8 l2cap_get_ident(struct l2cap_conn
>>> *conn)
>>>  static inline void l2cap_send_cmd(struct l2cap_conn *conn, u8 ident, u8
>>> code, u16 len, void *data)
>>>  {
>>>        struct sk_buff *skb = l2cap_build_cmd(conn, code, ident, len,
>>> data);
>>> +       u8 flags;
>>>
>>>        BT_DBG("code 0x%2.2x", code);
>>>
>>>        if (!skb)
>>>                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;
>>> +
>>> +       hci_send_acl(conn->hcon, skb, flags);
>>
>> I also agree that l2cap commands should be non-flushable. Just pass
>> ACL_START_NO_FLUSH to hci_send_acl() here.

Gustavo if the controller does not support non flushable packets we
may have bug here.

>> You should add this check to l2cap_do_send() instead.

This is done for data packets.

>>>  }
>>>
>>>  static inline void l2cap_send_sframe(struct l2cap_pinfo *pi, u16
>>> control)
>>> @@ -900,6 +906,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;
>>> @@ -915,6 +922,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 = BT_FLUSHABLE_OFF;
>>>        }
>>>
>>>        /* Default config options */
>>> @@ -2098,6 +2106,20 @@ static int l2cap_sock_setsockopt(struct socket
>>> *sock, int level, int optname, ch
>>>                bt_sk(sk)->defer_setup = opt;
>>>                break;
>>>
>>> +       case BT_FLUSHABLE:
>>> +               if (get_user(opt, (u32 __user *) optval)) {
>>> +                       err = -EFAULT;
>>> +                       break;
>>> +               }
>>> +
>>> +               if (opt > BT_FLUSHABLE_ON) {
>>> +                       err = -EINVAL;
>>> +                       break;
>>> +               }
>>> +
>>> +               l2cap_pi(sk)->flushable = opt;
>
> Does it make sense to check the HCI device for flush capability here? If the
> HCI device is not capable, this option should remain false. This would also
> let the application use getsockopt() to find out if the link is really
> flushable or not.

Thanks for the hint, I will add the check here and return -EINVAL then.

>> You are not using this flushable value anywhere. Something is wrong here.
>
> I agree - like Gustavo mentioned above, l2cap_do_send() needs to be checking
> l2cap_pi(sk)->flushable and setting the flags in hci_send_acl()
> appropriately.

Yes, I have lost one chunk when reformatting patches against different branches.

The lost chunk is below:

diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
index c7f25c2..f7260ad 100644
--- a/net/bluetooth/l2cap.c
+++ b/net/bluetooth/l2cap.c
@@ -1458,10 +1458,16 @@ static void l2cap_drop_acked_frames(struct sock *sk)
 static inline void l2cap_do_send(struct sock *sk, struct sk_buff *skb)
 {
        struct l2cap_pinfo *pi = l2cap_pi(sk);
+       struct hci_conn *hcon = pi->conn->hcon;

        BT_DBG("sk %p, skb %p len %d", sk, skb, skb->len);

-       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;
+
+       hci_send_acl(pi->conn->hcon, skb, flags);
 }

 static void l2cap_streaming_send(struct sock *sk)

> There is one more thing missing:  Even though there is an L2CAP socket
> option to set flush_to, and the flush_to is passed around during L2CAP
> configuration, there is no use of the "Write Automatic Flush Timeout" HCI
> command to tell the baseband what the flush timeout is!  Since the flush
> timeout is shared across all connections on the ACL, how should BlueZ handle
> the case where different flush timeouts are set on connections that share
> the same ACL?  (My guess is that either the longest or shortest timeout
> should be used, but there are good arguments either way)
>
> --
> Mat Martineau
> Employee of Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
>

Re: [RFC] Bluetooth: Use non-flushable pb flag by default for ACL data on capable chipsets.

[Andrei Emeltchenko]

Hi,

On Wed, Dec 15, 2010 at 6:35 PM, Mat Martineau <mathewm@codeaurora.org> wrote:
<skipped>
> There is one more thing missing:  Even though there is an L2CAP socket
> option to set flush_to, and the flush_to is passed around during L2CAP
> configuration, there is no use of the "Write Automatic Flush Timeout" HCI
> command to tell the baseband what the flush timeout is!  Since the flush
> timeout is shared across all connections on the ACL, how should BlueZ handle
> the case where different flush timeouts are set on connections that share
> the same ACL?  (My guess is that either the longest or shortest timeout
> should be used, but there are good arguments either way)

I would suggest bluetoothd to take care about setting Flush Timeout.
There is of course possibility to have sockopt for timeout and send HCI command
but this looks like a dirty hack.

I think bluetoothd can read compare and write new flush timeout value.

Regards,
Andrei

Re: [RFC] Bluetooth: Use non-flushable pb flag by default for ACL data on capable chipsets.

[Mat Martineau]

On Thu, 16 Dec 2010, Andrei Emeltchenko wrote:

> Hi,
>
> On Wed, Dec 15, 2010 at 6:35 PM, Mat Martineau <mathewm@codeaurora.org> wrote:
> <skipped>
>> There is one more thing missing:  Even though there is an L2CAP socket
>> option to set flush_to, and the flush_to is passed around during L2CAP
>> configuration, there is no use of the "Write Automatic Flush Timeout" HCI
>> command to tell the baseband what the flush timeout is!  Since the flush
>> timeout is shared across all connections on the ACL, how should BlueZ handle
>> the case where different flush timeouts are set on connections that share
>> the same ACL?  (My guess is that either the longest or shortest timeout
>> should be used, but there are good arguments either way)
>
> I would suggest bluetoothd to take care about setting Flush Timeout. 
> There is of course possibility to have sockopt for timeout and send 
> HCI command but this looks like a dirty hack.
>
> I think bluetoothd can read compare and write new flush timeout value.

There is *already* a sockopt for flush timeout (flush_to in 
l2cap_options, it was added before 2005), but it is not terribly 
useful because it does not configure the baseband.

One more piece of background information: The spec requires the 
default flush timeout to be infinite, so if no "Write Automatic Flush 
Timeout" command is sent, none of this flush code will accomplish 
anything.  Android has customized their version of BlueZ to send this 
command for A2DP connections, with certain BR/EDR basebands.

I don't have any major objection to managing this setting from 
bluetoothd.  My only minor objection is that it requires an 
application to manipulate the setting via DBus instead of using the 
existing sockopt - it would be confusing to have both available, but I 
suppose the sockopt could be deprecated or made read-only.  The flush 
timeout would seem to fit well as a device property.


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

Re: [RFC] Bluetooth: Use non-flushable pb flag by default for ACL data on capable chipsets.

[Andrei Emeltchenko]

Hi,

On Thu, Dec 16, 2010 at 7:03 PM, Mat Martineau <mathewm@codeaurora.org> wrote:
>> <skipped>
>>>
>>> There is one more thing missing:  Even though there is an L2CAP socket
>>> option to set flush_to, and the flush_to is passed around during L2CAP
>>> configuration, there is no use of the "Write Automatic Flush Timeout" HCI
>>> command to tell the baseband what the flush timeout is!  Since the flush
>>> timeout is shared across all connections on the ACL, how should BlueZ
>>> handle
>>> the case where different flush timeouts are set on connections that share
>>> the same ACL?  (My guess is that either the longest or shortest timeout
>>> should be used, but there are good arguments either way)
>>
>> I would suggest bluetoothd to take care about setting Flush Timeout. There
>> is of course possibility to have sockopt for timeout and send HCI command
>> but this looks like a dirty hack.
>>
>> I think bluetoothd can read compare and write new flush timeout value.
>
> There is *already* a sockopt for flush timeout (flush_to in l2cap_options,
> it was added before 2005), but it is not terribly useful because it does not
> configure the baseband.

Yes, I just check, that code is just dummy code and only used to set dummy
value in L2CAP configuration phase with no actual use.

If I execute command like:
#Write Automatic Flush Timeout
~# hcitool cmd 0x03 0x0028 0x01 0x00 0xa0 0x0
#for handle 0x0001 to 100ms

it will still report old value through that kernel interface.

> One more piece of background information: The spec requires the default
> flush timeout to be infinite, so if no "Write Automatic Flush Timeout"
> command is sent, none of this flush code will accomplish anything.  Android
> has customized their version of BlueZ to send this command for A2DP
> connections, with certain BR/EDR basebands.

I believe they do it right way.

> I don't have any major objection to managing this setting from bluetoothd.
>  My only minor objection is that it requires an application to manipulate
> the setting via DBus instead of using the existing sockopt - it would be
> confusing to have both available, but I suppose the sockopt could be
> deprecated or made read-only.  The flush timeout would seem to fit well as a
> device property.

I would remove it at all, but of course it can be done as read-only.

Regards,
Andrei