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] [day] [month] [year] [list]
Message-Id: <872ECDCE-D007-47EA-B9EA-F971CA34540D@holtmann.org>
Date:   Thu, 27 May 2021 17:13:27 +0200
From:   Marcel Holtmann <marcel@...tmann.org>
To:     Archie Pusaka <apusaka@...gle.com>
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 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.
>>>>>> 
>>>>>> It looks like we still need the WBS quirk because otherwise the host
>>>>>> wouldn't know whether the controller supports WBS or not. It's used in
>>>>>> get_supported_settings() in mgmt.c.
>>>>> 
>>>>> and why not set it unconditionally for all Realtek chips?
>>>> 
>>>> Not all Realtek chips supports WBS, therefore
>>>> HCI_QUIRK_WIDEBAND_SPEECH_SUPPORTED is only set on some of them.
>>> 
>>> Are there any other concerns you might have?
>> 
>> can we do the quirk setting in btrtl_setup_realtek() instead of creating another exported function.
> 
> It cannot be done easily since the first part of btrtl_setup_realtek()
> is used exclusively for btusb, which is done differently in hci_h5.
> 
> We can have it another way: define btrtl_setup_realtek_h5() to do the
> setup for h5 part in btrtl.c. This would effectively move all of
> h5_btrtl_setup() inside hci_h5.c, most notably the serdev setup. In
> turn, we don't have to expose btrtl_set_quirks(), and we can even hide
> btrtl_initialize(), btrtl_free(), and btrtl_download_firmware() inside
> btrtl.c.
> I'm not sure though why would one want that? We still need to export
> the new btrtl_setup_realtek_h5().

I am a bit disappointed that nobody took up the work on bt3wire.c so that we can have a clean serdev based driver for 3-Wire / H:5 support. That would make supporting USB and UART vendor setups from the same manufacturer a lot easier.

The consistent hci_h5.c hacking is not doing anybody any favor in the long run. It will get more and more complicated especially since the underlying core design is a line discipline. This is a hint with a massively large hammer.

Regards

Marcel