[PATCH v3 1/3] Bluetooth: Fix possible race with userspace of sysfs isoc_alt

[PATCH v3 1/3] Bluetooth: Fix possible race with userspace of sysfs isoc_alt

[Hsin-chen Chuang]

From: Hsin-chen Chuang <chharry@chromium.org>

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 <chharry@chromium.org>
---

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.

 drivers/bluetooth/btintel_pcie.c |  3 +-
 drivers/bluetooth/btusb.c        | 69 +++++++++++---------------------
 drivers/bluetooth/hci_serdev.c   |  2 +-
 include/net/bluetooth/hci_core.h |  8 ++--
 net/bluetooth/hci_core.c         |  4 +-
 net/bluetooth/hci_sysfs.c        | 52 +++++++++++++++++++++++-
 6 files changed, 84 insertions(+), 54 deletions(-)

diff --git a/drivers/bluetooth/btintel_pcie.c b/drivers/bluetooth/btintel_pcie.c
index b8b241a92bf9..856de070b440 100644
--- a/drivers/bluetooth/btintel_pcie.c
+++ b/drivers/bluetooth/btintel_pcie.c
@@ -1514,7 +1514,8 @@ static int btintel_pcie_setup_hdev(struct btintel_pcie_data *data)
 	int err;
 	struct hci_dev *hdev;
 
-	hdev = hci_alloc_dev_priv(sizeof(struct btintel_data));
+	hdev = hci_alloc_dev_priv(sizeof(struct btintel_data),
+				  /* add_isoc_alt_attr = */ false);
 	if (!hdev)
 		return -ENOMEM;
 
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 1caf7a071a73..a451403c62eb 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -2247,6 +2247,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)
 {
@@ -3676,32 +3683,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)
 {
@@ -3821,7 +3802,20 @@ static int btusb_probe(struct usb_interface *intf,
 
 	data->recv_acl = hci_recv_frame;
 
-	hdev = hci_alloc_dev_priv(priv_size);
+	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;
+
+	hdev = hci_alloc_dev_priv(priv_size,
+				  /* add_isoc_alt_attr = */ data->isoc);
 	if (!hdev)
 		return -ENOMEM;
 
@@ -3969,15 +3963,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 +3995,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);
 
@@ -4066,9 +4048,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) {
@@ -4115,10 +4096,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/drivers/bluetooth/hci_serdev.c b/drivers/bluetooth/hci_serdev.c
index 89a22e9b3253..41a4a91be4b8 100644
--- a/drivers/bluetooth/hci_serdev.c
+++ b/drivers/bluetooth/hci_serdev.c
@@ -326,7 +326,7 @@ int hci_uart_register_device_priv(struct hci_uart *hu,
 	set_bit(HCI_UART_PROTO_READY, &hu->flags);
 
 	/* Initialize and register HCI device */
-	hdev = hci_alloc_dev_priv(sizeof_priv);
+	hdev = hci_alloc_dev_priv(sizeof_priv, /* add_isoc_alt_attr = */ false);
 	if (!hdev) {
 		BT_ERR("Can't allocate HCI device");
 		err = -ENOMEM;
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 05919848ea95..2a596ea40308 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)
@@ -1686,11 +1688,11 @@ static inline void *hci_get_priv(struct hci_dev *hdev)
 struct hci_dev *hci_dev_get(int index);
 struct hci_dev *hci_get_route(bdaddr_t *dst, bdaddr_t *src, u8 src_type);
 
-struct hci_dev *hci_alloc_dev_priv(int sizeof_priv);
+struct hci_dev *hci_alloc_dev_priv(int sizeof_priv, bool add_isoc_alt_attr);
 
 static inline struct hci_dev *hci_alloc_dev(void)
 {
-	return hci_alloc_dev_priv(0);
+	return hci_alloc_dev_priv(0, /* add_isoc_alt_attr = */ false);
 }
 
 void hci_free_dev(struct hci_dev *hdev);
@@ -1843,7 +1845,7 @@ int hci_get_adv_monitor_offload_ext(struct hci_dev *hdev);
 
 void hci_event_packet(struct hci_dev *hdev, struct sk_buff *skb);
 
-void hci_init_sysfs(struct hci_dev *hdev);
+void hci_init_sysfs(struct hci_dev *hdev, bool add_isoc_alt_attr);
 void hci_conn_init_sysfs(struct hci_conn *conn);
 void hci_conn_add_sysfs(struct hci_conn *conn);
 void hci_conn_del_sysfs(struct hci_conn *conn);
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index e7ec12437c8b..7c90391721ba 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -2405,7 +2405,7 @@ static int hci_suspend_notifier(struct notifier_block *nb, unsigned long action,
 }
 
 /* Alloc HCI device */
-struct hci_dev *hci_alloc_dev_priv(int sizeof_priv)
+struct hci_dev *hci_alloc_dev_priv(int sizeof_priv, bool add_isoc_alt_attr)
 {
 	struct hci_dev *hdev;
 	unsigned int alloc_size;
@@ -2530,7 +2530,7 @@ struct hci_dev *hci_alloc_dev_priv(int sizeof_priv)
 
 	hci_devcd_setup(hdev);
 
-	hci_init_sysfs(hdev);
+	hci_init_sysfs(hdev, add_isoc_alt_attr);
 	discovery_init(hdev);
 
 	return hdev;
diff --git a/net/bluetooth/hci_sysfs.c b/net/bluetooth/hci_sysfs.c
index 041ce9adc378..3242f1ce00b2 100644
--- a/net/bluetooth/hci_sysfs.c
+++ b/net/bluetooth/hci_sysfs.c
@@ -102,6 +102,38 @@ 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,
 	NULL,
@@ -114,11 +146,27 @@ static const struct device_type bt_host = {
 	.groups = bt_host_groups,
 };
 
-void hci_init_sysfs(struct hci_dev *hdev)
+static struct attribute *bt_host_isoc_alt_attrs[] = {
+	&dev_attr_reset.attr,
+	&dev_attr_isoc_alt.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(bt_host_isoc_alt);
+
+static const struct device_type bt_host_isoc_alt = {
+	.name    = "host",
+	.release = bt_host_release,
+	.groups  = bt_host_isoc_alt_groups,
+};
+
+void hci_init_sysfs(struct hci_dev *hdev, bool add_isoc_alt_attr)
 {
 	struct device *dev = &hdev->dev;
 
-	dev->type = &bt_host;
+	if (add_isoc_alt_attr)
+		dev->type = &bt_host_isoc_alt;
+	else
+		dev->type = &bt_host;
 	dev->class = &bt_class;
 
 	__module_get(THIS_MODULE);
-- 
2.48.1.502.g6dc24dfdaf-goog

[PATCH v3 2/3] Bluetooth: Always allow SCO packets for user channel

[Hsin-chen Chuang]

From: Hsin-chen Chuang <chharry@chromium.org>

The SCO packets from Bluetooth raw socket are now rejected because
hci_conn_num is left 0. This patch allows such the usecase to enable
the userspace SCO support.

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

(no changes since v2)

Changes in v2:
- Check HCI_USER_CHANNEL rather than just remove the hci_conn_num check

 drivers/bluetooth/btusb.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index a451403c62eb..2bf6b37344e2 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -2130,7 +2130,8 @@ static int btusb_send_frame(struct hci_dev *hdev, struct sk_buff *skb)
 		return submit_or_queue_tx_urb(hdev, urb);
 
 	case HCI_SCODATA_PKT:
-		if (hci_conn_num(hdev, SCO_LINK) < 1)
+		if (!hci_dev_test_flag(hdev, HCI_USER_CHANNEL) &&
+		    hci_conn_num(hdev, SCO_LINK) < 1)
 			return -ENODEV;
 
 		urb = alloc_isoc_urb(hdev, skb);
@@ -2611,7 +2612,8 @@ static int btusb_send_frame_intel(struct hci_dev *hdev, struct sk_buff *skb)
 		return submit_or_queue_tx_urb(hdev, urb);
 
 	case HCI_SCODATA_PKT:
-		if (hci_conn_num(hdev, SCO_LINK) < 1)
+		if (!hci_dev_test_flag(hdev, HCI_USER_CHANNEL) &&
+		    hci_conn_num(hdev, SCO_LINK) < 1)
 			return -ENODEV;
 
 		urb = alloc_isoc_urb(hdev, skb);
-- 
2.48.1.502.g6dc24dfdaf-goog

[PATCH v3 3/3] Bluetooth: Add ABI doc for sysfs isoc_alt

[Hsin-chen Chuang]

From: Hsin-chen Chuang <chharry@chromium.org>

The functionality was completed in commit 5e5c3898ef49 ("Bluetooth: Fix
possible race with userspace of sysfs isoc_alt")

Fixes: 5e5c3898ef49 ("Bluetooth: Fix possible race with userspace of sysfs isoc_alt")
Signed-off-by: Hsin-chen Chuang <chharry@chromium.org>
---

(no changes since v1)

 Documentation/ABI/stable/sysfs-class-bluetooth | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/Documentation/ABI/stable/sysfs-class-bluetooth b/Documentation/ABI/stable/sysfs-class-bluetooth
index 36be02471174..1168e0318e35 100644
--- a/Documentation/ABI/stable/sysfs-class-bluetooth
+++ b/Documentation/ABI/stable/sysfs-class-bluetooth
@@ -7,3 +7,15 @@ 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/hci<index>/isoc_alt
+Date:		10-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 data is not a valid number, the write fails with -EINVAL.
+		The other failures are vendor specific.
-- 
2.48.1.502.g6dc24dfdaf-goog

Re: [PATCH v3 3/3] Bluetooth: Add ABI doc for sysfs isoc_alt

[Luiz Augusto von Dentz]

Hi Hsin-chen,

On Mon, Feb 10, 2025 at 5:32 AM Hsin-chen Chuang <chharry@google.com> wrote:
>
> From: Hsin-chen Chuang <chharry@chromium.org>
>
> The functionality was completed in commit 5e5c3898ef49 ("Bluetooth: Fix
> possible race with userspace of sysfs isoc_alt")
>
> Fixes: 5e5c3898ef49 ("Bluetooth: Fix possible race with userspace of sysfs isoc_alt")
> Signed-off-by: Hsin-chen Chuang <chharry@chromium.org>
> ---
>
> (no changes since v1)
>
>  Documentation/ABI/stable/sysfs-class-bluetooth | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/Documentation/ABI/stable/sysfs-class-bluetooth b/Documentation/ABI/stable/sysfs-class-bluetooth
> index 36be02471174..1168e0318e35 100644
> --- a/Documentation/ABI/stable/sysfs-class-bluetooth
> +++ b/Documentation/ABI/stable/sysfs-class-bluetooth
> @@ -7,3 +7,15 @@ 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/hci<index>/isoc_alt
> +Date:          10-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 data is not a valid number, the write fails with -EINVAL.
> +               The other failures are vendor specific.

Still not really convinced this is the right way to expose it, it is
not an HCI attribute to begin with, not sure if we couldn't perhaps
add another node to control it or add via USB device?

-- 
Luiz Augusto von Dentz

Re: [PATCH v3 3/3] Bluetooth: Add ABI doc for sysfs isoc_alt

[Hsin-chen Chuang]

Hi Luiz,

Thanks for the feedback.

On Tue, Feb 11, 2025 at 2:17 AM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Hsin-chen,
>
> On Mon, Feb 10, 2025 at 5:32 AM Hsin-chen Chuang <chharry@google.com> wrote:
> >
> > From: Hsin-chen Chuang <chharry@chromium.org>
> >
> > The functionality was completed in commit 5e5c3898ef49 ("Bluetooth: Fix
> > possible race with userspace of sysfs isoc_alt")
> >
> > Fixes: 5e5c3898ef49 ("Bluetooth: Fix possible race with userspace of sysfs isoc_alt")
> > Signed-off-by: Hsin-chen Chuang <chharry@chromium.org>
> > ---
> >
> > (no changes since v1)
> >
> >  Documentation/ABI/stable/sysfs-class-bluetooth | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/Documentation/ABI/stable/sysfs-class-bluetooth b/Documentation/ABI/stable/sysfs-class-bluetooth
> > index 36be02471174..1168e0318e35 100644
> > --- a/Documentation/ABI/stable/sysfs-class-bluetooth
> > +++ b/Documentation/ABI/stable/sysfs-class-bluetooth
> > @@ -7,3 +7,15 @@ 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/hci<index>/isoc_alt
> > +Date:          10-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 data is not a valid number, the write fails with -EINVAL.
> > +               The other failures are vendor specific.
>
> Still not really convinced this is the right way to expose it, it is
> not an HCI attribute to begin with, not sure if we couldn't perhaps

Could you tell more about why this is not an HCI attr to begin with?
The alt setting is bonded to the USB device which is now under btusb's
control, and btusb creates a sysfs node for this. This attr location
decision seems natural to me.

> add another node to control it or add via USB device?

I feel allowing this in the USB sysfs API might be an overkill as that
makes all USB devices' alt settings be controllable from the user
space. Limiting this usage in the scope of BT makes more sense to me.

>
> --
> Luiz Augusto von Dentz

-- 
Best Regards,
Hsin-chen

Re: [PATCH v3 3/3] Bluetooth: Add ABI doc for sysfs isoc_alt

[Luiz Augusto von Dentz]

Hi Hsin-chen,

On Mon, Feb 10, 2025 at 11:23 PM Hsin-chen Chuang <chharry@google.com> wrote:
>
> Hi Luiz,
>
> Thanks for the feedback.
>
> On Tue, Feb 11, 2025 at 2:17 AM Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
> >
> > Hi Hsin-chen,
> >
> > On Mon, Feb 10, 2025 at 5:32 AM Hsin-chen Chuang <chharry@google.com> wrote:
> > >
> > > From: Hsin-chen Chuang <chharry@chromium.org>
> > >
> > > The functionality was completed in commit 5e5c3898ef49 ("Bluetooth: Fix
> > > possible race with userspace of sysfs isoc_alt")
> > >
> > > Fixes: 5e5c3898ef49 ("Bluetooth: Fix possible race with userspace of sysfs isoc_alt")
> > > Signed-off-by: Hsin-chen Chuang <chharry@chromium.org>
> > > ---
> > >
> > > (no changes since v1)
> > >
> > >  Documentation/ABI/stable/sysfs-class-bluetooth | 12 ++++++++++++
> > >  1 file changed, 12 insertions(+)
> > >
> > > diff --git a/Documentation/ABI/stable/sysfs-class-bluetooth b/Documentation/ABI/stable/sysfs-class-bluetooth
> > > index 36be02471174..1168e0318e35 100644
> > > --- a/Documentation/ABI/stable/sysfs-class-bluetooth
> > > +++ b/Documentation/ABI/stable/sysfs-class-bluetooth
> > > @@ -7,3 +7,15 @@ 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/hci<index>/isoc_alt
> > > +Date:          10-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 data is not a valid number, the write fails with -EINVAL.
> > > +               The other failures are vendor specific.
> >
> > Still not really convinced this is the right way to expose it, it is
> > not an HCI attribute to begin with, not sure if we couldn't perhaps
>
> Could you tell more about why this is not an HCI attr to begin with?
> The alt setting is bonded to the USB device which is now under btusb's
> control, and btusb creates a sysfs node for this. This attr location
> decision seems natural to me.

Well alt setting is (obviously) not covered as part of HCI
specification, USB is one possible transport bus of HCI, but that
itself is already saying that we would be mixing the transport with
actual HCI protocol.

> > add another node to control it or add via USB device?
>
> I feel allowing this in the USB sysfs API might be an overkill as that
> makes all USB devices' alt settings be controllable from the user
> space. Limiting this usage in the scope of BT makes more sense to me.

Not all, just btusb ones, that said what we could add another level to
control transport specific attribute e.g
/sys/class/bluetooth/usb#/isoc_alt than that becomes the parent of
/sys/class/bluetooth/hci<index>

> >
> > --
> > Luiz Augusto von Dentz
>
> --
> Best Regards,
> Hsin-chen



-- 
Luiz Augusto von Dentz

Re: [PATCH v3 3/3] Bluetooth: Add ABI doc for sysfs isoc_alt

[Hsin-chen Chuang]

Hi Luiz,

Thanks, that makes sense to me. I'll resolve this comment in the next version.

On Wed, Feb 12, 2025 at 6:01 AM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Hsin-chen,
>
> On Mon, Feb 10, 2025 at 11:23 PM Hsin-chen Chuang <chharry@google.com> wrote:
> >
> > Hi Luiz,
> >
> > Thanks for the feedback.
> >
> > On Tue, Feb 11, 2025 at 2:17 AM Luiz Augusto von Dentz
> > <luiz.dentz@gmail.com> wrote:
> > >
> > > Hi Hsin-chen,
> > >
> > > On Mon, Feb 10, 2025 at 5:32 AM Hsin-chen Chuang <chharry@google.com> wrote:
> > > >
> > > > From: Hsin-chen Chuang <chharry@chromium.org>
> > > >
> > > > The functionality was completed in commit 5e5c3898ef49 ("Bluetooth: Fix
> > > > possible race with userspace of sysfs isoc_alt")
> > > >
> > > > Fixes: 5e5c3898ef49 ("Bluetooth: Fix possible race with userspace of sysfs isoc_alt")
> > > > Signed-off-by: Hsin-chen Chuang <chharry@chromium.org>
> > > > ---
> > > >
> > > > (no changes since v1)
> > > >
> > > >  Documentation/ABI/stable/sysfs-class-bluetooth | 12 ++++++++++++
> > > >  1 file changed, 12 insertions(+)
> > > >
> > > > diff --git a/Documentation/ABI/stable/sysfs-class-bluetooth b/Documentation/ABI/stable/sysfs-class-bluetooth
> > > > index 36be02471174..1168e0318e35 100644
> > > > --- a/Documentation/ABI/stable/sysfs-class-bluetooth
> > > > +++ b/Documentation/ABI/stable/sysfs-class-bluetooth
> > > > @@ -7,3 +7,15 @@ 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/hci<index>/isoc_alt
> > > > +Date:          10-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 data is not a valid number, the write fails with -EINVAL.
> > > > +               The other failures are vendor specific.
> > >
> > > Still not really convinced this is the right way to expose it, it is
> > > not an HCI attribute to begin with, not sure if we couldn't perhaps
> >
> > Could you tell more about why this is not an HCI attr to begin with?
> > The alt setting is bonded to the USB device which is now under btusb's
> > control, and btusb creates a sysfs node for this. This attr location
> > decision seems natural to me.
>
> Well alt setting is (obviously) not covered as part of HCI
> specification, USB is one possible transport bus of HCI, but that
> itself is already saying that we would be mixing the transport with
> actual HCI protocol.
>
> > > add another node to control it or add via USB device?
> >
> > I feel allowing this in the USB sysfs API might be an overkill as that
> > makes all USB devices' alt settings be controllable from the user
> > space. Limiting this usage in the scope of BT makes more sense to me.
>
> Not all, just btusb ones, that said what we could add another level to
> control transport specific attribute e.g
> /sys/class/bluetooth/usb#/isoc_alt than that becomes the parent of
> /sys/class/bluetooth/hci<index>
>
> > >
> > > --
> > > Luiz Augusto von Dentz
> >
> > --
> > Best Regards,
> > Hsin-chen
>
>
>
> --
> Luiz Augusto von Dentz