Received-SPF: pass (google.com: best guess record for domain of linux-bluetooth-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20200403133806.246918-1-alainm@chromium.org> <D3C05492-13B9-447C-982B-26B681C9EF8C@holtmann.org>
In-Reply-To: <D3C05492-13B9-447C-982B-26B681C9EF8C@holtmann.org>
From:   Alain Michaud <alainmichaud@google.com>
Date:   Fri, 3 Apr 2020 09:54:24 -0400
Message-ID: <CALWDO_Xy0m4PyAACDAHmf9wmPGwKoW6keYWHy8UKjNcsLF5SDQ@mail.gmail.com>
Subject: Re: [PATCH v1] bluetooth:Adding driver and quirk defs for multi-role LE
To:     Marcel Holtmann <marcel@holtmann.org>
Cc:     Alain Michaud <alainm@chromium.org>,
        BlueZ <linux-bluetooth@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-bluetooth-owner@vger.kernel.org
Precedence: bulk

Hi Marcel,

On Fri, Apr 3, 2020 at 9:49 AM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Alain,
>
> > This change adds the relevant driver and quirk to allow drivers to
> > report that concurrent roles are well supported by the controller.
> >
> > This has historically been disabled as controllers did not reliably
> > support this. In particular, this will be used to relax this condition
> > for controllers that have been well tested and reliable.
> >
> >       /* Most controller will fail if we try to create new connections
> >        * while we have an existing one in slave role.
> >        */
> >       if (hdev->conn_hash.le_num_slave > 0)
> >               return NULL;
> >
> > Signed-off-by: Alain Michaud <alainm@chromium.org>
> > ---
> >
> > drivers/bluetooth/btusb.c   | 2 ++
> > include/net/bluetooth/hci.h | 8 ++++++++
> > 2 files changed, 10 insertions(+)
> >
> > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> > index 3bdec42c9612..22e61a502d40 100644
> > --- a/drivers/bluetooth/btusb.c
> > +++ b/drivers/bluetooth/btusb.c
> > @@ -58,6 +58,8 @@ static struct usb_driver btusb_driver;
> > #define BTUSB_CW6622          0x100000
> > #define BTUSB_MEDIATEK                0x200000
> > #define BTUSB_WIDEBAND_SPEECH 0x400000
> > +#define BTUSB_LE_CONCURRENT_ROLES_SUPPORTED \
> > +                             0x800000
> >
> > static const struct usb_device_id btusb_table[] =3D {
> >       /* Generic Bluetooth USB device */
> > diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> > index 5f60e135aeb6..b2c76cde7cd4 100644
> > --- a/include/net/bluetooth/hci.h
> > +++ b/include/net/bluetooth/hci.h
> > @@ -214,6 +214,14 @@ enum {
> >        * This quirk must be set before hci_register_dev is called.
> >        */
> >       HCI_QUIRK_WIDEBAND_SPEECH_SUPPORTED,
> > +
> > +     /* When this quirk is set, the controller has validated that
> > +      * concurrent LE roles are supported.  This mechanism is necessar=
y
> > +      * as many controllers have been seen has having trouble initiati=
ng
> > +      * a connectable advertisement after at least one connection in
> > +      * central had already been established.
> > +      */
> > +     HCI_QUIRK_LE_CONCURRENT_ROLES_SUPPORTED,
> > };
>
> lets do this the other way around. Lets remove the limitation we have in =
our HCI core code. And then we see which controllers report errors. Trying =
to enable each controller individually is cumbersome. I rather later on bla=
cklist the one or two historic controllers that don=E2=80=99t support it.
>

I agree it's cumbersome but given we don't have much data or existing
tests around this, we don't have too many options.  I strongly
disagree with the approach of simply enabling it and seeing what
breaks as scenarios using this functionality are expected to scale out
quickly, likely before we have time to fine all controller issues.

My team is building test infrastructure to help collect data with
this, we already know it's already problematic on a range of
controllers and we have a good idea on which controllers which will be
set.  We will contribute to the cumbersome part of this.

> Regards
>
> Marcel
>