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;
From:   Trent Piepho <tpiepho@gmail.com>
To:     Joseph Hwang <josephsih@google.com>,
        linux-bluetooth <linux-bluetooth@vger.kernel.org>
Cc:     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,
        Pali =?ISO-8859-1?Q?Roh=E1r?= <pali@kernel.org>
Subject: Re: [PATCH v3 1/2] Bluetooth: btusb: define HCI packet sizes of USB Alts
Date:   Tue, 08 Dec 2020 15:04:29 -0800
Message-ID: <9810329.nUPlyArG6x@zen.local>
In-Reply-To: <20200923102215.hrfzl7c7q2omeiws@pali>
References: <20200910060403.144524-1-josephsih@chromium.org> <CAHFy418Ln9ONHGVhg513g0v+GxUZMDtLpe5NFONO3HuAZz=r7g@mail.gmail.com> <20200923102215.hrfzl7c7q2omeiws@pali>
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wednesday, September 23, 2020 3:22:15 AM PST Pali Roh=C3=A1r wrote:
> On Monday 14 September 2020 20:18:27 Joseph Hwang wrote:
> > On Thu, Sep 10, 2020 at 4:18 PM Pali Roh=C3=A1r <pali@kernel.org> wrote:
> > > And this part of code which you write is Realtek specific.
> >=20
> > 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?
>=20
> 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.
>=20
> 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=
=20
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 m=
ode=20
selected) * 3 packets - 3 bytes HCI header.  None of these devices support =
alt=20
6 mode, where supposedly one packet is better, but I can find no BT adapter=
 on=20
which to test this.

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

Note that the packet sizes here are based on the max isoc packet length for=
=20
the USB alt mode used, e.g. alt 1 is 9 bytes.  That value is only a=20
"recommended" value from the bluetooth spec.  It seems like it would be mor=
e=20
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 =E2=80=9Chdev->sco_mtu =
=3D
> > hci_packet_size_usb_alt[i]=E2=80=9D because there is no guarantee how s=
oon the
> > btusb_work() can be finished and get =E2=80=9Chdev->sco_mtu=E2=80=9D va=
lue 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 =E2=80=9Chdev->sco_mtu =
=3D
> > hci_packet_size_usb_alt[i]=E2=80=9D 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 setti=
ng,=20
_before_ data is sent over SCO connection?  It will not work if sco_mtu is =
not=20
valid until after userspace sends data to SCO connection with incorrect mtu.