On Fri, Feb 14, 2025 at 7:37 PM Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
>
> On Fri, Feb 14, 2025 at 07:16:17PM +0800, Hsin-chen Chuang wrote:
> > From: Hsin-chen Chuang <chharry@xxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxx>
> > ---
> >
> > 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@xxxxxxxxxxxxxxx
> > +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
--
Best Regards,
Hsin-chen
