| Message ID | 20240108183938.468426-1-verdre@v0yd.nl |
|---|---|
| Headers | show |
| Series | Bluetooth: Improve retrying of connection attempts | expand |
Hi Jonas, On Mon, Jan 8, 2024 at 1:39 PM Jonas Dreßler <verdre@v0yd.nl> 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 >
Since commit 4c67bc74f016 ("[Bluetooth] Support concurrent connect requests"), the kernel supports trying to connect again in case the bluetooth card is busy and fails to connect. The logic that should handle this became a bit spotty over time, and also cards these days appear to fail with more errors than just "Command Disallowed". This series refactores the handling of concurrent connection requests by serializing all "Create Connection" commands for ACL connections similar to how we do it for LE connections. --- v1: https://lore.kernel.org/linux-bluetooth/20240102185933.64179-1-verdre@v0yd.nl/ v2: - Move to using hci_sync queue for "Create Connection" and therefore always serialize those requests. - Follow commit message style better and properly cite patches Jonas Dreßler (4): Bluetooth: Remove superfluous call to hci_conn_check_pending() Bluetooth: hci_event: Use HCI error defines instead of magic values Bluetooth: hci_conn: Only do ACL connections sequentially Bluetooth: Remove pending ACL connection attempts include/net/bluetooth/hci.h | 3 ++ include/net/bluetooth/hci_core.h | 1 - net/bluetooth/hci_conn.c | 51 ++++++++++++++++---------------- net/bluetooth/hci_event.c | 29 +++++------------- 4 files changed, 35 insertions(+), 49 deletions(-)