[PATCH v3 0/4] Disconnect devices before rfkilling adapter

[PATCH v3 0/4] Disconnect devices before rfkilling adapter

[Jonas Dreßler]

Apparently the firmware is supposed to power off the bluetooth card
properly, including disconnecting devices, when we use rfkill to block
bluetooth. This doesn't work on a lot of laptops though, leading to weird
issues after turning off bluetooth, like the connection timing out on the
peripherals which were connected, and bluetooth not connecting properly
when the adapter is turned on again after rfkilling.

This series uses the rfkill hook in the bluetooth subsystem
to execute a few more shutdown commands and make sure that all
devices get disconnected before we close the HCI connection to the adapter.

---

v1: https://lore.kernel.org/linux-bluetooth/20240102133311.6712-1-verdre@v0yd.nl/
v2: https://lore.kernel.org/linux-bluetooth/20240102181946.57288-1-verdre@v0yd.nl/
v3:
 - Update commit message titles to reflect what's actually happening
   (disconnecting devices, not sending a power-off command).
 - Doing the shutdown sequence synchronously instead of async now.
 - Move HCI_RFKILLED flag back again to be set before shutdown.
 - Added a "fallback" hci_dev_do_close() to the error path because
   hci_set_powered_sync() might bail-out early on error.

Jonas Dreßler (4):
  Bluetooth: Remove HCI_POWER_OFF_TIMEOUT
  Bluetooth: mgmt: Remove leftover queuing of power_off work
  Bluetooth: Add new state HCI_POWERING_DOWN
  Bluetooth: Disconnect connected devices before rfkilling adapter

 include/net/bluetooth/hci.h |  2 +-
 net/bluetooth/hci_core.c    | 35 +++++++++++++++++++++++++++++++++--
 net/bluetooth/hci_sync.c    | 16 +++++++++++-----
 net/bluetooth/mgmt.c        | 30 ++++++++++++++----------------
 4 files changed, 59 insertions(+), 24 deletions(-)

-- 
2.43.0

[PATCH v3 1/4] Bluetooth: Remove HCI_POWER_OFF_TIMEOUT

[Jonas Dreßler]

With commit cf75ad8b41d2 ("Bluetooth: hci_sync: Convert MGMT_SET_POWERED"),
the power off sequence got refactored so that this timeout was no longer
necessary, let's remove the leftover define from the header too.

Signed-off-by: Jonas Dreßler <verdre@v0yd.nl>
---
 include/net/bluetooth/hci.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index 111e8f8e5..cf5d6230c 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -426,7 +426,6 @@ enum {
 #define HCI_NCMD_TIMEOUT	msecs_to_jiffies(4000)	/* 4 seconds */
 #define HCI_ACL_TX_TIMEOUT	msecs_to_jiffies(45000)	/* 45 seconds */
 #define HCI_AUTO_OFF_TIMEOUT	msecs_to_jiffies(2000)	/* 2 seconds */
-#define HCI_POWER_OFF_TIMEOUT	msecs_to_jiffies(5000)	/* 5 seconds */
 #define HCI_LE_CONN_TIMEOUT	msecs_to_jiffies(20000)	/* 20 seconds */
 #define HCI_LE_AUTOCONN_TIMEOUT	msecs_to_jiffies(4000)	/* 4 seconds */
 
-- 
2.43.0

[PATCH v3 2/4] Bluetooth: mgmt: Remove leftover queuing of power_off work

[Jonas Dreßler]

Queuing of power_off work was introduced in these functions with commits
8b064a3ad377 ("Bluetooth: Clean up HCI state when doing power off") and
c9910d0fb4fc ("Bluetooth: Fix disconnecting connections in non-connected
states") in an effort to clean up state and do things like disconnecting
devices before actually powering off the device.

After that, commit a3172b7eb4a2 ("Bluetooth: Add timer to force power off")
introduced a timeout to ensure that the device actually got powered off,
even if some of the cleanup work would never complete.

This code later got refactored with commit cf75ad8b41d2 ("Bluetooth:
hci_sync: Convert MGMT_SET_POWERED"), which made powering off the device
synchronous and removed the need for initiating the power_off work from
other places. The timeout mentioned above got removed too, because we now
also made use of the command timeout during power on/off.

These days the power_off work still exists, but it only seems to only be
used for HCI_AUTO_OFF functionality, which is why we never noticed
those two leftover places where we queue power_off work. So let's remove
that code.

Signed-off-by: Jonas Dreßler <verdre@v0yd.nl>
---
 net/bluetooth/mgmt.c | 16 ----------------
 1 file changed, 16 deletions(-)

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index d4498037f..c5291e139 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -9760,14 +9760,6 @@ void mgmt_device_disconnected(struct hci_dev *hdev, bdaddr_t *bdaddr,
 	struct mgmt_ev_device_disconnected ev;
 	struct sock *sk = NULL;
 
-	/* The connection is still in hci_conn_hash so test for 1
-	 * instead of 0 to know if this is the last one.
-	 */
-	if (mgmt_powering_down(hdev) && hci_conn_count(hdev) == 1) {
-		cancel_delayed_work(&hdev->power_off);
-		queue_work(hdev->req_workqueue, &hdev->power_off.work);
-	}
-
 	if (!mgmt_connected)
 		return;
 
@@ -9824,14 +9816,6 @@ void mgmt_connect_failed(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
 {
 	struct mgmt_ev_connect_failed ev;
 
-	/* The connection is still in hci_conn_hash so test for 1
-	 * instead of 0 to know if this is the last one.
-	 */
-	if (mgmt_powering_down(hdev) && hci_conn_count(hdev) == 1) {
-		cancel_delayed_work(&hdev->power_off);
-		queue_work(hdev->req_workqueue, &hdev->power_off.work);
-	}
-
 	bacpy(&ev.addr.bdaddr, bdaddr);
 	ev.addr.type = link_to_bdaddr(link_type, addr_type);
 	ev.status = mgmt_status(status);
-- 
2.43.0

[PATCH v3 3/4] Bluetooth: Add new state HCI_POWERING_DOWN

[Jonas Dreßler]

Add a new state HCI_POWERING_DOWN that indicates that the device is
currently powering down, this will be useful for the next commit.

Signed-off-by: Jonas Dreßler <verdre@v0yd.nl>
---
 include/net/bluetooth/hci.h |  1 +
 net/bluetooth/hci_sync.c    | 16 +++++++++++-----
 net/bluetooth/mgmt.c        | 14 ++++++++++++++
 3 files changed, 26 insertions(+), 5 deletions(-)

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index cf5d6230c..e08afd870 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -361,6 +361,7 @@ enum {
 	HCI_SETUP,
 	HCI_CONFIG,
 	HCI_DEBUGFS_CREATED,
+	HCI_POWERING_DOWN,
 	HCI_AUTO_OFF,
 	HCI_RFKILLED,
 	HCI_MGMT,
diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
index e6eee1808..c920de0a2 100644
--- a/net/bluetooth/hci_sync.c
+++ b/net/bluetooth/hci_sync.c
@@ -5389,27 +5389,33 @@ static int hci_power_off_sync(struct hci_dev *hdev)
 	if (!test_bit(HCI_UP, &hdev->flags))
 		return 0;
 
+	hci_dev_set_flag(hdev, HCI_POWERING_DOWN);
+
 	if (test_bit(HCI_ISCAN, &hdev->flags) ||
 	    test_bit(HCI_PSCAN, &hdev->flags)) {
 		err = hci_write_scan_enable_sync(hdev, 0x00);
 		if (err)
-			return err;
+			goto out;
 	}
 
 	err = hci_clear_adv_sync(hdev, NULL, false);
 	if (err)
-		return err;
+		goto out;
 
 	err = hci_stop_discovery_sync(hdev);
 	if (err)
-		return err;
+		goto out;
 
 	/* Terminated due to Power Off */
 	err = hci_disconnect_all_sync(hdev, HCI_ERROR_REMOTE_POWER_OFF);
 	if (err)
-		return err;
+		goto out;
+
+	err = hci_dev_close_sync(hdev);
 
-	return hci_dev_close_sync(hdev);
+out:
+	hci_dev_clear_flag(hdev, HCI_POWERING_DOWN);
+	return err;
 }
 
 int hci_set_powered_sync(struct hci_dev *hdev, u8 val)
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index c5291e139..8f42ee059 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -1382,6 +1382,14 @@ static int set_powered(struct sock *sk, struct hci_dev *hdev, void *data,
 
 	hci_dev_lock(hdev);
 
+	if (!cp->val) {
+		if (hci_dev_test_flag(hdev, HCI_POWERING_DOWN)) {
+			err = mgmt_cmd_status(sk, hdev->id, MGMT_OP_SET_POWERED,
+					      MGMT_STATUS_BUSY);
+			goto failed;
+		}
+	}
+
 	if (pending_find(MGMT_OP_SET_POWERED, hdev)) {
 		err = mgmt_cmd_status(sk, hdev->id, MGMT_OP_SET_POWERED,
 				      MGMT_STATUS_BUSY);
@@ -9742,6 +9750,9 @@ bool mgmt_powering_down(struct hci_dev *hdev)
 	struct mgmt_pending_cmd *cmd;
 	struct mgmt_mode *cp;
 
+	if (hci_dev_test_flag(hdev, HCI_POWERING_DOWN))
+		return true;
+
 	cmd = pending_find(MGMT_OP_SET_POWERED, hdev);
 	if (!cmd)
 		return false;
@@ -10049,6 +10060,9 @@ void mgmt_set_local_name_complete(struct hci_dev *hdev, u8 *name, u8 status)
 		/* If this is a HCI command related to powering on the
 		 * HCI dev don't send any mgmt signals.
 		 */
+		if (hci_dev_test_flag(hdev, HCI_POWERING_DOWN))
+			return;
+
 		if (pending_find(MGMT_OP_SET_POWERED, hdev))
 			return;
 	}
-- 
2.43.0

[PATCH v3 4/4] Bluetooth: Disconnect connected devices before rfkilling adapter

[Jonas Dreßler]

On a lot of platforms (at least the MS Surface devices, M1 macbooks, and
a few ThinkPads) firmware doesn't do its job when rfkilling a device
and the bluetooth adapter is not actually shut down properly on rfkill.
This leads to connected devices remaining in connected state and the
bluetooth connection eventually timing out after rfkilling an adapter.

Use the rfkill hook in the HCI driver to go through the full power-off
sequence (including stopping scans and disconnecting devices) before
rfkilling it, just like MGMT_OP_SET_POWERED would do.

In case anything during the larger power-off sequence fails, make sure
the device is still closed and the rfkill ends up being effective in
the end.

Signed-off-by: Jonas Dreßler <verdre@v0yd.nl>
---
 net/bluetooth/hci_core.c | 35 +++++++++++++++++++++++++++++++++--
 1 file changed, 33 insertions(+), 2 deletions(-)

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 1ec83985f..43e042338 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -940,20 +940,51 @@ int hci_get_dev_info(void __user *arg)
 
 /* ---- Interface to HCI drivers ---- */
 
+static int hci_dev_do_poweroff(struct hci_dev *hdev)
+{
+	int err;
+
+	BT_DBG("%s %p", hdev->name, hdev);
+
+	hci_req_sync_lock(hdev);
+
+	err = hci_set_powered_sync(hdev, false);
+
+	hci_req_sync_unlock(hdev);
+
+	return err;
+}
+
 static int hci_rfkill_set_block(void *data, bool blocked)
 {
 	struct hci_dev *hdev = data;
+	int err;
 
 	BT_DBG("%p name %s blocked %d", hdev, hdev->name, blocked);
 
 	if (hci_dev_test_flag(hdev, HCI_USER_CHANNEL))
 		return -EBUSY;
 
+	if (blocked == hci_dev_test_flag(hdev, HCI_RFKILLED))
+		return 0;
+
 	if (blocked) {
 		hci_dev_set_flag(hdev, HCI_RFKILLED);
+
 		if (!hci_dev_test_flag(hdev, HCI_SETUP) &&
-		    !hci_dev_test_flag(hdev, HCI_CONFIG))
-			hci_dev_do_close(hdev);
+		    !hci_dev_test_flag(hdev, HCI_CONFIG)) {
+			err = hci_dev_do_poweroff(hdev);
+			if (err) {
+				bt_dev_err(hdev, "Error when powering off device on rfkill (%d)",
+					   err);
+
+				/* Make sure the device is still closed even if
+				 * anything during power off sequence (eg.
+				 * disconnecting devices) failed.
+				 */
+				hci_dev_do_close(hdev);
+			}
+		}
 	} else {
 		hci_dev_clear_flag(hdev, HCI_RFKILLED);
 	}
-- 
2.43.0

RE: Disconnect devices before rfkilling adapter

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 1422 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=814930

---Test result---

Test Summary:
CheckPatch                    PASS      1.97 seconds
GitLint                       PASS      0.76 seconds
SubjectPrefix                 PASS      0.26 seconds
BuildKernel                   PASS      28.64 seconds
CheckAllWarning               PASS      31.16 seconds
CheckSparse                   PASS      37.44 seconds
CheckSmatch                   PASS      99.20 seconds
BuildKernel32                 PASS      27.28 seconds
TestRunnerSetup               PASS      436.83 seconds
TestRunner_l2cap-tester       PASS      22.96 seconds
TestRunner_iso-tester         PASS      45.40 seconds
TestRunner_bnep-tester        PASS      6.80 seconds
TestRunner_mgmt-tester        PASS      168.15 seconds
TestRunner_rfcomm-tester      PASS      10.99 seconds
TestRunner_sco-tester         PASS      14.78 seconds
TestRunner_ioctl-tester       PASS      12.60 seconds
TestRunner_mesh-tester        PASS      8.83 seconds
TestRunner_smp-tester         PASS      9.72 seconds
TestRunner_userchan-tester    PASS      7.23 seconds
IncrementalBuild              PASS      61.99 seconds



---
Regards,
Linux Bluetooth

Re: [PATCH v3 0/4] Disconnect devices before rfkilling adapter

[Luiz Augusto von Dentz]

Hi Jonas,

On Sun, Jan 7, 2024 at 1:03 PM Jonas Dreßler <verdre@v0yd.nl> wrote:
>
> Apparently the firmware is supposed to power off the bluetooth card
> properly, including disconnecting devices, when we use rfkill to block
> bluetooth. This doesn't work on a lot of laptops though, leading to weird
> issues after turning off bluetooth, like the connection timing out on the
> peripherals which were connected, and bluetooth not connecting properly
> when the adapter is turned on again after rfkilling.
>
> This series uses the rfkill hook in the bluetooth subsystem
> to execute a few more shutdown commands and make sure that all
> devices get disconnected before we close the HCI connection to the adapter.
>
> ---
>
> v1: https://lore.kernel.org/linux-bluetooth/20240102133311.6712-1-verdre@v0yd.nl/
> v2: https://lore.kernel.org/linux-bluetooth/20240102181946.57288-1-verdre@v0yd.nl/
> v3:
>  - Update commit message titles to reflect what's actually happening
>    (disconnecting devices, not sending a power-off command).
>  - Doing the shutdown sequence synchronously instead of async now.
>  - Move HCI_RFKILLED flag back again to be set before shutdown.
>  - Added a "fallback" hci_dev_do_close() to the error path because
>    hci_set_powered_sync() might bail-out early on error.
>
> Jonas Dreßler (4):
>   Bluetooth: Remove HCI_POWER_OFF_TIMEOUT
>   Bluetooth: mgmt: Remove leftover queuing of power_off work
>   Bluetooth: Add new state HCI_POWERING_DOWN
>   Bluetooth: Disconnect connected devices before rfkilling adapter
>
>  include/net/bluetooth/hci.h |  2 +-
>  net/bluetooth/hci_core.c    | 35 +++++++++++++++++++++++++++++++++--
>  net/bluetooth/hci_sync.c    | 16 +++++++++++-----
>  net/bluetooth/mgmt.c        | 30 ++++++++++++++----------------
>  4 files changed, 59 insertions(+), 24 deletions(-)
>
> --
> 2.43.0

I will probably be applying this sortly, but let's try to add tests to
mgmt-tester just to make sure we don't introduce regressions later,
btw it seems there are a few suspend test that do connect, for
example:

Suspend - Success 5 (Pairing - Legacy) - waiting 1 seconds
random: crng init done
  New connection with handle 0x002a
  Test condition complete, 1 left
Suspend - Success 5 (Pairing - Legacy) - waiting done
  Set the system into Suspend via force_suspend
  New Controller Suspend event received
  Test condition complete, 0 left

-- 
Luiz Augusto von Dentz

Re: [PATCH v3 0/4] Disconnect devices before rfkilling adapter

[patchwork-bot+bluetooth]

Hello:

This series was applied to bluetooth/bluetooth-next.git (master)
by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>:

On Sun,  7 Jan 2024 19:02:46 +0100 you wrote:
> Apparently the firmware is supposed to power off the bluetooth card
> properly, including disconnecting devices, when we use rfkill to block
> bluetooth. This doesn't work on a lot of laptops though, leading to weird
> issues after turning off bluetooth, like the connection timing out on the
> peripherals which were connected, and bluetooth not connecting properly
> when the adapter is turned on again after rfkilling.
> 
> [...]

Here is the summary with links:
  - [v3,1/4] Bluetooth: Remove HCI_POWER_OFF_TIMEOUT
    https://git.kernel.org/bluetooth/bluetooth-next/c/f48705f473ce
  - [v3,2/4] Bluetooth: mgmt: Remove leftover queuing of power_off work
    https://git.kernel.org/bluetooth/bluetooth-next/c/2e7a6a997c9a
  - [v3,3/4] Bluetooth: Add new state HCI_POWERING_DOWN
    https://git.kernel.org/bluetooth/bluetooth-next/c/2b16c80d8011
  - [v3,4/4] Bluetooth: Disconnect connected devices before rfkilling adapter
    https://git.kernel.org/bluetooth/bluetooth-next/c/088656165c2d

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH v3 0/4] Disconnect devices before rfkilling adapter

[Jonas Dreßler]

Hi Luiz,

On 1/8/24 19:05, Luiz Augusto von Dentz wrote:
> Hi Jonas,
> 
> On Sun, Jan 7, 2024 at 1:03 PM Jonas Dreßler <verdre@v0yd.nl> wrote:
>>
>> Apparently the firmware is supposed to power off the bluetooth card
>> properly, including disconnecting devices, when we use rfkill to block
>> bluetooth. This doesn't work on a lot of laptops though, leading to weird
>> issues after turning off bluetooth, like the connection timing out on the
>> peripherals which were connected, and bluetooth not connecting properly
>> when the adapter is turned on again after rfkilling.
>>
>> This series uses the rfkill hook in the bluetooth subsystem
>> to execute a few more shutdown commands and make sure that all
>> devices get disconnected before we close the HCI connection to the adapter.
>>
>> ---
>>
>> v1: https://lore.kernel.org/linux-bluetooth/20240102133311.6712-1-verdre@v0yd.nl/
>> v2: https://lore.kernel.org/linux-bluetooth/20240102181946.57288-1-verdre@v0yd.nl/
>> v3:
>>   - Update commit message titles to reflect what's actually happening
>>     (disconnecting devices, not sending a power-off command).
>>   - Doing the shutdown sequence synchronously instead of async now.
>>   - Move HCI_RFKILLED flag back again to be set before shutdown.
>>   - Added a "fallback" hci_dev_do_close() to the error path because
>>     hci_set_powered_sync() might bail-out early on error.
>>
>> Jonas Dreßler (4):
>>    Bluetooth: Remove HCI_POWER_OFF_TIMEOUT
>>    Bluetooth: mgmt: Remove leftover queuing of power_off work
>>    Bluetooth: Add new state HCI_POWERING_DOWN
>>    Bluetooth: Disconnect connected devices before rfkilling adapter
>>
>>   include/net/bluetooth/hci.h |  2 +-
>>   net/bluetooth/hci_core.c    | 35 +++++++++++++++++++++++++++++++++--
>>   net/bluetooth/hci_sync.c    | 16 +++++++++++-----
>>   net/bluetooth/mgmt.c        | 30 ++++++++++++++----------------
>>   4 files changed, 59 insertions(+), 24 deletions(-)
>>
>> --
>> 2.43.0
> 
> I will probably be applying this sortly, but let's try to add tests to
> mgmt-tester just to make sure we don't introduce regressions later,
> btw it seems there are a few suspend test that do connect, for
> example:
> 
> Suspend - Success 5 (Pairing - Legacy) - waiting 1 seconds
> random: crng init done
>    New connection with handle 0x002a
>    Test condition complete, 1 left
> Suspend - Success 5 (Pairing - Legacy) - waiting done
>    Set the system into Suspend via force_suspend
>    New Controller Suspend event received
>    Test condition complete, 0 left
> 

Thanks for that hint, I've been starting to write a test and managed to
write to the rfkill file and it's blocking the device just fine, except
I've run into what might be a bug in the virtual HCI driver:

So the power down sequence is initiated on the rfkill as expected and
hci_set_powered_sync(false) is called. That then calls
hci_write_scan_enable_sync(), and this HCI command never gets a response
from the virtual HCI driver. Strangely, BT_HCI_CMD_WRITE_SCAN_ENABLE is
implemented in btdev.c and the callback does get executed (I checked), it
just doesn't send the command completed event:

< HCI Command: Write Scan Enable (0x03|0x001a) plen 1                                                                                                                                       #1588 [hci1] 12.294234
         Scan enable: No Scans (0x00)

no response after...

Below is my current mgmt-tester patch adding the test:

diff --git a/tools/mgmt-tester.c b/tools/mgmt-tester.c
index 7dfd1b0c7..2095b7203 100644
--- a/tools/mgmt-tester.c
+++ b/tools/mgmt-tester.c
@@ -12439,6 +12439,30 @@ static const struct generic_data suspend_resume_success_5 = {
  	.expect_alt_ev_len = sizeof(suspend_state_param_disconnect),
  };
  
+static const uint8_t rfkill_hci_disconnect_device[] = {
+   0x00, 0x00, 0x01, 0x01, 0xaa, 0x00, 0x00,
+   0x05,
+};
+
+static const uint8_t rfkill_new_settings_ev[] = {
+   0x92, 0x00, 0x00, 0x00,
+};
+
+
+static const struct generic_data rfkill_disconnect_devices = {
+	.setup_settings = settings_powered_connectable_bondable,
+	.pin = pair_device_pin,
+	.pin_len = sizeof(pair_device_pin),
+	.client_pin = pair_device_pin,
+	.client_pin_len = sizeof(pair_device_pin),
+	.expect_hci_command = BT_HCI_CMD_DISCONNECT,
+	.expect_hci_param = rfkill_hci_disconnect_device,
+	.expect_hci_len = sizeof(rfkill_hci_disconnect_device),
+	.expect_alt_ev = MGMT_EV_NEW_SETTINGS,
+	.expect_alt_ev_param = rfkill_new_settings_ev,
+	.expect_alt_ev_len = sizeof(rfkill_new_settings_ev),
+};
+
  static void trigger_force_suspend(void *user_data)
  {
  	struct test_data *data = tester_get_data();
@@ -12454,6 +12478,81 @@ static void trigger_force_suspend(void *user_data)
  	}
  }
  
+enum rfkill_type {
+	RFKILL_TYPE_ALL = 0,
+	RFKILL_TYPE_WLAN,
+	RFKILL_TYPE_BLUETOOTH,
+	RFKILL_TYPE_UWB,
+	RFKILL_TYPE_WIMAX,
+	RFKILL_TYPE_WWAN,
+};
+
+enum rfkill_operation {
+	RFKILL_OP_ADD = 0,
+	RFKILL_OP_DEL,
+	RFKILL_OP_CHANGE,
+	RFKILL_OP_CHANGE_ALL,
+};
+
+struct rfkill_event {
+	uint32_t idx;
+	uint8_t  type;
+	uint8_t  op;
+	uint8_t  soft;
+	uint8_t  hard;
+};
+#define RFKILL_EVENT_SIZE_V1    8
+
+static void trigger_rfkill(void *user_data)
+{
+	int fd;
+	int latest_rfkill_idx;
+        struct rfkill_event write_event;
+        ssize_t l;
+
+	tester_print("Triggering rfkill block of hci device");
+
+	fd = open("/dev/rfkill", O_RDWR|O_CLOEXEC|O_NOCTTY|O_NONBLOCK);
+	if (fd < 0) {
+		tester_warn("Failed to open RFKILL control device");
+		return;
+	}
+
+	latest_rfkill_idx = -1;
+	while (1) {
+		struct rfkill_event event = { 0 };
+		ssize_t len;
+
+		len = read(fd, &event, sizeof(event));
+		if (len < RFKILL_EVENT_SIZE_V1)
+			break;
+
+		if (event.type == RFKILL_TYPE_BLUETOOTH)
+			latest_rfkill_idx = event.idx;
+	}
+
+	if (latest_rfkill_idx < 0) {
+		tester_warn("No rfkill device to block found");
+		return;
+	}
+
+	write_event.idx = latest_rfkill_idx;
+	write_event.op = RFKILL_OP_CHANGE;
+	write_event.soft = true;
+	
+        l = write(fd, &write_event, sizeof write_event);
+
+	close(fd);
+
+	if (l < 0) {
+		tester_warn("Failed to execute rfkill op");
+		return;
+	}
+
+	if ((size_t)l < RFKILL_EVENT_SIZE_V1)
+		tester_warn("Failed to write to rfkill file");
+}
+
  static void trigger_force_resume(void *user_data)
  {
  	struct test_data *data = tester_get_data();
@@ -12475,6 +12574,12 @@ static void test_suspend_resume_success_5(const void *test_data)
  	tester_wait(1, trigger_force_suspend, NULL);
  }
  
+static void test_disconnect_on_rfkill(const void *test_data)
+{
+	test_pairing_acceptor(test_data);
+	tester_wait(1, trigger_rfkill, NULL);
+}
+
  static const struct generic_data suspend_resume_success_6 = {
  	.setup_settings = settings_powered_connectable_bondable_ssp,
  	.client_enable_ssp = true,
@@ -14534,6 +14639,15 @@ int main(int argc, char *argv[])
  				&suspend_resume_success_5, NULL,
  				test_suspend_resume_success_5);
  
+	/* Suspend/Resume
+	 * Setup: Pair.
+	 * Run: Enable suspend
+	 * Expect: Receive the Suspend Event
+	 */
+	test_bredrle("Rfkill - disconnect devices",
+				&rfkill_disconnect_devices, NULL,
+				test_disconnect_on_rfkill);
+
  	/* Suspend/Resume
  	 * Setup: Pair.
  	 * Run: Enable suspend

Re: [PATCH v3 0/4] Disconnect devices before rfkilling adapter

[Jonas Dreßler]

Hi Luiz,

On 1/8/24 11:25 PM, Jonas Dreßler wrote:
> Hi Luiz,
> 
> On 1/8/24 19:05, Luiz Augusto von Dentz wrote:
>> Hi Jonas,
>>
>> On Sun, Jan 7, 2024 at 1:03 PM Jonas Dreßler <verdre@v0yd.nl> wrote:
>>>
>>> Apparently the firmware is supposed to power off the bluetooth card
>>> properly, including disconnecting devices, when we use rfkill to block
>>> bluetooth. This doesn't work on a lot of laptops though, leading to weird
>>> issues after turning off bluetooth, like the connection timing out on the
>>> peripherals which were connected, and bluetooth not connecting properly
>>> when the adapter is turned on again after rfkilling.
>>>
>>> This series uses the rfkill hook in the bluetooth subsystem
>>> to execute a few more shutdown commands and make sure that all
>>> devices get disconnected before we close the HCI connection to the adapter.
>>>
>>> ---
>>>
>>> v1: https://lore.kernel.org/linux-bluetooth/20240102133311.6712-1-verdre@v0yd.nl/
>>> v2: https://lore.kernel.org/linux-bluetooth/20240102181946.57288-1-verdre@v0yd.nl/
>>> v3:
>>>   - Update commit message titles to reflect what's actually happening
>>>     (disconnecting devices, not sending a power-off command).
>>>   - Doing the shutdown sequence synchronously instead of async now.
>>>   - Move HCI_RFKILLED flag back again to be set before shutdown.
>>>   - Added a "fallback" hci_dev_do_close() to the error path because
>>>     hci_set_powered_sync() might bail-out early on error.
>>>
>>> Jonas Dreßler (4):
>>>    Bluetooth: Remove HCI_POWER_OFF_TIMEOUT
>>>    Bluetooth: mgmt: Remove leftover queuing of power_off work
>>>    Bluetooth: Add new state HCI_POWERING_DOWN
>>>    Bluetooth: Disconnect connected devices before rfkilling adapter
>>>
>>>   include/net/bluetooth/hci.h |  2 +-
>>>   net/bluetooth/hci_core.c    | 35 +++++++++++++++++++++++++++++++++--
>>>   net/bluetooth/hci_sync.c    | 16 +++++++++++-----
>>>   net/bluetooth/mgmt.c        | 30 ++++++++++++++----------------
>>>   4 files changed, 59 insertions(+), 24 deletions(-)
>>>
>>> -- 
>>> 2.43.0
>>
>> I will probably be applying this sortly, but let's try to add tests to
>> mgmt-tester just to make sure we don't introduce regressions later,
>> btw it seems there are a few suspend test that do connect, for
>> example:
>>
>> Suspend - Success 5 (Pairing - Legacy) - waiting 1 seconds
>> random: crng init done
>>    New connection with handle 0x002a
>>    Test condition complete, 1 left
>> Suspend - Success 5 (Pairing - Legacy) - waiting done
>>    Set the system into Suspend via force_suspend
>>    New Controller Suspend event received
>>    Test condition complete, 0 left
>>
> 
> Thanks for that hint, I've been starting to write a test and managed to
> write to the rfkill file and it's blocking the device just fine, except
> I've run into what might be a bug in the virtual HCI driver:
> 
> So the power down sequence is initiated on the rfkill as expected and
> hci_set_powered_sync(false) is called. That then calls
> hci_write_scan_enable_sync(), and this HCI command never gets a response
> from the virtual HCI driver. Strangely, BT_HCI_CMD_WRITE_SCAN_ENABLE is
> implemented in btdev.c and the callback does get executed (I checked), it
> just doesn't send the command completed event:
> 
> < HCI Command: Write Scan Enable (0x03|0x001a) plen 1                                                                                                                                       #1588 [hci1] 12.294234
>          Scan enable: No Scans (0x00)
> 
> no response after...
> 

So I think I found the problem here too:

The problem with this one is that calling hci_set_powered_sync() from
within the context of the write to the rfkill device blocks the write()
until the HCI commands have returned. Because the mgmt-tester process is
stuck in write(), it can't reply to the HCI commands using the emulator
(which runs in the same thread), and after two seconds the HCI command
times out and the test ends.

I haven't really been able to confirm this other than that we're indeed
blocked in write(), does this sound like a sane explanation to you?

Seems like for this to work we'd either have to stop blocking userspace
until the rfkill has finished/failed (don't think that's a good idea), or
write to the rfkill device from an separate thread in mgmt-tester? The
latter should be fairly easy, so I'll give that a shot.

Cheers,
Jonas

Re: [PATCH v3 0/4] Disconnect devices before rfkilling adapter

[Luiz Augusto von Dentz]

Hi Jonas,

On Wed, Jan 24, 2024 at 1:00 PM Jonas Dreßler <verdre@v0yd.nl> wrote:
>
> Hi Luiz,
>
> On 1/8/24 11:25 PM, Jonas Dreßler wrote:
> > Hi Luiz,
> >
> > On 1/8/24 19:05, Luiz Augusto von Dentz wrote:
> >> Hi Jonas,
> >>
> >> On Sun, Jan 7, 2024 at 1:03 PM Jonas Dreßler <verdre@v0yd.nl> wrote:
> >>>
> >>> Apparently the firmware is supposed to power off the bluetooth card
> >>> properly, including disconnecting devices, when we use rfkill to block
> >>> bluetooth. This doesn't work on a lot of laptops though, leading to weird
> >>> issues after turning off bluetooth, like the connection timing out on the
> >>> peripherals which were connected, and bluetooth not connecting properly
> >>> when the adapter is turned on again after rfkilling.
> >>>
> >>> This series uses the rfkill hook in the bluetooth subsystem
> >>> to execute a few more shutdown commands and make sure that all
> >>> devices get disconnected before we close the HCI connection to the adapter.
> >>>
> >>> ---
> >>>
> >>> v1: https://lore.kernel.org/linux-bluetooth/20240102133311.6712-1-verdre@v0yd.nl/
> >>> v2: https://lore.kernel.org/linux-bluetooth/20240102181946.57288-1-verdre@v0yd.nl/
> >>> v3:
> >>>   - Update commit message titles to reflect what's actually happening
> >>>     (disconnecting devices, not sending a power-off command).
> >>>   - Doing the shutdown sequence synchronously instead of async now.
> >>>   - Move HCI_RFKILLED flag back again to be set before shutdown.
> >>>   - Added a "fallback" hci_dev_do_close() to the error path because
> >>>     hci_set_powered_sync() might bail-out early on error.
> >>>
> >>> Jonas Dreßler (4):
> >>>    Bluetooth: Remove HCI_POWER_OFF_TIMEOUT
> >>>    Bluetooth: mgmt: Remove leftover queuing of power_off work
> >>>    Bluetooth: Add new state HCI_POWERING_DOWN
> >>>    Bluetooth: Disconnect connected devices before rfkilling adapter
> >>>
> >>>   include/net/bluetooth/hci.h |  2 +-
> >>>   net/bluetooth/hci_core.c    | 35 +++++++++++++++++++++++++++++++++--
> >>>   net/bluetooth/hci_sync.c    | 16 +++++++++++-----
> >>>   net/bluetooth/mgmt.c        | 30 ++++++++++++++----------------
> >>>   4 files changed, 59 insertions(+), 24 deletions(-)
> >>>
> >>> --
> >>> 2.43.0
> >>
> >> I will probably be applying this sortly, but let's try to add tests to
> >> mgmt-tester just to make sure we don't introduce regressions later,
> >> btw it seems there are a few suspend test that do connect, for
> >> example:
> >>
> >> Suspend - Success 5 (Pairing - Legacy) - waiting 1 seconds
> >> random: crng init done
> >>    New connection with handle 0x002a
> >>    Test condition complete, 1 left
> >> Suspend - Success 5 (Pairing - Legacy) - waiting done
> >>    Set the system into Suspend via force_suspend
> >>    New Controller Suspend event received
> >>    Test condition complete, 0 left
> >>
> >
> > Thanks for that hint, I've been starting to write a test and managed to
> > write to the rfkill file and it's blocking the device just fine, except
> > I've run into what might be a bug in the virtual HCI driver:
> >
> > So the power down sequence is initiated on the rfkill as expected and
> > hci_set_powered_sync(false) is called. That then calls
> > hci_write_scan_enable_sync(), and this HCI command never gets a response
> > from the virtual HCI driver. Strangely, BT_HCI_CMD_WRITE_SCAN_ENABLE is
> > implemented in btdev.c and the callback does get executed (I checked), it
> > just doesn't send the command completed event:
> >
> > < HCI Command: Write Scan Enable (0x03|0x001a) plen 1                                                                                                                                       #1588 [hci1] 12.294234
> >          Scan enable: No Scans (0x00)
> >
> > no response after...
> >
>
> So I think I found the problem here too:
>
> The problem with this one is that calling hci_set_powered_sync() from
> within the context of the write to the rfkill device blocks the write()
> until the HCI commands have returned. Because the mgmt-tester process is
> stuck in write(), it can't reply to the HCI commands using the emulator
> (which runs in the same thread), and after two seconds the HCI command
> times out and the test ends.
>
> I haven't really been able to confirm this other than that we're indeed
> blocked in write(), does this sound like a sane explanation to you?
>
> Seems like for this to work we'd either have to stop blocking userspace
> until the rfkill has finished/failed (don't think that's a good idea), or
> write to the rfkill device from an separate thread in mgmt-tester? The
> latter should be fairly easy, so I'll give that a shot.

Userspace code is normally not thread safe since we usually have been
using the concept of mainloop to avoid entering into the threading
support which might require locking, etc. That said we could perhaps
either not block at vhci driver, with use of hci_cmd_sync_queue, etc,
or use async IO mechanism in userspace so we avoid blocking btdev
handling.

> Cheers,
> Jonas



-- 
Luiz Augusto von Dentz