* [PATCH v2 1/3] Bluetooth: Fix possible race with userspace of sysfs isoc_alt
@ 2025-01-22 5:19 Hsin-chen Chuang
2025-01-22 5:19 ` [PATCH v2 2/3] Bluetooth: Always allow SCO packets for user channel Hsin-chen Chuang
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Hsin-chen Chuang @ 2025-01-22 5:19 UTC (permalink / raw)
To: linux-bluetooth, luiz.dentz
Cc: chromeos-bluetooth-upstreaming, Hsin-chen Chuang,
David S. Miller, Eric Dumazet, Jakub Kicinski, Johan Hedberg,
Marcel Holtmann, Paolo Abeni, Simon Horman, Ying Hsu,
linux-kernel, netdev
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>
---
(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);
--
2.48.1.262.g85cc9f2d1e-goog
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH v2 2/3] Bluetooth: Always allow SCO packets for user channel 2025-01-22 5:19 [PATCH v2 1/3] Bluetooth: Fix possible race with userspace of sysfs isoc_alt Hsin-chen Chuang @ 2025-01-22 5:19 ` Hsin-chen Chuang 2025-01-22 5:19 ` [PATCH v2 3/3] Bluetooth: Add ABI doc for sysfs isoc_alt Hsin-chen Chuang 2025-01-22 19:35 ` [PATCH v2 1/3] Bluetooth: Fix possible race with userspace of " Luiz Augusto von Dentz 2 siblings, 0 replies; 10+ messages in thread From: Hsin-chen Chuang @ 2025-01-22 5:19 UTC (permalink / raw) To: linux-bluetooth, luiz.dentz Cc: chromeos-bluetooth-upstreaming, Hsin-chen Chuang, Marcel Holtmann, Ying Hsu, linux-kernel 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> --- 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 bf210275e5b7..acfa83228b25 100644 --- a/drivers/bluetooth/btusb.c +++ b/drivers/bluetooth/btusb.c @@ -2104,7 +2104,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); @@ -2585,7 +2586,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.262.g85cc9f2d1e-goog ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 3/3] Bluetooth: Add ABI doc for sysfs isoc_alt 2025-01-22 5:19 [PATCH v2 1/3] Bluetooth: Fix possible race with userspace of sysfs isoc_alt Hsin-chen Chuang 2025-01-22 5:19 ` [PATCH v2 2/3] Bluetooth: Always allow SCO packets for user channel Hsin-chen Chuang @ 2025-01-22 5:19 ` Hsin-chen Chuang 2025-01-22 5:32 ` Hsin-chen Chuang 2025-01-22 19:35 ` [PATCH v2 1/3] Bluetooth: Fix possible race with userspace of " Luiz Augusto von Dentz 2 siblings, 1 reply; 10+ messages in thread From: Hsin-chen Chuang @ 2025-01-22 5:19 UTC (permalink / raw) To: linux-bluetooth, luiz.dentz Cc: chromeos-bluetooth-upstreaming, Hsin-chen Chuang, Johan Hedberg, Marcel Holtmann, linux-kernel 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 | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/Documentation/ABI/stable/sysfs-class-bluetooth b/Documentation/ABI/stable/sysfs-class-bluetooth index 36be02471174..8cc5f3cfe133 100644 --- a/Documentation/ABI/stable/sysfs-class-bluetooth +++ b/Documentation/ABI/stable/sysfs-class-bluetooth @@ -7,3 +7,17 @@ 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: 22-Jan-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 doesn't support USB Alternate setting + configuration, the read/write fails with -ENODEV. + If the data is not a valid number, the write fails with -EINVAL. + The other failures are vendor specific. -- 2.48.1.262.g85cc9f2d1e-goog ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 3/3] Bluetooth: Add ABI doc for sysfs isoc_alt 2025-01-22 5:19 ` [PATCH v2 3/3] Bluetooth: Add ABI doc for sysfs isoc_alt Hsin-chen Chuang @ 2025-01-22 5:32 ` Hsin-chen Chuang 0 siblings, 0 replies; 10+ messages in thread From: Hsin-chen Chuang @ 2025-01-22 5:32 UTC (permalink / raw) To: linux-bluetooth, luiz.dentz Cc: chromeos-bluetooth-upstreaming, Hsin-chen Chuang, Johan Hedberg, Marcel Holtmann, linux-kernel Hi Luiz, On Wed, Jan 22, 2025 at 1:21 PM 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") 5e5c3898ef49 is the first patch in this series, I assume the hash would change after it is applied? Shall I instead squash this patch to that or shall I wait for 5e5c3898ef49 to be applied first? > > 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 | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/Documentation/ABI/stable/sysfs-class-bluetooth b/Documentation/ABI/stable/sysfs-class-bluetooth > index 36be02471174..8cc5f3cfe133 100644 > --- a/Documentation/ABI/stable/sysfs-class-bluetooth > +++ b/Documentation/ABI/stable/sysfs-class-bluetooth > @@ -7,3 +7,17 @@ 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: 22-Jan-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 doesn't support USB Alternate setting > + configuration, the read/write fails with -ENODEV. > + If the data is not a valid number, the write fails with -EINVAL. > + The other failures are vendor specific. > -- > 2.48.1.262.g85cc9f2d1e-goog > -- Best Regards, Hsin-chen ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/3] Bluetooth: Fix possible race with userspace of sysfs isoc_alt 2025-01-22 5:19 [PATCH v2 1/3] Bluetooth: Fix possible race with userspace of sysfs isoc_alt Hsin-chen Chuang 2025-01-22 5:19 ` [PATCH v2 2/3] Bluetooth: Always allow SCO packets for user channel Hsin-chen Chuang 2025-01-22 5:19 ` [PATCH v2 3/3] Bluetooth: Add ABI doc for sysfs isoc_alt Hsin-chen Chuang @ 2025-01-22 19:35 ` Luiz Augusto von Dentz 2025-01-23 4:57 ` Hsin-chen Chuang 2 siblings, 1 reply; 10+ messages in thread From: Luiz Augusto von Dentz @ 2025-01-22 19:35 UTC (permalink / raw) To: Hsin-chen Chuang Cc: linux-bluetooth, chromeos-bluetooth-upstreaming, Hsin-chen Chuang, David S. Miller, Eric Dumazet, Jakub Kicinski, Johan Hedberg, Marcel Holtmann, Paolo Abeni, Simon Horman, Ying Hsu, linux-kernel, netdev Hi Hsin-chen, On Wed, Jan 22, 2025 at 12:20 AM Hsin-chen Chuang <chharry@google.com> wrote: > > 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> > --- > > (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. > -- > 2.48.1.262.g85cc9f2d1e-goog > -- Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/3] Bluetooth: Fix possible race with userspace of sysfs isoc_alt 2025-01-22 19:35 ` [PATCH v2 1/3] Bluetooth: Fix possible race with userspace of " Luiz Augusto von Dentz @ 2025-01-23 4:57 ` Hsin-chen Chuang 2025-01-24 15:54 ` Luiz Augusto von Dentz 0 siblings, 1 reply; 10+ messages in thread From: Hsin-chen Chuang @ 2025-01-23 4:57 UTC (permalink / raw) To: Luiz Augusto von Dentz Cc: linux-bluetooth, chromeos-bluetooth-upstreaming, Hsin-chen Chuang, David S. Miller, Eric Dumazet, Jakub Kicinski, Johan Hedberg, Marcel Holtmann, Paolo Abeni, Simon Horman, Ying Hsu, linux-kernel, netdev Hi Luiz, On Thu, Jan 23, 2025 at 3:35 AM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > > Hi Hsin-chen, > > On Wed, Jan 22, 2025 at 12:20 AM Hsin-chen Chuang <chharry@google.com> wrote: > > > > 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> > > --- > > > > (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. > > > -- > > 2.48.1.262.g85cc9f2d1e-goog > > > > > -- > Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/3] Bluetooth: Fix possible race with userspace of sysfs isoc_alt 2025-01-23 4:57 ` Hsin-chen Chuang @ 2025-01-24 15:54 ` Luiz Augusto von Dentz 2025-01-24 16:06 ` Luiz Augusto von Dentz 0 siblings, 1 reply; 10+ messages in thread From: Luiz Augusto von Dentz @ 2025-01-24 15:54 UTC (permalink / raw) To: Hsin-chen Chuang Cc: linux-bluetooth, chromeos-bluetooth-upstreaming, Hsin-chen Chuang, David S. Miller, Eric Dumazet, Jakub Kicinski, Johan Hedberg, Marcel Holtmann, Paolo Abeni, Simon Horman, Ying Hsu, linux-kernel, netdev Hi Hsin-chen, On Wed, Jan 22, 2025 at 11:57 PM Hsin-chen Chuang <chharry@google.com> wrote: > > Hi Luiz, > > On Thu, Jan 23, 2025 at 3:35 AM Luiz Augusto von Dentz > <luiz.dentz@gmail.com> wrote: > > > > Hi Hsin-chen, > > > > On Wed, Jan 22, 2025 at 12:20 AM Hsin-chen Chuang <chharry@google.com> wrote: > > > > > > 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> > > > --- > > > > > > (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. We might have to do something like the following within hci_init_sysfs: if (hdev->isoc_alt) dev->type = bt_host_isoc_alt else dev->type = bt_host Now perhaps instead of adding the callbacks to hdev we add the attribute itself, btw did you check if there isn't a sysfs entry to interact with USB alternate settings? Because contrary to reset this actually operates directly on the driver bus so it sort of made more sense to me that this would be handled by USB rather than Bluetooth. > > > > > -- > > > 2.48.1.262.g85cc9f2d1e-goog > > > > > > > > > -- > > Luiz Augusto von Dentz -- Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/3] Bluetooth: Fix possible race with userspace of sysfs isoc_alt 2025-01-24 15:54 ` Luiz Augusto von Dentz @ 2025-01-24 16:06 ` Luiz Augusto von Dentz 2025-01-31 10:23 ` Hsin-chen Chuang 0 siblings, 1 reply; 10+ messages in thread From: Luiz Augusto von Dentz @ 2025-01-24 16:06 UTC (permalink / raw) To: Hsin-chen Chuang Cc: linux-bluetooth, chromeos-bluetooth-upstreaming, Hsin-chen Chuang, David S. Miller, Eric Dumazet, Jakub Kicinski, Johan Hedberg, Marcel Holtmann, Paolo Abeni, Simon Horman, Ying Hsu, linux-kernel, netdev Hi Hsin-chen, On Fri, Jan 24, 2025 at 10:54 AM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > > Hi Hsin-chen, > > On Wed, Jan 22, 2025 at 11:57 PM Hsin-chen Chuang <chharry@google.com> wrote: > > > > Hi Luiz, > > > > On Thu, Jan 23, 2025 at 3:35 AM Luiz Augusto von Dentz > > <luiz.dentz@gmail.com> wrote: > > > > > > Hi Hsin-chen, > > > > > > On Wed, Jan 22, 2025 at 12:20 AM Hsin-chen Chuang <chharry@google.com> wrote: > > > > > > > > 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> > > > > --- > > > > > > > > (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. > > We might have to do something like the following within hci_init_sysfs: > > if (hdev->isoc_alt) > dev->type = bt_host_isoc_alt > else > dev->type = bt_host > > Now perhaps instead of adding the callbacks to hdev we add the > attribute itself, btw did you check if there isn't a sysfs entry to > interact with USB alternate settings? Because contrary to reset this > actually operates directly on the driver bus so it sort of made more > sense to me that this would be handled by USB rather than Bluetooth. A quick git grep shows that this exists: Documentation/ABI/testing/sysfs-bus-usb:What: /sys/bus/usb/devices/usbX/bAlternateSetting > > > > > > > -- > > > > 2.48.1.262.g85cc9f2d1e-goog > > > > > > > > > > > > > -- > > > Luiz Augusto von Dentz > > > > -- > Luiz Augusto von Dentz -- Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/3] Bluetooth: Fix possible race with userspace of sysfs isoc_alt 2025-01-24 16:06 ` Luiz Augusto von Dentz @ 2025-01-31 10:23 ` Hsin-chen Chuang 2025-02-08 12:07 ` Hsin-chen Chuang 0 siblings, 1 reply; 10+ messages in thread From: Hsin-chen Chuang @ 2025-01-31 10:23 UTC (permalink / raw) To: Luiz Augusto von Dentz Cc: linux-bluetooth, chromeos-bluetooth-upstreaming, Hsin-chen Chuang, David S. Miller, Eric Dumazet, Jakub Kicinski, Johan Hedberg, Marcel Holtmann, Paolo Abeni, Simon Horman, Ying Hsu, linux-kernel, netdev Hi Luiz, Good point. Although the sysfs-bus-usb API only supports reading rather than writing the alt setting, I'll look for the opportunity to configure it through libusb first. Thanks On Sat, Jan 25, 2025 at 12:06 AM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > > Hi Hsin-chen, > > On Fri, Jan 24, 2025 at 10:54 AM Luiz Augusto von Dentz > <luiz.dentz@gmail.com> wrote: > > > > Hi Hsin-chen, > > > > On Wed, Jan 22, 2025 at 11:57 PM Hsin-chen Chuang <chharry@google.com> wrote: > > > > > > Hi Luiz, > > > > > > On Thu, Jan 23, 2025 at 3:35 AM Luiz Augusto von Dentz > > > <luiz.dentz@gmail.com> wrote: > > > > > > > > Hi Hsin-chen, > > > > > > > > On Wed, Jan 22, 2025 at 12:20 AM Hsin-chen Chuang <chharry@google.com> wrote: > > > > > > > > > > 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> > > > > > --- > > > > > > > > > > (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. > > > > We might have to do something like the following within hci_init_sysfs: > > > > if (hdev->isoc_alt) > > dev->type = bt_host_isoc_alt > > else > > dev->type = bt_host > > > > Now perhaps instead of adding the callbacks to hdev we add the > > attribute itself, btw did you check if there isn't a sysfs entry to > > interact with USB alternate settings? Because contrary to reset this > > actually operates directly on the driver bus so it sort of made more > > sense to me that this would be handled by USB rather than Bluetooth. > > A quick git grep shows that this exists: > > Documentation/ABI/testing/sysfs-bus-usb:What: > /sys/bus/usb/devices/usbX/bAlternateSetting > > > > > > > > > > -- > > > > > 2.48.1.262.g85cc9f2d1e-goog > > > > > > > > > > > > > > > > > -- > > > > Luiz Augusto von Dentz > > > > > > > > -- > > Luiz Augusto von Dentz > > > > -- > Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/3] Bluetooth: Fix possible race with userspace of sysfs isoc_alt 2025-01-31 10:23 ` Hsin-chen Chuang @ 2025-02-08 12:07 ` Hsin-chen Chuang 0 siblings, 0 replies; 10+ messages in thread From: Hsin-chen Chuang @ 2025-02-08 12:07 UTC (permalink / raw) To: Luiz Augusto von Dentz Cc: linux-bluetooth, chromeos-bluetooth-upstreaming, Hsin-chen Chuang, David S. Miller, Eric Dumazet, Jakub Kicinski, Johan Hedberg, Marcel Holtmann, Paolo Abeni, Simon Horman, Ying Hsu, linux-kernel, netdev Hi Luiz, On Fri, Jan 31, 2025 at 6:23 PM Hsin-chen Chuang <chharry@google.com> wrote: > > Hi Luiz, > > Good point. Although the sysfs-bus-usb API only supports reading > rather than writing the alt setting, I'll look for the opportunity to > configure it through libusb first. > > Thanks > > On Sat, Jan 25, 2025 at 12:06 AM Luiz Augusto von Dentz > <luiz.dentz@gmail.com> wrote: > > > > Hi Hsin-chen, > > > > On Fri, Jan 24, 2025 at 10:54 AM Luiz Augusto von Dentz > > <luiz.dentz@gmail.com> wrote: > > > > > > Hi Hsin-chen, > > > > > > On Wed, Jan 22, 2025 at 11:57 PM Hsin-chen Chuang <chharry@google.com> wrote: > > > > > > > > Hi Luiz, > > > > > > > > On Thu, Jan 23, 2025 at 3:35 AM Luiz Augusto von Dentz > > > > <luiz.dentz@gmail.com> wrote: > > > > > > > > > > Hi Hsin-chen, > > > > > > > > > > On Wed, Jan 22, 2025 at 12:20 AM Hsin-chen Chuang <chharry@google.com> wrote: > > > > > > > > > > > > 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> > > > > > > --- > > > > > > > > > > > > (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. > > > > > > We might have to do something like the following within hci_init_sysfs: > > > > > > if (hdev->isoc_alt) > > > dev->type = bt_host_isoc_alt > > > else > > > dev->type = bt_host > > > > > > Now perhaps instead of adding the callbacks to hdev we add the > > > attribute itself, btw did you check if there isn't a sysfs entry to > > > interact with USB alternate settings? Because contrary to reset this > > > actually operates directly on the driver bus so it sort of made more > > > sense to me that this would be handled by USB rather than Bluetooth. Unfortunately I tried the libusb API and it requires detaching the kernel driver, which means the user channel would be broken. I'll send out a new version which addresses the above comments, thanks. > > > > A quick git grep shows that this exists: > > > > Documentation/ABI/testing/sysfs-bus-usb:What: > > /sys/bus/usb/devices/usbX/bAlternateSetting > > > > > > > > > > > > > -- > > > > > > 2.48.1.262.g85cc9f2d1e-goog > > > > > > > > > > > > > > > > > > > > > -- > > > > > Luiz Augusto von Dentz > > > > > > > > > > > > -- > > > Luiz Augusto von Dentz > > > > > > > > -- > > Luiz Augusto von Dentz -- Best Regards, Hsin-chen ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2025-02-08 12:08 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2025-01-22 5:19 [PATCH v2 1/3] Bluetooth: Fix possible race with userspace of sysfs isoc_alt Hsin-chen Chuang 2025-01-22 5:19 ` [PATCH v2 2/3] Bluetooth: Always allow SCO packets for user channel Hsin-chen Chuang 2025-01-22 5:19 ` [PATCH v2 3/3] Bluetooth: Add ABI doc for sysfs isoc_alt Hsin-chen Chuang 2025-01-22 5:32 ` Hsin-chen Chuang 2025-01-22 19:35 ` [PATCH v2 1/3] Bluetooth: Fix possible race with userspace of " Luiz Augusto von Dentz 2025-01-23 4:57 ` Hsin-chen Chuang 2025-01-24 15:54 ` Luiz Augusto von Dentz 2025-01-24 16:06 ` Luiz Augusto von Dentz 2025-01-31 10:23 ` Hsin-chen Chuang 2025-02-08 12:07 ` Hsin-chen Chuang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®