* [PATCH v3 1/2] shared/mgmt: Add request timeout handling
@ 2022-01-28 21:05 Luiz Augusto von Dentz
2022-01-28 21:05 ` [PATCH v3 2/2] adapter: Remove custom MGMT send/reply timeout Luiz Augusto von Dentz
2022-01-30 11:31 ` [v3,1/2] shared/mgmt: Add request timeout handling bluez.test.bot
0 siblings, 2 replies; 4+ messages in thread
From: Luiz Augusto von Dentz @ 2022-01-28 21:05 UTC (permalink / raw)
To: linux-bluetooth
From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
This adds request timeout handling when using mgmt_send_timeout and
mgmt_reply_timeout, the timeout is applied to the request only when it
is actually transmitted and not while queued.
---
v2: Remove default timeout so the likes of mgmt_send/mgmt_reply use 0
instead which disables the timeout handling.
v3: Keep comments explaining why certain requests need a timeout.
src/shared/mgmt.c | 85 ++++++++++++++++++++++++++++++++++++++++-------
src/shared/mgmt.h | 10 ++++++
2 files changed, 83 insertions(+), 12 deletions(-)
diff --git a/src/shared/mgmt.c b/src/shared/mgmt.c
index 41457b430..95229c248 100644
--- a/src/shared/mgmt.c
+++ b/src/shared/mgmt.c
@@ -25,6 +25,7 @@
#include "src/shared/queue.h"
#include "src/shared/util.h"
#include "src/shared/mgmt.h"
+#include "src/shared/timeout.h"
struct mgmt {
int ref_count;
@@ -49,6 +50,7 @@ struct mgmt {
};
struct mgmt_request {
+ struct mgmt *mgmt;
unsigned int id;
uint16_t opcode;
uint16_t index;
@@ -57,6 +59,8 @@ struct mgmt_request {
mgmt_request_func_t callback;
mgmt_destroy_func_t destroy;
void *user_data;
+ int timeout;
+ unsigned int timeout_id;
};
struct mgmt_notify {
@@ -81,6 +85,9 @@ static void destroy_request(void *data)
if (request->destroy)
request->destroy(request->user_data);
+ if (request->timeout_id)
+ timeout_remove(request->timeout_id);
+
free(request->buf);
free(request);
}
@@ -150,6 +157,26 @@ static void write_watch_destroy(void *user_data)
mgmt->writer_active = false;
}
+static bool request_timeout(void *data)
+{
+ struct mgmt_request *request = data;
+
+ if (!request)
+ return false;
+
+ request->timeout_id = 0;
+
+ queue_remove_if(request->mgmt->pending_list, NULL, request);
+
+ if (request->callback)
+ request->callback(MGMT_STATUS_TIMEOUT, 0, NULL,
+ request->user_data);
+
+ destroy_request(request);
+
+ return false;
+}
+
static bool send_request(struct mgmt *mgmt, struct mgmt_request *request)
{
struct iovec iov;
@@ -169,6 +196,12 @@ static bool send_request(struct mgmt *mgmt, struct mgmt_request *request)
return false;
}
+ if (request->timeout)
+ request->timeout_id = timeout_add_seconds(request->timeout,
+ request_timeout,
+ request,
+ NULL);
+
util_debug(mgmt->debug_callback, mgmt->debug_data,
"[0x%04x] command 0x%04x",
request->index, request->opcode);
@@ -566,7 +599,8 @@ bool mgmt_set_close_on_unref(struct mgmt *mgmt, bool do_close)
static struct mgmt_request *create_request(struct mgmt *mgmt, uint16_t opcode,
uint16_t index, uint16_t length,
const void *param, mgmt_request_func_t callback,
- void *user_data, mgmt_destroy_func_t destroy)
+ void *user_data, mgmt_destroy_func_t destroy,
+ int timeout)
{
struct mgmt_request *request;
struct mgmt_hdr *hdr;
@@ -598,12 +632,18 @@ static struct mgmt_request *create_request(struct mgmt *mgmt, uint16_t opcode,
hdr->index = htobs(index);
hdr->len = htobs(length);
+ /* Use a weak reference so requests don't prevent mgmt_unref to
+ * cancel requests and free in case of the last reference is dropped by
+ * the user.
+ */
+ request->mgmt = mgmt;
request->opcode = opcode;
request->index = index;
request->callback = callback;
request->destroy = destroy;
request->user_data = user_data;
+ request->timeout = timeout;
return request;
}
@@ -735,10 +775,11 @@ unsigned int mgmt_send_tlv(struct mgmt *mgmt, uint16_t opcode, uint16_t index,
return ret;
}
-unsigned int mgmt_send(struct mgmt *mgmt, uint16_t opcode, uint16_t index,
- uint16_t length, const void *param,
- mgmt_request_func_t callback,
- void *user_data, mgmt_destroy_func_t destroy)
+unsigned int mgmt_send_timeout(struct mgmt *mgmt, uint16_t opcode,
+ uint16_t index, uint16_t length,
+ const void *param, mgmt_request_func_t callback,
+ void *user_data, mgmt_destroy_func_t destroy,
+ int timeout)
{
struct mgmt_request *request;
@@ -746,7 +787,7 @@ unsigned int mgmt_send(struct mgmt *mgmt, uint16_t opcode, uint16_t index,
return 0;
request = create_request(mgmt, opcode, index, length, param,
- callback, user_data, destroy);
+ callback, user_data, destroy, timeout);
if (!request)
return 0;
@@ -766,6 +807,15 @@ unsigned int mgmt_send(struct mgmt *mgmt, uint16_t opcode, uint16_t index,
return request->id;
}
+unsigned int mgmt_send(struct mgmt *mgmt, uint16_t opcode, uint16_t index,
+ uint16_t length, const void *param,
+ mgmt_request_func_t callback,
+ void *user_data, mgmt_destroy_func_t destroy)
+{
+ return mgmt_send_timeout(mgmt, opcode, index, length, param, callback,
+ user_data, destroy, 0);
+}
+
unsigned int mgmt_send_nowait(struct mgmt *mgmt, uint16_t opcode, uint16_t index,
uint16_t length, const void *param,
mgmt_request_func_t callback,
@@ -777,7 +827,8 @@ unsigned int mgmt_send_nowait(struct mgmt *mgmt, uint16_t opcode, uint16_t index
return 0;
request = create_request(mgmt, opcode, index, length, param,
- callback, user_data, destroy);
+ callback, user_data, destroy,
+ 0);
if (!request)
return 0;
@@ -792,10 +843,11 @@ unsigned int mgmt_send_nowait(struct mgmt *mgmt, uint16_t opcode, uint16_t index
return request->id;
}
-unsigned int mgmt_reply(struct mgmt *mgmt, uint16_t opcode, uint16_t index,
- uint16_t length, const void *param,
- mgmt_request_func_t callback,
- void *user_data, mgmt_destroy_func_t destroy)
+unsigned int mgmt_reply_timeout(struct mgmt *mgmt, uint16_t opcode,
+ uint16_t index, uint16_t length,
+ const void *param, mgmt_request_func_t callback,
+ void *user_data, mgmt_destroy_func_t destroy,
+ int timeout)
{
struct mgmt_request *request;
@@ -803,7 +855,7 @@ unsigned int mgmt_reply(struct mgmt *mgmt, uint16_t opcode, uint16_t index,
return 0;
request = create_request(mgmt, opcode, index, length, param,
- callback, user_data, destroy);
+ callback, user_data, destroy, timeout);
if (!request)
return 0;
@@ -823,6 +875,15 @@ unsigned int mgmt_reply(struct mgmt *mgmt, uint16_t opcode, uint16_t index,
return request->id;
}
+unsigned int mgmt_reply(struct mgmt *mgmt, uint16_t opcode, uint16_t index,
+ uint16_t length, const void *param,
+ mgmt_request_func_t callback,
+ void *user_data, mgmt_destroy_func_t destroy)
+{
+ return mgmt_reply_timeout(mgmt, opcode, index, length, param, callback,
+ user_data, destroy, 0);
+}
+
bool mgmt_cancel(struct mgmt *mgmt, unsigned int id)
{
struct mgmt_request *request;
diff --git a/src/shared/mgmt.h b/src/shared/mgmt.h
index 56add613d..b413cea78 100644
--- a/src/shared/mgmt.h
+++ b/src/shared/mgmt.h
@@ -55,6 +55,11 @@ unsigned int mgmt_send(struct mgmt *mgmt, uint16_t opcode, uint16_t index,
uint16_t length, const void *param,
mgmt_request_func_t callback,
void *user_data, mgmt_destroy_func_t destroy);
+unsigned int mgmt_send_timeout(struct mgmt *mgmt, uint16_t opcode,
+ uint16_t index, uint16_t length,
+ const void *param, mgmt_request_func_t callback,
+ void *user_data, mgmt_destroy_func_t destroy,
+ int timeout);
unsigned int mgmt_send_nowait(struct mgmt *mgmt, uint16_t opcode, uint16_t index,
uint16_t length, const void *param,
mgmt_request_func_t callback,
@@ -63,6 +68,11 @@ unsigned int mgmt_reply(struct mgmt *mgmt, uint16_t opcode, uint16_t index,
uint16_t length, const void *param,
mgmt_request_func_t callback,
void *user_data, mgmt_destroy_func_t destroy);
+unsigned int mgmt_reply_timeout(struct mgmt *mgmt, uint16_t opcode,
+ uint16_t index, uint16_t length,
+ const void *param, mgmt_request_func_t callback,
+ void *user_data, mgmt_destroy_func_t destroy,
+ int timeout);
bool mgmt_cancel(struct mgmt *mgmt, unsigned int id);
bool mgmt_cancel_index(struct mgmt *mgmt, uint16_t index);
bool mgmt_cancel_all(struct mgmt *mgmt);
--
2.34.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH v3 2/2] adapter: Remove custom MGMT send/reply timeout
2022-01-28 21:05 [PATCH v3 1/2] shared/mgmt: Add request timeout handling Luiz Augusto von Dentz
@ 2022-01-28 21:05 ` Luiz Augusto von Dentz
2022-01-30 11:31 ` [v3,1/2] shared/mgmt: Add request timeout handling bluez.test.bot
1 sibling, 0 replies; 4+ messages in thread
From: Luiz Augusto von Dentz @ 2022-01-28 21:05 UTC (permalink / raw)
To: linux-bluetooth
From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
This removes the custom MGMT send/reply timeout since bt_mgmt itself
can handle them itself and it actually start the timer only when the
command is actually sent to the kernel rather then when it is queued.
Fixes: https://github.com/bluez/bluez/issues/275
---
src/adapter.c | 167 ++++++--------------------------------------------
1 file changed, 19 insertions(+), 148 deletions(-)
diff --git a/src/adapter.c b/src/adapter.c
index 9772e843a..bdae10e35 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -311,15 +311,6 @@ struct btd_adapter {
struct oob_handler *oob_handler;
- unsigned int load_ltks_id;
- unsigned int load_ltks_timeout;
-
- unsigned int confirm_name_id;
- unsigned int confirm_name_timeout;
-
- unsigned int pair_device_id;
- unsigned int pair_device_timeout;
-
unsigned int db_id; /* Service event handler for GATT db */
bool is_default; /* true if adapter is default one */
@@ -4134,21 +4125,6 @@ static void load_link_keys(struct btd_adapter *adapter, GSList *keys,
adapter->dev_id);
}
-static bool load_ltks_timeout(gpointer user_data)
-{
- struct btd_adapter *adapter = user_data;
-
- btd_error(adapter->dev_id, "Loading LTKs timed out for hci%u",
- adapter->dev_id);
-
- adapter->load_ltks_timeout = 0;
-
- mgmt_cancel(adapter->mgmt, adapter->load_ltks_id);
- adapter->load_ltks_id = 0;
-
- return FALSE;
-}
-
static void load_ltks_complete(uint8_t status, uint16_t length,
const void *param, void *user_data)
{
@@ -4160,11 +4136,6 @@ static void load_ltks_complete(uint8_t status, uint16_t length,
adapter->dev_id, mgmt_errstr(status), status);
}
- adapter->load_ltks_id = 0;
-
- timeout_remove(adapter->load_ltks_timeout);
- adapter->load_ltks_timeout = 0;
-
DBG("LTKs loaded for hci%u", adapter->dev_id);
}
@@ -4237,27 +4208,18 @@ static void load_ltks(struct btd_adapter *adapter, GSList *keys)
}
}
- adapter->load_ltks_id = mgmt_send(adapter->mgmt,
- MGMT_OP_LOAD_LONG_TERM_KEYS,
- adapter->dev_id, cp_size, cp,
- load_ltks_complete, adapter, NULL);
-
- g_free(cp);
-
- if (adapter->load_ltks_id == 0) {
- btd_error(adapter->dev_id, "Failed to load LTKs for hci%u",
- adapter->dev_id);
- return;
- }
-
/*
* This timeout handling is needed since the kernel is stupid
* and forgets to send a command complete response. However in
* case of failures it does send a command status.
*/
- adapter->load_ltks_timeout = timeout_add_seconds(2,
- load_ltks_timeout, adapter,
- NULL);
+ if (!mgmt_send_timeout(adapter->mgmt, MGMT_OP_LOAD_LONG_TERM_KEYS,
+ adapter->dev_id, cp_size, cp, load_ltks_complete,
+ adapter, NULL, 2))
+ btd_error(adapter->dev_id, "Failed to load LTKs for hci%u",
+ adapter->dev_id);
+
+ g_free(cp);
}
static void load_irks_complete(uint8_t status, uint16_t length,
@@ -5610,15 +5572,6 @@ static void adapter_free(gpointer user_data)
adapter->passive_scan_timeout = 0;
}
- if (adapter->load_ltks_timeout > 0)
- timeout_remove(adapter->load_ltks_timeout);
-
- if (adapter->confirm_name_timeout > 0)
- timeout_remove(adapter->confirm_name_timeout);
-
- if (adapter->pair_device_timeout > 0)
- timeout_remove(adapter->pair_device_timeout);
-
if (adapter->auth_idle_id)
g_source_remove(adapter->auth_idle_id);
@@ -6746,21 +6699,6 @@ const bdaddr_t *btd_adapter_get_address(struct btd_adapter *adapter)
return &adapter->bdaddr;
}
-static bool confirm_name_timeout(gpointer user_data)
-{
- struct btd_adapter *adapter = user_data;
-
- btd_error(adapter->dev_id, "Confirm name timed out for hci%u",
- adapter->dev_id);
-
- adapter->confirm_name_timeout = 0;
-
- mgmt_cancel(adapter->mgmt, adapter->confirm_name_id);
- adapter->confirm_name_id = 0;
-
- return FALSE;
-}
-
static void confirm_name_complete(uint8_t status, uint16_t length,
const void *param, void *user_data)
{
@@ -6770,13 +6708,9 @@ static void confirm_name_complete(uint8_t status, uint16_t length,
btd_error(adapter->dev_id,
"Failed to confirm name for hci%u: %s (0x%02x)",
adapter->dev_id, mgmt_errstr(status), status);
+ return;
}
- adapter->confirm_name_id = 0;
-
- timeout_remove(adapter->confirm_name_timeout);
- adapter->confirm_name_timeout = 0;
-
DBG("Confirm name complete for hci%u", adapter->dev_id);
}
@@ -6790,49 +6724,21 @@ static void confirm_name(struct btd_adapter *adapter, const bdaddr_t *bdaddr,
DBG("hci%d bdaddr %s name_known %u", adapter->dev_id, addr,
name_known);
- /*
- * If the kernel does not answer the confirm name command with
- * a command complete or command status in time, this might
- * race against another device found event that also requires
- * to confirm the name. If there is a pending command, just
- * cancel it to be safe here.
- */
- if (adapter->confirm_name_id > 0) {
- btd_warn(adapter->dev_id,
- "Found pending confirm name for hci%u",
- adapter->dev_id);
- mgmt_cancel(adapter->mgmt, adapter->confirm_name_id);
- }
-
- if (adapter->confirm_name_timeout > 0) {
- timeout_remove(adapter->confirm_name_timeout);
- adapter->confirm_name_timeout = 0;
- }
-
memset(&cp, 0, sizeof(cp));
bacpy(&cp.addr.bdaddr, bdaddr);
cp.addr.type = bdaddr_type;
cp.name_known = name_known;
- adapter->confirm_name_id = mgmt_reply(adapter->mgmt,
- MGMT_OP_CONFIRM_NAME,
- adapter->dev_id, sizeof(cp), &cp,
- confirm_name_complete, adapter, NULL);
-
- if (adapter->confirm_name_id == 0) {
- btd_error(adapter->dev_id, "Failed to confirm name for hci%u",
- adapter->dev_id);
- return;
- }
-
/*
* This timeout handling is needed since the kernel is stupid
* and forgets to send a command complete response. However in
* case of failures it does send a command status.
*/
- adapter->confirm_name_timeout = timeout_add_seconds(2,
- confirm_name_timeout, adapter,
- NULL);
+ if (!mgmt_reply_timeout(adapter->mgmt, MGMT_OP_CONFIRM_NAME,
+ adapter->dev_id, sizeof(cp), &cp,
+ confirm_name_complete, adapter, NULL, 2))
+ btd_error(adapter->dev_id, "Failed to confirm name for hci%u",
+ adapter->dev_id);
}
static void adapter_msd_notify(struct btd_adapter *adapter,
@@ -8106,21 +8012,6 @@ static void free_pair_device_data(void *user_data)
g_free(data);
}
-static bool pair_device_timeout(gpointer user_data)
-{
- struct pair_device_data *data = user_data;
- struct btd_adapter *adapter = data->adapter;
-
- btd_error(adapter->dev_id, "Pair device timed out for hci%u",
- adapter->dev_id);
-
- adapter->pair_device_timeout = 0;
-
- adapter_cancel_bonding(adapter, &data->bdaddr, data->addr_type);
-
- return FALSE;
-}
-
static void pair_device_complete(uint8_t status, uint16_t length,
const void *param, void *user_data)
{
@@ -8130,13 +8021,6 @@ static void pair_device_complete(uint8_t status, uint16_t length,
DBG("%s (0x%02x)", mgmt_errstr(status), status);
- adapter->pair_device_id = 0;
-
- if (adapter->pair_device_timeout > 0) {
- timeout_remove(adapter->pair_device_timeout);
- adapter->pair_device_timeout = 0;
- }
-
/* Workaround for a kernel bug
*
* Broken kernels may reply to device pairing command with command
@@ -8164,12 +8048,6 @@ static void pair_device_complete(uint8_t status, uint16_t length,
int adapter_create_bonding(struct btd_adapter *adapter, const bdaddr_t *bdaddr,
uint8_t addr_type, uint8_t io_cap)
{
- if (adapter->pair_device_id > 0) {
- btd_error(adapter->dev_id,
- "Unable pair since another pairing is in progress");
- return -EBUSY;
- }
-
suspend_discovery(adapter);
return adapter_bonding_attempt(adapter, bdaddr, addr_type, io_cap);
@@ -8201,11 +8079,14 @@ int adapter_bonding_attempt(struct btd_adapter *adapter, const bdaddr_t *bdaddr,
bacpy(&data->bdaddr, bdaddr);
data->addr_type = addr_type;
- id = mgmt_send(adapter->mgmt, MGMT_OP_PAIR_DEVICE,
+ /* Due to a bug in the kernel it is possible that a LE pairing
+ * request never times out. Therefore, add a timer to clean up
+ * if no response arrives
+ */
+ id = mgmt_send_timeout(adapter->mgmt, MGMT_OP_PAIR_DEVICE,
adapter->dev_id, sizeof(cp), &cp,
pair_device_complete, data,
- free_pair_device_data);
-
+ free_pair_device_data, BONDING_TIMEOUT);
if (id == 0) {
btd_error(adapter->dev_id, "Failed to pair %s for hci%u",
addr, adapter->dev_id);
@@ -8213,16 +8094,6 @@ int adapter_bonding_attempt(struct btd_adapter *adapter, const bdaddr_t *bdaddr,
return -EIO;
}
- adapter->pair_device_id = id;
-
- /* Due to a bug in the kernel it is possible that a LE pairing
- * request never times out. Therefore, add a timer to clean up
- * if no response arrives
- */
- adapter->pair_device_timeout = timeout_add_seconds(BONDING_TIMEOUT,
- pair_device_timeout, data,
- NULL);
-
return 0;
}
--
2.34.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* RE: [v3,1/2] shared/mgmt: Add request timeout handling
2022-01-28 21:05 [PATCH v3 1/2] shared/mgmt: Add request timeout handling Luiz Augusto von Dentz
2022-01-28 21:05 ` [PATCH v3 2/2] adapter: Remove custom MGMT send/reply timeout Luiz Augusto von Dentz
@ 2022-01-30 11:31 ` bluez.test.bot
2022-01-31 18:44 ` Luiz Augusto von Dentz
1 sibling, 1 reply; 4+ messages in thread
From: bluez.test.bot @ 2022-01-30 11:31 UTC (permalink / raw)
To: linux-bluetooth, luiz.dentz
[-- Attachment #1: Type: text/plain, Size: 998 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=609643
---Test result---
Test Summary:
CheckPatch PASS 1.31 seconds
GitLint PASS 0.74 seconds
Prep - Setup ELL PASS 49.36 seconds
Build - Prep PASS 0.61 seconds
Build - Configure PASS 9.55 seconds
Build - Make PASS 1706.14 seconds
Make Check PASS 12.84 seconds
Make Check w/Valgrind PASS 515.20 seconds
Make Distcheck PASS 270.46 seconds
Build w/ext ELL - Configure PASS 9.86 seconds
Build w/ext ELL - Make PASS 1689.54 seconds
Incremental Build with patchesPASS 3406.26 seconds
---
Regards,
Linux Bluetooth
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [v3,1/2] shared/mgmt: Add request timeout handling
2022-01-30 11:31 ` [v3,1/2] shared/mgmt: Add request timeout handling bluez.test.bot
@ 2022-01-31 18:44 ` Luiz Augusto von Dentz
0 siblings, 0 replies; 4+ messages in thread
From: Luiz Augusto von Dentz @ 2022-01-31 18:44 UTC (permalink / raw)
To: linux-bluetooth@vger.kernel.org
Hi,
On Sun, Jan 30, 2022 at 3:31 AM <bluez.test.bot@gmail.com> wrote:
>
> 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=609643
>
> ---Test result---
>
> Test Summary:
> CheckPatch PASS 1.31 seconds
> GitLint PASS 0.74 seconds
> Prep - Setup ELL PASS 49.36 seconds
> Build - Prep PASS 0.61 seconds
> Build - Configure PASS 9.55 seconds
> Build - Make PASS 1706.14 seconds
> Make Check PASS 12.84 seconds
> Make Check w/Valgrind PASS 515.20 seconds
> Make Distcheck PASS 270.46 seconds
> Build w/ext ELL - Configure PASS 9.86 seconds
> Build w/ext ELL - Make PASS 1689.54 seconds
> Incremental Build with patchesPASS 3406.26 seconds
>
>
>
> ---
> Regards,
> Linux Bluetooth
Applied.
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-01-31 18:44 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-01-28 21:05 [PATCH v3 1/2] shared/mgmt: Add request timeout handling Luiz Augusto von Dentz
2022-01-28 21:05 ` [PATCH v3 2/2] adapter: Remove custom MGMT send/reply timeout Luiz Augusto von Dentz
2022-01-30 11:31 ` [v3,1/2] shared/mgmt: Add request timeout handling bluez.test.bot
2022-01-31 18:44 ` Luiz Augusto von Dentz
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox