| Message ID | 20250224045237.1290971-1-chharry@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] Bluetooth: btusb: Configure altsetting for USER_CHANNEL | expand |
Hi Hsin-chen, On Mon, Feb 24, 2025 at 2:13 AM Hsin-chen Chuang <chharry@google.com> wrote: > > On Mon, Feb 24, 2025 at 2:44 PM Greg KH <gregkh@linuxfoundation.org> 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 <gregkh@linuxfoundation.org> wrote: > > > > > > > > On Mon, Feb 24, 2025 at 12:52:32PM +0800, Hsin-chen Chuang wrote: > > > > > From: Hsin-chen Chuang <chharry@chromium.org> > > > > > > > > > > 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 <chharry@chromium.org> > > > > > --- > > > > > > > > > > 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
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 <marcel@holtmann.org>"); MODULE_DESCRIPTION("Generic Bluetooth USB driver ver " VERSION); MODULE_VERSION(VERSION);