| 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
| ||
|
Date: Wed, 26 May 2021 16:59:28 +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.
Regards
Marcel
Powered by blists - more mailing lists