From: Mattijs Korpershoek <mkorpershoek@baylibre•com>
To: Marcel Holtmann <marcel@holtmann•org>
Cc: Bluez mailing list <linux-bluetooth@vger•kernel.org>,
Sean Wang <sean.wang@mediatek•com>,
Johan Hedberg <johan.hedberg@gmail•com>,
"David S. Miller" <davem@davemloft•net>,
netdev@vger•kernel.org, linux-kernel@vger•kernel.org
Subject: Re: [PATCH] Bluetooth: hci_core: fix init with HCI_QUIRK_NON_PERSISTENT_SETUP
Date: Wed, 16 Oct 2019 12:53:21 -0700 [thread overview]
Message-ID: <87sgnsfowe.fsf@baylibre.com> (raw)
In-Reply-To: <474814D3-A97F-48D1-8268-3D200BE60795@holtmann.org>
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
next prev parent reply other threads:[~2019-10-16 19:53 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-10-04 0:09 [PATCH] Bluetooth: hci_core: fix init with HCI_QUIRK_NON_PERSISTENT_SETUP Mattijs Korpershoek
2019-10-16 19:27 ` Marcel Holtmann
2019-10-16 19:53 ` Mattijs Korpershoek [this message]
2019-10-17 3:20 ` [PATCH v2] Bluetooth: hci_core: fix init for HCI_USER_CHANNEL Mattijs Korpershoek
2019-10-17 5:11 ` Marcel Holtmann
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87sgnsfowe.fsf@baylibre.com \
--to=mkorpershoek@baylibre$(echo .)com \
--cc=davem@davemloft$(echo .)net \
--cc=johan.hedberg@gmail$(echo .)com \
--cc=linux-bluetooth@vger$(echo .)kernel.org \
--cc=linux-kernel@vger$(echo .)kernel.org \
--cc=marcel@holtmann$(echo .)org \
--cc=netdev@vger$(echo .)kernel.org \
--cc=sean.wang@mediatek$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox