From: Luiz Augusto von Dentz <[email protected]>
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 <[email protected]>
---
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
From: Luiz Augusto von Dentz <[email protected]>
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 <[email protected]>
---
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
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
Hello:
This series was applied to bluetooth/bluetooth-next.git (master)
by Luiz Augusto von Dentz <[email protected]>:
On Mon, 27 Mar 2023 13:53:46 -0700 you wrote:
> From: Luiz Augusto von Dentz <[email protected]>
>
> 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
Hello Luiz,
On 27.03.23 22:53, Luiz Augusto von Dentz wrote:
> From: Luiz Augusto von Dentz <[email protected]>
>
> 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/[email protected]/
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/[email protected]/
> Fixes: Fixes: 8e8b92ee60de ("Bluetooth: hci_sync: Add hci_le_create_conn_sync")
> Signed-off-by: Luiz Augusto von Dentz <[email protected]>
> ---
> 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 |