lkml.org 
[lkml]   [2024]   [Jan]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v2 4/4] Bluetooth: Queue a HCI power-off command before rfkilling adapters
From
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
>>
>
>

\
 
 \ /
  Last update: 2024-01-02 19:50    [W:3.059 / U:0.068 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site