Return-Path: Date: Tue, 14 Dec 2010 17:22:23 -0200 From: "Gustavo F. Padovan" To: Emeltchenko Andrei Cc: linux-bluetooth@vger.kernel.org Subject: Re: [RFC] Bluetooth: Use non-flushable pb flag by default for ACL data on capable chipsets. Message-ID: <20101214192223.GD18509@vigoh> References: <1292251105-31130-1-git-send-email-Andrei.Emeltchenko.news@gmail.com> <1292251105-31130-2-git-send-email-Andrei.Emeltchenko.news@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii In-Reply-To: <1292251105-31130-2-git-send-email-Andrei.Emeltchenko.news@gmail.com> Sender: linux-bluetooth-owner@vger.kernel.org List-ID: 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). > > 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. 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