linux-kernel - Re: [PATCH] Bluetooth: hci_h5: Add RTL8822CS capabilities

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAJQfnxHg50mKGVpQoH-dobphAzpFwyc2gQMzVkLZeNUW0Yyh3Q@mail.gmail.com>
Date:   Fri, 14 May 2021 19:40:52 +0800
From:   Archie Pusaka <apusaka@...gle.com>
To:     Marcel Holtmann <marcel@...tmann.org>
Cc:     linux-bluetooth <linux-bluetooth@...r.kernel.org>,
        CrosBT Upstreaming <chromeos-bluetooth-upstreaming@...omium.org>,
        Archie Pusaka <apusaka@...omium.org>,
        Abhishek Pandit-Subedi <abhishekpandit@...omium.org>,
        Johan Hedberg <johan.hedberg@...il.com>,
        Luiz Augusto von Dentz <luiz.dentz@...il.com>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] Bluetooth: hci_h5: Add RTL8822CS capabilities

Hi Marcel,

On Fri, 14 May 2021 at 03:03, Marcel Holtmann <marcel@...tmann.org> wrote:
>
> Hi Archie,
>
> >>> RTL8822 chipset supports WBS, and this information is conveyed in
> >>> btusb.c. However, the UART driver doesn't have this information just
> >>> yet.
> >>>
> >>> Signed-off-by: Archie Pusaka <apusaka@...omium.org>
> >>> Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@...omium.org>
> >>> ---
> >>>
> >>> drivers/bluetooth/btrtl.c  | 26 ++++++++++++++++----------
> >>> drivers/bluetooth/btrtl.h  |  2 ++
> >>> drivers/bluetooth/hci_h5.c |  5 +----
> >>> 3 files changed, 19 insertions(+), 14 deletions(-)
> >>>
> >>> diff --git a/drivers/bluetooth/btrtl.c b/drivers/bluetooth/btrtl.c
> >>> index e7fe5fb22753..988a09860c6b 100644
> >>> --- a/drivers/bluetooth/btrtl.c
> >>> +++ b/drivers/bluetooth/btrtl.c
> >>> @@ -719,17 +719,8 @@ int btrtl_download_firmware(struct hci_dev *hdev,
> >>> }
> >>> EXPORT_SYMBOL_GPL(btrtl_download_firmware);
> >>>
> >>> -int btrtl_setup_realtek(struct hci_dev *hdev)
> >>> +void btrtl_set_quirks(struct hci_dev *hdev, struct btrtl_device_info *btrtl_dev)
> >>> {
> >>> -     struct btrtl_device_info *btrtl_dev;
> >>> -     int ret;
> >>> -
> >>> -     btrtl_dev = btrtl_initialize(hdev, NULL);
> >>> -     if (IS_ERR(btrtl_dev))
> >>> -             return PTR_ERR(btrtl_dev);
> >>> -
> >>> -     ret = btrtl_download_firmware(hdev, btrtl_dev);
> >>> -
> >>>      /* Enable controller to do both LE scan and BR/EDR inquiry
> >>>       * simultaneously.
> >>>       */
> >>> @@ -750,6 +741,21 @@ int btrtl_setup_realtek(struct hci_dev *hdev)
> >>>              rtl_dev_dbg(hdev, "WBS supported not enabled.");
> >>>              break;
> >>>      }
> >>> +}
> >>> +EXPORT_SYMBOL_GPL(btrtl_set_quirks);
> >>> +
> >>> +int btrtl_setup_realtek(struct hci_dev *hdev)
> >>> +{
> >>> +     struct btrtl_device_info *btrtl_dev;
> >>> +     int ret;
> >>> +
> >>> +     btrtl_dev = btrtl_initialize(hdev, NULL);
> >>> +     if (IS_ERR(btrtl_dev))
> >>> +             return PTR_ERR(btrtl_dev);
> >>> +
> >>> +     ret = btrtl_download_firmware(hdev, btrtl_dev);
> >>> +
> >>> +     btrtl_set_quirks(hdev, btrtl_dev);
> >>>
> >>>      btrtl_free(btrtl_dev);
> >>>      return ret;
> >>> diff --git a/drivers/bluetooth/btrtl.h b/drivers/bluetooth/btrtl.h
> >>> index 2a582682136d..260167f01b08 100644
> >>> --- a/drivers/bluetooth/btrtl.h
> >>> +++ b/drivers/bluetooth/btrtl.h
> >>> @@ -54,6 +54,8 @@ struct btrtl_device_info *btrtl_initialize(struct hci_dev *hdev,
> >>> void btrtl_free(struct btrtl_device_info *btrtl_dev);
> >>> int btrtl_download_firmware(struct hci_dev *hdev,
> >>>                          struct btrtl_device_info *btrtl_dev);
> >>> +void btrtl_set_quirks(struct hci_dev *hdev,
> >>> +                   struct btrtl_device_info *btrtl_dev);
> >>> int btrtl_setup_realtek(struct hci_dev *hdev);
> >>> int btrtl_shutdown_realtek(struct hci_dev *hdev);
> >>> int btrtl_get_uart_settings(struct hci_dev *hdev,
> >>> diff --git a/drivers/bluetooth/hci_h5.c b/drivers/bluetooth/hci_h5.c
> >>> index 27e96681d583..e0520639f4ba 100644
> >>> --- a/drivers/bluetooth/hci_h5.c
> >>> +++ b/drivers/bluetooth/hci_h5.c
> >>> @@ -906,10 +906,7 @@ static int h5_btrtl_setup(struct h5 *h5)
> >>>      /* Give the device some time before the hci-core sends it a reset */
> >>>      usleep_range(10000, 20000);
> >>>
> >>> -     /* Enable controller to do both LE scan and BR/EDR inquiry
> >>> -      * simultaneously.
> >>> -      */
> >>> -     set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &h5->hu->hdev->quirks);
> >>> +     btrtl_set_quirks(h5->hu->hdev, btrtl_dev);
> >>
> >> any reason why not just setting WBS quirk here?
> >
> > Hmm, I think WBS is the feature of the chipset and not the transport.
> > Therefore isn't it better to just have it set in one place?
> > Setting the quirks here means we need to copy paste the settings from btrtl.c.
>
> but since you are already setting HCI_QUIRK_SIMULTANEOUS_DISCOVERY right now, I don’t see the difference.

Sorry, I don't get what you mean.
With this patch I also moved HCI_QUIRK_SIMULTANEOUS_DISCOVERY into
btrtl.c, so it's together with the WBS quirk.

> Can we actually verify that we still need the WBS quirk. I think we fixed the broken errerrnous packet flag handling.

To be honest, I am not aware about the story of the broken erroneous
packet flag.
Last time I checked I still needed the quirk to have RTL8822 on UART
properly run WBS, but that was months ago...
Let me verify whether this quirk is still needed.

Cheers,
Archie