drivers/bluetooth/btusb.c | 224 +++++++++++++++++++++++++++++++------- 1 file changed, 186 insertions(+), 38 deletions(-)
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.
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);
+
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-googDear 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
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
>
--
Luiz Augusto von Dentz
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
--
Best Regards,
Hsin-chen
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
--
Luiz Augusto von Dentz
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
© 2016 - 2025 Red Hat, Inc.