| Message ID | 20250331083523.1132457-1-chharry@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Bluetooth: btusb: Reset altsetting when no SCO connection | expand |
| Context | Check | Description |
|---|---|---|
| tedd_an/pre-ci_am | success | Success |
| tedd_an/SubjectPrefix | success | Gitlint PASS |
| tedd_an/BuildKernel | success | BuildKernel PASS |
| tedd_an/CheckAllWarning | success | CheckAllWarning PASS |
| tedd_an/CheckSparse | success | CheckSparse PASS |
| tedd_an/BuildKernel32 | success | BuildKernel32 PASS |
| tedd_an/TestRunnerSetup | success | TestRunnerSetup PASS |
| tedd_an/TestRunner_l2cap-tester | success | TestRunner PASS |
| tedd_an/TestRunner_iso-tester | success | TestRunner PASS |
| tedd_an/TestRunner_bnep-tester | success | TestRunner PASS |
| tedd_an/TestRunner_mgmt-tester | fail | TestRunner_mgmt-tester: Total: 490, Passed: 484 (98.8%), Failed: 2, Not Run: 4 |
| tedd_an/TestRunner_rfcomm-tester | success | TestRunner PASS |
| tedd_an/TestRunner_sco-tester | success | TestRunner PASS |
| tedd_an/TestRunner_ioctl-tester | success | TestRunner PASS |
| tedd_an/TestRunner_mesh-tester | success | TestRunner PASS |
| tedd_an/TestRunner_smp-tester | success | TestRunner PASS |
| tedd_an/TestRunner_userchan-tester | success | TestRunner PASS |
Hi Luiz, On Mon, Mar 31, 2025 at 4:36 PM Hsin-chen Chuang <chharry@google.com> wrote: > > From: Hsin-chen Chuang <chharry@chromium.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@chromium.org > Fixes: 75ddcd5ad40e ("Bluetooth: btusb: Configure altsetting for HCI_USER_CHANNEL") > Signed-off-by: Hsin-chen Chuang <chharry@chromium.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 > 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]; > + 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 > Please kindly let me know if you would prefer the below approach: - Define a vendor HCI command, which indicates the expected altsetting from the user space program - btusb intercepts the command and adjusts the Isoc endpoint correspondingly Best Regards, Hsin-chen
This is automated email and please do not reply to this email! Dear submitter, Thank you for submitting the patches to the linux bluetooth mailing list. This is a CI test results with your patch series: PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=948479 ---Test result--- Test Summary: CheckPatch PENDING 0.35 seconds GitLint PENDING 0.35 seconds SubjectPrefix PASS 0.07 seconds BuildKernel PASS 24.41 seconds CheckAllWarning PASS 27.01 seconds CheckSparse PASS 30.02 seconds BuildKernel32 PASS 24.00 seconds TestRunnerSetup PASS 430.44 seconds TestRunner_l2cap-tester PASS 21.06 seconds TestRunner_iso-tester PASS 39.22 seconds TestRunner_bnep-tester PASS 4.67 seconds TestRunner_mgmt-tester FAIL 120.34 seconds TestRunner_rfcomm-tester PASS 7.93 seconds TestRunner_sco-tester PASS 12.67 seconds TestRunner_ioctl-tester PASS 8.37 seconds TestRunner_mesh-tester PASS 6.54 seconds TestRunner_smp-tester PASS 7.25 seconds TestRunner_userchan-tester PASS 5.23 seconds IncrementalBuild PENDING 0.77 seconds Details ############################## Test: CheckPatch - PENDING Desc: Run checkpatch.pl script Output: ############################## Test: GitLint - PENDING Desc: Run gitlint Output: ############################## Test: TestRunner_mgmt-tester - FAIL Desc: Run mgmt-tester with test-runner Output: Total: 490, Passed: 484 (98.8%), Failed: 2, Not Run: 4 Failed Test Cases LL Privacy - Set Flags 2 (Enable RL) Failed 0.146 seconds LL Privacy - Set Flags 3 (2 Devices to RL) Failed 0.194 seconds ############################## Test: IncrementalBuild - PENDING Desc: Incremental build with the patches in the series Output: --- Regards, Linux Bluetooth
Hi Hsin-chen, On Mon, Mar 31, 2025 at 4:36 AM Hsin-chen Chuang <chharry@google.com> wrote: > > From: Hsin-chen Chuang <chharry@chromium.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@chromium.org > Fixes: 75ddcd5ad40e ("Bluetooth: btusb: Configure altsetting for HCI_USER_CHANNEL") > Signed-off-by: Hsin-chen Chuang <chharry@chromium.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 >
Hi Hsin-chen, On Mon, Mar 31, 2025 at 4:45 AM Hsin-chen Chuang <chharry@google.com> wrote: > > Hi Luiz, > > On Mon, Mar 31, 2025 at 4:36 PM Hsin-chen Chuang <chharry@google.com> wrote: > > > > From: Hsin-chen Chuang <chharry@chromium.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@chromium.org > > Fixes: 75ddcd5ad40e ("Bluetooth: btusb: Configure altsetting for HCI_USER_CHANNEL") > > Signed-off-by: Hsin-chen Chuang <chharry@chromium.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 > > 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]; > > + 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 > > > > Please kindly let me know if you would prefer the below approach: > - Define a vendor HCI command, which indicates the expected altsetting > from the user space program > - btusb intercepts the command and adjusts the Isoc endpoint correspondingly Yeah, that would probably be a lot simpler in terms of implementation and we can signal errors back to upper layers as well, that said that is the first time we would be doing it so we may need to design it in a way that it is extensible e.g. just reserve a single opcode and then use a header to determine the operation in the driver, well that if we want these operations to be valid for all drivers but perhaps we don't since in this case specifically it is only really valid for btusb. Or perhaps we should do: diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c index 022b86797acd..202e14578a8f 100644 --- a/net/bluetooth/hci_sock.c +++ b/net/bluetooth/hci_sock.c @@ -1852,19 +1852,6 @@ static int hci_sock_sendmsg(struct socket *sock, struct msghdr *msg, skb_pull(skb, 1); if (hci_pi(sk)->channel == HCI_CHANNEL_USER) { - /* No permission check is needed for user channel - * since that gets enforced when binding the socket. - * - * However check that the packet type is valid. - */ - if (hci_skb_pkt_type(skb) != HCI_COMMAND_PKT && - hci_skb_pkt_type(skb) != HCI_ACLDATA_PKT && - hci_skb_pkt_type(skb) != HCI_SCODATA_PKT && - hci_skb_pkt_type(skb) != HCI_ISODATA_PKT) { - err = -EINVAL; - goto drop; - } - skb_queue_tail(&hdev->raw_q, skb); queue_work(hdev->workqueue, &hdev->tx_work); } else if (hci_skb_pkt_type(skb) == HCI_COMMAND_PKT) { Then you can use e.g. HCI_VENDOR_PKT (0xff) which can take any form we like and doesn't use the HCI opcode space, perhaps we can even define a dedicated packet type for driver communication e.g HCI_DRV_PKT.
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 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]; + 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); }