[v2] Bluetooth: btusb: Configure altsetting for USER_CHANNEL

Commit Message

Hsin-chen Chuang Feb. 24, 2025, 4:52 a.m. UTC

From: Hsin-chen Chuang <[e-mail]>

Automatically configure the altsetting for USER_CHANNEL when a SCO is
connected. 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 <[e-mail]>
---

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 | 46 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 57 insertions(+)

Comments

[e-mail] Feb. 24, 2025, 5:32 a.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=936884

---Test result---

Test Summary:
CheckPatch                    PENDING   0.31 seconds
GitLint                       PENDING   0.19 seconds
SubjectPrefix                 PASS      0.10 seconds
BuildKernel                   PASS      23.90 seconds
CheckAllWarning               PASS      26.56 seconds
CheckSparse                   PASS      31.56 seconds
BuildKernel32                 PASS      23.64 seconds
TestRunnerSetup               PASS      426.38 seconds
TestRunner_l2cap-tester       PASS      20.56 seconds
TestRunner_iso-tester         PASS      37.10 seconds
TestRunner_bnep-tester        PASS      4.74 seconds
TestRunner_mgmt-tester        FAIL      119.03 seconds
TestRunner_rfcomm-tester      PASS      7.90 seconds
TestRunner_sco-tester         PASS      9.50 seconds
TestRunner_ioctl-tester       PASS      8.51 seconds
TestRunner_mesh-tester        PASS      5.80 seconds
TestRunner_smp-tester         PASS      7.12 seconds
TestRunner_userchan-tester    PASS      4.91 seconds
IncrementalBuild              PENDING   0.37 seconds

Details
##############################
Test: CheckPatch - PENDING
Desc: Run checkpatch.pl script
Output:

##############################
Test: GitLint - PENDING
Desc: Run gitlint
Output:

##############################
Test: TestRunner_mgmt-tester - FAIL
Desc: Run mgmt-tester with test-runner
Output:
Total: 490, Passed: 484 (98.8%), Failed: 2, Not Run: 4

Failed Test Cases
LL Privacy - Add Device 3 (AL is full)               Failed       0.200 seconds
LL Privacy - Set Flags 2 (Enable RL)                 Failed       0.146 seconds
##############################
Test: IncrementalBuild - PENDING
Desc: Incremental build with the patches in the series
Output:



---
Regards,
Linux Bluetooth

Greg KH Feb. 24, 2025, 6:10 a.m. UTC | #2

On Mon, Feb 24, 2025 at 12:52:32PM +0800, Hsin-chen Chuang wrote:
> From: Hsin-chen Chuang <[e-mail]>
> 
> Automatically configure the altsetting for USER_CHANNEL when a SCO is
> connected. 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 <[e-mail]>
> ---
> 
> 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 | 46 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 57 insertions(+)
> 
> diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
> index 4ab32abf0f48..7c497f878732 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 USER_CHANNEL when SCO connected"
> +	depends on BT_HCIBTUSB
> +	default n
> +	help
> +	  Say Y here to enable auto set isoc_altsetting for USER_CHANNEL
> +	  when SCO connected
> +
> +	  This can be overridden by passing btusb.auto_set_isoc_alt=[y|n]
> +	  on the kernel commandline.
> +
>  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..af93d757911b 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);
>  }
>  
> @@ -4354,6 +4397,9 @@ MODULE_PARM_DESC(enable_autosuspend, "Enable USB autosuspend by default");
>  module_param(reset, bool, 0644);
>  MODULE_PARM_DESC(reset, "Send HCI reset command on initialization");
>  
> +module_param(auto_set_isoc_alt, bool, 0644);
> +MODULE_PARM_DESC(auto_set_isoc_alt, "Auto set isoc_altsetting for USER_CHANNEL when SCO connected");

This is not the 1990's, why are you adding new module parameters when we
have so many other more proper ways to do this?  And really, this would
not work at all for multiple controllers in teh same system, right?
That should cause it to not even be considered at all as a viable
solution.

confused,

greg k-h

Hsin-chen Chuang Feb. 24, 2025, 6:25 a.m. UTC | #3

Hi Greg,

On Mon, Feb 24, 2025 at 2:10 PM Greg KH <[e-mail]> wrote:
>
> On Mon, Feb 24, 2025 at 12:52:32PM +0800, Hsin-chen Chuang wrote:
> > From: Hsin-chen Chuang <[e-mail]>
> >
> > Automatically configure the altsetting for USER_CHANNEL when a SCO is
> > connected. 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 <[e-mail]>
> > ---
> >
> > 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 | 46 +++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 57 insertions(+)
> >
> > diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
> > index 4ab32abf0f48..7c497f878732 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 USER_CHANNEL when SCO connected"
> > +     depends on BT_HCIBTUSB
> > +     default n
> > +     help
> > +       Say Y here to enable auto set isoc_altsetting for USER_CHANNEL
> > +       when SCO connected
> > +
> > +       This can be overridden by passing btusb.auto_set_isoc_alt=[y|n]
> > +       on the kernel commandline.
> > +
> >  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..af93d757911b 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);
> >  }
> >
> > @@ -4354,6 +4397,9 @@ MODULE_PARM_DESC(enable_autosuspend, "Enable USB autosuspend by default");
> >  module_param(reset, bool, 0644);
> >  MODULE_PARM_DESC(reset, "Send HCI reset command on initialization");
> >
> > +module_param(auto_set_isoc_alt, bool, 0644);
> > +MODULE_PARM_DESC(auto_set_isoc_alt, "Auto set isoc_altsetting for USER_CHANNEL when SCO connected");
>
> This is not the 1990's, why are you adding new module parameters when we
> have so many other more proper ways to do this?  And really, this would

Sorry but could you please provide an example to guard a feature like this.

> not work at all for multiple controllers in teh same system, right?

Do you mean we can't have separate parameters for different
controllers? Yes that's true, but why would a user want the different
behavior on the same machine?

> That should cause it to not even be considered at all as a viable
> solution.
>
> confused,
>
> greg k-h

Greg KH Feb. 24, 2025, 6:44 a.m. UTC | #4

On Mon, Feb 24, 2025 at 02:25:52PM +0800, Hsin-chen Chuang wrote:
> Hi Greg,
> 
> On Mon, Feb 24, 2025 at 2:10 PM Greg KH <[e-mail]> wrote:
> >
> > On Mon, Feb 24, 2025 at 12:52:32PM +0800, Hsin-chen Chuang wrote:
> > > From: Hsin-chen Chuang <[e-mail]>
> > >
> > > Automatically configure the altsetting for USER_CHANNEL when a SCO is
> > > connected. 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 <[e-mail]>
> > > ---
> > >
> > > 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 | 46 +++++++++++++++++++++++++++++++++++++++
> > >  2 files changed, 57 insertions(+)
> > >
> > > diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
> > > index 4ab32abf0f48..7c497f878732 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 USER_CHANNEL when SCO connected"
> > > +     depends on BT_HCIBTUSB
> > > +     default n
> > > +     help
> > > +       Say Y here to enable auto set isoc_altsetting for USER_CHANNEL
> > > +       when SCO connected
> > > +
> > > +       This can be overridden by passing btusb.auto_set_isoc_alt=[y|n]
> > > +       on the kernel commandline.
> > > +
> > >  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..af93d757911b 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);
> > >  }
> > >
> > > @@ -4354,6 +4397,9 @@ MODULE_PARM_DESC(enable_autosuspend, "Enable USB autosuspend by default");
> > >  module_param(reset, bool, 0644);
> > >  MODULE_PARM_DESC(reset, "Send HCI reset command on initialization");
> > >
> > > +module_param(auto_set_isoc_alt, bool, 0644);
> > > +MODULE_PARM_DESC(auto_set_isoc_alt, "Auto set isoc_altsetting for USER_CHANNEL when SCO connected");
> >
> > This is not the 1990's, why are you adding new module parameters when we
> > have so many other more proper ways to do this?  And really, this would
> 
> Sorry but could you please provide an example to guard a feature like this.

Depends on what you want to do with this configuration.  Why is it an
option at all?  Why can't it "just work"?  Module parameters are a pain
to configure, we have loads of other ways to do this now (configfs,
debugfs for debugging stuff, sysfs for device-specific things, etc.)

> > not work at all for multiple controllers in teh same system, right?
> 
> Do you mean we can't have separate parameters for different
> controllers? Yes that's true, but why would a user want the different
> behavior on the same machine?

Why would you prevent them from allowing this to happen for a
device-specific option?

thanks,

greg k-h

Hsin-chen Chuang Feb. 24, 2025, 7:13 a.m. UTC | #5

On Mon, Feb 24, 2025 at 2:44 PM Greg KH <[e-mail]> wrote:
>
> On Mon, Feb 24, 2025 at 02:25:52PM +0800, Hsin-chen Chuang wrote:
> > Hi Greg,
> >
> > On Mon, Feb 24, 2025 at 2:10 PM Greg KH <[e-mail]> wrote:
> > >
> > > On Mon, Feb 24, 2025 at 12:52:32PM +0800, Hsin-chen Chuang wrote:
> > > > From: Hsin-chen Chuang <[e-mail]>
> > > >
> > > > Automatically configure the altsetting for USER_CHANNEL when a SCO is
> > > > connected. 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 <[e-mail]>
> > > > ---
> > > >
> > > > 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 | 46 +++++++++++++++++++++++++++++++++++++++
> > > >  2 files changed, 57 insertions(+)
> > > >
> > > > diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
> > > > index 4ab32abf0f48..7c497f878732 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 USER_CHANNEL when SCO connected"
> > > > +     depends on BT_HCIBTUSB
> > > > +     default n
> > > > +     help
> > > > +       Say Y here to enable auto set isoc_altsetting for USER_CHANNEL
> > > > +       when SCO connected
> > > > +
> > > > +       This can be overridden by passing btusb.auto_set_isoc_alt=[y|n]
> > > > +       on the kernel commandline.
> > > > +
> > > >  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..af93d757911b 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);
> > > >  }
> > > >
> > > > @@ -4354,6 +4397,9 @@ MODULE_PARM_DESC(enable_autosuspend, "Enable USB autosuspend by default");
> > > >  module_param(reset, bool, 0644);
> > > >  MODULE_PARM_DESC(reset, "Send HCI reset command on initialization");
> > > >
> > > > +module_param(auto_set_isoc_alt, bool, 0644);
> > > > +MODULE_PARM_DESC(auto_set_isoc_alt, "Auto set isoc_altsetting for USER_CHANNEL when SCO connected");
> > >
> > > This is not the 1990's, why are you adding new module parameters when we
> > > have so many other more proper ways to do this?  And really, this would
> >
> > Sorry but could you please provide an example to guard a feature like this.
>
> Depends on what you want to do with this configuration.  Why is it an
> option at all?  Why can't it "just work"?  Module parameters are a pain

I would like to hand this question to Luiz. I believe this patch just
works because this configuration is defined in the spec.
I think Luiz's point is to project the potential existing user, but
there's probably no User channel user sending SCO data with the latest
btusb driver because:
a) There's no way to configure alt setting from userspace
b) Before eafcfcfca97d, SCO data would be rejected since User channel
shouldn't be able to modify hci_conn_num

> to configure, we have loads of other ways to do this now (configfs,
> debugfs for debugging stuff, sysfs for device-specific things, etc.)
>
> > > not work at all for multiple controllers in teh same system, right?
> >
> > Do you mean we can't have separate parameters for different
> > controllers? Yes that's true, but why would a user want the different
> > behavior on the same machine?
>
> Why would you prevent them from allowing this to happen for a
> device-specific option?
>
> thanks,
>
> greg k-h

Luiz Augusto von Dentz Feb. 26, 2025, 8:54 p.m. UTC | #6

Hi Hsin-chen,

On Mon, Feb 24, 2025 at 2:13 AM Hsin-chen Chuang <[e-mail]> wrote:
>
> On Mon, Feb 24, 2025 at 2:44 PM Greg KH <[e-mail]> wrote:
> >
> > On Mon, Feb 24, 2025 at 02:25:52PM +0800, Hsin-chen Chuang wrote:
> > > Hi Greg,
> > >
> > > On Mon, Feb 24, 2025 at 2:10 PM Greg KH <[e-mail]> wrote:
> > > >
> > > > On Mon, Feb 24, 2025 at 12:52:32PM +0800, Hsin-chen Chuang wrote:
> > > > > From: Hsin-chen Chuang <[e-mail]>
> > > > >
> > > > > Automatically configure the altsetting for USER_CHANNEL when a SCO is
> > > > > connected. 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 <[e-mail]>
> > > > > ---
> > > > >
> > > > > 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 | 46 +++++++++++++++++++++++++++++++++++++++
> > > > >  2 files changed, 57 insertions(+)
> > > > >
> > > > > diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
> > > > > index 4ab32abf0f48..7c497f878732 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 USER_CHANNEL when SCO connected"
> > > > > +     depends on BT_HCIBTUSB
> > > > > +     default n

Maybe we can do just:

 default y if CHROME_PLATFORMS

> > > > > +     help
> > > > > +       Say Y here to enable auto set isoc_altsetting for USER_CHANNEL
> > > > > +       when SCO connected
> > > > > +
> > > > > +       This can be overridden by passing btusb.auto_set_isoc_alt=[y|n]
> > > > > +       on the kernel commandline.
> > > > > +
> > > > >  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..af93d757911b 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);
> > > > >  }
> > > > >
> > > > > @@ -4354,6 +4397,9 @@ MODULE_PARM_DESC(enable_autosuspend, "Enable USB autosuspend by default");
> > > > >  module_param(reset, bool, 0644);
> > > > >  MODULE_PARM_DESC(reset, "Send HCI reset command on initialization");
> > > > >
> > > > > +module_param(auto_set_isoc_alt, bool, 0644);
> > > > > +MODULE_PARM_DESC(auto_set_isoc_alt, "Auto set isoc_altsetting for USER_CHANNEL when SCO connected");
> > > >
> > > > This is not the 1990's, why are you adding new module parameters when we
> > > > have so many other more proper ways to do this?  And really, this would
> > >
> > > Sorry but could you please provide an example to guard a feature like this.
> >
> > Depends on what you want to do with this configuration.  Why is it an
> > option at all?  Why can't it "just work"?  Module parameters are a pain
>
> I would like to hand this question to Luiz. I believe this patch just
> works because this configuration is defined in the spec.
> I think Luiz's point is to project the potential existing user, but
> there's probably no User channel user sending SCO data with the latest
> btusb driver because:
> a) There's no way to configure alt setting from userspace
> b) Before eafcfcfca97d, SCO data would be rejected since User channel
> shouldn't be able to modify hci_conn_num

Perhaps you can just use CHROME_PLATFORMS (suggested above) in Kconfig
to enable intercepting of the events, etc, so we don't need any
runtime parameters.

> > to configure, we have loads of other ways to do this now (configfs,
> > debugfs for debugging stuff, sysfs for device-specific things, etc.)
> >
> > > > not work at all for multiple controllers in teh same system, right?
> > >
> > > Do you mean we can't have separate parameters for different
> > > controllers? Yes that's true, but why would a user want the different
> > > behavior on the same machine?
> >
> > Why would you prevent them from allowing this to happen for a
> > device-specific option?
> >
> > thanks,
> >
> > greg k-h
>
> --
> Best Regards,
> Hsin-chen

Hsin-chen Chuang Feb. 27, 2025, 2:22 a.m. UTC | #7

Hi Luiz,

On Thu, Feb 27, 2025 at 4:55 AM Luiz Augusto von Dentz
<[e-mail]> wrote:
>
> Hi Hsin-chen,
>
> On Mon, Feb 24, 2025 at 2:13 AM Hsin-chen Chuang <[e-mail]> wrote:
> >
> > On Mon, Feb 24, 2025 at 2:44 PM Greg KH <[e-mail]> wrote:
> > >
> > > On Mon, Feb 24, 2025 at 02:25:52PM +0800, Hsin-chen Chuang wrote:
> > > > Hi Greg,
> > > >
> > > > On Mon, Feb 24, 2025 at 2:10 PM Greg KH <[e-mail]> wrote:
> > > > >
> > > > > On Mon, Feb 24, 2025 at 12:52:32PM +0800, Hsin-chen Chuang wrote:
> > > > > > From: Hsin-chen Chuang <[e-mail]>
> > > > > >
> > > > > > Automatically configure the altsetting for USER_CHANNEL when a SCO is
> > > > > > connected. 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 <[e-mail]>
> > > > > > ---
> > > > > >
> > > > > > 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 | 46 +++++++++++++++++++++++++++++++++++++++
> > > > > >  2 files changed, 57 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
> > > > > > index 4ab32abf0f48..7c497f878732 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 USER_CHANNEL when SCO connected"
> > > > > > +     depends on BT_HCIBTUSB
> > > > > > +     default n
>
> Maybe we can do just:
>
>  default y if CHROME_PLATFORMS
>
> > > > > > +     help
> > > > > > +       Say Y here to enable auto set isoc_altsetting for USER_CHANNEL
> > > > > > +       when SCO connected
> > > > > > +
> > > > > > +       This can be overridden by passing btusb.auto_set_isoc_alt=[y|n]
> > > > > > +       on the kernel commandline.
> > > > > > +
> > > > > >  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..af93d757911b 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);
> > > > > >  }
> > > > > >
> > > > > > @@ -4354,6 +4397,9 @@ MODULE_PARM_DESC(enable_autosuspend, "Enable USB autosuspend by default");
> > > > > >  module_param(reset, bool, 0644);
> > > > > >  MODULE_PARM_DESC(reset, "Send HCI reset command on initialization");
> > > > > >
> > > > > > +module_param(auto_set_isoc_alt, bool, 0644);
> > > > > > +MODULE_PARM_DESC(auto_set_isoc_alt, "Auto set isoc_altsetting for USER_CHANNEL when SCO connected");
> > > > >
> > > > > This is not the 1990's, why are you adding new module parameters when we
> > > > > have so many other more proper ways to do this?  And really, this would
> > > >
> > > > Sorry but could you please provide an example to guard a feature like this.
> > >
> > > Depends on what you want to do with this configuration.  Why is it an
> > > option at all?  Why can't it "just work"?  Module parameters are a pain
> >
> > I would like to hand this question to Luiz. I believe this patch just
> > works because this configuration is defined in the spec.
> > I think Luiz's point is to project the potential existing user, but
> > there's probably no User channel user sending SCO data with the latest
> > btusb driver because:
> > a) There's no way to configure alt setting from userspace
> > b) Before eafcfcfca97d, SCO data would be rejected since User channel
> > shouldn't be able to modify hci_conn_num
>
> Perhaps you can just use CHROME_PLATFORMS (suggested above) in Kconfig
> to enable intercepting of the events, etc, so we don't need any
> runtime parameters.

I'm afraid that this doesn't resolve Greg's comment below because the
multiple controllers are still bonded to the same config.
Also I would hesitate to put this Chrome devices specific because
project Floss shall be able to run on a general Linux environment.
If you have a strong opinion to guard this behind a flag, perhaps we
could try other options suggested by Greg (configfs or maybe back to
sysfs).

>
> > > to configure, we have loads of other ways to do this now (configfs,
> > > debugfs for debugging stuff, sysfs for device-specific things, etc.)
> > >
> > > > > not work at all for multiple controllers in teh same system, right?
> > > >
> > > > Do you mean we can't have separate parameters for different
> > > > controllers? Yes that's true, but why would a user want the different
> > > > behavior on the same machine?
> > >
> > > Why would you prevent them from allowing this to happen for a
> > > device-specific option?
> > >
> > > thanks,
> > >
> > > greg k-h
> >
> > --
> > Best Regards,
> > Hsin-chen
>
>
>
> --
> Luiz Augusto von Dentz


Best Regards,

Hsin-chen

Luiz Augusto von Dentz Feb. 27, 2025, 2:37 p.m. UTC | #8

Hi Hsin-chen,

On Wed, Feb 26, 2025 at 9:22 PM Hsin-chen Chuang <[e-mail]> wrote:
>
> Hi Luiz,
>
> On Thu, Feb 27, 2025 at 4:55 AM Luiz Augusto von Dentz
> <[e-mail]> wrote:
> >
> > Hi Hsin-chen,
> >
> > On Mon, Feb 24, 2025 at 2:13 AM Hsin-chen Chuang <[e-mail]> wrote:
> > >
> > > On Mon, Feb 24, 2025 at 2:44 PM Greg KH <[e-mail]> wrote:
> > > >
> > > > On Mon, Feb 24, 2025 at 02:25:52PM +0800, Hsin-chen Chuang wrote:
> > > > > Hi Greg,
> > > > >
> > > > > On Mon, Feb 24, 2025 at 2:10 PM Greg KH <[e-mail]> wrote:
> > > > > >
> > > > > > On Mon, Feb 24, 2025 at 12:52:32PM +0800, Hsin-chen Chuang wrote:
> > > > > > > From: Hsin-chen Chuang <[e-mail]>
> > > > > > >
> > > > > > > Automatically configure the altsetting for USER_CHANNEL when a SCO is
> > > > > > > connected. 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 <[e-mail]>
> > > > > > > ---
> > > > > > >
> > > > > > > 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 | 46 +++++++++++++++++++++++++++++++++++++++
> > > > > > >  2 files changed, 57 insertions(+)
> > > > > > >
> > > > > > > diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
> > > > > > > index 4ab32abf0f48..7c497f878732 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 USER_CHANNEL when SCO connected"
> > > > > > > +     depends on BT_HCIBTUSB
> > > > > > > +     default n
> >
> > Maybe we can do just:
> >
> >  default y if CHROME_PLATFORMS
> >
> > > > > > > +     help
> > > > > > > +       Say Y here to enable auto set isoc_altsetting for USER_CHANNEL
> > > > > > > +       when SCO connected
> > > > > > > +
> > > > > > > +       This can be overridden by passing btusb.auto_set_isoc_alt=[y|n]
> > > > > > > +       on the kernel commandline.
> > > > > > > +
> > > > > > >  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..af93d757911b 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);
> > > > > > >  }
> > > > > > >
> > > > > > > @@ -4354,6 +4397,9 @@ MODULE_PARM_DESC(enable_autosuspend, "Enable USB autosuspend by default");
> > > > > > >  module_param(reset, bool, 0644);
> > > > > > >  MODULE_PARM_DESC(reset, "Send HCI reset command on initialization");
> > > > > > >
> > > > > > > +module_param(auto_set_isoc_alt, bool, 0644);
> > > > > > > +MODULE_PARM_DESC(auto_set_isoc_alt, "Auto set isoc_altsetting for USER_CHANNEL when SCO connected");
> > > > > >
> > > > > > This is not the 1990's, why are you adding new module parameters when we
> > > > > > have so many other more proper ways to do this?  And really, this would
> > > > >
> > > > > Sorry but could you please provide an example to guard a feature like this.
> > > >
> > > > Depends on what you want to do with this configuration.  Why is it an
> > > > option at all?  Why can't it "just work"?  Module parameters are a pain
> > >
> > > I would like to hand this question to Luiz. I believe this patch just
> > > works because this configuration is defined in the spec.
> > > I think Luiz's point is to project the potential existing user, but
> > > there's probably no User channel user sending SCO data with the latest
> > > btusb driver because:
> > > a) There's no way to configure alt setting from userspace
> > > b) Before eafcfcfca97d, SCO data would be rejected since User channel
> > > shouldn't be able to modify hci_conn_num
> >
> > Perhaps you can just use CHROME_PLATFORMS (suggested above) in Kconfig
> > to enable intercepting of the events, etc, so we don't need any
> > runtime parameters.
>
> I'm afraid that this doesn't resolve Greg's comment below because the
> multiple controllers are still bonded to the same config.

Well that would be enabled for every controller plugged into the system.

> Also I would hesitate to put this Chrome devices specific because
> project Floss shall be able to run on a general Linux environment.

It can be enabled for Linux in general, it just not enabled by default
which can be changed later if someone decides to use Floss outside of
Chrome OS, that said I very much doubt Google would be supporting
anything other then Chrome based platforms or Floss don't depend on
Android vendor HCI commands? At least the Android documentation seems
to suggest a bunch of vendor commands are required:

https://source.android.com/docs/core/connect/bluetooth/hci_requirements

> If you have a strong opinion to guard this behind a flag, perhaps we
> could try other options suggested by Greg (configfs or maybe back to
> sysfs).

I think those would be more complicated then handling this via
Kconfig, besides I think this works best since there is no new API
introduced, just a new Kconfig option which can be enabled in any
platform/distro.

> >
> > > > to configure, we have loads of other ways to do this now (configfs,
> > > > debugfs for debugging stuff, sysfs for device-specific things, etc.)
> > > >
> > > > > > not work at all for multiple controllers in teh same system, right?
> > > > >
> > > > > Do you mean we can't have separate parameters for different
> > > > > controllers? Yes that's true, but why would a user want the different
> > > > > behavior on the same machine?
> > > >
> > > > Why would you prevent them from allowing this to happen for a
> > > > device-specific option?
> > > >
> > > > thanks,
> > > >
> > > > greg k-h
> > >
> > > --
> > > Best Regards,
> > > Hsin-chen
> >
> >
> >
> > --
> > Luiz Augusto von Dentz
>
>
> Best Regards,
>
> Hsin-chen

Hsin-chen Chuang Feb. 27, 2025, 3:36 p.m. UTC | #9

Hi Luiz,

On Thu, Feb 27, 2025 at 10:37 PM Luiz Augusto von Dentz
<[e-mail]> wrote:
>
> Hi Hsin-chen,
>
> On Wed, Feb 26, 2025 at 9:22 PM Hsin-chen Chuang <[e-mail]> wrote:
> >
> > Hi Luiz,
> >
> > On Thu, Feb 27, 2025 at 4:55 AM Luiz Augusto von Dentz
> > <[e-mail]> wrote:
> > >
> > > Hi Hsin-chen,
> > >
> > > On Mon, Feb 24, 2025 at 2:13 AM Hsin-chen Chuang <[e-mail]> wrote:
> > > >
> > > > On Mon, Feb 24, 2025 at 2:44 PM Greg KH <[e-mail]> wrote:
> > > > >
> > > > > On Mon, Feb 24, 2025 at 02:25:52PM +0800, Hsin-chen Chuang wrote:
> > > > > > Hi Greg,
> > > > > >
> > > > > > On Mon, Feb 24, 2025 at 2:10 PM Greg KH <[e-mail]> wrote:
> > > > > > >
> > > > > > > On Mon, Feb 24, 2025 at 12:52:32PM +0800, Hsin-chen Chuang wrote:
> > > > > > > > From: Hsin-chen Chuang <[e-mail]>
> > > > > > > >
> > > > > > > > Automatically configure the altsetting for USER_CHANNEL when a SCO is
> > > > > > > > connected. 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 <[e-mail]>
> > > > > > > > ---
> > > > > > > >
> > > > > > > > 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 | 46 +++++++++++++++++++++++++++++++++++++++
> > > > > > > >  2 files changed, 57 insertions(+)
> > > > > > > >
> > > > > > > > diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
> > > > > > > > index 4ab32abf0f48..7c497f878732 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 USER_CHANNEL when SCO connected"
> > > > > > > > +     depends on BT_HCIBTUSB
> > > > > > > > +     default n
> > >
> > > Maybe we can do just:
> > >
> > >  default y if CHROME_PLATFORMS
> > >
> > > > > > > > +     help
> > > > > > > > +       Say Y here to enable auto set isoc_altsetting for USER_CHANNEL
> > > > > > > > +       when SCO connected
> > > > > > > > +
> > > > > > > > +       This can be overridden by passing btusb.auto_set_isoc_alt=[y|n]
> > > > > > > > +       on the kernel commandline.
> > > > > > > > +
> > > > > > > >  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..af93d757911b 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);
> > > > > > > >  }
> > > > > > > >
> > > > > > > > @@ -4354,6 +4397,9 @@ MODULE_PARM_DESC(enable_autosuspend, "Enable USB autosuspend by default");
> > > > > > > >  module_param(reset, bool, 0644);
> > > > > > > >  MODULE_PARM_DESC(reset, "Send HCI reset command on initialization");
> > > > > > > >
> > > > > > > > +module_param(auto_set_isoc_alt, bool, 0644);
> > > > > > > > +MODULE_PARM_DESC(auto_set_isoc_alt, "Auto set isoc_altsetting for USER_CHANNEL when SCO connected");
> > > > > > >
> > > > > > > This is not the 1990's, why are you adding new module parameters when we
> > > > > > > have so many other more proper ways to do this?  And really, this would
> > > > > >
> > > > > > Sorry but could you please provide an example to guard a feature like this.
> > > > >
> > > > > Depends on what you want to do with this configuration.  Why is it an
> > > > > option at all?  Why can't it "just work"?  Module parameters are a pain
> > > >
> > > > I would like to hand this question to Luiz. I believe this patch just
> > > > works because this configuration is defined in the spec.
> > > > I think Luiz's point is to project the potential existing user, but
> > > > there's probably no User channel user sending SCO data with the latest
> > > > btusb driver because:
> > > > a) There's no way to configure alt setting from userspace
> > > > b) Before eafcfcfca97d, SCO data would be rejected since User channel
> > > > shouldn't be able to modify hci_conn_num
> > >
> > > Perhaps you can just use CHROME_PLATFORMS (suggested above) in Kconfig
> > > to enable intercepting of the events, etc, so we don't need any
> > > runtime parameters.
> >
> > I'm afraid that this doesn't resolve Greg's comment below because the
> > multiple controllers are still bonded to the same config.
>
> Well that would be enabled for every controller plugged into the system.

To clarify, I'm totally fine with runtime parameters and/or any
Kconfig like CHROME_PLATFORMS, although I'd prefer the current patch
more.

But I guess Greg's point is that the multiple controllers should be
able to have different configurations at the same time. I'd respect
your decision to accept this patch or another patch with
CHROME_PLATFORMS, but I guess it's better to convince Greg first.

>
> > Also I would hesitate to put this Chrome devices specific because
> > project Floss shall be able to run on a general Linux environment.
>
> It can be enabled for Linux in general, it just not enabled by default
> which can be changed later if someone decides to use Floss outside of
> Chrome OS, that said I very much doubt Google would be supporting
> anything other then Chrome based platforms or Floss don't depend on
> Android vendor HCI commands? At least the Android documentation seems

Floss doesn't depend on Android extensions - Our hardwares doesn't
support that either :p

> to suggest a bunch of vendor commands are required:
>
> https://source.android.com/docs/core/connect/bluetooth/hci_requirements
>
> > If you have a strong opinion to guard this behind a flag, perhaps we
> > could try other options suggested by Greg (configfs or maybe back to
> > sysfs).
>
> I think those would be more complicated then handling this via
> Kconfig, besides I think this works best since there is no new API
> introduced, just a new Kconfig option which can be enabled in any
> platform/distro.
>
> > >
> > > > > to configure, we have loads of other ways to do this now (configfs,
> > > > > debugfs for debugging stuff, sysfs for device-specific things, etc.)
> > > > >
> > > > > > > not work at all for multiple controllers in teh same system, right?
> > > > > >
> > > > > > Do you mean we can't have separate parameters for different
> > > > > > controllers? Yes that's true, but why would a user want the different
> > > > > > behavior on the same machine?
> > > > >
> > > > > Why would you prevent them from allowing this to happen for a
> > > > > device-specific option?
> > > > >
> > > > > thanks,
> > > > >
> > > > > greg k-h
> > > >
> > > > --
> > > > Best Regards,
> > > > Hsin-chen
> > >
> > >
> > >
> > > --
> > > Luiz Augusto von Dentz
> >
> >
> > Best Regards,
> >
> > Hsin-chen
>
>
>
> --
> Luiz Augusto von Dentz


Best Regards,

Hsin-chen

Luiz Augusto von Dentz Feb. 27, 2025, 3:58 p.m. UTC | #10

Hi Hsin-chen,

On Thu, Feb 27, 2025 at 10:37 AM Hsin-chen Chuang <[e-mail]> wrote:
>
> Hi Luiz,
>
> On Thu, Feb 27, 2025 at 10:37 PM Luiz Augusto von Dentz
> <[e-mail]> wrote:
> >
> > Hi Hsin-chen,
> >
> > On Wed, Feb 26, 2025 at 9:22 PM Hsin-chen Chuang <[e-mail]> wrote:
> > >
> > > Hi Luiz,
> > >
> > > On Thu, Feb 27, 2025 at 4:55 AM Luiz Augusto von Dentz
> > > <[e-mail]> wrote:
> > > >
> > > > Hi Hsin-chen,
> > > >
> > > > On Mon, Feb 24, 2025 at 2:13 AM Hsin-chen Chuang <[e-mail]> wrote:
> > > > >
> > > > > On Mon, Feb 24, 2025 at 2:44 PM Greg KH <[e-mail]> wrote:
> > > > > >
> > > > > > On Mon, Feb 24, 2025 at 02:25:52PM +0800, Hsin-chen Chuang wrote:
> > > > > > > Hi Greg,
> > > > > > >
> > > > > > > On Mon, Feb 24, 2025 at 2:10 PM Greg KH <[e-mail]> wrote:
> > > > > > > >
> > > > > > > > On Mon, Feb 24, 2025 at 12:52:32PM +0800, Hsin-chen Chuang wrote:
> > > > > > > > > From: Hsin-chen Chuang <[e-mail]>
> > > > > > > > >
> > > > > > > > > Automatically configure the altsetting for USER_CHANNEL when a SCO is
> > > > > > > > > connected. 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 <[e-mail]>
> > > > > > > > > ---
> > > > > > > > >
> > > > > > > > > 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 | 46 +++++++++++++++++++++++++++++++++++++++
> > > > > > > > >  2 files changed, 57 insertions(+)
> > > > > > > > >
> > > > > > > > > diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
> > > > > > > > > index 4ab32abf0f48..7c497f878732 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 USER_CHANNEL when SCO connected"
> > > > > > > > > +     depends on BT_HCIBTUSB
> > > > > > > > > +     default n
> > > >
> > > > Maybe we can do just:
> > > >
> > > >  default y if CHROME_PLATFORMS
> > > >
> > > > > > > > > +     help
> > > > > > > > > +       Say Y here to enable auto set isoc_altsetting for USER_CHANNEL
> > > > > > > > > +       when SCO connected
> > > > > > > > > +
> > > > > > > > > +       This can be overridden by passing btusb.auto_set_isoc_alt=[y|n]
> > > > > > > > > +       on the kernel commandline.
> > > > > > > > > +
> > > > > > > > >  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..af93d757911b 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);
> > > > > > > > >  }
> > > > > > > > >
> > > > > > > > > @@ -4354,6 +4397,9 @@ MODULE_PARM_DESC(enable_autosuspend, "Enable USB autosuspend by default");
> > > > > > > > >  module_param(reset, bool, 0644);
> > > > > > > > >  MODULE_PARM_DESC(reset, "Send HCI reset command on initialization");
> > > > > > > > >
> > > > > > > > > +module_param(auto_set_isoc_alt, bool, 0644);
> > > > > > > > > +MODULE_PARM_DESC(auto_set_isoc_alt, "Auto set isoc_altsetting for USER_CHANNEL when SCO connected");
> > > > > > > >
> > > > > > > > This is not the 1990's, why are you adding new module parameters when we
> > > > > > > > have so many other more proper ways to do this?  And really, this would
> > > > > > >
> > > > > > > Sorry but could you please provide an example to guard a feature like this.
> > > > > >
> > > > > > Depends on what you want to do with this configuration.  Why is it an
> > > > > > option at all?  Why can't it "just work"?  Module parameters are a pain
> > > > >
> > > > > I would like to hand this question to Luiz. I believe this patch just
> > > > > works because this configuration is defined in the spec.
> > > > > I think Luiz's point is to project the potential existing user, but
> > > > > there's probably no User channel user sending SCO data with the latest
> > > > > btusb driver because:
> > > > > a) There's no way to configure alt setting from userspace
> > > > > b) Before eafcfcfca97d, SCO data would be rejected since User channel
> > > > > shouldn't be able to modify hci_conn_num
> > > >
> > > > Perhaps you can just use CHROME_PLATFORMS (suggested above) in Kconfig
> > > > to enable intercepting of the events, etc, so we don't need any
> > > > runtime parameters.
> > >
> > > I'm afraid that this doesn't resolve Greg's comment below because the
> > > multiple controllers are still bonded to the same config.
> >
> > Well that would be enabled for every controller plugged into the system.
>
> To clarify, I'm totally fine with runtime parameters and/or any
> Kconfig like CHROME_PLATFORMS, although I'd prefer the current patch
> more.
>
> But I guess Greg's point is that the multiple controllers should be
> able to have different configurations at the same time. I'd respect
> your decision to accept this patch or another patch with
> CHROME_PLATFORMS, but I guess it's better to convince Greg first.

Perhaps I got Greg's response wrong, but I think he was suggesting
that it should just work without any special configuration, which is
what I'm suggesting here, the use of CHROME_PLATFORMS is more of a
stopgap until proper test automation is introduced.

You can already have different configurations with respect to
HCI_CHANNEL_USER since that is done per controller, btw that is used
by the likes of btproxy tool for example which is normally used to
hook the controller into a VM.

Hsin-chen Chuang Feb. 27, 2025, 4:44 p.m. UTC | #11

Hi Luiz,

On Thu, Feb 27, 2025 at 11:59 PM Luiz Augusto von Dentz
<[e-mail]> wrote:
>
> Hi Hsin-chen,
>
> On Thu, Feb 27, 2025 at 10:37 AM Hsin-chen Chuang <[e-mail]> wrote:
> >
> > Hi Luiz,
> >
> > On Thu, Feb 27, 2025 at 10:37 PM Luiz Augusto von Dentz
> > <[e-mail]> wrote:
> > >
> > > Hi Hsin-chen,
> > >
> > > On Wed, Feb 26, 2025 at 9:22 PM Hsin-chen Chuang <[e-mail]> wrote:
> > > >
> > > > Hi Luiz,
> > > >
> > > > On Thu, Feb 27, 2025 at 4:55 AM Luiz Augusto von Dentz
> > > > <[e-mail]> wrote:
> > > > >
> > > > > Hi Hsin-chen,
> > > > >
> > > > > On Mon, Feb 24, 2025 at 2:13 AM Hsin-chen Chuang <[e-mail]> wrote:
> > > > > >
> > > > > > On Mon, Feb 24, 2025 at 2:44 PM Greg KH <[e-mail]> wrote:
> > > > > > >
> > > > > > > On Mon, Feb 24, 2025 at 02:25:52PM +0800, Hsin-chen Chuang wrote:
> > > > > > > > Hi Greg,
> > > > > > > >
> > > > > > > > On Mon, Feb 24, 2025 at 2:10 PM Greg KH <[e-mail]> wrote:
> > > > > > > > >
> > > > > > > > > On Mon, Feb 24, 2025 at 12:52:32PM +0800, Hsin-chen Chuang wrote:
> > > > > > > > > > From: Hsin-chen Chuang <[e-mail]>
> > > > > > > > > >
> > > > > > > > > > Automatically configure the altsetting for USER_CHANNEL when a SCO is
> > > > > > > > > > connected. 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 <[e-mail]>
> > > > > > > > > > ---
> > > > > > > > > >
> > > > > > > > > > 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 | 46 +++++++++++++++++++++++++++++++++++++++
> > > > > > > > > >  2 files changed, 57 insertions(+)
> > > > > > > > > >
> > > > > > > > > > diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
> > > > > > > > > > index 4ab32abf0f48..7c497f878732 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 USER_CHANNEL when SCO connected"
> > > > > > > > > > +     depends on BT_HCIBTUSB
> > > > > > > > > > +     default n
> > > > >
> > > > > Maybe we can do just:
> > > > >
> > > > >  default y if CHROME_PLATFORMS
> > > > >
> > > > > > > > > > +     help
> > > > > > > > > > +       Say Y here to enable auto set isoc_altsetting for USER_CHANNEL
> > > > > > > > > > +       when SCO connected
> > > > > > > > > > +
> > > > > > > > > > +       This can be overridden by passing btusb.auto_set_isoc_alt=[y|n]
> > > > > > > > > > +       on the kernel commandline.
> > > > > > > > > > +
> > > > > > > > > >  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..af93d757911b 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);
> > > > > > > > > >  }
> > > > > > > > > >
> > > > > > > > > > @@ -4354,6 +4397,9 @@ MODULE_PARM_DESC(enable_autosuspend, "Enable USB autosuspend by default");
> > > > > > > > > >  module_param(reset, bool, 0644);
> > > > > > > > > >  MODULE_PARM_DESC(reset, "Send HCI reset command on initialization");
> > > > > > > > > >
> > > > > > > > > > +module_param(auto_set_isoc_alt, bool, 0644);
> > > > > > > > > > +MODULE_PARM_DESC(auto_set_isoc_alt, "Auto set isoc_altsetting for USER_CHANNEL when SCO connected");
> > > > > > > > >
> > > > > > > > > This is not the 1990's, why are you adding new module parameters when we
> > > > > > > > > have so many other more proper ways to do this?  And really, this would
> > > > > > > >
> > > > > > > > Sorry but could you please provide an example to guard a feature like this.
> > > > > > >
> > > > > > > Depends on what you want to do with this configuration.  Why is it an
> > > > > > > option at all?  Why can't it "just work"?  Module parameters are a pain
> > > > > >
> > > > > > I would like to hand this question to Luiz. I believe this patch just
> > > > > > works because this configuration is defined in the spec.
> > > > > > I think Luiz's point is to project the potential existing user, but
> > > > > > there's probably no User channel user sending SCO data with the latest
> > > > > > btusb driver because:
> > > > > > a) There's no way to configure alt setting from userspace
> > > > > > b) Before eafcfcfca97d, SCO data would be rejected since User channel
> > > > > > shouldn't be able to modify hci_conn_num
> > > > >
> > > > > Perhaps you can just use CHROME_PLATFORMS (suggested above) in Kconfig
> > > > > to enable intercepting of the events, etc, so we don't need any
> > > > > runtime parameters.
> > > >
> > > > I'm afraid that this doesn't resolve Greg's comment below because the
> > > > multiple controllers are still bonded to the same config.
> > >
> > > Well that would be enabled for every controller plugged into the system.
> >
> > To clarify, I'm totally fine with runtime parameters and/or any
> > Kconfig like CHROME_PLATFORMS, although I'd prefer the current patch
> > more.
> >
> > But I guess Greg's point is that the multiple controllers should be
> > able to have different configurations at the same time. I'd respect
> > your decision to accept this patch or another patch with
> > CHROME_PLATFORMS, but I guess it's better to convince Greg first.
>
> Perhaps I got Greg's response wrong, but I think he was suggesting
> that it should just work without any special configuration, which is
> what I'm suggesting here, the use of CHROME_PLATFORMS is more of a
> stopgap until proper test automation is introduced.
>
> You can already have different configurations with respect to
> HCI_CHANNEL_USER since that is done per controller, btw that is used
> by the likes of btproxy tool for example which is normally used to
> hook the controller into a VM.

Thanks for the explanation. I'll follow the suggestion in v3.


Best Regards,

Hsin-chen

Ying Hsu Feb. 28, 2025, 4:47 a.m. UTC | #12

Beyond power concerns, another potential issue is USB host controller
scheduling fairness. SCO data is transmitted over isochronous pipes,
and if the alternate setting remains non-zero, the USB host controller
reserves bandwidth even when no audio data is being transmitted. This
could reduce throughput for some USB 1.x/2.0 applications, such as
file transfers from USB mass storage devices, when sharing the bus
with a Bluetooth controller in SCO mode. I don’t have a Chromebook to
verify this theory, but I suggest the ChromeOS team measure the
potential impact and follow up if necessary.

On Fri, Feb 28, 2025 at 12:44 AM Hsin-chen Chuang <[e-mail]> wrote:
>
> Hi Luiz,
>
> On Thu, Feb 27, 2025 at 11:59 PM Luiz Augusto von Dentz
> <[e-mail]> wrote:
> >
> > Hi Hsin-chen,
> >
> > On Thu, Feb 27, 2025 at 10:37 AM Hsin-chen Chuang <[e-mail]> wrote:
> > >
> > > Hi Luiz,
> > >
> > > On Thu, Feb 27, 2025 at 10:37 PM Luiz Augusto von Dentz
> > > <[e-mail]> wrote:
> > > >
> > > > Hi Hsin-chen,
> > > >
> > > > On Wed, Feb 26, 2025 at 9:22 PM Hsin-chen Chuang <[e-mail]> wrote:
> > > > >
> > > > > Hi Luiz,
> > > > >
> > > > > On Thu, Feb 27, 2025 at 4:55 AM Luiz Augusto von Dentz
> > > > > <[e-mail]> wrote:
> > > > > >
> > > > > > Hi Hsin-chen,
> > > > > >
> > > > > > On Mon, Feb 24, 2025 at 2:13 AM Hsin-chen Chuang <[e-mail]> wrote:
> > > > > > >
> > > > > > > On Mon, Feb 24, 2025 at 2:44 PM Greg KH <[e-mail]> wrote:
> > > > > > > >
> > > > > > > > On Mon, Feb 24, 2025 at 02:25:52PM +0800, Hsin-chen Chuang wrote:
> > > > > > > > > Hi Greg,
> > > > > > > > >
> > > > > > > > > On Mon, Feb 24, 2025 at 2:10 PM Greg KH <[e-mail]> wrote:
> > > > > > > > > >
> > > > > > > > > > On Mon, Feb 24, 2025 at 12:52:32PM +0800, Hsin-chen Chuang wrote:
> > > > > > > > > > > From: Hsin-chen Chuang <[e-mail]>
> > > > > > > > > > >
> > > > > > > > > > > Automatically configure the altsetting for USER_CHANNEL when a SCO is
> > > > > > > > > > > connected. 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 <[e-mail]>
> > > > > > > > > > > ---
> > > > > > > > > > >
> > > > > > > > > > > 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 | 46 +++++++++++++++++++++++++++++++++++++++
> > > > > > > > > > >  2 files changed, 57 insertions(+)
> > > > > > > > > > >
> > > > > > > > > > > diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
> > > > > > > > > > > index 4ab32abf0f48..7c497f878732 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 USER_CHANNEL when SCO connected"
> > > > > > > > > > > +     depends on BT_HCIBTUSB
> > > > > > > > > > > +     default n
> > > > > >
> > > > > > Maybe we can do just:
> > > > > >
> > > > > >  default y if CHROME_PLATFORMS
> > > > > >
> > > > > > > > > > > +     help
> > > > > > > > > > > +       Say Y here to enable auto set isoc_altsetting for USER_CHANNEL
> > > > > > > > > > > +       when SCO connected
> > > > > > > > > > > +
> > > > > > > > > > > +       This can be overridden by passing btusb.auto_set_isoc_alt=[y|n]
> > > > > > > > > > > +       on the kernel commandline.
> > > > > > > > > > > +
> > > > > > > > > > >  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..af93d757911b 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);
> > > > > > > > > > >  }
> > > > > > > > > > >
> > > > > > > > > > > @@ -4354,6 +4397,9 @@ MODULE_PARM_DESC(enable_autosuspend, "Enable USB autosuspend by default");
> > > > > > > > > > >  module_param(reset, bool, 0644);
> > > > > > > > > > >  MODULE_PARM_DESC(reset, "Send HCI reset command on initialization");
> > > > > > > > > > >
> > > > > > > > > > > +module_param(auto_set_isoc_alt, bool, 0644);
> > > > > > > > > > > +MODULE_PARM_DESC(auto_set_isoc_alt, "Auto set isoc_altsetting for USER_CHANNEL when SCO connected");
> > > > > > > > > >
> > > > > > > > > > This is not the 1990's, why are you adding new module parameters when we
> > > > > > > > > > have so many other more proper ways to do this?  And really, this would
> > > > > > > > >
> > > > > > > > > Sorry but could you please provide an example to guard a feature like this.
> > > > > > > >
> > > > > > > > Depends on what you want to do with this configuration.  Why is it an
> > > > > > > > option at all?  Why can't it "just work"?  Module parameters are a pain
> > > > > > >
> > > > > > > I would like to hand this question to Luiz. I believe this patch just
> > > > > > > works because this configuration is defined in the spec.
> > > > > > > I think Luiz's point is to project the potential existing user, but
> > > > > > > there's probably no User channel user sending SCO data with the latest
> > > > > > > btusb driver because:
> > > > > > > a) There's no way to configure alt setting from userspace
> > > > > > > b) Before eafcfcfca97d, SCO data would be rejected since User channel
> > > > > > > shouldn't be able to modify hci_conn_num
> > > > > >
> > > > > > Perhaps you can just use CHROME_PLATFORMS (suggested above) in Kconfig
> > > > > > to enable intercepting of the events, etc, so we don't need any
> > > > > > runtime parameters.
> > > > >
> > > > > I'm afraid that this doesn't resolve Greg's comment below because the
> > > > > multiple controllers are still bonded to the same config.
> > > >
> > > > Well that would be enabled for every controller plugged into the system.
> > >
> > > To clarify, I'm totally fine with runtime parameters and/or any
> > > Kconfig like CHROME_PLATFORMS, although I'd prefer the current patch
> > > more.
> > >
> > > But I guess Greg's point is that the multiple controllers should be
> > > able to have different configurations at the same time. I'd respect
> > > your decision to accept this patch or another patch with
> > > CHROME_PLATFORMS, but I guess it's better to convince Greg first.
> >
> > Perhaps I got Greg's response wrong, but I think he was suggesting
> > that it should just work without any special configuration, which is
> > what I'm suggesting here, the use of CHROME_PLATFORMS is more of a
> > stopgap until proper test automation is introduced.
> >
> > You can already have different configurations with respect to
> > HCI_CHANNEL_USER since that is done per controller, btw that is used
> > by the likes of btproxy tool for example which is normally used to
> > hook the controller into a VM.
>
> Thanks for the explanation. I'll follow the suggestion in v3.
>
>
> Best Regards,
>
> Hsin-chen

Patch

diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
index 4ab32abf0f48..7c497f878732 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 USER_CHANNEL when SCO connected"
+	depends on BT_HCIBTUSB
+	default n
+	help
+	  Say Y here to enable auto set isoc_altsetting for USER_CHANNEL
+	  when SCO connected
+
+	  This can be overridden by passing btusb.auto_set_isoc_alt=[y|n]
+	  on the kernel commandline.
+
 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..af93d757911b 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);
 }
 
@@ -4354,6 +4397,9 @@  MODULE_PARM_DESC(enable_autosuspend, "Enable USB autosuspend by default");
 module_param(reset, bool, 0644);
 MODULE_PARM_DESC(reset, "Send HCI reset command on initialization");
 
+module_param(auto_set_isoc_alt, bool, 0644);
+MODULE_PARM_DESC(auto_set_isoc_alt, "Auto set isoc_altsetting for USER_CHANNEL when SCO connected");
+
 MODULE_AUTHOR("Marcel Holtmann <[e-mail]>");
 MODULE_DESCRIPTION("Generic Bluetooth USB driver ver " VERSION);
 MODULE_VERSION(VERSION);