| Message ID | 20250122131925.v2.1.If6f14aa2512336173a53fc3552756cd8a332b0a3@changeid (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/3] Bluetooth: Fix possible race with userspace of sysfs isoc_alt | expand |
| Context | Check | Description |
|---|---|---|
| tedd_an/pre-ci_am | success | Success |
| tedd_an/SubjectPrefix | success | Gitlint PASS |
| tedd_an/BuildKernel | success | BuildKernel PASS |
| tedd_an/CheckAllWarning | success | CheckAllWarning PASS |
| tedd_an/CheckSparse | success | CheckSparse PASS |
| tedd_an/BuildKernel32 | success | BuildKernel32 PASS |
| tedd_an/TestRunnerSetup | success | TestRunnerSetup PASS |
| tedd_an/TestRunner_l2cap-tester | success | TestRunner PASS |
| tedd_an/TestRunner_iso-tester | success | TestRunner PASS |
| tedd_an/TestRunner_bnep-tester | success | TestRunner PASS |
| tedd_an/TestRunner_mgmt-tester | fail | TestRunner_mgmt-tester: Total: 490, Passed: 485 (99.0%), Failed: 1, Not Run: 4 |
| tedd_an/TestRunner_rfcomm-tester | success | TestRunner PASS |
| tedd_an/TestRunner_sco-tester | success | TestRunner PASS |
| tedd_an/TestRunner_ioctl-tester | success | TestRunner PASS |
| tedd_an/TestRunner_mesh-tester | success | TestRunner PASS |
| tedd_an/TestRunner_smp-tester | success | TestRunner PASS |
| tedd_an/TestRunner_userchan-tester | success | TestRunner PASS |
This is automated email and please do not reply to this email! Dear submitter, Thank you for submitting the patches to the linux bluetooth mailing list. This is a CI test results with your patch series: PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=927394 ---Test result--- Test Summary: CheckPatch PENDING 0.48 seconds GitLint PENDING 0.29 seconds SubjectPrefix PASS 0.19 seconds BuildKernel PASS 24.01 seconds CheckAllWarning PASS 26.31 seconds CheckSparse PASS 29.89 seconds BuildKernel32 PASS 23.61 seconds TestRunnerSetup PASS 424.15 seconds TestRunner_l2cap-tester PASS 20.28 seconds TestRunner_iso-tester PASS 29.46 seconds TestRunner_bnep-tester PASS 4.76 seconds TestRunner_mgmt-tester FAIL 122.54 seconds TestRunner_rfcomm-tester PASS 7.51 seconds TestRunner_sco-tester PASS 9.28 seconds TestRunner_ioctl-tester PASS 8.02 seconds TestRunner_mesh-tester PASS 5.87 seconds TestRunner_smp-tester PASS 6.86 seconds TestRunner_userchan-tester PASS 7.03 seconds IncrementalBuild PENDING 0.46 seconds Details ############################## Test: CheckPatch - PENDING Desc: Run checkpatch.pl script Output: ############################## Test: GitLint - PENDING Desc: Run gitlint Output: ############################## Test: TestRunner_mgmt-tester - FAIL Desc: Run mgmt-tester with test-runner Output: Total: 490, Passed: 485 (99.0%), Failed: 1, Not Run: 4 Failed Test Cases LL Privacy - Set Flags 2 (Enable RL) Failed 0.138 seconds ############################## Test: IncrementalBuild - PENDING Desc: Incremental build with the patches in the series Output: --- Regards, Linux Bluetooth
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
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
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
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);