| 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
| ||
|
Message-ID: <03bbfa8d-6a43-47ff-b075-8b38718f812f@molgen.mpg.de>
Date: Tue, 26 Nov 2024 08:18:03 +0100
From: Paul Menzel <pmenzel@...gen.mpg.de>
To: Hilda Wu <hildawu@...ltek.com>
Cc: marcel@...tmann.org, luiz.dentz@...il.com,
linux-bluetooth@...r.kernel.org, linux-kernel@...r.kernel.org,
max.chou@...ltek.com, alex_lu@...lsil.com.cn, kidman@...ltek.com
Subject: Re: [PATCH v3] Bluetooth: add quirk using packet size 60
Dear Hilda,
Thank you for your patch. Some minor comments.
It’d be great if you addad the chip model to the commit message
summary/title.
Am 26.11.24 um 08:05 schrieb Hilda Wu:
> The RTL8852BE-VT supports USB alternate setting 6.
Please add a blank line between paragraphs.
> However, its descriptor does not report this capability to the host.
Is that a firmware limitation? Is it going to be fixed in the future in
other revisions?
> Therefore, a quirk is needed to bypass the RTL8852BE-VT's descriptor
> and allow it to use USB ALT 6 directly.
Does Linux log the application of this quirk?
> The btmon log below shows the case that WBS with the USB alternate
> setting 6.
>
> < HCI Command: Enhanced.. (0x01|0x003d) plen 59 #2123 [hci0] 82.701813
> Handle: 1 Address: 78:A7:EB:4C:53:4D (1MORE)
> Transmit bandwidth: 8000
> Receive bandwidth: 8000
> Max latency: 13
> Packet type: 0x0380
> 3-EV3 may not be used
> 2-EV5 may not be used
> 3-EV5 may not be used
> Retransmission effort: Optimize for link quality (0x02)
> < ACL Data TX: Handle 1 flags 0x00 dlen 22 #2124 [hci0] 82.701825
> Channel: 65 len 18 [PSM 3 mode Basic (0x00)] {chan 1}
> RFCOMM: Unnumbered Info with Header Check (UIH) (0xef)
> Address: 0x0b cr 1 dlci 0x02
> Control: 0xef poll/final 0
> Length: 14
> FCS: 0x9a
> 0d 0a 2b 43 49 45 56 3a 20 32 2c 31 0d 0a 9a ..+CIEV: 2,1..>
>> HCI Event: Command Status (0x0f) plen 4 #2125 [hci0] 82.703812
> Enhanced Setup Synchronous Connection (0x01|0x003d) ncmd 2
> Status: Success (0x00)
>> HCI Event: Number of Complete.. (0x13) plen 5 #2126 [hci0] 82.710834
> Num handles: 1
> Handle: 1 Address: 78:A7:EB:4C:53:4D (1MORE)
> Count: 1
> #2124: len 22 (19 Kb/s)
> Latency: 9 msec (3-56 msec ~13 msec)
> Channel: 65 [PSM 3 mode Basic (0x00)] {chan 1}
> Channel Latency: 9 msec (4-27 msec ~15 msec)
>> HCI Event: Synchronous Conne.. (0x2c) plen 17 #2127 [hci0] 82.741840
> Status: Success (0x00)
> Handle: 2
> Address: 78:A7:EB:4C:53:4D (1MORE)
> Link type: eSCO (0x02)
> Transmission interval: 0x0c
> Retransmission window: 0x04
> RX packet length: 60
> TX packet length: 60
> Air mode: Transparent (0x03)
> @ RAW Open: btmon (privileged) version 2.22 {0x0002} 82.742580
> @ RAW Close: btmon {0x0002} 82.742594
>> SCO Data RX: Handle 2 flags 0x00 dlen 60 #2128 [hci0] 82.764812
> < ACL Data TX: Handle 1 flags 0x00 dlen 19 #2129 [hci0] 82.764994
> Channel: 65 len 15 [PSM 3 mode Basic (0x00)] {chan 1}
> RFCOMM: Unnumbered Info with Header Check (UIH) (0xef)
> Address: 0x0b cr 1 dlci 0x02
> Control: 0xef poll/final 0
> Length: 11
> FCS: 0x9a
> 0d 0a 2b 56 47 53 3a 20 36 0d 0a 9a ..+VGS: 6... >
>> HCI Event: Max Slots Change (0x1b) plen 3 #2130 [hci0] 82.765814
> Handle: 1 Address: 78:A7:EB:4C:53:4D (1MORE)
> Max slots: 1
> < SCO Data TX: Handle 2 flags 0x00 dlen 60 #2131 [hci0] 82.765897
>> HCI Event: Number of Complete.. (0x13) plen 5 #2132 [hci0] 82.771855
> Num handles: 1
> Handle: 1 Address: 78:A7:EB:4C:53:4D (1MORE)
> Count: 1
> #2129: len 19 (25 Kb/s)
> Latency: 6 msec (3-56 msec ~10 msec)
> Channel: 65 [PSM 3 mode Basic (0x00)] {chan 1}
> Channel Latency: 6 msec (4-27 msec ~11 msec)
> < SCO Data TX: Handle 2 flags 0x00 dlen 60 #2133 [hci0] 82.773344
>> SCO Data RX: Handle 2 flags 0x00 dlen 60 #2134 [hci0] 82.774836
>> SCO Data RX: Handle 2 flags 0x00 dlen 60 #2135 [hci0] 82.774839
>> SCO Data RX: Handle 2 flags 0x00 dlen 60 #2136 [hci0] 82.784840
> < SCO Data TX: Handle 2 flags 0x00 dlen 60 #2137 [hci0] 82.787175
> < SCO Data TX: Handle 2 flags 0x00 dlen 60 #2138 [hci0] 82.788282
>> SCO Data RX: Handle 2 flags 0x00 dlen 60 #2139 [hci0] 82.794812
> < SCO Data TX: Handle 2 flags 0x00 dlen 60 #2140 [hci0] 82.795797
>> SCO Data RX: Handle 2 flags 0x00 dlen 60 #2141 [hci0] 82.804838
>> SCO Data RX: Handle 2 flags 0x00 dlen 60 #2142 [hci0] 82.804840
> < SCO Data TX: Handle 2 flags 0x00 dlen 60 #2143 [hci0] 82.808554
>
> Signed-off-by: Alex Lu <alex_lu@...lsil.com.cn>
> Signed-off-by: Hilda Wu <hildawu@...ltek.com>
>
> ---
> Change:
> V3: Fixed SubjectPrefix, use quirk instead of btrealtek_*_flag()
> v2: Use btusb_find_altsetting replace duplicating logic, add tested log.
> ---
> ---
> drivers/bluetooth/btrtl.c | 3 ++
> drivers/bluetooth/btrtl.h | 1 +
> drivers/bluetooth/btusb.c | 86 ++++++++++++++++++++++++++++++---------
> 3 files changed, 70 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/bluetooth/btrtl.c b/drivers/bluetooth/btrtl.c
> index 83025f457ca0..7efd733f9e84 100644
> --- a/drivers/bluetooth/btrtl.c
> +++ b/drivers/bluetooth/btrtl.c
> @@ -1312,6 +1312,9 @@ void btrtl_set_quirks(struct hci_dev *hdev, struct btrtl_device_info *btrtl_dev)
> btrtl_dev->project_id == CHIP_ID_8852C)
> set_bit(HCI_QUIRK_USE_MSFT_EXT_ADDRESS_FILTER, &hdev->quirks);
>
> + if (btrtl_dev->project_id == CHIP_ID_8852BT)
> + btrealtek_set_flag(hdev, REALTEK_ALT6_FORCE);
> +
> hci_set_aosp_capable(hdev);
> break;
> default:
> diff --git a/drivers/bluetooth/btrtl.h b/drivers/bluetooth/btrtl.h
> index a2d9d34f9fb0..ffec2fca88ec 100644
> --- a/drivers/bluetooth/btrtl.h
> +++ b/drivers/bluetooth/btrtl.h
> @@ -105,6 +105,7 @@ struct rtl_vendor_cmd {
>
> enum {
> REALTEK_ALT6_CONTINUOUS_TX_CHIP,
> + REALTEK_ALT6_FORCE,
>
> __REALTEK_NUM_FLAGS,
> };
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index 279fe6c115fa..6acdff772ca5 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -814,6 +814,8 @@ struct qca_dump_info {
> #define BTUSB_USE_ALT3_FOR_WBS 15
> #define BTUSB_ALT6_CONTINUOUS_TX 16
> #define BTUSB_HW_SSR_ACTIVE 17
> +#define BTUSB_ALT6_QUIRK 18
> +#define BTUSB_ISOC_ALT_CHANGED 19
What is BTUSB_ISOC_ALT_CHANGED needed for? For me it’d be great, if you
mentioned it in the commit message.
> struct btusb_data {
> struct hci_dev *hdev;
> @@ -866,6 +868,7 @@ struct btusb_data {
> unsigned int air_mode;
> bool usb_alt6_packet_flow;
> int isoc_altsetting;
> + u16 isoc_mps;
What is mps? Add a comment? Why not unsigned int?
> int suspend_count;
>
> int (*recv_event)(struct hci_dev *hdev, struct sk_buff *skb);
> @@ -2140,16 +2143,58 @@ static void btusb_notify(struct hci_dev *hdev, unsigned int evt)
> }
> }
>
> +static struct usb_host_interface *btusb_find_altsetting(struct btusb_data *data,
> + int alt)
> +{
> + struct usb_interface *intf = data->isoc;
> + int i;
> +
> + BT_DBG("Looking for Alt no :%d", alt);
> +
> + if (!intf)
> + return NULL;
> +
> + for (i = 0; i < intf->num_altsetting; i++) {
> + if (intf->altsetting[i].desc.bAlternateSetting == alt)
> + return &intf->altsetting[i];
> + }
> +
> + return NULL;
> +}
> +
> static inline int __set_isoc_interface(struct hci_dev *hdev, int altsetting)
> {
> struct btusb_data *data = hci_get_drvdata(hdev);
> struct usb_interface *intf = data->isoc;
> struct usb_endpoint_descriptor *ep_desc;
> + struct usb_host_interface *alt;
> int i, err;
>
> if (!data->isoc)
> return -ENODEV;
>
> + /* For some Realtek chips, they actually have the altsetting 6, but its
> + * altsetting descriptor is not exposed. We can activate altsetting 6 by
> + * replacing the altsetting 5.
> + */
> + if (altsetting == 6 && !btusb_find_altsetting(data, 6) &&
> + test_bit(BTUSB_ALT6_QUIRK, &data->flags)) {
> + alt = btusb_find_altsetting(data, 5);
> + if (alt) {
> + data->isoc_mps = 49;
> + for (i = 0; i < alt->desc.bNumEndpoints; i++) {
> + ep_desc = &alt->endpoint[i].desc;
> + if (!usb_endpoint_xfer_isoc(ep_desc))
> + continue;
> + data->isoc_mps =
> + le16_to_cpu(ep_desc->wMaxPacketSize);
> + ep_desc->wMaxPacketSize = cpu_to_le16(63);
> + }
> + alt->desc.bAlternateSetting = 6;
> + set_bit(BTUSB_ISOC_ALT_CHANGED, &data->flags);
> + }
> + }
> +
> err = usb_set_interface(data->udev, data->isoc_ifnum, altsetting);
> if (err < 0) {
> bt_dev_err(hdev, "setting interface failed (%d)", -err);
> @@ -2161,6 +2206,22 @@ static inline int __set_isoc_interface(struct hci_dev *hdev, int altsetting)
> data->isoc_tx_ep = NULL;
> data->isoc_rx_ep = NULL;
>
> + /* Recover alt 5 desc if alt 0 is set. */
Why?
> + if (!altsetting && test_bit(BTUSB_ISOC_ALT_CHANGED, &data->flags)) {
> + alt = btusb_find_altsetting(data, 6);
> + if (alt) {
> + for (i = 0; i < alt->desc.bNumEndpoints; i++) {
> + ep_desc = &alt->endpoint[i].desc;
> + if (!usb_endpoint_xfer_isoc(ep_desc))
> + continue;
> + ep_desc->wMaxPacketSize =
> + cpu_to_le16(data->isoc_mps);
> + }
> + alt->desc.bAlternateSetting = 5;
> + clear_bit(BTUSB_ISOC_ALT_CHANGED, &data->flags);
> + }
> + }
> +
> for (i = 0; i < intf->cur_altsetting->desc.bNumEndpoints; i++) {
> ep_desc = &intf->cur_altsetting->endpoint[i].desc;
>
> @@ -2223,25 +2284,6 @@ static int btusb_switch_alt_setting(struct hci_dev *hdev, int new_alts)
> return 0;
> }
>
> -static struct usb_host_interface *btusb_find_altsetting(struct btusb_data *data,
> - int alt)
> -{
> - struct usb_interface *intf = data->isoc;
> - int i;
> -
> - BT_DBG("Looking for Alt no :%d", alt);
> -
> - if (!intf)
> - return NULL;
> -
> - for (i = 0; i < intf->num_altsetting; i++) {
> - if (intf->altsetting[i].desc.bAlternateSetting == alt)
> - return &intf->altsetting[i];
> - }
> -
> - return NULL;
> -}
> -
> static void btusb_work(struct work_struct *work)
> {
> struct btusb_data *data = container_of(work, struct btusb_data, work);
> @@ -2279,7 +2321,8 @@ static void btusb_work(struct work_struct *work)
> * MTU >= 3 (packets) * 25 (size) - 3 (headers) = 72
> * see also Core spec 5, vol 4, B 2.1.1 & Table 2.1.
> */
> - if (btusb_find_altsetting(data, 6))
> + if (btusb_find_altsetting(data, 6) ||
> + test_bit(BTUSB_ALT6_QUIRK, &data->flags))
> new_alts = 6;
> else if (btusb_find_altsetting(data, 3) &&
> hdev->sco_mtu >= 72 &&
> @@ -2611,6 +2654,9 @@ static int btusb_setup_realtek(struct hci_dev *hdev)
> if (btrealtek_test_flag(data->hdev, REALTEK_ALT6_CONTINUOUS_TX_CHIP))
> set_bit(BTUSB_ALT6_CONTINUOUS_TX, &data->flags);
>
> + if (btrealtek_test_flag(data->hdev, REALTEK_ALT6_FORCE))
> + set_bit(BTUSB_ALT6_QUIRK, &data->flags);
> +
> return ret;
> }
Kind regards,
Paul
Powered by blists - more mailing lists