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

[Mattijs Korpershoek]

Hi Marcel,

Marcel Holtmann <marcel@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> > ---
> > 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