* [RFC 1/2] shared/mgmt: Add request timeout handling
@ 2022-01-25 1:04 Luiz Augusto von Dentz
2022-01-25 1:04 ` [RFC 2/2] adapter: Remove custom MGMT send/reply timeout Luiz Augusto von Dentz
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Luiz Augusto von Dentz @ 2022-01-25 1:04 UTC (permalink / raw)
To: linux-bluetooth
From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
This adds request timeout handling, by default the timeout is set to 2
seconds but in case the user want a different timeout it can use
mgmt_send_timeout to set a different value.
---
src/shared/mgmt.c | 70 +++++++++++++++++++++++++++++++++++++++++------
src/shared/mgmt.h | 5 ++++
2 files changed, 67 insertions(+), 8 deletions(-)
diff --git a/src/shared/mgmt.c b/src/shared/mgmt.c
index 41457b430..291a2c636 100644
--- a/src/shared/mgmt.c
+++ b/src/shared/mgmt.c
@@ -25,6 +25,9 @@
#include "src/shared/queue.h"
#include "src/shared/util.h"
#include "src/shared/mgmt.h"
+#include "src/shared/timeout.h"
+
+#define MGMT_TIMEOUT 2
struct mgmt {
int ref_count;
@@ -49,6 +52,7 @@ struct mgmt {
};
struct mgmt_request {
+ struct mgmt *mgmt;
unsigned int id;
uint16_t opcode;
uint16_t index;
@@ -57,6 +61,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 +87,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 +159,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 +198,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 +601,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 +634,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 +777,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 +789,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 +809,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, MGMT_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,
@@ -777,7 +829,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,
+ MGMT_TIMEOUT);
if (!request)
return 0;
@@ -803,7 +856,8 @@ 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,
+ MGMT_TIMEOUT);
if (!request)
return 0;
diff --git a/src/shared/mgmt.h b/src/shared/mgmt.h
index 56add613d..e4f1a7934 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,
--
2.34.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [RFC 2/2] adapter: Remove custom MGMT send/reply timeout
2022-01-25 1:04 [RFC 1/2] shared/mgmt: Add request timeout handling Luiz Augusto von Dentz
@ 2022-01-25 1:04 ` Luiz Augusto von Dentz
2022-01-25 4:40 ` [RFC,1/2] shared/mgmt: Add request timeout handling bluez.test.bot
2022-01-25 21:38 ` [RFC 1/2] " Marcel Holtmann
2 siblings, 0 replies; 5+ messages in thread
From: Luiz Augusto von Dentz @ 2022-01-25 1:04 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 | 160 +++-----------------------------------------------
1 file changed, 9 insertions(+), 151 deletions(-)
diff --git a/src/adapter.c b/src/adapter.c
index 42a9256a4..89b70e88a 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -318,15 +318,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 */
@@ -4141,21 +4132,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)
{
@@ -4167,11 +4143,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);
}
@@ -4244,27 +4215,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(adapter->mgmt, MGMT_OP_LOAD_LONG_TERM_KEYS,
+ adapter->dev_id, cp_size, cp, load_ltks_complete,
+ adapter, NULL))
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,
@@ -5617,15 +5574,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);
@@ -6753,21 +6701,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)
{
@@ -6777,13 +6710,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);
}
@@ -6797,49 +6726,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,
+ if (!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) {
+ confirm_name_complete, adapter, NULL))
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,
@@ -8113,21 +8009,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)
{
@@ -8137,13 +8018,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
@@ -8171,12 +8045,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);
@@ -8208,10 +8076,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",
@@ -8220,16 +8088,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: [RFC,1/2] shared/mgmt: Add request timeout handling
2022-01-25 1:04 [RFC 1/2] shared/mgmt: Add request timeout handling Luiz Augusto von Dentz
2022-01-25 1:04 ` [RFC 2/2] adapter: Remove custom MGMT send/reply timeout Luiz Augusto von Dentz
@ 2022-01-25 4:40 ` bluez.test.bot
2022-01-25 21:38 ` [RFC 1/2] " Marcel Holtmann
2 siblings, 0 replies; 5+ messages in thread
From: bluez.test.bot @ 2022-01-25 4:40 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=608129
---Test result---
Test Summary:
CheckPatch PASS 2.97 seconds
GitLint PASS 2.03 seconds
Prep - Setup ELL PASS 42.90 seconds
Build - Prep PASS 0.72 seconds
Build - Configure PASS 8.71 seconds
Build - Make PASS 1414.03 seconds
Make Check PASS 11.53 seconds
Make Check w/Valgrind PASS 446.01 seconds
Make Distcheck PASS 233.35 seconds
Build w/ext ELL - Configure PASS 8.72 seconds
Build w/ext ELL - Make PASS 1423.32 seconds
Incremental Build with patchesPASS 2825.31 seconds
---
Regards,
Linux Bluetooth
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFC 1/2] shared/mgmt: Add request timeout handling
2022-01-25 1:04 [RFC 1/2] shared/mgmt: Add request timeout handling Luiz Augusto von Dentz
2022-01-25 1:04 ` [RFC 2/2] adapter: Remove custom MGMT send/reply timeout Luiz Augusto von Dentz
2022-01-25 4:40 ` [RFC,1/2] shared/mgmt: Add request timeout handling bluez.test.bot
@ 2022-01-25 21:38 ` Marcel Holtmann
2022-01-25 22:20 ` Luiz Augusto von Dentz
2 siblings, 1 reply; 5+ messages in thread
From: Marcel Holtmann @ 2022-01-25 21:38 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth
Hi Luiz,
> This adds request timeout handling, by default the timeout is set to 2
> seconds but in case the user want a different timeout it can use
> mgmt_send_timeout to set a different value.
> ---
> src/shared/mgmt.c | 70 +++++++++++++++++++++++++++++++++++++++++------
> src/shared/mgmt.h | 5 ++++
> 2 files changed, 67 insertions(+), 8 deletions(-)
>
> diff --git a/src/shared/mgmt.c b/src/shared/mgmt.c
> index 41457b430..291a2c636 100644
> --- a/src/shared/mgmt.c
> +++ b/src/shared/mgmt.c
> @@ -25,6 +25,9 @@
> #include "src/shared/queue.h"
> #include "src/shared/util.h"
> #include "src/shared/mgmt.h"
> +#include "src/shared/timeout.h"
> +
> +#define MGMT_TIMEOUT 2
>
> struct mgmt {
> int ref_count;
> @@ -49,6 +52,7 @@ struct mgmt {
> };
>
> struct mgmt_request {
> + struct mgmt *mgmt;
> unsigned int id;
> uint16_t opcode;
> uint16_t index;
> @@ -57,6 +61,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 +87,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 +159,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 +198,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 +601,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 +634,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 +777,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 +789,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 +809,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, MGMT_TIMEOUT);
> +}
> +
I am not happy with this. No every command has to setup a timeout handler and so on. This is not really what should be done since the mgmt communication with the kernel is actually pretty low latency.
If you want to add a special send_timeout, then do that, but leave the send alone.
Regards
Marcel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFC 1/2] shared/mgmt: Add request timeout handling
2022-01-25 21:38 ` [RFC 1/2] " Marcel Holtmann
@ 2022-01-25 22:20 ` Luiz Augusto von Dentz
0 siblings, 0 replies; 5+ messages in thread
From: Luiz Augusto von Dentz @ 2022-01-25 22:20 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth
Hi Marcel,
On Tue, Jan 25, 2022 at 1:38 PM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Luiz,
>
> > This adds request timeout handling, by default the timeout is set to 2
> > seconds but in case the user want a different timeout it can use
> > mgmt_send_timeout to set a different value.
> > ---
> > src/shared/mgmt.c | 70 +++++++++++++++++++++++++++++++++++++++++------
> > src/shared/mgmt.h | 5 ++++
> > 2 files changed, 67 insertions(+), 8 deletions(-)
> >
> > diff --git a/src/shared/mgmt.c b/src/shared/mgmt.c
> > index 41457b430..291a2c636 100644
> > --- a/src/shared/mgmt.c
> > +++ b/src/shared/mgmt.c
> > @@ -25,6 +25,9 @@
> > #include "src/shared/queue.h"
> > #include "src/shared/util.h"
> > #include "src/shared/mgmt.h"
> > +#include "src/shared/timeout.h"
> > +
> > +#define MGMT_TIMEOUT 2
> >
> > struct mgmt {
> > int ref_count;
> > @@ -49,6 +52,7 @@ struct mgmt {
> > };
> >
> > struct mgmt_request {
> > + struct mgmt *mgmt;
> > unsigned int id;
> > uint16_t opcode;
> > uint16_t index;
> > @@ -57,6 +61,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 +87,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 +159,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 +198,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 +601,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 +634,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 +777,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 +789,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 +809,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, MGMT_TIMEOUT);
> > +}
> > +
>
> I am not happy with this. No every command has to setup a timeout handler and so on. This is not really what should be done since the mgmt communication with the kernel is actually pretty low latency.
Well we don't have any way to check if the kernel is really responding
within a reasonable time, so I thought it would be a good idea to have
a default to capture when a command /reply does not respond, otherwise
it is pretty hard to debug such conditions since the request will stay
in the queue indefinitely.
> If you want to add a special send_timeout, then do that, but leave the send alone.
>
> Regards
>
> Marcel
>
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-01-25 22:20 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-25 1:04 [RFC 1/2] shared/mgmt: Add request timeout handling Luiz Augusto von Dentz
2022-01-25 1:04 ` [RFC 2/2] adapter: Remove custom MGMT send/reply timeout Luiz Augusto von Dentz
2022-01-25 4:40 ` [RFC,1/2] shared/mgmt: Add request timeout handling bluez.test.bot
2022-01-25 21:38 ` [RFC 1/2] " Marcel Holtmann
2022-01-25 22:20 ` Luiz Augusto von Dentz
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.