diff mbox series

[v2,2/2] Bluetooth: MGMT: Fix not generating command complete for MGMT_OP_DISCONNECT

Message ID 20240826202518.524007-2-luiz.dentz@gmail.com (mailing list archive)
State Superseded
Headers show
Series [v2,1/2] Bluetooth: hci_sync: Introduce hci_cmd_sync_run/hci_cmd_sync_run_once | expand

Checks

Context Check Description
tedd_an/pre-ci_am success Success
tedd_an/CheckPatch warning WARNING: Please use correct Fixes: style 'Fixes: <12 chars of sha1> ("<title line>")' - ie: 'Fixes: ("Bluetooth: Move check for MGMT_CONNECTED flag into mgmt.c")' #110: Fixes: 12d4a3b ("Bluetooth: Move check for MGMT_CONNECTED flag into mgmt.c") total: 0 errors, 1 warnings, 0 checks, 125 lines checked NOTE: For some of the reported defects, checkpatch may be able to mechanically convert to the typical style using --fix or --fix-inplace. /github/workspace/src/src/13778386.patch has style problems, please review. NOTE: Ignored message types: UNKNOWN_COMMIT_ID NOTE: If any of the errors are false positives, please report them to the maintainer, see CHECKPATCH in MAINTAINERS. Use of uninitialized value $cid in concatenation (.) or string at /github/workspace/src/src/scripts/checkpatch.pl line 3247.
tedd_an/GitLint fail WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search 1: T1 Title exceeds max length (84>80): "[v2,2/2] Bluetooth: MGMT: Fix not generating command complete for MGMT_OP_DISCONNECT"
tedd_an/SubjectPrefix success Gitlint PASS

Commit Message

Luiz Augusto von Dentz Aug. 26, 2024, 8:25 p.m. UTC 
From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

MGMT_OP_DISCONNECT can be called while mgmt_device_connected has not
been called yet, which will cause the connection procedure to be
aborted, so mgmt_device_disconnected shall still respond with command
complete to MGMT_OP_DISCONNECT and just not emit
MGMT_EV_DEVICE_DISCONNECTED since MGMT_EV_DEVICE_CONNECTED was never
sent.

To fix this MGMT_OP_DISCONNECT is changed to work similarly to other
command which do use hci_cmd_sync_queue and then use hci_conn_abort to
disconnect and returns the result, in order for hci_conn_abort to be
used from hci_cmd_sync context it now uses hci_cmd_sync_run_once.

Link: https://github.com/bluez/bluez/issues/932
Fixes: 12d4a3b ("Bluetooth: Move check for MGMT_CONNECTED flag into mgmt.c")
Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
---
 net/bluetooth/hci_conn.c |  6 +++-
 net/bluetooth/mgmt.c     | 72 +++++++++++++++++++++++-----------------
 2 files changed, 47 insertions(+), 31 deletions(-)

Comments

kernel test robot Aug. 27, 2024, 4:28 a.m. UTC | #1 
Hi Luiz,

kernel test robot noticed the following build warnings:

[auto build test WARNING on bluetooth-next/master]
[also build test WARNING on bluetooth/master linus/master v6.11-rc5]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Luiz-Augusto-von-Dentz/Bluetooth-MGMT-Fix-not-generating-command-complete-for-MGMT_OP_DISCONNECT/20240827-042615
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git master
patch link:    https://lore.kernel.org/r/20240826202518.524007-2-luiz.dentz%40gmail.com
patch subject: [PATCH v2 2/2] Bluetooth: MGMT: Fix not generating command complete for MGMT_OP_DISCONNECT
config: sh-allmodconfig (https://download.01.org/0day-ci/archive/20240827/202408271234.sY3VKVIg-lkp@intel.com/config)
compiler: sh4-linux-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240827/202408271234.sY3VKVIg-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202408271234.sY3VKVIg-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> net/bluetooth/mgmt.c:9709:13: warning: 'disconnect_rsp' defined but not used [-Wunused-function]
    9709 | static void disconnect_rsp(struct mgmt_pending_cmd *cmd, void *data)
         |             ^~~~~~~~~~~~~~


vim +/disconnect_rsp +9709 net/bluetooth/mgmt.c

f7520543ab4034 Johan Hedberg 2011-01-20  9708  
3b0602cd01a571 Johan Hedberg 2015-03-06 @9709  static void disconnect_rsp(struct mgmt_pending_cmd *cmd, void *data)
8962ee74be48df Johan Hedberg 2011-01-20  9710  {
8962ee74be48df Johan Hedberg 2011-01-20  9711  	struct sock **sk = data;
8962ee74be48df Johan Hedberg 2011-01-20  9712  
f5818c2241247c Johan Hedberg 2014-12-05  9713  	cmd->cmd_complete(cmd, 0);
8962ee74be48df Johan Hedberg 2011-01-20  9714  
8962ee74be48df Johan Hedberg 2011-01-20  9715  	*sk = cmd->sk;
8962ee74be48df Johan Hedberg 2011-01-20  9716  	sock_hold(*sk);
8962ee74be48df Johan Hedberg 2011-01-20  9717  
a664b5bc77fbc8 Johan Hedberg 2011-02-19  9718  	mgmt_pending_remove(cmd);
8962ee74be48df Johan Hedberg 2011-01-20  9719  }
8962ee74be48df Johan Hedberg 2011-01-20  9720
Paul Menzel Aug. 27, 2024, 8:19 a.m. UTC | #2 
Dear Luiz,


Thank you for the patch. Two minor comments.

Am 26.08.24 um 22:25 schrieb Luiz Augusto von Dentz:
> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> 
> MGMT_OP_DISCONNECT can be called while mgmt_device_connected has not
> been called yet, which will cause the connection procedure to be
> aborted, so mgmt_device_disconnected shall still respond with command
> complete to MGMT_OP_DISCONNECT and just not emit
> MGMT_EV_DEVICE_DISCONNECTED since MGMT_EV_DEVICE_CONNECTED was never
> sent.
> 
> To fix this MGMT_OP_DISCONNECT is changed to work similarly to other
> command which do use hci_cmd_sync_queue and then use hci_conn_abort to
> disconnect and returns the result, in order for hci_conn_abort to be
> used from hci_cmd_sync context it now uses hci_cmd_sync_run_once.
> 
> Link: https://github.com/bluez/bluez/issues/932
> Fixes: 12d4a3b ("Bluetooth: Move check for MGMT_CONNECTED flag into mgmt.c")
> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> ---
>   net/bluetooth/hci_conn.c |  6 +++-
>   net/bluetooth/mgmt.c     | 72 +++++++++++++++++++++++-----------------
>   2 files changed, 47 insertions(+), 31 deletions(-)
> 
> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> index 8f0c9322eadb..6225f13177c3 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -2951,5 +2951,9 @@ int hci_abort_conn(struct hci_conn *conn, u8 reason)
>   		return 0;
>   	}
>   
> -	return hci_cmd_sync_queue_once(hdev, abort_conn_sync, conn, NULL);
> +	/* Run immediately if on cmd_sync_work since it maybe called from
> +	 * as a result to MGMT_OP_DISCONNECT and MGMT_OP_UNPAIR which does

I am unable to parse it from “since …”.

> +	 * already queue its callback on cmd_sync_work.
> +	 */
> +	return hci_cmd_sync_run_once(hdev, abort_conn_sync, conn, NULL);
>   }
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 25979f4283a6..54dc9976abcf 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -2921,7 +2921,12 @@ static int unpair_device_sync(struct hci_dev *hdev, void *data)
>   	if (!conn)
>   		return 0;
>   
> -	return hci_abort_conn_sync(hdev, conn, HCI_ERROR_REMOTE_USER_TERM);
> +	/* Disregard any possible error since the likes of hci_abort_conn_sync
> +	 * will cleanup the connection no matter the error.

The verb *clean up* is spelled with a space.


Kind regards,

Paul


> +	 */
> +	hci_abort_conn(conn, HCI_ERROR_REMOTE_USER_TERM);
> +
> +	return 0;
>   }
>   
>   static int unpair_device(struct sock *sk, struct hci_dev *hdev, void *data,
> @@ -3053,13 +3058,44 @@ static int unpair_device(struct sock *sk, struct hci_dev *hdev, void *data,
>   	return err;
>   }
>   
> +static void disconnect_complete(struct hci_dev *hdev, void *data, int err)
> +{
> +	struct mgmt_pending_cmd *cmd = data;
> +
> +	cmd->cmd_complete(cmd, mgmt_status(err));
> +	mgmt_pending_free(cmd);
> +}
> +
> +static int disconnect_sync(struct hci_dev *hdev, void *data)
> +{
> +	struct mgmt_pending_cmd *cmd = data;
> +	struct mgmt_cp_disconnect *cp = cmd->param;
> +	struct hci_conn *conn;
> +
> +	if (cp->addr.type == BDADDR_BREDR)
> +		conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK,
> +					       &cp->addr.bdaddr);
> +	else
> +		conn = hci_conn_hash_lookup_le(hdev, &cp->addr.bdaddr,
> +					       le_addr_type(cp->addr.type));
> +
> +	if (!conn)
> +		return -ENOTCONN;
> +
> +	/* Disregard any possible error since the likes of hci_abort_conn_sync
> +	 * will cleanup the connection no matter the error.
> +	 */
> +	hci_abort_conn(conn, HCI_ERROR_REMOTE_USER_TERM);
> +
> +	return 0;
> +}
> +
>   static int disconnect(struct sock *sk, struct hci_dev *hdev, void *data,
>   		      u16 len)
>   {
>   	struct mgmt_cp_disconnect *cp = data;
>   	struct mgmt_rp_disconnect rp;
>   	struct mgmt_pending_cmd *cmd;
> -	struct hci_conn *conn;
>   	int err;
>   
>   	bt_dev_dbg(hdev, "sock %p", sk);
> @@ -3082,27 +3118,7 @@ static int disconnect(struct sock *sk, struct hci_dev *hdev, void *data,
>   		goto failed;
>   	}
>   
> -	if (pending_find(MGMT_OP_DISCONNECT, hdev)) {
> -		err = mgmt_cmd_complete(sk, hdev->id, MGMT_OP_DISCONNECT,
> -					MGMT_STATUS_BUSY, &rp, sizeof(rp));
> -		goto failed;
> -	}
> -
> -	if (cp->addr.type == BDADDR_BREDR)
> -		conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK,
> -					       &cp->addr.bdaddr);
> -	else
> -		conn = hci_conn_hash_lookup_le(hdev, &cp->addr.bdaddr,
> -					       le_addr_type(cp->addr.type));
> -
> -	if (!conn || conn->state == BT_OPEN || conn->state == BT_CLOSED) {
> -		err = mgmt_cmd_complete(sk, hdev->id, MGMT_OP_DISCONNECT,
> -					MGMT_STATUS_NOT_CONNECTED, &rp,
> -					sizeof(rp));
> -		goto failed;
> -	}
> -
> -	cmd = mgmt_pending_add(sk, MGMT_OP_DISCONNECT, hdev, data, len);
> +	cmd = mgmt_pending_new(sk, MGMT_OP_DISCONNECT, hdev, data, len);
>   	if (!cmd) {
>   		err = -ENOMEM;
>   		goto failed;
> @@ -3110,9 +3126,10 @@ static int disconnect(struct sock *sk, struct hci_dev *hdev, void *data,
>   
>   	cmd->cmd_complete = generic_cmd_complete;
>   
> -	err = hci_disconnect(conn, HCI_ERROR_REMOTE_USER_TERM);
> +	err = hci_cmd_sync_queue(hdev, disconnect_sync, cmd,
> +				 disconnect_complete);
>   	if (err < 0)
> -		mgmt_pending_remove(cmd);
> +		mgmt_pending_free(cmd);
>   
>   failed:
>   	hci_dev_unlock(hdev);
> @@ -9744,8 +9761,6 @@ void mgmt_device_disconnected(struct hci_dev *hdev, bdaddr_t *bdaddr,
>   	if (link_type != ACL_LINK && link_type != LE_LINK)
>   		return;
>   
> -	mgmt_pending_foreach(MGMT_OP_DISCONNECT, hdev, disconnect_rsp, &sk);
> -
>   	bacpy(&ev.addr.bdaddr, bdaddr);
>   	ev.addr.type = link_to_bdaddr(link_type, addr_type);
>   	ev.reason = reason;
> @@ -9758,9 +9773,6 @@ void mgmt_device_disconnected(struct hci_dev *hdev, bdaddr_t *bdaddr,
>   
>   	if (sk)
>   		sock_put(sk);
> -
> -	mgmt_pending_foreach(MGMT_OP_UNPAIR_DEVICE, hdev, unpair_device_rsp,
> -			     hdev);
>   }
>   
>   void mgmt_disconnect_failed(struct hci_dev *hdev, bdaddr_t *bdaddr,
diff mbox series

Patch

diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 8f0c9322eadb..6225f13177c3 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -2951,5 +2951,9 @@  int hci_abort_conn(struct hci_conn *conn, u8 reason)
 		return 0;
 	}
 
-	return hci_cmd_sync_queue_once(hdev, abort_conn_sync, conn, NULL);
+	/* Run immediately if on cmd_sync_work since it maybe called from
+	 * as a result to MGMT_OP_DISCONNECT and MGMT_OP_UNPAIR which does
+	 * already queue its callback on cmd_sync_work.
+	 */
+	return hci_cmd_sync_run_once(hdev, abort_conn_sync, conn, NULL);
 }
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 25979f4283a6..54dc9976abcf 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -2921,7 +2921,12 @@  static int unpair_device_sync(struct hci_dev *hdev, void *data)
 	if (!conn)
 		return 0;
 
-	return hci_abort_conn_sync(hdev, conn, HCI_ERROR_REMOTE_USER_TERM);
+	/* Disregard any possible error since the likes of hci_abort_conn_sync
+	 * will cleanup the connection no matter the error.
+	 */
+	hci_abort_conn(conn, HCI_ERROR_REMOTE_USER_TERM);
+
+	return 0;
 }
 
 static int unpair_device(struct sock *sk, struct hci_dev *hdev, void *data,
@@ -3053,13 +3058,44 @@  static int unpair_device(struct sock *sk, struct hci_dev *hdev, void *data,
 	return err;
 }
 
+static void disconnect_complete(struct hci_dev *hdev, void *data, int err)
+{
+	struct mgmt_pending_cmd *cmd = data;
+
+	cmd->cmd_complete(cmd, mgmt_status(err));
+	mgmt_pending_free(cmd);
+}
+
+static int disconnect_sync(struct hci_dev *hdev, void *data)
+{
+	struct mgmt_pending_cmd *cmd = data;
+	struct mgmt_cp_disconnect *cp = cmd->param;
+	struct hci_conn *conn;
+
+	if (cp->addr.type == BDADDR_BREDR)
+		conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK,
+					       &cp->addr.bdaddr);
+	else
+		conn = hci_conn_hash_lookup_le(hdev, &cp->addr.bdaddr,
+					       le_addr_type(cp->addr.type));
+
+	if (!conn)
+		return -ENOTCONN;
+
+	/* Disregard any possible error since the likes of hci_abort_conn_sync
+	 * will cleanup the connection no matter the error.
+	 */
+	hci_abort_conn(conn, HCI_ERROR_REMOTE_USER_TERM);
+
+	return 0;
+}
+
 static int disconnect(struct sock *sk, struct hci_dev *hdev, void *data,
 		      u16 len)
 {
 	struct mgmt_cp_disconnect *cp = data;
 	struct mgmt_rp_disconnect rp;
 	struct mgmt_pending_cmd *cmd;
-	struct hci_conn *conn;
 	int err;
 
 	bt_dev_dbg(hdev, "sock %p", sk);
@@ -3082,27 +3118,7 @@  static int disconnect(struct sock *sk, struct hci_dev *hdev, void *data,
 		goto failed;
 	}
 
-	if (pending_find(MGMT_OP_DISCONNECT, hdev)) {
-		err = mgmt_cmd_complete(sk, hdev->id, MGMT_OP_DISCONNECT,
-					MGMT_STATUS_BUSY, &rp, sizeof(rp));
-		goto failed;
-	}
-
-	if (cp->addr.type == BDADDR_BREDR)
-		conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK,
-					       &cp->addr.bdaddr);
-	else
-		conn = hci_conn_hash_lookup_le(hdev, &cp->addr.bdaddr,
-					       le_addr_type(cp->addr.type));
-
-	if (!conn || conn->state == BT_OPEN || conn->state == BT_CLOSED) {
-		err = mgmt_cmd_complete(sk, hdev->id, MGMT_OP_DISCONNECT,
-					MGMT_STATUS_NOT_CONNECTED, &rp,
-					sizeof(rp));
-		goto failed;
-	}
-
-	cmd = mgmt_pending_add(sk, MGMT_OP_DISCONNECT, hdev, data, len);
+	cmd = mgmt_pending_new(sk, MGMT_OP_DISCONNECT, hdev, data, len);
 	if (!cmd) {
 		err = -ENOMEM;
 		goto failed;
@@ -3110,9 +3126,10 @@  static int disconnect(struct sock *sk, struct hci_dev *hdev, void *data,
 
 	cmd->cmd_complete = generic_cmd_complete;
 
-	err = hci_disconnect(conn, HCI_ERROR_REMOTE_USER_TERM);
+	err = hci_cmd_sync_queue(hdev, disconnect_sync, cmd,
+				 disconnect_complete);
 	if (err < 0)
-		mgmt_pending_remove(cmd);
+		mgmt_pending_free(cmd);
 
 failed:
 	hci_dev_unlock(hdev);
@@ -9744,8 +9761,6 @@  void mgmt_device_disconnected(struct hci_dev *hdev, bdaddr_t *bdaddr,
 	if (link_type != ACL_LINK && link_type != LE_LINK)
 		return;
 
-	mgmt_pending_foreach(MGMT_OP_DISCONNECT, hdev, disconnect_rsp, &sk);
-
 	bacpy(&ev.addr.bdaddr, bdaddr);
 	ev.addr.type = link_to_bdaddr(link_type, addr_type);
 	ev.reason = reason;
@@ -9758,9 +9773,6 @@  void mgmt_device_disconnected(struct hci_dev *hdev, bdaddr_t *bdaddr,
 
 	if (sk)
 		sock_put(sk);
-
-	mgmt_pending_foreach(MGMT_OP_UNPAIR_DEVICE, hdev, unpair_device_rsp,
-			     hdev);
 }
 
 void mgmt_disconnect_failed(struct hci_dev *hdev, bdaddr_t *bdaddr,