Re: [PATCH v3 1/2] Bluetooth: btusb: define HCI packet sizes of USB Alts

[Trent Piepho <tpiepho@gmail.com>]

On Wednesday, September 23, 2020 3:22:15 AM PST Pali Rohár wrote:
> On Monday 14 September 2020 20:18:27 Joseph Hwang wrote:
> > On Thu, Sep 10, 2020 at 4:18 PM Pali Rohár <pali@kernel.org> wrote:
> > > And this part of code which you write is Realtek specific.
> > 
> > We currently only have Intel and Realtek platforms to test with. If
> > making it generic without proper testing platforms is fine, I will
> > make it generic. Or do you think it might be better to make it
> > customized with particular vendors for now; and make it generic later
> > when it works well with sufficient vendors?
> 
> I understood that those packet size changes are generic to bluetooth
> specification and therefore it is not vendor specific code. Those packet
> sizes for me really seems to be USB specific.
> 
> Therefore it should apply for all vendors, not only for Realtek and
> Intel.

I have tried to test WBS with some different USB adapters.  So far, all use 
these packet sizes.  Tested were:

Broadcom BRCM20702A
Realtek RTL8167B
Realtek RTL8821C
CSR CSR8510 (probably fake)

In all cases, WBS works best with packet size of (USB packet size for alt mode 
selected) * 3 packets - 3 bytes HCI header.  None of these devices support alt 
6 mode, where supposedly one packet is better, but I can find no BT adapter on 
which to test this.

> +static const int hci_packet_size_usb_alt[] = { 0, 24, 48, 72, 96, 144, 60};

Note that the packet sizes here are based on the max isoc packet length for 
the USB alt mode used, e.g. alt 1 is 9 bytes.  That value is only a 
"recommended" value from the bluetooth spec.  It seems like it would be more 
correct use (btusb_data*)->isoc_tx_ep->wMaxPacketSize to find the MTU.

> > [Issue 2] The btusb_work() is performed by a worker. There would be a
> > timing issue here if we let btusb_work() to do “hdev->sco_mtu =
> > hci_packet_size_usb_alt[i]” because there is no guarantee how soon the
> > btusb_work() can be finished and get “hdev->sco_mtu” value set
> > correctly. In order to avoid the potential race condition, I suggest
> > to determine air_mode in btusb_notify() before
> > schedule_work(&data->work) is executed so that “hdev->sco_mtu =
> > hci_packet_size_usb_alt[i]” is guaranteed to be performed when
> > btusb_notify() finished. In this way, hci_sync_conn_complete_evt() can
> > set conn->mtu correctly as described in [Issue 1] above.

Does this also give userspace a clear point at which to determine MTU setting, 
_before_ data is sent over SCO connection?  It will not work if sco_mtu is not 
valid until after userspace sends data to SCO connection with incorrect mtu.