| Message ID | 20250122131925.v2.1.If6f14aa2512336173a53fc3552756cd8a332b0a3@changeid (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2,1/3] Bluetooth: Fix possible race with userspace of sysfs isoc_alt | expand |
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 >
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
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);