diff mbox series

[v2,2/2] adapter: Remove custom MGMT send/reply timeout

Message ID 20220128020735.3779202-2-luiz.dentz@gmail.com
State New
Headers show
Series [v2,1/2] shared/mgmt: Add request timeout handling | expand

Commit Message

Luiz Augusto von Dentz Jan. 28, 2022, 2:07 a.m. UTC 
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(-)

Comments

Marcel Holtmann Jan. 28, 2022, 1:37 p.m. UTC | #1 
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
Luiz Augusto von Dentz Jan. 28, 2022, 8:28 p.m. UTC | #2 
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
>
diff mbox series

Patch

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;
 }