Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 9 Dec 2020 02:13:36 +0100
From:   Pali =?utf-8?B?Um9ow6Fy?= <pali@kernel.org>
To:     Trent Piepho <tpiepho@gmail.com>
Cc:     Joseph Hwang <josephsih@google.com>,
        linux-bluetooth <linux-bluetooth@vger.kernel.org>,
        Marcel Holtmann <marcel@holtmann.org>,
        Luiz Augusto von Dentz <luiz.dentz@gmail.com>,
        chromeos-bluetooth-upstreaming 
        <chromeos-bluetooth-upstreaming@chromium.org>,
        Alain Michaud <alainm@chromium.org>,
        Abhishek Pandit-Subedi <abhishekpandit@chromium.org>,
        "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        Johan Hedberg <johan.hedberg@gmail.com>,
        linux-kernel@vger.kernel.org, netdev@vger.kernel.org
Subject: Re: [PATCH v3 1/2] Bluetooth: btusb: define HCI packet sizes of USB
 Alts
Message-ID: <20201209011336.4qdnnehnz3kdlqid@pali>
References: <20200910060403.144524-1-josephsih@chromium.org>
 <CAHFy418Ln9ONHGVhg513g0v+GxUZMDtLpe5NFONO3HuAZz=r7g@mail.gmail.com>
 <20200923102215.hrfzl7c7q2omeiws@pali>
 <9810329.nUPlyArG6x@zen.local>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <9810329.nUPlyArG6x@zen.local>
User-Agent: NeoMutt/20180716
Precedence: bulk

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.