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: <20220330182552.2613523-1-luiz.dentz@gmail.com>
 <20220330182552.2613523-2-luiz.dentz@gmail.com> <6AFB6D57-1342-445D-A9CC-C761DB356E16@holtmann.org>
In-Reply-To: <6AFB6D57-1342-445D-A9CC-C761DB356E16@holtmann.org>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Fri, 1 Apr 2022 14:01:34 -0700
Message-ID: <CABBYNZKtvNxPaBfKfjh_V7j-=1gJGXytz--Qcm4e_pSHzr+2Vg@mail.gmail.com>
Subject: Re: [PATCH v3 2/3] Bluetooth: Print broken quirks
To:     Marcel Holtmann <marcel@holtmann.org>
Cc:     "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Marcel,

On Fri, Apr 1, 2022 at 10:29 AM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Luiz,
>
> > 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 | 53 +++++++++++++++++++++++++++++++++++++---
> > 1 file changed, 49 insertions(+), 4 deletions(-)
> >
> > diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
> > index 8f4c5698913d..e97880d7bdb2 100644
> > --- a/net/bluetooth/hci_sync.c
> > +++ b/net/bluetooth/hci_sync.c
> > @@ -3825,6 +3825,54 @@ static int hci_init_sync(struct hci_dev *hdev)
> >       return 0;
> > }
> >
> > +#define HCI_QUIRK_BROKEN(_quirk, _desc) { HCI_QUIRK_BROKEN_##_quirk, _desc }
> > +
> > +static const struct {
> > +     unsigned long quirk;
> > +     const char *desc;
> > +} hci_broken_table[] = {
> > +     HCI_QUIRK_BROKEN(LOCAL_COMMANDS,
> > +                      "HCI Read Local Supported Commands not supported"),
> > +     HCI_QUIRK_BROKEN(STORED_LINK_KEY,
> > +                      "HCI Delete Stored Link Key command is advertised, "
> > +                      "but not supported."),
> > +     HCI_QUIRK_BROKEN(ERR_DATA_REPORTING,
> > +                      "HCI Read Default Erroneous Data Reporting command is "
> > +                      "advertised, but not supported."),
> > +     HCI_QUIRK_BROKEN(READ_TRANSMIT_POWER,
> > +                      "HCI Read Transmit Power Level command is advertised, "
> > +                      "but not supported."),
> > +     HCI_QUIRK_BROKEN(FILTER_CLEAR_ALL,
> > +                      "HCI Set Event Filter command not supported."),
> > +     HCI_QUIRK_BROKEN(ENHANCED_SETUP_SYNC_CONN,
> > +                      "HCI Enhanced Setup Synchronous Connection command is "
> > +                      "advertised, but not supported.")
> > +};
> > +
> > +static int hci_dev_setup_sync(struct hci_dev *hdev)
> > +{
> > +     size_t i;
> > +
> > +     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;
> > +     }
> > +
> > +     for (i = 0; i < ARRAY_SIZE(hci_broken_table); i++) {
> > +             if (test_bit(hci_broken_table[i].quirk, &hdev->quirks))
> > +                     bt_dev_warn(hdev, "%s", hci_broken_table[i].desc);
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > int hci_dev_open_sync(struct hci_dev *hdev)
> > {
> >       int ret = 0;
> > @@ -3887,10 +3935,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);
>
> just put the code here instead of creating another helper function.

Ive added another function since hci_dev_open_sync is quite big
already and it easier to find out where we call ->setup if it is a
separated function that does perform all the necessary checks and set
HCI_DEV_SETUP, I was actually considering moving the whole block under
the if statement so we avoid this weird goto setup_failed, but perhaps
you want any rewrite to be on its own patch to make it easier to
bisect?

> Regards
>
> Marcel
>


-- 
Luiz Augusto von Dentz