Return-Path: MIME-Version: 1.0 In-Reply-To: References: <1292251105-31130-1-git-send-email-Andrei.Emeltchenko.news@gmail.com> <1292251105-31130-2-git-send-email-Andrei.Emeltchenko.news@gmail.com> <20101214192223.GD18509@vigoh> Date: Thu, 16 Dec 2010 11:00:15 +0200 Message-ID: Subject: Re: [RFC] Bluetooth: Use non-flushable pb flag by default for ACL data on capable chipsets. From: Andrei Emeltchenko To: Mat Martineau Cc: "Gustavo F. Padovan" , linux-bluetooth@vger.kernel.org Content-Type: text/plain; charset=ISO-8859-1 Sender: linux-bluetooth-owner@vger.kernel.org List-ID: Hi, On Wed, Dec 15, 2010 at 6:35 PM, Mat Martineau wrote: > > On Tue, 14 Dec 2010, Gustavo F. Padovan wrote: > >> Hi Andrei, >> >> * Emeltchenko Andrei [2010-12-13 >> 16:38:25 +0200]: >> >>> From: Andrei Emeltchenko >>> >>> Modification of Nick Pelly 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 >>> --- >>> ?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 >