shared/mgmt: Add request timeout handling
Luiz Augusto von Dentz
maintainerPATCH v3 1/2 shared/mgmt: Add request timeout handling
From: Luiz Augusto von Dentz <[email protected]>
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(-)
src/shared/mgmt.c +73 -12
@@ -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 indexreturn 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 indexreturn 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;
src/shared/mgmt.h +10 -0
@@ -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);
Signature · 1 line
2.34.1
Luiz Augusto von Dentz
maintainerPATCH v3 2/2 adapter: Remove custom MGMT send/reply timeout
From: Luiz Augusto von Dentz <[email protected]>
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.
--- src/adapter.c | 167 ++++++-------------------------------------------- 1 file changed, 19 insertions(+), 148 deletions(-)
src/adapter.c +19 -148
@@ -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 ; }
Signature · 1 line
2.34.1
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
Hi,
Quoted 27 lines · On Sun, Jan 30, 2022 at 3:31 AM <[email protected]> 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
Quoted 3 lines
---
Regards,
Linux Bluetooth
Applied.
Signature · 1 line
Luiz Augusto von Dentz