From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: MIME-Version: 1.0 In-Reply-To: <35c90d961002111159h6727bc2cw28b55ce6e919fb4f@mail.gmail.com> References: <1265918068-14721-1-git-send-email-npelly@google.com> <35c90d961002111159h6727bc2cw28b55ce6e919fb4f@mail.gmail.com> From: Nick Pelly Date: Mon, 15 Feb 2010 13:15:34 -0800 Message-ID: <35c90d961002151315i5fc0f5b9y36aaba4415987a2f@mail.gmail.com> Subject: Re: [PATCH] Bluetooth: Allow SCO/eSCO packet type selection for outgoing SCO connections. To: linux-bluetooth@vger.kernel.org Cc: Nick Pelly Content-Type: text/plain; charset=ISO-8859-1 Sender: linux-bluetooth-owner@vger.kernel.org List-ID: On Thu, Feb 11, 2010 at 11:59 AM, Nick Pelly wrote: > On Thu, Feb 11, 2010 at 11:54 AM, Nick Pelly wrote: >> __u16 sco_pkt_type is introduced to struct sockaddr_sco. It allows bitwise >> selection of SCO/eSCO packet types as per the Packet_Type parameter in >> HCI Setup Synchrnous Connection Command. >> >> 0x0001 HV1 may be used. >> 0x0002 HV2 may be used. >> 0x0004 HV3 may be used. >> 0x0008 EV3 may be used. >> 0x0010 EV4 may be used. >> 0x0020 EV5 may be used. >> 0x0040 2-EV3 may not be used. >> 0x0080 3-EV3 may not be used. >> 0x0100 2-EV5 may not be used. >> 0x0200 3-EV5 may not be used. >> >> We've followed the Bluetooth SIG convention of reversing the logic on the >> EDR bits. >> >> Packet type selection is just a request made to the Bluetooth chipset, and >> it is up to the link manager on the chipset to negiotiate and decide on the >> actual packet types used. Furthermore, when a SCO/eSCO connection is eventually >> made there is no way for the host stack to determine which packet type was used >> (however it is possible to get the link type of SCO or eSCO). >> >> sco_pkt_type is ignored for incoming SCO connections. It is possible >> to add this in the future as a parameter to the Accept Synchronous Connection >> Command, however its a little trickier because the kernel does not >> currently preserve sockaddr_sco data between userspace calls to accept(). >> >> Typically userspace just wants to select eSCO or SCO packet types, which can be >> done with the new constants ALL_SCO_PKTS or ALL_ESCO_PKTS. If sco_pkt_type is >> zero, or userspace uses the old sockaddr_sco structure size, then ALL_ESCO_PKTS >> will be selected as default. >> >> This patch is motivated by broken Bluetooth carkits such as the Motorolo >> HF850 (it claims to support eSCO, but will actually reject eSCO connections >> after 5 seconds) and the 2007/2008 Infiniti G35/37 (fails to route audio >> if a 2-EV5 packet type is negiotiated). With this patch userspace can maintain >> a blacklist of packet types to workaround broken remote devices such as these. >> >> Signed-off-by: Nick Pelly >> --- >>  include/net/bluetooth/hci.h      |    7 +++- >>  include/net/bluetooth/hci_core.h |    7 +++- >>  include/net/bluetooth/sco.h      |    4 ++- >>  net/bluetooth/hci_conn.c         |   25 +++++++++------ >>  net/bluetooth/hci_event.c        |    6 ++- >>  net/bluetooth/l2cap.c            |    2 +- >>  net/bluetooth/sco.c              |   61 +++++++++++++++++++++++++------------ >>  7 files changed, 74 insertions(+), 38 deletions(-) >> >> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h >> index 4f7795f..72efc94 100644 >> --- a/include/net/bluetooth/hci.h >> +++ b/include/net/bluetooth/hci.h >> @@ -139,8 +139,11 @@ enum { >>  #define ESCO_2EV5      0x0100 >>  #define ESCO_3EV5      0x0200 >> >> -#define SCO_ESCO_MASK  (ESCO_HV1 | ESCO_HV2 | ESCO_HV3) >> -#define EDR_ESCO_MASK  (ESCO_2EV3 | ESCO_3EV3 | ESCO_2EV5 | ESCO_3EV5) >> +#define SCO_ESCO_MASK  (ESCO_HV1 | ESCO_HV2 | ESCO_HV3) >> +#define EDR_ESCO_MASK  (ESCO_2EV3 | ESCO_3EV3 | ESCO_2EV5 | ESCO_3EV5) >> + >> +#define ALL_SCO_PKTS   (SCO_ESCO_MASK | EDR_ESCO_MASK) >> +#define ALL_ESCO_PKTS  (SCO_ESCO_MASK | ESCO_EV3 | ESCO_EV4 | ESCO_EV5) >> >>  /* ACL flags */ >>  #define ACL_START              0x00 >> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h >> index b6d36cb..cbcc5b1 100644 >> --- a/include/net/bluetooth/hci_core.h >> +++ b/include/net/bluetooth/hci_core.h >> @@ -326,12 +326,15 @@ void hci_acl_disconn(struct hci_conn *conn, __u8 reason); >>  void hci_add_sco(struct hci_conn *conn, __u16 handle); >>  void hci_setup_sync(struct hci_conn *conn, __u16 handle); >> >> -struct hci_conn *hci_conn_add(struct hci_dev *hdev, int type, bdaddr_t *dst); >> +struct hci_conn *hci_conn_add(struct hci_dev *hdev, int type, >> +                                       __u16 pkt_type, bdaddr_t *dst); >>  int hci_conn_del(struct hci_conn *conn); >>  void hci_conn_hash_flush(struct hci_dev *hdev); >>  void hci_conn_check_pending(struct hci_dev *hdev); >> >> -struct hci_conn *hci_connect(struct hci_dev *hdev, int type, bdaddr_t *dst, __u8 sec_level, __u8 auth_type); >> +struct hci_conn *hci_connect(struct hci_dev *hdev, int type, >> +                                       __u16 pkt_type, bdaddr_t *dst, >> +                                       __u8 sec_level, __u8 auth_type); >>  int hci_conn_check_link_mode(struct hci_conn *conn); >>  int hci_conn_security(struct hci_conn *conn, __u8 sec_level, __u8 auth_type); >>  int hci_conn_change_link_key(struct hci_conn *conn); >> diff --git a/include/net/bluetooth/sco.h b/include/net/bluetooth/sco.h >> index e28a2a7..924338a 100644 >> --- a/include/net/bluetooth/sco.h >> +++ b/include/net/bluetooth/sco.h >> @@ -37,6 +37,7 @@ >>  struct sockaddr_sco { >>        sa_family_t     sco_family; >>        bdaddr_t        sco_bdaddr; >> +       __u16           sco_pkt_type; >>  }; >> >>  /* SCO socket options */ >> @@ -72,7 +73,8 @@ struct sco_conn { >> >>  struct sco_pinfo { >>        struct bt_sock  bt; >> -       __u32           flags; >> +       __u16           pkt_type; >> + >>        struct sco_conn *conn; >>  }; >> >> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c >> index fa8b412..8ced057 100644 >> --- a/net/bluetooth/hci_conn.c >> +++ b/net/bluetooth/hci_conn.c >> @@ -196,7 +196,8 @@ static void hci_conn_idle(unsigned long arg) >>        hci_conn_enter_sniff_mode(conn); >>  } >> >> -struct hci_conn *hci_conn_add(struct hci_dev *hdev, int type, bdaddr_t *dst) >> +struct hci_conn *hci_conn_add(struct hci_dev *hdev, int type, >> +                                       __u16 pkt_type, bdaddr_t *dst) >>  { >>        struct hci_conn *conn; >> >> @@ -221,14 +222,16 @@ struct hci_conn *hci_conn_add(struct hci_dev *hdev, int type, bdaddr_t *dst) >>                conn->pkt_type = hdev->pkt_type & ACL_PTYPE_MASK; >>                break; >>        case SCO_LINK: >> +               if (!pkt_type) >> +                       pkt_type = ALL_SCO_PKTS; >> +       case ESCO_LINK: >> +               if (!pkt_type) >> +                       pkt_type = ALL_ESCO_PKTS; >>                if (lmp_esco_capable(hdev)) >> -                       conn->pkt_type = (hdev->esco_type & SCO_ESCO_MASK) | >> -                                       (hdev->esco_type & EDR_ESCO_MASK); >> +                       conn->pkt_type = pkt_type & hdev->esco_type; >>                else >> -                       conn->pkt_type = hdev->pkt_type & SCO_PTYPE_MASK; >> -               break; >> -       case ESCO_LINK: >> -               conn->pkt_type = hdev->esco_type & ~EDR_ESCO_MASK; >> +                       conn->pkt_type = (pkt_type << 5) & hdev->pkt_type & >> +                                       SCO_PTYPE_MASK; >>                break; >>        } >> >> @@ -340,7 +343,9 @@ EXPORT_SYMBOL(hci_get_route); >> >>  /* Create SCO or ACL connection. >>  * Device _must_ be locked */ >> -struct hci_conn *hci_connect(struct hci_dev *hdev, int type, bdaddr_t *dst, __u8 sec_level, __u8 auth_type) >> +struct hci_conn *hci_connect(struct hci_dev *hdev, int type, >> +                                       __u16 pkt_type, bdaddr_t *dst, >> +                                       __u8 sec_level, __u8 auth_type) >>  { >>        struct hci_conn *acl; >>        struct hci_conn *sco; >> @@ -348,7 +353,7 @@ struct hci_conn *hci_connect(struct hci_dev *hdev, int type, bdaddr_t *dst, __u8 >>        BT_DBG("%s dst %s", hdev->name, batostr(dst)); >> >>        if (!(acl = hci_conn_hash_lookup_ba(hdev, ACL_LINK, dst))) { >> -               if (!(acl = hci_conn_add(hdev, ACL_LINK, dst))) >> +               if (!(acl = hci_conn_add(hdev, ACL_LINK, 0, dst))) >>                        return NULL; >>        } >> >> @@ -364,7 +369,7 @@ struct hci_conn *hci_connect(struct hci_dev *hdev, int type, bdaddr_t *dst, __u8 >>                return acl; >> >>        if (!(sco = hci_conn_hash_lookup_ba(hdev, type, dst))) { >> -               if (!(sco = hci_conn_add(hdev, type, dst))) { >> +               if (!(sco = hci_conn_add(hdev, type, pkt_type, dst))) { >>                        hci_conn_put(acl); >>                        return NULL; >>                } >> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c >> index 10edd1a..5343e0f 100644 >> --- a/net/bluetooth/hci_event.c >> +++ b/net/bluetooth/hci_event.c >> @@ -579,7 +579,7 @@ static inline void hci_cs_create_conn(struct hci_dev *hdev, __u8 status) >>                } >>        } else { >>                if (!conn) { >> -                       conn = hci_conn_add(hdev, ACL_LINK, &cp->bdaddr); >> +                       conn = hci_conn_add(hdev, ACL_LINK, 0, &cp->bdaddr); >>                        if (conn) { >>                                conn->out = 1; >>                                conn->link_mode |= HCI_LM_MASTER; >> @@ -964,7 +964,9 @@ static inline void hci_conn_request_evt(struct hci_dev *hdev, struct sk_buff *sk >> >>                conn = hci_conn_hash_lookup_ba(hdev, ev->link_type, &ev->bdaddr); >>                if (!conn) { >> -                       if (!(conn = hci_conn_add(hdev, ev->link_type, &ev->bdaddr))) { >> +                       /* pkt_type not yet used for incoming connections */ >> +                       if (!(conn = hci_conn_add(hdev, ev->link_type, 0, >> +                                                       &ev->bdaddr))) { >>                                BT_ERR("No memmory for new connection"); >>                                hci_dev_unlock(hdev); >>                                return; >> diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c >> index d056886..b342f06 100644 >> --- a/net/bluetooth/l2cap.c >> +++ b/net/bluetooth/l2cap.c >> @@ -955,7 +955,7 @@ static int l2cap_do_connect(struct sock *sk) >>                } >>        } >> >> -       hcon = hci_connect(hdev, ACL_LINK, dst, >> +       hcon = hci_connect(hdev, ACL_LINK, 0, dst, >>                                        l2cap_pi(sk)->sec_level, auth_type); >>        if (!hcon) >>                goto done; >> diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c >> index 77f4153..4976ad5 100644 >> --- a/net/bluetooth/sco.c >> +++ b/net/bluetooth/sco.c >> @@ -176,6 +176,7 @@ static int sco_connect(struct sock *sk) >>  { >>        bdaddr_t *src = &bt_sk(sk)->src; >>        bdaddr_t *dst = &bt_sk(sk)->dst; >> +       __u16 pkt_type = sco_pi(sk)->pkt_type; >>        struct sco_conn *conn; >>        struct hci_conn *hcon; >>        struct hci_dev  *hdev; >> @@ -192,10 +193,14 @@ static int sco_connect(struct sock *sk) >> >>        if (lmp_esco_capable(hdev) && !disable_esco) >>                type = ESCO_LINK; >> -       else >> +       else { >>                type = SCO_LINK; >> +               pkt_type &= SCO_ESCO_MASK; >> +               pkt_type |= EDR_ESCO_MASK; >> +       } >> >> -       hcon = hci_connect(hdev, type, dst, BT_SECURITY_LOW, HCI_AT_NO_BONDING); >> +       hcon = hci_connect(hdev, type, pkt_type, dst, >> +                                       BT_SECURITY_LOW, HCI_AT_NO_BONDING); >>        if (!hcon) >>                goto done; >> >> @@ -451,18 +456,22 @@ static int sco_sock_create(struct net *net, struct socket *sock, int protocol) >>        return 0; >>  } >> >> -static int sco_sock_bind(struct socket *sock, struct sockaddr *addr, int addr_len) >> +static int sco_sock_bind(struct socket *sock, struct sockaddr *addr, int alen) >>  { >> -       struct sockaddr_sco *sa = (struct sockaddr_sco *) addr; >> +       struct sockaddr_sco sa; >>        struct sock *sk = sock->sk; >> -       bdaddr_t *src = &sa->sco_bdaddr; >> -       int err = 0; >> +       bdaddr_t *src = &sa.sco_bdaddr; >> +       int len, err = 0; >> >> -       BT_DBG("sk %p %s", sk, batostr(&sa->sco_bdaddr)); >> +       BT_DBG("sk %p %s", sk, batostr(&sa.sco_bdaddr)); >> >>        if (!addr || addr->sa_family != AF_BLUETOOTH) >>                return -EINVAL; >> >> +       memset(&sa, 0, sizeof(sa)); >> +       len = min_t(unsigned int, sizeof(sa), alen); >> +       memcpy(&sa, addr, len); >> + >>        lock_sock(sk); >> >>        if (sk->sk_state != BT_OPEN) { >> @@ -476,7 +485,8 @@ static int sco_sock_bind(struct socket *sock, struct sockaddr *addr, int addr_le >>                err = -EADDRINUSE; >>        } else { >>                /* Save source address */ >> -               bacpy(&bt_sk(sk)->src, &sa->sco_bdaddr); >> +               bacpy(&bt_sk(sk)->src, &sa.sco_bdaddr); >> +               sco_pi(sk)->pkt_type = sa.sco_pkt_type; >>                sk->sk_state = BT_BOUND; >>        } >> >> @@ -489,26 +499,34 @@ done: >> >>  static int sco_sock_connect(struct socket *sock, struct sockaddr *addr, int alen, int flags) >>  { >> -       struct sockaddr_sco *sa = (struct sockaddr_sco *) addr; >>        struct sock *sk = sock->sk; >> -       int err = 0; >> - >> +       struct sockaddr_sco sa; >> +       int len, err = 0; >> >>        BT_DBG("sk %p", sk); >> >> -       if (addr->sa_family != AF_BLUETOOTH || alen < sizeof(struct sockaddr_sco)) >> +       if (!addr || addr->sa_family != AF_BLUETOOTH) >>                return -EINVAL; >> >> -       if (sk->sk_state != BT_OPEN && sk->sk_state != BT_BOUND) >> -               return -EBADFD; >> - >> -       if (sk->sk_type != SOCK_SEQPACKET) >> -               return -EINVAL; >> +       memset(&sa, 0, sizeof(sa)); >> +       len = min_t(unsigned int, sizeof(sa), alen); >> +       memcpy(&sa, addr, len); >> >>        lock_sock(sk); >> >> +       if (sk->sk_type != SOCK_SEQPACKET) { >> +               err = -EINVAL; >> +               goto done; >> +       } >> + >> +       if (sk->sk_state != BT_OPEN && sk->sk_state != BT_BOUND) { >> +               err = -EBADFD; >> +               goto done; >> +       } >> + >>        /* Set destination address and psm */ >> -       bacpy(&bt_sk(sk)->dst, &sa->sco_bdaddr); >> +       bacpy(&bt_sk(sk)->dst, &sa.sco_bdaddr); >> +       sco_pi(sk)->pkt_type = sa.sco_pkt_type; >> >>        if ((err = sco_connect(sk))) >>                goto done; >> @@ -610,10 +628,13 @@ static int sco_sock_getname(struct socket *sock, struct sockaddr *addr, int *len >>        addr->sa_family = AF_BLUETOOTH; >>        *len = sizeof(struct sockaddr_sco); >> >> -       if (peer) >> +       if (peer) { >>                bacpy(&sa->sco_bdaddr, &bt_sk(sk)->dst); >> -       else >> +               sa->sco_pkt_type = sco_pi(sk)->pkt_type; >> +       } else { >>                bacpy(&sa->sco_bdaddr, &bt_sk(sk)->src); >> +               sa->sco_pkt_type = sco_pi(sk)->pkt_type; >> +       } >> >>        return 0; >>  } >> -- >> 1.6.5.3 >> >> > > BTW i'm happy to switch the logic on the EDR bits, such that 1 is > always 'allow this packet type'. I stuck with the Bluetooth SIG > convention in this patch because the logic actually came out cleanly. > > Either way, its confusing for userspace, and we should document it > carefully in the patch to userspace headers. Ping. As a first step, can we get consensus on the userspace API: --- a/include/net/bluetooth/hci.h +++ b/include/net/bluetooth/hci.h @@ -139,8 +139,11 @@ enum { +#define ALL_SCO_PKTS   (SCO_ESCO_MASK | EDR_ESCO_MASK) +#define ALL_ESCO_PKTS  (SCO_ESCO_MASK | ESCO_EV3 | ESCO_EV4 | ESCO_EV5) --- a/include/net/bluetooth/sco.h +++ b/include/net/bluetooth/sco.h @@ -37,6 +37,7 @@ struct sockaddr_sco {        sa_family_t     sco_family;        bdaddr_t        sco_bdaddr; +       __u16           sco_pkt_type; }; This will at least unblock my work. Nick