[v2,1/3] Bluetooth: HCI: Add HCI_QUIRK_BROKEN_ENHANCED_SETUP_SYNC_CONN quirk

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

This adds HCI_QUIRK_BROKEN_ENHANCED_SETUP_SYNC_CONN quirk which can be
used to mark HCI_Enhanced_Setup_Synchronous_Connection as broken even
if its support command bit are set since some controller report it as
supported but the command don't work properly with some configurations
(e.g. BT_VOICE_TRANSPARENT/mSBC).

Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
---
v2: Change hci_broken/hci_quirk_broken and HCI_BROKEN/HCI_QUIRK_BROKEN and add
patch 3/3 setting HCI_QUIRK_BROKEN_ENHANCED_SETUP_SYNC_CONN for QCA
controllers.

 include/net/bluetooth/hci.h      | 9 +++++++++
 include/net/bluetooth/hci_core.h | 8 ++++++--
 net/bluetooth/hci_conn.c         | 2 +-
 net/bluetooth/sco.c              | 2 +-
 4 files changed, 17 insertions(+), 4 deletions(-)

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=627309

---Test result---

Test Summary:
CheckPatch                    FAIL      2.56 seconds
GitLint                       PASS      1.51 seconds
SubjectPrefix                 PASS      1.00 seconds
BuildKernel                   PASS      42.17 seconds
BuildKernel32                 PASS      37.86 seconds
Incremental Build with patchesPASS      66.78 seconds
TestRunner: Setup             PASS      637.22 seconds
TestRunner: l2cap-tester      PASS      19.72 seconds
TestRunner: bnep-tester       PASS      8.15 seconds
TestRunner: mgmt-tester       PASS      133.25 seconds
TestRunner: rfcomm-tester     PASS      10.46 seconds
TestRunner: sco-tester        PASS      10.34 seconds
TestRunner: smp-tester        PASS      10.30 seconds
TestRunner: userchan-tester   PASS      8.39 seconds

Details
##############################
Test: CheckPatch - FAIL - 2.56 seconds
Run checkpatch.pl script with rule in .checkpatch.conf
[v2,2/3] Bluetooth: Print broken quirks\WARNING:SPLIT_STRING: quoted string split across lines
#113: FILE: net/bluetooth/hci_sync.c:3842:
+			 "HCI Delete Stored Link Key command is advertised, "
+			 "but not supported."),

WARNING:SPLIT_STRING: quoted string split across lines
#116: FILE: net/bluetooth/hci_sync.c:3845:
+			 "HCI Read Default Erroneous Data Reporting command is "
+			 "advertised, but not supported."),

WARNING:SPLIT_STRING: quoted string split across lines
#119: FILE: net/bluetooth/hci_sync.c:3848:
+			 "HCI Read Transmit Power Level command is advertised, "
+			 "but not supported."),

WARNING:SPLIT_STRING: quoted string split across lines
#124: FILE: net/bluetooth/hci_sync.c:3853:
+			 "HCI Enhanced Setup Synchronous Connection command is "
+			 "advertised, but not supported.")

total: 0 errors, 4 warnings, 0 checks, 78 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

/github/workspace/src/12795219.patch has style problems, please review.

NOTE: Ignored message types: UNKNOWN_COMMIT_ID

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.




---
Regards,
Linux Bluetooth

Hi Paul,

On Tue, Mar 29, 2022 at 11:13 PM Paul Menzel <pmenzel@molgen.mpg.de> wrote:
>
> Dear Luiz,
>
>
> Thank you for your patch.
>
> Maybe as commit message summary:
>
> Warn about broken quirks
>
> Am 29.03.22 um 22:16 schrieb Luiz Augusto von Dentz:
> > From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> >
> > This prints warnings for controllers setting broken quirks to increase
> > their visibility and warn about broken controllers firmware that
> > probably needs updates to behave properly.
> >
> > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> > ---
> >   net/bluetooth/hci_sync.c | 66 +++++++++++++++++++++++++++++++++++++---
> >   1 file changed, 62 insertions(+), 4 deletions(-)
> >
> > diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
> > index 8f4c5698913d..8994ff1f94e6 100644
> > --- a/net/bluetooth/hci_sync.c
> > +++ b/net/bluetooth/hci_sync.c
> > @@ -3825,6 +3825,67 @@ static int hci_init_sync(struct hci_dev *hdev)
> >       return 0;
> >   }
> >
> > +#define HCI_QUIRK_BROKEN(_quirk, _desc) \
> > +{ \
> > +     .quirk = _quirk, \
> > +     .desc = _desc, \
> > +}
> > +
> > +static const struct hci_quirk_broken {
> > +     unsigned long quirk;
> > +     const char *desc;
> > +} hci_broken_table[] = {
> > +     HCI_QUIRK_BROKEN(HCI_QUIRK_BROKEN_LOCAL_COMMANDS,
> > +                      "HCI Read Local Supported Commands not supported"),
>
> The user wouldn’t know, that this is a device firmware problem. Could
> something be added to the warning, like:
>
> Therefore, device firmware VERSION violates the spec. Please contact the
> manufacturer.

We do print out the firmware filename when loading it so I'm not
convinced printing it again would make any difference for the user.

> > +     HCI_QUIRK_BROKEN(HCI_QUIRK_BROKEN_STORED_LINK_KEY,
> > +                      "HCI Delete Stored Link Key command is advertised, "
> > +                      "but not supported."),
> > +     HCI_QUIRK_BROKEN(HCI_QUIRK_BROKEN_ERR_DATA_REPORTING,
> > +                      "HCI Read Default Erroneous Data Reporting command is "
> > +                      "advertised, but not supported."),
> > +     HCI_QUIRK_BROKEN(HCI_QUIRK_BROKEN_READ_TRANSMIT_POWER,
> > +                      "HCI Read Transmit Power Level command is advertised, "
> > +                      "but not supported."),
> > +     HCI_QUIRK_BROKEN(HCI_QUIRK_BROKEN_FILTER_CLEAR_ALL,
> > +                      "HCI Set Event Filter command not supported."),
> > +     HCI_QUIRK_BROKEN(HCI_QUIRK_BROKEN_ENHANCED_SETUP_SYNC_CONN,
> > +                      "HCI Enhanced Setup Synchronous Connection command is "
> > +                      "advertised, but not supported.")
> > +};
> > +
> > +static void hci_dev_print_broken_quirks(struct hci_dev *hdev)
> > +{
> > +     int i;
>
> I’d use `unsigned int`, but no idea, what the subsystem does regarding
> counting variables.

I suppose size_t would be better given that sizeof is used.

>
> Kind regards,
>
> Paul
>
>
> > +
> > +     bt_dev_dbg(hdev, "");
> > +
> > +     for (i = 0; i < ARRAY_SIZE(hci_broken_table); i++) {
> > +             const struct hci_quirk_broken *broken = &hci_broken_table[i];
> > +
> > +             if (test_bit(broken->quirk, &hdev->quirks))
> > +                     bt_dev_warn(hdev, "%s", broken->desc);
> > +     }
> > +}
> > +
> > +static int hci_dev_setup_sync(struct hci_dev *hdev)
> > +{
> > +     bt_dev_dbg(hdev, "");
> > +
> > +     hci_sock_dev_event(hdev, HCI_DEV_SETUP);
> > +
> > +     if (hdev->setup) {
> > +             int ret;
> > +
> > +             ret = hdev->setup(hdev);
> > +             if (ret)
> > +                     return ret;
> > +
> > +             hci_dev_print_broken_quirks(hdev);
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> >   int hci_dev_open_sync(struct hci_dev *hdev)
> >   {
> >       int ret = 0;
> > @@ -3887,10 +3948,7 @@ int hci_dev_open_sync(struct hci_dev *hdev)
> >           test_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks)) {
> >               bool invalid_bdaddr;
> >
> > -             hci_sock_dev_event(hdev, HCI_DEV_SETUP);
> > -
> > -             if (hdev->setup)
> > -                     ret = hdev->setup(hdev);
> > +             ret = hci_dev_setup_sync(hdev);
> >
> >               /* The transport driver can set the quirk to mark the
> >                * BD_ADDR invalid before creating the HCI device or in