| Message ID | 20250214191615.v5.1.If6f14aa2512336173a53fc3552756cd8a332b0a3@changeid (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v5] Bluetooth: Fix possible race with userspace of sysfs isoc_alt | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Fri, Feb 14, 2025 at 7:16 PM Hsin-chen Chuang <chharry@google.com> wrote: > > From: Hsin-chen Chuang <chharry@chromium.org> > > 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 <chharry@chromium.org> > --- > > 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: linux-bluetooth@vger.kernel.org > +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
On Fri, Feb 14, 2025 at 07:16:17PM +0800, Hsin-chen Chuang wrote: > From: Hsin-chen Chuang <chharry@chromium.org> > > 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 <chharry@chromium.org> > --- > > 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: linux-bluetooth@vger.kernel.org > +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
On Fri, Feb 14, 2025 at 7:37 PM Greg KH <gregkh@linuxfoundation.org> wrote: > > On Fri, Feb 14, 2025 at 07:16:17PM +0800, Hsin-chen Chuang wrote: > > From: Hsin-chen Chuang <chharry@chromium.org> > > > > 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 <chharry@chromium.org> > > --- > > > > 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: linux-bluetooth@vger.kernel.org > > +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
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 <gregkh@linuxfoundation.org> wrote: > > > > On Fri, Feb 14, 2025 at 07:16:17PM +0800, Hsin-chen Chuang wrote: > > > From: Hsin-chen Chuang <chharry@chromium.org> > > > > > > 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
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 <gregkh@linuxfoundation.org> wrote: > > > > On Fri, Feb 14, 2025 at 07:16:17PM +0800, Hsin-chen Chuang wrote: > > > From: Hsin-chen Chuang <chharry@chromium.org> > > > > > > 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
On Mon, Feb 17, 2025 at 4:53 PM Greg KH <gregkh@linuxfoundation.org> 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 <gregkh@linuxfoundation.org> wrote: > > > > > > On Fri, Feb 14, 2025 at 07:16:17PM +0800, Hsin-chen Chuang wrote: > > > > From: Hsin-chen Chuang <chharry@chromium.org> > > > > > > > > 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 <gregkh@linuxfoundation.org> 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 <gregkh@linuxfoundation.org> wrote: > > > > > > On Fri, Feb 14, 2025 at 07:16:17PM +0800, Hsin-chen Chuang wrote: > > > > From: Hsin-chen Chuang <chharry@chromium.org> > > > > > > > > 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
Hi Greg, On Mon, Feb 17, 2025 at 4:53 PM Greg KH <gregkh@linuxfoundation.org> 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 <gregkh@linuxfoundation.org> wrote: > > > > > > On Fri, Feb 14, 2025 at 07:16:17PM +0800, Hsin-chen Chuang wrote: > > > > From: Hsin-chen Chuang <chharry@chromium.org> > > > > > > > > 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
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 <gregkh@linuxfoundation.org> 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 <gregkh@linuxfoundation.org> wrote: > > > > > > > > On Fri, Feb 14, 2025 at 07:16:17PM +0800, Hsin-chen Chuang wrote: > > > > > From: Hsin-chen Chuang <chharry@chromium.org> > > > > > > > > > > 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
Hi Greg, On Tue, Feb 18, 2025 at 4:23 PM Greg KH <gregkh@linuxfoundation.org> 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 <gregkh@linuxfoundation.org> 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 <gregkh@linuxfoundation.org> wrote: > > > > > > > > > > On Fri, Feb 14, 2025 at 07:16:17PM +0800, Hsin-chen Chuang wrote: > > > > > > From: Hsin-chen Chuang <chharry@chromium.org> > > > > > > > > > > > > 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.
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 <gregkh@linuxfoundation.org> 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 <gregkh@linuxfoundation.org> 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 <gregkh@linuxfoundation.org> wrote: > > > > > > > > > > > > On Fri, Feb 14, 2025 at 07:16:17PM +0800, Hsin-chen Chuang wrote: > > > > > > > From: Hsin-chen Chuang <chharry@chromium.org> > > > > > > > > > > > > > > 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
Hi Greg, On Tue, Feb 18, 2025 at 5:21 PM Greg KH <gregkh@linuxfoundation.org> 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 <gregkh@linuxfoundation.org> 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 <gregkh@linuxfoundation.org> 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 <gregkh@linuxfoundation.org> wrote: > > > > > > > > > > > > > > On Fri, Feb 14, 2025 at 07:16:17PM +0800, Hsin-chen Chuang wrote: > > > > > > > > From: Hsin-chen Chuang <chharry@chromium.org> > > > > > > > > > > > > > > > > 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?
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 <gregkh@linuxfoundation.org> 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 <gregkh@linuxfoundation.org> 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 <gregkh@linuxfoundation.org> 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 <gregkh@linuxfoundation.org> wrote: > > > > > > > > > > > > > > > > On Fri, Feb 14, 2025 at 07:16:17PM +0800, Hsin-chen Chuang wrote: > > > > > > > > > From: Hsin-chen Chuang <chharry@chromium.org> > > > > > > > > > > > > > > > > > > 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
Hi Greg, On Tue, Feb 18, 2025 at 6:56 PM Greg KH <gregkh@linuxfoundation.org> 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 <gregkh@linuxfoundation.org> 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 <gregkh@linuxfoundation.org> 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 <gregkh@linuxfoundation.org> 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 <gregkh@linuxfoundation.org> wrote: > > > > > > > > > > > > > > > > > > On Fri, Feb 14, 2025 at 07:16:17PM +0800, Hsin-chen Chuang wrote: > > > > > > > > > > From: Hsin-chen Chuang <chharry@chromium.org> > > > > > > > > > > > > > > > > > > > > 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.
Hi Greg, On Tue, Feb 18, 2025 at 8:24 PM Hsin-chen Chuang <chharry@google.com> wrote: > > Hi Greg, > > On Tue, Feb 18, 2025 at 6:56 PM Greg KH <gregkh@linuxfoundation.org> 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 <gregkh@linuxfoundation.org> 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 <gregkh@linuxfoundation.org> 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 <gregkh@linuxfoundation.org> 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 <gregkh@linuxfoundation.org> wrote: > > > > > > > > > > > > > > > > > > > > On Fri, Feb 14, 2025 at 07:16:17PM +0800, Hsin-chen Chuang wrote: > > > > > > > > > > > From: Hsin-chen Chuang <chharry@chromium.org> > > > > > > > > > > > > > > > > > > > > > > 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.
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: linux-bluetooth@vger.kernel.org +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) {