Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230322012428.2662664-1-raul.cheleguini@gmail.com> <b48ef5be-a64e-48a1-2b43-6e72cc3a0f9f@molgen.mpg.de>
In-Reply-To: <b48ef5be-a64e-48a1-2b43-6e72cc3a0f9f@molgen.mpg.de>
From:   Raul Cheleguini <raul.cheleguini@gmail.com>
Date:   Wed, 22 Mar 2023 12:07:52 -0300
Message-ID: <CACk7upz0sLrCeb8sm04PD8F0wxXm8yfBfpaXkRae=55F16EMig@mail.gmail.com>
Subject: Re: [PATCH v2] Bluetooth: Partial support for Actions Semi ATS2851
 based devices
To:     Paul Menzel <pmenzel@molgen.mpg.de>
Cc:     marcel@holtmann.org, johan.hedberg@gmail.com, luiz.dentz@gmail.com,
        linux-bluetooth@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi Paul,

On Wed, Mar 22, 2023 at 5:00=E2=80=AFAM Paul Menzel <pmenzel@molgen.mpg.de>=
 wrote:
>
> Dear Raul,
>
>
> Thank you for your patch. You could make the summary a statement by
> adding a verb (in imperative mood):
>
> Add partial support for Actions Semi ATS2851 based devices
>

Thanks Paul, noted it.

> Am 22.03.23 um 02:24 schrieb Raul Cheleguini:
> > The ATS2851 advertises support for commands "Set Random Private Address
> > Timeout" and "Extended Create Connection" but does not actually impleme=
nt
> > them and reply with unknown HCI command.
> >
> > The failed first command blocks the device initialization, and the fail=
ed
> > second command blocks the start of the pairing process.
> >
> > Add these two quirks to unblock the device initialization and to skip
> > the extended create connection command when start pairing.
>
> =E2=80=A6 when start*ing* pairing.

Thanks Paul, noted it too.

>
> > v2: Move the extended create connection quirk to use_ext_conn, edit com=
mit
> > description and add btmon logs.
> >
> > < HCI Command: LE Set Resolvable Private... (0x08|0x002e) plen 2
> >          Timeout: 900 seconds
> >> HCI Event: Command Status (0x0f) plen 4
> >        LE Set Resolvable Private Address Timeout (0x08|0x002e) ncmd 1
> >          Status: Unknown HCI Command (0x01)
> >
> > < HCI Command: LE Extended Create Conn.. (0x08|0x0043) plen 26
> >          Filter policy: Accept list is not used (0x00)
> >          Own address type: Public (0x00)
> >          Peer address type: Random (0x01)
> >          Peer address: DD:5E:B9:FE:49:3D (Static)
> >          Initiating PHYs: 0x01
> >          Entry 0: LE 1M
> >            Scan interval: 60.000 msec (0x0060)
> >            Scan window: 60.000 msec (0x0060)
> >            Min connection interval: 30.00 msec (0x0018)
> >            Max connection interval: 50.00 msec (0x0028)
> >            Connection latency: 0 (0x0000)
> >            Supervision timeout: 420 msec (0x002a)
> >            Min connection length: 0.000 msec (0x0000)
> >            Max connection length: 0.000 msec (0x0000)
> >> HCI Event: Command Status (0x0f) plen 4
> >        LE Extended Create Connection (0x08|0x0043) ncmd 1
> >          Status: Unknown HCI Command (0x01)
>
> The commit message summary/title says =E2=80=9Cpartial support=E2=80=9D. =
What is not
> working?

The pairing process is not working yet. There is some problem, related
to SMP I believe, that results in disconnection when pairing in Linux.

To give context, after I altered part of SMP code I managed to get my mouse
paired in Linux, but it needs more work to understand and isolate the probl=
em.
I plan to gather more details and start a new thread about it.

I chose to mark it as partial support to inform the readers that it is
work in progress, but at the same time leave the device support in a little
better state than before. Is it acceptable?

> I=E2=80=99d also split this commit into two. One for each quirk.

Thanks for suggesting this and reviewing everything else Paul.
I will split it in two and improve the commit messages.

> > Signed-off-by: Raul Cheleguini <raul.cheleguini@gmail.com>
> > ---
> >   drivers/bluetooth/btusb.c        |  2 ++
> >   include/net/bluetooth/hci.h      | 14 ++++++++++++++
> >   include/net/bluetooth/hci_core.h |  3 ++-
> >   net/bluetooth/hci_sync.c         | 10 +++++++++-
> >   4 files changed, 27 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> > index 7382b021f3df..8656ac491f13 100644
> > --- a/drivers/bluetooth/btusb.c
> > +++ b/drivers/bluetooth/btusb.c
> > @@ -4105,7 +4105,9 @@ static int btusb_probe(struct usb_interface *intf=
,
> >               /* Support is advertised, but not implemented */
> >               set_bit(HCI_QUIRK_BROKEN_ERR_DATA_REPORTING, &hdev->quirk=
s);
> >               set_bit(HCI_QUIRK_BROKEN_READ_TRANSMIT_POWER, &hdev->quir=
ks);
> > +             set_bit(HCI_QUIRK_BROKEN_SET_RPA_TIMEOUT, &hdev->quirks);
> >               set_bit(HCI_QUIRK_BROKEN_EXT_SCAN, &hdev->quirks);
> > +             set_bit(HCI_QUIRK_BROKEN_EXT_CREATE_CONN, &hdev->quirks);
> >       }
> >
> >       if (!reset)
> > diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> > index 997107bfc0b1..3ff1681fd2b8 100644
> > --- a/include/net/bluetooth/hci.h
> > +++ b/include/net/bluetooth/hci.h
> > @@ -301,6 +301,20 @@ enum {
> >        * don't actually support features declared there.
> >        */
> >       HCI_QUIRK_BROKEN_LOCAL_EXT_FEATURES_PAGE_2,
> > +
> > +     /*
> > +      * When this quirk is set, the HCI_OP_LE_SET_RPA_TIMEOUT command =
is
> > +      * disabled. This is required for the Actions Semiconductor ATS28=
51
> > +      * controller, which erroneously claim to support it.
>
> controller*s* or claim*s*

Thanks Paul.

>
> > +      */
> > +     HCI_QUIRK_BROKEN_SET_RPA_TIMEOUT,
> > +
> > +     /*
> > +      * When this quirk is set, the HCI_OP_LE_EXT_CREATE_CONN command =
is
> > +      * disabled. This is required for the Actions Semiconductor ATS28=
51
> > +      * controller, which erroneously claim to support it.
> > +      */
> > +     HCI_QUIRK_BROKEN_EXT_CREATE_CONN,
>
> Ditto.

Thanks Paul.