Re: [PATCH] Bluetooth: hci_core: fix init with HCI_QUIRK_NON_PERSISTENT_SETUP

[Mattijs Korpershoek <mkorpershoek@baylibre.com>]

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