lkml.org 
[lkml]   [2019]   [Oct]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH] Bluetooth: hci_core: fix init with HCI_QUIRK_NON_PERSISTENT_SETUP
Date

Hi Marcel,

Marcel Holtmann <marcel@holtmann.org> writes:

> Hi Mattijs,
>
>> Some HCI devices which have the HCI_QUIRK_NON_PERSISTENT_SETUP
>> [1]
>> require a call to setup() to be ran after every open().
>>
>> During the setup() stage, these devices expect the chip to
>> acknowledge
>> its setup() completion via vendor specific frames.
>>
>> If userspace opens() such HCI device in HCI_USER_CHANNEL [2]
>> mode,
>> the vendor specific frames are never tranmitted to the driver,
>> as
>> they are filtered in hci_rx_work().
>>
>> Allow HCI devices which have HCI_QUIRK_NON_PERSISTENT_SETUP to
>> process
>> frames if the HCI device is is HCI_INIT state.
>>
>> [1] https://lore.kernel.org/patchwork/patch/965071/
>> [2] https://www.spinics.net/lists/linux-bluetooth/msg37345.html
>>
>> Fixes: 740011cfe948 ("Bluetooth: Add new quirk for
>> non-persistent setup settings")
>> Signed-off-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
>> ---
>> Some more background on the change follows:
>>
>> The Android bluetooth stack (Bluedroid) also has a HAL
>> implementation
>> which follows Linux's standard rfkill interface [1].
>>
>> This implementation relies on the HCI_CHANNEL_USER feature to
>> get
>> exclusive access to the underlying bluetooth device.
>>
>> When testing this along with the btkmtksdio driver, the
>> chip appeared unresponsive when calling the following from
>> userspace:
>>
>> struct sockaddr_hci addr;
>> int fd;
>>
>> fd = socket(AF_BLUETOOTH, SOCK_RAW, BTPROTO_HCI);
>>
>> memset(&addr, 0, sizeof(addr));
>> addr.hci_family = AF_BLUETOOTH;
>> addr.hci_dev = 0;
>> addr.hci_channel = HCI_CHANNEL_USER;
>>
>> bind(fd, (struct sockaddr *) &addr, sizeof(addr)); # device
>> hangs
>>
>> In the case of bluetooth drivers exposing
>> QUIRK_NON_PERSISTENT_SETUP
>> such as btmtksdio, setup() is called each multiple times.
>> In particular, when userspace calls bind(), the setup() is
>> called again
>> and vendor specific commands might be send to re-initialize the
>> chip.
>>
>> Those commands are filtered out by hci_core in HCI_CHANNEL_USER
>> mode,
>> preventing setup() from completing successfully.
>>
>> This has been tested on a 4.19 kernel based on Android Common
>> Kernel.
>> It has also been compile tested on bluetooth-next.
>>
>> [1]
>> https://android.googlesource.com/platform/system/bt/+/refs/heads/master/vendor_libs/linux/interface/
>>
>> net/bluetooth/hci_core.c | 15 +++++++++++++--
>> 1 file changed, 13 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/bluetooth/hci_core.c
>> b/net/bluetooth/hci_core.c
>> index 04bc79359a17..5f12e8574d54 100644
>> --- a/net/bluetooth/hci_core.c
>> +++ b/net/bluetooth/hci_core.c
>> @@ -4440,9 +4440,20 @@ static void hci_rx_work(struct
>> work_struct *work)
>> hci_send_to_sock(hdev, skb);
>> }
>>
>> + /* If the device has been opened in HCI_USER_CHANNEL,
>> + * the userspace has exclusive access to device.
>> + * When HCI_QUIRK_NON_PERSISTENT_SETUP is set and
>> + * device is HCI_INIT, we still need to process
>> + * the data packets to the driver in order
>> + * to complete its setup().
>> + */
>> if (hci_dev_test_flag(hdev, HCI_USER_CHANNEL)) {
>> - kfree_skb(skb);
>> - continue;
>> + if (!test_bit(HCI_QUIRK_NON_PERSISTENT_SETUP,
>> + &hdev->quirks) ||
>> + !test_bit(HCI_INIT, &hdev->flags)) {
>> + kfree_skb(skb);
>> + continue;
>> + }
>> }
>
> if (hci_dev_test_flag(hdev, HCI_USER_CHANNEL) &&
> !test_bit(HCI_INIT, &hdev->flags)) {
> kfree_skb(skb);
> continue;
> }
>
> Wouldn’t it be enough to just add a check for HCI_INIT to this.
> I mean it makes no difference if ->setup is repeated on each
> device open or not. We want to process event during HCI_INIT
> when in user channel mode.
Thank you for your review. You are right. We always want to
process
events during HCI_INIT in user channel mode.

I'll send a v2

Regards,
Mattijs

>
> Regards
>
> Marcel

\
 
 \ /
  Last update: 2019-10-16 21:54    [W:0.085 / U:3.056 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site