* [PATCH v2 1/2] Bluetooth: hci_conn: Fix not cleaning up on LE Connection failure @ 2023-03-27 20:53 Luiz Augusto von Dentz 2023-03-27 20:53 ` [PATCH v2 2/2] Bluetooth: Fix printing errors if LE Connection times out Luiz Augusto von Dentz ` (2 more replies) 0 siblings, 3 replies; 5+ messages in thread From: Luiz Augusto von Dentz @ 2023-03-27 20:53 UTC (permalink / raw) To: linux-bluetooth From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> hci_connect_le_scan_cleanup shall always be invoked to cleanup the states and re-enable passive scanning if necessary, otherwise it may cause the pending action to stay active causing multiple attempts to connect. Fixes: 9b3628d79b46 ("Bluetooth: hci_sync: Cleanup hci_conn if it cannot be aborted") Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> --- net/bluetooth/hci_conn.c | 52 +++++++++++++++++++--------------------- 1 file changed, 24 insertions(+), 28 deletions(-) diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c index 17b946f9ba31..5af3f6b011c9 100644 --- a/net/bluetooth/hci_conn.c +++ b/net/bluetooth/hci_conn.c @@ -68,7 +68,7 @@ static const struct sco_param esco_param_msbc[] = { }; /* This function requires the caller holds hdev->lock */ -static void hci_connect_le_scan_cleanup(struct hci_conn *conn) +static void hci_connect_le_scan_cleanup(struct hci_conn *conn, u8 status) { struct hci_conn_params *params; struct hci_dev *hdev = conn->hdev; @@ -88,9 +88,28 @@ static void hci_connect_le_scan_cleanup(struct hci_conn *conn) params = hci_pend_le_action_lookup(&hdev->pend_le_conns, bdaddr, bdaddr_type); - if (!params || !params->explicit_connect) + if (!params) return; + if (params->conn) { + hci_conn_drop(params->conn); + hci_conn_put(params->conn); + params->conn = NULL; + } + + if (!params->explicit_connect) + return; + + /* If the status indicates successful cancellation of + * the attempt (i.e. Unknown Connection Id) there's no point of + * notifying failure since we'll go back to keep trying to + * connect. The only exception is explicit connect requests + * where a timeout + cancel does indicate an actual failure. + */ + if (status && status != HCI_ERROR_UNKNOWN_CONN_ID) + mgmt_connect_failed(hdev, &conn->dst, conn->type, + conn->dst_type, status); + /* The connection attempt was doing scan for new RPA, and is * in scan phase. If params are not associated with any other * autoconnect action, remove them completely. If they are, just unmark @@ -178,7 +197,7 @@ static void le_scan_cleanup(struct work_struct *work) rcu_read_unlock(); if (c == conn) { - hci_connect_le_scan_cleanup(conn); + hci_connect_le_scan_cleanup(conn, 0x00); hci_conn_cleanup(conn); } @@ -1179,31 +1198,8 @@ EXPORT_SYMBOL(hci_get_route); static void hci_le_conn_failed(struct hci_conn *conn, u8 status) { struct hci_dev *hdev = conn->hdev; - struct hci_conn_params *params; - params = hci_pend_le_action_lookup(&hdev->pend_le_conns, &conn->dst, - conn->dst_type); - if (params && params->conn) { - hci_conn_drop(params->conn); - hci_conn_put(params->conn); - params->conn = NULL; - } - - /* If the status indicates successful cancellation of - * the attempt (i.e. Unknown Connection Id) there's no point of - * notifying failure since we'll go back to keep trying to - * connect. The only exception is explicit connect requests - * where a timeout + cancel does indicate an actual failure. - */ - if (status != HCI_ERROR_UNKNOWN_CONN_ID || - (params && params->explicit_connect)) - mgmt_connect_failed(hdev, &conn->dst, conn->type, - conn->dst_type, status); - - /* Since we may have temporarily stopped the background scanning in - * favor of connection establishment, we should restart it. - */ - hci_update_passive_scan(hdev); + hci_connect_le_scan_cleanup(conn, status); /* Enable advertising in case this was a failed connection * attempt as a peripheral. @@ -1240,7 +1236,7 @@ static void create_le_conn_complete(struct hci_dev *hdev, void *data, int err) hci_dev_lock(hdev); if (!err) { - hci_connect_le_scan_cleanup(conn); + hci_connect_le_scan_cleanup(conn, 0x00); goto done; } -- 2.39.2 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH v2 2/2] Bluetooth: Fix printing errors if LE Connection times out 2023-03-27 20:53 [PATCH v2 1/2] Bluetooth: hci_conn: Fix not cleaning up on LE Connection failure Luiz Augusto von Dentz @ 2023-03-27 20:53 ` Luiz Augusto von Dentz 2023-03-29 8:58 ` Ahmad Fatoum 2023-03-27 21:38 ` [v2,1/2] Bluetooth: hci_conn: Fix not cleaning up on LE Connection failure bluez.test.bot 2023-03-28 20:30 ` [PATCH v2 1/2] " patchwork-bot+bluetooth 2 siblings, 1 reply; 5+ messages in thread From: Luiz Augusto von Dentz @ 2023-03-27 20:53 UTC (permalink / raw) To: linux-bluetooth From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> This fixes errors like bellow when LE Connection times out since that is actually not a controller error: Bluetooth: hci0: Opcode 0x200d failed: -110 Bluetooth: hci0: request failed to create LE connection: err -110 Instead the code shall properly detect if -ETIMEDOUT is returned and send HCI_OP_LE_CREATE_CONN_CANCEL to give up on the connection. Link: https://github.com/bluez/bluez/issues/340 Fixes: Fixes: 8e8b92ee60de ("Bluetooth: hci_sync: Add hci_le_create_conn_sync") Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> --- include/net/bluetooth/hci_core.h | 1 + net/bluetooth/hci_conn.c | 7 +++++-- net/bluetooth/hci_event.c | 16 ++++++---------- net/bluetooth/hci_sync.c | 13 ++++++++++--- 4 files changed, 22 insertions(+), 15 deletions(-) diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h index 53d3328c2b8b..e22e45fbe8db 100644 --- a/include/net/bluetooth/hci_core.h +++ b/include/net/bluetooth/hci_core.h @@ -954,6 +954,7 @@ enum { HCI_CONN_STK_ENCRYPT, HCI_CONN_AUTH_INITIATOR, HCI_CONN_DROP, + HCI_CONN_CANCEL, HCI_CONN_PARAM_REMOVAL_PEND, HCI_CONN_NEW_LINK_KEY, HCI_CONN_SCANNING, diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c index 5af3f6b011c9..e4aee5950c36 100644 --- a/net/bluetooth/hci_conn.c +++ b/net/bluetooth/hci_conn.c @@ -1233,6 +1233,8 @@ static void create_le_conn_complete(struct hci_dev *hdev, void *data, int err) { struct hci_conn *conn = data; + bt_dev_dbg(hdev, "err %d", err); + hci_dev_lock(hdev); if (!err) { @@ -1240,8 +1242,6 @@ static void create_le_conn_complete(struct hci_dev *hdev, void *data, int err) goto done; } - bt_dev_err(hdev, "request failed to create LE connection: err %d", err); - /* Check if connection is still pending */ if (conn != hci_lookup_le_connect(hdev)) goto done; @@ -2771,6 +2771,9 @@ int hci_abort_conn(struct hci_conn *conn, u8 reason) { int r = 0; + if (test_and_set_bit(HCI_CONN_CANCEL, &conn->flags)) + return 0; + switch (conn->state) { case BT_CONNECTED: case BT_CONFIG: diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c index 8d8547fa9032..ff99e8c2750f 100644 --- a/net/bluetooth/hci_event.c +++ b/net/bluetooth/hci_event.c @@ -2886,16 +2886,6 @@ static void cs_le_create_conn(struct hci_dev *hdev, bdaddr_t *peer_addr, conn->resp_addr_type = peer_addr_type; bacpy(&conn->resp_addr, peer_addr); - - /* We don't want the connection attempt to stick around - * indefinitely since LE doesn't have a page timeout concept - * like BR/EDR. Set a timer for any connection that doesn't use - * the accept list for connecting. - */ - if (filter_policy == HCI_LE_USE_PEER_ADDR) - queue_delayed_work(conn->hdev->workqueue, - &conn->le_conn_timeout, - conn->conn_timeout); } static void hci_cs_le_create_conn(struct hci_dev *hdev, u8 status) @@ -5907,6 +5897,12 @@ static void le_conn_complete_evt(struct hci_dev *hdev, u8 status, if (status) goto unlock; + /* Drop the connection if it has been aborted */ + if (test_bit(HCI_CONN_CANCEL, &conn->flags)) { + hci_conn_drop(conn); + goto unlock; + } + if (conn->dst_type == ADDR_LE_DEV_PUBLIC) addr_type = BDADDR_LE_PUBLIC; else diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c index 4376b6f06702..31231f0e4a28 100644 --- a/net/bluetooth/hci_sync.c +++ b/net/bluetooth/hci_sync.c @@ -246,8 +246,9 @@ int __hci_cmd_sync_status_sk(struct hci_dev *hdev, u16 opcode, u32 plen, skb = __hci_cmd_sync_sk(hdev, opcode, plen, param, event, timeout, sk); if (IS_ERR(skb)) { - bt_dev_err(hdev, "Opcode 0x%4x failed: %ld", opcode, - PTR_ERR(skb)); + if (!event) + bt_dev_err(hdev, "Opcode 0x%4x failed: %ld", opcode, + PTR_ERR(skb)); return PTR_ERR(skb); } @@ -5128,8 +5129,11 @@ static int hci_le_connect_cancel_sync(struct hci_dev *hdev, if (test_bit(HCI_CONN_SCANNING, &conn->flags)) return 0; + if (test_and_set_bit(HCI_CONN_CANCEL, &conn->flags)) + return 0; + return __hci_cmd_sync_status(hdev, HCI_OP_LE_CREATE_CONN_CANCEL, - 6, &conn->dst, HCI_CMD_TIMEOUT); + 0, NULL, HCI_CMD_TIMEOUT); } static int hci_connect_cancel_sync(struct hci_dev *hdev, struct hci_conn *conn) @@ -6103,6 +6107,9 @@ int hci_le_create_conn_sync(struct hci_dev *hdev, struct hci_conn *conn) conn->conn_timeout, NULL); done: + if (err == -ETIMEDOUT) + hci_le_connect_cancel_sync(hdev, conn); + /* Re-enable advertising after the connection attempt is finished. */ hci_resume_advertising_sync(hdev); return err; -- 2.39.2 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2 2/2] Bluetooth: Fix printing errors if LE Connection times out 2023-03-27 20:53 ` [PATCH v2 2/2] Bluetooth: Fix printing errors if LE Connection times out Luiz Augusto von Dentz @ 2023-03-29 8:58 ` Ahmad Fatoum 0 siblings, 0 replies; 5+ messages in thread From: Ahmad Fatoum @ 2023-03-29 8:58 UTC (permalink / raw) To: Luiz Augusto von Dentz, linux-bluetooth; +Cc: Pengutronix Kernel Team Hello Luiz, On 27.03.23 22:53, Luiz Augusto von Dentz wrote: > From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > > This fixes errors like bellow when LE Connection times out since that > is actually not a controller error: > > Bluetooth: hci0: Opcode 0x200d failed: -110 > Bluetooth: hci0: request failed to create LE connection: err -110 > > Instead the code shall properly detect if -ETIMEDOUT is returned and > send HCI_OP_LE_CREATE_CONN_CANCEL to give up on the connection. > > Link: https://github.com/bluez/bluez/issues/340 I assume the issue described in the Github PR to be the same issue I had reported here: https://lore.kernel.org/linux-bluetooth/a1ce1743-e450-6cdb-dfab-56a3e3eb9aed@pengutronix.de/ I gave these patches a test and all pairing attempts after the first pair/unpair are still unsuccessful. Only visible change to me is that there's no -110 error message printed anymore with default log level. Cheers, Ahmad #regzbot monitor: https://lore.kernel.org/linux-bluetooth/a1ce1743-e450-6cdb-dfab-56a3e3eb9aed@pengutronix.de/ > Fixes: Fixes: 8e8b92ee60de ("Bluetooth: hci_sync: Add hci_le_create_conn_sync") > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > --- > include/net/bluetooth/hci_core.h | 1 + > net/bluetooth/hci_conn.c | 7 +++++-- > net/bluetooth/hci_event.c | 16 ++++++---------- > net/bluetooth/hci_sync.c | 13 ++++++++++--- > 4 files changed, 22 insertions(+), 15 deletions(-) > > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h > index 53d3328c2b8b..e22e45fbe8db 100644 > --- a/include/net/bluetooth/hci_core.h > +++ b/include/net/bluetooth/hci_core.h > @@ -954,6 +954,7 @@ enum { > HCI_CONN_STK_ENCRYPT, > HCI_CONN_AUTH_INITIATOR, > HCI_CONN_DROP, > + HCI_CONN_CANCEL, > HCI_CONN_PARAM_REMOVAL_PEND, > HCI_CONN_NEW_LINK_KEY, > HCI_CONN_SCANNING, > diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c > index 5af3f6b011c9..e4aee5950c36 100644 > --- a/net/bluetooth/hci_conn.c > +++ b/net/bluetooth/hci_conn.c > @@ -1233,6 +1233,8 @@ static void create_le_conn_complete(struct hci_dev *hdev, void *data, int err) > { > struct hci_conn *conn = data; > > + bt_dev_dbg(hdev, "err %d", err); > + > hci_dev_lock(hdev); > > if (!err) { > @@ -1240,8 +1242,6 @@ static void create_le_conn_complete(struct hci_dev *hdev, void *data, int err) > goto done; > } > > - bt_dev_err(hdev, "request failed to create LE connection: err %d", err); > - > /* Check if connection is still pending */ > if (conn != hci_lookup_le_connect(hdev)) > goto done; > @@ -2771,6 +2771,9 @@ int hci_abort_conn(struct hci_conn *conn, u8 reason) > { > int r = 0; > > + if (test_and_set_bit(HCI_CONN_CANCEL, &conn->flags)) > + return 0; > + > switch (conn->state) { > case BT_CONNECTED: > case BT_CONFIG: > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c > index 8d8547fa9032..ff99e8c2750f 100644 > --- a/net/bluetooth/hci_event.c > +++ b/net/bluetooth/hci_event.c > @@ -2886,16 +2886,6 @@ static void cs_le_create_conn(struct hci_dev *hdev, bdaddr_t *peer_addr, > > conn->resp_addr_type = peer_addr_type; > bacpy(&conn->resp_addr, peer_addr); > - > - /* We don't want the connection attempt to stick around > - * indefinitely since LE doesn't have a page timeout concept > - * like BR/EDR. Set a timer for any connection that doesn't use > - * the accept list for connecting. > - */ > - if (filter_policy == HCI_LE_USE_PEER_ADDR) > - queue_delayed_work(conn->hdev->workqueue, > - &conn->le_conn_timeout, > - conn->conn_timeout); > } > > static void hci_cs_le_create_conn(struct hci_dev *hdev, u8 status) > @@ -5907,6 +5897,12 @@ static void le_conn_complete_evt(struct hci_dev *hdev, u8 status, > if (status) > goto unlock; > > + /* Drop the connection if it has been aborted */ > + if (test_bit(HCI_CONN_CANCEL, &conn->flags)) { > + hci_conn_drop(conn); > + goto unlock; > + } > + > if (conn->dst_type == ADDR_LE_DEV_PUBLIC) > addr_type = BDADDR_LE_PUBLIC; > else > diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c > index 4376b6f06702..31231f0e4a28 100644 > --- a/net/bluetooth/hci_sync.c > +++ b/net/bluetooth/hci_sync.c > @@ -246,8 +246,9 @@ int __hci_cmd_sync_status_sk(struct hci_dev *hdev, u16 opcode, u32 plen, > > skb = __hci_cmd_sync_sk(hdev, opcode, plen, param, event, timeout, sk); > if (IS_ERR(skb)) { > - bt_dev_err(hdev, "Opcode 0x%4x failed: %ld", opcode, > - PTR_ERR(skb)); > + if (!event) > + bt_dev_err(hdev, "Opcode 0x%4x failed: %ld", opcode, > + PTR_ERR(skb)); > return PTR_ERR(skb); > } > > @@ -5128,8 +5129,11 @@ static int hci_le_connect_cancel_sync(struct hci_dev *hdev, > if (test_bit(HCI_CONN_SCANNING, &conn->flags)) > return 0; > > + if (test_and_set_bit(HCI_CONN_CANCEL, &conn->flags)) > + return 0; > + > return __hci_cmd_sync_status(hdev, HCI_OP_LE_CREATE_CONN_CANCEL, > - 6, &conn->dst, HCI_CMD_TIMEOUT); > + 0, NULL, HCI_CMD_TIMEOUT); > } > > static int hci_connect_cancel_sync(struct hci_dev *hdev, struct hci_conn *conn) > @@ -6103,6 +6107,9 @@ int hci_le_create_conn_sync(struct hci_dev *hdev, struct hci_conn *conn) > conn->conn_timeout, NULL); > > done: > + if (err == -ETIMEDOUT) > + hci_le_connect_cancel_sync(hdev, conn); > + > /* Re-enable advertising after the connection attempt is finished. */ > hci_resume_advertising_sync(hdev); > return err; -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | ^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [v2,1/2] Bluetooth: hci_conn: Fix not cleaning up on LE Connection failure 2023-03-27 20:53 [PATCH v2 1/2] Bluetooth: hci_conn: Fix not cleaning up on LE Connection failure Luiz Augusto von Dentz 2023-03-27 20:53 ` [PATCH v2 2/2] Bluetooth: Fix printing errors if LE Connection times out Luiz Augusto von Dentz @ 2023-03-27 21:38 ` bluez.test.bot 2023-03-28 20:30 ` [PATCH v2 1/2] " patchwork-bot+bluetooth 2 siblings, 0 replies; 5+ messages in thread From: bluez.test.bot @ 2023-03-27 21:38 UTC (permalink / raw) To: linux-bluetooth, luiz.dentz [-- Attachment #1: Type: text/plain, Size: 1827 bytes --] This is automated email and please do not reply to this email! Dear submitter, Thank you for submitting the patches to the linux bluetooth mailing list. This is a CI test results with your patch series: PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=734342 ---Test result--- Test Summary: CheckPatch PASS 1.92 seconds GitLint PASS 0.50 seconds SubjectPrefix PASS 0.15 seconds BuildKernel PASS 44.56 seconds CheckAllWarning PASS 47.79 seconds CheckSparse WARNING 52.51 seconds CheckSmatch WARNING 139.93 seconds BuildKernel32 PASS 41.19 seconds TestRunnerSetup PASS 588.02 seconds TestRunner_l2cap-tester PASS 19.97 seconds TestRunner_iso-tester PASS 21.20 seconds TestRunner_bnep-tester PASS 6.99 seconds TestRunner_mgmt-tester PASS 131.45 seconds TestRunner_rfcomm-tester PASS 11.30 seconds TestRunner_sco-tester PASS 10.26 seconds TestRunner_ioctl-tester PASS 12.11 seconds TestRunner_mesh-tester PASS 8.99 seconds TestRunner_smp-tester PASS 9.94 seconds TestRunner_userchan-tester PASS 7.70 seconds IncrementalBuild PASS 74.35 seconds Details ############################## Test: CheckSparse - WARNING Desc: Run sparse tool with linux kernel Output: net/bluetooth/hci_event.c: note: in included file (through include/net/bluetooth/hci_core.h): ############################## Test: CheckSmatch - WARNING Desc: Run smatch tool with source Output: net/bluetooth/hci_event.c: note: in included file (through include/net/bluetooth/hci_core.h): --- Regards, Linux Bluetooth ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2 1/2] Bluetooth: hci_conn: Fix not cleaning up on LE Connection failure 2023-03-27 20:53 [PATCH v2 1/2] Bluetooth: hci_conn: Fix not cleaning up on LE Connection failure Luiz Augusto von Dentz 2023-03-27 20:53 ` [PATCH v2 2/2] Bluetooth: Fix printing errors if LE Connection times out Luiz Augusto von Dentz 2023-03-27 21:38 ` [v2,1/2] Bluetooth: hci_conn: Fix not cleaning up on LE Connection failure bluez.test.bot @ 2023-03-28 20:30 ` patchwork-bot+bluetooth 2 siblings, 0 replies; 5+ messages in thread From: patchwork-bot+bluetooth @ 2023-03-28 20:30 UTC (permalink / raw) To: Luiz Augusto von Dentz; +Cc: linux-bluetooth Hello: This series was applied to bluetooth/bluetooth-next.git (master) by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>: On Mon, 27 Mar 2023 13:53:46 -0700 you wrote: > From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > > hci_connect_le_scan_cleanup shall always be invoked to cleanup the > states and re-enable passive scanning if necessary, otherwise it may > cause the pending action to stay active causing multiple attempts to > connect. > > [...] Here is the summary with links: - [v2,1/2] Bluetooth: hci_conn: Fix not cleaning up on LE Connection failure https://git.kernel.org/bluetooth/bluetooth-next/c/501ba6f31a83 - [v2,2/2] Bluetooth: Fix printing errors if LE Connection times out https://git.kernel.org/bluetooth/bluetooth-next/c/2731e038a76d You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-03-29 9:00 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-03-27 20:53 [PATCH v2 1/2] Bluetooth: hci_conn: Fix not cleaning up on LE Connection failure Luiz Augusto von Dentz 2023-03-27 20:53 ` [PATCH v2 2/2] Bluetooth: Fix printing errors if LE Connection times out Luiz Augusto von Dentz 2023-03-29 8:58 ` Ahmad Fatoum 2023-03-27 21:38 ` [v2,1/2] Bluetooth: hci_conn: Fix not cleaning up on LE Connection failure bluez.test.bot 2023-03-28 20:30 ` [PATCH v2 1/2] " patchwork-bot+bluetooth
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.