[v3,0/2] To support the HFP WBS, a chip vendor may choose a particular

USB alternate seeting of which the packet size is distinct.
The patches are to expose the packet size to user space so that
the user space does not need to hard code those values.

We have verified this patch on Chromebooks which use
- Realtek 8822CE controller with USB alt setting 1
- Intel controller with USB alt setting 6
Our user space audio server, cras, can get the correct
packet length from the socket option.

Changes in v3:
- Set hdev->sco_mtu to rp->sco_mtu if the latter is smaller.
- Fixed the commit message.

Changes in v2:
- Used sco_mtu instead of a new sco_pkt_len member in hdev.
- Do not overwrite hdev->sco_mtu in hci_cc_read_buffer_size
  if it has been set in the USB interface.
- Used BT_SNDMTU/BT_RCVMTU instead of creating a new opt name.
- Used the existing conn->mtu instead of creating a new member
  in struct sco_pinfo.
- Noted that the old SCO_OPTIONS in sco_sock_getsockopt_old()
  would just work as it uses sco_pi(sk)->conn->mtu.

Joseph Hwang (2):
  Bluetooth: btusb: define HCI packet sizes of USB Alts
  Bluetooth: sco: new getsockopt options BT_SNDMTU/BT_RCVMTU

 drivers/bluetooth/btusb.c | 45 +++++++++++++++++++++++++++++----------
 net/bluetooth/hci_event.c | 14 +++++++++++-
 net/bluetooth/sco.c       |  6 ++++++
 3 files changed, 53 insertions(+), 12 deletions(-)

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.

On Tuesday 08 December 2020 15:04:29 Trent Piepho wrote:
> 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.

Yea, wMaxPacketSize looks like a candidate for determining MTU. Can we
use it or are there any known issues with it?

> > > [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.

IIRC connection is established after sync connection (SCO) complete
event. And sending data is possible after connection is established. So
based on these facts I think that userspace can determinate MTU settings
prior sending data over SCO socket.

Anyway, to whole MTU issue for SCO there is a nice workaround which
worked fine with more tested USB adapters and headsets. As SCO socket is
synchronous and most bluetooth headsets have own clocks, you can
synchronize sending packets to headsets based on time events when you
received packets from other side and also send packets of same size as
you received. I.e. for every received packet send own packet of the same
size.

On Tuesday, December 8, 2020 5:13:36 PM PST Pali Rohár wrote:
> On Tuesday 08 December 2020 15:04:29 Trent Piepho wrote:
> > 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.
> 
> IIRC connection is established after sync connection (SCO) complete
> event. And sending data is possible after connection is established. So
> based on these facts I think that userspace can determinate MTU settings
> prior sending data over SCO socket.
> 
> Anyway, to whole MTU issue for SCO there is a nice workaround which
> worked fine with more tested USB adapters and headsets. As SCO socket is
> synchronous and most bluetooth headsets have own clocks, you can
> synchronize sending packets to headsets based on time events when you
> received packets from other side and also send packets of same size as
> you received. I.e. for every received packet send own packet of the same
> size.

As I understand it, the RX side from the headset is not guaranteed, so in 
the TX only case this will not work and we still need to be told what MTU 
kernel has selected for the SCO link.

It seems also it would add some latency to start up, since it would be 
necessary to wait for packets to arrive before knowing what size packet to 
send.

Would timing based on matching TX to RX in the case of packet loss on RX 
side?

On Wednesday 09 December 2020 16:19:39 Trent Piepho wrote:
> On Tuesday, December 8, 2020 5:13:36 PM PST Pali Rohár wrote:
> > On Tuesday 08 December 2020 15:04:29 Trent Piepho wrote:
> > > 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.
> > 
> > IIRC connection is established after sync connection (SCO) complete
> > event. And sending data is possible after connection is established. So
> > based on these facts I think that userspace can determinate MTU settings
> > prior sending data over SCO socket.
> > 
> > Anyway, to whole MTU issue for SCO there is a nice workaround which
> > worked fine with more tested USB adapters and headsets. As SCO socket is
> > synchronous and most bluetooth headsets have own clocks, you can
> > synchronize sending packets to headsets based on time events when you
> > received packets from other side and also send packets of same size as
> > you received. I.e. for every received packet send own packet of the same
> > size.
> 
> As I understand it, the RX side from the headset is not guaranteed, so in 
> the TX only case this will not work and we still need to be told what MTU 
> kernel has selected for the SCO link.

I'm not sure if TX-only SCO link is possible. I always thought that SCO
is synchronous bidirectional link.

As I said, this "workaround" is useful for classic bluetooth headsets
and is it possible to use it immediately without any kernel changes.

And I agree that kernel should tell userspace correct MTU value. And
this should be fixed. "Workaround" is useful for immediate action to
deliver at least something which works with most bluetooth headsets.

> It seems also it would add some latency to start up, since it would be 
> necessary to wait for packets to arrive before knowing what size packet to 
> send.

I think this startup latency is negligible in HFP profile where start
needs non-trivial exchange of AT commands.

> Would timing based on matching TX to RX in the case of packet loss on RX 
> side?

That is a good question for some research. I remember that e.g.
pulseaudio used this technique for synchronizing bluetooth SCO RX and TX
streams.