MIME-Version: 1.0
References: <20230317204620.2809181-1-raul.cheleguini@gmail.com>
In-Reply-To: <20230317204620.2809181-1-raul.cheleguini@gmail.com>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Fri, 17 Mar 2023 13:53:48 -0700
Message-ID: <CABBYNZ+tW=w74V8bOYtjcwG=RUUKxSBKGkpe+w8A4gohCWbXZg@mail.gmail.com>
Subject: Re: [PATCH] Bluetooth: Partial support for Actions Semi ATS2851 based devices
To:     Raul Cheleguini <raul.cheleguini@gmail.com>
Cc:     linux-bluetooth@vger.kernel.org, marcel@holtmann.org,
        johan.hedberg@gmail.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi Raul,

On Fri, Mar 17, 2023 at 1:46=E2=80=AFPM Raul Cheleguini
<raul.cheleguini@gmail.com> wrote:
>
> This change enables support to device initialization and to scan
> process, by adding two new quirks for the following advertised but
> unsupported commands: "Set Random Private Address Timeout" and
> "Extended Create Connection".
>
> It offers partial device support: controller initialization and scan.
> The pairing process still needs work.
>
> At the moment there is little to none available documentation about the
> ATS2851 and its based USB dongles. It is known that it works in other
> systems via generic drivers, and this is one step towards have it working
> in Linux.
>
> Signed-off-by: Raul Cheleguini <raul.cheleguini@gmail.com>
> ---
>  drivers/bluetooth/btusb.c   |  2 ++
>  include/net/bluetooth/hci.h | 14 ++++++++++++++
>  net/bluetooth/hci_sync.c    | 13 +++++++++++--
>  3 files changed, 27 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index 7382b021f3df..7bba19061277 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -4106,6 +4106,8 @@ static int btusb_probe(struct usb_interface *intf,
>                 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_EXT_SCAN, &hdev->quirks);
> +               set_bit(HCI_QUIRK_BROKEN_SET_RPA_TIMEOUT, &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.
> +        */
> +       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,
>  };
>
>  /* HCI device flags */
> diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
> index 8e5fe73873a8..9b2a0d6d6c1a 100644
> --- a/net/bluetooth/hci_sync.c
> +++ b/net/bluetooth/hci_sync.c
> @@ -4090,9 +4090,11 @@ static int hci_le_set_rpa_timeout_sync(struct hci_=
dev *hdev)
>  {
>         __le16 timeout =3D cpu_to_le16(hdev->rpa_timeout);
>
> -       if (!(hdev->commands[35] & 0x04))
> +       if (!(hdev->commands[35] & 0x04) ||
> +           test_bit(HCI_QUIRK_BROKEN_SET_RPA_TIMEOUT, &hdev->quirks))
>                 return 0;

This one Im not so sure, if we can't set a timeout then we shouldn't
use the controller to rotate the RPA, although it seems we are already
doing it when the command bit is not set.

>
> +
>         return __hci_cmd_sync_status(hdev, HCI_OP_LE_SET_RPA_TIMEOUT,
>                                      sizeof(timeout), &timeout,
>                                      HCI_CMD_TIMEOUT);
> @@ -4530,6 +4532,12 @@ static const struct {
>                          "HCI Set Event Filter command not supported."),
>         HCI_QUIRK_BROKEN(ENHANCED_SETUP_SYNC_CONN,
>                          "HCI Enhanced Setup Synchronous Connection comma=
nd is "
> +                         "advertised, but not supported."),
> +       HCI_QUIRK_BROKEN(SET_RPA_TIMEOUT,
> +                        "HCI LE Set Random Private Address Timeout comma=
nd is "
> +                        "advertised, but not supported."),
> +       HCI_QUIRK_BROKEN(EXT_CREATE_CONN,
> +                        "HCI LE Extended Create Connection command is "
>                          "advertised, but not supported.")
>  };
>
> @@ -6067,7 +6075,8 @@ int hci_le_create_conn_sync(struct hci_dev *hdev, s=
truct hci_conn *conn)
>         if (err)
>                 goto done;
>
> -       if (use_ext_conn(hdev)) {
> +       if (use_ext_conn(hdev) &&
> +           !test_bit(HCI_QUIRK_BROKEN_EXT_CREATE_CONN, &hdev->quirks)) {
>                 err =3D hci_le_ext_create_conn_sync(hdev, conn, own_addr_=
type);
>                 goto done;
>         }

I guess we can incorporate the new quirk check inside use_ext_conn.

> --
> 2.39.2
>


--=20
Luiz Augusto von Dentz