| 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: <CABBYNZKeCY6AU7MBSu-kcv6B=nuS4-yLTfX68uvQW25e1SSeqw@mail.gmail.com>
Date: Mon, 31 Mar 2025 09:46:15 -0400
From: Luiz Augusto von Dentz <luiz.dentz@...il.com>
To: Hsin-chen Chuang <chharry@...gle.com>
Cc: Hsin-chen Chuang <chharry@...omium.org>, chromeos-bluetooth-upstreaming@...omium.org,
Marcel Holtmann <marcel@...tmann.org>, linux-bluetooth@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Bluetooth: btusb: Reset altsetting when no SCO connection
Hi Hsin-chen,
On Mon, Mar 31, 2025 at 4:36 AM Hsin-chen Chuang <chharry@...gle.com> wrote:
>
> From: Hsin-chen Chuang <chharry@...omium.org>
>
> Although commit 75ddcd5ad40e ("Bluetooth: btusb: Configure altsetting
> for HCI_USER_CHANNEL") has enabled the HCI_USER_CHANNEL user to send out
> SCO data through USB Bluetooth chips, it's observed that with the patch
> HFP is flaky on most of the existing USB Bluetooth controllers: Intel
> chips sometimes send out no packet for Transparent codec; MTK chips may
> generate SCO data with a wrong handle for CVSD codec; RTK could split
> the data with a wrong packet size for Transparent codec.
>
> To address the issue above btusb needs to reset the altsetting back to
> zero when there is no active SCO connection, which is the same as the
> BlueZ behavior, and another benefit is the bus doesn't need to reserve
> bandwidth when no SCO connection.
>
> This patch adds a fixed-size array in btusb_data which is used for
> tracing the active SCO handles. When the controller is reset or any
> tracing handle has disconnected, btusb adjusts the USB alternate setting
> correspondingly for the Isoc endpoint.
>
> The array size is configured by BT_HCIBTUSB_AUTO_ISOC_ALT_MAX_HANDLES.
> If the size is set to zero, the auto set altsetting feature is disabled.
>
> This patch is tested on ChromeOS devices. The USB Bluetooth models
> (CVSD, TRANS alt3 and alt6) could pass the stress HFP test narrow band
> speech and wide band speech.
>
> Cc: chromeos-bluetooth-upstreaming@...omium.org
> Fixes: 75ddcd5ad40e ("Bluetooth: btusb: Configure altsetting for HCI_USER_CHANNEL")
> Signed-off-by: Hsin-chen Chuang <chharry@...omium.org>
> ---
>
> drivers/bluetooth/Kconfig | 18 ++++--
> drivers/bluetooth/btusb.c | 116 ++++++++++++++++++++++++++++++--------
> 2 files changed, 105 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
> index 7771edf54fb3..5c811af8d15f 100644
> --- a/drivers/bluetooth/Kconfig
> +++ b/drivers/bluetooth/Kconfig
> @@ -56,17 +56,23 @@ config BT_HCIBTUSB_POLL_SYNC
> Say Y here to enable USB poll_sync for Bluetooth USB devices by
> default.
>
> -config BT_HCIBTUSB_AUTO_ISOC_ALT
> - bool "Automatically adjust alternate setting for Isoc endpoints"
> +config BT_HCIBTUSB_AUTO_ISOC_ALT_MAX_HANDLES
> + int "Automatically adjust alternate setting for Isoc endpoints"
> depends on BT_HCIBTUSB
> - default y if CHROME_PLATFORMS
> + default 2 if CHROME_PLATFORMS
> + default 0
We may need to limit to a maximum of 3 given that is the maximum
defined per spec.
> help
> - Say Y here to automatically adjusting the alternate setting for
> - HCI_USER_CHANNEL whenever a SCO link is established.
> + Say positive number here to automatically adjusting the alternate
> + setting for HCI_USER_CHANNEL whenever a SCO link is established.
>
> - When enabled, btusb intercepts the HCI_EV_SYNC_CONN_COMPLETE packets
> + When positive, btusb intercepts the HCI_EV_SYNC_CONN_COMPLETE packets
> and configures isoc endpoint alternate setting automatically when
> HCI_USER_CHANNEL is in use.
> + btusb traces at most the given number of SCO handles and intercepts
> + the HCI_EV_DISCONN_COMPLETE and the HCI_EV_CMD_COMPLETE of
> + HCI_OP_RESET packets. When the controller is reset, or all tracing
> + handles are disconnected, btusb reset the isoc endpoint alternate
> + setting to zero.
>
> config BT_HCIBTUSB_BCM
> bool "Broadcom protocol support"
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index 5012b5ff92c8..31439d66cf0e 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -34,7 +34,7 @@ static bool force_scofix;
> static bool enable_autosuspend = IS_ENABLED(CONFIG_BT_HCIBTUSB_AUTOSUSPEND);
> static bool enable_poll_sync = IS_ENABLED(CONFIG_BT_HCIBTUSB_POLL_SYNC);
> static bool reset = true;
> -static bool auto_isoc_alt = IS_ENABLED(CONFIG_BT_HCIBTUSB_AUTO_ISOC_ALT);
> +static bool auto_isoc_alt = CONFIG_BT_HCIBTUSB_AUTO_ISOC_ALT_MAX_HANDLES > 0;
>
> static struct usb_driver btusb_driver;
>
> @@ -907,6 +907,8 @@ struct btusb_data {
> __u8 cmdreq;
>
> unsigned int sco_num;
> + u16 sco_handles[CONFIG_BT_HCIBTUSB_AUTO_ISOC_ALT_MAX_HANDLES];
> +
> unsigned int air_mode;
> bool usb_alt6_packet_flow;
> int isoc_altsetting;
> @@ -1118,40 +1120,108 @@ static inline void btusb_free_frags(struct btusb_data *data)
> spin_unlock_irqrestore(&data->rxlock, flags);
> }
>
> -static void btusb_sco_connected(struct btusb_data *data, struct sk_buff *skb)
> +static void btusb_sco_changed(struct btusb_data *data, struct sk_buff *skb)
> {
> struct hci_event_hdr *hdr = (void *) skb->data;
> - struct hci_ev_sync_conn_complete *ev =
> - (void *) skb->data + sizeof(*hdr);
> struct hci_dev *hdev = data->hdev;
> - unsigned int notify_air_mode;
>
> - if (hci_skb_pkt_type(skb) != HCI_EVENT_PKT)
> - return;
> + if (data->sco_num > CONFIG_BT_HCIBTUSB_AUTO_ISOC_ALT_MAX_HANDLES) {
> + bt_dev_warn(hdev, "Invalid sco_num to HCI_USER_CHANNEL");
> + data->sco_num = 0;
> + }
>
> - if (skb->len < sizeof(*hdr) || hdr->evt != HCI_EV_SYNC_CONN_COMPLETE)
> + if (hci_skb_pkt_type(skb) != HCI_EVENT_PKT || skb->len < sizeof(*hdr))
> return;
>
> - if (skb->len != sizeof(*hdr) + sizeof(*ev) || ev->status)
> - return;
> + switch (hdr->evt) {
> + case HCI_EV_CMD_COMPLETE: {
> + struct hci_ev_cmd_complete *ev =
> + (void *) skb->data + sizeof(*hdr);
> + struct hci_ev_status *rp =
> + (void *) skb->data + sizeof(*hdr) + sizeof(*ev);
> + u16 opcode;
> +
> + if (skb->len != sizeof(*hdr) + sizeof(*ev) + sizeof(*rp))
> + return;
> +
> + opcode = __le16_to_cpu(ev->opcode);
> +
> + if (opcode != HCI_OP_RESET || rp->status)
> + return;
> +
> + bt_dev_info(hdev, "Resetting SCO");
> + data->sco_num = 0;
> + data->air_mode = HCI_NOTIFY_DISABLE_SCO;
> + schedule_work(&data->work);
>
> - switch (ev->air_mode) {
> - case BT_CODEC_CVSD:
> - notify_air_mode = HCI_NOTIFY_ENABLE_SCO_CVSD;
> break;
> + }
> + case HCI_EV_DISCONN_COMPLETE: {
> + struct hci_ev_disconn_complete *ev =
> + (void *) skb->data + sizeof(*hdr);
> + u16 handle;
> + int i;
> +
> + if (skb->len != sizeof(*hdr) + sizeof(*ev) || ev->status)
> + return;
> +
> + handle = __le16_to_cpu(ev->handle);
> + for (i = 0; i < data->sco_num; i++) {
> + if (data->sco_handles[i] == handle)
> + break;
> + }
> +
> + if (i == data->sco_num)
> + return;
> +
> + bt_dev_info(hdev, "Disabling SCO");
> + data->sco_handles[i] = data->sco_handles[data->sco_num - 1];
Not really sure what is the intent of the assignment above, shouldn't
it just be resetting it back to invalid handle?
> + data->sco_num--;
> + data->air_mode = HCI_NOTIFY_DISABLE_SCO;
> + schedule_work(&data->work);
>
> - case BT_CODEC_TRANSPARENT:
> - notify_air_mode = HCI_NOTIFY_ENABLE_SCO_TRANSP;
> break;
> + }
> + case HCI_EV_SYNC_CONN_COMPLETE: {
> + struct hci_ev_sync_conn_complete *ev =
> + (void *) skb->data + sizeof(*hdr);
> + unsigned int notify_air_mode;
> + u16 handle;
>
> + if (skb->len != sizeof(*hdr) + sizeof(*ev) || ev->status)
> + return;
> +
> + switch (ev->air_mode) {
> + case BT_CODEC_CVSD:
> + notify_air_mode = HCI_NOTIFY_ENABLE_SCO_CVSD;
> + break;
> +
> + case BT_CODEC_TRANSPARENT:
> + notify_air_mode = HCI_NOTIFY_ENABLE_SCO_TRANSP;
> + break;
> +
> + default:
> + return;
> + }
> +
> + handle = __le16_to_cpu(ev->handle);
> + if (data->sco_num
> + == CONFIG_BT_HCIBTUSB_AUTO_ISOC_ALT_MAX_HANDLES) {
> + bt_dev_err(hdev, "Failed to add the new SCO handle");
> + return;
> + }
> +
> + bt_dev_info(hdev, "Enabling SCO with air mode %u",
> + ev->air_mode);
> + data->sco_handles[data->sco_num++] = handle;
> + data->air_mode = notify_air_mode;
> + schedule_work(&data->work);
> +
> + break;
> + }
> default:
> - return;
> + break;
> }
> -
> - bt_dev_info(hdev, "enabling SCO with air mode %u", ev->air_mode);
> - data->sco_num = 1;
> - data->air_mode = notify_air_mode;
> - schedule_work(&data->work);
> }
>
> static int btusb_recv_event(struct btusb_data *data, struct sk_buff *skb)
> @@ -1161,9 +1231,9 @@ static int btusb_recv_event(struct btusb_data *data, struct sk_buff *skb)
> schedule_delayed_work(&data->rx_work, 0);
> }
>
> - /* Configure altsetting for HCI_USER_CHANNEL on SCO connected */
> + /* Configure altsetting for HCI_USER_CHANNEL on SCO changed */
> if (auto_isoc_alt && hci_dev_test_flag(data->hdev, HCI_USER_CHANNEL))
> - btusb_sco_connected(data, skb);
> + btusb_sco_changed(data, skb);
>
> return data->recv_event(data->hdev, skb);
> }
> --
> 2.49.0.472.ge94155a9ec-goog
>
--
Luiz Augusto von Dentz
Powered by blists - more mailing lists