diff mbox series

[v3] Bluetooth: btusb: Configure altsetting for HCI_USER_CHANNEL

Message ID 20250227171417.4023415-1-chharry@google.com (mailing list archive)
State New
Headers show
Series [v3] Bluetooth: btusb: Configure altsetting for HCI_USER_CHANNEL | expand

Checks

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: 483 (98.6%), Failed: 3, 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

Commit Message

Hsin-chen Chuang Feb. 27, 2025, 5:14 p.m. UTC 
From: Hsin-chen Chuang <chharry@chromium.org>

Automatically configure the altsetting for HCI_USER_CHANNEL when a SCO
is connected.

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, but
- 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 HCI_EV_SYNC_CONN_COMPLETE packets in btusb,
extracts the air mode, and configures the alt setting in btusb.

This patch is tested on ChromeOS devices. The USB Bluetooth models
(CVSD, TRANS alt3 and alt6) 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>
---

Changes in v3:
- Remove module parameter
- Set Kconfig to default y if CHROME_PLATFORMS

Changes in v2:
- Give up tracking the SCO handles. Only configure the altsetting when
  SCO connected.
- Put the change behind Kconfig/module parameter

 drivers/bluetooth/Kconfig | 11 ++++++++++
 drivers/bluetooth/btusb.c | 43 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 54 insertions(+)

Comments

bluez.test.bot@gmail.com Feb. 27, 2025, 5:57 p.m. UTC | #1 
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=938624

---Test result---

Test Summary:
CheckPatch                    PENDING   0.31 seconds
GitLint                       PENDING   0.34 seconds
SubjectPrefix                 PASS      0.06 seconds
BuildKernel                   PASS      25.68 seconds
CheckAllWarning               PASS      27.29 seconds
CheckSparse                   PASS      31.01 seconds
BuildKernel32                 PASS      24.60 seconds
TestRunnerSetup               PASS      441.41 seconds
TestRunner_l2cap-tester       PASS      21.31 seconds
TestRunner_iso-tester         PASS      35.17 seconds
TestRunner_bnep-tester        PASS      4.91 seconds
TestRunner_mgmt-tester        FAIL      125.40 seconds
TestRunner_rfcomm-tester      PASS      8.04 seconds
TestRunner_sco-tester         PASS      13.36 seconds
TestRunner_ioctl-tester       PASS      8.67 seconds
TestRunner_mesh-tester        PASS      6.28 seconds
TestRunner_smp-tester         PASS      7.48 seconds
TestRunner_userchan-tester    PASS      5.14 seconds
IncrementalBuild              PENDING   0.52 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: 483 (98.6%), Failed: 3, Not Run: 4

Failed Test Cases
LL Privacy - Add Device 2 (2 Devices to AL)          Failed       0.191 seconds
LL Privacy - Add Device 3 (AL is full)               Failed       0.209 seconds
LL Privacy - Set Flags 1 (Add to RL)                 Failed       0.167 seconds
##############################
Test: IncrementalBuild - PENDING
Desc: Incremental build with the patches in the series
Output:



---
Regards,
Linux Bluetooth
Luiz Augusto von Dentz Feb. 27, 2025, 9:40 p.m. UTC | #2 
Hi Hsin-chen,

On Thu, Feb 27, 2025 at 12:14 PM Hsin-chen Chuang <chharry@google.com> wrote:
>
> From: Hsin-chen Chuang <chharry@chromium.org>
>
> Automatically configure the altsetting for HCI_USER_CHANNEL when a SCO
> is connected.
>
> 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, but
> - 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 HCI_EV_SYNC_CONN_COMPLETE packets in btusb,
> extracts the air mode, and configures the alt setting in btusb.
>
> This patch is tested on ChromeOS devices. The USB Bluetooth models
> (CVSD, TRANS alt3 and alt6) 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>
> ---
>
> Changes in v3:
> - Remove module parameter
> - Set Kconfig to default y if CHROME_PLATFORMS
>
> Changes in v2:
> - Give up tracking the SCO handles. Only configure the altsetting when
>   SCO connected.
> - Put the change behind Kconfig/module parameter
>
>  drivers/bluetooth/Kconfig | 11 ++++++++++
>  drivers/bluetooth/btusb.c | 43 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 54 insertions(+)
>
> diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
> index 4ab32abf0f48..cdf7a5caa5c8 100644
> --- a/drivers/bluetooth/Kconfig
> +++ b/drivers/bluetooth/Kconfig
> @@ -56,6 +56,17 @@ config BT_HCIBTUSB_POLL_SYNC
>           Say Y here to enable USB poll_sync for Bluetooth USB devices by
>           default.
>
> +config BT_HCIBTUSB_AUTO_SET_ISOC_ALT
> +       bool "Auto set isoc_altsetting for HCI_USER_CHANNEL when SCO connected"
> +       depends on BT_HCIBTUSB
> +       default y if CHROME_PLATFORMS
> +       help
> +         Say Y here to enable auto set isoc_altsetting for HCI_USER_CHANNEL
> +         when SCO connected
> +
> +         When enabled, btusb intercepts the HCI_EV_SYNC_CONN_COMPLETE packets
> +         and configures isoc_altsetting automatically for HCI_USER_CHANNEL.
> +
>  config BT_HCIBTUSB_BCM
>         bool "Broadcom protocol support"
>         depends on BT_HCIBTUSB
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index de3fa725d210..2642d2ca885f 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -34,6 +34,8 @@ static bool force_scofix;
>  static bool enable_autosuspend = IS_ENABLED(CONFIG_BT_HCIBTUSB_AUTOSUSPEND);
>  static bool enable_poll_sync = IS_ENABLED(CONFIG_BT_HCIBTUSB_POLL_SYNC);
>  static bool reset = true;
> +static bool auto_set_isoc_alt =
> +       IS_ENABLED(CONFIG_BT_HCIBTUSB_AUTO_SET_ISOC_ALT);
>
>  static struct usb_driver btusb_driver;
>
> @@ -1113,6 +1115,42 @@ static inline void btusb_free_frags(struct btusb_data *data)
>         spin_unlock_irqrestore(&data->rxlock, flags);
>  }
>
> +static void btusb_sco_connected(struct btusb_data *data, struct sk_buff *skb)
> +{
> +       struct hci_event_hdr *hdr = (void *) skb->data;
> +       struct hci_ev_sync_conn_complete *ev =
> +               (void *) skb->data + sizeof(*hdr);
> +       struct hci_dev *hdev = data->hdev;
> +       unsigned int notify_air_mode;
> +
> +       if (hci_skb_pkt_type(skb) != HCI_EVENT_PKT)
> +               return;
> +
> +       if (skb->len < sizeof(*hdr) || hdr->evt != HCI_EV_SYNC_CONN_COMPLETE)
> +               return;
> +
> +       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;
> +       }
> +
> +       bt_dev_info(hdev, "enabling SCO with air mode %u", ev->air_mode);
> +       data->sco_num = 1;
> +       data->air_mode = notify_air_mode;
> +       schedule_work(&data->work);
> +}
> +
>  static int btusb_recv_event(struct btusb_data *data, struct sk_buff *skb)
>  {
>         if (data->intr_interval) {
> @@ -1120,6 +1158,11 @@ static int btusb_recv_event(struct btusb_data *data, struct sk_buff *skb)
>                 schedule_delayed_work(&data->rx_work, 0);
>         }
>
> +       /* Configure altsetting for HCI_USER_CHANNEL on SCO connected */
> +       if (auto_set_isoc_alt &&
> +           hci_dev_test_flag(data->hdev, HCI_USER_CHANNEL))
> +               btusb_sco_connected(data, skb);
> +
>         return data->recv_event(data->hdev, skb);
>  }
>
> --
> 2.48.1.658.g4767266eb4-goog

This has been merged, I did some minor rewording here and there but
the logic remains the same, now we can problem revert b16b327edb4d
("Bluetooth: btusb: add sysfs attribute to control USB alt setting")?
Hsin-chen Chuang Feb. 28, 2025, 1:08 a.m. UTC | #3 
Hi Luiz,

On Fri, Feb 28, 2025 at 5:40 AM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Hsin-chen,
>
> On Thu, Feb 27, 2025 at 12:14 PM Hsin-chen Chuang <chharry@google.com> wrote:
> >
> > From: Hsin-chen Chuang <chharry@chromium.org>
> >
> > Automatically configure the altsetting for HCI_USER_CHANNEL when a SCO
> > is connected.
> >
> > 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, but
> > - 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 HCI_EV_SYNC_CONN_COMPLETE packets in btusb,
> > extracts the air mode, and configures the alt setting in btusb.
> >
> > This patch is tested on ChromeOS devices. The USB Bluetooth models
> > (CVSD, TRANS alt3 and alt6) 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>
> > ---
> >
> > Changes in v3:
> > - Remove module parameter
> > - Set Kconfig to default y if CHROME_PLATFORMS
> >
> > Changes in v2:
> > - Give up tracking the SCO handles. Only configure the altsetting when
> >   SCO connected.
> > - Put the change behind Kconfig/module parameter
> >
> >  drivers/bluetooth/Kconfig | 11 ++++++++++
> >  drivers/bluetooth/btusb.c | 43 +++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 54 insertions(+)
> >
> > diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
> > index 4ab32abf0f48..cdf7a5caa5c8 100644
> > --- a/drivers/bluetooth/Kconfig
> > +++ b/drivers/bluetooth/Kconfig
> > @@ -56,6 +56,17 @@ config BT_HCIBTUSB_POLL_SYNC
> >           Say Y here to enable USB poll_sync for Bluetooth USB devices by
> >           default.
> >
> > +config BT_HCIBTUSB_AUTO_SET_ISOC_ALT
> > +       bool "Auto set isoc_altsetting for HCI_USER_CHANNEL when SCO connected"
> > +       depends on BT_HCIBTUSB
> > +       default y if CHROME_PLATFORMS
> > +       help
> > +         Say Y here to enable auto set isoc_altsetting for HCI_USER_CHANNEL
> > +         when SCO connected
> > +
> > +         When enabled, btusb intercepts the HCI_EV_SYNC_CONN_COMPLETE packets
> > +         and configures isoc_altsetting automatically for HCI_USER_CHANNEL.
> > +
> >  config BT_HCIBTUSB_BCM
> >         bool "Broadcom protocol support"
> >         depends on BT_HCIBTUSB
> > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> > index de3fa725d210..2642d2ca885f 100644
> > --- a/drivers/bluetooth/btusb.c
> > +++ b/drivers/bluetooth/btusb.c
> > @@ -34,6 +34,8 @@ static bool force_scofix;
> >  static bool enable_autosuspend = IS_ENABLED(CONFIG_BT_HCIBTUSB_AUTOSUSPEND);
> >  static bool enable_poll_sync = IS_ENABLED(CONFIG_BT_HCIBTUSB_POLL_SYNC);
> >  static bool reset = true;
> > +static bool auto_set_isoc_alt =
> > +       IS_ENABLED(CONFIG_BT_HCIBTUSB_AUTO_SET_ISOC_ALT);
> >
> >  static struct usb_driver btusb_driver;
> >
> > @@ -1113,6 +1115,42 @@ static inline void btusb_free_frags(struct btusb_data *data)
> >         spin_unlock_irqrestore(&data->rxlock, flags);
> >  }
> >
> > +static void btusb_sco_connected(struct btusb_data *data, struct sk_buff *skb)
> > +{
> > +       struct hci_event_hdr *hdr = (void *) skb->data;
> > +       struct hci_ev_sync_conn_complete *ev =
> > +               (void *) skb->data + sizeof(*hdr);
> > +       struct hci_dev *hdev = data->hdev;
> > +       unsigned int notify_air_mode;
> > +
> > +       if (hci_skb_pkt_type(skb) != HCI_EVENT_PKT)
> > +               return;
> > +
> > +       if (skb->len < sizeof(*hdr) || hdr->evt != HCI_EV_SYNC_CONN_COMPLETE)
> > +               return;
> > +
> > +       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;
> > +       }
> > +
> > +       bt_dev_info(hdev, "enabling SCO with air mode %u", ev->air_mode);
> > +       data->sco_num = 1;
> > +       data->air_mode = notify_air_mode;
> > +       schedule_work(&data->work);
> > +}
> > +
> >  static int btusb_recv_event(struct btusb_data *data, struct sk_buff *skb)
> >  {
> >         if (data->intr_interval) {
> > @@ -1120,6 +1158,11 @@ static int btusb_recv_event(struct btusb_data *data, struct sk_buff *skb)
> >                 schedule_delayed_work(&data->rx_work, 0);
> >         }
> >
> > +       /* Configure altsetting for HCI_USER_CHANNEL on SCO connected */
> > +       if (auto_set_isoc_alt &&
> > +           hci_dev_test_flag(data->hdev, HCI_USER_CHANNEL))
> > +               btusb_sco_connected(data, skb);
> > +
> >         return data->recv_event(data->hdev, skb);
> >  }
> >
> > --
> > 2.48.1.658.g4767266eb4-goog
>
> This has been merged, I did some minor rewording here and there but
> the logic remains the same, now we can problem revert b16b327edb4d
> ("Bluetooth: btusb: add sysfs attribute to control USB alt setting")?
>
> --
> Luiz Augusto von Dentz

Yes, sure. We could revert b16b327edb4d now.


Best Regards,

Hsin-chen
diff mbox series

Patch

diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
index 4ab32abf0f48..cdf7a5caa5c8 100644
--- a/drivers/bluetooth/Kconfig
+++ b/drivers/bluetooth/Kconfig
@@ -56,6 +56,17 @@  config BT_HCIBTUSB_POLL_SYNC
 	  Say Y here to enable USB poll_sync for Bluetooth USB devices by
 	  default.
 
+config BT_HCIBTUSB_AUTO_SET_ISOC_ALT
+	bool "Auto set isoc_altsetting for HCI_USER_CHANNEL when SCO connected"
+	depends on BT_HCIBTUSB
+	default y if CHROME_PLATFORMS
+	help
+	  Say Y here to enable auto set isoc_altsetting for HCI_USER_CHANNEL
+	  when SCO connected
+
+	  When enabled, btusb intercepts the HCI_EV_SYNC_CONN_COMPLETE packets
+	  and configures isoc_altsetting automatically for HCI_USER_CHANNEL.
+
 config BT_HCIBTUSB_BCM
 	bool "Broadcom protocol support"
 	depends on BT_HCIBTUSB
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index de3fa725d210..2642d2ca885f 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -34,6 +34,8 @@  static bool force_scofix;
 static bool enable_autosuspend = IS_ENABLED(CONFIG_BT_HCIBTUSB_AUTOSUSPEND);
 static bool enable_poll_sync = IS_ENABLED(CONFIG_BT_HCIBTUSB_POLL_SYNC);
 static bool reset = true;
+static bool auto_set_isoc_alt =
+	IS_ENABLED(CONFIG_BT_HCIBTUSB_AUTO_SET_ISOC_ALT);
 
 static struct usb_driver btusb_driver;
 
@@ -1113,6 +1115,42 @@  static inline void btusb_free_frags(struct btusb_data *data)
 	spin_unlock_irqrestore(&data->rxlock, flags);
 }
 
+static void btusb_sco_connected(struct btusb_data *data, struct sk_buff *skb)
+{
+	struct hci_event_hdr *hdr = (void *) skb->data;
+	struct hci_ev_sync_conn_complete *ev =
+		(void *) skb->data + sizeof(*hdr);
+	struct hci_dev *hdev = data->hdev;
+	unsigned int notify_air_mode;
+
+	if (hci_skb_pkt_type(skb) != HCI_EVENT_PKT)
+		return;
+
+	if (skb->len < sizeof(*hdr) || hdr->evt != HCI_EV_SYNC_CONN_COMPLETE)
+		return;
+
+	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;
+	}
+
+	bt_dev_info(hdev, "enabling SCO with air mode %u", ev->air_mode);
+	data->sco_num = 1;
+	data->air_mode = notify_air_mode;
+	schedule_work(&data->work);
+}
+
 static int btusb_recv_event(struct btusb_data *data, struct sk_buff *skb)
 {
 	if (data->intr_interval) {
@@ -1120,6 +1158,11 @@  static int btusb_recv_event(struct btusb_data *data, struct sk_buff *skb)
 		schedule_delayed_work(&data->rx_work, 0);
 	}
 
+	/* Configure altsetting for HCI_USER_CHANNEL on SCO connected */
+	if (auto_set_isoc_alt &&
+	    hci_dev_test_flag(data->hdev, HCI_USER_CHANNEL))
+		btusb_sco_connected(data, skb);
+
 	return data->recv_event(data->hdev, skb);
 }