Hi Hsin-chen,
On Wed, Jan 22, 2025 at 12:20 AM Hsin-chen Chuang <chharry@xxxxxxxxxx> wrote:
>
> From: Hsin-chen Chuang <chharry@xxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxx>
> ---
>
> (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
