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?
> 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?
> };
>
> 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
