linux-kernel - Re: [PATCH v2] Bluetooth: btusb: Configure altsetting for USER

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2025022431-ditto-shy-c62f@gregkh>
Date: Mon, 24 Feb 2025 07:10:31 +0100
From: Greg KH <gregkh@...uxfoundation.org>
To: Hsin-chen Chuang <chharry@...gle.com>
Cc: linux-bluetooth@...r.kernel.org, luiz.dentz@...il.com,
	chromeos-bluetooth-upstreaming@...omium.org,
	Hsin-chen Chuang <chharry@...omium.org>,
	Marcel Holtmann <marcel@...tmann.org>,
	Ying Hsu <yinghsu@...omium.org>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] Bluetooth: btusb: Configure altsetting for
 USER_CHANNEL

On Mon, Feb 24, 2025 at 12:52:32PM +0800, Hsin-chen Chuang wrote:
> From: Hsin-chen Chuang <chharry@...omium.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@...omium.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
> +	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