Hi Luiz,

On Thu, Jan 23, 2025 at 3:35 AM Luiz Augusto von Dentz
[off-list ref] wrote:

Hi Hsin-chen,

On Wed, Jan 22, 2025 at 12:20 AM Hsin-chen Chuang [off-list ref] wrote:

quoted

From: Hsin-chen Chuang <redacted>

Use device group to avoid the racing. To reuse the group defined in
hci_sysfs.c, defined 2 callbacks switch_usb_alt_setting and
read_usb_alt_setting which are only registered in btusb.

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

(no changes since v1)

 drivers/bluetooth/btusb.c        | 42 ++++++++------------------------
 include/net/bluetooth/hci_core.h |  2 ++
 net/bluetooth/hci_sysfs.c        | 33 +++++++++++++++++++++++++
 3 files changed, 45 insertions(+), 32 deletions(-)

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 75a0f15819c4..bf210275e5b7 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c

@@ -2221,6 +2221,13 @@ static int btusb_switch_alt_setting(struct hci_dev *hdev, int new_alts)
        return 0;
 }

+static int btusb_read_alt_setting(struct hci_dev *hdev)
+{
+       struct btusb_data *data = hci_get_drvdata(hdev);
+
+       return data->isoc_altsetting;
+}
+
 static struct usb_host_interface *btusb_find_altsetting(struct btusb_data *data,
                                                        int alt)
 {

@@ -3650,32 +3657,6 @@ static const struct file_operations force_poll_sync_fops = {
        .llseek         = default_llseek,
 };

-static ssize_t isoc_alt_show(struct device *dev,
-                            struct device_attribute *attr,
-                            char *buf)
-{
-       struct btusb_data *data = dev_get_drvdata(dev);
-
-       return sysfs_emit(buf, "%d\n", data->isoc_altsetting);
-}
-
-static ssize_t isoc_alt_store(struct device *dev,
-                             struct device_attribute *attr,
-                             const char *buf, size_t count)
-{
-       struct btusb_data *data = dev_get_drvdata(dev);
-       int alt;
-       int ret;
-
-       if (kstrtoint(buf, 10, &alt))
-               return -EINVAL;
-
-       ret = btusb_switch_alt_setting(data->hdev, alt);
-       return ret < 0 ? ret : count;
-}
-
-static DEVICE_ATTR_RW(isoc_alt);
-
 static int btusb_probe(struct usb_interface *intf,
                       const struct usb_device_id *id)
 {

@@ -4040,9 +4021,8 @@ static int btusb_probe(struct usb_interface *intf,
                if (err < 0)
                        goto out_free_dev;

-               err = device_create_file(&intf->dev, &dev_attr_isoc_alt);
-               if (err)
-                       goto out_free_dev;
+               hdev->switch_usb_alt_setting = btusb_switch_alt_setting;
+               hdev->read_usb_alt_setting = btusb_read_alt_setting;
        }

        if (IS_ENABLED(CONFIG_BT_HCIBTUSB_BCM) && data->diag) {

@@ -4089,10 +4069,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);

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index f756fac95488..5d3ec5ff5adb 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h

@@ -641,6 +641,8 @@ struct hci_dev {
                                     struct bt_codec *codec, __u8 *vnd_len,
                                     __u8 **vnd_data);
        u8 (*classify_pkt_type)(struct hci_dev *hdev, struct sk_buff *skb);
+       int (*switch_usb_alt_setting)(struct hci_dev *hdev, int new_alts);
+       int (*read_usb_alt_setting)(struct hci_dev *hdev);
 };

 #define HCI_PHY_HANDLE(handle) (handle & 0xff)

diff --git a/net/bluetooth/hci_sysfs.c b/net/bluetooth/hci_sysfs.c
index 041ce9adc378..887aa1935b1e 100644
--- a/net/bluetooth/hci_sysfs.c
+++ b/net/bluetooth/hci_sysfs.c

@@ -102,8 +102,41 @@ static ssize_t reset_store(struct device *dev, struct device_attribute *attr,
 }
 static DEVICE_ATTR_WO(reset);

+static ssize_t isoc_alt_show(struct device *dev,
+                            struct device_attribute *attr,
+                            char *buf)
+{
+       struct hci_dev *hdev = to_hci_dev(dev);
+
+       if (hdev->read_usb_alt_setting)
+               return sysfs_emit(buf, "%d\n", hdev->read_usb_alt_setting(hdev));
+
+       return -ENODEV;
+}
+
+static ssize_t isoc_alt_store(struct device *dev,
+                             struct device_attribute *attr,
+                             const char *buf, size_t count)
+{
+       struct hci_dev *hdev = to_hci_dev(dev);
+       int alt;
+       int ret;
+
+       if (kstrtoint(buf, 10, &alt))
+               return -EINVAL;
+
+       if (hdev->switch_usb_alt_setting) {
+               ret = hdev->switch_usb_alt_setting(hdev, alt);
+               return ret < 0 ? ret : count;
+       }
+
+       return -ENODEV;
+}
+static DEVICE_ATTR_RW(isoc_alt);
+
 static struct attribute *bt_host_attrs[] = {
        &dev_attr_reset.attr,
+       &dev_attr_isoc_alt.attr,
        NULL,
 };
 ATTRIBUTE_GROUPS(bt_host);

While this fixes the race it also forces the inclusion of an attribute
that is driver specific, so I wonder if we should introduce some
internal interface to register driver specific entries like this.

Do you mean you prefer the original interface that only exports the
attribute when isoc_altsetting is supported?
Agree it makes more sense but I hit the obstacle: hci_init_sysfs is
called earlier than data->isoc is determined. I need some time to
verify whether changing the order won't break anything.

quoted

--
2.48.1.262.g85cc9f2d1e-goog

--
Luiz Augusto von Dentz