[PATCH v2 0/4] Power off HCI devices before rfkilling them

[PATCH v2 0/4] Power off HCI devices before rfkilling them

[Jonas Dreßler]

In theory the firmware is supposed to power off the bluetooth card
when we use rfkill to block it. 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
quickly after rfkilling.

This series hooks into the rfkill driver from the bluetooth subsystem
to send a HCI_POWER_OFF command to the adapter before actually submitting
the rfkill to the firmware and killing the HCI connection.

---

v1 -> v2: Fixed commit message title to make CI happy

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: Queue a HCI power-off command before rfkilling adapters

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

-- 
2.43.0

[PATCH v2 1/4] Bluetooth: Remove HCI_POWER_OFF_TIMEOUT

[Jonas Dreßler]

With commit cf75ad8b41d2aa06f98f365d42a3ae8b059daddd, the power off
sequence got refactored so that this 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

RE: Power off HCI devices before rfkilling them

[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=813896

---Test result---

Test Summary:
CheckPatch                    PASS      2.26 seconds
GitLint                       PASS      0.81 seconds
SubjectPrefix                 PASS      0.26 seconds
BuildKernel                   PASS      27.77 seconds
CheckAllWarning               PASS      30.72 seconds
CheckSparse                   PASS      36.61 seconds
CheckSmatch                   PASS      99.26 seconds
BuildKernel32                 PASS      27.03 seconds
TestRunnerSetup               PASS      435.16 seconds
TestRunner_l2cap-tester       PASS      23.04 seconds
TestRunner_iso-tester         PASS      45.75 seconds
TestRunner_bnep-tester        PASS      7.02 seconds
TestRunner_mgmt-tester        PASS      163.90 seconds
TestRunner_rfcomm-tester      PASS      10.87 seconds
TestRunner_sco-tester         PASS      14.96 seconds
TestRunner_ioctl-tester       PASS      12.20 seconds
TestRunner_mesh-tester        PASS      8.73 seconds
TestRunner_smp-tester         PASS      9.76 seconds
TestRunner_userchan-tester    PASS      7.24 seconds
IncrementalBuild              PASS      60.77 seconds



---
Regards,
Linux Bluetooth

[PATCH v2 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
8b064a3ad377c016a17e74f676e7a204c2b8c9f2 and
c9910d0fb4fc2ede468b26d45a1d50c309897770 in an effort to clean up state
and do things like disconnecting devices before actually powering off
the device.

After that, commit a3172b7eb4a2719711187cfca12097d2326e85a7 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
cf75ad8b41d2aa06f98f365d42a3ae8b059daddd, 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 v2 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 v2 4/4] Bluetooth: Queue a HCI power-off command before rfkilling adapters

[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 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 actually power the device off
before rfkilling it.

Note that the wifi subsystem is doing something similar by calling
cfg80211_shutdown_all_interfaces()
in it's rfkill set_block callback (see cfg80211_rfkill_set_block).

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

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 1ec83985f..1c91d02f7 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -543,6 +543,23 @@ int hci_dev_open(__u16 dev)
 	return err;
 }
 
+static int set_powered_off_sync(struct hci_dev *hdev, void *data)
+{
+	return hci_set_powered_sync(hdev, false);
+}
+
+static void set_powered_off_sync_complete(struct hci_dev *hdev, void *data, int err)
+{
+	if (err)
+		bt_dev_err(hdev, "Powering HCI device off before rfkilling failed (%d)", err);
+}
+
+static int hci_dev_do_poweroff(struct hci_dev *hdev)
+{
+	return hci_cmd_sync_queue(hdev, set_powered_off_sync,
+				  NULL, set_powered_off_sync_complete);
+}
+
 int hci_dev_do_close(struct hci_dev *hdev)
 {
 	int err;
@@ -943,17 +960,27 @@ int hci_get_dev_info(void __user *arg)
 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, "Powering off device before rfkilling failed (%d)",
+					   err);
+			}
+		}
+
+		hci_dev_set_flag(hdev, HCI_RFKILLED);
 	} else {
 		hci_dev_clear_flag(hdev, HCI_RFKILLED);
 	}
-- 
2.43.0

Re: [PATCH v2 4/4] Bluetooth: Queue a HCI power-off command before rfkilling adapters

[Luiz Augusto von Dentz]

Hi Jonas,

On Tue, Jan 2, 2024 at 1:19 PM Jonas Dreßler <verdre@v0yd.nl> wrote:
>
> 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 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 actually power the device off
> before rfkilling it.
>
> Note that the wifi subsystem is doing something similar by calling
> cfg80211_shutdown_all_interfaces()
> in it's rfkill set_block callback (see cfg80211_rfkill_set_block).

So the rfkill is supposed to be wait for cleanup, not a forceful
shutdown of RF traffic? I assume it would be the later since to do a
proper cleanup that could cause more RF traffic while the current
assumption was to stop all traffic and then call hdev->shutdown to
ensure the driver does shutdown the RF traffic, perhaps this
assumption has changed over time since interrupting the RF traffic may
cause what you just described because the remote end will have to rely
on link-loss logic to detect the connection has been terminated.

> Signed-off-by: Jonas Dreßler <verdre@v0yd.nl>
> ---
>  net/bluetooth/hci_core.c | 33 ++++++++++++++++++++++++++++++---
>  1 file changed, 30 insertions(+), 3 deletions(-)
>
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 1ec83985f..1c91d02f7 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -543,6 +543,23 @@ int hci_dev_open(__u16 dev)
>         return err;
>  }
>
> +static int set_powered_off_sync(struct hci_dev *hdev, void *data)
> +{
> +       return hci_set_powered_sync(hdev, false);
> +}
> +
> +static void set_powered_off_sync_complete(struct hci_dev *hdev, void *data, int err)
> +{
> +       if (err)
> +               bt_dev_err(hdev, "Powering HCI device off before rfkilling failed (%d)", err);
> +}
> +
> +static int hci_dev_do_poweroff(struct hci_dev *hdev)
> +{
> +       return hci_cmd_sync_queue(hdev, set_powered_off_sync,
> +                                 NULL, set_powered_off_sync_complete);
> +}
> +
>  int hci_dev_do_close(struct hci_dev *hdev)
>  {
>         int err;
> @@ -943,17 +960,27 @@ int hci_get_dev_info(void __user *arg)
>  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, "Powering off device before rfkilling failed (%d)",
> +                                          err);
> +                       }

You already have the error printed on set_powered_off_sync_complete
not sure why you have it here as well.

> +               }
> +
> +               hci_dev_set_flag(hdev, HCI_RFKILLED);

Before we used to set the HCI_RFKILLED beforehand, is this change
really intended or not? I think we should keep doing it ahead of power
off sequence since we can probably use it to ignore if there are any
errors on the cleanup, etc.

>         } else {
>                 hci_dev_clear_flag(hdev, HCI_RFKILLED);
>         }
> --
> 2.43.0
>


-- 
Luiz Augusto von Dentz

Re: [PATCH v2 4/4] Bluetooth: Queue a HCI power-off command before rfkilling adapters

[Jonas Dreßler]

On 1/2/24 19:31, Luiz Augusto von Dentz wrote:
> Hi Jonas,
> 
> On Tue, Jan 2, 2024 at 1:19 PM Jonas Dreßler <verdre@v0yd.nl> wrote:
>>
>> 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 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 actually power the device off
>> before rfkilling it.
>>
>> Note that the wifi subsystem is doing something similar by calling
>> cfg80211_shutdown_all_interfaces()
>> in it's rfkill set_block callback (see cfg80211_rfkill_set_block).
> 
> So the rfkill is supposed to be wait for cleanup, not a forceful
> shutdown of RF traffic? I assume it would be the later since to do a
> proper cleanup that could cause more RF traffic while the current
> assumption was to stop all traffic and then call hdev->shutdown to
> ensure the driver does shutdown the RF traffic, perhaps this
> assumption has changed over time since interrupting the RF traffic may
> cause what you just described because the remote end will have to rely
> on link-loss logic to detect the connection has been terminated.

Yes, it seems to me that as soon as the rfkill happens, anything in the 
drivers hdev->shutdown to shut things down will no longer go through to 
the card. I'd assume this is something that's enforced by firmware and 
we can't change, or would that be a bug on our side?

But yeah, proper shutdown of the adapter requires a bit more RF traffic. 
If rfkill guarantees that it shuts down all RF traffic *immediately*, 
maybe it would be better to expect power-off MGMT commands from 
userspace before rfkilling? But given that the disconnect appears to 
happen fine on some hardware, this seemed like the obvious and more 
reliable way to me.

> 
>> Signed-off-by: Jonas Dreßler <verdre@v0yd.nl>
>> ---
>>   net/bluetooth/hci_core.c | 33 ++++++++++++++++++++++++++++++---
>>   1 file changed, 30 insertions(+), 3 deletions(-)
>>
>> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
>> index 1ec83985f..1c91d02f7 100644
>> --- a/net/bluetooth/hci_core.c
>> +++ b/net/bluetooth/hci_core.c
>> @@ -543,6 +543,23 @@ int hci_dev_open(__u16 dev)
>>          return err;
>>   }
>>
>> +static int set_powered_off_sync(struct hci_dev *hdev, void *data)
>> +{
>> +       return hci_set_powered_sync(hdev, false);
>> +}
>> +
>> +static void set_powered_off_sync_complete(struct hci_dev *hdev, void *data, int err)
>> +{
>> +       if (err)
>> +               bt_dev_err(hdev, "Powering HCI device off before rfkilling failed (%d)", err);
>> +}
>> +
>> +static int hci_dev_do_poweroff(struct hci_dev *hdev)
>> +{
>> +       return hci_cmd_sync_queue(hdev, set_powered_off_sync,
>> +                                 NULL, set_powered_off_sync_complete);
>> +}
>> +
>>   int hci_dev_do_close(struct hci_dev *hdev)
>>   {
>>          int err;
>> @@ -943,17 +960,27 @@ int hci_get_dev_info(void __user *arg)
>>   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, "Powering off device before rfkilling failed (%d)",
>> +                                          err);
>> +                       }
> 
> You already have the error printed on set_powered_off_sync_complete
> not sure why you have it here as well.
> 
>> +               }
>> +
>> +               hci_dev_set_flag(hdev, HCI_RFKILLED);
> 
> Before we used to set the HCI_RFKILLED beforehand, is this change
> really intended or not? I think we should keep doing it ahead of power
> off sequence since we can probably use it to ignore if there are any
> errors on the cleanup, etc.

Good point, it's been a while since I wrote that patch, maybe something 
in the power-off logic bails out if HCI_RFKILLED is set and that's why I 
moved it below, I'll check that...

> 
>>          } else {
>>                  hci_dev_clear_flag(hdev, HCI_RFKILLED);
>>          }
>> --
>> 2.43.0
>>
> 
> 

Re: [PATCH v2 0/4] Power off HCI devices before rfkilling them

[Luiz Augusto von Dentz]

Hi Jonas,

On Tue, Jan 2, 2024 at 1:19 PM Jonas Dreßler <verdre@v0yd.nl> wrote:
>
> In theory the firmware is supposed to power off the bluetooth card
> when we use rfkill to block it. 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
> quickly after rfkilling.
>
> This series hooks into the rfkill driver from the bluetooth subsystem
> to send a HCI_POWER_OFF command to the adapter before actually submitting
> the rfkill to the firmware and killing the HCI connection.
>
> ---
>
> v1 -> v2: Fixed commit message title to make CI happy
>
> 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: Queue a HCI power-off command before rfkilling adapters

Apart from the assumption of RFKILL actually killing the RF
immediately or not, I'm fine with these changes, that said it would be
great if we can have some proper way to test the behavior of rfkill,
perhaps via mgmt-tester, since it should behave like the
MGMT_OP_SET_POWERED.

>  include/net/bluetooth/hci.h |  2 +-
>  net/bluetooth/hci_core.c    | 33 ++++++++++++++++++++++++++++++---
>  net/bluetooth/hci_sync.c    | 16 +++++++++++-----
>  net/bluetooth/mgmt.c        | 30 ++++++++++++++----------------
>  4 files changed, 56 insertions(+), 25 deletions(-)
>
> --
> 2.43.0
>


-- 
Luiz Augusto von Dentz

Re: [PATCH v2 0/4] Power off HCI devices before rfkilling them

[Jonas Dreßler]

Hi Luiz,

On 1/2/24 19:39, Luiz Augusto von Dentz wrote:
> Hi Jonas,
> 
> On Tue, Jan 2, 2024 at 1:19 PM Jonas Dreßler <verdre@v0yd.nl> wrote:
>>
>> In theory the firmware is supposed to power off the bluetooth card
>> when we use rfkill to block it. 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
>> quickly after rfkilling.
>>
>> This series hooks into the rfkill driver from the bluetooth subsystem
>> to send a HCI_POWER_OFF command to the adapter before actually submitting
>> the rfkill to the firmware and killing the HCI connection.
>>
>> ---
>>
>> v1 -> v2: Fixed commit message title to make CI happy
>>
>> 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: Queue a HCI power-off command before rfkilling adapters
> 
> Apart from the assumption of RFKILL actually killing the RF
> immediately or not, I'm fine with these changes, that said it would be
> great if we can have some proper way to test the behavior of rfkill,
> perhaps via mgmt-tester, since it should behave like the
> MGMT_OP_SET_POWERED.

Testing this sounds like a good idea, I guess we'd have to teach 
mgmt-tester to write to rfkill. The bigger problem seems to be that 
there's no MGMT event for power changes and also no MGMT_OP_GET_POWERED, 
so that's a bit concerning, could userspace even be notified about 
changes to adapter power?

Another thing I'm thinking about now is that queuing the HCI command 
using hci_cmd_sync_queue() might not be enough: The command is still 
executed async in a thread, and we won't actually block until it has 
been sent, so this might be introducing a race (rfkill could kill the 
adapter before we actually send the HCI command). The proper way might 
be to use a completion and wait until the 
set_powered_off_sync_complete() callback is invoked?

> 
>>   include/net/bluetooth/hci.h |  2 +-
>>   net/bluetooth/hci_core.c    | 33 ++++++++++++++++++++++++++++++---
>>   net/bluetooth/hci_sync.c    | 16 +++++++++++-----
>>   net/bluetooth/mgmt.c        | 30 ++++++++++++++----------------
>>   4 files changed, 56 insertions(+), 25 deletions(-)
>>
>> --
>> 2.43.0
>>
> 
> 

Cheers,
Jonas

Re: [PATCH v2 0/4] Power off HCI devices before rfkilling them

[Jonas Dreßler]

On 1/3/24 13:15, Jonas Dreßler wrote:
> Hi Luiz,
> 
> On 1/2/24 19:39, Luiz Augusto von Dentz wrote:
>> Hi Jonas,
>>
>> On Tue, Jan 2, 2024 at 1:19 PM Jonas Dreßler <verdre@v0yd.nl> wrote:
>>>
>>> In theory the firmware is supposed to power off the bluetooth card
>>> when we use rfkill to block it. 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
>>> quickly after rfkilling.
>>>
>>> This series hooks into the rfkill driver from the bluetooth subsystem
>>> to send a HCI_POWER_OFF command to the adapter before actually 
>>> submitting
>>> the rfkill to the firmware and killing the HCI connection.
>>>
>>> ---
>>>
>>> v1 -> v2: Fixed commit message title to make CI happy
>>>
>>> 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: Queue a HCI power-off command before rfkilling adapters
>>
>> Apart from the assumption of RFKILL actually killing the RF
>> immediately or not, I'm fine with these changes, that said it would be
>> great if we can have some proper way to test the behavior of rfkill,
>> perhaps via mgmt-tester, since it should behave like the
>> MGMT_OP_SET_POWERED.
> 
> Testing this sounds like a good idea, I guess we'd have to teach 
> mgmt-tester to write to rfkill. The bigger problem seems to be that 
> there's no MGMT event for power changes and also no MGMT_OP_GET_POWERED, 
> so that's a bit concerning, could userspace even be notified about 
> changes to adapter power?

Sent v3 of the patchset now, I didn't add a test to mgmt-tester because 
it's actually quite tricky to notice the full shutdown sequence happened 
rather than just closing the device. As long as no devices are 
connected, the difference is mostly in a few (faily random) events:

btmon without the patch:

@ MGMT Event: Class Of Device Changed (0x0007) plen 3 
 
        {0x0001} [hci0] 169.101804
         Class: 0x000000
           Major class: Miscellaneous
           Minor class: 0x00
@ MGMT Event: New Settings (0x0006) plen 4 
 
        {0x0001} [hci0] 169.101820
         Current settings: 0x00000ac0
           Secure Simple Pairing
           BR/EDR
           Low Energy
           Secure Connections

btmon with the patch:

< HCI Command: Write Scan Enable (0x03|0x001a) plen 1 
 
              #109 [hci0] 7.031852
         Scan enable: No Scans (0x00)
 > HCI Event: Command Complete (0x0e) plen 4 
 
               #110 [hci0] 7.033026
       Write Scan Enable (0x03|0x001a) ncmd 1
         Status: Success (0x00)
< HCI Command: LE Set Extended Advertising Enable (0x08|0x0039) plen 2 
 
              #111 [hci0] 7.033055
         Extended advertising: Disabled (0x00)
         Number of sets: Disable all sets (0x00)
 > HCI Event: Command Complete (0x0e) plen 4 
 
               #112 [hci0] 7.034202
       LE Set Extended Advertising Enable (0x08|0x0039) ncmd 1
         Status: Success (0x00)
< HCI Command: LE Clear Advertising Sets (0x08|0x003d) plen 0 
 
              #113 [hci0] 7.034233
 > HCI Event: Command Complete (0x0e) plen 4 
 
               #114 [hci0] 7.035527
       LE Clear Advertising Sets (0x08|0x003d) ncmd 1
         Status: Success (0x00)
@ MGMT Event: Class Of Device Changed (0x0007) plen 3 
 
          {0x0001} [hci0] 7.035554
         Class: 0x000000
           Major class: Miscellaneous
           Minor class: 0x00
@ MGMT Event: New Settings (0x0006) plen 4 
 
          {0x0001} [hci0] 7.035568
         Current settings: 0x00000ac0
           Secure Simple Pairing
           BR/EDR
           Low Energy
           Secure Connections

Maybe we could add a fake connection and check whether that is 
disconnected on the rfkill, but I don't think mgmt-tester supports that..

Fwiw, I don't think having a test for this is super important, this is a 
regression a lot of people would notice very quickly I think.

> 
> Another thing I'm thinking about now is that queuing the HCI command 
> using hci_cmd_sync_queue() might not be enough: The command is still 
> executed async in a thread, and we won't actually block until it has 
> been sent, so this might be introducing a race (rfkill could kill the 
> adapter before we actually send the HCI command). The proper way might 
> be to use a completion and wait until the 
> set_powered_off_sync_complete() callback is invoked?
> 
>>
>>>   include/net/bluetooth/hci.h |  2 +-
>>>   net/bluetooth/hci_core.c    | 33 ++++++++++++++++++++++++++++++---
>>>   net/bluetooth/hci_sync.c    | 16 +++++++++++-----
>>>   net/bluetooth/mgmt.c        | 30 ++++++++++++++----------------
>>>   4 files changed, 56 insertions(+), 25 deletions(-)
>>>
>>> -- 
>>> 2.43.0
>>>
>>
>>
> 
> Cheers,
> Jonas

Cheers,
Jonas

Re: [PATCH v2 0/4] Power off HCI devices before rfkilling them

[Luiz Augusto von Dentz]

Hi Jonas,

On Sun, Jan 7, 2024 at 1:11 PM Jonas Dreßler <verdre@v0yd.nl> wrote:
>
> On 1/3/24 13:15, Jonas Dreßler wrote:
> > Hi Luiz,
> >
> > On 1/2/24 19:39, Luiz Augusto von Dentz wrote:
> >> Hi Jonas,
> >>
> >> On Tue, Jan 2, 2024 at 1:19 PM Jonas Dreßler <verdre@v0yd.nl> wrote:
> >>>
> >>> In theory the firmware is supposed to power off the bluetooth card
> >>> when we use rfkill to block it. 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
> >>> quickly after rfkilling.
> >>>
> >>> This series hooks into the rfkill driver from the bluetooth subsystem
> >>> to send a HCI_POWER_OFF command to the adapter before actually
> >>> submitting
> >>> the rfkill to the firmware and killing the HCI connection.
> >>>
> >>> ---
> >>>
> >>> v1 -> v2: Fixed commit message title to make CI happy
> >>>
> >>> 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: Queue a HCI power-off command before rfkilling adapters
> >>
> >> Apart from the assumption of RFKILL actually killing the RF
> >> immediately or not, I'm fine with these changes, that said it would be
> >> great if we can have some proper way to test the behavior of rfkill,
> >> perhaps via mgmt-tester, since it should behave like the
> >> MGMT_OP_SET_POWERED.
> >
> > Testing this sounds like a good idea, I guess we'd have to teach
> > mgmt-tester to write to rfkill. The bigger problem seems to be that
> > there's no MGMT event for power changes and also no MGMT_OP_GET_POWERED,
> > so that's a bit concerning, could userspace even be notified about
> > changes to adapter power?
>
> Sent v3 of the patchset now, I didn't add a test to mgmt-tester because
> it's actually quite tricky to notice the full shutdown sequence happened
> rather than just closing the device. As long as no devices are
> connected, the difference is mostly in a few (faily random) events:
>
> btmon without the patch:
>
> @ MGMT Event: Class Of Device Changed (0x0007) plen 3
>
>         {0x0001} [hci0] 169.101804
>          Class: 0x000000
>            Major class: Miscellaneous
>            Minor class: 0x00
> @ MGMT Event: New Settings (0x0006) plen 4
>
>         {0x0001} [hci0] 169.101820
>          Current settings: 0x00000ac0
>            Secure Simple Pairing
>            BR/EDR
>            Low Energy
>            Secure Connections
>
> btmon with the patch:
>
> < HCI Command: Write Scan Enable (0x03|0x001a) plen 1
>
>               #109 [hci0] 7.031852
>          Scan enable: No Scans (0x00)
>  > HCI Event: Command Complete (0x0e) plen 4
>
>                #110 [hci0] 7.033026
>        Write Scan Enable (0x03|0x001a) ncmd 1
>          Status: Success (0x00)
> < HCI Command: LE Set Extended Advertising Enable (0x08|0x0039) plen 2
>
>               #111 [hci0] 7.033055
>          Extended advertising: Disabled (0x00)
>          Number of sets: Disable all sets (0x00)
>  > HCI Event: Command Complete (0x0e) plen 4
>
>                #112 [hci0] 7.034202
>        LE Set Extended Advertising Enable (0x08|0x0039) ncmd 1
>          Status: Success (0x00)
> < HCI Command: LE Clear Advertising Sets (0x08|0x003d) plen 0
>
>               #113 [hci0] 7.034233
>  > HCI Event: Command Complete (0x0e) plen 4
>
>                #114 [hci0] 7.035527
>        LE Clear Advertising Sets (0x08|0x003d) ncmd 1
>          Status: Success (0x00)
> @ MGMT Event: Class Of Device Changed (0x0007) plen 3
>
>           {0x0001} [hci0] 7.035554
>          Class: 0x000000
>            Major class: Miscellaneous
>            Minor class: 0x00
> @ MGMT Event: New Settings (0x0006) plen 4
>
>           {0x0001} [hci0] 7.035568
>          Current settings: 0x00000ac0
>            Secure Simple Pairing
>            BR/EDR
>            Low Energy
>            Secure Connections
>
> Maybe we could add a fake connection and check whether that is
> disconnected on the rfkill, but I don't think mgmt-tester supports that..
>
> Fwiw, I don't think having a test for this is super important, this is a
> regression a lot of people would notice very quickly I think.

Afaik we did something similar to suspend to test its sequence when
suspending while connected, I will look it up tomorrow since I
responding from my phone.

> >
> > Another thing I'm thinking about now is that queuing the HCI command
> > using hci_cmd_sync_queue() might not be enough: The command is still
> > executed async in a thread, and we won't actually block until it has
> > been sent, so this might be introducing a race (rfkill could kill the
> > adapter before we actually send the HCI command). The proper way might
> > be to use a completion and wait until the
> > set_powered_off_sync_complete() callback is invoked?
> >
> >>
> >>>   include/net/bluetooth/hci.h |  2 +-
> >>>   net/bluetooth/hci_core.c    | 33 ++++++++++++++++++++++++++++++---
> >>>   net/bluetooth/hci_sync.c    | 16 +++++++++++-----
> >>>   net/bluetooth/mgmt.c        | 30 ++++++++++++++----------------
> >>>   4 files changed, 56 insertions(+), 25 deletions(-)
> >>>
> >>> --
> >>> 2.43.0
> >>>
> >>
> >>
> >
> > Cheers,
> > Jonas
>
> Cheers,
> Jonas



-- 
Luiz Augusto von Dentz

Re: [PATCH v2 0/4] Power off HCI devices before rfkilling them

[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 Tue,  2 Jan 2024 19:19:16 +0100 you wrote:
> In theory the firmware is supposed to power off the bluetooth card
> when we use rfkill to block it. 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
> quickly after rfkilling.
> 
> [...]

Here is the summary with links:
  - [v2,1/4] Bluetooth: Remove HCI_POWER_OFF_TIMEOUT
    https://git.kernel.org/bluetooth/bluetooth-next/c/f48705f473ce
  - [v2,2/4] Bluetooth: mgmt: Remove leftover queuing of power_off work
    https://git.kernel.org/bluetooth/bluetooth-next/c/2e7a6a997c9a
  - [v2,3/4] Bluetooth: Add new state HCI_POWERING_DOWN
    https://git.kernel.org/bluetooth/bluetooth-next/c/2b16c80d8011
  - [v2,4/4] Bluetooth: Queue a HCI power-off command before rfkilling adapters
    (no matching commit)

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