Re: [PATCH v2 3/4] Bluetooth: hci_conn: Only do ACL connections sequentially

[Luiz Augusto von Dentz]

Hi Jonas,

On Mon, Jan 8, 2024 at 1:39 PM Jonas Dreßler <verdre@xxxxxxx> wrote:
>
> Pretty much all bluetooth chipsets only support paging a single device at
> a time, and if they don't reject a secondary "Create Connection" request
> while another is still ongoing, they'll most likely serialize those
> requests in the firware.
>
> With commit 4c67bc74f016 ("[Bluetooth] Support concurrent connect
> requests") we started adding some serialization of our own in case the
> adapter returns "Command Disallowed" HCI error.
>
> This commit was using the BT_CONNECT2 state for the serialization, this
> state is also used for a few more things (most notably to indicate we're
> waiting for an inquiry to cancel) and therefore a bit unreliable. Also
> not all BT firwares would respond with "Command Disallowed" on too many
> connection requests, some will also respond with "Hardware Failure"
> (BCM4378), and others will error out later and send a "Connect Complete"
> event with error "Rejected Limited Resources" (Marvell 88W8897).
>
> We can clean things up a bit and also make the serialization more reliable
> by using our hci_sync machinery to always do "Create Connection" requests
> in a sequential manner.
>
> This is very similar to what we're already doing for establishing LE
> connections, and it works well there.
> ---
>  include/net/bluetooth/hci.h |  1 +
>  net/bluetooth/hci_conn.c    | 37 ++++++++++++++++++++++++++-----------
>  2 files changed, 27 insertions(+), 11 deletions(-)
>
> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> index fef723afd..f2bbc0a14 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -427,6 +427,7 @@ enum {
>  #define HCI_ACL_TX_TIMEOUT     msecs_to_jiffies(45000) /* 45 seconds */
>  #define HCI_AUTO_OFF_TIMEOUT   msecs_to_jiffies(2000)  /* 2 seconds */
>  #define HCI_POWER_OFF_TIMEOUT  msecs_to_jiffies(5000)  /* 5 seconds */
> +#define HCI_ACL_CONN_TIMEOUT   msecs_to_jiffies(20000) /* 20 seconds */
>  #define HCI_LE_CONN_TIMEOUT    msecs_to_jiffies(20000) /* 20 seconds */
>  #define HCI_LE_AUTOCONN_TIMEOUT        msecs_to_jiffies(4000)  /* 4 seconds */
>
> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> index 76222565e..541d55301 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -229,11 +229,12 @@ static void hci_connect_le_scan_remove(struct hci_conn *conn)
>         schedule_work(&conn->le_scan_cleanup);
>  }
>
> -static void hci_acl_create_connection(struct hci_conn *conn)
> +static int hci_acl_create_connection_sync(struct hci_dev *hdev, void *data)

Move the above function to hci_sync.c so it is simpler to reuse it in
the future.

>  {
> -       struct hci_dev *hdev = conn->hdev;
> +       struct hci_conn *conn = data;
>         struct inquiry_entry *ie;
>         struct hci_cp_create_conn cp;
> +       int err;
>
>         BT_DBG("hcon %p", conn);
>
> @@ -246,12 +247,10 @@ static void hci_acl_create_connection(struct hci_conn *conn)
>          * request for discovery again when this flag becomes false.
>          */
>         if (test_bit(HCI_INQUIRY, &hdev->flags)) {
> -               /* Put this connection to "pending" state so that it will be
> -                * executed after the inquiry cancel command complete event.
> -                */
> -               conn->state = BT_CONNECT2;
> -               hci_send_cmd(hdev, HCI_OP_INQUIRY_CANCEL, 0, NULL);
> -               return;
> +               err = __hci_cmd_sync_status(hdev, HCI_OP_INQUIRY_CANCEL, 0,
> +                                           NULL, HCI_CMD_TIMEOUT);
> +               if (err)
> +                       bt_dev_warn(hdev, "Failed to cancel inquiry %d", err);
>         }
>
>         conn->state = BT_CONNECT;
> @@ -284,7 +283,15 @@ static void hci_acl_create_connection(struct hci_conn *conn)
>         else
>                 cp.role_switch = 0x00;
>
> -       hci_send_cmd(hdev, HCI_OP_CREATE_CONN, sizeof(cp), &cp);
> +       err = __hci_cmd_sync_status_sk(hdev, HCI_OP_CREATE_CONN,
> +                                      sizeof(cp), &cp,
> +                                      HCI_EV_CONN_COMPLETE,
> +                                      HCI_ACL_CONN_TIMEOUT, NULL);
> +
> +       if (err == -ETIMEDOUT)
> +               hci_abort_conn(conn, HCI_ERROR_LOCAL_HOST_TERM);
> +
> +       return err;
>  }
>
>  int hci_disconnect(struct hci_conn *conn, __u8 reason)
> @@ -1622,10 +1629,18 @@ struct hci_conn *hci_connect_acl(struct hci_dev *hdev, bdaddr_t *dst,
>
>         acl->conn_reason = conn_reason;
>         if (acl->state == BT_OPEN || acl->state == BT_CLOSED) {
> +               int err;
> +
>                 acl->sec_level = BT_SECURITY_LOW;
>                 acl->pending_sec_level = sec_level;
>                 acl->auth_type = auth_type;
> -               hci_acl_create_connection(acl);
> +
> +               err = hci_cmd_sync_queue(hdev, hci_acl_create_connection_sync,
> +                                        acl, NULL);
> +               if (err) {
> +                       hci_conn_del(acl);
> +                       return ERR_PTR(err);
> +               }
>         }
>
>         return acl;
> @@ -2530,7 +2545,7 @@ void hci_conn_check_pending(struct hci_dev *hdev)
>
>         conn = hci_conn_hash_lookup_state(hdev, ACL_LINK, BT_CONNECT2);
>         if (conn)
> -               hci_acl_create_connection(conn);
> +               hci_cmd_sync_queue(hdev, hci_acl_create_connection_sync, conn, NULL);
>
>         hci_dev_unlock(hdev);
>  }
> --
> 2.43.0
>


-- 
Luiz Augusto von Dentz