| Message ID | 20240107180252.73436-1-verdre@v0yd.nl (mailing list archive) |
|---|---|
| Headers | show |
| Series | Disconnect devices before rfkilling adapter | expand |
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
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!
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
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
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