| Message ID | 20200821072155.18834-1-sonnysasaka@chromium.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [BlueZ] shared/mgmt: Provide timeout in mgmt_send() | expand |
Hi Sonny, On Fri, Aug 21, 2020 at 12:26 AM Sonny Sasaka <sonnysasaka@chromium.org> wrote: > > This patch adds mgmt_send_with_timeout() to help callers who want to use > mgmt_send with timeout handling. Ideally, all mgmt_send() caller should > use timeout, but in this patch only UNPAIR_DEVICE mgmt command is known > to have no-reply problem in the kernel. > > This prevents bluetoothd from getting stuck in a state of waiting for > kernel MGMT reply forever. We still need to fix the kernel side for the > missing reply. > > --- > src/adapter.c | 8 +++--- > src/shared/mgmt.c | 62 ++++++++++++++++++++++++++++++++++++++++++++--- > src/shared/mgmt.h | 6 +++++ > 3 files changed, 69 insertions(+), 7 deletions(-) > > diff --git a/src/adapter.c b/src/adapter.c > index 5e896a9f0..787d1bbb6 100644 > --- a/src/adapter.c > +++ b/src/adapter.c > @@ -114,6 +114,8 @@ static const struct mgmt_blocked_key_info blocked_keys[] = { > 0x22, 0x8e, 0x07, 0x56, 0xb4, 0xe8, 0x5f, 0x01}}, > }; > > +#define DEFAULT_MGMT_TIMEOUT 2 /* Timeout for MGMT commands (secs) */ > + > static DBusConnection *dbus_conn = NULL; > > static bool kernel_conn_control = false; > @@ -7217,9 +7219,9 @@ int btd_adapter_remove_bonding(struct btd_adapter *adapter, > cp.addr.type = bdaddr_type; > cp.disconnect = 1; > > - if (mgmt_send(adapter->mgmt, MGMT_OP_UNPAIR_DEVICE, > - adapter->dev_id, sizeof(cp), &cp, > - NULL, NULL, NULL) > 0) > + if (mgmt_send_with_timeout(adapter->mgmt, MGMT_OP_UNPAIR_DEVICE, > + adapter->dev_id, sizeof(cp), &cp, NULL, > + NULL, NULL, DEFAULT_MGMT_TIMEOUT) > 0) > return 0; > > return -EIO; > diff --git a/src/shared/mgmt.c b/src/shared/mgmt.c > index 277e361a6..7e70aba43 100644 > --- a/src/shared/mgmt.c > +++ b/src/shared/mgmt.c > @@ -38,6 +38,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; > @@ -60,6 +61,11 @@ struct mgmt { > void *debug_data; > }; > > +struct mgmt_pending_request { > + unsigned int request_id; > + struct mgmt *mgmt; > +}; > + > struct mgmt_request { > unsigned int id; > uint16_t opcode; > @@ -69,6 +75,8 @@ struct mgmt_request { > mgmt_request_func_t callback; > mgmt_destroy_func_t destroy; > void *user_data; > + /* Timeout for the request if > zero, otherwise timer is not used. */ > + int timeout_seconds; > }; > > struct mgmt_notify { > @@ -157,6 +165,8 @@ static void write_watch_destroy(void *user_data) > mgmt->writer_active = false; > } > > +bool mgmt_pending_timeout(void *user_data); > + > static bool send_request(struct mgmt *mgmt, struct mgmt_request *request) > { > struct iovec iov; > @@ -184,6 +194,14 @@ static bool send_request(struct mgmt *mgmt, struct mgmt_request *request) > mgmt->debug_data); > > queue_push_tail(mgmt->pending_list, request); > + if (request->timeout_seconds > 0) { > + struct mgmt_pending_request *pending_request = > + malloc(sizeof(struct mgmt_pending_request)); Use new0 instead of malloc. > + pending_request->request_id = request->id; > + pending_request->mgmt = mgmt; > + timeout_add(request->timeout_seconds * 1000, > + mgmt_pending_timeout, pending_request, NULL); You might need to attach the pending request to the request itself so if it is cancel/complete you will need to free the pointer. > + } > > return true; > } > @@ -267,6 +285,29 @@ static void request_complete(struct mgmt *mgmt, uint8_t status, > wakeup_writer(mgmt); > } > > +bool mgmt_pending_timeout(void *user_data) > +{ > + struct mgmt_pending_request *pending_request = user_data; > + struct mgmt *mgmt = pending_request->mgmt; > + const struct mgmt_request *request = > + queue_find(mgmt->pending_list, > + match_request_id, > + UINT_TO_PTR( > + pending_request->request_id)); Make the request aware of the timeout so it can cancel it when it is cancelled/complete it just cancel the timeout. > + free(pending_request); > + > + if (!request) > + return false; > + > + /* Pretend that kernel has replied with TIMEOUT status. */ > + request_complete(mgmt, MGMT_STATUS_TIMEOUT, request->opcode, > + request->index, /* length */ 0, > + /* param */ NULL); > + > + return false; > +} > + > struct event_index { > uint16_t event; > uint16_t index; > @@ -571,10 +612,12 @@ static struct mgmt_request *create_request(uint16_t opcode, uint16_t index, > return request; > } > > -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_with_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_seconds) > { > struct mgmt_request *request; > > @@ -590,6 +633,7 @@ unsigned int mgmt_send(struct mgmt *mgmt, uint16_t opcode, uint16_t index, > mgmt->next_request_id = 1; > > request->id = mgmt->next_request_id++; > + request->timeout_seconds = timeout_seconds; > > if (!queue_push_tail(mgmt->request_queue, request)) { > free(request->buf); > @@ -602,6 +646,16 @@ 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_with_timeout( > + mgmt, opcode, index, length, param, callback, user_data, > + destroy, /* timeout_seconds */ 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, > diff --git a/src/shared/mgmt.h b/src/shared/mgmt.h > index 7caeb3844..9aea64010 100644 > --- a/src/shared/mgmt.h > +++ b/src/shared/mgmt.h > @@ -46,6 +46,12 @@ bool mgmt_set_close_on_unref(struct mgmt *mgmt, bool do_close); > typedef void (*mgmt_request_func_t)(uint8_t status, uint16_t length, > const void *param, void *user_data); > > +unsigned int mgmt_send_with_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_seconds); > unsigned int mgmt_send(struct mgmt *mgmt, uint16_t opcode, uint16_t index, > uint16_t length, const void *param, > mgmt_request_func_t callback, > -- > 2.26.2 >
diff --git a/src/adapter.c b/src/adapter.c index 5e896a9f0..787d1bbb6 100644 --- a/src/adapter.c +++ b/src/adapter.c @@ -114,6 +114,8 @@ static const struct mgmt_blocked_key_info blocked_keys[] = { 0x22, 0x8e, 0x07, 0x56, 0xb4, 0xe8, 0x5f, 0x01}}, }; +#define DEFAULT_MGMT_TIMEOUT 2 /* Timeout for MGMT commands (secs) */ + static DBusConnection *dbus_conn = NULL; static bool kernel_conn_control = false; @@ -7217,9 +7219,9 @@ int btd_adapter_remove_bonding(struct btd_adapter *adapter, cp.addr.type = bdaddr_type; cp.disconnect = 1; - if (mgmt_send(adapter->mgmt, MGMT_OP_UNPAIR_DEVICE, - adapter->dev_id, sizeof(cp), &cp, - NULL, NULL, NULL) > 0) + if (mgmt_send_with_timeout(adapter->mgmt, MGMT_OP_UNPAIR_DEVICE, + adapter->dev_id, sizeof(cp), &cp, NULL, + NULL, NULL, DEFAULT_MGMT_TIMEOUT) > 0) return 0; return -EIO; diff --git a/src/shared/mgmt.c b/src/shared/mgmt.c index 277e361a6..7e70aba43 100644 --- a/src/shared/mgmt.c +++ b/src/shared/mgmt.c @@ -38,6 +38,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; @@ -60,6 +61,11 @@ struct mgmt { void *debug_data; }; +struct mgmt_pending_request { + unsigned int request_id; + struct mgmt *mgmt; +}; + struct mgmt_request { unsigned int id; uint16_t opcode; @@ -69,6 +75,8 @@ struct mgmt_request { mgmt_request_func_t callback; mgmt_destroy_func_t destroy; void *user_data; + /* Timeout for the request if > zero, otherwise timer is not used. */ + int timeout_seconds; }; struct mgmt_notify { @@ -157,6 +165,8 @@ static void write_watch_destroy(void *user_data) mgmt->writer_active = false; } +bool mgmt_pending_timeout(void *user_data); + static bool send_request(struct mgmt *mgmt, struct mgmt_request *request) { struct iovec iov; @@ -184,6 +194,14 @@ static bool send_request(struct mgmt *mgmt, struct mgmt_request *request) mgmt->debug_data); queue_push_tail(mgmt->pending_list, request); + if (request->timeout_seconds > 0) { + struct mgmt_pending_request *pending_request = + malloc(sizeof(struct mgmt_pending_request)); + pending_request->request_id = request->id; + pending_request->mgmt = mgmt; + timeout_add(request->timeout_seconds * 1000, + mgmt_pending_timeout, pending_request, NULL); + } return true; } @@ -267,6 +285,29 @@ static void request_complete(struct mgmt *mgmt, uint8_t status, wakeup_writer(mgmt); } +bool mgmt_pending_timeout(void *user_data) +{ + struct mgmt_pending_request *pending_request = user_data; + struct mgmt *mgmt = pending_request->mgmt; + const struct mgmt_request *request = + queue_find(mgmt->pending_list, + match_request_id, + UINT_TO_PTR( + pending_request->request_id)); + + free(pending_request); + + if (!request) + return false; + + /* Pretend that kernel has replied with TIMEOUT status. */ + request_complete(mgmt, MGMT_STATUS_TIMEOUT, request->opcode, + request->index, /* length */ 0, + /* param */ NULL); + + return false; +} + struct event_index { uint16_t event; uint16_t index; @@ -571,10 +612,12 @@ static struct mgmt_request *create_request(uint16_t opcode, uint16_t index, return request; } -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_with_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_seconds) { struct mgmt_request *request; @@ -590,6 +633,7 @@ unsigned int mgmt_send(struct mgmt *mgmt, uint16_t opcode, uint16_t index, mgmt->next_request_id = 1; request->id = mgmt->next_request_id++; + request->timeout_seconds = timeout_seconds; if (!queue_push_tail(mgmt->request_queue, request)) { free(request->buf); @@ -602,6 +646,16 @@ 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_with_timeout( + mgmt, opcode, index, length, param, callback, user_data, + destroy, /* timeout_seconds */ 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, diff --git a/src/shared/mgmt.h b/src/shared/mgmt.h index 7caeb3844..9aea64010 100644 --- a/src/shared/mgmt.h +++ b/src/shared/mgmt.h @@ -46,6 +46,12 @@ bool mgmt_set_close_on_unref(struct mgmt *mgmt, bool do_close); typedef void (*mgmt_request_func_t)(uint8_t status, uint16_t length, const void *param, void *user_data); +unsigned int mgmt_send_with_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_seconds); unsigned int mgmt_send(struct mgmt *mgmt, uint16_t opcode, uint16_t index, uint16_t length, const void *param, mgmt_request_func_t callback,