| Message ID | 20250224022447.1141413-1-chharry@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Bluetooth: btusb: Configure altsetting for USER_CHANNEL | 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: 485 (99.0%), Failed: 1, 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 Hsin-chen, On Sun, Feb 23, 2025 at 9:26 PM Hsin-chen Chuang <chharry@google.com> wrote: > > From: Hsin-chen Chuang <chharry@chromium.org> > > Automatically configure the altsetting for USER_CHANNEL when a SCO is > connected or disconnected. This adds support for the USER_CHANNEL to > transfer SCO data over USB transport. I guess the tracking of handles is about handling disconnections, right? I wonder if we can get away without doing that, I didn't intend to add a whole bunch of changes in order to switch to the right mode, I get that you may want to disable the isochronous endpoint in case there is no connection, but I guess that only matters if we are talking about power but the impact is probably small so I don't think it is worth it. There is an alternative to match the SCO/eSCO handle via mask, like we do with ISO handles, that is probably a lot cheaper than attempting to add a whole list for tracking handles, but it has the downside that it is vendor/model specific. > Fixes: b16b327edb4d ("Bluetooth: btusb: add sysfs attribute to control USB alt setting") > Signed-off-by: Hsin-chen Chuang <chharry@chromium.org> > --- > > drivers/bluetooth/btusb.c | 224 +++++++++++++++++++++++++++++++------- > 1 file changed, 186 insertions(+), 38 deletions(-) > > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c > index de3fa725d210..dfb0918dfe98 100644 > --- a/drivers/bluetooth/btusb.c > +++ b/drivers/bluetooth/btusb.c > @@ -854,6 +854,11 @@ struct qca_dump_info { > #define BTUSB_ALT6_CONTINUOUS_TX 16 > #define BTUSB_HW_SSR_ACTIVE 17 > > +struct sco_handle_list { > + struct list_head list; > + u16 handle; > +}; > + > struct btusb_data { > struct hci_dev *hdev; > struct usb_device *udev; > @@ -920,6 +925,9 @@ struct btusb_data { > int oob_wake_irq; /* irq for out-of-band wake-on-bt */ > > struct qca_dump_info qca_dump; > + > + /* Records the exsiting SCO handles for HCI_USER_CHANNEL */ > + struct list_head sco_handle_list; > }; > > static void btusb_reset(struct hci_dev *hdev) > @@ -1113,6 +1121,131 @@ static inline void btusb_free_frags(struct btusb_data *data) > spin_unlock_irqrestore(&data->rxlock, flags); > } > > +static void btusb_sco_handle_list_clear(struct list_head *list) > +{ > + struct sco_handle_list *entry, *n; > + > + list_for_each_entry_safe(entry, n, list, list) { > + list_del(&entry->list); > + kfree(entry); > + } > +} > + > +static struct sco_handle_list *btusb_sco_handle_list_find( > + struct list_head *list, > + u16 handle) > +{ > + struct sco_handle_list *entry; > + > + list_for_each_entry(entry, list, list) > + if (entry->handle == handle) > + return entry; > + > + return NULL; > +} > + > +static int btusb_sco_handle_list_add(struct list_head *list, u16 handle) > +{ > + struct sco_handle_list *entry; > + > + if (btusb_sco_handle_list_find(list, handle)) > + return -EEXIST; > + > + entry = kzalloc(sizeof(*entry), GFP_KERNEL); > + if (!entry) > + return -ENOMEM; > + > + entry->handle = handle; > + list_add(&entry->list, list); > + > + return 0; > +} > + > +static int btusb_sco_handle_list_del(struct list_head *list, u16 handle) > +{ > + struct sco_handle_list *entry; > + > + entry = btusb_sco_handle_list_find(list, handle); > + if (!entry) > + return -ENOENT; > + > + list_del(&entry->list); > + kfree(entry); > + > + return 0; > +} > + > +static void btusb_sco_conn_change(struct btusb_data *data, struct sk_buff *skb) > +{ > + struct hci_event_hdr *hdr = (void *) skb->data; > + struct hci_dev *hdev = data->hdev; > + > + if (hci_skb_pkt_type(skb) != HCI_EVENT_PKT || skb->len < sizeof(*hdr)) > + return; > + > + switch (hdr->evt) { > + case HCI_EV_DISCONN_COMPLETE: { > + struct hci_ev_disconn_complete *ev = > + (void *) skb->data + sizeof(*hdr); > + u16 handle; > + > + if (skb->len != sizeof(*hdr) + sizeof(*ev) || ev->status) > + return; > + > + handle = __le16_to_cpu(ev->handle); > + if (btusb_sco_handle_list_del(&data->sco_handle_list, > + handle) < 0) > + return; > + > + bt_dev_info(hdev, "disabling SCO"); > + data->sco_num--; > + data->air_mode = HCI_NOTIFY_DISABLE_SCO; > + schedule_work(&data->work); > + > + 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 (btusb_sco_handle_list_add(&data->sco_handle_list, > + handle) < 0) { > + 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_num++; > + data->air_mode = notify_air_mode; > + schedule_work(&data->work); > + > + break; > + } > + default: > + break; > + } > +} > + > static int btusb_recv_event(struct btusb_data *data, struct sk_buff *skb) > { > if (data->intr_interval) { > @@ -1120,6 +1253,10 @@ 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 dis/connected */ > + if (hci_dev_test_flag(data->hdev, HCI_USER_CHANNEL)) > + btusb_sco_conn_change(data, skb); I'd recommend adding a check for Kconfig/module parameter in the if statement so btusb_sco_conn_change only runs on distros with it enabled so we don't risk something not working as expected just because someone decided to open the user channel. > return data->recv_event(data->hdev, skb); > } > > @@ -1919,44 +2056,6 @@ static void btusb_stop_traffic(struct btusb_data *data) > usb_kill_anchored_urbs(&data->ctrl_anchor); > } > > -static int btusb_close(struct hci_dev *hdev) > -{ > - struct btusb_data *data = hci_get_drvdata(hdev); > - int err; > - > - BT_DBG("%s", hdev->name); > - > - cancel_delayed_work(&data->rx_work); > - cancel_work_sync(&data->work); > - cancel_work_sync(&data->waker); > - > - skb_queue_purge(&data->acl_q); > - > - clear_bit(BTUSB_ISOC_RUNNING, &data->flags); > - clear_bit(BTUSB_BULK_RUNNING, &data->flags); > - clear_bit(BTUSB_INTR_RUNNING, &data->flags); > - clear_bit(BTUSB_DIAG_RUNNING, &data->flags); > - > - btusb_stop_traffic(data); > - btusb_free_frags(data); > - > - err = usb_autopm_get_interface(data->intf); > - if (err < 0) > - goto failed; > - > - data->intf->needs_remote_wakeup = 0; > - > - /* Enable remote wake up for auto-suspend */ > - if (test_bit(BTUSB_WAKEUP_AUTOSUSPEND, &data->flags)) > - data->intf->needs_remote_wakeup = 1; > - > - usb_autopm_put_interface(data->intf); > - > -failed: > - usb_scuttle_anchored_urbs(&data->deferred); > - return 0; > -} > - > static int btusb_flush(struct hci_dev *hdev) > { > struct btusb_data *data = hci_get_drvdata(hdev); > @@ -2327,6 +2426,51 @@ static void btusb_work(struct work_struct *work) > } > } > > +static int btusb_close(struct hci_dev *hdev) > +{ > + struct btusb_data *data = hci_get_drvdata(hdev); > + int err; > + > + BT_DBG("%s", hdev->name); > + > + if (hci_dev_test_flag(hdev, HCI_USER_CHANNEL)) { > + btusb_sco_handle_list_clear(&data->sco_handle_list); > + data->sco_num = 0; > + if (data->isoc_altsetting != 0) > + btusb_switch_alt_setting(hdev, 0); > + } > + > + cancel_delayed_work(&data->rx_work); > + cancel_work_sync(&data->work); > + cancel_work_sync(&data->waker); > + > + skb_queue_purge(&data->acl_q); > + > + clear_bit(BTUSB_ISOC_RUNNING, &data->flags); > + clear_bit(BTUSB_BULK_RUNNING, &data->flags); > + clear_bit(BTUSB_INTR_RUNNING, &data->flags); > + clear_bit(BTUSB_DIAG_RUNNING, &data->flags); > + > + btusb_stop_traffic(data); > + btusb_free_frags(data); > + > + err = usb_autopm_get_interface(data->intf); > + if (err < 0) > + goto failed; > + > + data->intf->needs_remote_wakeup = 0; > + > + /* Enable remote wake up for auto-suspend */ > + if (test_bit(BTUSB_WAKEUP_AUTOSUSPEND, &data->flags)) > + data->intf->needs_remote_wakeup = 1; > + > + usb_autopm_put_interface(data->intf); > + > +failed: > + usb_scuttle_anchored_urbs(&data->deferred); > + return 0; > +} > + > static void btusb_waker(struct work_struct *work) > { > struct btusb_data *data = container_of(work, struct btusb_data, waker); > @@ -3782,6 +3926,8 @@ static int btusb_probe(struct usb_interface *intf, > data->udev = interface_to_usbdev(intf); > data->intf = intf; > > + INIT_LIST_HEAD(&data->sco_handle_list); > + > INIT_WORK(&data->work, btusb_work); > INIT_WORK(&data->waker, btusb_waker); > INIT_DELAYED_WORK(&data->rx_work, btusb_rx_work); > @@ -4117,6 +4263,8 @@ static void btusb_disconnect(struct usb_interface *intf) > hdev = data->hdev; > usb_set_intfdata(data->intf, NULL); > > + btusb_sco_handle_list_clear(&data->sco_handle_list); > + > if (data->isoc) { > device_remove_file(&intf->dev, &dev_attr_isoc_alt); > usb_set_intfdata(data->isoc, NULL); > -- > 2.48.1.601.g30ceb7b040-goog >
Hi Luiz, On Mon, Feb 24, 2025 at 10:53 AM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > > Hi Hsin-chen, > > On Sun, Feb 23, 2025 at 9:26 PM Hsin-chen Chuang <chharry@google.com> wrote: > > > > From: Hsin-chen Chuang <chharry@chromium.org> > > > > Automatically configure the altsetting for USER_CHANNEL when a SCO is > > connected or disconnected. This adds support for the USER_CHANNEL to > > transfer SCO data over USB transport. > > I guess the tracking of handles is about handling disconnections, > right? I wonder if we can get away without doing that, I didn't intend > to add a whole bunch of changes in order to switch to the right mode, > I get that you may want to disable the isochronous endpoint in case > there is no connection, but I guess that only matters if we are > talking about power but the impact is probably small so I don't think > it is worth it. There is an alternative to match the SCO/eSCO handle > via mask, like we do with ISO handles, that is probably a lot cheaper > than attempting to add a whole list for tracking handles, but it has > the downside that it is vendor/model specific. Yes, that's for handling disconnection. What do you think if we keep only one handle in the driver data? That is, assume there's at most one SCO connection. Then we don't need a list but just a u16. > > > Fixes: b16b327edb4d ("Bluetooth: btusb: add sysfs attribute to control USB alt setting") > > Signed-off-by: Hsin-chen Chuang <chharry@chromium.org> > > --- > > > > drivers/bluetooth/btusb.c | 224 +++++++++++++++++++++++++++++++------- > > 1 file changed, 186 insertions(+), 38 deletions(-) > > > > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c > > index de3fa725d210..dfb0918dfe98 100644 > > --- a/drivers/bluetooth/btusb.c > > +++ b/drivers/bluetooth/btusb.c > > @@ -854,6 +854,11 @@ struct qca_dump_info { > > #define BTUSB_ALT6_CONTINUOUS_TX 16 > > #define BTUSB_HW_SSR_ACTIVE 17 > > > > +struct sco_handle_list { > > + struct list_head list; > > + u16 handle; > > +}; > > + > > struct btusb_data { > > struct hci_dev *hdev; > > struct usb_device *udev; > > @@ -920,6 +925,9 @@ struct btusb_data { > > int oob_wake_irq; /* irq for out-of-band wake-on-bt */ > > > > struct qca_dump_info qca_dump; > > + > > + /* Records the exsiting SCO handles for HCI_USER_CHANNEL */ > > + struct list_head sco_handle_list; > > }; > > > > static void btusb_reset(struct hci_dev *hdev) > > @@ -1113,6 +1121,131 @@ static inline void btusb_free_frags(struct btusb_data *data) > > spin_unlock_irqrestore(&data->rxlock, flags); > > } > > > > +static void btusb_sco_handle_list_clear(struct list_head *list) > > +{ > > + struct sco_handle_list *entry, *n; > > + > > + list_for_each_entry_safe(entry, n, list, list) { > > + list_del(&entry->list); > > + kfree(entry); > > + } > > +} > > + > > +static struct sco_handle_list *btusb_sco_handle_list_find( > > + struct list_head *list, > > + u16 handle) > > +{ > > + struct sco_handle_list *entry; > > + > > + list_for_each_entry(entry, list, list) > > + if (entry->handle == handle) > > + return entry; > > + > > + return NULL; > > +} > > + > > +static int btusb_sco_handle_list_add(struct list_head *list, u16 handle) > > +{ > > + struct sco_handle_list *entry; > > + > > + if (btusb_sco_handle_list_find(list, handle)) > > + return -EEXIST; > > + > > + entry = kzalloc(sizeof(*entry), GFP_KERNEL); > > + if (!entry) > > + return -ENOMEM; > > + > > + entry->handle = handle; > > + list_add(&entry->list, list); > > + > > + return 0; > > +} > > + > > +static int btusb_sco_handle_list_del(struct list_head *list, u16 handle) > > +{ > > + struct sco_handle_list *entry; > > + > > + entry = btusb_sco_handle_list_find(list, handle); > > + if (!entry) > > + return -ENOENT; > > + > > + list_del(&entry->list); > > + kfree(entry); > > + > > + return 0; > > +} > > + > > +static void btusb_sco_conn_change(struct btusb_data *data, struct sk_buff *skb) > > +{ > > + struct hci_event_hdr *hdr = (void *) skb->data; > > + struct hci_dev *hdev = data->hdev; > > + > > + if (hci_skb_pkt_type(skb) != HCI_EVENT_PKT || skb->len < sizeof(*hdr)) > > + return; > > + > > + switch (hdr->evt) { > > + case HCI_EV_DISCONN_COMPLETE: { > > + struct hci_ev_disconn_complete *ev = > > + (void *) skb->data + sizeof(*hdr); > > + u16 handle; > > + > > + if (skb->len != sizeof(*hdr) + sizeof(*ev) || ev->status) > > + return; > > + > > + handle = __le16_to_cpu(ev->handle); > > + if (btusb_sco_handle_list_del(&data->sco_handle_list, > > + handle) < 0) > > + return; > > + > > + bt_dev_info(hdev, "disabling SCO"); > > + data->sco_num--; > > + data->air_mode = HCI_NOTIFY_DISABLE_SCO; > > + schedule_work(&data->work); > > + > > + 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 (btusb_sco_handle_list_add(&data->sco_handle_list, > > + handle) < 0) { > > + 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_num++; > > + data->air_mode = notify_air_mode; > > + schedule_work(&data->work); > > + > > + break; > > + } > > + default: > > + break; > > + } > > +} > > + > > static int btusb_recv_event(struct btusb_data *data, struct sk_buff *skb) > > { > > if (data->intr_interval) { > > @@ -1120,6 +1253,10 @@ 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 dis/connected */ > > + if (hci_dev_test_flag(data->hdev, HCI_USER_CHANNEL)) > > + btusb_sco_conn_change(data, skb); > > I'd recommend adding a check for Kconfig/module parameter in the if > statement so btusb_sco_conn_change only runs on distros with it > enabled so we don't risk something not working as expected just > because someone decided to open the user channel. Sure will add it in the next patch. > > > return data->recv_event(data->hdev, skb); > > } > > > > @@ -1919,44 +2056,6 @@ static void btusb_stop_traffic(struct btusb_data *data) > > usb_kill_anchored_urbs(&data->ctrl_anchor); > > } > > > > -static int btusb_close(struct hci_dev *hdev) > > -{ > > - struct btusb_data *data = hci_get_drvdata(hdev); > > - int err; > > - > > - BT_DBG("%s", hdev->name); > > - > > - cancel_delayed_work(&data->rx_work); > > - cancel_work_sync(&data->work); > > - cancel_work_sync(&data->waker); > > - > > - skb_queue_purge(&data->acl_q); > > - > > - clear_bit(BTUSB_ISOC_RUNNING, &data->flags); > > - clear_bit(BTUSB_BULK_RUNNING, &data->flags); > > - clear_bit(BTUSB_INTR_RUNNING, &data->flags); > > - clear_bit(BTUSB_DIAG_RUNNING, &data->flags); > > - > > - btusb_stop_traffic(data); > > - btusb_free_frags(data); > > - > > - err = usb_autopm_get_interface(data->intf); > > - if (err < 0) > > - goto failed; > > - > > - data->intf->needs_remote_wakeup = 0; > > - > > - /* Enable remote wake up for auto-suspend */ > > - if (test_bit(BTUSB_WAKEUP_AUTOSUSPEND, &data->flags)) > > - data->intf->needs_remote_wakeup = 1; > > - > > - usb_autopm_put_interface(data->intf); > > - > > -failed: > > - usb_scuttle_anchored_urbs(&data->deferred); > > - return 0; > > -} > > - > > static int btusb_flush(struct hci_dev *hdev) > > { > > struct btusb_data *data = hci_get_drvdata(hdev); > > @@ -2327,6 +2426,51 @@ static void btusb_work(struct work_struct *work) > > } > > } > > > > +static int btusb_close(struct hci_dev *hdev) > > +{ > > + struct btusb_data *data = hci_get_drvdata(hdev); > > + int err; > > + > > + BT_DBG("%s", hdev->name); > > + > > + if (hci_dev_test_flag(hdev, HCI_USER_CHANNEL)) { > > + btusb_sco_handle_list_clear(&data->sco_handle_list); > > + data->sco_num = 0; > > + if (data->isoc_altsetting != 0) > > + btusb_switch_alt_setting(hdev, 0); > > + } > > + > > + cancel_delayed_work(&data->rx_work); > > + cancel_work_sync(&data->work); > > + cancel_work_sync(&data->waker); > > + > > + skb_queue_purge(&data->acl_q); > > + > > + clear_bit(BTUSB_ISOC_RUNNING, &data->flags); > > + clear_bit(BTUSB_BULK_RUNNING, &data->flags); > > + clear_bit(BTUSB_INTR_RUNNING, &data->flags); > > + clear_bit(BTUSB_DIAG_RUNNING, &data->flags); > > + > > + btusb_stop_traffic(data); > > + btusb_free_frags(data); > > + > > + err = usb_autopm_get_interface(data->intf); > > + if (err < 0) > > + goto failed; > > + > > + data->intf->needs_remote_wakeup = 0; > > + > > + /* Enable remote wake up for auto-suspend */ > > + if (test_bit(BTUSB_WAKEUP_AUTOSUSPEND, &data->flags)) > > + data->intf->needs_remote_wakeup = 1; > > + > > + usb_autopm_put_interface(data->intf); > > + > > +failed: > > + usb_scuttle_anchored_urbs(&data->deferred); > > + return 0; > > +} > > + > > static void btusb_waker(struct work_struct *work) > > { > > struct btusb_data *data = container_of(work, struct btusb_data, waker); > > @@ -3782,6 +3926,8 @@ static int btusb_probe(struct usb_interface *intf, > > data->udev = interface_to_usbdev(intf); > > data->intf = intf; > > > > + INIT_LIST_HEAD(&data->sco_handle_list); > > + > > INIT_WORK(&data->work, btusb_work); > > INIT_WORK(&data->waker, btusb_waker); > > INIT_DELAYED_WORK(&data->rx_work, btusb_rx_work); > > @@ -4117,6 +4263,8 @@ static void btusb_disconnect(struct usb_interface *intf) > > hdev = data->hdev; > > usb_set_intfdata(data->intf, NULL); > > > > + btusb_sco_handle_list_clear(&data->sco_handle_list); > > + > > if (data->isoc) { > > device_remove_file(&intf->dev, &dev_attr_isoc_alt); > > usb_set_intfdata(data->isoc, NULL); > > -- > > 2.48.1.601.g30ceb7b040-goog > > > > > -- > Luiz Augusto von Dentz
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=936859 ---Test result--- Test Summary: CheckPatch PENDING 0.41 seconds GitLint PENDING 0.33 seconds SubjectPrefix PASS 0.06 seconds BuildKernel PASS 24.31 seconds CheckAllWarning PASS 26.77 seconds CheckSparse PASS 30.62 seconds BuildKernel32 PASS 24.18 seconds TestRunnerSetup PASS 428.79 seconds TestRunner_l2cap-tester PASS 21.02 seconds TestRunner_iso-tester PASS 37.62 seconds TestRunner_bnep-tester PASS 4.69 seconds TestRunner_mgmt-tester FAIL 122.71 seconds TestRunner_rfcomm-tester PASS 7.92 seconds TestRunner_sco-tester PASS 10.94 seconds TestRunner_ioctl-tester PASS 8.18 seconds TestRunner_mesh-tester PASS 6.09 seconds TestRunner_smp-tester PASS 7.23 seconds TestRunner_userchan-tester PASS 4.91 seconds IncrementalBuild PENDING 0.80 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: 485 (99.0%), Failed: 1, Not Run: 4 Failed Test Cases LL Privacy - Start Discovery 1 (Disable RL) Failed 0.170 seconds ############################## Test: IncrementalBuild - PENDING Desc: Incremental build with the patches in the series Output: --- Regards, Linux Bluetooth
Hi Hsin-chen, On Sun, Feb 23, 2025 at 10:21 PM Hsin-chen Chuang <chharry@google.com> wrote: > > Hi Luiz, > > On Mon, Feb 24, 2025 at 10:53 AM Luiz Augusto von Dentz > <luiz.dentz@gmail.com> wrote: > > > > Hi Hsin-chen, > > > > On Sun, Feb 23, 2025 at 9:26 PM Hsin-chen Chuang <chharry@google.com> wrote: > > > > > > From: Hsin-chen Chuang <chharry@chromium.org> > > > > > > Automatically configure the altsetting for USER_CHANNEL when a SCO is > > > connected or disconnected. This adds support for the USER_CHANNEL to > > > transfer SCO data over USB transport. > > > > I guess the tracking of handles is about handling disconnections, > > right? I wonder if we can get away without doing that, I didn't intend > > to add a whole bunch of changes in order to switch to the right mode, > > I get that you may want to disable the isochronous endpoint in case > > there is no connection, but I guess that only matters if we are > > talking about power but the impact is probably small so I don't think > > it is worth it. There is an alternative to match the SCO/eSCO handle > > via mask, like we do with ISO handles, that is probably a lot cheaper > > than attempting to add a whole list for tracking handles, but it has > > the downside that it is vendor/model specific. > > Yes, that's for handling disconnection. What do you think if we keep > only one handle in the driver data? That is, assume there's at most > one SCO connection. Then we don't need a list but just a u16. Hmm, if you can guarantee that it only support at most 1 SCO connection that is fine, that said the user channel can be opened/closed in between so we would have to monitor things like reset as well, so I think we actually need to depend on the Kconfig/module parameter to ensure that only user mode will be used so it is safe to track the handle, that said I think you will need to intercept things like reset anyway since it does affect any handles the driver would have stored so you probably need to change the alt setting in case an SCO connection was established. Btw, if we really want to be safe here we should probably think about ways to test loading the btusb on bluez CI and adding testing to it, that said that would require emulation to USB vid/pid and possibly the vendor commands necessary in order to set up the controller. > > > > > Fixes: b16b327edb4d ("Bluetooth: btusb: add sysfs attribute to control USB alt setting") > > > Signed-off-by: Hsin-chen Chuang <chharry@chromium.org> > > > --- > > > > > > drivers/bluetooth/btusb.c | 224 +++++++++++++++++++++++++++++++------- > > > 1 file changed, 186 insertions(+), 38 deletions(-) > > > > > > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c > > > index de3fa725d210..dfb0918dfe98 100644 > > > --- a/drivers/bluetooth/btusb.c > > > +++ b/drivers/bluetooth/btusb.c > > > @@ -854,6 +854,11 @@ struct qca_dump_info { > > > #define BTUSB_ALT6_CONTINUOUS_TX 16 > > > #define BTUSB_HW_SSR_ACTIVE 17 > > > > > > +struct sco_handle_list { > > > + struct list_head list; > > > + u16 handle; > > > +}; > > > + > > > struct btusb_data { > > > struct hci_dev *hdev; > > > struct usb_device *udev; > > > @@ -920,6 +925,9 @@ struct btusb_data { > > > int oob_wake_irq; /* irq for out-of-band wake-on-bt */ > > > > > > struct qca_dump_info qca_dump; > > > + > > > + /* Records the exsiting SCO handles for HCI_USER_CHANNEL */ > > > + struct list_head sco_handle_list; > > > }; > > > > > > static void btusb_reset(struct hci_dev *hdev) > > > @@ -1113,6 +1121,131 @@ static inline void btusb_free_frags(struct btusb_data *data) > > > spin_unlock_irqrestore(&data->rxlock, flags); > > > } > > > > > > +static void btusb_sco_handle_list_clear(struct list_head *list) > > > +{ > > > + struct sco_handle_list *entry, *n; > > > + > > > + list_for_each_entry_safe(entry, n, list, list) { > > > + list_del(&entry->list); > > > + kfree(entry); > > > + } > > > +} > > > + > > > +static struct sco_handle_list *btusb_sco_handle_list_find( > > > + struct list_head *list, > > > + u16 handle) > > > +{ > > > + struct sco_handle_list *entry; > > > + > > > + list_for_each_entry(entry, list, list) > > > + if (entry->handle == handle) > > > + return entry; > > > + > > > + return NULL; > > > +} > > > + > > > +static int btusb_sco_handle_list_add(struct list_head *list, u16 handle) > > > +{ > > > + struct sco_handle_list *entry; > > > + > > > + if (btusb_sco_handle_list_find(list, handle)) > > > + return -EEXIST; > > > + > > > + entry = kzalloc(sizeof(*entry), GFP_KERNEL); > > > + if (!entry) > > > + return -ENOMEM; > > > + > > > + entry->handle = handle; > > > + list_add(&entry->list, list); > > > + > > > + return 0; > > > +} > > > + > > > +static int btusb_sco_handle_list_del(struct list_head *list, u16 handle) > > > +{ > > > + struct sco_handle_list *entry; > > > + > > > + entry = btusb_sco_handle_list_find(list, handle); > > > + if (!entry) > > > + return -ENOENT; > > > + > > > + list_del(&entry->list); > > > + kfree(entry); > > > + > > > + return 0; > > > +} > > > + > > > +static void btusb_sco_conn_change(struct btusb_data *data, struct sk_buff *skb) > > > +{ > > > + struct hci_event_hdr *hdr = (void *) skb->data; > > > + struct hci_dev *hdev = data->hdev; > > > + > > > + if (hci_skb_pkt_type(skb) != HCI_EVENT_PKT || skb->len < sizeof(*hdr)) > > > + return; > > > + > > > + switch (hdr->evt) { > > > + case HCI_EV_DISCONN_COMPLETE: { > > > + struct hci_ev_disconn_complete *ev = > > > + (void *) skb->data + sizeof(*hdr); > > > + u16 handle; > > > + > > > + if (skb->len != sizeof(*hdr) + sizeof(*ev) || ev->status) > > > + return; > > > + > > > + handle = __le16_to_cpu(ev->handle); > > > + if (btusb_sco_handle_list_del(&data->sco_handle_list, > > > + handle) < 0) > > > + return; > > > + > > > + bt_dev_info(hdev, "disabling SCO"); > > > + data->sco_num--; > > > + data->air_mode = HCI_NOTIFY_DISABLE_SCO; > > > + schedule_work(&data->work); > > > + > > > + 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 (btusb_sco_handle_list_add(&data->sco_handle_list, > > > + handle) < 0) { > > > + 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_num++; > > > + data->air_mode = notify_air_mode; > > > + schedule_work(&data->work); > > > + > > > + break; > > > + } > > > + default: > > > + break; > > > + } > > > +} > > > + > > > static int btusb_recv_event(struct btusb_data *data, struct sk_buff *skb) > > > { > > > if (data->intr_interval) { > > > @@ -1120,6 +1253,10 @@ 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 dis/connected */ > > > + if (hci_dev_test_flag(data->hdev, HCI_USER_CHANNEL)) > > > + btusb_sco_conn_change(data, skb); > > > > I'd recommend adding a check for Kconfig/module parameter in the if > > statement so btusb_sco_conn_change only runs on distros with it > > enabled so we don't risk something not working as expected just > > because someone decided to open the user channel. > > Sure will add it in the next patch. > > > > > > return data->recv_event(data->hdev, skb); > > > } > > > > > > @@ -1919,44 +2056,6 @@ static void btusb_stop_traffic(struct btusb_data *data) > > > usb_kill_anchored_urbs(&data->ctrl_anchor); > > > } > > > > > > -static int btusb_close(struct hci_dev *hdev) > > > -{ > > > - struct btusb_data *data = hci_get_drvdata(hdev); > > > - int err; > > > - > > > - BT_DBG("%s", hdev->name); > > > - > > > - cancel_delayed_work(&data->rx_work); > > > - cancel_work_sync(&data->work); > > > - cancel_work_sync(&data->waker); > > > - > > > - skb_queue_purge(&data->acl_q); > > > - > > > - clear_bit(BTUSB_ISOC_RUNNING, &data->flags); > > > - clear_bit(BTUSB_BULK_RUNNING, &data->flags); > > > - clear_bit(BTUSB_INTR_RUNNING, &data->flags); > > > - clear_bit(BTUSB_DIAG_RUNNING, &data->flags); > > > - > > > - btusb_stop_traffic(data); > > > - btusb_free_frags(data); > > > - > > > - err = usb_autopm_get_interface(data->intf); > > > - if (err < 0) > > > - goto failed; > > > - > > > - data->intf->needs_remote_wakeup = 0; > > > - > > > - /* Enable remote wake up for auto-suspend */ > > > - if (test_bit(BTUSB_WAKEUP_AUTOSUSPEND, &data->flags)) > > > - data->intf->needs_remote_wakeup = 1; > > > - > > > - usb_autopm_put_interface(data->intf); > > > - > > > -failed: > > > - usb_scuttle_anchored_urbs(&data->deferred); > > > - return 0; > > > -} > > > - > > > static int btusb_flush(struct hci_dev *hdev) > > > { > > > struct btusb_data *data = hci_get_drvdata(hdev); > > > @@ -2327,6 +2426,51 @@ static void btusb_work(struct work_struct *work) > > > } > > > } > > > > > > +static int btusb_close(struct hci_dev *hdev) > > > +{ > > > + struct btusb_data *data = hci_get_drvdata(hdev); > > > + int err; > > > + > > > + BT_DBG("%s", hdev->name); > > > + > > > + if (hci_dev_test_flag(hdev, HCI_USER_CHANNEL)) { > > > + btusb_sco_handle_list_clear(&data->sco_handle_list); > > > + data->sco_num = 0; > > > + if (data->isoc_altsetting != 0) > > > + btusb_switch_alt_setting(hdev, 0); > > > + } > > > + > > > + cancel_delayed_work(&data->rx_work); > > > + cancel_work_sync(&data->work); > > > + cancel_work_sync(&data->waker); > > > + > > > + skb_queue_purge(&data->acl_q); > > > + > > > + clear_bit(BTUSB_ISOC_RUNNING, &data->flags); > > > + clear_bit(BTUSB_BULK_RUNNING, &data->flags); > > > + clear_bit(BTUSB_INTR_RUNNING, &data->flags); > > > + clear_bit(BTUSB_DIAG_RUNNING, &data->flags); > > > + > > > + btusb_stop_traffic(data); > > > + btusb_free_frags(data); > > > + > > > + err = usb_autopm_get_interface(data->intf); > > > + if (err < 0) > > > + goto failed; > > > + > > > + data->intf->needs_remote_wakeup = 0; > > > + > > > + /* Enable remote wake up for auto-suspend */ > > > + if (test_bit(BTUSB_WAKEUP_AUTOSUSPEND, &data->flags)) > > > + data->intf->needs_remote_wakeup = 1; > > > + > > > + usb_autopm_put_interface(data->intf); > > > + > > > +failed: > > > + usb_scuttle_anchored_urbs(&data->deferred); > > > + return 0; > > > +} > > > + > > > static void btusb_waker(struct work_struct *work) > > > { > > > struct btusb_data *data = container_of(work, struct btusb_data, waker); > > > @@ -3782,6 +3926,8 @@ static int btusb_probe(struct usb_interface *intf, > > > data->udev = interface_to_usbdev(intf); > > > data->intf = intf; > > > > > > + INIT_LIST_HEAD(&data->sco_handle_list); > > > + > > > INIT_WORK(&data->work, btusb_work); > > > INIT_WORK(&data->waker, btusb_waker); > > > INIT_DELAYED_WORK(&data->rx_work, btusb_rx_work); > > > @@ -4117,6 +4263,8 @@ static void btusb_disconnect(struct usb_interface *intf) > > > hdev = data->hdev; > > > usb_set_intfdata(data->intf, NULL); > > > > > > + btusb_sco_handle_list_clear(&data->sco_handle_list); > > > + > > > if (data->isoc) { > > > device_remove_file(&intf->dev, &dev_attr_isoc_alt); > > > usb_set_intfdata(data->isoc, NULL); > > > -- > > > 2.48.1.601.g30ceb7b040-goog > > > > > > > > > -- > > Luiz Augusto von Dentz > > -- > Best Regards, > Hsin-chen
Hi Luiz, On Mon, Feb 24, 2025 at 11:36 AM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > > Hi Hsin-chen, > > On Sun, Feb 23, 2025 at 10:21 PM Hsin-chen Chuang <chharry@google.com> wrote: > > > > Hi Luiz, > > > > On Mon, Feb 24, 2025 at 10:53 AM Luiz Augusto von Dentz > > <luiz.dentz@gmail.com> wrote: > > > > > > Hi Hsin-chen, > > > > > > On Sun, Feb 23, 2025 at 9:26 PM Hsin-chen Chuang <chharry@google.com> wrote: > > > > > > > > From: Hsin-chen Chuang <chharry@chromium.org> > > > > > > > > Automatically configure the altsetting for USER_CHANNEL when a SCO is > > > > connected or disconnected. This adds support for the USER_CHANNEL to > > > > transfer SCO data over USB transport. > > > > > > I guess the tracking of handles is about handling disconnections, > > > right? I wonder if we can get away without doing that, I didn't intend > > > to add a whole bunch of changes in order to switch to the right mode, > > > I get that you may want to disable the isochronous endpoint in case > > > there is no connection, but I guess that only matters if we are > > > talking about power but the impact is probably small so I don't think > > > it is worth it. There is an alternative to match the SCO/eSCO handle > > > via mask, like we do with ISO handles, that is probably a lot cheaper > > > than attempting to add a whole list for tracking handles, but it has > > > the downside that it is vendor/model specific. > > > > Yes, that's for handling disconnection. What do you think if we keep > > only one handle in the driver data? That is, assume there's at most > > one SCO connection. Then we don't need a list but just a u16. > > Hmm, if you can guarantee that it only support at most 1 SCO > connection that is fine, that said the user channel can be > opened/closed in between so we would have to monitor things like reset > as well, so I think we actually need to depend on the Kconfig/module > parameter to ensure that only user mode will be used so it is safe to > track the handle, that said I think you will need to intercept things > like reset anyway since it does affect any handles the driver would > have stored so you probably need to change the alt setting in case an > SCO connection was established. Thanks for the explanation and I understood your concern. Indeed tracking handles would require way too much effort to ensure it's correct. I will follow your first approach to keep it simple for now. > > Btw, if we really want to be safe here we should probably think about > ways to test loading the btusb on bluez CI and adding testing to it, > that said that would require emulation to USB vid/pid and possibly the > vendor commands necessary in order to set up the controller. > > > > > > > > Fixes: b16b327edb4d ("Bluetooth: btusb: add sysfs attribute to control USB alt setting") > > > > Signed-off-by: Hsin-chen Chuang <chharry@chromium.org> > > > > --- > > > > > > > > drivers/bluetooth/btusb.c | 224 +++++++++++++++++++++++++++++++------- > > > > 1 file changed, 186 insertions(+), 38 deletions(-) > > > > > > > > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c > > > > index de3fa725d210..dfb0918dfe98 100644 > > > > --- a/drivers/bluetooth/btusb.c > > > > +++ b/drivers/bluetooth/btusb.c > > > > @@ -854,6 +854,11 @@ struct qca_dump_info { > > > > #define BTUSB_ALT6_CONTINUOUS_TX 16 > > > > #define BTUSB_HW_SSR_ACTIVE 17 > > > > > > > > +struct sco_handle_list { > > > > + struct list_head list; > > > > + u16 handle; > > > > +}; > > > > + > > > > struct btusb_data { > > > > struct hci_dev *hdev; > > > > struct usb_device *udev; > > > > @@ -920,6 +925,9 @@ struct btusb_data { > > > > int oob_wake_irq; /* irq for out-of-band wake-on-bt */ > > > > > > > > struct qca_dump_info qca_dump; > > > > + > > > > + /* Records the exsiting SCO handles for HCI_USER_CHANNEL */ > > > > + struct list_head sco_handle_list; > > > > }; > > > > > > > > static void btusb_reset(struct hci_dev *hdev) > > > > @@ -1113,6 +1121,131 @@ static inline void btusb_free_frags(struct btusb_data *data) > > > > spin_unlock_irqrestore(&data->rxlock, flags); > > > > } > > > > > > > > +static void btusb_sco_handle_list_clear(struct list_head *list) > > > > +{ > > > > + struct sco_handle_list *entry, *n; > > > > + > > > > + list_for_each_entry_safe(entry, n, list, list) { > > > > + list_del(&entry->list); > > > > + kfree(entry); > > > > + } > > > > +} > > > > + > > > > +static struct sco_handle_list *btusb_sco_handle_list_find( > > > > + struct list_head *list, > > > > + u16 handle) > > > > +{ > > > > + struct sco_handle_list *entry; > > > > + > > > > + list_for_each_entry(entry, list, list) > > > > + if (entry->handle == handle) > > > > + return entry; > > > > + > > > > + return NULL; > > > > +} > > > > + > > > > +static int btusb_sco_handle_list_add(struct list_head *list, u16 handle) > > > > +{ > > > > + struct sco_handle_list *entry; > > > > + > > > > + if (btusb_sco_handle_list_find(list, handle)) > > > > + return -EEXIST; > > > > + > > > > + entry = kzalloc(sizeof(*entry), GFP_KERNEL); > > > > + if (!entry) > > > > + return -ENOMEM; > > > > + > > > > + entry->handle = handle; > > > > + list_add(&entry->list, list); > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static int btusb_sco_handle_list_del(struct list_head *list, u16 handle) > > > > +{ > > > > + struct sco_handle_list *entry; > > > > + > > > > + entry = btusb_sco_handle_list_find(list, handle); > > > > + if (!entry) > > > > + return -ENOENT; > > > > + > > > > + list_del(&entry->list); > > > > + kfree(entry); > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static void btusb_sco_conn_change(struct btusb_data *data, struct sk_buff *skb) > > > > +{ > > > > + struct hci_event_hdr *hdr = (void *) skb->data; > > > > + struct hci_dev *hdev = data->hdev; > > > > + > > > > + if (hci_skb_pkt_type(skb) != HCI_EVENT_PKT || skb->len < sizeof(*hdr)) > > > > + return; > > > > + > > > > + switch (hdr->evt) { > > > > + case HCI_EV_DISCONN_COMPLETE: { > > > > + struct hci_ev_disconn_complete *ev = > > > > + (void *) skb->data + sizeof(*hdr); > > > > + u16 handle; > > > > + > > > > + if (skb->len != sizeof(*hdr) + sizeof(*ev) || ev->status) > > > > + return; > > > > + > > > > + handle = __le16_to_cpu(ev->handle); > > > > + if (btusb_sco_handle_list_del(&data->sco_handle_list, > > > > + handle) < 0) > > > > + return; > > > > + > > > > + bt_dev_info(hdev, "disabling SCO"); > > > > + data->sco_num--; > > > > + data->air_mode = HCI_NOTIFY_DISABLE_SCO; > > > > + schedule_work(&data->work); > > > > + > > > > + 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 (btusb_sco_handle_list_add(&data->sco_handle_list, > > > > + handle) < 0) { > > > > + 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_num++; > > > > + data->air_mode = notify_air_mode; > > > > + schedule_work(&data->work); > > > > + > > > > + break; > > > > + } > > > > + default: > > > > + break; > > > > + } > > > > +} > > > > + > > > > static int btusb_recv_event(struct btusb_data *data, struct sk_buff *skb) > > > > { > > > > if (data->intr_interval) { > > > > @@ -1120,6 +1253,10 @@ 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 dis/connected */ > > > > + if (hci_dev_test_flag(data->hdev, HCI_USER_CHANNEL)) > > > > + btusb_sco_conn_change(data, skb); > > > > > > I'd recommend adding a check for Kconfig/module parameter in the if > > > statement so btusb_sco_conn_change only runs on distros with it > > > enabled so we don't risk something not working as expected just > > > because someone decided to open the user channel. > > > > Sure will add it in the next patch. > > > > > > > > > return data->recv_event(data->hdev, skb); > > > > } > > > > > > > > @@ -1919,44 +2056,6 @@ static void btusb_stop_traffic(struct btusb_data *data) > > > > usb_kill_anchored_urbs(&data->ctrl_anchor); > > > > } > > > > > > > > -static int btusb_close(struct hci_dev *hdev) > > > > -{ > > > > - struct btusb_data *data = hci_get_drvdata(hdev); > > > > - int err; > > > > - > > > > - BT_DBG("%s", hdev->name); > > > > - > > > > - cancel_delayed_work(&data->rx_work); > > > > - cancel_work_sync(&data->work); > > > > - cancel_work_sync(&data->waker); > > > > - > > > > - skb_queue_purge(&data->acl_q); > > > > - > > > > - clear_bit(BTUSB_ISOC_RUNNING, &data->flags); > > > > - clear_bit(BTUSB_BULK_RUNNING, &data->flags); > > > > - clear_bit(BTUSB_INTR_RUNNING, &data->flags); > > > > - clear_bit(BTUSB_DIAG_RUNNING, &data->flags); > > > > - > > > > - btusb_stop_traffic(data); > > > > - btusb_free_frags(data); > > > > - > > > > - err = usb_autopm_get_interface(data->intf); > > > > - if (err < 0) > > > > - goto failed; > > > > - > > > > - data->intf->needs_remote_wakeup = 0; > > > > - > > > > - /* Enable remote wake up for auto-suspend */ > > > > - if (test_bit(BTUSB_WAKEUP_AUTOSUSPEND, &data->flags)) > > > > - data->intf->needs_remote_wakeup = 1; > > > > - > > > > - usb_autopm_put_interface(data->intf); > > > > - > > > > -failed: > > > > - usb_scuttle_anchored_urbs(&data->deferred); > > > > - return 0; > > > > -} > > > > - > > > > static int btusb_flush(struct hci_dev *hdev) > > > > { > > > > struct btusb_data *data = hci_get_drvdata(hdev); > > > > @@ -2327,6 +2426,51 @@ static void btusb_work(struct work_struct *work) > > > > } > > > > } > > > > > > > > +static int btusb_close(struct hci_dev *hdev) > > > > +{ > > > > + struct btusb_data *data = hci_get_drvdata(hdev); > > > > + int err; > > > > + > > > > + BT_DBG("%s", hdev->name); > > > > + > > > > + if (hci_dev_test_flag(hdev, HCI_USER_CHANNEL)) { > > > > + btusb_sco_handle_list_clear(&data->sco_handle_list); > > > > + data->sco_num = 0; > > > > + if (data->isoc_altsetting != 0) > > > > + btusb_switch_alt_setting(hdev, 0); > > > > + } > > > > + > > > > + cancel_delayed_work(&data->rx_work); > > > > + cancel_work_sync(&data->work); > > > > + cancel_work_sync(&data->waker); > > > > + > > > > + skb_queue_purge(&data->acl_q); > > > > + > > > > + clear_bit(BTUSB_ISOC_RUNNING, &data->flags); > > > > + clear_bit(BTUSB_BULK_RUNNING, &data->flags); > > > > + clear_bit(BTUSB_INTR_RUNNING, &data->flags); > > > > + clear_bit(BTUSB_DIAG_RUNNING, &data->flags); > > > > + > > > > + btusb_stop_traffic(data); > > > > + btusb_free_frags(data); > > > > + > > > > + err = usb_autopm_get_interface(data->intf); > > > > + if (err < 0) > > > > + goto failed; > > > > + > > > > + data->intf->needs_remote_wakeup = 0; > > > > + > > > > + /* Enable remote wake up for auto-suspend */ > > > > + if (test_bit(BTUSB_WAKEUP_AUTOSUSPEND, &data->flags)) > > > > + data->intf->needs_remote_wakeup = 1; > > > > + > > > > + usb_autopm_put_interface(data->intf); > > > > + > > > > +failed: > > > > + usb_scuttle_anchored_urbs(&data->deferred); > > > > + return 0; > > > > +} > > > > + > > > > static void btusb_waker(struct work_struct *work) > > > > { > > > > struct btusb_data *data = container_of(work, struct btusb_data, waker); > > > > @@ -3782,6 +3926,8 @@ static int btusb_probe(struct usb_interface *intf, > > > > data->udev = interface_to_usbdev(intf); > > > > data->intf = intf; > > > > > > > > + INIT_LIST_HEAD(&data->sco_handle_list); > > > > + > > > > INIT_WORK(&data->work, btusb_work); > > > > INIT_WORK(&data->waker, btusb_waker); > > > > INIT_DELAYED_WORK(&data->rx_work, btusb_rx_work); > > > > @@ -4117,6 +4263,8 @@ static void btusb_disconnect(struct usb_interface *intf) > > > > hdev = data->hdev; > > > > usb_set_intfdata(data->intf, NULL); > > > > > > > > + btusb_sco_handle_list_clear(&data->sco_handle_list); > > > > + > > > > if (data->isoc) { > > > > device_remove_file(&intf->dev, &dev_attr_isoc_alt); > > > > usb_set_intfdata(data->isoc, NULL); > > > > -- > > > > 2.48.1.601.g30ceb7b040-goog > > > > > > > > > > > > > -- > > > Luiz Augusto von Dentz > > > > -- > > Best Regards, > > Hsin-chen > > > > -- > Luiz Augusto von Dentz -- Best Regards, Hsin-chen
Dear Hsin-chen, Thank you for your patch. Am 24.02.25 um 03:24 schrieb Hsin-chen Chuang: > From: Hsin-chen Chuang <chharry@chromium.org> > > Automatically configure the altsetting for USER_CHANNEL when a SCO is > connected or disconnected. This adds support for the USER_CHANNEL to > transfer SCO data over USB transport. Should you re-spin, it’d be great if you elaborated a little more. Especially for the motivation. It’d be also great, if you added how to test this. > Fixes: b16b327edb4d ("Bluetooth: btusb: add sysfs attribute to control USB alt setting") > Signed-off-by: Hsin-chen Chuang <chharry@chromium.org> > --- > > drivers/bluetooth/btusb.c | 224 +++++++++++++++++++++++++++++++------- > 1 file changed, 186 insertions(+), 38 deletions(-) […] Kind regards, Paul
Hi Paul, On Mon, Feb 24, 2025 at 4:42 PM Paul Menzel <pmenzel@molgen.mpg.de> wrote: > > Dear Hsin-chen, > > > Thank you for your patch. > > Am 24.02.25 um 03:24 schrieb Hsin-chen Chuang: > > From: Hsin-chen Chuang <chharry@chromium.org> > > > > Automatically configure the altsetting for USER_CHANNEL when a SCO is > > connected or disconnected. This adds support for the USER_CHANNEL to > > transfer SCO data over USB transport. > > Should you re-spin, it’d be great if you elaborated a little more. > Especially for the motivation. It’d be also great, if you added how to > test this. Sure and I'll update this to the commit message in the next patch version. The motivation is to enable the HCI_USER_CHANNEL user to send out SCO data through USB Bluetooth chips, which is mainly used for bidirectional audio transfer (voice call). This was not capable because - Per Bluetooth Core Spec v5, Vol 4, Part B, 2.1, the corresponding alternate setting should be set based on the air mode in order to transfer SCO data. - The Linux Bluetooth HCI_USER_CHANNEL exposes the Bluetooth Host Controller Interface to the user space, which is something above the USB layer. The user space is not able to configure the USB alt while keeping the channel open. This patch intercepts the specific packets that indicate the air mode change, and configure the alt setting transparently in btusb. I tested this patch on ChromeOS devices which are now using the Android Bluetooth stack built on top of the HCI_USER_CHANNEL. The USB Bluetooth models could work without a customized kernel. > > > Fixes: b16b327edb4d ("Bluetooth: btusb: add sysfs attribute to control USB alt setting") > > Signed-off-by: Hsin-chen Chuang <chharry@chromium.org> > > --- > > > > drivers/bluetooth/btusb.c | 224 +++++++++++++++++++++++++++++++------- > > 1 file changed, 186 insertions(+), 38 deletions(-) > > […] > > > Kind regards, > > Paul -- Best Regards, Hsin-chen
Dear Hsin-chen, Am 24.02.25 um 14:06 schrieb Hsin-chen Chuang: > On Mon, Feb 24, 2025 at 4:42 PM Paul Menzel <pmenzel@molgen.mpg.de> wrote: >> Am 24.02.25 um 03:24 schrieb Hsin-chen Chuang: >>> From: Hsin-chen Chuang <chharry@chromium.org> >>> >>> Automatically configure the altsetting for USER_CHANNEL when a SCO is >>> connected or disconnected. This adds support for the USER_CHANNEL to >>> transfer SCO data over USB transport. >> >> Should you re-spin, it’d be great if you elaborated a little more. >> Especially for the motivation. It’d be also great, if you added how to >> test this. > > Sure and I'll update this to the commit message in the next patch version. > > The motivation is to enable the HCI_USER_CHANNEL user to send out SCO > data through USB Bluetooth chips, which is mainly used for > bidirectional audio transfer (voice call). > This was not capable because > - Per Bluetooth Core Spec v5, Vol 4, Part B, 2.1, the corresponding > alternate setting should be set based on the air mode in order to > transfer SCO data. > - The Linux Bluetooth HCI_USER_CHANNEL exposes the Bluetooth Host > Controller Interface to the user space, which is something above the > USB layer. The user space is not able to configure the USB alt while > keeping the channel open. > > This patch intercepts the specific packets that indicate the air mode > change, and configure the alt setting transparently in btusb. > I tested this patch on ChromeOS devices which are now using the > Android Bluetooth stack built on top of the HCI_USER_CHANNEL. The USB > Bluetooth models could work without a customized kernel. Awesome. Great explanation. It’d be great to have in the commit message. I am looking forward to the next iteration. >>> Fixes: b16b327edb4d ("Bluetooth: btusb: add sysfs attribute to control USB alt setting") >>> Signed-off-by: Hsin-chen Chuang <chharry@chromium.org> >>> --- >>> >>> drivers/bluetooth/btusb.c | 224 +++++++++++++++++++++++++++++++------- >>> 1 file changed, 186 insertions(+), 38 deletions(-) >> >> […] > -- > Best Regards, > Hsin-chen Unrelated, and only if you care: Your signature delimiter is missing a space at the end [1]. Kind regards, Paul [1]: https://en.wikipedia.org/wiki/Signature_block#Standard_delimiter
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c index de3fa725d210..dfb0918dfe98 100644 --- a/drivers/bluetooth/btusb.c +++ b/drivers/bluetooth/btusb.c @@ -854,6 +854,11 @@ struct qca_dump_info { #define BTUSB_ALT6_CONTINUOUS_TX 16 #define BTUSB_HW_SSR_ACTIVE 17 +struct sco_handle_list { + struct list_head list; + u16 handle; +}; + struct btusb_data { struct hci_dev *hdev; struct usb_device *udev; @@ -920,6 +925,9 @@ struct btusb_data { int oob_wake_irq; /* irq for out-of-band wake-on-bt */ struct qca_dump_info qca_dump; + + /* Records the exsiting SCO handles for HCI_USER_CHANNEL */ + struct list_head sco_handle_list; }; static void btusb_reset(struct hci_dev *hdev) @@ -1113,6 +1121,131 @@ static inline void btusb_free_frags(struct btusb_data *data) spin_unlock_irqrestore(&data->rxlock, flags); } +static void btusb_sco_handle_list_clear(struct list_head *list) +{ + struct sco_handle_list *entry, *n; + + list_for_each_entry_safe(entry, n, list, list) { + list_del(&entry->list); + kfree(entry); + } +} + +static struct sco_handle_list *btusb_sco_handle_list_find( + struct list_head *list, + u16 handle) +{ + struct sco_handle_list *entry; + + list_for_each_entry(entry, list, list) + if (entry->handle == handle) + return entry; + + return NULL; +} + +static int btusb_sco_handle_list_add(struct list_head *list, u16 handle) +{ + struct sco_handle_list *entry; + + if (btusb_sco_handle_list_find(list, handle)) + return -EEXIST; + + entry = kzalloc(sizeof(*entry), GFP_KERNEL); + if (!entry) + return -ENOMEM; + + entry->handle = handle; + list_add(&entry->list, list); + + return 0; +} + +static int btusb_sco_handle_list_del(struct list_head *list, u16 handle) +{ + struct sco_handle_list *entry; + + entry = btusb_sco_handle_list_find(list, handle); + if (!entry) + return -ENOENT; + + list_del(&entry->list); + kfree(entry); + + return 0; +} + +static void btusb_sco_conn_change(struct btusb_data *data, struct sk_buff *skb) +{ + struct hci_event_hdr *hdr = (void *) skb->data; + struct hci_dev *hdev = data->hdev; + + if (hci_skb_pkt_type(skb) != HCI_EVENT_PKT || skb->len < sizeof(*hdr)) + return; + + switch (hdr->evt) { + case HCI_EV_DISCONN_COMPLETE: { + struct hci_ev_disconn_complete *ev = + (void *) skb->data + sizeof(*hdr); + u16 handle; + + if (skb->len != sizeof(*hdr) + sizeof(*ev) || ev->status) + return; + + handle = __le16_to_cpu(ev->handle); + if (btusb_sco_handle_list_del(&data->sco_handle_list, + handle) < 0) + return; + + bt_dev_info(hdev, "disabling SCO"); + data->sco_num--; + data->air_mode = HCI_NOTIFY_DISABLE_SCO; + schedule_work(&data->work); + + 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 (btusb_sco_handle_list_add(&data->sco_handle_list, + handle) < 0) { + 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_num++; + data->air_mode = notify_air_mode; + schedule_work(&data->work); + + break; + } + default: + break; + } +} + static int btusb_recv_event(struct btusb_data *data, struct sk_buff *skb) { if (data->intr_interval) { @@ -1120,6 +1253,10 @@ 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 dis/connected */ + if (hci_dev_test_flag(data->hdev, HCI_USER_CHANNEL)) + btusb_sco_conn_change(data, skb); + return data->recv_event(data->hdev, skb); } @@ -1919,44 +2056,6 @@ static void btusb_stop_traffic(struct btusb_data *data) usb_kill_anchored_urbs(&data->ctrl_anchor); } -static int btusb_close(struct hci_dev *hdev) -{ - struct btusb_data *data = hci_get_drvdata(hdev); - int err; - - BT_DBG("%s", hdev->name); - - cancel_delayed_work(&data->rx_work); - cancel_work_sync(&data->work); - cancel_work_sync(&data->waker); - - skb_queue_purge(&data->acl_q); - - clear_bit(BTUSB_ISOC_RUNNING, &data->flags); - clear_bit(BTUSB_BULK_RUNNING, &data->flags); - clear_bit(BTUSB_INTR_RUNNING, &data->flags); - clear_bit(BTUSB_DIAG_RUNNING, &data->flags); - - btusb_stop_traffic(data); - btusb_free_frags(data); - - err = usb_autopm_get_interface(data->intf); - if (err < 0) - goto failed; - - data->intf->needs_remote_wakeup = 0; - - /* Enable remote wake up for auto-suspend */ - if (test_bit(BTUSB_WAKEUP_AUTOSUSPEND, &data->flags)) - data->intf->needs_remote_wakeup = 1; - - usb_autopm_put_interface(data->intf); - -failed: - usb_scuttle_anchored_urbs(&data->deferred); - return 0; -} - static int btusb_flush(struct hci_dev *hdev) { struct btusb_data *data = hci_get_drvdata(hdev); @@ -2327,6 +2426,51 @@ static void btusb_work(struct work_struct *work) } } +static int btusb_close(struct hci_dev *hdev) +{ + struct btusb_data *data = hci_get_drvdata(hdev); + int err; + + BT_DBG("%s", hdev->name); + + if (hci_dev_test_flag(hdev, HCI_USER_CHANNEL)) { + btusb_sco_handle_list_clear(&data->sco_handle_list); + data->sco_num = 0; + if (data->isoc_altsetting != 0) + btusb_switch_alt_setting(hdev, 0); + } + + cancel_delayed_work(&data->rx_work); + cancel_work_sync(&data->work); + cancel_work_sync(&data->waker); + + skb_queue_purge(&data->acl_q); + + clear_bit(BTUSB_ISOC_RUNNING, &data->flags); + clear_bit(BTUSB_BULK_RUNNING, &data->flags); + clear_bit(BTUSB_INTR_RUNNING, &data->flags); + clear_bit(BTUSB_DIAG_RUNNING, &data->flags); + + btusb_stop_traffic(data); + btusb_free_frags(data); + + err = usb_autopm_get_interface(data->intf); + if (err < 0) + goto failed; + + data->intf->needs_remote_wakeup = 0; + + /* Enable remote wake up for auto-suspend */ + if (test_bit(BTUSB_WAKEUP_AUTOSUSPEND, &data->flags)) + data->intf->needs_remote_wakeup = 1; + + usb_autopm_put_interface(data->intf); + +failed: + usb_scuttle_anchored_urbs(&data->deferred); + return 0; +} + static void btusb_waker(struct work_struct *work) { struct btusb_data *data = container_of(work, struct btusb_data, waker); @@ -3782,6 +3926,8 @@ static int btusb_probe(struct usb_interface *intf, data->udev = interface_to_usbdev(intf); data->intf = intf; + INIT_LIST_HEAD(&data->sco_handle_list); + INIT_WORK(&data->work, btusb_work); INIT_WORK(&data->waker, btusb_waker); INIT_DELAYED_WORK(&data->rx_work, btusb_rx_work); @@ -4117,6 +4263,8 @@ static void btusb_disconnect(struct usb_interface *intf) hdev = data->hdev; usb_set_intfdata(data->intf, NULL); + btusb_sco_handle_list_clear(&data->sco_handle_list); + if (data->isoc) { device_remove_file(&intf->dev, &dev_attr_isoc_alt); usb_set_intfdata(data->isoc, NULL);