* [PATCH v2 1/2] shared/mgmt: Add request timeout handling
@ 2022-01-28 2:07 Luiz Augusto von Dentz
2022-01-28 2:07 ` [PATCH v2 2/2] adapter: Remove custom MGMT send/reply timeout Luiz Augusto von Dentz
2022-01-28 4:09 ` [v2,1/2] shared/mgmt: Add request timeout handling bluez.test.bot
0 siblings, 2 replies; 5+ messages in thread
From: Luiz Augusto von Dentz @ 2022-01-28 2:07 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.
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] 5+ messages in thread
* [PATCH v2 2/2] adapter: Remove custom MGMT send/reply timeout
2022-01-28 2:07 [PATCH v2 1/2] shared/mgmt: Add request timeout handling Luiz Augusto von Dentz
@ 2022-01-28 2:07 ` Luiz Augusto von Dentz
2022-01-28 13:37 ` Marcel Holtmann
2022-01-28 4:09 ` [v2,1/2] shared/mgmt: Add request timeout handling bluez.test.bot
1 sibling, 1 reply; 5+ messages in thread
From: Luiz Augusto von Dentz @ 2022-01-28 2:07 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 | 162 ++++----------------------------------------------
1 file changed, 10 insertions(+), 152 deletions(-)
diff --git a/src/adapter.c b/src/adapter.c
index 9772e843a..72e98ba0a 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,13 @@ 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) {
+ 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);
- 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);
+ g_free(cp);
}
static void load_irks_complete(uint8_t status, uint16_t length,
@@ -5610,15 +5567,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 +6694,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 +6703,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 +6719,16 @@ 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) {
+ 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);
- 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);
}
static void adapter_msd_notify(struct btd_adapter *adapter,
@@ -8106,21 +8002,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 +8011,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 +8038,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,10 +8069,10 @@ 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,
+ 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",
@@ -8213,16 +8081,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] 5+ messages in thread
* RE: [v2,1/2] shared/mgmt: Add request timeout handling
2022-01-28 2:07 [PATCH v2 1/2] shared/mgmt: Add request timeout handling Luiz Augusto von Dentz
2022-01-28 2:07 ` [PATCH v2 2/2] adapter: Remove custom MGMT send/reply timeout Luiz Augusto von Dentz
@ 2022-01-28 4:09 ` bluez.test.bot
1 sibling, 0 replies; 5+ messages in thread
From: bluez.test.bot @ 2022-01-28 4:09 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=609321
---Test result---
Test Summary:
CheckPatch PASS 3.07 seconds
GitLint PASS 2.06 seconds
Prep - Setup ELL PASS 49.45 seconds
Build - Prep PASS 0.80 seconds
Build - Configure PASS 9.73 seconds
Build - Make PASS 1439.59 seconds
Make Check PASS 12.59 seconds
Make Check w/Valgrind PASS 510.88 seconds
Make Distcheck PASS 265.99 seconds
Build w/ext ELL - Configure PASS 9.90 seconds
Build w/ext ELL - Make PASS 1417.04 seconds
Incremental Build with patchesPASS 2901.91 seconds
---
Regards,
Linux Bluetooth
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2 2/2] adapter: Remove custom MGMT send/reply timeout
2022-01-28 2:07 ` [PATCH v2 2/2] adapter: Remove custom MGMT send/reply timeout Luiz Augusto von Dentz
@ 2022-01-28 13:37 ` Marcel Holtmann
2022-01-28 20:28 ` Luiz Augusto von Dentz
0 siblings, 1 reply; 5+ messages in thread
From: Marcel Holtmann @ 2022-01-28 13:37 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth
Hi Luiz,
> 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 | 162 ++++----------------------------------------------
> 1 file changed, 10 insertions(+), 152 deletions(-)
>
> diff --git a/src/adapter.c b/src/adapter.c
> index 9772e843a..72e98ba0a 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,13 @@ 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) {
> + 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);
> - 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.
> - */
please don’t loose these comments. They are important because of the kernel bugs we had.
Regards
Marcel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2 2/2] adapter: Remove custom MGMT send/reply timeout
2022-01-28 13:37 ` Marcel Holtmann
@ 2022-01-28 20:28 ` Luiz Augusto von Dentz
0 siblings, 0 replies; 5+ messages in thread
From: Luiz Augusto von Dentz @ 2022-01-28 20:28 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth
Hi Marcel,
On Fri, Jan 28, 2022 at 5:37 AM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Luiz,
>
> > 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 | 162 ++++----------------------------------------------
> > 1 file changed, 10 insertions(+), 152 deletions(-)
> >
> > diff --git a/src/adapter.c b/src/adapter.c
> > index 9772e843a..72e98ba0a 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,13 @@ 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) {
> > + 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);
> > - 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.
> > - */
>
> please don’t loose these comments. They are important because of the kernel bugs we had.
Sure I will incorporate them back.
> Regards
>
> Marcel
>
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-01-28 20:28 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-28 2:07 [PATCH v2 1/2] shared/mgmt: Add request timeout handling Luiz Augusto von Dentz
2022-01-28 2:07 ` [PATCH v2 2/2] adapter: Remove custom MGMT send/reply timeout Luiz Augusto von Dentz
2022-01-28 13:37 ` Marcel Holtmann
2022-01-28 20:28 ` Luiz Augusto von Dentz
2022-01-28 4:09 ` [v2,1/2] shared/mgmt: Add request timeout handling bluez.test.bot
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.