[v5] Bluetooth: Fix possible race with userspace of sysfs isoc_alt

Commit Message

Hsin-chen Chuang Feb. 14, 2025, 11:16 a.m. UTC

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

Expose the isoc_alt attr with device group to avoid the racing.

Now we create a dev node for btusb. The isoc_alt attr belongs to it and
it also becomes the parent device of hci dev.

Fixes: b16b327edb4d ("Bluetooth: btusb: add sysfs attribute to control USB alt setting")
Signed-off-by: Hsin-chen Chuang <[e-mail]>
---

Changes in v5:
- Merge the ABI doc into this patch
- Manage the driver data with device

Changes in v4:
- Create a dev node for btusb. It's now hci dev's parent and the
  isoc_alt now belongs to it.
- Since the changes is almost limitted in btusb, no need to add the
  callbacks in hdev anymore.

Changes in v3:
- Make the attribute exported only when the isoc_alt is available.
- In btusb_probe, determine data->isoc before calling hci_alloc_dev_priv
  (which calls hci_init_sysfs).
- Since hci_init_sysfs is called before btusb could modify the hdev,
  add new argument add_isoc_alt_attr for btusb to inform hci_init_sysfs.

Changes in v2:
- The patch has been removed from series

 .../ABI/stable/sysfs-class-bluetooth          |  13 ++
 drivers/bluetooth/btusb.c                     | 111 ++++++++++++++----
 include/net/bluetooth/hci_core.h              |   1 +
 net/bluetooth/hci_sysfs.c                     |   3 +-
 4 files changed, 102 insertions(+), 26 deletions(-)

Comments

Hsin-chen Chuang Feb. 14, 2025, 11:24 a.m. UTC | #1

On Fri, Feb 14, 2025 at 7:16 PM Hsin-chen Chuang <[e-mail]> wrote:
>
> From: Hsin-chen Chuang <[e-mail]>
>
> Expose the isoc_alt attr with device group to avoid the racing.
>
> Now we create a dev node for btusb. The isoc_alt attr belongs to it and
> it also becomes the parent device of hci dev.
>
> Fixes: b16b327edb4d ("Bluetooth: btusb: add sysfs attribute to control USB alt setting")
> Signed-off-by: Hsin-chen Chuang <[e-mail]>
> ---
>
> Changes in v5:
> - Merge the ABI doc into this patch
> - Manage the driver data with device
>
> Changes in v4:
> - Create a dev node for btusb. It's now hci dev's parent and the
>   isoc_alt now belongs to it.
> - Since the changes is almost limitted in btusb, no need to add the
>   callbacks in hdev anymore.
>
> Changes in v3:
> - Make the attribute exported only when the isoc_alt is available.
> - In btusb_probe, determine data->isoc before calling hci_alloc_dev_priv
>   (which calls hci_init_sysfs).
> - Since hci_init_sysfs is called before btusb could modify the hdev,
>   add new argument add_isoc_alt_attr for btusb to inform hci_init_sysfs.
>
> Changes in v2:
> - The patch has been removed from series
>
>  .../ABI/stable/sysfs-class-bluetooth          |  13 ++
>  drivers/bluetooth/btusb.c                     | 111 ++++++++++++++----
>  include/net/bluetooth/hci_core.h              |   1 +
>  net/bluetooth/hci_sysfs.c                     |   3 +-
>  4 files changed, 102 insertions(+), 26 deletions(-)
>
> diff --git a/Documentation/ABI/stable/sysfs-class-bluetooth b/Documentation/ABI/stable/sysfs-class-bluetooth
> index 36be02471174..c1024c7c4634 100644
> --- a/Documentation/ABI/stable/sysfs-class-bluetooth
> +++ b/Documentation/ABI/stable/sysfs-class-bluetooth
> @@ -7,3 +7,16 @@ Description:   This write-only attribute allows users to trigger the vendor reset
>                 The reset may or may not be done through the device transport
>                 (e.g., UART/USB), and can also be done through an out-of-band
>                 approach such as GPIO.
> +
> +What:          /sys/class/bluetooth/btusb<usb-intf>/isoc_alt
> +Date:          13-Feb-2025
> +KernelVersion: 6.13
> +Contact:       [e-mail]
> +Description:   This attribute allows users to configure the USB Alternate setting
> +               for the specific HCI device. Reading this attribute returns the
> +               current setting, and writing any supported numbers would change
> +               the setting. See the USB Alternate setting definition in Bluetooth
> +               core spec 5, vol 4, part B, table 2.1.
> +               If the HCI device is not yet init-ed, the write fails with -ENODEV.
> +               If the data is not a valid number, the write fails with -EINVAL.
> +               The other failures are vendor specific.
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index 1caf7a071a73..e2fb3d08a5ed 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -920,6 +920,8 @@ struct btusb_data {
>         int oob_wake_irq;   /* irq for out-of-band wake-on-bt */
>
>         struct qca_dump_info qca_dump;
> +
> +       struct device dev;
>  };
>
>  static void btusb_reset(struct hci_dev *hdev)
> @@ -3693,6 +3695,9 @@ static ssize_t isoc_alt_store(struct device *dev,
>         int alt;
>         int ret;
>
> +       if (!data->hdev)
> +               return -ENODEV;
> +
>         if (kstrtoint(buf, 10, &alt))
>                 return -EINVAL;
>
> @@ -3702,6 +3707,36 @@ static ssize_t isoc_alt_store(struct device *dev,
>
>  static DEVICE_ATTR_RW(isoc_alt);
>
> +static struct attribute *btusb_sysfs_attrs[] = {
> +       NULL,
> +};
> +ATTRIBUTE_GROUPS(btusb_sysfs);
> +
> +static void btusb_sysfs_release(struct device *dev)
> +{
> +       struct btusb_data *data = dev_get_drvdata(dev);
> +
> +       kfree(data);
> +}
> +
> +static const struct device_type btusb_sysfs = {
> +       .name    = "btusb",
> +       .release = btusb_sysfs_release,
> +       .groups  = btusb_sysfs_groups,
> +};
> +
> +static struct attribute *btusb_sysfs_isoc_alt_attrs[] = {
> +       &dev_attr_isoc_alt.attr,
> +       NULL,
> +};
> +ATTRIBUTE_GROUPS(btusb_sysfs_isoc_alt);
> +
> +static const struct device_type btusb_sysfs_isoc_alt = {
> +       .name    = "btusb",
> +       .release = btusb_sysfs_release,
> +       .groups  = btusb_sysfs_isoc_alt_groups,
> +};
> +
>  static int btusb_probe(struct usb_interface *intf,
>                        const struct usb_device_id *id)
>  {
> @@ -3743,7 +3778,7 @@ static int btusb_probe(struct usb_interface *intf,
>                         return -ENODEV;
>         }
>
> -       data = devm_kzalloc(&intf->dev, sizeof(*data), GFP_KERNEL);
> +       data = kzalloc(sizeof(*data), GFP_KERNEL);
>         if (!data)
>                 return -ENOMEM;
>
> @@ -3766,8 +3801,10 @@ static int btusb_probe(struct usb_interface *intf,
>                 }
>         }
>
> -       if (!data->intr_ep || !data->bulk_tx_ep || !data->bulk_rx_ep)
> -               return -ENODEV;
> +       if (!data->intr_ep || !data->bulk_tx_ep || !data->bulk_rx_ep) {
> +               err = -ENODEV;
> +               goto out_free_data;
> +       }
>
>         if (id->driver_info & BTUSB_AMP) {
>                 data->cmdreq_type = USB_TYPE_CLASS | 0x01;
> @@ -3821,16 +3858,47 @@ static int btusb_probe(struct usb_interface *intf,
>
>         data->recv_acl = hci_recv_frame;
>
> +       if (id->driver_info & BTUSB_AMP) {
> +               /* AMP controllers do not support SCO packets */
> +               data->isoc = NULL;
> +       } else {
> +               /* Interface orders are hardcoded in the specification */
> +               data->isoc = usb_ifnum_to_if(data->udev, ifnum_base + 1);
> +               data->isoc_ifnum = ifnum_base + 1;
> +       }
> +
> +       if (id->driver_info & BTUSB_BROKEN_ISOC)
> +               data->isoc = NULL;
> +
> +       /* Init a dev for btusb. The attr depends on the support of isoc. */
> +       if (data->isoc)
> +               data->dev.type = &btusb_sysfs_isoc_alt;
> +       else
> +               data->dev.type = &btusb_sysfs;
> +       data->dev.class = &bt_class;
> +       data->dev.parent = &intf->dev;
> +
> +       err = dev_set_name(&data->dev, "btusb%s", dev_name(&intf->dev));
> +       if (err)
> +               goto out_free_data;
> +
> +       dev_set_drvdata(&data->dev, data);
> +       err = device_register(&data->dev);
> +       if (err < 0)
> +               goto out_put_sysfs;
> +
>         hdev = hci_alloc_dev_priv(priv_size);
> -       if (!hdev)
> -               return -ENOMEM;
> +       if (!hdev) {
> +               err = -ENOMEM;
> +               goto out_free_sysfs;
> +       }
>
>         hdev->bus = HCI_USB;
>         hci_set_drvdata(hdev, data);
>
>         data->hdev = hdev;
>
> -       SET_HCIDEV_DEV(hdev, &intf->dev);
> +       SET_HCIDEV_DEV(hdev, &data->dev);
>
>         reset_gpio = gpiod_get_optional(&data->udev->dev, "reset",
>                                         GPIOD_OUT_LOW);
> @@ -3969,15 +4037,6 @@ static int btusb_probe(struct usb_interface *intf,
>                 hci_set_msft_opcode(hdev, 0xFD70);
>         }
>
> -       if (id->driver_info & BTUSB_AMP) {
> -               /* AMP controllers do not support SCO packets */
> -               data->isoc = NULL;
> -       } else {
> -               /* Interface orders are hardcoded in the specification */
> -               data->isoc = usb_ifnum_to_if(data->udev, ifnum_base + 1);
> -               data->isoc_ifnum = ifnum_base + 1;
> -       }
> -
>         if (IS_ENABLED(CONFIG_BT_HCIBTUSB_RTL) &&
>             (id->driver_info & BTUSB_REALTEK)) {
>                 btrtl_set_driver_name(hdev, btusb_driver.name);
> @@ -4010,9 +4069,6 @@ static int btusb_probe(struct usb_interface *intf,
>                         set_bit(HCI_QUIRK_FIXUP_BUFFER_SIZE, &hdev->quirks);
>         }
>
> -       if (id->driver_info & BTUSB_BROKEN_ISOC)
> -               data->isoc = NULL;
> -
>         if (id->driver_info & BTUSB_WIDEBAND_SPEECH)
>                 set_bit(HCI_QUIRK_WIDEBAND_SPEECH_SUPPORTED, &hdev->quirks);
>
> @@ -4065,10 +4121,6 @@ static int btusb_probe(struct usb_interface *intf,
>                                                  data->isoc, data);
>                 if (err < 0)
>                         goto out_free_dev;
> -
> -               err = device_create_file(&intf->dev, &dev_attr_isoc_alt);
> -               if (err)
> -                       goto out_free_dev;
>         }
>
>         if (IS_ENABLED(CONFIG_BT_HCIBTUSB_BCM) && data->diag) {
> @@ -4099,6 +4151,16 @@ static int btusb_probe(struct usb_interface *intf,
>         if (data->reset_gpio)
>                 gpiod_put(data->reset_gpio);
>         hci_free_dev(hdev);
> +
> +out_free_sysfs:
> +       device_del(&data->dev);
> +
> +out_put_sysfs:
> +       put_device(&data->dev);
> +       return err;
> +
> +out_free_data:
> +       kfree(data);
>         return err;
>  }
>
> @@ -4115,10 +4177,8 @@ static void btusb_disconnect(struct usb_interface *intf)
>         hdev = data->hdev;
>         usb_set_intfdata(data->intf, NULL);
>
> -       if (data->isoc) {
> -               device_remove_file(&intf->dev, &dev_attr_isoc_alt);
> +       if (data->isoc)
>                 usb_set_intfdata(data->isoc, NULL);
> -       }
>
>         if (data->diag)
>                 usb_set_intfdata(data->diag, NULL);
> @@ -4150,6 +4210,7 @@ static void btusb_disconnect(struct usb_interface *intf)
>                 gpiod_put(data->reset_gpio);
>
>         hci_free_dev(hdev);
> +       device_unregister(&data->dev);
>  }
>
>  #ifdef CONFIG_PM
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 05919848ea95..776dd6183509 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -1843,6 +1843,7 @@ int hci_get_adv_monitor_offload_ext(struct hci_dev *hdev);
>
>  void hci_event_packet(struct hci_dev *hdev, struct sk_buff *skb);
>
> +extern const struct class bt_class;
>  void hci_init_sysfs(struct hci_dev *hdev);
>  void hci_conn_init_sysfs(struct hci_conn *conn);
>  void hci_conn_add_sysfs(struct hci_conn *conn);
> diff --git a/net/bluetooth/hci_sysfs.c b/net/bluetooth/hci_sysfs.c
> index 041ce9adc378..aab3ffaa264c 100644
> --- a/net/bluetooth/hci_sysfs.c
> +++ b/net/bluetooth/hci_sysfs.c
> @@ -6,9 +6,10 @@
>  #include <net/bluetooth/bluetooth.h>
>  #include <net/bluetooth/hci_core.h>
>
> -static const struct class bt_class = {
> +const struct class bt_class = {
>         .name = "bluetooth",
>  };
> +EXPORT_SYMBOL(bt_class);
>
>  static void bt_link_release(struct device *dev)
>  {
> --
> 2.48.1.601.g30ceb7b040-goog
>

Ouch, please kindly ignore this patch. I missed some discussion in V4

Greg Kroah-Hartman Feb. 14, 2025, 11:37 a.m. UTC | #2

On Fri, Feb 14, 2025 at 07:16:17PM +0800, Hsin-chen Chuang wrote:
> From: Hsin-chen Chuang <[e-mail]>
> 
> Expose the isoc_alt attr with device group to avoid the racing.
> 
> Now we create a dev node for btusb. The isoc_alt attr belongs to it and
> it also becomes the parent device of hci dev.
> 
> Fixes: b16b327edb4d ("Bluetooth: btusb: add sysfs attribute to control USB alt setting")

Wait, step back, why is this commit needed if you can change the alt
setting already today through usbfs/libusb without needing to mess with
the bluetooth stack at all?

> Signed-off-by: Hsin-chen Chuang <[e-mail]>
> ---
> 
> Changes in v5:
> - Merge the ABI doc into this patch
> - Manage the driver data with device
> 
> Changes in v4:
> - Create a dev node for btusb. It's now hci dev's parent and the
>   isoc_alt now belongs to it.
> - Since the changes is almost limitted in btusb, no need to add the
>   callbacks in hdev anymore.
> 
> Changes in v3:
> - Make the attribute exported only when the isoc_alt is available.
> - In btusb_probe, determine data->isoc before calling hci_alloc_dev_priv
>   (which calls hci_init_sysfs).
> - Since hci_init_sysfs is called before btusb could modify the hdev,
>   add new argument add_isoc_alt_attr for btusb to inform hci_init_sysfs.
> 
> Changes in v2:
> - The patch has been removed from series
> 
>  .../ABI/stable/sysfs-class-bluetooth          |  13 ++
>  drivers/bluetooth/btusb.c                     | 111 ++++++++++++++----
>  include/net/bluetooth/hci_core.h              |   1 +
>  net/bluetooth/hci_sysfs.c                     |   3 +-
>  4 files changed, 102 insertions(+), 26 deletions(-)
> 
> diff --git a/Documentation/ABI/stable/sysfs-class-bluetooth b/Documentation/ABI/stable/sysfs-class-bluetooth
> index 36be02471174..c1024c7c4634 100644
> --- a/Documentation/ABI/stable/sysfs-class-bluetooth
> +++ b/Documentation/ABI/stable/sysfs-class-bluetooth
> @@ -7,3 +7,16 @@ Description: 	This write-only attribute allows users to trigger the vendor reset
>  		The reset may or may not be done through the device transport
>  		(e.g., UART/USB), and can also be done through an out-of-band
>  		approach such as GPIO.
> +
> +What:		/sys/class/bluetooth/btusb<usb-intf>/isoc_alt
> +Date:		13-Feb-2025
> +KernelVersion:	6.13
> +Contact:	[e-mail]
> +Description:	This attribute allows users to configure the USB Alternate setting
> +		for the specific HCI device. Reading this attribute returns the
> +		current setting, and writing any supported numbers would change
> +		the setting. See the USB Alternate setting definition in Bluetooth
> +		core spec 5, vol 4, part B, table 2.1.
> +		If the HCI device is not yet init-ed, the write fails with -ENODEV.
> +		If the data is not a valid number, the write fails with -EINVAL.
> +		The other failures are vendor specific.

Again, what's wrong with libusb/usbfs to do this today?


> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index 1caf7a071a73..e2fb3d08a5ed 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -920,6 +920,8 @@ struct btusb_data {
>  	int oob_wake_irq;   /* irq for out-of-band wake-on-bt */
>  
>  	struct qca_dump_info qca_dump;
> +
> +	struct device dev;

Ah, so now this structure's lifecycle is determined by the device you
just embedded in it?  Are you sure you got this right?

>  };
>  
>  static void btusb_reset(struct hci_dev *hdev)
> @@ -3693,6 +3695,9 @@ static ssize_t isoc_alt_store(struct device *dev,
>  	int alt;
>  	int ret;
>  
> +	if (!data->hdev)
> +		return -ENODEV;
> +
>  	if (kstrtoint(buf, 10, &alt))
>  		return -EINVAL;
>  
> @@ -3702,6 +3707,36 @@ static ssize_t isoc_alt_store(struct device *dev,
>  
>  static DEVICE_ATTR_RW(isoc_alt);
>  
> +static struct attribute *btusb_sysfs_attrs[] = {
> +	NULL,
> +};
> +ATTRIBUTE_GROUPS(btusb_sysfs);
> +
> +static void btusb_sysfs_release(struct device *dev)
> +{
> +	struct btusb_data *data = dev_get_drvdata(dev);

That feels wrong, it's embedded in the device, not pointed to by the
device.  So this should be a container_of() call, right?

> +
> +	kfree(data);
> +}
> +
> +static const struct device_type btusb_sysfs = {
> +	.name    = "btusb",
> +	.release = btusb_sysfs_release,
> +	.groups  = btusb_sysfs_groups,
> +};
> +
> +static struct attribute *btusb_sysfs_isoc_alt_attrs[] = {
> +	&dev_attr_isoc_alt.attr,
> +	NULL,
> +};
> +ATTRIBUTE_GROUPS(btusb_sysfs_isoc_alt);
> +
> +static const struct device_type btusb_sysfs_isoc_alt = {
> +	.name    = "btusb",
> +	.release = btusb_sysfs_release,
> +	.groups  = btusb_sysfs_isoc_alt_groups,
> +};
> +
>  static int btusb_probe(struct usb_interface *intf,
>  		       const struct usb_device_id *id)
>  {
> @@ -3743,7 +3778,7 @@ static int btusb_probe(struct usb_interface *intf,
>  			return -ENODEV;
>  	}
>  
> -	data = devm_kzalloc(&intf->dev, sizeof(*data), GFP_KERNEL);
> +	data = kzalloc(sizeof(*data), GFP_KERNEL);
>  	if (!data)
>  		return -ENOMEM;
>  
> @@ -3766,8 +3801,10 @@ static int btusb_probe(struct usb_interface *intf,
>  		}
>  	}
>  
> -	if (!data->intr_ep || !data->bulk_tx_ep || !data->bulk_rx_ep)
> -		return -ENODEV;
> +	if (!data->intr_ep || !data->bulk_tx_ep || !data->bulk_rx_ep) {
> +		err = -ENODEV;
> +		goto out_free_data;
> +	}
>  
>  	if (id->driver_info & BTUSB_AMP) {
>  		data->cmdreq_type = USB_TYPE_CLASS | 0x01;
> @@ -3821,16 +3858,47 @@ static int btusb_probe(struct usb_interface *intf,
>  
>  	data->recv_acl = hci_recv_frame;
>  
> +	if (id->driver_info & BTUSB_AMP) {
> +		/* AMP controllers do not support SCO packets */
> +		data->isoc = NULL;
> +	} else {
> +		/* Interface orders are hardcoded in the specification */
> +		data->isoc = usb_ifnum_to_if(data->udev, ifnum_base + 1);
> +		data->isoc_ifnum = ifnum_base + 1;
> +	}
> +
> +	if (id->driver_info & BTUSB_BROKEN_ISOC)
> +		data->isoc = NULL;
> +
> +	/* Init a dev for btusb. The attr depends on the support of isoc. */
> +	if (data->isoc)
> +		data->dev.type = &btusb_sysfs_isoc_alt;
> +	else
> +		data->dev.type = &btusb_sysfs;

When walking the class, are you sure you check for the proper types now?
Does anyone walk all of the class devices anywhere?

> +	data->dev.class = &bt_class;
> +	data->dev.parent = &intf->dev;
> +
> +	err = dev_set_name(&data->dev, "btusb%s", dev_name(&intf->dev));

what does this name look like in a real system?  squashing them together
feels wrong, why is 'btusb' needed here at all?

> +	if (err)
> +		goto out_free_data;
> +
> +	dev_set_drvdata(&data->dev, data);
> +	err = device_register(&data->dev);
> +	if (err < 0)
> +		goto out_put_sysfs;
> +
>  	hdev = hci_alloc_dev_priv(priv_size);
> -	if (!hdev)
> -		return -ENOMEM;
> +	if (!hdev) {
> +		err = -ENOMEM;
> +		goto out_free_sysfs;
> +	}
>  
>  	hdev->bus = HCI_USB;
>  	hci_set_drvdata(hdev, data);
>  
>  	data->hdev = hdev;
>  
> -	SET_HCIDEV_DEV(hdev, &intf->dev);
> +	SET_HCIDEV_DEV(hdev, &data->dev);
>  
>  	reset_gpio = gpiod_get_optional(&data->udev->dev, "reset",
>  					GPIOD_OUT_LOW);
> @@ -3969,15 +4037,6 @@ static int btusb_probe(struct usb_interface *intf,
>  		hci_set_msft_opcode(hdev, 0xFD70);
>  	}
>  
> -	if (id->driver_info & BTUSB_AMP) {
> -		/* AMP controllers do not support SCO packets */
> -		data->isoc = NULL;
> -	} else {
> -		/* Interface orders are hardcoded in the specification */
> -		data->isoc = usb_ifnum_to_if(data->udev, ifnum_base + 1);
> -		data->isoc_ifnum = ifnum_base + 1;
> -	}
> -
>  	if (IS_ENABLED(CONFIG_BT_HCIBTUSB_RTL) &&
>  	    (id->driver_info & BTUSB_REALTEK)) {
>  		btrtl_set_driver_name(hdev, btusb_driver.name);
> @@ -4010,9 +4069,6 @@ static int btusb_probe(struct usb_interface *intf,
>  			set_bit(HCI_QUIRK_FIXUP_BUFFER_SIZE, &hdev->quirks);
>  	}
>  
> -	if (id->driver_info & BTUSB_BROKEN_ISOC)
> -		data->isoc = NULL;
> -
>  	if (id->driver_info & BTUSB_WIDEBAND_SPEECH)
>  		set_bit(HCI_QUIRK_WIDEBAND_SPEECH_SUPPORTED, &hdev->quirks);
>  
> @@ -4065,10 +4121,6 @@ static int btusb_probe(struct usb_interface *intf,
>  						 data->isoc, data);
>  		if (err < 0)
>  			goto out_free_dev;
> -
> -		err = device_create_file(&intf->dev, &dev_attr_isoc_alt);

You have now moved the file, are you sure you don't also need to update
the documentation?


> -		if (err)
> -			goto out_free_dev;
>  	}
>  
>  	if (IS_ENABLED(CONFIG_BT_HCIBTUSB_BCM) && data->diag) {
> @@ -4099,6 +4151,16 @@ static int btusb_probe(struct usb_interface *intf,
>  	if (data->reset_gpio)
>  		gpiod_put(data->reset_gpio);
>  	hci_free_dev(hdev);
> +
> +out_free_sysfs:
> +	device_del(&data->dev);
> +
> +out_put_sysfs:
> +	put_device(&data->dev);
> +	return err;
> +
> +out_free_data:
> +	kfree(data);
>  	return err;
>  }
>  
> @@ -4115,10 +4177,8 @@ static void btusb_disconnect(struct usb_interface *intf)
>  	hdev = data->hdev;
>  	usb_set_intfdata(data->intf, NULL);
>  
> -	if (data->isoc) {
> -		device_remove_file(&intf->dev, &dev_attr_isoc_alt);
> +	if (data->isoc)
>  		usb_set_intfdata(data->isoc, NULL);
> -	}
>  
>  	if (data->diag)
>  		usb_set_intfdata(data->diag, NULL);
> @@ -4150,6 +4210,7 @@ static void btusb_disconnect(struct usb_interface *intf)
>  		gpiod_put(data->reset_gpio);
>  
>  	hci_free_dev(hdev);
> +	device_unregister(&data->dev);
>  }
>  
>  #ifdef CONFIG_PM
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 05919848ea95..776dd6183509 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -1843,6 +1843,7 @@ int hci_get_adv_monitor_offload_ext(struct hci_dev *hdev);
>  
>  void hci_event_packet(struct hci_dev *hdev, struct sk_buff *skb);
>  
> +extern const struct class bt_class;
>  void hci_init_sysfs(struct hci_dev *hdev);
>  void hci_conn_init_sysfs(struct hci_conn *conn);
>  void hci_conn_add_sysfs(struct hci_conn *conn);
> diff --git a/net/bluetooth/hci_sysfs.c b/net/bluetooth/hci_sysfs.c
> index 041ce9adc378..aab3ffaa264c 100644
> --- a/net/bluetooth/hci_sysfs.c
> +++ b/net/bluetooth/hci_sysfs.c
> @@ -6,9 +6,10 @@
>  #include <net/bluetooth/bluetooth.h>
>  #include <net/bluetooth/hci_core.h>
>  
> -static const struct class bt_class = {
> +const struct class bt_class = {
>  	.name = "bluetooth",
>  };
> +EXPORT_SYMBOL(bt_class);

EXPORT_SYMBOL_GPL(), right?

thanks,

greg k-h

[e-mail] Feb. 14, 2025, 11:50 a.m. UTC | #3

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=933982

---Test result---

Test Summary:
CheckPatch                    PENDING   0.27 seconds
GitLint                       PENDING   0.20 seconds
SubjectPrefix                 PASS      0.12 seconds
BuildKernel                   PASS      23.96 seconds
CheckAllWarning               PASS      26.59 seconds
CheckSparse                   PASS      30.50 seconds
BuildKernel32                 PASS      23.98 seconds
TestRunnerSetup               PASS      430.81 seconds
TestRunner_l2cap-tester       FAIL      0.69 seconds
TestRunner_iso-tester         FAIL      0.31 seconds
TestRunner_bnep-tester        FAIL      0.21 seconds
TestRunner_mgmt-tester        FAIL      0.17 seconds
TestRunner_rfcomm-tester      FAIL      0.17 seconds
TestRunner_sco-tester         FAIL      0.15 seconds
TestRunner_ioctl-tester       FAIL      0.18 seconds
TestRunner_mesh-tester        FAIL      0.15 seconds
TestRunner_smp-tester         FAIL      0.15 seconds
TestRunner_userchan-tester    FAIL      0.15 seconds
IncrementalBuild              PENDING   0.51 seconds

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

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

##############################
Test: TestRunner_l2cap-tester - FAIL
Desc: Run l2cap-tester with test-runner
Output:

Could not access KVM kernel module: No such file or directory
qemu-system-x86_64: failed to initialize KVM: No such file or directory
qemu-system-x86_64: Back to tcg accelerator
qemu-system-x86_64: CPU model 'host' requires KVM
##############################
Test: TestRunner_iso-tester - FAIL
Desc: Run iso-tester with test-runner
Output:

Could not access KVM kernel module: No such file or directory
qemu-system-x86_64: failed to initialize KVM: No such file or directory
qemu-system-x86_64: Back to tcg accelerator
qemu-system-x86_64: CPU model 'host' requires KVM
##############################
Test: TestRunner_bnep-tester - FAIL
Desc: Run bnep-tester with test-runner
Output:

Could not access KVM kernel module: No such file or directory
qemu-system-x86_64: failed to initialize KVM: No such file or directory
qemu-system-x86_64: Back to tcg accelerator
qemu-system-x86_64: CPU model 'host' requires KVM
##############################
Test: TestRunner_mgmt-tester - FAIL
Desc: Run mgmt-tester with test-runner
Output:

Could not access KVM kernel module: No such file or directory
qemu-system-x86_64: failed to initialize KVM: No such file or directory
qemu-system-x86_64: Back to tcg accelerator
qemu-system-x86_64: CPU model 'host' requires KVM
##############################
Test: TestRunner_rfcomm-tester - FAIL
Desc: Run rfcomm-tester with test-runner
Output:

Could not access KVM kernel module: No such file or directory
qemu-system-x86_64: failed to initialize KVM: No such file or directory
qemu-system-x86_64: Back to tcg accelerator
qemu-system-x86_64: CPU model 'host' requires KVM
##############################
Test: TestRunner_sco-tester - FAIL
Desc: Run sco-tester with test-runner
Output:

Could not access KVM kernel module: No such file or directory
qemu-system-x86_64: failed to initialize KVM: No such file or directory
qemu-system-x86_64: Back to tcg accelerator
qemu-system-x86_64: CPU model 'host' requires KVM
##############################
Test: TestRunner_ioctl-tester - FAIL
Desc: Run ioctl-tester with test-runner
Output:

Could not access KVM kernel module: No such file or directory
qemu-system-x86_64: failed to initialize KVM: No such file or directory
qemu-system-x86_64: Back to tcg accelerator
qemu-system-x86_64: CPU model 'host' requires KVM
##############################
Test: TestRunner_mesh-tester - FAIL
Desc: Run mesh-tester with test-runner
Output:

Could not access KVM kernel module: No such file or directory
qemu-system-x86_64: failed to initialize KVM: No such file or directory
qemu-system-x86_64: Back to tcg accelerator
qemu-system-x86_64: CPU model 'host' requires KVM
##############################
Test: TestRunner_smp-tester - FAIL
Desc: Run smp-tester with test-runner
Output:

Could not access KVM kernel module: No such file or directory
qemu-system-x86_64: failed to initialize KVM: No such file or directory
qemu-system-x86_64: Back to tcg accelerator
qemu-system-x86_64: CPU model 'host' requires KVM
##############################
Test: TestRunner_userchan-tester - FAIL
Desc: Run userchan-tester with test-runner
Output:

Could not access KVM kernel module: No such file or directory
qemu-system-x86_64: failed to initialize KVM: No such file or directory
qemu-system-x86_64: Back to tcg accelerator
qemu-system-x86_64: CPU model 'host' requires KVM
##############################
Test: IncrementalBuild - PENDING
Desc: Incremental build with the patches in the series
Output:



---
Regards,
Linux Bluetooth

Hsin-chen Chuang Feb. 17, 2025, 8:44 a.m. UTC | #4

On Fri, Feb 14, 2025 at 7:37 PM Greg KH <[e-mail]> wrote:
>
> On Fri, Feb 14, 2025 at 07:16:17PM +0800, Hsin-chen Chuang wrote:
> > From: Hsin-chen Chuang <[e-mail]>
> >
> > Expose the isoc_alt attr with device group to avoid the racing.
> >
> > Now we create a dev node for btusb. The isoc_alt attr belongs to it and
> > it also becomes the parent device of hci dev.
> >
> > Fixes: b16b327edb4d ("Bluetooth: btusb: add sysfs attribute to control USB alt setting")
>
> Wait, step back, why is this commit needed if you can change the alt
> setting already today through usbfs/libusb without needing to mess with
> the bluetooth stack at all?

In short: We want to configure the alternate settings without
detaching the btusb driver, while detaching seems necessary for
libusb_set_interface_alt_setting to work (Please correct me if I'm
wrong!)

Background:
The Bluetooth Core Specification defines a protocol for the operating
system to communicate with a Bluetooth chipset, called HCI (Host
Controller Interface) (Host=OS, Controller=chipset).
We could say the main purpose of the Linux Bluetooth drivers is to set
up and get the HCI ready for the "upper layer" to use.

Who could be the "upper layer" then? There are mainly 2: "Control" and
"User" channels.
Linux has its default Bluetooth stack, BlueZ, which is splitted into 2
parts: the kernel space and the user space. The kernel space part
provides an abstracted Bluetooth API called MGMT, and is exposed
through the Bluetooth HCI socket "Control" channel.
On the other hand Linux also exposes the Bluetooth HCI socket "User"
channel, allowing the user space APPs to send/receive the HCI packets
directly to/from the chipset. Google's products (Android, ChromeOS,
etc) use this channel.

Now why this patch?
It's because the Bluetooth spec defines something specific to USB
transport: A USB Bluetooth chipset must/should support these alternate
settings; When transferring this type of the Audio data this alt must
be used, bla bla bla...
The Control channel handles this in the kernel part. However, the
applications built on top of the User channel are unable to configure
the alt setting, and I'd like to add the support through sysfs.

>
> > Signed-off-by: Hsin-chen Chuang <[e-mail]>
> > ---
> >
> > Changes in v5:
> > - Merge the ABI doc into this patch
> > - Manage the driver data with device
> >
> > Changes in v4:
> > - Create a dev node for btusb. It's now hci dev's parent and the
> >   isoc_alt now belongs to it.
> > - Since the changes is almost limitted in btusb, no need to add the
> >   callbacks in hdev anymore.
> >
> > Changes in v3:
> > - Make the attribute exported only when the isoc_alt is available.
> > - In btusb_probe, determine data->isoc before calling hci_alloc_dev_priv
> >   (which calls hci_init_sysfs).
> > - Since hci_init_sysfs is called before btusb could modify the hdev,
> >   add new argument add_isoc_alt_attr for btusb to inform hci_init_sysfs.
> >
> > Changes in v2:
> > - The patch has been removed from series
> >
> >  .../ABI/stable/sysfs-class-bluetooth          |  13 ++
> >  drivers/bluetooth/btusb.c                     | 111 ++++++++++++++----
> >  include/net/bluetooth/hci_core.h              |   1 +
> >  net/bluetooth/hci_sysfs.c                     |   3 +-
> >  4 files changed, 102 insertions(+), 26 deletions(-)
> >
> > diff --git a/Documentation/ABI/stable/sysfs-class-bluetooth b/Documentation/ABI/stable/sysfs-class-bluetooth
> > index 36be02471174..c1024c7c4634 100644
> > --- a/Documentation/ABI/stable/sysfs-class-bluetooth
> > +++ b/Documentation/ABI/stable/sysfs-class-bluetooth
> > @@ -7,3 +7,16 @@ Description:         This write-only attribute allows users to trigger the vendor reset
> >               The reset may or may not be done through the device transport
> >               (e.g., UART/USB), and can also be done through an out-of-band
> >               approach such as GPIO.
> > +
> > +What:                /sys/class/bluetooth/btusb<usb-intf>/isoc_alt
> > +Date:                13-Feb-2025
> > +KernelVersion:       6.13
> > +Contact:     [e-mail]
> > +Description: This attribute allows users to configure the USB Alternate setting
> > +             for the specific HCI device. Reading this attribute returns the
> > +             current setting, and writing any supported numbers would change
> > +             the setting. See the USB Alternate setting definition in Bluetooth
> > +             core spec 5, vol 4, part B, table 2.1.
> > +             If the HCI device is not yet init-ed, the write fails with -ENODEV.
> > +             If the data is not a valid number, the write fails with -EINVAL.
> > +             The other failures are vendor specific.
>
> Again, what's wrong with libusb/usbfs to do this today?
>
>
> > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> > index 1caf7a071a73..e2fb3d08a5ed 100644
> > --- a/drivers/bluetooth/btusb.c
> > +++ b/drivers/bluetooth/btusb.c
> > @@ -920,6 +920,8 @@ struct btusb_data {
> >       int oob_wake_irq;   /* irq for out-of-band wake-on-bt */
> >
> >       struct qca_dump_info qca_dump;
> > +
> > +     struct device dev;
>
> Ah, so now this structure's lifecycle is determined by the device you
> just embedded in it?  Are you sure you got this right?

Yes, I think so. The structure should be freed when usb disconnects.
In the current implementation all its members are released when usb
disconnects except for the structure itself, because it's allocated
through devm_kzalloc. Since we now make it a device we could make the
lifecycle clearer.

>
> >  };
> >
> >  static void btusb_reset(struct hci_dev *hdev)
> > @@ -3693,6 +3695,9 @@ static ssize_t isoc_alt_store(struct device *dev,
> >       int alt;
> >       int ret;
> >
> > +     if (!data->hdev)
> > +             return -ENODEV;
> > +
> >       if (kstrtoint(buf, 10, &alt))
> >               return -EINVAL;
> >
> > @@ -3702,6 +3707,36 @@ static ssize_t isoc_alt_store(struct device *dev,
> >
> >  static DEVICE_ATTR_RW(isoc_alt);
> >
> > +static struct attribute *btusb_sysfs_attrs[] = {
> > +     NULL,
> > +};
> > +ATTRIBUTE_GROUPS(btusb_sysfs);
> > +
> > +static void btusb_sysfs_release(struct device *dev)
> > +{
> > +     struct btusb_data *data = dev_get_drvdata(dev);
>
> That feels wrong, it's embedded in the device, not pointed to by the
> device.  So this should be a container_of() call, right?

Thanks for the feedback. So now rather than dev_set_drvdata() +
dev_get_drvdata() I am going to use container_of() only.

>
> > +
> > +     kfree(data);
> > +}
> > +
> > +static const struct device_type btusb_sysfs = {
> > +     .name    = "btusb",
> > +     .release = btusb_sysfs_release,
> > +     .groups  = btusb_sysfs_groups,
> > +};
> > +
> > +static struct attribute *btusb_sysfs_isoc_alt_attrs[] = {
> > +     &dev_attr_isoc_alt.attr,
> > +     NULL,
> > +};
> > +ATTRIBUTE_GROUPS(btusb_sysfs_isoc_alt);
> > +
> > +static const struct device_type btusb_sysfs_isoc_alt = {
> > +     .name    = "btusb",
> > +     .release = btusb_sysfs_release,
> > +     .groups  = btusb_sysfs_isoc_alt_groups,
> > +};
> > +
> >  static int btusb_probe(struct usb_interface *intf,
> >                      const struct usb_device_id *id)
> >  {
> > @@ -3743,7 +3778,7 @@ static int btusb_probe(struct usb_interface *intf,
> >                       return -ENODEV;
> >       }
> >
> > -     data = devm_kzalloc(&intf->dev, sizeof(*data), GFP_KERNEL);
> > +     data = kzalloc(sizeof(*data), GFP_KERNEL);
> >       if (!data)
> >               return -ENOMEM;
> >
> > @@ -3766,8 +3801,10 @@ static int btusb_probe(struct usb_interface *intf,
> >               }
> >       }
> >
> > -     if (!data->intr_ep || !data->bulk_tx_ep || !data->bulk_rx_ep)
> > -             return -ENODEV;
> > +     if (!data->intr_ep || !data->bulk_tx_ep || !data->bulk_rx_ep) {
> > +             err = -ENODEV;
> > +             goto out_free_data;
> > +     }
> >
> >       if (id->driver_info & BTUSB_AMP) {
> >               data->cmdreq_type = USB_TYPE_CLASS | 0x01;
> > @@ -3821,16 +3858,47 @@ static int btusb_probe(struct usb_interface *intf,
> >
> >       data->recv_acl = hci_recv_frame;
> >
> > +     if (id->driver_info & BTUSB_AMP) {
> > +             /* AMP controllers do not support SCO packets */
> > +             data->isoc = NULL;
> > +     } else {
> > +             /* Interface orders are hardcoded in the specification */
> > +             data->isoc = usb_ifnum_to_if(data->udev, ifnum_base + 1);
> > +             data->isoc_ifnum = ifnum_base + 1;
> > +     }
> > +
> > +     if (id->driver_info & BTUSB_BROKEN_ISOC)
> > +             data->isoc = NULL;
> > +
> > +     /* Init a dev for btusb. The attr depends on the support of isoc. */
> > +     if (data->isoc)
> > +             data->dev.type = &btusb_sysfs_isoc_alt;
> > +     else
> > +             data->dev.type = &btusb_sysfs;
>
> When walking the class, are you sure you check for the proper types now?
> Does anyone walk all of the class devices anywhere?

Sorry I don't quite understand. What does walk mean in this case? Is
it the user space program walks the /sys/class/bluetooth?

>
> > +     data->dev.class = &bt_class;
> > +     data->dev.parent = &intf->dev;
> > +
> > +     err = dev_set_name(&data->dev, "btusb%s", dev_name(&intf->dev));
>
> what does this name look like in a real system?  squashing them together
> feels wrong, why is 'btusb' needed here at all?

Below is the Bluetooth class layout that could be like after this patch.
I guess we better keep the "btusb" or "usb" prefix so it's less
confusing when more transports (UART, PCIe, etc) are added.

# ls -l /sys/class/bluetooth
total 0
lrwxrwxrwx. 1 root root 0 Feb 17 15:23 btusb2-1.5:1.0 ->
../../devices/platform/soc/16700000.usb/usb2/2-1/2-1.5/2-1.5:1.0/bluetooth/btusb2-1.5:1.0
lrwxrwxrwx. 1 root root 0 Feb 17 15:23 hci0 ->
../../devices/platform/soc/16700000.usb/usb2/2-1/2-1.5/2-1.5:1.0/bluetooth/btusb2-1.5:1.0/hci0

>
> > +     if (err)
> > +             goto out_free_data;
> > +
> > +     dev_set_drvdata(&data->dev, data);
> > +     err = device_register(&data->dev);
> > +     if (err < 0)
> > +             goto out_put_sysfs;
> > +
> >       hdev = hci_alloc_dev_priv(priv_size);
> > -     if (!hdev)
> > -             return -ENOMEM;
> > +     if (!hdev) {
> > +             err = -ENOMEM;
> > +             goto out_free_sysfs;
> > +     }
> >
> >       hdev->bus = HCI_USB;
> >       hci_set_drvdata(hdev, data);
> >
> >       data->hdev = hdev;
> >
> > -     SET_HCIDEV_DEV(hdev, &intf->dev);
> > +     SET_HCIDEV_DEV(hdev, &data->dev);
> >
> >       reset_gpio = gpiod_get_optional(&data->udev->dev, "reset",
> >                                       GPIOD_OUT_LOW);
> > @@ -3969,15 +4037,6 @@ static int btusb_probe(struct usb_interface *intf,
> >               hci_set_msft_opcode(hdev, 0xFD70);
> >       }
> >
> > -     if (id->driver_info & BTUSB_AMP) {
> > -             /* AMP controllers do not support SCO packets */
> > -             data->isoc = NULL;
> > -     } else {
> > -             /* Interface orders are hardcoded in the specification */
> > -             data->isoc = usb_ifnum_to_if(data->udev, ifnum_base + 1);
> > -             data->isoc_ifnum = ifnum_base + 1;
> > -     }
> > -
> >       if (IS_ENABLED(CONFIG_BT_HCIBTUSB_RTL) &&
> >           (id->driver_info & BTUSB_REALTEK)) {
> >               btrtl_set_driver_name(hdev, btusb_driver.name);
> > @@ -4010,9 +4069,6 @@ static int btusb_probe(struct usb_interface *intf,
> >                       set_bit(HCI_QUIRK_FIXUP_BUFFER_SIZE, &hdev->quirks);
> >       }
> >
> > -     if (id->driver_info & BTUSB_BROKEN_ISOC)
> > -             data->isoc = NULL;
> > -
> >       if (id->driver_info & BTUSB_WIDEBAND_SPEECH)
> >               set_bit(HCI_QUIRK_WIDEBAND_SPEECH_SUPPORTED, &hdev->quirks);
> >
> > @@ -4065,10 +4121,6 @@ static int btusb_probe(struct usb_interface *intf,
> >                                                data->isoc, data);
> >               if (err < 0)
> >                       goto out_free_dev;
> > -
> > -             err = device_create_file(&intf->dev, &dev_attr_isoc_alt);
>
> You have now moved the file, are you sure you don't also need to update
> the documentation?

There's no documentation for this attr so far, and this patch aims to
add the doc.

>
>
> > -             if (err)
> > -                     goto out_free_dev;
> >       }
> >
> >       if (IS_ENABLED(CONFIG_BT_HCIBTUSB_BCM) && data->diag) {
> > @@ -4099,6 +4151,16 @@ static int btusb_probe(struct usb_interface *intf,
> >       if (data->reset_gpio)
> >               gpiod_put(data->reset_gpio);
> >       hci_free_dev(hdev);
> > +
> > +out_free_sysfs:
> > +     device_del(&data->dev);
> > +
> > +out_put_sysfs:
> > +     put_device(&data->dev);
> > +     return err;
> > +
> > +out_free_data:
> > +     kfree(data);
> >       return err;
> >  }
> >
> > @@ -4115,10 +4177,8 @@ static void btusb_disconnect(struct usb_interface *intf)
> >       hdev = data->hdev;
> >       usb_set_intfdata(data->intf, NULL);
> >
> > -     if (data->isoc) {
> > -             device_remove_file(&intf->dev, &dev_attr_isoc_alt);
> > +     if (data->isoc)
> >               usb_set_intfdata(data->isoc, NULL);
> > -     }
> >
> >       if (data->diag)
> >               usb_set_intfdata(data->diag, NULL);
> > @@ -4150,6 +4210,7 @@ static void btusb_disconnect(struct usb_interface *intf)
> >               gpiod_put(data->reset_gpio);
> >
> >       hci_free_dev(hdev);
> > +     device_unregister(&data->dev);
> >  }
> >
> >  #ifdef CONFIG_PM
> > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> > index 05919848ea95..776dd6183509 100644
> > --- a/include/net/bluetooth/hci_core.h
> > +++ b/include/net/bluetooth/hci_core.h
> > @@ -1843,6 +1843,7 @@ int hci_get_adv_monitor_offload_ext(struct hci_dev *hdev);
> >
> >  void hci_event_packet(struct hci_dev *hdev, struct sk_buff *skb);
> >
> > +extern const struct class bt_class;
> >  void hci_init_sysfs(struct hci_dev *hdev);
> >  void hci_conn_init_sysfs(struct hci_conn *conn);
> >  void hci_conn_add_sysfs(struct hci_conn *conn);
> > diff --git a/net/bluetooth/hci_sysfs.c b/net/bluetooth/hci_sysfs.c
> > index 041ce9adc378..aab3ffaa264c 100644
> > --- a/net/bluetooth/hci_sysfs.c
> > +++ b/net/bluetooth/hci_sysfs.c
> > @@ -6,9 +6,10 @@
> >  #include <net/bluetooth/bluetooth.h>
> >  #include <net/bluetooth/hci_core.h>
> >
> > -static const struct class bt_class = {
> > +const struct class bt_class = {
> >       .name = "bluetooth",
> >  };
> > +EXPORT_SYMBOL(bt_class);
>
> EXPORT_SYMBOL_GPL(), right?
>
> thanks,
>
> greg k-h

Greg Kroah-Hartman Feb. 17, 2025, 8:53 a.m. UTC | #5

On Mon, Feb 17, 2025 at 04:44:35PM +0800, Hsin-chen Chuang wrote:
> On Fri, Feb 14, 2025 at 7:37 PM Greg KH <[e-mail]> wrote:
> >
> > On Fri, Feb 14, 2025 at 07:16:17PM +0800, Hsin-chen Chuang wrote:
> > > From: Hsin-chen Chuang <[e-mail]>
> > >
> > > Expose the isoc_alt attr with device group to avoid the racing.
> > >
> > > Now we create a dev node for btusb. The isoc_alt attr belongs to it and
> > > it also becomes the parent device of hci dev.
> > >
> > > Fixes: b16b327edb4d ("Bluetooth: btusb: add sysfs attribute to control USB alt setting")
> >
> > Wait, step back, why is this commit needed if you can change the alt
> > setting already today through usbfs/libusb without needing to mess with
> > the bluetooth stack at all?
> 
> In short: We want to configure the alternate settings without
> detaching the btusb driver, while detaching seems necessary for
> libusb_set_interface_alt_setting to work (Please correct me if I'm
> wrong!)

I think changing the alternate setting should work using usbfs as you
would send that command to the device, not the interface, so the driver
bound to the existing interface would not need to be removed.

Try it out and see yourself to verify this before you continue down any
of this.  There's no need to use libfs for just a single usbfs command,
right?

thanks,

greg k-h

Greg Kroah-Hartman Feb. 17, 2025, 8:55 a.m. UTC | #6

On Mon, Feb 17, 2025 at 04:44:35PM +0800, Hsin-chen Chuang wrote:
> On Fri, Feb 14, 2025 at 7:37 PM Greg KH <[e-mail]> wrote:
> >
> > On Fri, Feb 14, 2025 at 07:16:17PM +0800, Hsin-chen Chuang wrote:
> > > From: Hsin-chen Chuang <[e-mail]>
> > >
> > > Expose the isoc_alt attr with device group to avoid the racing.
> > >
> > > Now we create a dev node for btusb. The isoc_alt attr belongs to it and
> > > it also becomes the parent device of hci dev.
> > >
> > > Fixes: b16b327edb4d ("Bluetooth: btusb: add sysfs attribute to control USB alt setting")
> >
> > Wait, step back, why is this commit needed if you can change the alt
> > setting already today through usbfs/libusb without needing to mess with
> > the bluetooth stack at all?
> 
> In short: We want to configure the alternate settings without
> detaching the btusb driver, while detaching seems necessary for
> libusb_set_interface_alt_setting to work (Please correct me if I'm
> wrong!)
> 
> Background:
> The Bluetooth Core Specification defines a protocol for the operating
> system to communicate with a Bluetooth chipset, called HCI (Host
> Controller Interface) (Host=OS, Controller=chipset).
> We could say the main purpose of the Linux Bluetooth drivers is to set
> up and get the HCI ready for the "upper layer" to use.
> 
> Who could be the "upper layer" then? There are mainly 2: "Control" and
> "User" channels.
> Linux has its default Bluetooth stack, BlueZ, which is splitted into 2
> parts: the kernel space and the user space. The kernel space part
> provides an abstracted Bluetooth API called MGMT, and is exposed
> through the Bluetooth HCI socket "Control" channel.
> On the other hand Linux also exposes the Bluetooth HCI socket "User"
> channel, allowing the user space APPs to send/receive the HCI packets
> directly to/from the chipset. Google's products (Android, ChromeOS,
> etc) use this channel.
> 
> Now why this patch?
> It's because the Bluetooth spec defines something specific to USB
> transport: A USB Bluetooth chipset must/should support these alternate
> settings; When transferring this type of the Audio data this alt must
> be used, bla bla bla...
> The Control channel handles this in the kernel part. However, the
> applications built on top of the User channel are unable to configure
> the alt setting, and I'd like to add the support through sysfs.

So the "problem" is that Google doesn't want to use BlueZ, which is
fine, you do you :)

But how does BlueZ handle this same problem today?  What api to the
kernel does it use to change the interface that you can't also do with
your "BlueZ replacement"?

Surely this isn't a new issue suddenly, but if it is, it needs to be
solved so BOTH userspace stacks can handle it.

thanks,

greg k-h

Hsin-chen Chuang Feb. 17, 2025, 9:17 a.m. UTC | #7

On Mon, Feb 17, 2025 at 4:53 PM Greg KH <[e-mail]> wrote:
>
> On Mon, Feb 17, 2025 at 04:44:35PM +0800, Hsin-chen Chuang wrote:
> > On Fri, Feb 14, 2025 at 7:37 PM Greg KH <[e-mail]> wrote:
> > >
> > > On Fri, Feb 14, 2025 at 07:16:17PM +0800, Hsin-chen Chuang wrote:
> > > > From: Hsin-chen Chuang <[e-mail]>
> > > >
> > > > Expose the isoc_alt attr with device group to avoid the racing.
> > > >
> > > > Now we create a dev node for btusb. The isoc_alt attr belongs to it and
> > > > it also becomes the parent device of hci dev.
> > > >
> > > > Fixes: b16b327edb4d ("Bluetooth: btusb: add sysfs attribute to control USB alt setting")
> > >
> > > Wait, step back, why is this commit needed if you can change the alt
> > > setting already today through usbfs/libusb without needing to mess with
> > > the bluetooth stack at all?
> >
> > In short: We want to configure the alternate settings without
> > detaching the btusb driver, while detaching seems necessary for
> > libusb_set_interface_alt_setting to work (Please correct me if I'm
> > wrong!)
>
> I think changing the alternate setting should work using usbfs as you
> would send that command to the device, not the interface, so the driver
> bound to the existing interface would not need to be removed.
>
> Try it out and see yourself to verify this before you continue down any
> of this.  There's no need to use libfs for just a single usbfs command,
> right?

I will give it a try. Great thanks for this suggestion!

>
> thanks,
>
> greg k-h


On Mon, Feb 17, 2025 at 4:55 PM Greg KH <[e-mail]> wrote:
>
> On Mon, Feb 17, 2025 at 04:44:35PM +0800, Hsin-chen Chuang wrote:
> > On Fri, Feb 14, 2025 at 7:37 PM Greg KH <[e-mail]> wrote:
> > >
> > > On Fri, Feb 14, 2025 at 07:16:17PM +0800, Hsin-chen Chuang wrote:
> > > > From: Hsin-chen Chuang <[e-mail]>
> > > >
> > > > Expose the isoc_alt attr with device group to avoid the racing.
> > > >
> > > > Now we create a dev node for btusb. The isoc_alt attr belongs to it and
> > > > it also becomes the parent device of hci dev.
> > > >
> > > > Fixes: b16b327edb4d ("Bluetooth: btusb: add sysfs attribute to control USB alt setting")
> > >
> > > Wait, step back, why is this commit needed if you can change the alt
> > > setting already today through usbfs/libusb without needing to mess with
> > > the bluetooth stack at all?
> >
> > In short: We want to configure the alternate settings without
> > detaching the btusb driver, while detaching seems necessary for
> > libusb_set_interface_alt_setting to work (Please correct me if I'm
> > wrong!)
> >
> > Background:
> > The Bluetooth Core Specification defines a protocol for the operating
> > system to communicate with a Bluetooth chipset, called HCI (Host
> > Controller Interface) (Host=OS, Controller=chipset).
> > We could say the main purpose of the Linux Bluetooth drivers is to set
> > up and get the HCI ready for the "upper layer" to use.
> >
> > Who could be the "upper layer" then? There are mainly 2: "Control" and
> > "User" channels.
> > Linux has its default Bluetooth stack, BlueZ, which is splitted into 2
> > parts: the kernel space and the user space. The kernel space part
> > provides an abstracted Bluetooth API called MGMT, and is exposed
> > through the Bluetooth HCI socket "Control" channel.
> > On the other hand Linux also exposes the Bluetooth HCI socket "User"
> > channel, allowing the user space APPs to send/receive the HCI packets
> > directly to/from the chipset. Google's products (Android, ChromeOS,
> > etc) use this channel.
> >
> > Now why this patch?
> > It's because the Bluetooth spec defines something specific to USB
> > transport: A USB Bluetooth chipset must/should support these alternate
> > settings; When transferring this type of the Audio data this alt must
> > be used, bla bla bla...
> > The Control channel handles this in the kernel part. However, the
> > applications built on top of the User channel are unable to configure
> > the alt setting, and I'd like to add the support through sysfs.
>
> So the "problem" is that Google doesn't want to use BlueZ, which is
> fine, you do you :)
>
> But how does BlueZ handle this same problem today?  What api to the
> kernel does it use to change the interface that you can't also do with
> your "BlueZ replacement"?
>
> Surely this isn't a new issue suddenly, but if it is, it needs to be
> solved so BOTH userspace stacks can handle it.

BlueZ handles that in their MGMT command, that is, through Control
channel -> BlueZ kernel space code -> driver callbacks.
Once a Bluetooth chipset is opened with the User channel, it can't be
used with the Control channel simultaneously, and vice versa.

>
> thanks,
>
> greg k-h

Hsin-chen Chuang Feb. 18, 2025, 4:24 a.m. UTC | #8

Hi Greg,

On Mon, Feb 17, 2025 at 4:53 PM Greg KH <[e-mail]> wrote:
>
> On Mon, Feb 17, 2025 at 04:44:35PM +0800, Hsin-chen Chuang wrote:
> > On Fri, Feb 14, 2025 at 7:37 PM Greg KH <[e-mail]> wrote:
> > >
> > > On Fri, Feb 14, 2025 at 07:16:17PM +0800, Hsin-chen Chuang wrote:
> > > > From: Hsin-chen Chuang <[e-mail]>
> > > >
> > > > Expose the isoc_alt attr with device group to avoid the racing.
> > > >
> > > > Now we create a dev node for btusb. The isoc_alt attr belongs to it and
> > > > it also becomes the parent device of hci dev.
> > > >
> > > > Fixes: b16b327edb4d ("Bluetooth: btusb: add sysfs attribute to control USB alt setting")
> > >
> > > Wait, step back, why is this commit needed if you can change the alt
> > > setting already today through usbfs/libusb without needing to mess with
> > > the bluetooth stack at all?
> >
> > In short: We want to configure the alternate settings without
> > detaching the btusb driver, while detaching seems necessary for
> > libusb_set_interface_alt_setting to work (Please correct me if I'm
> > wrong!)
>
> I think changing the alternate setting should work using usbfs as you
> would send that command to the device, not the interface, so the driver
> bound to the existing interface would not need to be removed.

I thought USBDEVFS_SETINTERFACE was the right command to begin with,
but it seems not working in this case.
The command itself attempts to claim the interface, but the interface
is already claimed by btusb so it failed with Device or resource busy

drivers/usb/core/devio.c:
  USBDEVFS_SETINTERFACE -> proc_setintf -> checkintf -> claimintf

Greg Kroah-Hartman Feb. 18, 2025, 8:23 a.m. UTC | #9

On Tue, Feb 18, 2025 at 12:24:07PM +0800, Hsin-chen Chuang wrote:
> Hi Greg,
> 
> On Mon, Feb 17, 2025 at 4:53 PM Greg KH <[e-mail]> wrote:
> >
> > On Mon, Feb 17, 2025 at 04:44:35PM +0800, Hsin-chen Chuang wrote:
> > > On Fri, Feb 14, 2025 at 7:37 PM Greg KH <[e-mail]> wrote:
> > > >
> > > > On Fri, Feb 14, 2025 at 07:16:17PM +0800, Hsin-chen Chuang wrote:
> > > > > From: Hsin-chen Chuang <[e-mail]>
> > > > >
> > > > > Expose the isoc_alt attr with device group to avoid the racing.
> > > > >
> > > > > Now we create a dev node for btusb. The isoc_alt attr belongs to it and
> > > > > it also becomes the parent device of hci dev.
> > > > >
> > > > > Fixes: b16b327edb4d ("Bluetooth: btusb: add sysfs attribute to control USB alt setting")
> > > >
> > > > Wait, step back, why is this commit needed if you can change the alt
> > > > setting already today through usbfs/libusb without needing to mess with
> > > > the bluetooth stack at all?
> > >
> > > In short: We want to configure the alternate settings without
> > > detaching the btusb driver, while detaching seems necessary for
> > > libusb_set_interface_alt_setting to work (Please correct me if I'm
> > > wrong!)
> >
> > I think changing the alternate setting should work using usbfs as you
> > would send that command to the device, not the interface, so the driver
> > bound to the existing interface would not need to be removed.
> 
> I thought USBDEVFS_SETINTERFACE was the right command to begin with,
> but it seems not working in this case.
> The command itself attempts to claim the interface, but the interface
> is already claimed by btusb so it failed with Device or resource busy
> 
> drivers/usb/core/devio.c:
>   USBDEVFS_SETINTERFACE -> proc_setintf -> checkintf -> claimintf

Ah, ok, thanks for checking.  So as you control this device, why not
just disconnect it, change the setting, and then reconnect it?

Also, see my other review comment, how does BlueZ do this today?

thanks,

greg k-h

Hsin-chen Chuang Feb. 18, 2025, 8:57 a.m. UTC | #10

Hi Greg,

On Tue, Feb 18, 2025 at 4:23 PM Greg KH <[e-mail]> wrote:
>
> On Tue, Feb 18, 2025 at 12:24:07PM +0800, Hsin-chen Chuang wrote:
> > Hi Greg,
> >
> > On Mon, Feb 17, 2025 at 4:53 PM Greg KH <[e-mail]> wrote:
> > >
> > > On Mon, Feb 17, 2025 at 04:44:35PM +0800, Hsin-chen Chuang wrote:
> > > > On Fri, Feb 14, 2025 at 7:37 PM Greg KH <[e-mail]> wrote:
> > > > >
> > > > > On Fri, Feb 14, 2025 at 07:16:17PM +0800, Hsin-chen Chuang wrote:
> > > > > > From: Hsin-chen Chuang <[e-mail]>
> > > > > >
> > > > > > Expose the isoc_alt attr with device group to avoid the racing.
> > > > > >
> > > > > > Now we create a dev node for btusb. The isoc_alt attr belongs to it and
> > > > > > it also becomes the parent device of hci dev.
> > > > > >
> > > > > > Fixes: b16b327edb4d ("Bluetooth: btusb: add sysfs attribute to control USB alt setting")
> > > > >
> > > > > Wait, step back, why is this commit needed if you can change the alt
> > > > > setting already today through usbfs/libusb without needing to mess with
> > > > > the bluetooth stack at all?
> > > >
> > > > In short: We want to configure the alternate settings without
> > > > detaching the btusb driver, while detaching seems necessary for
> > > > libusb_set_interface_alt_setting to work (Please correct me if I'm
> > > > wrong!)
> > >
> > > I think changing the alternate setting should work using usbfs as you
> > > would send that command to the device, not the interface, so the driver
> > > bound to the existing interface would not need to be removed.
> >
> > I thought USBDEVFS_SETINTERFACE was the right command to begin with,
> > but it seems not working in this case.
> > The command itself attempts to claim the interface, but the interface
> > is already claimed by btusb so it failed with Device or resource busy
> >
> > drivers/usb/core/devio.c:
> >   USBDEVFS_SETINTERFACE -> proc_setintf -> checkintf -> claimintf
>
> Ah, ok, thanks for checking.  So as you control this device, why not
> just disconnect it, change the setting, and then reconnect it?

After dis/reconnecting, a Bluetooth chipset would lose all its state:
Existing connections/scanners/advertisers are all dropped.
This is as bad as (just an analogy) "Whenever you access a http web
page, you need to bring your ethernet interface down and up, and after
the page is downloaded, do that again".

>
> Also, see my other review comment, how does BlueZ do this today?

BlueZ handles that in their MGMT command, that is, through Control
channel -> BlueZ kernel space code -> driver callbacks.
Once a Bluetooth chipset is opened with the User channel, it can't be
used with the Control channel simultaneously, and vice versa.

Greg Kroah-Hartman Feb. 18, 2025, 9:21 a.m. UTC | #11

On Tue, Feb 18, 2025 at 04:57:38PM +0800, Hsin-chen Chuang wrote:
> Hi Greg,
> 
> On Tue, Feb 18, 2025 at 4:23 PM Greg KH <[e-mail]> wrote:
> >
> > On Tue, Feb 18, 2025 at 12:24:07PM +0800, Hsin-chen Chuang wrote:
> > > Hi Greg,
> > >
> > > On Mon, Feb 17, 2025 at 4:53 PM Greg KH <[e-mail]> wrote:
> > > >
> > > > On Mon, Feb 17, 2025 at 04:44:35PM +0800, Hsin-chen Chuang wrote:
> > > > > On Fri, Feb 14, 2025 at 7:37 PM Greg KH <[e-mail]> wrote:
> > > > > >
> > > > > > On Fri, Feb 14, 2025 at 07:16:17PM +0800, Hsin-chen Chuang wrote:
> > > > > > > From: Hsin-chen Chuang <[e-mail]>
> > > > > > >
> > > > > > > Expose the isoc_alt attr with device group to avoid the racing.
> > > > > > >
> > > > > > > Now we create a dev node for btusb. The isoc_alt attr belongs to it and
> > > > > > > it also becomes the parent device of hci dev.
> > > > > > >
> > > > > > > Fixes: b16b327edb4d ("Bluetooth: btusb: add sysfs attribute to control USB alt setting")
> > > > > >
> > > > > > Wait, step back, why is this commit needed if you can change the alt
> > > > > > setting already today through usbfs/libusb without needing to mess with
> > > > > > the bluetooth stack at all?
> > > > >
> > > > > In short: We want to configure the alternate settings without
> > > > > detaching the btusb driver, while detaching seems necessary for
> > > > > libusb_set_interface_alt_setting to work (Please correct me if I'm
> > > > > wrong!)
> > > >
> > > > I think changing the alternate setting should work using usbfs as you
> > > > would send that command to the device, not the interface, so the driver
> > > > bound to the existing interface would not need to be removed.
> > >
> > > I thought USBDEVFS_SETINTERFACE was the right command to begin with,
> > > but it seems not working in this case.
> > > The command itself attempts to claim the interface, but the interface
> > > is already claimed by btusb so it failed with Device or resource busy
> > >
> > > drivers/usb/core/devio.c:
> > >   USBDEVFS_SETINTERFACE -> proc_setintf -> checkintf -> claimintf
> >
> > Ah, ok, thanks for checking.  So as you control this device, why not
> > just disconnect it, change the setting, and then reconnect it?
> 
> After dis/reconnecting, a Bluetooth chipset would lose all its state:
> Existing connections/scanners/advertisers are all dropped.

If you are changing the alternate USB configuration, all state should be
dropped, right?  If not, huh how does the device know to keep that
state?

> This is as bad as (just an analogy) "Whenever you access a http web
> page, you need to bring your ethernet interface down and up, and after
> the page is downloaded, do that again".

Your ethernet interface does not contain state like this, we handle
chainging IP addresses and devices all the time, so perhaps wrong
analogy :)

> > Also, see my other review comment, how does BlueZ do this today?
> 
> BlueZ handles that in their MGMT command, that is, through Control
> channel -> BlueZ kernel space code -> driver callbacks.
> Once a Bluetooth chipset is opened with the User channel, it can't be
> used with the Control channel simultaneously, and vice versa.

So why not use that same control channel in your code?  Why are you
reinventing a new control channel for something that is obviously there
already?

So in short, what's preventing you from using the same exact driver
callbacks, OR the same exact kernel api.  Surely you all are not
replacing all of the in-kernel BlueZ code with an external kernel driver
just for this, right?  If so, that's not ok at all.

thanks,

greg k-h

Hsin-chen Chuang Feb. 18, 2025, 10:01 a.m. UTC | #12

Hi Greg,

On Tue, Feb 18, 2025 at 5:21 PM Greg KH <[e-mail]> wrote:
>
> On Tue, Feb 18, 2025 at 04:57:38PM +0800, Hsin-chen Chuang wrote:
> > Hi Greg,
> >
> > On Tue, Feb 18, 2025 at 4:23 PM Greg KH <[e-mail]> wrote:
> > >
> > > On Tue, Feb 18, 2025 at 12:24:07PM +0800, Hsin-chen Chuang wrote:
> > > > Hi Greg,
> > > >
> > > > On Mon, Feb 17, 2025 at 4:53 PM Greg KH <[e-mail]> wrote:
> > > > >
> > > > > On Mon, Feb 17, 2025 at 04:44:35PM +0800, Hsin-chen Chuang wrote:
> > > > > > On Fri, Feb 14, 2025 at 7:37 PM Greg KH <[e-mail]> wrote:
> > > > > > >
> > > > > > > On Fri, Feb 14, 2025 at 07:16:17PM +0800, Hsin-chen Chuang wrote:
> > > > > > > > From: Hsin-chen Chuang <[e-mail]>
> > > > > > > >
> > > > > > > > Expose the isoc_alt attr with device group to avoid the racing.
> > > > > > > >
> > > > > > > > Now we create a dev node for btusb. The isoc_alt attr belongs to it and
> > > > > > > > it also becomes the parent device of hci dev.
> > > > > > > >
> > > > > > > > Fixes: b16b327edb4d ("Bluetooth: btusb: add sysfs attribute to control USB alt setting")
> > > > > > >
> > > > > > > Wait, step back, why is this commit needed if you can change the alt
> > > > > > > setting already today through usbfs/libusb without needing to mess with
> > > > > > > the bluetooth stack at all?
> > > > > >
> > > > > > In short: We want to configure the alternate settings without
> > > > > > detaching the btusb driver, while detaching seems necessary for
> > > > > > libusb_set_interface_alt_setting to work (Please correct me if I'm
> > > > > > wrong!)
> > > > >
> > > > > I think changing the alternate setting should work using usbfs as you
> > > > > would send that command to the device, not the interface, so the driver
> > > > > bound to the existing interface would not need to be removed.
> > > >
> > > > I thought USBDEVFS_SETINTERFACE was the right command to begin with,
> > > > but it seems not working in this case.
> > > > The command itself attempts to claim the interface, but the interface
> > > > is already claimed by btusb so it failed with Device or resource busy
> > > >
> > > > drivers/usb/core/devio.c:
> > > >   USBDEVFS_SETINTERFACE -> proc_setintf -> checkintf -> claimintf
> > >
> > > Ah, ok, thanks for checking.  So as you control this device, why not
> > > just disconnect it, change the setting, and then reconnect it?
> >
> > After dis/reconnecting, a Bluetooth chipset would lose all its state:
> > Existing connections/scanners/advertisers are all dropped.
>
> If you are changing the alternate USB configuration, all state should be
> dropped, right?  If not, huh how does the device know to keep that
> state?

No, the Bluetooth chip doesn't drop any info when the alt is changed.
It only affects the data transfer bandwidth on that interface.

>
> > This is as bad as (just an analogy) "Whenever you access a http web
> > page, you need to bring your ethernet interface down and up, and after
> > the page is downloaded, do that again".
>
> Your ethernet interface does not contain state like this, we handle
> chainging IP addresses and devices all the time, so perhaps wrong
> analogy :)
>
> > > Also, see my other review comment, how does BlueZ do this today?
> >
> > BlueZ handles that in their MGMT command, that is, through Control
> > channel -> BlueZ kernel space code -> driver callbacks.
> > Once a Bluetooth chipset is opened with the User channel, it can't be
> > used with the Control channel simultaneously, and vice versa.
>
> So why not use that same control channel in your code?  Why are you

Because we're using the User channel, and they can't be used at the same time.

> reinventing a new control channel for something that is obviously there
> already?

Not quite the same as "reinventing". The Control channel command does
much more than just setting the alt; It just doesn't work with the
User channel.

>
> So in short, what's preventing you from using the same exact driver
> callbacks, OR the same exact kernel api.  Surely you all are not

The answer is the same as the above. This feature is missing in the
User channel, and I'm completing it with this patch.

> replacing all of the in-kernel BlueZ code with an external kernel driver
> just for this, right?  If so, that's not ok at all.

Sorry I don't quite get it. What do you mean by the external kernel driver?

Greg Kroah-Hartman Feb. 18, 2025, 10:56 a.m. UTC | #13

On Tue, Feb 18, 2025 at 06:01:42PM +0800, Hsin-chen Chuang wrote:
> Hi Greg,
> 
> On Tue, Feb 18, 2025 at 5:21 PM Greg KH <[e-mail]> wrote:
> >
> > On Tue, Feb 18, 2025 at 04:57:38PM +0800, Hsin-chen Chuang wrote:
> > > Hi Greg,
> > >
> > > On Tue, Feb 18, 2025 at 4:23 PM Greg KH <[e-mail]> wrote:
> > > >
> > > > On Tue, Feb 18, 2025 at 12:24:07PM +0800, Hsin-chen Chuang wrote:
> > > > > Hi Greg,
> > > > >
> > > > > On Mon, Feb 17, 2025 at 4:53 PM Greg KH <[e-mail]> wrote:
> > > > > >
> > > > > > On Mon, Feb 17, 2025 at 04:44:35PM +0800, Hsin-chen Chuang wrote:
> > > > > > > On Fri, Feb 14, 2025 at 7:37 PM Greg KH <[e-mail]> wrote:
> > > > > > > >
> > > > > > > > On Fri, Feb 14, 2025 at 07:16:17PM +0800, Hsin-chen Chuang wrote:
> > > > > > > > > From: Hsin-chen Chuang <[e-mail]>
> > > > > > > > >
> > > > > > > > > Expose the isoc_alt attr with device group to avoid the racing.
> > > > > > > > >
> > > > > > > > > Now we create a dev node for btusb. The isoc_alt attr belongs to it and
> > > > > > > > > it also becomes the parent device of hci dev.
> > > > > > > > >
> > > > > > > > > Fixes: b16b327edb4d ("Bluetooth: btusb: add sysfs attribute to control USB alt setting")
> > > > > > > >
> > > > > > > > Wait, step back, why is this commit needed if you can change the alt
> > > > > > > > setting already today through usbfs/libusb without needing to mess with
> > > > > > > > the bluetooth stack at all?
> > > > > > >
> > > > > > > In short: We want to configure the alternate settings without
> > > > > > > detaching the btusb driver, while detaching seems necessary for
> > > > > > > libusb_set_interface_alt_setting to work (Please correct me if I'm
> > > > > > > wrong!)
> > > > > >
> > > > > > I think changing the alternate setting should work using usbfs as you
> > > > > > would send that command to the device, not the interface, so the driver
> > > > > > bound to the existing interface would not need to be removed.
> > > > >
> > > > > I thought USBDEVFS_SETINTERFACE was the right command to begin with,
> > > > > but it seems not working in this case.
> > > > > The command itself attempts to claim the interface, but the interface
> > > > > is already claimed by btusb so it failed with Device or resource busy
> > > > >
> > > > > drivers/usb/core/devio.c:
> > > > >   USBDEVFS_SETINTERFACE -> proc_setintf -> checkintf -> claimintf
> > > >
> > > > Ah, ok, thanks for checking.  So as you control this device, why not
> > > > just disconnect it, change the setting, and then reconnect it?
> > >
> > > After dis/reconnecting, a Bluetooth chipset would lose all its state:
> > > Existing connections/scanners/advertisers are all dropped.
> >
> > If you are changing the alternate USB configuration, all state should be
> > dropped, right?  If not, huh how does the device know to keep that
> > state?
> 
> No, the Bluetooth chip doesn't drop any info when the alt is changed.
> It only affects the data transfer bandwidth on that interface.
> 
> >
> > > This is as bad as (just an analogy) "Whenever you access a http web
> > > page, you need to bring your ethernet interface down and up, and after
> > > the page is downloaded, do that again".
> >
> > Your ethernet interface does not contain state like this, we handle
> > chainging IP addresses and devices all the time, so perhaps wrong
> > analogy :)
> >
> > > > Also, see my other review comment, how does BlueZ do this today?
> > >
> > > BlueZ handles that in their MGMT command, that is, through Control
> > > channel -> BlueZ kernel space code -> driver callbacks.
> > > Once a Bluetooth chipset is opened with the User channel, it can't be
> > > used with the Control channel simultaneously, and vice versa.
> >
> > So why not use that same control channel in your code?  Why are you
> 
> Because we're using the User channel, and they can't be used at the same time.

This doesn't make sense.  Either BlueZ has this same problem, or it
doesn't.  As you say it does not, then again, why can't you use the
exact same user/kernel api to achieve this?

The user/kernel api is "fixed" right now, if you wish to replace the
userspace side of the BlueZ code with your own, then you should/must use
that same user/kernel api.  Don't go adding duplicate interfaces please.

> > reinventing a new control channel for something that is obviously there
> > already?
> 
> Not quite the same as "reinventing". The Control channel command does
> much more than just setting the alt; It just doesn't work with the
> User channel.
> 
> >
> > So in short, what's preventing you from using the same exact driver
> > callbacks, OR the same exact kernel api.  Surely you all are not
> 
> The answer is the same as the above. This feature is missing in the
> User channel, and I'm completing it with this patch.

Again, that seems to be your userspace's issue, not the kernel's.  Just
use the same api that bluez uses here.

> > replacing all of the in-kernel BlueZ code with an external kernel driver
> > just for this, right?  If so, that's not ok at all.
> 
> Sorry I don't quite get it. What do you mean by the external kernel driver?

You said you are not using the bluez kernel code, right?  So you must
have some kernel code to implement this instead for the same
functionality.  Otherwise you are using the bluez kernel api here.

Again, just use the same api please, don't go adding new one-off apis
through sysfs for this when it is not needed.

I'll also step back further and say, why not use bluez?  What is so
wrong with that that you all need a totally different bluetooth stack?
Why not just fix the bluez code for anything that is currently missing
or lacking there that required you to write a new one.

And yes, I know the inclination of Android to constantly rewrite the
bluetooth stack, it's on the what, third or fourth iteration already?
What's to guarantee that this really will be the last one?  :)

thanks,

greg k-h

Hsin-chen Chuang Feb. 18, 2025, 12:24 p.m. UTC | #14

Hi Greg,

On Tue, Feb 18, 2025 at 6:56 PM Greg KH <[e-mail]> wrote:
>
> On Tue, Feb 18, 2025 at 06:01:42PM +0800, Hsin-chen Chuang wrote:
> > Hi Greg,
> >
> > On Tue, Feb 18, 2025 at 5:21 PM Greg KH <[e-mail]> wrote:
> > >
> > > On Tue, Feb 18, 2025 at 04:57:38PM +0800, Hsin-chen Chuang wrote:
> > > > Hi Greg,
> > > >
> > > > On Tue, Feb 18, 2025 at 4:23 PM Greg KH <[e-mail]> wrote:
> > > > >
> > > > > On Tue, Feb 18, 2025 at 12:24:07PM +0800, Hsin-chen Chuang wrote:
> > > > > > Hi Greg,
> > > > > >
> > > > > > On Mon, Feb 17, 2025 at 4:53 PM Greg KH <[e-mail]> wrote:
> > > > > > >
> > > > > > > On Mon, Feb 17, 2025 at 04:44:35PM +0800, Hsin-chen Chuang wrote:
> > > > > > > > On Fri, Feb 14, 2025 at 7:37 PM Greg KH <[e-mail]> wrote:
> > > > > > > > >
> > > > > > > > > On Fri, Feb 14, 2025 at 07:16:17PM +0800, Hsin-chen Chuang wrote:
> > > > > > > > > > From: Hsin-chen Chuang <[e-mail]>
> > > > > > > > > >
> > > > > > > > > > Expose the isoc_alt attr with device group to avoid the racing.
> > > > > > > > > >
> > > > > > > > > > Now we create a dev node for btusb. The isoc_alt attr belongs to it and
> > > > > > > > > > it also becomes the parent device of hci dev.
> > > > > > > > > >
> > > > > > > > > > Fixes: b16b327edb4d ("Bluetooth: btusb: add sysfs attribute to control USB alt setting")
> > > > > > > > >
> > > > > > > > > Wait, step back, why is this commit needed if you can change the alt
> > > > > > > > > setting already today through usbfs/libusb without needing to mess with
> > > > > > > > > the bluetooth stack at all?
> > > > > > > >
> > > > > > > > In short: We want to configure the alternate settings without
> > > > > > > > detaching the btusb driver, while detaching seems necessary for
> > > > > > > > libusb_set_interface_alt_setting to work (Please correct me if I'm
> > > > > > > > wrong!)
> > > > > > >
> > > > > > > I think changing the alternate setting should work using usbfs as you
> > > > > > > would send that command to the device, not the interface, so the driver
> > > > > > > bound to the existing interface would not need to be removed.
> > > > > >
> > > > > > I thought USBDEVFS_SETINTERFACE was the right command to begin with,
> > > > > > but it seems not working in this case.
> > > > > > The command itself attempts to claim the interface, but the interface
> > > > > > is already claimed by btusb so it failed with Device or resource busy
> > > > > >
> > > > > > drivers/usb/core/devio.c:
> > > > > >   USBDEVFS_SETINTERFACE -> proc_setintf -> checkintf -> claimintf
> > > > >
> > > > > Ah, ok, thanks for checking.  So as you control this device, why not
> > > > > just disconnect it, change the setting, and then reconnect it?
> > > >
> > > > After dis/reconnecting, a Bluetooth chipset would lose all its state:
> > > > Existing connections/scanners/advertisers are all dropped.
> > >
> > > If you are changing the alternate USB configuration, all state should be
> > > dropped, right?  If not, huh how does the device know to keep that
> > > state?
> >
> > No, the Bluetooth chip doesn't drop any info when the alt is changed.
> > It only affects the data transfer bandwidth on that interface.
> >
> > >
> > > > This is as bad as (just an analogy) "Whenever you access a http web
> > > > page, you need to bring your ethernet interface down and up, and after
> > > > the page is downloaded, do that again".
> > >
> > > Your ethernet interface does not contain state like this, we handle
> > > chainging IP addresses and devices all the time, so perhaps wrong
> > > analogy :)
> > >
> > > > > Also, see my other review comment, how does BlueZ do this today?
> > > >
> > > > BlueZ handles that in their MGMT command, that is, through Control
> > > > channel -> BlueZ kernel space code -> driver callbacks.
> > > > Once a Bluetooth chipset is opened with the User channel, it can't be
> > > > used with the Control channel simultaneously, and vice versa.
> > >
> > > So why not use that same control channel in your code?  Why are you
> >
> > Because we're using the User channel, and they can't be used at the same time.
>
> This doesn't make sense.  Either BlueZ has this same problem, or it
> doesn't.  As you say it does not, then again, why can't you use the
> exact same user/kernel api to achieve this?
>
> The user/kernel api is "fixed" right now, if you wish to replace the
> userspace side of the BlueZ code with your own, then you should/must use
> that same user/kernel api.  Don't go adding duplicate interfaces please.

I would say the kernel provides 2 sets of the API, Control and User,
and now the User channel is missing something.
I think it makes sense to add support for it.

>
> > > reinventing a new control channel for something that is obviously there
> > > already?
> >
> > Not quite the same as "reinventing". The Control channel command does
> > much more than just setting the alt; It just doesn't work with the
> > User channel.
> >
> > >
> > > So in short, what's preventing you from using the same exact driver
> > > callbacks, OR the same exact kernel api.  Surely you all are not
> >
> > The answer is the same as the above. This feature is missing in the
> > User channel, and I'm completing it with this patch.
>
> Again, that seems to be your userspace's issue, not the kernel's.  Just
> use the same api that bluez uses here.
>
> > > replacing all of the in-kernel BlueZ code with an external kernel driver
> > > just for this, right?  If so, that's not ok at all.
> >
> > Sorry I don't quite get it. What do you mean by the external kernel driver?
>
> You said you are not using the bluez kernel code, right?  So you must
> have some kernel code to implement this instead for the same
> functionality.  Otherwise you are using the bluez kernel api here.

No, we don't have kernel code for Bluetooth. We have everything in user space.

>
> Again, just use the same api please, don't go adding new one-off apis
> through sysfs for this when it is not needed.
>
> I'll also step back further and say, why not use bluez?  What is so
> wrong with that that you all need a totally different bluetooth stack?
> Why not just fix the bluez code for anything that is currently missing
> or lacking there that required you to write a new one.

I think the main purpose is moving the stack to the user space.
When the user hits a Bluetooth issue, it's much easier to reset the stack.
Also, a simple Bluetooth bug just won't crash your kernel and we could
even crash MORE to detect an incorrect chipset behavior earlier.
Of course BlueZ has its own advantages, but it's just all trade-offs.

>
> And yes, I know the inclination of Android to constantly rewrite the
> bluetooth stack, it's on the what, third or fourth iteration already?
> What's to guarantee that this really will be the last one?  :)

That's incorrect. Android is still using and maintaining the same
stack since it left BlueZ.

Hsin-chen Chuang Feb. 19, 2025, 1:54 a.m. UTC | #15

Hi Greg,

On Tue, Feb 18, 2025 at 8:24 PM Hsin-chen Chuang <[e-mail]> wrote:
>
> Hi Greg,
>
> On Tue, Feb 18, 2025 at 6:56 PM Greg KH <[e-mail]> wrote:
> >
> > On Tue, Feb 18, 2025 at 06:01:42PM +0800, Hsin-chen Chuang wrote:
> > > Hi Greg,
> > >
> > > On Tue, Feb 18, 2025 at 5:21 PM Greg KH <[e-mail]> wrote:
> > > >
> > > > On Tue, Feb 18, 2025 at 04:57:38PM +0800, Hsin-chen Chuang wrote:
> > > > > Hi Greg,
> > > > >
> > > > > On Tue, Feb 18, 2025 at 4:23 PM Greg KH <[e-mail]> wrote:
> > > > > >
> > > > > > On Tue, Feb 18, 2025 at 12:24:07PM +0800, Hsin-chen Chuang wrote:
> > > > > > > Hi Greg,
> > > > > > >
> > > > > > > On Mon, Feb 17, 2025 at 4:53 PM Greg KH <[e-mail]> wrote:
> > > > > > > >
> > > > > > > > On Mon, Feb 17, 2025 at 04:44:35PM +0800, Hsin-chen Chuang wrote:
> > > > > > > > > On Fri, Feb 14, 2025 at 7:37 PM Greg KH <[e-mail]> wrote:
> > > > > > > > > >
> > > > > > > > > > On Fri, Feb 14, 2025 at 07:16:17PM +0800, Hsin-chen Chuang wrote:
> > > > > > > > > > > From: Hsin-chen Chuang <[e-mail]>
> > > > > > > > > > >
> > > > > > > > > > > Expose the isoc_alt attr with device group to avoid the racing.
> > > > > > > > > > >
> > > > > > > > > > > Now we create a dev node for btusb. The isoc_alt attr belongs to it and
> > > > > > > > > > > it also becomes the parent device of hci dev.
> > > > > > > > > > >
> > > > > > > > > > > Fixes: b16b327edb4d ("Bluetooth: btusb: add sysfs attribute to control USB alt setting")
> > > > > > > > > >
> > > > > > > > > > Wait, step back, why is this commit needed if you can change the alt
> > > > > > > > > > setting already today through usbfs/libusb without needing to mess with
> > > > > > > > > > the bluetooth stack at all?
> > > > > > > > >
> > > > > > > > > In short: We want to configure the alternate settings without
> > > > > > > > > detaching the btusb driver, while detaching seems necessary for
> > > > > > > > > libusb_set_interface_alt_setting to work (Please correct me if I'm
> > > > > > > > > wrong!)
> > > > > > > >
> > > > > > > > I think changing the alternate setting should work using usbfs as you
> > > > > > > > would send that command to the device, not the interface, so the driver
> > > > > > > > bound to the existing interface would not need to be removed.
> > > > > > >
> > > > > > > I thought USBDEVFS_SETINTERFACE was the right command to begin with,
> > > > > > > but it seems not working in this case.
> > > > > > > The command itself attempts to claim the interface, but the interface
> > > > > > > is already claimed by btusb so it failed with Device or resource busy
> > > > > > >
> > > > > > > drivers/usb/core/devio.c:
> > > > > > >   USBDEVFS_SETINTERFACE -> proc_setintf -> checkintf -> claimintf
> > > > > >
> > > > > > Ah, ok, thanks for checking.  So as you control this device, why not
> > > > > > just disconnect it, change the setting, and then reconnect it?
> > > > >
> > > > > After dis/reconnecting, a Bluetooth chipset would lose all its state:
> > > > > Existing connections/scanners/advertisers are all dropped.
> > > >
> > > > If you are changing the alternate USB configuration, all state should be
> > > > dropped, right?  If not, huh how does the device know to keep that
> > > > state?
> > >
> > > No, the Bluetooth chip doesn't drop any info when the alt is changed.
> > > It only affects the data transfer bandwidth on that interface.
> > >
> > > >
> > > > > This is as bad as (just an analogy) "Whenever you access a http web
> > > > > page, you need to bring your ethernet interface down and up, and after
> > > > > the page is downloaded, do that again".
> > > >
> > > > Your ethernet interface does not contain state like this, we handle
> > > > chainging IP addresses and devices all the time, so perhaps wrong
> > > > analogy :)
> > > >
> > > > > > Also, see my other review comment, how does BlueZ do this today?
> > > > >
> > > > > BlueZ handles that in their MGMT command, that is, through Control
> > > > > channel -> BlueZ kernel space code -> driver callbacks.
> > > > > Once a Bluetooth chipset is opened with the User channel, it can't be
> > > > > used with the Control channel simultaneously, and vice versa.
> > > >
> > > > So why not use that same control channel in your code?  Why are you
> > >
> > > Because we're using the User channel, and they can't be used at the same time.
> >
> > This doesn't make sense.  Either BlueZ has this same problem, or it
> > doesn't.  As you say it does not, then again, why can't you use the
> > exact same user/kernel api to achieve this?
> >
> > The user/kernel api is "fixed" right now, if you wish to replace the
> > userspace side of the BlueZ code with your own, then you should/must use
> > that same user/kernel api.  Don't go adding duplicate interfaces please.
>
> I would say the kernel provides 2 sets of the API, Control and User,
> and now the User channel is missing something.
> I think it makes sense to add support for it.
>
> >
> > > > reinventing a new control channel for something that is obviously there
> > > > already?
> > >
> > > Not quite the same as "reinventing". The Control channel command does
> > > much more than just setting the alt; It just doesn't work with the
> > > User channel.
> > >
> > > >
> > > > So in short, what's preventing you from using the same exact driver
> > > > callbacks, OR the same exact kernel api.  Surely you all are not
> > >
> > > The answer is the same as the above. This feature is missing in the
> > > User channel, and I'm completing it with this patch.
> >
> > Again, that seems to be your userspace's issue, not the kernel's.  Just
> > use the same api that bluez uses here.
> >
> > > > replacing all of the in-kernel BlueZ code with an external kernel driver
> > > > just for this, right?  If so, that's not ok at all.
> > >
> > > Sorry I don't quite get it. What do you mean by the external kernel driver?
> >
> > You said you are not using the bluez kernel code, right?  So you must
> > have some kernel code to implement this instead for the same
> > functionality.  Otherwise you are using the bluez kernel api here.
>
> No, we don't have kernel code for Bluetooth. We have everything in user space.
>
> >
> > Again, just use the same api please, don't go adding new one-off apis
> > through sysfs for this when it is not needed.
> >
> > I'll also step back further and say, why not use bluez?  What is so
> > wrong with that that you all need a totally different bluetooth stack?
> > Why not just fix the bluez code for anything that is currently missing
> > or lacking there that required you to write a new one.
>
> I think the main purpose is moving the stack to the user space.
> When the user hits a Bluetooth issue, it's much easier to reset the stack.
> Also, a simple Bluetooth bug just won't crash your kernel and we could
> even crash MORE to detect an incorrect chipset behavior earlier.
> Of course BlueZ has its own advantages, but it's just all trade-offs.
>
> >
> > And yes, I know the inclination of Android to constantly rewrite the
> > bluetooth stack, it's on the what, third or fourth iteration already?
> > What's to guarantee that this really will be the last one?  :)
>
> That's incorrect. Android is still using and maintaining the same
> stack since it left BlueZ.
>
> --
> Best Regards,
> Hsin-chen

I'm moving forward and sending out v6 for review as there's no more
comment related to the struct device usage.
We could continue the discussion on v6, thanks.

Patch

diff --git a/Documentation/ABI/stable/sysfs-class-bluetooth b/Documentation/ABI/stable/sysfs-class-bluetooth
index 36be02471174..c1024c7c4634 100644
--- a/Documentation/ABI/stable/sysfs-class-bluetooth
+++ b/Documentation/ABI/stable/sysfs-class-bluetooth
@@ -7,3 +7,16 @@  Description: 	This write-only attribute allows users to trigger the vendor reset
 		The reset may or may not be done through the device transport
 		(e.g., UART/USB), and can also be done through an out-of-band
 		approach such as GPIO.
+
+What:		/sys/class/bluetooth/btusb<usb-intf>/isoc_alt
+Date:		13-Feb-2025
+KernelVersion:	6.13
+Contact:	[e-mail]
+Description:	This attribute allows users to configure the USB Alternate setting
+		for the specific HCI device. Reading this attribute returns the
+		current setting, and writing any supported numbers would change
+		the setting. See the USB Alternate setting definition in Bluetooth
+		core spec 5, vol 4, part B, table 2.1.
+		If the HCI device is not yet init-ed, the write fails with -ENODEV.
+		If the data is not a valid number, the write fails with -EINVAL.
+		The other failures are vendor specific.
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 1caf7a071a73..e2fb3d08a5ed 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -920,6 +920,8 @@  struct btusb_data {
 	int oob_wake_irq;   /* irq for out-of-band wake-on-bt */
 
 	struct qca_dump_info qca_dump;
+
+	struct device dev;
 };
 
 static void btusb_reset(struct hci_dev *hdev)
@@ -3693,6 +3695,9 @@  static ssize_t isoc_alt_store(struct device *dev,
 	int alt;
 	int ret;
 
+	if (!data->hdev)
+		return -ENODEV;
+
 	if (kstrtoint(buf, 10, &alt))
 		return -EINVAL;
 
@@ -3702,6 +3707,36 @@  static ssize_t isoc_alt_store(struct device *dev,
 
 static DEVICE_ATTR_RW(isoc_alt);
 
+static struct attribute *btusb_sysfs_attrs[] = {
+	NULL,
+};
+ATTRIBUTE_GROUPS(btusb_sysfs);
+
+static void btusb_sysfs_release(struct device *dev)
+{
+	struct btusb_data *data = dev_get_drvdata(dev);
+
+	kfree(data);
+}
+
+static const struct device_type btusb_sysfs = {
+	.name    = "btusb",
+	.release = btusb_sysfs_release,
+	.groups  = btusb_sysfs_groups,
+};
+
+static struct attribute *btusb_sysfs_isoc_alt_attrs[] = {
+	&dev_attr_isoc_alt.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(btusb_sysfs_isoc_alt);
+
+static const struct device_type btusb_sysfs_isoc_alt = {
+	.name    = "btusb",
+	.release = btusb_sysfs_release,
+	.groups  = btusb_sysfs_isoc_alt_groups,
+};
+
 static int btusb_probe(struct usb_interface *intf,
 		       const struct usb_device_id *id)
 {
@@ -3743,7 +3778,7 @@  static int btusb_probe(struct usb_interface *intf,
 			return -ENODEV;
 	}
 
-	data = devm_kzalloc(&intf->dev, sizeof(*data), GFP_KERNEL);
+	data = kzalloc(sizeof(*data), GFP_KERNEL);
 	if (!data)
 		return -ENOMEM;
 
@@ -3766,8 +3801,10 @@  static int btusb_probe(struct usb_interface *intf,
 		}
 	}
 
-	if (!data->intr_ep || !data->bulk_tx_ep || !data->bulk_rx_ep)
-		return -ENODEV;
+	if (!data->intr_ep || !data->bulk_tx_ep || !data->bulk_rx_ep) {
+		err = -ENODEV;
+		goto out_free_data;
+	}
 
 	if (id->driver_info & BTUSB_AMP) {
 		data->cmdreq_type = USB_TYPE_CLASS | 0x01;
@@ -3821,16 +3858,47 @@  static int btusb_probe(struct usb_interface *intf,
 
 	data->recv_acl = hci_recv_frame;
 
+	if (id->driver_info & BTUSB_AMP) {
+		/* AMP controllers do not support SCO packets */
+		data->isoc = NULL;
+	} else {
+		/* Interface orders are hardcoded in the specification */
+		data->isoc = usb_ifnum_to_if(data->udev, ifnum_base + 1);
+		data->isoc_ifnum = ifnum_base + 1;
+	}
+
+	if (id->driver_info & BTUSB_BROKEN_ISOC)
+		data->isoc = NULL;
+
+	/* Init a dev for btusb. The attr depends on the support of isoc. */
+	if (data->isoc)
+		data->dev.type = &btusb_sysfs_isoc_alt;
+	else
+		data->dev.type = &btusb_sysfs;
+	data->dev.class = &bt_class;
+	data->dev.parent = &intf->dev;
+
+	err = dev_set_name(&data->dev, "btusb%s", dev_name(&intf->dev));
+	if (err)
+		goto out_free_data;
+
+	dev_set_drvdata(&data->dev, data);
+	err = device_register(&data->dev);
+	if (err < 0)
+		goto out_put_sysfs;
+
 	hdev = hci_alloc_dev_priv(priv_size);
-	if (!hdev)
-		return -ENOMEM;
+	if (!hdev) {
+		err = -ENOMEM;
+		goto out_free_sysfs;
+	}
 
 	hdev->bus = HCI_USB;
 	hci_set_drvdata(hdev, data);
 
 	data->hdev = hdev;
 
-	SET_HCIDEV_DEV(hdev, &intf->dev);
+	SET_HCIDEV_DEV(hdev, &data->dev);
 
 	reset_gpio = gpiod_get_optional(&data->udev->dev, "reset",
 					GPIOD_OUT_LOW);
@@ -3969,15 +4037,6 @@  static int btusb_probe(struct usb_interface *intf,
 		hci_set_msft_opcode(hdev, 0xFD70);
 	}
 
-	if (id->driver_info & BTUSB_AMP) {
-		/* AMP controllers do not support SCO packets */
-		data->isoc = NULL;
-	} else {
-		/* Interface orders are hardcoded in the specification */
-		data->isoc = usb_ifnum_to_if(data->udev, ifnum_base + 1);
-		data->isoc_ifnum = ifnum_base + 1;
-	}
-
 	if (IS_ENABLED(CONFIG_BT_HCIBTUSB_RTL) &&
 	    (id->driver_info & BTUSB_REALTEK)) {
 		btrtl_set_driver_name(hdev, btusb_driver.name);
@@ -4010,9 +4069,6 @@  static int btusb_probe(struct usb_interface *intf,
 			set_bit(HCI_QUIRK_FIXUP_BUFFER_SIZE, &hdev->quirks);
 	}
 
-	if (id->driver_info & BTUSB_BROKEN_ISOC)
-		data->isoc = NULL;
-
 	if (id->driver_info & BTUSB_WIDEBAND_SPEECH)
 		set_bit(HCI_QUIRK_WIDEBAND_SPEECH_SUPPORTED, &hdev->quirks);
 
@@ -4065,10 +4121,6 @@  static int btusb_probe(struct usb_interface *intf,
 						 data->isoc, data);
 		if (err < 0)
 			goto out_free_dev;
-
-		err = device_create_file(&intf->dev, &dev_attr_isoc_alt);
-		if (err)
-			goto out_free_dev;
 	}
 
 	if (IS_ENABLED(CONFIG_BT_HCIBTUSB_BCM) && data->diag) {
@@ -4099,6 +4151,16 @@  static int btusb_probe(struct usb_interface *intf,
 	if (data->reset_gpio)
 		gpiod_put(data->reset_gpio);
 	hci_free_dev(hdev);
+
+out_free_sysfs:
+	device_del(&data->dev);
+
+out_put_sysfs:
+	put_device(&data->dev);
+	return err;
+
+out_free_data:
+	kfree(data);
 	return err;
 }
 
@@ -4115,10 +4177,8 @@  static void btusb_disconnect(struct usb_interface *intf)
 	hdev = data->hdev;
 	usb_set_intfdata(data->intf, NULL);
 
-	if (data->isoc) {
-		device_remove_file(&intf->dev, &dev_attr_isoc_alt);
+	if (data->isoc)
 		usb_set_intfdata(data->isoc, NULL);
-	}
 
 	if (data->diag)
 		usb_set_intfdata(data->diag, NULL);
@@ -4150,6 +4210,7 @@  static void btusb_disconnect(struct usb_interface *intf)
 		gpiod_put(data->reset_gpio);
 
 	hci_free_dev(hdev);
+	device_unregister(&data->dev);
 }
 
 #ifdef CONFIG_PM
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 05919848ea95..776dd6183509 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -1843,6 +1843,7 @@  int hci_get_adv_monitor_offload_ext(struct hci_dev *hdev);
 
 void hci_event_packet(struct hci_dev *hdev, struct sk_buff *skb);
 
+extern const struct class bt_class;
 void hci_init_sysfs(struct hci_dev *hdev);
 void hci_conn_init_sysfs(struct hci_conn *conn);
 void hci_conn_add_sysfs(struct hci_conn *conn);
diff --git a/net/bluetooth/hci_sysfs.c b/net/bluetooth/hci_sysfs.c
index 041ce9adc378..aab3ffaa264c 100644
--- a/net/bluetooth/hci_sysfs.c
+++ b/net/bluetooth/hci_sysfs.c
@@ -6,9 +6,10 @@ 
 #include <net/bluetooth/bluetooth.h>
 #include <net/bluetooth/hci_core.h>
 
-static const struct class bt_class = {
+const struct class bt_class = {
 	.name = "bluetooth",
 };
+EXPORT_SYMBOL(bt_class);
 
 static void bt_link_release(struct device *dev)
 {