2024-01-02 18:20:12

by Jonas Dreßler

[permalink] [raw]
Subject: [PATCH v2 0/4] Power off HCI devices before rfkilling them

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



2024-01-02 18:21:35

by Jonas Dreßler

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

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 <[email protected]>
---
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


2024-01-03 12:16:16

by Jonas Dreßler

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

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 <[email protected]> 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